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

Open
#24,076 13 comments 54 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
35/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Stale
Tech stack
csharp
Domain
databases

Research direction

Start with src/EFCore/ChangeTracking/CollectionEntry.cs, especially the IsModified logic around lines 138-144, and the reproduction fixture in test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs. Run or extend the shown removal test; done means DetectChanges makes the collection entry report IsModified as true after an item is removed.

Written by the indexing model from the issue text.

Description

area-change-tracking customer-reported

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 https://github.com/dotnet/efcore/issues/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

Dominant language
C#
Stars
14.8k
Forks
3.4k
Avg merge
2d 5h
Merged PRs (30d)
134

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from dotnet/efcore

All issues in dotnet/efcore

Similar issues

More C# issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.