diff --git a/src/EFCore/Metadata/Internal/EntityType.cs b/src/EFCore/Metadata/Internal/EntityType.cs index 98ac9ab9f09..a3c9d8f0027 100644 --- a/src/EFCore/Metadata/Internal/EntityType.cs +++ b/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; diff --git a/src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs b/src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs index 72ca557cc45..f666e0dc67b 100644 --- a/src/EFCore/Metadata/Internal/InternalForeignKeyBuilder.cs +++ b/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 diff --git a/src/EFCore/Metadata/Internal/Property.cs b/src/EFCore/Metadata/Internal/Property.cs index c2614c05eea..a576e265be9 100644 --- a/src/EFCore/Metadata/Internal/Property.cs +++ b/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; } /// @@ -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 - 1) + { + // We need to use different primes to ensure a different cycleBreakingProperty is selected + // each time when traversing properties that participate in multiple relationship cycles + queue.Enqueue((principalProperty, property, 0, 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); } /// @@ -1376,7 +1401,7 @@ public virtual bool IsForeignKey() /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual IEnumerable GetContainingForeignKeys() - => ForeignKeys?.OrderBy(fk => fk.Properties, PropertyListComparer.Instance) ?? Enumerable.Empty(); + => ForeignKeys?.OrderBy(fk => fk, ForeignKeyComparer.Instance) ?? Enumerable.Empty(); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to diff --git a/src/EFCore/Metadata/RuntimeEntityType.cs b/src/EFCore/Metadata/RuntimeEntityType.cs index d8e42af0aa7..72fe3314338 100644 --- a/src/EFCore/Metadata/RuntimeEntityType.cs +++ b/src/EFCore/Metadata/RuntimeEntityType.cs @@ -212,7 +212,7 @@ public virtual RuntimeForeignKey AddForeignKey( { if (property.ForeignKeys == null) { - property.ForeignKeys = new List { foreignKey }; + property.ForeignKeys = new SortedSet(ForeignKeyComparer.Instance) { foreignKey }; } else { diff --git a/src/EFCore/Metadata/RuntimeProperty.cs b/src/EFCore/Metadata/RuntimeProperty.cs index 449816d4d01..1ec7020961d 100644 --- a/src/EFCore/Metadata/RuntimeProperty.cs +++ b/src/EFCore/Metadata/RuntimeProperty.cs @@ -207,7 +207,7 @@ private IEnumerable GetContainingKeys() /// doing so can result in application failures when updating to a new Entity Framework Core release. /// [EntityFrameworkInternal] - public virtual List? ForeignKeys { get; set; } + public virtual ISet? ForeignKeys { get; set; } private IEnumerable GetContainingForeignKeys() => ForeignKeys ?? Enumerable.Empty(); diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 458ee21dd2b..285f24ec937 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -5,6 +5,7 @@ using System.Resources; using System.Threading; using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Internal; using Microsoft.Extensions.Logging; @@ -622,6 +623,14 @@ public static string ConflictingPropertyOrNavigation(object? member, object? typ GetString("ConflictingPropertyOrNavigation", nameof(member), nameof(type), nameof(conflictingType)), member, type, conflictingType); + /// + /// The property '{entityType}.{property}' participates in several relationship chains that have conflicting conversions: '{valueConversion}' and '{conflictingValueConversion}'. + /// + public static string ConflictingRelationshipConversions(object? entityType, object? property, object? valueConversion, object? conflictingValueConversion) + => string.Format( + GetString("ConflictingRelationshipConversions", nameof(entityType), nameof(property), nameof(valueConversion), nameof(conflictingValueConversion)), + entityType, property, valueConversion, conflictingValueConversion); + /// /// 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'. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 4fef33b7946..b178505bd2f 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -342,6 +342,9 @@ 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. + + The property '{entityType}.{property}' participates in several relationship chains that have conflicting conversions: '{valueConversion}' and '{conflictingValueConversion}'. + 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'. diff --git a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs index db27f06f640..5b65e3e6443 100644 --- a/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs +++ b/test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs @@ -730,7 +730,7 @@ public virtual void Detects_shared_table_root_cycle() modelBuilder.Entity().HasOne().WithOne().HasForeignKey(a => a.Id).HasPrincipalKey(b => b.Id).IsRequired(); modelBuilder.Entity().ToTable("Table"); - VerifyError(CoreStrings.RelationshipCycle("B", "AId", "ValueConverter"), modelBuilder); + VerifyError(CoreStrings.IdentifyingRelationshipCycle("A -> B"), modelBuilder); } [ConditionalFact] diff --git a/test/EFCore.Specification.Tests/TestUtilities/ModelAsserter.cs b/test/EFCore.Specification.Tests/TestUtilities/ModelAsserter.cs index 8852de4018a..9dbeacec4b7 100644 --- a/test/EFCore.Specification.Tests/TestUtilities/ModelAsserter.cs +++ b/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; diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs index 671d77d5201..88d0ee7d707 100644 --- a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs +++ b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs @@ -593,28 +593,90 @@ public virtual void Detects_identifying_relationship_cycle() 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.RelationshipCycle("B", "AId", "ValueConverter"), modelBuilder); + VerifyError(CoreStrings.IdentifyingRelationshipCycle("A -> B -> C"), modelBuilder); } [ConditionalFact] - public virtual void Detects_relationship_cycle_for_property_configuration() + public virtual void Passes_on_relationship_cycle_for_property_configuration() { var modelBuilder = base.CreateConventionModelBuilder(); modelBuilder.Entity().HasBaseType((string)null); + 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().HasOne().WithOne().HasForeignKey(c => c.Id).HasPrincipalKey(a => a.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(b => b.Id).HasPrincipalKey(c => c.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(d => d.Id).HasPrincipalKey(b => b.Id).IsRequired(); + + var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id)); + + Assert.Null(dId.GetValueConverter()); + Assert.Null(dId.GetProviderClrType()); + } + + [ConditionalFact] + public virtual void Passes_on_multiple_relationship_cycles_for_property_configuration() + { + var modelBuilder = base.CreateConventionModelBuilder(); + + modelBuilder.Entity().HasBaseType((string)null); 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(c => c.Id).HasPrincipalKey(a => a.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(b => b.Id).HasPrincipalKey(c => c.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(d => d.Id).HasPrincipalKey(c => c.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(e => e.Id).HasPrincipalKey(d => d.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(c => c.Id).HasPrincipalKey(e => e.Id).IsRequired(); + + var aId = modelBuilder.Model.FindEntityType(typeof(A)).FindProperty(nameof(A.Id)); + + Assert.Null(aId.GetValueConverter()); + Assert.Null(aId.GetProviderClrType()); + } + + [ConditionalFact] + public virtual void Detects_conflicting_converter_and_provider_type_with_relationship_cycle() + { + var modelBuilder = base.CreateConventionModelBuilder(); + + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().Property(b => b.Id).HasConversion(); + modelBuilder.Entity().Property(b => b.Id).HasConversion>(); + + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(b => b.Id).HasPrincipalKey(c => c.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(c => c.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(d => d.Id).HasPrincipalKey(a => a.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(d => d.Id).HasPrincipalKey(c => c.Id).IsRequired(); var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id)); - Assert.Equal( - CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ValueConverter"), + Assert.Equal(CoreStrings.ConflictingRelationshipConversions("D", "Id", "string", "CastingConverter"), Assert.Throws(dId.GetValueConverter).Message); - Assert.Equal( - CoreStrings.RelationshipCycle(nameof(D), nameof(D.Id), "ProviderClrType"), + Assert.Equal(CoreStrings.ConflictingRelationshipConversions("D", "Id", "string", "CastingConverter"), + Assert.Throws(dId.GetProviderClrType).Message); + } + + [ConditionalFact] + public virtual void Detects_conflicting_provider_types_with_relationship_cycle() + { + var modelBuilder = base.CreateConventionModelBuilder(); + + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().HasBaseType((string)null); + modelBuilder.Entity().Property(c => c.Id).HasConversion(); + modelBuilder.Entity().Property(a => a.Id).HasConversion(); + + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(b => b.Id).HasPrincipalKey(c => c.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(c => c.Id).HasPrincipalKey(b => b.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(d => d.Id).HasPrincipalKey(a => a.Id).IsRequired(); + modelBuilder.Entity().HasOne().WithOne().HasForeignKey(d => d.Id).HasPrincipalKey(c => c.Id).IsRequired(); + + var dId = modelBuilder.Model.FindEntityType(typeof(D)).FindProperty(nameof(D.Id)); + + Assert.Equal(CoreStrings.ConflictingRelationshipConversions("D", "Id", "string", "long"), + Assert.Throws(dId.GetValueConverter).Message); + Assert.Equal(CoreStrings.ConflictingRelationshipConversions("D", "Id", "string", "long"), Assert.Throws(dId.GetProviderClrType).Message); }