From ccf6fab1a0f2fa0f6b4434cad73c170a0faf8c24 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 29 May 2025 13:56:31 +0200 Subject: [PATCH] Add missing parentheses for set operations (#36138) Fixes #36105 Fixes #36112 --- .../Query/QuerySqlGenerator.cs | 14 ++++- .../Query/Internal/SqliteQuerySqlGenerator.cs | 63 ++++++++++++++++++- .../NorthwindSetOperationsQueryTestBase.cs | 24 +++++++ ...orthwindSetOperationsQuerySqlServerTest.cs | 61 +++++++++++++++--- 4 files changed, 150 insertions(+), 12 deletions(-) diff --git a/src/EFCore.Relational/Query/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/QuerySqlGenerator.cs index 1906d587a40..c18fe6a3ffd 100644 --- a/src/EFCore.Relational/Query/QuerySqlGenerator.cs +++ b/src/EFCore.Relational/Query/QuerySqlGenerator.cs @@ -22,6 +22,9 @@ public class QuerySqlGenerator : SqlExpressionVisitor private IRelationalCommandBuilder _relationalCommandBuilder; private Dictionary? _repeatedParameterCounts; + private static readonly bool UseOldBehavior36105 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue36105", out var enabled36105) && enabled36105; + /// /// Creates a new instance of the class. /// @@ -1382,9 +1385,16 @@ static string GetSetOperation(SetOperationBase operation) protected virtual void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand) { // INTERSECT has higher precedence over UNION and EXCEPT, but otherwise evaluation is left-to-right. - // To preserve meaning, add parentheses whenever a set operation is nested within a different set operation. + // To preserve evaluation order, add parentheses whenever a set operation is nested within a different set operation + // - including different distinctness. + // In addition, EXCEPT is non-commutative (unlike UNION/INTERSECT), so add parentheses for that case too (see #36105). if (IsNonComposedSetOperation(operand) - && operand.Tables[0].GetType() != setOperation.GetType()) + && ((UseOldBehavior36105 && operand.Tables[0].GetType() != setOperation.GetType()) + || (!UseOldBehavior36105 + && operand.Tables[0] is SetOperationBase nestedSetOperation + && (nestedSetOperation is ExceptExpression + || nestedSetOperation.GetType() != setOperation.GetType() + || nestedSetOperation.IsDistinct != setOperation.IsDistinct)))) { _relationalCommandBuilder.AppendLine("("); using (_relationalCommandBuilder.Indent()) diff --git a/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs b/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs index ad1c62dc00e..f761526cf12 100644 --- a/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics.CodeAnalysis; using Microsoft.EntityFrameworkCore.Query.SqlExpressions; namespace Microsoft.EntityFrameworkCore.Sqlite.Query.Internal; @@ -13,6 +14,9 @@ namespace Microsoft.EntityFrameworkCore.Sqlite.Query.Internal; /// public class SqliteQuerySqlGenerator : QuerySqlGenerator { + private static readonly bool UseOldBehavior36112 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue36112", out var enabled36112) && enabled36112; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in @@ -97,8 +101,63 @@ protected override void GenerateLimitOffset(SelectExpression selectExpression) /// doing so can result in application failures when updating to a new Entity Framework Core release. /// protected override void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand) - // Sqlite doesn't support parentheses around set operation operands - => Visit(operand); + { + // Most databases support parentheses around set operations to determine evaluation order, but SQLite does not; + // however, we can instead wrap the nested set operation in a SELECT * FROM () to achieve the same effect. + // The following is a copy-paste of the base implementation from QuerySqlGenerator, adding the SELECT. + + // INTERSECT has higher precedence over UNION and EXCEPT, but otherwise evaluation is left-to-right. + // To preserve evaluation order, add parentheses whenever a set operation is nested within a different set operation + // - including different distinctness. + // In addition, EXCEPT is non-commutative (unlike UNION/INTERSECT), so add parentheses for that case too (see #36105). + if (!UseOldBehavior36112 + && TryUnwrapBareSetOperation(operand, out var nestedSetOperation) + && (nestedSetOperation is ExceptExpression + || nestedSetOperation.GetType() != setOperation.GetType() + || nestedSetOperation.IsDistinct != setOperation.IsDistinct)) + { + Sql.AppendLine("SELECT * FROM ("); + + using (Sql.Indent()) + { + Visit(operand); + } + + Sql.AppendLine().Append(")"); + } + else + { + Visit(operand); + } + + static bool TryUnwrapBareSetOperation(SelectExpression selectExpression, [NotNullWhen(true)] out SetOperationBase? setOperation) + { + if (selectExpression is + { + Tables: [SetOperationBase s], + Predicate: null, + Orderings: [], + Offset: null, + Limit: null, + IsDistinct: false, + Having: null, + GroupBy: [] + } + && selectExpression.Projection.Count == s.Source1.Projection.Count + && selectExpression.Projection.Select( + (pe, index) => pe.Expression is ColumnExpression column + && column.TableAlias == s.Alias + && column.Name == s.Source1.Projection[index].Alias) + .All(e => e)) + { + setOperation = s; + return true; + } + + setOperation = null; + return false; + } + } private void GenerateGlob(GlobExpression globExpression, bool negated = false) { diff --git a/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs index a1164523546..4478b052877 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindSetOperationsQueryTestBase.cs @@ -79,6 +79,19 @@ public virtual Task Except_nested(bool async) .Except(ss.Set().Where(s => s.City == "México D.F.")) .Except(ss.Set().Where(e => e.City == "Seattle"))); + // EXCEPT is non-commutative, unlike UNION/INTERSECT. Therefore, parentheses are needed in the following query + // to ensure that the inner EXCEPT is evaluated first. See #36105. + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Except_nested2(bool async) + => AssertQuery( + async, + ss => ss.Set() + .Except(ss.Set() + .Where(s => s.City == "Seattle") + .Except(ss.Set() + .Where(e => e.City == "Seattle")))); + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Except_non_entity(bool async) @@ -218,6 +231,17 @@ public virtual Task Union_Intersect(bool async) .Union(ss.Set().Where(c => c.City == "London")) .Intersect(ss.Set().Where(c => c.ContactName.Contains("Thomas")))); + // The evaluation order of Concat and Union can matter: A UNION ALL (B UNION C) can be different from (A UNION ALL B) UNION C. + // Make sure parentheses are added. + [ConditionalTheory] + [MemberData(nameof(IsAsyncData))] + public virtual Task Union_inside_Concat(bool async) + => AssertQuery( + async, + ss => ss.Set().Where(c => c.City == "Berlin") + .Concat(ss.Set().Where(c => c.City == "London") + .Union(ss.Set().Where(c => c.City == "Berlin")))); + [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Union_Take_Union_Take(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs index 0077db190b6..1f275431265 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSetOperationsQuerySqlServerTest.cs @@ -200,6 +200,28 @@ WHERE [c1].[ContactName] LIKE N'%Thomas%' """); } + public override async Task Union_inside_Concat(bool async) + { + await base.Union_inside_Concat(async); + +AssertSql( +""" +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].[City] = N'Berlin' +UNION ALL +( + SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] + FROM [Customers] AS [c0] + WHERE [c0].[City] = N'London' + UNION + SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region] + FROM [Customers] AS [c1] + WHERE [c1].[City] = N'Berlin' +) +"""); + } + public override async Task Union_Take_Union_Take(bool async) { await base.Union_Take_Union_Take(async); @@ -1233,14 +1255,16 @@ public override async Task Except_nested(bool async) await base.Except_nested(async); AssertSql( - """ -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].[ContactTitle] = N'Owner' -EXCEPT -SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] -FROM [Customers] AS [c0] -WHERE [c0].[City] = N'México D.F.' +""" +( + 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].[ContactTitle] = N'Owner' + EXCEPT + SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] + FROM [Customers] AS [c0] + WHERE [c0].[City] = N'México D.F.' +) EXCEPT SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region] FROM [Customers] AS [c1] @@ -1248,6 +1272,27 @@ FROM [Customers] AS [c1] """); } + public override async Task Except_nested2(bool async) + { + await base.Except_nested2(async); + + AssertSql( +""" +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] +EXCEPT +( + SELECT [c0].[CustomerID], [c0].[Address], [c0].[City], [c0].[CompanyName], [c0].[ContactName], [c0].[ContactTitle], [c0].[Country], [c0].[Fax], [c0].[Phone], [c0].[PostalCode], [c0].[Region] + FROM [Customers] AS [c0] + WHERE [c0].[City] = N'Seattle' + EXCEPT + SELECT [c1].[CustomerID], [c1].[Address], [c1].[City], [c1].[CompanyName], [c1].[ContactName], [c1].[ContactTitle], [c1].[Country], [c1].[Fax], [c1].[Phone], [c1].[PostalCode], [c1].[Region] + FROM [Customers] AS [c1] + WHERE [c1].[City] = N'Seattle' +) +"""); + } + public override async Task Intersect_non_entity(bool async) { await base.Intersect_non_entity(async);