feat: add composite type support for field resolvers#2368
Conversation
WalkthroughAdds two computed Project fields (criticalDeadline, topPriorityItem) with service implementations, fixes resolver-argument generation for repeated types in protographic, expands tests in router and protographic suites, and bumps graphql-go-tools dependency pins in router modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-08-20T22:13:25.222ZApplied to files:
📚 Learning: 2025-09-24T12:54:00.765ZApplied to files:
📚 Learning: 2025-09-24T12:54:00.765ZApplied to files:
📚 Learning: 2025-10-01T20:39:16.113ZApplied to files:
📚 Learning: 2025-08-15T10:21:45.838ZApplied to files:
📚 Learning: 2025-08-20T10:08:17.857ZApplied to files:
📚 Learning: 2025-11-19T15:13:57.821ZApplied to files:
⏰ 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)
🔇 Additional comments (5)
Comment |
Router-nonroot 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 #2368 +/- ##
=======================================
Coverage ? 32.01%
=======================================
Files ? 213
Lines ? 23242
Branches ? 0
=======================================
Hits ? 7442
Misses ? 14877
Partials ? 923 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…297-add-interface-support-for-field-resolvers
Noroth
requested review from
a team,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
demo/pkg/subgraphs/projects/src/service/service.go (2)
1198-1213: Avoid returning(nil, nil)from QueryProject gRPC handler — it causes marshal failureReturning
(nil, nil)from a gRPC handler is invalid. The gRPC contract states thaterror == nilsignals an OK status, but the server still attempts to marshal the response. When the response isnil, the encode path fails — resulting in a marshal error or runtime panic. The client receives an error and no response, violating the handler's contract.Return a concrete response object instead:
- } - - return nil, nil + } + + return &service.QueryProjectResponse{Project: nil}, nilThis preserves the "not found → null, no error" behavior while maintaining the gRPC invariant that
error == nilpairs with a marshalable response.
5-211: Use signed date difference to correctly handle "upcoming" deadline semantics
ResolveProjectCriticalDeadlinecomputes:daysUntil := int(math.Abs(endDate.Sub(now).Hours() / 24))This treats past and future deadlines identically. Any milestone or project within
withinDaysof "now"—whether expired or upcoming—will match. This contradicts the code comment "Find the nearest upcoming deadline that's within the specified days" and the function's purpose.All fixture milestones and projects have
EndDatevalues in the past (e.g., 2021–2025-08-20), so this behavior will return expired deadlines as "critical" when they should be ignored.Replace
math.Abswith a signed difference to fix:daysUntil := int(endDate.Sub(now).Hours() / 24) // Then check: daysUntil >= 0 && daysUntil <= withinDaysThis ensures only upcoming deadlines are considered critical.
🧹 Nitpick comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1291-1301: Propagating full ProtoType into resolver context message is correctUsing
...this.getProtoTypeFromGraphQL(field.type)here letsbuildMessagehonorisRepeated, so context fields for list‑typed parent fields (including nested/wrapper cases) are now emitted with the rightrepeated/wrapper semantics. This aligns with the new tests around list contexts.You may also want to apply the same pattern to the
Resolve*Argsmessage above so list‑typed resolver arguments, if introduced later, are also emitted asrepeatedrather than scalar.Based on learnings, this still keeps resolver request/response messages independent of the proto lock, as they’re only built via
buildMessageand never reconciled viaProtoLockManager.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
demo/pkg/subgraphs/projects/generated/mapping.jsonis excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**demo/pkg/subgraphs/projects/generated/service.protois excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service.proto.lock.jsonis excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
demo/pkg/subgraphs/projects/src/schema.graphql(1 hunks)demo/pkg/subgraphs/projects/src/service/service.go(3 hunks)protographic/src/sdl-to-proto-visitor.ts(1 hunks)protographic/tests/sdl-to-proto/13-field-arguments.test.ts(1 hunks)router-tests/go.mod(2 hunks)router-tests/grpc_subgraph_test.go(1 hunks)router-tests/router_plugin_test.go(1 hunks)router/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-10-27T09:45:41.622Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.
Applied to files:
protographic/tests/sdl-to-proto/13-field-arguments.test.tsprotographic/src/sdl-to-proto-visitor.ts
📚 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/grpc_subgraph_test.gorouter-tests/router_plugin_test.gorouter-tests/go.mod
📚 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-tests/grpc_subgraph_test.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/go.modrouter-tests/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:
router/go.modrouter-tests/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.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
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
protographic/src/sdl-to-proto-visitor.ts
📚 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-tests/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-tests/go.mod
🧬 Code graph analysis (2)
protographic/tests/sdl-to-proto/13-field-arguments.test.ts (2)
protographic/src/index.ts (1)
compileGraphQLToProto(53-79)protographic/tests/util.ts (1)
expectValidProto(29-31)
demo/pkg/subgraphs/projects/src/service/service.go (3)
demo/pkg/subgraphs/projects/src/data/projects.go (3)
GetMilestonesByProjectID(169-177)GetProjectByID(159-166)GetTasksByProjectID(180-188)demo/pkg/subgraphs/projects/src/data/milestones.go (1)
PopulateMilestoneRelationships(210-226)demo/pkg/subgraphs/projects/src/data/tasks.go (1)
PopulateTaskRelationships(326-349)
⏰ 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). (6)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
protographic/tests/sdl-to-proto/13-field-arguments.test.ts (1)
106-296: Good coverage for resolver context list and nested list handlingThe two added cases validate:
- simple non‑nullable list context fields (
posts: [Post!]!) becomingrepeated Post postsin both context and base messages, and- nullable / nested list contexts using
ListOfPostandListOfListOfCategorywrappers consistently.Together with
expectValidProto, this is a solid regression net around the new resolver‑context generation behavior.router-tests/router_plugin_test.go (1)
378-438: Plugin-side integration tests thoroughly cover new field resolversThe added
TestRouterPluginRequestscases fortopPriorityItemandcriticalDeadline(including category filtering, milestone vs task vs project fallback, nested fragments, and aliases) match the service logic and ensure the plugin path exercises the new resolvers end‑to‑end. No issues spotted.demo/pkg/subgraphs/projects/src/schema.graphql (1)
108-114: Schema wiring for new Project field resolvers looks consistent
criticalDeadlineandtopPriorityItemare correctly typed:
criticalDeadline(withinDays: Int): Timestampedmatches theTimestampedinterface implemented by bothProjectandMilestone.topPriorityItem(category: String): ProjectSearchResultmatches theProjectSearchResultunion.The resolver contexts (
"id status milestones"and"id status") reference existing fields onProjectand align with how the service methods consumeIdandStatus. No issues from the SDL side.router-tests/grpc_subgraph_test.go (1)
181-235: GRPC subgraph tests mirror plugin resolver scenarios effectivelyThe added cases for
topPriorityItemandcriticalDeadline(including category filters, fallback to project, nested interface/union fragments, and aliasing) give the gRPC subgraph path the same coverage as the router‑plugin tests, which is valuable for catching divergences between the two execution paths. The expected payloads align with the new service logic.router/go.mod (1)
34-34: graphql-go-tools/v2 versions are synchronized across router and router-testsBoth
router/go.mod(line 34) androuter-tests/go.mod(line 30) referencev2.0.0-rc.239.0.20251202082452-79629bfd1d55, keeping the critical dependency aligned. This consistency is important for test reliability.router-tests/go.mod (1)
30-30: Dependency versions are correct and aligned
graphql-go-tools/v2is at the same version asrouter/go.mod(v2.0.0-rc.239.0.20251202082452-79629bfd1d55), andmcp-goremains at the stablev0.36.0as required.
|
Using temporary engine version to test CI |
Noroth
deleted the
ludwig/eng-8297-add-interface-support-for-field-resolvers
branch
2 participants
This PR adds support for using composite type in field resolvers.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist