Severing required navigation deletes the dependent entity

[soloham]

Steps to reproduce

Getting the scenario from my project codebase into a stripped-down version to demonstrate the bug was tricky. Still, I have successfully managed to do so and the below snippet can be tested to see that TestStep with Id 2 when reparented from TestStep 1 to Test Stage 1, is being marked as deleted by EF.

using AutoMapper;
using AutoMapper.EquivalencyExpression;

using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;

class Program
{
    static ServiceCollection services = new ServiceCollection();

    static void Main(string[] args)
    {
        var dbContext = new TestingContext();
        services.AddSingleton<TestingContext>();
        var mapper = InitMap();

        // Init Scenario
        var stepType = new StepType { Name = "Test Type" };
        dbContext.StepTypes.Add(stepType);
        dbContext.SaveChanges();

        var testCase = new TestCase();
        var stage = new TestStage();
        var step = new TestStep();
        step.TypeId = 1;
        step.Children.Add(new TestStep { TypeId = 1 });
        stage.TestSteps.Add(step);
        testCase.TestStages.Add(stage);

        dbContext.TestCases.Add(testCase);
        dbContext.SaveChanges();

        // Construct DTO (goes to client side)
        dbContext.ChangeTracker.Clear(); // Clearing to mimick a new API request scope
        var existingTestCaseToMap = dbContext.TestCases
            .Include(x => x.TestStages)
                .ThenInclude(x => x.TestSteps)
                    .ThenInclude(x => x.Children)
                        .ThenInclude(x => x.Type)
            .First();
        var testCaseDto = mapper.Map<TestCaseDto>(existingTestCaseToMap);

        // Augment DTO (done at client side)
        var stepToReparent = testCaseDto.TestStages[0].TestSteps[0].Children[0];
        testCaseDto.TestStages[0].TestSteps[0].Children.Remove(stepToReparent);
        stepToReparent.ParentId = null;
        testCaseDto.TestStages[0].TestSteps.Add(stepToReparent);
        stepToReparent.TestStageId = testCaseDto.Id;

        var testStages = dbContext.TestStages.ToArray();
        Console.WriteLine($"{testStages[0].TestSteps.Count} {testStages[0].TestSteps[0].Children.Count()}"); // 1 1

        // Map to entity (done at server side)
        dbContext.ChangeTracker.Clear(); // Clearing to mimic a new API request scope
        var existingTestCase = dbContext.TestCases
            .Include(x => x.TestStages)
                .ThenInclude(x => x.TestSteps)
                    .ThenInclude(x => x.Children)
                        .ThenInclude(x => x.Type)
            .First();

        // mapper.Map(stageDto, existingStage);
        // If we directly map as above, automapper keeps all references intact except
        // for the entity being re-parented, which makes sense since
        // there was nothing to match against that it was aware of.
        // This causes EF to think we are adding a new TestStep with PK 2
        // while another with that PK is already being tracked, resulting in an exception.

        // Therefore we have to map while ensuring the references remain intact by re-hydrating the hierarchy afterward with references.
        // We're starting the update fresh to ensure we capture all references(some are lost after mapping)

        var allExistingSteps = existingTestCase.TestStages.SelectMany(x => x.TestSteps).Where(x => x.Id != default).ToList();
        var stepsToAdd = new List<TestStep>();

        void FlattenChildren(TestStep step)
        {
            if (!step.Children?.Any() ?? true)
            {
                return;
            }

            foreach (var child in step.Children)
            {
                if (child.Id == default)
                {
                    continue;
                }

                stepsToAdd.Add(child);

                if (child.Children?.Any() ?? false)
                {
                    FlattenChildren(child);
                }
            }
        }
        allExistingSteps.ForEach(FlattenChildren);
        allExistingSteps.AddRange(stepsToAdd);

        mapper.Map(testCaseDto, existingTestCase);

        List<TestStep> PreserveReferences(List<TestStep> steps, TestingContext dbContext)
        {
            if (!steps?.Any() ?? true)
            {
                return steps;
            }

            var denormalisedSteps = steps
                .GroupJoin(allExistingSteps, l => l.Id, r => r.Id, (l, r) => (Staged: l, Tracked: r.SingleOrDefault()))
                .ToList();

            foreach (var step in denormalisedSteps)
            {
                if (step.Staged.Children?.Any() ?? false)
                {
                    step.Staged.Children = PreserveReferences(step.Staged.Children, dbContext);
                }

                if (step.Tracked != null)
                {
                    var tracked = step.Tracked;

                    mapper.Map(step.Staged, tracked);
                    var stagedIndex = steps.IndexOf(step.Staged);
                    steps.RemoveAt(stagedIndex);
                    steps.Insert(stagedIndex, tracked);
                }
            }

            return steps;
        }
        foreach (var _stage in existingTestCase.TestStages)
        {
            PreserveReferences(_stage.TestSteps, dbContext);
        }

        // At this point, the DTO has been mapped to the entity(being tracked) with the existing references
        dbContext.ChangeTracker.DetectChanges();
        Console.WriteLine(dbContext.Entry(allExistingSteps[1]).State); // Deleted

        dbContext.SaveChanges();

        testStages = dbContext.TestStages.ToArray();
        Console.WriteLine($"{testStages[0].TestSteps.Count} {testStages[0].TestSteps[0].Children.Count()}"); // 1 0
    }

