fix(router): return typename on Cosmo Streams mutations #2422
Conversation
dkorittki
requested review from
Noroth,
StarpTech,
alepane21,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughAdds GraphQL Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
=======================================
Coverage 61.35% 61.36%
=======================================
Files 229 229
Lines 23814 23814
=======================================
+ Hits 14612 14614 +2
+ Misses 7970 7967 -3
- Partials 1232 1233 +1
🚀 New features to boost your workflow:
|
![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 (3)
router/core/supervisor_instance.go (1)
42-50: Switch tomemlimit.FromCgroupis consistent with the cgroup bugfix intentUsing
memlimit.WithProvider(memlimit.FromCgroup)with the existing 0.9 ratio aligns with the “correct cgroup policy” CHANGELOG entry and keeps behavior consistent with the plan generator path; logging and fallback remain unchanged.If you find yourself touching this logic again, consider extracting a small shared helper (e.g.
core.SetAutoGoMemLimit(logger)) so router startup and the plan generator stay in sync by construction.router/cmd/plan_generator.go (1)
98-118: Plan generator now uses the same cgroup-based GOMEMLIMIT policy as the routerSwitching to
memlimit.WithProvider(memlimit.FromCgroup)(still at 90% ratio) brings the plan generator in line with the router’s startup behavior and the documented cgroup policy fix; logging and error handling are preserved.As with the router initialization path, you may want to centralize this GOMEMLIMIT setup into a shared helper to avoid future drift between binaries.
router-tests/modules/context-error/module.go (1)
11-22: LGTM!The module structure and writer wrapper are well-designed for intercepting response writes and capturing error state.
One minor observation: the
statusCodefield is set inWriteHeaderbut never used. Since this is test module code, it's fine to keep for potential future debugging, but could be removed if not needed.
📜 Review details
Configuration used: Organization 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 (11)
demo/go.mod(1 hunks)router-tests/go.mod(1 hunks)router-tests/modules/context-error/module.go(1 hunks)router-tests/modules/context_error_field_test.go(1 hunks)router/CHANGELOG.md(1 hunks)router/cmd/plan_generator.go(1 hunks)router/core/context.go(2 hunks)router/core/graphql_handler.go(1 hunks)router/core/supervisor_instance.go(1 hunks)router/go.mod(1 hunks)router/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- router/package.json
- router-tests/go.mod
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 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:
demo/go.mod
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 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:
demo/go.mod
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/core/graphql_handler.gorouter-tests/modules/context_error_field_test.gorouter-tests/modules/context-error/module.gorouter/CHANGELOG.md
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/core/graphql_handler.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/core/graphql_handler.go
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/modules/context_error_field_test.go
📚 Learning: 2025-08-26T17:19:28.178Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2167
File: router/internal/expr/retry_context.go:3-10
Timestamp: 2025-08-26T17:19:28.178Z
Learning: In the Cosmo router codebase, prefer using netErr.Timeout() checks over explicit context.DeadlineExceeded checks for timeout detection in retry logic, as Go's networking stack typically wraps context deadline exceeded errors in proper net.Error implementations.
Applied to files:
router-tests/modules/context-error/module.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 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/CHANGELOG.md
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 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/CHANGELOG.md
🧬 Code graph analysis (2)
router/core/context.go (1)
router/pkg/pubsub/datasource/error.go (1)
Error(3-6)
router/core/graphql_handler.go (1)
router/internal/expr/expr.go (1)
Context(35-39)
🔇 Additional comments (13)
router/go.mod (1)
59-88: expr-lang/expr version bump looks consistentUpdating
github.com/expr-lang/exprtov1.17.7here is reasonable and keeps the router module aligned with the rest of the repo’s go.mod files; no code-level issues from this diff alone.router/CHANGELOG.md (1)
7-18: New 0.273.2 and 0.273.1 entries are well‑formedVersion headers, dates, links, and bugfix descriptions match the repository’s existing CHANGELOG conventions and the behavior changes in this PR.
demo/go.mod (1)
25-88: Demo module expr-lang/expr bump keeps dependencies alignedUpdating the indirect
github.com/expr-lang/exprdependency tov1.17.7here matches the router module and helps keep the overall build graph consistent.router/core/context.go (2)
142-145: LGTM!Clean addition to the interface that follows the existing pattern for accessors. The docstring is consistent with other methods in the interface.
479-483: LGTM!The implementation correctly exposes the internal error field, complementing the existing
SetErrormethod and following the same access pattern as other simple getters in this file.router/core/graphql_handler.go (1)
188-191: LGTM!Good addition to track subgraph errors as final response errors. This ensures proper error propagation when the resolver completes but subgraph errors were encountered, aligning with the PR objective to fix error handling for EDFS-backed mutations.
router-tests/modules/context_error_field_test.go (4)
17-19: LGTM!Well-structured test function with parallel execution enabled at both the parent and subtest levels.
20-60: LGTM!Comprehensive test for authentication failure scenario. Properly configures the access controller, validates the HTTP status code, response body content, and the custom error header propagation.
62-98: LGTM!Good test coverage for subgraph failure scenarios. The middleware correctly simulates a subgraph returning an error, and the assertions properly verify both the GraphQL error propagation and the custom header behavior.
100-124: LGTM!Important negative test ensuring the error header is only set when actual errors occur. The assertion using
require.Emptycorrectly validates the header absence.router-tests/modules/context-error/module.go (3)
24-32: LGTM!Clean implementation that correctly uses the new
Error()accessor from theRequestContextinterface and properly guards against duplicate error detection.
34-50: LGTM!The
WriteHeaderandWriteoverrides correctly handle the header interception. TheWritemethod properly relies on Go's implicit 200 status behavior whenWriteHeaderisn't explicitly called.
52-89: LGTM!The
Flushimplementation correctly supports streaming by delegating to the underlying writer. TheRouterOnRequesthandler andModulefunction follow the expected patterns for Cosmo router modules. The interface guard provides compile-time verification.
…-streams-edfs-mutations
dkorittki
force-pushed
the
dominik/eng-8647-__typename-is-null-on-cosmo-streams-edfs-mutations
branch
from
0e863ca to
fc44889
Compare
|
I did a force push to correct a messed up merge-from-main commit. It was a fast forward without merge commit but I want it to be a merge commit, which it now is. |
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist
Context
When a GraphQL operation calls a Cosmo Streams backed mutation and include
__typenameas a field the router does not resolve that field and instead tries to returnnull.The operation:
The response:
{ "errors": [ { "message": "Cannot return null for non-nullable field 'Mutation.updateEmployeeMyKafka.__typename'.", "path": [ "updateEmployeeMyKafka", "__typename" ] } ], }The SDL must define the type
edfs__PublishResultas named return on Cosmo Streams mutations.Expected is, that
__typenameresolves toedfs__PublishResultinstead ofnull.Affected are all EDFS / Cosmo Streams backed mutations, not only Kafka.
This pull request adds the correct return type on the place where we create the response event, adjusts the unit tests to expect this and add router-tests to test this as well.