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
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Outdated
Show resolved
Hide resolved
![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?
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