SqlServer: Throw for split query with skip without order by (#28056)

Resolves #21202

src/EFCore.Relational/Query/RelationalSplitCollectionShaperExpression.cs

@@ -124,7 +124,7 @@ public virtual RelationalSplitCollectionShaperExpression Update(
     /// <inheritdoc />
     void IPrintableExpression.Print(ExpressionPrinter expressionPrinter)
     {
-        expressionPrinter.AppendLine("RelationalCollectionShaper:");
+        expressionPrinter.AppendLine("RelationalSplitCollectionShaperExpression:");
         using (expressionPrinter.Indent())
         {
             expressionPrinter.Append("ParentIdentifier:");

src/EFCore.SqlServer/Extensions/SqlServerServiceCollectionExtensions.cs

@@ -125,6 +125,7 @@ public static IServiceCollection AddEntityFrameworkSqlServer(this IServiceCollec
             .TryAdd<IQuerySqlGeneratorFactory, SqlServerQuerySqlGeneratorFactory>()
             .TryAdd<IRelationalSqlTranslatingExpressionVisitorFactory, SqlServerSqlTranslatingExpressionVisitorFactory>()
             .TryAdd<ISqlExpressionFactory, SqlServerSqlExpressionFactory>()
+            .TryAdd<IQueryTranslationPostprocessorFactory, SqlServerQueryTranslationPostprocessorFactory>()
             .TryAdd<IRelationalParameterBasedSqlProcessorFactory, SqlServerParameterBasedSqlProcessorFactory>()
             .TryAdd<INavigationExpansionExtensibilityHelper, SqlServerNavigationExpansionExtensibilityHelper>()
             .TryAdd<IQueryableMethodTranslatingExpressionVisitorFactory, SqlServerQueryableMethodTranslatingExpressionVisitorFactory>()

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!--
-    Microsoft ResX Schema
-
+  <!-- 
+    Microsoft ResX Schema 
+    
     Version 2.0
-
-    The primary goals of this format is to allow a simple XML format
-    that is mostly human readable. The generation and parsing of the
-    various data types are done through the TypeConverter classes
+    
+    The primary goals of this format is to allow a simple XML format 
+    that is mostly human readable. The generation and parsing of the 
+    various data types are done through the TypeConverter classes 
     associated with the data types.
-
+    
     Example:
-
+    
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-
-    There are any number of "resheader" rows that contain simple
+                
+    There are any number of "resheader" rows that contain simple 
     name/value pairs.
-
-    Each data row contains a name, and value. The row also contains a
-    type or mimetype. Type corresponds to a .NET class that support
-    text/value conversion through the TypeConverter architecture.
-    Classes that don't support this are serialized and stored with the
+    
+    Each data row contains a name, and value. The row also contains a 
+    type or mimetype. Type corresponds to a .NET class that support 
+    text/value conversion through the TypeConverter architecture. 
+    Classes that don't support this are serialized and stored with the 
     mimetype set.
-
-    The mimetype is used for serialized objects, and tells the
-    ResXResourceReader how to depersist the object. This is currently not
+    
+    The mimetype is used for serialized objects, and tells the 
+    ResXResourceReader how to depersist the object. This is currently not 
     extensible. For a given mimetype the value must be set accordingly:
-
-    Note - application/x-microsoft.net.object.binary.base64 is the format
-    that the ResXResourceWriter will generate, however the reader can
+    
+    Note - application/x-microsoft.net.object.binary.base64 is the format 
+    that the ResXResourceWriter will generate, however the reader can 
     read any of the formats listed below.
-
+    
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-
+    
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array
+    value   : The object must be serialized into a byte array 
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -251,7 +251,7 @@
   <data name="LogMissingViewDefinitionRights" xml:space="preserve">
     <value>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.</value>
     <comment>Warning SqlServerEventId.MissingViewDefinitionRightsWarning string?</comment>
-  </data>  
+  </data>
   <data name="LogPrincipalColumnNotFound" xml:space="preserve">
     <value>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.</value>
     <comment>Warning SqlServerEventId.ForeignKeyPrincipalColumnMissingWarning string string string string</comment>
@@ -341,4 +341,7 @@
   <data name="TransientExceptionDetected" xml:space="preserve">
     <value>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.</value>
   </data>
-</root>
+  <data name="SplitQueryOffsetWithoutOrderBy" xml:space="preserve">
+    <value>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.</value>
+  </data>
+</root>

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;
+
+/// <summary>
+///     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.
+/// </summary>
+public class SqlServerQueryTranslationPostprocessor : RelationalQueryTranslationPostprocessor
+{
+    private readonly SkipWithoutOrderByInSplitQueryVerifyingExpressionVisitor
+        _skipWithoutOrderByInSplitQueryVerifyingExpressionVisitor = new();
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    public SqlServerQueryTranslationPostprocessor(
+        QueryTranslationPostprocessorDependencies dependencies,
+        RelationalQueryTranslationPostprocessorDependencies relationalDependencies,
+        QueryCompilationContext queryCompilationContext)
+        : base(dependencies, relationalDependencies, queryCompilationContext)
+    {
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    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);
+            }
+        }
+    }
+}

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;
+
+/// <summary>
+///     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.
+/// </summary>
+public class SqlServerQueryTranslationPostprocessorFactory : IQueryTranslationPostprocessorFactory
+{
+    /// <summary>
+    ///     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.
+    /// </summary>
+    public SqlServerQueryTranslationPostprocessorFactory(
+        QueryTranslationPostprocessorDependencies dependencies,
+        RelationalQueryTranslationPostprocessorDependencies relationalDependencies)
+    {
+        Dependencies = dependencies;
+        RelationalDependencies = relationalDependencies;
+    }
+
+    /// <summary>
+    ///     Dependencies for this service.
+    /// </summary>
+    protected virtual QueryTranslationPostprocessorDependencies Dependencies { get; }
+
+    /// <summary>
+    ///     Relational provider-specific dependencies for this service.
+    /// </summary>
+    protected virtual RelationalQueryTranslationPostprocessorDependencies RelationalDependencies { get; }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    public virtual QueryTranslationPostprocessor Create(QueryCompilationContext queryCompilationContext)
+        => new SqlServerQueryTranslationPostprocessor(
+            Dependencies,
+            RelationalDependencies,
+            queryCompilationContext);
+}

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<TargetInvocationException>(
-                () => 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<InvalidOperationException>(() => 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<TargetInvocationException>(
-                () => 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<InvalidOperationException>(() => base.Include_collection_skip_no_order_by(async))).Message);
     }
 
     public override async Task Include_reference_GroupBy_Select(bool async)