Avoid cascade cycles on SQL Server for derived-type referencing many-…

…to-many (#28937)
dotnet · Sep 2, 2022 · eb49719 · eb49719
1 parent 75cee40
commit eb49719
Show file tree

Hide file tree

Showing 36 changed files with 827 additions and 131 deletions.
diff --git a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerOnDeleteConvention.cs b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerOnDeleteConvention.cs
@@ -13,7 +13,9 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
 ///     <see href="https://aka.ms/efcore-docs-sqlserver">Accessing SQL Server and SQL Azure databases with EF Core</see>
 ///     for more information and examples.
 /// </remarks>
-public class SqlServerOnDeleteConvention : CascadeDeleteConvention, ISkipNavigationForeignKeyChangedConvention
+public class SqlServerOnDeleteConvention : CascadeDeleteConvention,
+    ISkipNavigationForeignKeyChangedConvention,
+    IEntityTypeAnnotationChangedConvention
 {
     /// <summary>
     ///     Creates a new instance of <see cref="SqlServerOnDeleteConvention" />.
@@ -60,23 +62,77 @@ protected override DeleteBehavior GetTargetDeleteBehavior(IConventionForeignKey
             return DeleteBehavior.ClientCascade;
         }
 
-        var selfReferencingSkipNavigation = foreignKey.GetReferencingSkipNavigations()
-            .FirstOrDefault(s => s.Inverse != null && s.TargetEntityType == s.DeclaringEntityType);
-        if (selfReferencingSkipNavigation == null)
+        return ProcessSkipNavigations(foreignKey.GetReferencingSkipNavigations()) ?? deleteBehavior;
+    }
+
+    private DeleteBehavior? ProcessSkipNavigations(IEnumerable<IConventionSkipNavigation> skipNavigations)
+    {
+        var skipNavigation = skipNavigations
+            .FirstOrDefault(
+                s => s.Inverse != null
+                    && IsMappedToSameTable(s.DeclaringEntityType, s.TargetEntityType));
+
+        if (skipNavigation != null)
         {
-            return deleteBehavior;
+            var isFirstSkipNavigation = IsFirstSkipNavigation(skipNavigation);
+            if (!isFirstSkipNavigation)
+            {
+                skipNavigation = skipNavigation.Inverse!;
+            }
+
+            var inverseSkipNavigation = skipNavigation.Inverse!;
+
+            var deleteBehavior = DefaultDeleteBehavior(skipNavigation);
+            var inverseDeleteBehavior = DefaultDeleteBehavior(inverseSkipNavigation);
+
+            if (deleteBehavior == DeleteBehavior.Cascade
+                && inverseDeleteBehavior == DeleteBehavior.Cascade
+                && !(inverseSkipNavigation.ForeignKey!.GetDeleteBehaviorConfigurationSource() == ConfigurationSource.Explicit
+                    && inverseSkipNavigation.ForeignKey!.DeleteBehavior != DeleteBehavior.Cascade))
+            {
+                deleteBehavior = DeleteBehavior.ClientCascade;
+            }
+
+            skipNavigation.ForeignKey!.Builder.OnDelete(deleteBehavior);
+            inverseSkipNavigation.ForeignKey!.Builder.OnDelete(inverseDeleteBehavior);
+
+            return isFirstSkipNavigation ? deleteBehavior : inverseDeleteBehavior;
         }
 
-        if (selfReferencingSkipNavigation
-            == selfReferencingSkipNavigation.DeclaringEntityType.GetDeclaredSkipNavigations()
-                .First(s => s == selfReferencingSkipNavigation || s == selfReferencingSkipNavigation.Inverse)
-            && selfReferencingSkipNavigation != selfReferencingSkipNavigation.Inverse)
+        return null;
+
+        DeleteBehavior DefaultDeleteBehavior(IConventionSkipNavigation conventionSkipNavigation)
+            => conventionSkipNavigation.ForeignKey!.IsRequired ? DeleteBehavior.Cascade : DeleteBehavior.ClientSetNull;
+
+        bool IsMappedToSameTable(IConventionEntityType entityType1, IConventionEntityType entityType2)
         {
-            selfReferencingSkipNavigation.Inverse!.ForeignKey?.Builder.OnDelete(
-                GetTargetDeleteBehavior(selfReferencingSkipNavigation.Inverse.ForeignKey));
-            return DeleteBehavior.ClientCascade;
+            var tableName1 = entityType1.GetTableName();
+            var tableName2 = entityType2.GetTableName();
+
+            return tableName1 != null
+                && tableName2 != null
+                && tableName1 == tableName2
+                && entityType1.GetSchema() == entityType2.GetSchema();
         }
 
-        return deleteBehavior;
+        bool IsFirstSkipNavigation(IConventionSkipNavigation navigation)
+            => navigation.DeclaringEntityType != navigation.TargetEntityType
+                ? string.Compare(navigation.DeclaringEntityType.Name, navigation.TargetEntityType.Name, StringComparison.Ordinal) < 0
+                : string.Compare(navigation.Name, navigation.Inverse!.Name, StringComparison.Ordinal) < 0;
+    }
+
+    /// <inheritdoc />
+    public virtual void ProcessEntityTypeAnnotationChanged(
+        IConventionEntityTypeBuilder entityTypeBuilder,
+        string name,
+        IConventionAnnotation? annotation,
+        IConventionAnnotation? oldAnnotation,
+        IConventionContext<IConventionAnnotation> context)
+    {
+        if (name == RelationalAnnotationNames.TableName
+            || name == RelationalAnnotationNames.Schema)
+        {
+            ProcessSkipNavigations(entityTypeBuilder.Metadata.GetDeclaredSkipNavigations());
+        }
     }
 }
