Skip to content

Commit

Permalink
Query: Expand navigations in Selector/Include in one pass
Browse files Browse the repository at this point in the history 
Issue: Earlier we expanded navigation in selector and include in separate phase. This causes issue because if the latter visitor expands a reference navigation then former visitor's collection expansions have incorrect references.
Fix: Fix is to make include expansion part of selector expansion as next phase. So by the time we apply include, the correlation predicate in collection hasn't been converted to actual reference. So when it gets converted, it takes correct reference.

Also apply pending selector inside lambda expression since it is a subquery. This caused issue when subquery has a projection which has navigation to expand, which we never visited.

Resolves #18127
Resolves #18090
Resolves #17852
Resolves #17756
  • Loading branch information
@smitpatel
smitpatel committed Oct 28, 2019
1 parent db1b4f7 commit 4cda7b2
Show file tree
Hide file tree
Showing 9 changed files with 300 additions and 218 deletions.
59 changes: 22 additions & 37 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ private class ExpandingExpressionVisitor : ExpressionVisitor
_source = source;
}

public Expression Expand(Expression expression, bool applyIncludes = false)
{
expression = Visit(expression);
if (applyIncludes)
{
expression = new IncludeExpandingExpressionVisitor(_navigationExpandingExpressionVisitor, _source)
.Visit(expression);
}

return expression;
}

protected override Expression VisitExtension(Expression expression)
{
switch (expression)
Expand Down Expand Up @@ -303,11 +315,10 @@ private class IncludeExpandingExpressionVisitor : ExpandingExpressionVisitor

public IncludeExpandingExpressionVisitor(
NavigationExpandingExpressionVisitor navigationExpandingExpressionVisitor,
NavigationExpansionExpression source,
bool tracking)
NavigationExpansionExpression source)
: base(navigationExpandingExpressionVisitor, source)
{
_isTracking = tracking;
_isTracking = navigationExpandingExpressionVisitor._queryCompilationContext.IsTracking;
}

public override Expression Visit(Expression expression)
Expand Down Expand Up @@ -501,42 +512,16 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR
}
}

private class IncludeApplyingExpressionVisitor : ExpressionVisitor
{
private readonly NavigationExpandingExpressionVisitor _visitor;
private readonly bool _isTracking;

public IncludeApplyingExpressionVisitor(NavigationExpandingExpressionVisitor visitor, bool tracking)
{
_visitor = visitor;
_isTracking = tracking;
}

public override Expression Visit(Expression expression)
{
if (expression is NavigationExpansionExpression navigationExpansionExpression)
{
var innerVisitor = new IncludeExpandingExpressionVisitor(_visitor, navigationExpansionExpression, _isTracking);
var pendingSelector = innerVisitor.Visit(navigationExpansionExpression.PendingSelector);
pendingSelector = _visitor.Visit(pendingSelector);
pendingSelector = Visit(pendingSelector);

navigationExpansionExpression.ApplySelector(pendingSelector);

return navigationExpansionExpression;
}

return base.Visit(expression);
}
}

