Updated to base cascade behavior on both FKs referencing the same table

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,75 @@ protected override DeleteBehavior GetTargetDeleteBehavior(IConventionForeignKey
  return DeleteBehavior.ClientCascade;
  }
 
- var selfReferencingSkipNavigation = foreignKey.GetReferencingSkipNavigations()
- .FirstOrDefault(s => s.Inverse != null && s.TargetEntityType.IsAssignableFrom(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)
+ {
+ 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());
+ }
  }
 }

test/EFCore.Design.Tests/Scaffolding/Internal/CSharpRuntimeModelCodeGeneratorTest.cs

@@ -1557,7 +1557,7 @@ public static RuntimeForeignKey CreateForeignKey1(RuntimeEntityType declaringEnt
  var runtimeForeignKey = declaringEntityType.AddForeignKey(new[] { declaringEntityType.FindProperty(""DerivedsId"")!, declaringEntityType.FindProperty(""DerivedsAlternateId"")! },
  principalEntityType.FindKey(new[] { principalEntityType.FindProperty(""Id"")!, principalEntityType.FindProperty(""AlternateId"")! })!,
  principalEntityType,
- deleteBehavior: DeleteBehavior.ClientCascade,
+ deleteBehavior: DeleteBehavior.Cascade,
  required: true);
 
  return runtimeForeignKey;
@@ -1568,7 +1568,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;

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");
+ });
+ }
+ }
+}

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");
+ });
+ }
 }

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");

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");

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();
@@ -325,6 +396,18 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
  .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)

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/EntityBranch2.cs

@@ -0,0 +1,13 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
+
+public abstract class EntityBranch2 : EntityRoot
+{
+ public virtual long Slumber { get; set; }
+ public virtual ICollection<EntityLeaf2> Leaf2SkipShared { get; set; }
+
+ public virtual ICollection<EntityBranch2> SelfSkipSharedLeft { get; set; }
+ public virtual ICollection<EntityBranch2> SelfSkipSharedRight { get; set; }
+}

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/EntityLeaf2.cs

@@ -0,0 +1,10 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
+
+public class EntityLeaf2 : EntityBranch2
+{
+ public virtual bool? IsBrown { get; set; }
+ public virtual ICollection<EntityBranch2> Branch2SkipShared { get; set; }
+}

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/EntityTableSharing1.cs

@@ -0,0 +1,11 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
+
+public class EntityTableSharing1
+{
+ public virtual int Id { get; set; }
+ public virtual string Name { get; set; }
+ public virtual ICollection<EntityTableSharing2> TableSharing2Shared { get; set; }
+}

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/EntityTableSharing2.cs

@@ -0,0 +1,11 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel;
+
+public class EntityTableSharing2
+{
+ public virtual int Id { get; set; }
+ public virtual long Cucumber { get; set; }
+ public virtual ICollection<EntityTableSharing1> TableSharing1Shared { get; set; }
+}

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/ManyToManyContext.cs

@@ -17,6 +17,8 @@ public ManyToManyContext(DbContextOptions options)
  public DbSet<EntityThree> EntityThrees { get; set; }
  public DbSet<EntityCompositeKey> EntityCompositeKeys { get; set; }
  public DbSet<EntityRoot> EntityRoots { get; set; }
+ public DbSet<EntityTableSharing1> EntityTableSharing1s { get; set; }
+ public DbSet<EntityTableSharing2> EntityTableSharing2s { get; set; }
  public DbSet<ImplicitManyToManyA> ImplicitManyToManyAs { get; set; }
  public DbSet<ImplicitManyToManyB> ImplicitManyToManyBs { get; set; }
  public DbSet<GeneratedKeysLeft> GeneratedKeysLefts { get; set; }