Complex json change tracking #36446
Merged
Complex json change tracking #36446
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roji
commented
Jul 27, 2025
| Visit(relationalGroupByResultExpression.KeyIdentifier), | ||
| QueryCompilationContext.QueryContextParameter, | ||
| _dataReaderParameter); | ||
| try |
There was a problem hiding this comment.
Just indented the code to add the management of _parentVisitor._currentShaperProcessor.
|
|
||
| relationalCommandResolver = _parentVisitor.CreateRelationalCommandResolverExpression(_selectExpression); | ||
| readerColumns = _readerColumns; | ||
| relatedDataLoaders = null; |
There was a problem hiding this comment.
As above, only indentation change.
test/EFCore.SqlServer.FunctionalTests/Update/ComplexCollectionJsonUpdateSqlServerTest.cs
Show resolved
Hide resolved
roji
force-pushed
the
ComplexJsonChangeTracking
branch
from
73d2e74 to
4b9bc54
Compare
roji
commented
Jul 27, 2025
| Name: 'Test Company' | ||
| Contacts (Complex: List<Contact>) | ||
| Department (Complex: Department) | ||
| Budget: 10000.00 |
There was a problem hiding this comment.
This failed on SQLite because we get Budget: 10000.0.
There was a problem hiding this comment.
Add something like Assert.DoesNotContain("Exception");
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
AndriySvyryd
approved these changes
Jul 28, 2025
roji
force-pushed
the
ComplexJsonChangeTracking
branch
from
4b9bc54 to
6cf70ef
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With JSON owned entities, the root (non-JSON) entity is materialized and tracked, and then JSON shaper code materializes the JSON owned entity and tracks that separately; the change tracker does the fix-up etc. When originally implementing the shaper support for complex JSON, I also added JSON shaper code after materialization - similar to the owned flow - but that's too late, since StartTracking already gets called as part of materiailzation.
I considered separating StartTracking from materialization and pulling the former up, so that I can inject JSON shaper code in between - but that's quite a significant change would probably break external (non-relational) providers. Instead I opted for adding an extension hook during materialization, that gets called after the structural type is instantiated and its regular properties are populated, but before it's handed off to StartTracking().
Adding such a hook proved difficult - our shaper generation really isn't designed with extensibility in mind. The most problematic is that in relational, ShapedQueryCompilingExpressionVisitor immediately calls a separate private visitor - ShaperProcessingExpressionVisitor - which does most of the work, and occasionally calls into the "parent" ShaperProcessingExpressionVisitor. One of these calls is for structural type materialization, making it difficult to have the extension hook call back into ShaperProcessingExpressionVisitor, which is where the JSON shaper logic lives (and it relies on various global state inside that visitor, and so can't easily be moved out). So I ended up tracking the current ShaperProcessingExpressionVisitor on RelationalShapedQueryCompilingExpressionVisitor, so that the hook can call into it; and since ShaperProcessingExpressionVisitor instantiates and calls itself recursively, that also needs to be accounted for.
There's a lot of potential for simplifying and cleaning everything up here, I hope we get to it at some point.
/cc @artl93