Skip to content

Commit d14f0ce

Browse files
lukemurrayclaude
lukemurray
and
claude
committed
only take the two-pass path for root service fields that have bulk to load
Measured on a root service list with no bulk resolver below it (100 rows, two scalar fields, Release): 0.088ms/query on main vs 0.158ms with the two-pass path always on - an extra expression compile and a projection of every row for a query with nothing to bulk load. Gated on a new HasBulkResolverAtOrBelow walk (the same shape as HasServicesAtOrBelow, and it looks inside fragment spreads - without that a hoisted selection quietly drops back to one call per row). Back to 0.083ms when no bulk field is selected. Which nodes materialized their own projection in the first pass is now recorded on the CompileContext, the way possibleNextContextTypes already is, so the second pass cannot disagree with the first about whether to project from that result or rebuild the service expression. Deciding it twice was the cause of the 6 failures the gate first produced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent af5827a commit d14f0ce

8 files changed

Lines changed: 174 additions & 25 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
## Fixes
44

5-
- Fixed `ResolveBulk` / `ResolveBulkAsync` nested under a **root service-resolved list** (e.g. `Query.apiKeys` from `.Resolve<TService>(...)` rather than a context/`DbSet` property) so nested bulk fields load once instead of falling back to per-item `Resolve` (or previously failing with a null `BulkParameter`). Root service list fields now participate in the two-pass flow. `UseFilter()`/`UseSort()` on those root lists still apply on the first pass and are not re-applied against the first-pass Dynamic on the second. Regression tests in `ServiceRootListBulkTests` / `ServiceBackedCollectionExtensionsTests`.
5+
- Fixed `ResolveBulk` / `ResolveBulkAsync` nested under a **root service-resolved list** (e.g. `Query.apiKeys` from `.Resolve<TService>(...)` rather than a context/`DbSet` property) so nested bulk fields load once instead of falling back to per-item `Resolve` (or previously failing with a null `BulkParameter`). Root service list fields now participate in the two-pass flow. `UseFilter()`/`UseSort()` on those root lists still apply on the first pass and are not re-applied against the first-pass Dynamic on the second. Regression tests in `ServiceRootListBulkTests` / `ServiceBackedCollectionExtensionsTests`. A root service list/object only takes the two-pass path when the selection actually has a bulk resolver in it (including through a fragment spread) - otherwise it stays on the single-pass path, as the extra pass costs a compile and a projection of every row for nothing. Note that for the fields that do take it, the service now runs during the *first* pass, so a `BeforeExecuting` hook sees two executions for them (`isFinal` false then true) where it saw one.
66
- Fixed `ResolveBulk` nested under a **root service-resolved object** that exposes a list (status-page shape: `statusPage { items { bulkField } }`). First pass used to return only `ExtractedFieldsFromServices` for root service objects, so nested list selection and bulk registration were skipped; the second pass then either N+1'd via `Resolve` or failed converting entity types to first-pass `Dynamic_*` types (`No coercion operator...`). Root service objects now participate in the two-pass flow (like root service lists), wrap once with `ProjectWithNullCheck` on the first pass, and on the second pass select from the materialized anon instead of re-invoking the service. Also rebinds the `ResolveBulk` `DataSelector` parameter (a different `ParameterExpression` than `FieldParam`) so multi-property keys no longer fail with `variable 'row' ... is not defined`. Nullable-nav bulk keys (`row.DetexyBoard == null ? null : row.DetexyBoard.SerialNumber`) are rewritten via `ExpressionReplacer.VisitConditional` onto the first-pass extracted field instead of leaving a second navigation `MemberExpression` unbound. Exact regression: `HardwareSensorStatusPageBulkTests` (mirrors Offline Active `hardwareSensorStatusPage { items { lastSeen isOffline floor { currentFloorStatus } } }`). Also: `ServiceResolvedPage_ItemsWithResolveBulk_Works`, `ServiceResolvedPage_ItemsWithComplexResolveBulkKey_Works`.
77

88
# 6.1.3

‎src/EntityGraphQL/Compiler/GqlNodes/BaseGraphQLField.cs‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,17 @@ public virtual bool HasServicesAtOrBelow(IReadOnlyDictionary<string, GraphQLFrag
117117
return Field?.Services.Count > 0 || Field?.ExecuteAsService == true || QueryFields.Any(f => f.HasServicesAtOrBelow(fragments));
118118
}
119119

