Skip to content

Commit

Permalink
Avoid infinite loop when processing InversePropertyAnnotation
Browse files Browse the repository at this point in the history 
Fixes #15738
  • Loading branch information
@AndriySvyryd
AndriySvyryd committed Aug 30, 2019
1 parent 43b867a commit 1c5e308
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 37 deletions.
1 change: 1 addition & 0 deletions src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs
View file
Expand Up @@ -176,6 +176,7 @@ public virtual ConventionSet CreateConventionSet()
conventionSet.ModelFinalizedConventions.Add(nonNullableReferencePropertyConvention);
conventionSet.ModelFinalizedConventions.Add(nonNullableNavigationConvention);
conventionSet.ModelFinalizedConventions.Add(new QueryFilterDefiningQueryRewritingConvention(Dependencies));
conventionSet.ModelFinalizedConventions.Add(inversePropertyAttributeConvention);
conventionSet.ModelFinalizedConventions.Add(new ValidatingConvention(Dependencies));
// Don't add any more conventions to ModelFinalizedConventions after ValidatingConvention

Expand Down
103 changes: 75 additions & 28 deletions src/EFCore/Metadata/Conventions/InversePropertyAttributeConvention.cs
View file
Expand Up @@ -119,6 +119,19 @@ public InversePropertyAttributeConvention([NotNull] ProviderConventionSetBuilder

var ambiguousInverse = FindAmbiguousInverse(
navigationMemberInfo, entityType, referencingNavigationsWithAttribute);
var baseType = targetEntityTypeBuilder.Metadata.BaseType;
while(ambiguousInverse == null
&& baseType != null)
{
var navigationMap = GetInverseNavigations(baseType);
if (navigationMap != null
&& navigationMap.TryGetValue(inverseNavigationPropertyInfo.Name, out var inverseTuple)) {
referencingNavigationsWithAttribute = inverseTuple.References;
ambiguousInverse = FindAmbiguousInverse(navigationMemberInfo, entityType, referencingNavigationsWithAttribute);
}
baseType = baseType.BaseType;
}

if (ambiguousInverse != null)
{
var existingInverse = targetEntityTypeBuilder.Metadata.FindNavigation(inverseNavigationPropertyInfo)?.FindInverse();
Expand Down Expand Up @@ -268,6 +281,12 @@ public InversePropertyAttributeConvention([NotNull] ProviderConventionSetBuilder
attribute);
if (newRelationship != relationshipBuilder)
{
if (newRelationship == null)
{
context.StopProcessingIfChanged(null);
return;
}

var newNavigation = navigation.IsDependentToPrincipal()
? newRelationship.Metadata.DependentToPrincipal
: newRelationship.Metadata.PrincipalToDependent;
Expand Down Expand Up @@ -356,14 +375,32 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
continue;
}

foreach (var inverseNavigation in inverseNavigations)
foreach (var inverseNavigation in inverseNavigations.Values)
{
foreach (var referencingNavigationWithAttribute in inverseNavigation.Value)
foreach (var referencingNavigationWithAttribute in inverseNavigation.References)
{
var ambiguousInverse = FindAmbiguousInverse(
referencingNavigationWithAttribute.Item1,
referencingNavigationWithAttribute.Item2,
inverseNavigation.Value);
inverseNavigation.References);

var baseType = entityType.BaseType;
while (ambiguousInverse == null
&& baseType != null)
{
var navigationMap = GetInverseNavigations(baseType);
if (navigationMap != null
&& navigationMap.TryGetValue(inverseNavigation.Navigation.Name, out var inverseTuple))
{
var referencingNavigationsWithAttribute = inverseTuple.References;
ambiguousInverse = FindAmbiguousInverse(
referencingNavigationWithAttribute.Item1,
referencingNavigationWithAttribute.Item2,
referencingNavigationsWithAttribute);
}
baseType = baseType.BaseType;
}

if (ambiguousInverse != null)
{
Dependencies.Logger.MultipleInversePropertiesSameTargetWarning(
Expand All @@ -373,13 +410,18 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
referencingNavigationWithAttribute.Item1, referencingNavigationWithAttribute.Item2.ClrType),
Tuple.Create(ambiguousInverse.Value.Item1, ambiguousInverse.Value.Item2.ClrType)
},
inverseNavigation.Key,
inverseNavigation.Navigation,
entityType.ClrType);
break;
}
}
}
}