private class PendingSelectorExpandingExpressionVisitor : ExpressionVisitor
{
private readonly NavigationExpandingExpressionVisitor _visitor;
private readonly bool _applyIncludes;

public PendingSelectorExpandingExpressionVisitor(NavigationExpandingExpressionVisitor visitor)
public PendingSelectorExpandingExpressionVisitor(
NavigationExpandingExpressionVisitor visitor, bool applyIncludes = false)
{
_visitor = visitor;
_applyIncludes = applyIncludes;
}

public override Expression Visit(Expression expression)
Expand All @@ -545,11 +530,11 @@ public override Expression Visit(Expression expression)
{
_visitor.ApplyPendingOrderings(navigationExpansionExpression);

var pendingSelector = _visitor.ExpandNavigationsInExpression(
navigationExpansionExpression, navigationExpansionExpression.PendingSelector);

var pendingSelector = new ExpandingExpressionVisitor(_visitor, navigationExpansionExpression)
.Expand(navigationExpansionExpression.PendingSelector, _applyIncludes);
pendingSelector = _visitor._subqueryMemberPushdownExpressionVisitor.Visit(pendingSelector);
pendingSelector = _visitor.Visit(pendingSelector);
pendingSelector = Visit(pendingSelector);

navigationExpansionExpression.ApplySelector(pendingSelector);

return navigationExpansionExpression;
Expand Down
37 changes: 13 additions & 24 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ private string GetParameterName(string prefix)

public virtual Expression Expand(Expression query)
{
var result = ExpandAndReduce(query, applyInclude: true);
var result = ExpandAndReduce(query, applyIncludes: true);

var dbContextOnQueryContextPropertyAccess =
Expression.Convert(
Expand Down Expand Up @@ -96,15 +96,10 @@ public virtual Expression Expand(Expression query)
return result;
}

private Expression ExpandAndReduce(Expression query, bool applyInclude)
private Expression ExpandAndReduce(Expression query, bool applyIncludes)
{
var result = Visit(query);
result = _pendingSelectorExpandingExpressionVisitor.Visit(result);
if (applyInclude)
{
result = new IncludeApplyingExpressionVisitor(this, _queryCompilationContext.IsTracking).Visit(result);
}

result = new PendingSelectorExpandingExpressionVisitor(this, applyIncludes: applyIncludes).Visit(result);
result = Reduce(result);

return result;
Expand All @@ -131,7 +126,7 @@ protected override Expression VisitConstant(ConstantExpression constantExpressio

navigationExpansionExpression = (NavigationExpansionExpression)Visit(processedDefiningQueryBody);

var expanded = ExpandAndReduce(navigationExpansionExpression, applyInclude: false);
var expanded = ExpandAndReduce(navigationExpansionExpression, applyIncludes: false);
navigationExpansionExpression = CreateNavigationExpansionExpression(expanded, entityType);
}
else
Expand Down Expand Up @@ -178,7 +173,7 @@ private Expression ApplyQueryFilter(NavigationExpansionExpression navigationExpa
QueryableMethods.Where.MakeGenericMethod(rootEntityType.ClrType),
NullAsyncQueryProvider.Instance.CreateEntityQueryableExpression(rootEntityType.ClrType),
filterPredicate);
var rewrittenFilterWrapper = (MethodCallExpression)_entityEqualityRewritingExpressionVisitor.Rewrite(filterWrapper);
var rewrittenFilterWrapper = (MethodCallExpression)_entityEqualityRewritingExpressionVisitor.Rewrite(filterWrapper);
filterPredicate = rewrittenFilterWrapper.Arguments[1].UnwrapLambdaFromQuote();

_parameterizedQueryFilterPredicateCache[rootEntityType] = filterPredicate;
Expand Down Expand Up @@ -950,9 +945,8 @@ private bool CompareIncludes(Expression outer, Expression inner)
else
{
source = (NavigationExpansionExpression)ProcessSelect(source, elementSelector);
source = (NavigationExpansionExpression)_pendingSelectorExpandingExpressionVisitor.Visit(source);
source = (NavigationExpansionExpression)new IncludeApplyingExpressionVisitor(
this, _queryCompilationContext.IsTracking).Visit(source);
source = (NavigationExpansionExpression)new PendingSelectorExpandingExpressionVisitor(this, applyIncludes: true)
.Visit(source);
keySelector = GenerateLambda(keySelectorBody, source.CurrentParameter);
elementSelector = GenerateLambda(source.PendingSelector, source.CurrentParameter);
if (resultSelector == null)
Expand Down Expand Up @@ -1336,7 +1330,6 @@ private Expression SnapshotExpression(Expression selector)
&& selector.ReturnType.GetGenericTypeDefinition() == typeof(IGrouping<,>)))
{
var selectorLambda = GenerateLambda(ExpandNavigationsInLambdaExpression(source, selector), source.CurrentParameter);

var newSource = Expression.Call(
QueryableMethods.Select.MakeGenericMethod(source.SourceElementType, selectorLambda.ReturnType),
source.Source,
Expand Down Expand Up @@ -1420,15 +1413,6 @@ private Expression Reduce(Expression source)
return _reducingExpressionVisitor.Visit(source);
}

private Expression ExpandNavigationsInExpression(NavigationExpansionExpression source, Expression expression)
{
expression = new ExpandingExpressionVisitor(this, source).Visit(expression);
expression = _subqueryMemberPushdownExpressionVisitor.Visit(expression);
expression = Visit(expression);

return expression;
}

private Expression ExpandNavigationsInLambdaExpression(
NavigationExpansionExpression source,
LambdaExpression lambdaExpression)
Expand All @@ -1438,7 +1422,12 @@ private Expression ExpandNavigationsInExpression(NavigationExpansionExpression s
source.PendingSelector,
lambdaExpression.Body);

return ExpandNavigationsInExpression(source, lambdaBody);
lambdaBody = new ExpandingExpressionVisitor(this, source).Visit(lambdaBody);
lambdaBody = _subqueryMemberPushdownExpressionVisitor.Visit(lambdaBody);
lambdaBody = Visit(lambdaBody);
lambdaBody = _pendingSelectorExpandingExpressionVisitor.Visit(lambdaBody);

return lambdaBody;
}

private class Parameters : IParameterValues
Expand Down
Loading

0 comments on commit 4cda7b2

Please sign in to comment.