feat: add composite type support for field resolvers#1341
Conversation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…or-graphql-operation
…or-graphql-operation
…or-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
… ludwig/eng-6993-implement-field-resolver-for-graphql-operation
…or-graphql-operation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
606-611: Error fromsetMessageValueis not checked.The call to
p.setMessageValue(valMsg, fieldName, value)on line 607 now returns an error that is not being handled. This could silently fail if the field doesn't exist in the message.for fieldName, value := range data { - p.setMessageValue(valMsg, fieldName, value) + if err := p.setMessageValue(valMsg, fieldName, value); err != nil { + return nil, err + } }
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
425-428: Update misleading commentThe comment states "If the field has arguments, we need to decrement..." but the code unconditionally decrements
parentCallIDon line 428 without checking for arguments. According to the learnings, the system enforces an invariant that field resolvers must have arguments, so the unconditional decrement is correct. However, the comment is misleading.Update the comment to reflect the actual behavior:
- // If the field has arguments, we need to decrement the related call ID. - // This is because we can also have nested arguments, which require the underlying field to be resolved - // by values provided by the parent call. + // Decrement the parent call ID to restore the caller context. + // Field resolvers always have arguments (system invariant), so we always increment in enterFieldResolver + // and decrement here to maintain correct caller references for sibling fields. r.parentCallID--
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/datasource/grpc_datasource/compiler.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
RPCExecutionPlan(57-62)v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
InvalidRef(21-21)Message(182-186)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)v2/pkg/ast/path.go (1)
Path(34-34)v2/pkg/ast/ast_field_definition.go (1)
FieldDefinition(17-26)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)v2/pkg/operationreport/externalerror.go (1)
ExternalError(34-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (20)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (6)
57-59: LGTM: Resolver tracking fieldsThe new fields (
parentCallID,fieldResolverAncestors,resolverFields) provide the necessary state to track resolver field hierarchy and support composite type resolution. The initialization in the constructor is consistent.
115-127: LGTM: Resolver call creationThe document cleanup correctly creates resolver RPC calls from
resolverFieldsand clears state. The logic is sound.
200-220: LGTM: Composite type handling in resolver contextsThe enhanced logic correctly distinguishes between composite types (interface/union) and simple object types within resolver fields. The call to
enterResolverCompositeSelectionSetfor composite types aligns with the PR objective of adding interface/union support to field resolvers.
351-406: LGTM: Centralized field handling logicThe refactored
EnterFieldcleanly separates concerns:
- Field resolvers → delegate to
enterFieldResolver- Non-resolver fields inside resolver context → skip (prevents incorrect nesting)
- Regular fields → standard processing with duplicate prevention
The
SkipNode()call on line 369 ensuresLeaveFieldisn't invoked for skipped fields, maintaining stack balance. Based on learnings, field resolvers must have arguments, so the skip behavior is safe.
438-449: LGTM: Clean extraction of finalization logicThe
finalizeCallmethod is a well-structured extraction that improves readability inLeaveField. The state management (assigning call, advancing IDs, resetting indices) is correct.
451-478: LGTM: Well-structured resolver field entryThe
enterFieldResolvermethod cleanly encapsulates resolver field setup:
- Creates
resolverFieldwith correctcallerReffrom currentparentCallID- Delegates detailed configuration to
planCtx.setResolvedField- Maintains stack (
push) and call ID (increment) state consistentlyThe stack push on line 473 is balanced by the pop in
LeaveFieldline 423, and theparentCallIDincrement on line 477 is balanced by the decrement on line 428. Based on learnings, the system guarantees field resolvers have arguments, ensuring these operations are always matched.v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (11)
152-154: LGTM! Variadic signature improves API ergonomics.The change from
Add(fragmentName string, field RPCField)toAdd(fragmentName string, field ...RPCField)with slice append simplifies call sites that add multiple fields.
719-794: Well-structured composite type handling.This function correctly separates inline fragment handling from regular field handling and properly initializes
FieldSelectionSetonly when inline fragments are present. The early return with error for fields without selections prevents invalid state.
805-823: LGTM! Clean struct design for composite type support.The
resolverFieldstruct extensions and newfragmentSelectiontype provide clear modeling for inline fragment handling.
825-854: LGTM! Proper handling of composite selection sets.The function correctly handles both cases: direct field selections on interfaces and inline fragment selections. Setting
fieldsSelectionSetReftoInvalidRefinitially and conditionally restoring it is the right approach.
856-863: LGTM!Simple and correct implementation leveraging the invariant that field resolvers must have arguments. Based on learnings, this invariant is enforced by the system design.
865-895: LGTM! Clear composite type resolution.Both functions correctly handle interface and union types, with appropriate error messages when member types cannot be resolved.
1053-1119: LGTM! Intentional two-branch design for field resolver responses.Based on learnings, this implementation correctly uses two distinct approaches:
message.Fieldsfor simple GraphQL object types, andmessage.FieldSelectionSetexclusively for composite types (interface/union) to align with protobuf oneOf structure. The early return at line 1087-1089 when only fragment selections exist is intentional.
1121-1149: LGTM! Proper nested message handling.The function correctly builds nested messages recursively when the field has selections, maintaining consistency with the composite field building pattern.
1151-1189: LGTM! Addresses past review feedback.The function now correctly skips nested field resolvers (line 1158-1160) as requested in the previous review. The recursive message building maintains consistency with
buildRequiredField.
1191-1203: LGTM!Clean helper function with appropriate use of
unsafebytes.StringToBytesfor efficient conversion.
1205-1275: LGTM! Consistent updates for resolverField-centric model.The changes correctly adapt to the renamed
resolverFieldtype and Node-based type references throughout the function.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
151-154: Verify error handling contract change is intentional.The error handling now writes errors to the output buffer and returns
nilinstead of propagating errors via the return value. This changes the API contract - callers previously checkingerr != nilwill now need to parse the output for errors.This appears intentional for consistency (matching the disabled datasource case at lines 97-99 and merge error at lines 164-166), but please confirm this is the desired behavior for the gRPC datasource.
Also applies to: 170-173
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
734-738: LGTM! Correct list value wrapping.Wrapping the list in
protoref.ValueOfList()ensures consistent value representation that aligns with the updatedsetMessageValuelist handling logic.
836-863: LGTM! Robust field validation and list handling.The updated
setMessageValueproperly validates field existence and handles the protobuf type descriptor constraint for list fields by copying values. ThecopyListValueshelper is clean and correct.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (4)
719-794: buildFieldMessage correctly separates direct fields vs inline fragments, but depends on type-conditioned fragmentsThe structure—splitting
SelectionRefsintofieldRefsandinlineFragmentRefs, computing composite metadata viagetCompositeType/getMemberTypes, then delegating fragment handling tobuildCompositeFields—looks sound and aligns with the new composite handling story.One subtle edge case: if an inline fragment without a type condition (
... {}) ever reaches this layer,InlineFragmentTypeConditionNameStringwill likely be empty, causingNodeByNameStrto fail and this function to error. If the planner pipeline guarantees that all inline fragments here are type-conditioned (which seems likely), this is fine; otherwise you may want an explicit guard that treats missing type conditions as “use enclosing type” rather than hard-failing.
855-862: isFieldResolver relies on “has arguments” invariantThe heuristic
len(r.operation.FieldArguments(fieldRef)) > 0for non-root fields matches the documented invariant that field resolvers must have arguments and that non-resolver nested fields in this datasource don’t. If this ever changes (e.g. you add non-resolver nested fields with arguments or support argument-less resolvers), this check will silently misclassify fields and skip them in required-field/composite planning.Consider either documenting this invariant here or centralizing the resolver classification in one place (e.g. via mapping/directive presence) so future extensions don’t break this assumption. Based on learnings, this is intentional today, but it’s brittle.
1120-1202: Special-case __typename and consider inline-fragment-without-type handling
buildRequiredFieldandbuildCompositeFieldsboth rely onNodeFieldDefinitionByName/fieldDefinitionRefForTypefor resolving field definitions:
- For explicit
__typenameselections, there is normally no SDLFieldDefinitionon the type;NodeFieldDefinitionByNamewill return “not found”, andbuildRequiredFieldwill error with “unable to build required field: field definition not found for field __typename”. At the same time, tests inTestExecutionPlanFieldResolvers_CustomSchemasexpect__typenameto be supported on composite resolver results and to produce aRPCFieldwith a static value (using thetypenameFieldNamelogic).- Unless you’ve extended the AST to synthesize
__typenamedefinitions intoNodeFieldDefinitions, this path seems fragile.A robust approach would be to special-case
__typenameinbuildRequiredField:
- If
fieldName == typenameFieldName, construct theRPCFielddirectly (usingparentTypeNode.NameStringandDataTypeString) instead of going throughNodeFieldDefinitionByName, mirroring whatAppendTypeNameFielddoes.Separately,
buildCompositeFieldsobtains the underlyingtypeNamefromfragmentSelection.typeName, which is derived fromInlineFragmentTypeConditionNameString. Inline fragments without a type condition would give an empty type name and makeNodeByNameStr/NodeFieldDefinitionByNamefail. If your normalization/validation guarantees all inline fragments here are type-conditioned, that’s fine; otherwise consider an explicit check that falls back to the enclosing type or errors with a clearer message whentypeName == "".Please double-check that your current AST utilities indeed provide usable
FieldDefinitionentries for__typenamein all cases needed by these helpers; otherwise the new __typename custom-schema test may only pass accidentally or fail under different schema shapes.
1204-1274: Avoid taking the address of the range variable in createResolverRPCCallsIn the loop:
for _, resolvedField := range resolvedFields { // ... call, err := r.newResolveRPCCall(&resolveRPCCallConfig{ // ... resolvedField: &resolvedField, }) // ... }passing
&resolvedFieldis safe today because the pointer is only used synchronously insidenewResolveRPCCalland does not escape the loop. However, this pattern is brittle—ifnewResolveRPCCallis ever changed to store the pointer or use it asynchronously, it will suffer from the classic “pointer to range variable” issue.Given this is hot-path planning code, using an index-based loop (or explicitly copying into a new variable before taking its address) would remove that latent footgun with negligible cost:
for i := range resolvedFields { rf := &resolvedFields[i] // pass rf as resolvedField }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (9)
RPCExecutionPlan(57-62)RPCCall(78-94)RPCMessage(98-112)RPCField(183-212)OneOfType(29-29)OneOfTypeInterface(36-36)RPCFieldSelectionSet(149-149)OneOfTypeUnion(38-38)RPCFields(265-265)v2/pkg/ast/ast.go (1)
Document(10-56)v2/pkg/engine/datasource/grpc_datasource/configuration.go (7)
GRPCMapping(26-46)RPCConfigMap(19-19)RPCConfig(62-69)FieldMap(23-23)ResolveRPCMapping(90-90)FieldMapData(81-84)FieldArgumentMap(87-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
148-154: Variadic RPCFieldSelectionSet.Add is appropriateUsing a variadic parameter and appending into
r[fragmentName]is a clean improvement and matches how you useAdd(typeName, fields...)elsewhere. No issues here.v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (3)
901-1116: Nested resolver execution-plan tests look consistent and thoroughThe
TestExecutionPlanFieldResolvers_WithNestedResolverscases accurately exercise:
- Dependent call ordering (
DependentCalls: []int{0}then[]int{1}),- Context wiring via
ResolvePath(e.g.categories.categoryMetrics.id/value/metricType),- Argument mapping for nested resolvers (
baseline).The expected plans align with the new resolver planning logic and provide good coverage for nested resolver scenarios.
1118-2181: Composite-type resolver tests comprehensively validate interface/union handlingThe
TestExecutionPlanFieldResolvers_WithCompositeTypessuite does a solid job of verifying:
- Interface resolvers (
Animal) with per-typeFieldSelectionSetentries and correctOneOfType/MemberTypes.- Union resolvers (
ActionResult) for both top-level and nested cases.- Nested interface/union fields inside non-resolver objects (
TestDetails.pet,TestDetails.status), ensuring regular query planning also usesFieldSelectionSetplus oneof metadata appropriately.- Complex nested inline fragments with deep object graphs (owner/contact/breed/characteristics), which stress
buildFieldMessageandbuildCompositeFields.The expected plans match the execution-plan behavior in
execution_plan.go(including snake_caseResolvePathvs camelCaseJSONPathfor proto vs GraphQL), and should catch regressions in composite handling effectively.
2515-2620: Custom schema and mapping helpers are well-structured and consistent
schemaWithNestedResolverAndCompositeTypeandmappingWithNestedResolverAndCompositeTypeneatly define a minimal schema:
- An interface
Barwith implementationBaz,- Nested resolvers
Foo.fooResolverandBaz.bazResolverannotated with@connect__fieldResolver,- A corresponding
GRPCMappingwhoseQueryRPCs,Fields, andResolveRPCsentries line up with the schema (e.g.fooResolver→foo_resolver,bazResolver→baz_resolver).Running
DefaultDefinitionValidatoragainst the schema and failing fast on errors is good practice and ensures these tests won’t silently rely on an invalid definition. No issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (1)
2322-2620: Custom schema tests: behaviour LGTM; unused$bazvariable still present
- The first custom-schema test (“Should not include nested resolvers in composite type selection set…”) correctly asserts that the
BarinterfaceFieldSelectionSetfor memberBazis empty, i.e. the nestedbazResolveris not pulled into the composite selection, which matches the newisFieldResolver-skipping logic.- The second test (“Should correctly include typename field when providing empty selection set”) nicely pins the
__typenamehandling for interface-returning resolvers: the result message forBaris a oneof interface with a plainFieldsentry for__typenameand static value"Bar".One minor leftover: the second operation still declares but doesn’t use
$baz:query FooQuery($foo: String!, $baz: String!) { foo { fooResolver(foo: $foo) { __typename } } }If
NoUnusedVariablesvalidation is enabled, this operation will be rejected before planning. Consider either dropping$bazfrom the variable list or actually using it if there’s a future extension planned.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
805-853: ResolverField composite selection bookkeeping is mostly clear; consider edge-case testsThe
resolverFieldstruct plusenterResolverCompositeSelectionSet,getCompositeType, andgetMemberTypesgive a clear model for composite resolver return types:
enterResolverCompositeSelectionSet:
- records a top-level selection set for interface-level fields (when any exist), and
- collects per-inline-fragment
fragmentSelectionsfor member types.isFieldResolverrelies on the invariant that all non-root resolvers have arguments, which is aligned with prior design notes; nested resolvers are correctly excluded from bothbuildFieldMessageandbuildCompositeFields.getCompositeTypeandgetMemberTypesderive oneof metadata from the schema in an idiomatic way.One subtle corner case: if you ever support a resolver selection that mixes interface-level fields and inline fragments on the same composite return type (e.g.
mascot { id ... on Cat { name } }), the current bookkeeping will set bothfieldsSelectionSetRefandfragmentSelections. That leadsbuildFieldResolverTypeMessageto produce a message that has bothFieldsandFieldSelectionSetpopulated for a oneof. If that combination is intended and supported by the JSON builder, a targeted test exercising this pattern would be useful; otherwise, you might want to makeenterResolverCompositeSelectionSetorbuildFieldResolverTypeMessageexplicitly choose one representation.Also applies to: 856-893
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-11-19T10:53:06.342Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:42:17.644Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-11-19T09:38:25.112Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 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/ast/path.go (1)
Path(34-34)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
InvalidRef(21-21)DataTypeMessage(38-38)Message(182-186)v2/pkg/ast/ast.go (1)
InvalidRef(8-8)
⏰ 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 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (8)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (4)
698-823: CategoryMetrics alias resolver test matches expected RPC shapeThe new
CategoriesWithNullableTypes+ aliases case correctly exercises aliases on message-returning field resolvers and asserts that:
- the response path remains
categories.categoryMetrics, and- the nested
CategoryMetricsfields keep proto names (id,metric_type,value) while usingAliasfor the GraphQL names (myId,myMetricType,myValue).This lines up with the JSON-building semantics and should guard alias handling for resolver result messages well.
1027-1229: Nested resolver test validates dependency ordering and context wiring
TestExecutionPlanFieldResolvers_WithNestedResolverslooks consistent:
- Call 0 fetches
categories.- Call 1 resolves
categoryMetricswith context fromcategoriesand argument mapping formetricType.- Call 2 depends on call 1, resolving
normalizedScorewith context fromcategoryMetrics(id/value/metricType) plus thebaselineargument.Skipping nested resolvers via
ResolvePathwhile still feeding their results into dependent calls matches the intended planner behaviour.
1244-2307: Composite resolver tests cover interface/union and nested combinations wellThe
WithCompositeTypessuite does a good job of pinning down planner behaviour for composite resolver return types:
- Interface and union resolvers (
mascot,categoryStatus) correctly setOneOfType,MemberTypes, andFieldSelectionSetper concrete type.- Nested interface/union cases under
testContainers.details.petand.statusassert that:
- top-level container messages (
TestContainer,TestDetails) remain plain message types, andAnimal/ActionResultare shaped as oneof-style messages with per-memberFieldSelectionSetentries.Also, the use of
ResolvePath: buildPath("test_containers...")againstJSONPath: "testContainers..."correctly reflects proto vs GraphQL casing. Based on learnings, this distinction is intentional and should not be “normalized” further.
2641-2745: Schema/mapping helpers align cleanly with custom tests
schemaWithNestedResolverAndCompositeTypeandmappingWithNestedResolverAndCompositeTypeare consistent with the custom tests:
Foo.fooResolverandBaz.bazResolverare annotated with@connect__fieldResolver(context: "id"), and the mapping wiresTargetNames to the expected proto fields (foo_resolver,baz_resolver, etc.).ResolveRPCsentries forFoo.fooResolverandBaz.bazResolvermatch the RPC names and request/response messages asserted in the tests.This scaffolding should give the planner exactly the surface the custom tests exercise.
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (4)
96-152: OneOfType and RPCFieldSelectionSet API shape looks soundThe extensions to
RPCMessage(FieldSelectionSet,OneOfType,MemberTypes) plus theOneOfTypeenum and variadicRPCFieldSelectionSet.Addare coherent:
IsOneOfandSelectValidTypesgive the JSON builder enough information to resolve fragment selections for interface/union responses.AppendTypeNameFieldprovides a simple hook to inject__typenamewhen needed without duplicating checks.- The variadic
Addkeeps callers explicit about fragment membership while allowingbuildCompositeFieldsto pass slices directly.No issues noted in this foundational wiring.
719-794: Field message construction for nested selections and composites is consistentThe trio
buildFieldMessage,buildRequiredField, andbuildCompositeFieldsintegrates well:
buildFieldMessagesplits a field’s selection set into:
- plain fields (
fieldRefs), and- inline fragments (
inlineFragmentRefs),
then:- attaches
FieldSelectionSetentries per fragment type for composite parent types, and- populates
message.Fieldswith non-resolver fields viabuildRequiredField.buildRequiredFielddelegates tobuildFieldfor scalar/list/enum handling and only recurses intobuildFieldMessagewhen the field is amessagetype and has selections, which avoids spurious nested messages.buildCompositeFieldsmirrors this for a specific inline-fragment type, usingfieldDefinitionRefForTypeand again skipping nested field resolvers viaisFieldResolver, which matches the new tests that ensure nested resolvers aren’t inlined into composite selection sets.Overall this fixes the earlier issue of composite inline-fragment fields dropping their nested messages and keeps recursion strictly tied to actual selection sets, bounded by the operation structure.
Also applies to: 1120-1188, 1190-1202
1052-1117: buildFieldResolverTypeMessage enforces selections and skips nested resolvers correctly
buildFieldResolverTypeMessageplus its usage innewResolveRPCCalllooks robust:
- It enforces that any non-scalar resolver return type must have either a composite-fragment selection or a plain selection set, otherwise it fails fast with a clear error.
- For composite resolver return types (
fragmentSelectionsnon-empty), it buildsFieldSelectionSetentries viabuildCompositeFields, inheriting the nested-resolver skipping there.- For non-composite return types, or composite types with only interface-level selections (like the
__typename-only custom-schema test), it populatesmessage.FieldsfromfieldsSelectionSetRefviabuildRequiredField, again skipping nested resolvers.newResolveRPCCallonly calls this helper when the underlying type isDataTypeMessage, and wires the resulting message into theresultpayload under the correctTargetName, preserving nullability fromfieldDefinitionTypeRef.This gives a single, well-defined path to shape resolver result messages and aligns with the new test expectations for nested and composite resolver outputs.
Also applies to: 1283-1325
1204-1274: createResolverRPCCalls correctly reuses resolver metadata and context/args messagesThe updated
createResolverRPCCallsuses the newresolverFieldstructure cleanly:
- Resolve mapping lookup now keys on
resolvedField.parentTypeNode.NameString(r.definition)and the operation field name, which is more robust than stringly-typed type names from elsewhere.- Passing
&resolvedFieldintonewResolveRPCCallis safe here because it’s only used synchronously; the later population ofcontextMessage.FieldsandfieldArgsMessage.Fieldsmutates the same message instances already embedded into the RPCCall.- Context paths and argument mappings are preserved:
contextMessagefields are built fromresolvedField.contextFieldswithResolvePathset, andfieldArgsMessagecomes fromcreateRPCFieldFromFieldArgument.This keeps resolver call construction coherent with the visitor’s bookkeeping and the expectations encoded in the tests.
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 (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1039-1097
Timestamp: 2025-11-19T10:53:06.342Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling, the `resolveRequiredFields` function intentionally uses two distinct approaches: for simple GraphQL object types it populates `message.Fields`, while for composite types (interface/union) it exclusively uses `message.FieldSelectionSet` with fragment-based selections. This differs from `buildFieldMessage` (regular queries) because field resolver responses returning composite types must align with protobuf oneOf structure, where all selections—including common interface fields—are handled through fragment selections built by `buildCompositeField`. The two approaches cannot be mixed in field resolver responses.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go:1418-1526
Timestamp: 2025-11-19T09:38:25.112Z
Learning: In v2/pkg/engine/datasource/grpc_datasource test files, ResolvePath fields use snake_case (e.g., "test_containers.id") because they reference proto message field names, while JSONPath fields use camelCase (e.g., "testContainers") because they reference GraphQL/JSON response field names. Both casing styles are intentional and correct.
📚 Learning: 2025-12-02T08:25:26.682Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan.go:1053-1118
Timestamp: 2025-12-02T08:25:26.682Z
Learning: In v2/pkg/engine/datasource/grpc_datasource field resolver response handling for composite types (interface/union), buildFieldResolverTypeMessage correctly combines both message.Fields and message.FieldSelectionSet: message.Fields contains interface-level fields that can be selected directly on the interface (such as __typename or fields defined in the interface itself), while message.FieldSelectionSet contains type-specific fields from inline fragments. This mixing is intentional and correct for GraphQL interfaces, as interface-level fields exist outside inline fragment selections and must be handled separately from type-specific fragment selections.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
📚 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.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.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.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.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.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-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
⏰ 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 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go (3)
1027-1242: LGTM!The nested resolver test correctly models the dependency chain where
ResolveCategoryMetricsNormalizedScore(call 2) depends onResolveCategoryCategoryMetrics(call 1), and the expected plan accurately reflects the nested resolver context and response paths.
1244-2320: LGTM!Comprehensive test coverage for composite types including interfaces, unions, and complex nested scenarios. The use of
FieldSelectionSetwithOneOfTypeandMemberTypescorrectly models GraphQL polymorphic selections, and the casing conventions (snake_case forResolvePathproto fields, camelCase forJSONPathGraphQL fields) are correctly applied throughout.Based on learnings, ResolvePath uses snake_case for proto message field names while JSONPath uses camelCase for GraphQL/JSON response field names.
2322-2746: LGTM!The custom schema tests validate complex scenarios including nested resolvers within composite types and typename handling. Both test cases correctly declare and use their variables, and the helper functions properly construct and validate the test schema and mappings. The expected plans accurately reflect how field resolvers should behave with composite return types.
…resolvers_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
dkorittki
left a comment
dkorittki
left a comment
There was a problem hiding this comment.
2 comments. Will already approve this but better look at comments. I might or might not have catched something.
Noroth
deleted the
ludwig/eng-8297-add-interface-support-for-field-resolvers
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.240](v2.0.0-rc.239...v2.0.0-rc.240) (2025-12-08) ### Features * add composite type support for field resolvers ([#1341](#1341)) ([701ea8d](701ea8d)) * allow empty arguments for field resolvers ([#1350](#1350)) ([5db8c5a](5db8c5a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for composite types in field resolvers * Added support for empty arguments in field resolvers <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
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