Cascade deletions ignore current state of entities resulting in unexpected data loss

[bachratyg]

Regarding this breaking change: https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-3.0/breaking-changes#cascade-deletions-now-happen-immediately-by-default

When the parent entity is removed cascade deletes happen immediately by default but ignores changes already made on child entities. This causes unexpected data loss as entities which are not orphaned are also removed.

Steps to reproduce

Using the following model

class Context : DbContext
{
	public DbSet<Parent> Parents { get; set; }
	public DbSet<Child> Children { get; set; }
}
class Parent
{
	public long Id { get; set; }
}
class Child
{
	public long Id { get; set; }

	[Required]
	public Parent Parent { get; set; }
}

and the following helper

void DumpState()
{
	foreach( var entry in context.ChangeTracker.Entries() )
		Console.WriteLine($"{entry.Entity.GetType().Name}:{((dynamic)entry).Entity.Id}:{entry.State}");
}

Construct initial state

var parent1 = new Parent() { Id = 1 };
var parent2 = new Parent() { Id = 2 };
var child = new Child() { Id = 3, Parent = parent1 };
context.AddRange(parent1, parent2, child);
context.ChangeTracker.AcceptAllChanges();
DumpState();
// actual output is
//     parent:1:unchanged
//     parent:2:unchanged
//     child:3:unchanged

Retarget the child to a different parent, remove the old parent

child.Parent = parent2;
context.Remove(parent1);
DumpState();
// expected output is
//     parent:1:deleted
//     parent:2:unchanged
//     child:3:modified
// actual output is
//     parent:1:deleted
//     parent:2:unchanged
//     child:3:deleted

The child gets marked for deletion even though it is no longer associated with the old parent.

Possible workarounds

1. Use the mitigation as per the docs:

context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.OnSaveChanges;

This has the drawback of completely reverting the new behavior and all its benefits.

2. Call DetectChanges before removing stuff

child.Parent = parent2;
context.DetectChanges();
context.Remove(parent1);
// or
public override EntityEntry Remove(object entity) { this.ChangeTracker.DetectChanges(); return base.Remove(entity); }
public override EntityEntry<TEntity> Remove<TEntity>(TEntity entity) { this.ChangeTracker.DetectChanges(); return base.Remove(entity); }
public override void RemoveRange(IEnumerable<object> entities) { this.ChangeTracker.DetectChanges(); base.RemoveRange(entities); }
public override void RemoveRange(params object[] entities) { this.ChangeTracker.DetectChanges(); base.RemoveRange(entities); }

This is quite tedious and error prone to use at multiple call sites, somewhat better but still a nuisance for multiple contexts. Calling DetectChanges may also have some perf impact.

3. Detect changes before cascade by overriding the behavior of the state manager.

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
{
	optionsBuilder.ReplaceService<IStateManager, FixedStateManager>();
}
class FixedStateManager : StateManager
{
	// Some code omitted for brevity

	// Detect changes on all entities, with some perf impact
	public override void CascadeDelete(InternalEntityEntry entry, bool force, IEnumerable<IForeignKey> foreignKeys = null)
	{
		this.Context.ChangeTracker.DetectChanges();
		base.CascadeChanges(entry, force, foreignKeys);
	}

	// Detect changes for affected entities only
	public override void CascadeDelete(InternalEntityEntry entry, bool force, IEnumerable<IForeignKey> foreignKeys = null)
	{
		foreach( var foreignKey in foreignKeys ?? entry.EntityType.GetReferencingForeignKeys() )
		{
			if( foreignKey.DeleteBehavior != DeleteBehavior.ClientNoAction )
			{
				foreach( var item in (GetDependentsFromNavigation(entry, foreignKey) ?? GetDependents(entry, foreignKey)).ToList() )
					item.ToEntityEntry().DetectChanges();
			}
		}
		base.CascadeDelete(entry, force, foreignKeys);
	}
}

This is somewhat more complex but uses internal APIs therefore fragile.

Further technical details

