Skip to content

Commit

Permalink
[2.0.1] Query: Unique-fy ordering & projection created by IncludeComp…
Browse files Browse the repository at this point in the history 
…iler with existing ordering

Resolves #10045

The issue: When you have reference.collection include and orderby reference.property, the ordering is expanded by navigation rewrite and it is null-compensated. Whereas the ordering collection include adds missed null-compensation hence they were not matching property causing multiple orderings/projection in QueryModel, which would map to same SqlFragment causing projection count mismatch.
The fix is to unique-fy ordering by including null-compensation. Due to new pattern generated by Include, downstream code needs to understand & match more patterns.
  • Loading branch information
@smitpatel
smitpatel committed Oct 12, 2017
1 parent 196400e commit 74d4438
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 15 deletions.
4 changes: 2 additions & 2 deletions src/EFCore.Relational/Query/Expressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -818,14 +818,14 @@ var existingOrdering
return true;
}

if (o.Expression is NullableExpression nullableExpression1
if (o.Expression.RemoveConvert() is NullableExpression nullableExpression1
&& _expressionEqualityComparer
.Equals(nullableExpression1.Operand.RemoveConvert(), ordering.Expression))
{
return true;
}

return ordering.Expression is NullableExpression nullableExpression2
return ordering.Expression.RemoveConvert() is NullableExpression nullableExpression2
&& _expressionEqualityComparer
.Equals(nullableExpression2.Operand.RemoveConvert(), o.Expression);
}
Expand Down
4 changes: 4 additions & 0 deletions src/EFCore.Specification.Tests/Query/ComplexNavigationsOwnedQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,10 @@ public override void Include_collection_with_groupby_in_subquery_and_filter_afte
{
}

public override void Include_reference_collection_order_by_reference_navigation()
{
}

protected override IQueryable<Level1> GetExpectedLevelOne()
=> ComplexNavigationsData.SplitLevelOnes.AsQueryable();

Expand Down
17 changes: 17 additions & 0 deletions src/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3674,6 +3674,23 @@ public virtual void Include_collection_with_groupby_in_subquery_and_filter_after
expectedIncludes: new List<IExpectedInclude> { new ExpectedInclude<Level1>(e => e.OneToMany_Optional, "OneToMany_Optional") });
}

[ConditionalFact]
public virtual void Include_reference_collection_order_by_reference_navigation()
{
AssertIncludeQuery<Level1>(
l1s => l1s
.Include(l1 => l1.OneToOne_Optional_FK.OneToMany_Optional)
.OrderBy(l1 => l1.OneToOne_Optional_FK.Id),
l1s => l1s
.Include(l1 => l1.OneToOne_Optional_FK.OneToMany_Optional)
.OrderBy(l1 => MaybeScalar<int>(l1.OneToOne_Optional_FK, () => l1.OneToOne_Optional_FK.Id)),
expectedIncludes: new List<IExpectedInclude>
{
new ExpectedInclude<Level1>(e => e.OneToOne_Optional_FK, "OneToOne_Optional_FK"),
new ExpectedInclude<Level2>(e => e.OneToMany_Optional, "OneToMany_Optional", "OneToOne_Optional_FK")
});
}

private static TResult Maybe<TResult>(object caller, Func<TResult> expression) where TResult : class
{
if (caller == null)
Expand Down
49 changes: 38 additions & 11 deletions src/EFCore/Query/Internal/IncludeCompiler.CollectionQueryModelRewritingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,19 +197,40 @@ var orderByClause
{
var propertyExpression = querySourceReferenceExpression.CreateEFPropertyExpression(property);

var orderingExpression = Expression.Convert(
new NullConditionalExpression(
querySourceReferenceExpression,
propertyExpression),
propertyExpression.Type);

if (!orderings.Any(
o =>
_expressionEqualityComparer.Equals(o.Expression, propertyExpression)
|| o.Expression is MemberExpression memberExpression
&& memberExpression.Expression is QuerySourceReferenceExpression memberQuerySourceReferenceExpression
&& ReferenceEquals(memberQuerySourceReferenceExpression.ReferencedQuerySource, querySourceReferenceExpression.ReferencedQuerySource)
&& memberExpression.Member.Equals(property.PropertyInfo)))
o => _expressionEqualityComparer.Equals(o.Expression, orderingExpression)
|| (o.Expression is MemberExpression memberExpression1
&& propertyExpression is MethodCallExpression methodCallExpression
&& MatchEfPropertyToMemberExpression(memberExpression1, methodCallExpression))
|| (o.Expression.RemoveConvert() is NullConditionalExpression nullConditionalExpression
&& nullConditionalExpression.AccessOperation is MemberExpression memberExpression
&& propertyExpression is MethodCallExpression methodCallExpression1
&& MatchEfPropertyToMemberExpression(memberExpression, methodCallExpression1))))
{
parentOrderings.Add(new Ordering(propertyExpression, OrderingDirection.Asc));
parentOrderings.Add(new Ordering(orderingExpression, OrderingDirection.Asc));
}
}
}

private static bool MatchEfPropertyToMemberExpression(MemberExpression memberExpression, MethodCallExpression methodCallExpression)
{
if (methodCallExpression.IsEFProperty())
{
var propertyName = (string)(methodCallExpression.Arguments[1] as ConstantExpression)?.Value;

return memberExpression.Member.Name.Equals(propertyName)
&& _expressionEqualityComparer.Equals(memberExpression.Expression, methodCallExpression.Arguments[0]);
}

return false;
}

