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

Fix ValueGenerated On Add Or Update, , make ValueGeneratedOnUpdate() and ValueGeneratedOnAddOrUpdate() working #26713

Closed
wants to merge 6 commits into from

Conversation

eation
Copy link

@eation eation commented Nov 16, 2021

Fixes #6999 and #19765
docs #3165

When useing ValueGeneratedOnUpdate(), set PropertyBuilder.Metadata.SetAfterSaveBehavior(PropertySaveBehavior.Save) for update.

When useing ValueGeneratedOnAddOrUpdate(), set PropertyBuilder.Metadata.SetBeforeSaveBehavior(PropertySaveBehavior.Save) for add and PropertyBuilder.Metadata.SetAfterSaveBehavior(PropertySaveBehavior.Save) for update.

@eation
Copy link
Author

eation commented Nov 17, 2021

@ajcvickers @AndriySvyryd
Can Cosmos modify explicitly or default PK value with a temporary value on state changed?

public async Task Entities_can_be_tracked_with_normal_use_of_DbContext_methods_and_have_correct_resultant_state_and_id_shadow_value()
{
await using var testDatabase = CosmosTestStore.Create("IdentifierShadowValuePresenceTest");
using var context = new IdentifierShadowValuePresenceTestContext(testDatabase);
var item = new Item { Id = 1337 };
var entry = context.Attach(item);
Assert.Equal($"Item|{item.Id}", entry.Property("__id").CurrentValue);
Assert.Equal(EntityState.Unchanged, entry.State);
entry.State = EntityState.Detached;
entry = context.Update(item = new Item { Id = 71 });
Assert.Equal($"Item|{item.Id}", entry.Property("__id").CurrentValue);
Assert.Equal(EntityState.Modified, entry.State);
entry.State = EntityState.Detached;
entry = context.Remove(item = new Item { Id = 33 });
Assert.Equal($"Item|{item.Id}", entry.Property("__id").CurrentValue);
Assert.Equal(EntityState.Deleted, entry.State);
}

Related issues and pull requests

@AndriySvyryd
Copy link
Member

#4073 (and #19135) would need to be fixed first

@ajcvickers ajcvickers self-assigned this Nov 23, 2021
…te action in RelationalDB provider and None-RelationalDB provider, so each provider can has it's owe logic to use ValueGenerator for Generate values. Like RelationalDB,the HasDefaultValueSql() and HasDefaultValue AnnotationNames means property has default value, different from None-RelationalDB.
Comment on lines 669 to 674
if (attachFirst)
{
context.Entry(gu1).State = EntityState.Unchanged;
Assert.NotEqual(default, gu1.Id);
Assert.Equal(EntityState.Unchanged, context.Entry(gu1).State);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong formatting

Comment on lines 581 to 586
[DatabaseGenerated(DatabaseGeneratedOption.None)]
public int Id
{
get => _id;
set => SetWithNotify(value, ref _id);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong formatting

@@ -1883,7 +1915,7 @@ public EntityEntry ToEntityEntry()
break;
case NotifyCollectionChangedAction.Reset:
throw new InvalidOperationException(CoreStrings.ResetNotSupported);
// Note: ignoring Move since index not important
// Note: ignoring Move since index not important

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong formatting?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Saibamen Not sure,but this src file auto-format by vs after last marge

@ajcvickers
Copy link
Member

As @AndriySvyryd mentioned above, making this kind of change is dangerous without more generally updating the handling of modified key properties, as tracked by #4073 and #19135, and possibly other issues. This kind of change would also need significant new testing to make sure it does not result in bad state that cannot be handled by other parts of the stack. For these reasons, this is not a change we plan to merge.

@ajcvickers ajcvickers closed this Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can ValueGenerator be applied in add and update situation?
4 participants