Fix to #28816 - Json: add support for Sqlite provider #30302

maumar · 2023-02-17T00:09:18Z

Adding support for Sqlite.

Limitation:
When accessing element of a JSON array we can only use constant values. Unlike Sql Server which supports parameters, columns or even arbitrary expressions.

Also since JSON aggregates are mapped to own types, in case of collections users need to ignore SqliteEventId.CompositeKeyWithValueGeneration

Fixes #28816

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

maumar · 2023-02-23T11:02:34Z

updated and ready for review @AndriySvyryd @roji @bricelam

maumar · 2023-02-23T11:03:30Z

src/EFCore.Sqlite.Core/Update/Internal/SqliteModificationCommand.cs

+                // Sqlite converts true/false into native 0/1 when using json_extract
+                // so we convert those values to strings so that they stay as true/false
+                // which is what we want to store in json object in the end
+                var modifiedPropertyValue = boolPropertyValue


is there more "elegant" way to do this? i.e. without hard-coding the "true"/"false" values? @bricelam

src/EFCore.Sqlite.Core/Update/Internal/SqliteModificationCommand.cs

roji

Really nice that this required so little changes etc. - but see comments; I think we may need some more generalized infra, looking forward.

One additional note... SQLite supports two methods for extracting values out of JSON documents: json_extract and the ->/->> operators (which PG and MySQL support too). At least from a clean SQL perspective, these may be a better fit: instead of piecing together a JSONPATH string (and concatenating where non-constant data is provided), we can just use these operators. Compare the following two:

SELECT json_extract(json, '$.c[' || ind || ']') FROM data;
SELECT json->'$.c'->ind FROM data;

There's even a (theoretical) chance the -> would be faster, since we're expressing things more directly/simply, rather than concatenating arbitrary things to form the JSONPATH string.

Note that these operators were introduced in SQLite 3.38.0 (2022-02-22), though I don't think that should be an issue.

src/EFCore.Relational/Update/ModificationCommand.cs

src/EFCore.Sqlite.Core/Update/Internal/SqliteModificationCommand.cs

src/EFCore.Sqlite.Core/Metadata/Internal/SqliteAnnotationProvider.cs

roji · 2023-02-23T12:09:19Z

src/EFCore.Sqlite.Core/Metadata/Internal/SqliteAnnotationProvider.cs

-            && property.ClrType.UnwrapNullableType().IsInteger()
-            && !HasConverter(property))
+        // JSON columns have no property mappings so all annotations that rely on property mappings should be skipped for them
+        if (column is not JsonColumn)


Over in SqlServerAnnotationProvider, most of the code isn't under a check against JsonColumn; for example, I can see the identity code doing it's thinking without checking for JSON.

Should we change things over in SQL Server too?

code in SqlServer now has null ref protection, so it should be fine. We used to access property like this:

var property = column.PropertyMappings.First().Property

OK, I see.

It would probably be a bit clearer if the entire SQL Server code block were wrapped in if (column is not JsonColumn) like this PR does for Sqlite, just to make it explicit exactly when we don't expect the code to be relevant.

src/EFCore.Sqlite.Core/Metadata/Internal/SqliteAnnotationProvider.cs

roji · 2023-02-23T12:45:42Z

src/EFCore.Sqlite.Core/Storage/Internal/SqliteJsonTypeMapping.cs

+///     any release. You should only use it directly in your code with extreme caution and knowing that
+///     doing so can result in application failures when updating to a new Entity Framework Core release.
+/// </summary>
+public class SqliteJsonTypeMapping : JsonTypeMapping


This looks like a copy-paste of SqlServerJsonTypeMapping; should it be in relational?

I'm ok to put the common logic in relational, but won't you need some sort of customization in npg? (e.g. two json type mappings one for json and one for jsonb).

Right, Npgsql would probably have two instances of this (it already accepts the store type in the constructor).

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

roji · 2023-02-23T12:50:09Z

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

