feat: implement multi fetch to the same subgraph - #1594
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMultiFetch adds planner configuration and postprocessing for merging same-wave federation entity fetches. It records structured operations, builds aliased requests, executes shared requests, and maps results and errors back to individual response paths. ChangesFederation MultiFetch
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionEngine
participant GraphQLPlanner
participant Postprocessor
participant Subgraph
participant MultiEntityLoader
ExecutionEngine->>GraphQLPlanner: EnableMultiFetch configuration
GraphQLPlanner->>Postprocessor: Record entity-fetch artifacts
Postprocessor->>Postprocessor: Merge compatible same-wave fetches
Postprocessor->>Subgraph: Send aliased merged request
Subgraph-->>MultiEntityLoader: Return aliased entities and errors
MultiEntityLoader-->>ExecutionEngine: Fan out results to response paths
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
9dbfbdd to
a5f4e7a
Compare
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_multi_fetch_test.go (1)
895-915: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
pathparameter fromemployeeProducts.The closure ignores
path. The value always resolves[]string{"products"}. The call sites at Line 936 and Line 956 pass different values, which suggests a difference that does not exist.♻️ Proposed cleanup
- employeeProducts := func(path string) *resolve.Field { + employeeProducts := func() *resolve.Field { return &resolve.Field{ Name: []byte("products"),Then update both call sites to
employeeProducts().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_multi_fetch_test.go` around lines 895 - 915, Remove the unused path parameter from the employeeProducts closure in multiFetchEmployeeProductsData, then update both call sites to invoke employeeProducts() without arguments. Preserve the existing fixed products path and field configuration.v2/pkg/engine/postprocess/render_subgraph_inputs_test.go (1)
76-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for
disableRewriteOpNames.Every subtest constructs
&renderSubgraphInputs{}, sodisableRewriteOpNamesis always false and the suffix branch always runs. No test pins the behaviour when the flag is true, where the operation name must stay unsuffixed in both the input and the query plan.💚 Proposed test case
+ t.Run("suffix is skipped when rewriting operation names is disabled", func(t *testing.T) { + const namedSource = `query MyOp($representations: [_Any!]!){_entities(representations: $representations){__typename}}` + node := renderInputFetch(t, 3, "MyOp", namedSource, resolve.SubgraphRequestEnvelope{Method: "POST", URL: "http://x"}, fragments, nil) + (&renderSubgraphInputs{disableRewriteOpNames: true}).ProcessFetchTree(resolve.Sequence(node)) + require.Contains(t, renderedInput(t, node), `query MyOp(`) + require.NotContains(t, renderedInput(t, node), `MyOp__3`) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/postprocess/render_subgraph_inputs_test.go` around lines 76 - 84, Add a subtest in the operation-name suffix coverage that constructs renderSubgraphInputs with disableRewriteOpNames enabled, then assert the rendered input and query-plan operation name remain unsuffixed. Preserve the existing suffixed behavior test for the default false setting.v2/pkg/asttransform/merge_operations_test.go (1)
22-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case with an incomplete
VariableRenamemap.
mergeMemberalways builds a total mapping from the document's own variable definitions. No test therefore reaches the miss path inmergeVariableDefinitions, where a missing key produces an empty variable name. Add a case that omits one variable so the chosen behaviour is pinned.This relates to the defect raised on
v2/pkg/asttransform/merge_operations.goline 68.💚 Proposed test case
+ t.Run("incomplete variable rename is rejected", func(t *testing.T) { + member := mergeMember(t, m1, "_f1", "f1", "includeF1") + delete(member.VariableRename, "first") + _, err := MergeOperationDocuments("", []OperationMergeMember{member}) + require.Error(t, err) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/asttransform/merge_operations_test.go` around lines 22 - 31, Add a merge-operation test case that constructs an OperationMergeMember with a deliberately incomplete VariableRename map, omitting one variable defined in the document, so mergeVariableDefinitions exercises its missing-key path and pins the expected empty-name behavior. Keep the existing mergeMember helper unchanged for total mappings and target the new case at the merge operation test flow.v2/pkg/asttransform/merge_operations.go (1)
35-59: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate that member aliases, include variables and renamed variable names are unique.
MergeOperationDocumentsis exported and trusts the caller for uniqueness. Two members with the sameAliasproduce two identically aliased root fields. A consumer that demultiplexes the response by alias then collapses both entries and loses one member's data. Two members whoseVariableRenamemaps yield the same target name produce a duplicate variable definition, which the upstream subgraph rejects.The current caller derives
"f"+strconv.Itoa(i+1)and"includeF"+strconv.Itoa(i+1), so it is safe today. A cheap check keeps the invariant enforced at the API boundary.🛡️ Proposed uniqueness check
importer := &astimport.Importer{} nonNullBool := merged.AddNonNullNamedType([]byte("Boolean")) + seenAliases := make(map[string]struct{}, len(members)) + seenVariables := make(map[string]struct{}, len(members)) for i, member := range members { if member.Document == nil { return nil, fmt.Errorf("asttransform: member %d has no document", i+1) } + if _, ok := seenAliases[member.Alias]; ok { + return nil, fmt.Errorf("asttransform: duplicate member alias %q", member.Alias) + } + seenAliases[member.Alias] = struct{}{} + if _, ok := seenVariables[member.IncludeVariable]; ok { + return nil, fmt.Errorf("asttransform: duplicate include variable %q", member.IncludeVariable) + } + seenVariables[member.IncludeVariable] = struct{}{} + for _, renamed := range member.VariableRename { + if _, ok := seenVariables[renamed]; ok { + return nil, fmt.Errorf("asttransform: duplicate merged variable %q", renamed) + } + seenVariables[renamed] = struct{}{} + } mergeVariableDefinitions(merged, opRef, importer, member, nonNullBool)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/asttransform/merge_operations.go` around lines 35 - 59, Update MergeOperationDocuments to validate member uniqueness before merging: reject duplicate member Alias values, duplicate include-variable names, and duplicate target names produced by VariableRename mappings. Track each category independently, return a descriptive error identifying the conflicting name, and only proceed to mergeVariableDefinitions and mergeMemberSelection after validation succeeds.v2/pkg/engine/postprocess/create_multi_fetch_test.go (1)
478-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a member document with no operation definition.
TestBuildMergedOperationcovers a wrong root field. It does not cover a document that parses to zero operation definitions. That input currently panics invalidateEntitiesRootField(see the comment oncreate_multi_fetch_document.goLines 75-84). Add the case together with the bounds guard so the abort path is locked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/postprocess/create_multi_fetch_test.go` around lines 478 - 495, The TestBuildMergedOperation cases should include a member whose SubgraphOperation document has no operation definition, and assert buildMergedOperation returns an error without panicking. Add the corresponding bounds guard in validateEntitiesRootField before accessing an operation definition, preserving the existing invalid-root-field error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@execution/engine/execution_engine_multi_fetch_test.go`:
- Around line 48-63: The testRoundTripper callback must not call require.NotNil
or require.NoError because it runs on resolver fetch goroutines. Replace those
fatal assertions with non-fatal validation and return an HTTP error response
when the request body is missing or cannot be read, keeping failures reportable
on the test goroutine.
In `@v2/pkg/astimport/astimport.go`:
- Around line 243-266: Update ImportVariableDefinitionWithVariableNameRename to
import and preserve the source variable definition’s Directives, applying the
variable-rename map to each directive before attaching them to
variableDefinition. Ensure MergeOperationDocuments retains VARIABLE_DEFINITION
directives in the merged operation.
In `@v2/pkg/asttransform/merge_operations.go`:
- Around line 64-73: In v2/pkg/asttransform/merge_operations.go lines 64-73,
update mergeVariableDefinitions to use a two-value lookup for
member.VariableRename[origName], return an error when the entry is missing or
empty, and propagate that error through MergeOperationDocuments. In
v2/pkg/asttransform/merge_operations_test.go lines 22-31, add a test that
removes one entry from the map produced by mergeMember and verifies
MergeOperationDocuments returns an error.
In `@v2/pkg/engine/postprocess/create_multi_fetch_document.go`:
- Around line 75-84: The validateEntitiesRootField function must reject
documents with no operation definition or an invalid root SelectionSet reference
before indexing, returning descriptive errors for each case; update
v2/pkg/engine/postprocess/create_multi_fetch_document.go lines 75-84
accordingly. Add a TestBuildMergedOperation case in
v2/pkg/engine/postprocess/create_multi_fetch_test.go lines 478-495 using a
member document without an operation definition and assert that
buildMergedOperation returns an error.
In `@v2/pkg/engine/postprocess/render_subgraph_inputs.go`:
- Around line 76-89: Make both `sjson.SetRawBytes` and `op.PrintedQuery()`
failures observable in the render flow instead of returning with an empty
request. Check and record each error on the plan, or attach a non-empty error
marker that `processSingleFetch` preserves, so `fetch.SubgraphOperation` is not
silently replaced by an unrecoverable empty `Input`. Keep successful variable
assembly and query rendering unchanged.
---
Nitpick comments:
In `@v2/pkg/asttransform/merge_operations_test.go`:
- Around line 22-31: Add a merge-operation test case that constructs an
OperationMergeMember with a deliberately incomplete VariableRename map, omitting
one variable defined in the document, so mergeVariableDefinitions exercises its
missing-key path and pins the expected empty-name behavior. Keep the existing
mergeMember helper unchanged for total mappings and target the new case at the
merge operation test flow.
In `@v2/pkg/asttransform/merge_operations.go`:
- Around line 35-59: Update MergeOperationDocuments to validate member
uniqueness before merging: reject duplicate member Alias values, duplicate
include-variable names, and duplicate target names produced by VariableRename
mappings. Track each category independently, return a descriptive error
identifying the conflicting name, and only proceed to mergeVariableDefinitions
and mergeMemberSelection after validation succeeds.
In
`@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_multi_fetch_test.go`:
- Around line 895-915: Remove the unused path parameter from the
employeeProducts closure in multiFetchEmployeeProductsData, then update both
call sites to invoke employeeProducts() without arguments. Preserve the existing
fixed products path and field configuration.
In `@v2/pkg/engine/postprocess/create_multi_fetch_test.go`:
- Around line 478-495: The TestBuildMergedOperation cases should include a
member whose SubgraphOperation document has no operation definition, and assert
buildMergedOperation returns an error without panicking. Add the corresponding
bounds guard in validateEntitiesRootField before accessing an operation
definition, preserving the existing invalid-root-field error behavior.
In `@v2/pkg/engine/postprocess/render_subgraph_inputs_test.go`:
- Around line 76-84: Add a subtest in the operation-name suffix coverage that
constructs renderSubgraphInputs with disableRewriteOpNames enabled, then assert
the rendered input and query-plan operation name remain unsuffixed. Preserve the
existing suffixed behavior test for the default false setting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86bf69d8-64f4-47ae-b44d-07da8c1b3ae2
📒 Files selected for processing (36)
execution/engine/engine_config.goexecution/engine/execution_engine.goexecution/engine/execution_engine_multi_fetch_test.gov2/pkg/astimport/astimport.gov2/pkg/astimport/astimport_test.gov2/pkg/asttransform/merge_operations.gov2/pkg/asttransform/merge_operations_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_multi_fetch_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.gov2/pkg/engine/datasource/httpclient/httpclient_test.gov2/pkg/engine/datasource/httpclient/input_assembly.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/datasource_configuration.gov2/pkg/engine/plan/multihop_compound_key_test.gov2/pkg/engine/plan/planner_configuration.gov2/pkg/engine/postprocess/create_multi_fetch.gov2/pkg/engine/postprocess/create_multi_fetch_document.gov2/pkg/engine/postprocess/create_multi_fetch_test.gov2/pkg/engine/postprocess/deduplicate_single_fetches.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/postprocess/postprocess_test.gov2/pkg/engine/postprocess/render_subgraph_inputs.gov2/pkg/engine/postprocess/render_subgraph_inputs_test.gov2/pkg/engine/postprocess/resolve_input_templates.gov2/pkg/engine/resolve/fetch.gov2/pkg/engine/resolve/fetch_multi.gov2/pkg/engine/resolve/fetch_test.gov2/pkg/engine/resolve/fetchtree.gov2/pkg/engine/resolve/fetchtree_test.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_multi_entity.gov2/pkg/engine/resolve/loader_multi_entity_test.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/tainted_objects.gov2/pkg/engine/resolve/tainted_objects_test.go
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
43284b7 to
227e3b7
Compare
Detailed spec for merging same-subgraph entity fetches into one aliased _entities request: postprocess createMultiFetch stage, resolve.MultiEntityFetch, astimport cross-document selection import with variable renaming, loader prepare/merge, ART tracing and query plans. Includes user Q&A decisions and the corrections from an adversarial verification pass against the codebase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Key corrections: dual sjson input shapes (v1.0.4 prepend in-repo vs append downstream), order-agnostic input scanner, public-API-only document merge, record-time representations-collision guard, error-preserving variable recording helper, expanded test coverage per review findings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shape-neutral envelope remainder, survivor-ID rewrite for all member IDs, satisfiable per-DeferID fixtures, per-entry out copying, all-excluded early return before rate limiting, hardened scanner rules, verified test-harness instructions, printer-accurate directive spacing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unique-match fail-safe for the append-shape query anchor, dedupe-proof test fixtures, excluded-entry carve-out in error fan-out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…equest (MultiFetch) Opt-in via plan.Configuration.EnableMultiFetch + postprocess.EnableMultiFetch(). The graphql datasource records the normalized upstream operation document and its body.variables fragments on the fetch configuration; a new postprocess stage groups same-wave entity fetches per subgraph and defer scope, merges their documents through new astimport cross-document selection-set import with variable renaming, and emits a resolve.MultiEntityFetch whose aliased _entities fields are guarded by @include(if: $includeFN) booleans. The loader renders per-entry representations with per-entry dedup and authorization, fires one request, and demuxes the response per alias through the existing mergeResult machinery with per-entry error attribution. Spec: docs/multi-fetch/spec.md. Plan: docs/multi-fetch/plan.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fetch backpointer Entry items no longer reference the replaced member fetch (kept the merged-away SingleFetch and its MergeableOperation document alive in the cached plan, and a multi backpointer would make plans cyclic, breaking structural comparison). Deduplicate the merged DependsOnFetchIDs union and strip empty static segments from entry representations templates. Rewrite the datasource multi-fetch tests onto datasourcetesting.RunTest with full expected-plan equality. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Level-by-level walkthrough (planning, postprocessing, runtime, observability) with file and test maps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The planning and implementation notes duplicate the code and would become stale as the implementation evolves.
The envelope is now constructed explicitly in the documented order
{method,url,header,body:{query,variables}} — byte-identical to the previous
pinned output, independent of the sjson version.
227e3b7 to
0dd960e
Compare
🤖 I have created a release *beep* *boop* --- ## [2.16.0](v2.15.1...v2.16.0) (2026-08-13) ### Features * implement multi fetch to the same subgraph ([#1594](#1594)) ([22584e2](22584e2)) * schedule fetch trees optimally ([#1612](#1612)) ([bd03deb](bd03deb)) ### Bug Fixes * check interface implementation in `potentiallySameObject` for nullability relaxation ([#1454](#1454)) ([5bacb9e](5bacb9e)) * improve handling of nullable lists for required fields ([#1631](#1631)) ([0af4dd3](0af4dd3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
Adds an opt-in MultiFetch optimization: entity fetches that
target the same subgraph and execute in the same parallel wave
are merged into a single upstream request,
cutting the number of subgraph round-trips.
How it works
Planner: entity fetches record their normalized upstream operation,
variables, and request envelope as merge material.
Postprocess: the new createMultiFetch stage groups same-datasource entity fetches
within each parallel wave and merges them into one MultiEntityFetch.
Member operations are combined via AST merging with collision-safe variable renaming;
each member becomes an aliased _entities field (f1, f2, …) guarded by @include(if: $includeFN).
Loader: a merged fetch renders per-entry representations, sends one request,
then demuxes the response by alias; per-entry data merging, error partitioning and path rewriting.
Entries excluded at prepare time (denied, no items) ship includeFN:false and
an empty representations array, so variable coercion still passes upstream.
Defaults
Off by default at both gates: the postprocess stage is the pipeline's only opt-in stage,
and without the planner flag no merge material is recorded.
Co-authored-by: Claude Fable 5 noreply@anthropic.com
Co-authored-by: Alberto Garcia Hierro alberto@wundergraph.com
Co-authored-by: Yury Smolski 140245+ysmolski@users.noreply.github.com