Improve the relationship cycle breaking logic to detect cycles even w…

…hen not starting on one.

Additionally, detect multiple relationship chains that end with incompatible conversions.

Fixes #32422

src/EFCore/Metadata/Internal/EntityType.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Diagnostics.CodeAnalysis;
-using System.Runtime.CompilerServices;
 using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
 using Microsoft.EntityFrameworkCore.Internal;
 

src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs

@@ -3677,7 +3677,11 @@ private bool CanSetRelatedTypes(
             && (((navigationToPrincipal != null)
                     && (navigationToPrincipal.Value.Name == Metadata.PrincipalToDependent?.Name))
                 || ((navigationToDependent != null)
-                    && (navigationToDependent.Value.Name == Metadata.DependentToPrincipal?.Name)));
+                    && (navigationToDependent.Value.Name == Metadata.DependentToPrincipal?.Name))
+                || ((navigationToPrincipal == null)
+                    && (navigationToDependent == null)
+                    && principalEntityType == Metadata.DeclaringEntityType
+                    && dependentEntityType == Metadata.PrincipalEntityType));
 
         var someAspectsFitNonInverted = false;
         if (!sameHierarchyInvertedNavigations

src/EFCore/Metadata/Internal/Property.cs

@@ -4,6 +4,7 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Storage.Json;
 
@@ -767,53 +768,10 @@ private object? DefaultSentinel
     public virtual ValueConverter? GetValueConverter()
     {
         var annotation = FindAnnotation(CoreAnnotationNames.ValueConverter);
-        if (annotation != null)
-        {
-            return (ValueConverter?)annotation.Value;
-        }
-
-        var property = this;
-        var i = 0;
-        for (; i < ForeignKey.LongestFkChainAllowedLength; i++)
-        {
-            Property? nextProperty = null;
-            foreach (var foreignKey in property.GetContainingForeignKeys())
-            {
-                for (var propertyIndex = 0; propertyIndex < foreignKey.Properties.Count; propertyIndex++)
-                {
-                    if (property == foreignKey.Properties[propertyIndex])
-                    {
-                        var principalProperty = foreignKey.PrincipalKey.Properties[propertyIndex];
-                        if (principalProperty == this
-                            || principalProperty == property)
-                        {
-                            break;
-                        }
-
-                        annotation = principalProperty.FindAnnotation(CoreAnnotationNames.ValueConverter);
-                        if (annotation != null)
-                        {
-                            return (ValueConverter?)annotation.Value;
-                        }
-
-                        nextProperty = principalProperty;
-                    }
-                }
-            }
-
-            if (nextProperty == null)
-            {
-                break;
-            }
-
-            property = nextProperty;
-        }
-
-        return i == ForeignKey.LongestFkChainAllowedLength
-            ? throw new InvalidOperationException(
-                CoreStrings.RelationshipCycle(
-                    DeclaringType.DisplayName(), Name, "ValueConverter"))
-            : null;
+        return annotation != null
+            ? (ValueConverter?)annotation.Value
+            : GetConversion(throwOnProviderClrTypeConflict: FindAnnotation(CoreAnnotationNames.ProviderClrType) == null)
+                .ValueConverter;
     }
 
     /// <summary>
@@ -859,53 +817,120 @@ private object? DefaultSentinel
     public virtual Type? GetProviderClrType()
     {
         var annotation = FindAnnotation(CoreAnnotationNames.ProviderClrType);
-        if (annotation != null)
-        {
-            return (Type?)annotation.Value;
-        }
+        return annotation != null
+            ? (Type?)annotation.Value
+            : GetConversion(throwOnValueConverterConflict: FindAnnotation(CoreAnnotationNames.ValueConverter) == null)
+                .ProviderClrType;
+    }
+
+    private (ValueConverter? ValueConverter, Type? ProviderClrType) GetConversion(
+        bool throwOnValueConverterConflict = true, bool throwOnProviderClrTypeConflict = true)
+    {
+        var queue = new Queue<(Property CurrentProperty, Property CycleBreakingPropert, int CyclePosition, int MaxCycleLength)>();
+        queue.Enqueue((this, this, 0, 2));
 
-        var property = this;
-        var i = 0;
-        for (; i < ForeignKey.LongestFkChainAllowedLength; i++)
+        ValueConverter? valueConverter = null;
+        Type? providerClrType = null;
+        while (queue.Count > 0)
         {
-            Property? nextProperty = null;
+            var (property, cycleBreakingProperty, cyclePosition, maxCycleLength) = queue.Dequeue();
+            if (cyclePosition >= ForeignKey.LongestFkChainAllowedLength)
+            {
+                throw new InvalidOperationException(
+                    CoreStrings.RelationshipCycle(DeclaringType.DisplayName(), Name, "ValueConverter"));
+            }
+
             foreach (var foreignKey in property.GetContainingForeignKeys())
             {
                 for (var propertyIndex = 0; propertyIndex < foreignKey.Properties.Count; propertyIndex++)
                 {
-                    if (property == foreignKey.Properties[propertyIndex])
+                    if (property != foreignKey.Properties[propertyIndex])
+                    {
+                        continue;
+                    }
+
+                    var principalProperty = foreignKey.PrincipalKey.Properties[propertyIndex];
+                    if (principalProperty == cycleBreakingProperty)
+                    {
+                        break;
+                    }
+
+                    var annotationFound = false;
+                    var valueConverterAnnotation = principalProperty.FindAnnotation(CoreAnnotationNames.ValueConverter);
+                    if (valueConverterAnnotation != null)
                     {
-                        var principalProperty = foreignKey.PrincipalKey.Properties[propertyIndex];
-                        if (principalProperty == this
-                            || principalProperty == property)
+                        var annotationValue = (ValueConverter?)valueConverterAnnotation.Value;
+                        if (annotationValue != null)
                         {
-                            break;
+                            if (valueConverter != null)
+                            {
+                                throw new InvalidOperationException(
+                                    CoreStrings.ConflictingRelationshipConversions(
+                                        DeclaringType.DisplayName(), Name,
+                                        valueConverter.GetType().ShortDisplayName(), annotationValue.GetType().ShortDisplayName()));
+                            }
+
+                            if (providerClrType != null
+                                && throwOnProviderClrTypeConflict)
+                            {
+                                throw new InvalidOperationException(
+                                    CoreStrings.ConflictingRelationshipConversions(
+                                        DeclaringType.DisplayName(), Name,
+                                        providerClrType.ShortDisplayName(), annotationValue.GetType().ShortDisplayName()));
+                            }
+
+                            valueConverter = annotationValue;
                         }
+                        annotationFound = true;
+                    }
 
-                        annotation = principalProperty.FindAnnotation(CoreAnnotationNames.ProviderClrType);
-                        if (annotation != null)
+                    var providerClrTypeAnnotation = principalProperty.FindAnnotation(CoreAnnotationNames.ProviderClrType);
+                    if (providerClrTypeAnnotation != null)
+                    {
+                        var annotationValue = (Type?)providerClrTypeAnnotation.Value;
+                        if (annotationValue != null)
                         {
-                            return (Type?)annotation.Value;
+                            if (providerClrType != null)
+                            {
+                                throw new InvalidOperationException(
+                                    CoreStrings.ConflictingRelationshipConversions(
+                                        DeclaringType.DisplayName(), Name,
+                                        providerClrType.ShortDisplayName(), annotationValue.ShortDisplayName()));
+                            }
+
+                            if (valueConverter != null
+                                && throwOnValueConverterConflict)
+                            {
+                                throw new InvalidOperationException(
+                                    CoreStrings.ConflictingRelationshipConversions(
+                                        DeclaringType.DisplayName(), Name,
+                                        valueConverter.GetType().ShortDisplayName(), annotationValue.ShortDisplayName()));
+                            }
+
+                            providerClrType = annotationValue;
                         }
+                        annotationFound = true;
+                    }
 
-                        nextProperty = principalProperty;
+                    if (!annotationFound)
+                    {
+                        if (cyclePosition == maxCycleLength)
+                        {
+                            // We need to use all possible factors to ensure a different cycleBreakingProperty is selected
+                            // each time when traversing properties that participate in multiple relationship cycles
+                            queue.Enqueue((principalProperty, property, 0, maxCycleLength * HashHelpers.GetPrime(maxCycleLength + 1)));
+                        }
+                        else
+                        {
+                            queue.Enqueue((principalProperty, cycleBreakingProperty, cyclePosition + 1, maxCycleLength));
+                        }
                     }
+                    break;
                 }
             }
-
-            if (nextProperty == null)
-            {
-                break;
-            }
-
-            property = nextProperty;
         }
 
-        return i == ForeignKey.LongestFkChainAllowedLength
-            ? throw new InvalidOperationException(
-                CoreStrings.RelationshipCycle(
-                    DeclaringType.DisplayName(), Name, "ProviderClrType"))
-            : null;
+        return (valueConverter, providerClrType);
     }
 
     /// <summary>
@@ -1376,7 +1401,7 @@ public virtual bool IsForeignKey()
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual IEnumerable<ForeignKey> GetContainingForeignKeys()
-        => ForeignKeys?.OrderBy(fk => fk.Properties, PropertyListComparer.Instance) ?? Enumerable.Empty<ForeignKey>();
+        => ForeignKeys?.OrderBy(fk => fk, ForeignKeyComparer.Instance) ?? Enumerable.Empty<ForeignKey>();
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

src/EFCore/Metadata/RuntimeEntityType.cs

@@ -212,7 +212,7 @@ public virtual RuntimeForeignKey AddForeignKey(
         {
             if (property.ForeignKeys == null)
             {
-                property.ForeignKeys = new List<RuntimeForeignKey> { foreignKey };
+                property.ForeignKeys = new SortedSet<RuntimeForeignKey>(ForeignKeyComparer.Instance) { foreignKey };
             }
             else
             {

src/EFCore/Metadata/RuntimeProperty.cs

@@ -207,7 +207,7 @@ private IEnumerable<RuntimeKey> GetContainingKeys()
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     [EntityFrameworkInternal]
-    public virtual List<RuntimeForeignKey>? ForeignKeys { get; set; }
+    public virtual ISet<RuntimeForeignKey>? ForeignKeys { get; set; }
 
     private IEnumerable<RuntimeForeignKey> GetContainingForeignKeys()
         => ForeignKeys ?? Enumerable.Empty<RuntimeForeignKey>();

src/EFCore/Properties/CoreStrings.resx

@@ -342,6 +342,9 @@
   <data name="ConflictingPropertyOrNavigation" xml:space="preserve">
     <value>The property or navigation '{member}' cannot be added to the '{type}' type because a property or navigation with the same name already exists on the '{conflictingType}' type.</value>
   </data>
+  <data name="ConflictingRelationshipConversions" xml:space="preserve">
+    <value>The property '{entityType}.{property}' participates in several relationship chains that have conflicting conversions: '{valueConversion}' and '{conflictingValueConversion}'.</value>
+  </data>
   <data name="ConflictingRelationshipNavigation" xml:space="preserve">
     <value>Cannot create a relationship between '{newPrincipalNavigationSpecification}' and '{newDependentNavigationSpecification}' because a relationship already exists between '{existingPrincipalNavigationSpecification}' and '{existingDependentNavigationSpecification}'. Navigations can only participate in a single relationship. If you want to override an existing relationship call 'Ignore' on the navigation '{newDependentNavigationSpecification}' first in 'OnModelCreating'.</value>
   </data>

test/EFCore.Specification.Tests/TestUtilities/ModelAsserter.cs

@@ -665,6 +665,13 @@ public virtual bool AssertEqual(
                     Assert.Equal(expected.DeclaringEntityType.Name, actual.DeclaringEntityType.Name);
                 }
             },
+            () =>
+            {
+                if (compareBackreferences)
+                {
+                    Assert.Equal(expected.GetReferencingForeignKeys().ToList(), actual.GetReferencingForeignKeys(), ForeignKeyComparer.Instance);
+                }
+            },
             () => Assert.Equal(expectedAnnotations, actualAnnotations, TestAnnotationComparer.Instance));
 
         return true;