From 496aa05d54112cb609bc2fc9f6e4bdc84c8e94c2 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 24 Jul 2026 15:39:34 +1000 Subject: [PATCH] Add throwOnRedundantOrderBy detection Opt-in flag on UseDefaultOrderBy that throws when a query explicitly orders by exactly the same properties, sequence, and directions as the configured default ordering. Primarily useful in tests to find query ordering left over from before the default ordering was configured. RedundantOrder walks the outermost ordering chain of the query spine and compares the collected clauses against the entity configuration. It bails out without throwing on anything that cannot be represented as configuration, such as comparer overloads or non property key selectors. Both Queryable and Enumerable ordering are handled, so nested collection ordering in Include() is checked the same way. The flag is read from IDbContextOptions per query compilation rather than cached per DbContext type, since the interceptor is a singleton shared across contexts and a type keyed cache would be wrong when the same context type is built with different options. --- readme.md | 83 +++++- src/EfOrderBy/IncludeOrderingApplicator.cs | 7 +- src/EfOrderBy/Interceptor.cs | 9 +- src/EfOrderBy/OrderByExtensions.cs | 10 +- src/EfOrderBy/OrderRequiredExtension.cs | 12 +- src/EfOrderBy/RedundantOrder.cs | 177 +++++++++++++ src/Tests/RedundantOrderByTests.cs | 285 +++++++++++++++++++++ src/Tests/Snippets.cs | 11 + 8 files changed, 580 insertions(+), 14 deletions(-) create mode 100644 src/EfOrderBy/RedundantOrder.cs create mode 100644 src/Tests/RedundantOrderByTests.cs diff --git a/readme.md b/readme.md index 7713fd0..7401fa9 100644 --- a/readme.md +++ b/readme.md @@ -21,6 +21,7 @@ https://nuget.org/packages/EfOrderBy/ - **Multi-column ordering**: Chain multiple ordering clauses with `ThenBy` and `ThenByDescending` - **Automatic indexes**: Database indexes are automatically created for ordering columns - **Validation mode**: Optionally require all entities to have default ordering configured +- **Redundant ordering detection**: Optionally throw when a query explicitly applies the same ordering as the configured default ## Usage @@ -77,7 +78,7 @@ var employeesByName = await context.Employees .OrderBy(_ => _.Name) .ToListAsync(); ``` -snippet source | anchor +snippet source | anchor @@ -94,7 +95,7 @@ var departments = await context.Departments .Include(_ => _.Employees) .ToListAsync(); ``` -snippet source | anchor +snippet source | anchor @@ -148,7 +149,7 @@ public class InheritanceDbContext : DbContext public DbSet DerivedEntitiesB => Set(); } ``` -snippet source | anchor +snippet source | anchor Behavior: @@ -171,7 +172,7 @@ builder.Entity() .ThenBy(_ => _.Name) .ThenByDescending(_ => _.Price); ``` -snippet source | anchor +snippet source | anchor @@ -254,7 +255,7 @@ protected override void OnConfiguring(DbContextOptionsBuilder builder) => builder.UseDefaultOrderBy( createIndexes: false); ``` -snippet source | anchor +snippet source | anchor When index creation is disabled, calling `WithIndexName()` throws an `Exception`. @@ -285,6 +286,76 @@ Use modelBuilder.Entity().OrderBy() to configure default ordering. Validation occurs once per `DbContext` type for performance. +## Detect Redundant Ordering + +Explicit ordering in a query takes precedence over the configured default. When that explicit ordering is an exact match for the default, it is redundant. This is usually a leftover from before the default ordering was configured. + +Enable detection to throw when a query does this. This is primarily useful in tests: + + + +```cs +protected override void OnConfiguring(DbContextOptionsBuilder builder) => + builder.UseDefaultOrderBy( + throwOnRedundantOrderBy: true); +``` +snippet source | anchor + + +Given this configuration: + +```cs +builder.Entity() + .OrderBy(_ => _.HireDate) + .ThenByDescending(_ => _.Salary); +``` + +The following query throws, since it applies exactly the default ordering: + +```cs +var employees = await context.Employees + .OrderBy(_ => _.HireDate) + .ThenByDescending(_ => _.Salary) + .ToListAsync(); +``` + +``` +The query explicitly orders 'Employee' by OrderBy(HireDate).ThenByDescending(Salary), +which exactly matches the default ordering configured for that entity. +Remove the ordering from the query since it is applied automatically, +or change it to a different ordering. +``` + +The match must be exact: the same properties, in the same sequence, with the same directions. Anything else is treated as an intentional override and does not throw: + +```cs +// Only part of the default ordering +.OrderBy(_ => _.HireDate) + +// Different direction +.OrderByDescending(_ => _.HireDate) +.ThenByDescending(_ => _.Salary) + +// Additional ordering column +.OrderBy(_ => _.HireDate) +.ThenByDescending(_ => _.Salary) +.ThenBy(_ => _.Name) +``` + +Ordering of nested collections in `Include()` is checked the same way: + +```cs +// Throws, since it matches the default ordering for Employee +var departments = await context.Departments + .Include(_ => _.Employees + .OrderBy(employee => employee.HireDate) + .ThenByDescending(employee => employee.Salary)) + .ToListAsync(); +``` + +Detection happens during query compilation, so it applies to every distinct query the application executes, whether or not the query hits the database. + + ## Configuration Errors Calling `OrderBy` or `OrderByDescending` multiple times for the same entity type throws an `Exception`: @@ -351,7 +422,7 @@ public class AppDbContext : DbContext public DbSet Employees => Set(); } ``` -snippet source | anchor +snippet source | anchor diff --git a/src/EfOrderBy/IncludeOrderingApplicator.cs b/src/EfOrderBy/IncludeOrderingApplicator.cs index eb8a5c5..b1f270c 100644 --- a/src/EfOrderBy/IncludeOrderingApplicator.cs +++ b/src/EfOrderBy/IncludeOrderingApplicator.cs @@ -1,7 +1,7 @@ /// /// Visitor that applies default ordering to nested collections in Include(). /// -sealed class IncludeOrderingApplicator(IModel model) : +sealed class IncludeOrderingApplicator(IModel model, bool detectRedundantOrdering) : ExpressionVisitor { static readonly ConcurrentDictionary collectionElementTypeCache = new(); @@ -48,6 +48,11 @@ MethodCallExpression ProcessInclude(MethodCallExpression includeCall) if (Interceptor.HasOrdering(lambda.Body)) { // Already has explicit ordering, don't apply default + if (detectRedundantOrdering) + { + RedundantOrder.Validate(lambda.Body); + } + return includeCall; } diff --git a/src/EfOrderBy/Interceptor.cs b/src/EfOrderBy/Interceptor.cs index dbe0a6e..58c66dc 100644 --- a/src/EfOrderBy/Interceptor.cs +++ b/src/EfOrderBy/Interceptor.cs @@ -16,8 +16,10 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven var model = context.Model; RequiredOrder.Validate(context); + var detectRedundantOrdering = RedundantOrder.IsEnabled(context); + // First, process Include nodes to add ordering to nested collections - var visitor = new IncludeOrderingApplicator(model); + var visitor = new IncludeOrderingApplicator(model, detectRedundantOrdering); var queryWithOrderedIncludes = visitor.Visit(query); // Analyze the query for ordering and includes in a single pass @@ -26,6 +28,11 @@ public Expression QueryCompilationStarting(Expression query, QueryExpressionEven if (analyzer.HasOrdering) { + if (detectRedundantOrdering) + { + RedundantOrder.Validate(query); + } + return queryWithOrderedIncludes; } diff --git a/src/EfOrderBy/OrderByExtensions.cs b/src/EfOrderBy/OrderByExtensions.cs index 1587378..a2dd6bb 100644 --- a/src/EfOrderBy/OrderByExtensions.cs +++ b/src/EfOrderBy/OrderByExtensions.cs @@ -21,16 +21,22 @@ public static class OrderByExtensions /// When true (default), automatically creates database indexes for configured orderings. /// Set to false to disable automatic index creation. /// + /// + /// When true, throws an exception when a query explicitly orders by exactly the same + /// properties and directions as the configured default ordering. Useful in tests to + /// find query ordering that is redundant because the default ordering already applies it. + /// public static DbContextOptionsBuilder UseDefaultOrderBy( this DbContextOptionsBuilder builder, bool requireOrderingForAllEntities = false, - bool? createIndexes = true) + bool? createIndexes = true, + bool throwOnRedundantOrderBy = false) { builder.AddInterceptors(interceptor); // Always add the extension to register the convention that marks the model ((IDbContextOptionsBuilderInfrastructure)builder).AddOrUpdateExtension( - new OrderRequiredExtension(requireOrderingForAllEntities, createIndexes ?? true)); + new OrderRequiredExtension(requireOrderingForAllEntities, createIndexes ?? true, throwOnRedundantOrderBy)); return builder; } diff --git a/src/EfOrderBy/OrderRequiredExtension.cs b/src/EfOrderBy/OrderRequiredExtension.cs index 3ba6b99..8d5d61d 100644 --- a/src/EfOrderBy/OrderRequiredExtension.cs +++ b/src/EfOrderBy/OrderRequiredExtension.cs @@ -1,7 +1,8 @@ -sealed class OrderRequiredExtension(bool requireOrderingForAllEntities, bool createIndexes) : +sealed class OrderRequiredExtension(bool requireOrderingForAllEntities, bool createIndexes, bool throwOnRedundantOrderBy) : IDbContextOptionsExtension { public bool RequireOrderingForAllEntities { get; } = requireOrderingForAllEntities; + public bool ThrowOnRedundantOrderBy { get; } = throwOnRedundantOrderBy; bool CreateIndexes { get; } = createIndexes; public DbContextOptionsExtensionInfo Info => new ExtensionInfo(this); @@ -32,20 +33,23 @@ class ExtensionInfo(IDbContextOptionsExtension extension) : public override string LogFragment => $"{(Extension.RequireOrderingForAllEntities ? "RequireOrderingForAllEntities " : "")}" + - $"{(Extension.CreateIndexes ? "" : "CreateIndexes=false ")}"; + $"{(Extension.CreateIndexes ? "" : "CreateIndexes=false ")}" + + $"{(Extension.ThrowOnRedundantOrderBy ? "ThrowOnRedundantOrderBy " : "")}"; public override int GetServiceProviderHashCode() => - HashCode.Combine(Extension.RequireOrderingForAllEntities, Extension.CreateIndexes); + HashCode.Combine(Extension.RequireOrderingForAllEntities, Extension.CreateIndexes, Extension.ThrowOnRedundantOrderBy); public override bool ShouldUseSameServiceProvider(DbContextOptionsExtensionInfo other) => other is ExtensionInfo otherInfo && Extension.RequireOrderingForAllEntities == otherInfo.Extension.RequireOrderingForAllEntities && - Extension.CreateIndexes == otherInfo.Extension.CreateIndexes; + Extension.CreateIndexes == otherInfo.Extension.CreateIndexes && + Extension.ThrowOnRedundantOrderBy == otherInfo.Extension.ThrowOnRedundantOrderBy; public override void PopulateDebugInfo(IDictionary debugInfo) { debugInfo["DefaultOrderBy:RequireOrderingForAllEntities"] = Extension.RequireOrderingForAllEntities.ToString(); debugInfo["DefaultOrderBy:CreateIndexes"] = Extension.CreateIndexes.ToString(); + debugInfo["DefaultOrderBy:ThrowOnRedundantOrderBy"] = Extension.ThrowOnRedundantOrderBy.ToString(); } } } diff --git a/src/EfOrderBy/RedundantOrder.cs b/src/EfOrderBy/RedundantOrder.cs new file mode 100644 index 0000000..bba1327 --- /dev/null +++ b/src/EfOrderBy/RedundantOrder.cs @@ -0,0 +1,177 @@ +/// +/// Detects explicit ordering in a query that exactly duplicates the configured default ordering. +/// +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) + { + return; + } + + var (elementType, clauses) = ordering; + + if (Configuration.TryGet(elementType) is not { Clauses.Count: > 0 } configuration) + { + return; + } + + if (!configuration.ClauseMetadataList.SequenceEqual(clauses)) + { + return; + } + + throw new( + $""" + The query explicitly orders '{elementType.Name}' by {Describe(clauses)}, which exactly matches the default ordering configured for that entity. + Remove the ordering from the query since it is applied automatically, or change it to a different ordering. + """); + } + + /// + /// Walks the outermost ordering chain of a query and returns the ordered element type + /// along with the clauses in the order they are applied. Returns null when the query has + /// no ordering, or when a clause cannot be expressed as default ordering configuration. + /// + static (Type ElementType, List Clauses)? FindOrdering(Expression expression) + { + var clauses = new List(); + Type? elementType = null; + + while (expression is MethodCallExpression call) + { + var method = call.Method; + if (IsOrderingMethod(method, out var descending, out var isThenBy)) + { + // The overloads taking an IComparer cannot be expressed as configuration + if (call.Arguments.Count != 2) + { + return null; + } + + if (FindPropertyName(call.Arguments[1]) is not { } propertyName) + { + return null; + } + + // The chain is walked outermost first, so each clause found is applied before the previous + clauses.Insert(0, new(propertyName, descending, isThenBy)); + elementType = method.GetGenericArguments()[0]; + expression = call.Arguments[0]; + continue; + } + + // The chain ends at the first non ordering call + if (clauses.Count > 0) + { + break; + } + + if (FindSource(call) is not { } source) + { + return null; + } + + expression = source; + } + + if (elementType == null) + { + return null; + } + + return (elementType, clauses); + } + + // 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 && + call.Arguments.Count > 0 && + typeof(System.Collections.IEnumerable).IsAssignableFrom(call.Arguments[0].Type)) + { + return call.Arguments[0]; + } + + return null; + } + + static bool IsOrderingMethod(MethodInfo method, out bool descending, out bool isThenBy) + { + var declaringType = method.DeclaringType; + if (declaringType == typeof(Queryable) || + declaringType == typeof(Enumerable)) + { + switch (method.Name) + { + case "OrderBy": + descending = false; + isThenBy = false; + return true; + case "OrderByDescending": + descending = true; + isThenBy = false; + return true; + case "ThenBy": + descending = false; + isThenBy = true; + return true; + case "ThenByDescending": + descending = true; + isThenBy = true; + return true; + } + } + + descending = false; + isThenBy = false; + return false; + } + + // Queryable methods take a quoted lambda, Enumerable methods take the lambda directly + static string? FindPropertyName(Expression keySelector) + { + if (keySelector is UnaryExpression + { + NodeType: ExpressionType.Quote, + Operand: LambdaExpression quoted + }) + { + keySelector = quoted; + } + + if (keySelector is LambdaExpression + { + Body: MemberExpression + { + Expression: ParameterExpression, + Member: PropertyInfo property + } + }) + { + return property.Name; + } + + return null; + } + + static string Describe(List clauses) => + string.Join(".", clauses.Select(_ => $"{MethodName(_)}({_.PropertyName})")); + + static string MethodName(Configuration.ClauseMetadata clause) => + (clause.IsThenBy, clause.Descending) switch + { + (false, false) => "OrderBy", + (false, true) => "OrderByDescending", + (true, false) => "ThenBy", + _ => "ThenByDescending" + }; +} diff --git a/src/Tests/RedundantOrderByTests.cs b/src/Tests/RedundantOrderByTests.cs new file mode 100644 index 0000000..32a4fda --- /dev/null +++ b/src/Tests/RedundantOrderByTests.cs @@ -0,0 +1,285 @@ +[TestFixture] +public class RedundantOrderByTests +{ + static DbContextOptions enabled = + new DbContextOptionsBuilder() + .UseSqlServer("Server=.;Database=Test;") + .UseDefaultOrderBy(throwOnRedundantOrderBy: true) + .Options; + + static DbContextOptions disabled = + new DbContextOptionsBuilder() + .UseSqlServer("Server=.;Database=Test;") + .UseDefaultOrderBy() + .Options; + + [Test] + public void ExactMatch_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + var exception = Assert.Throws( + () => context.Entities + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .ToQueryString()); + + Assert.That(exception!.Message, Does.Contain("RedundantEntity")); + Assert.That(exception.Message, Does.Contain("OrderBy(Name).ThenByDescending(Priority)")); + } + + [Test] + public void ExactMatchAfterWhere_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.Throws( + () => context.Entities + .Where(_ => _.Priority > 1) + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .ToQueryString()); + } + + [Test] + public void ExactMatchBeforeWhere_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.Throws( + () => context.Entities + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .Where(_ => _.Priority > 1) + .ToQueryString()); + } + + [Test] + public void ExactMatchWithSelect_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.Throws( + () => context.Entities + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .Select(_ => _.Name) + .ToQueryString()); + } + + [Test] + public void ExactMatchOnSingleClause_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.Throws( + () => context.Children + .OrderBy(_ => _.SortOrder) + .ToQueryString()); + } + + [Test] + public void PartialMatch_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderBy(_ => _.Name) + .ToQueryString()); + } + + [Test] + public void ExtraClause_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .ThenBy(_ => _.Id) + .ToQueryString()); + } + + [Test] + public void DifferentDirection_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderByDescending(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .ToQueryString()); + } + + [Test] + public void DifferentProperty_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderBy(_ => _.Id) + .ToQueryString()); + } + + [Test] + public void ReorderedClauses_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderByDescending(_ => _.Priority) + .ThenBy(_ => _.Name) + .ToQueryString()); + } + + [Test] + public void NoExplicitOrdering_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .ToQueryString()); + } + + [Test] + public void EntityWithoutConfiguration_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Unordered + .OrderBy(_ => _.Value) + .ToQueryString()); + } + + [Test] + public void Include_ExactMatch_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + var exception = Assert.Throws( + () => context.Entities + .Include(_ => _.Children.OrderBy(child => child.SortOrder)) + .ToQueryString()); + + Assert.That(exception!.Message, Does.Contain("RedundantChild")); + Assert.That(exception.Message, Does.Contain("OrderBy(SortOrder)")); + } + + [Test] + public void Include_ExactMatchWithFilter_Throws() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.Throws( + () => context.Entities + .Include(_ => _.Children + .Where(child => child.SortOrder > 0) + .OrderBy(child => child.SortOrder)) + .ToQueryString()); + } + + [Test] + public void Include_DifferentOrdering_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .Include(_ => _.Children.OrderBy(child => child.Title)) + .ToQueryString()); + } + + [Test] + public void Include_WithoutOrdering_DoesNotThrow() + { + using var context = new RedundantEnabledContext(enabled); + + Assert.DoesNotThrow( + () => context.Entities + .Include(_ => _.Children) + .ToQueryString()); + } + + [Test] + public void Disabled_DoesNotThrow() + { + using var context = new RedundantDisabledContext(disabled); + + Assert.DoesNotThrow( + () => context.Entities + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority) + .ToQueryString()); + } +} + +public class RedundantEntity +{ + public int Id { get; set; } + public string Name { get; set; } = ""; + public int Priority { get; set; } + public List Children { get; set; } = []; +} + +public class RedundantChild +{ + public int Id { get; set; } + public int RedundantEntityId { get; set; } + public RedundantEntity Parent { get; set; } = null!; + public string Title { get; set; } = ""; + public int SortOrder { get; set; } +} + +public class RedundantUnorderedEntity +{ + public int Id { get; set; } + public string Value { get; set; } = ""; +} + +public class RedundantEnabledContext(DbContextOptions options) : + DbContext(options) +{ + public DbSet Entities => Set(); + public DbSet Children => Set(); + public DbSet Unordered => Set(); + + protected override void OnModelCreating(ModelBuilder builder) => + builder.ConfigureRedundantEntities(); +} + +public class RedundantDisabledContext(DbContextOptions options) : + DbContext(options) +{ + public DbSet Entities => Set(); + public DbSet Children => Set(); + + protected override void OnModelCreating(ModelBuilder builder) => + builder.ConfigureRedundantEntities(); +} + +static class RedundantModelBuilder +{ + // Both contexts share these entities, so the configuration must match + public static void ConfigureRedundantEntities(this ModelBuilder builder) + { + builder.Entity() + .HasMany(_ => _.Children) + .WithOne(_ => _.Parent) + .HasForeignKey(_ => _.RedundantEntityId) + .IsRequired(); + + builder.Entity() + .OrderBy(_ => _.Name) + .ThenByDescending(_ => _.Priority); + + builder.Entity() + .OrderBy(_ => _.SortOrder); + } +} diff --git a/src/Tests/Snippets.cs b/src/Tests/Snippets.cs index a30ea14..b144747 100644 --- a/src/Tests/Snippets.cs +++ b/src/Tests/Snippets.cs @@ -40,6 +40,17 @@ protected override void OnConfiguring(DbContextOptionsBuilder builder) => #endregion } +public class ThrowOnRedundantOrderByExample : DbContext +{ + #region ThrowOnRedundantOrderBy + + protected override void OnConfiguring(DbContextOptionsBuilder builder) => + builder.UseDefaultOrderBy( + throwOnRedundantOrderBy: true); + + #endregion +} + public class DisableIndexCreationExample : DbContext { #region DisableIndexCreation