Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow delete fixup and cascade timing to be configured #10114

Closed
ajcvickers opened this issue Oct 18, 2017 · 19 comments · Fixed by #14470
Closed

Allow delete fixup and cascade timing to be configured #10114

ajcvickers opened this issue Oct 18, 2017 · 19 comments · Fixed by #14470
Assignees
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Member

Currently:

  • Cascade delete happens as part of SaveChanges. This prevents cascades happening when all that is intended is a re-parenting.
  • Fixup of non-deleted entities to no longer reference deleted entities happens after SaveChanges. This prevents graph shredding when marking entities as deleted.

However, there are scenarios where it is useful for these things to happen at different times:

  • In a data-binding app, cascade delete should probably happen immediately so that the view of the data correlates to what will happen when SaveChanges is called.
    • Similarly, it can be useful to have the fixup to the deleted entities happen
  • When attempting audit state before or in SaveChanges, it is useful to know what will be deleted beforehand.

There are a few things we could do:

  • Add a "PerformCascades" method to the context. This would run cascade delete code when called.
  • Add a context option to make cascading happen immediately on delete
  • Add similar method/option for delete fixup, or have the cascade method/option do the fixup also.
@nehresma
Copy link

Another use case here: Being able to detect the entities that were removed (possibly by getting a tree of them prior to SaveChanges) so that you can perform an action afterwards. For example, removing referenced objects from the filesystem or S3.

@jonathanford
Copy link

Fixup of non-deleted entities to no longer reference deleted entities happens after SaveChanges. This prevents graph shredding when marking entities as deleted.

Should the same behaviour occur when query filters are applied such as when doing a soft delete? For example in a Blog/Posts scenario, if a Post is soft deleted (using shadow property and query filter), the Post no longer appears in the DbSet, but I still see it in the navigation collection of the Blog.

@johnkwaters
Copy link

My 2 cents - my primary interest is in soft delete. I check which entities have been deleted, and instead set an IsDeleted=true and change them to Modified. It would suffice for me to just get some kind of callback where I can do this logic. Since you said the current logic is in order to do the right thing for reparenting, and prevent graph shredding, I don't necessarily want that just undone in the general case. All I need is some point where I get the REAL state of the objects prior to the DB save...

ajcvickers added a commit that referenced this issue Jan 19, 2019
Fixes #10114

Allows timing cascade delete (i.e. delete of dependent on principal deletion) and delete orphans (i.e. delete of dependent on severing its relationship to the principal) to be configured as:
* Immediate: Dependents changed to Deleted immediately, or at least at the next DetectChanges
* OnSaveChanges: Dependents are not changed to Deleted until SaveChanges is called
* Never: Cascades don't happen automatically. They can be triggered by a new method: `context.ChangeTracker.CascadeChanges()`

The default for both has been changed to `Immediate` which is a breaking change, but likely a better default because:
* Auditing in overridden SaveChanges will now report anything that will be deleted
* Data binding will reflect the changes immediately

However, deleting orphans can be more difficult. This doesn't appear to be a major problem in EF Core because we don't aggressively graph shred, we don't typically have entities that handle their own fixup, and conceptual nulls still exist to allow transitionary states.

Note that EF6 does not support deleting orphans, which means the behavior here with both set to Immediate is not quite the same as the default behavior of EF6.
ajcvickers added a commit that referenced this issue Jan 20, 2019
Fixes #10114

Allows timing cascade delete (i.e. delete of dependent on principal deletion) and delete orphans (i.e. delete of dependent on severing its relationship to the principal) to be configured as:
* Immediate: Dependents changed to Deleted immediately, or at least at the next DetectChanges
* OnSaveChanges: Dependents are not changed to Deleted until SaveChanges is called
* Never: Cascades don't happen automatically. They can be triggered by a new method: `context.ChangeTracker.CascadeChanges()`

The default for both has been changed to `Immediate` which is a breaking change, but likely a better default because:
* Auditing in overridden SaveChanges will now report anything that will be deleted
* Data binding will reflect the changes immediately

However, deleting orphans can be more difficult. This doesn't appear to be a major problem in EF Core because we don't aggressively graph shred, we don't typically have entities that handle their own fixup, and conceptual nulls still exist to allow transitionary states.

