efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Shouldn't DbContext.Update() result in commands to delete (set all null) an entity's owned type navigation property if it is null?

Open felinepc opened this issue 4 years ago • 2 comments

When the DbContext.Update method is used in a disconnected scenario, I see it having such balancing characteristics:

  1. It does lack some update efficiency because it would generate commands to update all non-navigation fields, regardless if they've actually changed
  2. On the other hand, it requires no separate DB trip to read in an entity first before update. This can be convenient and efficient.

Currently, in a disconnected scenario, when the method encounters related navigation properties that are null, it ignores them, which makes sense because EF Core has no idea about the true state of those entities if they were not previously loaded/tracked.

But I wonder why this behavior also applies to owned types? Unlike traditional related properties, owned types are always loaded by default. There is no ambiguity as in whether the developer intended to lazy/eager load them from the DB.

This means if DbContext.Update encounters an entity with a null owned type, shouldn't it be safe to assume that the item is either deleted, or never existed in the first place, and then generate DB commands to set all of its fields to null (which would be appropriate in either case)?

Asking this because if we consider a typical scenario where owned types are used: Profile and Address

public class Profile
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    
    public Address PhysicalAddress { get; set; } //This is an owned type!! Address has line1/line2 etc...
}

Now imagine we have a Blazor web form to edit this profile. On this form, the user has the option to delete this address completely.

When the user clicks the client-side delete button, our Blazor code simply sets PhysicalAddress = null, which is logical.

Now the entire updated Profile entity is passed to our service layer code:

 public async Task UpdateProfileAsync(Profile profile)
{
    context.Update(profile);

    await context.SaveChangesAsync();
}

Even though this code will update all of the other fields such as names, it will not delete the PhysicalAddress (not setting all of its column fields to null), because the Update method treats this owned type as if it's any other related navigation entity, and ignores it because it's null.

Considering the typical use case for owned types (where it's kind of like syntax sugar to group related fields together in codes, while in database tables they're treated like any other column), and the fact that they're pretty much always loaded by default, does it make more sense for DbContext.Update to treat null owned types as deleted/non-existent, and generate DB commands accordingly?

felinepc avatar Oct 30 '21 20:10 felinepc

@felinepc I think this makes a lot of sense. It's related to #1985. Changing this now would be a breaking change, but we might be able to do something in the future in combination with other items in #1985, while still allowing the current behavior. I will discuss with the team.

ajcvickers avatar Nov 02 '21 11:11 ajcvickers

Note from triage: Added a link to this issue in #1985. Backlogging this for now, but we will consider this behavior as part of better aggregate support in a later release.

ajcvickers avatar Nov 03 '21 13:11 ajcvickers