From ca78b8de7087f5d761c442c0fdaecc4cf3fb6f83 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Wed, 18 May 2022 14:33:23 -0700 Subject: [PATCH] SqlServer: Throw for split query with skip without order by Resolves #21202 --- ...lationalSplitCollectionShaperExpression.cs | 2 +- .../SqlServerServiceCollectionExtensions.cs | 1 + .../Properties/SqlServerStrings.Designer.cs | 6 ++ .../Properties/SqlServerStrings.resx | 61 +++++++------- .../SqlServerQueryTranslationPostprocessor.cs | 82 +++++++++++++++++++ ...verQueryTranslationPostprocessorFactory.cs | 49 +++++++++++ ...plitIncludeNoTrackingQuerySqlServerTest.cs | 55 ++----------- ...NorthwindSplitIncludeQuerySqlServerTest.cs | 55 ++----------- 8 files changed, 181 insertions(+), 130 deletions(-) create mode 100644 src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs create mode 100644 src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessorFactory.cs diff --git a/src/EFCore.Relational/Query/RelationalSplitCollectionShaperExpression.cs b/src/EFCore.Relational/Query/RelationalSplitCollectionShaperExpression.cs index 289d62143e0..8459a03da41 100644 --- a/src/EFCore.Relational/Query/RelationalSplitCollectionShaperExpression.cs +++ b/src/EFCore.Relational/Query/RelationalSplitCollectionShaperExpression.cs @@ -124,7 +124,7 @@ public virtual RelationalSplitCollectionShaperExpression Update( /// void IPrintableExpression.Print(ExpressionPrinter expressionPrinter) { - expressionPrinter.AppendLine("RelationalCollectionShaper:"); + expressionPrinter.AppendLine("RelationalSplitCollectionShaperExpression:"); using (expressionPrinter.Indent()) { expressionPrinter.Append("ParentIdentifier:"); diff --git a/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs index 7588e14adec..7a4587438c6 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs @@ -125,6 +125,7 @@ public static IServiceCollection AddEntityFrameworkSqlServer(this IServiceCollec .TryAdd() .TryAdd() .TryAdd() + .TryAdd() .TryAdd() .TryAdd() .TryAdd() diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs index 9efc6efc08b..aeb4ab9fa6d 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs @@ -372,6 +372,12 @@ public static string TemporalSetOperationOnMismatchedSources(object? entityType) public static string TransientExceptionDetected => GetString("TransientExceptionDetected"); + /// + /// The query uses 'Skip' without specifying ordering and uses split query mode. This generates incorrect results. Either provide ordering or run query in single query mode using `AsSingleQuery()`. See https://go.microsoft.com/fwlink/?linkid=2196526 for more information. + /// + public static string SplitQueryOffsetWithoutOrderBy + => GetString("SplitQueryOffsetWithoutOrderBy"); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name)!; diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx index 79abd10b741..a56bc54b8d4 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx @@ -1,17 +1,17 @@  - @@ -251,7 +251,7 @@ The database user has not been granted 'VIEW DEFINITION' rights. Scaffolding requires these rights to construct the Entity Framework model correctly. Without these rights, parts of the scaffolded model may be missing, resulting in incorrect interactions between Entity Framework and the database at runtime. Warning SqlServerEventId.MissingViewDefinitionRightsWarning string? - + Skipping foreign key with identity '{id}' on table '{tableName}', since the principal column '{principalColumnName}' on the foreign key's principal table, '{principalTableName}', was not found in the model. Warning SqlServerEventId.ForeignKeyPrincipalColumnMissingWarning string string string string @@ -341,4 +341,7 @@ An exception has been raised that is likely due to a transient failure. Consider enabling transient error resiliency by adding 'EnableRetryOnFailure' to the 'UseSqlServer' call. - + + The query uses 'Skip' without specifying ordering and uses split query mode. This generates incorrect results. Either provide ordering or run query in single query mode using `AsSingleQuery()`. See https://go.microsoft.com/fwlink/?linkid=2196526 for more information. + + \ No newline at end of file diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs new file mode 100644 index 00000000000..973c5f71cbd --- /dev/null +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs @@ -0,0 +1,82 @@ +// 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; +using Microsoft.EntityFrameworkCore.SqlServer.Internal; + +namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal; + +/// +/// 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 +/// any release. You should only use it directly in your code with extreme caution and knowing that +/// doing so can result in application failures when updating to a new Entity Framework Core release. +/// +public class SqlServerQueryTranslationPostprocessor : RelationalQueryTranslationPostprocessor +{ + private readonly SkipWithoutOrderByInSplitQueryVerifyingExpressionVisitor + _skipWithoutOrderByInSplitQueryVerifyingExpressionVisitor = new(); + + /// + /// 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 + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public SqlServerQueryTranslationPostprocessor( + QueryTranslationPostprocessorDependencies dependencies, + RelationalQueryTranslationPostprocessorDependencies relationalDependencies, + QueryCompilationContext queryCompilationContext) + : base(dependencies, relationalDependencies, queryCompilationContext) + { + } + + /// + /// 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 + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public override Expression Process(Expression query) + { + var result = base.Process(query); + + _skipWithoutOrderByInSplitQueryVerifyingExpressionVisitor.Visit(result); + + return result; + } + + private sealed class SkipWithoutOrderByInSplitQueryVerifyingExpressionVisitor : ExpressionVisitor + { + [return: NotNullIfNotNull("expression")] + public override Expression? Visit(Expression? expression) + { + switch (expression) + { + case ShapedQueryExpression shapedQueryExpression: + Visit(shapedQueryExpression.ShaperExpression); + return shapedQueryExpression; + + case RelationalSplitCollectionShaperExpression relationalSplitCollectionShaperExpression: + foreach (var table in relationalSplitCollectionShaperExpression.SelectExpression.Tables) + { + Visit(table); + } + + Visit(relationalSplitCollectionShaperExpression.InnerShaper); + + return relationalSplitCollectionShaperExpression; + + case SelectExpression selectExpression + when selectExpression.Offset != null + && selectExpression.Orderings.Count == 0: + throw new InvalidOperationException(SqlServerStrings.SplitQueryOffsetWithoutOrderBy); + + + default: + return base.Visit(expression); + } + } + } +} diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessorFactory.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessorFactory.cs new file mode 100644 index 00000000000..de09d1dc5c2 --- /dev/null +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessorFactory.cs @@ -0,0 +1,49 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal; + +/// +/// 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 +/// any release. You should only use it directly in your code with extreme caution and knowing that +/// doing so can result in application failures when updating to a new Entity Framework Core release. +/// +public class SqlServerQueryTranslationPostprocessorFactory : IQueryTranslationPostprocessorFactory +{ + /// + /// 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 + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public SqlServerQueryTranslationPostprocessorFactory( + QueryTranslationPostprocessorDependencies dependencies, + RelationalQueryTranslationPostprocessorDependencies relationalDependencies) + { + Dependencies = dependencies; + RelationalDependencies = relationalDependencies; + } + + /// + /// Dependencies for this service. + /// + protected virtual QueryTranslationPostprocessorDependencies Dependencies { get; } + + /// + /// Relational provider-specific dependencies for this service. + /// + protected virtual RelationalQueryTranslationPostprocessorDependencies RelationalDependencies { get; } + + /// + /// 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 + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual QueryTranslationPostprocessor Create(QueryCompilationContext queryCompilationContext) + => new SqlServerQueryTranslationPostprocessor( + Dependencies, + RelationalDependencies, + queryCompilationContext); +} diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeNoTrackingQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeNoTrackingQuerySqlServerTest.cs index 8474dfd5852..41dbe06312c 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeNoTrackingQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeNoTrackingQuerySqlServerTest.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 Microsoft.EntityFrameworkCore.SqlServer.Internal; using Xunit.Sdk; namespace Microsoft.EntityFrameworkCore.Query; @@ -24,62 +25,16 @@ public virtual void Check_all_tests_overridden() public override async Task Include_collection_skip_take_no_order_by(bool async) { - // Split query Skip without OrderBy. Issue #21202. Assert.Equal( - "0", - (((EqualException)(await Assert.ThrowsAsync( - () => base.Include_collection_skip_take_no_order_by(async))).InnerException!.InnerException)!).Actual); - - AssertSql( - @"@__p_0='10' -@__p_1='5' - -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] -ORDER BY [c].[CustomerID] -OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY", - // - @"@__p_0='10' -@__p_1='5' - -SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate], [t].[CustomerID] -FROM ( - SELECT [c].[CustomerID] - FROM [Customers] AS [c] - ORDER BY (SELECT 1) - OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY -) AS [t] -INNER JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] -ORDER BY [t].[CustomerID]"); + SqlServerStrings.SplitQueryOffsetWithoutOrderBy, + (await Assert.ThrowsAsync(() => base.Include_collection_skip_take_no_order_by(async))).Message); } public override async Task Include_collection_skip_no_order_by(bool async) { - // Split query Skip without OrderBy. Issue #21202. Assert.Equal( - "0", - (((EqualException)(await Assert.ThrowsAsync( - () => base.Include_collection_skip_no_order_by(async))).InnerException!.InnerException)!).Actual); - - AssertSql( - @"@__p_0='10' - -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] -ORDER BY [c].[CustomerID] -OFFSET @__p_0 ROWS", - // - @"@__p_0='10' - -SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate], [t].[CustomerID] -FROM ( - SELECT [c].[CustomerID] - FROM [Customers] AS [c] - ORDER BY (SELECT 1) - OFFSET @__p_0 ROWS -) AS [t] -INNER JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] -ORDER BY [t].[CustomerID]"); + SqlServerStrings.SplitQueryOffsetWithoutOrderBy, + (await Assert.ThrowsAsync(() => base.Include_collection_skip_no_order_by(async))).Message); } public override async Task Include_reference_GroupBy_Select(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs index 8120fc6b61d..db17c8ebb18 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.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 Microsoft.EntityFrameworkCore.SqlServer.Internal; using Microsoft.EntityFrameworkCore.TestModels.Northwind; using Xunit.Sdk; @@ -106,31 +107,9 @@ ORDER BY [c].[CompanyName] DESC public override async Task Include_collection_skip_no_order_by(bool async) { - // Split query Skip without OrderBy. Issue #21202. Assert.Equal( - "0", - (((EqualException)(await Assert.ThrowsAsync( - () => base.Include_collection_skip_no_order_by(async))).InnerException!.InnerException)!).Actual); - - AssertSql( - @"@__p_0='10' - -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] -ORDER BY [c].[CustomerID] -OFFSET @__p_0 ROWS", - // - @"@__p_0='10' - -SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate], [t].[CustomerID] -FROM ( - SELECT [c].[CustomerID] - FROM [Customers] AS [c] - ORDER BY (SELECT 1) - OFFSET @__p_0 ROWS -) AS [t] -INNER JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] -ORDER BY [t].[CustomerID]"); + SqlServerStrings.SplitQueryOffsetWithoutOrderBy, + (await Assert.ThrowsAsync(() => base.Include_collection_skip_no_order_by(async))).Message); } public override async Task Include_collection_take_no_order_by(bool async) @@ -157,33 +136,9 @@ FROM [Customers] AS [c] public override async Task Include_collection_skip_take_no_order_by(bool async) { - // Split query Skip without OrderBy. Issue #21202. Assert.Equal( - "0", - (((EqualException)(await Assert.ThrowsAsync( - () => base.Include_collection_skip_take_no_order_by(async))).InnerException!.InnerException)!).Actual); - - AssertSql( - @"@__p_0='10' -@__p_1='5' - -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] -ORDER BY [c].[CustomerID] -OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY", - // - @"@__p_0='10' -@__p_1='5' - -SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate], [t].[CustomerID] -FROM ( - SELECT [c].[CustomerID] - FROM [Customers] AS [c] - ORDER BY (SELECT 1) - OFFSET @__p_0 ROWS FETCH NEXT @__p_1 ROWS ONLY -) AS [t] -INNER JOIN [Orders] AS [o] ON [t].[CustomerID] = [o].[CustomerID] -ORDER BY [t].[CustomerID]"); + SqlServerStrings.SplitQueryOffsetWithoutOrderBy, + (await Assert.ThrowsAsync(() => base.Include_collection_skip_take_no_order_by(async))).Message); } public override async Task Include_reference_and_collection(bool async)