Structural equality support for complex JSON #36404
Conversation
Now that the update pipeline works, dotnet#36379
No actual changes
| @@ -7,99 +7,4 @@ public class ComplexJsonSqlServerFixture : ComplexJsonRelationalFixtureBase | |||
| { | |||
| protected override ITestStoreFactory TestStoreFactory | |||
| => SqlServerTestStoreFactory.Instance; | |||
|
|
|||
| protected override async Task SeedAsync(PoolableDbContext context) | |||
There was a problem hiding this comment.
@AndriySvyryd update pipeline is working perfectly - thanks!
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| [EntityFrameworkInternal] | ||
| public static string? SerializeComplexTypeToJson(IComplexType complexType, object? value, bool isCollection) |
There was a problem hiding this comment.
@AndriySvyryd note this code, which serializes a complex town to JSON, similar to what we already have in ModificationCommand for the update pipeline. Here we get the object itself (as specified inline in the query, or as a parameter), whereas in the update pipeline we of course deal with entries - so we probably can't deduplicate.
| @@ -105,7 +105,7 @@ protected virtual string EscapeSqlLiteral(string literal) | |||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | |||
| /// </summary> | |||
| protected override string GenerateNonNullSqlLiteral(object value) | |||
| => $"'{EscapeSqlLiteral(JsonSerializer.Serialize(value))}'"; | |||
There was a problem hiding this comment.
Note this pretty problematic code we had here - we can't use the high-level JsonSerializer ever since we have our own model of the type (shadow properties, ignored CLR properties...). I don't think this was actually getting used.
As usual, there's quite a lot of confusion in the query pipeline around scalars and structural JSON values... The type mapping should be a pure representation of the database type here (so nvarchar(max), json) and not of the CLR/structural type, so we should never be receiving an arbitrary .NET object here, only a string.
Going even further, I'm still not sure why we need a separate type mapping for owned/structural JSON as opposed to just JSON - I think it's because we use the type mapping to know whether we're dealing with a structural type or not. That's again conflating client-side and server-side (what's being read from the column is the domain of the shaper, and should not be in the type mapping).
|
Thanks for the quick review @cincuranet. I'll go ahead and merge once it's green, but @AndriySvyryd would also appreciate a review from you (product changes aren't huge here actually, and very localized) - can always integrate comments into a later PR. |
roji
force-pushed
the
ComplexJsonEquality
branch
from
6aa9f27 to
59f98b1
Compare
Review last commit only (the first two are boring cleanup/reorganization).
Part of #36296