[5.0.1] Use topological sort to detect identifying relationship cycles.

src/EFCore/Infrastructure/ModelValidator.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
@@ -463,50 +463,42 @@ protected virtual void ValidateNoCycles(
         {
             Check.NotNull(model, nameof(model));
 
-            var typesToValidate = new Queue<IEntityType>();
-            var reachableTypes = new HashSet<IEntityType>();
-            var unvalidatedEntityTypes = new HashSet<IEntityType>(model.GetEntityTypes());
-            while (unvalidatedEntityTypes.Count > 0)
+            var useOldBehavior = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23289", out var isEnabled)
+                    && isEnabled;
+            if (useOldBehavior)
             {
-                var rootEntityType = unvalidatedEntityTypes.First();
-                reachableTypes.Clear();
-                reachableTypes.Add(rootEntityType);
-                typesToValidate.Enqueue(rootEntityType);
+                return;
+            }
 
-                while (typesToValidate.Count > 0)
+            var graph = new Multigraph<IEntityType, IForeignKey>();
+            foreach (var entityType in model.GetEntityTypes())
+            {
+                var primaryKey = entityType.FindPrimaryKey();
+                if (primaryKey == null)
                 {
-                    var entityType = typesToValidate.Dequeue();
-                    var primaryKey = entityType.FindPrimaryKey();
-                    if (primaryKey == null)
-                    {
-                        continue;
-                    }
+                    continue;
+                }
 
-                    foreach (var foreignKey in entityType.GetForeignKeys())
+                foreach (var foreignKey in entityType.GetForeignKeys())
+                {
+                    var principalType = foreignKey.PrincipalEntityType;
+                    if (!foreignKey.PrincipalKey.IsPrimaryKey()
+                        || !PropertyListComparer.Instance.Equals(foreignKey.Properties, primaryKey.Properties)
+                        || foreignKey.PrincipalEntityType.IsAssignableFrom(entityType))
                     {
-                        var principalType = foreignKey.PrincipalEntityType;
-                        if (!foreignKey.PrincipalKey.IsPrimaryKey()
-                            || !unvalidatedEntityTypes.Contains(principalType)
-                            || foreignKey.PrincipalEntityType.IsAssignableFrom(entityType)
-                            || !PropertyListComparer.Instance.Equals(foreignKey.Properties, primaryKey.Properties))
-                        {
-                            continue;
-                        }
-
-                        if (!reachableTypes.Add(principalType))
-                        {
-                            throw new InvalidOperationException(CoreStrings.IdentifyingRelationshipCycle(rootEntityType.DisplayName()));
-                        }
-
-                        typesToValidate.Enqueue(principalType);
+                        continue;
                     }
-                }
 
-                foreach (var entityType in reachableTypes)
-                {
-                    unvalidatedEntityTypes.Remove(entityType);
+                    graph.AddVertex(entityType);
+                    graph.AddVertex(principalType);
+                    graph.AddEdge(entityType, principalType, foreignKey);
                 }
             }
+
+            graph.TopologicalSort(
+                tryBreakEdge: null,
+                formatCycle: c => c.Select(d => d.Item1.DisplayName()).Join(" -> "),
+                c => CoreStrings.IdentifyingRelationshipCycle(c));
         }
 
         /// <summary>

src/EFCore/Properties/CoreStrings.resx

@@ -490,7 +490,7 @@
     <value>The block size used for Hi-Lo value generation must be positive. When the Hi-Lo generator is backed by a SQL sequence this means that the sequence increment must be positive.</value>
   </data>
   <data name="IdentifyingRelationshipCycle" xml:space="preserve">
-    <value>The entity type '{entityType}' is part of a relationship cycle involving its primary key.</value>
+    <value>A relationship cycle involving the primary keys of the following enitity types was detected: '{entityType}'. This would prevent any entity to be inserted without violating the store constraints. Review the foreign keys defined on the primary keys and either remove or use other properties for at least one of them.</value>
   </data>
   <data name="IdentityConflict" xml:space="preserve">
     <value>The instance of entity type '{entityType}' cannot be tracked because another instance with the same key value for {keyProperties} is already being tracked. When attaching existing entities, ensure that only one entity instance with a given key value is attached. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the conflicting key values.</value>

src/Shared/Multigraph.cs

@@ -135,7 +135,8 @@ public IReadOnlyList<TVertex> TopologicalSort(
 
         public IReadOnlyList<TVertex> TopologicalSort(
             [CanBeNull] Func<TVertex, TVertex, IEnumerable<TEdge>, bool> tryBreakEdge,
-            [CanBeNull] Func<IReadOnlyList<Tuple<TVertex, TVertex, IEnumerable<TEdge>>>, string> formatCycle)
+            [CanBeNull] Func<IReadOnlyList<Tuple<TVertex, TVertex, IEnumerable<TEdge>>>, string> formatCycle,
+            Func<string, string> formatException = null)
         {
             var sortedQueue = new List<TVertex>();
             var predecessorCounts = new Dictionary<TVertex, int>();
@@ -248,15 +249,18 @@ public IReadOnlyList<TVertex> TopologicalSort(
 
                         cycle.Reverse();
 
-                        ThrowCycle(cycle, formatCycle);
+                        ThrowCycle(cycle, formatCycle, formatException);
                     }
                 }
             }
 
             return sortedQueue;
         }
 