foreach (var entityType in model.GetEntityTypes())
{
entityType.RemoveAnnotation(CoreAnnotationNames.InverseNavigations);
}
}

/// <summary>
Expand All @@ -395,19 +437,26 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
public static bool IsAmbiguous(
[NotNull] IConventionEntityType entityType, [NotNull] MemberInfo navigation, [NotNull] IConventionEntityType targetEntityType)
{
var inverseNavigations = GetInverseNavigations(targetEntityType);
if (inverseNavigations == null)
if (!Attribute.IsDefined(navigation, typeof(InversePropertyAttribute)))
{
return false;
}

foreach (var inverseNavigation in inverseNavigations)
while (targetEntityType != null)
{
if (inverseNavigation.Key.GetMemberType().IsAssignableFrom(entityType.ClrType)
&& IsAmbiguousInverse(navigation, entityType, inverseNavigation.Value))
var navigationMap = GetInverseNavigations(targetEntityType);
if (navigationMap != null)
{
return true;
foreach (var inverseNavigationTuple in navigationMap.Values)
{
if (inverseNavigationTuple.Navigation.GetMemberType().IsAssignableFrom(entityType.ClrType)
&& IsAmbiguousInverse(navigation, entityType, inverseNavigationTuple.References))
{
return true;
}
}
}
targetEntityType = targetEntityType.BaseType;
}

return false;
Expand All @@ -424,11 +473,6 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
IConventionEntityType entityType,
List<(MemberInfo, IConventionEntityType)> referencingNavigationsWithAttribute)
{
if (referencingNavigationsWithAttribute.Count == 1)
{
return null;
}

List<(MemberInfo, IConventionEntityType)> tuplesToRemove = null;
(MemberInfo, IConventionEntityType)? ambiguousTuple = null;
foreach (var referencingTuple in referencingNavigationsWithAttribute)
Expand Down Expand Up @@ -471,14 +515,19 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
var inverseNavigations = GetInverseNavigations(targetEntityType);
if (inverseNavigations == null)
{
inverseNavigations = new Dictionary<MemberInfo, List<(MemberInfo, IConventionEntityType)>>();
inverseNavigations = new Dictionary<string, (MemberInfo, List<(MemberInfo, IConventionEntityType)>)>();
SetInverseNavigations(targetEntityType.Builder, inverseNavigations);
}

if (!inverseNavigations.TryGetValue(inverseNavigation, out var referencingNavigationsWithAttribute))
List<(MemberInfo, IConventionEntityType)> referencingNavigationsWithAttribute;
if (!inverseNavigations.TryGetValue(inverseNavigation.Name, out var inverseTuple))
{
referencingNavigationsWithAttribute = new List<(MemberInfo, IConventionEntityType)>();
inverseNavigations[inverseNavigation] = referencingNavigationsWithAttribute;
inverseNavigations[inverseNavigation.Name] = (inverseNavigation, referencingNavigationsWithAttribute);
}
else
{
referencingNavigationsWithAttribute = inverseTuple.References;
}

foreach (var referencingTuple in referencingNavigationsWithAttribute)
Expand Down Expand Up @@ -508,11 +557,9 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
return;
}

