Conversation
# Conflicts: # execution/engine/execution_engine_test.go # v2/pkg/engine/plan/visitor.go
…l-requires-dependencies
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughAdds a configuration flag to validate nullable external @requires, propagates it through planner and resolver, records Nullable on fetch reasons, implements taint-tracking in the loader to skip entities whose required fields failed, forces __typename emission in nested selections when enabled, and updates/extends tests and test helpers for these behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 5
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)
1422-1450: Nullable derivation can silently miscompute when the field isn’t foundfieldDefinitionRef may return 0 on miss (depending on upstream API), which passes the ast.InvalidRef check and can read the wrong type, producing incorrect Nullable. Use the boolean “ok” and return InvalidRef when false.
Apply this diff:
- var nullable bool - fieldDefRef := v.fieldDefinitionRef(typeName, fieldName) - if fieldDefRef != ast.InvalidRef { - typeRef := v.Definition.FieldDefinitionType(fieldDefRef) - nullable = !v.Definition.TypeIsNonNull(typeRef) - } + var nullable bool + fieldDefRef := v.fieldDefinitionRef(typeName, fieldName) + if fieldDefRef != ast.InvalidRef { + typeRef := v.Definition.FieldDefinitionType(fieldDefRef) + nullable = !v.Definition.TypeIsNonNull(typeRef) + }(and fix fieldDefinitionRef below).
🧹 Nitpick comments (12)
v2/pkg/engine/plan/node_selection_visitor.go (1)
50-51: Name/intent clarity for new flag.
enforceTypenameForRequiredis clear, but it overlaps conceptually withallowTypenameused elsewhere. Consider a short comment explaining how it differs fromallowTypenameto prevent misuse.- enforceTypenameForRequired bool + // enforceTypenameForRequired forces __typename injection only for @requires-driven additions + // (distinct from allowTypename used along key paths). + enforceTypenameForRequired boolv2/pkg/engine/resolve/fetch.go (1)
371-372: Keep Nullable internal-only (do not serialize) or opt-in explicitlyPropagatedFetchReasons are JSON-marshaled into the "fetch_reasons" extension (loader.go) but FetchReason.Nullable has
json:"-"so it is not sent to subgraphs — leave as-is if this is a planner-only hint; otherwise change the tag and update tests.type FetchReason struct { TypeName string `json:"typename"` FieldName string `json:"field"` BySubgraphs []string `json:"by_subgraphs,omitempty"` ByUser bool `json:"by_user,omitempty"` IsKey bool `json:"is_key,omitempty"` IsRequires bool `json:"is_requires,omitempty"` - Nullable bool `json:"-"` + // Nullable is derived from the field's AST type. It is intentionally not serialized + // into the "fetch_reason" extension and is used internally for planning/analysis. + Nullable bool `json:"-"` }v2/pkg/engine/plan/required_fields_visitor.go (1)
49-51: Doc tweak: clarify scope beyond “has inline fragments”The comment says “in the 'requires' key”, but with enforceTypenameForRequired true we also add __typename even when there are no fragments (non-key requires). Consider expanding the comment for accuracy.
v2/pkg/engine/plan/configuration.go (1)
43-48: Guard rail: validate HandleOptionalRequiresDeps requires BuildFetchReasonsThe struct doc states this flag requires BuildFetchReasons=true, but nothing enforces it. Recommend validating during planner/engine construction and either:
- auto-enable BuildFetchReasons when HandleOptionalRequiresDeps is true, or
- return a clear error/log a warning.
This prevents silent no-ops or confusing behavior.
v2/pkg/engine/resolve/loader_test.go (1)
1453-1454: Fix test message formattingThe assert message has a dangling format specifier with no args. It prints literally “%s”.
- assert.Equal(t, tt.expectedIndices, indices, "Tainted indices mismatch: %s") + assert.Equal(t, tt.expectedIndices, indices, "Tainted indices mismatch")execution/engine/execution_engine_helpers_test.go (2)
69-93: Assert HTTP method in conditional round-tripperconditionalTestCase includes expectedMethod but it’s not asserted. Add the check to catch accidental GET/POST regressions.
assert.Equal(t, testCase.expectedHost, req.URL.Host) assert.Equal(t, testCase.expectedPath, req.URL.Path) + if testCase.expectedMethod != "" { + assert.Equal(t, testCase.expectedMethod, req.Method) + }
55-67: Optional: reduce brittleness of body-keyed responsesKeying the response map by the raw request body is fragile (whitespace/order). If this becomes flaky, parse/canonicalize the JSON before map lookup.
execution/engine/execution_engine_test.go (3)
279-286: Ensure option coupling in testsThe tests correctly set both BuildFetchReasons and HandleOptionalRequiresDeps. Add a defensive guard to fail fast if someone enables HandleOptionalRequiresDeps without withFetchReasons() in future edits.
- engineConf.plannerConfig.BuildFetchReasons = opts.propagateFetchReasons - engineConf.plannerConfig.HandleOptionalRequiresDeps = opts.handleOptionalRequiresDeps + engineConf.plannerConfig.BuildFetchReasons = opts.propagateFetchReasons + engineConf.plannerConfig.HandleOptionalRequiresDeps = opts.handleOptionalRequiresDeps + if opts.handleOptionalRequiresDeps && !opts.propagateFetchReasons { + t.Fatalf("HandleOptionalRequiresDeps requires BuildFetchReasons (use withFetchReasons())") + }
4589-4601: Body-equality routing can be brittleThese conditional responses rely on exact body matches. If planner output formatting changes (field order/spacing), tests may fail spuriously. Consider switching the round-tripper to compare canonicalized JSON bodies.
5267-5277: Reuse method assertion for conditional clientPass expectedMethod in callers (e.g., "POST") and assert it inside the round-tripper (see helpers change). Keeps HTTP semantics explicit in these federation tests.
v2/pkg/engine/resolve/loader.go (2)
1096-1105: Fix known bug: integer segments in error path rewritingUse type-aware conversion when rebuilding paths to avoid empty strings for numbers.
Apply this diff:
- for j := i + 1; j < len(pathItems); j++ { - // BUG: for pathItems containing integers, it will append empty strings - newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) - } + for j := i + 1; j < len(pathItems); j++ { + switch pathItems[j].Type() { + case astjson.TypeString: + newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + case astjson.TypeNumber: + newPath = append(newPath, strconv.Itoa(pathItems[j].GetInt())) + } + }
619-644: Optional @requires nulls without errors won't taint — add a feature-gated scan of _entities for nulls.loader.go only computes taintedIndices when subgraph errors are present (res.postProcessing.SelectResponseErrorsPath → getTaintedIndices), so a subgraph returning null for a requested nullable @requires field without emitting an error will not mark the entity tainted; consider a follow-up that scans responseData._entities[*] for nulls of requested nullable @requires fields (feature-gated).
Location: v2/pkg/engine/resolve/loader.go (error handling / getTaintedIndices call).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
execution/engine/execution_engine_helpers_test.go(1 hunks)execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go(5 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/node_selection_builder.go(1 hunks)v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)v2/pkg/engine/plan/required_fields_visitor.go(2 hunks)v2/pkg/engine/plan/required_fields_visitor_test.go(1 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/resolve/fetch.go(1 hunks)v2/pkg/engine/resolve/loader.go(16 hunks)v2/pkg/engine/resolve/loader_test.go(2 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
v2/pkg/engine/plan/required_fields_visitor_test.go (1)
v2/pkg/engine/plan/required_fields_visitor.go (2)
RequiredFieldsFragment(19-27)QueryPlanRequiredFieldsFragment(29-38)
v2/pkg/engine/plan/visitor.go (2)
v2/pkg/engine/resolve/fetch.go (1)
FetchDependency(336-344)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)
v2/pkg/engine/resolve/loader_test.go (2)
v2/pkg/engine/resolve/fetch.go (5)
FetchReason(364-372)FetchInfo(376-397)FetchKind(11-11)FetchKindSingle(14-14)FetchDependencies(111-114)v2/pkg/engine/resolve/loader.go (1)
Loader(158-186)
execution/engine/execution_engine_test.go (3)
execution/engine/execution_engine.go (1)
NewExecutionEngine(112-148)v2/pkg/engine/resolve/resolve.go (1)
ResolverOptions(92-170)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
DefaultNetHttpClient(39-45)
v2/pkg/engine/resolve/loader.go (4)
v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(34-42)v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (3)
Fetch(20-27)FetchInfo(376-397)GraphCoordinate(399-403)
⏰ 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.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (18)
v2/pkg/engine/resolve/resolve.go (2)
253-254: Propagation to Loader acknowledged — default behavior preserved.handleOptionalRequiresDeps is only read to gate getTaintedIndices; when false no tainted indices are computed/applied and ignoreTainted is a no-op for an empty map, so error semantics are unchanged.
169-170: Public API addition: check for unkeyed struct literals. Adding HandleOptionalRequiresDeps to ResolverOptions is source-compatible for keyed literals but will break any unkeyed composite (positional) literals. Sweep the repo and examples for unkeyed ResolverOptions usages and convert them to keyed form (FieldName: value). Quick check: rg -n --type=go 'ResolverOptions\s*{' and inspect each match for entries without explicit keys.v2/pkg/engine/plan/node_selection_visitor.go (1)
465-475: Resolved — enforceTypenameForRequired is honored when allowTypename=falseaddRequiredFields builds the fragment with config.allowTypename (RequiredFieldsFragment at required_fields_visitor.go:62) but still calls addTypenameSelection when (config.enforceTypenameForRequired && !config.isKey), so __typename is added for non-key requires even if allowTypename is false (see required_fields_visitor.go around lines 156–160).
v2/pkg/engine/plan/node_selection_builder.go (1)
55-57: Confirmed — flag only affects 'requires' additions, not keys or regular selections.enforceTypenameForRequired is only used by addRequiredFields (v2/pkg/engine/plan/required_fields_visitor.go); the code adds __typename only when keySelectionSetHasFragments OR (enforceTypenameForRequired && !isKey). node_selection_builder.go wires the flag from config.HandleOptionalRequiresDeps.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go (4)
3139-3179: LGTM: FetchReason.Nullable matches SDL nullability for user/account/address
- Query.user (definition shows user: User), User.account (account: Account), and Account.address (address: Address) are all nullable in the SDL here, so marking these fetch reasons as Nullable: true is correct and prevents false negatives when entities are missing.
3271-3290: LGTM: Account.address reason marked nullableAddress is nullable on Account in the SDL; the added Nullable: true aligns with that.
3426-3446: LGTM: Consistent nullability on address fetch reasonSame rationale as above; good consistency across steps of the requires chain.
3580-3591: LGTM: Final hop keeps Account.address nullableKeeps behavior consistent across the pipeline; expected.
v2/pkg/engine/resolve/loader_test.go (1)
1153-1241: Nice coverage for entity index extractionGood table tests across mixed paths, missing keys, negatives, and complex cases.
Consider adding one case where the path crosses an object with a non-array numeric key (e.g., {"1": {...}}) to assert it doesn’t get treated as an array index.
v2/pkg/engine/plan/required_fields_visitor_test.go (3)
13-33: Solid table-driven coverage; asserts target the right internal countersThe suite meaningfully exercises keys vs. requires, nested selections, alias remapping, and typename enforcement. Good use of astprinter normalization for structural equality.
168-199: Alias remap expectation looks correctThe remap path assertion for conflicting arguments matches the internal aliasing contract. No issues spotted.
376-429: Typename enforcement cases cover both key and requiresGood distinction: no __typename for keys even with enforcement; added for requires on nested objects. This aligns with the intended wiring.
v2/pkg/engine/plan/visitor.go (2)
1342-1342: Switched to buildFetchDependencies — OKThe rename and call site update are consistent. No behavior concerns here.
1352-1364: Propagating only required fetch reasons — OK; minor micro-nitLogic is sound. Minor: skip building the propagated slice when lookup is empty to avoid a loop.
[raise_nitpick_issue]v2/pkg/engine/resolve/loader.go (4)
181-187: Good: taint tracking state kept per LoaderFields and map type are appropriate for pointer identity to entity objects.
195-201: Initialize taint map per executionCorrect lifecycle placement.
590-617: Error handling flow is consistent with Apollo compatibility flagsHasErrors gating and later value-completion suppression are coherent. No change required.
1147-1148: Good: dedicated reason for missing @requires dependenciesClear message; pairs well with the taint logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/loader.go (2)
367-381: Prune tainted items after every traversal step to prevent child leaks (still pending)Currently pruning only at start (empty path) and once at the end allows children of tainted parents to slip through during intermediate traversals.
Apply this diff:
func (l *Loader) selectItemsForPath(path []FetchItemPathElement) []*astjson.Value { - items := []*astjson.Value{l.resolvable.data} - if len(path) == 0 { - items = l.ignoreTainted(items) - return items - } + items := []*astjson.Value{l.resolvable.data} + if len(path) == 0 { + return l.ignoreTainted(items) + } for i := range path { if len(items) == 0 { break } items = l.selectItems(items, path[i]) + items = l.ignoreTainted(items) } - items = l.ignoreTainted(items) return items }
835-867: Array traversal bug in extractEntityIndex (indexes treated as object keys)Numeric path elements must index arrays, not call Get("idx"). Current code returns nil for arrays and breaks taint detection.
Apply this diff:
-func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { - index := -1 - if len(path) == 0 { - return nil, index - } - for _, el := range path { - var key string - switch el.Type() { - case astjson.TypeNumber: - parsed := el.GetInt() - if parsed < 0 { - return nil, index - } - if index == -1 { - // index is assigned only once - index = parsed - } - key = strconv.Itoa(parsed) - case astjson.TypeString: - key = unsafebytes.BytesToString(el.GetStringBytes()) - default: - return nil, -1 - } - response = response.Get(key) - if response == nil { - return nil, -1 - } - } - return response, index -} +func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { + idx := -1 + if len(path) == 0 || response == nil { + return nil, idx + } + cur := response + for _, el := range path { + switch el.Type() { + case astjson.TypeNumber: + i := el.GetInt() + if i < 0 || cur.Type() != astjson.TypeArray { + return nil, idx + } + if idx == -1 { + idx = i + } + arr := cur.GetArray() + if i >= len(arr) { + return nil, idx + } + cur = arr[i] + case astjson.TypeString: + key := unsafebytes.BytesToString(el.GetStringBytes()) + cur = cur.Get(key) + if cur == nil { + return nil, idx + } + default: + return nil, idx + } + } + return cur, idx +}
🧹 Nitpick comments (3)
v2/pkg/engine/resolve/loader.go (3)
655-657: Guard single-item tainting by response shapeIndex 0 only makes sense when responseData is an array (e.g., _entities). Add a shape check.
Apply this diff:
- if slices.Contains(taintedIndices, 0) { + if responseData.Type() == astjson.TypeArray && slices.Contains(taintedIndices, 0) { l.taintedEntities[items[0]] = struct{}{} }
766-833: getTaintedIndices: solid logic; dedupe indices to avoid redundant workFunctionally correct. Consider de-duplicating indices to avoid repeated Contains checks downstream.
Apply this diff:
-func (l *Loader) getTaintedIndices(fetch Fetch, response *astjson.Value, errs *astjson.Value) (indices []int) { +func (l *Loader) getTaintedIndices(fetch Fetch, response *astjson.Value, errs *astjson.Value) (indices []int) { + seen := make(map[int]struct{}) info := fetch.FetchInfo() if info == nil { return nil } // build a map to search with requestedForRequires := map[GraphCoordinate]struct{}{} @@ - indices = append(indices, index) + if _, ok := seen[index]; !ok { + indices = append(indices, index) + seen[index] = struct{}{} + } break
1097-1099: Fix BUG in error path rewriting: preserve numeric indicesWhen rewriting paths, integers are converted to empty strings. Build a []interface{} mixing strings and ints before marshaling.
Apply this diff:
- // BUG: for pathItems containing integers, it will append empty strings - newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + var newPathAny []interface{} + newPathAny = make([]interface{}, 0, len(pathPrefix)+len(pathItems)-i-1) + for _, s := range pathPrefix { + newPathAny = append(newPathAny, s) + } + for j := i + 1; j < len(pathItems); j++ { + switch pathItems[j].Type() { + case astjson.TypeNumber: + newPathAny = append(newPathAny, pathItems[j].GetInt()) + default: + newPathAny = append(newPathAny, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + } + } - newPathJSON, _ := json.Marshal(newPath) + newPathJSON, _ := json.Marshal(newPathAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/plan/required_fields_visitor.go(2 hunks)v2/pkg/engine/plan/visitor.go(7 hunks)v2/pkg/engine/resolve/loader.go(16 hunks)v2/pkg/engine/resolve/loader_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/pkg/engine/resolve/loader_test.go
- v2/pkg/engine/plan/required_fields_visitor.go
- v2/pkg/engine/plan/visitor.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (3)
v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(34-42)v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (3)
Fetch(20-27)FetchInfo(376-397)GraphCoordinate(399-403)
⏰ 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.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (5)
v2/pkg/engine/resolve/loader.go (5)
181-186: Taint tracking lifecycle looks correctState is initialized per request and cleared in Free(); no leaks.
Also applies to: 192-193, 199-200
383-396: ignoreTainted: OKSimple and cheap filter; fits the hot path.
680-687: Batch taint marking: LGTMIndex-based taint mapping aligns with batchStats semantics.
Also applies to: 703-705
1148-1149: Clear, user-facing error messageThe new constant is concise and accurate.
181-186: Verify HandleOptionalRequiresDeps wiring and test coverage
- Wiring exists end‑to‑end: plan/configuration.go (HandleOptionalRequiresDeps), plan/node_selection_builder.go (uses config.HandleOptionalRequiresDeps), v2/pkg/engine/resolve/resolve.go (passes options.HandleOptionalRequiresDeps into resolver/loader), v2/pkg/engine/resolve/loader.go (handleOptionalRequiresDeps field + usage when hasErrors && postProcessing.SelectResponseDataPath != nil).
- Tests: execution/engine/execution_engine_test.go defines withHandleOptionalRequiresDeps() and multiple tests exercise the enabled path (helper at ~line 241; usages around ~lines 279, 4884, 5050, 5251).
- Action: confirm there are explicit tests asserting the disabled behavior (flag unset); add tests if missing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/pkg/engine/resolve/loader.go (2)
684-699: Batch+dedup: defer taint marking until after merges per itemSame pointer-stability concern. Track a per-item taint flag and mark after the last merge for that item.
Apply this diff:
- for i, stats := range res.batchStats { - for _, idx := range stats { + for i, stats := range res.batchStats { + taintedThis := false + for _, idx := range stats { if idx == -1 { continue } - if slices.Contains(taintedIndices, idx) { - l.taintedEntities[items[i]] = struct{}{} - } items[i], _, err = astjson.MergeValuesWithPath(items[i], batch[idx], res.postProcessing.MergePath...) if err != nil { return errors.WithStack(ErrMergeResult{ Subgraph: res.ds.Name, Reason: err, Path: fetchItem.ResponsePath, }) } + if !taintedThis && slices.Contains(taintedIndices, idx) { + taintedThis = true + } } + if taintedThis { + l.taintedEntities[items[i]] = struct{}{} + } }
706-718: Non-dedup batch: capture returned value and mark after mergeYou ignore the returned pointer and mark before merging. Reassign items[i] and mark after to keep taint filtering effective.
Apply this diff:
- for i, item := range items { - if slices.Contains(taintedIndices, i) { - l.taintedEntities[item] = struct{}{} - } - _, _, err = astjson.MergeValuesWithPath(item, batch[i], res.postProcessing.MergePath...) + for i := range items { + items[i], _, err = astjson.MergeValuesWithPath(items[i], batch[i], res.postProcessing.MergePath...) if err != nil { return errors.WithStack(ErrMergeResult{ Subgraph: res.ds.Name, Reason: err, Path: fetchItem.ResponsePath, }) } + if slices.Contains(taintedIndices, i) { + l.taintedEntities[items[i]] = struct{}{} + } }
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/loader.go (2)
367-381: Prune tainted after every traversal step to prevent child leaksCalling ignoreTainted only at the end lets children of tainted parents slip through on deeper paths. Prune immediately after each selectItems step and early-break when empty.
Apply this diff:
func (l *Loader) selectItemsForPath(path []FetchItemPathElement) []*astjson.Value { items := []*astjson.Value{l.resolvable.data} if len(path) == 0 { - items = l.ignoreTainted(items) - return items + return l.ignoreTainted(items) } for i := range path { if len(items) == 0 { break } items = l.selectItems(items, path[i]) + items = l.ignoreTainted(items) + if len(items) == 0 { + break + } } - items = l.ignoreTainted(items) return items }
844-877: Array traversal bug in extractEntityIndex: index arrays, don’t Get("idx")Numeric path elements must index into arrays; converting to string and calling Get("1") is incorrect/fragile.
Apply this diff:
-func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { - index := -1 - if len(path) == 0 { - return nil, index - } - for _, el := range path { - var key string - switch el.Type() { - case astjson.TypeNumber: - parsed := el.GetInt() - if parsed < 0 { - return nil, index - } - if index == -1 { - // index is assigned only once - index = parsed - } - key = strconv.Itoa(parsed) - case astjson.TypeString: - key = unsafebytes.BytesToString(el.GetStringBytes()) - default: - return nil, -1 - } - response = response.Get(key) - if response == nil { - return nil, -1 - } - } - return response, index -} +func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { + index := -1 + if len(path) == 0 || response == nil { + return nil, index + } + for _, el := range path { + switch el.Type() { + case astjson.TypeNumber: + i := el.GetInt() + if i < 0 || response.Type() != astjson.TypeArray { + return nil, index + } + if index == -1 { + index = i + } + arr := response.GetArray() + if i >= len(arr) { + return nil, index + } + response = arr[i] + case astjson.TypeString: + key := unsafebytes.BytesToString(el.GetStringBytes()) + response = response.Get(key) + if response == nil { + return nil, index + } + default: + return nil, index + } + } + return response, index +}
🧹 Nitpick comments (9)
v2/pkg/engine/resolve/loader_test.go (2)
1153-1249: Safer string quoting for JSON path elementsBuilding JSON strings via concatenation can break with special characters. Use json.Marshal (or strconv.Quote) to ensure correct escaping.
- case string: - path[i] = astjson.MustParse(`"` + v + `"`) + case string: + // ensure proper JSON string escaping + b, _ := json.Marshal(v) + path[i] = astjson.MustParse(string(b))
1250-1463: Harden assertions and align with production behavior
- getTaintedIndices order isn’t guaranteed; assert order-insensitively to avoid flaky tests.
- Avoid nil vs empty slice brittleness when expecting “no taints”.
- Tests exercise optional @requires logic; mirror production by enabling the loader flag for clarity.
- loader := &Loader{} + loader := &Loader{} + // mirror production behavior when testing optional @requires handling + loader.handleOptionalRequiresDeps = true @@ - indices := loader.getTaintedIndicesAndCleanErrors(mockFetch, response, errors) - - assert.Equal(t, tt.expectedIndices, indices, "Tainted indices mismatch: %s") + indices := loader.getTaintedIndicesAndCleanErrors(mockFetch, response, errors) + // order-insensitive and tolerant to nil vs empty + if tt.expectedIndices == nil { + assert.Empty(t, indices, "Expected no tainted indices") + } else { + assert.ElementsMatch(t, tt.expectedIndices, indices, "Tainted indices mismatch") + }Optional: also assert that errors are cleaned as expected in positive-taint cases. I can add that if you want.
execution/engine/execution_engine_test.go (5)
279-286: Ensure fetch reasons are built when optional @requires handling is enabledOptional @requires handling depends on fetch reasons being available. Consider enabling BuildFetchReasons when either propagateFetchReasons or handleOptionalRequiresDeps is set to avoid silent misconfiguration in future tests.
- engineConf.plannerConfig.BuildFetchReasons = opts.propagateFetchReasons + engineConf.plannerConfig.BuildFetchReasons = opts.propagateFetchReasons || opts.handleOptionalRequiresDeps
4560-4718: Make conditional round-tripper robust to field order in printed queriesThe key
{"query":"{accounts {id __typename}}"}is brittle; the planner may print__typenamebeforeid. Either (a) normalize/parse the request JSON in createConditionalTestRoundTripper, or (b) accept both orders here.responses: map[string]sendResponse{ - `{"query":"{accounts {id __typename}}"}`: { + `{"query":"{accounts {id __typename}}"}`: { statusCode: 200, body: `{"data":{"accounts":[{"__typename":"User","id":"1"},{"__typename":"User","id":"3"}]}}`, }, + // tolerate alternative field order + `{"query":"{accounts {__typename id}}"}`: { + statusCode: 200, + body: `{"data":{"accounts":[{"__typename":"User","id":"1"},{"__typename":"User","id":"3"}]}}`, + },If you prefer, I can update the round-tripper to JSON-parse and compare query ASTs/normalized JSON to avoid string exact matches.
4720-4885: Apply same order-tolerance to this testDuplicate the “__typename id” variant (or improve the round-tripper to be order-insensitive) for the first accounts fetch.
4887-5051: Apply same order-tolerance to this testFirst accounts fetch uses
{id __typename}; accept{__typename id}as well.
5053-5252: Apply same order-tolerance to this testFirst accounts fetch uses
{id __typename}; accept{__typename id}as well.v2/pkg/engine/resolve/loader.go (2)
1079-1116: Rewrite error paths: preserve numeric segmentsThe BUG comment is valid. Build []any and copy strings as strings and numbers as ints.
Apply this diff:
- // rewrite the path to pathPrefix + pathItems after _entities - newPath := make([]string, 0, len(pathPrefix)+len(pathItems)-i) - newPath = append(newPath, pathPrefix...) - for j := i + 1; j < len(pathItems); j++ { - // BUG: for pathItems containing integers, it will append empty strings - newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) - } - newPathJSON, _ := json.Marshal(newPath) + // rewrite the path to pathPrefix + pathItems after _entities, preserving numbers + newPath := make([]any, 0, len(pathPrefix)+len(pathItems)-i) + for _, seg := range pathPrefix { + newPath = append(newPath, seg) + } + for j := i + 1; j < len(pathItems); j++ { + switch pathItems[j].Type() { + case astjson.TypeNumber: + newPath = append(newPath, pathItems[j].GetInt()) + case astjson.TypeString: + newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + } + } + newPathJSON, _ := json.Marshal(newPath)
659-663: Optional: speed up membership checksIf taintedIndices can grow, precompute a small bool set/map to avoid repeated slices.Contains calls.
Apply this outside-local diff where convenient:
tainted := make(map[int]struct{}, len(taintedIndices)) for _, i := range taintedIndices { tainted[i] = struct{}{} } // then: if _, ok := tainted[idx]; ok { ... }Also applies to: 684-699, 706-713
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/resolve/loader.go(16 hunks)v2/pkg/engine/resolve/loader_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (3)
v2/pkg/engine/resolve/loader_test.go (2)
v2/pkg/engine/resolve/fetch.go (5)
FetchReason(364-372)FetchInfo(376-397)FetchKind(11-11)FetchKindSingle(14-14)FetchDependencies(111-114)v2/pkg/engine/resolve/loader.go (1)
Loader(158-186)
execution/engine/execution_engine_test.go (3)
execution/engine/execution_engine.go (1)
NewExecutionEngine(112-148)v2/pkg/engine/resolve/resolve.go (1)
ResolverOptions(92-170)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
DefaultNetHttpClient(39-45)
v2/pkg/engine/resolve/loader.go (2)
v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (4)
Fetch(20-27)FetchInfo(376-397)GraphCoordinate(399-403)FetchItem(29-34)
⏰ 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). (1)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (14)
v2/pkg/engine/resolve/loader_test.go (2)
6-6: LGTM: importsUsing fmt and astjson here makes sense for path construction and JSON handling in these tests.
Also applies to: 13-14
1465-1480: LGTM: minimal mock fetchSimple mock with FetchInfo suffices for exercising getTaintedIndicesAndCleanErrors.
execution/engine/execution_engine_test.go (3)
222-223: LGTM: test option addedhandleOptionalRequiresDeps on the test options struct is clear and scoped.
241-246: LGTM: helper for enabling optional @requireswithHandleOptionalRequiresDeps is consistent with the existing options style.
5267-5277: Helper definitions present — no action requiredcreateConditionalTestRoundTripper, conditionalTestCase, and sendResponse are defined in execution/engine/execution_engine_helpers_test.go, so testConditionalNetHttpClient has the required helpers.
v2/pkg/engine/resolve/loader.go (9)
192-193: Lifecycle reset LGTMtaintedEntities is cleared on Free() and reinitialized per response. Looks good.
Also applies to: 199-200
383-396: ignoreTainted is fine, but depends on stable pointersGiven pointer-based keys, ensure you add the post-merge pointers to taintedEntities (see diffs around merging blocks).
Would you like me to add a small invariant test to assert that tainted items are pruned on nested paths?
584-590: Response data selection LGTMThe SelectResponseDataPath handling reads cleanly and is used consistently below.
593-619: Nice: drop optional-@requires errors in-place and still add a single generic errorThis preserves unrelated subgraph errors. Good balance.
878-951: mergeErrors signature simplification LGTMConsolidating the values parameter cleans up call sites.
1158-1159: Constant message LGTMmissingRequiresDependencies reads clearly in user-facing errors.
536-566: Error-path handling overallThe status-code fallbacks and invalid shape handling are consistent with prior behavior. No issues spotted.
Also applies to: 568-573
770-842: In-place error filtering — astjson.Value.Del supports numeric-string array indices (OK)Verified: astjson.Value.Del accepts decimal-string indexes and removes the array element (shifting remaining items); the current reverse-order deletion (errors.Del(strconv.Itoa(errorIdx))) is safe — no array-rebuild required. (pkg.go.dev)
367-381: Confirm MergeValuesWithPath semantics (in-place vs returning new pointer)Some call sites reassign the returned Value while others ignore it — verify astjson.MergeValuesWithPath's contract and make usage consistent (prefer reassigning the returned value unless the function is guaranteed to mutate the first arg in-place).
Locations: v2/pkg/engine/resolve/loader.go:662, 691, 710, 1318; v2/pkg/engine/resolve/resolvable.go:137, 144.
If entity was fetched and stored deep in the resolvable.data, we should detect it and not use it.
There was a problem hiding this comment.
Actionable comments posted: 3
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/loader.go (1)
1127-1135: Fix numeric path handling in optionallyRewriteErrorPathsCurrent code appends empty strings for integer path items. Preserve numeric indices.
- newPath := make([]string, 0, len(pathPrefix)+len(pathItems)-i) - newPath = append(newPath, pathPrefix...) + newPath := make([]any, 0, len(pathPrefix)+len(pathItems)-i) + for _, s := range pathPrefix { + newPath = append(newPath, s) + } for j := i + 1; j < len(pathItems); j++ { - // BUG: for pathItems containing integers, it will append empty strings - newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + switch pathItems[j].Type() { + case astjson.TypeString: + newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + case astjson.TypeNumber: + newPath = append(newPath, pathItems[j].GetInt()) + } } newPathJSON, _ := json.Marshal(newPath)
♻️ Duplicate comments (4)
v2/pkg/engine/resolve/loader.go (4)
367-379: Prune tainted items at each traversal step to prevent child leaksFilter after every traversal hop, not only at the start/end, so children of tainted parents don’t slip through.
func (l *Loader) selectItemsForPath(path []FetchItemPathElement) []*astjson.Value { items := []*astjson.Value{l.resolvable.data} if len(path) == 0 { - return l.filterNonTainted(items) + return l.filterNonTainted(items) } for i := range path { if len(items) == 0 { break } items = l.selectItems(items, path[i]) + items = l.filterNonTainted(items) } - return l.filterNonTainted(items) + return items }
685-689: Mark taint after MergeValuesWithPath to keep pointer identity correctMove the taint mark after the merge so the map holds the post-merge pointer.
- if slices.Contains(taintedIndices, 0) { - l.taintedEntities[items[0]] = struct{}{} - } - items[0], _, err = astjson.MergeValuesWithPath(items[0], responseData, res.postProcessing.MergePath...) + items[0], _, err = astjson.MergeValuesWithPath(items[0], responseData, res.postProcessing.MergePath...) if err != nil { return errors.WithStack(ErrMergeResult{ Subgraph: res.ds.Name, Reason: err, Path: fetchItem.ResponsePath, }) } + if slices.Contains(taintedIndices, 0) { + l.taintedEntities[items[0]] = struct{}{} + }
870-902: extractEntityIndex: index arrays via GetArray/bounds, not Get("idx")Array traversal using string keys is incorrect and breaks on astjson arrays. Use type/bounds checks and direct indexing.
func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { - index := -1 - if len(path) == 0 { - return nil, index - } - for _, el := range path { - var key string - switch el.Type() { - case astjson.TypeNumber: - parsed := el.GetInt() - if parsed < 0 { - return nil, index - } - if index == -1 { - // index is assigned only once - index = parsed - } - key = strconv.Itoa(parsed) - case astjson.TypeString: - key = unsafebytes.BytesToString(el.GetStringBytes()) - default: - return nil, -1 - } - response = response.Get(key) - if response == nil { - return nil, -1 - } - } - return response, index + idx := -1 + if len(path) == 0 || response == nil { + return nil, idx + } + cur := response + for _, el := range path { + switch el.Type() { + case astjson.TypeNumber: + i := el.GetInt() + if i < 0 || cur.Type() != astjson.TypeArray { + return nil, idx + } + if idx == -1 { + idx = i + } + arr := cur.GetArray() + if i >= len(arr) { + return nil, idx + } + cur = arr[i] + case astjson.TypeString: + key := unsafebytes.BytesToString(el.GetStringBytes()) + cur = cur.Get(key) + if cur == nil { + return nil, idx + } + default: + return nil, idx + } + } + return cur, idx }
709-717: Batch (deduped) merges: re-mark taint after each mergeSame pointer-identity issue here. Mark taint after assigning the merged pointer.
- if slices.Contains(taintedIndices, idx) { - l.taintedEntities[items[i]] = struct{}{} - } - items[i], _, err = astjson.MergeValuesWithPath(items[i], batch[idx], res.postProcessing.MergePath...) + items[i], _, err = astjson.MergeValuesWithPath(items[i], batch[idx], res.postProcessing.MergePath...) if err != nil { return errors.WithStack(ErrMergeResult{ Subgraph: res.ds.Name, Reason: err, Path: fetchItem.ResponsePath, }) } + if slices.Contains(taintedIndices, idx) { + l.taintedEntities[items[i]] = struct{}{} + }
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/loader.go (2)
381-422: isTaintedEntity is O(N) deep-scan; prefer post-merge re-marking to avoid hot-path walksOnce you re-mark post-merge pointers (see other comments), you can drop the recursive scan and only check direct membership, which is much cheaper in select hot paths.
871-873: Typo in comment“atl east” → “at least”.
-// Path should contain atl east the index as the first element. Other elements would lead +// Path should contain at least the index as the first element. Other elements lead
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader.go(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (3)
v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(34-42)v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (4)
Fetch(20-27)FetchInfo(376-397)GraphCoordinate(399-403)FetchItem(29-34)
⏰ 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.25 / windows-latest)
- 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)
🔇 Additional comments (1)
v2/pkg/engine/resolve/loader.go (1)
809-868: fastjson.Value.Del supports numeric-string indices — no change required.
Value.Del accepts decimal-string keys to remove array elements (e.g. v.Get("bar").Del("0")), so errors.Del(strconv.Itoa(errorIdx)) combined with reverse iteration correctly removes array entries; do not rebuild the array.Likely an incorrect or invalid review comment.
and rename couple of things
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/loader.go (2)
367-379: Fix tainted children leak: prune after each traversal stepChild items of tainted parents can still pass through because filtering only happens at the end. Prune after each step to prevent downstream fetches from using children of tainted entities.
Apply:
func (l *Loader) selectItemsForPath(path []FetchItemPathElement) []*astjson.Value { items := []*astjson.Value{l.resolvable.data} if len(path) == 0 { return l.filterNonTainted(items) } for i := range path { if len(items) == 0 { break } items = l.selectItems(items, path[i]) + // Immediately drop tainted items to avoid leaking children from tainted parents + items = l.filterNonTainted(items) } - return l.filterNonTainted(items) + return items }
872-904: Array traversal bug in extractEntityIndex (indexes arrays via Get("idx"))On numeric path elements you convert to string and call Get("idx"), which is incorrect for arrays and will fail to locate entities. Index arrays via GetArray()[i] with bounds/type checks.
Apply:
-func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { - index := -1 - if len(path) == 0 { - return nil, index - } - for _, el := range path { - var key string - switch el.Type() { - case astjson.TypeNumber: - parsed := el.GetInt() - if parsed < 0 { - return nil, index - } - if index == -1 { - // index is assigned only once - index = parsed - } - key = strconv.Itoa(parsed) - case astjson.TypeString: - key = unsafebytes.BytesToString(el.GetStringBytes()) - default: - return nil, -1 - } - response = response.Get(key) - if response == nil { - return nil, -1 - } - } - return response, index -} +func extractEntityIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { + index := -1 + if len(path) == 0 || response == nil { + return nil, index + } + for _, el := range path { + switch el.Type() { + case astjson.TypeNumber: + i := el.GetInt() + if i < 0 || response.Type() != astjson.TypeArray { + return nil, index + } + if index == -1 { + index = i + } + arr := response.GetArray() + if i >= len(arr) { + return nil, index + } + response = arr[i] + case astjson.TypeString: + key := unsafebytes.BytesToString(el.GetStringBytes()) + response = response.Get(key) + if response == nil { + return nil, index + } + default: + return nil, index + } + } + return response, index +}
🧹 Nitpick comments (5)
v2/pkg/engine/plan/configuration.go (1)
43-48: Clarify scope: only nullable @requires are treated as optionalDoc the nullability constraint explicitly to avoid confusion, e.g., “Only nullable @requires fields are treated as optional; non‑nullable remain enforced. This requires BuildFetchReasons=true; otherwise this option is a no‑op.”
v2/pkg/engine/resolve/loader.go (2)
381-394: Avoid recursive deep scans in isTaintedEntity (hot path)Once per-step pruning is in place, recursion over arrays/objects becomes unnecessary and expensive. Consider replacing isTaintedEntity with a simple pointer-set lookup and keep filtering only at traversal boundaries.
Also applies to: 396-422
1105-1148: Fix path rewriting for integer componentsRewriting error paths currently drops integer indices (empty strings). Handle numbers explicitly.
Apply:
- for j := i + 1; j < len(pathItems); j++ { - // BUG: for pathItems containing integers, it will append empty strings - newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) - } + for j := i + 1; j < len(pathItems); j++ { + switch pathItems[j].Type() { + case astjson.TypeString: + newPath = append(newPath, unsafebytes.BytesToString(pathItems[j].GetStringBytes())) + case astjson.TypeNumber: + newPath = append(newPath, strconv.Itoa(pathItems[j].GetInt())) + default: + // skip other types + } + }v2/pkg/engine/resolve/resolve.go (1)
169-170: Document new ResolverOptions flagAdd a short comment explaining behavior and the BuildFetchReasons prerequisite, mirroring the planner config field docs.
execution/engine/execution_engine_test.go (1)
4560-5051: Solid coverage for optional @requires (nullable vs non-nullable)Tests exercise both behaviors and assert error shaping. Consider adding one case with ValidateRequiredExternalFields=true but BuildFetchReasons=false to assert it’s a no-op.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
execution/engine/execution_engine_test.go(5 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/node_selection_builder.go(1 hunks)v2/pkg/engine/resolve/loader.go(14 hunks)v2/pkg/engine/resolve/resolve.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/plan/node_selection_builder.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (2)
v2/pkg/engine/resolve/loader.go (4)
v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(34-42)v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (4)
Fetch(20-27)FetchInfo(376-397)GraphCoordinate(399-403)FetchItem(29-34)
execution/engine/execution_engine_test.go (3)
execution/engine/execution_engine.go (1)
NewExecutionEngine(112-148)v2/pkg/engine/resolve/resolve.go (1)
ResolverOptions(92-170)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
DefaultNetHttpClient(39-45)
⏰ 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.25 / windows-latest)
- 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)
🔇 Additional comments (2)
v2/pkg/engine/resolve/resolve.go (1)
253-254: Propagation of option to Loader looks goodFlag is correctly wired from ResolverOptions to Loader.
execution/engine/execution_engine_test.go (1)
5267-5277: Good helper for conditional routingThe conditional NetHTTP client simplifies federation-style test setups.
... and remove method receivers where it was not needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
v2/pkg/engine/resolve/tainted_objects.go (4)
3-8: Fix import ordering (gci).Please run gci/goimports; CI reports gci formatting failure.
10-22: Doc comment is outdated—errors are not removed anymore.The implementation doesn’t delete errors; it only returns tainted indices and leaves error handling to the caller. Update the comment to avoid confusion.
-// are required for other fetches. If an error was used to find such an entity, then this error is -// removed from the errors. +// are required for other fetches. The function does not mutate the errors array; callers decide how +// to propagate or filter errors.
49-53: Harden path handling and de‑duplicate indices.
- Guard type assumptions (require string for "_entities" and for the final field name).
- Avoid duplicate indices to reduce merge-time O(n*m) checks downstream.
func getTaintedIndices(fetch Fetch, data *astjson.Value, errors *astjson.Value) (indices []int) { + seen := make(map[int]struct{}) @@ - for i, item := range pathItems { - if unsafebytes.BytesToString(item.GetStringBytes()) != "_entities" { + for i, item := range pathItems { + if item.Type() != astjson.TypeString || unsafebytes.BytesToString(item.GetStringBytes()) != "_entities" { continue } @@ - fieldName := unsafebytes.BytesToString(pathItems[lastIndex].GetStringBytes()) + if pathItems[lastIndex].Type() != astjson.TypeString { + break + } + fieldName := unsafebytes.BytesToString(pathItems[lastIndex].GetStringBytes()) @@ - indices = append(indices, index) + if _, ok := seen[index]; !ok { + indices = append(indices, index) + seen[index] = struct{}{} + } break } } return }Also applies to: 63-71, 83-86
128-141: Name clarity: prefer “filter/remove” over “clear”.clearTainted reads like it wipes the set; “removeTainted” or “filterOutTainted” better conveys filtering the slice.
-func (t taintedObjects) clearTainted(items []*astjson.Value) []*astjson.Value { +func (t taintedObjects) filterOutTainted(items []*astjson.Value) []*astjson.Value {And update call sites accordingly.
v2/pkg/engine/resolve/tainted_objects_test.go (2)
3-9: Fix import ordering (gci).Linters report gci failure; run gci/goimports to sort groups.
108-299: Add edge‑case tests: numeric terminal path and duplicate optional‑requires errors.To lock in the hardened logic and dedup, add cases where the last path segment is a number (should be ignored) and where multiple errors point to the same entity (should return a single index).
@@ func TestGetTaintedIndices(t *testing.T) { { name: "invalid error path format", @@ expectedIndices: nil, }, + { + name: "terminal numeric segment should be ignored", + fetchReasons: []FetchReason{ + {TypeName: "User", FieldName: "email", IsRequires: true, Nullable: true}, + }, + responseJSON: `[ + {"__typename":"User","id":"1","emails":["a@example.com","b@example.com"]} + ]`, + errorsJSON: `[ + {"message":"index error","path":["_entities",0,"emails",1]} + ]`, + expectedIndices: nil, + }, + { + name: "duplicate errors for same entity are de-duplicated", + fetchReasons: []FetchReason{ + {TypeName: "Product", FieldName: "reviews", IsRequires: true, Nullable: true}, + }, + responseJSON: `[ + {"__typename":"Product","upc":"1","reviews":null} + ]`, + errorsJSON: `[ + {"message":"Cannot resolve field reviews","path":["_entities",0,"reviews"]}, + {"message":"Cannot resolve field reviews","path":["_entities",0,"reviews"]} + ]`, + expectedIndices: []int{0}, + },Would you like me to push a patch updating the tests once you adopt the function changes?
v2/pkg/engine/resolve/loader.go (2)
645-647: Avoid O(n*m) membership checks—use a set for tainted indices.Convert taintedIndices to a set once and use O(1) lookups in all branches.
- var taintedIndices []int + var taintedIndices []int + var taintedSet map[int]struct{} @@ - if l.validateRequiredExternalFields && res.postProcessing.SelectResponseDataPath != nil { - taintedIndices = getTaintedIndices(fetchItem.Fetch, responseData, responseErrors) - } + if l.validateRequiredExternalFields && res.postProcessing.SelectResponseDataPath != nil { + taintedIndices = getTaintedIndices(fetchItem.Fetch, responseData, responseErrors) + if len(taintedIndices) > 0 { + taintedSet = make(map[int]struct{}, len(taintedIndices)) + for _, ti := range taintedIndices { + taintedSet[ti] = struct{}{} + } + } + } @@ - if slices.Contains(taintedIndices, 0) { + if _, ok := taintedSet[0]; ok { l.taintedObjs.add(items[0]) } @@ - if slices.Contains(taintedIndices, idx) { + if _, ok := taintedSet[idx]; ok { l.taintedObjs.add(items[i]) } @@ - if slices.Contains(taintedIndices, i) { + if _, ok := taintedSet[i]; ok { l.taintedObjs.add(items[i]) }Also applies to: 674-676, 695-697
1071-1081: Give the generic error a stable extension code.To help clients detect this condition, include an extensions.code (e.g., OPTIONAL_REQUIRES_DEPS_FAILED) instead of a plain message.
- msg := fmt.Sprintf(`{"message":"Failed to obtain field dependencies from Subgraph '%s'%s."}`, res.ds.Name, path) + msg := fmt.Sprintf(`{"message":"Failed to obtain field dependencies from Subgraph '%s'%s.","extensions":{"code":"OPTIONAL_REQUIRES_DEPS_FAILED"}}`, res.ds.Name, path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/pkg/engine/plan/node_selection_visitor.go(2 hunks)v2/pkg/engine/plan/required_fields_visitor.go(2 hunks)v2/pkg/engine/resolve/loader.go(21 hunks)v2/pkg/engine/resolve/tainted_objects.go(1 hunks)v2/pkg/engine/resolve/tainted_objects_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/plan/node_selection_visitor.go
- v2/pkg/engine/plan/required_fields_visitor.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (3)
v2/pkg/engine/resolve/tainted_objects.go (1)
v2/pkg/engine/resolve/fetch.go (2)
FetchInfo(376-397)GraphCoordinate(399-403)
v2/pkg/engine/resolve/tainted_objects_test.go (1)
v2/pkg/engine/resolve/fetch.go (3)
FetchReason(364-372)FetchInfo(376-397)FetchDependencies(111-114)
v2/pkg/engine/resolve/loader.go (4)
v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(34-42)v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (3)
FetchItemPathElement(78-82)Fetch(20-27)FetchItem(29-34)
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/tainted_objects.go
[failure] 6-6:
File is not properly formatted (gci)
v2/pkg/engine/resolve/tainted_objects_test.go
[failure] 7-7:
File is not properly formatted (gci)
⏰ 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 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/loader.go (2)
365-377: Prune tainted items after every traversal step to prevent child leaks.Filtering only at the end allows selecting into children of tainted parents. Prune on each hop.
func (l *Loader) selectItemsForPath(path []FetchItemPathElement) []*astjson.Value { - items := []*astjson.Value{l.resolvable.data} - if len(path) == 0 { - return l.taintedObjs.clearTainted(items) - } + items := []*astjson.Value{l.resolvable.data} + if len(path) == 0 { + return l.taintedObjs.clearTainted(items) + } for i := range path { if len(items) == 0 { break } - items = selectItems(items, path[i]) + items = selectItems(items, path[i]) + items = l.taintedObjs.clearTainted(items) + if len(items) == 0 { + break + } } - return l.taintedObjs.clearTainted(items) + return items }
556-571: Confirmed: nil SelectResponseDataPath intentionally means “use full response” (GraphQL sets the data path).GraphQL datasource provides DefaultPostProcessingConfiguration.SelectResponseDataPath = []string{"data"} (v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go), so GraphQL responses will hit the Get(...) branch; when SelectResponseDataPath == nil the loader intentionally uses the whole parsed response and resolvable.go handles merging based on that. No change required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
v2/pkg/engine/resolve/tainted_objects.go (3)
3-9: Remove unused import after fixing array traversal.strconv becomes unused after the selectObjectAndIndex change. Clean up imports.
-import ( - "strconv" - - "github.com/wundergraph/astjson" +import ( + "github.com/wundergraph/astjson"
49-67: Harden path parsing: ensure last element is a field name.Guard against non-string terminal path items to avoid panics/false positives.
- fieldName := unsafebytes.BytesToString(pathItems[lastIndex].GetStringBytes()) + if pathItems[lastIndex].Type() != astjson.TypeString { + break + } + fieldName := unsafebytes.BytesToString(pathItems[lastIndex].GetStringBytes())
39-86: Optional: de-duplicate tainted indices.Multiple errors can reference the same entity index. Dedup to avoid redundant work later.
-func getTaintedIndices(fetch Fetch, data *astjson.Value, errors *astjson.Value) (indices []int) { +func getTaintedIndices(fetch Fetch, data *astjson.Value, errors *astjson.Value) (indices []int) { + seen := make(map[int]struct{}) ... - indices = append(indices, index) + if _, ok := seen[index]; !ok { + indices = append(indices, index) + seen[index] = struct{}{} + }v2/pkg/engine/resolve/loader.go (1)
973-986: Avoid injecting empty path segments when rewriting error paths.Appending "" for unknown path item types produces confusing paths. Skip unknown types instead.
- default: - newPath = append(newPath, "") + default: + // skip unknown path element types + continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/resolve/loader.go(21 hunks)v2/pkg/engine/resolve/tainted_objects.go(1 hunks)v2/pkg/engine/resolve/tainted_objects_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/tainted_objects_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/tainted_objects.gov2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (2)
v2/pkg/engine/resolve/tainted_objects.go (1)
v2/pkg/engine/resolve/fetch.go (2)
FetchInfo(376-397)GraphCoordinate(399-403)
v2/pkg/engine/resolve/loader.go (4)
v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(34-42)v2/pkg/engine/resolve/resolvable.go (1)
Resolvable(26-61)v2/pkg/engine/resolve/fetch.go (3)
FetchItemPathElement(78-82)Fetch(20-27)FetchItem(29-34)
⏰ 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 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (6)
v2/pkg/engine/resolve/loader.go (5)
574-599: Confirm intended ordering: generic dependency error is added in addition to subgraph errors.You now add a generic “Failed to obtain field dependencies …” error and still merge all subgraph errors. If that’s the desired UX, LGTM; otherwise, filter optional-@requires errors first before merging.
636-647: Good: mark taint after merge to keep pointer identity stable.Tainting post-MergeValuesWithPath avoids losing the taint due to pointer replacement.
661-677: Good: batch+dedupe path captures merge result and taints after merge.Capturing the returned merged value per item and marking taint afterwards is correct.
686-697: Good: non-deduped batch marks taint after merge as well.Prevents stale-pointer tainting.
365-377: Prune tainted after every traversal step to prevent child leaks.Filtering only at the end lets children of tainted parents pass through. Prune after each select to maintain taint semantics.
func (l *Loader) selectItemsForPath(path []FetchItemPathElement) []*astjson.Value { - items := []*astjson.Value{l.resolvable.data} - if len(path) == 0 { - return l.taintedObjs.filterOutTainted(items) - } + items := []*astjson.Value{l.resolvable.data} + if len(path) == 0 { + return l.taintedObjs.filterOutTainted(items) + } for i := range path { if len(items) == 0 { break } - items = selectItems(items, path[i]) + items = selectItems(items, path[i]) + items = l.taintedObjs.filterOutTainted(items) } - return l.taintedObjs.filterOutTainted(items) + return items }v2/pkg/engine/resolve/tainted_objects.go (1)
90-122: Fix array traversal: index arrays directly; don’t use string keys for numeric path elements.Numeric elements in error paths represent array indices. Using strconv+Get("idx") on arrays is incorrect and will miss/return nil. This also causes wrong taint detection. Handle arrays via GetArray() with bounds/type checks, and guard against nil response.
Apply:
-func selectObjectAndIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { - index := -1 - if len(path) == 0 { - return nil, index - } - for _, el := range path { - var key string - switch el.Type() { - case astjson.TypeNumber: - parsed := el.GetInt() - if parsed < 0 { - return nil, index - } - if index == -1 { - // index is assigned only once - index = parsed - } - key = strconv.Itoa(parsed) - case astjson.TypeString: - key = unsafebytes.BytesToString(el.GetStringBytes()) - default: - return nil, -1 - } - response = response.Get(key) - if response == nil { - return nil, -1 - } - } - return response, index -} +func selectObjectAndIndex(response *astjson.Value, path []*astjson.Value) (*astjson.Value, int) { + index := -1 + if response == nil || len(path) == 0 { + return nil, index + } + for _, el := range path { + switch el.Type() { + case astjson.TypeNumber: + i := el.GetInt() + if i < 0 || response.Type() != astjson.TypeArray { + return nil, index + } + if index == -1 { + index = i + } + arr := response.GetArray() + if i >= len(arr) { + return nil, index + } + response = arr[i] + case astjson.TypeString: + key := unsafebytes.BytesToString(el.GetStringBytes()) + response = response.Get(key) + if response == nil { + return nil, index + } + default: + return nil, index + } + } + return response, index +}Note: drop the now-unused strconv import.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
execution/engine/execution_engine_test.go (3)
241-246: Name this helper consistently with existing patternFor consistency with withValueCompletion and withFetchReasons, consider renaming to withValidateRequiredExternalFields.
Apply locally in this file:
-func validateRequiredExternalFields() executionTestOptions { +func withValidateRequiredExternalFields() executionTestOptions { return func(options *_executionTestOptions) { options.validateRequiredExternalFields = true } }Then update the call sites in this file accordingly (e.g., Lines ~4886, ~5053, ~5255).
4722-4722: Clarify test name to reflect intentCurrent name can be misread. Consider: “non-nullable @requires dependencies are enforced (not optional)”.
- t.Run("do not validate non-nullable @requires dependencies", func(t *testing.T) { + t.Run("non-nullable @requires dependencies are enforced (not optional)", func(t *testing.T) {
5272-5282: Test helpers exist — make createConditionalTestRoundTripper match JSON structurally
- execution/engine/execution_engine_helpers_test.go defines conditionalTestCase and createConditionalTestRoundTripper and they are accessible from the tests.
- createConditionalTestRoundTripper currently matches request bodies by exact string (require.Containsf(..., string(gotBody))), which is brittle. Update it to compare JSON structurally (unmarshal req.Body into interface{} and compare with unmarshalled map keys using reflect.DeepEqual, or compare compact/normalized JSON strings via json.Compact) — or store responses keyed by normalized JSON when the map is built.
📜 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/resolve.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/resolve.go
🔇 Additional comments (6)
execution/engine/execution_engine_test.go (6)
221-223: Option plumbing added to test harnessAdding validateRequiredExternalFields to _executionTestOptions is fine.
4560-4561: Wrapping the new scenarios under a single t.Run block is goodThis addresses the prior review to group these cases.
4562-4720: Happy-path federation requires/external scenario is well specifiedRequests include needed __typename and representations; metadata (keys/requires/external) align with the SDL. Expected result matches.
4889-5054: Nullable @requires validation case is accurateAsserting both dependency-obtainment and fetch errors here makes sense with the flag enabled.
5056-5256: Nested nullable @requires coverage looks solidGood inclusion of __typename in nested selections and selective representations for the second fetch; expectations match the taint/skip behavior.
279-286: Verify planner/resolver support for ValidateRequiredExternalFieldsWiring looks correct in execution/engine/execution_engine_test.go:279–286. Automated repo search couldn't run in this environment — confirm plannerConfig and ResolverOptions declare ValidateRequiredExternalFields and that planner/engine code reads/enforces it (check call sites/conditionals).
ysmolski
deleted the
yury/eng-7767-validate-presence-of-optional-requires-dependencies
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.228](v2.0.0-rc.227...v2.0.0-rc.228) (2025-09-24) ### Features * validate presence of optional "requires" dependencies ([#1297](#1297)) ([ba75e25](ba75e25)) --- 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** * Adds validation to ensure optional "requires" dependencies are present when needed. * **Documentation** * Updates changelog for v2.0.0-rc.228 to document the new validation and confirm no public API changes. * **Chores** * Bumps the v2 release candidate to 2.0.0-rc.228. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Yury Smolski <140245+ysmolski@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [1.6.0](execution/v1.5.0...execution/v1.6.0) (2025-10-21) ### Features * support the oneOf directive ([#1308](#1308)) ([251cb02](251cb02)) * validate presence of optional [@requires](https://github.com/requires) dependencies ([#1297](#1297)) ([ba75e25](ba75e25)) ### Bug Fixes * bump engine to v2.0.0-rc.231 for execution ([#1329](#1329)) ([ebddb25](ebddb25)) * propagate fetch reasons for interface-related fields ([#1312](#1312)) ([5ee3014](5ee3014)) --- 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 oneOf directive support * Added validation of optional @requires dependencies * **Bug Fixes** * Fixed fetch reason propagation for interface-related fields <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
In GraphQL Federation, when a field has @requires dependencies on
external nullable field and those dependencies fail to resolve,
the entire entity resolution would lead to malformed data being sent
to later fetches.
For example, 2 subgraphs:
If the fetch from Subgraph1 returned the null value for
the "info" field along with error pointing to that field, then
this bit of information should not be used in the fetch
from Subgraph 2 that provides info to get the value of
the "fullInfo" field.
This change implements validation of optional @requires dependencies
in GraphQL Federation. The core feature allows the system to
gracefully handle scenarios where entities fail to resolve their
@requires dependencies, particularly for nullable fields.
This change introduces selective handling where only nullable @requires fields
are treated as "optional" - if they fail, the entity is marked as "tainted" but
processing continues for other entities.
Summary by CodeRabbit