fixup! Translate Order & OrderDescending

Address feedback
dotnet · Aug 13, 2024 · cec5112 · cec5112
1 parent 7105ace
commit cec5112
Show file tree

Hide file tree

Showing 16 changed files with 120 additions and 173 deletions.
diff --git a/src/EFCore/Infrastructure/ExpressionExtensions.cs b/src/EFCore/Infrastructure/ExpressionExtensions.cs
@@ -438,15 +438,4 @@ static Expression AddConvertToObject(Expression expression)
                 ? Expression.Convert(expression, typeof(object))
                 : expression;
     }
-
-    /// <summary>
-    ///     Creates an <see cref="Expression" /> tree representing an identity lambda <c>x => x</c>.
-    /// </summary>
-    /// <param name="type">The parameter type.</param>
-    /// <returns>An identity lambda expression.</returns>
-    public static LambdaExpression CreateIdentityLambda(Type type)
-    {
-        var parameter = Expression.Parameter(type);
-        return Expression.Lambda(parameter, parameter);
-    }
 }
diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
@@ -1195,7 +1195,8 @@ private NavigationExpansionExpression ProcessInclude(
                 var (result, filterExpression) = ExtractIncludeFilter(methodCallExpression.Arguments[0], includeExpression);
                 if (filterExpression == null)
                 {
-                    filterExpression = ExpressionExtensions.CreateIdentityLambda(result.Type);
+                    var prm = Expression.Parameter(result.Type);
+                    filterExpression = Expression.Lambda(prm, prm);
                 }
 
                 var arguments = new List<Expression> { filterExpression.Body };

diff --git a/src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs b/src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs
@@ -134,7 +134,6 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
         Expression? visitedExpression = null;
         if (method.DeclaringType == typeof(Enumerable))
         {
-            // TODO: Will these be further normailzed?
             visitedExpression = TryConvertEnumerableToQueryable(methodCallExpression);
         }
 
@@ -500,14 +499,15 @@ private MethodCallExpression TryNormalizeOrderAndOrderDescending(MethodCallExpre
         if (genericMethod == QueryableMethods.Order
             || genericMethod == QueryableMethods.OrderDescending)
         {
-            var sourceType = methodCallExpression.Method.GetGenericArguments()[0];
+            var sourceType = methodCallExpression.Method.GetGenericArguments()[0];            
+            var parameter = Expression.Parameter(sourceType);
 
             return Expression.Call(
                 genericMethod == QueryableMethods.Order
                     ? QueryableMethods.OrderBy.MakeGenericMethod(sourceType, sourceType)
                     : QueryableMethods.OrderByDescending.MakeGenericMethod(sourceType, sourceType),
                 methodCallExpression.Arguments[0],
-                Expression.Quote(ExpressionExtensions.CreateIdentityLambda(sourceType)));
+                Expression.Quote(Expression.Lambda(parameter, parameter)));
         }
 
         return methodCallExpression;

diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs
@@ -5146,6 +5146,44 @@ public override Task Static_member_access_gets_parameterized_within_larger_evalu
                 AssertSql("ReadItem(None, Customer|ALFKI)");
             });
 
+    public override Task Select_Order(bool async)
+        => Fixture.NoSyncTest(
+            async, async a =>
+            {
+                await base.Select_Order(a);
+
+                AssertSql(
+                    """
+SELECT VALUE c["CustomerID"]
+FROM root c
+WHERE (c["Discriminator"] = "Customer")
+ORDER BY c["CustomerID"]
+""");
+            });
+
+    public override Task Select_OrderDescending(bool async)
+        => Fixture.NoSyncTest(
+            async, async a =>
+            {
+                await base.Select_OrderDescending(a);
+
+                AssertSql(
+                    """
+SELECT VALUE c["CustomerID"]
+FROM root c
+WHERE (c["Discriminator"] = "Customer")
+ORDER BY c["CustomerID"] DESC
+""");
+            });
+
+    public override async Task Where_Order_First(bool async)
+    {
+        await AssertTranslationFailed(
+            () => base.Where_Order_First(async));
+
+        AssertSql();
+    }
+
     #region ToPageAsync
 
     [ConditionalFact]

diff --git a/test/EFCore.InMemory.FunctionalTests/Query/NorthwindMiscellaneousQueryInMemoryTest.cs b/test/EFCore.InMemory.FunctionalTests/Query/NorthwindMiscellaneousQueryInMemoryTest.cs
@@ -55,4 +55,9 @@ public override Task Collection_navigation_equal_to_null_for_subquery_using_Elem
 
     public override Task Collection_navigation_equal_to_null_for_subquery_using_ElementAtOrDefault_parameter(bool async)
         => Task.CompletedTask;
