diff --git a/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs b/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs index 1191f992ad2..884cf13d900 100644 --- a/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/Pipeline/InExpressionValuesExpandingExpressionVisitor.cs @@ -1,9 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections; using System.Collections.Generic; using System.Linq.Expressions; +using Microsoft.EntityFrameworkCore.Query.Pipeline; using Microsoft.EntityFrameworkCore.Relational.Query.Pipeline.SqlExpressions; using Microsoft.EntityFrameworkCore.Storage; @@ -34,6 +36,7 @@ public override Expression Visit(Expression expression) switch (inExpression.Values) { + // TODO: This shouldn't be here - should be part of compilation instead (#16375) case SqlConstantExpression sqlConstant: { typeMapping = sqlConstant.TypeMapping; @@ -99,6 +102,66 @@ public override Expression Visit(Expression expression) return base.Visit(expression); } + + protected override Expression VisitExtension(Expression extensionExpression) + { + var visited = base.VisitExtension(extensionExpression); + if (visited is OrderingExpression orderingExpression) + { + // Our rewriting of InExpression above may have left an ordering which is completely + // constant, which is invalid in SqlServer. QuerySqlGenerator will recognize the constant + // expression and render (SELECT 1). + var columnSearchingVisitor = new SearchingExpressionVisitor(typeof(ColumnExpression)); + columnSearchingVisitor.Visit(orderingExpression); + return columnSearchingVisitor.FoundExpression == null + ? new OrderingExpression( + new SqlConstantExpression(Expression.Constant(1), _sqlExpressionFactory.FindMapping(typeof(int))), + true) + : orderingExpression; + } + + return visited; + } + } + + /// + /// Searches an expression tree for the first occurrence of a node meeting the given criteria, and returns that node. + /// + public class SearchingExpressionVisitor : ExpressionVisitor + { + private Func _predicate; + + public Expression FoundExpression { get; private set; } + + public SearchingExpressionVisitor(Func predicate) + { + _predicate = predicate; + } + + public SearchingExpressionVisitor(Type searchForType) + : this(searchForType.IsInstanceOfType) + { + } + + /// + /// Resets the visitor, making it ready to run again. + /// + public void Reset() + { + FoundExpression = null; + } + + public override Expression Visit(Expression node) + { + // TODO: can be optimized by immediately returning null when a matching node is found (but must be handled everywhere...) + + if (FoundExpression == null && _predicate(node)) + { + FoundExpression = node; + } + + return base.Visit(node); + } } } } diff --git a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs index 92680ddb76b..e49134807ab 100644 --- a/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/Pipeline/SqlExpressions/SelectExpression.cs @@ -1070,9 +1070,9 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) var orderings = new List(); foreach (var ordering in _orderings) { - var orderingExpression = (SqlExpression)visitor.Visit(ordering.Expression); - changed |= orderingExpression != ordering.Expression; - orderings.Add(ordering.Update(orderingExpression)); + var newOrdering = (OrderingExpression)visitor.Visit(ordering); + changed |= newOrdering != ordering; + orderings.Add(newOrdering); } var offset = (SqlExpression)visitor.Visit(Offset); diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs index 765e07a30b4..83fa71be901 100644 --- a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs @@ -6872,7 +6872,7 @@ public virtual Task Cast_ordered_subquery_to_base_type_using_typed_ToArray(bool elementAsserter: CollectionAsserter(e => e.Nickname, (e, a) => Assert.Equal(e.Nickname, a.Nickname))); } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Correlated_collection_with_complex_order_by_funcletized_to_constant_bool(bool isAsync) { diff --git a/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs b/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs index 4fea65ad523..61c08b19941 100644 --- a/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/IncludeTestBase.cs @@ -3933,7 +3933,7 @@ var orders } } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [InlineData(false)] [InlineData(true)] public virtual void Include_collection_OrderBy_empty_list_contains(bool useString) @@ -3968,7 +3968,7 @@ var customers } } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [InlineData(false)] [InlineData(true)] public virtual void Include_collection_OrderBy_empty_list_does_not_contains(bool useString) diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs index 372f8a06424..b901990f617 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs @@ -5735,7 +5735,7 @@ on o.CustomerID equals c.CustomerID .Take(5)); } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task OrderBy_empty_list_contains(bool isAsync) { @@ -5747,7 +5747,7 @@ public virtual Task OrderBy_empty_list_contains(bool isAsync) entryCount: 91); } - [ConditionalTheory(Skip = "Issue#15713")] + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task OrderBy_empty_list_does_not_contains(bool isAsync) { diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs index 13859b3f097..61e8f3e3ba0 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs @@ -7413,19 +7413,11 @@ public override async Task Correlated_collection_with_complex_order_by_funcletiz await base.Correlated_collection_with_complex_order_by_funcletized_to_constant_bool(isAsync); AssertSql( - @"SELECT [g].[Nickname], [g].[FullName] + @"SELECT [g].[Nickname], [g].[SquadId], [w].[Name], [w].[Id], [w].[OwnerFullName] FROM [Gears] AS [g] -WHERE [g].[Discriminator] IN (N'Officer', N'Gear') -ORDER BY [g].[Nickname], [g].[SquadId], [g].[FullName]", - // - @"SELECT [t].[c], [t].[Nickname], [t].[SquadId], [t].[FullName], [g.Weapons].[Name], [g.Weapons].[OwnerFullName] -FROM [Weapons] AS [g.Weapons] -INNER JOIN ( - SELECT CAST(0 AS bit) AS [c], [g0].[Nickname], [g0].[SquadId], [g0].[FullName] - FROM [Gears] AS [g0] - WHERE [g0].[Discriminator] IN (N'Officer', N'Gear') -) AS [t] ON [g.Weapons].[OwnerFullName] = [t].[FullName] -ORDER BY [t].[c] DESC, [t].[Nickname], [t].[SquadId], [t].[FullName]"); +LEFT JOIN [Weapons] AS [w] ON [g].[FullName] = [w].[OwnerFullName] +WHERE [g].[Discriminator] IN (N'Gear', N'Officer') +ORDER BY [g].[Nickname], [g].[SquadId], [w].[Id]"); } public override async Task Double_order_by_on_nullable_bool_coming_from_optional_navigation(bool isAsync) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs index 68523cf6cad..5107da59eb4 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/IncludeSqlServerTest.cs @@ -1116,24 +1116,19 @@ public override void Include_collection_OrderBy_empty_list_contains(bool useStri AssertSql( @"@__p_1='1' -SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] -FROM [Customers] AS [c] -WHERE [c].[CustomerID] LIKE N'A%' -ORDER BY (SELECT 1), [c].[CustomerID] -OFFSET @__p_1 ROWS", - // - @"@__p_1='1' - -SELECT [c.Orders].[OrderID], [c.Orders].[CustomerID], [c.Orders].[EmployeeID], [c.Orders].[OrderDate] -FROM [Orders] AS [c.Orders] -INNER JOIN ( - SELECT [c0].[CustomerID], CAST(0 AS bit) AS [c] - FROM [Customers] AS [c0] - WHERE [c0].[CustomerID] LIKE N'A%' - ORDER BY [c], [c0].[CustomerID] +SELECT [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate] +FROM ( + SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], CASE + WHEN CAST(1 AS bit) = CAST(0 AS bit) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) + END AS [c] + FROM [Customers] AS [c] + WHERE [c].[CustomerID] LIKE N'A%' + ORDER BY (SELECT 1) OFFSET @__p_1 ROWS -) AS [t] ON [c.Orders].[CustomerID] = [t].[CustomerID] -ORDER BY [t].[c], [t].[CustomerID]"); +) AS [t] +LEFT JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] +ORDER BY [t].[c], [t].[CustomerID], [o].[OrderID]"); } public override void Include_collection_OrderBy_empty_list_does_not_contains(bool useString) @@ -1143,24 +1138,19 @@ public override void Include_collection_OrderBy_empty_list_does_not_contains(boo AssertSql( @"@__p_1='1' -SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] -FROM [Customers] AS [c] -WHERE [c].[CustomerID] LIKE N'A%' -ORDER BY (SELECT 1), [c].[CustomerID] -OFFSET @__p_1 ROWS", - // - @"@__p_1='1' - -SELECT [c.Orders].[OrderID], [c.Orders].[CustomerID], [c.Orders].[EmployeeID], [c.Orders].[OrderDate] -FROM [Orders] AS [c.Orders] -INNER JOIN ( - SELECT [c0].[CustomerID], CAST(1 AS bit) AS [c] - FROM [Customers] AS [c0] - WHERE [c0].[CustomerID] LIKE N'A%' - ORDER BY [c], [c0].[CustomerID] +SELECT [t].[CustomerID], [t].[Address], [t].[City], [t].[CompanyName], [t].[ContactName], [t].[ContactTitle], [t].[Country], [t].[Fax], [t].[Phone], [t].[PostalCode], [t].[Region], [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate] +FROM ( + SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], CASE + WHEN CAST(1 AS bit) = CAST(1 AS bit) THEN CAST(1 AS bit) + ELSE CAST(0 AS bit) + END AS [c] + FROM [Customers] AS [c] + WHERE [c].[CustomerID] LIKE N'A%' + ORDER BY (SELECT 1) OFFSET @__p_1 ROWS -) AS [t] ON [c.Orders].[CustomerID] = [t].[CustomerID] -ORDER BY [t].[c], [t].[CustomerID]"); +) AS [t] +LEFT JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] +ORDER BY [t].[c], [t].[CustomerID], [o].[OrderID]"); } public override void Include_collection_OrderBy_list_contains(bool useString)