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

Handle added/modified/deleted child entries with single call to TrackChanges or other API #9312

Closed
kayle opened this issue Aug 1, 2017 · 8 comments
Labels
closed-no-further-action The issue is closed and no further action is planned.

Comments

@kayle
Copy link

kayle commented Aug 1, 2017

From what I can see, there's no simple way to detect deleted entities when working with detached data. From searching stackoverflow, it looks like this is a common issue and currently has no built-in solution.

Consider the following code as an example where I want to auto-detect deleted Child entities when calling Update:

    public partial class Parent
    {
        public int Id { get; set; }
        public IList<Child> Children { get; set; } = new List<Child>();
    }

    public partial class Child
    {
        public int Id { get; set; }
        public int ParentId { get; set; }
        public Parent Parent { get; set; }
    }

    public void Update(Parent parent)
    {
        // context.ChangeTracker.TrackGraph(parent, ..., trackMissingEntities: true); 
        // or context.Parents.UpdateGraph(parent).TrackMissing(p => p.Children); 
        context.Parents.Update(parent);
        context.SaveChanges();
    }

I'm not a sql expert, but I believe I want this to add an extra sql command in addition to the code that already runs.

delete from Child where ParentId = @p0 and Id not in (...)

This seems reasonable to me because I'm in a mode where I'm updating the entire object graph. For small graphs, I don't want to bother with fine-grained tracking. Just assume everything that already exists has been modified, and any values that are missing were deleted.

Alternatively, my specific scenario would also be handled by having JSON column support, since my graph is basically a tree. Lots of user profile data is created or modified by a single asp.net core endpoint.

public IActionResult Get(int id) => Json(context.Parents.Include(p=>p.Children).First(p=>p.Id == id));
public IActionResult Update(Parent parent) 
{
    // Can I use one or two lines of code to implement this correctly with existing ef core apis?
    context.SaveChanges();
}
@divega
Copy link
Contributor

divega commented Aug 5, 2017

@kayle TL;DR: fundamentally, a simple graph of objects cannot convey the intent to perform all possible insert, update and delete operations.

The easiest way to handle this is to maintain a list of deleted objects and serialize it alongside the graph.

You may also be able to take advantage of a turn key solution such as Trackable Entities. However I am not sure about the current status of Trackable Entities for EF Core. @tonysneed might be able to comment.

Details:

If you understand the semantics of a specific relationship in your model you may be able to tell unequivocally that if an object exists in the database but it is no longer contained in a graph (i.e. that cannot longer be reached through any of the navigation properties) then it needs to be deleted. You may even expect EF Core (or any other similar library) to be able to infer this automatically, however EF Core doesn't necessarily have the same level of understanding of your model that you do 😄

The canonical case for this is Order - OrderLines: You know that when an OrderLine gets removed from the OrderLines collection of an Order, it needs to be removed from the database.

But let's say that each OrderLine in turn contains a navigation property to a Product. There is where things become interesting: You cannot say that because a Product is no longer reachable thought the edges of the graph it automatically needs to be deleted.

In order to make the right delete decisions based only on reachability, you would need relationships that have stronger semantics than just "has many" or "has one".

Yet another way to describe the same thing is using the DDD idea of an aggregate (which again is a graph, but enriched with stronger semantics). For the example above:

  • Order and OrderLines form an aggregate.
  • Order is the root.
  • OrderLines can only be reached through the root and they cannot exist independently.
  • If an OrderLine gets removed from the OrderLines collection of an Order, then it is safe to assume it needs to be deleted from the database.
  • An OrderLine contains a reference to a Product, but Product does not belong in the Order aggregate. It is its own aggregate instead. When an OrderLine goes away, if the Product it pointed to becomes no longer reachable, it does not mean that the Product needs to be deleted.

You can only make decision based on reachability on the objects that belong in the aggregate you are looking at.

This is one of the scenarios in which in the long term we believe a new feature we are introducing in EF Core 2.0 called "owned types" is going to be able to help: with owned types you can express strong relationships like "owns one" and in the future you will be able to express "owns many", at which point it will be possible to express the shape of aggregates in the model.

@divega divega added the closed-no-further-action The issue is closed and no further action is planned. label Aug 5, 2017
@kayle
Copy link
Author

