From 937ab57ad04337e53e6408e2ec5c89837aef434d Mon Sep 17 00:00:00 2001 From: AndriySvyryd Date: Wed, 23 Oct 2019 22:01:30 -0700 Subject: [PATCH 1/2] Set Cascade OnDelete for ownerships without conventions to avoid false model diffs. Fixes #18045 --- .../Internal/MigrationsModelDiffer.cs | 22 +--- .../Internal/InternalRelationshipBuilder.cs | 13 ++ .../Internal/MigrationsModelDifferTest.cs | 123 +++++++++++++----- .../Internal/MigrationsModelDifferTestBase.cs | 43 +++--- 4 files changed, 135 insertions(+), 66 deletions(-) diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs index cbc9b7a0bce..c30bd156efd 100644 --- a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs +++ b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs @@ -1639,6 +1639,8 @@ protected virtual void TrackData( var entry = _sourceUpdateAdapter .CreateEntry(sourceSeed, sourceEntityType); + // Mark as added first to generate missing values + // Issue #15289 entry.EntityState = EntityState.Added; entry.EntityState = EntityState.Unchanged; } @@ -1754,21 +1756,11 @@ protected virtual void DiffData( { var (sourceProperty, sourceConverter, targetConverter) = keyPropertiesMap[i]; var sourceValue = sourceEntry.GetCurrentValue(sourceProperty); - if (targetKey.Properties[i].ClrType != sourceProperty.ClrType) - { - if (sourceConverter != null) - { - targetKeyValues[i] = sourceConverter.ConvertToProvider(sourceValue); - } - else - { - targetKeyValues[i] = targetConverter.ConvertFromProvider(sourceValue); - } - } - else - { - targetKeyValues[i] = sourceValue; - } + targetKeyValues[i] = targetKey.Properties[i].ClrType != sourceProperty.ClrType + ? sourceConverter != null + ? sourceConverter.ConvertToProvider(sourceValue) + : targetConverter.ConvertFromProvider(sourceValue) + : sourceValue; } var entry = _targetUpdateAdapter.TryGetEntry(targetKey, targetKeyValues); diff --git a/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs b/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs index 7075bd1cbca..dbf9b91e5b5 100644 --- a/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs @@ -1045,6 +1045,12 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur } newRelationshipBuilder = Metadata.SetIsOwnership(ownership: true, configurationSource)?.Builder; + newRelationshipBuilder = newRelationshipBuilder?.OnDelete(DeleteBehavior.Cascade, ConfigurationSource.Convention); + + if (newRelationshipBuilder == null) + { + return null; + } foreach (var otherOwnership in otherOwnerships) { @@ -1074,6 +1080,12 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur } newRelationshipBuilder = Metadata.SetIsOwnership(ownership: true, configurationSource)?.Builder; + newRelationshipBuilder = newRelationshipBuilder?.OnDelete(DeleteBehavior.Cascade, ConfigurationSource.Convention); + + if (newRelationshipBuilder == null) + { + return null; + } using (var batch = ModelBuilder.Metadata.ConventionDispatcher.DelayConventions()) { @@ -1113,6 +1125,7 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur } newRelationshipBuilder = Metadata.SetIsOwnership(ownership: true, configurationSource)?.Builder; + newRelationshipBuilder = newRelationshipBuilder?.OnDelete(DeleteBehavior.Cascade, ConfigurationSource.Convention); if (newRelationshipBuilder == null && Metadata.PrincipalEntityType.Builder != null diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs index c5428420662..ca81a6601ad 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs @@ -38,6 +38,20 @@ public void Model_differ_does_not_detect_views() [ConditionalFact] public void Model_differ_does_not_detect_views_with_owned_types() + { + Execute( + _ => { }, + target => target.Entity( + x => + { + x.ToView("Orders"); + x.OwnsOne(y => y.Shipping); + }), + upOperations => Assert.Equal(0, upOperations.Count)); + } + + [ConditionalFact] + public void Model_differ_does_not_detect_views_with_weak_types() { Execute( _ => { }, @@ -1678,10 +1692,8 @@ public void Rename_property_and_column() [ConditionalFact] public void Rename_property_and_column_when_snapshot() { - // NB: No conventions to simulate the snapshot. - var sourceModelBuilder = new ModelBuilder(new ConventionSet()); - sourceModelBuilder.Entity( - // ReSharper disable once AssignNullToNotNullAttribute + Execute( + source => source.Entity( typeof(Crab).FullName, x => { @@ -1690,23 +1702,18 @@ public void Rename_property_and_column_when_snapshot() x.Property("CrabId"); x.HasKey("CrabId"); - }); - - var targetModelBuilder = CreateModelBuilder(); - targetModelBuilder.Entity(); - - targetModelBuilder.FinalizeModel(); - - var modelDiffer = RelationalTestHelpers.Instance.CreateContextServices() - .GetRequiredService(); - var operations = modelDiffer.GetDifferences(sourceModelBuilder.Model, targetModelBuilder.Model); - - Assert.Equal(1, operations.Count); + }), + target => target.Entity(), + operations => + { + Assert.Equal(1, operations.Count); - var operation = Assert.IsType(operations[0]); - Assert.Equal("Crab", operation.Table); - Assert.Equal("CrabId", operation.Name); - Assert.Equal("Id", operation.NewName); + var operation = Assert.IsType(operations[0]); + Assert.Equal("Crab", operation.Table); + Assert.Equal("CrabId", operation.Name); + Assert.Equal("Id", operation.NewName); + }, + skipSourceConventions: true); } private class Crab @@ -2561,7 +2568,8 @@ public void Add_primary_key() Assert.Equal("dbo", operation.Schema); Assert.Equal("Puffin", operation.Table); Assert.Equal("PK_Puffin", operation.Name); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -2630,7 +2638,8 @@ public void Alter_primary_key_columns() Assert.Equal("Raven", addOperation.Table); Assert.Equal("PK_Raven", addOperation.Name); Assert.Equal(new[] { "RavenId" }, addOperation.Columns); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -2667,7 +2676,8 @@ public void Alter_primary_key_column_order_with_seed_data() Assert.Equal("Raven", addOperation.Table); Assert.Equal("PK_Raven", addOperation.Name); Assert.Equal(new[] { "RavenId", "Id" }, addOperation.Columns); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -3523,7 +3533,8 @@ public void Alter_sequence_increment_by() Assert.Equal(1, operation.MinValue); Assert.Equal(4, operation.MaxValue); Assert.True(operation.IsCyclic); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -3553,7 +3564,8 @@ public void Alter_sequence_max_value() Assert.Equal(1, operation.MinValue); Assert.Equal(5, operation.MaxValue); Assert.True(operation.IsCyclic); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -3613,7 +3625,8 @@ public void Alter_sequence_cycle() Assert.Equal(1, operation.MinValue); Assert.Equal(4, operation.MaxValue); Assert.False(operation.IsCyclic); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -3649,7 +3662,8 @@ public void Alter_sequence_type() Assert.Equal(1, createOperation.MinValue); Assert.Equal(4, createOperation.MaxValue); Assert.True(createOperation.IsCyclic); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -3677,7 +3691,8 @@ public void Alter_sequence_start() Assert.Equal("dbo", operation.Schema); Assert.Equal("Golf", operation.Name); Assert.Equal(5, operation.StartValue); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -3699,7 +3714,8 @@ public void Restart_altered_sequence() operations => Assert.Collection( operations, o => Assert.IsType(o), - o => Assert.IsType(o))); + o => Assert.IsType(o)), + skipSourceConventions: true); } [ConditionalFact] @@ -5856,7 +5872,8 @@ public void Alter_renamed_sequence() var alterSequenceOperation = Assert.IsType(operations[2]); Assert.Equal("new", alterSequenceOperation.Schema); Assert.Equal("Sequence", alterSequenceOperation.Name); - }); + }, + skipSourceConventions: true); } [ConditionalFact] @@ -7429,6 +7446,7 @@ private class Customer public int Id { get; set; } public Address Mailing { get; set; } + public ICollection Orders { get; set; } } private class Address @@ -7507,7 +7525,9 @@ public void Add_type_with_additional_ownership() { Execute( source => source - .Entity().OwnsOne(y => y.Mailing), + .Entity() + .Ignore(c => c.Orders) + .OwnsOne(y => y.Mailing), target => target .Entity( x => @@ -7515,7 +7535,9 @@ public void Add_type_with_additional_ownership() x.OwnsOne(y => y.Billing); x.OwnsOne(y => y.Shipping); }) - .Entity().OwnsOne(y => y.Mailing), + .Entity() + .Ignore(c => c.Orders) + .OwnsOne(y => y.Mailing), operations => { var operation = Assert.IsType(Assert.Single(operations)); @@ -7567,7 +7589,8 @@ public void Add_type_with_ownership_SeedData() { var m = Assert.IsType(o); Assert.Equal("Order", m.Name); - })); + }), + skipSourceConventions: true); } [ConditionalFact] @@ -7595,6 +7618,40 @@ public void SeedData_type_with_ownership_no_changes() Assert.Empty); } + [ConditionalFact] + public void SeedData_type_with_owned_collection_no_changes() + { + Execute( + common => + { + common.Entity( + c => + { + c.Ignore(x => x.Mailing); + + c.HasKey(x => x.Id); + c.HasData(new Customer { Id = 1 }); + + c.OwnsMany(y => y.Orders, x => + { + x.Ignore(o => o.Billing); + x.Ignore(o => o.Shipping); + + x.WithOwner() + .HasForeignKey("CustomerId"); + + x.HasKey("CustomerId", "Id"); + x.HasData(new { Id = 2, CustomerId = 1 }); + }); + }); + }, + _ => { }, + _ => { }, + Assert.Empty, + Assert.Empty, + skipSourceConventions: true); + } + [ConditionalFact] public void Old_style_ownership_to_new_style() { diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTestBase.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTestBase.cs index 64113abffb1..16ae20f4290 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTestBase.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTestBase.cs @@ -6,11 +6,10 @@ using System.Diagnostics; using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Infrastructure; -using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Metadata.Conventions; using Microsoft.EntityFrameworkCore.Migrations.Operations; using Microsoft.EntityFrameworkCore.Storage; using Microsoft.EntityFrameworkCore.TestUtilities; -using Microsoft.EntityFrameworkCore.TestUtilities.FakeProvider; using Microsoft.EntityFrameworkCore.Update; using Microsoft.EntityFrameworkCore.Update.Internal; using Xunit; @@ -22,23 +21,26 @@ public abstract class MigrationsModelDifferTestBase protected void Execute( Action buildSourceAction, Action buildTargetAction, - Action> assertAction) - => Execute(m => { }, buildSourceAction, buildTargetAction, assertAction, null); + Action> assertAction, + bool skipSourceConventions = false) + => Execute(m => { }, buildSourceAction, buildTargetAction, assertAction, null, skipSourceConventions); protected void Execute( Action buildCommonAction, Action buildSourceAction, Action buildTargetAction, - Action> assertAction) - => Execute(buildCommonAction, buildSourceAction, buildTargetAction, assertAction, null); + Action> assertAction, + bool skipSourceConventions = false) + => Execute(buildCommonAction, buildSourceAction, buildTargetAction, assertAction, null, skipSourceConventions); protected void Execute( Action buildCommonAction, Action buildSourceAction, Action buildTargetAction, Action> assertActionUp, - Action> assertActionDown) - => Execute(buildCommonAction, buildSourceAction, buildTargetAction, assertActionUp, assertActionDown, null); + Action> assertActionDown, + bool skipSourceConventions = false) + => Execute(buildCommonAction, buildSourceAction, buildTargetAction, assertActionUp, assertActionDown, null, skipSourceConventions); protected void Execute( Action buildCommonAction, @@ -46,24 +48,25 @@ protected void Execute( Action buildTargetAction, Action> assertActionUp, Action> assertActionDown, - Action builderOptionsAction) + Action builderOptionsAction, + bool skipSourceConventions = false) { - var sourceModelBuilder = CreateModelBuilder(); + var sourceModelBuilder = CreateModelBuilder(skipSourceConventions); buildCommonAction(sourceModelBuilder); buildSourceAction(sourceModelBuilder); - sourceModelBuilder.FinalizeModel(); + var sourceModel = sourceModelBuilder.FinalizeModel(); var sourceOptionsBuilder = TestHelpers .AddProviderOptions(new DbContextOptionsBuilder()) - .UseModel(sourceModelBuilder.Model) + .UseModel(sourceModel) .EnableSensitiveDataLogging(); - var targetModelBuilder = CreateModelBuilder(); + var targetModelBuilder = CreateModelBuilder(skipConventions: false); buildCommonAction(targetModelBuilder); buildTargetAction(targetModelBuilder); - targetModelBuilder.FinalizeModel(); + var targetModel = targetModelBuilder.FinalizeModel(); var targetOptionsBuilder = TestHelpers .AddProviderOptions(new DbContextOptionsBuilder()) - .UseModel(targetModelBuilder.Model) + .UseModel(targetModel) .EnableSensitiveDataLogging(); if (builderOptionsAction != null) @@ -74,14 +77,14 @@ protected void Execute( var modelDiffer = CreateModelDiffer(targetOptionsBuilder.Options); - var operationsUp = modelDiffer.GetDifferences(sourceModelBuilder.Model, targetModelBuilder.Model); + var operationsUp = modelDiffer.GetDifferences(sourceModel, targetModel); assertActionUp(operationsUp); if (assertActionDown != null) { modelDiffer = CreateModelDiffer(sourceOptionsBuilder.Options); - var operationsDown = modelDiffer.GetDifferences(targetModelBuilder.Model, sourceModelBuilder.Model); + var operationsDown = modelDiffer.GetDifferences(targetModel, sourceModel); assertActionDown(operationsDown); } } @@ -131,7 +134,11 @@ protected static T[][] ToJaggedArray(T[,] twoDimensionalArray, bool firstDime } protected abstract TestHelpers TestHelpers { get; } - protected virtual ModelBuilder CreateModelBuilder() => TestHelpers.CreateConventionBuilder(skipValidation: true); + + protected virtual ModelBuilder CreateModelBuilder(bool skipConventions) + => skipConventions + ? new ModelBuilder(new ConventionSet()) + : TestHelpers.CreateConventionBuilder(skipValidation: true); protected virtual MigrationsModelDiffer CreateModelDiffer(DbContextOptions options) { From 680b887408744f5163c1e09ae4216072665c402b Mon Sep 17 00:00:00 2001 From: AndriySvyryd Date: Thu, 24 Oct 2019 16:37:31 -0700 Subject: [PATCH 2/2] asfasf --- .../Internal/InternalRelationshipBuilder.cs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs b/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs index dbf9b91e5b5..cd04df4ae29 100644 --- a/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalRelationshipBuilder.cs @@ -1024,7 +1024,7 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur if (ownership.Value) { InternalRelationshipBuilder newRelationshipBuilder; - var otherOwnerships = declaringType.GetDeclaredForeignKeys().Where(fk => fk.IsOwnership).ToList(); + var otherOwnership = declaringType.GetDeclaredForeignKeys().SingleOrDefault(fk => fk.IsOwnership); var invertedOwnerships = declaringType.GetDeclaredReferencingForeignKeys() .Where(fk => fk.IsOwnership && fk.DeclaringEntityType.ClrType == Metadata.PrincipalEntityType.ClrType).ToList(); @@ -1039,7 +1039,8 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur Metadata.PrincipalToDependent == null || declaringType.DefiningNavigationName == Metadata.PrincipalToDependent.Name); - if (otherOwnerships.Any(fk => !configurationSource.Overrides(fk.GetConfigurationSource()))) + if (otherOwnership != null + && !configurationSource.Overrides(otherOwnership.GetConfigurationSource())) { return null; } @@ -1052,12 +1053,9 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur return null; } - foreach (var otherOwnership in otherOwnerships) + if (otherOwnership?.Builder != null) { - if (otherOwnership.Builder != null) - { - otherOwnership.DeclaringEntityType.Builder.HasNoRelationship(otherOwnership, configurationSource); - } + otherOwnership.DeclaringEntityType.Builder.HasNoRelationship(otherOwnership, configurationSource); } foreach (var invertedOwnership in invertedOwnerships) @@ -1071,7 +1069,7 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur return newRelationshipBuilder; } - if (otherOwnerships.Count > 0) + if (otherOwnership != null) { if (!Metadata.GetConfigurationSource().Overrides(ConfigurationSource.Explicit) && Metadata.PrincipalEntityType.IsInDefinitionPath(Metadata.DeclaringEntityType.ClrType)) @@ -1097,7 +1095,6 @@ public virtual InternalRelationshipBuilder IsOwnership(bool? ownership, Configur var fk = newRelationshipBuilder.Metadata; fk.DeclaringEntityType.Builder.HasNoRelationship(fk, fk.GetConfigurationSource()); - var otherOwnership = otherOwnerships.Single(); if (otherOwnership.Builder.IsWeakTypeDefinition(configurationSource) == null) { return null;