Don't overwrite reference when querying for old principal

[ulrich-berkmueller]

How to reproduce:

A working xUnit test project is attached (see TestProject.zip)

1.) Given an entity (e.g. User) with a relation property (e.g. Address) and key property (e.g. AddressId)
2. ) And given some database entries

User(Id=1, AddressId=22)
Address(AddressId=22)
Address(AddressId=44)

3.) Load the entity without loading the relation

var user = context.Users.FirstOrDefault(x => x.Id == 1);

4.) Load the new relation we like to assign to the entity

var newAddress = context.Addresses.FirstOrDefault(x => x.AddressId == 44);

5.) Change the relation entity

user.Address = newAddress;

6.) When I load the initially assigned relation entity

context.Addresses.FirstOrDefault(x => x.AddressId == 22);

7.) Then the relation of the entity unexpectedly changes as well

Assert.Equal(44, user.Address.AddressId); // expected 44 but is actually 22

Workarounds

The test project also includes two workarounds:

Workaround 1: Load the initial and new relation entity.

context.Entry(user).Reference(x => x.Address).Load();
user.Address = newAddress;

Workaround 2: Access the change tracker entry of the entity that contains the relation. Read-access is sufficient.

user.Address = newAddress;
dbContext.Entry(user)

The following did not work (explicitly telling the DbContext that the entity has changed after assigning the new relation entity)

user.Address = newAddress;
context.Update(user);

Environment

The unexpected behavior was observed

• with EFCore 3.x as well es EFCore 6.x.
• with .NET Core 3.1 as well as .NET 6.0.
• independently of the database provider. (SqlServer, SQLite, ...)
• on Windows (not tested on another platforms)

[ajcvickers]

@ulrich-berkmueller context.ChangeTracker.DetectChanges() needs to be called after 'user.Address = newAddress;` so that EF is aware that change has been made. See Change Detection and Notifications.

[ulrich-berkmueller]

@ajcvickers thanks for the hint. It totally makes sense to me that the change tracker needs to be notified about the change.

For me however it was unexpected that user.Address = newAddress; context.Update(user); did not do this. It feels like a more natural unit of work API call, especially since the programmer knows what entity has changed.

Further it feels strange that accessing the meta data with dbContext.Entry(user) has the side effect to make the code work.

Anyway your recommendation is to always call context.ChangeTracker.DetectChanges() after changing a relation in case one is not sure if the entity and its relation was loaded before?

[ajcvickers]

@ulrich-berkmueller It's very unusual to use context.Update(user); like this. Update is typically used when you're not using the change tracker to track changes. The docs discuss when and when not to call DetectChanges, but typically it is needed if you are going to do something with the context that doesn't call DetectChanges automatically after changing the graph. We limit when it is called automatically because it has a significant perf impact.

[ulrich-berkmueller]

@ajcvickers Yes, the performance impact doing a full change tracking with context.ChangeTracker.DetectChanges() was also my concern. So it is better in that case to just notify the change tracker about the modified entity with dbContext.Entry(user)?

And you can definitively say that the problem described above is not a bug but a taken decision (e.g. performance reasons)?

Still, for me it doesn't feel good to silently overwrite an entity's relation that has not been loaded before even so it has been changed by the user (entity.relationId != entity.relation.id && entity.relationWAS == null && entity.relation != null) and this just by loading the originally assigned entity (e.g. content.MyRelation.ToList()`).
In simple CURD applications where the context is used just in one method it is most probably not an issue. But in applications where the context is passed to other methods you might not know what is loaded or changed and in what order.

So that others don't fall into the same trap, maybe one of the following can be done:

• Mention the problem in the docs (e.g. a section called "common pitfalls").
• Have a dev/debug option for DbContext that whould throw an exception in that case so that such issues can be detected more easily at runtime (see ValidateScopes for service providers)

[ajcvickers]

So it is better in that case to just notify the change tracker about the modified entity with dbContext.Entry(user)?

Not necessarily. Again, the docs I linked to go through various options. Notification entities could also be an option.

And you can definitively say that the problem described above is not a bug but a taken decision (e.g. performance reasons)?

I will investigate if this particular case is something that could be made better with a targeted local DetectChanges. But regardless, the general principle still applies.

But in applications where the context is passed to other methods you might not know what is loaded or changed and in what order.

I think you need to design your application around this. It's not that common to do explicit loading at random places, but in those places if the model may have been changed, then a call to DetectChanges before calling Load would make sense.

Mention the problem in the docs (e.g. a section called "common pitfalls").

Is there something missing from the change tracking docs that I have referenced?

Have a dev/debug option for DbContext that whould throw an exception in that case so that such issues can be detected more easily at runtime (see ValidateScopes for service providers)

This is an interesting idea. I'm not sure it really has enough value, but we will discuss.

[ulrich-berkmueller]

I think you need to design your application around this. It's not that common to do explicit loading at random places, but in those places if the model may have been changed, then a call to DetectChanges before calling Load would make sense.

Yes, our "workaround" will be to add calls to DetectChanges() after changing references as the documentation says. However it clutters the codebase a lot.

Imagine you have an application that has an event system where you can inject plugin code. Let's say it is a webshop. You place an order and when placing the order the inventory needs to get updated which is happening in an inventory plugin. This should run in the same unit of work (transaction). Here you have multiple code bases using the same DbContext. Not calling DetectChanges() on either side might brake the code of the other one (if the old reference was loaded on one side but has changed in the meanwhile on the other side).

Is there something missing from the change tracking docs that I have referenced?

Just the consequences if you miss to add the DetectChanges() call. Especially that loading the previous reference entity from database after it was changed to another one in the principal entity overwrites you change. This would be helpful for debugging purpose or when you search for it ("Why is my changed reference entity overwritten when loading a list of referenced entities", ...).
The current documentation, if I haven't missed anything, only tells about synchronisation between reference key and reference entity but not about the side-effects like in our case. Following the documentation for me a call to SaveChanges() automatically calls DetectChanges() and synchronisation of that key property happens automatically. But I did not found a notice anywhere that loading of entities (of reference type) can cause side-effects when you don't do that on partially loaded principal entities.
As a programmer, you get quite mindful when you read about side-effects in the documentation.

Thank you also for the time you spend or have spent on this!