From 2c4876d0a5de173b562f2b69a84bdc6186d72c3e Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Fri, 15 Jan 2021 11:27:25 -0800 Subject: [PATCH] Don't leave unknown FK values when principal is known but has not-set, non-generated, value (#23875) Fixes #23730 --- .../ChangeTracking/Internal/KeyPropagator.cs | 27 ++--- .../ChangeTracking/Internal/OwnedFixupTest.cs | 112 +++++++++++++++++- 2 files changed, 123 insertions(+), 16 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs b/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs index 542c37e1299..92987e57a04 100644 --- a/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs +++ b/src/EFCore/ChangeTracking/Internal/KeyPropagator.cs @@ -53,13 +53,14 @@ public virtual InternalEntityEntry PropagateValue(InternalEntityEntry entry, IPr { Check.DebugAssert(property.IsForeignKey(), $"property {property} is not part of an FK"); - var principalEntry = TryPropagateValue(entry, property); + var generationProperty = property.FindGenerationProperty(); + var principalEntry = TryPropagateValue(entry, property, generationProperty); + if (principalEntry == null && property.IsKey() && !property.IsForeignKeyToSelf()) { - var valueGenerator = TryGetValueGenerator(property); - + var valueGenerator = TryGetValueGenerator(generationProperty); if (valueGenerator != null) { var value = valueGenerator.Next(new EntityEntry(entry)); @@ -93,12 +94,13 @@ public virtual async Task PropagateValueAsync( { Check.DebugAssert(property.IsForeignKey(), $"property {property} is not part of an FK"); - var principalEntry = TryPropagateValue(entry, property); + var generationProperty = property.FindGenerationProperty(); + var principalEntry = TryPropagateValue(entry, property, generationProperty); + if (principalEntry == null && property.IsKey()) { - var valueGenerator = TryGetValueGenerator(property); - + var valueGenerator = TryGetValueGenerator(generationProperty); if (valueGenerator != null) { var value = await valueGenerator.NextAsync(new EntityEntry(entry), cancellationToken) @@ -120,7 +122,7 @@ public virtual async Task PropagateValueAsync( return principalEntry; } - private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, IProperty property) + private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, IProperty property, IProperty generationProperty) { var entityType = entry.EntityType; var stateManager = entry.StateManager; @@ -158,7 +160,8 @@ private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, if (principalProperty != property) { var principalValue = principalEntry[principalProperty]; - if (!principalProperty.ClrType.IsDefaultValue(principalValue)) + if (generationProperty == null + || !principalProperty.ClrType.IsDefaultValue(principalValue)) { if (principalEntry.HasTemporaryValue(principalProperty)) { @@ -182,13 +185,9 @@ private static InternalEntityEntry TryPropagateValue(InternalEntityEntry entry, return null; } - private ValueGenerator TryGetValueGenerator(IProperty property) - { - var generationProperty = property.GetGenerationProperty(); - - return generationProperty != null + private ValueGenerator TryGetValueGenerator(IProperty generationProperty) + => generationProperty != null ? _valueGeneratorSelector.Select(generationProperty, generationProperty.DeclaringEntityType) : null; - } } } diff --git a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs index cbfe5b43024..310ffd4fa21 100644 --- a/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs +++ b/test/EFCore.Tests/ChangeTracking/Internal/OwnedFixupTest.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.ComponentModel.DataAnnotations.Schema; using System.Linq; using System.Threading.Tasks; using Microsoft.EntityFrameworkCore.Diagnostics; @@ -384,7 +385,17 @@ public void Add_principal_with_dependent_principal_nav(EntityState entityState, var subDependentEntry = context.Entry(subDependent); Assert.Equal(principal.Id, subDependentEntry.Property("ParentId").CurrentValue); Assert.Equal(useTrackGraph == null ? EntityState.Added : entityState, subDependentEntry.State); - Assert.Equal(nameof(ChildPN.SubChild), subDependentEntry.Metadata.DefiningNavigationName); + Assert.Equal( + typeof(Parent).ShortDisplayName() + + "." + + nameof(Parent.Child1) + + "#" + + nameof(Child) + + "." + + nameof(Child.SubChild) + + "#" + + nameof(Child.SubChild), + subDependentEntry.Metadata.DisplayName()); }); } @@ -2441,7 +2452,6 @@ public void Parent_swapped_bidirectional(EntityState entityState) Assert.Equal(principal2.Id, dependent2Entry.Property("ParentId").CurrentValue); Assert.Equal(entityState == EntityState.Added ? EntityState.Added : EntityState.Modified, dependent2Entry.State); Assert.Equal(nameof(Parent.Child1), dependent2Entry.Metadata.DefiningNavigationName); - Assert.Same(subDependent1, dependent1.SubChild); Assert.Same(dependent1, subDependent1.Parent); var subDependentEntry1 = dependent1Entry.Reference(p => p.SubChild).TargetEntry; @@ -2845,6 +2855,14 @@ public void Parent_and_identity_changed_bidirectional(EntityState entityState) principal2.Child1 = dependent; principal1.Child2 = null; + if (entityState != EntityState.Added) + { + Assert.Equal( + CoreStrings.KeyReadOnly("ParentId", "Parent.Child2#Child"), + Assert.Throws(() => context.ChangeTracker.DetectChanges()).Message); + return; + } + context.ChangeTracker.DetectChanges(); Assert.True(context.ChangeTracker.HasChanges()); @@ -3043,6 +3061,14 @@ public void Parent_and_identity_changed_bidirectional_collection(EntityState ent principal2.ChildCollection1 = principal1.ChildCollection2; principal1.ChildCollection2 = null; + if (entityState != EntityState.Added) + { + Assert.Equal( + CoreStrings.KeyReadOnly("ParentId", "Parent.ChildCollection2#Child"), + Assert.Throws(() => context.ChangeTracker.DetectChanges()).Message); + return; + } + context.ChangeTracker.DetectChanges(); Assert.True(context.ChangeTracker.HasChanges()); @@ -3212,6 +3238,14 @@ public void Parent_and_identity_swapped_bidirectional(EntityState entityState) principal2.Child1 = dependent1; principal1.Child2 = dependent2; + if (entityState != EntityState.Added) + { + Assert.Equal( + CoreStrings.KeyReadOnly("ParentId", "Parent.Child2#Child"), + Assert.Throws(() => context.ChangeTracker.DetectChanges()).Message); + return; + } + context.ChangeTracker.DetectChanges(); Assert.True(context.ChangeTracker.HasChanges()); @@ -3500,6 +3534,14 @@ public void Parent_and_identity_swapped_bidirectional_collection(EntityState ent .FindEntry(subDependent2); newSubDependentEntry2.Property("Id").CurrentValue = subDependentEntry2.Property("Id").CurrentValue; + if (entityState != EntityState.Added) + { + Assert.Equal( + CoreStrings.KeyReadOnly("ParentId", "Parent.ChildCollection2#Child"), + Assert.Throws(() => context.ChangeTracker.DetectChanges()).Message); + return; + } + context.ChangeTracker.DetectChanges(); Assert.True(context.ChangeTracker.HasChanges()); @@ -4211,6 +4253,72 @@ EntityState GetEntryState(EquatableEntitiesContext context, string role } } + [ConditionalTheory] + [InlineData(false)] + [InlineData(true)] + public async Task SaveChanges_when_owner_has_PK_with_default_values(bool async) + { + using (var context = new OneRowContext(async)) + { + var blog = new Blog { Type = new OwnedType { Value = "A" } }; + + _ = async + ? await context.AddAsync(blog) + : context.Add(blog); + + Assert.Equal(EntityState.Added, context.Entry(blog).State); + Assert.Equal(EntityState.Added, context.Entry(blog.Type).State); + Assert.Equal(0, blog.Id); + Assert.Equal(0, context.Entry(blog.Type).Property("BlogId").CurrentValue); + + _ = async + ? await context.SaveChangesAsync() + : context.SaveChanges(); + + Assert.Equal(EntityState.Unchanged, context.Entry(blog).State); + Assert.Equal(EntityState.Unchanged, context.Entry(blog.Type).State); + Assert.Equal(0, blog.Id); + Assert.Equal(0, context.Entry(blog.Type).Property("BlogId").CurrentValue); + } + + using (var context = new OneRowContext(async)) + { + // Trying to do the same thing again will throw since only one row can have ID zero. + + context.Add(new Blog { Type = new OwnedType { Value = "A" } }); + Assert.Throws(() => context.SaveChanges()); + } + } + + private class OneRowContext : DbContext + { + private readonly bool _async; + + public OneRowContext(bool async) + { + _async = async; + } + + protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + => optionsBuilder.UseInMemoryDatabase(nameof(OneRowContext) + _async); + + public DbSet Blogs { get; set; } + } + + public class Blog + { + [DatabaseGenerated(DatabaseGeneratedOption.None)] + public int Id { get; set; } + + public OwnedType Type { get; set; } + } + + [Owned] + public class OwnedType + { + public string Value { get; set; } + } + private class User { public Guid UserId { get; set; }