From bc1fdcc066186d877dc1a025398ecf764b46f4d6 Mon Sep 17 00:00:00 2001 From: Andriy Svyryd Date: Thu, 8 Dec 2022 15:15:59 -0800 Subject: [PATCH] Avoid reduntant looping in property metadata Fixes #29642 --- .../CSharpRuntimeModelCodeGenerator.cs | 34 ++++++--- src/EFCore/Infrastructure/ModelValidator.cs | 2 +- src/EFCore/Metadata/Internal/ForeignKey.cs | 8 +++ src/EFCore/Metadata/Internal/Property.cs | 70 +++++++++++++------ src/EFCore/Properties/CoreStrings.Designer.cs | 10 ++- src/EFCore/Properties/CoreStrings.resx | 5 +- .../SqlServerEndToEndTest.cs | 8 ++- .../Infrastructure/ModelValidatorTest.cs | 49 ++++++++++++- 8 files changed, 145 insertions(+), 41 deletions(-) diff --git a/src/EFCore.Design/Scaffolding/Internal/CSharpRuntimeModelCodeGenerator.cs b/src/EFCore.Design/Scaffolding/Internal/CSharpRuntimeModelCodeGenerator.cs index 690e39d60a9..154ef67e210 100644 --- a/src/EFCore.Design/Scaffolding/Internal/CSharpRuntimeModelCodeGenerator.cs +++ b/src/EFCore.Design/Scaffolding/Internal/CSharpRuntimeModelCodeGenerator.cs @@ -849,15 +849,17 @@ 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++) @@ -865,25 +867,35 @@ private void Create( 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( diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs index 52df30d463c..57e6cc52d6c 100644 --- a/src/EFCore/Infrastructure/ModelValidator.cs +++ b/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); } /// diff --git a/src/EFCore/Metadata/Internal/ForeignKey.cs b/src/EFCore/Metadata/Internal/ForeignKey.cs index 166bec83544..7832a03aa01 100644 --- a/src/EFCore/Metadata/Internal/ForeignKey.cs +++ b/src/EFCore/Metadata/Internal/ForeignKey.cs @@ -34,6 +34,14 @@ public class ForeignKey : ConventionAnnotatable, IMutableForeignKey, IConvention private IDependentKeyValueFactory? _dependentKeyValueFactory; private Func? _dependentsMapFactory; + /// + /// 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. + /// + public static readonly int LongestFkChainAllowedLength = 10000; + /// /// 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 diff --git a/src/EFCore/Metadata/Internal/Property.cs b/src/EFCore/Metadata/Internal/Property.cs index 158c1025604..d377348f9d7 100644 --- a/src/EFCore/Metadata/Internal/Property.cs +++ b/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; } /// @@ -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() /// 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; } /// @@ -730,7 +742,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior() /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual Type? SetProviderClrType(Type? providerClrType, ConfigurationSource configurationSource) - => (Type?)SetOrRemoveAnnotation(CoreAnnotationNames.ProviderClrType, providerClrType, configurationSource)?.Value; + => (Type?)SetAnnotation(CoreAnnotationNames.ProviderClrType, providerClrType, configurationSource)?.Value; /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -740,15 +752,17 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior() /// 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; } /// diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 502aa450e37..bbedd200447 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -429,7 +429,7 @@ public static string ComparerPropertyMismatch(object? type, object? entityType, /// public static string CompiledQueryDifferentModel(object? queryExpression) => string.Format( - GetString("CompiledQueryDifferentModel", "queryExpression"), + GetString("CompiledQueryDifferentModel", nameof(queryExpression)), queryExpression); /// @@ -2355,6 +2355,14 @@ public static string RelationshipConceptualNullSensitive(object? firstType, obje GetString("RelationshipConceptualNullSensitive", nameof(firstType), nameof(secondType), nameof(secondKeyValue)), firstType, secondType, secondKeyValue); + /// + /// 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. + /// + public static string RelationshipCycle(object? entityType, object? property, object? configuration) + => string.Format( + GetString("RelationshipCycle", nameof(entityType), nameof(property), nameof(configuration)), + entityType, property, configuration); + /// /// '{entityType}.{navigation}' cannot be configured as required since it represents a skip navigation. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index b164e76ae65..1c041ce9dd9 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1307,6 +1307,9 @@ 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. + + 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. + '{entityType}.{navigation}' cannot be configured as required since it represents a skip navigation. @@ -1505,4 +1508,4 @@ Cannot start tracking the entry for entity type '{entityType}' because it was created by a different StateManager instance. - + \ No newline at end of file diff --git a/test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs b/test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs index a354af53ae0..d820f27c429 100644 --- a/test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs +++ b/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(); + modelBuilder.Entity( + eb => + { + eb.HasMany(c => c.Items) + .WithOne() + .HasForeignKey("GameId", "ContainerId"); + }); modelBuilder.Entity( eb => diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs index 88ee0c29aa6..3c959f66bfb 100644 --- a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs +++ b/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().HasBaseType((string)null); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(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(); - modelBuilder.Entity(); modelBuilder.Entity().HasBaseType((string)null); modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(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(dId.GetValueConverter).Message); + Assert.Equal(CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ProviderClrType"), + Assert.Throws(dId.GetProviderClrType).Message); + } + + [ConditionalFact] + public virtual void Detects_relationship_cycle_for_explicit_property_configuration() + { + var modelBuilder = base.CreateConventionModelBuilder(); + + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(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"),