@@ -143,6 +145,64 @@ protected override void AppendRowsAffectedWhereCondition(StringBuilder commandSt
    public override string GenerateNextSequenceValueOperation(string name, string? schema)
        => throw new NotSupportedException(SqliteStrings.SequencesNotSupported);

+
+    /// <inheritdoc />
+    protected override void AppendUpdateColumnValue(


Should we extract out an overridable AppendJsonModification method, called when columnModification.JsonPath is non-null? This would more cleanly separate JSON updates from non-JSON updates, and be the extension point for implementing partial updates etc.

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

roji · 2023-02-23T12:57:04Z

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

+                // json_extract converts true/false into native 0/1 values,
+                // but we want to store the values as true/false in JSON
+                // in order to do that we modify the parameter value to "true"/"false"
+                // and wrap json() function around it to avoid conversion to 0/1


Isn't it enough that we're transforming the value into true/false strings in SqliteModificationCommand.GenerateJsonForSinglePropertyUpdate? The json() function is documented as "verifies that its argument X is a valid JSON string and returns a minified version of that JSON string"; is it somehow preventing a conversion here or something?

yes, without json around the call the value after update is stored as:

"TestBoolean":"true"

whereas before (and what we ultimately want) is:

"TestBoolean":true

Funny enough, select json_extract('["true"]', '$[0]'); and select json(json_extract('["true"]', '$[0]')); seem to return the same result: true

¯\_(ツ)_/¯

OK...

I think this also related to #30302 (comment); if we had a more general thing that says how to create a literal JSON from the value, we'd just be able to directly get true out, rather than having to jump through hoops and use server-side logic to convert stuff. Unless I'm mistaken, this would allow us to remove the CAST in SQL Server and the json_extract/json here in SQLite.

Adding support for Sqlite. Also adding some more query and update tests for properties with value converters. Fixes #28816

bricelam

LGTM

maumar requested review from a team, bricelam and roji February 17, 2023 00:09

maumar force-pushed the fix28816 branch 2 times, most recently from 7dd826e to 2c6e447 Compare February 17, 2023 00:16

maumar commented Feb 20, 2023

View reviewed changes

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs Outdated Show resolved Hide resolved

maumar force-pushed the fix28816 branch 2 times, most recently from e5adba7 to 4e32563 Compare February 23, 2023 11:00

maumar commented Feb 23, 2023

View reviewed changes

src/EFCore.Sqlite.Core/Update/Internal/SqliteModificationCommand.cs Outdated Show resolved Hide resolved

roji requested changes Feb 23, 2023

View reviewed changes

maumar force-pushed the fix28816 branch from 4e32563 to 0db38cf Compare February 24, 2023 11:47

roji approved these changes Feb 24, 2023

View reviewed changes

Fix to #28816 - Json: add support for Sqlite provider

201fe76

Adding support for Sqlite. Also adding some more query and update tests for properties with value converters. Fixes #28816

maumar force-pushed the fix28816 branch from 0db38cf to 201fe76 Compare February 24, 2023 18:55

maumar merged commit 358b9ca into main Feb 24, 2023

maumar deleted the fix28816 branch February 24, 2023 21:01

bricelam reviewed Feb 28, 2023

View reviewed changes

ajcvickers mentioned this pull request Mar 3, 2023

Latest news and progress on .NET 8 and EF8 #29989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to #28816 - Json: add support for Sqlite provider #30302

Fix to #28816 - Json: add support for Sqlite provider #30302

maumar commented Feb 17, 2023

maumar commented Feb 23, 2023

maumar Feb 23, 2023

roji left a comment

roji Feb 23, 2023

maumar Feb 24, 2023

roji Feb 24, 2023

roji Feb 23, 2023

maumar Feb 24, 2023

roji Feb 24, 2023

roji Feb 23, 2023

roji Feb 23, 2023

maumar Feb 24, 2023 •

edited

Loading

roji Feb 24, 2023

bricelam left a comment

Fix to #28816 - Json: add support for Sqlite provider #30302

Fix to #28816 - Json: add support for Sqlite provider #30302

Conversation

maumar commented Feb 17, 2023

maumar commented Feb 23, 2023

Choose a reason for hiding this comment

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam left a comment

Choose a reason for hiding this comment

maumar Feb 24, 2023 •

edited

Loading