Skip to content

Commit

Permalink
Fix to #17531 - Query: incorrect results for queries with optional na…
Browse files Browse the repository at this point in the history 
…vigation followed by collection navigation with skip/take/distinct

Problem was that when expanding collection navigations we convert them into subqueries with a correlation predicate being outerKey == innerKey. For relational, most of those queries would later be converted into joins, however for some complex cases e.g. with Skip/Take (and also on InMemory) the query would stay in the form of subquery with correlation predicate. Then, null semantics kicks in and converts the correlation predicate to a form that returns true when both keys are null. This is incorrect in the context of chaining navigations - if the parent entity is null then it should never return any children.

Fix is to add null check to the correlation predicate during nav rewrite, like so: outerKey != null && outerKey == innerKey
Additionally, when trying to convert those subqueries into joins we need to account for a new pattern and remove the null check, since its irrelevant when it comes to join key comparison on relational
  • Loading branch information
@maumar
maumar committed Sep 10, 2019
1 parent 59465e2 commit 90fc979
Show file tree
Hide file tree
Showing 18 changed files with 284 additions and 183 deletions.
11 changes: 10 additions & 1 deletion src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -933,8 +933,17 @@ private Expression Expand(Expression source, MemberIdentity member)
: foreignKey.Properties,
makeNullable);

var correlationPredicate = _expressionTranslator.Translate(Expression.Equal(outerKey, innerKey));
var outerKeyFirstProperty = outerKey is NewExpression newExpression
? ((UnaryExpression)((NewArrayExpression)newExpression.Arguments[0]).Expressions[0]).Operand
: outerKey;

var predicate = outerKeyFirstProperty.Type.IsNullableType()
? Expression.AndAlso(
Expression.NotEqual(outerKeyFirstProperty, Expression.Constant(null, outerKeyFirstProperty.Type)),
Expression.Equal(outerKey, innerKey))
: Expression.Equal(outerKey, innerKey);

var correlationPredicate = _expressionTranslator.Translate(predicate);
innerQueryExpression.ServerQueryExpression = Expression.Call(
InMemoryLinqOperatorProvider.Where.MakeGenericMethod(innerQueryExpression.CurrentParameter.Type),
innerQueryExpression.ServerQueryExpression,
Expand Down
11 changes: 10 additions & 1 deletion src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1122,8 +1122,17 @@ private Expression Expand(Expression source, MemberIdentity member)
: foreignKey.Properties,
makeNullable);

var correlationPredicate = _sqlTranslator.Translate(Expression.Equal(outerKey, innerKey));
var outerKeyFirstProperty = outerKey is NewExpression newExpression
? ((UnaryExpression)((NewArrayExpression)newExpression.Arguments[0]).Expressions[0]).Operand
: outerKey;

var predicate = outerKeyFirstProperty.Type.IsNullableType()
? Expression.AndAlso(
Expression.NotEqual(outerKeyFirstProperty, Expression.Constant(null, outerKeyFirstProperty.Type)),
Expression.Equal(outerKey, innerKey))
: Expression.Equal(outerKey, innerKey);

var correlationPredicate = _sqlTranslator.Translate(predicate);
innerSelectExpression.ApplyPredicate(correlationPredicate);

return innerShapedQuery;
Expand Down
92 changes: 75 additions & 17 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,13 @@ private SqlExpression TryExtractJoinKey(SelectExpression selectExpression)
&& selectExpression.Offset == null
&& selectExpression.Predicate != null)
{
var joinPredicate = TryExtractJoinKey(selectExpression, selectExpression.Predicate, out var predicate);
var columnExpressions = new List<ColumnExpression>();
var joinPredicate = TryExtractJoinKey(selectExpression, selectExpression.Predicate, columnExpressions, out var predicate);
if (joinPredicate != null)
{
joinPredicate = RemoveRedundantNullChecks(joinPredicate, columnExpressions);
}

selectExpression.Predicate = predicate;

return joinPredicate;
Expand All @@ -946,11 +952,11 @@ private SqlExpression TryExtractJoinKey(SelectExpression selectExpression)
}