kayle commented Aug 5, 2017

@divega Thanks for the detailed response. After reading up on it, the DDD aggregate concept is exactly the scenario I encountered.

I saw the new OwnsOne feature in EF Core 2.0, but wasn't aware of anything related to OwnsMany/collections. That could also solve this. What would the canonical sql command be if the feature were implemented?

delete from OrderLine where OrderId = @p0 and Id not in (...)

Or is there a better way to handle this with sql server?

I'm fully with you that EF Core would need extra information to understand what should be deleted. As an experienced C# developer with less sql experience, I'd propose considering an API to mirror the existing IIncludableQueryable apis.

I may be missing some things still, but it seems EF Core already supports 'reading' an aggregate with Include calls. It would be reasonable to me to support 'updating' an aggregate with something similar. The asp.net case could look like this:

public IActionResult Get(int id) => Json(context.Orders.Include(x=>x.OrderLines).First(x=>x.Id == id));
public IActionResult Update(Order order)  
{
    context.Orders.RemoveDeleted(x=>x.OrderLines).Update(order);
    context.SaveChanges(); 
    return Json(order);
}

RemoveDeleted isn't a great name, but I hope you get the idea. EF core would then have knowledge to delete the OrderLines where required. Product would not be included in the graph unless there was also a .RemoveDeleted(x=>x.Product).

@divega
Copy link
Contributor

divega commented Aug 5, 2017

@kayle I don't know what SQL will be generated to delete objects in collections of owned types when we have support for that. At least currently, EF Core never generates set-based delete operations in SQL but it generates individual delete statements for each object.

... I'd propose considering an API to mirror the existing IIncludableQueryable apis.
... it seems EF Core already supports 'reading' an aggregate with Include calls.

You may use Include() to retrieve aggregates, but again when you do that, EF Core knows nothing about any special semantics of the relationships it traverses. In order to be able to reason about aggregates, EF Core would need ownership relationships to be described in the model, rather than in individual queries.

It would be reasonable to me to support 'updating' an aggregate with something similar. The asp.net case could look like this:

But how is this RemoveDeleted() method going to work?

Collections of owned types are in our backlog. In the meanwhile for your scenario, I am going reiterate my recommendation that you serialize a list of objects to delete alongside the graph, or that you resort to Trackable Entities (assuming a version is available for EF Core).

@divega divega closed this as completed Aug 5, 2017
@tonysneed
Copy link

A bit late to the convo, but thought I would chime in to mention that we made an initial pass at Trackable Entities for EF Core 1.x, but then decided to hold off until EF Core 2.0 updated TrackGraph to include access to the referencing entity, which was implemented in #8801. Now that it's in there, we plan to release Trackable Entities as a Net Standard 2.0 library for EF Core 2.0. That will allow us to do things like cascade adds and deletes (see #7308).

@divega
Copy link
Contributor

divega commented Aug 5, 2017

Sounds great @tonysneed. Thanks for the update.

@rwil02
Copy link

rwil02 commented Apr 7, 2019

"An OrderLine contains a reference to a Product, but Product does not belong in the Order aggregate. It is its own aggregate instead. When an OrderLine goes away, if the Product it pointed to becomes no longer reachable, it does not mean that the Product needs to be deleted."

Product wouldn't be deleted for the same reason Order wouldn't be deleted when deleting the last Order Line that refers to it.

Not because of the "aggregate", but because of the defined direction of the relationship.
"Order" is referenced by "Order Line". "Product" is referenced by "Order Line".
Both are therefore "parent" to "Order Line"

@Dunge
Copy link

Dunge commented Aug 23, 2019

Is there any advancements on this since this was last discussed? I see EFCore starting from 2.2 now have a OwnsMany() declaration, but it still doesn't automatically delete items missing from the collection on a call to Update().

Any example of what would be a clean solution to do that? We should have a way to mark every relations that should track deletes in the same way we mark cascade delete.

@ajcvickers
Copy link
Member

@Dunge This issue is closed and not tracking anything. If you are requesting a feature, then please file new issue and include a detailed description of what you are requesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned.
Projects
None yet
Development

No branches or pull requests

6 participants