+
+    public override Task Where_Order_First(bool async)
+        // Sequence contains no elements.
+        => Assert.ThrowsAsync<InvalidOperationException>(
+            () => base.Where_Order_First(async));
 }
diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
@@ -4346,22 +4346,6 @@ public virtual Task Order_by_entity_qsre_with_other_orderbys(bool async)
                 .ThenBy(w => w.Name),
             assertOrder: true);
 
-    [ConditionalTheory]
-    [MemberData(nameof(IsAsyncData))]
-    public virtual Task Order(bool async)
-        => AssertQuery(
-            async,
-            ss => ss.Set<Gear>().Select(f => f.FullName).Order(),
-            assertOrder: true);
-
-    [ConditionalTheory]
-    [MemberData(nameof(IsAsyncData))]
-    public virtual Task OrderDescending(bool async)
-        => AssertQuery(
-            async,
-            ss => ss.Set<Gear>().Select(f => f.FullName).OrderDescending(),
-            assertOrder: true);
-
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Join_on_entity_qsre_keys(bool async)

diff --git a/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs
@@ -2480,6 +2480,32 @@ public virtual Task OrderByDescending_ThenByDescending(bool async)
                 .Select(c => c.City),
             assertOrder: true);
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Select_Order(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Customer>().Select(c => c.CustomerID).Order(),
+            ss => ss.Set<Customer>().Select(c => c.CustomerID).Order(StringComparer.Ordinal),
+            assertOrder: true);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Select_OrderDescending(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Customer>().Select(c => c.CustomerID).OrderDescending(),
+            ss => ss.Set<Customer>().Select(c => c.CustomerID).OrderDescending(StringComparer.Ordinal),
+            assertOrder: true);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Where_Order_First(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Customer>().Where(c => c.Orders.Order().First().OrderID == 10248).Select(c => c.CustomerID),
+            ss => ss.Set<Customer>().AsEnumerable().Where(c => c.Orders.OrderBy(o => o.OrderID).FirstOrDefault()?.OrderID == 10248).Select(c => c.CustomerID).AsQueryable());
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task OrderBy_ThenBy_Any(bool async)

diff --git a/...ication.Tests/TestUtilities/QueryTestGeneration/AppendOrderByIdentityExpressionMutator.cs b/...ication.Tests/TestUtilities/QueryTestGeneration/AppendOrderByIdentityExpressionMutator.cs
@@ -1,8 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using ExpressionExtensions = Microsoft.EntityFrameworkCore.Infrastructure.ExpressionExtensions;
-
 namespace Microsoft.EntityFrameworkCore.TestUtilities.QueryTestGeneration;
 
 public class AppendOrderByIdentityExpressionMutator(DbContext context) : ExpressionMutator(context)
@@ -20,8 +18,8 @@ public override Expression Apply(Expression expression, Random random)
             ? QueryableMethods.OrderByDescending.MakeGenericMethod(typeArgument, typeArgument)
             : QueryableMethods.OrderBy.MakeGenericMethod(typeArgument, typeArgument);
 
-
-        var lambda = ExpressionExtensions.CreateIdentityLambda(typeArgument);
+        var prm = Expression.Parameter(typeArgument, "prm");
+        var lambda = Expression.Lambda(prm, prm);
         var resultExpression = Expression.Call(orderBy, expression, lambda);
 
         return resultExpression;

diff --git a/...fication.Tests/TestUtilities/QueryTestGeneration/AppendSelectIdentityExpressionMutator.cs b/...fication.Tests/TestUtilities/QueryTestGeneration/AppendSelectIdentityExpressionMutator.cs
@@ -1,8 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using ExpressionExtensions = Microsoft.EntityFrameworkCore.Infrastructure.ExpressionExtensions;
-
 namespace Microsoft.EntityFrameworkCore.TestUtilities.QueryTestGeneration;
 
 public class AppendSelectIdentityExpressionMutator(DbContext context) : ExpressionMutator(context)
