Bug when calling DetectChanges repeatedly

[Angelinsky7]

If I'm trying to update a complex MainEntity with many-to-many SubEntities attach to it, i need to call exactly the same number of time

context.ChangeTracker.Entries<TEntity>().ToList().ForEach(p => p.State = EntityState.Unchanged);

to change the SubEntities state to Unchanged otherwise i got an exception. (See Example 4, the other example are working correctly and are only there to explain the logic of example 4)
The way i update the MainEntity and SubEntity are based on AutoMapper logic (any version so far) but in the sample project any dependencies to AutoMapper were removed and everything is done by "Hand".

We can argue of the way i'm updating the MainEntity many-to-many relationship and that in a maybe next release version we could use the many-to-many ef core way, but for now it's not possible.
What i don't like is the fact that there is NO consistency between the different call to context.ChangeTracker.Entries and it shouldn't: Either we never have this exception at all, of we have it all the time but NOT this current behavior.
I would gladly prefer the 'no exception' situation and be able to clean the state of the entities that i don't care and only update the one that had changed. (because ef core seems to be able to recognize that it's the same entity, because they share the same key but cannot handle the 'i don't want to update it situation'.

Thanks for this amazing lib and i hope that my explanation and sample project could be helpful.

Steps to reproduce

Sample project attached
EfCoreBugUpdate.zip

Further technical details

EF Core version: 3.1.6
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: NET Core 3.1
Operating system: Windows 10
IDE: Visual Studio 2019 16.6.4

[ajcvickers]

@Angelinsky7 This line:

entity.SubEntities = updateEntity.SubEntities;

Replaces the list with a new list containing different instances. This results in the context attempting to track multiple instances with the same primary key, which is not supported.

However, I'm really not sure what you are attempting to do here. What is the purpose of setting entity states to Unchanged here?

[Angelinsky7]

Thanks for your answer.
Yes, i know this is why i got an issue.

But the things is I don't see any other way to update a many-to-many collection without doing that or by traversing the whole many-to-many collection and checking every link from both side (to find the ones that i need to delete, update or insert), this way ef core does all the job for me (and totally correctly)

What i don't like is the fact that if i call multiple time context.ChangeTracker.Entries<TEntity>() the behavior of the ef core context is NOT consistent and change ! (and like i said i would prefer that ef core let me handle the error by choosing which entity with the same key i want to detach or delete or whatever, than crashing. Because now, when you are in this situation, you need to either destroy the context and retry the operation again from the start or you do my "hack" and refresh the context as many time to need and then ef core behave correctly and let you handle the faulty entities -> what i would like to have without the "hack")

I set the state of the entities that already exist in the database. Like i said it's a many-to-many issue, MainEntity and SubEntity already exist in the db, the only thing that i change is the link between them. Of course, not directly editing each links but by editing either MainEntity or SubEntity... So each time ef core try to update the entities it also want to create the linked ones, i simply tell ef core, "don't bother, it's ok", and in fact, it's working really well like that (i can update a whole complex tree of data without any issue and really easily like that, by making the difference between entities linked with the one that i'm trying to update and entities that are part of the one)

I hope i made myself clear (that i don't want that ef core resolve my issues, but that it behaves all the time the same way, or at least that someone explain me why it doesn't behave all the time the same way, because for me it's a bug and not by design and if it's by design i cannot understand why you want something not predictable by design)

Thanks !

[ajcvickers]

@Angelinsky7 Can you clearly describe the inconsistent behaviors of context.ChangeTracker.Entries<TEntity>()? When is it doing something you don't expect?

[Angelinsky7]

@ajcvickers
in the example 4, the first 6th times i'm calling it, it responds with an exception (that you can see if you launch my sample app) the 7th and all the next, it responds with the full list of entities that i ask (that you can see if you launch my sample app).

On more time, i feel it's not normal, either way it should never send the exception (what i would like, like this i could replace all the line between 378 to 385 by a simple context.ChangeTracker.Entries<SubEntity>().ToList().ForEach(p => p.State = EntityState.Unchanged); and catch the real exceptions when they appear... because in this case there shouldn't be any exception) or always...

[Angelinsky7]

@ajcvickers and of course the number of time depends on the number of the same entities that you update (you can see that in the sample app, by switching the commented lines from 351 to 369)

[ajcvickers]

@Angelinsky7 The resulting behavior of setting the state of every tracked instance to Unchanged is dependent on which instances are tracked and what their current states are. For example, an instance in the Added state can have the same key value as an instance in the Deleted state, which is valid because the latter will be deleted from the database before inserting the former. Setting the state of both instances to Unchanged will then result in an exception since there are now two instances both asserting that they are Unchanged from what is in the database, but both with the same key value.

