-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Fix duplicate entity tracking error when re-attaching detached entities with client-generated keys #37054
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 duplicate entity tracking error when re-attaching detached entities with client-generated keys #37054
Changes from all commits
d1efc47
9333720
39ea21f
d30ba01
df7b7dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -384,6 +384,70 @@ public void Can_add_two_aggregates_linked_down_the_tree() | |||||
| Assert.Equal(EntityState.Added, context.Entry(reminders[1]).State); | ||||||
| } | ||||||
|
|
||||||
| [ConditionalFact] | ||||||
| public void Can_reattach_graph_with_client_generated_keys() | ||||||
| { | ||||||
| using var context = new GuidKeyContext(); | ||||||
| var foo = new FooGuid { Name = "Foo1" }; | ||||||
| var bar1 = new BarGuid { Name = "Bar1", Foo = foo }; | ||||||
| var bar2 = new BarGuid { Name = "Bar2", Foo = foo, RelatedBar = bar1 }; | ||||||
|
|
||||||
| var fooEntry = context.Foos.Add(foo); | ||||||
| var bar1Entry = context.Bars.Add(bar1); | ||||||
| var bar2Entry = context.Bars.Add(bar2); | ||||||
|
|
||||||
| fooEntry.State = EntityState.Detached; | ||||||
| bar1Entry.State = EntityState.Detached; | ||||||
| bar2Entry.State = EntityState.Detached; | ||||||
|
|
||||||
| // When detaching entities in Added state, EF Core automatically nulls out navigations | ||||||
| // to maintain consistency (since Added entities don't exist in the database yet). | ||||||
| // This means bar2.RelatedBar is now null and needs to be manually restored. | ||||||
|
|
||||||
| fooEntry.State = EntityState.Added; | ||||||
| fooEntry.DetectChanges(); | ||||||
|
|
||||||
| bar2Entry.State = EntityState.Added; | ||||||
| bar2.RelatedBar = bar1; // Manually restore the navigation | ||||||
| bar2Entry.DetectChanges(); | ||||||
|
|
||||||
| // This should not throw | ||||||
| bar1Entry.State = EntityState.Added; | ||||||
| bar1Entry.DetectChanges(); | ||||||
|
|
||||||
| Assert.Equal(EntityState.Added, context.Entry(foo).State); | ||||||
| Assert.Equal(EntityState.Added, context.Entry(bar1).State); | ||||||
| Assert.Equal(EntityState.Added, context.Entry(bar2).State); | ||||||
| } | ||||||
|
|
||||||
| private class GuidKeyContext : DbContext | ||||||
| { | ||||||
| protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) | ||||||
| => optionsBuilder | ||||||
| .UseInMemoryDatabase(nameof(GuidKeyContext)) | ||||||
| .UseInternalServiceProvider(InMemoryFixture.DefaultServiceProvider); | ||||||
|
|
||||||
| public DbSet<FooGuid> Foos => Set<FooGuid>(); | ||||||
| public DbSet<BarGuid> Bars => Set<BarGuid>(); | ||||||
| } | ||||||
|
|
||||||
| private class FooGuid | ||||||
| { | ||||||
| public Guid Id { get; set; } | ||||||
| public string Name { get; set; } | ||||||
|
||||||
| public string Name { get; set; } | |
| public string Name { get; set; } = string.Empty; |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nullable reference type annotations are missing for reference type properties. According to the coding guidelines, nullable reference types should be used. These properties should either be marked as nullable (e.g., public string? Name { get; set; }) or initialized to prevent potential null reference warnings.
| public BarGuid RelatedBar { get; set; } | |
| public BarGuid? RelatedBar { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The early return at line 216 bypasses the rest of the
OnStateChangingflow, which means the subsequent call toStateManager.StartTracking(this)at line 219 is skipped. However, this also means thatOnStateChanged(line 229) will never be called for this stale entry. This could result in state change events not being fired, change counters not being updated, and potentially other side effects. Consider whetherOnStateChangedor other cleanup should be called before the early return, or if the current entry needs different handling (e.g., should this entry be marked as detached or disposed?).