Generate All Possible Values Other Than PKs When Entities Are Tracked From Detached State

[TheFanatr]

Makes it so a value for __id is created on entities which were in a Detached state but are then tracked in a new state on an EFCore.Cosmos-backed context; DbContext methods used to transition detached entities into another state would not otherwise work, other than Add and AddAsync. This is discussed in #15289.

The initial fix for this was local to EFCore.Cosmos, but was then it was modified and written into EFCore, so that now all non-PK values will be created when detached entities are tracked in another state.

Fixes #15289

[AndriySvyryd]

StateManager is not intended to be overridden by providers. Instead you'd need to modify the core implementation to call the value generator on non-pk properties that don't have a value when attaching.

@ajcvickers Might provide some thoughts

[ajcvickers]

@AndriySvyryd

Instead you'd need to modify the core implementation to call the value generator on non-pk properties that don't have a value when attaching.

Do you think this is something we can start doing always?

[AndriySvyryd]

Do you think this is something we can start doing always?

Yes, it might be a minor breaking change, but I can't come up with a scenario where this would be blocking

[TheFanatr]

StateManager is not intended to be overridden by providers. Instead you'd need to modify the core implementation to call the value generator on non-pk properties that don't have a value when attaching.

Is this behavior of populating the __id property not specific to the Cosmos DB provider? I realize this can be generalized, but would it not be easier to simply handle this scenario, as it is preventing context.Attach from operating correctly in this provider specifically?

[TheFanatr]

Will start working on this anyways.

[AndriySvyryd]

Is this behavior of populating the __id property not specific to the Cosmos DB provider? I realize this can be generalized, but would it not be easier to simply handle this scenario, as it is preventing context.Attach from operating correctly in this provider specifically?

While it's most evident in Cosmos it would also be used for data seeding across all providers.

[TheFanatr]

@AndriySvyryd

call the value generator on non-pk properties that don't have a value when attaching

The logic needs to also happen on update for the Cosmos DB provider, because otherwise it will also cause entity tracking to throw. Also, how to I restrict ValueGenerationManager to only non-PKs without just copying out all the logic and changing it. I came up with a stripped-down version, but I'm sure it misses some edge cases.

foreach (var property in entry.EntityType.GetProperties().Where(property => !property.IsPrimaryKey() && entry.GetCurrentValue(property) is null))
{
    var valueGenerator = property.GetValueGeneratorFactory()(property, entry.EntityType);
    entry.SetStoreGeneratedValue(property, valueGenerator.Next(entry.ToEntityEntry()));
}

Can I just change IValueGenerationManager.Generate to include bool includePKs = true which just conditionally includes or excludes PKs in the value generation process based on that flag being set, then just call the method from the state manager on every change to Unchanged or Modified?

[AndriySvyryd]

Can I just change IValueGenerationManager.Generate to include bool includePKs = true which just conditionally includes or excludes PKs in the value generation process based on that flag being set,

Yes

then just call the method from the state manager on every change to Unchanged or Modified?

Only if changing from Detached

[TheFanatr]

I added bool includePKs = true to GenerateAsync on IValueGenerationManager as well, just for consistency's sake.

[TheFanatr]

Had to make value generation occur even when entities are newly tracked in the Deleted state. Is this an issue?

[TheFanatr]

Do I need to check that newState is not Added here? I do now because EFCore.Cosmos would create a value for __id in this case without this fix, but I'm unsure as to if this exclusion makes sense or is intuitive in terms of behavior for the rest of EF.

[AndriySvyryd]

Do I need to check that newState is not Added here?

You should do it here instead

[TheFanatr]

You should do it here instead

What do you mean? I was just asking whether or not it matters that generation does not occur in the StateChanging method on StateManager when newState is Added. Also what does adding mean in the context of the line you were referencing? Is it meant as in newly attached in any state or as in it will be attached in the Unchanged state after save? The following is what I have come up with based on my understanding.

if (adding || oldState is EntityState.Detached)
{
    StateManager.ValueGenerationManager.Generate(this, includePKs: adding);
}

[TheFanatr]

This seems to work.

[TheFanatr]

I have added a baseline test to make sure that the Add behavior of creating a value for the PK when it is a nullable value with a generator (such as Guid?) still works as intended.

[AndriySvyryd]

@TheFanatr You understood me correctly

[TheFanatr]

Is the adding check redundant?

[AndriySvyryd]

Is the adding check redundant?

No, we've been generating values when changing to Added from states other than Detached, better not to change this behavior

[AndriySvyryd]

Thanks for your contribution!