Set CollectionEntry.IsModified to true when removing items from a collection

[joaodvargas]

File a bug

The IsModified property on a CollectionEntry does not correctly reflect it has been modified if the action is only to remove an entry from the collection.

The intent seems to be for that field to reflect that, given in the source code there are code checks for entries with a State of Deleted and previous work done for #10450.

Include your code

This is easy to reproduce adding some additional cases to your SqlServerEndToEndTests fixture, which I've tested in my own efcore fork:

[ConditionalFact]
public void Removing_an_item_from_a_collection_marks_it_as_modified()
{
	using var testDatabase = SqlServerTestStore.CreateInitialized(DatabaseName);
	var options = Fixture.CreateOptions(testDatabase);

	using var context = new GameDbContext(options);
	context.Database.EnsureCreatedResiliently();

	var player = new PlayerCharacter(
		new Level { Game = new Game() });

	var weapon = new Item { Id = 1, Game = player.Game };

	player.Items.Add(weapon);

	context.Characters.Add(player);

	context.SaveChanges();

	player.Items.Remove(weapon);

	context.ChangeTracker.DetectChanges();
	
	// this fails
	Assert.True(context.Entry(player).Collection(p => p.Items).IsModified);
}

⚠️ As I was testing these behaviours I noticed that attempting to delete the weapon entity after setting it as player.CurrentWeapon = weapon; would throw an exception as it seems EF tries to clear both foreign keys for CurrentWeapon in PlayerCharacter - CurrentWeaponId and GameId - however because GameId is used in other required relationships that causes an exception to be thrown. Clearing the weapon player.CurrentWeapon = null; as opposed to deleting the entity context.Remove(weapon); seems to work fine, so perhaps that's a possible bug as well?

Include stack traces

n/a

Include verbose output

n/a

Include provider and version information

Using latest released EF Core version (3.1.1) targeting .NET 5.0

[ajcvickers]

@joaodvargas In retrospect, I wish we had not implemented IsModified on navigation properties because its meaning is ambiguous when it is referring to a collection of other entities. What we settled on is that the collection is considered modified if any of the entities it contains will result in a database update on SaveChanges. This means that entities removed from the collection do not impact IsModified even if they were previously in the collection. We will discuss whether this is something we can/should change.

[joaodvargas]

@ajcvickers thanks for the reply. I'd say this is also a problem of consistency, not just ambiguity - and it affects both Collection and Reference navigations. It's also very counterintuitive as manipulating the navigation properties directly doesn't flag them as modified, but doing so indirectly by removing referenced entities via context does. I added a few very simple test scenarios on my fork to exemplify this using a simple hierarchy of Order with one optional Address and many optional OrderLine:

Test	Result
Remove collection entity from context context.OrderLines.Remove(orderLine)	context.Entry(order).Collection(o => o.Lines).IsModified✔️
Remove entity from collection order.Lines.Remove(orderLine);	context.Entry(order).Collection(o => o.Lines).IsModified ❌ - code here
Remove reference entity from context context.Addresses.Remove(address)	context.Entry(order).Reference(o => o.ShippingAddress).IsModified✔️
Remove reference from entity order.ShippingAddress = null;	context.Entry(order).Reference(o => o.ShippingAddress).IsModified ❌ - code here

I've added tests for adding entities via those 4 different options and they all pass, furthering the inconsistency. Full test suite and setup can be seen here.

My actual use case for this is to from a top level entity (my aggregate root) be able to identify, on SaveChanges(), if any of its navigation entities have been modified so I can bump it's own version. This would be relatively easy if I could trust IsModified to return true for removed entities as I explore its graph. But currently I am struggling to consistently determine removed entities. I've had limited success by tapping into the internal EF apis and using GetRelationshipSnapshotValue to determine if a value existed when it was first loaded from the db but even that only works for reference entities because for collections we actually update the saved snapshot when we run the ChangeTracker for removing an entity from the collection! Any thoughts on how I could achieve my goal of reliably finding if a top level entity has had any of its navigation entities added/modified/deleted at SaveChanges() time?

[Asherslab]

Would just like to add that i've just come across this very issue myself (and find it interesting that this issue is fresh hahah).
Would greatly appreciate it if (as @joaodvargas said) the IsModified property was set correctly on removal as well.

As a note if you are able to figure out a stopgap for the meantime before something is implemented it would be great if you could share.

[joaodvargas]

As a note if you are able to figure out a stopgap for the meantime before something is implemented it would be great if you could share.

I'm afraid I only got as far as I mentioned in my previous comment - I can use the internal snapshots to figure out when a reference navigation is deleted (the current entity has the property clear but the snapshot still points to the previous entity, which is now marked Deleted), but once DetectChanges() runs EF seems to update the snapshotted value for the collection navigations which means I've no way of knowing what values were there before, and check if their state is now Deleted... perhaps attempting to create some "hooks" to create my own manual snapshot when the entity is read from the db and compare back to that on saving? Doesn't seem ideal 😓

I'm currently considering having to track these sort of changes in my BL/Domain and manually mark the aggregate entity as changed there, forcing a version bump that way. Not as bullet proof as something the DbContext would enforce but don't see a better option until / unless this is resolved 🤷🏼‍♂️

[Asherslab]

I'm currently considering having to track these sort of changes in my BL/Domain and manually mark the aggregate entity as changed there, forcing a version bump that way. Not as bullet proof as something the DbContext would enforce but don't see a better option until / unless this is resolved 🤷🏼‍♂️

that's exactly what i started doing yesterday. not exactly the best solution but hey it works for now

[sergiohms]

My actual use case for this is to from a top level entity (my aggregate root) be able to identify, on SaveChanges(), if any of its navigation entities have been modified so I can bump it's own version.

I am in the same situation as described by @joaodvargas . A solution for such a scenario without having to contaminate the domain would be great.

[admg]

Same problem here. I'm trying to implement mixed concurrency mode - if there's a conflict (DbUpdateConcurrencyException) I apply changes only for properties that have IsModified = true (updated in logic). It doesn't work when item was removed from collection. Moreover there's something wrong with Reload() method on entity because it doesn't reload collection items from database (I'm using Cosmos db provider)

[julian-altech]

I've just cam across this after wondering why our BL was failing when looking at IsModified in certain scenarios (i.e. removing entries), Would love it if this got some traction, guess I'm stuck with implementing myself for the time being

[nettashamir-allocate]

I have the same issue too. Would be great to see a fix for this.

[alanmachadouk]

Gald it wasn't only me finding CollectionEntry.IsModified not acting as logic dictates

[mrukas]

I also noticed this behaviour. It currently really a pain for me to bypass this issue. Is it possible to load the original database values for the collection and do a manual compare?

[roji]

Everyone, please refrain from posting comments that say "me too" or "please give this more priority"; this issue currently has only 13 votes in total, meaning that it's affecting very few users, and therefore comes up pretty low in the list of issues. That doesn't mean it necesarily won't get done, but for now please just upvote it rather than posting comments.