dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.74k stars 3.18k forks source link

How to make soft delete in cascade with Entity Framework Core including navigation properties? #11240

Closed zinov closed 2 years ago

zinov commented 6 years ago

I can get all the entities marked as IsDeleted = true, applying query filters, where IsDeleted is a field of my entities.

Now, my question is very simple, how to make a soft delete in cascade with Entity Framework Core when I am soft deleting an entity that has navigation properties that I want to mark as IsDeleted too.

I also posted the question here: https://stackoverflow.com/questions/49242151/how-to-make-soft-delete-in-cascade-with-entity-framework-core-including-navigati

ajcvickers commented 6 years ago

@zinov This is not implemented yet--it will likely happen as part of #4025. As a workaround you can process the entities in SaveChanges to find dependents and flag them as deleted. You might want to use the code EF uses internally as a starting point: https://github.com/aspnet/EntityFrameworkCore/blob/0a513ae5df2c846bd6790ae1a0740fa000e02633/src/EFCore/ChangeTracking/Internal/StateManager.cs#L826

embryologist commented 6 years ago

@ajcvickers , understood but how to obtain InternalEntityEntry I know that I have to iterate over each Navigation prop for that entity but I dont know where to start from,

My simples code is below,

private void OnBeforeSaving() {
  foreach(var entry in ChangeTracker.Entries()) {
   switch (entry.State) {
    case EntityState.Added:
     entry.CurrentValues["IsDeleted"] = false;
     // iterate over each nav. prop to performe cascade soft delete = false

     break;

    case EntityState.Deleted:
     entry.State = EntityState.Modified;
     entry.CurrentValues["IsDeleted"] = true;
     // iterate over each nav. prop to performe cascade soft delete = true

     break;
   }
  }
ajcvickers commented 6 years ago

@embryologist Something like this:

private void HandleDependent(EntityEntry entry)
{
    // ...
}

private void ProcessEntities()
{
    foreach (var entry in ChangeTracker.Entries())
    {
        foreach (var navigationEntry in entry.Navigations
            .Where(n => !n.Metadata.IsDependentToPrincipal()))
        {
            if (navigationEntry is CollectionEntry collectionEntry)
            {
                foreach (var dependentEntry in collectionEntry.CurrentValue)
                {
                    HandleDependent(Entry(dependentEntry));
                }
            }
            else
            {
                var dependentEntry = navigationEntry.CurrentValue;
                if (dependentEntry != null)
                {
                    HandleDependent(Entry(dependentEntry));
                }
            }
        }
    }
}
jtbrower commented 4 years ago

@ajcvickers my apologies for commenting on a closed issue, but since the URL to this page seems to be the most relevant search engine hit when trying to find the best approach to implement cascading soft deletes with EFCore, I thought it was the best place to update the .Net audience.

I reviewed the source file StateManager.cs, compared commit 0a513ae to more recent commits including those on branches 5.0 prev 3 and master latest. I focused on the function CascadeDelete and noted the great improvements your team has been making since March 2018. I noticed that as the product has matured, it seems that the logic complexity required to perform cascading hard deletes has increased, maybe as a result of corner cases being detected? Considering the significant changes to the CascadeDelete function you referenced above, along with the fact that the relevant code is considered 'public internal', I am hesitant to use any related functions directly let only copy it into my own projects without having direct feedback from the experts like yourself.

Here I summarize my use case for those out there who may have similar requirements, I am sure this is a common need. (Although I won't involve the complexity I have in my own use case where off-line entities are also a factor).

I am solely responsible for a large application with about 150 green-field assemblies, all NetStandard/Core. Soft Deletes are used on most if not all of the various databases and tables. Auto-Cascades are disabled because I don't want myself or future developers making the mistake of deleting more than they bargained for (exceptions are welcomed here). As this green-field app is progressing towards 'completion' I have been adding more test cases to supplement the ones that have been running for years. Last night I stumbled into a situation that really made me question my custom DbContext base class that I thought was stable.

An existing test case had been assuring soft deletes worked as expected. Grab a row, delete it. Test to assure it doesn't show up on the DbContext it was deleted from and then test to assure it does not show up on a newly created DbContext. Then disable query filters, assure it still exists in the table, reverse the delete process and compare the initial state with current.

Here is the catch that really caught me off-guard. For whatever hair-brained reason I had come up with at the time, the existing test was purposefully using rows that had no related dependents, so this was an obvious point for test supplementation. Once I used records that had dependents, when I soft deleted a row no exception was thrown (no surprise here) and the related entities remained undeleted both virtually and physically. The surprise to me happened when I decided to load those related entities on that DbContext before I called the 'Remove' API method. When those related dependents are loaded on the DbContext before the 'Remove' method is called, an exception is thrown (as I want and expect) indicating that the entity has FK dependents, cascading is disabled and the operation failed as a result. So the shock to me was this lack of deterministic behavior in my own design.

Before I proceed with a design change, I need to fully appreciate why the Remove method throws an exception only when related entities are loaded on that DbContext while does nothing when they are not. I can see where you wouldn't want the Remove method hitting the database to check for related dependents, yet this leads to inconsistency. I ask myself, "should execution outcome be independent of whether data has been loaded into memory or not?" i.e. loaded on the DbContext or not.

I obviously cannot predict whether the caller will have loaded all related entities before they try to delete the primary. If by chance they had loaded those related entities, they will get an exception as we could all hope for, but if not loaded the issue will go undetected. I am not suggesting that a change is required by the EF team, I just think this is something that people (me included) really need to understand.

Am I correct in saying that the 'Remove' method only throws if the related entities are loaded on the context, yet if not loaded on the context then the SaveChanges method would normally throw the exception? If that is true, the point to others reading this is that you will never see the exception on SaveChanges when using soft deletes because you intercept the deletion by changing it to a modify while setting your tombstone to indicate that it has been virtually deleted? (Disclaimer, once I complete current unrelated tasks, I will have to investigate how much of this behavior is caused by my own code.)

I, and I am sure others might hope for some guidance in the context of both EF 3.1 and the planned release that will coincide with .Net 5.0 this fall. Isn't this a common problem or maybe I am wrong about that. There could be a lot of kluged up versions of soft deletes that are impacted by the rapidly changing internal public StateManager.cs. It would be great if we had a csproj example that was kept up to date that could guide us all.

On a final note; this app has been a migration in process since NetCore 1.0 back in December 2016. I cannot say enough positive compliments to show the level of excitement I have for what Microsoft has done and that includes the work from the Entity Framework team. This code is impressive! (Can you give some encouragement to the XAML and Visual Studio team, its the only missing piece IMO :). Jokes aside, keep up the great work.

ajcvickers commented 4 years ago

@zinov EF Core will perform cascade deletes on entities that are tracked. If you want to rely on this behavior being consistent regardless of the database behavior, then all the related entities to be deleted should be loaded first. If the application isn't always loading related entities, then this may require the code that does the deletion to ensure they are loaded.

Beyond this, you can set cascade deletes up in the database which is useful precisely because it doesn't require all related entities to be loaded. However, you are then opting into the database behavior. This clearly isn't useful for soft deletes as you have found. We should update the documentation to make this clearer.

fradsham commented 4 years ago

@ajcvickers stated "all the related entities to be deleted should be loaded first".

I had tried that approach but it led to a performance hit, especially when that record is a root object (or near the root) with many linked child objects. To support SoftDelete in the interim I added a method to my repository class with a SoftDeleteAsync that took in the guid for the parent object and executed an Update on all the child objects, setting the DeletedOn and DeletedBy fields. I used ExecuteSqlCommand method for netcore 2.1 and ExecuteSqlRaw method for netcore3.1. I then allowed EF Core to remove the parent object in the entities being tracked. It was a trade-off to apply the softdelete on all child objects but still maintain some level of performance.

I am really looking for to seeing this addressed in the upcoming changes that were mentioned, so I can clean up that code. Hats off to the team for really doing a wonderful job on ef core.

ajcvickers commented 4 years ago

@fradsham This will likely require #795 to be implemented.

predefinedx commented 5 months ago

Is there any plans to allow us at least to get option to retrieve info about what kind of DeleteBehavior ir set on the navigation?

For now, I have started to implement my own SaveChanges interceptor, to handle SoftDelete. So far I've added [CascadeDeleteable] attribute, to mark child entities, that I want to be marked as deleted on deletion of the parent entity. Next, I started to realize, that I need to address other delete behaviors, but while in interceptor, I cannot retrieve, what DeleteBehavior is set for that navigation. Maybe I'm missing something?

EDIT: It would be easier to achieve, if I had option to use CodeFirst approach, but in my case it is not possible, also, there are too much tables, to start adding other attributes manually, just to know, what delete behavior is expected on navigation.