Fix to #8754 - Query: manually created GroupJoin with order by key de…

…scending generates invalid SQL

Problem was that ordering by the same column twice, but with different direction is invalid. This would usually be user error, however in some cases we introduce order by ourselves (e.g. in case of group join).
If then customer provides their own ordering with opposite direction, we would generate invalid query. We already have logic to catch duplicate ordering (if they are the same).

Fix is to expand the logic that catches "duplicate" ordering to match ordering with different direction and being the same.

src/EFCore.Relational/Query/Expressions/SelectExpression.cs

@@ -813,11 +813,6 @@ var existingOrdering
                 = _orderBy.Find(
                     o =>
                         {
-                            if (o.OrderingDirection != ordering.OrderingDirection)
-                            {
-                                return false;
-                            }
-
                             if (_expressionEqualityComparer.Equals(o.Expression, ordering.Expression))
                             {
                                 return true;
@@ -857,9 +852,8 @@ public virtual void PrependToOrderBy([NotNull] IEnumerable<Ordering> orderings)
             var oldOrderBy = _orderBy.ToList();
 
             _orderBy.Clear();
-            _orderBy.AddRange(orderings);
 
-            foreach (var ordering in oldOrderBy)
+            foreach (var ordering in orderings.Concat(oldOrderBy))
             {
                 AddToOrderBy(ordering);
             }

src/EFCore.Specification.Tests/Query/QueryTestBase.JoinGroupJoin.cs

@@ -639,5 +639,29 @@ join o in os on c.CustomerID equals o.CustomerID into lo
                             from o in lo.Where(x => x.OrderID > 5).OrderBy(x => x.OrderDate).DefaultIfEmpty()
                             select new { c.ContactName, o });
         }
+
+        [ConditionalFact]
+        public virtual void GroupJoin_with_order_by_key_descending1()
+        {
+            AssertQuery<Customer, Order>(
+                (cs, os) => from c in cs
+                            join o in os on c.CustomerID equals o.CustomerID into grouping
+                            where c.CustomerID.StartsWith("A")
+                            orderby c.CustomerID descending
+                            select grouping.Count(),
+                assertOrder: true);
+        }
+
+        [ConditionalFact]
+        public virtual void GroupJoin_with_order_by_key_descending2()
+        {
+            AssertQuery<Customer, Order>(
+                (cs, os) => from c in cs
+                            orderby c.CustomerID descending
+                            join o in os on c.CustomerID equals o.CustomerID into grouping
+                            where c.CustomerID.StartsWith("A")
+                            select grouping.Count(),
+                assertOrder: true);
+        }
     }
 }

src/EFCore.Specification.Tests/Query/QueryTestBase.cs

@@ -3937,6 +3937,30 @@ from o in os
                             select new { Id1 = c.CustomerID, Id2 = o.OrderID });
         }
 
+        [ConditionalFact]
+        public virtual void OrderBy_ThenBy_same_column_different_direction()
+        {
+            AssertQuery<Customer>(
+                cs => cs
+                    .Where(c => c.CustomerID.StartsWith("A"))
+                    .OrderBy(c => c.CustomerID)
+                    .ThenByDescending(c => c.CustomerID)
+                    .Select(c => c.CustomerID),
+                assertOrder: true);
+        }
+
+        [ConditionalFact]
+        public virtual void OrderBy_OrderBy_same_column_different_direction()
+        {
+            AssertQuery<Customer>(
+                cs => cs
+                    .Where(c => c.CustomerID.StartsWith("A"))
+                    .OrderBy(c => c.CustomerID)
+                    .OrderByDescending(c => c.CustomerID)
+                    .Select(c => c.CustomerID),
+                assertOrder: true);
+        }
+
         protected NorthwindContext CreateContext() => Fixture.CreateContext();
 
         protected QueryTestBase(TFixture fixture)

test/EFCore.SqlServer.FunctionalTests/Query/QuerySqlServerTest.JoinGroupJoin.cs

@@ -516,5 +516,29 @@ FROM [Customers] AS [c]
 LEFT JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID]
 ORDER BY [c].[CustomerID]");
         }
+
+        public override void GroupJoin_with_order_by_key_descending1()
+        {
+            base.GroupJoin_with_order_by_key_descending1();
+
+            AssertSql(
+                @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
+FROM [Customers] AS [c]
+LEFT JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID]
+WHERE [c].[CustomerID] LIKE N'A' + N'%' AND (LEFT([c].[CustomerID], LEN(N'A')) = N'A')
+ORDER BY [c].[CustomerID] DESC");
+        }
+
+        public override void GroupJoin_with_order_by_key_descending2()
+        {
+            base.GroupJoin_with_order_by_key_descending2();
+
+            AssertSql(
+                @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
+FROM [Customers] AS [c]
+LEFT JOIN [Orders] AS [o] ON [c].[CustomerID] = [o].[CustomerID]
+WHERE [c].[CustomerID] LIKE N'A' + N'%' AND (LEFT([c].[CustomerID], LEN(N'A')) = N'A')
+ORDER BY [c].[CustomerID] DESC");
+        }
     }
 }

test/EFCore.SqlServer.FunctionalTests/Query/QuerySqlServerTest.cs

@@ -4169,6 +4169,28 @@ CROSS JOIN [Orders] AS [o]
 ORDER BY [Id1], [Id2]");
         }
 
+        public override void OrderBy_ThenBy_same_column_different_direction()
+        {
+            base.OrderBy_ThenBy_same_column_different_direction();
+
+            AssertSql(
+                @"SELECT [c].[CustomerID]
+FROM [Customers] AS [c]
+WHERE [c].[CustomerID] LIKE N'A' + N'%' AND (LEFT([c].[CustomerID], LEN(N'A')) = N'A')
+ORDER BY [c].[CustomerID]");
+        }
+
+        public override void OrderBy_OrderBy_same_column_different_direction()
+        {
+            base.OrderBy_OrderBy_same_column_different_direction();
+
+            AssertSql(
+                @"SELECT [c].[CustomerID]
+FROM [Customers] AS [c]
+WHERE [c].[CustomerID] LIKE N'A' + N'%' AND (LEFT([c].[CustomerID], LEN(N'A')) = N'A')
+ORDER BY [c].[CustomerID] DESC");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);