feat: add support for composite types in go-tools#1448
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors the gRPC compiler to use descriptor-driven field assignment, adds modular per-field handlers (oneofs, repeated, message, enum), changes null/presence semantics, threads GRPCMapping into the required-fields visitor with inline-fragment tracking, and expands federation tests, mappings, mock RPCs, and proto types for abstract/oneof scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Compiler
participant Dispatcher
participant Handlers
participant Proto
Client->>Compiler: provide JSON input + RPCMessage template
Compiler->>Dispatcher: iterate RPC fields -> processRPCField(field, fd, data)
alt oneof field
Dispatcher->>Handlers: buildOneOfMessage(__typename, fragmentFields)
Handlers->>Proto: select variant descriptor and populate sub-message
else repeated field
Dispatcher->>Handlers: processRepeatedField(elements, fd)
Handlers->>Handlers: build each element (may recurse)
Handlers->>Proto: append element to repeated field
else message field
Dispatcher->>Handlers: processMessageField(data, fd)
Handlers->>Handlers: resolveNestedMessage / wrapper handling
Handlers->>Proto: set nested message
else enum field
Dispatcher->>Handlers: processEnumField(name, fd)
Handlers->>Proto: set numeric enum value
else scalar field
Dispatcher->>Proto: set scalar value using fd
end
Compiler->>Compiler: isNullValue checks enforce optional/required semantics
Compiler->>Client: return populated protobuf message
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…n requiredFieldsVisitor
… ludwig/eng-9219-add-support-for-abstract-types-in-go-tools
Noroth
requested review from
StarpTech,
devsergiy,
dkorittki,
jensneuse and
ysmolski
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/required_fields_visitor.go (1)
110-125:⚠️ Potential issue | 🔴 CriticalReuse the existing parent field before descending into fragment-only sub-selections.
If the same object field is selected once at the interface level and then refined inside an inline fragment, the duplicate check skips creating the fragment-local parent field,
enterNestedFieldreturnsfalse, and the walker still records the fragment’s grandchildren on the enclosing composite message. For example,pet { owner { id } ... on Cat { owner { age } } }will shape theCatfragment incorrectly becauseageis no longer attached underowner.Message. This needs to merge/reuse the existingownerfield before descent, or stop traversing that subtree when no target child message was found.Based on learnings: for composite messages in this package, interface-level fields in
message.Fieldsand type-specific inline-fragment fields both need to be preserved correctly.Also applies to: 129-142, 163-200
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go (1)
1807-1819: Add one direct-interface@requirescase.These new cases only exercise fragment-local selections, so a regression that drops shared interface fields like
nameor__typenamefrom the composite request would still pass. Please add one case likeprimaryItem { name ... on PalletItem { palletCount } ... on ContainerItem { containerSize } }so the planner path for common interface fields is covered too.Based on learnings: composite
@requireshandling carries common interface fields through fragment-based selections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go` around lines 1807 - 1819, Add a test case that exercises a direct-interface `@requires` path which preserves common interface fields in fragment-local selections: in execution_plan_requires_test.go add a new case alongside "requires with interface type in selection set" that uses the same query structure but sets the federationConfigs SelectionSet for the Storage.itemInfo/primaryItem to "primaryItem { name ... on PalletItem { palletCount } ... on ContainerItem { containerSize } }" (or the equivalent selection used in the comment) so the planner is forced to carry through the shared interface field "name" and __typename across the composite request; update the test vector (the case name and mapping/testMapping()) accordingly so this new case runs with the existing test harness.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (1)
1724-1810: Please add one nested abstract-path datasource case.The planner suite now covers nested shapes like
securitySummary,itemHandlerInfo,itemSpecsInfo, anddeepItemInfo, but this end-to-end suite only runs the flatitemInfo/operationReportpaths. A single nested case here would protect the compiler + JSON execution path that is most likely to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go` around lines 1724 - 1810, Add one nested abstract-path test case to the existing testCases slice: create a case named like "Query Storage with deepItemInfo nested abstract path" that queries _entities for Storage returning id, name and a nested abstract field (use deepItemInfo or securitySummary/itemHandlerInfo as the nested path) and supply vars with representations that include the nested __typename structures (e.g., Storage -> deepItemInfo -> primaryItem with PalletItem/ContainerItem). In federationConfigs add the base Storage selection (id) and a FieldName entry for the nested abstract field with a SelectionSet that drills into the nested object and includes inline fragments for concrete types (e.g., primaryItem { ... on PalletItem { name palletCount } ... on ContainerItem { name containerSize } }). Implement validate to assert _entities length, Storage id/name and that the nested field string is formatted as expected, and set validateError to require.Empty; reuse existing pattern from the "itemInfo" and "operationReport" cases so test structure and names (testCases, federationConfigs, validate, validateError) match the surrounding tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go`:
- Around line 1807-1819: Add a test case that exercises a direct-interface
`@requires` path which preserves common interface fields in fragment-local
selections: in execution_plan_requires_test.go add a new case alongside
"requires with interface type in selection set" that uses the same query
structure but sets the federationConfigs SelectionSet for the
Storage.itemInfo/primaryItem to "primaryItem { name ... on PalletItem {
palletCount } ... on ContainerItem { containerSize } }" (or the equivalent
selection used in the comment) so the planner is forced to carry through the
shared interface field "name" and __typename across the composite request;
update the test vector (the case name and mapping/testMapping()) accordingly so
this new case runs with the existing test harness.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go`:
- Around line 1724-1810: Add one nested abstract-path test case to the existing
testCases slice: create a case named like "Query Storage with deepItemInfo
nested abstract path" that queries _entities for Storage returning id, name and
a nested abstract field (use deepItemInfo or securitySummary/itemHandlerInfo as
the nested path) and supply vars with representations that include the nested
__typename structures (e.g., Storage -> deepItemInfo -> primaryItem with
PalletItem/ContainerItem). In federationConfigs add the base Storage selection
(id) and a FieldName entry for the nested abstract field with a SelectionSet
that drills into the nested object and includes inline fragments for concrete
types (e.g., primaryItem { ... on PalletItem { name palletCount } ... on
ContainerItem { name containerSize } }). Implement validate to assert _entities
length, Storage id/name and that the nested field string is formatted as
expected, and set validateError to require.Empty; reuse existing pattern from
the "itemInfo" and "operationReport" cases so test structure and names
(testCases, federationConfigs, validate, validateError) match the surrounding
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c60c436e-f4cc-4659-bc5e-69ebd96ac345
⛔ Files ignored due to path filters (2)
v2/pkg/grpctest/productv1/product.pb.gois excluded by!**/*.pb.gov2/pkg/grpctest/productv1/product_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
v2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.gov2/pkg/engine/datasource/grpc_datasource/compiler_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.gov2/pkg/engine/datasource/grpc_datasource/required_fields_visitor_test.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/grpctest/mockservice_requires.gov2/pkg/grpctest/product.protov2/pkg/grpctest/testdata/products.graphqls
💤 Files with no reviewable changes (1)
- v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
1131-1147: Consider logging when oneof resolution falls back to empty message.When
FragmentFields[typeName]is not found (line 1139) orfindOneOfFieldDescriptorreturns nil (line 1145), the function returns an empty message without any indication. This could make debugging schema misconfigurations difficult in production.💡 Optional: Add debug logging for silent fallbacks
If debugging visibility is desired, consider adding optional logging when these fallbacks occur:
fragmentFields, ok := rpcMessage.FragmentFields[typeName] if !ok { + // Consider: log.Debug("no fragment fields found for typename", "typename", typeName) return message, nil } // Find the matching oneof field descriptor for the expected concrete type. oneofFieldDesc := p.findOneOfFieldDescriptor(inputMessage.Desc.Oneofs(), protoref.Name(rpcMessage.OneOfType.FieldName()), protoref.Name(typeName)) if oneofFieldDesc == nil { + // Consider: log.Debug("no oneof field descriptor found", "typename", typeName) return message, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/compiler.go` around lines 1131 - 1147, In buildOneOfMessage on RPCCompiler, add debug/error logging when the function currently silently returns an empty message: log when rpcMessage.FragmentFields[typeName] is missing (include typeName and rpcMessage identity) and when findOneOfFieldDescriptor(...) returns nil (include typeName and rpcMessage.OneOfType.FieldName()); ensure logs give enough context to trace schema/resolution issues (use existing logger/ctx in this package) before returning the empty message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/compiler.go`:
- Around line 1131-1147: In buildOneOfMessage on RPCCompiler, add debug/error
logging when the function currently silently returns an empty message: log when
rpcMessage.FragmentFields[typeName] is missing (include typeName and rpcMessage
identity) and when findOneOfFieldDescriptor(...) returns nil (include typeName
and rpcMessage.OneOfType.FieldName()); ensure logs give enough context to trace
schema/resolution issues (use existing logger/ctx in this package) before
returning the empty message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8616a130-9400-4c10-b055-2e22dd2d868a
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go
devsergiy
approved these changes
Mar 25, 2026
Noroth
deleted the
ludwig/eng-9219-add-support-for-abstract-types-in-go-tools
branch
Noroth
pushed a commit
that referenced
this pull request
Mar 26, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.266](v2.0.0-rc.265...v2.0.0-rc.266) (2026-03-26) ### Features * add support for composite types in go-tools ([#1448](#1448)) ([646ea97](646ea97)) ### Bug Fixes * preserve nullable nested input presence in gRPC compiler ([#1447](#1447)) ([900d450](900d450)) --- 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 go-tools. * **Bug Fixes** * Fixed handling of nullable nested input presence in gRPC compiler. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for composite types in the selection set of a field with a
@requiresdirective.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist