Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SQLite native representations in JSON #31490

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

roji
Copy link
Member

@roji roji commented Aug 17, 2023

Closes #30727

/cc @danmoseley

@roji roji requested a review from ajcvickers August 17, 2023 11:05
WHERE (
SELECT COUNT(*)
FROM json_each("t"."SomeArray") AS "s"
WHERE unhex("s"."value") = X'0102') = 2
Copy link
Member Author

@roji roji Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we the JSON representation for binary data is now a hexadecimal string (same as the output of the SQLite function hex; we do a server-side conversion back to blob via unhex. Note that unhex was only introduced relatively recently (version 3.41.0, released 2023-02-08).

This is now the only case where we need a server-side conversion, because the other types (e.g. date/time) really are strings in SQLite, whereas BLOB is its own separate type.

@roji roji force-pushed the JsonRepresentations branch from 2d499a4 to d8164f9 Compare August 17, 2023 16:19
Copy link
Contributor

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!

@roji roji force-pushed the JsonRepresentations branch 2 times, most recently from 233e015 to 2c85727 Compare August 17, 2023 18:33
namespace Microsoft.EntityFrameworkCore;

[SpatialiteRequired]
public class JsonTypesSqliteTest : JsonTypesRelationalTestBase
{
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> base.OnConfiguring(optionsBuilder.UseSqlite(b => b.UseNetTopologySuite()));

public override void Can_read_write_binary_JSON_values(string value, string json)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajcvickers note this annoying thing... when you override a virtual test method, apparently the [InlineData] attributes on the base are still inherited; this makes it impossible to override methods when what you want is to replace the [InlineData] (which is the case here).

I hacked around it for now, we can think about it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing you can do is add a placeholder and then mutate the value before passing it down to the base method. See public override void Can_read_write_collection_of_ASCII_string_JSON_values(object? storeType) in JsonTypesSqlServerTest. But maybe this is just as much as a hack, and is only useful in limited cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... Unfortunately [MemberData] can only point to static properties/methods, otherwise that could have been virtual and overridden (xunit is really limited here). Let's revisit this later.

@roji roji force-pushed the JsonRepresentations branch from 2c85727 to e294428 Compare August 17, 2023 19:06
@roji roji merged commit 011f0ec into dotnet:release/8.0-rc1 Aug 17, 2023
@roji roji deleted the JsonRepresentations branch August 17, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants