#23267 Warn when Orderby is silently ignored when using Distinct (#24160

)

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -387,8 +387,15 @@ private static ShapedQueryExpression CreateShapedQueryExpression(IEntityType ent
         {
             Check.NotNull(source, nameof(source));
 
-            ((SelectExpression)source.QueryExpression).ApplyDistinct();
+            var selectExpression = (SelectExpression)source.QueryExpression;
+            if (selectExpression.Orderings.Count > 0
+                && selectExpression.Limit == null
+                && selectExpression.Offset == null)
+            {
+                _queryCompilationContext.Logger.DistinctAfterOrderByWithoutRowLimitingOperatorWarning();
+            }
 
+            selectExpression.ApplyDistinct();
             return source;
         }
 

src/EFCore/Diagnostics/CoreEventId.cs

@@ -72,6 +72,7 @@ private enum Id
             InvalidIncludePathError,
             QueryCompilationStarting,
             NavigationBaseIncluded,
+            DistinctAfterOrderByWithoutRowLimitingOperatorWarning,
 
             // Infrastructure events
             SensitiveDataLoggingEnabledWarning = CoreBaseId + 400,
@@ -274,6 +275,18 @@ public static readonly EventId RowLimitingOperationWithoutOrderByWarning
         public static readonly EventId FirstWithoutOrderByAndFilterWarning
             = MakeQueryId(Id.FirstWithoutOrderByAndFilterWarning);
 
+        /// <summary>
+        ///     <para>
+        ///         The query uses the 'Distinct' operator after applying an ordering. If there are any row limiting operation used before `Distinct` and after ordering then ordering will be used for it.
+        ///         Ordering(s) will be erased after `Distinct` and results afterwards would be unordered.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Query" /> category.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId DistinctAfterOrderByWithoutRowLimitingOperatorWarning
+            = MakeQueryId(Id.DistinctAfterOrderByWithoutRowLimitingOperatorWarning);
+
         private static readonly string _infraPrefix = DbLoggerCategory.Infrastructure.Name + ".";
 
         private static EventId MakeInfraId(Id id)

src/EFCore/Diagnostics/CoreLoggerExtensions.cs

@@ -386,6 +386,36 @@ private static string RowLimitingOperationWithoutOrderByWarning(EventDefinitionB
             return d.GenerateMessage();
         }
 
+        /// <summary>
+        ///     Logs for the <see cref="CoreEventId.DistinctAfterOrderByWithoutRowLimitingOperatorWarning" /> event.
+        /// </summary>
+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        public static void DistinctAfterOrderByWithoutRowLimitingOperatorWarning(
+            [NotNull] this IDiagnosticsLogger<DbLoggerCategory.Query> diagnostics)
+        {
+            var definition = CoreResources.LogDistinctAfterOrderByWithoutRowLimitingOperatorWarning(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics);
+            }
+
+            if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+            {
+                var eventData = new EventData(
+                    definition,
+                    DistinctAfterOrderByWithoutRowLimitingOperatorWarning);
+
+                diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+            }
+        }
+
+        private static string DistinctAfterOrderByWithoutRowLimitingOperatorWarning(EventDefinitionBase definition, EventData payload)
+        {
+            var d = (EventDefinition)definition;
+            return d.GenerateMessage();
+        }
+
         /// <summary>
         ///     Logs for the <see cref="CoreEventId.NavigationBaseIncluded" /> event.
         /// </summary>

src/EFCore/Diagnostics/LoggingDefinitions.cs

@@ -637,6 +637,15 @@ public abstract class LoggingDefinitions
         [EntityFrameworkInternal]
         public EventDefinitionBase? LogFirstWithoutOrderByAndFilter;
 
+        /// <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>
+        [EntityFrameworkInternal]
+        public EventDefinitionBase? LogDistinctAfterOrderByWithoutRowLimitingOperatorWarning;
+
         /// <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

src/EFCore/Properties/CoreStrings.resx

@@ -766,6 +766,10 @@
     <value>The query uses the 'First'/'FirstOrDefault' operator without 'OrderBy' and filter operators. This may lead to unpredictable results.</value>
     <comment>Warning CoreEventId.FirstWithoutOrderByAndFilterWarning</comment>
   </data>
+  <data name="LogDistinctAfterOrderByWithoutRowLimitingOperatorWarning" xml:space="preserve">
+    <value>The query uses the 'Distinct' operator after applying an ordering. If there are any row limiting operation used before 'Distinct' and after ordering then ordering will be used for it. Ordering(s) will be erased after 'Distinct' and results afterwards would be unordered.</value>
+    <comment>Warning CoreEventId.DistinctAfterOrderByWithoutRowLimitingOperatorWarning</comment>
+  </data>
   <data name="LogForeignKeyAttributesOnBothNavigations" xml:space="preserve">
     <value>Navigations '{dependentEntityType}.{dependentNavigation}' and '{principalEntityType}.{principalNavigation}' were separated into two relationships since the [ForeignKey] attribute was specified on navigations on both sides.</value>
     <comment>Warning CoreEventId.ForeignKeyAttributesOnBothNavigationsWarning string string string string</comment>