EF Core version: 3.0.0-preview9.19423.6
Database provider: Microsoft.EntityFrameworkCore.SqlServer (not relevant)
Target framework: .NET Core 3.0
Operating system: Windows 10 Pro 1903 (18362.356)
IDE: Visual Studio 2019 16.3.0 Preview 3.0

Complete runnable code listing to reproduce the issue

ConsoleApp6.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="3.0.0-preview9.19423.6" />
  </ItemGroup>
</Project>

Program.cs

using System;
using System.ComponentModel.DataAnnotations;
using Microsoft.EntityFrameworkCore;

namespace ConsoleApp6
{
	class Context : DbContext
	{
		public DbSet<Parent> Parents { get; set; }
		public DbSet<Child> Children { get; set; }
		protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
		{
			optionsBuilder.UseSqlServer("data source=dummy");
		}
	}
	class Parent
	{
		public long Id { get; set; }
	}
	class Child
	{
		public long Id { get; set; }

		[Required]
		public Parent Parent { get; set; }
	}
	class Program
	{
		static void Main()
		{
			using var context = new Context();
			void DumpState()
			{
				foreach( var entry in context.ChangeTracker.Entries() )
					Console.WriteLine($"{entry.Entity.GetType().Name}:{((dynamic)entry).Entity.Id}:{entry.State}");
			}

			// Construct initial state
			var parent1 = new Parent() { Id = 1 };
			var parent2 = new Parent() { Id = 2 };
			var child = new Child() { Id = 3, Parent = parent1 };
			context.AddRange(parent1, parent2, child);
			context.ChangeTracker.AcceptAllChanges();
			DumpState();

			// Reproduce issue
			child.Parent = parent2;
			context.Remove(parent1);
			DumpState();
			Console.ReadLine();
		}
	}
}

[bachratyg]

Also reproduces on the nightly build: 3.0.0-rc2.19462.2

[divega]

Triage: The behavior observed is currently by design. You can opt out of immediate cascade behavior. We think it would be good to re-think if immediate cascading is the best default, but we can't change it now, so marking it as consider for next release.

[bachratyg]

I'm not sure I follow you. This is not a problem with immediate cascade behavior in general, or that it is the default. The problem is with immediate cascade deleting non-related entities.

[AndriySvyryd]

@bachratyg The immediate cascade delete only does a local DetectChanges, that is it doesn't take into account any changes on the dependents. As another workaround you can call local DetectChanges on every dependent.

context.Entry(child).DetectChanges();

[bachratyg]

I don't see any difference if I declare the principal to dependent navigation and manipulate that:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
	modelBuilder.Entity<Parent>().HasMany(p => p.Children).WithOne().IsRequired();
}
class Parent
{
	public long Id { get; set; }
	public ICollection<Child> Children { get; set; } = new HashSet<Child>();
}
class Child
{
	public long Id { get; set; }
}
parent1.Children.Remove(child);
parent2.Children.Add(child);
context.Remove(parent1);
// actual output is
//     parent:1:deleted
//     parent:2:unchanged
//     child:3:deleted

In fact in this case the child is actually marked for deletion without even removing the old parent:

parent1.Children.Remove(child);
parent2.Children.Add(child);
// actual output is
//     parent:1:unchanged
//     parent:2:unchanged
//     child:3:deleted

Same happens if I also add the dependent to principal navigation to the model and set that too (i.e. child.Parent = parent2).

This works, but doesn't feel like the pit of success:

parent2.Children.Add(child);
context.ChangeTracker.DetectChanges();  // must be called *beore* remove and *after* add
parent1.Children.Remove(child);
// actual output is
//     parent:1:unchanged
//     parent:2:unchanged
//     child:3:modified

[ajcvickers]

Verified that behavior is correct in 5.0 if DetectChanges is called between these two lines:

child.Parent = parent2;
context.Remove(parent1);

This is a case where DetectChanges is needed since child isn't in the local graph of parent1.

[bachratyg]

@divega wrote in #17828 (comment)

We think it would be good to re-think if immediate cascading is the best default

Were there any thoughts given to the default behavior? I'm really curious about the reasoning.

