Identifying shadow FK values are not propagated when tracked as unchanged or modified

[AndriySvyryd]

If the shadow PK is of nullable type
context.ChangeTracker.TrackGraph(principal, e => e.Entry.State = EntityState.Unchanged);
throws
Unable to track an entity of type '...' because primary key property '...' is null

context.Attach(principal) works, but marks the dependent as added

[ajcvickers]

@AndriySvyryd Having trouble fully understanding this. If the key value of the dependent is null, then changing state to Unchanged without setting the key value should throw. Likewise, using Attach will mark the dependent as Added because it didn't have a key value set, assuming the key is generated. If the key is not generated, then it should throw like TrackGraph does.

What am I missing?

[AndriySvyryd]

@ajcvickers The principal has the key set. How can I attach the principal and the dependent as Unchanged?

[ajcvickers]

@AndriySvyryd When attaching, the primary key must be set. Because when you attach, you are saying that this entity already exists in the database, and hence already has a primary key value assigned.

If the primary key is in shadow state, and you disconnected the entity, then you need to carry the primary key with the entity in some way so that it can then be set again on the state entry before the entity is attached.

[ajcvickers]

Changed the behavior for owned types where the state must match that of the owner, and the key values can be propagated said owner. Behavior for non-owned types remains the same.

[AndriySvyryd]

@ajcvickers I don't see why owned types would be different, an owned dependent is not required to exist as there's no way of enforcing it.
Carrying around the key values and setting them feels too cumbersome when dealing with an untracked entity graph. In the common cases the dependents will have the same state as the principal, and if not it's simpler to fix the state once the entities are tracked.

[ajcvickers]

I don't think we can say that an existing principal and an existing dependent is any more or less common than an existing principal and an new dependent.

Also, "an owned dependent is not required to exist as there's no way of enforcing it." I think we should discuss this since it seems to be contrary to the concept of an aggregate. That is, it's okay for some entity in an aggregate to not exist, but I don't think it's necessarily okay to then later add it to the aggregate and have it inserted. But if this is really okay, then I think aggregates have the same problem as non-aggregates and I don't see anyway to make Attach work in a standard way for primary keys in shadow state--we simply have no way of knowing the the dependent is new or not and making either choice will be right or wrong in different circumstances.

@divega?

[AndriySvyryd]

We don't know whether the dependent will be new or not, but we do know that it will have the same key values as the principal.
We can detect the case where the shadow key values haven't been set as opposed to set to the default values.

[ajcvickers]

Agreed that we can tell what the key values should be, but that's not the real issue here. The issue here is what state the dependent should be put into. If we follow what we do everywhere else, then it will always be Added. That's going to be a pretty bad experience. Likewise, if we change what we do everywhere else, then it's going to be a bad experience everywhere else.

[ajcvickers]

I think we really need to decide what it means to have optional entities in an aggregate. @divega?

[AndriySvyryd]

We can either mark them as Added or the same state as the principal, just in this particular case. This would be enough to allow the user to then change the state to what it should be.

[ajcvickers]

@AndriySvyryd For owned types? Yes. The PR I sent out puts them in the same state as the principal. But based on previous comments we still need to understand if this might, at least sometimes, be wrong, since I think this impacts the entire experience of aggregates. @divega?

For non-owned types, I'm 99.999999% certain we will not change the existing behavior.

[AndriySvyryd]

By particular case I mean identifying shadow FK that doesn't have the values set. I don't think we should do this for owned types that have non-shadow keys, but that depends on what we decide wrt aggregates.

But I'm 99.999999% certain that owned types are not aggregates.

[ajcvickers]

Then what are owned types? If I have an owner and an owned entity, then together they form an aggregate.

We can't change the behavior in general for identifying relationships--that would be a massive breaking change.

[AndriySvyryd]

An aggregate has more restrictions, as the one mentioned above. It would be implemented using owned types, but we don't have real aggregates yet.

Yes, if we propagate principal state it could be a breaking change for all code calling Attach on a graph that has identifying shadow FKs. But I find it hard to believe that this is a popular scenario, considering that we throw for shadow PKs found by convention and warn for explicit shadow PKs.

[ajcvickers]

If the state ends up as Added, then we don't need to special case anything--the code as is works fine. If we start saying that entities that have a key in shadow state are going to be Unchanged, but if you move the key to the CLR type then the same code will result in Added, then I think this is extremely unexpected and goes against the general policy of shadow stateness--that properties in shadow state behave the same as properties not in shadow state.

[ajcvickers]

With regard to aggregates, it was my understanding that the OwnsOne was the building block for aggregates support, just limited to certain relationship types for now. If that's not the case, then I think we need to discuss. @divega?

[AndriySvyryd]

Marking them as Added would be fine, as long as this works:

context.ChangeTracker.TrackGraph(principal, e => e.Entry.State = EntityState.Unchanged);

We could treat shadow and non-shadow the same if we add API to mark any property as "Set" or "Not Set", this would also allow the users to save CLR default values to the database.

[divega]

@AndriySvyryd @ajcvickers can't wait chat about this in person 😅 Hopefully the three of us will be able to meet in the office soon.

[ajcvickers]

Thinking of both complex type and aggregate scenarios, I think it would be very unfortunate if this throws an exception rather than doing a correct update:

Customer customer;
using (var context = new MyContext())
{
    customer = context.Customers.First();
}

customer.Address.CellPhone = "555-555-5555";

using (var context = new MyContext())
{
    context.Update(customer);
    context.SaveChanges();
}

Using this model:

public class Customer
{
    public int Id { get; set; }
    public int Name { get; set; }
    public Address Address { get; set; }
}

public class Address
{
   public string Street { get; set; }
   public int Zip { get; set; }
   public string HomePhone { get; set; }
   public string CellPhone { get; set; }
}

This will work with the PR I sent, but not if the dependent is marked Added. And it doesn't involve the app developer having to know anything about setting special state and/or flags.

Interestingly, this will also work, which is pretty nice:

Customer customer;
using (var context = new MyContext())
{
    customer = context.Customers.First();
}

customer.Address = new Address
{
    Street = "1 Microsoft Way",
    Zip = 98052;
};

using (var context = new MyContext())
{
    context.Update(customer);
    context.SaveChanges();
}

[ajcvickers]

@divega Can you take a look at this today? I'm holding off merging the PR until we decide what the behavior should be for owned dependents, but I think we need to make a decision in time to do something for 2.0.