@@ -911,7 +915,7 @@
     <comment>Debug CoreEventId.RequiredAttributeOnSkipNavigation string string</comment>
   </data>
   <data name="LogRowLimitingOperationWithoutOrderBy" xml:space="preserve">
-    <value>The query uses a row limiting operator ('Skip'/'Take') without an 'OrderBy' operator. This may lead to unpredictable results.</value>
+    <value>The query uses a row limiting operator ('Skip'/'Take') without an 'OrderBy' operator. This may lead to unpredictable results. If the 'Distinct' operator is used after 'OrderBy', then make sure to use the 'OrderBy' operator after 'Distinct' as the ordering would otherwise get erased.</value>
     <comment>Warning CoreEventId.RowLimitingOperationWithoutOrderByWarning</comment>
   </data>
   <data name="LogSaveChangesCompleted" xml:space="preserve">

test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs

@@ -3337,7 +3337,6 @@ public virtual Task Member_doesnt_get_pushed_down_into_subquery_with_result_oper
                     from l1 in ss.Set<Level1>()
                     where l1.Id < 3
                     select (from l3 in ss.Set<Level3>()
-                            orderby l3.Id
                             select l3).Distinct().OrderBy(l => l.Id).Skip(1).FirstOrDefault().Name);
         }
 
@@ -5697,7 +5696,6 @@ public virtual Task Distinct_skip_without_orderby(bool async)
                 ss => from l1 in ss.Set<Level1>()
                       where l1.Id < 3
                       select (from l3 in ss.Set<Level3>()
-                              orderby l3.Id
                               select l3).Distinct().Skip(1).OrderBy(e => e.Id).FirstOrDefault().Name);
         }
 
@@ -5710,7 +5708,6 @@ public virtual Task Distinct_take_without_orderby(bool async)
                 ss => from l1 in ss.Set<Level1>()
                       where l1.Id < 3
                       select (from l3 in ss.Set<Level3>()
-                              orderby l3.Id
                               select l3).Distinct().Take(1).OrderBy(e => e.Id).FirstOrDefault().Name);
         }
 

test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs

@@ -4412,7 +4412,7 @@ public virtual Task Correlated_collections_with_Distinct(bool async)
         {
             return AssertQuery(
                 async,
-                ss => ss.Set<Squad>().OrderBy(s => s.Name).Select(s => s.Members.OrderBy(m => m.Nickname).Distinct()),
+                ss => ss.Set<Squad>().OrderBy(s => s.Name).Select(s => s.Members.OrderBy(m => m.Nickname).Skip(0).Distinct()),
                 assertOrder: true,
                 elementAsserter: (e, a) => AssertCollection(e, a, ordered: true));
         }

test/EFCore.Specification.Tests/Query/NorthwindQueryFixtureBase.cs

@@ -111,6 +111,7 @@ public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder build
                 c => c
                     .Log(CoreEventId.RowLimitingOperationWithoutOrderByWarning)
                     .Log(CoreEventId.FirstWithoutOrderByAndFilterWarning)
+                    .Log(CoreEventId.DistinctAfterOrderByWithoutRowLimitingOperatorWarning)
                     .Log(CoreEventId.PossibleUnintendedCollectionNavigationNullComparisonWarning)
                     .Log(CoreEventId.PossibleUnintendedReferenceComparisonWarning));
     }

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

@@ -4221,13 +4221,19 @@ public override async Task Correlated_collections_with_Distinct(bool async)
             await base.Correlated_collections_with_Distinct(async);
 
             AssertSql(
-                @"SELECT [s].[Id], [t].[Nickname], [t].[SquadId], [t].[AssignedCityName], [t].[CityOfBirthName], [t].[Discriminator], [t].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank]
+                @"SELECT [s].[Id], [t0].[Nickname], [t0].[SquadId], [t0].[AssignedCityName], [t0].[CityOfBirthName], [t0].[Discriminator], [t0].[FullName], [t0].[HasSoulPatch], [t0].[LeaderNickname], [t0].[LeaderSquadId], [t0].[Rank]
 FROM [Squads] AS [s]
-LEFT JOIN (
-    SELECT DISTINCT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
-    FROM [Gears] AS [g]
-) AS [t] ON [s].[Id] = [t].[SquadId]
-ORDER BY [s].[Name], [s].[Id], [t].[Nickname], [t].[SquadId]");
+OUTER APPLY (
+    SELECT DISTINCT [t].[Nickname], [t].[SquadId], [t].[AssignedCityName], [t].[CityOfBirthName], [t].[Discriminator], [t].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank]
+    FROM (
+        SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[Discriminator], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank]
+        FROM [Gears] AS [g]
+        WHERE [s].[Id] = [g].[SquadId]
+        ORDER BY [g].[Nickname]
+        OFFSET 0 ROWS
+    ) AS [t]
+) AS [t0]
+ORDER BY [s].[Name], [s].[Id], [t0].[Nickname], [t0].[SquadId]");
         }
 
         public override async Task Correlated_collections_with_FirstOrDefault(bool async)

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

