Skip to content

Commit bc1fdcc

Browse files
committed
Avoid reduntant looping in property metadata
Fixes #29642
1 parent 4a2118f commit bc1fdcc

File tree

8 files changed

+145
-41
lines changed

8 files changed

+145
-41
lines changed

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

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -849,41 +849,53 @@ private void Create(
849849

850850
private static Type? GetValueConverterType(IProperty property)
851851
{
852-
var type = (Type?)property[CoreAnnotationNames.ValueConverterType];
853-
if (type != null)
852+
var annotation = property.FindAnnotation(CoreAnnotationNames.ValueConverterType);
853+
if (annotation != null)
854854
{
855-
return type;
855+
return (Type?)annotation.Value;
856856
}
857857

858858
var principalProperty = property;
859-
for (var i = 0; i < 10000; i++)
859+
var i = 0;
860+
for (; i < ForeignKey.LongestFkChainAllowedLength; i++)
860861
{
862+
IProperty? nextProperty = null;
861863
foreach (var foreignKey in principalProperty.GetContainingForeignKeys())
862864
{
863865
for (var propertyIndex = 0; propertyIndex < foreignKey.Properties.Count; propertyIndex++)
864866
{
865867
if (principalProperty == foreignKey.Properties[propertyIndex])
866868
{
867869
var newPrincipalProperty = foreignKey.PrincipalKey.Properties[propertyIndex];
868-
if (property == principalProperty
870+
if (newPrincipalProperty == property
869871
|| newPrincipalProperty == principalProperty)
870872
{
871873
break;
872874
}
873875

874-
principalProperty = newPrincipalProperty;
875-
876-
type = (Type?)principalProperty[CoreAnnotationNames.ValueConverterType];
877-
if (type != null)
876+
annotation = newPrincipalProperty.FindAnnotation(CoreAnnotationNames.ValueConverterType);
877+
if (annotation != null)
878878
{
879-
return type;
879+
return (Type?)annotation.Value;
880880
}
881+
882+
nextProperty = newPrincipalProperty;
881883
}
882884
}
883885
}
886+
887+
if (nextProperty == null)
888+
{
889+
break;
890+
}
891+
892+
principalProperty = nextProperty;
884893
}
885894

886-
return null;
895+
return i == ForeignKey.LongestFkChainAllowedLength
896+
? throw new InvalidOperationException(CoreStrings.RelationshipCycle(
897+
property.DeclaringEntityType.DisplayName(), property.Name, "ValueConverterType"))
898+
: null;
887899
}
888900

889901
private void PropertyBaseParameters(

src/EFCore/Infrastructure/ModelValidator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,7 @@ protected virtual void ValidateNoCycles(
464464
graph.TopologicalSort(
465465
tryBreakEdge: null,
466466
formatCycle: c => c.Select(d => d.Item1.DisplayName()).Join(" -> "),
467-
c => CoreStrings.IdentifyingRelationshipCycle(c));
467+
CoreStrings.IdentifyingRelationshipCycle);
468468
}
469469

470470
/// <summary>

src/EFCore/Metadata/Internal/ForeignKey.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@ public class ForeignKey : ConventionAnnotatable, IMutableForeignKey, IConvention
3434
private IDependentKeyValueFactory? _dependentKeyValueFactory;
3535
private Func<IDependentsMap>? _dependentsMapFactory;
3636

37+
/// <summary>
38+
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
39+
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
40+
/// any release. You should only use it directly in your code with extreme caution and knowing that
41+
/// doing so can result in application failures when updating to a new Entity Framework Core release.
42+
/// </summary>
43+
public static readonly int LongestFkChainAllowedLength = 10000;
44+
3745
/// <summary>
3846
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
3947
/// the same compatibility standards as public APIs. It may be changed or removed without notice in

src/EFCore/Metadata/Internal/Property.cs

Lines changed: 47 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -613,7 +613,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
613613
}
614614

615615
RemoveAnnotation(CoreAnnotationNames.ValueConverterType);
616-
return (ValueConverter?)SetOrRemoveAnnotation(CoreAnnotationNames.ValueConverter, converter, configurationSource)?.Value;
616+
return (ValueConverter?)SetAnnotation(CoreAnnotationNames.ValueConverter, converter, configurationSource)?.Value;
617617
}
618618

619619
/// <summary>
@@ -648,7 +648,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
648648
}
649649

650650
SetValueConverter(converter, configurationSource);
651-
SetOrRemoveAnnotation(CoreAnnotationNames.ValueConverterType, converterType, configurationSource);
651+
SetAnnotation(CoreAnnotationNames.ValueConverterType, converterType, configurationSource);
652652

