diff --git a/docs/defining-graphs.md b/docs/defining-graphs.md index 316bf196..5d8979c8 100644 --- a/docs/defining-graphs.md +++ b/docs/defining-graphs.md @@ -351,6 +351,8 @@ Creating a page-able field is supported through [GraphQL Connections](https://gr Cursors are the zero based index of the edge in the ordered set. `after` and `before` are exclusive, and bound the window of items a page is taken from. `first` keeps that many from the start of the window, then `last` keeps that many from the end, in the order the [Relay specification](https://relay.dev/graphql/connections.htm#sec-Pagination-algorithm) applies them. Edges are always in the set's order, whichever end the page was taken from. `hasPreviousPage` is true when the page starts after the first item, `hasNextPage` when items exist after the page. An empty page has null `startCursor` and `endCursor`. +A root connection reads its items with one query and the total with another. The count query only runs when the selection needs it: when `totalCount` is selected, or when paging with `last` or `before`, which place the window from the end. Paged with `first` and `after`, a selection of `edges`, `items` and `pageInfo` is a single query: it reads one row past the page, and that row answers `hasNextPage`. Without the count, an empty page past the end reports `hasPreviousPage` false, which the Relay specification allows when paging forward. + ### Root Query diff --git a/docs/mdsource/defining-graphs.source.md b/docs/mdsource/defining-graphs.source.md index 313cb72a..3977b8fc 100644 --- a/docs/mdsource/defining-graphs.source.md +++ b/docs/mdsource/defining-graphs.source.md @@ -247,6 +247,8 @@ Creating a page-able field is supported through [GraphQL Connections](https://gr Cursors are the zero based index of the edge in the ordered set. `after` and `before` are exclusive, and bound the window of items a page is taken from. `first` keeps that many from the start of the window, then `last` keeps that many from the end, in the order the [Relay specification](https://relay.dev/graphql/connections.htm#sec-Pagination-algorithm) applies them. Edges are always in the set's order, whichever end the page was taken from. `hasPreviousPage` is true when the page starts after the first item, `hasNextPage` when items exist after the page. An empty page has null `startCursor` and `endCursor`. +A root connection reads its items with one query and the total with another. The count query only runs when the selection needs it: when `totalCount` is selected, or when paging with `last` or `before`, which place the window from the end. Paged with `first` and `after`, a selection of `edges`, `items` and `pageInfo` is a single query: it reads one row past the page, and that row answers `hasNextPage`. Without the count, an empty page past the end reports `hasPreviousPage` false, which the Relay specification allows when paging forward. + ### Root Query diff --git a/src/Benchmarks/RequestSplitBenchmark.cs b/src/Benchmarks/RequestSplitBenchmark.cs index c92926c1..46f6fc48 100644 --- a/src/Benchmarks/RequestSplitBenchmark.cs +++ b/src/Benchmarks/RequestSplitBenchmark.cs @@ -39,6 +39,19 @@ public class RequestSplitBenchmark } """; + const string inMemoryArgumentsQuery = """ + { + parents { + id + property + childrenReversed(where: {property: {startsWith: "Child"}}, orderBy: {property: descending}) { + id + property + } + } + } + """; + const string fragmentsQuery = """ { parents { @@ -111,6 +124,7 @@ public async Task Setup() await Execute(librarySchema, argumentsQuery); capturedArguments = CapturingQuery.Captured!; await Execute(librarySchema, fragmentsQuery); + await Execute(librarySchema, inMemoryArgumentsQuery); await Execute(plainSchema, simpleQuery); projected = includeAppender.ApplyProjection(capturedSimple, null, Parents); @@ -153,6 +167,14 @@ public Task FullWithArguments() => public Task FullWithFragments() => Execute(librarySchema, fragmentsQuery); + /// + /// The where and orderBy of a navigation whose resolver returns a collection other than the + /// projected one, so they are evaluated in memory in the resolver, once per parent row. + /// + [Benchmark] + public Task FullWithInMemoryArguments() => + Execute(librarySchema, inMemoryArgumentsQuery); + [Benchmark] public System.Linq.Expressions.Expression ApplyArguments() => Parents.ApplyGraphQlArguments(capturedArguments, keyNames, true, false).Expression; diff --git a/src/Benchmarks/SimpleQueryBenchmark.cs b/src/Benchmarks/SimpleQueryBenchmark.cs index ac8abdfb..1103ddde 100644 --- a/src/Benchmarks/SimpleQueryBenchmark.cs +++ b/src/Benchmarks/SimpleQueryBenchmark.cs @@ -40,6 +40,12 @@ public ParentGraphType(IEfGraphQLService graphQlService) : name: "children", projection: _ => _.Children, resolve: _ => _.Projection); + // Returns a collection other than the projected one, so its arguments are applied in + // memory, once per parent row + AddNavigationListField( + name: "childrenReversed", + projection: _ => _.Children, + resolve: _ => _.Projection.Reverse()); AutoMap(); } } diff --git a/src/GraphQL.EntityFramework/ConnectionConverter.cs b/src/GraphQL.EntityFramework/ConnectionConverter.cs index 591cb5ca..6454d6cc 100644 --- a/src/GraphQL.EntityFramework/ConnectionConverter.cs +++ b/src/GraphQL.EntityFramework/ConnectionConverter.cs @@ -39,7 +39,8 @@ public static Connection ApplyConnectionContext(List list, int? first, var count = list.Count; var (skip, take) = Window(first, after, last, before, count); var page = list.Skip(skip).Take(take); - return Build(skip, take, count, page); + // long, since a large `first` makes take + skip overflow and wrap negative + return Build(skip, count, skip > 0, count > (long) take + skip, page); } /// @@ -112,22 +113,100 @@ public static async Task> ApplyConnectionContext result = await page.ToListAsync(cancel); + + int skip; + int? count = null; + bool hasPreviousPage; + bool hasNextPage; + List rows; + if (NeedsCount(context, last, before)) + { + count = await queryable.CountAsync(cancel); + cancel.ThrowIfCancellationRequested(); + int take; + (skip, take) = Window(first, after, last, before, count.Value); + var page = queryable.Skip(skip).Take(take); + QueryLogger.Write(page); + rows = await page.ToListAsync(cancel); + hasPreviousPage = skip > 0; + // long, since a large `first` makes take + skip overflow and wrap negative + hasNextPage = count > (long) take + skip; + } + else + { + // The window is bounded from the start only, so it needs no count to place it. The + // count clamped the offset to the end; past it the page query reads an empty page. + skip = after + 1 ?? 0; + var page = queryable.Skip(skip); + if (first is not null) + { + // One row past the page says whether a next page exists + page = page.Take(Peek(first.Value)); + } + + QueryLogger.Write(page); + rows = await page.ToListAsync(cancel); + hasNextPage = first is not null && rows.Count > first.Value; + // Rows on the page prove rows before it. An empty page proves nothing, and paging + // forward the spec allows false when that is unknown. + hasPreviousPage = skip > 0 && rows.Count > 0; + if (hasNextPage) + { + rows.RemoveAt(rows.Count - 1); + } + } + + IEnumerable result = rows; if (filters != null) { result = await filters.ApplyFilter(result, context.UserContext, data, context.User); } cancel.ThrowIfCancellationRequested(); - return Build(skip, take, count, result); + return Build(skip, count, hasPreviousPage, hasNextPage, result); + } + + /// + /// The page size plus the one row read past it, capped so the largest page size does not wrap. + /// + static int Peek(int first) => + first == int.MaxValue ? first : first + 1; + + /// + /// Whether the count query has to run: when totalCount is selected, or when the window is + /// bounded from the end, by last or before, so the count is needed to place it. The page + /// info alone does not need it, since hasNextPage is answered by the row read past the page. + /// A connection selecting edges, items and page info otherwise paid a second round trip, a + /// COUNT over the whole filtered set, for a number nothing read. The selection is unknown + /// for a context built outside an execution, which counts. + /// + static bool NeedsCount(IResolveFieldContext context, int? last, int? before) + { + if (last is not null || + before is not null) + { + return true; + } + + var subFields = context.SubFields; + if (subFields is null) + { + return true; + } + + foreach (var (field, _) in subFields.Values) + { + if (field.Name.Value.Equals("totalCount")) + { + return true; + } + } + + return false; } - static Connection Build(int skip, int take, int count, IEnumerable result) + /// Null when the count query was skipped, which only happens when the total count was not selected. + static Connection Build(int skip, int? count, bool hasPreviousPage, bool hasNextPage, IEnumerable result) { var edges = result .Select((item, index) => @@ -144,9 +223,8 @@ static Connection Build(int skip, int take, int count, IEnumerable resu Edges = edges, PageInfo = new() { - // long, since a large `first` makes take + skip overflow and wrap negative - HasNextPage = count > (long) take + skip, - HasPreviousPage = skip > 0, + HasNextPage = hasNextPage, + HasPreviousPage = hasPreviousPage, // Null when there are no edges, as the spec has it. The edges are used rather // than the window since filters can remove items after the query. StartCursor = edges.FirstOrDefault()?.Cursor, diff --git a/src/GraphQL.EntityFramework/Filters/Filters.cs b/src/GraphQL.EntityFramework/Filters/Filters.cs index 8bd97680..79a30633 100644 --- a/src/GraphQL.EntityFramework/Filters/Filters.cs +++ b/src/GraphQL.EntityFramework/Filters/Filters.cs @@ -93,6 +93,7 @@ void AddEntry(IFilterEntry entry) forType.Add(entry); filtersByType.Clear(); + filtersForHierarchy.Clear(); } /// @@ -116,10 +117,17 @@ List> GetFilters(Type entityType) => /// load: those on the type and its base types, which apply to every item, and those on derived /// types, which apply to the derived items the query can return. /// - internal IEnumerable> GetFiltersForHierarchy(Type entityType) => - entries - .Where(_ => _.Key.IsAssignableFrom(entityType) || entityType.IsAssignableFrom(_.Key)) - .SelectMany(_ => _.Value); + internal IReadOnlyList> GetFiltersForHierarchy(Type entityType) => + filtersForHierarchy.GetOrAdd( + entityType, + type => entries + .Where(_ => _.Key.IsAssignableFrom(type) || type.IsAssignableFrom(_.Key)) + .SelectMany(_ => _.Value) + .ToList()); + + // Looked up for every entity type in a projection on every request, so cached per type the + // same way as GetFilters, and reset when a filter is added + ConcurrentDictionary>> filtersForHierarchy = new(); /// /// Returns true if there are any filters registered. diff --git a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Navigation.cs b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Navigation.cs index ccc3d07f..a2883d64 100644 --- a/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Navigation.cs +++ b/src/GraphQL.EntityFramework/GraphApi/EfGraphQLService_Navigation.cs @@ -28,15 +28,20 @@ public FieldBuilder AddNavigationField( async context => { - var fieldContext = BuildContext(context); + // Runs once per parent row. Building a ResolveEfFieldContext here copied every property + // of the GraphQL.NET context, which forced the lazily computed ones, SubFields, Path, + // ResponsePath, Parent and Arguments, to be computed and allocated per row, when all + // this needs is the DbContext and the filters. + var dbContext = ResolveDbContext(context); + var filters = ResolveFilters(context); var projected = compiledProjection(context.Source); var projectionContext = new ResolveProjectionContext { Projection = projected, - DbContext = fieldContext.DbContext, + DbContext = dbContext, User = context.User, - Filters = fieldContext.Filters, + Filters = filters, FieldContext = context }; @@ -57,12 +62,12 @@ public FieldBuilder AddNavigationField AddNavigationConnectionField(); builder.ResolveAsync(async context => { - var efFieldContext = BuildContext(context); + // Runs once per parent row. Building a ResolveEfFieldContext here copied every property + // of the GraphQL.NET context, which forced the lazily computed ones, SubFields, Path, + // ResponsePath, Parent and Arguments, to be computed and allocated per row, when all + // this needs is the DbContext and the filters. + var dbContext = ResolveDbContext(context); + var filters = ResolveFilters(context); var projected = compiledProjection(context.Source); var projectionContext = new ResolveProjectionContext { Projection = projected, - DbContext = efFieldContext.DbContext, + DbContext = dbContext, User = context.User, - Filters = efFieldContext.Filters, + Filters = filters, FieldContext = context }; @@ -59,11 +64,17 @@ public ConnectionBuilder AddNavigationConnectionField AddNavigationListField>(async context => { - var fieldContext = BuildContext(context); + // Runs once per parent row. Building a ResolveEfFieldContext here copied every property + // of the GraphQL.NET context, which forced the lazily computed ones, SubFields, Path, + // ResponsePath, Parent and Arguments, to be computed and allocated per row, when all + // this needs is the DbContext and the filters. + var dbContext = ResolveDbContext(context); + var filters = ResolveFilters(context); var projected = compiledProjection(context.Source); var projectionContext = new ResolveProjectionContext { Projection = projected, - DbContext = fieldContext.DbContext, + DbContext = dbContext, User = context.User, - Filters = fieldContext.Filters, + Filters = filters, FieldContext = context }; @@ -48,16 +53,22 @@ public FieldBuilder AddNavigationListField +/// The property paths a projection expression reads, grouped by the root property each starts +/// from, in the order the expression reads them. Analyzed once when the field is registered and +/// kept in the field metadata. It was analyzed again on every request that selected the field, +/// with a visitor walk over the expression and the grouping rebuilt each time. +/// +sealed class ProjectionPaths +{ + ProjectionPaths(IReadOnlyList groups) => + Groups = groups; + + public IReadOnlyList Groups { get; } + + /// + /// The root of the first path read. It receives the field's selection set when the type the + /// field returns cannot say which navigation the selection applies to. + /// + public string? PrimaryRoot => + Groups.Count == 0 ? null : Groups[0].Root; + + public static ProjectionPaths Analyze(LambdaExpression projection) + { + var groups = new List(); + var byRoot = new Dictionary(StringComparer.OrdinalIgnoreCase); + + foreach (var path in ProjectionAnalyzer.ExtractPropertyPaths(projection)) + { + var dotIndex = path.IndexOf('.'); + var root = dotIndex >= 0 ? path[..dotIndex] : path; + + if (!byRoot.TryGetValue(root, out var group)) + { + group = new(root); + byRoot[root] = group; + groups.Add(group); + } + + if (dotIndex >= 0) + { + group.Add(path[(dotIndex + 1)..]); + } + } + + return new(groups); + } +} + +/// +/// The paths read below one property of the entity. +/// +sealed class ProjectionPathGroup(string root) +{ + List nested = []; + List nestedScalars = []; + + /// + /// The property on the entity the paths start from. + /// + public string Root { get; } = root; + + /// + /// The paths below , without it. Empty when the root was read whole. + /// + public IReadOnlyList Nested => nested; + + /// + /// The single segment paths in : the scalars read from the navigation. + /// + public IReadOnlyList NestedScalars => nestedScalars; + + internal void Add(string path) + { + nested.Add(path); + if (!path.Contains('.')) + { + nestedScalars.Add(path); + } + } +} diff --git a/src/GraphQL.EntityFramework/IncludeAppender.cs b/src/GraphQL.EntityFramework/IncludeAppender.cs index c7cb77fd..d390801c 100644 --- a/src/GraphQL.EntityFramework/IncludeAppender.cs +++ b/src/GraphQL.EntityFramework/IncludeAppender.cs @@ -329,7 +329,8 @@ void ProcessSelectionSet( { IComplexGraphType? leafGraphType; (selectionSet, leafGraphType) = GetLeafSelection(selectionSet, graphType); - if (selectionSet?.Selections is null) + if (selectionSet?.Selections is null || + !HasFragments(selectionSet)) { return null; } @@ -388,6 +389,24 @@ void ProcessSelectionSet( return result; } + /// + /// Only a fragment can select from a type other than the parent's, so a selection set of plain + /// fields has no derived navigations to collect, and the walk over it is skipped. Most + /// selection sets are plain fields, and the walk ran for every one on every request. + /// + static bool HasFragments(GraphQLSelectionSet selectionSet) + { + foreach (var selection in selectionSet.Selections) + { + if (selection is not GraphQLField) + { + return true; + } + } + + return false; + } + /// /// Navigate through connection wrapper fields (edges/items/node) to find the leaf selection set /// that contains the actual entity fields and inline fragments, and the graph type it selects from. @@ -552,44 +571,20 @@ void ProcessProjectionExpression( GraphQLField field, FieldType fieldType, IComplexGraphType? parentGraphType, - LambdaExpression projection, + ProjectionPaths projection, IReadOnlyDictionary? navigationProperties, HashSet scalarFields, Dictionary navProjections, IResolveFieldContext context) { - var accessedPaths = ProjectionAnalyzer.ExtractPropertyPaths(projection); - - // Group paths by their root navigation property - var pathsByNavigation = new Dictionary>(StringComparer.OrdinalIgnoreCase); - string? primaryNavigation = null; - - foreach (var path in accessedPaths) - { - var dotIndex = path.IndexOf('.'); - var rootProperty = dotIndex >= 0 ? path[..dotIndex] : path; - - if (!pathsByNavigation.TryGetValue(rootProperty, out var paths)) - { - paths = []; - pathsByNavigation[rootProperty] = paths; - } - - if (dotIndex >= 0) - { - paths.Add(path[(dotIndex + 1)..]); - } - - primaryNavigation ??= rootProperty; - } - // The field's selection set applies to the navigations of the type the field returns. // It used to go to whichever path the analyzer visited first, so with `new { _.Child2, // _.Child1 }` resolving Child1, Child2 got the selection and Child1 got nothing under it. var itemType = FieldItemType(fieldType); - foreach (var (navName, nestedPaths) in pathsByNavigation) + foreach (var group in projection.Groups) { + var navName = group.Root; if (!TryFindNavigation(navigationProperties, navName, out var navigation)) { // Scalar field path (no navigation) — add root property to scalarFields @@ -598,7 +593,7 @@ void ProcessProjectionExpression( } var receivesSelection = itemType is null - ? navName == primaryNavigation + ? navName == projection.PrimaryRoot : itemType.IsAssignableFrom(navigation.Type) || navigation.Type.IsAssignableFrom(itemType); var navType = navigation.Type; @@ -612,27 +607,24 @@ void ProcessProjectionExpression( // A navigation accessed as a whole, with nothing read from it in the expression and no // selection set to say which fields are wanted, needs the whole entity. It was // projected with only its keys, so the resolver saw every other property as null. - var isWhole = nestedPaths.Count == 0; + var isWhole = group.Nested.Count == 0; if (receivesSelection && field.SelectionSet is not null) { // A navigation list or connection field projecting the collection itself. Its ids, // where and orderBy are applied inside the collection subquery. if (navigation.IsCollection && - pathsByNavigation.Count == 1 && - nestedPaths.Count == 0) + projection.Groups.Count == 1 && + group.Nested.Count == 0) { arguments = ReadArguments(field, fieldType, parentGraphType, context); } // Primary navigation: merge GraphQL fields with projection-required fields nestedProjection = GetNestedProjection(field.SelectionSet, GetComplexGraphType(fieldType), navType, nestedNavProps, nestedKeys, nestedFks, context); - foreach (var nestedPath in nestedPaths) + foreach (var nestedPath in group.NestedScalars) { - if (!nestedPath.Contains('.')) - { - nestedProjection.ScalarFields.Add(nestedPath); - } + nestedProjection.ScalarFields.Add(nestedPath); } isWhole = false; @@ -640,9 +632,7 @@ void ProcessProjectionExpression( else { // Secondary navigation: include only projection-required fields - var nestedScalarFields = nestedPaths - .Where(_ => !_.Contains('.')) - .ToHashSet(StringComparer.OrdinalIgnoreCase); + var nestedScalarFields = new HashSet(group.NestedScalars, StringComparer.OrdinalIgnoreCase); nestedProjection = new(nestedScalarFields, nestedKeys ?? [], nestedFks ?? new HashSet(), []); } @@ -869,14 +859,31 @@ static void AddNavigation( ? existing.Merge(navProjection) : navProjection; - public static void SetProjectionMetadata(FieldType fieldType, LambdaExpression projection) => - fieldType.Metadata["_EF_Projection"] = projection; + const string projectionKey = "_EF_Projection"; + const string projectionPathsKey = "_EF_ProjectionPaths"; + + public static void SetProjectionMetadata(FieldType fieldType, LambdaExpression projection) + { + fieldType.Metadata[projectionKey] = projection; + // Analyzed here, once, rather than on every request that selects the field + fieldType.Metadata[projectionPathsKey] = ProjectionPaths.Analyze(projection); + } - static bool TryGetProjectionMetadata(FieldType fieldType, [NotNullWhen(true)] out LambdaExpression? projection) + static bool TryGetProjectionMetadata(FieldType fieldType, [NotNullWhen(true)] out ProjectionPaths? projection) { - if (fieldType.Metadata.TryGetValue("_EF_Projection", out var projectionObj)) + var metadata = fieldType.Metadata; + if (metadata.TryGetValue(projectionPathsKey, out var pathsObj) && + pathsObj is ProjectionPaths paths) + { + projection = paths; + return true; + } + + // The expression placed in the metadata directly, without the analysis + if (metadata.TryGetValue(projectionKey, out var projectionObj) && + projectionObj is LambdaExpression expression) { - projection = (LambdaExpression)projectionObj!; + projection = ProjectionPaths.Analyze(expression); return true; } diff --git a/src/GraphQL.EntityFramework/Where/ArgumentProcessor_List.cs b/src/GraphQL.EntityFramework/Where/ArgumentProcessor_List.cs index d55cbd69..2445a8ea 100644 --- a/src/GraphQL.EntityFramework/Where/ArgumentProcessor_List.cs +++ b/src/GraphQL.EntityFramework/Where/ArgumentProcessor_List.cs @@ -32,7 +32,10 @@ public static IEnumerable ApplyGraphQlArguments( bool omitQueryArguments, bool argumentsAppliedInQuery) { - if (omitQueryArguments) + // A field selected without arguments has nothing to apply. Reading them anyway made + // GraphQL.NET build the argument dictionary, which it does lazily, per field per row. + if (omitQueryArguments || + !ArgumentReader.HasArguments(context)) { return items; } @@ -42,20 +45,10 @@ public static IEnumerable ApplyGraphQlArguments( if (!argumentsAppliedInQuery) { - if (keyNames is not null) + var predicate = PredicateCache.GetOrAdd(context, keyNames, static (keyNames, context) => BuildPredicate(keyNames, context)); + if (predicate is not null) { - if (ArgumentReader.TryReadIds(context, out var idValues)) - { - var keyName = GetKeyName(keyNames); - var predicate = ExpressionBuilder.BuildIdPredicate(keyName, idValues); - items = items.Where(Compile(predicate)); - } - } - - if (ArgumentReader.TryReadWhere(context, out var wheres)) - { - var predicate = ExpressionBuilder.BuildPredicate(wheres); - items = items.Where(Compile(predicate)); + items = items.Where(predicate); } (items, order) = Order(items, context); @@ -79,9 +72,42 @@ public static IEnumerable ApplyGraphQlArguments( } /// - /// This path runs once per parent node, so a navigation list field is compiled as many times as there - /// are parents. Emitting IL costs ~700us a call and leaves behind a DynamicMethod that is never - /// collected, which dwarfs the cost of running the predicate over an in memory collection. Interpreting + /// The ids and where of the field as one in memory predicate, or null when it has neither. + /// Built once per request per field, through , since the + /// arguments are the same for every parent row. + /// + static Func? BuildPredicate(List? keyNames, IResolveFieldContext context) + { + Func? ids = null; + if (keyNames is not null && + ArgumentReader.TryReadIds(context, out var idValues)) + { + var keyName = GetKeyName(keyNames); + ids = Compile(ExpressionBuilder.BuildIdPredicate(keyName, idValues)); + } + + Func? where = null; + if (ArgumentReader.TryReadWhere(context, out var wheres)) + { + where = Compile(ExpressionBuilder.BuildPredicate(wheres)); + } + + if (ids is null) + { + return where; + } + + if (where is null) + { + return ids; + } + + return _ => ids(_) && where(_); + } + + /// + /// Emitting IL costs ~700us a call and leaves behind a DynamicMethod that is never collected, + /// which dwarfs the cost of running the predicate over an in memory collection. Interpreting /// is ~20x cheaper to construct and stays ahead until a collection reaches several thousand items. /// static Func Compile(Expression> predicate) => diff --git a/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs b/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs index b8a7d0ce..029e0328 100644 --- a/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs +++ b/src/GraphQL.EntityFramework/Where/ArgumentProcessor_Queryable.cs @@ -10,7 +10,9 @@ public static IQueryable ApplyGraphQlArguments( bool omitQueryArguments) where TItem : class { - if (omitQueryArguments) + // A field selected without arguments has nothing to apply + if (omitQueryArguments || + !ArgumentReader.HasArguments(context)) { return queryable; } diff --git a/src/GraphQL.EntityFramework/Where/ArgumentReader.cs b/src/GraphQL.EntityFramework/Where/ArgumentReader.cs index c7d31889..3f1e01d8 100644 --- a/src/GraphQL.EntityFramework/Where/ArgumentReader.cs +++ b/src/GraphQL.EntityFramework/Where/ArgumentReader.cs @@ -15,6 +15,23 @@ public static bool TryReadWhere(IResolveFieldContext context, out IReadOnlyColle return false; } + /// + /// Whether the field was selected with any arguments. GraphQL.NET builds the argument + /// dictionary lazily, per field per row, so the ast is consulted instead: a field selected + /// without arguments has nothing to read, and that is the common case for a navigation. + /// A context built by hand, outside an execution, carries no ast, so its arguments are used. + /// + public static bool HasArguments(IResolveFieldContext context) + { + var field = context.FieldAst; + if (field is null) + { + return context.Arguments is { Count: > 0 }; + } + + return field.Arguments is { Count: > 0 }; + } + public static IReadOnlyCollection ReadOrderBy(IResolveFieldContext context) { if (TryReadArgument(context, "orderBy", out var value) && diff --git a/src/GraphQL.EntityFramework/Where/ExpressionBuilder.cs b/src/GraphQL.EntityFramework/Where/ExpressionBuilder.cs index 845ea765..67204db6 100644 --- a/src/GraphQL.EntityFramework/Where/ExpressionBuilder.cs +++ b/src/GraphQL.EntityFramework/Where/ExpressionBuilder.cs @@ -16,7 +16,8 @@ public static Expression> BuildPredicate(IReadOnlyCollection wheres) { Expression? mainExpression = null; - var previousWhere = new WhereExpression(); + // The connector on an expression joins it to the next one + var previousConnector = Connector.And; // Iterate over wheres foreach (var where in wheres) @@ -62,11 +63,10 @@ static Expression MakePredicateBody(IReadOnlyCollection wheres) else { // Otherwise combine expression by specified connector or default (AND) if not provided - mainExpression = CombineExpressions(previousWhere.Connector, mainExpression, nextExpression); + mainExpression = CombineExpressions(previousConnector, mainExpression, nextExpression); } - // Save the previous where so the connector can be retrieved - previousWhere = where; + previousConnector = where.Connector; } return mainExpression ?? Expression.Constant(false); diff --git a/src/GraphQL.EntityFramework/Where/PredicateCache.cs b/src/GraphQL.EntityFramework/Where/PredicateCache.cs new file mode 100644 index 00000000..3862ed58 --- /dev/null +++ b/src/GraphQL.EntityFramework/Where/PredicateCache.cs @@ -0,0 +1,39 @@ +/// +/// The in memory predicate of a navigation list or connection field, compiled once per request. +/// The field's resolver runs once per parent row, and building and interpreting the ids and +/// where for every row cost about 4us a row. The arguments of a field are fixed for the request, +/// literal or from variables, so the predicate is the same for every row. Keyed on the execution +/// context, which is per request, since with document caching the ast nodes are shared between +/// requests; the entries are collected with it. The item type is part of the key, since a field +/// selected on an interface is one ast node resolved by each implementing type. +/// +static class PredicateCache +{ + static ConditionalWeakTable> predicates = new(); + + public static Func? GetOrAdd( + IResolveFieldContext context, + List? keyNames, + Func?, IResolveFieldContext, Func?> build) + { + var key = context.ExecutionContext; + var field = context.FieldAst; + // A context built by hand, outside an execution, has no request to cache against + if (key is null || + field is null) + { + return build(keyNames, context); + } + + var forRequest = predicates.GetValue(key, _ => new()); + var cacheKey = (field, typeof(TItem)); + if (forRequest.TryGetValue(cacheKey, out var existing)) + { + return (Func?) existing; + } + + var predicate = build(keyNames, context); + forRequest[cacheKey] = predicate; + return predicate; + } +} diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_past_the_end_is_empty.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_past_the_end_is_empty.verified.txt new file mode 100644 index 00000000..daf4e686 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_past_the_end_is_empty.verified.txt @@ -0,0 +1,21 @@ +{ + target: { + Data: { + parentEntitiesConnection: { + items: [] + } + } + }, + sql: { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 21, + @p1: 3 + } + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_skips_count.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_skips_count.verified.txt new file mode 100644 index 00000000..c97ca379 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_skips_count.verified.txt @@ -0,0 +1,28 @@ +{ + target: { + Data: { + parentEntitiesConnection: { + items: [ + { + property: Value1 + }, + { + property: Value2 + } + ] + } + } + }, + sql: { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 1, + @p1: 3 + } + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_with_last_counts.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_with_last_counts.verified.txt new file mode 100644 index 00000000..30713074 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_items_only_with_last_counts.verified.txt @@ -0,0 +1,35 @@ +{ + target: { + Data: { + parentEntitiesConnection: { + items: [ + { + property: Value6 + }, + { + property: Value7 + } + ] + } + } + }, + sql: [ + { + Text: +select COUNT(*) +from ParentEntities as p + }, + { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 6, + @p1: 2 + } + } + ] +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_last_page.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_last_page.verified.txt new file mode 100644 index 00000000..f9796974 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_last_page.verified.txt @@ -0,0 +1,33 @@ +{ + target: { + Data: { + parentEntitiesConnection: { + pageInfo: { + hasNextPage: false, + hasPreviousPage: true, + endCursor: 7 + }, + items: [ + { + property: Value6 + }, + { + property: Value7 + } + ] + } + } + }, + sql: { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 6, + @p1: 3 + } + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_past_the_end.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_past_the_end.verified.txt new file mode 100644 index 00000000..dd80270c --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_past_the_end.verified.txt @@ -0,0 +1,26 @@ +{ + target: { + Data: { + parentEntitiesConnection: { + pageInfo: { + hasNextPage: false, + hasPreviousPage: false, + endCursor: null + }, + items: [] + } + } + }, + sql: { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 21, + @p1: 3 + } + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_peeks.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_peeks.verified.txt new file mode 100644 index 00000000..08d447d6 --- /dev/null +++ b/src/Tests/IntegrationTests/IntegrationTests.Connection_page_info_without_total_count_peeks.verified.txt @@ -0,0 +1,33 @@ +{ + target: { + Data: { + parentEntitiesConnection: { + pageInfo: { + hasNextPage: true, + hasPreviousPage: true, + endCursor: 2 + }, + items: [ + { + property: Value1 + }, + { + property: Value2 + } + ] + } + } + }, + sql: { + Text: +select p.Id, + p.Property +from ParentEntities as p +order by p.Property +offset @p rows fetch next @p1 rows only, + Parameters: { + @p: 1, + @p1: 3 + } + } +} \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_on_derived_type_requirements_loaded_by_base_typed_query.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_on_derived_type_requirements_loaded_by_base_typed_query.verified.txt index efa9e7d5..a4225e38 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Filter_on_derived_type_requirements_loaded_by_base_typed_query.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_on_derived_type_requirements_loaded_by_base_typed_query.verified.txt @@ -10,14 +10,8 @@ } } }, - sql: [ - { - Text: -select COUNT(*) -from BaseEntities as b - }, - { - Text: + sql: { + Text: select b.Id, b.Discriminator, b.Property, @@ -25,10 +19,9 @@ select b.Id, from BaseEntities as b order by b.Property offset @p rows fetch next @p1 rows only, - Parameters: { - @p: 0, - @p1: 10 - } + Parameters: { + @p: 0, + @p1: 11 } - ] + } } \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Filter_with_projection_accesses_foreign_key.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_projection_accesses_foreign_key.verified.txt index 0c5f9eb2..bfbb4d22 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Filter_with_projection_accesses_foreign_key.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Filter_with_projection_accesses_foreign_key.verified.txt @@ -23,14 +23,8 @@ } } }, - sql: [ - { - Text: -select COUNT(*) -from ParentEntities as p - }, - { - Text: + sql: { + Text: select p0.Id, c.Id, c.ParentId, @@ -47,10 +41,9 @@ from (select p.Id, order by p0.Property, p0.Id, c.Id, - Parameters: { - @p: 0, - @p1: 10 - } + Parameters: { + @p: 0, + @p1: 11 } - ] + } } \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Nested_connection_under_root_connection_projects_scalars.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Nested_connection_under_root_connection_projects_scalars.verified.txt index 197ffc33..953440ca 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Nested_connection_under_root_connection_projects_scalars.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Nested_connection_under_root_connection_projects_scalars.verified.txt @@ -17,14 +17,8 @@ } } }, - sql: [ - { - Text: -select COUNT(*) -from ParentEntities as p - }, - { - Text: + sql: { + Text: select p0.Id, c.Id, c.ParentId, @@ -41,10 +35,9 @@ from (select p.Id, order by p0.Property, p0.Id, c.Id, - Parameters: { - @p: 0, - @p1: 10 - } + Parameters: { + @p: 0, + @p1: 11 } - ] + } } \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.Simplified_filter_connection_query_filtering.verified.txt b/src/Tests/IntegrationTests/IntegrationTests.Simplified_filter_connection_query_filtering.verified.txt index bfb32fde..bc32e090 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.Simplified_filter_connection_query_filtering.verified.txt +++ b/src/Tests/IntegrationTests/IntegrationTests.Simplified_filter_connection_query_filtering.verified.txt @@ -17,23 +17,16 @@ } } }, - sql: [ - { - Text: -select COUNT(*) -from FilterParentEntities as f - }, - { - Text: + sql: { + Text: select f.Id, f.Property from FilterParentEntities as f order by f.Property offset @p rows fetch next @p1 rows only, - Parameters: { - @p: 0, - @p1: 10 - } + Parameters: { + @p: 0, + @p1: 11 } - ] + } } \ No newline at end of file diff --git a/src/Tests/IntegrationTests/IntegrationTests.cs b/src/Tests/IntegrationTests/IntegrationTests.cs index d1ccc8fe..c95fc7da 100644 --- a/src/Tests/IntegrationTests/IntegrationTests.cs +++ b/src/Tests/IntegrationTests/IntegrationTests.cs @@ -650,6 +650,153 @@ public async Task Connection_first_page() await RunQuery(database, query, null, null, false, entities.ToArray()); } + /// + /// The page info without totalCount needs no count: the page query reads one row past the page, and that row answers hasNextPage. + /// + [Fact] + public async Task Connection_page_info_without_total_count_peeks() + { + var query = + """ + { + parentEntitiesConnection(first:2, after: "0") { + pageInfo { + hasNextPage + hasPreviousPage + endCursor + } + items { + property + } + } + } + """; + var entities = BuildEntities(8); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, entities.ToArray()); + } + + /// + /// The last page has no row past it to read, so hasNextPage is false without the count. + /// + [Fact] + public async Task Connection_page_info_without_total_count_last_page() + { + var query = + """ + { + parentEntitiesConnection(first:2, after: "5") { + pageInfo { + hasNextPage + hasPreviousPage + endCursor + } + items { + property + } + } + } + """; + var entities = BuildEntities(8); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, entities.ToArray()); + } + + /// + /// A page past the end is empty, which proves nothing about rows before it, so hasPreviousPage is false, as the spec allows when paging forward. + /// + [Fact] + public async Task Connection_page_info_without_total_count_past_the_end() + { + var query = + """ + { + parentEntitiesConnection(first:2, after: "20") { + pageInfo { + hasNextPage + hasPreviousPage + endCursor + } + items { + property + } + } + } + """; + var entities = BuildEntities(8); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, entities.ToArray()); + } + + /// + /// Nothing selected reads the count, and first and after place the window from the start, so the count query is skipped and the page is a single query. + /// + [Fact] + public async Task Connection_items_only_skips_count() + { + var query = + """ + { + parentEntitiesConnection(first:2, after: "0") { + items { + property + } + } + } + """; + var entities = BuildEntities(8); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, entities.ToArray()); + } + + /// + /// Without the count the offset is not clamped to the end, so a page past it is read from the database as an empty page. + /// + [Fact] + public async Task Connection_items_only_past_the_end_is_empty() + { + var query = + """ + { + parentEntitiesConnection(first:2, after: "20") { + items { + property + } + } + } + """; + var entities = BuildEntities(8); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, entities.ToArray()); + } + + /// + /// last places the window from the end, which needs the count even when nothing selected reads it. + /// + [Fact] + public async Task Connection_items_only_with_last_counts() + { + var query = + """ + { + parentEntitiesConnection(last:2) { + items { + property + } + } + } + """; + var entities = BuildEntities(8); + + await using var database = await sqlInstance.Build(); + await RunQuery(database, query, null, null, false, entities.ToArray()); + } + [Fact] public async Task Connection_page_back() {