-        private void ThrowCycle(List<TVertex> cycle, Func<IReadOnlyList<Tuple<TVertex, TVertex, IEnumerable<TEdge>>>, string> formatCycle)
+        private void ThrowCycle(
+            List<TVertex> cycle,
+            Func<IReadOnlyList<Tuple<TVertex, TVertex, IEnumerable<TEdge>>>, string> formatCycle,
+            Func<string, string> formatException = null)
         {
             string cycleString;
             if (formatCycle == null)
@@ -277,7 +281,8 @@ private void ThrowCycle(List<TVertex> cycle, Func<IReadOnlyList<Tuple<TVertex, T
                 cycleString = formatCycle(cycleData);
             }
 
-            throw new InvalidOperationException(CoreStrings.CircularDependency(cycleString));
+            var message = formatException == null ? CoreStrings.CircularDependency(cycleString) : formatException(cycleString);
+            throw new InvalidOperationException(message);
         }
 
         protected virtual string ToString(TVertex vertex)

test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs

@@ -377,7 +377,7 @@ public virtual void Detects_shared_table_root_cycle()
             modelBuilder.Entity<B>().ToTable("Table");
 
             VerifyError(
-                CoreStrings.IdentifyingRelationshipCycle(nameof(A)),
+                CoreStrings.IdentifyingRelationshipCycle("A -> B"),
                 modelBuilder.Model);
         }
 

test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
@@ -321,10 +321,25 @@ public virtual void Detects_relationship_cycle()
             modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
 
             VerifyError(
-                CoreStrings.IdentifyingRelationshipCycle(nameof(A)),
+                CoreStrings.IdentifyingRelationshipCycle("A -> B -> C"),
                 modelBuilder.Model);
         }
 
+        [ConditionalFact]
+        public virtual void Passes_on_multiple_relationship_paths()
+        {
+            var modelBuilder = base.CreateConventionalModelBuilder();
+
+            modelBuilder.Entity<A>();
+            modelBuilder.Entity<B>();
+            modelBuilder.Entity<C>().HasBaseType((string)null);
+            modelBuilder.Entity<A>().HasOne<B>().WithOne().HasForeignKey<A>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
+            modelBuilder.Entity<A>().HasOne<C>().WithOne().HasForeignKey<A>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
+            modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
+
+            Validate(modelBuilder.Model);
+        }
+
         [ConditionalFact]
         public virtual void Passes_on_redundant_foreign_key()
         {