From 022b0ffc9301196c315e75c36b87ddce8cf34fc7 Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Fri, 14 Aug 2020 13:10:50 -0700 Subject: [PATCH] Propagate original values for shared columns to added dependents in table splitting (#22059) Use public types in OwnedNavigationBuilder constructor Fixes #21465 --- .../Update/ColumnModification.cs | 39 ++++++----- .../Update/ModificationCommand.cs | 66 +++++++++++-------- .../ChangeTracking/Internal/OriginalValues.cs | 1 - .../Metadata/Builders/EntityTypeBuilder.cs | 10 +-- .../Metadata/Builders/EntityTypeBuilder`.cs | 10 +-- .../Builders/OwnedNavigationBuilder.cs | 23 +++---- .../Builders/OwnedNavigationBuilder`.cs | 17 ++--- .../TPTTableSplittingTestBase.cs | 5 ++ .../TableSplittingTestBase.cs | 61 +++++++++++++++++ .../ComplexNavigationsWeakQueryFixtureBase.cs | 6 +- 10 files changed, 147 insertions(+), 91 deletions(-) diff --git a/src/EFCore.Relational/Update/ColumnModification.cs b/src/EFCore.Relational/Update/ColumnModification.cs index ae649d5ac37..74fed80aa5d 100644 --- a/src/EFCore.Relational/Update/ColumnModification.cs +++ b/src/EFCore.Relational/Update/ColumnModification.cs @@ -296,13 +296,13 @@ public ColumnModification( /// The parameter name to use for the current value parameter (), if needed. /// public virtual string ParameterName - => _parameterName ?? (_parameterName = UseCurrentValueParameter ? _generateParameterName() : null); + => _parameterName ??= UseCurrentValueParameter ? _generateParameterName() : null; /// /// The parameter name to use for the original value parameter (), if needed. /// public virtual string OriginalParameterName - => _originalParameterName ?? (_originalParameterName = UseOriginalValueParameter ? _generateParameterName() : null); + => _originalParameterName ??= UseOriginalValueParameter ? _generateParameterName() : null; /// /// The name of the column. @@ -392,25 +392,34 @@ public virtual void AddSharedColumnModification([NotNull] ColumnModification mod if (UseOriginalValueParameter && !StructuralComparisons.StructuralEqualityComparer.Equals(OriginalValue, modification.OriginalValue)) { - if (_sensitiveLoggingEnabled) + if (Entry.EntityState == EntityState.Modified + && modification.Entry.EntityState == EntityState.Added + && modification.Entry.SharedIdentityEntry == null) + { + modification.Entry.SetOriginalValue(modification.Property, OriginalValue); + } + else { + if (_sensitiveLoggingEnabled) + { + throw new InvalidOperationException( + RelationalStrings.ConflictingOriginalRowValuesSensitive( + Entry.EntityType.DisplayName(), + modification.Entry.EntityType.DisplayName(), + Entry.BuildCurrentValuesString(Entry.EntityType.FindPrimaryKey().Properties), + Entry.BuildOriginalValuesString(new[] { Property }), + modification.Entry.BuildOriginalValuesString(new[] { modification.Property }), + "{'" + ColumnName + "'}")); + } + throw new InvalidOperationException( - RelationalStrings.ConflictingOriginalRowValuesSensitive( + RelationalStrings.ConflictingOriginalRowValues( Entry.EntityType.DisplayName(), modification.Entry.EntityType.DisplayName(), - Entry.BuildCurrentValuesString(Entry.EntityType.FindPrimaryKey().Properties), - Entry.BuildOriginalValuesString(new[] { Property }), - modification.Entry.BuildOriginalValuesString(new[] { modification.Property }), + new[] { Property }.Format(), + new[] { modification.Property }.Format(), "{'" + ColumnName + "'}")); } - - throw new InvalidOperationException( - RelationalStrings.ConflictingOriginalRowValues( - Entry.EntityType.DisplayName(), - modification.Entry.EntityType.DisplayName(), - new[] { Property }.Format(), - new[] { modification.Property }.Format(), - "{'" + ColumnName + "'}")); } _sharedColumnModifications.Add(modification); diff --git a/src/EFCore.Relational/Update/ModificationCommand.cs b/src/EFCore.Relational/Update/ModificationCommand.cs index bdf28fe2e9b..1275477c6c7 100644 --- a/src/EFCore.Relational/Update/ModificationCommand.cs +++ b/src/EFCore.Relational/Update/ModificationCommand.cs @@ -6,7 +6,6 @@ using System.Diagnostics; using System.Linq; using JetBrains.Annotations; -using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Internal; @@ -255,12 +254,12 @@ private IReadOnlyList GenerateColumnModifications() var adding = state == EntityState.Added; var updating = state == EntityState.Modified; var columnModifications = new List(); - Dictionary sharedColumnMap = null; + Dictionary sharedTableColumnMap = null; if (_entries.Count > 1 || (_entries.Count == 1 && _entries[0].SharedIdentityEntry != null)) { - sharedColumnMap = new Dictionary(); + sharedTableColumnMap = new Dictionary(); if (_comparer != null) { @@ -269,12 +268,22 @@ private IReadOnlyList GenerateColumnModifications() foreach (var entry in _entries) { + var tableMapping = GetTableMapping(entry.EntityType); + if (tableMapping == null) + { + continue; + } + if (entry.SharedIdentityEntry != null) { - InitializeSharedColumns(entry.SharedIdentityEntry, updating, sharedColumnMap); + var sharedTableMapping = GetTableMapping(entry.SharedIdentityEntry.EntityType); + if (sharedTableMapping != null) + { + InitializeSharedColumns(entry.SharedIdentityEntry, sharedTableMapping, updating, sharedTableColumnMap); + } } - InitializeSharedColumns(entry, updating, sharedColumnMap); + InitializeSharedColumns(entry, tableMapping, updating, sharedTableColumnMap); } } @@ -284,18 +293,7 @@ private IReadOnlyList GenerateColumnModifications() && (entry.EntityState == EntityState.Deleted || entry.EntityState == EntityState.Added); - ITableMappingBase tableMapping = null; - foreach (var mapping in entry.EntityType.GetTableMappings()) - { - var table = ((ITableMappingBase)mapping).Table; - if (table.Name == TableName - && table.Schema == Schema) - { - tableMapping = mapping; - break; - } - } - + var tableMapping = GetTableMapping(entry.EntityType); if (tableMapping == null) { continue; @@ -308,11 +306,9 @@ private IReadOnlyList GenerateColumnModifications() var isKey = property.IsPrimaryKey(); var isCondition = !adding && (isKey || property.IsConcurrencyToken); var readValue = state != EntityState.Deleted && entry.IsStoreGenerated(property); + ColumnValuePropagator columnPropagator = null; - if (sharedColumnMap != null) - { - columnPropagator = sharedColumnMap[column.Name]; - } + sharedTableColumnMap?.TryGetValue(column.Name, out columnPropagator); var writeValue = false; if (!readValue) @@ -350,7 +346,8 @@ private IReadOnlyList GenerateColumnModifications() isCondition, _sensitiveLoggingEnabled); - if (columnPropagator != null) + if (columnPropagator != null + && column.PropertyMappings.Count() != 1) { if (columnPropagator.ColumnModification != null) { @@ -370,16 +367,29 @@ private IReadOnlyList GenerateColumnModifications() return columnModifications; } - private void InitializeSharedColumns(IUpdateEntry entry, bool updating, Dictionary columnMap) + private ITableMappingBase GetTableMapping(IEntityType entityType) { - foreach (var property in entry.EntityType.GetProperties()) + ITableMappingBase tableMapping = null; + foreach (var mapping in entityType.GetTableMappings()) { - var columnName = property.FindColumn(StoreObjectIdentifier.Table(TableName, Schema))?.Name; - if (columnName == null) + var table = ((ITableMappingBase)mapping).Table; + if (table.Name == TableName + && table.Schema == Schema) { - continue; + tableMapping = mapping; + break; } + } + + return tableMapping; + } + private void InitializeSharedColumns( + IUpdateEntry entry, ITableMappingBase tableMapping, bool updating, Dictionary columnMap) + { + foreach (var columnMapping in tableMapping.ColumnMappings) + { + var columnName = columnMapping.Column.Name; if (!columnMap.TryGetValue(columnName, out var columnPropagator)) { columnPropagator = new ColumnValuePropagator(); @@ -388,7 +398,7 @@ private void InitializeSharedColumns(IUpdateEntry entry, bool updating, Dictiona if (updating) { - columnPropagator.RecordValue(property, entry); + columnPropagator.RecordValue(columnMapping.Property, entry); } } } diff --git a/src/EFCore/ChangeTracking/Internal/OriginalValues.cs b/src/EFCore/ChangeTracking/Internal/OriginalValues.cs index 92df05e0d84..c07b4b789b1 100644 --- a/src/EFCore/ChangeTracking/Internal/OriginalValues.cs +++ b/src/EFCore/ChangeTracking/Internal/OriginalValues.cs @@ -24,7 +24,6 @@ public OriginalValues(InternalEntityEntry entry) public object GetValue(InternalEntityEntry entry, IProperty property) { var index = property.GetOriginalValueIndex(); - if (index == -1) { throw new InvalidOperationException( diff --git a/src/EFCore/Metadata/Builders/EntityTypeBuilder.cs b/src/EFCore/Metadata/Builders/EntityTypeBuilder.cs index 676b174200c..c58466e7a02 100644 --- a/src/EFCore/Metadata/Builders/EntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Builders/EntityTypeBuilder.cs @@ -496,10 +496,7 @@ private OwnedNavigationBuilder OwnsOneBuilder(in TypeIdentity ownedType, string relationship.IsUnique(true, ConfigurationSource.Explicit); } - return new OwnedNavigationBuilder( - Builder.Metadata, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// @@ -712,10 +709,7 @@ private OwnedNavigationBuilder OwnsManyBuilder(in TypeIdentity ownedType, string relationship.IsUnique(false, ConfigurationSource.Explicit); } - return new OwnedNavigationBuilder( - Builder.Metadata, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// diff --git a/src/EFCore/Metadata/Builders/EntityTypeBuilder`.cs b/src/EFCore/Metadata/Builders/EntityTypeBuilder`.cs index 04ea567d391..28c1f7a8c94 100644 --- a/src/EFCore/Metadata/Builders/EntityTypeBuilder`.cs +++ b/src/EFCore/Metadata/Builders/EntityTypeBuilder`.cs @@ -698,10 +698,7 @@ private OwnedNavigationBuilder OwnsOneBuilder( - Builder.Metadata, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// @@ -1080,10 +1077,7 @@ private OwnedNavigationBuilder OwnsManyBuilder( - Builder.Metadata, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// diff --git a/src/EFCore/Metadata/Builders/OwnedNavigationBuilder.cs b/src/EFCore/Metadata/Builders/OwnedNavigationBuilder.cs index a2dc993f532..944ae7fead4 100644 --- a/src/EFCore/Metadata/Builders/OwnedNavigationBuilder.cs +++ b/src/EFCore/Metadata/Builders/OwnedNavigationBuilder.cs @@ -26,24 +26,23 @@ public class OwnedNavigationBuilder : IInfrastructure [EntityFrameworkInternal] - public OwnedNavigationBuilder( - [NotNull] EntityType principalEntityType, - [NotNull] EntityType dependentEntityType, - [NotNull] InternalForeignKeyBuilder builder) + public OwnedNavigationBuilder([NotNull] IMutableForeignKey ownership) { - PrincipalEntityType = principalEntityType; - DependentEntityType = dependentEntityType; - _builder = builder; + PrincipalEntityType = (EntityType)ownership.PrincipalEntityType; + DependentEntityType = (EntityType)ownership.DeclaringEntityType; + _builder = ((ForeignKey)ownership).Builder; } /// /// Gets the principal entity type used to configure this relationship. /// + [EntityFrameworkInternal] protected virtual EntityType PrincipalEntityType { get; } /// /// Gets the dependent entity type used to configure this relationship. /// + [EntityFrameworkInternal] protected virtual EntityType DependentEntityType { get; } /// @@ -527,10 +526,7 @@ private OwnedNavigationBuilder OwnsOneBuilder(in TypeIdentity ownedType, string relationship.IsUnique(true, ConfigurationSource.Explicit); } - return new OwnedNavigationBuilder( - DependentEntityType, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// @@ -752,10 +748,7 @@ private OwnedNavigationBuilder OwnsManyBuilder(in TypeIdentity ownedType, string relationship.IsUnique(false, ConfigurationSource.Explicit); } - return new OwnedNavigationBuilder( - DependentEntityType, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// diff --git a/src/EFCore/Metadata/Builders/OwnedNavigationBuilder`.cs b/src/EFCore/Metadata/Builders/OwnedNavigationBuilder`.cs index 83ef07b4e95..d61b139ca4d 100644 --- a/src/EFCore/Metadata/Builders/OwnedNavigationBuilder`.cs +++ b/src/EFCore/Metadata/Builders/OwnedNavigationBuilder`.cs @@ -27,11 +27,8 @@ public class OwnedNavigationBuilder : OwnedNavigation /// doing so can result in application failures when updating to a new Entity Framework Core release. /// [EntityFrameworkInternal] - public OwnedNavigationBuilder( - [NotNull] EntityType principalEntityType, - [NotNull] EntityType dependentEntityType, - [NotNull] InternalForeignKeyBuilder builder) - : base(principalEntityType, dependentEntityType, builder) + public OwnedNavigationBuilder([NotNull] IMutableForeignKey ownership) + : base(ownership) { } @@ -638,10 +635,7 @@ private OwnedNavigationBuilder OwnsOneBui relationship = (InternalForeignKeyBuilder)batch.Run(relationship.Metadata).Builder; } - return new OwnedNavigationBuilder( - relationship.Metadata.PrincipalEntityType, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// @@ -1033,10 +1027,7 @@ private OwnedNavigationBuilder OwnsManyBuil relationship = (InternalForeignKeyBuilder)batch.Run(relationship.Metadata).Builder; } - return new OwnedNavigationBuilder( - DependentEntityType, - relationship.Metadata.DeclaringEntityType, - relationship); + return new OwnedNavigationBuilder(relationship.Metadata); } /// diff --git a/test/EFCore.Relational.Specification.Tests/TPTTableSplittingTestBase.cs b/test/EFCore.Relational.Specification.Tests/TPTTableSplittingTestBase.cs index 2836d412193..ccaadd37d44 100644 --- a/test/EFCore.Relational.Specification.Tests/TPTTableSplittingTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/TPTTableSplittingTestBase.cs @@ -14,6 +14,11 @@ protected TPTTableSplittingTestBase(ITestOutputHelper testOutputHelper) { } + public override void Can_use_optional_dependents_with_shared_concurrency_tokens() + { + // TODO: Issue #22060 + } + protected override string DatabaseName { get; } = "TPTTableSplittingTest"; protected override void OnModelCreating(ModelBuilder modelBuilder) diff --git a/test/EFCore.Relational.Specification.Tests/TableSplittingTestBase.cs b/test/EFCore.Relational.Specification.Tests/TableSplittingTestBase.cs index d4dab4cdc40..314b988e82f 100644 --- a/test/EFCore.Relational.Specification.Tests/TableSplittingTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/TableSplittingTestBase.cs @@ -211,6 +211,67 @@ public virtual void Can_share_required_columns() } } + [ConditionalFact] + public virtual void Can_use_optional_dependents_with_shared_concurrency_tokens() + { + using (CreateTestStore( + modelBuilder => + { + OnModelCreating(modelBuilder); + modelBuilder.Entity( + vb => + { + vb.Property(v => v.SeatingCapacity).HasColumnName("SeatingCapacity").IsConcurrencyToken(); + }); + modelBuilder.Entity( + cb => + { + cb.Property("SeatingCapacity").HasColumnName("SeatingCapacity").IsConcurrencyToken(); + }); + modelBuilder.Entity().HasOne(e => e.FuelTank).WithOne().HasForeignKey(e => e.VehicleName); + modelBuilder.Entity().Ignore(f => f.Engine); + }, seed: false)) + { + using (var context = CreateContext()) + { + var scooterEntry = context.Add( + new PoweredVehicle + { + Name = "Electric scooter", + SeatingCapacity = 1, + Operator = new Operator { Name = "Kai Saunders" } + }); + + context.SaveChanges(); + } + + using (var context = CreateContext()) + { + var scooter = context.Set().Include(v => v.Engine).Single(v => v.Name == "Electric scooter"); + + Assert.Equal(1, scooter.SeatingCapacity); + + scooter.Engine = new Engine(); + + var engineCapacityEntry = context.Entry(scooter.Engine).Property("SeatingCapacity"); + + Assert.Equal(0, engineCapacityEntry.OriginalValue); + + context.SaveChanges(); + + Assert.Equal(0, engineCapacityEntry.OriginalValue); + Assert.Equal(0, engineCapacityEntry.CurrentValue); + } + + using (var context = CreateContext()) + { + var scooter = context.Set().Include(v => v.Engine).Single(v => v.Name == "Electric scooter"); + + Assert.Equal(scooter.SeatingCapacity, context.Entry(scooter.Engine).Property("SeatingCapacity").CurrentValue); + } + } + } + protected void Test_roundtrip(Action onModelCreating) { using (CreateTestStore(onModelCreating)) diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryFixtureBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryFixtureBase.cs index 03046bfa7f0..cea40202ae0 100644 --- a/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryFixtureBase.cs +++ b/test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryFixtureBase.cs @@ -49,7 +49,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con level2Fk = (ForeignKey)batch.Run(level2Fk); } - Configure(new OwnedNavigationBuilder((EntityType)level1, level2Fk.DeclaringEntityType, level2Fk.Builder)); + Configure(new OwnedNavigationBuilder(level2Fk)); modelBuilder.Entity().Property(e => e.Id).ValueGeneratedNever(); modelBuilder.Entity().Property(e => e.Id).ValueGeneratedNever(); @@ -142,7 +142,7 @@ protected virtual void Configure(OwnedNavigationBuilder l2) level3Fk = (ForeignKey)batch.Run(level3Fk); } - Configure(new OwnedNavigationBuilder((EntityType)level2, level3Fk.DeclaringEntityType, level3Fk.Builder)); + Configure(new OwnedNavigationBuilder(level3Fk)); } protected virtual void Configure(OwnedNavigationBuilder l3) @@ -193,7 +193,7 @@ protected virtual void Configure(OwnedNavigationBuilder l3) level4Fk = (ForeignKey)batch.Run(level4Fk); } - Configure(new OwnedNavigationBuilder((EntityType)level3, level4Fk.DeclaringEntityType, level4Fk.Builder)); + Configure(new OwnedNavigationBuilder(level4Fk)); } protected virtual void Configure(OwnedNavigationBuilder l4)