private SqlExpression TryExtractJoinKey(
SelectExpression selectExpression, SqlExpression predicate, out SqlExpression updatedPredicate)
SelectExpression selectExpression, SqlExpression predicate, List<ColumnExpression> columnExpressions, out SqlExpression updatedPredicate)
{
if (predicate is SqlBinaryExpression sqlBinaryExpression)
{
var joinPredicate = ValidateKeyComparison(selectExpression, sqlBinaryExpression);
var joinPredicate = ValidateKeyComparison(selectExpression, sqlBinaryExpression, columnExpressions);
if (joinPredicate != null)
{
updatedPredicate = null;
Expand All @@ -960,29 +966,28 @@ private SqlExpression TryExtractJoinKey(

if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso)
{
static SqlExpression combineNonNullExpressions(SqlExpression left, SqlExpression right)
{
return left != null
? right != null
? new SqlBinaryExpression(ExpressionType.AndAlso, left, right, left.Type, left.TypeMapping)
: left
: right;
}

var leftJoinKey = TryExtractJoinKey(selectExpression, sqlBinaryExpression.Left, out var leftPredicate);
var rightJoinKey = TryExtractJoinKey(selectExpression, sqlBinaryExpression.Right, out var rightPredicate);
var leftJoinKey = TryExtractJoinKey(selectExpression, sqlBinaryExpression.Left, columnExpressions, out var leftPredicate);
var rightJoinKey = TryExtractJoinKey(selectExpression, sqlBinaryExpression.Right, columnExpressions, out var rightPredicate);

updatedPredicate = combineNonNullExpressions(leftPredicate, rightPredicate);
updatedPredicate = CombineNonNullExpressions(leftPredicate, rightPredicate);

return combineNonNullExpressions(leftJoinKey, rightJoinKey);
return CombineNonNullExpressions(leftJoinKey, rightJoinKey);
}
}

updatedPredicate = predicate;

return null;
}

private SqlBinaryExpression ValidateKeyComparison(SelectExpression inner, SqlBinaryExpression sqlBinaryExpression)
private static SqlExpression CombineNonNullExpressions(SqlExpression left, SqlExpression right)
=> left != null
? right != null
? new SqlBinaryExpression(ExpressionType.AndAlso, left, right, left.Type, left.TypeMapping)
: left
: right;

private SqlBinaryExpression ValidateKeyComparison(SelectExpression inner, SqlBinaryExpression sqlBinaryExpression, List<ColumnExpression> columnExpressions)
{
if (sqlBinaryExpression.OperatorType == ExpressionType.Equal)
{
Expand All @@ -992,22 +997,75 @@ private SqlBinaryExpression ValidateKeyComparison(SelectExpression inner, SqlBin
if (ContainsTableReference(leftColumn.Table)
&& inner.ContainsTableReference(rightColumn.Table))
{
columnExpressions.Add(leftColumn);
columnExpressions.Add(rightColumn);

return sqlBinaryExpression;
}

if (ContainsTableReference(rightColumn.Table)
&& inner.ContainsTableReference(leftColumn.Table))
{
columnExpressions.Add(leftColumn);
columnExpressions.Add(rightColumn);

return sqlBinaryExpression.Update(
sqlBinaryExpression.Right,
sqlBinaryExpression.Left);
}
}
}

// null checks are considered part of join key
if (sqlBinaryExpression.OperatorType == ExpressionType.NotEqual)
{
if (sqlBinaryExpression.Left is ColumnExpression leftNullCheckColumn
&& (ContainsTableReference(leftNullCheckColumn.Table) || inner.ContainsTableReference(leftNullCheckColumn.Table))
&& sqlBinaryExpression.Right is SqlConstantExpression rightConstant
&& rightConstant.Value == null)
{
return sqlBinaryExpression;
}

if (sqlBinaryExpression.Right is ColumnExpression rightNullCheckColumn
&& (ContainsTableReference(rightNullCheckColumn.Table) || inner.ContainsTableReference(rightNullCheckColumn.Table))
&& sqlBinaryExpression.Left is SqlConstantExpression leftConstant
&& leftConstant.Value == null)
{
return sqlBinaryExpression.Update(
sqlBinaryExpression.Right,
sqlBinaryExpression.Left);
}
}

return null;
}

private SqlExpression RemoveRedundantNullChecks(SqlExpression predicate, List<ColumnExpression> columnExpressions)
{
if (predicate is SqlBinaryExpression sqlBinaryExpression)
{
if (sqlBinaryExpression.OperatorType == ExpressionType.NotEqual
&& sqlBinaryExpression.Left is ColumnExpression leftColumn
&& columnExpressions.Contains(leftColumn)
&& sqlBinaryExpression.Right is SqlConstantExpression sqlConstantExpression
&& sqlConstantExpression.Value == null)
{
return null;
}

if (sqlBinaryExpression.OperatorType == ExpressionType.AndAlso)
{
var leftPredicate = RemoveRedundantNullChecks(sqlBinaryExpression.Left, columnExpressions);
var rightPredicate = RemoveRedundantNullChecks(sqlBinaryExpression.Right, columnExpressions);

return CombineNonNullExpressions(leftPredicate, rightPredicate);
}
}

return predicate;
}

private bool ContainsTableReference(TableExpressionBase table)
=> Tables.Any(te => ReferenceEquals(te is JoinExpressionBase jeb ? jeb.Table : te, table));

Expand Down
41 changes: 20 additions & 21 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,11 @@ protected Expression ExpandNavigation(
{
// This is FirstOrDefault ending so we need to push down properties.
var temporaryParameter = Expression.Parameter(root.Type);
var temporaryKey = CreateKeyAccessExpression(
temporaryParameter,
var temporaryKey = temporaryParameter.CreateKeyAccessExpression(
navigation.IsDependentToPrincipal()
? navigation.ForeignKey.Properties
: navigation.ForeignKey.PrincipalKey.Properties);
: navigation.ForeignKey.PrincipalKey.Properties,
makeNullable: true);
var newSelector = ReplacingExpressionVisitor.Replace(
temporaryParameter,
innerNavigationExpansionExpression.PendingSelector,
Expand All @@ -193,18 +193,18 @@ protected Expression ExpandNavigation(
}
else
{
outerKey = CreateKeyAccessExpression(
root,
outerKey = root.CreateKeyAccessExpression(
navigation.IsDependentToPrincipal()
? navigation.ForeignKey.Properties
: navigation.ForeignKey.PrincipalKey.Properties);
: navigation.ForeignKey.PrincipalKey.Properties,
makeNullable: true);
}

var innerKey = CreateKeyAccessExpression(
innerParameter,
var innerKey = innerParameter.CreateKeyAccessExpression(
navigation.IsDependentToPrincipal()
? navigation.ForeignKey.PrincipalKey.Properties
: navigation.ForeignKey.Properties);
: navigation.ForeignKey.Properties,
makeNullable: true);

if (outerKey.Type != innerKey.Type)
{
Expand All @@ -221,14 +221,24 @@ protected Expression ExpandNavigation(

if (navigation.IsCollection())
{
var outerKeyFirstProperty = outerKey is NewExpression newExpression
? ((UnaryExpression)((NewArrayExpression)newExpression.Arguments[0]).Expressions[0]).Operand
: outerKey;

// This is intentionally deferred to be applied to innerSource.Source
// Since outerKey's reference could change if a reference navigation is expanded afterwards
var predicateBody = outerKeyFirstProperty.Type.IsNullableType()
? Expression.AndAlso(
Expression.NotEqual(outerKeyFirstProperty, Expression.Constant(null, outerKeyFirstProperty.Type)),
Expression.Equal(outerKey, innerKey))
: Expression.Equal(outerKey, innerKey);

var subquery = Expression.Call(
QueryableMethods.Where.MakeGenericMethod(innerSoureSequenceType),
innerSource,
Expression.Quote(
Expression.Lambda(
Expression.Equal(outerKey, innerKey), innerParameter)));
predicateBody, innerParameter)));

return new MaterializeCollectionNavigationExpression(subquery, navigation);
}
Expand Down Expand Up @@ -286,17 +296,6 @@ protected Expression ExpandNavigation(

return innerSource.PendingSelector;
}

private static Expression CreateKeyAccessExpression(Expression target, IReadOnlyList<IProperty> properties)
=> properties.Count == 1
? target.CreateEFPropertyExpression(properties[0])
: Expression.New(
AnonymousObject.AnonymousObjectCtor,
Expression.NewArrayInit(
typeof(object),
properties
.Select(p => Expression.Convert(target.CreateEFPropertyExpression(p), typeof(object)))
.ToArray()));
}

private class IncludeExpandingExpressionVisitor : ExpandingExpressionVisitor
Expand Down
54 changes: 0 additions & 54 deletions test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,64 +15,10 @@ public ComplexNavigationsQueryInMemoryTest(ComplexNavigationsQueryInMemoryFixtur
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_with_nested_navigations_and_explicit_DefaultIfEmpty_followed_by_Select_required_navigation_using_different_navs(bool isAsync)
{
return base.SelectMany_with_nested_navigations_and_explicit_DefaultIfEmpty_followed_by_Select_required_navigation_using_different_navs(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_with_nested_navigation_and_explicit_DefaultIfEmpty(bool isAsync)
{
return base.SelectMany_with_nested_navigation_and_explicit_DefaultIfEmpty(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_with_nested_navigation_filter_and_explicit_DefaultIfEmpty(bool isAsync)
{
return base.SelectMany_with_nested_navigation_filter_and_explicit_DefaultIfEmpty(isAsync);
}

[ConditionalTheory(Skip = "issue #17386")]
public override Task Complex_query_with_optional_navigations_and_client_side_evaluation(bool isAsync)
{
return base.Complex_query_with_optional_navigations_and_client_side_evaluation(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_collection_navigation_nested(bool isAsync)
{
return base.Project_collection_navigation_nested(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_collection_navigation_nested_anonymous(bool isAsync)
{
return base.Project_collection_navigation_nested_anonymous(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_collection_navigation_using_ef_property(bool isAsync)
{
return base.Project_collection_navigation_using_ef_property(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_navigation_and_collection(bool isAsync)
{
return base.Project_navigation_and_collection(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_nested_navigation_property_optional_and_projection(bool isAsync)
{
return base.SelectMany_nested_navigation_property_optional_and_projection(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_nested_navigation_property_required(bool isAsync)
{
return base.SelectMany_nested_navigation_property_required(isAsync);
}
}
}
42 changes: 0 additions & 42 deletions test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,48 +24,6 @@ public override Task Complex_query_with_optional_navigations_and_client_side_eva
return base.Complex_query_with_optional_navigations_and_client_side_evaluation(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_with_nested_navigations_and_explicit_DefaultIfEmpty_followed_by_Select_required_navigation_using_different_navs(bool isAsync)
{
return base.SelectMany_with_nested_navigations_and_explicit_DefaultIfEmpty_followed_by_Select_required_navigation_using_different_navs(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_with_nested_navigation_filter_and_explicit_DefaultIfEmpty(bool isAsync)
{
return base.SelectMany_with_nested_navigation_filter_and_explicit_DefaultIfEmpty(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_collection_navigation_nested(bool isAsync)
{
return base.Project_collection_navigation_nested(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_collection_navigation_nested_anonymous(bool isAsync)
{
return base.Project_collection_navigation_nested_anonymous(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_collection_navigation_using_ef_property(bool isAsync)
{
return base.Project_collection_navigation_using_ef_property(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task Project_navigation_and_collection(bool isAsync)
{
return base.Project_navigation_and_collection(isAsync);
}

[ConditionalTheory(Skip = "issue #17531")]
public override Task SelectMany_nested_navigation_property_optional_and_projection(bool isAsync)
{
return base.SelectMany_nested_navigation_property_optional_and_projection(isAsync);
}

[ConditionalTheory(Skip = "17539")]
public override Task Join_navigations_in_inner_selector_translated_without_collision(bool isAsync)
{
Expand Down
Loading

0 comments on commit 90fc979

Please sign in to comment.