-
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
Fix to #33073 - JSON columns throws Invalid token type: 'StartObject' exception with AsNoTrackingWithIdentityResolution() #33101
Conversation
there is a problem with this fix, some tests regress into data corruption (from invalid token now), need to investigate in-depth. failing tests for NTWIR Json_branch_collection_distinct_and_other_collection - NRE |
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
a856e2c
to
1dea399
Compare
@roji @ajcvickers new version up |
Semi-related: the same error gets thrown when it tries to deserialize a json object as a list of objects.
-> Database is expected to return something like the following for the Files property: Meanwhile, if you would try to deserialize that incorrect object as a List via Newtonsoft (haven't tried with System.Text.Json), you'd get a much clearer Exception message. |
@maumar Remind me again why this ordering is not needed for tracking queries? |
@ajcvickers you can't project a "naked" owned type in a tracking query scenario. #31047, so we always have the parent entities and they include everything in the correct order. |
… exception with AsNoTrackingWithIdentityResolution() Problem was that we were treating NoTrackingWithIdentityResolution queries as regular NoTracking in the context of JSON queries. However due to how JSON is materialized (streaming + nested includes are part of the parent materialization, rather than each entity materialized separately) we have special provisions when ChangeTracker is being used. In NoTrackingWithIdentityResolution, ChangeTracker is being used but the materializer didn't adjust to that, which lead to errors. Fix is to generate JSON materializer code based on whether query uses Change Tracker rather than if it's a Tracking/NoTracking query. However, this uncovered some issue with NoTrackingWithIdentityResolution - depending on the order in which entities are processed during materialization, they could cause data corruption (wrong order) when materializing JSON collections. Also, using queryable operators on JSON collections may cause errors or data corruption - we don't propagate key values of those queries to the materializer, so those entities end up with null keys. Adding a validator that makes sure that entities are visited in the correct order and issues exception instructing what to do, if the order is wrong. Also we disable usage of queryable operators, due to the issue mentioned above, and cases where parameters are being used to access collection element in the navigatio chain. For cases with parameters, we can't tell if the value is the same or different so can't properly validate those cases. Two different parameters can have the same value, leading to the same entity being materialized, but when we analyze their JSON path, those paths look different. Fixes #33073
Problem was that we were treating NoTrackingWithIdentityResolution queries as regular NoTracking in the context of JSON queries. However due to how JSON is materialized (streaming + nested includes are part of the parent materialization, rather than each entity materialized separately) we have special provisions when ChangeTracker is being used. In NoTrackingWithIdentityResolution, ChangeTracker is being used but the materializer didn't adjust to that, which lead to errors.
Fix is to generate JSON materializer code based on whether query uses Change Tracker rather than if it's a Tracking/NoTracking query.
Fixes #33073