@@ -14,7 +12,8 @@ public override Expression Apply(Expression expression, Random random)
     {
         var typeArgument = expression.Type.GetGenericArguments()[0];
         var select = QueryableMethods.Select.MakeGenericMethod(typeArgument, typeArgument);
-        var lambda = ExpressionExtensions.CreateIdentityLambda(typeArgument);
+        var prm = Expression.Parameter(typeArgument, "prm");
+        var lambda = Expression.Lambda(prm, prm);
         var resultExpression = Expression.Call(select, expression, lambda);
 
         return resultExpression;

diff --git a/...fication.Tests/TestUtilities/QueryTestGeneration/AppendThenByIdentityExpressionMutator.cs b/...fication.Tests/TestUtilities/QueryTestGeneration/AppendThenByIdentityExpressionMutator.cs
@@ -1,8 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using ExpressionExtensions = Microsoft.EntityFrameworkCore.Infrastructure.ExpressionExtensions;
-
 namespace Microsoft.EntityFrameworkCore.TestUtilities.QueryTestGeneration;
 
 public class AppendThenByIdentityExpressionMutator(DbContext context) : ExpressionMutator(context)
@@ -20,7 +18,8 @@ public override Expression Apply(Expression expression, Random random)
             ? QueryableMethods.ThenByDescending.MakeGenericMethod(typeArgument, typeArgument)
             : QueryableMethods.ThenBy.MakeGenericMethod(typeArgument, typeArgument);
 
-        var lambda = ExpressionExtensions.CreateIdentityLambda(typeArgument);
+        var prm = Expression.Parameter(typeArgument, "prm");
+        var lambda = Expression.Lambda(prm, prm);
         var resultExpression = Expression.Call(thenBy, expression, lambda);
 
         return resultExpression;

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs
@@ -5223,30 +5223,6 @@ FROM [Weapons] AS [w]
 """);
     }
 
-    public override async Task Order(bool async)
-    {
-        await base.Order(async);
-
-        AssertSql(
-            """
-SELECT [g].[FullName]
-FROM [Gears] AS [g]
-ORDER BY [g].[FullName]
-""");
-    }
-
-    public override async Task OrderDescending(bool async)
-    {
-        await base.OrderDescending(async);
-
-        AssertSql(
-            """
-SELECT [g].[FullName]
-FROM [Gears] AS [g]
-ORDER BY [g].[FullName] DESC
-""");
-    }
-
     public override async Task Join_on_entity_qsre_keys(bool async)
     {
         await base.Join_on_entity_qsre_keys(async);

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs
@@ -7367,6 +7367,46 @@ FROM [Customers] AS [c]
 """);
     }
 
+    public override async Task Select_Order(bool async)
+    {
+        await base.Select_Order(async);
+
+        AssertSql(
+            """
+SELECT [c].[CustomerID]
+FROM [Customers] AS [c]
+ORDER BY [c].[CustomerID]
+""");
+    }
+
+    public override async Task Select_OrderDescending(bool async)
+    {
+        await base.Select_OrderDescending(async);
+
+        AssertSql(
+            """
+SELECT [c].[CustomerID]
+FROM [Customers] AS [c]
+ORDER BY [c].[CustomerID] DESC
+""");
+    }
+
+    public override async Task Where_Order_First(bool async)
+    {
+        await base.Where_Order_First(async);
+
+        AssertSql(
+            """
+SELECT [c].[CustomerID]
+FROM [Customers] AS [c]
+WHERE (
+    SELECT TOP(1) [o].[OrderID]
+    FROM [Orders] AS [o]
+    WHERE [c].[CustomerID] = [o].[CustomerID]
+    ORDER BY [o].[OrderID]) = 10248
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/TPCGearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/TPCGearsOfWarQuerySqlServerTest.cs
@@ -7112,42 +7112,6 @@ FROM [Officers] AS [o]
 """);
     }
 
-    public override async Task Order(bool async)
-    {
-        await base.Order(async);
-
-        AssertSql(
-            """
-SELECT [u].[FullName]
-FROM (
-    SELECT [g].[FullName]
-    FROM [Gears] AS [g]
-    UNION ALL
-    SELECT [o].[FullName]
-    FROM [Officers] AS [o]
-) AS [u]
-ORDER BY [u].[FullName]
-""");
-    }
-
-    public override async Task OrderDescending(bool async)
-    {
-        await base.OrderDescending(async);
-
-        AssertSql(
-            """
-SELECT [u].[FullName]
-FROM (
-    SELECT [g].[FullName]
-    FROM [Gears] AS [g]
-    UNION ALL
-    SELECT [o].[FullName]
-    FROM [Officers] AS [o]
-) AS [u]
-ORDER BY [u].[FullName] DESC
-""");
-    }
-
     public override async Task Join_on_entity_qsre_keys(bool async)
     {
         await base.Join_on_entity_qsre_keys(async);

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs
@@ -6006,30 +6006,6 @@ FROM [Gears] AS [g]
 """);
     }
 
-    public override async Task Order(bool async)
-    {
-        await base.Order(async);
-
-        AssertSql(
-            """
-SELECT [g].[FullName]
-FROM [Gears] AS [g]
-ORDER BY [g].[FullName]
-""");
-    }
-
-    public override async Task OrderDescending(bool async)
-    {
-        await base.OrderDescending(async);
-
-        AssertSql(
-            """
-SELECT [g].[FullName]
-FROM [Gears] AS [g]
-ORDER BY [g].[FullName] DESC
-""");
-    }
-
     public override async Task Join_on_entity_qsre_keys(bool async)
     {
         await base.Join_on_entity_qsre_keys(async);