Conversation
r.inlineFragmentRef goes stale precisely when EnterSelectionSet descends into a nested message. Prevent this by stacking inline fragment access.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames RPCMessage.FieldSelectionSet → FragmentFields, adds rpcPlanningContext traversal helpers (enterNestedField, lastResponseField, leaveNestedField), updates visitors and JSON builder to use FragmentFields for inline/oneOf fragment fields, and adds Owner.pet (Animal) plus corresponding mapping and tests. Changes
Sequence DiagramsequenceDiagram
participant Visitor as rpcPlanVisitor
participant PlanCtx as rpcPlanningContext
participant Message as RPCMessage
participant Ancestors as AncestorStack
Ancestors->>Visitor: push AST nodes (field/selection/inlineFragment)
Visitor->>PlanCtx: enterNestedField(info, enclosingTypeNode, selectionSetRef, inlineFragmentRef)
Note over PlanCtx,Message: PlanCtx creates/accesses nested RPCMessage and tracks ancestors
PlanCtx->>Message: return current nested RPCMessage
Visitor->>Message: EnterField(name)
Message->>Message: add field to Fields and/or FragmentFields[typeName]
Visitor->>PlanCtx: lastResponseField(message, inlineFragmentRef)
Visitor->>PlanCtx: leaveNestedField(info)
Note over PlanCtx,Ancestors: PlanCtx pops/restores previous response-message context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go (1)
2141-2170: Prefer fresh mapping/config instances per test case for parallel isolation.Line 2173 and Line 2683 run tests in parallel while sharing package-level vars (
nestedInlineFragmentFederationMapping/nestedInlineFragmentFederationConfigs). Creating fresh instances per case will avoid hidden coupling and future mutation hazards.♻️ Suggested refactor
-var nestedInlineFragmentFederationMapping = &GRPCMapping{ - ... -} +func newNestedInlineFragmentFederationMapping() *GRPCMapping { + return &GRPCMapping{ + ... + } +} -var nestedInlineFragmentFederationConfigs = plan.FederationFieldConfigurations{ - { - TypeName: "User", - SelectionSet: "id", - }, -} +func newNestedInlineFragmentFederationConfigs() plan.FederationFieldConfigurations { + return plan.FederationFieldConfigurations{ + { + TypeName: "User", + SelectionSet: "id", + }, + } +} - mapping: nestedInlineFragmentFederationMapping, - federationConfigs: nestedInlineFragmentFederationConfigs, + mapping: newNestedInlineFragmentFederationMapping(), + federationConfigs: newNestedInlineFragmentFederationConfigs(),🤖 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_federation_test.go` around lines 2141 - 2170, The package-level shared variables nestedInlineFragmentFederationMapping and nestedInlineFragmentFederationConfigs cause data races when tests run in parallel; change tests to create fresh instances per test by replacing references to these globals with factory calls or local constructions (e.g., NewNestedInlineFragmentFederationMapping() / NewNestedInlineFragmentFederationConfigs()) inside each test case or before t.Run so each test gets its own GRPCMapping and plan.FederationFieldConfigurations instance; ensure any slices/maps are newly allocated (not shared) to prevent mutation hazards during parallel execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`:
- Around line 142-143: When entering an inline fragment handler you overwrite
r.planInfo.inlineFragmentRef without preserving the outer context; change the
enter/leave logic to save the previous inlineFragmentRef into a temporary
variable (e.g., prev := r.planInfo.inlineFragmentRef) before setting
r.planInfo.inlineFragmentRef = ref, and on leave restore it
(r.planInfo.inlineFragmentRef = prev) so nested inline fragments do not clobber
their parent scope; apply the same save/restore pattern to the other
inline-fragment enter/leave pair referenced (the second occurrence that
currently sets and then clears inlineFragmentRef).
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go`:
- Around line 99-105: EnterInlineFragment currently overwrites
planInfo.inlineFragmentRef without saving the outer context; LeaveInlineFragment
then unconditionally clears it which drops outer fragment context for nested
fragments. Modify EnterInlineFragment (method on rpcPlanVisitor) to save the
current planInfo.inlineFragmentRef (e.g., push onto a small stack or temporary
field) before setting the new ref, and modify LeaveInlineFragment to restore the
previously saved ref instead of setting planInfo.inlineFragmentRef to
ast.InvalidRef; ensure the saved context is cleared after restore so nested
fragment state is preserved correctly.
---
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go`:
- Around line 2141-2170: The package-level shared variables
nestedInlineFragmentFederationMapping and nestedInlineFragmentFederationConfigs
cause data races when tests run in parallel; change tests to create fresh
instances per test by replacing references to these globals with factory calls
or local constructions (e.g., NewNestedInlineFragmentFederationMapping() /
NewNestedInlineFragmentFederationConfigs()) inside each test case or before
t.Run so each test gets its own GRPCMapping and
plan.FederationFieldConfigurations instance; ensure any slices/maps are newly
allocated (not shared) to prevent mutation hazards during parallel execution.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_composite_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/grpctest/product.protov2/pkg/grpctest/testdata/products.graphqls
dkorittki
requested review from
Noroth,
StarpTech,
devsergiy,
jensneuse and
ysmolski
as code owners
Noroth
reviewed
Mar 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan.go`:
- Around line 496-501: The bug is that leaveNestedField pops the ancestors
unconditionally which corrupts currentResponseMessage when EnterSelectionSet
handlers (resolver selections) returned early and didn't push; fix by making
enterNestedField always push the prior currentResponseMessage onto
planningInfo.responseMessageAncestors before changing currentResponseMessage,
and keep leaveNestedField to only pop/restore when responseMessageAncestors is
non-empty (i.e., restore the exact prior value that enterNestedField pushed).
Update the enterNestedField and leaveNestedField implementations (referencing
enterNestedField, leaveNestedField, planningInfo.responseMessageAncestors, and
planningInfo.currentResponseMessage) so pushes and pops are symmetric.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go`:
- Around line 2735-2752: The mapping block for Fields (the FieldMap literal
including "Product" and "ProductSpecs") is misformatted per gci; reformat the
block so it passes the linter — ensure consistent indentation, spacing, and
alignment for entries like "ProductSpecs", its nested fields ("id", "weight",
"dimensions") and the "relatedProduct" entry (with TargetName "related_product"
and ArgumentMappings using FieldArgumentMap{"category":"category"} style); run
the project formatter and gci (or gofmt/goimports/gci) on
execution_plan_federation_test.go to apply the automatic cleanup so the
FieldMap/FieldArgumentMap literal conforms to the repo style.
In `@v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go`:
- Around line 381-405: The map literal around "Subcategory" and the following
"TestContainer" block has indentation/formatting drift causing gci to fail;
reformat mapping_test_helper.go (the map entries using FieldMapData and
FieldArgumentMap, and keys "Subcategory" and "TestContainer") with your
formatter/import sorter (e.g., gofmt/goimports and gci -w) so the map
indentation and import grouping are corrected and the file passes CI.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.gov2/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.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/grpctest/testdata/products.graphqls
…rpc-nested-list-fields-in-response
dkorittki
deleted the
dominik/eng-9007-router-does-not-resolve-grpc-nested-list-fields-in-response
branch
dkorittki
pushed a commit
that referenced
this pull request
Mar 5, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.258](v2.0.0-rc.257...v2.0.0-rc.258) (2026-03-05) ### Bug Fixes * handle complex types inside inline fragments in gRPC execution plan ([#1411](#1411)) ([3b4dbcb](3b4dbcb)) --- 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 * **Bug Fixes** * Fixed an issue with handling complex types in inline fragments. <!-- 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.
Summary by CodeRabbit
Schema Updates
Refactor
Tests
Checklist
When a GraphQL operation contains complex types inside an inline fragment for gRPC subgraphs the fields of that complex type never get populated with the data of the subgraphs response and hence are becoming null, even if there actually was is data. This is because the gRPC execution visitors don't create the expected response for complex types inside inline fragments. This is because the visitor doesn't account for inline fragments when traversing through selection sets and tries to place the response message at
currentResponse.Fieldsrather thancurrentResponse.FieldSelectionSet. This is fixed by ensuring the latter fields are populated in this case.