private static void AdjustPredicate(
QueryModel queryModel,
IQuerySource parentQuerySource,
Expand Down Expand Up @@ -503,8 +524,13 @@ in fromQueryModel.BodyClauses.OfType<OrderByClause>().ToArray())
foreach (var ordering in orderByClause.Orderings)
{
int projectionIndex;
var orderingExpression = ordering.Expression;
if (ordering.Expression.RemoveConvert() is NullConditionalExpression nullConditionalExpression)
{
orderingExpression = nullConditionalExpression.AccessOperation;
}

if (ordering.Expression is MemberExpression memberExpression
if (orderingExpression is MemberExpression memberExpression
&& memberExpression.Expression is QuerySourceReferenceExpression memberQsre
&& memberQsre.ReferencedQuerySource == querySource)
{
Expand Down Expand Up @@ -542,7 +568,8 @@ in fromQueryModel.BodyClauses.OfType<OrderByClause>().ToArray())
{
projectionIndex
= subQueryProjection
.FindIndex(e => _expressionEqualityComparer.Equals(e.RemoveConvert(), ordering.Expression));
// Do NOT use orderingExpression variable here
.FindIndex(e => _expressionEqualityComparer.Equals(e.RemoveConvert(), ordering.Expression.RemoveConvert()));
}

if (projectionIndex == -1)
Expand All @@ -553,8 +580,8 @@ in fromQueryModel.BodyClauses.OfType<OrderByClause>().ToArray())
Expression.Convert(
// Workaround re-linq#RMLNQ-111 - When this is fixed the Clone can go away
CloningExpressionVisitor.AdjustExpressionAfterCloning(
ordering.Expression,
new QuerySourceMapping()),
ordering.Expression,
new QuerySourceMapping()),
typeof(object)));
}

Expand Down
8 changes: 7 additions & 1 deletion test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsQueryInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,16 @@ public override void Nested_group_join_with_take()
base.Nested_group_join_with_take();
}

[ConditionalFact(Skip = " issue #9591")]
[ConditionalFact(Skip = "issue #9591")]
public override void Multi_include_with_groupby_in_subquery()
{
base.Multi_include_with_groupby_in_subquery();
}

[ConditionalFact(Skip = "Issue #10060")]
public override void Include_reference_collection_order_by_reference_navigation()
{
base.Include_reference_collection_order_by_reference_navigation();
}
}
}
23 changes: 22 additions & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ public ComplexNavigationsQuerySqlServerTest(
ComplexNavigationsQuerySqlServerFixture fixture, ITestOutputHelper testOutputHelper)
: base(fixture)
{
fixture.TestSqlLoggerFactory.Clear();
Fixture.TestSqlLoggerFactory.Clear();
//Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

private bool SupportsOffset => TestEnvironment.GetFlag(nameof(SqlServerCondition.SupportsOffset)) ?? true;
Expand Down Expand Up @@ -3236,6 +3237,26 @@ FROM [Level2] AS [l20]
ORDER BY [t].[c], [t].[Name], [t].[Id]");
}

public override void Include_reference_collection_order_by_reference_navigation()
{
base.Include_reference_collection_order_by_reference_navigation();

AssertSql(
@"SELECT [l1].[Id], [l1].[Date], [l1].[Name], [l1].[OneToMany_Optional_Self_InverseId], [l1].[OneToMany_Required_Self_InverseId], [l1].[OneToOne_Optional_SelfId], [l1.OneToOne_Optional_FK].[Id], [l1.OneToOne_Optional_FK].[Date], [l1.OneToOne_Optional_FK].[Level1_Optional_Id], [l1.OneToOne_Optional_FK].[Level1_Required_Id], [l1.OneToOne_Optional_FK].[Name], [l1.OneToOne_Optional_FK].[OneToMany_Optional_InverseId], [l1.OneToOne_Optional_FK].[OneToMany_Optional_Self_InverseId], [l1.OneToOne_Optional_FK].[OneToMany_Required_InverseId], [l1.OneToOne_Optional_FK].[OneToMany_Required_Self_InverseId], [l1.OneToOne_Optional_FK].[OneToOne_Optional_PK_InverseId], [l1.OneToOne_Optional_FK].[OneToOne_Optional_SelfId]
FROM [Level1] AS [l1]
LEFT JOIN [Level2] AS [l1.OneToOne_Optional_FK] ON [l1].[Id] = [l1.OneToOne_Optional_FK].[Level1_Optional_Id]
ORDER BY [l1.OneToOne_Optional_FK].[Id]",
//
@"SELECT [l1.OneToOne_Optional_FK.OneToMany_Optional].[Id], [l1.OneToOne_Optional_FK.OneToMany_Optional].[Level2_Optional_Id], [l1.OneToOne_Optional_FK.OneToMany_Optional].[Level2_Required_Id], [l1.OneToOne_Optional_FK.OneToMany_Optional].[Name], [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToMany_Optional_InverseId], [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToMany_Optional_Self_InverseId], [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToMany_Required_InverseId], [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToMany_Required_Self_InverseId], [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToOne_Optional_PK_InverseId], [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToOne_Optional_SelfId]
FROM [Level3] AS [l1.OneToOne_Optional_FK.OneToMany_Optional]
INNER JOIN (
SELECT DISTINCT [l1.OneToOne_Optional_FK0].[Id]
FROM [Level1] AS [l10]
LEFT JOIN [Level2] AS [l1.OneToOne_Optional_FK0] ON [l10].[Id] = [l1.OneToOne_Optional_FK0].[Level1_Optional_Id]
) AS [t] ON [l1.OneToOne_Optional_FK.OneToMany_Optional].[OneToMany_Optional_InverseId] = [t].[Id]
ORDER BY [t].[Id]");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down

0 comments on commit 74d4438

Please sign in to comment.