653653
return converterType;
654654
}
@@ -661,15 +661,17 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
661661
/// </summary>
662662
public virtual ValueConverter? GetValueConverter()
663663
{
664-
var converter = (ValueConverter?)this[CoreAnnotationNames.ValueConverter];
665-
if (converter != null)
664+
var annotation = FindAnnotation(CoreAnnotationNames.ValueConverter);
665+
if (annotation != null)
666666
{
667-
return converter;
667+
return (ValueConverter?)annotation.Value;
668668
}
669669

670670
var property = this;
671-
for (var i = 0; i < 10000; i++)
671+
var i = 0;
672+
for (; i < ForeignKey.LongestFkChainAllowedLength; i++)
672673
{
674+
Property? nextProperty = null;
673675
foreach (var foreignKey in property.GetContainingForeignKeys())
674676
{
675677
for (var propertyIndex = 0; propertyIndex < foreignKey.Properties.Count; propertyIndex++)
@@ -683,19 +685,29 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
683685
break;
684686
}
685687

686-
property = principalProperty;
687-
688-
converter = (ValueConverter?)property[CoreAnnotationNames.ValueConverter];
689-
if (converter != null)
688+
annotation = principalProperty.FindAnnotation(CoreAnnotationNames.ValueConverter);
689+
if (annotation != null)
690690
{
691-
return converter;
691+
return (ValueConverter?)annotation.Value;
692692
}
693+
694+
nextProperty = principalProperty;
693695
}
694696
}
695697
}
698+
699+
if (nextProperty == null)
700+
{
701+
break;
702+
}
703+
704+
property = nextProperty;
696705
}
697706

698-
return null;
707+
return i == ForeignKey.LongestFkChainAllowedLength
708+
? throw new InvalidOperationException(CoreStrings.RelationshipCycle(
709+
DeclaringEntityType.DisplayName(), Name, "ValueConverter"))
710+
: null;
699711
}
700712

701713
/// <summary>
@@ -730,7 +742,7 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
730742
/// doing so can result in application failures when updating to a new Entity Framework Core release.
731743
/// </summary>
732744
public virtual Type? SetProviderClrType(Type? providerClrType, ConfigurationSource configurationSource)
733-
=> (Type?)SetOrRemoveAnnotation(CoreAnnotationNames.ProviderClrType, providerClrType, configurationSource)?.Value;
745+
=> (Type?)SetAnnotation(CoreAnnotationNames.ProviderClrType, providerClrType, configurationSource)?.Value;
734746

735747
/// <summary>
736748
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -740,15 +752,17 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
740752
/// </summary>
741753
public virtual Type? GetProviderClrType()
742754
{
743-
var type = (Type?)this[CoreAnnotationNames.ProviderClrType];
744-
if (type != null)
755+
var annotation = FindAnnotation(CoreAnnotationNames.ProviderClrType);
756+
if (annotation != null)
745757
{
746-
return type;
758+
return (Type?)annotation.Value;
747759
}
748760

749761
var property = this;
750-
for (var i = 0; i < 10000; i++)
762+
var i = 0;
763+
for (; i < ForeignKey.LongestFkChainAllowedLength; i++)
751764
{
765+
Property? nextProperty = null;
752766
foreach (var foreignKey in property.GetContainingForeignKeys())
753767
{
754768
for (var propertyIndex = 0; propertyIndex < foreignKey.Properties.Count; propertyIndex++)
@@ -762,19 +776,29 @@ public virtual PropertySaveBehavior GetAfterSaveBehavior()
762776
break;
763777
}
764778

765-
property = principalProperty;
766-
767-
type = (Type?)property[CoreAnnotationNames.ProviderClrType];
768-
if (type != null)
779+
annotation = principalProperty.FindAnnotation(CoreAnnotationNames.ProviderClrType);
780+
if (annotation != null)
769781
{
770-
return type;
782+
return (Type?)annotation.Value;
771783
}
784+
785+
nextProperty = principalProperty;
772786
}
773787
}
774788
}
789+
790+
if (nextProperty == null)
791+
{
792+
break;
793+
}
794+
795+
property = nextProperty;
775796
}
776797

777-
return null;
798+
return i == ForeignKey.LongestFkChainAllowedLength
799+
? throw new InvalidOperationException(CoreStrings.RelationshipCycle(
800+
DeclaringEntityType.DisplayName(), Name, "ProviderClrType"))
801+
: null;
778802
}
779803

780804
/// <summary>

src/EFCore/Properties/CoreStrings.Designer.cs

