-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Fixes and cleanup around JSON collection SQL value conversions #31261
Conversation
src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
@@ -54,6 +72,10 @@ SELECT COUNT(*) | |||
"""); | |||
} | |||
|
|||
[ConditionalFact] | |||
public override Task Array_of_byte() | |||
=> base.Array_of_byte(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we need list of test names to exempt from Check_all_tests_overridden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, though I think it's not too bad like this... It even makes me explicitly see that we're not really overriding, i.e. that for byte arrays we assert failure in the base etc.
Pay special attention to SQLite Array_of_TimeOnly after #31272 is merged (https://github.com/dotnet/efcore/pull/31272/files#r1264439804 after that's merged) |
d0a8f59
to
63f2828
Compare
This does work around SQL logic for converting values out of JSON so they can be further queried in SQL. Originally I thought we may want to add a general API on the type mappings, but I don't think that makes sense at this point. The conversions needed for SQLite are concentrated in a single simple method, and there's no real value in refactoring that out to the type mappings (SQL Server doesn't need any). We can always change this in the future, but for now I think we should leave this as an internal implementation detail of each provider.
The update pipeline doesn't need to perform server-side conversions to JSON; we simply generate the JSON representation we want client-side, and send the appropriate parameter for that (e.g. a JSON string representation of a timestamp). An alternative approach would be to e.g. send a regular DateTime parameter, and then use SQL to convert that to the JSON string representation. But there's no real advantage in that, and it's complex/not always possible. I've done some cleanup/improvement to partial updating in #31232, but I don't think anything else is needed at this point.
(Note that if we ever want to allow a JSON property to be a key/concurrency token, the update pipeline would need to apply the same conversions as the query pipeline (because it would need to appear in the UPDATE WHERE clause)
Specific notes on what's converted and what's not supported:
datetime()
to remove the T) and Guid (uppercase)Related to #30727