Note that EF6 does not support deleting orphans, which means the behavior here with both set to Immediate is not quite the same as the default behavior of EF6.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 20, 2019
ajcvickers added a commit that referenced this issue Feb 1, 2019
Fixes #10114

Allows timing cascade delete (i.e. delete of dependent on principal deletion) and delete orphans (i.e. delete of dependent on severing its relationship to the principal) to be configured as:
* Immediate: Dependents changed to Deleted immediately, or at least at the next DetectChanges
* OnSaveChanges: Dependents are not changed to Deleted until SaveChanges is called
* Never: Cascades don't happen automatically. They can be triggered by a new method: `context.ChangeTracker.CascadeChanges()`

The default for both has been changed to `Immediate` which is a breaking change, but likely a better default because:
* Auditing in overridden SaveChanges will now report anything that will be deleted
* Data binding will reflect the changes immediately

However, deleting orphans can be more difficult. This doesn't appear to be a major problem in EF Core because we don't aggressively graph shred, we don't typically have entities that handle their own fixup, and conceptual nulls still exist to allow transitionary states.

Note that EF6 does not support deleting orphans, which means the behavior here with both set to Immediate is not quite the same as the default behavior of EF6.
ajcvickers added a commit that referenced this issue Feb 3, 2019
Fixes #10114

Allows timing cascade delete (i.e. delete of dependent on principal deletion) and delete orphans (i.e. delete of dependent on severing its relationship to the principal) to be configured as:
* Immediate: Dependents changed to Deleted immediately, or at least at the next DetectChanges
* OnSaveChanges: Dependents are not changed to Deleted until SaveChanges is called
* Never: Cascades don't happen automatically. They can be triggered by a new method: `context.ChangeTracker.CascadeChanges()`

The default for both has been changed to `Immediate` which is a breaking change, but likely a better default because:
* Auditing in overridden SaveChanges will now report anything that will be deleted
* Data binding will reflect the changes immediately

However, deleting orphans can be more difficult. This doesn't appear to be a major problem in EF Core because we don't aggressively graph shred, we don't typically have entities that handle their own fixup, and conceptual nulls still exist to allow transitionary states.

Note that EF6 does not support deleting orphans, which means the behavior here with both set to Immediate is not quite the same as the default behavior of EF6.
ajcvickers added a commit that referenced this issue Feb 3, 2019
Fixes #10114

Allows timing cascade delete (i.e. delete of dependent on principal deletion) and delete orphans (i.e. delete of dependent on severing its relationship to the principal) to be configured as:
* Immediate: Dependents changed to Deleted immediately, or at least at the next DetectChanges
* OnSaveChanges: Dependents are not changed to Deleted until SaveChanges is called
* Never: Cascades don't happen automatically. They can be triggered by a new method: `context.ChangeTracker.CascadeChanges()`

The default for both has been changed to `Immediate` which is a breaking change, but likely a better default because:
* Auditing in overridden SaveChanges will now report anything that will be deleted
* Data binding will reflect the changes immediately

However, deleting orphans can be more difficult. This doesn't appear to be a major problem in EF Core because we don't aggressively graph shred, we don't typically have entities that handle their own fixup, and conceptual nulls still exist to allow transitionary states.

Note that EF6 does not support deleting orphans, which means the behavior here with both set to Immediate is not quite the same as the default behavior of EF6.
@ajcvickers
Copy link
Member Author

Re-opening to ensure breaking change is announced.

@markhobson
Copy link

What was the conclusion with regard to fixing up navigation properties to no longer reference deleted entities before SaveChanges? i.e. This part of the original issue:

Currently:

  • Fixup of non-deleted entities to no longer reference deleted entities happens after SaveChanges

There are a few things we could do:

  • Add similar method/option for delete fixup

As far as I can tell deleted entities are only removed from navigation properties after SaveChanges.

@ajcvickers
Copy link
Member Author

@markhobson That part of the changed proved problematic and very breaking. Cascade of states happens immediately by default, but fixup only of navigations only happens after the entity is really deleted by SaveChanges.

@markhobson
Copy link

Thanks for clarifying.

@deivydas321
Copy link

deivydas321 commented Jun 19, 2020

@ajcvickers I am using EF Core 3.1.
I am deleting a navigation property which is configured by delete behaviour cascade.

