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