    static IMapper InitMap()
    {
        var config = new MapperConfiguration(cfg =>
        {
            cfg.CreateMap<TestCase, TestCaseDto>();
            cfg.CreateMap<TestCaseDto, TestCase>();

            cfg.CreateMap<TestStage, TestStageDto>();
            cfg.CreateMap<TestStageDto, TestStage>();

            cfg.CreateMap<TestStep, TestStepDto>();
            cfg.CreateMap<TestStepDto, TestStep>();

            cfg.CreateMap<TestStep, TestStep>();

            cfg.AddCollectionMappers();
            cfg.UseEntityFrameworkCoreModel<TestingContext>(services);
        });

        return config.CreateMapper();
    }
}

public class TestingContext : DbContext
{
    public DbSet<TestCase> TestCases { get; set; }
    public DbSet<TestStage> TestStages { get; set; }
    public DbSet<TestStepDto> TestSteps { get; set; }
    public DbSet<StepType> StepTypes { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder options)
    {
        options.UseInMemoryDatabase("test");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<TestCase>().HasMany(x => x.TestStages).WithOne(x => x.TestCase);
        modelBuilder.Entity<TestStage>().HasMany(x => x.TestSteps).WithOne(x => x.TestStage);
        modelBuilder.Entity<TestStep>().HasMany(x => x.Children).WithOne(x => x.Parent);

        base.OnModelCreating(modelBuilder);
    }
}

public class TestCase
{
    public int Id { get; set; }

    public List<TestStage> TestStages { get; set; } = new List<TestStage>();
}
public class TestStage
{
    public int Id { get; set; }

    public int TestCaseId { get; set; }
    public TestCase TestCase { get; set; }

    public List<TestStep> TestSteps { get; set; } = new List<TestStep>();
}
public class TestStep
{
    public int Id { get; set; }

    public int? TestStageId { get; set; }
    public TestStage TestStage { get; set; }

    public int? ParentId { get; set; }
    public TestStep Parent { get; set; }

    public int TypeId { get; set; }
    public StepType Type { get; set; }

    public List<TestStep> Children { get; set; } = new List<TestStep>();
}
public class StepType
{
    public int Id { get; set; }

    public string Name { get; set; }
}


public class TestCaseDto
{
    public int Id { get; set; }

    public List<TestStageDto> TestStages { get; set; }
}
public class TestStageDto
{
    public int Id { get; set; }

    public int TestCaseId { get; set; }

    public List<TestStepDto> TestSteps { get; set; }
}
public class TestStepDto
{
    public int Id { get; set; }

    public int? TestStageId { get; set; }

    public int? ParentId { get; set; }

    public int TypeId { get; set; }

    public List<TestStepDto> Children { get; set; }
}

Output

1 1
Deleted
1 0

Expected

1 1
Modified
2 0

Include provider and version information

EF Core version: 8.0.4
AutoMapper version: 13.0.1
AutoMapper.Collections version: 10.0.0
AutoMapper.Collections.EntityFrameworkCore version: 10.0.0
Target framework: .NET 8.0
Operating system: Win 11
IDE: Visual Studio 2022