Lines changed: 9 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/EFCore/Properties/CoreStrings.resx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1307,6 +1307,9 @@
13071307
<data name="RelationshipConceptualNullSensitive" xml:space="preserve">
13081308
<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>
13091309
</data>
1310+
<data name="RelationshipCycle" xml:space="preserve">
1311+
<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>
1312+
</data>
13101313
<data name="RequiredSkipNavigation" xml:space="preserve">
13111314
<value>'{entityType}.{navigation}' cannot be configured as required since it represents a skip navigation.</value>
13121315
</data>
@@ -1505,4 +1508,4 @@
15051508
<data name="WrongStateManager" xml:space="preserve">
15061509
<value>Cannot start tracking the entry for entity type '{entityType}' because it was created by a different StateManager instance.</value>
15071510
</data>
1508-
</root>
1511+
</root>

test/EFCore.SqlServer.FunctionalTests/SqlServerEndToEndTest.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1396,7 +1396,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
13961396
eb.HasKey(l => new { l.GameId, l.Id });
13971397
});
13981398

1399-
modelBuilder.Entity<Container>();
1399+
modelBuilder.Entity<Container>(
1400+
eb =>
1401+
{
1402+
eb.HasMany(c => c.Items)
1403+
.WithOne()
1404+
.HasForeignKey("GameId", "ContainerId");
1405+
});
14001406

14011407
modelBuilder.Entity<Game>(
14021408
eb =>

test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -472,16 +472,59 @@ public virtual void Detects_key_property_with_value_generated_on_add_or_update()
472472
}
473473

474474
[ConditionalFact]
475-
public virtual void Detects_relationship_cycle()
475+
public virtual void Detects_identifying_relationship_cycle()
476+
{
477+
var modelBuilder = base.CreateConventionModelBuilder();
478+
479+
modelBuilder.Entity<C>().HasBaseType((string)null);
480+
modelBuilder.Entity<A>().HasOne<B>().WithOne().HasForeignKey<A>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
481+
modelBuilder.Entity<A>().HasOne<C>().WithOne().HasForeignKey<C>(a => a.Id).HasPrincipalKey<A>(b => b.Id).IsRequired();
482+
modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
483+
484+
VerifyError(
485+
CoreStrings.IdentifyingRelationshipCycle("A -> B -> C"),
486+
modelBuilder);
487+
}
488+
489+
[ConditionalFact]
490+
public virtual void Detects_relationship_cycle_for_property_configuration()
476491
{
477492
var modelBuilder = base.CreateConventionModelBuilder();
478493

479-
modelBuilder.Entity<A>();
480-
modelBuilder.Entity<B>();
481494
modelBuilder.Entity<C>().HasBaseType((string)null);
482495
modelBuilder.Entity<A>().HasOne<B>().WithOne().HasForeignKey<A>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
483496
modelBuilder.Entity<A>().HasOne<C>().WithOne().HasForeignKey<C>(a => a.Id).HasPrincipalKey<A>(b => b.Id).IsRequired();
484497
modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
498+
modelBuilder.Entity<D>().HasBaseType((string)null);
499+
modelBuilder.Entity<D>().HasOne<B>().WithOne().HasForeignKey<D>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
500+
501+
var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id));
502+
503+
Assert.Equal(CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ValueConverter"),
504+
Assert.Throws<InvalidOperationException>(dId.GetValueConverter).Message);
505+
Assert.Equal(CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ProviderClrType"),
506+
Assert.Throws<InvalidOperationException>(dId.GetProviderClrType).Message);
507+
}
508+
509+
[ConditionalFact]
510+
public virtual void Detects_relationship_cycle_for_explicit_property_configuration()
511+
{
512+
var modelBuilder = base.CreateConventionModelBuilder();
513+
514+
modelBuilder.Entity<C>().HasBaseType((string)null);
515+
modelBuilder.Entity<A>().HasOne<B>().WithOne().HasForeignKey<A>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
516+
modelBuilder.Entity<A>().HasOne<C>().WithOne().HasForeignKey<C>(a => a.Id).HasPrincipalKey<A>(b => b.Id).IsRequired();
517+
modelBuilder.Entity<C>().HasOne<B>().WithOne().HasForeignKey<B>(a => a.Id).HasPrincipalKey<C>(b => b.Id).IsRequired();
518+
modelBuilder.Entity<D>().HasBaseType((string)null);
519+
modelBuilder.Entity<D>().HasOne<B>().WithOne().HasForeignKey<D>(a => a.Id).HasPrincipalKey<B>(b => b.Id).IsRequired();
520+
521+
var aId = modelBuilder.Model.FindEntityType(typeof(A)).FindProperty(nameof(A.Id));
522+
aId.SetValueConverter((ValueConverter)null);
523+
aId.SetProviderClrType(null);
524+
525+
var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id));
526+
Assert.Null(dId.GetValueConverter());
527+
Assert.Null(dId.GetProviderClrType());
485528

486529
VerifyError(
487530
CoreStrings.IdentifyingRelationshipCycle("A -> B -> C"),

0 commit comments

Comments
 (0)