feat: initial support for field resolvers in connect #2290
Conversation
WalkthroughAdds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (16)
Comment |
Noroth
force-pushed
the
ludwig/eng-6993-implement-field-resolver-for-graphql-operation
branch
from
f159069 to
6d68c30
Compare
Router image scan passed✅ No security vulnerabilities found in image: |
Noroth
requested review from
a team,
Aenimus,
JivusAyrus,
StarpTech,
devsergiy,
endigma,
jensneuse and
wilsonrivera
as code owners
![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 (2)
protographic/src/sdl-to-mapping-visitor.ts (1)
455-466: Outdated JSDoccreateFieldMapping no longer takes type, but the comment still documents it. Update the doc to match the new signature.
protographic/src/sdl-to-proto-visitor.ts (1)
1410-1416: Root Subscription type not skippedqueueAllSchemaTypes skips Query/Mutation/_Entity, but not Subscription. This will generate a “message Subscription { … }” unexpectedly.
Apply:
- typeName === 'Query' || - typeName === 'Mutation' || + typeName === 'Query' || + typeName === 'Mutation' || + typeName === 'Subscription' || typeName === '_Entity' ||
🧹 Nitpick comments (17)
demo/pkg/subgraphs/employees/employees.go (1)
18-22: Use a custom type for the context key.Using a string literal as a context key can lead to collisions. Go best practice recommends defining a custom unexported type for context keys.
Consider this pattern:
+type contextKey string + +const openfedRequireFetchReasonsKey contextKey = "Openfed__requireFetchReasons" + func NewSchema(natsPubSubByProviderID map[string]nats.Adapter) graphql.ExecutableSchema { return generated.NewExecutableSchema(generated.Config{ Resolvers: &subgraph.Resolver{ NatsPubSubByProviderID: natsPubSubByProviderID, EmployeesData: subgraph.Employees, }, Directives: generated.DirectiveRoot{ Openfed__requireFetchReasons: func(ctx context.Context, obj any, next graphql.Resolver) (res any, err error) { - return next(context.WithValue(ctx, "Openfed__requireFetchReasons", obj)) + return next(context.WithValue(ctx, openfedRequireFetchReasonsKey, obj)) }, }, }) }router/core/factoryresolver.go (1)
671-688: Consider nil checks for nested field access.The logic correctly implements a get-or-create pattern for the nested map structure. However, accessing
resolve.LookupMapping.FieldMappingwithout nil checks could cause a panic if the proto message is malformed.While this is consistent with similar loops in this file (e.g., lines 660-669, 690-701), adding defensive checks would improve robustness:
for _, resolve := range in.ResolveMappings { + if resolve.LookupMapping == nil || resolve.LookupMapping.FieldMapping == nil { + continue + } resolveMap, ok := result.ResolveRPCs[resolve.LookupMapping.Type]router-tests/grpc_subgraph_test.go (1)
166-180: Prefer structural JSON assertions and dedup test fixtures
- Use require.JSONEq to avoid brittle string comparisons.
- These cases duplicate in router_plugin_test.go; consider extracting shared fixtures/helpers to keep expectations in one place.
- require.Equal(t, test.expected, response.Body) + require.JSONEq(t, test.expected, response.Body)router-tests/router_plugin_test.go (1)
307-321: Mirror of GRPC tests: switch to JSONEq and centralize cases
- Replace string equality with require.JSONEq for resilience.
- Extract the resolver test table to a shared helper to avoid drift between plugin and grpc suites.
- require.Equal(t, test.expected, response.Body) + require.JSONEq(t, test.expected, response.Body)proto/wg/cosmo/node/v1/node.proto (3)
230-247: New GRPCMapping.resolve_mappings: versioning and migration noteField addition is backward-compatible. Consider documenting when Engine/Router should start honoring resolve_mappings vs ignoring them, and whether GRPCMapping.version needs semantics beyond structural versioning.
262-269: Naming consistency for type fieldYou use string type here; elsewhere both type and type_name exist. Consider standardizing (type vs type_name) across new messages for consistency.
270-278: LookupType enum: reserve space for futureEnum is fine. Optionally reserve additional values for future expansion to avoid renumbering later.
demo/pkg/subgraphs/projects/src/service/service.go (2)
74-80: Consider logging date parsing errors.Date parsing errors are silently ignored, which could make debugging difficult if invalid timestamps are present in the data. Consider logging these errors or tracking them.
createdAt, err1 := time.Parse(time.RFC3339, task.CreatedAt.Value) completedAt, err2 := time.Parse(time.RFC3339, task.CompletedAt.Value) - if err1 == nil && err2 == nil { + if err1 != nil || err2 != nil { + // Consider logging: logger.Warn("Failed to parse task timestamps", "taskId", task.Id, "err1", err1, "err2", err2) + continue + } - days := completedAt.Sub(createdAt).Hours() / 24 - totalDays += days - taskCount++ - } + days := completedAt.Sub(createdAt).Hours() / 24 + totalDays += days + taskCount++
154-158: Date parsing errors silently ignored.Similar to the previous handler, date parsing errors for
fromDateare silently ignored. If an invalid date format is provided in the field arguments, the function will fall back totime.Now()without any indication of the error.Consider logging when date parsing fails to help with debugging.
protographic/src/sdl-to-mapping-visitor.ts (1)
98-100: Method name spelling and scopeRename to processResolvableFields (single “a”) for consistency, and consider gating on the presence of @connect__fieldResolver to avoid generating resolver RPCs for every field-with-args by default (if that’s not intended).
Also applies to: 221-238
protographic/src/sdl-to-proto-visitor.ts (1)
1244-1327: Stabilize field numbering with the proto lock for new resolver messagesArgs/Context/Request for resolvers assign sequential numbers (++fieldNumber) and don’t reconcile with the lock. This can churn numbers across schema evolutions.
Suggested minimal changes:
- Before building each message, reconcile field order with the lock.
- Use getFieldNumber(...) instead of ++fieldNumber.
Example for Args/Context/Request:
- let fieldNumber = 0; + let fieldNumber = 0; + const argProtoNames = field.args.map((a) => graphqlFieldToProtoField(a.name)); + this.lockManager.reconcileMessageFieldOrder(argsMessageName, argProtoNames); messageLines.push( ...this.buildMessage({ messageName: argsMessageName, - fields: field.args.map((arg) => ({ - fieldName: graphqlFieldToProtoField(arg.name), - typeName: this.getProtoTypeFromGraphQL(arg.type).typeName, - fieldNumber: ++fieldNumber, - })), + fields: field.args.map((arg) => { + const proto = graphqlFieldToProtoField(arg.name); + return { + fieldName: proto, + typeName: this.getProtoTypeFromGraphQL(arg.type).typeName, + fieldNumber: this.getFieldNumber(argsMessageName, proto, ++fieldNumber), + }; + }), }), ); - fieldNumber = 0; + fieldNumber = 0; + const contextProtoNames = fieldFilter.map((f) => graphqlFieldToProtoField(f.name)); + this.lockManager.reconcileMessageFieldOrder(contextMessageName, contextProtoNames); messageLines.push( ...this.buildMessage({ messageName: contextMessageName, - fields: fieldFilter.map((field) => ({ - fieldName: field.name, - typeName: this.getProtoTypeFromGraphQL(field.type).typeName, - fieldNumber: ++fieldNumber, - })), + fields: fieldFilter.map((f) => { + const proto = graphqlFieldToProtoField(f.name); + return { + fieldName: proto, + typeName: this.getProtoTypeFromGraphQL(f.type).typeName, + fieldNumber: this.getFieldNumber(contextMessageName, proto, ++fieldNumber), + }; + }), }), ); - fieldNumber = 0; + fieldNumber = 0; + this.lockManager.reconcileMessageFieldOrder(requestName, [CONTEXT, FIELD_ARGS]); let keyMessageFields: ProtoMessageField[] = []; // add the context message to the key message keyMessageFields.push({ fieldName: CONTEXT, typeName: contextMessageName, - fieldNumber: ++fieldNumber, + fieldNumber: this.getFieldNumber(requestName, CONTEXT, ++fieldNumber), isRepeated: true, description: `${CONTEXT} provides the resolver context for the field ${field.name} of type ${parent.name}.`, }); // add the args message to the key message keyMessageFields.push({ fieldName: FIELD_ARGS, typeName: argsMessageName, - fieldNumber: ++fieldNumber, + fieldNumber: this.getFieldNumber(requestName, FIELD_ARGS, ++fieldNumber), description: `${FIELD_ARGS} provides the arguments for the resolver field ${field.name} of type ${parent.name}.`, });Do similarly for createResolverResponseMessage: reconcile [RESULT] and use getFieldNumber.
composition/src/v1/normalization/walkers.ts (1)
85-92: Prevent duplicate directive definitions if subgraphs declare @connect__fieldResolverIf a subgraph also defines this directive, normalization may emit both the subgraph’s definition and the built-in one. Add CONNECT_CONFIGURE_RESOLVER to ALL_IN_BUILT_DIRECTIVE_NAMES so custom definitions are ignored and duplicates avoided. Apply in composition/src/v1/utils/constants.ts.
--- a/composition/src/v1/utils/constants.ts +++ b/composition/src/v1/utils/constants.ts @@ export const ALL_IN_BUILT_DIRECTIVE_NAMES = new Set<DirectiveName>([ AUTHENTICATED, COMPOSE_DIRECTIVE, CONFIGURE_DESCRIPTION, CONFIGURE_CHILD_DESCRIPTIONS, + CONNECT_CONFIGURE_RESOLVER, DEPRECATED, EDFS_NATS_PUBLISH, EDFS_NATS_REQUEST, EDFS_NATS_SUBSCRIBE, EDFS_KAFKA_PUBLISH,composition/tests/v1/directives/connect-configure-resolver.test.ts (2)
20-26: Strengthen assertion to verify definition presence programmaticallyIn addition to string contains, assert the directive is registered in directiveDefinitionByDirectiveName to reduce snapshot brittleness.
- expect(normalizationSuccess.subgraphString).toContain( - `directive @connect__fieldResolver(context: openfed__FieldSet!) on FIELD_DEFINITION`, - ); + expect(normalizationSuccess.directiveDefinitionByDirectiveName.has('connect__fieldResolver')).toBe(true);
9-9: Remove unused importprintSchema is imported but unused.
-import { parse, printSchema } from 'graphql'; +import { parse } from 'graphql';composition/src/v1/utils/constants.ts (2)
502-532: Treat @connect__fieldResolver as in-built to avoid subgraph overridesAdd CONNECT_CONFIGURE_RESOLVER to ALL_IN_BUILT_DIRECTIVE_NAMES so custom subgraph definitions are ignored and we emit the canonical one only.
export const ALL_IN_BUILT_DIRECTIVE_NAMES = new Set<DirectiveName>([ AUTHENTICATED, COMPOSE_DIRECTIVE, CONFIGURE_DESCRIPTION, CONFIGURE_CHILD_DESCRIPTIONS, + CONNECT_CONFIGURE_RESOLVER, DEPRECATED,
49-50: Minor: remove unused FIELD importFIELD is imported but unused in this file.
- FIELD, FIELD_DEFINITION_UPPER,protographic/tests/sdl-to-proto/13-field-arguments.test.ts (1)
576-585: Test name duplicationTwo tests share the same description “should correctly handle multiple same return types…”. Rename one to clarify scope (e.g., “with list result” vs “with multi-field context”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**demo/pkg/subgraphs/employees/subgraph/generated/generated.gois excluded by!**/generated/**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/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
composition/src/utils/string-constants.ts(1 hunks)composition/src/v1/normalization/directive-definition-data.ts(3 hunks)composition/src/v1/normalization/normalization-factory.ts(3 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/src/v1/normalization/walkers.ts(3 hunks)composition/src/v1/utils/constants.ts(3 hunks)composition/tests/v1/directives/connect-configure-resolver.test.ts(1 hunks)connect/src/wg/cosmo/node/v1/node_pb.ts(5 hunks)demo/pkg/subgraphs/employees/employees.go(1 hunks)demo/pkg/subgraphs/projects/src/schema.graphql(4 hunks)demo/pkg/subgraphs/projects/src/service/service.go(1 hunks)proto/wg/cosmo/node/v1/node.proto(2 hunks)protographic/src/index.ts(1 hunks)protographic/src/naming-conventions.ts(4 hunks)protographic/src/sdl-to-mapping-visitor.ts(9 hunks)protographic/src/sdl-to-proto-visitor.ts(9 hunks)protographic/src/sdl-validation-visitor.ts(10 hunks)protographic/src/string-constants.ts(1 hunks)protographic/tests/sdl-to-mapping/04-field-resolvers.test.ts(1 hunks)protographic/tests/sdl-to-proto/04-federation.test.ts(1 hunks)protographic/tests/sdl-to-proto/13-field-arguments.test.ts(1 hunks)protographic/tests/sdl-validation/01-basic-validation.test.ts(6 hunks)router-tests/go.mod(1 hunks)router-tests/grpc_subgraph_test.go(1 hunks)router-tests/router_plugin_test.go(1 hunks)router/core/factoryresolver.go(3 hunks)router/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T13:00:30.288Z
Learnt from: Noroth
PR: wundergraph/cosmo#2108
File: protographic/tests/sdl-validation/01-basic-validation.test.ts:20-57
Timestamp: 2025-08-05T13:00:30.288Z
Learning: In the protographic SDL validation system, the validation logic for nullable list items only refers to the actual list type and not the nesting. For example, `[[String!]]` (nullable list of non-nullable lists) would not trigger a nullable list warning because the validation focuses on the immediate list structure rather than nested list structures.
Applied to files:
protographic/tests/sdl-validation/01-basic-validation.test.ts
📚 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-tests/go.mod
🧬 Code graph analysis (18)
protographic/tests/sdl-to-mapping/04-field-resolvers.test.ts (1)
protographic/src/index.ts (1)
compileGraphQLToMapping(16-32)
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)
composition/src/utils/string-constants.ts (1)
protographic/src/string-constants.ts (2)
CONNECT_CONFIGURE_RESOLVER(1-1)CONTEXT(2-2)
protographic/tests/sdl-validation/01-basic-validation.test.ts (1)
protographic/src/sdl-validation-visitor.ts (1)
SDLValidationVisitor(87-639)
composition/src/v1/utils/constants.ts (2)
composition/src/ast/utils.ts (2)
stringToNameNode(77-82)stringArrayToNameNodeArray(84-90)composition/src/utils/string-constants.ts (3)
CONTEXT(21-21)FIELD_DEFINITION_UPPER(56-56)CONNECT_CONFIGURE_RESOLVER(20-20)
composition/src/v1/normalization/normalization-factory.ts (2)
composition/src/utils/string-constants.ts (1)
CONNECT_CONFIGURE_RESOLVER(20-20)composition/src/v1/utils/constants.ts (1)
CONNECT_CONFIGURE_RESOLVER_DEFINITION(785-797)
protographic/src/string-constants.ts (1)
composition/src/utils/string-constants.ts (2)
CONNECT_CONFIGURE_RESOLVER(20-20)CONTEXT(21-21)
demo/pkg/subgraphs/employees/employees.go (1)
demo/pkg/subgraphs/employees/subgraph/resolver.go (1)
Resolver(16-20)
composition/src/v1/normalization/utils.ts (2)
composition/src/utils/string-constants.ts (1)
CONNECT_CONFIGURE_RESOLVER(20-20)composition/src/v1/normalization/directive-definition-data.ts (1)
CONNECT_CONFIGURE_RESOLVER_DEFINITION_DATA(211-227)
composition/tests/v1/directives/connect-configure-resolver.test.ts (3)
composition/src/v1/normalization/normalization-factory.ts (1)
normalizeSubgraph(393-400)composition/src/router-compatibility-version/router-compatibility-version.ts (1)
ROUTER_COMPATIBILITY_VERSION_ONE(3-3)composition/src/normalization/types.ts (2)
NormalizationSuccess(22-45)NormalizationFailure(16-20)
composition/src/v1/normalization/directive-definition-data.ts (3)
composition/src/schema-building/types.ts (2)
DirectiveDefinitionData(41-50)ArgumentData(30-34)composition/src/utils/string-constants.ts (3)
CONTEXT(21-21)FIELD_DEFINITION_UPPER(56-56)CONNECT_CONFIGURE_RESOLVER(20-20)composition/src/v1/utils/constants.ts (2)
REQUIRED_FIELDSET_TYPE_NODE(321-324)CONNECT_CONFIGURE_RESOLVER_DEFINITION(785-797)
router/core/factoryresolver.go (2)
connect/src/wg/cosmo/node/v1/node_pb.ts (4)
LookupMapping(1860-1926)FieldMapping(2185-2235)Response(447-487)ArgumentMapping(2242-2284)connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (12)
LookupMapping(2471-2486)LookupMapping(2501-2501)LookupMapping(2516-2518)FieldMapping(2851-2862)FieldMapping(2877-2877)FieldMapping(2892-2894)Response(683-691)Response(706-706)Response(721-723)ArgumentMapping(2918-2927)ArgumentMapping(2942-2942)ArgumentMapping(2957-2959)
protographic/src/sdl-to-mapping-visitor.ts (3)
protographic/src/naming-conventions.ts (3)
createResolverMethodName(36-38)createRequestMessageName(43-45)createResponseMessageName(50-52)connect/src/wg/cosmo/node/v1/node_pb.ts (3)
LookupMapping(1860-1926)FieldMapping(2185-2235)LookupFieldMapping(1933-1975)protographic/src/index.ts (1)
FieldMapping(107-107)
protographic/src/sdl-to-proto-visitor.ts (2)
protographic/src/naming-conventions.ts (7)
createResolverMethodName(36-38)createRequestMessageName(43-45)createResponseMessageName(50-52)typeFieldArgsName(97-99)typeFieldContextName(107-109)graphqlFieldToProtoField(18-20)resolverResponseResultName(87-89)protographic/src/string-constants.ts (4)
CONTEXT(2-2)CONNECT_CONFIGURE_RESOLVER(1-1)FIELD_ARGS(3-3)RESULT(4-4)
demo/pkg/subgraphs/projects/src/service/service.go (3)
demo/pkg/subgraphs/projects/generated/service.pb.go (63)
ResolveEmployeeAverageTaskCompletionDaysRequest(5777-5786)ResolveEmployeeAverageTaskCompletionDaysRequest(5801-5801)ResolveEmployeeAverageTaskCompletionDaysRequest(5816-5818)ResolveEmployeeAverageTaskCompletionDaysResult(5834-5840)ResolveEmployeeAverageTaskCompletionDaysResult(5855-5855)ResolveEmployeeAverageTaskCompletionDaysResult(5870-5872)TaskPriority_TASK_PRIORITY_UNSPECIFIED(195-195)TaskStatus_TASK_STATUS_COMPLETED(141-141)ResolveEmployeeCurrentWorkloadRequest(5524-5533)ResolveEmployeeCurrentWorkloadRequest(5548-5548)ResolveEmployeeCurrentWorkloadRequest(5563-5565)ResolveEmployeeCurrentWorkloadResult(5581-5587)ResolveEmployeeCurrentWorkloadResult(5602-5602)ResolveEmployeeCurrentWorkloadResult(5617-5619)ResolveMilestoneDaysUntilDueRequest(4757-4766)ResolveMilestoneDaysUntilDueRequest(4781-4781)ResolveMilestoneDaysUntilDueRequest(4796-4798)ResolveMilestoneDaysUntilDueResult(4814-4820)ResolveMilestoneDaysUntilDueResult(4835-4835)ResolveMilestoneDaysUntilDueResult(4850-4852)ResolveMilestoneIsAtRiskRequest(4512-4521)ResolveMilestoneIsAtRiskRequest(4536-4536)ResolveMilestoneIsAtRiskRequest(4551-4553)ResolveMilestoneIsAtRiskResult(4569-4575)ResolveMilestoneIsAtRiskResult(4590-4590)ResolveMilestoneIsAtRiskResult(4605-4607)MilestoneStatus_MILESTONE_STATUS_DELAYED(86-86)MilestoneStatus_MILESTONE_STATUS_COMPLETED(85-85)ResolveProjectCompletionRateRequest(3982-3991)ResolveProjectCompletionRateRequest(4006-4006)ResolveProjectCompletionRateRequest(4021-4023)ResolveProjectCompletionRateResult(4039-4045)ResolveProjectCompletionRateResult(4060-4060)ResolveProjectCompletionRateResult(4075-4077)ResolveProjectEstimatedDaysRemainingRequest(4243-4252)ResolveProjectEstimatedDaysRemainingRequest(4267-4267)ResolveProjectEstimatedDaysRemainingRequest(4282-4284)ResolveProjectEstimatedDaysRemainingResult(4300-4306)ResolveProjectEstimatedDaysRemainingResult(4321-4321)ResolveProjectEstimatedDaysRemainingResult(4336-4338)ResolveProjectFilteredTasksRequest(3713-3722)ResolveProjectFilteredTasksRequest(3737-3737)ResolveProjectFilteredTasksRequest(3752-3754)ResolveProjectFilteredTasksResult(3770-3776)ResolveProjectFilteredTasksResult(3791-3791)ResolveProjectFilteredTasksResult(3806-3808)TaskStatus_TASK_STATUS_UNSPECIFIED(137-137)Task(6246-6271)Task(6286-6286)Task(6301-6303)ResolveTaskIsBlockedRequest(5010-5019)ResolveTaskIsBlockedRequest(5034-5034)ResolveTaskIsBlockedRequest(5049-5051)ResolveTaskIsBlockedResult(5067-5073)ResolveTaskIsBlockedResult(5088-5088)ResolveTaskIsBlockedResult(5103-5105)TaskStatus_TASK_STATUS_BLOCKED(142-142)ResolveTaskTotalEffortRequest(5271-5280)ResolveTaskTotalEffortRequest(5295-5295)ResolveTaskTotalEffortRequest(5310-5312)ResolveTaskTotalEffortResult(5328-5334)ResolveTaskTotalEffortResult(5349-5349)ResolveTaskTotalEffortResult(5364-5366)demo/pkg/subgraphs/projects/src/data/tasks.go (2)
ServiceTasks(8-218)GetTaskByID(316-323)demo/pkg/subgraphs/projects/src/data/projects.go (1)
GetTasksByProjectID(180-188)
connect/src/wg/cosmo/node/v1/node_pb.ts (2)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (13)
LookupType(169-169)LookupType(204-206)LookupType(208-210)LookupType(217-219)LookupMapping(2471-2486)LookupMapping(2501-2501)LookupMapping(2516-2518)LookupFieldMapping(2556-2565)LookupFieldMapping(2580-2580)LookupFieldMapping(2595-2597)FieldMapping(2851-2862)FieldMapping(2877-2877)FieldMapping(2892-2894)router/gen/proto/wg/cosmo/node/v1/node.pb.go (13)
LookupType(169-169)LookupType(204-206)LookupType(208-210)LookupType(217-219)LookupMapping(2471-2486)LookupMapping(2501-2501)LookupMapping(2516-2518)LookupFieldMapping(2556-2565)LookupFieldMapping(2580-2580)LookupFieldMapping(2595-2597)FieldMapping(2851-2862)FieldMapping(2877-2877)FieldMapping(2892-2894)
protographic/src/sdl-validation-visitor.ts (2)
composition/src/utils/string-constants.ts (2)
CONNECT_CONFIGURE_RESOLVER(20-20)CONTEXT(21-21)protographic/src/string-constants.ts (2)
CONNECT_CONFIGURE_RESOLVER(1-1)CONTEXT(2-2)
composition/src/v1/normalization/walkers.ts (2)
composition/src/utils/string-constants.ts (1)
CONNECT_CONFIGURE_RESOLVER(20-20)composition/src/v1/utils/constants.ts (1)
CONNECT_CONFIGURE_RESOLVER_DEFINITION(785-797)
🔇 Additional comments (29)
router/go.mod (1)
34-34: Dependency bump aligns with field resolver feature.The version update to
graphql-go-tools/v2with the pinned commit hash is appropriate. The pseudo-version indicates a specific feature commit that supports resolver/lookup mappings needed for the field resolver implementation described in the PR.demo/pkg/subgraphs/employees/employees.go (2)
4-5: LGTM: Import addition.The
contextimport is correctly added to support the new directive implementation.
13-17: LGTM: Config-based schema construction.The refactoring to use
generated.Configwith explicitResolversandDirectivesfields aligns well with gqlgen patterns and enables the new directive functionality.router/core/factoryresolver.go (3)
635-638: Good use of typed maps for improved type safety.The refactoring to use
grpcdatasource.RPCConfigMapwith generics is a solid improvement that will catch type mismatches at compile time.
696-696: Good refactoring to use the shared helper.This change ensures consistent argument mapping behavior across both regular field mappings and resolve mappings.
725-732: Clean helper function implementation.The function correctly converts the proto argument mappings into the internal map structure. The implementation handles a nil slice gracefully (the loop simply won't execute).
connect/src/wg/cosmo/node/v1/node_pb.ts (4)
82-115: Generated enum LookupType matches protoEnum metadata maps to LOOKUP_TYPE_*; symbols are concise (UNSPECIFIED/RESOLVE/REQUIRES). Looks correct.
1855-1926: LookupMapping message generation LGTMFields/types align with proto; standard buf TS patterns present.
1928-1975: LookupFieldMapping message generation LGTMMatches proto and integrates with existing FieldMapping.
1814-1836: No issues found—TS shape correctly aligns with proto definition and all consumers use the field consistently.The verification confirms that
GRPCMapping.resolveMappingsis properly defined and integrated. The field descriptor (field number 7, typeLookupMapping, repeated) matches the proto definition, and usage across the codebase is consistent with appropriate naming conventions per language (TypeScript:resolveMappings, Go:ResolveMappings, proto:resolve_mappings). No stale names or breaking changes detected.proto/wg/cosmo/node/v1/node.proto (1)
248-261: Based on the verification results, I can now address the review comment:Confirmed: ArgumentMapping and LookupMapping wiring are adequate for both RESOLVE and REQUIRES cases
The proto structure is properly designed:
LookupMapping.typedistinguishes betweenLOOKUP_TYPE_RESOLVEandLOOKUP_TYPE_REQUIRESLookupFieldMappingcontains field mappings withArgumentMapping(mapping source GraphQL argument names to target gRPC request field names)- In federation patterns, parent/context data flows separately as the entity representation itself;
ArgumentMappingcorrectly handles argument-to-field transformation only- The @requires directive specifies external entity fields needed to resolve a target field and is used in query planning where required fields may not be needed by the client but the service needs additional information from other subgraphs
- The factory resolver in
router/core/factoryresolver.gocorrectly routes byLookupMapping.Typeand appliesArgumentMappingsto construct gRPC request messagesNo additional parent/context/selection_set fields are needed in
ArgumentMapping—parent object fields are passed as part of the entity representation in the request/response messages, not through argument mappings.demo/pkg/subgraphs/projects/src/service/service.go (1)
41-437: LGTM! Comprehensive implementation of field resolvers.The nine new RPC handlers are well-implemented with:
- Consistent thread-safe patterns using read locks
- Proper handling of optional field arguments
- Correct business logic for calculations and filtering
- Appropriate use of nullable wrapper types
The implementations align well with the proto definitions and provide the necessary computed fields for the GraphQL schema.
protographic/tests/sdl-to-proto/04-federation.test.ts (1)
1-1: Minor import addition.Added
itto the vitest imports for consistency with testing conventions. No functional changes.composition/src/v1/normalization/utils.ts (1)
28-28: Correct directive wiring for CONNECT_CONFIGURE_RESOLVER.The new directive is properly integrated into the directive definitions map following the established pattern. The imports and initialization are consistent with other directives in the system.
Also applies to: 60-60, 399-399
composition/src/utils/string-constants.ts (1)
20-21: New string constants properly defined.The new constants follow the existing naming conventions and use the appropriate
connect__namespace prefix for the directive name.composition/src/v1/normalization/directive-definition-data.ts (1)
211-227: Directive definition follows established patterns.The
CONNECT_CONFIGURE_RESOLVER_DEFINITION_DATAis properly structured and consistent with similar directives likePROVIDESandREQUIRES. The configuration is appropriate:
- Single required
contextargument of typeopenfed__FieldSet!- Non-repeatable
- Applied to field definitions only
protographic/tests/sdl-to-mapping/04-field-resolvers.test.ts (1)
1-115: Comprehensive test coverage for field resolver mappings.The test validates the complete SDL-to-mapping transformation for field resolvers, including:
- Operation mappings for queries
- Resolve mappings with lookup configuration
- Type field mappings with argument handling
- Proper handling of the
@connect__fieldResolverdirectiveThe inline snapshot approach provides clear visibility into the expected output structure.
protographic/src/string-constants.ts (1)
1-4: String constants for protographic package.These constants provide standardized strings for the protographic module. Note that
CONNECT_CONFIGURE_RESOLVERandCONTEXTare also defined in the composition package, which is expected since these are separate packages with different responsibilities.protographic/tests/sdl-validation/01-basic-validation.test.ts (1)
134-447: Excellent test coverage for resolver context validation.The new tests thoroughly validate the
@connect__fieldResolverdirective behavior, including:
- Invalid resolver context scenarios
- Empty context handling with ID field fallback
- Multiple ID field disambiguation
- Cycle detection in resolver dependencies
- Invalid field references in context
The cycle detection tests (lines 403-447) are particularly valuable for preventing difficult-to-debug runtime issues.
protographic/src/naming-conventions.ts (1)
36-38: Resolver method naming looks goodPrefix + Parent + Field pattern is clear and consistent.
protographic/src/sdl-to-proto-visitor.ts (1)
1487-1526: Good call: skip object fields with args in type messagesAvoids duplicating resolver-handled fields in object messages.
demo/pkg/subgraphs/projects/src/schema.graphql (1)
108-112: Directive definition presenceSchema uses @connect__fieldResolver but it isn’t imported in the @link stanza here. If composition injects this directive globally, fine; otherwise add the directive definition/import to keep SDL self-contained.
Also applies to: 130-133, 156-159, 230-233
protographic/src/sdl-validation-visitor.ts (1)
195-205: Visitor refactor reads cleanPassing a VisitContext to rules keeps handlers simple and typed.
Also applies to: 211-214
protographic/src/sdl-to-mapping-visitor.ts (1)
298-310: The review comment is incorrect.The
resolveMappingsproperty in theGRPCMappingclass is already initialized to an empty array by default. The class definition (connect/src/wg/cosmo/node/v1/node_pb.ts:1819) showsresolveMappings: LookupMapping[] = [];, which means the property will always be an array when a newGRPCMappinginstance is created. The.push()call at line 309 is safe and will not throw at runtime. No initialization is needed in the constructor.Likely an incorrect or invalid review comment.
composition/src/v1/normalization/walkers.ts (1)
85-92: Good wiring for CONNECT_CONFIGURE_RESOLVER discoverySwitch-case correctly registers the definition in nf.directiveDefinitionByDirectiveName and marks it referenced. LGTM.
composition/tests/v1/directives/connect-configure-resolver.test.ts (1)
36-41: Error message assertion looks right; keep it resilientUsing toContain on the normalized message is fine. Consider also asserting errors.length === 1 (already present) to catch extra validation noise.
composition/src/v1/utils/constants.ts (1)
784-797: Directive definition looks correctcontext is non-null openfed__FieldSet; location FIELD_DEFINITION; non-repeatable. Matches intended semantics. LGTM.
protographic/tests/sdl-to-proto/13-field-arguments.test.ts (1)
5-104: Comprehensive coverage for args/context — niceSnapshots exercise args, context messages, and service RPCs. Looks good.
composition/src/v1/normalization/normalization-factory.ts (1)
3504-3507: Verification confirmed — all checks pass
- ✓
contextargument is present and correctly marked required (requiredArgumentNames: new Set([CONTEXT]))- ✓ Location is
FIELD_DEFINITION(viaFIELD_DEFINITION_UPPER)- ✓ Directive-definition-data properly structured with no optional arguments
- ✓ Conditional push pattern at lines 3504–3507 is correct and consistent
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
protographic/src/sdl-validation-visitor.ts (4)
223-230: Type assertion bypasses safety—consider adding documentation.The type assertion at line 228 is necessary because TypeScript cannot narrow the union type of validation functions based on the runtime
nodeKindfilter. However, this bypasses type checking.Consider adding a comment explaining why the assertion is safe:
private executeValidationRules(ctx: VisitContext<ASTNode>): void { const applicableRules = this.lintingRules.filter((rule) => rule.nodeKind === ctx.node.kind && rule.enabled); for (const rule of applicableRules) { - // Type assertion is safe here because we've filtered by nodeKind + // Type assertion is safe: the filter ensures ctx.node.kind matches rule.nodeKind, + // guaranteeing the node type is compatible with the validation function's expected type (rule.validationFunction as any)(ctx); } }
353-356: Warning message could be more precise.This warning is shown when
fieldNames.length === 0(fromgetContextFields), which occurs in three cases:
- The
@connect__fieldResolverdirective is completely absent- The directive exists but has no
contextargument- The directive exists with an empty
contextvalueThe current message implies case 1, but the actual condition covers all three. Since the resolver will fall back to the ID field in all cases, the message is functionally correct but could be clearer for developers debugging their schema.
Consider:
this.addWarning( - `No @${CONNECT_CONFIGURE_RESOLVER} directive found on the field ${ctx.node.name.value} - falling back to ID field`, + `No resolver context specified for field ${ctx.node.name.value} - falling back to ID field`, ctx.node.loc, );
536-551: Minor: Redundant null-check in Array.from.At line 549,
parent.fieldshas already been checked for existence and length at line 544, so the?? []fallback is unnecessary:private getParentFields(parent: ASTNode | ReadonlyArray<ASTNode>): { fields: FieldDefinitionNode[]; error: string } { const result: { fields: FieldDefinitionNode[]; error: string } = { fields: [], error: '' }; if (!this.isASTObjectTypeNode(parent)) { result.error = 'Invalid context provided for resolver. Could not determine parent type'; return result; } if (!parent.fields || parent.fields.length === 0) { result.error = 'Invalid context provided for resolver. Parent type has no fields'; return result; } - result.fields = Array.from(parent.fields ?? []); + result.fields = Array.from(parent.fields); return result; }
569-576: Variable naming inconsistency in lambda.The
findcallback uses parameter namegatebut the method operates onlintingRules. While functional, this inconsistency suggests leftover naming from the FeatureGate → LintingRule refactor:public configureRule(ruleName: string, enabled: boolean): boolean { - const rule = this.lintingRules.find((gate) => gate.name === ruleName); + const rule = this.lintingRules.find((rule) => rule.name === ruleName); if (rule) { rule.enabled = enabled; return true; } return false; }
📜 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 (14)
composition/src/v1/constants/constants.ts(3 hunks)composition/src/v1/constants/directive-definitions.ts(2 hunks)composition/src/v1/constants/strings.ts(2 hunks)composition/src/v1/normalization/directive-definition-data.ts(3 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/tests/v1/directives/connect-configure-resolver.test.ts(1 hunks)composition/tests/v1/utils/utils.ts(1 hunks)protographic/src/sdl-validation-visitor.ts(9 hunks)protographic/tests/sdl-validation/01-basic-validation.test.ts(6 hunks)router-tests/go.mod(1 hunks)router-tests/mcp_test.go(1 hunks)router/go.mod(1 hunks)router/pkg/schemaloader/loader_test.go(3 hunks)router/pkg/schemaloader/schema_builder.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- router/pkg/schemaloader/schema_builder.go
- router/pkg/schemaloader/loader_test.go
- router-tests/mcp_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- protographic/tests/sdl-validation/01-basic-validation.test.ts
- router/go.mod
- composition/src/v1/normalization/directive-definition-data.ts
- composition/tests/v1/directives/connect-configure-resolver.test.ts
🧰 Additional context used
🧠 Learnings (9)
📚 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-tests/go.mod
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
PR: wundergraph/cosmo#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/go.mod
📚 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-tests/go.mod
📚 Learning: 2025-08-15T10:21:45.838Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2142
File: helm/cosmo/Chart.yaml:0-0
Timestamp: 2025-08-15T10:21:45.838Z
Learning: In the WunderGraph Cosmo project, helm chart version upgrades and README badge synchronization are handled in separate helm release PRs, not in the initial version bump PRs.
Applied to files:
router-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-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-tests/go.mod
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#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-validation-visitor.ts
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
PR: wundergraph/cosmo#2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
protographic/src/sdl-validation-visitor.ts
📚 Learning: 2025-08-05T13:00:30.288Z
Learnt from: Noroth
PR: wundergraph/cosmo#2108
File: protographic/tests/sdl-validation/01-basic-validation.test.ts:20-57
Timestamp: 2025-08-05T13:00:30.288Z
Learning: In the protographic SDL validation system, the validation logic for nullable list items only refers to the actual list type and not the nesting. For example, `[[String!]]` (nullable list of non-nullable lists) would not trigger a nullable list warning because the validation focuses on the immediate list structure rather than nested list structures.
Applied to files:
protographic/src/sdl-validation-visitor.ts
🔇 Additional comments (9)
router-tests/go.mod (1)
30-30: Pre-release version bump for field resolver testing.The version bump to a timestamped RC aligns with the field resolver feature work in this PR. Given your clarification that this is for testing purposes and will be updated once a stable version is available, this looks good.
composition/src/v1/constants/strings.ts (1)
5-5: LGTM!The import and dependency mapping for
CONNECT_CONFIGURE_RESOLVERare correctly configured. The directive's dependency onFIELD_SET_SCALAR_DEFINITIONaligns with itscontext: openfed__FieldSet!argument, following the same pattern as other FieldSet-based directives likeKEY,PROVIDES, andREQUIRES.Also applies to: 112-112
composition/src/v1/normalization/utils.ts (1)
28-28: LGTM!The imports and initialization mapping for
CONNECT_CONFIGURE_RESOLVERare correctly implemented, following the established pattern for directive registration.Also applies to: 60-60, 399-399
composition/src/v1/constants/directive-definitions.ts (1)
17-18: LGTM!The
CONNECT_CONFIGURE_RESOLVER_DEFINITIONis correctly structured with:
- Proper imports for
CONNECT_CONFIGURE_RESOLVERandCONTEXT- Correct argument configuration using
REQUIRED_FIELDSET_TYPE_NODEfor thecontextparameter- Appropriate location restriction to
FIELD_DEFINITION- Non-repeatable flag set correctly
The definition follows the established pattern of similar FieldSet-based directives.
Also applies to: 188-201
composition/src/v1/constants/constants.ts (2)
8-8: LGTM!The import statements and registration of
CONNECT_CONFIGURE_RESOLVERin theDIRECTIVE_DEFINITION_BY_NAMEmap are correctly implemented.Also applies to: 47-47, 83-83
123-132: Verify if the new directive should be included in V2_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAME.The
V2_DIRECTIVE_DEFINITION_BY_DIRECTIVE_NAMEmap (lines 123-132) does not include the newCONNECT_CONFIGURE_RESOLVERdirective. Verify whether:
- This directive is available in both v1 and v2 (current state is correct)
- This is a v2-only directive (should be added to the map)
- This is a v1-only directive (current state is correct)
Understanding the versioning is important for proper schema validation and directive availability.
protographic/src/sdl-validation-visitor.ts (3)
237-325: LGTM!The migration to context-based validation is clean and consistent. The distinction between
@requires(warning - future support) and@provides(error - never supported in Connect) is appropriate.
387-431: LGTM!The validation logic correctly handles all error cases:
- Missing parent type
- Invalid field names in context
- Self-referential context
- Circular dependencies between fields
The return value properly controls whether ID fallback validation should proceed.
461-514: LGTM! Transitive cycle detection properly implemented.The recursive DFS approach correctly detects both direct and transitive cycles by:
- Tracking visited nodes in the current path
- Recursively checking context dependencies
- Properly backtracking to allow different paths through the same node
This addresses the previous concern about missing transitive dependencies.
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
LGTM on behalf of router and router-tests
…993-implement-field-resolver-for-graphql-operation
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (14)
composition/src/utils/string-constants.ts(1 hunks)composition/src/v1/constants/constants.ts(3 hunks)composition/src/v1/constants/directive-definitions.ts(2 hunks)composition/src/v1/constants/strings.ts(2 hunks)composition/src/v1/normalization/directive-definition-data.ts(3 hunks)composition/src/v1/normalization/utils.ts(3 hunks)composition/tests/v1/directives/connect-configure-resolver.test.ts(1 hunks)demo/pkg/subgraphs/employees/employees.go(1 hunks)demo/pkg/subgraphs/employees/gqlgen.yml(1 hunks)protographic/src/sdl-to-proto-visitor.ts(9 hunks)protographic/src/sdl-validation-visitor.ts(9 hunks)protographic/src/string-constants.ts(1 hunks)router-tests/go.mod(1 hunks)router/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- router-tests/go.mod
- composition/tests/v1/directives/connect-configure-resolver.test.ts
- composition/src/v1/constants/constants.ts
- composition/src/v1/normalization/directive-definition-data.ts
- composition/src/v1/normalization/utils.ts
- protographic/src/string-constants.ts
- router/go.mod
- demo/pkg/subgraphs/employees/employees.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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-validation-visitor.ts
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.
Applied to files:
protographic/src/sdl-validation-visitor.ts
📚 Learning: 2025-08-05T13:00:30.288Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2108
File: protographic/tests/sdl-validation/01-basic-validation.test.ts:20-57
Timestamp: 2025-08-05T13:00:30.288Z
Learning: In the protographic SDL validation system, the validation logic for nullable list items only refers to the actual list type and not the nesting. For example, `[[String!]]` (nullable list of non-nullable lists) would not trigger a nullable list warning because the validation focuses on the immediate list structure rather than nested list structures.
Applied to files:
protographic/src/sdl-validation-visitor.ts
📚 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/src/sdl-to-proto-visitor.ts
🧬 Code graph analysis (5)
composition/src/v1/constants/directive-definitions.ts (3)
composition/src/ast/utils.ts (2)
stringToNameNode(77-82)stringArrayToNameNodeArray(84-90)composition/src/utils/string-constants.ts (3)
CONTEXT(21-21)FIELD_DEFINITION_UPPER(56-56)CONNECT_FIELD_RESOLVER(20-20)composition/src/v1/constants/type-nodes.ts (1)
REQUIRED_FIELDSET_TYPE_NODE(10-13)
composition/src/utils/string-constants.ts (1)
protographic/src/string-constants.ts (2)
CONNECT_FIELD_RESOLVER(1-1)CONTEXT(2-2)
composition/src/v1/constants/strings.ts (2)
composition/src/utils/string-constants.ts (1)
CONNECT_FIELD_RESOLVER(20-20)composition/src/v1/constants/non-directive-definitions.ts (1)
FIELD_SET_SCALAR_DEFINITION(71-74)
protographic/src/sdl-validation-visitor.ts (1)
protographic/src/string-constants.ts (2)
CONNECT_FIELD_RESOLVER(1-1)CONTEXT(2-2)
protographic/src/sdl-to-proto-visitor.ts (2)
protographic/src/naming-conventions.ts (7)
createResolverMethodName(36-38)createRequestMessageName(43-45)createResponseMessageName(50-52)typeFieldArgsName(96-98)typeFieldContextName(105-107)graphqlFieldToProtoField(18-20)resolverResponseResultName(87-89)protographic/src/string-constants.ts (4)
CONTEXT(2-2)CONNECT_FIELD_RESOLVER(1-1)FIELD_ARGS(3-3)RESULT(4-4)
⏰ 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). (17)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: filter-changes
- GitHub Check: integration_test (./telemetry)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
demo/pkg/subgraphs/employees/gqlgen.yml (1)
28-30: Verify that theopenfed__requireFetchReasonsdirective is defined in the schema.The added directive configuration skips runtime code generation for
openfed__requireFetchReasons. Please confirm that this directive is actually defined in your GraphQL schema files (undersubgraph/*.graphqls).Additionally, clarify the relationship between this directive and the
@connect__fieldResolverfeature mentioned in the PR title—the directive name doesn't appear to be directly related to field resolvers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
protographic/src/sdl-to-mapping-visitor.ts (1)
227-227: Use Object.entries() for consistent iteration.For consistency with the updated pattern at line 116 and as suggested in previous review feedback, consider using
Object.entries()for type map iteration.Apply this diff:
- for (const typeName in typeMap) { - const type = typeMap[typeName]; + for (const [typeName, type] of Object.entries(typeMap)) {Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protographic/src/sdl-to-mapping-visitor.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/src/sdl-to-mapping-visitor.ts
🧬 Code graph analysis (1)
protographic/src/sdl-to-mapping-visitor.ts (4)
connect-go/gen/proto/wg/cosmo/node/v1/node.pb.go (13)
LookupType(169-169)LookupType(204-206)LookupType(208-210)LookupType(217-219)LookupMapping(2471-2486)LookupMapping(2501-2501)LookupMapping(2516-2518)FieldMapping(2851-2862)FieldMapping(2877-2877)FieldMapping(2892-2894)LookupFieldMapping(2556-2565)LookupFieldMapping(2580-2580)LookupFieldMapping(2595-2597)protographic/src/naming-conventions.ts (3)
createResolverMethodName(36-38)createRequestMessageName(43-45)createResponseMessageName(50-52)connect/src/wg/cosmo/node/v1/node_pb.ts (3)
LookupMapping(1860-1926)FieldMapping(2185-2235)LookupFieldMapping(1933-1975)protographic/src/index.ts (1)
FieldMapping(106-106)
⏰ 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_test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
protographic/src/sdl-to-mapping-visitor.ts (5)
18-18: LGTM! Import additions support the resolver functionality.The new imports for resolver naming and lookup types are correctly added and properly utilized in the new methods below.
Also applies to: 32-34
98-99: LGTM! Proper placement of resolvable field processing.The call to
processResolvableFields()is correctly positioned after operation types are processed but before general type mappings, ensuring proper mapping generation order.
275-275: LGTM! Consistent field mapping signature refactor.The simplified
createFieldMapping(field)signature is appropriate since the method only needs the field information. All call sites at lines 275 and 376 are correctly updated to use the new signature.Also applies to: 376-376, 458-469
471-485: LGTM! Proper lookup field mapping encapsulation.The method correctly creates a
LookupFieldMappingby wrapping the field mapping with the GraphQL type context, which aligns with the protobuf definition structure.
301-313: No issues found; review comment is incorrect.The
resolveMappingsfield is properly initialized. As a repeated field in the protobuf definition (repeated LookupMapping resolve_mappings = 7;in GRPCMapping), it is automatically initialized as an empty array (resolveMappings: LookupMapping[] = [];) in the protobuf-generated TypeScript code at the class property level, not requiring explicit constructor initialization. Pushing to this field at line 312 is safe and correct.Likely an incorrect or invalid review comment.
ysmolski
left a comment
ysmolski
left a comment
There was a problem hiding this comment.
I have reviewed demo and router* directories.
…-graphql-operation
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
On behalf of router* and protographic
Noroth
deleted the
ludwig/eng-6993-implement-field-resolver-for-graphql-operation
branch
Summary by CodeRabbit
New Features
Documentation
Validation
Tests
Chores
Checklist