[soloham]

I have noticed that if you don't include the Type entity in the query under the comment // Map to entity (done at server side) then it does work as expected, but that doesn't make any sense to me considering TestStep 2 is being referenced by TestStage 1 which is being tracked by EF.

// Map to entity (done at server side)
dbContext.ChangeTracker.Clear(); // Clearing to mimic a new API request scope
var existingTestCase = dbContext.TestCases
    .Include(x => x.TestStages)
        .ThenInclude(x => x.TestSteps)
            .ThenInclude(x => x.Children)
                //.ThenInclude(x => x.Type)    <---- Works as expected after commenting this out!
    .First();

[ajcvickers]

The DbContext is tracking this before the PreserveReferences code is run:

StepType {Id: 1} Unchanged
    Id: 1 PK
    Name: 'Test Type'
TestCase {Id: 1} Unchanged
    Id: 1 PK
  TestStages: [{Id: 1}]
TestStage {Id: 1} Unchanged
    Id: 1 PK
    TestCaseId: 1 FK
  TestCase: {Id: 1}
  TestSteps: [{Id: 1}, <not found>]
TestStep {Id: 1} Unchanged
    Id: 1 PK
    ParentId: <null> FK
    TestStageId: 1 FK
    TypeId: 1 FK
  Children: []
  Parent: <null>
  TestStage: {Id: 1}
  Type: {Id: 1}
TestStep {Id: 2} Unchanged
    Id: 2 PK
    ParentId: 1 FK
    TestStageId: <null> FK
    TypeId: 1 FK
  Children: []
  Parent: {Id: 1}
  TestStage: <null>
  Type: {Id: 1}

After the PreserveReferences code is run, the Type has been set to null:

StepType {Id: 1} Unchanged
    Id: 1 PK
    Name: 'Test Type'
TestCase {Id: 1} Unchanged
    Id: 1 PK
  TestStages: [{Id: 1}]
TestStage {Id: 1} Unchanged
    Id: 1 PK
    TestCaseId: 1 FK
  TestCase: {Id: 1}
  TestSteps: [{Id: 1}, {Id: 2}]
TestStep {Id: 1} Unchanged
    Id: 1 PK
    ParentId: <null> FK
    TestStageId: 1 FK
    TypeId: 1 FK
  Children: []
  Parent: <null>
  TestStage: {Id: 1}
  Type: {Id: 1}
TestStep {Id: 2} Unchanged
    Id: 2 PK
    ParentId: <null> FK Originally 1
    TestStageId: 1 FK Originally <null>
    TypeId: 1 FK
  Children: []
  Parent: <null>
  TestStage: <null>
  Type: <null>

This is detected by EF Core as severing a navigation property--since EF saw it as first non-null, and it was then changed to null. If the Type navigation is not set to null in the PreserveReferences code, then the behavior is as you expect.

[soloham]

That sounds wrong to me, considering the step is still reachable by being linked to TestStage 1 it should not be considered as being Deleted, and there can be many business reasons to not map the navigation entity(it may not exist in the DTO) while it being included in the original query, but we still have the FK mapped which should be enough.

Unless I'm missing something?

[ajcvickers]

@soloham If you never change the navigation property value, then EF will use only the FK value. But if the navigation is changed, then EF will reflect that change.

It can be argued both ways whether this is "correct" but at this point the behavior is not going to change anyway, since it would be a big breaking change.

[soloham]

@ajcvickers I would like to understand the argument for the current behaviour, how would it ever be valid to mark an entity as Deleted if a linked navigation property is removed while keeping the FK as is? There are very real scenarios as I have shared in the issue where a previously loaded entity with navigations included is mapped onto by a DTO that doesn't include that navigation(very valid/common for DTOs)

I understand the point around this being a massive breaking change but could it not be and would request that it be put behind a feature flag similar to let's say ChangeTracker.CascadeDeleteTiming from #10114?

[ajcvickers]

EF supports changing the navigation properties as a way to change relationships. If the relationship is required, and the FK property is non-nullable, then severing a relationship results in that dependent becoming deleted. For example:

