feat(router): support the "oneOf" directive #2246
Conversation
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughAdds a OneOf-style GraphQL input Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (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). (10)
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
demo/pkg/subgraphs/employees/subgraph/schema.graphqls (1)
194-198: Correct use of @OneOf; keep members nullableSDL matches the oneOf spec: members are nullable and exactly one non-null must be provided. Consider adding a short description to document precedence if multiple fields are mistakenly provided by older clients (the engine should error).
router-tests/integration_test.go (1)
1313-1391: Great coverage; add two focused cases to harden oneOf behavior
- Multiple fields provided should error (exactly one field rule).
- Positive case for title criterion to exercise all branches.
Optionally, prefer JSONEq or substring match for error messages to reduce brittleness across parser versions.
t.Run("explicit null value", func(t *testing.T) { // ... }) + +t.Run("multiple fields provided errors", func(t *testing.T) { + res := xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ + Query: `query($criteria: FindEmployeeCriteria!) { + findEmployeesBy(criteria: $criteria) { id } + }`, + Variables: json.RawMessage(`{"criteria": {"id": 1, "department": "ENGINEERING"}}`), + }) + require.Contains(t, res.Body, `"errors"`) + require.Contains(t, res.Body, `must have exactly one field`) +}) + +t.Run("find employees by title", func(t *testing.T) { + res := xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ + Query: `query { + findEmployeesBy(criteria: { title: "Tech Lead" }) { + id + details { forename } + } + }`, + }) + require.Contains(t, res.Body, `"forename":"Dustin"`) + require.NotContains(t, res.Body, `"errors"`) +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
demo/pkg/subgraphs/employees/subgraph/generated/generated.gois excluded by!**/generated/**router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
demo/pkg/subgraphs/employees/subgraph/model/models_gen.go(1 hunks)demo/pkg/subgraphs/employees/subgraph/schema.graphqls(2 hunks)demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go(2 hunks)router-tests/Makefile(1 hunks)router-tests/README.md(2 hunks)router-tests/go.mod(1 hunks)router-tests/integration_test.go(1 hunks)router-tests/testdata/introspection/base-graph-schema.json(3 hunks)router-tests/testdata/introspection/feature-graph-schema.json(3 hunks)router/README.md(2 hunks)router/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router/README.md
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/go.modrouter-tests/go.mod
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
PR: wundergraph/cosmo#2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/go.modrouter-tests/go.mod
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
PR: wundergraph/cosmo#2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/go.modrouter-tests/go.mod
🧬 Code graph analysis (2)
demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go (2)
demo/pkg/subgraphs/employees/subgraph/model/models_gen.go (5)
FindEmployeeCriteria(166-170)Employee(97-110)Employee(112-112)Employee(115-115)Department(287-287)demo/cmd/subscriptiontest/main.go (1)
Role(51-53)
router-tests/integration_test.go (1)
router-tests/testenv/testenv.go (4)
Run(107-124)Config(286-342)Environment(1729-1765)GraphQLRequest(1905-1913)
🪛 markdownlint-cli2 (0.18.1)
router/README.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (12)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
🔇 Additional comments (14)
router/README.md (1)
33-35: Clearer localreplaceinstructionsNice job clarifying the local
graphql-go-toolsreplace workflow—helps contributors avoid guesswork when testing router changes against local engine builds.router-tests/Makefile (1)
17-17: GOLDIE_UPDATE environment switch looks goodSetting
GOLDIE_UPDATE=1for the snapshot run keeps the command portable and avoids the deprecated-updateflag without changing race coverage.demo/pkg/subgraphs/employees/subgraph/model/models_gen.go (1)
166-170: FindEmployeeCriteria is shaped correctly for @OneOfPointers on each field let the resolver tell which criterion was picked, matching the exclusivity the directive expects.
router/go.mod (1)
34-34: Pinning graphql-go-tools to the new pseudo-version makes senseThis ties the router to the commit with the oneOf support you need while keeping the rest of the dependency graph untouched.
router-tests/go.mod (1)
30-30: Mirroring the graphql-go-tools pin in tests keeps modules alignedHaving router-tests follow the same pseudo-version prevents drift between the binary and its fixtures.
router-tests/README.md (1)
9-34: Docs cleanup improves clarityThe extra sentence break and snapshot wording tweaks make the instructions easier to follow without altering the workflow.
router-tests/testdata/introspection/feature-graph-schema.json (3)
178-218: New finder field lines up with oneOf usage
The newfindEmployeesByfield is wired correctly—argument nullability and return type mirror the intended[Employee!]!contract. 👍
2405-2450: Input object matches @OneOf constraints
FindEmployeeCriteriakeeps every field nullable, so it’s ready for the router’s one‑of validation without further tweaks.
17427-17435: Directive definition looks spot on
Great to see the built‑in@oneOfsurfaced with the right location and no extra args—this keeps introspection in sync with the spec.demo/pkg/subgraphs/employees/subgraph/schema.graphqls (1)
34-35: Addition looks good; mind the non-null list contractThe field returns [Employee!]!, so the resolver must always return an empty slice, not nil, when no matches. See resolver comment for a small fix to guarantee this invariant.
demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go (1)
10-10: OK to use stdlib slicesAssuming Go ≥ 1.21 (the tests also import slices), this import is fine. If CI still targets older Go, switch to golang.org/x/exp/slices.
router-tests/testdata/introspection/base-graph-schema.json (3)
178-218: Schema addition aligns with new oneOf query support.The new
findEmployeesByfield matches the existing employees collection shape and wires in the non-null criteria argument as expected. Nice work keeping the response list non-null for consistency.
2404-2450: Input object is oneOf-ready.Each field in
FindEmployeeCriteriastays nullable with no defaults, so it satisfies the@oneOfcontract without further tweaks.
1727-1735: Directive definition looks good.The new
@oneOfdirective entry follows the spec description and location requirements, rounding out introspection coverage.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
…4-router-implement-oneof
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
demo/pkg/subgraphs/employees/subgraph/schema.graphqls(2 hunks)router-tests/Makefile(1 hunks)router-tests/go.mod(4 hunks)router-tests/integration_test.go(1 hunks)router/go.mod(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- router-tests/integration_test.go
- demo/pkg/subgraphs/employees/subgraph/schema.graphqls
- router-tests/Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/go.mod
⏰ 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). (12)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
router/go.mod (3)
53-54: Broad standard library dependency updates warrant verification.Multiple simultaneous updates across Go standard libraries (golang.org/x/sync, x/sys, x/crypto, x/net, x/text; google.golang.org/protobuf) suggest systematic compatibility work. While generally safe, verify:
- x/sync v0.17.0 and x/sys v0.37.0 compatibility with router's concurrency model
- x/text v0.30.0 (jump from v0.23.0) compatibility with i18n/l10n handling
- x/crypto and x/net updates don't introduce unexpected breaking changes
Also applies to: 56-56, 86-86, 167-168
76-76: Good: mark3labs/mcp-go correctly pinned to stable v0.36.0.The package is correctly locked to v0.36.0, avoiding the regressions present in v0.38.0. Based on learnings
34-34: wundergraph/graphql-go-tools/v2 rc.231 correctly includes OneOf directive support.graphql-go-tools v2.0.0-rc.231 (released Oct 20, 2025) adds support for the oneOf directive, confirming this update is necessary and appropriate for the PR. No issues identified.
router-tests/go.mod (2)
38-38: Coordinate bulk golang.org/x package updates with feature requirements.Multiple
golang.org/x/*packages are being updated in tandem (net, crypto, mod, sync, sys, text, tools). These appear to be transitive dependency updates pulled in by the main dependency changes.Verify that:
- These are transitive updates and not driven by breaking API changes elsewhere in the codebase
- The bulk update doesn't mask incompatibilities or version skew that could cause issues downstream
Also applies to: 40-40, 172-172, 174-179
22-22: Minor version bump for testify looks routine.Testify updated from v1.10.0 to v1.11.1. This appears to be a standard maintenance update unrelated to oneOf feature support. No concerns flagged.
It means validation of operations and values being sent with operations.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores