feat: add composite type support for field resolvers #1341
Conversation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…or-graphql-operation
…or-graphql-operation
…or-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…or-graphql-operation
![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 (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
852-862: Clarify the function comment.The comment "returns the most recent composite type from the type definitions" is confusing—the function simply checks the node kind and returns the corresponding OneOfType enum value. Consider revising to: "returns the composite type (interface or union) for the given node, or OneOfTypeNone if not composite."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
v2/pkg/ast/ast_selection.go (3)
SelectionSet(21-25)SelectionKindField(16-16)SelectionKindInlineFragment(18-18)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
InvalidRef(21-21)DataTypeMessage(38-38)Message(182-186)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (15)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (15)
4-12: LGTM!Import additions are appropriate and used within the file.
27-53: LGTM!The OneOfType enum and FieldName method are well-structured. The empty string return for invalid types is a reasonable fallback.
65-65: LGTM!Type narrowing from int to uint8 is appropriate for this enum-like usage with only three values.
152-154: LGTM!The variadic signature enhancement allows adding multiple fields in a single call, improving ergonomics without breaking existing single-field calls.
799-817: LGTM!The type rename and field additions properly support the composite type resolution feature.
819-841: LGTM!The method correctly accumulates inline fragment selections for composite types, with appropriate early-return optimization and error handling.
843-850: LGTM!The isFieldResolver logic correctly returns false for root fields and checks for field arguments for non-root fields. Based on learnings.
864-881: LGTM!The method correctly retrieves member types for both interfaces and unions with proper error handling for each case.
719-788: LGTM!The buildFieldMessage method is well-structured with clear separation of concerns: collecting field/fragment refs, handling composite types, building inline fragment fields, and building regular fields. Error handling is appropriate throughout.
1039-1099: LGTM!The method correctly implements the two distinct approaches for field resolver responses: composite types use fragment-based selections via FieldSelectionSet, while simple object types populate message.Fields. This aligns with the protobuf oneOf structure requirements. Based on learnings.
1101-1129: LGTM!The buildRequiredField method correctly builds fields with proper recursion for nested message types. The logic ensures that complex field types have their nested structures properly constructed.
1131-1163: LGTM!The buildCompositeField method correctly handles inline fragment fields with proper recursion for nested message structures. The nested message handling mirrors buildRequiredField, ensuring consistency.
1165-1177: LGTM!The method correctly resolves field definitions by type and field name. The pattern follows existing AST API conventions.
884-908: LGTM!Parameter update correctly reflects the type rename from resolvedField to resolverField.
1180-1249: LGTM!The method signature correctly uses the renamed resolverField type, and all usages properly access the new parentTypeNode field.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
1101-1163: Nested message construction looks correct; align composite error message wordingThe
buildRequiredField/buildFieldMessage/buildCompositeFieldstack correctly recurses into nested selections, including inline fragments, and attachesRPCMessagetrees only for message-typed fields that actually have subselections. That should fix the earlier problem where inline-fragment fields lost their nested messages.One minor nit: the error text in
buildCompositeFieldstill mentions “required field” even though this is the composite path:if !found { return nil, fmt.Errorf("unable to build required field: unable to resolve field type node for field %s", r.operation.FieldNameString(fieldRef)) }You could make this clearer and easier to debug by aligning the wording with the function:
- if !found { - return nil, fmt.Errorf("unable to build required field: unable to resolve field type node for field %s", r.operation.FieldNameString(fieldRef)) - } + if !found { + return nil, fmt.Errorf("unable to build composite field: unable to resolve field type node for field %s", r.operation.FieldNameString(fieldRef)) + }
1179-1246: Avoid taking the address of the range variable increateResolverRPCCalls
createResolverRPCCallscurrently ranges by value and passes&resolvedFieldintonewResolveRPCCall:for _, resolvedField := range resolvedFields { ... call, err := r.newResolveRPCCall(&resolveRPCCallConfig{ resolveConfig: resolveConfig, resolvedField: &resolvedField, ... })This is safe today because the pointer is only used synchronously, but it follows the well-known anti-pattern of taking the address of the range variable. If
resolvedFieldever gets stored or used asynchronously, this will break.You can future-proof this by iterating by index and taking the address of the slice element instead:
-func (r *rpcPlanningContext) createResolverRPCCalls(subgraphName string, resolvedFields []resolverField) ([]RPCCall, error) { +func (r *rpcPlanningContext) createResolverRPCCalls(subgraphName string, resolvedFields []resolverField) ([]RPCCall, error) { @@ - for _, resolvedField := range resolvedFields { + for i := range resolvedFields { + resolvedField := &resolvedFields[i] @@ - resolveConfig := r.mapping.FindResolveTypeFieldMapping( - resolvedField.parentTypeNode.NameString(r.definition), - r.operation.FieldNameString(resolvedField.fieldRef), - ) + resolveConfig := r.mapping.FindResolveTypeFieldMapping( + resolvedField.parentTypeNode.NameString(r.definition), + r.operation.FieldNameString(resolvedField.fieldRef), + ) @@ - call, err := r.newResolveRPCCall(&resolveRPCCallConfig{ + call, err := r.newResolveRPCCall(&resolveRPCCallConfig{ resolveConfig: resolveConfig, - resolvedField: &resolvedField, + resolvedField: resolvedField, contextMessage: contextMessage, fieldArgsMessage: fieldArgsMessage, }) @@ - contextMessage.Fields = make(RPCFields, len(resolvedField.contextFields)) + contextMessage.Fields = make(RPCFields, len(resolvedField.contextFields)) @@ - fieldArgsMessage.Fields = make(RPCFields, len(resolvedField.fieldArguments)) - for i := range resolvedField.fieldArguments { - field, err := r.createRPCFieldFromFieldArgument(resolvedField.fieldArguments[i]) + fieldArgsMessage.Fields = make(RPCFields, len(resolvedField.fieldArguments)) + for i := range resolvedField.fieldArguments { + field, err := r.createRPCFieldFromFieldArgument(resolvedField.fieldArguments[i])(Adjust inner index variable names as needed.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (4)
v2/pkg/ast/ast_selection.go (3)
SelectionSet(21-25)SelectionKindField(16-16)SelectionKindInlineFragment(18-18)v2/pkg/astvisitor/visitor.go (1)
Walker(65-98)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
InvalidRef(21-21)DataTypeMessage(38-38)Message(182-186)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
843-850: Field-resolver detection matches repository invariantUsing
len(r.operation.FieldArguments(fieldRef)) > 0to classify resolvers (with an explicitisRootFieldescape hatch) aligns with the documented invariant that field resolvers must have arguments and that root fields are treated separately. Looks good and keeps the resolver detection cheap.Based on learnings
1039-1099: Composite vs. simple resolver result handling is consistent with the intended design
buildFieldResolverTypeMessagecleanly splits behavior:
- When
resolverField.fragmentSelectionsis non-empty, it builds a composite oneof message usingFieldSelectionSet,fragmentType, andmemberTypes, without populatingmessage.Fields.- Otherwise, it builds a plain object message from
fieldsSelectionSetRef, skipping nested resolvers and deduplicating by name/alias.This matches the documented requirement that resolver responses for composite types use fragment-based
FieldSelectionSetexclusively, while simple object types useFields. No changes needed here.Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
852-863: Clarify the function comment.The comment "checks whether the node is an interface or union type" (implied from context) would be more accurate than mentioning "the most recent composite type from the type definitions," since the function simply inspects the
node.Kindfield without any lookup or tracking of "recent" types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (4)
v2/pkg/ast/ast_selection.go (3)
SelectionSet(21-25)SelectionKindField(16-16)SelectionKindInlineFragment(18-18)v2/pkg/astvisitor/visitor.go (1)
Walker(65-98)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
InvalidRef(21-21)DataTypeMessage(38-38)Message(182-186)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (15)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (15)
4-4: LGTM!The new imports are properly utilized:
errorsfor constructing error messages (line 1070) andunsafebytesfor efficient string-to-bytes conversion (line 1172).Also applies to: 12-12
65-65: LGTM!Upgrading
CallKindfrominttouint8is a sensible optimization for an enum-like type with only three values.
152-154: LGTM!The variadic signature for
Addis a useful API enhancement that allows adding multiple fields in a single call (e.g., line 776).
719-788: LGTM!The
buildFieldMessagemethod correctly handles both regular object types and composite types (interfaces/unions) by:
- Separating field selections from inline fragment selections
- Detecting and configuring composite types with member types
- Building fragment-specific fields via
buildCompositeField- Building regular fields via
buildRequiredField
799-812: LGTM!The
resolverFieldstruct enhancements properly support composite type resolution by addingfragmentSelections,fragmentType, andmemberTypesfields, which align with the PR's objective of handling interfaces and unions.
814-817: LGTM!The
fragmentSelectionstruct is a clean data structure for tracking inline fragment type conditions and their selection sets.
819-841: LGTM!The method correctly identifies inline fragment selections for composite field resolvers and populates the
resolverFieldwith fragment type conditions and selection sets.
843-850: LGTM!The logic correctly identifies field resolvers based on the invariant that they must have arguments and cannot be root fields.
Based on learnings
865-882: LGTM!The method correctly retrieves member types for both interface (implementors) and union (members) composite types with appropriate error handling.
1040-1100: LGTM!The method correctly implements the two distinct approaches for field resolver response handling: composite types (interfaces/unions) exclusively use
FieldSelectionSetwith fragment-based selections, while simple object types usemessage.Fields. This aligns with the protobuf oneOf structure requirements.Based on learnings
1102-1130: LGTM!The method cleanly extracts field-building logic and correctly handles nested message structures through recursive
buildFieldMessagecalls when the field has selections.
1132-1164: LGTM!The method now correctly handles nested message structures for inline fragment fields by recursively building messages (lines 1147-1159), addressing the previous review concern.
1166-1178: LGTM!The helper cleanly returns a field definition reference for a specific type and field name, following the
InvalidRefpattern instead of a boolean flag for not-found cases.
1181-1250: LGTM!The refactoring to use
parentTypeNode(ast.Node) instead of a string type name improves type safety while preserving the original resolver RPC call creation logic.
1269-1271: LGTM!The call site correctly uses the renamed
buildFieldResolverTypeMessagemethod with the refactoredresolverFieldstructure.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
Outdated
Show resolved
Hide resolved
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 (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
183-213: Fix resolver stack and path handling inLeaveField
LeaveFieldcurrently runs for every field and always:
- Removes the last path segment:
r.fieldPath = r.fieldPath.RemoveLastItem().- Pops from
fieldResolverAncestors:r.fieldResolverAncestors.pop().This breaks resolver state in two ways:
Stack underflow / misalignment
- For non-resolver fields inside a resolver selection set,
EnterFieldonly doesr.walker.SkipNode()whenr.fieldResolverAncestors.len() > 0; it never pushes.- On
LeaveFieldfor those fields we still callpop(), so we:
- Pop the ancestor entry for the resolver itself, and
- Potentially call
pop()on an empty stack on the next nested field.This corrupts resolver ancestry so subsequent calls to
fieldResolverAncestors.peek()(e.g. inEnterSelectionSet) can refer to the wrong resolver or panic, depending onstack.pop’s implementation.Incorrect
fieldPathfor nested resolvers
enterFieldResolverextendsfieldPathfor resolver fields, andsetResolvedFielduses that prefix to compute each contextResolvePath.- For non-resolver children inside a resolver (e.g.
id,valueundercategoryMetrics),EnterFielddoes not extendfieldPath, butLeaveFieldstill callsRemoveLastItem.- That trims the resolver’s own path segment before we enter any nested resolver in the same selection set, so nested resolvers compute their context paths from the wrong prefix (e.g.
result.categories.idinstead ofresult.categories.categoryMetrics.id).To keep resolver state correct:
- Only pop
fieldResolverAncestorsand adjustparentCallIDwhen leaving a resolver field (non-root field with arguments).- Only remove a path segment when we actually appended one:
- Always for non-resolver fields outside resolver context.
- For resolver fields themselves, but not for non-resolver children inside a resolver.
A minimal, targeted fix could look like:
func (r *rpcPlanVisitorFederation) LeaveField(ref int) { - r.fieldPath = r.fieldPath.RemoveLastItem() - r.fieldResolverAncestors.pop() - // If we are not in the operation field, we can increment the response field index. - if !r.walker.InRootField() { - // If the field has arguments, we need to decrement the related call ID. - // This is because we can also have nested arguments, which require the underlying field to be resolved - // by values provided by the parent call. - if r.operation.FieldHasArguments(ref) { - r.parentCallID-- - } - - r.planInfo.currentResponseFieldIndex++ - return - } - - r.planInfo.currentResponseFieldIndex = 0 + inResolverCtx := r.fieldResolverAncestors.len() > 0 + isResolverField := !r.walker.InRootField() && r.operation.FieldHasArguments(ref) + + // Only drop a path segment if we previously added one: + // - for non-resolver fields outside resolver context, or + // - for resolver fields themselves. + if !inResolverCtx || isResolverField { + r.fieldPath = r.fieldPath.RemoveLastItem() + } + + if inResolverCtx { + // We only maintain resolver stack / parentCallID for resolver fields, + // not for non-resolver children inside a resolver selection. + if isResolverField { + r.fieldResolverAncestors.pop() + r.parentCallID-- + } + return + } + + // Normal non-resolver behaviour. + if !r.walker.InRootField() { + r.planInfo.currentResponseFieldIndex++ + return + } + + r.planInfo.currentResponseFieldIndex = 0 }This keeps:
fieldResolverAncestorsin sync withenterFieldResolverpushes.parentCallIDbalanced with the invariant that resolver fields always have arguments.fieldPathstable across non-resolver children inside resolver selections, so nested resolvers still computeResolvePathfrom the correct prefix.Based on learnings about resolver argument invariants and composite resolver behaviour.
Also applies to: 303-399, 401-428
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (1)
3-9: Addt.Parallel()toTestExecutionPlanFieldResolvers_CustomSchemasThe subtests in
TestExecutionPlanFieldResolvers_CustomSchemasalready callt.Parallel(), but the top-level test does not. Addingt.Parallel()at the beginning of the function will satisfy the linter hint and keep this test consistent with the other parallelized suites in this file.func TestExecutionPlanFieldResolvers_CustomSchemas(t *testing.T) { - tests := []struct { + t.Parallel() + + tests := []struct {Also applies to: 2196-2398
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(7 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
🧬 Code graph analysis (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (3)
v2/pkg/engine/datasource/grpc_datasource/configuration.go (1)
GRPCMapping(26-46)v2/pkg/grpctest/schema.go (1)
MustGraphQLSchema(75-87)v2/pkg/astparser/parser.go (1)
ParseGraphqlDocumentString(37-45)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (7)
v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
InvalidRef(21-21)Message(182-186)v2/pkg/ast/path.go (1)
Path(34-34)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
OneOfTypeNone(34-34)v2/pkg/ast/ast_field_definition.go (1)
FieldDefinition(17-26)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)v2/pkg/operationreport/externalerror.go (1)
ExternalError(34-42)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
v2/pkg/ast/ast_selection.go (2)
SelectionKindField(16-16)SelectionKindInlineFragment(18-18)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
InvalidRef(21-21)DataTypeMessage(38-38)Message(182-186)
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
[failure] 2196-2196:
TestExecutionPlanFieldResolvers_CustomSchemas should call t.Parallel on the top level as well as its subtests (tparallel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
23-37: Nice extraction of reusable test harnessThe
testConfig+runTestWithConfigsplit cleanly factors out the common visitor/plan+error/diff logic and keepsrunTestas a thin wrapper over the default Products mapping/schema. This matches how newer resolver tests use custom schemas without duplicating setup.Also applies to: 51-67
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
183-213: Resolver selection-set handling for composites looks correctThe resolver-aware
EnterSelectionSetbranch that:
- Detects composite enclosing types via
planCtx.getCompositeType,- Populates
memberTypeson the correspondingresolverField, and- Delegates to
enterResolverCompositeSelectionSetwhile clearingfieldsSelectionSetRef,matches the intended split between simple object resolvers and composite (interface/union) resolvers, where composite responses are driven purely by fragment-based selection sets.
Based on learnings about exclusive
FieldSelectionSetusage for composite resolver responses.v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
148-154: Resolver/composite planning helpers align with the intended designThe new resolver plumbing—
resolverField/fragmentSelection,buildFieldMessage,enterResolverCompositeSelectionSet,buildFieldResolverTypeMessage,buildRequiredField,buildCompositeFields, andcreateResolverRPCCalls/newResolveRPCCall—fits together cleanly:
buildFieldMessagenow builds nested messages and fragment-basedFieldSelectionSetfor inline fragments while skipping nested field resolvers.buildFieldResolverTypeMessageenforces the split between:
- Simple object resolver results (using
fieldsSelectionSetRef+buildRequiredField→message.Fields), and- Composite resolver results (using
fragmentSelections/fragmentType/memberTypes→FieldSelectionSetonly),
which matches the constraint that resolver responses for interfaces/unions must be driven purely via oneof/fragment selections.createResolverRPCCallsandnewResolveRPCCallcorrectly derive context/args messages and wrap resolver outputs underresult→<TargetName>with optionality from the original field type.This matches the documented invariants about resolver argument presence and composite-type handling in the gRPC datasource.
Based on learnings about composite resolver response structure and resolver argument invariants.
Also applies to: 719-792, 803-886, 1044-1104, 1136-1187, 1190-1260, 1268-1335
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
803-822: Document thefieldsSelectionSetReffield.The
fieldsSelectionSetReffield on line 808 lacks documentation explaining what it represents and when it's used. Based on the code, it appears to hold the selection set reference for non-composite field resolver responses.Consider adding a comment like:
type resolverField struct { callerRef int parentTypeNode ast.Node fieldRef int fieldDefinitionTypeRef int // fieldsSelectionSetRef is the selection set reference for the field resolver's return type. // Used when the resolver returns a non-composite (simple object) type. fieldsSelectionSetRef int responsePath ast.Path // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (11)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (11)
4-4: LGTM!The new imports (
errorsandunsafebytes) are appropriately used for error handling and efficient string-to-bytes conversion in the new composite type handling logic.Also applies to: 12-12
27-53: LGTM!The
OneOfTypeenum and itsFieldName()method provide a clean abstraction for handling interface and union types in protobuf messages.
65-65: LGTM!Changing
CallKindfrominttouint8is a good optimization given the small number of enum values.
152-154: LGTM!The variadic signature for
Addimproves the API by allowing multiple fields to be added in a single call, which is used effectively on line 776.
719-792: LGTM!The
buildFieldMessagefunction properly handles both simple fields and composite types (interfaces/unions) with inline fragments. The logic correctly separates field selections from inline fragment selections, skips field resolvers, and recursively builds nested message structures.
823-886: LGTM!The new helper functions provide clean abstractions for:
- Collecting inline fragment selections for composite types
- Identifying field resolvers by checking for arguments
- Determining composite type kind (interface/union)
- Retrieving member types for composite types
Based on learnings, the
isFieldResolverlogic correctly relies on the invariant that field resolvers must have arguments.
1044-1108: LGTM!The
buildFieldResolverTypeMessagefunction correctly implements the intentional design pattern for field resolver responses, properly skips nested field resolvers (line 1090), and deduplicates fields (line 1094).Based on learnings, the two distinct approaches (composite types using
FieldSelectionSetexclusively vs. simple types usingFields) align with protobuf oneOf structure requirements and cannot be mixed for resolver responses.
1110-1138: LGTM!The
buildRequiredFieldfunction correctly builds fields and recursively constructs nested message structures. The recursion throughbuildFieldMessageis safe as it follows the finite selection set structure.
1140-1178: LGTM!The
buildCompositeFieldsfunction correctly handles inline fragment fields including their nested message structures (lines 1161-1173), properly skips field resolvers (lines 1147-1149), and uses type-specific field definition lookups.
1180-1192: LGTM!The
fieldDefinitionRefForTypehelper provides a clean abstraction for type-specific field definition lookups with appropriate error handling.
1195-1264: LGTM!The
createResolverRPCCallsfunction has been correctly updated to work with the newresolverFieldtype and consistently uses theparentTypeNodefield for type name lookups.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
1078-1080: Add documentation for the selection set validation.As noted in a past review, this check needs documentation explaining when and why it's used. The condition validates that non-scalar field resolvers have a selection set, but this could be clearer.
Consider adding a comment and improving the error message:
- // field resolvers which return a non scalar type must have a selection set. - // If we don't have a selection set we return an error. - if resolverField.fieldsSelectionSetRef == ast.InvalidRef { - return nil, errors.New("unable to resolve required fields: no fields selection set found") + // Field resolvers returning non-scalar types (messages) must have a selection set + // to specify which fields to fetch. This is enforced by GraphQL validation, but we + // defensively check here to provide a clear error if the invariant is violated. + if resolverField.fieldsSelectionSetRef == ast.InvalidRef { + return nil, fmt.Errorf("field resolver for type %s must have a selection set when returning a message type", typeName) }
1142-1180: Consider extracting common logic shared withbuildRequiredField.Both
buildRequiredField(lines 1112-1140) andbuildCompositeFieldsfollow nearly identical patterns:
- Get field definition
- Build field via
buildField- Check if it's a message type with selections
- Recursively build nested message
The duplication is manageable given the slightly different contexts (single field vs. multiple fields with fragment type lookup), but extracting the nested-message-building logic into a helper could improve maintainability.
Example helper function:
// attachNestedMessage builds and attaches a nested message to the field if it's a message type with selections func (r *rpcPlanningContext) attachNestedMessage(field *RPCField, fieldDef int, fieldRef int) error { if field.ProtoTypeName == DataTypeMessage && r.operation.FieldHasSelections(fieldRef) { fieldTypeNode, found := r.definition.ResolveNodeFromTypeRef(r.definition.FieldDefinitionType(fieldDef)) if !found { return fmt.Errorf("unable to resolve field type node for field %s", r.operation.FieldNameString(fieldRef)) } message, err := r.buildFieldMessage(fieldTypeNode, fieldRef) if err != nil { return err } field.Message = message } return nil }This would simplify both functions and make the pattern more maintainable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (11)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (11)
3-13: LGTM!The import additions for
errorsandinternal/unsafebytesare appropriate for the new composite type functionality.
27-53: LGTM!The
OneOfTypeenum is well-designed with clear semantics. TheFieldName()method provides a clean abstraction for mapping to protobuf field names based on whether the type is an interface or union.
65-65: LGTM!The change from
inttouint8forCallKindis reasonable for memory efficiency, given the small number of call kinds.
152-154: LGTM!The variadic signature for
Addimproves API ergonomics by allowing multiple fields to be added in a single call.
719-794: LGTM!The
buildFieldMessagefunction is well-structured and correctly handles both composite types (interfaces/unions) and regular object types. The separation of field refs and inline fragment refs is clear, and the error handling is appropriate. The check at Line 782 to skip field resolvers was added based on previous review feedback.
805-823: LGTM!The updated
resolverFieldstruct and newfragmentSelectionstruct provide the necessary data structures to support composite type resolution. The field naming is clear and consistent with the codebase conventions.
825-869: LGTM!These helper functions are well-designed:
enterResolverCompositeSelectionSetclearly collects inline fragment selectionsisFieldResolvercorrectly identifies field resolvers based on the presence of arguments (aligning with the system invariant that field resolvers must have arguments)getCompositeTypeprovides a clean abstraction for type checkingBased on learnings, field resolvers must have arguments.
871-888: LGTM!The
getMemberTypesfunction properly handles both interface and union types with appropriate error handling for each case.
1112-1140: LGTM!The
buildRequiredFieldfunction is well-structured and correctly handles nested message types. The check at Line 1125 ensuresbuildFieldMessageis only called when the field has selections, preventing potential nil message issues.
1182-1194: LGTM!The
fieldDefinitionRefForTypehelper is a clean abstraction that properly usesInvalidRefas a sentinel value and efficiently converts strings to bytes usingunsafebytes.
1196-1266: LGTM!The updates to
createResolverRPCCallscorrectly use theparentTypeNodefield from theresolverFieldstruct, aligning with the new resolverField-centric model introduced in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/astvisitor/visitor.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/astvisitor/visitor.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
RPCExecutionPlan(57-62)OneOfTypeNone(34-34)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
InvalidRef(21-21)v2/pkg/ast/path.go (1)
Path(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (11)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (5)
53-76: LGTM - Resolver field tracking initialization is appropriate for federation context.The initialization with
fieldPathstarting at "result" correctly reflects the federation response structure. The zero-capacity stack is acceptable given the earlier discussion about field resolver frequency.
190-213: Composite type handling within resolver context looks correct.The logic correctly differentiates between composite types (interface/union) and simple object types. For composite types, it invalidates
fieldsSelectionSetRefand delegates toenterResolverCompositeSelectionSet, which aligns with the design where composite types use fragment-based selections exclusively (per retrieved learnings).
382-402: Stack operations and parentCallID handling are correct based on the field resolver invariant.Based on learnings, field resolvers must have arguments, so the unconditional
parentCallID--when leaving a field resolver is safe. The pop operation correctly pairs with the push inenterFieldResolver.
404-431: Field resolver entry logic is well-structured.The method correctly:
- Captures the caller reference from the current
parentCallID- Builds the response path by stripping root and inline fragment names
- Pushes the resolver index onto the stack for tracking nested resolvers
- Increments
parentCallIDso nested resolvers reference this resolver as their parent
469-471: No issues found. The methodvisitWithDefaultsis correctly implemented and appropriately used for entity key field scaffolding. The configuration it applies (including member types and processing field resolvers) is suitable for federation lookup scaffolding.v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (6)
57-83: LGTM - Resolver tracking fields properly initialized.The initialization correctly sets up the resolver field tracking with an empty path (appropriate for non-federation queries) and the field resolver ancestors stack.
200-222: Composite type handling in resolver context is well-implemented.The logic correctly handles inline fragments, composite types (interface/union), and simple object types, mirroring the federation visitor's approach.
368-379: Correct handling of nested non-resolver fields and duplicates.The
SkipNode()call with the explanatory comment clarifies that non-resolver fields within resolver contexts are intentionally ignored. The duplicate field check correctly returns without modifyingfieldPath. Per past review discussion, this ensures the stack invariant is maintained since only resolver fields have theirLeaveFieldcalled within resolver contexts.
410-434: LeaveField logic correctly handles resolver exit and call finalization.The extraction of
finalizeCall()improves readability. The unconditionalparentCallID--for field resolvers is safe per the invariant that field resolvers must have arguments.
436-447: Clean extraction of call finalization logic.The
finalizeCall()method properly encapsulates the call completion workflow, including defensive nil assignment forcurrentCall.
449-476: Field resolver entry implementation is consistent with federation visitor.The method correctly captures the resolver hierarchy for parallel call ordering, maintains the field path, and tracks the parent call relationship. The implementation aligns with the federation visitor's approach.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(6 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
🧬 Code graph analysis (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (4)
v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
InvalidRef(21-21)v2/pkg/ast/path.go (1)
Path(34-34)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
OneOfTypeNone(34-34)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
RPCExecutionPlan(57-62)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
InvalidRef(21-21)v2/pkg/ast/path.go (1)
Path(34-34)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (11)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (5)
53-58: Resolver state and initial federation setup look consistentThe new resolver-related fields (
parentCallID,fieldResolverAncestors,resolvedFields) and their initialisation innewRPCPlanVisitorFederation(includingfieldPathstarting from"result") match the entity response shape and the non‑federated visitor’s resolver model. This should give stable callerRefs and resolver stacks across entity lookups.Also applies to: 71-76
189-212: ResolverEnterSelectionSetbranch correctly wires composite resolver selectionsThe resolver-specific branch in
EnterSelectionSet(driven byfieldResolverAncestors.len() > 0) cleanly separates resolver planning from normal response-message building, and the composite type handling (getCompositeType+getMemberTypes+enterResolverCompositeSelectionSet) aligns with the composite resolver behavior described forresolveRequiredFields. This looks correct and consistent with the non‑federated visitor.
324-379: Field-definition resolution and resolver/SkipNode routing are soundResolving
fieldDefRefviawalker.FieldDefinitionwith a precise external error, routing resolver fields throughenterFieldResolver, and skipping non‑resolver fields inside resolver calls (plus duplicate prevention viaExists+SkipNode) gives a clear separation between base RPC planning and resolver-specific work. The added comments also address earlier confusion about SkipNode semantics inside resolver contexts.
404-431:enterFieldResolverwiring for federation looks correctUsing
parentCallIDascallerRef, derivingresponsePathfromwalker.Path[1:].WithoutInlineFragmentNames().WithFieldNameItem(...), passingfieldPathintosetResolvedField, then pushing the resolver index ontofieldResolverAncestorsand incrementingparentCallIDfor nested resolvers matches the intended resolver‑chaining semantics for federation. Assuming theLeaveFieldfix above is applied, the resolver stack and call IDs should remain balanced.
469-471: Entity lookup now includes default/required fieldsSwitching
scaffoldEntityLookuptorequiredFieldsVisitor.visitWithDefaults(instead of only visiting explicitkeyFields) makes sense for federation: it allows the key message to be populated with both key and additional required fields as defined in configuration, while still reusing the same visitor infrastructure.v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (6)
57-59: Resolver tracking fields are wired and initialised correctlyAdding
parentCallID,fieldResolverAncestors, andresolverFieldstorpcPlanVisitorand initialising them innewRPCPlanVisitor(withparentCallIDset toInvalidRefand an empty stack/path) matches the resolver planning model and keeps the non‑federated visitor in sync with the federation visitor’s state layout.Also applies to: 75-84
113-127: Resolver calls are only compiled when neededThe updated
LeaveDocumentshort‑circuits when there are noresolverFieldsand otherwise delegates tocreateResolverRPCCalls, then nils the slice. That avoids unnecessary work and ensures resolver RPC calls are appended to the plan exactly once.
199-221: Resolver-awareEnterSelectionSetand composite handling look correctThe resolver-specific block in
EnterSelectionSetuses the resolver ancestor index to capture the selection set (or, for composite types, to delegate toenterResolverCompositeSelectionSetwith computedmemberTypes). Skipping inline fragments here and relying on the composite helper aligns with the composite-type semantics described for field resolvers.
353-408: Field resolution, resolver routing, and duplicate prevention are well structuredUsing
walker.FieldDefinitionwith a clear external error, routing resolver fields intoenterFieldResolver, skipping non-resolver fields under resolver contexts, and guarding against duplicate fields viaExists+SkipNodegive a clean split between base RPC planning and resolver planning. Inline-fragment selections are correctly redirected intoFieldSelectionSet.
437-448:finalizeCallcleanly encapsulates root call completion
finalizeCallstoring the current call intoplan.Calls[currentCallID], advancingcurrentCallID/operationFieldRef, and resettingcurrentResponseFieldIndexmakes root-call lifecycle explicit and keeps the planner state ready for the next root field. Coupled with the early return inLeaveFieldfor root fields, this is a nice simplification of the top-level planning flow.
450-477:enterFieldResolvercorrectly anchors resolver calls to parent calls and pathsUsing
parentCallIDforcallerRef,walker.EnclosingTypeDefinitionfor the parent type,walker.Path[1:].WithoutInlineFragmentNames().WithFieldNameItem(...)forresponsePath, and separately passingfieldPathintosetResolvedFieldmatches the intended resolver context resolution. Pushing the resolver index ontofieldResolverAncestorsand incrementingparentCallIDfor nested resolvers matches the invariants captured in the learnings about resolver hierarchies.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
55-55: Naming inconsistency:resolvedFieldsvsresolverFieldsThe field is named
resolvedFieldsbut its type is[]resolverField. The non-federated visitor atexecution_plan_visitor.go:59correctly usesresolverFieldsfor the same type. Consider renaming for consistency:- resolvedFields []resolverField + resolverFields []resolverFieldThis would require updating all references in this file (lines 72, 107, 111, 118, etc.).
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
368-373: Consider expanding the comment to clarify the control flow.Per previous review feedback from ysmolski, the interaction between
SkipNode()andLeaveFieldis non-trivial. The current comment explains what but could better explain why:// Check if the field is inside of a resolver call. if r.fieldResolverAncestors.len() > 0 { - // We don't want to call LeaveField here because we ignore the field entirely. + // Non-resolver fields inside a resolver context are ignored entirely. + // SkipNode() prevents traversal into this field's children AND skips LeaveField, + // which is important because: + // 1. We don't push to fieldResolverAncestors for these fields + // 2. We don't want to increment currentResponseFieldIndex for them + // Only nested resolver fields (handled above) participate in the visitor lifecycle. r.walker.SkipNode() return }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(8 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
🧬 Code graph analysis (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (9)
RPCExecutionPlan(57-62)RPCCall(78-94)RPCMessage(98-112)RPCField(183-212)OneOfType(29-29)OneOfTypeInterface(36-36)RPCFieldSelectionSet(149-149)OneOfTypeUnion(38-38)RPCFields(265-265)v2/pkg/astparser/parser.go (1)
ParseGraphqlDocumentString(37-45)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
RPCExecutionPlan(57-62)OneOfTypeNone(34-34)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
InvalidRef(21-21)v2/pkg/ast/path.go (1)
Path(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (4)
901-1116: Comprehensive test coverage for nested field resolvers.The test cases properly validate:
- Nested resolver call chain with correct
DependentCalls(call 2 depends on call 1, call 1 depends on call 0)- Proper
ResponsePathconstruction through nested fields- Context field
ResolvePathtraversal through parent resolver responseThe expected plan structure correctly reflects that nested resolvers create separate RPC calls with proper dependency ordering.
1118-1247: Good coverage of interface type resolver handling.The test validates that:
- Interface fields (
Animal) in resolver responses correctly setOneOfType: OneOfTypeInterfaceMemberTypesproperly lists implementing types (Cat,Dog)FieldSelectionSetmaps each concrete type to its field selectionsThis aligns with the learning that composite types use
FieldSelectionSetfor fragment-based selections rather than theFieldsarray.
2196-2398: Tests properly validate nested resolvers within composite types.The
TestExecutionPlanFieldResolvers_CustomSchemastest confirms that:
- Resolver fields within inline fragments on composite types generate separate RPC calls
- The
FieldSelectionSetfor the composite type remains empty (no non-resolver fields), with resolver handled via its own call- The nested resolver correctly references the parent resolver's call ID (
DependentCalls: []int{1})This validates the core PR objective of supporting composite return types in field resolvers.
2382-2397: Test loop is safe with Go 1.23+ semantics.The
operationvariable is parsed outside thet.Runclosure, but this is safe because:
- The project requires Go 1.23+ (per learnings)
- "In Go 1.22 Go changed the semantics of for loop variables to prevent unintended sharing in per-iteration closures and goroutines."
Each iteration creates a fresh
operationvalue that is correctly captured.v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (3)
411-438: Good fix for thecurrentResponseFieldIndexalignment issue.The early return after handling resolver fields (lines 432-433) correctly prevents incrementing
currentResponseFieldIndexfor resolver fields that don't add entries tocurrentResponseMessage.Fields. This addresses the past review concern about index misalignment.
440-451: Clean extraction of call finalization logic.The
finalizeCall()method properly encapsulates:
- Storing the completed call in the plan
- Clearing the current call state
- Advancing to the next operation field
This improves readability of
LeaveField.
453-480: Well-structured resolver field entry logic.The
enterFieldResolvermethod correctly:
- Captures the current
parentCallIDascallerReffor dependency tracking- Extracts the response path without inline fragment names
- Pushes the resolver index onto
fieldResolverAncestorsfor nested resolver tracking- Increments
parentCallIDafter creating the resolver field so nested resolvers reference the correct parentBased on learnings, field resolvers must have arguments, ensuring the
parentCallID--inLeaveFieldalways executes.
| { | ||
| name: "Should include typename when requested", | ||
| query: `query UsersWithTypename { users { __typename id name } }`, | ||
| query: `query UsersWithTypename { users { __typename id __typename name } }`, |
There was a problem hiding this comment.
is double __typename intended?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
606-611: Error fromsetMessageValueis not checked.The call to
p.setMessageValue(valMsg, fieldName, value)on line 607 now returns an error that is not being handled. This could silently fail if the field doesn't exist in the message.for fieldName, value := range data { - p.setMessageValue(valMsg, fieldName, value) + if err := p.setMessageValue(valMsg, fieldName, value); err != nil { + return nil, err + } }
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
425-428: Update misleading commentThe comment states "If the field has arguments, we need to decrement..." but the code unconditionally decrements
parentCallIDon line 428 without checking for arguments. According to the learnings, the system enforces an invariant that field resolvers must have arguments, so the unconditional decrement is correct. However, the comment is misleading.Update the comment to reflect the actual behavior:
- // If the field has arguments, we need to decrement the related call ID. - // This is because we can also have nested arguments, which require the underlying field to be resolved - // by values provided by the parent call. + // Decrement the parent call ID to restore the caller context. + // Field resolvers always have arguments (system invariant), so we always increment in enterFieldResolver + // and decrement here to maintain correct caller references for sibling fields. r.parentCallID--
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/datasource/grpc_datasource/compiler.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
RPCExecutionPlan(57-62)v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
InvalidRef(21-21)Message(182-186)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/ast/path.go (1)
Path(34-34)v2/pkg/ast/ast_field_definition.go (1)
FieldDefinition(17-26)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)v2/pkg/operationreport/externalerror.go (1)
ExternalError(34-42)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (20)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (6)
57-59: LGTM: Resolver tracking fieldsThe new fields (
parentCallID,fieldResolverAncestors,resolverFields) provide the necessary state to track resolver field hierarchy and support composite type resolution. The initialization in the constructor is consistent.
115-127: LGTM: Resolver call creationThe document cleanup correctly creates resolver RPC calls from
resolverFieldsand clears state. The logic is sound.
200-220: LGTM: Composite type handling in resolver contextsThe enhanced logic correctly distinguishes between composite types (interface/union) and simple object types within resolver fields. The call to
enterResolverCompositeSelectionSetfor composite types aligns with the PR objective of adding interface/union support to field resolvers.
351-406: LGTM: Centralized field handling logicThe refactored
EnterFieldcleanly separates concerns:
- Field resolvers → delegate to
enterFieldResolver- Non-resolver fields inside resolver context → skip (prevents incorrect nesting)
- Regular fields → standard processing with duplicate prevention
The
SkipNode()call on line 369 ensuresLeaveFieldisn't invoked for skipped fields, maintaining stack balance. Based on learnings, field resolvers must have arguments, so the skip behavior is safe.
438-449: LGTM: Clean extraction of finalization logicThe
finalizeCallmethod is a well-structured extraction that improves readability inLeaveField. The state management (assigning call, advancing IDs, resetting indices) is correct.
451-478: LGTM: Well-structured resolver field entryThe
enterFieldResolvermethod cleanly encapsulates resolver field setup:
- Creates
resolverFieldwith correctcallerReffrom currentparentCallID- Delegates detailed configuration to
planCtx.setResolvedField- Maintains stack (
push) and call ID (increment) state consistentlyThe stack push on line 473 is balanced by the pop in
LeaveFieldline 423, and theparentCallIDincrement on line 477 is balanced by the decrement on line 428. Based on learnings, the system guarantees field resolvers have arguments, ensuring these operations are always matched.v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (11)
152-154: LGTM! Variadic signature improves API ergonomics.The change from
Add(fragmentName string, field RPCField)toAdd(fragmentName string, field ...RPCField)with slice append simplifies call sites that add multiple fields.
719-794: Well-structured composite type handling.This function correctly separates inline fragment handling from regular field handling and properly initializes
FieldSelectionSetonly when inline fragments are present. The early return with error for fields without selections prevents invalid state.
805-823: LGTM! Clean struct design for composite type support.The
resolverFieldstruct extensions and newfragmentSelectiontype provide clear modeling for inline fragment handling.
825-854: LGTM! Proper handling of composite selection sets.The function correctly handles both cases: direct field selections on interfaces and inline fragment selections. Setting
fieldsSelectionSetReftoInvalidRefinitially and conditionally restoring it is the right approach.
856-863: LGTM!Simple and correct implementation leveraging the invariant that field resolvers must have arguments. Based on learnings, this invariant is enforced by the system design.
865-895: LGTM! Clear composite type resolution.Both functions correctly handle interface and union types, with appropriate error messages when member types cannot be resolved.
1053-1119: LGTM! Intentional two-branch design for field resolver responses.Based on learnings, this implementation correctly uses two distinct approaches:
message.Fieldsfor simple GraphQL object types, andmessage.FieldSelectionSetexclusively for composite types (interface/union) to align with protobuf oneOf structure. The early return at line 1087-1089 when only fragment selections exist is intentional.
1121-1149: LGTM! Proper nested message handling.The function correctly builds nested messages recursively when the field has selections, maintaining consistency with the composite field building pattern.
1151-1189: LGTM! Addresses past review feedback.The function now correctly skips nested field resolvers (line 1158-1160) as requested in the previous review. The recursive message building maintains consistency with
buildRequiredField.
1191-1203: LGTM!Clean helper function with appropriate use of
unsafebytes.StringToBytesfor efficient conversion.
1205-1275: LGTM! Consistent updates for resolverField-centric model.The changes correctly adapt to the renamed
resolverFieldtype and Node-based type references throughout the function.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
151-154: Verify error handling contract change is intentional.The error handling now writes errors to the output buffer and returns
nilinstead of propagating errors via the return value. This changes the API contract - callers previously checkingerr != nilwill now need to parse the output for errors.This appears intentional for consistency (matching the disabled datasource case at lines 97-99 and merge error at lines 164-166), but please confirm this is the desired behavior for the gRPC datasource.
Also applies to: 170-173
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
734-738: LGTM! Correct list value wrapping.Wrapping the list in
protoref.ValueOfList()ensures consistent value representation that aligns with the updatedsetMessageValuelist handling logic.
836-863: LGTM! Robust field validation and list handling.The updated
setMessageValueproperly validates field existence and handles the protobuf type descriptor constraint for list fields by copying values. ThecopyListValueshelper is clean and correct.
| if fd.IsList() { | ||
| return []protoref.Value{field} | ||
| return []protoref.Value{protoref.ValueOfList(field.List())} | ||
| } |
There was a problem hiding this comment.
Error from setMessageValue is not checked.
Similarly, the call on line 629 ignores the returned error. If the field_args field doesn't exist in the root message, this would fail silently.
- // Set the args field
- p.setMessageValue(rootMessage, argsRPCField.Name, protoref.ValueOfMessage(args))
+ // Set the args field
+ if err := p.setMessageValue(rootMessage, argsRPCField.Name, protoref.ValueOfMessage(args)); err != nil {
+ return nil, err
+ }🧰 Tools
🪛 GitHub Check: Linters (1.25)
[failure] 629-629:
Error return value of p.setMessageValue is not checked (errcheck)
🤖 Prompt for AI Agents
In v2/pkg/engine/datasource/grpc_datasource/compiler.go around lines 628 to 630,
the call to p.setMessageValue(rootMessage, argsRPCField.Name,
protoref.ValueOfMessage(args)) ignores the returned error; modify the code to
capture the error return and handle it instead of failing silently — e.g.,
assign the result to err and if err != nil propagate it (return the error,
wrapping with context that setting field "field_args" failed) or otherwise
handle/log and return so callers are aware of the failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (4)
719-794: buildFieldMessage correctly separates direct fields vs inline fragments, but depends on type-conditioned fragmentsThe structure—splitting
SelectionRefsintofieldRefsandinlineFragmentRefs, computing composite metadata viagetCompositeType/getMemberTypes, then delegating fragment handling tobuildCompositeFields—looks sound and aligns with the new composite handling story.One subtle edge case: if an inline fragment without a type condition (
... {}) ever reaches this layer,InlineFragmentTypeConditionNameStringwill likely be empty, causingNodeByNameStrto fail and this function to error. If the planner pipeline guarantees that all inline fragments here are type-conditioned (which seems likely), this is fine; otherwise you may want an explicit guard that treats missing type conditions as “use enclosing type” rather than hard-failing.
855-862: isFieldResolver relies on “has arguments” invariantThe heuristic
len(r.operation.FieldArguments(fieldRef)) > 0for non-root fields matches the documented invariant that field resolvers must have arguments and that non-resolver nested fields in this datasource don’t. If this ever changes (e.g. you add non-resolver nested fields with arguments or support argument-less resolvers), this check will silently misclassify fields and skip them in required-field/composite planning.Consider either documenting this invariant here or centralizing the resolver classification in one place (e.g. via mapping/directive presence) so future extensions don’t break this assumption. Based on learnings, this is intentional today, but it’s brittle.
1120-1202: Special-case __typename and consider inline-fragment-without-type handling
buildRequiredFieldandbuildCompositeFieldsboth rely onNodeFieldDefinitionByName/fieldDefinitionRefForTypefor resolving field definitions:
- For explicit
__typenameselections, there is normally no SDLFieldDefinitionon the type;NodeFieldDefinitionByNamewill return “not found”, andbuildRequiredFieldwill error with “unable to build required field: field definition not found for field __typename”. At the same time, tests inTestExecutionPlanFieldResolvers_CustomSchemasexpect__typenameto be supported on composite resolver results and to produce aRPCFieldwith a static value (using thetypenameFieldNamelogic).- Unless you’ve extended the AST to synthesize
__typenamedefinitions intoNodeFieldDefinitions, this path seems fragile.A robust approach would be to special-case
__typenameinbuildRequiredField:
- If
fieldName == typenameFieldName, construct theRPCFielddirectly (usingparentTypeNode.NameStringandDataTypeString) instead of going throughNodeFieldDefinitionByName, mirroring whatAppendTypeNameFielddoes.Separately,
buildCompositeFieldsobtains the underlyingtypeNamefromfragmentSelection.typeName, which is derived fromInlineFragmentTypeConditionNameString. Inline fragments without a type condition would give an empty type name and makeNodeByNameStr/NodeFieldDefinitionByNamefail. If your normalization/validation guarantees all inline fragments here are type-conditioned, that’s fine; otherwise consider an explicit check that falls back to the enclosing type or errors with a clearer message whentypeName == "".Please double-check that your current AST utilities indeed provide usable
FieldDefinitionentries for__typenamein all cases needed by these helpers; otherwise the new __typename custom-schema test may only pass accidentally or fail under different schema shapes.
1204-1274: Avoid taking the address of the range variable in createResolverRPCCallsIn the loop:
for _, resolvedField := range resolvedFields { // ... call, err := r.newResolveRPCCall(&resolveRPCCallConfig{ // ... resolvedField: &resolvedField, }) // ... }passing
&resolvedFieldis safe today because the pointer is only used synchronously insidenewResolveRPCCalland does not escape the loop. However, this pattern is brittle—ifnewResolveRPCCallis ever changed to store the pointer or use it asynchronously, it will suffer from the classic “pointer to range variable” issue.Given this is hot-path planning code, using an index-based loop (or explicitly copying into a new variable before taking its address) would remove that latent footgun with negligible cost:
for i := range resolvedFields { rf := &resolvedFields[i] // pass rf as resolvedField }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (9)
RPCExecutionPlan(57-62)RPCCall(78-94)RPCMessage(98-112)RPCField(183-212)OneOfType(29-29)OneOfTypeInterface(36-36)RPCFieldSelectionSet(149-149)OneOfTypeUnion(38-38)RPCFields(265-265)v2/pkg/ast/ast.go (1)
Document(10-56)v2/pkg/engine/datasource/grpc_datasource/configuration.go (7)
GRPCMapping(26-46)RPCConfigMap(19-19)RPCConfig(62-69)FieldMap(23-23)ResolveRPCMapping(90-90)FieldMapData(81-84)FieldArgumentMap(87-87)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
148-154: Variadic RPCFieldSelectionSet.Add is appropriateUsing a variadic parameter and appending into
r[fragmentName]is a clean improvement and matches how you useAdd(typeName, fields...)elsewhere. No issues here.v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (3)
901-1116: Nested resolver execution-plan tests look consistent and thoroughThe
TestExecutionPlanFieldResolvers_WithNestedResolverscases accurately exercise:
- Dependent call ordering (
DependentCalls: []int{0}then[]int{1}),- Context wiring via
ResolvePath(e.g.categories.categoryMetrics.id/value/metricType),- Argument mapping for nested resolvers (
baseline).The expected plans align with the new resolver planning logic and provide good coverage for nested resolver scenarios.
1118-2181: Composite-type resolver tests comprehensively validate interface/union handlingThe
TestExecutionPlanFieldResolvers_WithCompositeTypessuite does a solid job of verifying:
- Interface resolvers (
Animal) with per-typeFieldSelectionSetentries and correctOneOfType/MemberTypes.- Union resolvers (
ActionResult) for both top-level and nested cases.- Nested interface/union fields inside non-resolver objects (
TestDetails.pet,TestDetails.status), ensuring regular query planning also usesFieldSelectionSetplus oneof metadata appropriately.- Complex nested inline fragments with deep object graphs (owner/contact/breed/characteristics), which stress
buildFieldMessageandbuildCompositeFields.The expected plans match the execution-plan behavior in
execution_plan.go(including snake_caseResolvePathvs camelCaseJSONPathfor proto vs GraphQL), and should catch regressions in composite handling effectively.
2515-2620: Custom schema and mapping helpers are well-structured and consistent
schemaWithNestedResolverAndCompositeTypeandmappingWithNestedResolverAndCompositeTypeneatly define a minimal schema:
- An interface
Barwith implementationBaz,- Nested resolvers
Foo.fooResolverandBaz.bazResolverannotated with@connect__fieldResolver,- A corresponding
GRPCMappingwhoseQueryRPCs,Fields, andResolveRPCsentries line up with the schema (e.g.fooResolver→foo_resolver,bazResolver→baz_resolver).Running
DefaultDefinitionValidatoragainst the schema and failing fast on errors is good practice and ensures these tests won’t silently rely on an invalid definition. No issues spotted here.
| { | ||
| name: "Should not include nested resolvers in composite type selection set when building the execution plan", | ||
| subgraphName: "Foo", | ||
| operation: ` | ||
| query FooQuery($foo: String!, $baz: String!) { | ||
| foo { | ||
| fooResolver(foo: $foo) { | ||
| ... on Baz { | ||
| bazResolver(baz: $baz) | ||
| } | ||
| } | ||
| } | ||
| }`, | ||
| schema: schemaWithNestedResolverAndCompositeType(t), | ||
| mapping: mappingWithNestedResolverAndCompositeType(t), | ||
| expectedPlan: &RPCExecutionPlan{ | ||
| Calls: []RPCCall{ | ||
| { | ||
| ServiceName: "Foo", | ||
| MethodName: "QueryFoo", | ||
| Request: RPCMessage{ | ||
| Name: "QueryFooRequest", | ||
| }, | ||
| Response: RPCMessage{ | ||
| Name: "QueryFooResponse", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "foo", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "foo", | ||
| Message: &RPCMessage{ | ||
| Name: "Foo", | ||
| Fields: RPCFields{}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Kind: CallKindResolve, | ||
| DependentCalls: []int{0}, | ||
| ResponsePath: buildPath("foo.fooResolver"), | ||
| ServiceName: "Foo", | ||
| MethodName: "ResolveFooFooResolver", | ||
| Request: RPCMessage{ | ||
| Name: "ResolveFooFooResolverRequest", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "context", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveFooFooResolverContext", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "id", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "id", | ||
| ResolvePath: buildPath("foo.id"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "field_args", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveFooFooResolverArgs", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "foo", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "foo", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Response: RPCMessage{ | ||
| Name: "ResolveFooFooResolverResponse", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "result", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| JSONPath: "result", | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveFooFooResolverResult", | ||
| Fields: RPCFields{ | ||
| { | ||
| Name: "foo_resolver", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "fooResolver", | ||
| Message: &RPCMessage{ | ||
| Name: "Bar", | ||
| FieldSelectionSet: RPCFieldSelectionSet{"Baz": {}}, | ||
| OneOfType: OneOfTypeInterface, | ||
| MemberTypes: []string{"Baz"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Kind: CallKindResolve, | ||
| DependentCalls: []int{1}, | ||
| ResponsePath: buildPath("foo.fooResolver.bazResolver"), | ||
| ServiceName: "Foo", | ||
| MethodName: "ResolveBazBazResolver", | ||
| Request: RPCMessage{ | ||
| Name: "ResolveBazBazResolverRequest", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "context", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveBazBazResolverContext", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "id", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "id", | ||
| ResolvePath: buildPath("foo.fooResolver.id"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "field_args", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveBazBazResolverArgs", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "baz", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "baz", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Response: RPCMessage{ | ||
| Name: "ResolveBazBazResolverResponse", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "result", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| JSONPath: "result", | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveBazBazResolverResult", | ||
| Fields: RPCFields{ | ||
| { | ||
| Name: "baz_resolver", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "bazResolver", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Should correctly include typename field when providing empty selection set", | ||
| subgraphName: "Foo", | ||
| operation: ` | ||
| query FooQuery($foo: String!, $baz: String!) { | ||
| foo { | ||
| fooResolver(foo: $foo) { | ||
| __typename | ||
| } | ||
| } | ||
| }`, | ||
| schema: schemaWithNestedResolverAndCompositeType(t), | ||
| mapping: mappingWithNestedResolverAndCompositeType(t), | ||
| expectedPlan: &RPCExecutionPlan{ | ||
| Calls: []RPCCall{ | ||
| { | ||
| ServiceName: "Foo", | ||
| MethodName: "QueryFoo", | ||
| Request: RPCMessage{ | ||
| Name: "QueryFooRequest", | ||
| }, | ||
| Response: RPCMessage{ | ||
| Name: "QueryFooResponse", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "foo", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "foo", | ||
| Message: &RPCMessage{ | ||
| Name: "Foo", | ||
| Fields: RPCFields{}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Kind: CallKindResolve, | ||
| DependentCalls: []int{0}, | ||
| ResponsePath: buildPath("foo.fooResolver"), | ||
| ServiceName: "Foo", | ||
| MethodName: "ResolveFooFooResolver", | ||
| Request: RPCMessage{ | ||
| Name: "ResolveFooFooResolverRequest", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "context", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveFooFooResolverContext", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "id", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "id", | ||
| ResolvePath: buildPath("foo.id"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "field_args", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveFooFooResolverArgs", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "foo", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "foo", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Response: RPCMessage{ | ||
| Name: "ResolveFooFooResolverResponse", | ||
| Fields: []RPCField{ | ||
| { | ||
| Name: "result", | ||
| ProtoTypeName: DataTypeMessage, | ||
| Repeated: true, | ||
| JSONPath: "result", | ||
| Message: &RPCMessage{ | ||
| Name: "ResolveFooFooResolverResult", | ||
| Fields: RPCFields{ | ||
| { | ||
| Name: "foo_resolver", | ||
| ProtoTypeName: DataTypeMessage, | ||
| JSONPath: "fooResolver", | ||
| Message: &RPCMessage{ | ||
| Name: "Bar", | ||
| OneOfType: OneOfTypeInterface, | ||
| MemberTypes: []string{"Baz"}, | ||
| Fields: RPCFields{ | ||
| { | ||
| Name: "__typename", | ||
| ProtoTypeName: DataTypeString, | ||
| JSONPath: "__typename", | ||
| StaticValue: "Bar", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Second custom-schema test operation defines an unused variable $baz
In TestExecutionPlanFieldResolvers_CustomSchemas, the second test case (“Should correctly include typename field when providing empty selection set”) declares:
query FooQuery($foo: String!, $baz: String!) {
foo {
fooResolver(foo: $foo) {
__typename
}
}
}but $baz is never used. If your operation validation includes the standard NoUnusedVariables rule, this operation will be rejected before planning, and the test will fail. Even if that rule is currently disabled, keeping unused variables is confusing and can mask future validation issues.
Suggest either:
- Removing
$bazfrom the variable definitions, or - Using
$bazsomewhere in the selection (if that was originally intended).
This keeps the test strictly valid under common GraphQL validators and better reflects real-world operations.
🤖 Prompt for AI Agents
In
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
around lines 2207-2494 the GraphQL operation for the test "Should correctly
include typename field when providing empty selection set" declares an unused
variable $baz which will break validators enforcing NoUnusedVariables; remove
the unused variable from the operation definition (delete ", $baz: String!" from
the query variable list) or alternatively use $baz in the operation if it was
intended—prefer removing it to keep the test minimal and valid.
| // When a field resolver returns a complex or composite type, we need to build a message for the type. | ||
| func (r *rpcPlanningContext) buildFieldResolverTypeMessage(typeName string, resolverField *resolverField) (*RPCMessage, error) { | ||
| message := &RPCMessage{ | ||
| Name: typeName, | ||
| Name: typeName, | ||
| OneOfType: resolverField.fragmentType, | ||
| MemberTypes: resolverField.memberTypes, | ||
| } | ||
|
|
||
| rfv := newRequiredFieldsVisitor(walker, message, r) | ||
| if err := rfv.visitWithMemberTypes(r.definition, typeName, r.operation.SelectionSetFieldSetString(requiredFieldSelection), nil); err != nil { | ||
| return nil, err | ||
| // field resolvers which return a non scalar type must have a selection set. | ||
| // If we don't have a selection set we return an error. | ||
| if len(resolverField.fragmentSelections) == 0 && resolverField.fieldsSelectionSetRef == ast.InvalidRef { | ||
| return nil, errors.New("unable to resolve required fields: no fields selection set found") | ||
| } | ||
|
|
||
| // If the resolved field returns a composite type we need to handle the selection set for the inline fragment. | ||
| if len(resolverField.fragmentSelections) > 0 { | ||
| message.FieldSelectionSet = make(RPCFieldSelectionSet, len(resolverField.fragmentSelections)) | ||
|
|
||
| for _, fragmentSelection := range resolverField.fragmentSelections { | ||
| inlineFragmentTypeNode, found := r.definition.NodeByNameStr(fragmentSelection.typeName) | ||
| if !found { | ||
| return nil, fmt.Errorf("unable to build composite field: underlying fragment type node not found for type %s", fragmentSelection.typeName) | ||
| } | ||
|
|
||
| fields, err := r.buildCompositeFields(inlineFragmentTypeNode, fragmentSelection) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| message.FieldSelectionSet[fragmentSelection.typeName] = fields | ||
| } | ||
| } | ||
|
|
||
| if resolverField.fieldsSelectionSetRef == ast.InvalidRef { | ||
| return message, nil | ||
| } | ||
|
|
||
| // If the resolved field does not return a composite type we handle the selection set for the required field. | ||
| parentTypeNode, found := r.definition.NodeByNameStr(typeName) | ||
| if !found { | ||
| return nil, fmt.Errorf("parent type node not found for type %s", typeName) | ||
| } | ||
|
|
||
| fieldRefs := r.operation.SelectionSetFieldRefs(resolverField.fieldsSelectionSetRef) | ||
| message.Fields = make(RPCFields, 0, len(fieldRefs)) | ||
|
|
||
| for _, fieldRef := range fieldRefs { | ||
| if r.isFieldResolver(fieldRef, false) { | ||
| continue | ||
| } | ||
|
|
||
| if message.Fields.Exists(r.operation.FieldNameString(fieldRef), "") { | ||
| continue | ||
| } | ||
|
|
||
| field, err := r.buildRequiredField(parentTypeNode, fieldRef) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| message.Fields = append(message.Fields, field) | ||
| } | ||
|
|
||
| message.Fields = slices.Clip(message.Fields) | ||
| return message, nil | ||
| } |
There was a problem hiding this comment.
Composite resolver messages can mix Fields and FieldSelectionSet, contradicting earlier oneof constraints
buildFieldResolverTypeMessage always sets OneOfType/MemberTypes from resolverField, builds FieldSelectionSet when fragmentSelections is non-empty, and then also populates message.Fields whenever fieldsSelectionSetRef != ast.InvalidRef.
For composite resolver return types (interface/union), enterResolverCompositeSelectionSet can set both:
fieldsSelectionSetRef(for interface-level fields selected directly on the interface), andfragmentSelections(for inline fragments).
In that case this function will produce a message that combines:
FieldSelectionSetentries for each fragment type, and- top-level
Fieldsfor the interface-level selections.
Earlier in this PR, resolveRequiredFields was deliberately changed to avoid exactly this mix for composite resolver responses, ensuring that all selections—including common interface fields—are encoded via fragment-based FieldSelectionSet to align with the protobuf oneof representation. The prior conclusion was that “we can’t have both” for resolver responses. Based on learnings, this function appears to reintroduce the mixed structure whenever a composite resolver selection contains both inline fragments and interface-level fields.
You might want to:
- Either forbid this combination (e.g. error or ignore
fieldsSelectionSetRefwhenfragmentType != OneOfTypeNone && len(fragmentSelections) > 0), or - Change the visitor so that interface-level fields are duplicated into each fragment and
fieldsSelectionSetRefstays invalid for composite resolvers, keeping resolver responses purely fragment-based.
Right now there are no tests that cover “interface fields + inline fragments” for resolver return types, so this corner case is unverified but could cause mismatches in the oneof mapping at runtime.
Based on learnings, the composite resolver path should remain fragment-only for oneof responses.
🤖 Prompt for AI Agents
In v2/pkg/engine/datasource/grpc_datasource/execution_plan.go around lines
1053-1118, the function buildFieldResolverTypeMessage can produce a message that
mixes FieldSelectionSet (fragment-based entries) and top-level Fields when a
resolver returns a composite (oneof) type, which violates the intended protobuf
oneof model; change the logic so that if resolverField.fragmentType !=
OneOfTypeNone and len(resolverField.fragmentSelections) > 0 and
resolverField.fieldsSelectionSetRef != ast.InvalidRef, you should reject this
combination (return a clear error) or otherwise ignore fieldsSelectionSetRef —
implement the safer approach by returning an error indicating that composite
resolver responses cannot contain both fragment selections and interface-level
fields, and ensure no top-level message.Fields are built when fragmentSelections
are present.
This PR improves the existing functionality for field resolvers.
Currently field resolvers do not support returning any form of composite types (interfaces and unions). This feature will be provided in this PR.
The planner will now determine the correct selection set and identify, whether the field selections are part of an inline fragment. The calling logic will always inline all fragments provided to the datasource, therefore we do not need to handle fragments and spreads.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist