-
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
JSON columns throws Invalid token type: 'StartObject' exception with AsNoTrackingWithIdentityResolution() #33073
Comments
Note for triage: still repros on latest daily; regression from EF7. |
for queries with JSON (when we are streaming), we still need to go through the entire stream even if we have an entry in the change tracker already. In case of TrackAll we have special logic that stores entity from the change tracker into a variable and we store bool value indicating that we already have entity instance. We loop through entire stream and and the end we discard the object that we constructed from the stream and instead use the one from change tracker. For NoTrackingWithIdentityResolution we don't do that. Instead, when we find that the object is in the change tracker already we short circuit. This leaves JSON stream in unexpected state, since the data (that is there and waiting to be read) is pending but we already move to processing next entity/whatnot. |
… 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. Fixes #33073
… 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. Fixes #33073
There is a fundamental problem with using NTWIR and not projecting root entity. For Tracking queries we don't allow that (i.e. we throw), but currently for NTWIR we do. When using change tracker, the problem we can get ourselves into is that collection navigation results are being populated in the order they are being materialized in the query. When parent entity is present we process it in a separate code path (Include), and results of that processing always come first in the materializer code. So, "full" JSON collection projections always are discovered and materialized first and therefore all subsequent instances of those collections are populated in the correct ("original") order. For no tracking we always materialize everything separately so the problem doesn't exist. For NTWIR, we can have a situation right now where a collection element is projected first, and only later the full collection is projected. In that scenario, in the "full" collection, that entity discovered earlier would come first in the list. This is an issue for JSON, because collections are supposed to be ordered. Example problematic query:
in the Original, OwnedCollectionBranch will contain OwnedCollectionBranch[1], before the rest of the elements. |
We can either do sophisticated ordering of which elements get materialized before which (i.e. full collections before individual element accesses), or block the scenario where you don't project root in NTWIR, just like we do for tracking query. |
OTOH blocking this scenario (for now) doesn't seem too bad to me. But given that this does work correctly for tracked queries, is it simply an implementation difficulty for the NTWIR code path or something? I mean, if tracking queries can do this correctly without ordered collections, it seems odd that we wouldn't be able to do the same for NTWIT no? In any case, we may really want to have a design discussion on whether we want to continue investing in owned entity JSON mapping - as opposed to complex types. Unless I'm mistaken, this wouldn't be a problem with complex types, since we don't do identity resolution on them anyway (i.e. we'd project different instances, just like in the no-tracking case?). |
NTWIT works fine for the case where you also project parent entity (just like regular Tracking), after fix from #33101 is applied. Cases where we don't project the parent are blocked for Tracking queries, so there is no issue. If you removed the block, we would be hitting the same problem. NTWIT doesn't have the same block so the problem gets exposed. |
… 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
… 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
… 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
… 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
… 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
… 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
… 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
… exception with AsNoTrackingWithIdentityResolution() (#33101) 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
Thanks for fixing this! Looks like its not going to roll out until EF9. Any chance this will be fixed in EF8 considering it's a regression from EF7? |
@maumar Hello, My models are like below: public class Foo
{
public Foo1? Foo1{get;set;}
}
public class Foo1
{
public List<CustomField> CustomFields{get;set;}
}
public record CustomField
{
public Guid CustomFieldIdentifier { get; set; }
public decimal? Value { get; set; } = default(decimal?);
} And the configuration is like this: protected override void ConfigureOtherProperties(EntityTypeBuilder<Foo> builder)
{
builder.ToTable(nameof(Foo), "Foo");
builder.OwnsOne(x => x.Foo1, ownedNavigationBuilder =>
{
// this were not working.
//ownedNavigationBuilder.OwnsMany(p => p.CustomFields);
//ownedNavigationBuilder.Ignore(p => p.CustomFields);
ownedNavigationBuilder.ToJson();
});
} Every time I try to query the list of Foo I get the exception:
I am using EFCore 8.0.7, with postgres as database. Please can you tell me if there is any workaround into this?! |
@OrgesKreka can you file a new issue for this? Ideally, please include the JSON data that has been used to reproduce this. This generally happens when the JSON is shaped in a way that is according to what EF thinks it should be (e.g. expecting collection, got a single object). So for start I'd take a look at the JSON itself, but impossible to say more without seeing a full repro. |
@lenardchristopher apologies for late reply. We were debating whether to port the fix but ultimately decided against it. Reason is that the fix is quite complex, with a significant risk of introducing a regression (in fact, the initial fix did cause a regression which we luckily caught) and AsNoTrackingWithIdentityResolution is bit of a niche functionality. So this is a rare case where we won't be fixing a regression :( Sorry about that! |
… throws ArgumentOutOfRangeException This is a regression introduced in 9.0 when trying to address a different regression (#33073) Error is caused by a bug in JsonCorrectOrderOfEntitiesForChangeTrackerValidator, specifically it uses the initial SelectExpression to analyze structure of various shaper expressions in the query. Problem is that RelationalSplitCollectionShaperExpression has its own SelectExpression that described the collection - we should use that select expression rather than the parent. Fix is to update SelectExpression used to process the expression when are processing RelationalSplitCollectionShaperExpression Fixes #34728
… throws ArgumentOutOfRangeException Port of #34742 Fixes #34728 Description Error is caused by a bug in JsonCorrectOrderOfEntitiesForChangeTrackerValidator, specifically it uses the initial SelectExpression to analyze structure of various shaper expressions in the query. Problem is that RelationalSplitCollectionShaperExpression has its own SelectExpression that described the collection - we should use that select expression rather than the parent. Customer impact Some queries using split query and AsNoTrackingWithIdentityResolution fail during compilation with a cryptic error (index out of range) there is no good workaround outside of not using split query and/or AsNoTrackingWithIdentityResolution. The scenario is quite niche, requiring two query customizations (split query and AsNoTrackingWithIdentityResolution) as well as a specific shape. How found Customer reported on 9 RC1. Regression Yes, introduced in 9 while attempting to fix another regression from EF8 (#33073) Testing Multiple tests added. Risk Low. Change is straightforward - updating the state used in one of our visitors, we use that strategy in numerous places. The visitor itself is used for validation - it doesn't manipulate the shape of the query.
… throws ArgumentOutOfRangeException (#34742) This is a regression introduced in 9.0 when trying to address a different regression (#33073) Error is caused by a bug in JsonCorrectOrderOfEntitiesForChangeTrackerValidator, specifically it uses the initial SelectExpression to analyze structure of various shaper expressions in the query. Problem is that RelationalSplitCollectionShaperExpression has its own SelectExpression that described the collection - we should use that select expression rather than the parent. Fix is to update SelectExpression used to process the expression when are processing RelationalSplitCollectionShaperExpression Fixes #34728
… throws ArgumentOutOfRangeException (#34742) This is a regression introduced in 9.0 when trying to address a different regression (#33073) Error is caused by a bug in JsonCorrectOrderOfEntitiesForChangeTrackerValidator, specifically it uses the initial SelectExpression to analyze structure of various shaper expressions in the query. Problem is that RelationalSplitCollectionShaperExpression has its own SelectExpression that described the collection - we should use that select expression rather than the parent. Fix is to update SelectExpression used to process the expression when are processing RelationalSplitCollectionShaperExpression Fixes #34728
… throws ArgumentOutOfRangeException (#34742) (#34743) This is a regression introduced in 9.0 when trying to address a different regression (#33073) Error is caused by a bug in JsonCorrectOrderOfEntitiesForChangeTrackerValidator, specifically it uses the initial SelectExpression to analyze structure of various shaper expressions in the query. Problem is that RelationalSplitCollectionShaperExpression has its own SelectExpression that described the collection - we should use that select expression rather than the parent. Fix is to update SelectExpression used to process the expression when are processing RelationalSplitCollectionShaperExpression Fixes #34728
I have issues using Json columns.
I'm getting this error via single or split query while trying to query entities with JsonColumns
Error occurs if multiple instances of same object is queried. Works with AsNoTracking() but error with AsNoTrackingWithIdentityResolution()
project to reproduce issue:
https://github.com/DieBunnyxxx/EFCoreJsonException/tree/master
Include provider and version information
EF Core version: 8.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: NET 8.0
The text was updated successfully, but these errors were encountered: