Detect unknown shadow primary key values when attempting to save owned collection

[keatkeat87]

When an entity have owned collection, it can't to use Context.Entry and Attach.
when Attach + update the collection, error come out

Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: 'Database operation expected to affect 1 row(s) but actually affected 0 row(s). Data may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=527962 for information on understanding and handling

when Entry + update the collection, ef only generate insert query, doesn't generate delete query,

for owned type is ok, only owned collection have the problem.

Steps to reproduce

1. git clone https://github.com/keatkeat87/attach-owned-collection-issue.git
2. F5 run

Model.cs

public class StreetAddress
{
    public string Street { get; set; } = "";
    public string City { get; set; } = "";
}

public class Distributor
{
    public int Id { get; set; }
    public string name { get; set; } = "";
    public ICollection<StreetAddress> ShippingCenters { get; set; } = null!;
    public StreetAddress ShippingCenter { get; set; } = null!;
}

public class DistributorConfiguration : IEntityTypeConfiguration<Distributor>
{
    public void Configure(EntityTypeBuilder<Distributor> builder)
    {
        builder.OwnsOne(p => p.ShippingCenter);
        builder.OwnsMany(p => p.ShippingCenters, a =>
        {
            a.WithOwner().HasForeignKey("OwnerId");
            a.Property<int>("Id");
            a.HasKey("Id");
        });
    }
}

Index.cshtml.cs

public void OnGet(
    [FromServices] ApplicationDbContext db
)
{
    // var distributor = db.Distributors.AsTracking().First(); // work perfect.
    var distributor = db.Distributors.AsNoTracking().First();
    //db.Entry(distributor).State = EntityState.Unchanged;  // SaveChanges doesn't generate sql delete query
    db.Attach(distributor); // error when SaveChanges
    distributor.ShippingCenters = new List<StreetAddress> {
        new StreetAddress {
            City = "22",
            Street = "22"
        }
    };
    distributor.ShippingCenter = new StreetAddress
    {
        City = "22",
        Street = "22"
    };
    db.SaveChanges();
}

Further technical details

EF Core version: 3.1.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1.1
Operating system: Windows 10
IDE: Visual Studio 2019 16.4.4

[AngleOSaxon]

I also ran across this the other week. (Same technical details, except EF Core v3.0.1)

To add some hopefully helpful detail, I believe the exact behavior shown is caused by the fact that when Update() attaches and tracks the new entity, it stores the provided values as the Original Values in the snapshot store and then marks all properties as Modified. The IsModified prop for the Owned Type Collection ignores this, because it actually compares the content of the new list against the content of the "original" list and finds no changes. This prevents it from being detected by the change tracking.

It works successfully when the owning entity is loaded AsTracking() because the Original Values are loaded and placed in the snapshot store, and are then available for comparison.

---

I'm not sure what the best general behavior would be for this situation. I can think of only two ways the framework can fulfill the expectation of automatically placing the entity and its owned collection in the supplied state:

• Automatically load the current values for comparison before generating the SQL commands
• Delete all entries for the owned type collection and then insert all entries supplied from the object

Either one seems like it could lead to excessive work at unexpected times.

My current workload works well with loading the current values for comparison, so I have a version of Update() to do that for all collections of owned types, but it would be nice to have an official way to do it.

[ajcvickers]

@keatkeat87 I am able to reproduce this and the analysis from @AngleOSaxon seems correct.

@AndriySvyryd Thoughts on this? The generated updates are shown below. We are incorrectly using a temporary value here. It's possible this is a duplicate--I haven't run on master yet.

info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (1ms) [Parameters=[@p2='1', @p0='22' (Size = 4000), @p1='22' (Size = 4000)], CommandType='Text', CommandTimeout='30']
      SET NOCOUNT ON;
      UPDATE [Distributors] SET [ShippingCenter_City] = @p0, [ShippingCenter_Street] = @p1
      WHERE [Id] = @p2;
      SELECT @@ROWCOUNT;
info: Microsoft.EntityFrameworkCore.Database.Command[20101]
      Executed DbCommand (1ms) [Parameters=[@p0='-2147482646'], CommandType='Text', CommandTimeout='30']
      SET NOCOUNT ON;
      DELETE FROM [Distributors_ShippingCenters]
      WHERE [Id] = @p0;
      SELECT @@ROWCOUNT;

[AndriySvyryd]

Every entity need to have the primary key values set to be able to be tracked. For owned reference we propagate the value from the owner, but for owned collection there's no way to get the correct value, so we end up generating and using a temporary value. Perhaps in this case we should throw early if the value generator generates temporary values.

@keatkeat87 A workaround is to set the StreetAddress.Id to the value in the store

distributor.ShippingCenters = new List<StreetAddress> {
    new StreetAddress {
        City = "22",
        Street = "22"
    }
};
distributor.ShippingCenter = new StreetAddress
{
    City = "22",
    Street = "22"
};

var principalEntry = context.Entry(distributor);

var i = 0;
foreach (var center in distributor.ShippingCenters)
{
    var childEntry = principalEntry.Collection("ShippingCenters").FindEntry(center);
    childEntry.Property("Id").CurrentValue = storeIds[i++];
    childEntry.State = EntityState.Unchanged;
}

[ajcvickers]

Notes: this should already throw a better exception in the 5.0 code. However, we may be able to make it better still. It should not mention or use temporary values, since these are a side effect rather than a root cause.

[coder925]

Thank you for the provided workaround, @AndriySvyryd . But how do you assign the variable storeIds? (i.e. get the real IDs from database)

In my case the collection is reasonably small. I'd be OK if there were an easy workaround where all previous values are always deleted first, and then the new values are added (even if they are the same).

[ajcvickers]

The fundamental issue here is that the key for owned collections is, by default, formed from shadow properties. This means the values of these properties are lost when the entities in the collection are no longer tracked by a DbContext. When the entities are then later attached there is no way to get these values back. (Contrast this with owned non-collection entities, where the shadow key value can be synthesized from the owner key.) This in turn means that there is no way to identify these entities in the database, and there is therefore no way to update or delete them.

The best way to deal with this is to add a CLR property for the key (which can be private) to the owned entity type. The key values are then preserved while the entities are not being tracked, and re-attaching the entities works correctly. We should document this.