fix: group subgraph errors by subgraph name #1351
Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
jensneuse and
ysmolski
as code owners
WalkthroughRefactors Context to track subgraph errors as a map keyed by data source, changes appendSubgraphErrors to accept DataSourceInfo and store per-source joined errors, makes SubgraphErrors() return nil when empty or a deterministic joined error, updates loader/resolvable call sites and many tests to use the new context/error shape. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-12-09T15:30:57.980ZApplied to files:
📚 Learning: 2025-10-16T13:05:19.838ZApplied to files:
📚 Learning: 2025-07-02T15:28:02.122ZApplied to files:
📚 Learning: 2025-08-08T09:43:07.433ZApplied to files:
📚 Learning: 2025-09-19T14:50:19.528ZApplied to files:
📚 Learning: 2025-11-19T09:42:17.644ZApplied to files:
📚 Learning: 2025-07-28T12:44:56.405ZApplied to files:
🧬 Code graph analysis (1)v2/pkg/engine/resolve/context.go (1)
⏰ 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)
🔇 Additional comments (7)
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
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/context.go (2)
9-9: Per-subgraph error aggregation and deterministic ordering look correct; consider a tiny guard for nil-only error slicesThe shift to
subgraphErrors map[string]error, aggregation inappendSubgraphErrorskeyed byds.Name, and the deterministicSubgraphErrors()implementation (sorted keys +errors.Join) all align well with the goal of scoping errors to the correct subgraph and making results idempotent across calls.NewContextinitializing the map and the defensivenilcheck inappendSubgraphErrorsplus the deep-copy inclonetogether make this robust.One small, optional refinement: to avoid storing entries whose combined error is
niland to skip an extraJoinwhen all providederrsarenil, you could early‑out:func (c *Context) appendSubgraphErrors(ds DataSourceInfo, errs ...error) { if c.subgraphErrors == nil { c.subgraphErrors = make(map[string]error) } - c.subgraphErrors[ds.Name] = errors.Join(c.subgraphErrors[ds.Name], errors.Join(errs...)) + joinedErr := errors.Join(errs...) + if joinedErr == nil { + return + } + c.subgraphErrors[ds.Name] = errors.Join(c.subgraphErrors[ds.Name], joinedErr) }This keeps the public behaviour of
SubgraphErrors()the same while slightly reducing unnecessary allocations and map entries for subgraphs that never actually produced a non‑nil error.Also applies to: 35-36, 141-165, 172-179, 213-218
186-193: Confirm whetherWithContextshould also deep‑copysubgraphErrorslikeclonedoes
clonenow creates an independentsubgraphErrorsmap:if c.subgraphErrors != nil { cpy.subgraphErrors = make(map[string]error, len(c.subgraphErrors)) for k, v := range c.subgraphErrors { cpy.subgraphErrors[k] = v } }but
WithContextstill does a shallow struct copy:cpy := *c cpy.ctx = ctx return &cpywhich means any
WithContextresult shares the samesubgraphErrorsmap with the originalContext. That may be intentional (same logical request, different basecontext.Context) or it may be surprising, given thatclonenow isolates the map.If you want
WithContextto behave likeclonew.r.t. subgraph errors, you could mirror the same map copy there:func (c *Context) WithContext(ctx context.Context) *Context { if ctx == nil { panic("nil context.Context") } cpy := *c cpy.ctx = ctx + if c.subgraphErrors != nil { + cpy.subgraphErrors = make(map[string]error, len(c.subgraphErrors)) + for k, v := range c.subgraphErrors { + cpy.subgraphErrors[k] = v + } + } return &cpy }Worth double‑checking usages of
WithContextto ensure the current sharing vs. proposed copying matches the intended lifecycle of per‑subgraph error state.Also applies to: 213-218
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/context.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/context.go (1)
v2/pkg/engine/resolve/loader.go (1)
DataSourceInfo(45-48)
⏰ 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 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/context.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/context.go (1)
v2/pkg/engine/resolve/loader.go (1)
DataSourceInfo(45-48)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/context.go
[error] 161-161: panic: assignment to entry in nil map during TestAuthorization (disallow_root_fetch).
⏰ 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 (5)
v2/pkg/engine/resolve/context.go (5)
9-9: LGTM!The sort package is correctly imported to enable deterministic ordering of subgraph errors.
35-35: LGTM!The field type change from
errortomap[string]errorcorrectly implements per-subgraph error tracking.
141-158: LGTM!The refactored
SubgraphErrors()method correctly returns a deterministically ordered aggregation of per-subgraph errors. The nil check, sorting, and error joining logic are all properly implemented.
174-175: LGTM!The map initialization in
NewContextis correct. However, as noted in the past comments and confirmed by the pipeline failure, not allContextinstances are created viaNewContext(), which is why defensive initialization is needed inappendSubgraphErrors.
210-216: LGTM!The map cloning logic correctly performs a defensive nil check and deep copy to prevent shared references.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/resolve_test.go (1)
165-2250: ConsistentsubgraphErrorsinitialization looks good; consider a tiny helper to reduce repetitionAll the updated
Contextliterals that now includesubgraphErrors: make(map[string]error)are correct and help ensure any write-path intosubgraphErrorscan’t panic on a nil map. From a maintenance angle, you might eventually want a small test helper (e.g.newTestContext()returning aContextwithctx: context.Background()andsubgraphErrorsinitialized) to avoid touching dozens of call sites every timeContextgrows a new field. Not required for this PR, just an optional clean‑up for later.Also applies to: 1431-2250
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
v2/pkg/engine/resolve/authorization_test.go(14 hunks)v2/pkg/engine/resolve/extensions_test.go(5 hunks)v2/pkg/engine/resolve/loader_hooks_test.go(13 hunks)v2/pkg/engine/resolve/loader_test.go(7 hunks)v2/pkg/engine/resolve/ratelimit_test.go(9 hunks)v2/pkg/engine/resolve/resolve_federation_test.go(18 hunks)v2/pkg/engine/resolve/resolve_test.go(62 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/authorization_test.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolve_federation_test.go
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/resolve/resolve_test.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
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/resolve/resolve_test.go
🧬 Code graph analysis (4)
v2/pkg/engine/resolve/extensions_test.go (1)
v2/pkg/engine/resolve/context.go (2)
Context(17-36)RateLimitOptions(108-121)
v2/pkg/engine/resolve/ratelimit_test.go (1)
v2/pkg/engine/resolve/context.go (3)
Context(17-36)RateLimitOptions(108-121)RateLimitErrorExtensionCode(123-126)
v2/pkg/engine/resolve/resolve_test.go (1)
v2/pkg/engine/resolve/context.go (1)
Context(17-36)
v2/pkg/engine/resolve/loader_hooks_test.go (2)
v2/pkg/engine/resolve/loader.go (1)
LoaderHooks(38-43)v2/pkg/engine/resolve/context.go (1)
Context(17-36)
⏰ 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)
🔇 Additional comments (5)
v2/pkg/engine/resolve/ratelimit_test.go (1)
58-58: Subgraph error map initialization in rate-limit tests is consistent and correctAll updated Context literals now initialize
subgraphErrorswithmake(map[string]error), which aligns with the new per-subgraph error tracking and avoids nil-map writes during resolution. No further issues spotted in these tests.Also applies to: 76-76, 92-92, 108-108, 124-124, 143-143, 168-168, 191-191, 214-214
v2/pkg/engine/resolve/extensions_test.go (1)
22-22: Context updates for extensions tests correctly wire subgraphErrorsThe added
subgraphErrors: make(map[string]error)on all Context literals used inTestExtensionsmatches the updated Context API and ensures downstream error aggregation can safely write per-subgraph errors. Everything looks consistent.Also applies to: 47-47, 72-72, 94-94, 119-119
v2/pkg/engine/resolve/resolve_federation_test.go (1)
179-179: Federation tests correctly initialize Context.subgraphErrors across all scenariosEach federation test now passes a Context with
subgraphErrors: make(map[string]error), both for pointer and value contexts. This is in line with the new per-subgraph error tracking and avoids nil-map writes during parallel fetches. No issues found in these changes.Also applies to: 370-370, 526-526, 672-672, 816-816, 1021-1021, 1102-1102, 1240-1240, 1521-1521, 1649-1649, 1777-1777, 1906-1906, 2148-2148, 2420-2420, 2692-2692, 2832-2832, 2989-2989
v2/pkg/engine/resolve/loader_test.go (1)
288-290: Loader tests and benchmark correctly updated for subgraphErrorsAll loader tests and the benchmark now initialize
subgraphErrorson the Contexts withmake(map[string]error), matching the new Context shape and preventing nil-map writes duringLoadGraphQLResponseData. Reusing a single Context in the benchmark remains safe given these scenarios are success paths. No further issues noted.Also applies to: 377-380, 469-472, 751-755, 1028-1031, 1124-1127, 1427-1430
v2/pkg/engine/resolve/loader_hooks_test.go (1)
60-63: Loader hooks tests are correctly wired to the new subgraph error trackingThe updated resolve contexts used in loader-hooks tests now initialize
subgraphErrorsand, where applicable,LoaderHooks. This allows:
TestLoaderHooksto capture per-subgraphSubgraphErrorinstances viaResponseInfo.Err, andresolveCtx.SubgraphErrors()to return a non-nil aggregated error when subgraph errors occur.The wiring is consistent across serial, parallel, and list-item fetch scenarios, and expectations around
SubgraphErrorcontents remain valid.Also applies to: 135-138, 204-207, 270-273, 337-340, 428-428, 466-466, 504-504, 542-542, 580-580, 618-618, 656-656
SkArchon
force-pushed
the
milinda/eng-8637-error-message-attached-to-multiple-unrelated-subgraphs
branch
from
6dee1a6 to
75ea694
Compare
SkArchon
deleted the
milinda/eng-8637-error-message-attached-to-multiple-unrelated-subgraphs
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.241](v2.0.0-rc.240...v2.0.0-rc.241) (2025-12-10) ### Bug Fixes * group subgraph errors by subgraph name ([#1351](#1351)) ([d65d348](d65d348)) --- 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 * **Bug Fixes** * Fixed an issue where subgraph errors are now properly grouped by subgraph name in v2.0.0-rc.241. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Right now we pass in all subgraph errors to the engine loader hook OnFinished function (See Here), which means that whenever any subgraph has an error, we will pass that/those errors to the OnFinished function for an irrelevant Subgraph, which leads to the following
Checklist
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.