using (var context = new SomeDbContext())
{
    await context.Database.EnsureDeletedAsync();
    await context.Database.EnsureCreatedAsync();

    context.Add(new TestStep { Type = new StepType { Name = "S1" } });
    
    await context.SaveChangesAsync();
}

using (var context = new SomeDbContext())
{
    var step = await context.TestSteps.Include(e => e.Type).SingleAsync();

    step.Type = null; // Sever the relationship to the Type.
    
    await context.SaveChangesAsync(); // TestStep cannot exist without a StepType, so it is deleted.
}

public class SomeDbContext : DbContext
{
    public DbSet<TestStep> TestSteps => Set<TestStep>();

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
            .UseSqlServer("Data Source=localhost;Database=BuildBlogs;Integrated Security=True;Trust Server Certificate=True;ConnectRetryCount=0")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();
}

public class TestStep
{
    public int Id { get; set; }

    public int TypeId { get; set; }
    public StepType Type { get; set; }
}

public class StepType
{
    public int Id { get; set; }

    public string Name { get; set; }
}

[soloham]

@ajcvickers Thanks for that. I still don't get the use case for this behaviour. Not sure on what basis reacting to a severance of an undeleted required navigation entity by deleting the dependant would make sense, If however, the navigation entity was deleted thus severing the relationship with the dependant it would make sense for that to cascade to delete the dependent entity.

But if EF knows the navigation entity is undeleted, the dependant is still reachable in the data model, and that it has an FK to the navigation entity it should consider that as Unchanged, or if it has to react to the severance, the more cautious approach would be to throw an exception demanding the required navigation as opposed to assuming on behalf of the user to delete the dependent entity which can lead to unintentional data loss as it has for me in production here.

I really feel this behaviour needs re-considering, and as a minimum, there needs to be a flag to allow the user to control what EF does in such a scenario. Anyhow, with the knowledge of what's going on, I have worked around this for my use case.

If you decide in favour of making this change, I would be interested in contributing to this change, and would be great to get a headstart on where to begin in the codebase. Thanks for looking into this!

[ajcvickers]

The behavior of EF is identical if the relationship is severed from one side or the other, or using the foreign key.

[soloham]

@ajcvickers For the record, I don't think this issue has been given the attention it deserves - there are still unanswered questions from my comments above which clearly outline real-world data loss risks of the current behaviour.

At the very least I expected a justification for the current risky behaviour but sounds like we're saying that's just how it is.

[ajcvickers]

