From 14349df8ee0365c27a1b6102630c6ef22337f440 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 25 May 2020 11:41:05 -0700 Subject: [PATCH] Mark owned collections composite key properties use generated values even after the first two usages Fixes #20932 The first time a CLR type is used as an owned collection it is not weak. Value generation is set correctly. The second time, it becomes weak, and value generation is ste correction for both usages. The first time, it is already weak, but the FK is marked as IsOwnersShip after value generation has been set. The fix to run the value generation convention again when FK ownership changes. --- .../Storage/Internal/InMemoryTable.cs | 6 +- .../Update/Internal/CommandBatchPreparer.cs | 9 +- .../Update/Internal/IKeyValueIndexFactory.cs | 9 +- .../Update/Internal/KeyValueIndexFactory.cs | 9 +- .../Update/ModificationCommand.cs | 3 +- .../ProviderConventionSetBuilder.cs | 1 + .../Conventions/ValueGenerationConvention.cs | 17 +++- .../Metadata/Internal/EntityTypeTest.cs | 98 +++++++++++++++++++ 8 files changed, 130 insertions(+), 22 deletions(-) diff --git a/src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs b/src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs index 8b1168bf7ff..efc5bc9a6a5 100644 --- a/src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs +++ b/src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs @@ -8,7 +8,6 @@ using System.Linq; using JetBrains.Annotations; using Microsoft.EntityFrameworkCore.ChangeTracking; -using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.InMemory.Internal; using Microsoft.EntityFrameworkCore.InMemory.ValueGeneration.Internal; using Microsoft.EntityFrameworkCore.Metadata; @@ -26,7 +25,6 @@ namespace Microsoft.EntityFrameworkCore.InMemory.Storage.Internal /// public class InMemoryTable : IInMemoryTable { - // WARNING: The in-memory provider is using EF internal code here. This should not be copied by other providers. See #15096 private readonly IPrincipalKeyValueFactory _keyValueFactory; private readonly bool _sensitiveLoggingEnabled; private readonly Dictionary _rows; @@ -45,7 +43,6 @@ public InMemoryTable([NotNull] IEntityType entityType, [CanBeNull] IInMemoryTabl { EntityType = entityType; BaseTable = baseTable; - // WARNING: The in-memory provider is using EF internal code here. This should not be copied by other providers. See #15096 _keyValueFactory = entityType.FindPrimaryKey().GetPrincipalKeyValueFactory(); _sensitiveLoggingEnabled = sensitiveLoggingEnabled; _rows = new Dictionary(_keyValueFactory.EqualityComparer); @@ -314,9 +311,8 @@ public virtual void BumpValueGenerators(object[] row) } } - // WARNING: The in-memory provider is using EF internal code here. This should not be copied by other providers. See #15096 private TKey CreateKey(IUpdateEntry entry) - => _keyValueFactory.CreateFromCurrentValues((InternalEntityEntry)entry); + => _keyValueFactory.CreateFromCurrentValues(entry); private static object SnapshotValue(IProperty property, ValueComparer comparer, IUpdateEntry entry) { diff --git a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs index dc8784bca6f..67ffc10d907 100644 --- a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs +++ b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs @@ -6,7 +6,6 @@ using System.Linq; using System.Text; using JetBrains.Annotations; -using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Metadata; @@ -491,7 +490,7 @@ private Dictionary> CreateKeyValuePred var principalKeyValue = _keyValueIndexFactorySource .GetKeyValueIndexFactory(foreignKey.PrincipalKey) - .CreatePrincipalKeyValue((InternalEntityEntry)entry, foreignKey); + .CreatePrincipalKeyValue(entry, foreignKey); if (principalKeyValue != null) { @@ -527,7 +526,7 @@ private Dictionary> CreateKeyValuePred var dependentKeyValue = _keyValueIndexFactorySource .GetKeyValueIndexFactory(foreignKey.PrincipalKey) - .CreateDependentKeyValueFromOriginalValues((InternalEntityEntry)entry, foreignKey); + .CreateDependentKeyValueFromOriginalValues(entry, foreignKey); if (dependentKeyValue != null) { @@ -577,7 +576,7 @@ private void AddForeignKeyEdges( var dependentKeyValue = _keyValueIndexFactorySource .GetKeyValueIndexFactory(foreignKey.PrincipalKey) - .CreateDependentKeyValue((InternalEntityEntry)entry, foreignKey); + .CreateDependentKeyValue(entry, foreignKey); if (dependentKeyValue == null) { continue; @@ -605,7 +604,7 @@ private void AddForeignKeyEdges( var principalKeyValue = _keyValueIndexFactorySource .GetKeyValueIndexFactory(foreignKey.PrincipalKey) - .CreatePrincipalKeyValueFromOriginalValues((InternalEntityEntry)entry, foreignKey); + .CreatePrincipalKeyValueFromOriginalValues(entry, foreignKey); if (principalKeyValue != null) { AddMatchingPredecessorEdge( diff --git a/src/EFCore.Relational/Update/Internal/IKeyValueIndexFactory.cs b/src/EFCore.Relational/Update/Internal/IKeyValueIndexFactory.cs index 6575a350317..65259493c70 100644 --- a/src/EFCore.Relational/Update/Internal/IKeyValueIndexFactory.cs +++ b/src/EFCore.Relational/Update/Internal/IKeyValueIndexFactory.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using JetBrains.Annotations; -using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Metadata; namespace Microsoft.EntityFrameworkCore.Update.Internal @@ -21,7 +20,7 @@ public interface IKeyValueIndexFactory /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - IKeyValueIndex CreatePrincipalKeyValue([NotNull] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey); + IKeyValueIndex CreatePrincipalKeyValue([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -29,7 +28,7 @@ public interface IKeyValueIndexFactory /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues([NotNull] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey); + IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -37,7 +36,7 @@ public interface IKeyValueIndexFactory /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - IKeyValueIndex CreateDependentKeyValue([NotNull] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey); + IKeyValueIndex CreateDependentKeyValue([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -45,6 +44,6 @@ public interface IKeyValueIndexFactory /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - IKeyValueIndex CreateDependentKeyValueFromOriginalValues([NotNull] InternalEntityEntry entry, [NotNull] IForeignKey foreignKey); + IKeyValueIndex CreateDependentKeyValueFromOriginalValues([NotNull] IUpdateEntry entry, [NotNull] IForeignKey foreignKey); } } diff --git a/src/EFCore.Relational/Update/Internal/KeyValueIndexFactory.cs b/src/EFCore.Relational/Update/Internal/KeyValueIndexFactory.cs index 3e78ace2517..d5fefa18e15 100644 --- a/src/EFCore.Relational/Update/Internal/KeyValueIndexFactory.cs +++ b/src/EFCore.Relational/Update/Internal/KeyValueIndexFactory.cs @@ -3,7 +3,6 @@ using JetBrains.Annotations; using Microsoft.EntityFrameworkCore.ChangeTracking; -using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Metadata; namespace Microsoft.EntityFrameworkCore.Update.Internal @@ -33,7 +32,7 @@ public KeyValueIndexFactory([NotNull] IPrincipalKeyValueFactory principalK /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public virtual IKeyValueIndex CreatePrincipalKeyValue(InternalEntityEntry entry, IForeignKey foreignKey) + public virtual IKeyValueIndex CreatePrincipalKeyValue(IUpdateEntry entry, IForeignKey foreignKey) => new KeyValueIndex( foreignKey, _principalKeyValueFactory.CreateFromCurrentValues(entry), @@ -46,7 +45,7 @@ public virtual IKeyValueIndex CreatePrincipalKeyValue(InternalEntityEntry entry, /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public virtual IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues(InternalEntityEntry entry, IForeignKey foreignKey) + public virtual IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues(IUpdateEntry entry, IForeignKey foreignKey) => new KeyValueIndex( foreignKey, _principalKeyValueFactory.CreateFromOriginalValues(entry), @@ -59,7 +58,7 @@ public virtual IKeyValueIndex CreatePrincipalKeyValueFromOriginalValues(Internal /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public virtual IKeyValueIndex CreateDependentKeyValue(InternalEntityEntry entry, IForeignKey foreignKey) + public virtual IKeyValueIndex CreateDependentKeyValue(IUpdateEntry entry, IForeignKey foreignKey) => foreignKey.GetDependentKeyValueFactory().TryCreateFromCurrentValues(entry, out var keyValue) ? new KeyValueIndex(foreignKey, keyValue, _principalKeyValueFactory.EqualityComparer, fromOriginalValues: false) : null; @@ -70,7 +69,7 @@ public virtual IKeyValueIndex CreateDependentKeyValue(InternalEntityEntry entry, /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public virtual IKeyValueIndex CreateDependentKeyValueFromOriginalValues(InternalEntityEntry entry, IForeignKey foreignKey) + public virtual IKeyValueIndex CreateDependentKeyValueFromOriginalValues(IUpdateEntry entry, IForeignKey foreignKey) => foreignKey.GetDependentKeyValueFactory().TryCreateFromOriginalValues(entry, out var keyValue) ? new KeyValueIndex(foreignKey, keyValue, _principalKeyValueFactory.EqualityComparer, fromOriginalValues: true) : null; diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs index 9f7bfdcd606..05cc2e8f114 100644 --- a/src/EFCore.Relational/Update/ModificationCommand.cs +++ b/src/EFCore.Relational/Update/ModificationCommand.cs @@ -279,7 +279,7 @@ private IReadOnlyList GenerateColumnModifications() else if ((updating && property.GetAfterSaveBehavior() == PropertySaveBehavior.Save) || (!isKey && nonMainEntry)) { - writeValue = columnPropagator?.TryPropagate(property, (InternalEntityEntry)entry) + writeValue = columnPropagator?.TryPropagate(property, entry) ?? entry.IsModified(property); } } @@ -421,6 +421,7 @@ public bool TryPropagate(IProperty property, IUpdateEntry entry) || (entry.EntityState == EntityState.Modified && !entry.IsModified(property)) || (entry.EntityState == EntityState.Added && Equals(_originalValue, entry.GetCurrentValue(property))))) { + // Should be `entry.SetStoreGeneratedValue(property, _currentValue);` but see issue #21041 ((InternalEntityEntry)entry)[property] = _currentValue; return false; diff --git a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs index b069f775d6e..20ffe6f304b 100644 --- a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs +++ b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs @@ -164,6 +164,7 @@ public virtual ConventionSet CreateConventionSet() conventionSet.ForeignKeyOwnershipChangedConventions.Add(new NavigationEagerLoadingConvention(Dependencies)); conventionSet.ForeignKeyOwnershipChangedConventions.Add(keyDiscoveryConvention); conventionSet.ForeignKeyOwnershipChangedConventions.Add(relationshipDiscoveryConvention); + conventionSet.ForeignKeyOwnershipChangedConventions.Add(valueGeneratorConvention); conventionSet.ModelInitializedConventions.Add(new DbSetFindingConvention(Dependencies)); diff --git a/src/EFCore/Metadata/Conventions/ValueGenerationConvention.cs b/src/EFCore/Metadata/Conventions/ValueGenerationConvention.cs index 5fcc5acf7ee..f24a0977ead 100644 --- a/src/EFCore/Metadata/Conventions/ValueGenerationConvention.cs +++ b/src/EFCore/Metadata/Conventions/ValueGenerationConvention.cs @@ -19,7 +19,8 @@ public class ValueGenerationConvention : IForeignKeyAddedConvention, IForeignKeyRemovedConvention, IForeignKeyPropertiesChangedConvention, - IEntityTypeBaseTypeChangedConvention + IEntityTypeBaseTypeChangedConvention, + IForeignKeyOwnershipChangedConvention { /// /// Creates a new instance of . @@ -205,5 +206,19 @@ private static bool CanBeGenerated(IProperty property) && propertyType != typeof(byte)) || propertyType == typeof(Guid); } + + /// + /// Called after the ownership value for a foreign key is changed. + /// + /// The builder for the foreign key. + /// Additional information associated with convention execution. + public virtual void ProcessForeignKeyOwnershipChanged( + IConventionForeignKeyBuilder relationshipBuilder, IConventionContext context) + { + foreach (var property in relationshipBuilder.Metadata.DeclaringEntityType.GetProperties()) + { + property.Builder.ValueGenerated(GetValueGenerated(property)); + } + } } } diff --git a/test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs b/test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs index e61f494b7cb..d5e6f3571e6 100644 --- a/test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/EntityTypeTest.cs @@ -2266,6 +2266,104 @@ protected internal override void OnModelCreating(ModelBuilder modelBuilder) } } + [ConditionalFact] + public void Indexes_for_owned_collection_types_are_calculated_correctly() + { + using var context = new SideBySide(); + var model = context.Model; + + var parent = model.FindEntityType(typeof(Parent1Entity)); + var indexes = GetIndexes(parent.GetPropertiesAndNavigations()); + Assert.Equal(2, indexes.Count); + // Order: Index, Shadow, Original, StoreGenerated, Relationship + Assert.Equal((0, -1, 0, 0, 0), indexes[nameof(Parent1Entity.Id)]); + Assert.Equal((0, -1, -1, -1, 1), indexes[nameof(Parent1Entity.Children)]); + + indexes = GetIndexes(model.FindEntityType(typeof(ChildEntity), nameof(Parent1Entity.Children), parent).GetProperties()); + Assert.Equal(3, indexes.Count); + // Order: Index, Shadow, Original, StoreGenerated, Relationship + Assert.Equal((0, 0, 0, 0, 0), indexes[nameof(Parent1Entity) + "Id"]); + Assert.Equal((1, 1, 1, 1, 1), indexes["Id"]); + Assert.Equal((2, -1, 2, -1, -1), indexes[nameof(ChildEntity.Name)]); + + parent = model.FindEntityType(typeof(Parent2Entity)); + indexes = GetIndexes(parent.GetPropertiesAndNavigations()); + Assert.Equal(2, indexes.Count); + // Order: Index, Shadow, Original, StoreGenerated, Relationship + Assert.Equal((0, -1, 0, 0, 0), indexes[nameof(Parent2Entity.Id)]); + Assert.Equal((0, -1, -1, -1, 1), indexes[nameof(Parent2Entity.Children)]); + + indexes = GetIndexes(model.FindEntityType(typeof(ChildEntity), nameof(Parent2Entity.Children), parent).GetProperties()); + Assert.Equal(3, indexes.Count); + // Order: Index, Shadow, Original, StoreGenerated, Relationship + Assert.Equal((0, 0, 0, 0, 0), indexes[nameof(Parent2Entity) + "Id"]); + Assert.Equal((1, 1, 1, 1, 1), indexes["Id"]); + Assert.Equal((2, -1, 2, -1, -1), indexes[nameof(ChildEntity.Name)]); + + parent = model.FindEntityType(typeof(Parent3Entity)); + indexes = GetIndexes(parent.GetPropertiesAndNavigations()); + Assert.Equal(2, indexes.Count); + // Order: Index, Shadow, Original, StoreGenerated, Relationship + Assert.Equal((0, -1, 0, 0, 0), indexes[nameof(Parent3Entity.Id)]); + Assert.Equal((0, -1, -1, -1, 1), indexes[nameof(Parent3Entity.Children)]); + + indexes = GetIndexes(model.FindEntityType(typeof(ChildEntity), nameof(Parent3Entity.Children), parent).GetProperties()); + Assert.Equal(3, indexes.Count); + // Order: Index, Shadow, Original, StoreGenerated, Relationship + Assert.Equal((0, 0, 0, 0, 0), indexes[nameof(Parent3Entity) + "Id"]); + Assert.Equal((1, 1, 1, 1, 1), indexes["Id"]); + Assert.Equal((2, -1, 2, -1, -1), indexes[nameof(ChildEntity.Name)]); + + Dictionary GetIndexes(IEnumerable properties) + => properties.ToDictionary( + p => p.Name, + p => + (p.GetIndex(), + p.GetShadowIndex(), + p.GetOriginalValueIndex(), + p.GetStoreGeneratedIndex(), + p.GetRelationshipIndex() + )); + } + + private class SideBySide : DbContext + { + protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + => optionsBuilder + .UseInternalServiceProvider(InMemoryFixture.DefaultServiceProvider) + .UseInMemoryDatabase(Guid.NewGuid().ToString()); + + protected internal override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity().OwnsMany(e => e.Children); + modelBuilder.Entity().OwnsMany(e => e.Children); + modelBuilder.Entity().OwnsMany(e => e.Children); + } + } + + private class Parent1Entity + { + public Guid Id { get; set; } + public ICollection Children { get; set; } + } + + private class Parent2Entity + { + public Guid Id { get; set; } + public ICollection Children { get; set; } + } + + private class Parent3Entity + { + public Guid Id { get; set; } + public ICollection Children { get; set; } + } + + private class ChildEntity + { + public string Name { get; set; } + } + [ConditionalFact] public void Indexes_are_ordered_by_property_count_then_property_names() {