120+
/// <summary>
121+
/// True if this field or anything selected below it has a bulk resolver to load. A root field resolved
122+
/// from services only takes part in the two-pass flow when that is the case: the first pass exists so the
123+
/// bulk keys can be collected from the materialised rows, and it costs an extra expression compile plus a
124+
/// projection of every row, which is not worth paying for a selection with nothing to bulk load.
125+
/// </summary>
126+
public virtual bool HasBulkResolverAtOrBelow(IReadOnlyDictionary<string, GraphQLFragmentStatement> fragments)
127+
{
128+
return Field?.BulkResolver != null || QueryFields.Any(f => f.HasBulkResolverAtOrBelow(fragments));
129+
}
130+
120131
/// <summary>
121132
/// Checks if this field or any of its child fields are async (return Task<T>)
122133
/// </summary>
@@ -336,6 +347,13 @@ internal bool IsExcludedByDirectives(ParameterExpression? docParam, IArgumentsTr
336347
return ProcessDirectivesVisitNode(LocationForDirectives, this, docParam, docVariables) == null;
337348
}
338349

350+
/// <summary>
351+
/// True on the second pass when the first pass materialized this field's own projection (see
352+
/// <see cref="CompileContext.SetFirstPassMaterialized"/>), so the selection is built over that result.
353+
/// </summary>
354+
protected bool UseFirstPassResult(CompileContext compileContext, Expression replacementNextFieldContext) =>
355+
compileContext.WasFirstPassMaterialized(this) && Field?.ReturnType.TypeDotnet != null && replacementNextFieldContext.Type != Field.ReturnType.TypeDotnet;
356+
339357
protected Expression ReplaceContext(Expression replacementNextFieldContext, ParameterReplacer replacer, Expression nextFieldContext, List<Type>? possibleNextContextTypes)
340358
{
341359
var possibleField = replacementNextFieldContext.Type.GetField(Name);
@@ -346,14 +364,6 @@ protected Expression ReplaceContext(Expression replacementNextFieldContext, Para
346364
// If this is a root field, we replace the whole expression unless there is services at the root level
347365
if (IsRootField && !HasServices)
348366
nextFieldContext = replacementNextFieldContext;
349-
else if (IsRootField && HasServices && Field?.ReturnType.TypeDotnet != null && replacementNextFieldContext.Type != Field.ReturnType.TypeDotnet)
350-
{
351-
// Second pass over a root service list/object (e.g. apiKeys { bulkField }, statusPage { items { ... } }):
352-
// the first pass already materialized the projection (extracted egql__ deps / nested Dynamics).
353-
// Use that — rebuilding from the service expression runs the service twice and leaves nested
354-
// selections on the entity type while possibleNextContextTypes still point at the Dynamic.
355-
nextFieldContext = replacementNextFieldContext;
356-
}
357367
else if (HasServices)
358368
{
359369
// if we have services we need to replace any context expressions in the service expression with the new context

‎src/EntityGraphQL/Compiler/GqlNodes/CompileContext.cs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,28 @@ public void AddConstant(IField? fromField, ParameterExpression parameterExpressi
9393

9494
public List<Type>? GetPossibleNextContextTypes(IGraphQLNode node) => possibleNextContextTypes.TryGetValue(node, out var types) ? types : null;
9595

96+
/// <summary>
97+
/// The nodes whose first pass built and materialised their own projection - a root field resolved from
98+
/// services taking part in the two passes so a bulk resolver below it can collect its keys from the rows.
99+
/// The second pass projects from that result instead of rebuilding (and re-running) the service
100+
/// expression, so both passes have to agree on which nodes did it. Stored here rather than on the node as
101+
/// the node belongs to the cached document shared across requests.
102+
/// </summary>
103+
private readonly HashSet<IGraphQLNode> firstPassMaterializedNodes = [];
104+
105+
public void SetFirstPassMaterialized(IGraphQLNode node) => firstPassMaterializedNodes.Add(node);
106+
107+
public bool WasFirstPassMaterialized(IGraphQLNode node) => firstPassMaterializedNodes.Contains(node);
108+
96109
/// <summary>
97110
/// Carry the first pass's state into the context used for the second (services) pass.
98111
/// </summary>
99112
internal void CopyPossibleNextContextTypesFrom(CompileContext other)
100113
{
101114
foreach (var kvp in other.possibleNextContextTypes)
102115
possibleNextContextTypes[kvp.Key] = kvp.Value;
116+
foreach (var node in other.firstPassMaterializedNodes)
117+
firstPassMaterializedNodes.Add(node);
103118
}
104119

105120
/// <summary>

‎src/EntityGraphQL/Compiler/GqlNodes/GraphQLCollectionToSingleField.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ public GraphQLCollectionToSingleField(ISchemaProvider schema, GraphQLListSelecti
6060
CombineExpression = combineExpression;
6161
}
6262

63+
public override bool HasBulkResolverAtOrBelow(IReadOnlyDictionary<string, GraphQLFragmentStatement> fragments)
64+
{
65+
return CollectionSelectionNode.HasBulkResolverAtOrBelow(fragments) || ObjectProjectionNode.HasBulkResolverAtOrBelow(fragments);
66+
}
67+
6368
public override bool HasServicesAtOrBelow(IReadOnlyDictionary<string, GraphQLFragmentStatement> fragments)
6469
{
6570
return CollectionSelectionNode.HasServicesAtOrBelow(fragments) || ObjectProjectionNode.HasServicesAtOrBelow(fragments);

‎src/EntityGraphQL/Compiler/GqlNodes/GraphQLFragmentSpreadField.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,13 @@ public GraphQLFragmentSpreadField(ISchemaProvider schema, string name, Expressio
1616
LocationForDirectives = ExecutableDirectiveLocation.FragmentSpread;
1717
}
1818

19+
public override bool HasBulkResolverAtOrBelow(IReadOnlyDictionary<string, GraphQLFragmentStatement> fragments)
20+
{
21+
// unknown fragment names error later in Expand - do not fail the shape check on them
22+
var fragment = fragments.GetValueOrDefault(Name);
23+
return fragment?.QueryFields.Any(f => f.HasBulkResolverAtOrBelow(fragments)) == true;
24+
}
25+
1926
public override bool HasServicesAtOrBelow(IReadOnlyDictionary<string, GraphQLFragmentStatement> fragments)
2027
{
2128
var fragment = fragments.GetValueOrDefault(Name) ?? throw new EntityGraphQLException(GraphQLErrorCategory.DocumentError, $"Fragment {Name} not found in query document");

‎src/EntityGraphQL/Compiler/GqlNodes/GraphQLListSelectionField.cs‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,19 +76,31 @@ public GraphQLListSelectionField(GraphQLListSelectionField context, ParameterExp
7676
ParameterReplacer replacer
7777
)
7878
{
79-
// Root service fields used to return null here so the statement fell through to a single-pass
80-
// execution (no EF "first pass"). That skipped ResolveBulkLoadersAsync and left BulkParameter
81-
// null, so nested ResolveBulk fields then threw when building the bulk dictionary lookup
82-
// ("Static property requires null instance..." from MakeIndex on a null BulkParameter).
83-
// Fall through instead: execute the service list selecting non-service / extracted key fields,
84-
// run bulk loaders, then the second pass resolves nested bulk fields.
79+
// A root service list returns null here so the statement falls through to a single-pass execution -
80+
// there is no context query for a first pass to run. The exception is a selection with a bulk
81+
// resolver in it: those need a materialised row set to collect their keys from, so the list takes
82+
// part in the two passes instead (run the service selecting non-service / extracted key fields, run
83+
// the bulk loaders, then project the full selection from that result). Doing this unconditionally
84+
// costs an extra compile and a projection of every row for selections with nothing to bulk load.
85+
if (withoutServiceFields && IsRootField && HasServices)
86+
{
87+
if (!HasBulkResolverAtOrBelow(fragments))
88+
return null;
89+
// taking part in the two passes: the second one projects from what this one materializes
90+
compileContext.SetFirstPassMaterialized(this);
91+
}
8592

8693
var listContext = HandleBulkServiceResolver(compileContext, withoutServiceFields, ListExpression)!;
8794

8895
ParameterExpression? nextFieldContext = (ParameterExpression)NextFieldContext!;
8996
if (contextChanged && replacementNextFieldContext != null)
9097
{
91-
listContext = ReplaceContext(replacementNextFieldContext!, replacer, listContext!, possibleNextContextTypes);
98+
// The first pass materialized this field's own projection, so use it. Rebuilding from the service
99+
// expression would run the service a second time and leave nested selections on the entity type
100+
// while possibleNextContextTypes point at the first pass's dynamic type.
101+
listContext = UseFirstPassResult(compileContext, replacementNextFieldContext)
102+
? replacementNextFieldContext
103+
: ReplaceContext(replacementNextFieldContext!, replacer, listContext!, possibleNextContextTypes);
92104
// For async fields (e.g. Task<IEnumerable<T>>), GetEnumerableOrArrayType returns
93105
// IEnumerable<T> (the Task's single type argument) instead of T (the list element type).
94106
// Unwrap Task<>/ValueTask<> first so the element parameter has the correct type.

‎src/EntityGraphQL/Compiler/GqlNodes/GraphQLObjectProjectionField.cs‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,13 @@ ParameterReplacer replacer
7272
// Detect interleaved paging once so we can use it in both passes.
7373
bool isInterleavedPagingField =
7474
Field?.Extensions.Any(e => (e is ConnectionPagingExtension cp && cp.IsInterleavedServiceField) || (e is OffsetPagingExtension op && op.IsInterleavedServiceField)) == true;
75-
if (HasServices && withoutServiceFields && !IsRootField)
75+
// A root service object only takes part in the two passes when the selection has a bulk resolver to
76+
// load (see GraphQLListSelectionField) - otherwise it stays on the single-pass path as before.
77+
var twoPassForBulk = IsRootField && HasServices && !isInterleavedPagingField && HasBulkResolverAtOrBelow(fragments);
78+
if (twoPassForBulk && withoutServiceFields)
79+
// taking part in the two passes: the second one projects from what this one materializes
80+
compileContext.SetFirstPassMaterialized(this);
81+
if (HasServices && withoutServiceFields && !twoPassForBulk)
7682
{
7783
// Detect paging fields where the service is embedded inside the resolver expression
7884
// (e.g. ctx.People.Where(service.Filter).OrderBy()). The service cannot be separated
@@ -94,18 +100,17 @@ ParameterReplacer replacer
94100
replacer
95101
);
96102

103+
if (IsRootField)
104+
// Root interleaved paging: return null so the caller (CompileAndExecuteNodeAsync)
105+
// skips the two-pass path and uses the single-pass fallback, which executes the
106+
// whole field (including the service) in one shot.
107+
return null;
108+
97109
// Non-root interleaved paging: fall through. We build the full selection (edges +
98110
// totalCount etc.) here in the first pass with the service included so that the
99111
// parent's first-pass anon type contains a complete pre-computed value for this field.
100112
// The second pass then reads the pre-computed value via ReplaceContext.GetField().
101113
}
102-
else if (HasServices && withoutServiceFields && IsRootField && isInterleavedPagingField)
103-
{
104-
// Root interleaved paging: return null so the caller (CompileAndExecuteNodeAsync)
105-
// skips the two-pass path and uses the single-pass fallback, which executes the
106-
// whole field (including the service) in one shot.
107-
return null;
108-
}
109114

110115
// Root service object fields (e.g. statusPage { items { bulkField } }) used to return only
111116
// ExtractedFieldsFromServices here, which skipped nested list selection and left ResolveBulk
@@ -115,7 +120,9 @@ ParameterReplacer replacer
115120

116121
if (contextChanged && replacementNextFieldContext != null)
117122
{
118-
nextFieldContext = ReplaceContext(replacementNextFieldContext!, replacer, nextFieldContext!, possibleNextContextTypes);
123+
nextFieldContext = UseFirstPassResult(compileContext, replacementNextFieldContext)
124+
? replacementNextFieldContext
125+
: ReplaceContext(replacementNextFieldContext!, replacer, nextFieldContext!, possibleNextContextTypes);
119126
}
120127
(nextFieldContext, var argumentParam) =
121128
Field?.GetExpression(

‎src/tests/EntityGraphQL.Tests/QueryTests/ServiceRootListBulkTests.cs‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.Collections.Generic;
33
using System.Linq;
4+
using System.Linq.Expressions;
45
using EntityGraphQL.Schema;
56
using Microsoft.Extensions.DependencyInjection;
67
using Xunit;
@@ -516,4 +517,96 @@ public void ServiceResolvedPage_ItemsWithComplexResolveBulkKey_Works()
516517
Assert.Equal("Alice", page.items[0].createdByName);
517518
Assert.Equal("Bob", page.items[1].createdByName);
518519
}
520+
/// <summary>
521+
/// The two-pass flow exists so a bulk resolver can collect its keys from the materialized rows. A
522+
/// selection with nothing to bulk load stays on the single-pass path - the extra pass would be an extra
523+
/// compile and a projection of every row for no gain.
524+
/// </summary>
525+
[Fact]
526+
public void ServiceRootList_WithoutBulkFieldSelected_RunsInOnePass()
527+
{
528+
var (schema, sp, _, names, _) = BuildApiKeysSchema(apiKey =>
529+
{
530+
apiKey
531+
.AddField("createdByName", "Creator display name")
532+
.Resolve<CreatedByNameService>((k, svc) => svc.GetName(k.CreatedById))
533+
.ResolveBulk<CreatedByNameService, string, string?>(k => k.CreatedById!, (ids, svc) => svc.GetNames(ids));
534+
});
535+
536+
var passes = ExecuteCountingPasses(schema, sp, "{ apiKeys { key customerId } }");
537+
538+
Assert.Equal([true], passes);
539+
Assert.Equal(0, names.CallCount);
540+
}
541+
542+
/// <summary>
543+
/// Selecting the bulk field opts the root service list into the two passes - one load for all rows.
544+
/// </summary>
545+
[Fact]
546+
public void ServiceRootList_WithBulkFieldSelected_RunsTwoPasses()
547+
{
548+
var (schema, sp, _, names, _) = BuildApiKeysSchema(apiKey =>
549+
{
550+
apiKey
551+
.AddField("createdByName", "Creator display name")
552+
.Resolve<CreatedByNameService>((k, svc) => svc.GetName(k.CreatedById))
553+
.ResolveBulk<CreatedByNameService, string, string?>(k => k.CreatedById!, (ids, svc) => svc.GetNames(ids));
554+
});
555+
556+
var passes = ExecuteCountingPasses(schema, sp, "{ apiKeys { key createdByName } }");
557+
558+
Assert.Equal([false, true], passes);
559+
Assert.Equal(1, names.CallCount);
560+
}
561+
562+
/// <summary>
563+
/// The bulk field reached through a fragment spread counts too - clients hoist selections into fragments,
564+
/// and a shape check that did not look inside them would quietly drop back to one call per row.
565+
/// </summary>
566+
[Fact]
567+
public void ServiceRootList_BulkFieldInFragment_StillLoadsOnce()
568+
{
569+
var (schema, sp, _, names, _) = BuildApiKeysSchema(apiKey =>
570+
{
571+
apiKey
572+
.AddField("createdByName", "Creator display name")
573+
.Resolve<CreatedByNameService>((k, svc) => svc.GetName(k.CreatedById))
574+
.ResolveBulk<CreatedByNameService, string, string?>(k => k.CreatedById!, (ids, svc) => svc.GetNames(ids));
575+
});
576+
577+
var res = schema.ExecuteRequest(
578+
new QueryRequest { Query = "query { apiKeys { ...KeyFields } } fragment KeyFields on ApiKey { key createdByName }" },
579+
sp,
580+
null
581+
);
582+
583+
if (res.Errors != null)
584+
Assert.Fail(string.Join(" | ", res.Errors.Select(e => e.Message)));
585+
Assert.Equal(1, names.CallCount);
586+
dynamic keys = res.Data!["apiKeys"]!;
587+
Assert.Equal("Alice", keys[0].createdByName);
588+
}
589+
590+
/// <summary>The isFinal flag of each execution the engine runs for a query.</summary>
591+
private static List<bool> ExecuteCountingPasses(SchemaProvider<EmptyContext> schema, ServiceProvider sp, string query)
592+
{
593+
List<bool> passes = [];
594+
var res = schema.ExecuteRequest(
595+
new QueryRequest { Query = query },
596+
sp,
597+
null,
598+
new ExecutionOptions
599+
{
600+
BeforeExecuting = (Expression expression, bool isFinal) =>
601+
{
602+
passes.Add(isFinal);
603+
return expression;
604+
},
605+
}
606+
);
607+
if (res.Errors != null)
608+
Assert.Fail(string.Join(" | ", res.Errors.Select(e => e.Message)));
609+
return passes;
610+
}
611+
519612
}

0 commit comments

Comments
 (0)