Configuration:
builder.HasMany(p => p.Suggestions) .WithOne() .IsRequired() .HasForeignKey(p => p.CreatorUserId) .OnDelete(DeleteBehavior.Restrict);

Delete method:
var principal = await repository.GetWithSuggestion(userId, suggestionId); var suggestion = principal.suggestions.FirstOrDefault(s => s.Id == suggestionId); principal.suggestions.Remove(suggestion); await repository.Save();

However, in save changes I am getting
An error occurred while updating the entries. See the inner exception for details. Cannot insert the value NULL into column 'CreatorUserId', table 'Suggestions'; column does not allow nulls. UPDATE fails.
The statement has been terminated.

And in the SaveChanges I see that Suggestion entity is not Deleted but Modified. I see that it is the same problem as in this issue, could be it an ef core bug?

@ajcvickers
Copy link
Member Author

@deivyd321 .OnDelete(DeleteBehavior.Restrict) means don't cascade delete. Use Cascade.

@deivydas321
Copy link

@ajcvickers thanks for reply.

But I don't want to delete the principal entity. The principal entity contains many suggestions and I just want to delete a single suggestion from the principal entity. I do not want the principal entity to be deleted in any case.

So I think Restrict is ok here and the problem is that in SaveChanges method, Suggestion entity is marked as Modified but not Deleted. As I understand this issue was solved before but I still noticed similar issues with mine.

@TOuhrouche
Copy link

Thanks for all the hard work that went into EF.Core.
We've built all our logic around the issue being mentioned here. That is, when an entity is deleted, it also gets removed from all navigation properties without calling SaveChanges. We heavily rely on this behavior so our bindings update before we call save changes. This way, the user can see the impact of her changes without changing the database.

Is there a way to achieve a similar outcome without calling SaveChanges?

@gaglia
Copy link

gaglia commented Oct 5, 2022

Hello everybody,

we are also in the same situation as @TOuhrouche.
We are migrating from Entity Framework 4.0 and it is crucial for us right now to be able to maintain the behavior described perfectly above:

"When an entity is deleted, it also gets removed from all navigation properties without calling SaveChanges"

So I'm also wondering if there is a way to be able to achieve this behavior?
Because from the tests I'm doing with EF 7 I can't find a solution.

@TOuhrouche did you find a solution in the meantime ?

I appreciate any of your feedback
Thank you

@cjblomqvist
Copy link

That part of the changed proved problematic and very breaking. Cascade of states happens immediately by default, but fixup only of navigations only happens after the entity is really deleted by SaveChanges.

@ajcvickers - could you explain why this was problematic? I'm thinking about doing something about this for our own purposes, but if you already have good reasons why one should not do this then I'd prefer not to waste the time :-)

@ajcvickers
Copy link
Member Author

I'm thinking about doing something about this for our own purposes

What are you thinking of doing?

@cjblomqvist
Copy link

I'm thinking about doing something about this for our own purposes

What are you thinking of doing?

Automatic fixup 😊 Specifically when doing context.Add/remove we're looking into handling navigation properties on "neighboring" entities.

Eg table A and B are "joined" by table C (one-to-many on one side, one-to-one on the other). When a C is added to the context (eg context.Add) we'll automatically add C to navigation properties A.ListOfManyCs (List) and B.MyC. Same when removed, but obviously removed from each navigation property. Same when moved (but less of a use case for us).

@ajcvickers

@ajcvickers
Copy link
Member Author

@cjblomqvist EF Core already does this.

@cjblomqvist
Copy link

cjblomqvist commented Jul 18, 2023

Hmm I see. I interpreted #23940 like it wasn't/won't be (before SaveChanges). I created a repro based on an example in that issue, but maybe I'm missing something/there's something wrong with the code?

(or maybe I was unclear that I meant before SaveChanges)

https://dotnetfiddle.net/J2uzXJ

@ajcvickers

@ajcvickers
Copy link
Member Author

@cjblomqvist For deleted entities, this is tracked by #29486.

@cjblomqvist
Copy link

That part of the changed proved problematic and very breaking. Cascade of states happens immediately by default, but fixup only of navigations only happens after the entity is really deleted by SaveChanges.

@ajcvickers - could you explain why this was problematic? I'm thinking about doing something about this for our own purposes, but if you already have good reasons why one should not do this then I'd prefer not to waste the time :-)

One can find our solution here: #29486 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants