Avoid redundant looping in property metadata

src/EFCore.Design/Scaffolding/Internal/CSharpRuntimeModelCodeGenerator.cs

@@ -849,41 +849,53 @@ private void Create(
 
     private static Type? GetValueConverterType(IProperty property)
     {
-        var type = (Type?)property[CoreAnnotationNames.ValueConverterType];
-        if (type != null)
+        var annotation = property.FindAnnotation(CoreAnnotationNames.ValueConverterType);
+        if (annotation != null)
         {
-            return type;
+            return (Type?)annotation.Value;
         }
 
         var principalProperty = property;
-        for (var i = 0; i < 10000; i++)
+        var i = 0;
+        for (; i < ForeignKey.LongestFkChainAllowedLength; i++)
         {
+            IProperty? nextProperty = null;
             foreach (var foreignKey in principalProperty.GetContainingForeignKeys())
             {
                 for (var propertyIndex = 0; propertyIndex < foreignKey.Properties.Count; propertyIndex++)
                 {
                     if (principalProperty == foreignKey.Properties[propertyIndex])
                     {
                         var newPrincipalProperty = foreignKey.PrincipalKey.Properties[propertyIndex];
-                        if (property == principalProperty
+                        if (newPrincipalProperty == property
                             || newPrincipalProperty == principalProperty)
                         {
                             break;
                         }
 
-                        principalProperty = newPrincipalProperty;
-
-                        type = (Type?)principalProperty[CoreAnnotationNames.ValueConverterType];
-                        if (type != null)
+                        annotation = newPrincipalProperty.FindAnnotation(CoreAnnotationNames.ValueConverterType);
+                        if (annotation != null)
                         {
-                            return type;
+                            return (Type?)annotation.Value;
                         }
+
+                        nextProperty = newPrincipalProperty;
                     }
                 }
             }
+
+            if (nextProperty == null)
+            {
+                break;
+            }
+
+            principalProperty = nextProperty;
         }
 
-        return null;
+        return i == ForeignKey.LongestFkChainAllowedLength
+            ? throw new InvalidOperationException(CoreStrings.RelationshipCycle(
+                property.DeclaringEntityType.DisplayName(), property.Name, "ValueConverterType"))
+            : null;
     }
 
     private void PropertyBaseParameters(

src/EFCore/Infrastructure/ModelValidator.cs

@@ -464,7 +464,7 @@ protected virtual void ValidateNoCycles(
         graph.TopologicalSort(
             tryBreakEdge: null,
             formatCycle: c => c.Select(d => d.Item1.DisplayName()).Join(" -> "),
-            c => CoreStrings.IdentifyingRelationshipCycle(c));
+            CoreStrings.IdentifyingRelationshipCycle);
     }
 
     /// <summary>

src/EFCore/Metadata/Internal/ForeignKey.cs

@@ -34,6 +34,14 @@ public class ForeignKey : ConventionAnnotatable, IMutableForeignKey, IConvention
     private IDependentKeyValueFactory? _dependentKeyValueFactory;
     private Func<IDependentsMap>? _dependentsMapFactory;
 
+    /// <summary>
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    public static readonly int LongestFkChainAllowedLength = 10000;
+
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
     ///     the same compatibility standards as public APIs. It may be changed or removed without notice in

src/EFCore/Metadata/Internal/Property.cs

@@ -613,7 +613,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
         }
 
         RemoveAnnotation(CoreAnnotationNames.ValueConverterType);
-        return (ValueConverter?)SetOrRemoveAnnotation(CoreAnnotationNames.ValueConverter, converter, configurationSource)?.Value;
+        return (ValueConverter?)SetAnnotation(CoreAnnotationNames.ValueConverter, converter, configurationSource)?.Value;
     }
 
     /// <summary>
@@ -648,7 +648,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
         }
 
         SetValueConverter(converter, configurationSource);
-        SetOrRemoveAnnotation(CoreAnnotationNames.ValueConverterType, converterType, configurationSource);
+        SetAnnotation(CoreAnnotationNames.ValueConverterType, converterType, configurationSource);
 
         return converterType;
     }
@@ -661,15 +661,17 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
     /// </summary>
     public virtual ValueConverter? GetValueConverter()
     {
-        var converter = (ValueConverter?)this[CoreAnnotationNames.ValueConverter];
-        if (converter != null)
+        var annotation = FindAnnotation(CoreAnnotationNames.ValueConverter);
+        if (annotation != null)
         {
-            return converter;
+            return (ValueConverter?)annotation.Value;
         }
 
         var property = this;
-        for (var i = 0; i < 10000; i++)
+        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++)
@@ -683,19 +685,29 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
                             break;
                         }
 
-                        property = principalProperty;
-
-                        converter = (ValueConverter?)property[CoreAnnotationNames.ValueConverter];
-                        if (converter != null)
+                        annotation = principalProperty.FindAnnotation(CoreAnnotationNames.ValueConverter);
+                        if (annotation != null)
                         {
-                            return converter;
+                            return (ValueConverter?)annotation.Value;
                         }
+
+                        nextProperty = principalProperty;
                     }
                 }
             }
+
+            if (nextProperty == null)
+            {
+                break;
+            }
+
+            property = nextProperty;
         }
 
-        return null;
+        return i == ForeignKey.LongestFkChainAllowedLength
+            ? throw new InvalidOperationException(CoreStrings.RelationshipCycle(
+                DeclaringEntityType.DisplayName(), Name, "ValueConverter"))
+            : null;
     }
 
     /// <summary>
