Query: Expand navigations together in OrderBy/ThenBy (#18913)

Fixes #18904
dotnet · Feb 14, 2020 · a6ebade · a6ebade
1 parent 81e1dc5
commit a6ebade
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 10 deletions.
diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
@@ -950,15 +950,21 @@ private Expression ProcessLeftJoin(
         private Expression ProcessOrderByThenBy(
             NavigationExpansionExpression source, MethodInfo genericMethod, LambdaExpression keySelector, bool thenBy)
         {
-            var keySelectorBody = ExpandNavigationsInLambdaExpression(source, keySelector);
+            var lambdaBody = ReplacingExpressionVisitor.Replace(
+                keySelector.Parameters[0],
+                source.PendingSelector,
+                keySelector.Body);
+
+            lambdaBody = new ExpandingExpressionVisitor(this, source).Visit(lambdaBody);
+            lambdaBody = _subqueryMemberPushdownExpressionVisitor.Visit(lambdaBody);
 
             if (thenBy)
             {
-                source.AppendPendingOrdering(genericMethod, keySelectorBody);
+                source.AppendPendingOrdering(genericMethod, lambdaBody);
             }
             else
             {
-                source.AddPendingOrdering(genericMethod, keySelectorBody);
+                source.AddPendingOrdering(genericMethod, lambdaBody);
             }
 
             return source;
@@ -1142,7 +1148,10 @@ private void ApplyPendingOrderings(NavigationExpansionExpression source)
             {
                 foreach (var (orderingMethod, keySelector) in source.PendingOrderings)
                 {
-                    var keySelectorLambda = GenerateLambda(keySelector, source.CurrentParameter);
+                    var lambdaBody = Visit(keySelector);
+                    lambdaBody = _pendingSelectorExpandingExpressionVisitor.Visit(lambdaBody);
+
+                    var keySelectorLambda = GenerateLambda(lambdaBody, source.CurrentParameter);
 
                     source.UpdateSource(
                         Expression.Call(
@@ -1388,7 +1397,7 @@ private Expression ExpandNavigationsInLambdaExpression(NavigationExpansionExpres
             return lambdaBody;
         }
 
-        private IEnumerable<INavigation> FindNavigations(IEntityType entityType, string navigationName)
+        private static IEnumerable<INavigation> FindNavigations(IEntityType entityType, string navigationName)
         {
             var navigation = entityType.FindNavigation(navigationName);
             if (navigation != null)

diff --git a/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/ComplexNavigationsWeakQueryInMemoryTest.cs
@@ -53,5 +53,11 @@ public override Task Lift_projection_mapping_when_pushing_down_subquery(bool isA
         {
             return base.Lift_projection_mapping_when_pushing_down_subquery(isAsync);
         }
+
+        [ConditionalTheory(Skip = "issue #18912")]
+        public override Task OrderBy_collection_count_ThenBy_reference_navigation(bool async)
+        {
+            return base.OrderBy_collection_count_ThenBy_reference_navigation(async);
+        }
     }
 }
diff --git a/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
@@ -5941,5 +5941,22 @@ public virtual Task Including_reference_navigation_and_projecting_collection_nav
                         First = e.OneToMany_Required1.OrderByDescending(e => e.Id).FirstOrDefault()
                     }));
         }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task OrderBy_collection_count_ThenBy_reference_navigation(bool async)
+        {
+            return AssertQuery(
+                async,
+                ss => ss.Set<Level1>()
+                    .OrderBy(l1 => l1.OneToOne_Required_FK1.OneToMany_Required2.Count())
+                    .ThenBy(l1 => l1.OneToOne_Required_FK1.OneToOne_Required_FK2.Name),
+                ss => ss.Set<Level1>()
+                    .OrderBy(l1 => MaybeScalar<int>(Maybe(l1.OneToOne_Required_FK1, () => l1.OneToOne_Required_FK1.OneToMany_Required2),
+                        () => l1.OneToOne_Required_FK1.OneToMany_Required2.Count()) ?? 0)
+                    .ThenBy(l1 => Maybe(Maybe(l1.OneToOne_Required_FK1, () => l1.OneToOne_Required_FK1.OneToOne_Required_FK2),
+                        () => l1.OneToOne_Required_FK1.OneToOne_Required_FK2.Name)),
+                assertOrder: true);
+        }
     }
 }
diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs
@@ -4445,9 +4445,21 @@ FROM [LevelTwo] AS [l1]
 ORDER BY [l].[Id], [l2].[Id]");
         }
 
-        private void AssertSql(params string[] expected)
+        public override async Task OrderBy_collection_count_ThenBy_reference_navigation(bool async)
         {
-            Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
+            await base.OrderBy_collection_count_ThenBy_reference_navigation(async);
+
+            AssertSql(
+                @"SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
+FROM [LevelOne] AS [l]
+LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Required_Id]
+LEFT JOIN [LevelThree] AS [l1] ON [l0].[Id] = [l1].[Level2_Required_Id]
+ORDER BY (
+    SELECT COUNT(*)
+    FROM [LevelThree] AS [l2]
+    WHERE [l0].[Id] IS NOT NULL AND ([l0].[Id] = [l2].[OneToMany_Required_Inverse3Id])), [l1].[Name]");
         }
+
+        private void AssertSql(params string[] expected) => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
     }
 }
diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs
@@ -7386,13 +7386,13 @@ private SqlServerTestStore CreateDatabase18759()
                     ClearLog();
                 });
 
-        public class Person18759
+        private class Person18759
         {
             public int Id { get; set; }
             public User18759 UserDelete { get; set; }
         }
 
-        public class User18759
+        private class User18759
         {
             public int Id { get; set; }
         }
@@ -7410,7 +7410,7 @@ public BugContext18759(DbContextOptions options)
             }
         }
 
-        #endregion Issue18759
+        #endregion
 
         private DbContextOptions _options;