The docs and #10114 refers to two main reasons this breaking change was made:

1. data binding app
   This feels like UI concerns are driving data access features. Can you share some data about how prevalent it seems to you to use the context directly for data binding? Or maybe post some concrete use cases? In my experience it is usually better to separate these concerns.
   Besides, the context has no information about what UI technology is used or whether it needs the up-to-date state immediately or only in a render pass somewhat later.

2. audit state
   If I would want to audit state I would first make sure the state is current. Calling the relevant methods are expected and intuitive. The context might even do it for me assuming something like Implement events for before and after SaveChanges #15910 is implemented. Then I would see no difference between Immediate and OnSaveChanges. This does not seem to be a strong reason to change the default behavior from pre-3.

@ajcvickers wrote in #17828 (comment)

Verified that behavior is correct in 5.0 if DetectChanges is called between these two lines:
This is a case where DetectChanges is needed since child isn't in the local graph of parent1

Are you suggesting I should litter my code with DetectChanges (all in nontrivial places) because the default behavior was changed so that the code need not be littered with DetectChanges in a different use case?

Even if the current default is sound I would still think doing a recursive local detect on entities just about to be cascade deleted (see workaround 3 in op) would be the correct thing to do. DbSet.Local also does a full DetectChanges when the getter is called (i.e. data binding kicks in). Why not go all the way then?

The altered object graph should reflect the desired state yet immediate cascades change it even if cascade happens only during SaveChanges. Was any thought given to my last 2 examples in #17828 (comment)?

Also when DeleteOrphansTiming is set to Immediate (the default) the result is different depending on the order change detection processes entities. Basic setup:

class Context : DbContext
{
	public DbSet<Parent> Parents { get; set; }
	public DbSet<Child> Children { get; set; }
	protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
	{
		optionsBuilder.UseSqlServer("data source=dummy");
	}
	protected override void OnModelCreating(ModelBuilder modelBuilder)
	{
		modelBuilder.Entity<Parent>().HasMany(p => p.Children).WithOne().IsRequired();
	}
}
class Parent
{
	public long Id { get; set; }
	public ICollection<Child> Children { get; set; } = new HashSet<Child>();
}
class Child
{
	public long Id { get; set; }
}
class Program
{
	static void Main()
	{
		// arrange
		using var context = new Context();
		var parent1 = new Parent() { Id = 1 };
		var parent2 = new Parent() { Id = 2 };
		var child = new Child() { Id = 3 };
		parent1.Children.Add(child);
		context.AddRange(parent1, parent2, child);
		context.ChangeTracker.AcceptAllChanges();

		// act - this does not trigger a state change
		parent1.Children.Remove(child);
		parent2.Children.Add(child);

		// either of the next two snippets come here
	}
}

Snippet 1:

Console.WriteLine(context.Entry(parent2).State);    // prints "Unchanged"
Console.WriteLine(context.Entry(child).State);    // prints "Modified"

Snippet 2

Console.WriteLine(context.Entry(child).State);    // prints "Deleted" ==> not "Modified" as in snippet 1 above
Console.WriteLine(context.Entry(parent2).State);    // prints "Unchanged"

[ajcvickers]

@bachratyg In general full DetectChanges needs to be called after making modifications to the graph before continuing to use EF to work with it. This hasn't changed between 2.2 and 3.0. It's just that in 2.2 you were not hitting any issues even when not calling DetectChanges.

The general problem here is that DetectChanges is slow, so we only call it automatically when doing so is likely to always be necessary. Add rarely really needs a full DetectChanges, so EF doesn't call it automatically. (See https://blog.oneunicorn.com/2012/03/10/secrets-of-detectchanges-part-1-what-does-detectchanges-do/ for further discussion of the general problem here.)

That being said it is not easy as an application developer to understand when a call to DetectChanges can be safely skipped. We will discuss this as a team.

[ajcvickers]

Notes from team discussion:

• Look into improving local DetectChanges (avoiding any big perf impact)
• Document DetectChanges better

[ajcvickers]

See also the slightly different scenario in #19652