@@ -133,6 +133,19 @@ public virtual void FirstOrDefault_without_filter_order_by()
                 Fixture.TestSqlLoggerFactory.Log[1].Message);
         }
 
+        [ConditionalFact]
+        public virtual void Distinct_used_after_order_by()
+        {
+            using var context = CreateContext();
+            var customers = context.Set<Customer>().OrderBy(x => x.Address).Distinct().Take(5).ToList();
+
+            Assert.NotEmpty(customers);
+
+            Assert.Equal(
+                CoreResources.LogDistinctAfterOrderByWithoutRowLimitingOperatorWarning(new TestLogger<SqlServerLoggingDefinitions>()).GenerateMessage(),
+                Fixture.TestSqlLoggerFactory.Log[1].Message);
+        }
+
         [ConditionalFact]
         public void SelectExpression_does_not_use_an_old_logger()
         {

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

@@ -5038,16 +5038,22 @@ public override async Task Correlated_collections_with_Distinct(bool async)
             await base.Correlated_collections_with_Distinct(async);
 
             AssertSql(
-                @"SELECT [s].[Id], [t].[Nickname], [t].[SquadId], [t].[AssignedCityName], [t].[CityOfBirthName], [t].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank], [t].[Discriminator]
+                @"SELECT [s].[Id], [t0].[Nickname], [t0].[SquadId], [t0].[AssignedCityName], [t0].[CityOfBirthName], [t0].[FullName], [t0].[HasSoulPatch], [t0].[LeaderNickname], [t0].[LeaderSquadId], [t0].[Rank], [t0].[Discriminator]
 FROM [Squads] AS [s]
-LEFT JOIN (
-    SELECT DISTINCT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank], CASE
-        WHEN [o].[Nickname] IS NOT NULL THEN N'Officer'
-    END AS [Discriminator]
-    FROM [Gears] AS [g]
-    LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[SquadId] = [o].[SquadId])
-) AS [t] ON [s].[Id] = [t].[SquadId]
-ORDER BY [s].[Name], [s].[Id], [t].[Nickname], [t].[SquadId]");
+OUTER APPLY (
+    SELECT DISTINCT [t].[Nickname], [t].[SquadId], [t].[AssignedCityName], [t].[CityOfBirthName], [t].[FullName], [t].[HasSoulPatch], [t].[LeaderNickname], [t].[LeaderSquadId], [t].[Rank], [t].[Discriminator]
+    FROM (
+        SELECT [g].[Nickname], [g].[SquadId], [g].[AssignedCityName], [g].[CityOfBirthName], [g].[FullName], [g].[HasSoulPatch], [g].[LeaderNickname], [g].[LeaderSquadId], [g].[Rank], CASE
+            WHEN [o].[Nickname] IS NOT NULL THEN N'Officer'
+        END AS [Discriminator]
+        FROM [Gears] AS [g]
+        LEFT JOIN [Officers] AS [o] ON ([g].[Nickname] = [o].[Nickname]) AND ([g].[SquadId] = [o].[SquadId])
+        WHERE [s].[Id] = [g].[SquadId]
+        ORDER BY [g].[Nickname]
+        OFFSET 0 ROWS
+    ) AS [t]
+) AS [t0]
+ORDER BY [s].[Name], [s].[Id], [t0].[Nickname], [t0].[SquadId]");
         }
 
         public override async Task Correlated_collections_with_FirstOrDefault(bool async)

test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs

@@ -184,6 +184,12 @@ public override async Task Correlated_collection_after_distinct_3_levels(bool as
                 (await Assert.ThrowsAsync<InvalidOperationException>(
                     () => base.Correlated_collection_after_distinct_3_levels(async))).Message);
 
+        public override async Task Correlated_collections_with_Distinct(bool async)
+            => Assert.Equal(
+                SqliteStrings.ApplyNotSupported,
+                (await Assert.ThrowsAsync<InvalidOperationException>(
+                    () => base.Correlated_collections_with_Distinct(async))).Message);
+
         public override async Task Negate_on_binary_expression(bool async)
         {
             await base.Negate_on_binary_expression(async);

test/EFCore.Sqlite.FunctionalTests/Query/TPTGearsOfWarQuerySqliteTest.cs

@@ -185,6 +185,12 @@ public override async Task Correlated_collection_after_distinct_3_levels(bool as
                 (await Assert.ThrowsAsync<InvalidOperationException>(
                     () => base.Correlated_collection_after_distinct_3_levels(async))).Message);
 
+        public override async Task Correlated_collections_with_Distinct(bool async)
+            => Assert.Equal(
+                SqliteStrings.ApplyNotSupported,
+                (await Assert.ThrowsAsync<InvalidOperationException>(
+                    () => base.Correlated_collections_with_Distinct(async))).Message);
+
         public override async Task Negate_on_binary_expression(bool async)
         {
             await base.Negate_on_binary_expression(async);