Skip to content

Commit

Permalink
Throw exception for cycles in auto included navigations
Browse files Browse the repository at this point in the history 
Resolves #22568
  • Loading branch information
@smitpatel
smitpatel committed Sep 19, 2020
1 parent ac2b37f commit 9a92538
Show file tree
Hide file tree
Showing 5 changed files with 343 additions and 20 deletions.
8 changes: 8 additions & 0 deletions 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.

3 changes: 3 additions & 0 deletions src/EFCore/Properties/CoreStrings.resx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@
<data name="ArgumentPropertyNull" xml:space="preserve">
<value>The property '{property}' of the argument '{argument}' cannot be null.</value>
</data>
<data name="AutoIncludeNavigationCycle" xml:space="preserve">
<value>Cycle detected while auto-including navigations: {cycleNavigations}. To fix this issue, either don't configure at least one navigation in the cycle as auto included in `OnModelCreating` or call 'IgnoreAutoInclude' method on the query.</value>
</data>
<data name="BackingFieldOnIndexer" xml:space="preserve">
<value>Cannot set backing field '{field}' for the indexer property '{entityType}.{property}'. Ensure no backing fields are specified for indexer properties.</value>
</data>
Expand Down
43 changes: 38 additions & 5 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ protected Expression ExpandNavigation(
}

var ownedEntityReference = new EntityReference(targetType);
_navigationExpandingExpressionVisitor.PopulateEagerLoadedNavigations(ownedEntityReference.IncludePaths);
ownedEntityReference.MarkAsOptional();
if (entityReference.IncludePaths.TryGetValue(navigation, out var includePath))
{
Expand Down Expand Up @@ -482,6 +483,7 @@ private sealed class IncludeExpandingExpressionVisitor : ExpandingExpressionVisi
typeof(IncludeExpandingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(FetchJoinEntity));

private readonly bool _queryStateManager;
private readonly bool _ignoreAutoIncludes;
private readonly IDiagnosticsLogger<DbLoggerCategory.Query> _logger;

public IncludeExpandingExpressionVisitor(
Expand All @@ -494,6 +496,7 @@ public IncludeExpandingExpressionVisitor(
== QueryTrackingBehavior.TrackAll
|| navigationExpandingExpressionVisitor._queryCompilationContext.QueryTrackingBehavior
== QueryTrackingBehavior.NoTrackingWithIdentityResolution;
_ignoreAutoIncludes = navigationExpandingExpressionVisitor._queryCompilationContext.IgnoreAutoIncludes;
}

protected override Expression VisitExtension(Expression extensionExpression)
Expand Down Expand Up @@ -653,7 +656,7 @@ private Expression ExpandInclude(Expression root, EntityReference entityReferenc
VerifyNoCycles(entityReference.IncludePaths);
}

return ExpandIncludesHelper(root, entityReference);
return ExpandIncludesHelper(root, entityReference, previousNavigation: null);
}

private void VerifyNoCycles(IncludeTreeNode includeTreeNode)
Expand All @@ -673,13 +676,19 @@ private void VerifyNoCycles(IncludeTreeNode includeTreeNode)
}
}