diff --git a/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpRuntimeModelCodeGeneratorTest.cs b/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpRuntimeModelCodeGeneratorTest.cs
@@ -1566,7 +1566,7 @@ public static RuntimeForeignKey CreateForeignKey2(RuntimeEntityType declaringEnt
             var runtimeForeignKey = declaringEntityType.AddForeignKey(new[] { declaringEntityType.FindProperty(""PrincipalsId"")!, declaringEntityType.FindProperty(""PrincipalsAlternateId"")! },
                 principalEntityType.FindKey(new[] { principalEntityType.FindProperty(""Id"")!, principalEntityType.FindProperty(""AlternateId"")! })!,
                 principalEntityType,
-                deleteBehavior: DeleteBehavior.Cascade,
+                deleteBehavior: DeleteBehavior.ClientCascade,
                 required: true);
 
             return runtimeForeignKey;

diff --git a/test/EFCore.Relational.Specification.Tests/ManyToManyTrackingRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/ManyToManyTrackingRelationalTestBase.cs
@@ -0,0 +1,64 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
+
+namespace Microsoft.EntityFrameworkCore;
+
+public abstract class ManyToManyTrackingRelationalTestBase<TFixture> : ManyToManyTrackingTestBase<TFixture>
+    where TFixture : ManyToManyTrackingRelationalTestBase<TFixture>.ManyToManyTrackingRelationalFixture
+{
+    protected ManyToManyTrackingRelationalTestBase(TFixture fixture)
+        : base(fixture)
+    {
+    }
+
+    [ConditionalFact]
+    public void Many_to_many_delete_behaviors_are_set()
+    {
+        using var context = CreateContext();
+        var model = context.Model;
+
+        var navigations = model.GetEntityTypes().SelectMany(e => e.GetDeclaredSkipNavigations())
+            .Where(e => e.ForeignKey.DeleteBehavior != DeleteBehavior.Cascade).ToList();
+
+        var builder = new StringBuilder();
+        foreach (var navigation in navigations)
+        {
+            builder.AppendLine($"{{ \"{navigation.DeclaringEntityType.ShortName()}.{navigation.Name}\", DeleteBehavior.ClientCascade }},");
+        }
+
+        var x = builder.ToString();
+
+        foreach (var skipNavigation in model.GetEntityTypes().SelectMany(e => e.GetSkipNavigations()))
+        {
+            Assert.Equal(
+                CustomDeleteBehaviors.TryGetValue(
+                    $"{skipNavigation.DeclaringEntityType.ShortName()}.{skipNavigation.Name}", out var deleteBehavior)
+                    ? deleteBehavior
+                    : DeleteBehavior.Cascade,
+                skipNavigation.ForeignKey.DeleteBehavior);
+        }
+    }
+
+    protected virtual Dictionary<string, DeleteBehavior> CustomDeleteBehaviors { get; } = new();
+
+    protected override void UseTransaction(DatabaseFacade facade, IDbContextTransaction transaction)
+        => facade.UseTransaction(transaction.GetDbTransaction());
+
+    public abstract class ManyToManyTrackingRelationalFixture : ManyToManyTrackingFixtureBase
+    {
+        protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
+        {
+            base.OnModelCreating(modelBuilder, context);
+
+            modelBuilder.Entity<EntityTableSharing1>().ToTable("TableSharing");
+            modelBuilder.Entity<EntityTableSharing2>(
+                b =>
+                {
+                    b.HasOne<EntityTableSharing1>().WithOne().HasForeignKey<EntityTableSharing2>(e => e.Id);
+                    b.ToTable("TableSharing");
+                });
+        }
+    }
+}
diff --git a/test/EFCore.Relational.Specification.Tests/Query/ManyToManyQueryRelationalFixture.cs b/test/EFCore.Relational.Specification.Tests/Query/ManyToManyQueryRelationalFixture.cs
@@ -1,10 +1,25 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
+
 namespace Microsoft.EntityFrameworkCore.Query;
 
 public abstract class ManyToManyQueryRelationalFixture : ManyToManyQueryFixtureBase
 {
     public TestSqlLoggerFactory TestSqlLoggerFactory
         => (TestSqlLoggerFactory)ListLoggerFactory;
+
+    protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
+    {
+        base.OnModelCreating(modelBuilder, context);
+
+        modelBuilder.Entity<EntityTableSharing1>().ToTable("TableSharing");
+        modelBuilder.Entity<EntityTableSharing2>(
+            b =>
+            {
+                b.HasOne<EntityTableSharing1>().WithOne().HasForeignKey<EntityTableSharing2>(e => e.Id);
+                b.ToTable("TableSharing");
+            });
+    }
 }
diff --git a/test/EFCore.Relational.Specification.Tests/Query/TPCManyToManyQueryRelationalFixture.cs b/test/EFCore.Relational.Specification.Tests/Query/TPCManyToManyQueryRelationalFixture.cs
@@ -23,6 +23,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
         modelBuilder.Entity<EntityRoot>().ToTable("Roots");
         modelBuilder.Entity<EntityBranch>().ToTable("Branches");
         modelBuilder.Entity<EntityLeaf>().ToTable("Leaves");
+        modelBuilder.Entity<EntityBranch2>().ToTable("Branch2s");
+        modelBuilder.Entity<EntityLeaf2>().ToTable("Leaf2s");
 
         modelBuilder.Entity<UnidirectionalEntityRoot>().UseTpcMappingStrategy();
         modelBuilder.Entity<UnidirectionalEntityRoot>().ToTable("UnidirectionalRoots");

diff --git a/test/EFCore.Relational.Specification.Tests/Query/TPTManyToManyQueryRelationalFixture.cs b/test/EFCore.Relational.Specification.Tests/Query/TPTManyToManyQueryRelationalFixture.cs
@@ -17,6 +17,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
         modelBuilder.Entity<EntityRoot>().ToTable("Roots");
         modelBuilder.Entity<EntityBranch>().ToTable("Branches");
         modelBuilder.Entity<EntityLeaf>().ToTable("Leaves");
+        modelBuilder.Entity<EntityBranch2>().ToTable("Branch2s");
+        modelBuilder.Entity<EntityLeaf2>().ToTable("Leaf2s");
 
         modelBuilder.Entity<UnidirectionalEntityRoot>().ToTable("UnidirectionalRoots");
         modelBuilder.Entity<UnidirectionalEntityBranch>().ToTable("UnidirectionalBranches");

diff --git a/test/EFCore.Specification.Tests/Query/ManyToManyFieldsQueryFixtureBase.cs b/test/EFCore.Specification.Tests/Query/ManyToManyFieldsQueryFixtureBase.cs
@@ -269,6 +269,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
             .HasMany(e => e.TwoSkipShared)
             .WithMany(e => e.OneSkipShared);
 
+        modelBuilder.Entity<EntityRoot>()
+            .HasMany(e => e.BranchSkipShared)
+            .WithMany(e => e.RootSkipShared);
+
         // Nav:2 Payload:No Join:Concrete Extra:None
         modelBuilder.Entity<EntityOne>()
             .HasMany(e => e.TwoSkip)

diff --git a/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs b/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs
@@ -40,6 +40,10 @@ public ISetSource GetExpectedData()
         { typeof(EntityRoot), e => ((EntityRoot)e)?.Id },
         { typeof(EntityBranch), e => ((EntityBranch)e)?.Id },
         { typeof(EntityLeaf), e => ((EntityLeaf)e)?.Id },
+        { typeof(EntityBranch2), e => ((EntityBranch2)e)?.Id },
+        { typeof(EntityLeaf2), e => ((EntityLeaf2)e)?.Id },
+        { typeof(EntityTableSharing1), e => ((EntityTableSharing1)e)?.Id },
+        { typeof(EntityTableSharing2), e => ((EntityTableSharing2)e)?.Id },
         { typeof(UnidirectionalEntityOne), e => ((UnidirectionalEntityOne)e)?.Id },
         { typeof(UnidirectionalEntityTwo), e => ((UnidirectionalEntityTwo)e)?.Id },
         { typeof(UnidirectionalEntityThree), e => ((UnidirectionalEntityThree)e)?.Id },
@@ -165,6 +169,69 @@ public ISetSource GetExpectedData()
                 }
             }
         },
