From 734598749ec3f5d4a11bf5d62e55dc0b99f76ac9 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Fri, 6 Nov 2026 21:56:15 +1100 Subject: [PATCH] Name the collections in the MaxSingleQueryCollections message A query over the level only said how many collections it loaded side by side, so the developer had to read the query to find which Include to move to a query of its own. The violation now carries the collections, in query order, named as the entity they are from and the navigations to them, and the message and log line list them. --- readme.md | 2 + src/EfQueryComplexity/CollectionCounter.cs | 120 ++++++++++++++---- .../QueryComplexityViolation.cs | 7 + src/EfQueryComplexity/QueryShape.cs | 2 +- src/EfQueryComplexity/Violations.cs | 17 ++- ...SingleQueryCollectionsMessage.verified.txt | 23 ++++ src/Tests/ShapeTests.cs | 100 ++++++++++++++- 7 files changed, 241 insertions(+), 30 deletions(-) create mode 100644 src/Tests/ShapeTests.SingleQueryCollectionsMessage.verified.txt diff --git a/readme.md b/readme.md index 7632206..3e90e87 100644 --- a/readme.md +++ b/readme.md @@ -163,6 +163,8 @@ It does not count: The log default of 1 allows one chain of nested collections, and logs as soon as two load side by side. +The message names the collections loaded side by side, in query order, and so does `QueryComplexityViolation.Collections`. Each is the entity it is from and the navigations to it, so `Employees.Include(_ => _.Tasks).Include(_ => _.Department).ThenInclude(_ => _.Employees)` reports `Employee.Tasks, Employee.Department.Employees`. A chain of nested collections is named by the one at its end. + ### Unbounded queries diff --git a/src/EfQueryComplexity/CollectionCounter.cs b/src/EfQueryComplexity/CollectionCounter.cs index 21c632e..ce710b3 100644 --- a/src/EfQueryComplexity/CollectionCounter.cs +++ b/src/EfQueryComplexity/CollectionCounter.cs @@ -14,61 +14,88 @@ sealed class CollectionCounter(IModel model) : ExpressionVisitor { + // A counted collection, named for the message as the entity it is from and the navigations to + // it, with where in the query it was found so the message lists them in query order + readonly record struct Collection(int Order, string Name); + // Keyed on the full path from the root, so an Include chain that restates a collection, to // ThenInclude something else below it, counts that collection once HashSet includePaths = []; HashSet ignoredIncludes = []; // The collections a projection returns in place of the entity they belong to, keyed like - // includePaths, with the count of what the projection loads inside each. An Include of one of - // them only says what to load under it, so the two are one collection. - Dictionary projectedPaths = []; + // includePaths, with what the projection loads inside each. An Include of one of them only says + // what to load under it, so the two are one collection. + Dictionary> projectedPaths = []; // What projections load that has no path, so cannot be nested under an Include - int projected; + List projected = []; + + // The name and position of each path, from where it was first found + Dictionary paths = []; + int order; - public static int Count(Expression query, IModel model, bool splitByDefault) + /// + /// The collections the query loads side by side, in query order, named as the entity they are + /// from and the navigations to them, for example Department.Employees. + /// + public static IReadOnlyList Count(Expression query, IModel model, bool splitByDefault) { if (IsSplit(query, splitByDefault)) { - return 0; + return []; } var counter = new CollectionCounter(model); counter.Visit(query); - return counter.CountPaths() + counter.projected; + var collections = counter.CountPaths(); + collections.AddRange(counter.projected); + return collections + .OrderBy(_ => _.Order) + .Select(_ => _.Name) + .ToList(); } // Each collection counts as one, or as the sum of what it holds when that is more. A path is held // by the longest other path it starts with, so walking the longest first counts every collection // before the one holding it. - int CountPaths() + List CountPaths() { - var held = new Dictionary(projectedPaths); + var held = new Dictionary>(); + foreach (var (path, inner) in projectedPaths) + { + held[path] = [..inner]; + } + foreach (var path in includePaths) { - held.TryAdd(path, 0); + held.TryAdd(path, []); } - var total = 0; + var total = new List(); foreach (var path in held.Keys.OrderByDescending(_ => _.Length).ToList()) { - var count = Math.Max(1, held[path]); + var collections = held[path]; + if (collections.Count == 0) + { + collections = [paths[path]]; + } + var holder = Holder(path, held); if (holder == null) { - total += count; + total.AddRange(collections); } else { - held[holder] += count; + held[holder].AddRange(collections); } } return total; } - static string? Holder(string path, Dictionary paths) + static string? Holder(string path, Dictionary> paths) { var current = path; while (true) @@ -117,12 +144,14 @@ protected override Expression VisitMethodCall(MethodCallExpression node) if (IsInclude(method)) { + // The source first, so the Includes are found in query order + var result = base.VisitMethodCall(node); if (!ignoredIncludes.Contains(node)) { AddIncludePaths(node); } - return base.VisitMethodCall(node); + return result; } if (declaringType != typeof(Queryable) && @@ -142,7 +171,7 @@ protected override Expression VisitMethodCall(MethodCallExpression node) Visit(node.Arguments[0]); var projection = new ProjectionCounter(this, RowNotReturned(selector)); projection.Visit(selector); - projected += projection.Count; + projected.AddRange(projection.Collections); return node; } @@ -243,19 +272,22 @@ void AddIncludePaths(MethodCallExpression node) var rootType = IncludeChainRoot(node, path); var type = rootType; var key = PathRoot(rootType); + var name = rootType.Name; - foreach (var name in path) + foreach (var segment in path) { - var navigation = FindNavigation(type, name); + var navigation = FindNavigation(type, segment); if (navigation == null) { return; } - key += "." + name; - if (navigation.IsCollection) + key += "." + segment; + name += "." + segment; + if (navigation.IsCollection && + includePaths.Add(key)) { - includePaths.Add(key); + AddPath(key, name); } type = navigation.TargetEntityType.ClrType; @@ -266,6 +298,12 @@ void AddIncludePaths(MethodCallExpression node) static string PathRoot(Type type) => type.FullName ?? type.Name; + void AddPath(string key, string name) => + paths.TryAdd(key, Name(name)); + + Collection Name(string name) => + new(order++, name); + // Fills path with the navigation names from the root entity to the end of this Include chain, // and returns the root entity type static Type IncludeChainRoot(MethodCallExpression node, List path) @@ -376,7 +414,7 @@ sealed class ProjectionCounter(CollectionCounter counter, ParameterExpression? r ExpressionVisitor { // The collections with no path, which an Include cannot be nested under - public int Count { get; private set; } + public List Collections { get; } = []; public override Expression? Visit(Expression? node) { @@ -388,13 +426,22 @@ sealed class ProjectionCounter(CollectionCounter counter, ParameterExpression? r if (Sequences.IsSequence(node.Type) && LoadsRows(node, out var navigation)) { + var name = counter.Name(Describe(node, navigation)); + // A projection inside the collection can return further collections, which are // nested in it var inner = new ProjectionCounter(counter, null); inner.VisitSelectors(node); - if (!AddProjectedPath(navigation, inner.Count)) + if (!AddProjectedPath(navigation, inner.Collections)) { - Count += Math.Max(1, inner.Count); + if (inner.Collections.Count == 0) + { + Collections.Add(name); + } + else + { + Collections.AddRange(inner.Collections); + } } return node; @@ -403,6 +450,18 @@ sealed class ProjectionCounter(CollectionCounter counter, ParameterExpression? r return base.Visit(node); } + // A collection navigation is named for the entity it is read from, and a query for the set + // it reads + static string Describe(Expression node, MemberExpression? navigation) + { + if (navigation == null) + { + return $"DbSet<{Sequences.ElementType(node.Type).Name}>"; + } + + return $"{navigation.Expression!.Type.Name}.{navigation.Member.Name}"; + } + // An aggregate, like Count or Any, reads a collection without returning it protected override Expression VisitMethodCall(MethodCallExpression node) { @@ -461,7 +520,7 @@ bool LoadsRows(Expression node, out MemberExpression? navigation) // Records a collection read from the row the projection is given, by the path an Include // would name it with. A collection projected twice is loaded twice, so the second has no path. - bool AddProjectedPath(MemberExpression? navigation, int inner) + bool AddProjectedPath(MemberExpression? navigation, List inner) { if (navigation == null || row == null) @@ -495,7 +554,14 @@ bool AddProjectedPath(MemberExpression? navigation, int inner) return false; } - return counter.projectedPaths.TryAdd(PathRoot(row.Type) + path, inner); + var key = PathRoot(row.Type) + path; + if (!counter.projectedPaths.TryAdd(key, inner)) + { + return false; + } + + counter.AddPath(key, row.Type.Name + path); + return true; } void VisitSelectors(Expression node) diff --git a/src/EfQueryComplexity/QueryComplexityViolation.cs b/src/EfQueryComplexity/QueryComplexityViolation.cs index b91955e..c595643 100644 --- a/src/EfQueryComplexity/QueryComplexityViolation.cs +++ b/src/EfQueryComplexity/QueryComplexityViolation.cs @@ -13,4 +13,11 @@ public sealed record QueryComplexityViolation(string Limit, int? Max, int? Actua /// every other level. /// public IReadOnlyList? RowTypes { get; init; } + + /// + /// For MaxSingleQueryCollections, the collections the query loads side by side, in query order, + /// each named as the entity it is from and the navigations to it, for example + /// Department.Employees. Null for every other level. + /// + public IReadOnlyList? Collections { get; init; } } diff --git a/src/EfQueryComplexity/QueryShape.cs b/src/EfQueryComplexity/QueryShape.cs index 70c4dd9..400b189 100644 --- a/src/EfQueryComplexity/QueryShape.cs +++ b/src/EfQueryComplexity/QueryShape.cs @@ -8,6 +8,6 @@ readonly record struct QueryShape( int NavigationDepth, int Includes, int IncludeDepth, - int SingleQueryCollections, + IReadOnlyList SingleQueryCollections, IReadOnlyList UnboundedTypes, IReadOnlyList UnboundedTypesWhenListsLimited); diff --git a/src/EfQueryComplexity/Violations.cs b/src/EfQueryComplexity/Violations.cs index 062e821..2a41cda 100644 --- a/src/EfQueryComplexity/Violations.cs +++ b/src/EfQueryComplexity/Violations.cs @@ -12,7 +12,16 @@ public static List ForShape(QueryShape shape, QueryCom Add(violations, nameof(QueryComplexityLimits.MaxNavigationDepth), limits.MaxNavigationDepth, shape.NavigationDepth); Add(violations, nameof(QueryComplexityLimits.MaxIncludes), limits.MaxIncludes, shape.Includes); Add(violations, nameof(QueryComplexityLimits.MaxIncludeDepth), limits.MaxIncludeDepth, shape.IncludeDepth); - Add(violations, nameof(QueryComplexityLimits.MaxSingleQueryCollections), limits.MaxSingleQueryCollections, shape.SingleQueryCollections); + + var collections = shape.SingleQueryCollections; + if (collections.Count > limits.MaxSingleQueryCollections) + { + violations.Add( + new(nameof(QueryComplexityLimits.MaxSingleQueryCollections), limits.MaxSingleQueryCollections, collections.Count) + { + Collections = collections + }); + } // A key looked up in a list returns a row for each value in it, so only bounds the query when // MaxInValues limits the list @@ -124,6 +133,12 @@ static string Describe(QueryComplexityViolation violation) return $"RejectUnbounded: the query returns {names} rows without a Take"; } + if (violation.Collections != null) + { + var names = string.Join(", ", violation.Collections); + return $"{violation.Limit}: {violation.Actual} exceeds {violation.Max}. The query loads {names} side by side. Load one of them in a query of its own, or use AsSplitQuery()"; + } + return $"{violation.Limit}: {violation.Actual} exceeds {violation.Max}"; } } diff --git a/src/Tests/ShapeTests.SingleQueryCollectionsMessage.verified.txt b/src/Tests/ShapeTests.SingleQueryCollectionsMessage.verified.txt new file mode 100644 index 0000000..3cebf30 --- /dev/null +++ b/src/Tests/ShapeTests.SingleQueryCollectionsMessage.verified.txt @@ -0,0 +1,23 @@ +{ + Type: QueryComplexityException, + Violations: [ + { + Limit: MaxSingleQueryCollections, + Max: 1, + Actual: 2, + Collections: [ + Employee.Tasks, + Employee.Department.Employees + ] + } + ], + Message: +Query complexity limits exceeded: + * MaxSingleQueryCollections: 2 exceeds 1. The query loads Employee.Tasks, Employee.Department.Employees side by side. Load one of them in a query of its own, or use AsSplitQuery() +Change a level for this query with WithQueryComplexity(), skip every check with IgnoreQueryComplexity(), or change the levels passed to UseQueryComplexity(). +Query: +DbSet() + .Include(_ => _.Tasks) + .Include(_ => _.Department) + .ThenInclude(_ => _.Employees) +} \ No newline at end of file diff --git a/src/Tests/ShapeTests.cs b/src/Tests/ShapeTests.cs index 397baab..ea95dbe 100644 --- a/src/Tests/ShapeTests.cs +++ b/src/Tests/ShapeTests.cs @@ -443,7 +443,7 @@ public async Task SingleQueryCollectionsIgnoresIncludesAnAggregateDrops() .ThenInclude(_ => _.Employees); var count = Expression.Call(typeof(Queryable), nameof(Queryable.Count), [typeof(Company)], query.Expression); - await Assert.That(CollectionCounter.Count(count, context.Model, splitByDefault: false)).IsEqualTo(0); + await Assert.That(CollectionCounter.Count(count, context.Model, splitByDefault: false)).IsEmpty(); } // The projection loads the department names itself, and the Include is ignored @@ -548,6 +548,104 @@ await Assert.That( SingleQueryCollections)) .IsEqualTo(2); + // The message names the collections side by side, so the developer knows which to move + [Test] + public Task SingleQueryCollectionsMessage() + { + var (context, _) = ContextBuilder.Build(throwAt: SingleQueryCollections(Limits.None, 1)); + + return Throws( + () => context.Employees + .Include(_ => _.Tasks) + .Include(_ => _.Department) + .ThenInclude(_ => _.Employees) + .ToQueryString()) + .IgnoreStackTrace(); + } + + [Test] + public async Task SingleQueryCollectionsLogNamesCollections() + { + var (context, logs) = ContextBuilder.Build(logAt: SingleQueryCollections(Limits.None, 1)); + + context.Employees + .Include(_ => _.Tasks) + .Include(_ => _.Department) + .ThenInclude(_ => _.Employees) + .ToQueryString(); + + await Assert.That(logs.Single()).Contains("Employee.Tasks, Employee.Department.Employees"); + } + + // Includes are named in query order, as the full path from the root entity + [Test] + public async Task SingleQueryCollectionsNamesIncludesInQueryOrder() => + await Assert.That( + Collections( + context => context.Companies + .Include(_ => _.Departments) + .ThenInclude(_ => _.Employees) + .Include(_ => _.Departments) + .ThenInclude(_ => _.Company.Departments))) + .IsEquivalentTo(["Company.Departments.Employees", "Company.Departments.Company.Departments"]); + + // A chain is named by the collection at its end, since that is what the row is per + [Test] + public async Task SingleQueryCollectionsNamesNestedIncludesOnce() => + await Assert.That( + Collections( + context => context.Companies + .Include(_ => _.Departments) + .ThenInclude(_ => _.Employees) + .ThenInclude(_ => _.Tasks))) + .IsEquivalentTo(["Company.Departments.Employees.Tasks"]); + + [Test] + public async Task SingleQueryCollectionsNamesStringIncludes() => + await Assert.That( + Collections(context => context.Employees.Include("Tasks").Include("Department.Employees.Tasks"))) + .IsEquivalentTo(["Employee.Tasks", "Employee.Department.Employees.Tasks"]); + + [Test] + public async Task SingleQueryCollectionsNamesProjectedCollections() => + await Assert.That( + Collections( + context => context.Departments.Select( + _ => new + { + _.Name, + Employees = _.Employees.ToList(), + Siblings = _.Company.Departments + .Select(department => new + { + department.Name, + Employees = department.Employees.ToList() + }) + .ToList() + }))) + .IsEquivalentTo(["Department.Employees", "Department.Employees"]); + + [Test] + public async Task SingleQueryCollectionsNamesAProjectedAndIncludedCollectionOnce() => + await Assert.That( + Collections( + context => context.Employees + .Include(_ => _.Tasks) + .Select( + _ => new + { + Employee = _, + Tasks = _.Tasks.ToList() + }))) + .IsEquivalentTo(["Employee.Tasks", "Employee.Tasks"]); + + static IReadOnlyList Collections(Func query) + { + var (context, _) = ContextBuilder.Build(throwAt: SingleQueryCollections(Limits.None, 0)); + var exception = Assert.Throws(() => query(context).ToQueryString()); + return exception.Violations.Single().Collections!; + } + static string Describe(Company company) => company.Name;