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;