@@ -730,7 +742,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual Type? SetProviderClrType(Type? providerClrType, ConfigurationSource configurationSource)
-        => (Type?)SetOrRemoveAnnotation(CoreAnnotationNames.ProviderClrType, providerClrType, configurationSource)?.Value;
+        => (Type?)SetAnnotation(CoreAnnotationNames.ProviderClrType, providerClrType, configurationSource)?.Value;
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -740,15 +752,17 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
     /// </summary>
     public virtual Type? GetProviderClrType()
     {
-        var type = (Type?)this[CoreAnnotationNames.ProviderClrType];
-        if (type != null)
+        var annotation = FindAnnotation(CoreAnnotationNames.ProviderClrType);
+        if (annotation != null)
         {
-            return type;
+            return (Type?)annotation.Value;
         }
 
         var property = this;
-        for (var i = 0; i < 10000; i++)
+        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++)
@@ -762,19 +776,29 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
                             break;
                         }
 
-                        property = principalProperty;
-
-                        type = (Type?)property[CoreAnnotationNames.ProviderClrType];
-                        if (type != null)
+                        annotation = principalProperty.FindAnnotation(CoreAnnotationNames.ProviderClrType);
+                        if (annotation != null)
                         {
-                            return type;
+                            return (Type?)annotation.Value;
                         }
+
+                        nextProperty = principalProperty;
                     }
                 }
             }
+
+            if (nextProperty == null)
+            {
+                break;
+            }
+
+            property = nextProperty;
         }
 
-        return null;
+        return i == ForeignKey.LongestFkChainAllowedLength
+            ? throw new InvalidOperationException(CoreStrings.RelationshipCycle(
+                DeclaringEntityType.DisplayName(), Name, "ProviderClrType"))
+            : null;
     }
 
     /// <summary>

src/EFCore/Properties/CoreStrings.resx

@@ -1307,6 +1307,9 @@
   <data name="RelationshipConceptualNullSensitive" xml:space="preserve">
     <value>The association between entities '{firstType}' and '{secondType}' with the key value '{secondKeyValue}' has been severed, but the relationship is either marked as required or is implicitly required because the foreign key is not nullable. If the dependent/child entity should be deleted when a required relationship is severed, configure the relationship to use cascade deletes.</value>
   </data>
+  <data name="RelationshipCycle" xml:space="preserve">
+    <value>A relationship cycle involving the property '{entityType}.{property}' was detected. This prevents Entity Framework from determining the correct configuration. Review the foreign keys defined on the property and the corresponding principal property and either remove one of them or specify '{configuration}' explicitly on one of the properties.</value>
+  </data>
   <data name="RequiredSkipNavigation" xml:space="preserve">
     <value>'{entityType}.{navigation}' cannot be configured as required since it represents a skip navigation.</value>
   </data>
@@ -1505,4 +1508,4 @@
   <data name="WrongStateManager" xml:space="preserve">
     <value>Cannot start tracking the entry for entity type '{entityType}' because it was created by a different StateManager instance.</value>
   </data>
-</root>
+</root>

test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs

@@ -1396,7 +1396,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
                     eb.HasKey(l => new { l.GameId, l.Id });
                 });
 
-            modelBuilder.Entity<Container>();
+            modelBuilder.Entity<Container>(
+                eb =>
+                {
+                    eb.HasMany(c => c.Items)
+                        .WithOne()
+                        .HasForeignKey("GameId", "ContainerId");
+                });
 
             modelBuilder.Entity<Game>(
                 eb =>

test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs

@@ -472,16 +472,59 @@ public virtual void Detects_key_property_with_value_generated_on_add_or_update()
     }
 
     [ConditionalFact]
-    public virtual void Detects_relationship_cycle()
+    public virtual void Detects_identifying_relationship_cycle()
+    {
+        var modelBuilder = base.CreateConventionModelBuilder();
+
+        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<C>(a => a.Id).HasPrincipalKey<A>(b => b.Id).IsRequired();
+        modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
+
+        VerifyError(
+            CoreStrings.IdentifyingRelationshipCycle("A -> B -> C"),
+            modelBuilder);
+    }
+
+    [ConditionalFact]
+    public virtual void Detects_relationship_cycle_for_property_configuration()
     {
         var modelBuilder = base.CreateConventionModelBuilder();
 
-        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<C>(a => a.Id).HasPrincipalKey<A>(b => b.Id).IsRequired();
         modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
+        modelBuilder.Entity<D>().HasBaseType((string)null);
+        modelBuilder.Entity<D>().HasOne<B>().WithOne().HasForeignKey<D>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
+
+        var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id));
+
+        Assert.Equal(CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ValueConverter"),
+            Assert.Throws<InvalidOperationException>(dId.GetValueConverter).Message);
+        Assert.Equal(CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ProviderClrType"),
+            Assert.Throws<InvalidOperationException>(dId.GetProviderClrType).Message);
+    }
+
+    [ConditionalFact]
+    public virtual void Detects_relationship_cycle_for_explicit_property_configuration()
+    {
+        var modelBuilder = base.CreateConventionModelBuilder();
+
+        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<C>(a => a.Id).HasPrincipalKey<A>(b => b.Id).IsRequired();
+        modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
+        modelBuilder.Entity<D>().HasBaseType((string)null);
+        modelBuilder.Entity<D>().HasOne<B>().WithOne().HasForeignKey<D>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
+
+        var aId = modelBuilder.Model.FindEntityType(typeof(A)).FindProperty(nameof(A.Id));
+        aId.SetValueConverter((ValueConverter)null);
+        aId.SetProviderClrType(null);
+
+        var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id));
+        Assert.Null(dId.GetValueConverter());
+        Assert.Null(dId.GetProviderClrType());
 
         VerifyError(
             CoreStrings.IdentifyingRelationshipCycle("A -> B -> C"),