private Expression ExpandIncludesHelper(Expression root, EntityReference entityReference)
private Expression ExpandIncludesHelper(Expression root, EntityReference entityReference, INavigationBase previousNavigation)
{
var result = root;
var convertedRoot = root;
foreach (var kvp in entityReference.IncludePaths)
{
var navigationBase = kvp.Key;
if (!navigationBase.IsCollection
&& previousNavigation?.Inverse == navigationBase)
{
continue;
}

var converted = false;
if (entityReference.EntityType != navigationBase.DeclaringEntityType
&& entityReference.EntityType.IsAssignableFrom(navigationBase.DeclaringEntityType))
Expand All @@ -694,19 +703,43 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR
ISkipNavigation skipNavigation => ExpandSkipNavigation(convertedRoot, entityReference, skipNavigation, converted),
_ => throw new InvalidOperationException(CoreStrings.UnhandledNavigationBase(navigationBase.GetType())),
};

_logger.NavigationBaseIncluded(navigationBase);

// Collection will expand it's includes when reducing the navigationExpansionExpression
if (!navigationBase.IsCollection)
{
var innerEntityReference = UnwrapEntityReference(included);

included = ExpandIncludesHelper(included, innerEntityReference);
included = ExpandIncludesHelper(included, innerEntityReference, navigationBase);
}

if (included is MaterializeCollectionNavigationExpression materializeCollectionNavigation)
else
{
var materializeCollectionNavigation = (MaterializeCollectionNavigationExpression)included;
var subquery = materializeCollectionNavigation.Subquery;
if (!_ignoreAutoIncludes
&& navigationBase is INavigation
&& navigationBase.Inverse != null
&& subquery is MethodCallExpression subqueryMethodCallExpression
&& subqueryMethodCallExpression.Method.IsGenericMethod)
{
EntityReference innerEntityReference = null;
if (subqueryMethodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.Where
&& subqueryMethodCallExpression.Arguments[0] is NavigationExpansionExpression navigationExpansionExpression)
{
innerEntityReference = UnwrapEntityReference(navigationExpansionExpression.CurrentTree);
}
else if (subqueryMethodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.AsQueryable)
{
innerEntityReference = UnwrapEntityReference(subqueryMethodCallExpression.Arguments[0]);
}

if (innerEntityReference != null)
{
innerEntityReference.IncludePaths.Remove(navigationBase.Inverse);
}
}

var filterExpression = entityReference.IncludePaths[navigationBase].FilterExpression;
if (_queryStateManager
&& navigationBase is ISkipNavigation
Expand Down
71 changes: 60 additions & 11 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ private static readonly PropertyInfo _queryContextContextPropertyInfo
.GetTypeInfo()
.GetDeclaredProperty(nameof(QueryContext.Context));

private static readonly IDictionary<MethodInfo, MethodInfo> _predicateLessMethodInfo = new Dictionary<MethodInfo, MethodInfo>
private static readonly Dictionary<MethodInfo, MethodInfo> _predicateLessMethodInfo = new Dictionary<MethodInfo, MethodInfo>
{
{ QueryableMethods.FirstWithPredicate, QueryableMethods.FirstWithoutPredicate },
{ QueryableMethods.FirstOrDefaultWithPredicate, QueryableMethods.FirstOrDefaultWithoutPredicate },
Expand Down Expand Up @@ -59,8 +59,9 @@ private static readonly PropertyInfo _queryContextContextPropertyInfo
private readonly ReducingExpressionVisitor _reducingExpressionVisitor;
private readonly EntityReferenceOptionalMarkingExpressionVisitor _entityReferenceOptionalMarkingExpressionVisitor;
private readonly RemoveRedundantNavigationComparisonExpressionVisitor _removeRedundantNavigationComparisonExpressionVisitor;
private readonly ISet<string> _parameterNames = new HashSet<string>();
private readonly HashSet<string> _parameterNames = new HashSet<string>();
private readonly ParameterExtractingExpressionVisitor _parameterExtractingExpressionVisitor;
private readonly HashSet<IEntityType> _nonCyclicAutoIncludeEntityTypes;

private readonly Dictionary<IEntityType, LambdaExpression> _parameterizedQueryFilterPredicateCache
= new Dictionary<IEntityType, LambdaExpression>();
Expand Down Expand Up @@ -95,6 +96,11 @@ public NavigationExpandingExpressionVisitor(
_queryCompilationContext.Logger,
parameterize: false,
generateContextAccessors: true);

if (!_queryCompilationContext.IgnoreAutoIncludes)
{
_nonCyclicAutoIncludeEntityTypes = new HashSet<IEntityType>();
}
}

/// <summary>
Expand Down Expand Up @@ -1707,13 +1713,14 @@ private string GetParameterName(string prefix)
private void PopulateEagerLoadedNavigations(IncludeTreeNode includeTreeNode)
{
var entityType = includeTreeNode.EntityType;
var outboundNavigations
= entityType.GetNavigations()
.Cast<INavigationBase>()
.Concat(entityType.GetSkipNavigations())
.Concat(entityType.GetDerivedNavigations())
.Concat(entityType.GetDerivedSkipNavigations())
.Where(n => n.IsEagerLoaded);

if (!_queryCompilationContext.IgnoreAutoIncludes
&& !_nonCyclicAutoIncludeEntityTypes.Contains(entityType))
{
VerifyNoAutoIncludeCycles(entityType, new HashSet<IEntityType>(), new List<INavigationBase>());
}

var outboundNavigations = GetOutgoingEagerLoadedNavigations(entityType);

if (_queryCompilationContext.IgnoreAutoIncludes)
{
Expand All @@ -1722,11 +1729,53 @@ var outboundNavigations

foreach (var navigation in outboundNavigations)
{
var addedIncludeTreeNode = includeTreeNode.AddNavigation(navigation);
PopulateEagerLoadedNavigations(addedIncludeTreeNode);
includeTreeNode.AddNavigation(navigation);
}
}

private void VerifyNoAutoIncludeCycles(
IEntityType entityType, HashSet<IEntityType> visitedEntityTypes, List<INavigationBase> navigationChain)
{
if (_nonCyclicAutoIncludeEntityTypes.Contains(entityType))
{
return;
}

if (!visitedEntityTypes.Add(entityType))
{
throw new InvalidOperationException(CoreStrings.AutoIncludeNavigationCycle(
navigationChain.Select(e => $"'{e.DeclaringEntityType.ShortName()}.{e.Name}'").Join()));
}

var autoIncludedNavigations = GetOutgoingEagerLoadedNavigations(entityType)
.Where(n => !(n is INavigation navigation && navigation.ForeignKey.IsOwnership));

foreach (var navigationBase in autoIncludedNavigations)
{
if (navigationChain.Count > 0
&& navigationChain[^1].Inverse == navigationBase
&& navigationBase is INavigation)
{
continue;
}

navigationChain.Add(navigationBase);
VerifyNoAutoIncludeCycles(navigationBase.TargetEntityType, visitedEntityTypes, navigationChain);
navigationChain.Remove(navigationBase);
}

_nonCyclicAutoIncludeEntityTypes.Add(entityType);
visitedEntityTypes.Remove(entityType);
}

private static IEnumerable<INavigationBase> GetOutgoingEagerLoadedNavigations(IEntityType entityType)
=> entityType.GetNavigations()
.Cast<INavigationBase>()
.Concat(entityType.GetSkipNavigations())
.Concat(entityType.GetDerivedNavigations())
.Concat(entityType.GetDerivedSkipNavigations())
.Where(n => n.IsEagerLoaded);

private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Expression expression)
{
switch (expression)
Expand Down
Loading

0 comments on commit 9a92538

Please sign in to comment.