feat: add support of field selection reasons extensions#1282
feat: add support of field selection reasons extensions#1282
Conversation
This option of Resolver enables router to send to subgraphs an extension that includes reasons why fields where included in the request. The name of fields in the extension are open for discussion.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughPropagates per-field "fetch reasons" through planner and resolve layers, indexes fetch-reason fields in data-source metadata, computes inverse field-ref dependants, surfaces per-fetch reasons on concrete fetch types and public APIs, and optionally injects Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/visitor.go (1)
1398-1461: Bug: user-requested fields can be dropped when requestedBy exists but resolves to no subgraphsIf fieldRefRequestedBy[fieldRef] exists but yields no BySubgraphs (e.g., nil ObjectFetchConfiguration or unmapped planners), the function currently doesn’t append the record—even when ByUser is true. That loses attribution for genuinely user-requested fields.
Fix: append when requested.ByUser OR there are any BySubgraphs.
Also, avoid shadowing the fetchID parameter inside the inner loop and defensively handle missing TypeName.
for _, fieldRef := range fields { fieldName := v.Operation.FieldNameString(fieldRef) if fieldName == "__typename" { continue } requested := resolve.RequestedField{ - TypeName: v.fieldEnclosingTypeNames[fieldRef], + TypeName: v.fieldEnclosingTypeNames[fieldRef], FieldName: fieldName, } requested.ByUser = !v.skipField(fieldRef) requestedByRefs, ok := v.fieldRefRequestedBy[fieldRef] if !ok { if requested.ByUser { result = append(result, requested) } continue } requested.BySubgraphs = make([]string, 0, len(requestedByRefs)) for _, reqByRef := range requestedByRefs { - depFieldPlanners, ok := v.fieldPlanners[reqByRef] + depFieldPlanners, ok := v.fieldPlanners[reqByRef] if !ok { continue } // find the subgraph's name by iterating over planners that are responsible for reqByRef - for _, fetchID := range depFieldPlanners { - ofc := v.planners[fetchID].ObjectFetchConfiguration() + for _, plannerID := range depFieldPlanners { + ofc := v.planners[plannerID].ObjectFetchConfiguration() if ofc == nil { continue } requested.BySubgraphs = append(requested.BySubgraphs, ofc.sourceName) - depKind, ok := v.fieldDependencyKind[fieldDependencyKey{field: reqByRef, dependsOn: fieldRef}] + depKind, ok := v.fieldDependencyKind[fieldDependencyKey{field: reqByRef, dependsOn: fieldRef}] if !ok { continue } switch depKind { case fieldDependencyKindKey: requested.ReasonIsKey = true case fieldDependencyKindRequires: requested.ReasonIsRequires = true } } } - if len(requested.BySubgraphs) > 0 { + if requested.ByUser || len(requested.BySubgraphs) > 0 { slices.Sort(requested.BySubgraphs) requested.BySubgraphs = slices.Compact(requested.BySubgraphs) result = append(result, requested) } }Optional: if TypeName might be missing (shouldn’t, but defensive), consider skipping such entries or defaulting with a clear sentinel; otherwise downstream JSON may contain empty typeName.
🧹 Nitpick comments (15)
execution/engine/config_factory_federation.go (1)
309-314: Stale commented code references undefined symbol; clarify intent or remove.The commented-out
FetchURLRulesline referencesrouterEngineConfig.Headerswhich isn't in scope here (likely meantrouterConfig.EngineConfig.Headers). If you plan to re-enable this soon, fix the symbol and keep a precise TODO; otherwise, delete the block to avoid confusion.Apply this minimal cleanup to keep the comment accurate until the code is enabled:
- // dataSourceRules := FetchURLRules(&routerEngineConfig.Headers, routerConfig.BySubgraphs, subscriptionUrl) + // TODO(eng-7769): enable once header rules are wired. Likely: + // dataSourceRules := FetchURLRules(&routerConfig.EngineConfig.Headers, routerConfig.BySubgraphs, subscriptionUrl)v2/pkg/engine/plan/configuration.go (1)
30-35: Go doc comments should start with the field name.To keep golint/godoc happy and improve clarity, start doc comments with the exported field names and tighten phrasing.
Apply:
- // Tests can suppress generating resolve.FieldInfo in the planner. - DisableIncludeInfo bool + // DisableIncludeInfo controls whether the planner generates resolve.FieldInfo (useful in tests). + DisableIncludeInfo bool - // Test can suppress generating field dependencies structures in the planner. - DisableIncludeFieldDependencies bool + // DisableIncludeFieldDependencies controls whether the planner generates field dependency structures (useful in tests). + DisableIncludeFieldDependencies boolv2/pkg/engine/postprocess/create_concrete_single_fetch_types.go (1)
77-112: Propagating FieldsRequestedBy to BatchEntityFetch looks correct; confirm immutability.You pass through
fetch.FieldsRequestedByas-is. If the source slice is treated as immutable after planning (expected), this is fine. If anything mutates it later, consider cloning to avoid aliasing.Would you like a follow-up patch that defensively clones this slice during post-processing?
v2/pkg/engine/resolve/loader.go (2)
1565-1581: Skip propagation for introspection and keep extensions collision-safe.Two small improvements:
- Don’t attach
fieldsRequestedByto introspection requests; it’s noise and could confuse tooling.- You already assume
body.extensionsis an object;jsonparser.Setwill error if it’s incompatible. Current error handling is fine, but tightening the condition keeps input shape predictable.Here’s a focused tweak to guard introspection without changing behavior elsewhere.
Apply:
- if l.propagateFieldsRequestedBy { + if l.propagateFieldsRequestedBy && !IsIntrospectionDataSource(res.ds.ID) { fieldsRequestedBy := fetchItem.Fetch.RequestedFields() if fieldsRequestedBy != nil { // We expect that body.extensions is an object var encoded []byte encoded, res.err = json.Marshal(fieldsRequestedBy) if res.err != nil { res.err = errors.WithStack(res.err) return } input, res.err = jsonparser.Set(input, encoded, "body", "extensions", "fieldsRequestedBy") if res.err != nil { res.err = errors.WithStack(res.err) return } } }
1565-1581: Consider namespacing the extension key to prevent collisions.If subgraphs or clients use a similarly named field, collisions are possible. Consider a vendor-prefixed key like
wgFieldsRequestedByor a namespaced objectextensions.wg.fieldsRequestedBy.v2/pkg/engine/plan/planner.go (1)
231-239: Make inverseMap deterministic to avoid flaky tests.Map iteration order is random; sorting the resulting slices gives stable outputs for debugging/tests.
Apply:
func inverseMap(m map[int][]int) map[int][]int { inverse := make(map[int][]int) for k, v := range m { for _, v2 := range v { inverse[v2] = append(inverse[v2], k) } } - return inverse + // Normalize ordering for deterministic plans/tests + for key := range inverse { + sort.Ints(inverse[key]) + } + return inverse }And add the import:
// at top of file import ( "fmt" "strings" "sort" ... )execution/engine/execution_engine_test.go (4)
218-219: Add a helper option for symmetry and reuse (nit).You added propagateFieldsRequestedBy to the internal test options. To keep the test harness consistent with withValueCompletion() and reduce inline closures, add a small helper:
func withPropagateFieldsRequestedBy() executionTestOptions { return func(options *_executionTestOptions) { options.propagateFieldsRequestedBy = true } }
827-876: Stability of JSON body assertion for fieldsRequestedBy.The test asserts a byte-for-byte body string including the extensions.fieldsRequestedBy array. If the upstream computation ever changes the order in that slice, this becomes flaky. Consider making the round-tripper support JSON assertions (e.g., decode and compare specific fields ignoring array element order) or build the expected body via json.Marshal on a struct to guarantee stable key ordering.
Apply this diff to reuse a helper option (from my other comment) and avoid the inline closure:
- func(eto *_executionTestOptions) { - eto.propagateFieldsRequestedBy = true - }, + withPropagateFieldsRequestedBy(),
4347-4470: Reduce duplication in fieldsRequestedBy and centralize expected body construction.Two separate entries for the same coordinate (User.id) carry different reasons (requestedBySubgraphs+reasonIsKey vs requestedByUser). Consider deduplicating per (typeName, fieldName) and merging reasons into a single object before emitting. This will shrink payload size and simplify consumption by subgraphs.
Additionally, the long raw JSON strings are brittle. A small local builder that marshals:
- query string
- optional variables
- extensions.fieldsRequestedBy (slice of structs)
would improve maintainability and cut quoting mistakes. Because tests elsewhere already assert exact JSON bodies, using structs (not maps) will keep key order deterministic.
Example local type to drive json.Marshal deterministically:
type expectedBody struct { Query string `json:"query"` Variables any `json:"variables,omitempty"` Extensions struct { FieldsRequestedBy []struct { TypeName string `json:"typeName"` FieldName string `json:"fieldName"` RequestedByUser bool `json:"requestedByUser,omitempty"` RequestedBySubgraphs []string `json:"requestedBySubgraphs,omitempty"` ReasonIsKey bool `json:"reasonIsKey,omitempty"` } `json:"fieldsRequestedBy,omitempty"` } `json:"extensions,omitempty"` }
4537-4539: Use the helper option instead of an inline closure (nit).Same suggestion as above for consistency.
- func(eto *_executionTestOptions) { - eto.propagateFieldsRequestedBy = true - }, + withPropagateFieldsRequestedBy(),v2/pkg/engine/plan/visitor.go (2)
49-49: New inverse map is fine, but document/guarantee producer semanticsAdding fieldRefRequestedBy (inverse of fieldRefDependsOnFieldRefs) is appropriate. Please ensure its population (likely in planner.go) is always done before configureFetch runs, and consider adding a brief doc on whether refs included here are only direct dependents or also transitive. This affects expectations for buildFieldsRequestedBy.
636-646: Helper extraction improves clarity; consider naming and fast-pathskipField helper is clear and correctly preserves previous semantics. Minor: if skipFieldsRefs can grow, consider a small set/map for O(1) checks, or keep as-is if the list is tiny.
- for _, skipRef := range v.skipFieldsRefs { - if skipRef == ref { return true } - } + // If this grows, switch to map[int]struct{} for O(1). + for _, skipRef := range v.skipFieldsRefs { + if skipRef == ref { return true } + }v2/pkg/engine/resolve/fetch.go (2)
416-427: Tighten docstring grammar and naming consistencyMinor grammar nits and align wording with the JSON tags (“requestedBySubgraphs”, “requestedByUser”).
-// RequestedField contains an answer about who requested a specific TypeName, FieldName combination. -// It can be requested by set Subgraphs and by a User. +// RequestedField explains who requested a specific (typeName, fieldName) combination. +// A field can be requested by the user and/or by one or more subgraphs, with optional reasons.
350-354: Surface looks good; be explicit about equality semanticsFetchConfiguration now carries FieldsRequestedBy. Note that Equals() does not compare this field (nor CoordinateDependencies). If tests or tooling need to assert exact propagation of requested fields, rely on explicit checks rather than Equals(). Consider adding a comment near Equals() to make this intentional.
func (fc *FetchConfiguration) Equals(other *FetchConfiguration) bool { + // Note: FieldsRequestedBy and CoordinateDependencies are intentionally excluded from equality. + // They carry diagnostic/metadata that doesn't affect fetch identity.v2/pkg/engine/resolve/resolve.go (1)
168-171: Option name and docs LGTM; clarify exposure scopePropagateFieldsRequestedBy is clear. Consider clarifying in the comment that this only propagates to upstream subgraphs (extensions) and is not exposed to clients unless separately surfaced, to avoid confusion about information leakage.
-// PropagateFieldsRequestedBy enables sending to upstream subgraphs the "fieldsRequestedBy" -// extension that contains information why each field was requested. +// PropagateFieldsRequestedBy enables sending to upstream subgraphs (only) the "fieldsRequestedBy" +// extension that explains why each field was requested. This flag does not expose the data to clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
execution/engine/config_factory_federation.go(1 hunks)execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(4 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/planner.go(2 hunks)v2/pkg/engine/plan/visitor.go(5 hunks)v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go(2 hunks)v2/pkg/engine/resolve/fetch.go(11 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (1)
Fetch(20-26)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(419-426)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(419-426)
v2/pkg/engine/resolve/resolve.go (1)
v2/pkg/engine/resolve/resolvable.go (1)
ResolvableOptions(62-67)
execution/engine/execution_engine_test.go (4)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceMetadata(34-56)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-354)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (17)
v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go (1)
124-147: Same propagation for EntityFetch; mirror the immutability consideration.Same note as above; the propagation is correct. Just ensure upstream treats
FieldsRequestedByas read-only post plan.v2/pkg/engine/resolve/loader.go (1)
152-155: New Loader flag is well-scoped.Adding
propagateFieldsRequestedBybehind a boolean is the right safety valve. Ensure it’s plumbed fromResolverOptions.PropagateFieldsRequestedByinnewToolsso tests can toggle it.v2/pkg/engine/plan/planner.go (1)
156-157: Wiring inverse dependencies is a good call; watch for nondeterministic order.Deriving
fieldRefRequestedByfromfieldRefDependsOnis correct. Note that map iteration in Go is randomized; if ordering matters downstream (tests, deterministic plans), ensure consumers sort.execution/engine/execution_engine_test.go (2)
4505-4541: Federation scenario with fieldsRequestedBy propagation — good coverage.This mirrors the non-extension path, verifies both subgraphs see the extension, and exercises key-based entity fetch reasoning. Nicely scoped and readable.
266-267: All good: PropagateFieldsRequestedBy gating confirmedA ripgrep search shows that:
PropagateFieldsRequestedByis only added toResolverOptionsinresolve.goand correctly passed through to the engine.- In
loader.go, the"fieldsRequestedBy"extension is set on the request only within theif l.propagateFieldsRequestedBy { … }block, with no occurrences elsewhere.No further action needed.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (4)
3183-3201: LGTM: requires for country/city are accurately attributed.Marking Address.country and Address.city as requires from address.service, while preserving Account.address as user-requested, matches the dependency chain. No issues spotted.
3295-3313: LGTM: downstream requires on line3/zip are precise.Both fields are correctly attributed to account.service via requires, and the enclosing Account.address remains user-requested. This aligns with the later fullAddress resolution.
3420-3431: LGTM: user-requested fullAddress attribution is clear.Capturing Address.fullAddress and Account.address as ByUser in the final fetch is appropriate and keeps the provenance readable.
3097-3135: No helper needed: BySubgraphs is already deterministically sortedThe test’s expected slice literal (
[]string{"account.service", "address-enricher.service", "address.service"}) already matches the alphabetical order enforced in the production code, so there’s no risk of flakiness here.• In
v2/pkg/engine/plan/visitor.go(lines 1453–1454), the code callsslices.Sort(requested.BySubgraphs) requested.BySubgraphs = slices.Compact(requested.BySubgraphs)which guarantees that
BySubgraphsis sorted and deduplicated before being returned.You can safely leave the test as-is.
v2/pkg/engine/plan/visitor.go (2)
58-66: Good: tracking maps for “who planned what” and enclosing typesplannerFields, fieldPlanners, and fieldEnclosingTypeNames are the right primitives for later attribution. Nice that they’re initialized in EnterDocument.
1335-1338: Plumbing LGTM; relies on promoted fields via embeddingAssigning CoordinateDependencies and FieldsRequestedBy on singleFetch works via field promotion (FetchConfiguration is embedded). Gated by DisableIncludeFieldDependencies, which is consistent with where the underlying maps are populated.
v2/pkg/engine/resolve/fetch.go (5)
25-26: Interface extension is coherentAdding RequestedFields() to Fetch cleanly exposes the new metadata without leaking struct details.
108-111: SingleFetch delegation is correctReturning FetchConfiguration.FieldsRequestedBy via promotion is appropriate and keeps SingleFetch lean.
180-181: BatchEntityFetch wiring is consistentStoring FieldsRequestedBy directly on BatchEntityFetch and exposing via RequestedFields() matches the post-processing pattern used elsewhere.
Please confirm create_concrete_single_fetch_types.go copies FetchConfiguration.FieldsRequestedBy from SingleFetch to BatchEntityFetch during post-processing.
Also applies to: 191-194
232-234: EntityFetch wiring is consistentCoordinateDependencies and FieldsRequestedBy are exposed consistently.
Double-check post-processing populates EntityFetch.FieldsRequestedBy and preserves CoordinateDependencies.
Also applies to: 244-247
283-286: ParallelListItemFetch delegation is correctDelegating to the underlying SingleFetch configuration makes sense here.
v2/pkg/engine/resolve/resolve.go (1)
253-254: Good: loader gets the new flagPassing propagateFieldsRequestedBy into Loader completes the plumbing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
v2/pkg/engine/plan/visitor.go (2)
1399-1461: Tighten semantics: exclude “self” and stabilize output order for FieldsRequestedByTwo suggestions:
- Exclude the current fetch’s planner/subgraph from BySubgraphs to avoid self-referencing noise.
- Sort the resulting slice deterministically (by TypeName, FieldName) for stable tests/traces.
Apply this diff:
func (v *Visitor) buildFieldsRequestedBy(fetchID int) []resolve.RequestedField { fields, ok := v.plannerFields[fetchID] if !ok { return nil } result := make([]resolve.RequestedField, 0, len(fields)) + // Identify current subgraph for this fetch (to omit self) + currentSubgraphName := "" + if ofc := v.planners[fetchID].ObjectFetchConfiguration(); ofc != nil { + currentSubgraphName = ofc.sourceName + } + for _, fieldRef := range fields { fieldName := v.Operation.FieldNameString(fieldRef) if fieldName == "__typename" { continue } requested := resolve.RequestedField{ TypeName: v.fieldEnclosingTypeNames[fieldRef], FieldName: fieldName, } requested.ByUser = !v.skipField(fieldRef) requestedByRefs, ok := v.fieldRefAllowsFieldRefs[fieldRef] if !ok { if requested.ByUser { result = append(result, requested) } continue } requested.BySubgraphs = make([]string, 0, len(requestedByRefs)) for _, reqByRef := range requestedByRefs { depFieldPlanners, ok := v.fieldPlanners[reqByRef] if !ok { continue } // find the subgraph's name by iterating over planners that are responsible for reqByRef for _, plannerID := range depFieldPlanners { ofc := v.planners[plannerID].ObjectFetchConfiguration() - if ofc == nil { + if ofc == nil || ofc.sourceName == currentSubgraphName || plannerID == fetchID { continue } requested.BySubgraphs = append(requested.BySubgraphs, ofc.sourceName) depKind, ok := v.fieldDependencyKind[fieldDependencyKey{field: reqByRef, dependsOn: fieldRef}] if !ok { continue } switch depKind { case fieldDependencyKindKey: requested.ReasonIsKey = true case fieldDependencyKindRequires: requested.ReasonIsRequires = true } } } if requested.ByUser || len(requested.BySubgraphs) > 0 { slices.Sort(requested.BySubgraphs) requested.BySubgraphs = slices.Compact(requested.BySubgraphs) result = append(result, requested) } } + // Stable ordering across runs + slices.SortFunc(result, func(a, b resolve.RequestedField) int { + if c := strings.Compare(a.TypeName, b.TypeName); c != 0 { + return c + } + return strings.Compare(a.FieldName, b.FieldName) + }) return result }
636-646: skipField is fine; optimize only if neededLooping a small slice is OK. If skipFieldsRefs grows, switch to a map[int]struct{} as your TODO suggests.
v2/pkg/engine/resolve/fetch.go (2)
416-425: RequestedField payload shape is sensible; minor naming thoughtSchema looks good and compact. If you ever need per-subgraph reasons, you’ll outgrow booleans; for now, this is pragmatic.
Optional future-proofing: consider nesting reasons to avoid widening the struct later:
- reasons: { key: bool, requires: bool }
No change required for this PR.
350-354: Document metadata exclusions in FetchConfiguration.EqualsThe current implementation of
FetchConfiguration.Equals(lines 356–364) deliberately omits bothFieldsRequestedByandOperationName. These two fields carry metadata (the set of user/subgraph–requested fields and an optional operation name for tracing) and do not influence the structural equivalence of fetch plans.Please add an explicit note above the
Equalsmethod to clarify this behavior:func (fc *FetchConfiguration) Equals(other *FetchConfiguration) bool { + // Intentionally ignore metadata fields: + // • FieldsRequestedBy: for tracking which fields were requested + // • OperationName: for propagating the operation name upstream + // These do not affect structural equality of the fetch plan. if fc.Input != other.Input { return false } // … if fc.SetTemplateOutputToNullOnVariableNull != other.SetTemplateOutputToNullOnVariableNull { return false } return true }This documentation will help future maintainers understand why these fields are excluded, and ensure any caching or testing logic based on
Equalsremains accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/planner.go(3 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/resolve/fetch.go(11 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/plan/configuration.go
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/engine/resolve/loader.go
- v2/pkg/engine/plan/planner.go
- v2/pkg/engine/resolve/resolve.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (8)
v2/pkg/engine/plan/visitor.go (2)
58-66: Clear inline documentation for planner field mapsGood clarifications for plannerFields/fieldPlanners/fieldEnclosingTypeNames; improves maintainability.
49-49: No action required: inverse mapping is always initialized unconditionally in Plan()I’ve verified that in
planner.gothePlanmethod begins by assigning:p.planningVisitor.fieldRefDependsOnFieldRefs = selectionsConfig.fieldRefDependsOn p.planningVisitor.fieldDependencyKind = selectionsConfig.fieldDependencyKind p.planningVisitor.fieldRefAllowsFieldRefs = inverseMap(selectionsConfig.fieldRefDependsOn)This assignment at lines 155–157 is unguarded—there are no early
returnstatements preceding it—so on every call toPlan,fieldRefAllowsFieldRefsis populated.v2/pkg/engine/resolve/fetch.go (6)
25-26: Good: Fetch API gains RequestedFields() for a uniform surfaceThis will let loader/transport code query per-fetch field provenance without type switches.
171-181: BatchEntityFetch: per-fetch storage for requested fieldsLooks consistent with EntityFetch and ParallelListItemFetch. No issues.
191-194: BatchEntityFetch.RequestedFields implementation is straightforwardLGTM.
224-234: EntityFetch adds FieldsRequestedBy and exposes itAlignment with BatchEntityFetch is good.
283-286: ParallelListItemFetch delegates RequestedFields to underlying SingleFetchMakes sense for non-batching paths.
108-111: SingleFetch.RequestedFields is correctly delegated and populatedThe
RequestedFieldsmethod onSingleFetchsimply returnsFetchConfiguration.FieldsRequestedBy. In the planner (v2/pkg/engine/plan/visitor.go:1338), you can see:
singleFetch.FieldsRequestedBy = v.buildFieldsRequestedBy(internal.fetchID)This confirms that the planner populates
FetchConfiguration.FieldsRequestedBybefore execution. No further changes are needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/pkg/engine/plan/visitor.go (1)
1336-1339: Confirm field ownership on SingleFetch vs embedded FetchConfigurationYou now assign SingleFetch.CoordinateDependencies and SingleFetch.WhoRequestedFields directly. Earlier versions placed these on the embedded FetchConfiguration; there was a compile error noted in a prior review when assigning on the wrong struct.
Please confirm SingleFetch defines these fields. If not, switch to:
- singleFetch.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) - singleFetch.WhoRequestedFields = v.buildRequestedFields(internal.fetchID) + singleFetch.FetchConfiguration.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) + singleFetch.FetchConfiguration.WhoRequestedFields = v.buildRequestedFields(internal.fetchID)
🧹 Nitpick comments (9)
execution/engine/execution_engine_test.go (2)
827-876: Potential GET-vs-body ambiguity: prefer POST when sending extensionsThis test configures the subgraph fetch with Method: "GET" but asserts a non-empty JSON body including extensions.whoRequestedFields. Typical GraphQL over HTTP conventions avoid bodies on GET; some clients drop them. Unless the datasource automatically switches to POST when extensions are present, this may be brittle across transports.
- If the datasource auto-upgrades GET→POST when a body is needed, keep as-is but confirm in loader/datasource tests.
- Otherwise, set Method: "POST" for clarity and portability.
Suggested change:
- Fetch: &graphql_datasource.FetchConfiguration{ - URL: "https://example.com/", - Method: "GET", - }, + Fetch: &graphql_datasource.FetchConfiguration{ + URL: "https://example.com/", + Method: "POST", + },
4347-4471: Stability of whoRequestedFields ordering across runsexpectedBody1/2 compare raw JSON strings. buildRequestedFields iterates v.plannerFields[fetchID] insertion order; while likely stable, it’s not explicitly ordered. You already sort and compact requested.BySubgraphs; consider also sorting the top-level whoRequestedFields by (typeName, fieldName) for deterministic test behavior and future-proofing.
I’ve proposed a concrete diff on the builder in v2/pkg/engine/plan/visitor.go to sort the result slice.
v2/pkg/engine/plan/visitor.go (2)
636-647: skipField is O(n); acceptable, but simple to harden if the list growsGiven this runs once per visited field, the linear scan over skipFieldsRefs is likely fine. If skip lists can grow large in big plans, switch to a map[int]struct{} for O(1) lookups.
- // If this grows, switch to map[int]struct{} for O(1). - for _, skipRef := range v.skipFieldsRefs { - if skipRef == ref { - return true - } - } + // If this grows, consider using a map[int]struct{} for O(1) checks. + for _, skipRef := range v.skipFieldsRefs { + if skipRef == ref { + return true + } + }
1399-1461: Deterministic whoRequestedFields ordering for test stabilityThe construction is correct: you exclude __typename, mark requestedByUser via skipField, derive requestedBySubgraphs and reason flags from planners/dependency kinds, then sort/compact subgraphs. To avoid any flakiness in tests that compare raw request bodies, also sort the result slice.
Apply this diff to sort the top-level list by (TypeName, FieldName):
func (v *Visitor) buildRequestedFields(fetchID int) []resolve.RequestedField { @@ - for _, fieldRef := range fields { + for _, fieldRef := range fields { fieldName := v.Operation.FieldNameString(fieldRef) if fieldName == "__typename" { continue } @@ - if requested.ByUser || len(requested.BySubgraphs) > 0 { + if requested.ByUser || len(requested.BySubgraphs) > 0 { slices.Sort(requested.BySubgraphs) requested.BySubgraphs = slices.Compact(requested.BySubgraphs) result = append(result, requested) } } - - return result + // Ensure deterministic order of whoRequestedFields + slices.SortFunc(result, func(a, b resolve.RequestedField) int { + if a.TypeName == b.TypeName { + if a.FieldName == b.FieldName { + return 0 + } + if a.FieldName < b.FieldName { + return -1 + } + return 1 + } + if a.TypeName < b.TypeName { + return -1 + } + return 1 + }) + return result }v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go (1)
124-129: Same propagation for EntityFetchMirrors the batch path; looks good. If other concrete fetch types can include the extension in the future (e.g., ParallelListItemFetch envelopes), consider adding propagation there too for completeness, though not required for entity paths today.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (4)
3097-3135: Good addition: captures why parent fields and keys are pulled into the first fetch; ensure order is deterministicThe entries correctly mark Address.line1/line2 as requires from account.service, aggregate Address.id across all dependent subgraphs with ReasonIsKey, and flag the user-driven path (Query.user → User.account → Account.address). One nit: if BySubgraphs is built from a set/map in the planner, its order could be non-deterministic and make this assertion flaky. Consider either sorting BySubgraphs when producing WhoRequestedFields or sorting it in the test before assertion to avoid order-related flakes.
3183-3201: Should representation keys appear in WhoRequestedFields for this fetch?This fetch relies on Address.id in $representations, but the WhoRequestedFields here only lists Address.country and Address.city (ReasonIsRequires). If WhoRequestedFields is meant to describe all fields “included in the request” (query + variables) for a given fetch, consider adding Address.id with ReasonIsKey as well; if it’s meant to capture only the newly selected fields (not representation keys), a brief comment in the code/tests clarifying the intended scope would help prevent confusion.
3295-3313: Consistent requires attribution; clarify key usage if intendedSimilar to the previous block: Address.line3 and Address.zip are correctly attributed to account.service (ReasonIsRequires) for computing Address.fullAddress. If the intent is to also account for keys used in $representations per fetch, consider including Address.id with ReasonIsKey here, or confirm that keys are only tracked on the fetch where they’re actually selected (as in the first fetch).
3420-3431: User-driven final field looks right; optional enhancementAddress.fullAddress and the pathway Account.address are accurately marked as user requested. Optional: to aid future debugging, you could also assert the absence of requires/key reasons here (i.e., that only ByUser is set), to make the contract explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(4 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go(2 hunks)v2/pkg/engine/resolve/fetch.go(11 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/engine/resolve/loader.go
- v2/pkg/engine/resolve/resolve.go
- v2/pkg/engine/resolve/fetch.go
🧰 Additional context used
🧬 Code graph analysis (3)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
execution/engine/execution_engine_test.go (4)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceMetadata(34-56)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-354)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (6)
execution/engine/execution_engine_test.go (3)
218-219: Good addition: toggle for who-requested-fields propagationIntroducing the propagateWhoRequestedFields test flag on _executionTestOptions keeps the test harness clean and avoids leaking resolver concerns into every test case. No issues.
266-267: ResolverOptions wiring looks correctPropagateWhoRequestedFields is plumbed through ResolverOptions alongside existing flags. This ensures tests can flip the behavior per run without global impact.
4501-4502: Great coverage: with/without propagation path on federated planRunning the same scenario with and without the extension, and validating per-subgraph payloads (including reasonIsKey and requestedBySubgraphs) exercises the end-to-end plumbing well.
Also applies to: 4505-4540
v2/pkg/engine/plan/visitor.go (2)
1357-1397: Minor: consistent coordinate assemblyGood call building coordinates with FieldName from the operation and TypeName from fieldEnclosingTypeNames, mirroring the same approach for dependency origins. This keeps coordinates stable across aliasing.
49-66: Verify selection dependency maps are populated correctlyI see that
fieldRefAllowsFieldRefsis indeed initialized inv2/pkg/engine/plan/planner.go:157viap.planningVisitor.fieldRefAllowsFieldRefs = inverseMap(selectionsConfig.fieldRefDependsOn)and subsequently used in
v2/pkg/engine/plan/visitor.go:1417when building requested fields.However, please confirm that:
selectionsConfig.fieldRefDependsOnis populated with allfieldRef→[]dependentFieldRefsentries before the inverseMap call.- The inverse mapping occurs prior to any invocations of
buildRequestedFields, so thatBySubgraphsand reason flags will be set correctly.No code change needed if the above is already true; otherwise, ensure dependency analysis runs early enough.
v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go (1)
77-82: PropagateWhoRequestedFields for BatchEntityFetch Verified
- BatchEntityFetch (v2/pkg/engine/resolve/fetch.go) defines a FieldsRequestedBy slice and its RequestedFields() method returns this slice.
- Loader’s executeSourceLoad (v2/pkg/engine/resolve/loader.go) checks propagateWhoRequestedFields and calls fetchItem.Fetch.RequestedFields(), encoding and injecting the “whoRequestedFields” extension for both EntityFetch and BatchEntityFetch.
No further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
execution/engine/execution_engine_test.go (5)
214-219: Add a short doc on the new test knob for claritypropagateWhoRequestedFields is self-explanatory, but a one-line comment on its intent (propagates field provenance into subgraph request extensions) would help future readers scanning options.
type _executionTestOptions struct { resolvableOptions resolve.ResolvableOptions apolloRouterCompatibilitySubrequestHTTPError bool - propagateWhoRequestedFields bool + // when true, expected test round-trips must include extensions.whoRequestedFields in upstream requests + propagateWhoRequestedFields bool }
827-876: Good coverage for basic propagation; guard against brittle string comparisonsNice, this validates that enabling propagation produces the expected extensions payload and correctly switches to POST. However, matching the request body as a raw JSON string is fragile (key order and array ordering can change with innocuous refactors). Consider comparing bodies semantically in the test RoundTripper (parse JSON and deep-equal) or generate expected bodies via json.Marshal of typed structs to keep ordering deterministic.
If you want, I can provide a small change to createTestRoundTripper to accept an expected JSON value and compare canonically. As a local improvement in this file, you can also marshal a struct for expectedBody to avoid hand-crafted JSON strings.
4347-4471: Helper is useful; build expected JSON programmatically to reduce brittlenessThe makeDataSource(t, expectRequestedBy) abstraction is solid and keeps the two federation subgraphs readable. The only weak spot is the large hand-written expectedBody JSON strings. Building them via structs and json.Marshal will:
- eliminate quoting/escaping errors,
- lock key ordering to struct field order,
- make diffs smaller if the payload shape evolves.
You can keep the test harness unchanged since the output will still be a compact JSON string.
Apply this focused refactor to use a builder for the two expected bodies:
- var expectedBody1 string - var expectedBody2 string - if !expectRequestedBy { - expectedBody1 = `{"query":"{accounts {__typename ... on User {some {__typename id}} ... on Admin {some {__typename id}}}}"}` - expectedBody2 = `{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on User {__typename title}}}","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"3"}]}}` - } else { - expectedBody1 = `{"query":"{accounts {__typename ... on User {some {__typename id}} ... on Admin {some {__typename id}}}}","extensions":{"whoRequestedFields":[{"typeName":"User","fieldName":"id","requestedBySubgraphs":["id-2"],"reasonIsKey":true},{"typeName":"User","fieldName":"some","requestedByUser":true},{"typeName":"User","fieldName":"id","requestedByUser":true},{"typeName":"Admin","fieldName":"some","requestedByUser":true},{"typeName":"Query","fieldName":"accounts","requestedByUser":true}]}}` - expectedBody2 = `{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on User {__typename title}}}","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"3"}]},"extensions":{"whoRequestedFields":[{"typeName":"User","fieldName":"title","requestedByUser":true},{"typeName":"User","fieldName":"some","requestedByUser":true}]}}` - } + expectedBody1, expectedBody2 := buildFederationExpectedBodies(expectRequestedBy)Add this helper somewhere in this test file (outside the function):
// support code for the diff above type gqlReq struct { Query string `json:"query"` Variables map[string]any `json:"variables,omitempty"` Extensions map[string]any `json:"extensions,omitempty"` } func mustMarshal(v any) string { b, err := json.Marshal(v) if err != nil { panic(err) } return string(b) } func buildFederationExpectedBodies(withReasons bool) (string, string) { // first subgraph body body1 := gqlReq{ Query: `{accounts {__typename ... on User {some {__typename id}} ... on Admin {some {__typename id}}}}`, } if withReasons { body1.Extensions = map[string]any{ "whoRequestedFields": []any{ map[string]any{"typeName": "User", "fieldName": "id", "requestedBySubgraphs": []string{"id-2"}, "reasonIsKey": true}, map[string]any{"typeName": "User", "fieldName": "some", "requestedByUser": true}, map[string]any{"typeName": "User", "fieldName": "id", "requestedByUser": true}, map[string]any{"typeName": "Admin", "fieldName": "some", "requestedByUser": true}, map[string]any{"typeName": "Query", "fieldName": "accounts", "requestedByUser": true}, }, } } // second subgraph body body2 := gqlReq{ Query: `query($representations: [_Any!]!){_entities(representations: $representations){... on User {__typename title}}}`, Variables: map[string]any{ "representations": []any{ map[string]any{"__typename": "User", "id": "1"}, map[string]any{"__typename": "User", "id": "3"}, }, }, } if withReasons { body2.Extensions = map[string]any{ "whoRequestedFields": []any{ map[string]any{"typeName": "User", "fieldName": "title", "requestedByUser": true}, map[string]any{"typeName": "User", "fieldName": "some", "requestedByUser": true}, }, } } return mustMarshal(body1), mustMarshal(body2) }
4505-4540: End-to-end federation path with propagation enabledThis complements the non-propagating run and validates the extension plumbing across both subgraphs. Consider one more variant where Method remains POST but propagation is disabled (to assert that no extensions key leaks when the flag is false).
I can add a follow-up test variant if you’d like.
827-876: Reduce risk from ordering of whoRequestedFields itemsDepending on how the planner assembles WhoRequestedFields, the slice ordering could change with future refactors. If the order is not guaranteed, tests may become flaky. Either:
- sort the whoRequestedFields slice before marshalling upstream bodies, or
- adapt the RoundTripper to compare the whoRequestedFields array as a set (order-insensitive).
If ordering is guaranteed by construction, please add a brief comment stating that invariant so future changes don’t unintentionally break it.
Would you like me to add a deterministic sort (e.g., by typeName, then fieldName, then flags) at the injection point?
Also applies to: 4347-4471
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
execution/engine/execution_engine_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
execution/engine/execution_engine_test.go (6)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceMetadata(34-56)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-354)v2/pkg/engine/plan/federation_metadata.go (2)
FederationMetaData(10-16)FederationFieldConfigurations(113-113)v2/pkg/engine/resolve/context.go (1)
Request(148-151)
🔇 Additional comments (4)
execution/engine/execution_engine_test.go (4)
262-267: Plumbing the option through ResolverOptions looks correctPassing PropagateWhoRequestedFields from test options into resolve.ResolverOptions is consistent with the new API surface. No issues spotted.
858-861: POST is required to carry extensionsGood call to switch the fetch method to POST for this case; a GET would drop the body and the extension. This keeps behavior explicit in the test.
4411-4418: Federation config is set on SchemaConfigurationGood to see federation explicitly enabled with the corresponding ServiceSDL; matches the entity fetches used in this test.
262-267: All good:PropagateWhoRequestedFieldsis wired and the extension key is consistent
- The
ResolverOptionsstruct in v2/pkg/engine/resolve/resolve.go includesPropagateWhoRequestedFieldsand it’s passed through to the loader via
propagateWhoRequestedFields: options.PropagateWhoRequestedFields.- In v2/pkg/engine/resolve/loader.go the JSON extension
"whoRequestedFields"is injected (at line 1575) using that flag.- Tests in execution/engine/execution_engine_test.go and execution_engine_helpers_test.go uniformly use
"whoRequestedFields"in their expected bodies.- The
createTestRoundTripperimplementations in both pkg/graphql and execution/engine compare the raw request body string, so formatting changes would break existing tests.No further action required here.
ysmolski
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/resolve/fetch.go (1)
20-26: Critical: ImplementRequestedFields()on all built-in Fetch types and verify downstream consumersI ran a quick grep for types that implement
FetchKind()—the heuristic for Fetch implementers—and confirmed that none currently defineRequestedFields(), so the interface change breaks internal code immediately:
- SingleFetch (v2/pkg/engine/resolve/fetch.go:163)
- BatchEntityFetch (v2/pkg/engine/resolve/fetch.go:217)
- EntityFetch (v2/pkg/engine/resolve/fetch.go:262)
- ParallelListItemFetch (v2/pkg/engine/resolve/fetch.go:287)
Please add a
RequestedFields() []RequestedFieldmethod to each of these types (for example, returningnilor the appropriate field list) so they satisfy the updated interface. For example:func (f *SingleFetch) RequestedFields() []RequestedField { return nil }Once internal types compile, also audit any out-of-tree implementations of
Fetchin downstream repos. If you find external clients relying on the old interface, consider:
- Providing a default/no-op adapter that implements
RequestedFields()- Introducing a wrapper interface to preserve backward compatibility
♻️ Duplicate comments (2)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
3296-3313: Same consideration as the previous fetch about representation keys and determinism.The comments on Lines 3183–3201 apply here as well (potential inclusion of Address.id as ReasonIsKey and sorting of BySubgraphs for stability).
v2/pkg/engine/plan/visitor.go (1)
1337-1339: Fix assignments to embedded FetchConfiguration to avoid compile error.SingleFetch does not (currently) declare CoordinateDependencies or FieldsRequestedBy at the top level; they live on the embedded FetchConfiguration. Assigning to the wrong receiver won’t compile.
Apply this diff:
- singleFetch.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) - singleFetch.FieldsRequestedBy = v.buildRequestedFields(internal.fetchID) + singleFetch.FetchConfiguration.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) + singleFetch.FetchConfiguration.FieldsRequestedBy = v.buildRequestedFields(internal.fetchID)To double-check the struct shape in your current branch, run:
#!/bin/bash # Inspect where FieldsRequestedBy is declared rg -nP 'type\s+SingleFetch\b[\s\S]*?\{' -n v2/pkg/engine/resolve/fetch.go -A30 rg -nP 'type\s+FetchConfiguration\b[\s\S]*?\{' -n v2/pkg/engine/resolve/fetch.go -A60 | sed -n '1,120p'
🧹 Nitpick comments (11)
v2/pkg/engine/resolve/fetch.go (4)
171-181: Inconsistent storage location for FieldsRequestedBy across fetch typesBatchEntityFetch stores FieldsRequestedBy on the fetch itself, while SingleFetch keeps it in FetchConfiguration. Consider standardizing to a single location (preferably on FetchConfiguration for consistency) or add a short comment explaining the rationale to avoid confusion for contributors.
Apply a clarifying comment locally if you keep the current layout:
type BatchEntityFetch struct { FetchDependencies - + // FieldsRequestedBy contains the per-fetch field request provenance. + // Note: Unlike SingleFetch, this is stored on the fetch itself rather than FetchConfiguration. Input BatchInput DataSource DataSource PostProcessing PostProcessingConfiguration DataSourceIdentifier []byte Trace *DataSourceLoadTrace Info *FetchInfo CoordinateDependencies []FetchDependency FieldsRequestedBy []RequestedField }
226-234: Mirror comment from BatchEntityFetch: storage consistencyEntityFetch mirrors BatchEntityFetch by storing FieldsRequestedBy on the struct itself and not in a FetchConfiguration. If you keep this asymmetry with SingleFetch/ParallelListItemFetch, please document it to prevent accidental future regressions.
350-354: Equality semantics: FieldsRequestedBy and OperationName are not comparedFetchConfiguration.Equals does not compare FieldsRequestedBy or OperationName. If plan equality is used for deduplication or caching, differing values here won’t affect equality. That could be fine if these are diagnostics-only, but please confirm.
Option A (document intent): add an explicit comment in Equals:
func (fc *FetchConfiguration) Equals(other *FetchConfiguration) bool { @@ - // Note: we do not compare CoordinateDependencies, as they contain more detailed + // Note: we do not compare CoordinateDependencies, as they contain more detailed // dependencies information that is already present in the FetchDependencies on the fetch itself + // Note: we intentionally do not compare FieldsRequestedBy and OperationName, + // as they are used for diagnostics/propagation only and should not affect plan equality.Option B (enforce equality): include these fields in the comparison if they must influence plan identity.
416-425: Shape and naming of RequestedField; consider extensibility and consistent casing
- The by/reason information is encoded as booleans on the whole coordinate. If a coordinate is requested by multiple subgraphs for different reasons, we lose per-subgraph reason fidelity. Consider a structure like per-subgraph entries or a reasons []string enum (["key","requires"]) for future-proofing.
- JSON key casing: struct fields use lowerCamelCase (requestedBySubgraphs/requestedByUser), while the request extension key added in loader.go is "FieldsRequestedBy" (PascalCase). Recommend aligning on lowerCamelCase for consistency with other extension fields (e.g., statusCode).
If you decide to compact the reasons:
type RequestedField struct { TypeName string `json:"typeName"` FieldName string `json:"fieldName"` - BySubgraphs []string `json:"requestedBySubgraphs,omitempty"` - ByUser bool `json:"requestedByUser,omitempty"` - ReasonIsKey bool `json:"reasonIsKey,omitempty"` - ReasonIsRequires bool `json:"reasonIsRequires,omitempty"` + BySubgraphs []string `json:"requestedBySubgraphs,omitempty"` + ByUser bool `json:"requestedByUser,omitempty"` + Reasons []string `json:"reasons,omitempty"` // e.g. ["key","requires"] }At minimum, ensure BySubgraphs is de-duplicated and stable-sorted before marshalling to keep payloads deterministic.
v2/pkg/engine/resolve/loader.go (2)
1565-1581: Only inject when non-empty and align extension key casing; keep extensions object robust
- Skip injection when the slice is empty to reduce payload churn.
- Prefer "fieldsRequestedBy" (lowerCamelCase) to be consistent with other extension keys (e.g., statusCode).
- The jsonparser.Set call is correct and will create missing objects, but will error if extensions is not an object. Your error handling is good.
Apply this diff:
- fieldsRequestedBy := fetchItem.Fetch.RequestedFields() - if fieldsRequestedBy != nil { + fieldsRequestedBy := fetchItem.Fetch.RequestedFields() + if len(fieldsRequestedBy) > 0 { var encoded []byte encoded, res.err = json.Marshal(fieldsRequestedBy) if res.err != nil { res.err = errors.WithStack(res.err) return } - // We expect that body.extensions is an object - input, res.err = jsonparser.Set(input, encoded, "body", "extensions", "FieldsRequestedBy") + // Ensure body.extensions exists (jsonparser will create intermediate objects) and set the field. + input, res.err = jsonparser.Set(input, encoded, "body", "extensions", "fieldsRequestedBy") if res.err != nil { res.err = errors.WithStack(res.err) return } }If you anticipate frequent reuse of the same slice per fetch, consider precomputing and caching the marshalled bytes on the fetch to avoid per-request marshalling.
1565-1581: Add coverage: verify propagation behavior across fetch kinds and introspection exclusionPlease add tests that assert:
- Nothing is injected for introspection data sources.
- fieldsRequestedBy is present only when the option is enabled.
- Injection works for SingleFetch, EntityFetch, BatchEntityFetch, and ParallelListItemFetch.
- Empty slices are not injected once the guard is added.
I can draft table-driven tests around executeSourceLoad/loadSingleFetch with fake DataSource stubs to validate the input body payloads. Let me know if you want me to push a test scaffolding.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
3183-3201: Should representation keys also appear in FieldsRequestedBy, or is this selection-set only?This fetch sends Address.id in variables.representations, but id isn’t included in FieldsRequestedBy. If the extension is meant to explain “why fields were included in the request” to the receiving subgraph, keys passed via variables may also be relevant (e.g., mark id with ReasonIsKey). If the intent is “selection set only,” consider documenting that and keeping keys out for noise reduction.
Would you like me to add a test/helper asserting the chosen semantics (selection-set-only vs selection+variables) so future changes don’t regress this?
execution/engine/execution_engine_test.go (2)
827-876: Solid happy-path coverage for per-fetch extensions; one naming consideration.The round-tripper assertion verifies the extension payload and HTTP method switch to POST when extensions are present. Minor: consider aligning the extension key casing with existing keys like "valueCompletion" (camelCase) for consistency, e.g., "fieldsRequestedBy". If you decide to change it, we can update the tests and loader in one pass.
If you opt in, I can provide a scoped diff for the tests once the extension key name is finalized.
4505-4541: Great federated coverage; suggest adding one more case for '@requires'.This test exercises keys and cross-subgraph dependency reporting. Consider adding a similar case with @requires to prove ReasonIsRequires toggles correctly and that requestedBySubgraphs attribution matches the providing subgraph.
Happy to draft a minimal schema+plan snippet for @requires if helpful.
v2/pkg/engine/plan/visitor.go (1)
1399-1461: Requested-field computation is coherent and stable; minor nit on allocations.Logic cleanly:
- filters out __typename,
- flags user vs dependency,
- collects subgraph names from planners,
- marks key/requires,
- sorts and compacts.
Nit: FieldNameString and repeated slice appends can allocate in tight loops on large selections. Not urgent, but consider pre-sizing BySubgraphs using the max of fieldPlanners[reqByRef] lengths, or caching FieldNameString results if this shows up in profiles.
v2/pkg/engine/resolve/resolve.go (1)
168-171: Clarify transport requirement in the option doc.Since extensions are carried in the GraphQL request body, this effectively requires HTTP POST to subgraphs. Add a note to avoid surprises when a datasource is configured for GET.
Apply this doc tweak:
- // PropagateFieldsRequestedBy enables sending to upstream subgraphs (only) the "FieldsRequestedBy" - // extension that explains why each field was requested. This flag does not expose the data to clients. + // PropagateFieldsRequestedBy enables sending to upstream subgraphs (only) the "FieldsRequestedBy" + // extension that explains why each field was requested. This flag does not expose the data to clients. + // Note: This requires GraphQL over HTTP POST (JSON body). GET datasources will not include this extension.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(4 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go(2 hunks)v2/pkg/engine/resolve/fetch.go(11 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/postprocess/create_concrete_single_fetch_types.go
🧰 Additional context used
🧬 Code graph analysis (6)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (1)
Fetch(20-26)
execution/engine/execution_engine_test.go (4)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceMetadata(34-56)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-354)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
v2/pkg/engine/resolve/resolve.go (1)
v2/pkg/engine/resolve/resolvable.go (1)
ResolvableOptions(62-67)
v2/pkg/engine/resolve/fetch.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(256-269)v2/pkg/engine/resolve/datasource.go (1)
DataSource(12-15)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (11)
v2/pkg/engine/resolve/fetch.go (5)
108-110: LGTM: SingleFetch.RequestedFields exposes per-fetch metadataSimple passthrough to FetchConfiguration.FieldsRequestedBy. Looks good.
126-142: Docs-only tweaks in PostProcessingConfigurationNon-functional comment changes; nothing to flag.
191-194: LGTM: BatchEntityFetch.RequestedFields passthroughStraightforward accessor.
244-247: LGTM: EntityFetch.RequestedFields passthroughAccessor reads from the struct field as expected.
283-286: LGTM: ParallelListItemFetch.RequestedFields delegates to underlying SingleFetchCorrectly exposes the same metadata per list item fetch.
v2/pkg/engine/resolve/loader.go (1)
154-155: ✅ propagateFieldsRequestedBy correctly wired (default remains false)All looks wired up as intended:
- In v2/pkg/engine/resolve/resolve.go (line 253),
options.PropagateFieldsRequestedByis passed through into the loader’spropagateFieldsRequestedByfield.- The
Loaderstruct in v2/pkg/engine/resolve/loader.go definespropagateFieldsRequestedBy bool, so the flag has a proper home.- Because Go’s zero value for
boolisfalseand there’s no override, existing users won’t see any payload changes by default.No changes needed here.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (2)
3420-3431: LGTM: user-requested fields are captured cleanly at the final fetch.Capturing Address.fullAddress and Account.address as ByUser=true aligns with the intent of the extension. No issues spotted.
3097-3135: BySubgraphs ordering is already deterministic in buildRequestedFieldsI checked v2/pkg/engine/plan/visitor.go around lines 1453–1455, and you’ll see we already call slices.Sort(requested.BySubgraphs) followed by slices.Compact(requested.BySubgraphs) before returning the slice. That guarantees the same lexicographical order of BySubgraphs every run, so this test won’t flake. No changes needed here.
execution/engine/execution_engine_test.go (2)
218-219: Plumbs the new test knob cleanly.Adding propagateFieldsRequestedBy to the local options struct is straightforward and keeps the feature flag scoped to tests. LGTM.
266-267: ResolverOptions wiring looks correct.PropagateFieldsRequestedBy is passed through to the engine; no other behavior is altered. LGTM.
v2/pkg/engine/resolve/resolve.go (1)
237-256: Loader wiring LGTM; validate GET-to-POST behavior when needed.Flag propagation to Loader is correct. One operational consideration: if a datasource is configured with Method: GET and the flag is enabled, do we silently omit the extension or upgrade to POST? Either behavior is fine, but it should be explicit.
If the current behavior is “omit”, consider logging once per fetch when the flag is set but the transport can’t carry extensions. If you prefer “upgrade”, we can add a guarded switch in the GraphQL loader path.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/visitor.go (1)
49-66: The grep output shows no assignments or initializations forplannerFields,fieldPlanners, orfieldEnclosingTypeNamesanywhere inv2/pkg/engine/plan. OnlyfieldRefAllowsFieldRefsis populated viainverseMapin the planner. The other new maps remain uninitialized and will be nil at runtime.Please add initialization and population logic for these maps in your planner setup—similar to how you handle
fieldRefAllowsFieldRefs—to ensure they’re safe to read:• In
planner.go, after constructingplanningVisitor, initialize:
plannerFields = make(map[int][]int)fieldPlanners = make(map[int][]int)fieldEnclosingTypeNames = make(map[int]string)• Populate each map from
selectionsConfig(or other source) before invokingplanningWalker.ResetVisitors().Once these are wired up, your visitor’s reads for these maps will never encounter nil/map lookups.
♻️ Duplicate comments (1)
execution/engine/execution_engine_test.go (1)
4336-4473: Nested federation test: expectations and metadata align with planner semantics.
- expectedBody1’s FieldsRequestedBy covers: User.id (byUser and bySubgraphs with reasonIsKey) and Admin.some (byUser). Matches ProtectedFieldNames and dependency chain.
- expectedBody2 intentionally has no extensions (no protected fields on subgraph 2). This fixes the earlier “some” leak.
LGTM.
🧹 Nitpick comments (10)
v2/pkg/engine/plan/type_field.go (3)
4-7: Document field semantics and intent (esp. “Protected”).Add brief GoDoc-style comments to avoid ambiguity and help downstream users.
type TypeField struct { - TypeName string - FieldNames []string - ExternalFieldNames []string - ProtectedFieldNames []string + TypeName string // GraphQL type name + FieldNames []string // Fields owned/resolved by this subgraph + ExternalFieldNames []string // Fields that are externally resolved (federated) + ProtectedFieldNames []string // Fields included due to auth/policy requirements }If you also want a type-level comment (outside the changed lines):
// TypeField describes fields associated with a GraphQL type, including // externally resolved and authorization-protected fields.
4-7: Enforce uniqueness and deterministic ordering of name slices.Downstream set-membership and diff-based tests benefit from canonicalized data. Consider normalizing when constructing instances.
// Helper: returns sorted unique copy. func uniqueSorted(in []string) []string { if len(in) == 0 { return nil } seen := make(map[string]struct{}, len(in)) out := make([]string, 0, len(in)) for _, s := range in { if _, ok := seen[s]; ok { continue } seen[s] = struct{}{} out = append(out, s) } slices.Sort(out) // Go 1.21+ return out } func NewTypeField(typeName string, fields, external, protected []string) TypeField { return TypeField{ TypeName: typeName, FieldNames: uniqueSorted(fields), ExternalFieldNames: uniqueSorted(external), ProtectedFieldNames: uniqueSorted(protected), } }
4-7: If serialized, prefer non-nil slices for stable JSON shape.If TypeField is marshaled into subgraph request extensions, decide between null vs [] and keep it consistent (initialize to empty slices if needed).
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (3)
1086-1089: ProtectedFieldNames on Address: confirm intent and keep consistent across subgraphsYou protect id, line1, line2 here. Later for Address in address.service you only protect line3, zip. If asymmetry is intentional, add a short comment; otherwise, align both (e.g., protect id in both or neither).
1268-1271: ProtectedFieldNames asymmetry with users subgraphHere id is not protected, unlike users.service. Consider adding id for consistency, or document why it's different.
Apply if you want to align:
- ProtectedFieldNames: []string{"line3", "zip"}, + ProtectedFieldNames: []string{"id", "line3", "zip"},
3263-3276: Requires reasons captured well; consider expanding coverageZip and line3 are correctly marked ReasonIsRequires by account.service. Optionally add a case where ByUser=true is exercised, and/or a fetch showing requires from a sibling (e.g., address-enricher) to validate multi-hop attribution.
execution/engine/execution_engine_test.go (1)
827-879: Good positive test for extension payload on simple hero.
- Body asserts the extensions.FieldsRequestedBy structure and key naming.
- ProtectedFieldNames on Query.hero and Character.name align with filter logic.
Consider also asserting absence when flag is off for this same scenario (mirrors nested test). Optional.
v2/pkg/engine/plan/datasource_configuration.go (1)
99-146: Micro-optimization: pre-size child maps.You pre-sized root maps using slice lengths; do the same for child to reduce rehashing.
- if _, ok := d.childNodesIndex[typeName]; !ok { - d.childNodesIndex[typeName] = fieldsIndex{ - fields: make(map[string]struct{}), - externalFields: make(map[string]struct{}), - protectedFields: make(map[string]struct{}), - } - } + if _, ok := d.childNodesIndex[typeName]; !ok { + d.childNodesIndex[typeName] = fieldsIndex{ + fields: make(map[string]struct{}, len(d.ChildNodes[i].FieldNames)), + externalFields: make(map[string]struct{}, len(d.ChildNodes[i].ExternalFieldNames)), + protectedFields: make(map[string]struct{}, len(d.ChildNodes[i].ProtectedFieldNames)), + } + }v2/pkg/engine/plan/visitor.go (2)
636-646: Potential hot path: convert to set if it grows.You already noted switching to map if skipFieldsRefs expands. Keep an eye on size in large ops.
1399-1466: Requested-fields builder works; consider deduping by coordinate.Today, identical coordinates can appear multiple times (e.g., byUser and bySubgraphs for User.id), inflating payloads. Optional: merge per (typeName, fieldName) to a single entry with combined flags and sources for simpler consumption and smaller bodies.
Example minimal refactor inside this block:
- result := make([]resolve.RequestedField, 0, len(fields)) + merged := make(map[string]*resolve.RequestedField, len(fields)) ... - requested := resolve.RequestedField{ TypeName: typeName, FieldName: fieldName } + key := typeName + "\x00" + fieldName + rf, ok := merged[key] + if !ok { + rf = &resolve.RequestedField{ TypeName: typeName, FieldName: fieldName } + merged[key] = rf + } + requested := *rf ... - if requested.ByUser || len(requested.BySubgraphs) > 0 { - slices.Sort(requested.BySubgraphs) - requested.BySubgraphs = slices.Compact(requested.BySubgraphs) - result = append(result, requested) - } + if requested.ByUser || len(requested.BySubgraphs) > 0 { + slices.Sort(requested.BySubgraphs) + requested.BySubgraphs = slices.Compact(requested.BySubgraphs) + *rf = requested + } ... - return result + // flatten in stable order + out := make([]resolve.RequestedField, 0, len(merged)) + for _, v := range merged { + out = append(out, *v) + } + slices.SortFunc(out, func(a, b resolve.RequestedField) int { + if a.TypeName == b.TypeName { + if a.FieldName == b.FieldName { return 0 } + if a.FieldName < b.FieldName { return -1 } + return 1 + } + if a.TypeName < b.TypeName { return -1 } + return 1 + }) + return outNote: would require test updates if order changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(4 hunks)v2/pkg/engine/plan/datasource_configuration.go(5 hunks)v2/pkg/engine/plan/federation_metadata.go(1 hunks)v2/pkg/engine/plan/type_field.go(1 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/resolve/fetch.go(11 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/resolve/loader.go
- v2/pkg/engine/resolve/fetch.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-09T09:34:51.269Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
Applied to files:
v2/pkg/engine/plan/federation_metadata.go
🧬 Code graph analysis (3)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
v2/pkg/engine/resolve/fetch.go (1)
RequestedField(418-425)
execution/engine/execution_engine_test.go (4)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(293-306)DataSourceMetadata(34-56)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-354)
🔇 Additional comments (13)
v2/pkg/engine/plan/federation_metadata.go (1)
108-111: Confirm value-based fmt.Stringer usage for FederationFieldConfigurationI ran targeted searches but found no direct uses of
fmt.Print*on value instances or explicit calls toString()/Format()onFederationFieldConfiguration. However, absence of matches isn’t proof of safety. Please manually verify that no code paths pass a value (non-pointer) tofmtverbs expectingfmt.Stringer, as the pointer-receiver change alters:
- Value-based
fmt.Sprintf("%s", fValue)will no longer callString(), producing Go’s default struct formatting instead of JSON.- A nil-pointer
fnow marshals to"null"rather than{}; confirm this semantic shift is acceptable.If you need backward-compatible behavior, consider:
- Adding an explicit nil guard in
String():func (f *FederationFieldConfiguration) String() string { + if f == nil { + return "null" + } b, _ := json.Marshal(f) return string(b) }- Implementing a
Formaton the value receiver to preserve%s/%vbehavior:func (f FederationFieldConfiguration) Format(s fmt.State, verb rune) { _, _ = fmt.Fprint(s, (&f).String()) }Please review all formatting and logging sites to ensure no regressions occur.
v2/pkg/engine/plan/type_field.go (1)
4-7: No lingering usages of removed TypeFields helpersI’ve verified that there are no remaining calls to HasNode, HasExternalNode, or HasNodeWithTypename across the codebase. The only matches for
TypeFields.*.are in type signatures and function definitions (not method invocations on the struct), so all helper call sites have been successfully removed.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (1)
3099-3122: No action needed—BySubgraphs is already sorted and deduplicated
The implementation ofbuildRequestedFieldsinv2/pkg/engine/plan/visitor.gocallsslices.Sort(requested.BySubgraphs)followed byslices.Compact(requested.BySubgraphs)before returning its results (lines 1459–1461). This ensures a deterministic, lexicographically ordered, and deduplicated slice of subgraph names, matching the test’s expectations. You can ignore the suggestion to add sorting logic here.Likely an incorrect or invalid review comment.
execution/engine/execution_engine_test.go (3)
218-219: Test option plumbing looks good.Adding propagateFieldsRequestedBy to _executionTestOptions is clear and default-safe.
262-267: ResolverOptions flag wired correctly.PropagateFieldsRequestedBy is passed through to the engine; no regressions seen.
4503-4543: Nice A/B coverage.
- “run” (flag off) vs “run with extension” (flag on) validates toggle behavior for nested fetches.
v2/pkg/engine/plan/datasource_configuration.go (4)
54-56: Indexing by typeName improves lookup performance.The new root/child indexes with protectedFields support are appropriate for fast membership checks.
67-76: NodesInfo expanded coherently.HasProtectedRootNode/HasProtectedChildNode API is consistent with existing Has* methods.
178-188: Protected-root accessor is straightforward and safe on nil indexes.LGTM.
223-233: Protected-child accessor mirrors root accessor correctly.LGTM.
v2/pkg/engine/plan/visitor.go (3)
342-345: Capturing enclosing type names early is necessary for coordinates.This enables stable GraphCoordinate and extension entries.
211-223: Recording planner-field associations on LeaveField is correct.Post-order ensures children precede parents, matching test ordering of FieldsRequestedBy.
1344-1397: Dependency extraction to GraphCoordinates looks consistent.Uses fieldEnclosingTypeNames and dependencyKind to flag IsKey/IsRequires. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/pkg/engine/plan/visitor.go (1)
1336-1339: Assignments to promoted fields are valid; previous compile error is resolvedUsing singleFetch.CoordinateDependencies and singleFetch.FieldsRequestedBy works via the embedded FetchConfiguration; no need to qualify further.
🧹 Nitpick comments (5)
v2/pkg/engine/plan/visitor.go (2)
636-646: Consider O(1) membership for skipFieldsRefsLinear scan is fine for small slices; if this grows, switch to a map[int]struct{} or build a lazy index once per document.
func (v *Visitor) skipField(ref int) bool { - // If this grows, switch to map[int]struct{} for O(1). - for _, skipRef := range v.skipFieldsRefs { - if skipRef == ref { - return true - } - } - return false + // If this grows, switch to map[int]struct{} for O(1). + for _, skipRef := range v.skipFieldsRefs { + if skipRef == ref { + return true + } + } + return false }
1399-1489: Prevent duplicate BySubgraphs entries when merging duplicatesIf the same (type, field) appears multiple times in plannerFields, the merge path appends BySubgraphs without re-deduping, leading to duplicates.
Apply this minimal fix inside the merge branch:
- if i, ok := index[key]; ok { + if i, ok := index[key]; ok { // True should overwrite false. result[i].ByUser = result[i].ByUser || byUser if len(bySubgraphs) > 0 { - result[i].BySubgraphs = append(result[i].BySubgraphs, bySubgraphs...) + result[i].BySubgraphs = append(result[i].BySubgraphs, bySubgraphs...) + slices.Sort(result[i].BySubgraphs) + result[i].BySubgraphs = slices.Compact(result[i].BySubgraphs) result[i].ReasonIsKey = result[i].ReasonIsKey || isKey result[i].ReasonIsRequires = result[i].ReasonIsRequires || isRequires } continue }v2/pkg/engine/resolve/fetch.go (3)
350-351: Document semantics of FieldsRequestedBy on FetchConfigurationAdd a short doc comment: protected fields only; used for upstream extension propagation; no execution impact.
type FetchConfiguration struct { @@ - FieldsRequestedBy []RequestedField + // FieldsRequestedBy contains provenance for protected fields (schema @protected). + // Used for optional propagation to subgraphs via request extensions; does not affect execution. + FieldsRequestedBy []RequestedField
356-386: Clarify equality: explicitly ignore FieldsRequestedByEquals already ignores CoordinateDependencies; call out FieldsRequestedBy too to avoid confusion.
// Note: we do not compare CoordinateDependencies, as they contain more detailed // dependencies information that is already present in the FetchDependencies on the fetch itself + // Note: we also intentionally do not compare FieldsRequestedBy, as it is diagnostic/telemetry-only + // and does not affect fetch execution semantics.
416-425: Align JSON field names with existing GraphCoordinate conventionsUse typeName/fieldName for consistency and to avoid conflation with GraphQL’s __typename.
type RequestedField struct { - TypeName string `json:"__typename"` - FieldName string `json:"field"` + TypeName string `json:"typeName"` + FieldName string `json:"fieldName"` BySubgraphs []string `json:"bySubgraphs,omitempty"` ByUser bool `json:"byUser,omitempty"` ReasonIsKey bool `json:"reasonIsKey,omitempty"` ReasonIsRequires bool `json:"reasonIsRequires,omitempty"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/resolve/fetch.go(11 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/resolve/loader.go
- execution/engine/execution_engine_test.go
🔇 Additional comments (9)
v2/pkg/engine/plan/visitor.go (2)
49-49: Good addition: inverse dependency map capturedStoring the inverse mapping (fieldRefAllowsFieldRefs) on the visitor simplifies reasoning when assembling “requested by” provenance.
58-66: Planner/field tracking maps look correctThe three maps (plannerFields, fieldPlanners, fieldEnclosingTypeNames) are initialized in EnterDocument and populated in AllowVisitor/EnterField — solid groundwork for per-fetch attribution.
v2/pkg/engine/resolve/fetch.go (7)
104-111: OK: methods use promoted fields from embedded FetchConfigurationReturning s.CoordinateDependencies / s.FieldsRequestedBy via embedding keeps the API simple.
171-181: BatchEntityFetch wiring looks consistentNew storage and accessor for FieldsRequestedBy aligns with SingleFetch semantics.
191-194: OK: BatchEntityFetch.RequestedFields implemented
224-234: EntityFetch carries FieldsRequestedBy and CoordinateDependenciesMatches BatchEntityFetch; consistent across variants.
244-247: OK: EntityFetch.RequestedFields implemented
283-285: OK: ParallelListItemFetch delegates to underlying SingleFetch
25-26: Ensure all Fetch implementations define RequestedFields()The
Fetchinterface inv2/pkg/engine/resolve/fetch.gowas extended with:RequestedFields() []RequestedFieldBecause Go requires that every type implementing an interface must define all its methods, please manually verify that each concrete type providing a
FetchKind()method also implementsRequestedFields(). A quick way to locate them is:rg -nP 'func\s*\(\*?[A-Za-z_][A-Za-z0-9_]*\)\s*FetchKind\(' -C1 v2/pkg/engine/resolveThen, for every listed type
T, ensure there is:func (t *T) RequestedFields() []RequestedField { // TODO: return the list of fields this fetch operation needs }This check couldn’t be fully automated with the previous script—absence of results isn’t evidence that everything is covered. Please confirm by eyeballing or enhancing the search pattern to match any variations in receiver names or return-type qualifiers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/pkg/engine/plan/visitor.go (1)
1338-1341: Assign via embedded struct for clarity (no-op behavior-wise).Being explicit avoids confusion if SingleFetch gains same-named fields later.
- singleFetch.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) - singleFetch.FieldFetchReasons = v.buildFetchReasons(internal.fetchID) + singleFetch.FetchConfiguration.CoordinateDependencies = v.resolveFetchDependencies(internal.fetchID) + singleFetch.FetchConfiguration.FieldFetchReasons = v.buildFetchReasons(internal.fetchID)
🧹 Nitpick comments (3)
execution/engine/execution_engine_test.go (2)
218-218: Rename test option to match ResolverOptions and feature naming.Use “fetchReasons” consistently (not “fieldsRequestedBy”).
- propagateFieldsRequestedBy bool + propagateFetchReasons bool- PropagateFetchReasons: opts.propagateFieldsRequestedBy, + PropagateFetchReasons: opts.propagateFetchReasons,- eto.propagateFieldsRequestedBy = true + eto.propagateFetchReasons = true- eto.propagateFieldsRequestedBy = true + eto.propagateFetchReasons = trueAlso applies to: 267-267, 876-876, 4540-4540
4349-4472: Add a requires-chain coverage to FetchReasons.Current tests validate IsKey. Add a case where a field is included due to @requires to assert is_requires=true in the propagated extension.
Also applies to: 4507-4543
v2/pkg/engine/plan/visitor.go (1)
637-640: Tighten the comment for skipField.Minor doc polish.
-// skipField returns true if the field was added by the query planner as a dependency. -// For another field and should not be included in the response. -// If it returns false, the user requests the field. +// skipField returns true if the field was added by the planner as a dependency +// for another field and should not be included in the response. +// If it returns false, the field was requested by the client.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(4 hunks)v2/pkg/engine/plan/visitor.go(8 hunks)v2/pkg/engine/resolve/fetch.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
execution/engine/execution_engine_test.go (5)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(296-309)DataSourceMetadata(34-56)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-357)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
FetchReason(423-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (3)
v2/pkg/engine/resolve/fetch.go (3)
104-110: FetchReasons API wiring looks correct.All fetch kinds expose FetchReasons through concrete implementations. Good separation and promotion usage.
Also applies to: 279-285, 191-197, 244-247
350-354: Exclude FieldFetchReasons from equality — aligns with intent.Treating reasons as derived metadata (non-semantic) is appropriate for plan caching.
Also applies to: 359-374
421-430: JSON contract for FetchReason is concise and matches tests.Naming (typename, field, by_subgraphs, etc.) is consistent with expected payloads.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
v2/pkg/engine/plan/planner.go (1)
232-244: Minor: pre-size the map; optionally dedup inputsPre-size to reduce rehashing; dedup only if upstream can emit duplicates.
Apply this diff:
-func inverseMap(m map[int][]int) map[int][]int { - inverse := make(map[int][]int) +func inverseMap(m map[int][]int) map[int][]int { + inverse := make(map[int][]int, len(m)) for k, v := range m { for _, v2 := range v { inverse[v2] = append(inverse[v2], k) } }v2/pkg/engine/plan/visitor.go (3)
637-648: Fix comment grammar; keep TODO conciseSmall readability nit; logic can stay linear for now.
Apply this diff:
-// skipField returns true if the field was added by the query planner as a dependency. -// For another field and should not be included in the response. -// If it returns false, the user requests the field. +// skipField returns true if the field was added by the query planner as a dependency +// for another field and should not be included in the response. +// If it returns false, the field was requested by the client. func (v *Visitor) skipField(ref int) bool { - // TODO: If this grows, switch to map[int]struct{} for O(1). + // TODO: If this grows, switch to map[int]struct{} for O(1). for _, skipRef := range v.skipFieldsRefs { if skipRef == ref { return true } } return false }
1358-1396: Avoid parameter shadowing for readabilityInner loop variable fetchID shadows function param; use a distinct name.
Apply this diff:
- for _, fetchID := range depFieldPlanners { // planner index == fetchID - ofc := v.planners[fetchID].ObjectFetchConfiguration() + for _, depPlannerID := range depFieldPlanners { // planner index == fetchID + ofc := v.planners[depPlannerID].ObjectFetchConfiguration() if ofc == nil { continue } origin := resolve.FetchDependencyOrigin{ - FetchID: fetchID, + FetchID: depPlannerID, Subgraph: ofc.sourceName, Coordinate: resolve.GraphCoordinate{ FieldName: v.Operation.FieldNameString(depFieldRef), TypeName: v.fieldEnclosingTypeNames[depFieldRef], }, }
1485-1489: Optional: move per-reason dedup outside the loopSorting/compacting on every merge can be O(k log k) repeatedly. Do a single pass after the loop.
Apply this diff:
- if reasons[i].BySubgraphs != nil { - slices.Sort(reasons[i].BySubgraphs) - reasons[i].BySubgraphs = slices.Compact(reasons[i].BySubgraphs) - } + // Defer sort/compact of BySubgraphs to a final pass.Then, below the loop and before sorting reasons:
- slices.SortFunc(reasons, func(a, b resolve.FetchReason) int { + for i := range reasons { + if reasons[i].BySubgraphs != nil { + slices.Sort(reasons[i].BySubgraphs) + reasons[i].BySubgraphs = slices.Compact(reasons[i].BySubgraphs) + } + } + slices.SortFunc(reasons, func(a, b resolve.FetchReason) int { return cmp.Or( cmp.Compare(a.TypeName, b.TypeName), cmp.Compare(a.FieldName, b.FieldName), ) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
v2/pkg/engine/plan/planner.go(3 hunks)v2/pkg/engine/plan/visitor.go(8 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T09:35:47.950Z
Learnt from: ysmolski
PR: wundergraph/graphql-go-tools#1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.950Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/plan/visitor.go
🧬 Code graph analysis (2)
v2/pkg/engine/plan/visitor.go (1)
v2/pkg/engine/resolve/fetch.go (1)
FetchReason(423-430)
v2/pkg/engine/resolve/resolve.go (1)
v2/pkg/engine/resolve/resolvable.go (1)
ResolvableOptions(62-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (10)
v2/pkg/engine/plan/planner.go (2)
5-5: LGTM: deterministic inverse ordering dependencyImporting sort for normalization is appropriate.
157-158: Wire inverse dependencies into planning visitorGood placement and timing; ensures downstream consumers (e.g., buildFetchReasons) have the inverse relation available before Walk.
v2/pkg/engine/resolve/resolve.go (2)
170-170: Doc tweak LGTMConstructor comment reads well and is accurate.
250-251: No changes needed
Feature flag plumbing and gating verified:propagateFieldFetchReasonsis driven byoptions.PropagateFetchReasonsinResolver→Loader, and fetch reasons are only added whenDataSourceMetadata.RequiresFetchReasonreturns true for@openfed__requireFetchReasonfields.v2/pkg/engine/plan/visitor.go (6)
5-5: LGTM: use cmp for deterministic sort keysGo 1.23+ is supported per project learnings; cmp.Or usage is fine.
50-50: LGTM: explicit inverse mapping on visitorClear naming and purpose; pairs with planner.inverseMap().
59-67: LGTM: tracking planners⇄fields and enclosing type namesThe maps enable later dependency/reason computation with minimal coupling.
343-345: LGTM: record enclosing type early (before skip)
Captures type for both user and planner-added fields; required by dependencies/reasons.
1338-1341: SingleFetch CoordinateDependencies and FieldFetchReasons are defined and consumed; no further action needed.
1401-1499: DS.RequiresFetchReason and fetch_reasons extension verified
Implementation covers both root and child nodes (via hasFetchReasonRootNode/ChildNode) and resolve/loader.go injects thefetch_reasonsextension. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/resolve.go (1)
164-165: Resolved: extension key uses “fetch_reasons” (plural). LGTM.
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/resolve.go (2)
152-154: Fix doc/field name mismatch.Comment says AllowedCustomSubgraphErrorFields but the field is AllowedSubgraphErrorFields. Align to avoid confusion in public API docs.
- // AllowedCustomSubgraphErrorFields defines which fields are allowed in the subgraph error when in passthrough mode + // AllowedSubgraphErrorFields defines which fields are allowed in the subgraph error when in passthrough mode
164-167: Clarify directive gating in the option’s doc.Explicitly note that only fields annotated with @openfed__requireFetchReason are included.
- // PropagateFetchReasons enables adding the "fetch_reasons" extension to - // upstream subgraph requests. This extension explains why each field was requested. - // This flag does not expose the data to clients. + // PropagateFetchReasons enables adding the "fetch_reasons" request extension to + // upstream subgraph requests. The extension explains why each field was requested. + // The router only includes reasons for fields annotated with @openfed__requireFetchReason. + // This flag does not expose the data to clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v2/pkg/engine/resolve/resolve.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/resolve.go (1)
v2/pkg/engine/resolve/resolvable.go (1)
ResolvableOptions(62-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/resolve.go (2)
170-170: Doc comment tweak: New() description looks good.
250-251: Verified Loader propagation flag wiringLoader struct includes the propagateFieldFetchReasons flag, injection into body.extensions.fetch_reasons is correctly gated by that flag (and skips introspection), and no client responses expose fetch_reasons.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/loader.go (1)
1611-1611: Re: “will it work if extensions are not present yet?” — yesjsonparser.Set creates intermediate objects; this path will work when extensions is absent.
execution/engine/execution_engine_test.go (1)
4355-4357: Confirming fix: removed extraneous "some" from second fetch expectationsThis resolves the earlier discrepancy where a field not present in the second fetch was listed in the extension.
🧹 Nitpick comments (7)
v2/pkg/engine/resolve/loader.go (3)
178-180: Field name consistency with optionsConsider renaming enableRequireFetchReasons to propagateFetchReasons to align with ResolverOptions.EnableRequireFetchReasons and the test nomenclature. This reduces cognitive load.
Apply within this file:
- enableRequireFetchReasons bool + propagateFetchReasons boolAnd update references accordingly:
- if l.enableRequireFetchReasons && !IsIntrospectionDataSource(res.ds.ID) { + if l.propagateFetchReasons && !IsIntrospectionDataSource(res.ds.ID) {
1601-1617: Gated fetch_reasons injection looks correct; ensure deterministic ordering to avoid test flakesThe guard and path are right, and jsonparser.Set will create body.extensions if missing. One potential source of flakiness: the order of fetchReasons. If generation isn’t already stable, sort by (typename, field) before marshaling or adjust producer-side ordering to guarantee stability.
Would you like me to add a small sort at the producer (where FetchReasons are built) to guarantee order?
1601-1617: Do not silently overwrite an existing extensions.fetch_reasonsIf upstream middleware already set extensions.fetch_reasons via l.ctx.Extensions, this will overwrite it. If coexistence is desired, merge/append instead or skip when present.
execution/engine/execution_engine_test.go (4)
218-219: Option naming: align with feature namepropagateFieldsRequestedBy → propagateFetchReasons for consistency with extensions.fetch_reasons and ResolverOptions.EnableRequireFetchReasons.
- propagateFieldsRequestedBy bool + propagateFetchReasons bool
263-268: Correct wiring of the flag into ResolverOptionsEnableRequireFetchReasons uses the test option as intended. After the suggested rename, adjust the mapping.
- EnableRequireFetchReasons: opts.propagateFieldsRequestedBy, + EnableRequireFetchReasons: opts.propagateFetchReasons,
827-879: Great positive path coverage for fetch_reasonsTest validates injection and expected shape. If FetchReasons ordering isn’t guaranteed upstream, this exact string assertion could become brittle. Consider normalizing JSON in the round-tripper for body equality or sorting fetchReasons before encode.
4349-4472: makeDataSource: expectations and metadata look coherent; minor robustness nits
- expectedBody1 matches the annotated FetchReasonFields on the first subgraph. Good.
- expectedBody2 intentionally omits fetch_reasons; matches metadata (no FetchReasonFields). Good.
- To reduce brittleness, prefer comparing JSON semantically (object/array equality) in the test transport rather than raw string order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/resolve.go
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (1)
Fetch(20-26)
execution/engine/execution_engine_test.go (5)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(296-309)DataSourceMetadata(34-56)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-357)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (1)
execution/engine/execution_engine_test.go (1)
4507-4542: Both “with” and “without” extension scenarios covered — niceDual runs verify gating behavior via EnableRequireFetchReasons and expected request bodies.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/loader.go (1)
1611-1611: Re: “Will it work if extensions are not present yet?”—yesThis path creates/mutates extensions as needed; tests confirm it. Nothing to change.
🧹 Nitpick comments (3)
v2/pkg/engine/resolve/loader.go (2)
33-37: Hoist extension key to a package constant to avoid driftUse a single constant for the "fetch_reasons" key to keep parity across files/tests and prevent typos.
@@ const ( IntrospectionSchemaTypeDataSourceID = "introspection__schema&__type" IntrospectionTypeFieldsDataSourceID = "introspection__type__fields" IntrospectionTypeEnumValuesDataSourceID = "introspection__type__enumValues" ) +// request extension keys +const extensionKeyFetchReasons = "fetch_reasons" @@ - input, res.err = jsonparser.Set(input, encoded, "body", "extensions", "fetch_reasons") + input, res.err = jsonparser.Set(input, encoded, "body", "extensions", extensionKeyFetchReasons)Also applies to: 1601-1617
1594-1600: Double-check non-object extensions inputIf ctx.Extensions is present but not an object (e.g., "null" or a scalar), jsonparser.Set on body.extensions.fetch_reasons may behave unexpectedly. Consider normalizing extensions to {} when non-object.
Would you like a small helper to coerce body.extensions to an object before adding keys?
Also applies to: 1601-1617
execution/engine/execution_engine_test.go (1)
827-827: Tighten test nameConsider renaming to: “execute simple hero operation with fetch reason propagation”.
- t.Run("execute simple hero operation with propagating to subgraphs reason for fields being requested", runWithoutError( + t.Run("execute simple hero operation with fetch reason propagation", runWithoutError(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
v2/pkg/engine/resolve/resolve.go (1)
v2/pkg/engine/resolve/resolvable.go (1)
ResolvableOptions(62-67)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (1)
Fetch(20-26)
execution/engine/execution_engine_test.go (5)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(296-309)DataSourceMetadata(34-56)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-357)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (6)
v2/pkg/engine/resolve/loader.go (1)
1601-1617: fetch_reasons injection logic looks correct and safely gated
- Gated by option and introspection IDs.
- Only sets when reasons are non-empty.
- Proper error propagation via res.err.
v2/pkg/engine/resolve/resolve.go (1)
164-168: Option + wiring LGTMPropagateFetchReasons is documented clearly and correctly wired through newTools into Loader.
Also applies to: 250-251
execution/engine/execution_engine_test.go (4)
214-219: Test harness flagging/wiring is cleanNew propagateFetchReasons option is plumbed into ResolverOptions correctly.
Also applies to: 262-267
827-879: Solid happy-path coverage for fetch_reasons propagation
- Uses POST to carry extensions.
- Verifies exact payload including reasons for Query.hero and Character.name.
4355-4357: Nested fetch scenario expectations look consistentexpectedBody1/2 match the planned selections, and the second subgraph body omits fetch_reasons appropriately.
Also applies to: 4426-4431
4349-4472: Deterministic ordering confirmed in buildFetchReasons: the finalreasonsslice is sorted viaslices.SortFuncon(TypeName, FieldName), and eachBySubgraphslist is sorted and compacted, guaranteeing stable JSON output.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
execution/engine/execution_engine_test.go (1)
4357-4357: Previous issue about extraneous "some" in expectedBody2 is resolved.expectedBody2 no longer lists unrelated fields; thanks for fixing.
🧹 Nitpick comments (5)
execution/engine/execution_engine_test.go (5)
214-219: Add a dedicated test option for fetch-reason propagation (LGTM, minor ergonomics).The new propagateFetchReasons toggle is fine. Consider adding a small helper like withValueCompletion() to keep call sites consistent.
Proposed helper (place near withValueCompletion):
func withPropagateFetchReasons() executionTestOptions { return func(o *_executionTestOptions) { o.propagateFetchReasons = true } }
827-874: Positive test for extension payload is precise and scoped (POST + targeted FetchReasonFields).
- The expected body matches the POST method and only includes fields whitelisted via FetchReasonFields.
- Nice: both root (Query.hero) and child (Character.name) reasons are asserted.
Potential flake guard: comparing the outbound body structurally (JSON) rather than byte-for-byte would reduce brittleness if field order changes.
875-878: Prefer a helper to set the option.Minor consistency win with withValueCompletion().
- func(eto *_executionTestOptions) { - eto.propagateFetchReasons = true - }, + withPropagateFetchReasons(),Add the helper:
func withPropagateFetchReasons() executionTestOptions { return func(o *_executionTestOptions) { o.propagateFetchReasons = true } }
4349-4472: De-duplicate "id-2" and make expectedBody1 resilient to DS id drift.Hardcoding id-2 in the embedded JSON is easy to desync from the DS id below. Suggest deriving it once and formatting into the JSON.
- makeDataSource := func(t *testing.T, expectFetchReasons bool) []plan.DataSource { + makeDataSource := func(t *testing.T, expectFetchReasons bool) []plan.DataSource { + const secondID = "id-2" var expectedBody1 string var expectedBody2 string if !expectFetchReasons { expectedBody1 = `{"query":"{accounts {__typename ... on User {some {__typename id}} ... on Admin {some {__typename id}}}}"}` } else { - expectedBody1 = `{"query":"{accounts {__typename ... on User {some {__typename id}} ... on Admin {some {__typename id}}}}","extensions":{"fetch_reasons":[{"typename":"Admin","field":"some","by_user":true},{"typename":"User","field":"id","by_subgraphs":["id-2"],"by_user":true,"is_key":true}]}}` + expectedBody1 = fmt.Sprintf(`{"query":"{accounts {__typename ... on User {some {__typename id}} ... on Admin {some {__typename id}}}}","extensions":{"fetch_reasons":[{"typename":"Admin","field":"some","by_user":true},{"typename":"User","field":"id","by_subgraphs":["%s"],"by_user":true,"is_key":true}]}}`, secondID) } expectedBody2 = `{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on User {__typename title}}}","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"3"}]}}` return []plan.DataSource{ mustGraphqlDataSourceConfiguration(t, "id-1", @@ - mustGraphqlDataSourceConfiguration(t, - "id-2", + mustGraphqlDataSourceConfiguration(t, + secondID,
4507-4542: Extension-enabled run is correctly gated by test option and DS expectations.Nice separation via makeDataSource(t, true) and the option toggle.
Consider switching to withPropagateFetchReasons() for symmetry with other option helpers (see prior suggestion).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
execution/engine/execution_engine_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
execution/engine/execution_engine_test.go (6)
v2/pkg/starwars/starwars.go (2)
FileSimpleHeroQuery(13-13)Schema(55-59)v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(296-309)DataSourceMetadata(34-56)v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)v2/pkg/engine/datasource/graphql_datasource/configuration.go (4)
ConfigurationInput(16-23)FetchConfiguration(124-128)SchemaConfiguration(135-139)FederationConfiguration(130-133)v2/pkg/engine/resolve/fetch.go (2)
Fetch(20-26)FetchConfiguration(314-357)v2/pkg/engine/plan/configuration.go (1)
FieldConfiguration(101-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (3)
execution/engine/execution_engine_test.go (3)
262-267: Wiring PropagateFetchReasons into ResolverOptions looks correct.Good plumb-through; default remains false for all other tests.
Please confirm there are no unintended extensions added in tests that don’t enable the option (a quick grep for "fetch_reasons" in round-tripper expectations should only hit the two new cases).
844-857: FetchReasonFields are minimal and accurate.Limiting to hero and name avoids leaking reasons on non-annotated fields.
4503-4504: Baseline run without extension uses makeDataSource(..., false) — good control.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/loader.go (2)
178-180: Name the intent in a doc comment for clarityA short comment helps future readers understand when and how this flag affects outbound requests.
apolloRouterCompatibilitySubrequestHTTPError bool - - propagateFetchReasons bool + // If true, injects per-fetch selection reasons into the subgraph request as body.extensions.fetch_reasons. + // Only applied to non-introspection data sources. + propagateFetchReasons bool
1601-1617: Propagation logic looks good; consider minor polish for maintainabilityThe gating and error handling are sound. Two tiny nits to reduce footguns.
- Use a named const for the extension key to avoid typos across the codebase (and to centralize the wire contract).
- Inline the marshal assignment to avoid a predeclared var.
Diff within this hunk:
- if l.propagateFetchReasons && !IsIntrospectionDataSource(res.ds.ID) { + if l.propagateFetchReasons && !IsIntrospectionDataSource(res.ds.ID) { fetchReasons := fetchItem.Fetch.FetchReasons() if len(fetchReasons) > 0 { - var encoded []byte - encoded, res.err = json.Marshal(fetchReasons) + encoded, res.err := json.Marshal(fetchReasons) if res.err != nil { res.err = errors.WithStack(res.err) return } // We expect that body.extensions is an object - input, res.err = jsonparser.Set(input, encoded, "body", "extensions", "fetch_reasons") + input, res.err = jsonparser.Set(input, encoded, "body", "extensions", fetchReasonsExtensionKey) if res.err != nil { res.err = errors.WithStack(res.err) return } } }Add once near your other consts:
const fetchReasonsExtensionKey = "fetch_reasons"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/engine/execution_engine_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (1)
Fetch(20-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
ysmolski
deleted the
yury/eng-7769-fields-authorization-for-subgraph-access
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.225](v2.0.0-rc.224...v2.0.0-rc.225) (2025-09-09) ### Features * add support of field selection reasons extensions ([#1282](#1282)) ([37c9582](37c9582)) * upgrade all components to go 1.25 ([#1289](#1289)) ([6bd2713](6bd2713)) ### Bug Fixes * refactor CoordinateDependencies, FetchReasons ([#1293](#1293)) ([cfebc16](cfebc16)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for field selection reasons extensions. * Upgraded all components to Go 1.25 for improved compatibility and performance. * **Chores** * Bumped release version to 2.0.0-rc.225. * **Documentation** * Updated changelog with a new 2.0.0-rc.225 entry detailing recent features. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
The PropagateFetchReasons option of Resolver enables router to send to subgraphs an extension that includes reasons why fields where included in the request.
This data is sent only for the fields marked with
@openfed__requireFetchReasonin the schema.Directive is implemented in the router repo.
Router part: wundergraph/cosmo#2160
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
Chores
Checklist