From 17dbfdda09c6cac0bc0d307ba977767d4097883b Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:38:02 +1000 Subject: [PATCH 1/7] Only detect explicit ordering in the top level query chain QueryAnalyzer was an ExpressionVisitor that descended into every node, including the lambdas passed to operators. Any OrderBy inside a subquery lambda, for example context.Employees.Where(_ => _.Tasks.OrderBy(task => task.Title).Count() >= 0) set HasOrdering, so the interceptor treated the query as explicitly ordered and applied no default ordering at all. The generated SQL had no ORDER BY. Include and Select lambdas were already guarded against this, but every other operator was not. The same false positive hit filtered Include, where an ordering nested in the filter suppressed the default ordering of the included collection. Replace the visitor with a walk of the chain itself, which is the only place top level ordering and Include can appear. Ordering nested in a lambda now orders its own subquery and leaves the outer query alone. --- src/EfOrderBy/IncludeOrderingApplicator.cs | 2 +- src/EfOrderBy/Interceptor.cs | 14 +-- src/EfOrderBy/QueryAnalyzer.cs | 79 +++++++++------- ...mbda_StillAppliesDefaultOrder.verified.txt | 61 ++++++++++++ ...r_StillOrdersNestedCollection.verified.txt | 94 +++++++++++++++++++ ...mbda_StillAppliesDefaultOrder.verified.txt | 60 ++++++++++++ ...mbda_StillAppliesDefaultOrder.verified.txt | 60 ++++++++++++ src/Tests/NestedOrderingTests.cs | 84 +++++++++++++++++ 8 files changed, 406 insertions(+), 48 deletions(-) create mode 100644 src/Tests/NestedOrderingTests.OrderingInsideAnyLambda_StillAppliesDefaultOrder.verified.txt create mode 100644 src/Tests/NestedOrderingTests.OrderingInsideIncludeFilter_StillOrdersNestedCollection.verified.txt create mode 100644 src/Tests/NestedOrderingTests.OrderingInsideSelectManyLambda_StillAppliesDefaultOrder.verified.txt create mode 100644 src/Tests/NestedOrderingTests.OrderingInsideWhereLambda_StillAppliesDefaultOrder.verified.txt create mode 100644 src/Tests/NestedOrderingTests.cs diff --git a/src/EfOrderBy/IncludeOrderingApplicator.cs b/src/EfOrderBy/IncludeOrderingApplicator.cs index b1f270c..fc42e1b 100644 --- a/src/EfOrderBy/IncludeOrderingApplicator.cs +++ b/src/EfOrderBy/IncludeOrderingApplicator.cs @@ -45,7 +45,7 @@ MethodCallExpression ProcessInclude(MethodCallExpression includeCall) } // Check if the navigation already has ordering - if (Interceptor.HasOrdering(lambda.Body)) + if (QueryAnalyzer.HasOrdering(lambda.Body)) { // Already has explicit ordering, don't apply default if (detectRedundantOrdering) diff --git a/src/EfOrderBy/Interceptor.cs b/src/EfOrderBy/Interceptor.cs index 58c66dc..59d275b 100644 --- a/src/EfOrderBy/Interceptor.cs +++ b/src/EfOrderBy/Interceptor.cs @@ -23,10 +23,9 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven var queryWithOrderedIncludes = visitor.Visit(query); // Analyze the query for ordering and includes in a single pass - var analyzer = new QueryAnalyzer(); - analyzer.Visit(queryWithOrderedIncludes); + var (hasOrdering, hasInclude) = QueryAnalyzer.Analyze(queryWithOrderedIncludes); - if (analyzer.HasOrdering) + if (hasOrdering) { if (detectRedundantOrdering) { @@ -60,14 +59,7 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven // Apply default ordering to the top-level query // If there's a Select projection at the end, we need to insert OrderBy before it - return ApplyOrderingBeforeSelect(queryWithOrderedIncludes, configuration, analyzer.HasInclude); - } - - public static bool HasOrdering(Expression expression) - { - var analyzer = new QueryAnalyzer(); - analyzer.Visit(expression); - return analyzer.HasOrdering; + return ApplyOrderingBeforeSelect(queryWithOrderedIncludes, configuration, hasInclude); } static Expression GetSourceBeforeProjection(Expression expression) diff --git a/src/EfOrderBy/QueryAnalyzer.cs b/src/EfOrderBy/QueryAnalyzer.cs index dbca5d3..c1c61dc 100644 --- a/src/EfOrderBy/QueryAnalyzer.cs +++ b/src/EfOrderBy/QueryAnalyzer.cs @@ -1,46 +1,53 @@ -sealed class QueryAnalyzer : - ExpressionVisitor +/// +/// Scans the outermost chain of a query for existing ordering and for Include calls. +/// +/// +/// Only the chain is walked, never the lambdas passed to it. Ordering nested inside a lambda, +/// for example Where(_ => _.Children.OrderBy(child => child.Name).Any()), orders a subquery and +/// must not suppress the default ordering of the query it is nested in. +/// +static class QueryAnalyzer { - public bool HasOrdering { get; private set; } - public bool HasInclude { get; private set; } - - protected override Expression VisitMethodCall(MethodCallExpression node) + public static (bool HasOrdering, bool HasInclude) Analyze(Expression expression) { - var name = node.Method.Name; - var type = node.Method.DeclaringType; + var hasOrdering = false; + var hasInclude = false; - // Check if this is an Include/ThenInclude - don't look for ordering inside its lambda - if (type == typeof(EntityFrameworkQueryableExtensions) && - name is "Include" or "ThenInclude") + while (expression is MethodCallExpression call) { - HasInclude = true; - // Visit the source (first argument) but skip the lambda (second argument) - // to avoid detecting ordering within Include(_ => _.Collection.OrderBy(...)) - Visit(node.Arguments[0]); - return node; - } + var method = call.Method; + var declaringType = method.DeclaringType; + var name = method.Name; - // Check if this is a Select - don't look for ordering inside its lambda - if (type == typeof(Queryable) && - name == "Select") - { - // Visit the source (first argument) but skip the lambda (second argument) - // to avoid detecting ordering within Select(_ => new { ... _.Children.OrderBy(...) }) - Visit(node.Arguments[0]); - return node; - } + if (declaringType == typeof(EntityFrameworkQueryableExtensions) && + name is "Include" or "ThenInclude") + { + hasInclude = true; + } + else if ((declaringType == typeof(Queryable) || + declaringType == typeof(Enumerable)) && + name is + "OrderBy" or + "OrderByDescending" or + "ThenBy" or + "ThenByDescending") + { + hasOrdering = true; + } - if ((type == typeof(Queryable) || - type == typeof(Enumerable)) && - name is - "OrderBy" or - "OrderByDescending" or - "ThenBy" or - "ThenByDescending") - { - HasOrdering = true; + // Only static operators compose a chain, and the source is always their first argument + if (!method.IsStatic || + call.Arguments.Count == 0) + { + break; + } + + expression = call.Arguments[0]; } - return base.VisitMethodCall(node); + return (hasOrdering, hasInclude); } + + public static bool HasOrdering(Expression expression) => + Analyze(expression).HasOrdering; } diff --git a/src/Tests/NestedOrderingTests.OrderingInsideAnyLambda_StillAppliesDefaultOrder.verified.txt b/src/Tests/NestedOrderingTests.OrderingInsideAnyLambda_StillAppliesDefaultOrder.verified.txt new file mode 100644 index 0000000..5f5d671 --- /dev/null +++ b/src/Tests/NestedOrderingTests.OrderingInsideAnyLambda_StillAppliesDefaultOrder.verified.txt @@ -0,0 +1,61 @@ +{ + target: [ + { + Id: 6, + DepartmentId: 3, + Name: Frank, + HireDate: 2024-04-10, + Salary: 65000 + }, + { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + }, + { + Id: 1, + DepartmentId: 1, + Name: Alice, + HireDate: 2024-01-15, + Salary: 90000 + }, + { + Id: 5, + DepartmentId: 2, + Name: Eve, + HireDate: 2023-11-01, + Salary: 72000 + }, + { + Id: 3, + DepartmentId: 1, + Name: Charlie, + HireDate: 2023-06-10, + Salary: 95000 + } + ], + sql: { + Text: +select e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +where not exists (select 1 + from EmployeeTasks as e0 + where e.Id = e0.EmployeeId + and e0.Priority < 0) +order by e.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/NestedOrderingTests.OrderingInsideIncludeFilter_StillOrdersNestedCollection.verified.txt b/src/Tests/NestedOrderingTests.OrderingInsideIncludeFilter_StillOrdersNestedCollection.verified.txt new file mode 100644 index 0000000..d69afed --- /dev/null +++ b/src/Tests/NestedOrderingTests.OrderingInsideIncludeFilter_StillOrdersNestedCollection.verified.txt @@ -0,0 +1,94 @@ +{ + target: [ + { + Id: 1, + Name: Engineering, + DisplayOrder: 1, + Employees: [ + { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + { + Id: 1, + DepartmentId: 1, + Name: Alice, + HireDate: 2024-01-15, + Salary: 90000 + }, + { + Id: 3, + DepartmentId: 1, + Name: Charlie, + HireDate: 2023-06-10, + Salary: 95000 + } + ] + }, + { + Id: 2, + Name: Sales, + DisplayOrder: 2, + Employees: [ + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + }, + { + Id: 5, + DepartmentId: 2, + Name: Eve, + HireDate: 2023-11-01, + Salary: 72000 + } + ] + }, + { + Id: 3, + Name: HR, + DisplayOrder: 3, + Employees: [ + { + Id: 6, + DepartmentId: 3, + Name: Frank, + HireDate: 2024-04-10, + Salary: 65000 + } + ] + } + ], + sql: { + Text: +select d.Id, + d.DisplayOrder, + d.Name, + e1.Id, + e1.DepartmentId, + e1.HireDate, + e1.Name, + e1.Salary +from Departments as d + left outer join + (select e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary + from Employees as e + where (select COUNT(*) + from EmployeeTasks as e0 + where e.Id = e0.EmployeeId) >= 0) as e1 + on d.Id = e1.DepartmentId +order by d.DisplayOrder, + d.Id, + e1.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/NestedOrderingTests.OrderingInsideSelectManyLambda_StillAppliesDefaultOrder.verified.txt b/src/Tests/NestedOrderingTests.OrderingInsideSelectManyLambda_StillAppliesDefaultOrder.verified.txt new file mode 100644 index 0000000..8cc878a --- /dev/null +++ b/src/Tests/NestedOrderingTests.OrderingInsideSelectManyLambda_StillAppliesDefaultOrder.verified.txt @@ -0,0 +1,60 @@ +{ + target: [ + { + Id: 6, + DepartmentId: 3, + Name: Frank, + HireDate: 2024-04-10, + Salary: 65000 + }, + { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + }, + { + Id: 1, + DepartmentId: 1, + Name: Alice, + HireDate: 2024-01-15, + Salary: 90000 + }, + { + Id: 5, + DepartmentId: 2, + Name: Eve, + HireDate: 2023-11-01, + Salary: 72000 + }, + { + Id: 3, + DepartmentId: 1, + Name: Charlie, + HireDate: 2023-06-10, + Salary: 95000 + } + ], + sql: { + Text: +select e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Departments as d + inner join + Employees as e + on d.Id = e.DepartmentId +order by e.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/NestedOrderingTests.OrderingInsideWhereLambda_StillAppliesDefaultOrder.verified.txt b/src/Tests/NestedOrderingTests.OrderingInsideWhereLambda_StillAppliesDefaultOrder.verified.txt new file mode 100644 index 0000000..693fed2 --- /dev/null +++ b/src/Tests/NestedOrderingTests.OrderingInsideWhereLambda_StillAppliesDefaultOrder.verified.txt @@ -0,0 +1,60 @@ +{ + target: [ + { + Id: 6, + DepartmentId: 3, + Name: Frank, + HireDate: 2024-04-10, + Salary: 65000 + }, + { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + }, + { + Id: 1, + DepartmentId: 1, + Name: Alice, + HireDate: 2024-01-15, + Salary: 90000 + }, + { + Id: 5, + DepartmentId: 2, + Name: Eve, + HireDate: 2023-11-01, + Salary: 72000 + }, + { + Id: 3, + DepartmentId: 1, + Name: Charlie, + HireDate: 2023-06-10, + Salary: 95000 + } + ], + sql: { + Text: +select e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +where (select COUNT(*) + from EmployeeTasks as e0 + where e.Id = e0.EmployeeId) >= 0 +order by e.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/NestedOrderingTests.cs b/src/Tests/NestedOrderingTests.cs new file mode 100644 index 0000000..980f8d4 --- /dev/null +++ b/src/Tests/NestedOrderingTests.cs @@ -0,0 +1,84 @@ +// Ordering that appears inside a lambda belongs to a subquery. It must not be mistaken +// for explicit ordering of the query the lambda is nested in. +[TestFixture] +public class NestedOrderingTests +{ + [Test] + public async Task OrderingInsideWhereLambda_StillAppliesDefaultOrder() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Employees + .Where(_ => _.Tasks.OrderBy(task => task.Title).Count() >= 0) + .ToListAsync(); + + // Employees keep their default ordering of HireDate descending + Assert.That(results.Select(_ => _.Name), Is.EqualTo(new[] + { + "Frank", // 2024-04-10 + "Bob", // 2024-03-20 + "Diana", // 2024-02-05 + "Alice", // 2024-01-15 + "Eve", // 2023-11-01 + "Charlie" // 2023-06-10 + })); + await Verify(results); + } + + [Test] + public async Task OrderingInsideAnyLambda_StillAppliesDefaultOrder() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Employees + .Where(_ => !_.Tasks.OrderBy(task => task.Title).Any(task => task.Priority < 0)) + .ToListAsync(); + + Assert.That(results[0].Name, Is.EqualTo("Frank")); + Assert.That(results[^1].Name, Is.EqualTo("Charlie")); + await Verify(results); + } + + [Test] + public async Task OrderingInsideSelectManyLambda_StillAppliesDefaultOrder() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Departments + .SelectMany(_ => _.Employees.OrderBy(employee => employee.Name)) + .ToListAsync(); + + // The projected employees get their own default ordering of HireDate descending + Assert.That(results[0].Name, Is.EqualTo("Frank")); + Assert.That(results[^1].Name, Is.EqualTo("Charlie")); + await Verify(results); + } + + [Test] + public async Task OrderingInsideIncludeFilter_StillOrdersNestedCollection() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Departments + .Include(_ => _.Employees.Where(employee => employee.Tasks.OrderBy(task => task.Title).Count() >= 0)) + .ToListAsync(); + + // The ordering inside the filter applies to Tasks, so Employees still get their default + var engineering = results[0].Employees; + Assert.That(engineering.Select(_ => _.Name), Is.EqualTo(new[] + { + "Bob", // 2024-03-20 + "Alice", // 2024-01-15 + "Charlie" // 2023-06-10 + })); + await Verify(results); + } +} From 52545039351d8f62fa0460884e27fdd166503375 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:40:53 +1000 Subject: [PATCH 2/7] Apply default ordering before row limiting and single result operators Ordering was only pushed beneath Select and Include, and appended to the end of the chain otherwise. So Take produced select * from (select top(3) * from Employees) order by HireDate desc which takes three arbitrary rows and only then sorts them. Paging could skip and repeat rows across pages while looking correctly sorted, and EF Core still raised RowLimitingOperationWithoutOrderByWarning. First, FirstOrDefault, Last, LastOrDefault and ElementAt were missed entirely. Their result type is the entity rather than IQueryable, so element type detection returned null and no ordering was applied at all, leaving EF Core to raise FirstWithoutOrderByAndFilterWarning. Replace the two placement walks with one predicate naming the operators the ordering has to precede, used both to find the source entity type and to insert the ordering. Aggregates and Single are deliberately excluded, since ordering them is pointless work. --- src/EfOrderBy/Interceptor.cs | 145 ++++++++---------- ...ts.Count_DoesNotApplyOrdering.verified.txt | 9 ++ ...ElementAt_AppliesDefaultOrder.verified.txt | 24 +++ ...Predicate_AppliesDefaultOrder.verified.txt | 21 +++ ...sts.First_AppliesDefaultOrder.verified.txt | 20 +++ ...ThenPage_OrdersBeforeLimiting.verified.txt | 52 +++++++ ...t_AppliesDefaultOrderToSource.verified.txt | 10 ++ ...s.Single_DoesNotApplyOrdering.verified.txt | 20 +++ ...SkipTake_OrdersBeforeLimiting.verified.txt | 34 ++++ ...sts.Take_OrdersBeforeLimiting.verified.txt | 39 +++++ src/Tests/RowLimitingTests.cs | 143 +++++++++++++++++ 11 files changed, 434 insertions(+), 83 deletions(-) create mode 100644 src/Tests/RowLimitingTests.Count_DoesNotApplyOrdering.verified.txt create mode 100644 src/Tests/RowLimitingTests.ElementAt_AppliesDefaultOrder.verified.txt create mode 100644 src/Tests/RowLimitingTests.FirstOrDefaultWithPredicate_AppliesDefaultOrder.verified.txt create mode 100644 src/Tests/RowLimitingTests.First_AppliesDefaultOrder.verified.txt create mode 100644 src/Tests/RowLimitingTests.IncludeThenPage_OrdersBeforeLimiting.verified.txt create mode 100644 src/Tests/RowLimitingTests.SelectThenFirst_AppliesDefaultOrderToSource.verified.txt create mode 100644 src/Tests/RowLimitingTests.Single_DoesNotApplyOrdering.verified.txt create mode 100644 src/Tests/RowLimitingTests.SkipTake_OrdersBeforeLimiting.verified.txt create mode 100644 src/Tests/RowLimitingTests.Take_OrdersBeforeLimiting.verified.txt create mode 100644 src/Tests/RowLimitingTests.cs diff --git a/src/EfOrderBy/Interceptor.cs b/src/EfOrderBy/Interceptor.cs index 59d275b..6187c0b 100644 --- a/src/EfOrderBy/Interceptor.cs +++ b/src/EfOrderBy/Interceptor.cs @@ -23,7 +23,7 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven var queryWithOrderedIncludes = visitor.Visit(query); // Analyze the query for ordering and includes in a single pass - var (hasOrdering, hasInclude) = QueryAnalyzer.Analyze(queryWithOrderedIncludes); + var (hasOrdering, _) = QueryAnalyzer.Analyze(queryWithOrderedIncludes); if (hasOrdering) { @@ -35,12 +35,11 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven return queryWithOrderedIncludes; } - // For queries with Select projection, we need to get the entity type from the source - // BEFORE the Select, not from the result type (which would be the projected type) - // Walk through Select/Include to find the actual source entity type - var sourceExpression = GetSourceBeforeProjection(queryWithOrderedIncludes); + // The entity type has to come from the source the ordering will be applied to, not from + // the result type, which may be a projection, a single entity, or a limited page + var source = GetOrderingSource(queryWithOrderedIncludes); - var elementType = GetQueryElementType(sourceExpression.Type); + var elementType = GetQueryElementType(source.Type); if (elementType == null) { return queryWithOrderedIncludes; @@ -57,37 +56,62 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven return queryWithOrderedIncludes; } - // Apply default ordering to the top-level query - // If there's a Select projection at the end, we need to insert OrderBy before it - return ApplyOrderingBeforeSelect(queryWithOrderedIncludes, configuration, hasInclude); + return ApplyOrdering(queryWithOrderedIncludes, configuration); } - static Expression GetSourceBeforeProjection(Expression expression) + /// + /// Operators the default ordering has to be applied beneath rather than after. + /// + /// + /// Skip, Take and the single result operators choose which rows come back, so ordering + /// them afterwards sorts an arbitrary subset instead of choosing from an ordered one. + /// Select projects the entity away. EF Core replaces ordering that sits outside an Include + /// with key ordering. The rest only tag the query and pass their source through unchanged. + /// + static bool ShouldOrderBefore(MethodCallExpression call) { - // Walk through Select and Include to find the source entity expression - while (expression is MethodCallExpression call) + var declaringType = call.Method.DeclaringType; + var name = call.Method.Name; + + if (declaringType == typeof(Queryable)) { - var method = call.Method.Name; - var declaringType = call.Method.DeclaringType; + return name is + "Select" or + "Skip" or + "Take" or + "First" or + "FirstOrDefault" or + "Last" or + "LastOrDefault" or + "ElementAt" or + "ElementAtOrDefault"; + } - // Skip over Select projections - if (declaringType == typeof(Queryable) && - method == "Select") - { - expression = call.Arguments[0]; - continue; - } + if (declaringType == typeof(EntityFrameworkQueryableExtensions)) + { + return name is + "Include" or + "ThenInclude" or + "AsTracking" or + "AsNoTracking" or + "AsNoTrackingWithIdentityResolution" or + "AsSingleQuery" or + "AsSplitQuery" or + "IgnoreAutoIncludes" or + "IgnoreQueryFilters" or + "TagWith" or + "TagWithCallSite"; + } - // Skip over Include operations - if (declaringType == typeof(EntityFrameworkQueryableExtensions) && - method is "Include" or "ThenInclude") - { - expression = call.Arguments[0]; - continue; - } + return false; + } - // Stop at any other operation - break; + static Expression GetOrderingSource(Expression expression) + { + while (expression is MethodCallExpression call && + ShouldOrderBefore(call)) + { + expression = call.Arguments[0]; } return expression; @@ -118,66 +142,21 @@ static Expression GetSourceBeforeProjection(Expression expression) return null; }); - static Expression ApplyOrderingBeforeSelect(Expression query, Configuration configuration, bool hasInclude) + static Expression ApplyOrdering(Expression query, Configuration configuration) { - // Check if the query contains Include first - need to apply ordering before Include - // to prevent EF Core from replacing it with just Id ordering - // This must be checked before Select, because queries can be: OrderBy().Include().Select() - if (hasInclude) - { - return ApplyOrderingBeforeInclude(query, configuration); - } - - // Check if the query ends with a Select call + // Push past everything the ordering has to precede, then rebuild the chain around it if (query is MethodCallExpression call && - call.Method.DeclaringType == typeof(Queryable) && - call.Method.Name == "Select") + ShouldOrderBefore(call)) { - // Apply ordering to the source of the Select, then recreate the Select - var orderedSource = ApplyOrdering(call.Arguments[0], configuration); - return Expression.Call(call.Method, orderedSource, call.Arguments[1]); - } - - // No Select or Include, apply ordering normally - return ApplyOrdering(query, configuration); - } - - static Expression ApplyOrderingBeforeInclude(Expression query, Configuration configuration) - { - // Check if the query ends with a Select call (after Include) - if (query is MethodCallExpression selectCall && - selectCall.Method.DeclaringType == typeof(Queryable) && - selectCall.Method.Name == "Select") - { - // Recursively apply ordering before Include in the source, then recreate Select - var orderedSource = ApplyOrderingBeforeInclude(selectCall.Arguments[0], configuration); - return Expression.Call(selectCall.Method, orderedSource, selectCall.Arguments[1]); - } - - // Find the last method call before Include - if (query is MethodCallExpression methodCall && - methodCall.Method.DeclaringType == typeof(EntityFrameworkQueryableExtensions) && - methodCall.Method.Name is "Include" or "ThenInclude") - { - // Apply ordering to the source of the Include, then recreate the Include - var orderedSource = ApplyOrderingBeforeInclude(methodCall.Arguments[0], configuration); - - // Recreate the Include call with the ordered source - var args = new Expression[methodCall.Arguments.Count]; - args[0] = orderedSource; - for (var i = 1; i < methodCall.Arguments.Count; i++) - { - args[i] = methodCall.Arguments[i]; - } - - return Expression.Call(methodCall.Method, args); + var arguments = call.Arguments.ToArray(); + arguments[0] = ApplyOrdering(arguments[0], configuration); + return call.Update(call.Object, arguments); } - // No Include at this level, apply ordering here - return ApplyOrdering(query, configuration); + return AppendOrdering(query, configuration); } - static Expression ApplyOrdering(Expression source, Configuration configuration) + static Expression AppendOrdering(Expression source, Configuration configuration) { var result = source; diff --git a/src/Tests/RowLimitingTests.Count_DoesNotApplyOrdering.verified.txt b/src/Tests/RowLimitingTests.Count_DoesNotApplyOrdering.verified.txt new file mode 100644 index 0000000..0440aeb --- /dev/null +++ b/src/Tests/RowLimitingTests.Count_DoesNotApplyOrdering.verified.txt @@ -0,0 +1,9 @@ +{ + target: 6, + sql: { + Text: +select COUNT(*) +from Employees as e, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.ElementAt_AppliesDefaultOrder.verified.txt b/src/Tests/RowLimitingTests.ElementAt_AppliesDefaultOrder.verified.txt new file mode 100644 index 0000000..daf581f --- /dev/null +++ b/src/Tests/RowLimitingTests.ElementAt_AppliesDefaultOrder.verified.txt @@ -0,0 +1,24 @@ +{ + target: { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + }, + sql: { + Text: +select e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +order by e.HireDate desc +offset @p rows fetch next 1 rows only, + Parameters: { + @p: 2 + }, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.FirstOrDefaultWithPredicate_AppliesDefaultOrder.verified.txt b/src/Tests/RowLimitingTests.FirstOrDefaultWithPredicate_AppliesDefaultOrder.verified.txt new file mode 100644 index 0000000..96b66b6 --- /dev/null +++ b/src/Tests/RowLimitingTests.FirstOrDefaultWithPredicate_AppliesDefaultOrder.verified.txt @@ -0,0 +1,21 @@ +{ + target: { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + sql: { + Text: +select top (1) e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +where e.Salary > 70000 +order by e.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.First_AppliesDefaultOrder.verified.txt b/src/Tests/RowLimitingTests.First_AppliesDefaultOrder.verified.txt new file mode 100644 index 0000000..17ae05a --- /dev/null +++ b/src/Tests/RowLimitingTests.First_AppliesDefaultOrder.verified.txt @@ -0,0 +1,20 @@ +{ + target: { + Id: 6, + DepartmentId: 3, + Name: Frank, + HireDate: 2024-04-10, + Salary: 65000 + }, + sql: { + Text: +select top (1) e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +order by e.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.IncludeThenPage_OrdersBeforeLimiting.verified.txt b/src/Tests/RowLimitingTests.IncludeThenPage_OrdersBeforeLimiting.verified.txt new file mode 100644 index 0000000..69e2e2f --- /dev/null +++ b/src/Tests/RowLimitingTests.IncludeThenPage_OrdersBeforeLimiting.verified.txt @@ -0,0 +1,52 @@ +{ + target: [ + { + Id: 2, + Name: Sales, + DisplayOrder: 2, + Employees: [ + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + }, + { + Id: 5, + DepartmentId: 2, + Name: Eve, + HireDate: 2023-11-01, + Salary: 72000 + } + ] + } + ], + sql: { + Text: +select d0.Id, + d0.DisplayOrder, + d0.Name, + e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from (select d.Id, + d.DisplayOrder, + d.Name + from Departments as d + order by d.DisplayOrder + offset @p rows fetch next @p rows only) as d0 + left outer join + Employees as e + on d0.Id = e.DepartmentId +order by d0.DisplayOrder, + d0.Id, + e.HireDate desc, + Parameters: { + @p: 1 + }, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.SelectThenFirst_AppliesDefaultOrderToSource.verified.txt b/src/Tests/RowLimitingTests.SelectThenFirst_AppliesDefaultOrderToSource.verified.txt new file mode 100644 index 0000000..13f97e6 --- /dev/null +++ b/src/Tests/RowLimitingTests.SelectThenFirst_AppliesDefaultOrderToSource.verified.txt @@ -0,0 +1,10 @@ +{ + target: Frank, + sql: { + Text: +select top (1) e.Name +from Employees as e +order by e.HireDate desc, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.Single_DoesNotApplyOrdering.verified.txt b/src/Tests/RowLimitingTests.Single_DoesNotApplyOrdering.verified.txt new file mode 100644 index 0000000..bce32dd --- /dev/null +++ b/src/Tests/RowLimitingTests.Single_DoesNotApplyOrdering.verified.txt @@ -0,0 +1,20 @@ +{ + target: { + Id: 5, + DepartmentId: 2, + Name: Eve, + HireDate: 2023-11-01, + Salary: 72000 + }, + sql: { + Text: +select top (2) e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +where e.Name = N'Eve', + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.SkipTake_OrdersBeforeLimiting.verified.txt b/src/Tests/RowLimitingTests.SkipTake_OrdersBeforeLimiting.verified.txt new file mode 100644 index 0000000..8579c9d --- /dev/null +++ b/src/Tests/RowLimitingTests.SkipTake_OrdersBeforeLimiting.verified.txt @@ -0,0 +1,34 @@ +{ + target: [ + { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + } + ], + sql: { + Text: +select e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +order by e.HireDate desc +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 1, + @p1: 2 + }, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.Take_OrdersBeforeLimiting.verified.txt b/src/Tests/RowLimitingTests.Take_OrdersBeforeLimiting.verified.txt new file mode 100644 index 0000000..0d49d11 --- /dev/null +++ b/src/Tests/RowLimitingTests.Take_OrdersBeforeLimiting.verified.txt @@ -0,0 +1,39 @@ +{ + target: [ + { + Id: 6, + DepartmentId: 3, + Name: Frank, + HireDate: 2024-04-10, + Salary: 65000 + }, + { + Id: 2, + DepartmentId: 1, + Name: Bob, + HireDate: 2024-03-20, + Salary: 85000 + }, + { + Id: 4, + DepartmentId: 2, + Name: Diana, + HireDate: 2024-02-05, + Salary: 70000 + } + ], + sql: { + Text: +select top (@p) e.Id, + e.DepartmentId, + e.HireDate, + e.Name, + e.Salary +from Employees as e +order by e.HireDate desc, + Parameters: { + @p: 3 + }, + HasTransaction: false + } +} \ No newline at end of file diff --git a/src/Tests/RowLimitingTests.cs b/src/Tests/RowLimitingTests.cs new file mode 100644 index 0000000..1016707 --- /dev/null +++ b/src/Tests/RowLimitingTests.cs @@ -0,0 +1,143 @@ +// The default ordering has to be applied before any operator that chooses rows, otherwise +// an arbitrary subset is chosen first and only then sorted. +[TestFixture] +public class RowLimitingTests +{ + // Employees in default order of HireDate descending: + // Frank, Bob, Diana, Alice, Eve, Charlie + + [Test] + public async Task Take_OrdersBeforeLimiting() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Employees + .Take(3) + .ToListAsync(); + + Assert.That(results.Select(_ => _.Name), Is.EqualTo(new[] { "Frank", "Bob", "Diana" })); + await Verify(results); + } + + [Test] + public async Task SkipTake_OrdersBeforeLimiting() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Employees + .Skip(1) + .Take(2) + .ToListAsync(); + + Assert.That(results.Select(_ => _.Name), Is.EqualTo(new[] { "Bob", "Diana" })); + await Verify(results); + } + + [Test] + public async Task IncludeThenPage_OrdersBeforeLimiting() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var results = await context.Departments + .Include(_ => _.Employees) + .Skip(1) + .Take(1) + .ToListAsync(); + + // Departments in default order are Engineering, Sales, HR + Assert.That(results.Single().Name, Is.EqualTo("Sales")); + + // The included collection keeps its own default ordering + Assert.That(results[0].Employees.Select(_ => _.Name), Is.EqualTo(new[] { "Diana", "Eve" })); + await Verify(results); + } + + [Test] + public async Task First_AppliesDefaultOrder() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var result = await context.Employees.FirstAsync(); + + Assert.That(result.Name, Is.EqualTo("Frank")); + await Verify(result); + } + + [Test] + public async Task FirstOrDefaultWithPredicate_AppliesDefaultOrder() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var result = await context.Employees.FirstOrDefaultAsync(_ => _.Salary > 70000); + + // Of Alice, Bob, Charlie and Eve, Bob was hired most recently + Assert.That(result!.Name, Is.EqualTo("Bob")); + await Verify(result); + } + + [Test] + public async Task SelectThenFirst_AppliesDefaultOrderToSource() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var result = await context.Employees + .Select(_ => _.Name) + .FirstAsync(); + + Assert.That(result, Is.EqualTo("Frank")); + await Verify(result); + } + + [Test] + public async Task ElementAt_AppliesDefaultOrder() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var result = await context.Employees.ElementAtAsync(2); + + Assert.That(result.Name, Is.EqualTo("Diana")); + await Verify(result); + } + + [Test] + public async Task Count_DoesNotApplyOrdering() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var result = await context.Employees.CountAsync(); + + // Ordering an aggregate is pointless work, so it must be left alone + Assert.That(result, Is.EqualTo(6)); + await Verify(result); + } + + [Test] + public async Task Single_DoesNotApplyOrdering() + { + await using var database = await ModuleInitializer.SqlInstance.Build(); + await using var context = database.NewDbContext(); + + Recording.Start(); + var result = await context.Employees.SingleAsync(_ => _.Name == "Eve"); + + // Single matches at most one row, so ordering it is pointless work + Assert.That(result.Salary, Is.EqualTo(72000)); + await Verify(result); + } +} From 8d01ee5af7317fb323ea61fbaab2768d45f9506a Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:42:05 +1000 Subject: [PATCH 3/7] Stop redundant order detection walking into combined sequences FindSource followed the first argument of any static method whose source looked enumerable, which includes Concat, Union, Join and the other operators that combine two sequences. Ordering applied to one side was then reported as redundant, even though the default ordering is never applied to the combined result, so the advice to remove it was wrong. Skip those operators when walking to the source. --- src/EfOrderBy/RedundantOrder.cs | 24 +++++++++++++++++++++- src/Tests/RedundantOrderByTests.cs | 33 ++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/EfOrderBy/RedundantOrder.cs b/src/EfOrderBy/RedundantOrder.cs index 5e81372..f655429 100644 --- a/src/EfOrderBy/RedundantOrder.cs +++ b/src/EfOrderBy/RedundantOrder.cs @@ -90,12 +90,34 @@ public static void Validate(Expression expression) return (elementType, clauses); } + // Operators that combine two sequences. Their first argument is only one side of the + // query, and the default ordering is never applied to the combined result, so ordering + // found down that side is not made redundant by it. + static readonly HashSet combining = + [ + "Concat", + "Except", + "ExceptBy", + "GroupJoin", + "Intersect", + "IntersectBy", + "Join", + "LeftJoin", + "RightJoin", + "SequenceEqual", + "Union", + "UnionBy", + "Zip" + ]; + // Ordering can sit behind calls like Where, AsNoTracking, or TagWith, // all of which take the query being composed as their first argument. static Expression? FindSource(MethodCallExpression call) { - if (call.Method.IsStatic && + var method = call.Method; + if (method.IsStatic && call.Arguments.Count > 0 && + !combining.Contains(method.Name) && typeof(IEnumerable).IsAssignableFrom(call.Arguments[0].Type)) { return call.Arguments[0]; diff --git a/src/Tests/RedundantOrderByTests.cs b/src/Tests/RedundantOrderByTests.cs index 32a4fda..965aebf 100644 --- a/src/Tests/RedundantOrderByTests.cs +++ b/src/Tests/RedundantOrderByTests.cs @@ -207,6 +207,39 @@ public void Include_WithoutOrdering_DoesNotThrow() .ToQueryString()); } + [Test] + public void ExactMatchInsideConcat_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + // The default ordering is not applied to a combined sequence, so ordering one + // side of it is not redundant + Assert.DoesNotThrow( + () => context.Entities + .Where(_ => _.Priority > 1) + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .Concat(context.Entities.Where(_ => _.Priority <= 1)) + .ToQueryString()); + } + + [Test] + public void ExactMatchInsideJoin_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .Join( + context.Children, + entity => entity.Id, + child => child.RedundantEntityId, + (entity, child) => child.Title) + .ToQueryString()); + } + [Test] public void Disabled_DoesNotThrow() { From 999a349c7bc11457e75f9a6d322e4bd6a35ea64f Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:43:18 +1000 Subject: [PATCH 4/7] Find non public properties when inheriting ordering to derived types CreateForDerivedType looked the property up with the default binding flags, which only match public members. An entity can map a non public property, and ordering configured on one failed to inherit with Property 'SortOrder' not found on derived type 'Derived'. even though the derived type does have it. Include non public instance properties in the lookup. --- src/EfOrderBy/Configuration.cs | 13 ++++++-- src/Tests/InheritedOrderingTests.cs | 49 +++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 src/Tests/InheritedOrderingTests.cs diff --git a/src/EfOrderBy/Configuration.cs b/src/EfOrderBy/Configuration.cs index 8efa092..b15c299 100644 --- a/src/EfOrderBy/Configuration.cs +++ b/src/EfOrderBy/Configuration.cs @@ -59,15 +59,22 @@ internal void AddClause(PropertyInfo propertyInfo, bool descending, bool isThenB /// Creates a new Configuration for a derived type by replaying the clause metadata. /// The derived type must have the same properties (inherited from the base). /// + // The default lookup omits non public properties, which an entity can map, + // so without these the ordering fails to inherit for those properties. + const BindingFlags propertyFlags = + BindingFlags.Public | + BindingFlags.NonPublic | + BindingFlags.Instance; + internal Configuration CreateForDerivedType(Type derivedType) { var derived = new Configuration(derivedType) { IsInherited = true }; foreach (var meta in ClauseMetadataList) { - var prop = derivedType.GetProperty(meta.PropertyName); - if (prop != null) + var property = derivedType.GetProperty(meta.PropertyName, propertyFlags); + if (property != null) { - derived.AddClause(prop, meta.Descending, meta.IsThenBy); + derived.AddClause(property, meta.Descending, meta.IsThenBy); continue; } diff --git a/src/Tests/InheritedOrderingTests.cs b/src/Tests/InheritedOrderingTests.cs new file mode 100644 index 0000000..f545464 --- /dev/null +++ b/src/Tests/InheritedOrderingTests.cs @@ -0,0 +1,49 @@ +[TestFixture] +public class InheritedOrderingTests +{ + static DbContextOptions options = + new DbContextOptionsBuilder() + .UseSqlServer("Server=.;Database=Test;") + .UseDefaultOrderBy() + .Options; + + [Test] + public void NonPublicProperty_InheritsOrderingToDerivedType() + { + using var context = new InternalOrderContext(options); + + var sql = context.Derived.ToQueryString(); + + Assert.That(sql, Does.Contain("ORDER BY")); + Assert.That(sql, Does.Contain("SortOrder")); + } +} + +public class InternalOrderBase +{ + public int Id { get; set; } + + // Mapped explicitly below, since conventions only pick up public properties + internal int SortOrder { get; set; } +} + +public class InternalOrderDerived : InternalOrderBase +{ + public string Extra { get; set; } = ""; +} + +public class InternalOrderContext(DbContextOptions options) : + DbContext(options) +{ + public DbSet Bases => Set(); + public DbSet Derived => Set(); + + protected override void OnModelCreating(ModelBuilder builder) + { + builder.Entity() + .Property(_ => _.SortOrder); + + builder.Entity() + .OrderBy(_ => _.SortOrder); + } +} From c4e41014625d1dd4ef0fe15e7f72e249dcb1b535 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:44:39 +1000 Subject: [PATCH 5/7] Skip the Include rewrite for queries that have no Include IncludeOrderingApplicator is an ExpressionVisitor, so it rebuilt the entire query tree on every compilation even though most queries have no Include for it to touch. The chain analysis already reports whether one is present, so run the analysis first and only visit when it is. The rewrite only replaces the inside of Include lambdas, so neither the ordering nor the Include flag can change as a result of it. Also resolve the options extension once and pass it to both callers, instead of RequiredOrder and RedundantOrder each resolving IDbContextOptions from the context on every compilation. --- src/EfOrderBy/Interceptor.cs | 24 ++++++++++++++++++------ src/EfOrderBy/RedundantOrder.cs | 6 ------ src/EfOrderBy/RequiredOrder.cs | 10 +++------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/EfOrderBy/Interceptor.cs b/src/EfOrderBy/Interceptor.cs index 6187c0b..c8e04d5 100644 --- a/src/EfOrderBy/Interceptor.cs +++ b/src/EfOrderBy/Interceptor.cs @@ -14,16 +14,28 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven } var model = context.Model; - RequiredOrder.Validate(context); - var detectRedundantOrdering = RedundantOrder.IsEnabled(context); + // Both of the options below live on the same extension, so resolve it once + var extension = context.GetService() + .FindExtension(); - // First, process Include nodes to add ordering to nested collections - var visitor = new IncludeOrderingApplicator(model, detectRedundantOrdering); - var queryWithOrderedIncludes = visitor.Visit(query); + RequiredOrder.Validate(context, extension); + + var detectRedundantOrdering = extension?.ThrowOnRedundantOrderBy ?? false; // Analyze the query for ordering and includes in a single pass - var (hasOrdering, _) = QueryAnalyzer.Analyze(queryWithOrderedIncludes); + var (hasOrdering, hasInclude) = QueryAnalyzer.Analyze(query); + + var queryWithOrderedIncludes = query; + + // Ordering nested collections rewrites the whole tree, so only do it when there is an + // Include to rewrite. Neither flag above can change as a result, since the rewrite only + // touches the inside of Include lambdas. + if (hasInclude) + { + var visitor = new IncludeOrderingApplicator(model, detectRedundantOrdering); + queryWithOrderedIncludes = visitor.Visit(query); + } if (hasOrdering) { diff --git a/src/EfOrderBy/RedundantOrder.cs b/src/EfOrderBy/RedundantOrder.cs index f655429..77f7ddb 100644 --- a/src/EfOrderBy/RedundantOrder.cs +++ b/src/EfOrderBy/RedundantOrder.cs @@ -3,12 +3,6 @@ /// static class RedundantOrder { - public static bool IsEnabled(DbContext context) => - context.GetService() - .FindExtension() - ?.ThrowOnRedundantOrderBy ?? - false; - public static void Validate(Expression expression) { if (FindOrdering(expression) is not { } ordering) diff --git a/src/EfOrderBy/RequiredOrder.cs b/src/EfOrderBy/RequiredOrder.cs index 8b310bd..5d148cb 100644 --- a/src/EfOrderBy/RequiredOrder.cs +++ b/src/EfOrderBy/RequiredOrder.cs @@ -2,7 +2,7 @@ static class RequiredOrder { static ConcurrentDictionary validated = []; - public static void Validate(DbContext context) + public static void Validate(DbContext context, OrderRequiredExtension? extension) { var contextType = context.GetType(); @@ -12,12 +12,8 @@ public static void Validate(DbContext context) return; } - // Check if this DbContext requires ordering for all entities (opt-in feature) - var requireOrdering = context.GetService() - .FindExtension() - ?.RequireOrderingForAllEntities ?? false; - - if (requireOrdering) + // Requiring ordering for all entities is opt-in + if (extension is { RequireOrderingForAllEntities: true }) { ValidateAllEntitiesHaveOrdering(context.Model); } From 32b9d45121b65d05c33d0d6e5a4f4af8912a9c41 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:46:17 +1000 Subject: [PATCH 6/7] Drop state that is retained but not paying for itself OrderByClause held the full method tables of Queryable and Enumerable in static fields, used only to resolve eight methods during static init. The MethodInfo instances are cached by the runtime either way, so what the fields actually kept alive was the two arrays. Resolve per method and let them go. The element type lookups were cached in static dictionaries keyed by Type. A query root is already IQueryable and a navigation is declared as one of a handful of collection types, so both lookups hit a fast path of a few type comparisons, which is cheaper than the dictionary probe guarding it. The dictionaries also pinned every type they were handed for the life of the process, which stops a collectible load context unloading. Removed. The Include method cache stays, since MakeGenericMethod is genuinely expensive. Also append and reverse instead of repeatedly inserting at the head of the clause list, and mark the remaining mutable statics readonly. --- src/EfOrderBy/IncludeOrderingApplicator.cs | 59 +++++++++++----------- src/EfOrderBy/Interceptor.cs | 38 +++++++------- src/EfOrderBy/OrderByClause.cs | 36 ++++++------- src/EfOrderBy/OrderByExtensions.cs | 2 +- src/EfOrderBy/RedundantOrder.cs | 5 +- src/EfOrderBy/RequiredOrder.cs | 2 +- 6 files changed, 73 insertions(+), 69 deletions(-) diff --git a/src/EfOrderBy/IncludeOrderingApplicator.cs b/src/EfOrderBy/IncludeOrderingApplicator.cs index fc42e1b..26819e0 100644 --- a/src/EfOrderBy/IncludeOrderingApplicator.cs +++ b/src/EfOrderBy/IncludeOrderingApplicator.cs @@ -4,7 +4,7 @@ sealed class IncludeOrderingApplicator(IModel model, bool detectRedundantOrdering) : ExpressionVisitor { - static readonly ConcurrentDictionary collectionElementTypeCache = new(); + // MakeGenericMethod is expensive enough to be worth caching, unlike the element type lookup static readonly ConcurrentDictionary<(MethodInfo, Type), MethodInfo> includeMethodCache = new(); protected override Expression VisitMethodCall(MethodCallExpression node) @@ -112,35 +112,36 @@ static Expression ApplyOrdering(Expression source, Configuration configuration) return result; } - static Type? GetCollectionElementType(Type type) => - collectionElementTypeCache.GetOrAdd(type, - static type => + // Not cached. A navigation is declared as one of the types below, so the fast path is a + // handful of type comparisons, which is cheaper than a dictionary lookup and does not pin + // every type it is handed for the life of the process. + static Type? GetCollectionElementType(Type type) + { + // Check for IEnumerable + if (type.IsGenericType) + { + var genericDef = type.GetGenericTypeDefinition(); + if (genericDef == typeof(IEnumerable<>) || + genericDef == typeof(ICollection<>) || + genericDef == typeof(IList<>) || + genericDef == typeof(List<>)) { - // Check for IEnumerable - if (type.IsGenericType) - { - var genericDef = type.GetGenericTypeDefinition(); - if (genericDef == typeof(IEnumerable<>) || - genericDef == typeof(ICollection<>) || - genericDef == typeof(IList<>) || - genericDef == typeof(List<>)) - { - return type.GetGenericArguments()[0]; - } - } - - // Check interfaces - foreach (var iface in type.GetInterfaces()) - { - if (iface.IsGenericType && - iface.GetGenericTypeDefinition() == typeof(IEnumerable<>)) - { - return iface.GetGenericArguments()[0]; - } - } - - return null; - }); + return type.GetGenericArguments()[0]; + } + } + + // Check interfaces + foreach (var iface in type.GetInterfaces()) + { + if (iface.IsGenericType && + iface.GetGenericTypeDefinition() == typeof(IEnumerable<>)) + { + return iface.GetGenericArguments()[0]; + } + } + + return null; + } Configuration? GetConfiguration(Type element) { diff --git a/src/EfOrderBy/Interceptor.cs b/src/EfOrderBy/Interceptor.cs index c8e04d5..63ab96a 100644 --- a/src/EfOrderBy/Interceptor.cs +++ b/src/EfOrderBy/Interceptor.cs @@ -3,8 +3,6 @@ /// sealed class Interceptor : IQueryExpressionInterceptor { - static readonly ConcurrentDictionary queryElementTypeCache = new(); - public Expression QueryCompilationStarting(Expression query, QueryExpressionEventData eventData) { var context = eventData.Context; @@ -129,30 +127,32 @@ static Expression GetOrderingSource(Expression expression) return expression; } - static Type? GetQueryElementType(Type type) => - queryElementTypeCache.GetOrAdd(type, static type => + // Not cached. A query root is already IQueryable, so the fast path below is two type + // comparisons, which is cheaper than a dictionary lookup and does not pin every type it + // is handed for the life of the process. + static Type? GetQueryElementType(Type type) + { + if (type.IsGenericType) { - if (type.IsGenericType) + var genericDef = type.GetGenericTypeDefinition(); + if (genericDef == typeof(IQueryable<>) || + genericDef == typeof(IOrderedQueryable<>)) { - var genericDef = type.GetGenericTypeDefinition(); - if (genericDef == typeof(IQueryable<>) || - genericDef == typeof(IOrderedQueryable<>)) - { - return type.GetGenericArguments()[0]; - } + return type.GetGenericArguments()[0]; } + } - foreach (var iface in type.GetInterfaces()) + foreach (var iface in type.GetInterfaces()) + { + if (iface.IsGenericType && + iface.GetGenericTypeDefinition() == typeof(IQueryable<>)) { - if (iface.IsGenericType && - iface.GetGenericTypeDefinition() == typeof(IQueryable<>)) - { - return iface.GetGenericArguments()[0]; - } + return iface.GetGenericArguments()[0]; } + } - return null; - }); + return null; + } static Expression ApplyOrdering(Expression query, Configuration configuration) { diff --git a/src/EfOrderBy/OrderByClause.cs b/src/EfOrderBy/OrderByClause.cs index 645c031..52c47be 100644 --- a/src/EfOrderBy/OrderByClause.cs +++ b/src/EfOrderBy/OrderByClause.cs @@ -26,38 +26,40 @@ internal OrderByClause(Type elementType, ParameterExpression parameter, Property quotedLambda = Expression.Quote(lambda); } - static MethodInfo FindMethod(MethodInfo[] methods, string name) => - methods.First(_ => _.Name == name && - _.GetParameters().Length == 2); + // The overload taking a comparer cannot be expressed as configuration, so match on the + // two parameter one. The method table is not held in a field, since keeping it alive for + // the life of the process buys nothing once these eight have been resolved. + static MethodInfo FindMethod(Type type, string name) => + type.GetMethods() + .First(_ => _.Name == name && + _.GetParameters().Length == 2); - static MethodInfo[] queryableMethods = typeof(Queryable).GetMethods(); - static MethodInfo[] enumerableMethods = typeof(Enumerable).GetMethods(); + static readonly MethodInfo queryableOrderBy = FindMethod(typeof(Queryable), nameof(Queryable.OrderBy)); + static readonly MethodInfo queryableOrderByDescending = FindMethod(typeof(Queryable), nameof(Queryable.OrderByDescending)); + static readonly MethodInfo queryableThenBy = FindMethod(typeof(Queryable), nameof(Queryable.ThenBy)); + static readonly MethodInfo queryableThenByDescending = FindMethod(typeof(Queryable), nameof(Queryable.ThenByDescending)); - static MethodInfo queryableOrderBy = FindMethod(queryableMethods, nameof(Queryable.OrderBy)); - static MethodInfo queryableOrderByDescending = FindMethod(queryableMethods, nameof(Queryable.OrderByDescending)); - static MethodInfo queryableThenBy = FindMethod(queryableMethods, nameof(Queryable.ThenBy)); - static MethodInfo queryableThenByDescending = FindMethod(queryableMethods, nameof(Queryable.ThenByDescending)); + static readonly MethodInfo enumerableOrderBy = FindMethod(typeof(Enumerable), nameof(Enumerable.OrderBy)); + static readonly MethodInfo enumerableOrderByDescending = FindMethod(typeof(Enumerable), nameof(Enumerable.OrderByDescending)); + static readonly MethodInfo enumerableThenBy = FindMethod(typeof(Enumerable), nameof(Enumerable.ThenBy)); + static readonly MethodInfo enumerableThenByDescending = FindMethod(typeof(Enumerable), nameof(Enumerable.ThenByDescending)); - static MethodInfo enumerableOrderBy = FindMethod(enumerableMethods, nameof(Enumerable.OrderBy)); - static MethodInfo enumerableOrderByDescending = FindMethod(enumerableMethods, nameof(Enumerable.OrderByDescending)); - static MethodInfo enumerableThenBy = FindMethod(enumerableMethods, nameof(Enumerable.ThenBy)); - static MethodInfo enumerableThenByDescending = FindMethod(enumerableMethods, nameof(Enumerable.ThenByDescending)); - LambdaExpression lambda; + readonly LambdaExpression lambda; // The fully generic Enumerable method (e.g., OrderBy) // ready to be invoked without further generic type arguments. - MethodInfo enumerableMethod; + readonly MethodInfo enumerableMethod; // The fully generic Queryable method (e.g., OrderBy) // ready to be invoked without further generic type arguments. - MethodInfo queryableMethod; + readonly MethodInfo queryableMethod; public Expression AppendEnumerableOrder(Expression result) => // Enumerable methods expect Func, so we pass the lambda directly (no Quote) Expression.Call(enumerableMethod, result, lambda); // Queryable methods expect Expression>, so we use Quote() - UnaryExpression quotedLambda; + readonly UnaryExpression quotedLambda; public Expression AppendQueryableOrder(Expression result) => Expression.Call(queryableMethod, result, quotedLambda); diff --git a/src/EfOrderBy/OrderByExtensions.cs b/src/EfOrderBy/OrderByExtensions.cs index c4e77e5..06c43e2 100644 --- a/src/EfOrderBy/OrderByExtensions.cs +++ b/src/EfOrderBy/OrderByExtensions.cs @@ -5,7 +5,7 @@ namespace EfOrderBy; /// public static class OrderByExtensions { - static Interceptor interceptor = new(); + static readonly Interceptor interceptor = new(); /// /// Adds the default ordering interceptor to automatically apply ordering to queries diff --git a/src/EfOrderBy/RedundantOrder.cs b/src/EfOrderBy/RedundantOrder.cs index 77f7ddb..b04ad4a 100644 --- a/src/EfOrderBy/RedundantOrder.cs +++ b/src/EfOrderBy/RedundantOrder.cs @@ -55,8 +55,7 @@ public static void Validate(Expression expression) return null; } - // The chain is walked outermost first, so each clause found is applied before the previous - clauses.Insert(0, new(propertyName, descending, isThenBy)); + clauses.Add(new(propertyName, descending, isThenBy)); elementType = method.GetGenericArguments()[0]; expression = call.Arguments[0]; continue; @@ -81,6 +80,8 @@ public static void Validate(Expression expression) return null; } + // The chain is walked outermost first, so the clauses come out in reverse of how they apply + clauses.Reverse(); return (elementType, clauses); } diff --git a/src/EfOrderBy/RequiredOrder.cs b/src/EfOrderBy/RequiredOrder.cs index 5d148cb..3013e14 100644 --- a/src/EfOrderBy/RequiredOrder.cs +++ b/src/EfOrderBy/RequiredOrder.cs @@ -1,6 +1,6 @@ static class RequiredOrder { - static ConcurrentDictionary validated = []; + static readonly ConcurrentDictionary validated = []; public static void Validate(DbContext context, OrderRequiredExtension? extension) { From ffd72920d6a3610e399e876def37147162acce20 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 16:47:54 +1000 Subject: [PATCH 7/7] Document paging and single result ordering The placement of the default ordering relative to Skip, Take and First is now user visible, so say where it goes and which operators are left alone. --- readme.md | 40 ++++++++++++++++++++++++++++++++++++---- src/Tests/Snippets.cs | 21 +++++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/readme.md b/readme.md index 7401fa9..a06a96b 100644 --- a/readme.md +++ b/readme.md @@ -16,6 +16,7 @@ https://nuget.org/packages/EfOrderBy/ - **Automatic ordering**: Queries without explicit `OrderBy` automatically use configured default ordering - **Include() support**: Nested collections in `.Include()` expressions are automatically ordered +- **Deterministic paging**: Ordering is applied before `Skip`/`Take`/`First`, so pages and single results are stable - **Inheritance support**: Ordering configured on a base entity type is automatically inherited by derived types (TPH) - **Fluent configuration**: Configure default ordering using the familiar EF Core fluent API - **Multi-column ordering**: Chain multiple ordering clauses with `ThenBy` and `ThenByDescending` @@ -82,6 +83,37 @@ var employeesByName = await context.Employees +## Paging and Single Results + +The default ordering is applied *before* any operator that chooses which rows come back, so +`Skip`, `Take`, `First`, `FirstOrDefault`, `Last`, `LastOrDefault` and `ElementAt` all select +from an ordered sequence: + + + +```cs +// The ordering is applied before the page is taken, so the page is +// taken from an ordered sequence rather than sorted after the fact +var secondPage = await context.Employees + .Skip(20) + .Take(20) + .ToListAsync(); + +// Ordered by HireDate, then Salary descending, so this is the +// earliest hire rather than an arbitrary row +var first = await context.Employees + .FirstAsync(); +``` +snippet source | anchor + + +Without this, a page would be an arbitrary set of rows that happens to be sorted, and pages +could both skip and repeat rows. + +Aggregates such as `Count` and `Any`, and `Single`, are left alone, since ordering them is +pointless work. + + ## Include() Support Nested collections in `.Include()` expressions are automatically ordered: @@ -95,7 +127,7 @@ var departments = await context.Departments .Include(_ => _.Employees) .ToListAsync(); ``` -snippet source | anchor +snippet source | anchor @@ -149,7 +181,7 @@ public class InheritanceDbContext : DbContext public DbSet DerivedEntitiesB => Set(); } ``` -snippet source | anchor +snippet source | anchor Behavior: @@ -172,7 +204,7 @@ builder.Entity() .ThenBy(_ => _.Name) .ThenByDescending(_ => _.Price); ``` -snippet source | anchor +snippet source | anchor @@ -422,7 +454,7 @@ public class AppDbContext : DbContext public DbSet Employees => Set(); } ``` -snippet source | anchor +snippet source | anchor diff --git a/src/Tests/Snippets.cs b/src/Tests/Snippets.cs index b144747..149aafd 100644 --- a/src/Tests/Snippets.cs +++ b/src/Tests/Snippets.cs @@ -82,6 +82,27 @@ static async Task QueryWithoutOrderBy() #endregion } + static async Task PagingAndSingleResults() + { + AppDbContext context = null!; + + #region PagingAndSingleResults + + // The ordering is applied before the page is taken, so the page is + // taken from an ordered sequence rather than sorted after the fact + var secondPage = await context.Employees + .Skip(20) + .Take(20) + .ToListAsync(); + + // Ordered by HireDate, then Salary descending, so this is the + // earliest hire rather than an arbitrary row + var first = await context.Employees + .FirstAsync(); + + #endregion + } + static async Task IncludeSupport() { AppDbContext context = null!;