foreach (var inverseNavigationPair in inverseNavigations)
foreach (var inverseNavigationPair in inverseNavigations.Values)
{
var inverseNavigation = inverseNavigationPair.Key;
var referencingNavigationsWithAttribute = inverseNavigationPair.Value;

var (inverseNavigation, referencingNavigationsWithAttribute) = inverseNavigationPair;
for (var index = 0; index < referencingNavigationsWithAttribute.Count; index++)
{
var referencingTuple = referencingNavigationsWithAttribute[index];
Expand All @@ -523,7 +570,7 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
referencingNavigationsWithAttribute.RemoveAt(index);
if (referencingNavigationsWithAttribute.Count == 0)
{
inverseNavigations.Remove(inverseNavigation);
inverseNavigations.Remove(inverseNavigation.Name);
}

if (referencingNavigationsWithAttribute.Count == 1)
Expand All @@ -533,8 +580,8 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
{
targetEntityType.Builder.HasRelationship(
otherEntityType,
(PropertyInfo)inverseNavigation,
(PropertyInfo)referencingNavigationsWithAttribute[0].Item1,
inverseNavigation,
referencingNavigationsWithAttribute[0].Item1,
fromDataAnnotation: true);
}
}
Expand All @@ -548,14 +595,14 @@ public virtual void ProcessModelFinalized(IConventionModelBuilder modelBuilder,
private static IConventionEntityType FindActualEntityType(IConventionEntityType entityType)
=> ((Model)entityType.Model).FindActualEntityType((EntityType)entityType);

private static Dictionary<MemberInfo, List<(MemberInfo, IConventionEntityType)>> GetInverseNavigations(
private static Dictionary<string, (MemberInfo Navigation, List<(MemberInfo, IConventionEntityType)> References)> GetInverseNavigations(
IConventionAnnotatable entityType)
=> entityType.FindAnnotation(CoreAnnotationNames.InverseNavigations)?.Value
as Dictionary<MemberInfo, List<(MemberInfo, IConventionEntityType)>>;
as Dictionary<string, (MemberInfo, List<(MemberInfo, IConventionEntityType)>)>;

private static void SetInverseNavigations(
IConventionAnnotatableBuilder entityTypeBuilder,
Dictionary<MemberInfo, List<(MemberInfo, IConventionEntityType)>> inverseNavigations)
Dictionary<string, (MemberInfo, List<(MemberInfo, IConventionEntityType)>)> inverseNavigations)
=> entityTypeBuilder.HasAnnotation(CoreAnnotationNames.InverseNavigations, inverseNavigations);
}
}
1 change: 0 additions & 1 deletion src/EFCore/Metadata/Conventions/ModelCleanupConvention.cs
View file
Expand Up @@ -92,7 +92,6 @@ private void RemoveModelBuildingAnnotations(IConventionModelBuilder modelBuilder
{
entityType.RemoveAnnotation(CoreAnnotationNames.AmbiguousNavigations);
entityType.RemoveAnnotation(CoreAnnotationNames.NavigationCandidates);
entityType.RemoveAnnotation(CoreAnnotationNames.InverseNavigations);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore/Properties/CoreStrings.Designer.cs
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/EFCore/Properties/CoreStrings.resx
View file
Expand Up @@ -782,7 +782,7 @@
<value>Cannot create a relationship between '{newPrincipalEntityType}.{newPrincipalNavigation}' and '{newDependentEntityType}.{newDependentNavigation}', because there already is a relationship between '{existingPrincipalEntityType}.{existingPrincipalNavigation}' and '{existingDependentEntityType}.{existingDependentNavigation}'. Navigation properties can only participate in a single relationship.</value>
</data>
<data name="WarningAsErrorTemplate" xml:space="preserve">
<value>Error generated for warning '{eventName}: {message}'. This exception can be suppressed or logged by passing event ID '{eventId}' to the 'ConfigureWarnings' method in 'DbContext.OnConfiguring' or 'AddDbContext'.</value>
<value>Error generated for warning '{eventName}': {message} This exception can be suppressed or logged by passing event ID '{eventId}' to the 'ConfigureWarnings' method in 'DbContext.OnConfiguring' or 'AddDbContext'.</value>
</data>
<data name="ContextDisposed" xml:space="preserve">
<value>Cannot access a disposed object. A common cause of this error is disposing a context that was resolved from dependency injection and then later trying to use the same context instance elsewhere in your application. This may occur if you are calling Dispose() on the context, or wrapping the context in a using statement. If you are using dependency injection, you should let the dependency injection container take care of disposing context instances.</value>
Expand Down
57 changes: 51 additions & 6 deletions test/EFCore.Specification.Tests/DataAnnotationTestBase.cs
View file
Expand Up @@ -1318,12 +1318,12 @@ public virtual void ForeignKeyAttribute_configures_relationships_when_inverse_on
Assert.Equal(nameof(PartialAnswerRepeating.AnswerId), fk1.Properties.Single().Name);
}

public abstract class Answer
private abstract class Answer
{
public int Id { get; set; }
}

public class PartialAnswerBase
private class PartialAnswerBase
{
public int Id { get; set; }
public int AnswerId { get; set; }
Expand All @@ -1332,20 +1332,20 @@ public class PartialAnswerBase
public virtual Answer Answer { get; set; }
}

public class PartialAnswer : PartialAnswerBase
private class PartialAnswer : PartialAnswerBase
{
}

public class PartialAnswerRepeating : PartialAnswerBase
private class PartialAnswerRepeating : PartialAnswerBase
{
}

public class MultipleAnswers : Answer
private class MultipleAnswers : Answer
{
public virtual ICollection<PartialAnswer> Answers { get; set; }
}

public class MultipleAnswersRepeating : Answer
private class MultipleAnswersRepeating : Answer
{
public virtual ICollection<PartialAnswerRepeating> Answers { get; set; }
}
Expand Down Expand Up @@ -1841,6 +1841,51 @@ protected class SpecialPost7698 : Post7698
public Blog7698 BlogInverseNav { get; set; }
}