@soloham You have a different opinion about how EF Core should handle modification of relationships. I have discussed this kind of thing with people for 16 years now. There are lots of internally consistent mental models that could be used to reason about what should happen when a relationship is severed. EF has one that was chosen more than 16 years ago (we didn't change it in EF Core), and even if you come up with something that it super compelling, we can't change it now without it being a breaking change. What's more it would be a breaking change that introduced multiple models of how change tracking works, and that's not a good way to go with EF.

[soloham]

@ajcvickers I get that and totally respect there may be different justifiable opinions on how EF should do certain things considering the complex use case it is addressing and I always have and will continue to use EF in my projects due to the value it adds.

However, what I wanted to understand here was the reasoning/justification for the opinion and decision driving the current behaviour and understand how that makes sense in the real world, surely this analysis was done? I haven't seen that and/or any example to suggest why the current behaviour is useful enough to ignore the major risks I have encountered and shared here. What it feels I got was this is how EF works, and it can't change now.

I hope it's ok to challenge these opinions as old as they may be especially if it turns out they have major risks associated with them, breaking change or not.

At the core of what I don't understand is how severing a required navigation entity necessitates the deletion of the dependent given EF knows that the navigation and the dependent entities are not deleted? and how that's better than throwing an FK constraint violation exception?

What's more it would be a breaking change that introduced multiple models of how change tracking works, and that's not a good way to go with EF.

We already have a lot of precedence for configuring how and what the change tracker does in various scenarios, how would this be any different? and in my opinion, is a major selling point of EF

[ajcvickers]

@soloham As I understand it, the "major risks" are that an entity gets put in a Deleted state when you don't think it should. EF does this if a navigation representing a required relationship is severed by setting a navigation to null, removing an entity from a navigation collection, or setting the FK value to null. This is a fundamental behavior in EF's change tracking. The idea that changing a relationship is the same depending on what side it is done from, or if it is done with an FK instead of a navigation, is not going to change.

[soloham]

As I understand it, the "major risks" are that an entity gets put in a Deleted state when you don't think it should.

Correct, I'm considering data loss on production a major risk.

The idea that changing a relationship is the same depending on what side it is done from, or if it is done with an FK instead of a navigation, is not going to change.

I'm not suggesting the behaviour of severing an undeleted required relationship be different depending on how you do it, I'm suggesting what it is currently needs re-considering regardless of where you do it from and that if a required relationship is severed without deleting the navigation entity(thereby not invoking cascade delete on the dependant) and dependant entity is saved, it should be an FK constraint violation(what SQL Server would do) exception as a minimum as opposed to deleting that dependant entity.

EF does this if a navigation representing a required relationship is severed by setting a navigation to null, removing an entity from a navigation collection, or setting the FK value to null.

My question still stands as to why this approach, knowing that it has the risk outlined above, has been adopted(regardless of where from the severance happens)?

[ajcvickers]

EF Core deletes orphaned dependents of required relationships that are identifying or configured for cascade delete. If you don't think that EF should put entities in the Deleted state by convention when they cannot exist anymore, then I would suggest that you don't use EF Core. This is one of the fundamental built in behaviors.

Why does EF Core do this? Because it's by far the most common behavior people want for this scenario. EF6 did not do this, and it was a major pain point having to write code to manually do it.

If you configure the relationship to not cascade delete, then EF won't do this. We don't currently support configuring cascade delete and not configuring deletion of orphans, or vice versa, but that is tracked by #10066.

[soloham]

@ajcvickers Reading up on the issue you have linked, I can now see the argument for the current behaviour i.e when the child is being removed from the navigation collection of the parent explicitly -but since EF treats and identifies parent.Children.Remove(child) and child.Parent = null as the same thing the dependant is also deleted in my scenario which I still don't think is right but can now actually see why it is the way it is.

If you don't think that EF should put entities in the Deleted state by convention when they cannot exist anymore, then I would suggest that you don't use EF Core. This is one of the fundamental built in behaviors.

Well, you say that but the linked issue proves that EF intentionally does not always put entities that can't exist anymore in a deleted state based on configuration and can throw a very helpful exception to explain why:

The association between entity types 'ParentEntity' and 'ChildEntity' has been severed but the foreign key for this relationship cannot be set to null. If the dependent entity should be deleted, then setup the relationship to use cascade deletes.

Which is exactly what I'm asking for here, in my scenario where I'm using automapper it doesn't make sense that EF delete orphaned entities, for others it may be the opposite and I think once you separate out orphan deletion from parent cascade behaviour and have a dedicate configurable orphan deletion behaviour, you will have resolved this issue as well and I would expect an exception like the following:

The association between entity types 'ParentEntity' and 'ChildEntity' has been severed but the foreign key for this relationship cannot be set to null. If the dependent entity should be deleted, then setup the relationship to allow deleting orhpaned dependents.

Considering all this, I think this issue is still valid and should be closed only when #10066 is resolved or this can be consolidated into that issue.

[ajcvickers]

If you want to treat this as a duplicate of #10066, that's fine by me. I don't see anything additional that this issue adds to that.

[soloham]

@ajcvickers The important difference is that this is the inverse of what's being asked for by the OP in that issue, so as long as the output of that results in an orphan delete configuration option that also caters for this scenario, I'm also fine by this being considered a "duplicate" of that.

[soloham]

To be clear regarding the difference...

From #10066,

I think it makes sense that if the foreign key for the relationship cannot be set to null that deletion occurs irrespective of the relationship's OnDelete behavior.

The request as I understand it is to allow deleting orphans regardless of cascade delete behaviour, what this adds to that is for the current behaviour to still exist but only move that away from being dependant on cascade delete configuration to a new orphan delete configuration option. Both issues are configuring the same thing i.e orphan deletion but in very different ways thus making this not simply a duplicate unless this configuration option is added/tracked in that issue as well.