In addition to this, context.ChangeTracker.Entries automatically calls DetectChanges to detect changes made to the tracked object graph. If the changes to the object graph result in an invalid state, then an exception is thrown.

[Angelinsky7]

EfCoreBugUpdate.zip
@ajcvickers thanks again for you answer.
I updated the sample to test if what you were saying was true. And either, i don't understand something (and i can totally accept that it's certainly the case) either you're wrong about this behavior...

Example 5 will only call 7 times context.ChangeTracker.Entries<SubEntity>().ToList() without updating the state of any SubEntity and will crash exactly the same way as the others (not totally true, one more time because i'm adding one more SubEntity to the list). So calling something to GET informations about the state of my entities CHANGE the state of those (one more time seems to me not normal). Line 534, finally update the entities without any issue.

Example 6 will only ask for Added SubEntities and will crash also. It also shows that changing the state between Unchanged and Detached does not change anything in this particular issue (one more time we don't care about any 'SubEntities', because in this many-to-many problem, we don't want those entities to be handle by the context. If i could tell ef core: In this "transaction" do as if 'SubEntity' don't exist it will be the same, in fact it's why i'm trying to acheive)

[Angelinsky7]

And of course, i didn't read until the end.
So now the question is : Why do i need to call DetectChanges n times to get my context back in a correct state ? I feel that i should be able to call it only once to get either always an exception or directly a correct state.
With the current behavior, ef core is saying : "if you continue to ask me about the same thing over and over, at some point i'll tell you what you want to hear.... But does it make it true ? "

[Angelinsky7]

@ajcvickers so i did try this last test in the sample app to confirm what you said... in example 7 i'm only doing call to context.ChangeTracker.DetectChanges() and then finally a context.ChangeTracker.Entries<SubEntity>().ToList().ForEach(p => p.State = EntityState.Unchanged); and it's working...
The only issue is that i need to make 8 call to context.ChangeTracker.DetectChanges() to make it work. One less and everything fail.
EfCoreBugUpdate.zip

[ajcvickers]

@Angelinsky7 This does look like a bug. DetectChanges should keep failing with the same error no matter how many times it is called. However, this kind of error indicates that the application has done something that is not valid--in this case attempting to track two instances with the same ID. In general, applications should not assume that the context instance can necessarily be put back into a clean state after this happens. In other words, this kind of exception is not expected to be caught and handled.

[Angelinsky7]

@ajcvickers so now... what should i do to avoid putting ef core into that state ?

what i'am thinking :

• A way to tell ef core : please can you don't care about this SubEntity for this particular operation.
• A way to tell ef core, if you find 2 entities with the same key (but that are a different instance) let me decide which one is invalid
• A way to ask ef core a list of all entities without calling DetectChanges, or even better without sending the Exception (like that i could maybe detach the ones that i know are invalid)

What would be fantastic is to have a way to restore a correct state... Because this is clear that ef core cannot choose for me and that it cannot do this automatically but if we had a way to iterate through all entities that would have a EntityState.Invalid and choose what we would like to do with them, it would be awesome.

I can totally understand that it's certainly not the way it's intended or it's maybe not doable in the current state.
But now, with this bug, i can handle complex update over a complex data structure with simple operation and clarity of code. I certainly don't like the existence of the bug but if it's removed (even if from what i understand about you answer is that this behavior is because i don't use ef core correctly), i need to find a way to continue to do it, so have you any idea of how i could handle this kind of issue without unnecessary round trip to the database and a simple solution (without needing to check every instance of my object to replace them with the one the ef core knows) ??

But i could ask in another issue/stackoverflow of course.

Thanks again for our answers

[ajcvickers]

@Angelinsky7 The disconnected entities documentation goes through some of the basic patterns for this. In addition, #21910 is tracking adding a hook to help resolve identity conflicts.

[Angelinsky7]

@ajcvickers thanks for you answer.
The disconnected entities documentation is really too simple for this kind of problem but thanks for the link.
I think you wanted to talk about #20124 for this one, no ?
But i would very want to talk about the possibility to tell ef core to skip handling any operation of a particular entity in some operation (with the same context), because it could be so useful.
I know it's not really how ef core was design, but i really want to understand how you think we should handle the insert / update operation of entities that reference other entities already in the database in a disconnected pattern. Because i really think, it's what is causing the bug, or at least it would be a good way to not cause it.

In any case, thanks for your support.

[ajcvickers]

@Angelinsky7 Can you describe the specific behavior you want for, "the possibility to tell ef core to skip handling any operation of a particular entity in some operation." What do you mean by "skip handling"?

[Angelinsky7]

@ajcvickers What i would like (but did not think about all the issues that could depend on that choice) is something like that : (Taken from Example 1 from SampleApp, but it would be the same for update or delete)

I don't particularly like my design proposal and i could already see some issue with it, but it could be very handy to have this kind of feature, because like that we could easily do complex many-to-many operations inside complex graph of object without relaying on some obscure code that would do it for use. (we could choose what we want to track and don't during some operations)

try {
   // Convert Dto Entity to Database using whatever... (automapper or vanilla transformation for example)
    MainEntity mainEntity = mainDto.ToEntity();

    // Remove 'SubEntities' because we don't care
    context.Ignore<SubEntity>();

    // Add to the context (Here a JSON representation of the Entity added to the context)
    // What is important here is that all SubEntity already exist in the database and don't need ANY operation on them. 
    // In this operational context (and only this one) SubEntity should be ignored.
    // Of course, in a normal operation Id (7f27426c-f3cb-400b-8eb1-a3813c132dcb) should be null in case of an 'Insert' but like that it was easier to understand
    // 
    // MainEntity {
    //   "Id": "7f27426c-f3cb-400b-8eb1-a3813c132dcb",
    //   "Property": "Main Entity 1",
    //   "SubEntities: [
    //    {
    //       "MainEntityId": "7f27426c-f3cb-400b-8eb1-a3813c132dcb",
    //       "MainEntity": "REF_TO_PARENT,
    //       "SubEntityId": "e749e897-8886-404d-91f3-cd32ceb4ca44",
    //       "SubEntity": {
    //           "Id": "e749e897-8886-404d-91f3-cd32ceb4ca44"
    //           "Property": "Sub Entity 1"
    //        }
    //    },
    //    {
    //       "MainEntityId": "7f27426c-f3cb-400b-8eb1-a3813c132dcb",
    //       "MainEntity": "REF_TO_PARENT,
    //       "SubEntityId": "5456eaa1-3036-4abd-ae73-1b7b8ded01bd",
    //       "SubEntity": {
    //           "Id": "5456eaa1-3036-4abd-ae73-1b7b8ded01bd"
    //           "Property": "Sub Entity 2"
    //        }
    //    },
    //    ]
    // }
    context.Add(mainEntity);

    // Now if we look at the ChangeTracker.Entries
    // 1. MainEntity (Key: 7f27426c-f3cb-400b-8eb1-a3813c132dcb) Added
    // 2. MMMainSub (Key: 7f27426c-f3cb-400b-8eb1-a3813c132dcb, e749e897-8886-404d-91f3-cd32ceb4ca44) Added
    // 3. MMMainSub (Key: 7f27426c-f3cb-400b-8eb1-a3813c132dcb, 5456eaa1-3036-4abd-ae73-1b7b8ded01bd) Added

    // Normally what we would have (without the proposal)
    // 4. SubEntity (Key: e749e897-8886-404d-91f3-cd32ceb4ca44) Added
    // 5. SubEntity (Key: 5456eaa1-3036-4abd-ae73-1b7b8ded01bd) Added

    // And so this command comes : 
    // context.ChangeTracker.Entries<SubEntity>().ToList().ForEach(p => p.State = EntityState.Unchanged);

    // Save to database
    context.SaveChanges();

    // Restore the normal context (or we could imagine that on SaveChanges we restore automatically)
    context.RestoreIgnore();

} catch (Exception ex) {
    Console.WriteLine(ex.InnerException?.Message);
    Console.WriteLine("Should not crash here because ef core is not trying to insert SubEntities that already exists
}

I hope that i made myself clear and thanks again to take the time.

[ajcvickers]

@Angelinsky7 I don't think this is something we're going to do. Resolving ambiguities/conflicts in such an approach would be difficult.

[Angelinsky7]

@ajcvickers i can understand that there would be some complexity but i was more thinking that it would be exactly like if my fields were temporarily mark as unmapped, so no ambiguities/conflicts to handle. Otherwise, again, i don't have any ideas how to solve this without this bug. I tried to make some test but everytime it comes back to this issue. Because of the many-to-many behavior the SubEntity is so deep in the MainEntity (in some scenari) that trying to traverse all field, all list, all sub-objects to find the ones that shouldn't be handle by ef core starts to become so complexe that i'm starting to wonder if it wouldn't be easier to drop totally ef core (and it make me sad)

[ajcvickers]

Note for triage: this issue is about retaining state after an exception is thrown to ensure if the application tries to continue the same exception is thrown again. Preserving state after the first exception is thrown so that it will always be thrown again is not trivial here. I don't think the value is worth the risk of breaking something else, or the cost of doing the work and maintaining the additional complexity.

See also dotnet/EntityFramework.Docs#3401