+        {
+            typeof(EntityBranch2), (e, a) =>
+            {
+                Assert.Equal(e == null, a == null);
+
+                if (a != null)
+                {
+                    var ee = (EntityBranch2)e;
+                    var aa = (EntityBranch2)a;
+
+                    Assert.Equal(ee.Id, aa.Id);
+                    Assert.Equal(ee.Name, aa.Name);
+                    Assert.Equal(ee.Slumber, aa.Slumber);
+                }
+            }
+        },
+        {
+            typeof(EntityLeaf2), (e, a) =>
+            {
+                Assert.Equal(e == null, a == null);
+
+                if (a != null)
+                {
+                    var ee = (EntityLeaf2)e;
+                    var aa = (EntityLeaf2)a;
+
+                    Assert.Equal(ee.Id, aa.Id);
+                    Assert.Equal(ee.Name, aa.Name);
+                    Assert.Equal(ee.Slumber, aa.Slumber);
+                    Assert.Equal(ee.IsBrown, aa.IsBrown);
+                }
+            }
+        },
+        {
+            typeof(EntityTableSharing1), (e, a) =>
+            {
+                Assert.Equal(e == null, a == null);
+
+                if (a != null)
+                {
+                    var ee = (EntityTableSharing1)e;
+                    var aa = (EntityTableSharing1)a;
+
+                    Assert.Equal(ee.Id, aa.Id);
+                    Assert.Equal(ee.Name, aa.Name);
+                }
+            }
+        },
+        {
+            typeof(EntityTableSharing2), (e, a) =>
+            {
+                Assert.Equal(e == null, a == null);
+
+                if (a != null)
+                {
+                    var ee = (EntityTableSharing2)e;
+                    var aa = (EntityTableSharing2)a;
+
+                    Assert.Equal(ee.Id, aa.Id);
+                    Assert.Equal(ee.Cucumber, aa.Cucumber);
+                }
+            }
+        },
         {
             typeof(UnidirectionalEntityOne), (e, a) =>
             {
@@ -292,6 +359,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
         modelBuilder.Entity<EntityRoot>().Property(e => e.Id).ValueGeneratedNever();
         modelBuilder.Entity<EntityBranch>().HasBaseType<EntityRoot>();
         modelBuilder.Entity<EntityLeaf>().HasBaseType<EntityBranch>();
+        modelBuilder.Entity<EntityBranch2>().HasBaseType<EntityRoot>();
+        modelBuilder.Entity<EntityLeaf2>().HasBaseType<EntityBranch2>();
+        modelBuilder.Entity<EntityTableSharing1>().Property(e => e.Id).ValueGeneratedNever();
+        modelBuilder.Entity<EntityTableSharing2>().Property(e => e.Id).ValueGeneratedNever();
 
         modelBuilder.Entity<UnidirectionalEntityOne>().Property(e => e.Id).ValueGeneratedNever();
         modelBuilder.Entity<UnidirectionalEntityTwo>().Property(e => e.Id).ValueGeneratedNever();
@@ -321,6 +392,22 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
             .HasMany(e => e.TwoSkipShared)
             .WithMany(e => e.OneSkipShared);
 
+        modelBuilder.Entity<EntityRoot>()
+            .HasMany(e => e.BranchSkipShared)
+            .WithMany(e => e.RootSkipShared);
+
+        modelBuilder.Entity<EntityBranch2>()
+            .HasMany(e => e.SelfSkipSharedLeft)
+            .WithMany(e => e.SelfSkipSharedRight);
+
+        modelBuilder.Entity<EntityBranch2>()
+            .HasMany(e => e.Leaf2SkipShared)
+            .WithMany(e => e.Branch2SkipShared);
+
+        modelBuilder.Entity<EntityTableSharing1>()
+            .HasMany(e => e.TableSharing2Shared)
+            .WithMany(e => e.TableSharing1Shared);
+
         // Nav:2 Payload:No Join:Concrete Extra:None
         modelBuilder.Entity<EntityOne>()
             .HasMany(e => e.TwoSkip)
@@ -439,6 +526,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
             .HasMany(e => e.TwoSkipShared)
             .WithMany();
 
+        modelBuilder.Entity<UnidirectionalEntityBranch>()
+            .HasMany<UnidirectionalEntityRoot>()
+            .WithMany(e => e.BranchSkipShared);
+
         // Nav:2 Payload:No Join:Concrete Extra:None
         modelBuilder.Entity<UnidirectionalEntityOne>()
             .HasMany(e => e.TwoSkip)

diff --git a/test/EFCore.Specification.Tests/TestModels/ManyToManyFieldsModel/EntityBranch.cs b/test/EFCore.Specification.Tests/TestModels/ManyToManyFieldsModel/EntityBranch.cs
@@ -7,4 +7,5 @@ public class EntityBranch : EntityRoot
 {
     public long Number;
     public ICollection<EntityOne> OneSkip;
+    public ICollection<EntityRoot> RootSkipShared;
 }
diff --git a/test/EFCore.Specification.Tests/TestModels/ManyToManyFieldsModel/EntityRoot.cs b/test/EFCore.Specification.Tests/TestModels/ManyToManyFieldsModel/EntityRoot.cs
@@ -9,4 +9,5 @@ public class EntityRoot
     public string Name;
     public ICollection<EntityThree> ThreeSkipShared;
     public ICollection<EntityCompositeKey> CompositeKeySkipShared;
+    public ICollection<EntityBranch> BranchSkipShared;
 }