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 9, 2019
1 parent 5dd4b63 commit 12174a1
Show file tree
Hide file tree
Showing 17 changed files with 209 additions and 173 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
50 changes: 40 additions & 10 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,11 @@ private SqlExpression TryExtractJoinKey(SelectExpression selectExpression)
&& selectExpression.Predicate != null)
{
var joinPredicate = TryExtractJoinKey(selectExpression, selectExpression.Predicate, out var predicate);
if (predicate != null)
{
predicate = RemoveNullChecks(predicate);
}

selectExpression.Predicate = predicate;

return joinPredicate;
Expand All @@ -944,28 +949,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);

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

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

updatedPredicate = predicate;

return null;
}

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)
{
if (sqlBinaryExpression.OperatorType == ExpressionType.Equal)
Expand All @@ -992,6 +997,31 @@ private SqlBinaryExpression ValidateKeyComparison(SelectExpression inner, SqlBin
return null;
}

private SqlExpression RemoveNullChecks(SqlExpression predicate)
{
if (predicate is SqlBinaryExpression sqlBinaryExpression)
{
if (sqlBinaryExpression.OperatorType == ExpressionType.NotEqual
&& sqlBinaryExpression.Left is ColumnExpression leftColumn
&& ContainsTableReference(leftColumn.Table)
&& sqlBinaryExpression.Right is SqlConstantExpression sqlConstantExpression
&& sqlConstantExpression.Value == null)
{
return null;
}

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

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
24 changes: 0 additions & 24 deletions test/EFCore.InMemory.FunctionalTests/Query/GearsOfWarQueryInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,30 +45,6 @@ public override Task GetValueOrDefault_on_DateTimeOffset(bool isAsync)
return base.GetValueOrDefault_on_DateTimeOffset(isAsync);
}

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

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

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

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

[ConditionalFact(Skip = "issue #17537")]
public override void Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result1()
{
Expand Down
4 changes: 2 additions & 2 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4450,7 +4450,7 @@ select Maybe(l1.OneToOne_Optional_FK1, () => l1.OneToOne_Optional_FK1.OneToMany_
});
}

[ConditionalTheory(Skip = "issue #17531")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Project_collection_navigation_nested_with_take(bool isAsync)
{
Expand All @@ -4460,7 +4460,7 @@ public virtual Task Project_collection_navigation_nested_with_take(bool isAsync)
select l1.OneToOne_Optional_FK1.OneToMany_Optional2.Take(50),
l1s => from l1 in l1s
select Maybe(l1.OneToOne_Optional_FK1, () => l1.OneToOne_Optional_FK1.OneToMany_Optional2.Take(50)),
elementSorter: e => e != null ? e.Count : 0,
elementSorter: e => ((IEnumerable<Level3>)e)?.Count() ?? 0,
elementAsserter: (e, a) =>
{
var actualCollection = new List<Level3>();
Expand Down
Loading

0 comments on commit 12174a1

Please sign in to comment.