[ConditionalFact]
public virtual void InversePropertyAttribute_pointing_to_same_nav_on_base_causes_ambiguity()
{
var modelBuilder = CreateModelBuilder();
modelBuilder.Entity<MultipleAnswersInverse>();
modelBuilder.Entity<MultipleAnswersRepeatingInverse>();

Assert.Equal(
CoreStrings.WarningAsErrorTemplate(
CoreEventId.MultipleInversePropertiesSameTargetWarning,
CoreResources.LogMultipleInversePropertiesSameTarget(new TestLogger<TestLoggingDefinitions>())
.GenerateMessage(
$"{nameof(MultipleAnswersRepeatingInverse)}.{nameof(MultipleAnswersRepeatingInverse.Answers)},"
+ $" {nameof(MultipleAnswersInverse)}.{nameof(MultipleAnswersInverse.Answers)}",
nameof(PartialAnswerInverse.Answer)),
"CoreEventId.MultipleInversePropertiesSameTargetWarning"),
Assert.Throws<InvalidOperationException>(() => modelBuilder.FinalizeModel()).Message);
}

private class PartialAnswerInverse
{
public int Id { get; set; }
public int AnswerId { get; set; }
public virtual AnswerBaseInverse Answer { get; set; }
}

private class PartialAnswerRepeatingInverse : PartialAnswerInverse { }

private abstract class AnswerBaseInverse
{
public int Id { get; set; }
}

private class MultipleAnswersInverse : AnswerBaseInverse
{
[InverseProperty("Answer")]
public virtual ICollection<PartialAnswerInverse> Answers { get; set; }
}

private class MultipleAnswersRepeatingInverse : AnswerBaseInverse
{
[InverseProperty("Answer")]
public virtual IEnumerable<PartialAnswerRepeatingInverse> Answers { get; set; }
}

[ConditionalFact]
public virtual void ForeignKeyAttribute_creates_two_relationships_if_applied_on_property_on_both_side()
{
Expand Down

0 comments on commit 1c5e308

Please sign in to comment.