fix: incorrect field resolver path #1363
Conversation
📝 WalkthroughWalkthroughAssign stable per-call IDs to RPC calls, switch resolver path resolution to mapping-rooted names (result.*), refine nested-list/context traversal and JSON merging, and add child/optional category resolver RPCs, mappings, mocks, schema fields, and extensive resolver tests. Changes
Sequence Diagram(s)mermaid Client->>Planner: GraphQL request with nested field resolvers Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Noroth
requested review from
StarpTech,
devsergiy,
jensneuse and
ysmolski
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan.go`:
- Around line 909-918: The loop over contextFields currently returns an error
when r.mapping.FindFieldMapping(...) yields !ok; change this to fall back to the
same identity-mapping logic used elsewhere (i.e., call resolveFieldMapping or
replicate its behavior) so that when no explicit mapping exists the
proto/GraphQL field name is used instead of hard-failing; update the branch in
the contextFields loop that assigns fieldName to use
resolveFieldMapping(walker.EnclosingTypeDefinition.NameString(r.definition),
r.definition.FieldDefinitionNameString(contextFieldRef)) (or its equivalent) and
then continue to build resolvedPath with
fieldPath.WithFieldNameItem(unsafebytes.StringToBytes(fieldName)).
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go`:
- Around line 4585-4629: The recursive traversal in the test's validate function
(variables: categories, currentLevel, nextChildren, depth) can hang if
childCategories cycles; add a guard such as a maxDepth constant (e.g., maxDepth
:= 10) and/or a visited set keyed by category id to detect repeats, and
break/fail the test if maxDepth is exceeded or an id is seen twice; update the
loop to check the guard before iterating and assert a clear failure message
(e.g., "possible cycle or excessive depth") so the test fails fast instead of
stalling CI.
🧹 Nitpick comments (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
432-449: Correctly fixes nested resolver path resolution.The logic properly overrides the first path segment to "result" for nested field resolvers, ensuring the resolve path matches the protobuf message structure. Note that
fieldPath := r.fieldPathcreates a shallow copy sharing the underlying array, so the modification tofieldPath[0]also affectsr.fieldPath—this appears intentional for maintaining consistent root paths across nested resolvers.Consider enhancing the error message to include both type and field names for easier debugging:
💡 Suggested improvement
fieldName, ok := r.mapping.FindFieldMapping(r.walker.EnclosingTypeDefinition.NameString(r.definition), r.definition.FieldDefinitionNameString(fieldDefRef)) if !ok { - r.walker.StopWithInternalErr(fmt.Errorf("field mapping not found for field %s", r.definition.FieldDefinitionNameString(fieldDefRef))) + r.walker.StopWithInternalErr(fmt.Errorf("field mapping not found for type: %s, field: %s", r.walker.EnclosingTypeDefinition.NameString(r.definition), r.definition.FieldDefinitionNameString(fieldDefRef))) return }v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
696-704: Correctly handles nested list data retrieval.The changes properly fix nested list traversal:
- Using
path[1:]when recursing ensures the path advances correctly through nested structures- The else branch correctly appends non-list scalar values that were previously being dropped
Minor suggestion: the inner
valuesvariable shadows the outer loop variable, which can be confusing. Consider renaming for clarity:♻️ Optional refactor
for _, val := range values { if list, isList := val.Interface().(protoref.List); isList { - values := p.resolveListDataForPath(list, fd, path[1:]) - result = append(result, values...) + nestedValues := p.resolveListDataForPath(list, fd, path[1:]) + result = append(result, nestedValues...) continue } else { result = append(result, val) } }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
4569-4584: Strengthen nested-resolver assertions (normalizedScore).Line 4571: this test only checks the categories list, so it can pass even if
normalizedScorefails to resolve. Add assertions oncategoryMetricsandnormalizedScoreto exercise the nested resolver path directly.💡 Suggested assertion upgrade
validate: func(t *testing.T, data map[string]interface{}) { require.NotEmpty(t, data) categories, ok := data["categories"].([]interface{}) require.True(t, ok, "categories should be an array") require.NotEmpty(t, categories, "categories should not be empty") require.Len(t, categories, 4, "Should return 4 categories") + + for _, category := range categories { + category, ok := category.(map[string]interface{}) + require.True(t, ok, "category should be an object") + metrics, ok := category["categoryMetrics"].(map[string]interface{}) + require.True(t, ok, "categoryMetrics should be an object") + require.NotEmpty(t, metrics["id"]) + require.NotEmpty(t, metrics["metricType"]) + require.IsType(t, float64(0), metrics["normalizedScore"]) + } },
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go`:
- Around line 1422-1430: The recursive expected plan for the field named
"child_categories" (ProtoTypeName DataTypeMessage, JSONPath "childCategories"
within the RPCMessage "Category" and its RPCField entries) must mark both
response occurrences as repeated; update the recursive plan entries that define
"child_categories" to include Repeated: true (matching the non-recursive case)
so both response occurrences use Repeated: true to avoid the mismatch; apply the
same change to the corresponding recursive entries around the other occurrence
range noted (lines referenced in the review: the second block around 1505-1529).
♻️ Duplicate comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
909-918: Fallback to identity mapping for context fields.This code returns an error when
FindFieldMappingdoesn't find a mapping entry. However,resolveFieldMapping(lines 479-485) exists and provides an identity fallback when no explicit mapping is defined. Using the hard-fail approach breaks backward compatibility for cases where proto field names match GraphQL field names.🔧 Suggested fix
- fieldName, ok := r.mapping.FindFieldMapping( + fieldName := r.resolveFieldMapping( walker.EnclosingTypeDefinition.NameString(r.definition), r.definition.FieldDefinitionNameString(contextFieldRef), ) - if !ok { - return fmt.Errorf("field mapping not found for field %s", r.definition.FieldDefinitionNameString(contextFieldRef)) - } - resolvedPath := fieldPath.WithFieldNameItem(unsafebytes.StringToBytes(fieldName))v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
4585-4629: Guard recursive traversal to avoid hanging tests.
The loop only exits whennextChildrenis empty, so cyclic data can hang the test. Add a max-depth or visited-set guard to fail fast.🛡️ Suggested guard
currentLevel := categories depth := 0 + const maxDepth = 5 for { var nextChildren []interface{} @@ if len(nextChildren) == 0 { break } depth++ + if depth > maxDepth { + t.Fatalf("possible childCategories cycle; exceeded maxDepth=%d", maxDepth) + } currentLevel = nextChildren }
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
1312-1336: Add defensive guard for empty TargetName in resolve field mapping.Line 1322 directly assigns
resolveConfig.FieldMappingData.TargetNamewithout validation. WhileFindResolveTypeFieldMappingchecks for nil, it does not validate thatTargetNameis non-empty—unlikeFindFieldMappingwhich explicitly rejects empty values. This could produce an empty proto field name if a resolve configuration omits TargetName. Consider guarding the assignment:- field.Name = resolveConfig.FieldMappingData.TargetName + if resolveConfig.FieldMappingData.TargetName != "" { + field.Name = resolveConfig.FieldMappingData.TargetName + }Alternatively, enforce validation in
FindResolveTypeFieldMappingto match the behavior ofFindFieldMappingand reject empty TargetName values at the source.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go`:
- Around line 4569-4584: The validate function for the test case "Query with
nested field resolver" currently only asserts categories exist; extend it to
iterate each category's "categoryMetrics" array and assert it's present and
non-empty, then for each metric assert presence and types/values for fields
"id", "normalizedScore" (ensure it returns a numeric value when given baseline),
"metricType" (equals the requested "popularity_score") and "value"; update the
validate closure referenced in the diff to check these nested fields and to fail
with clear messages if any metric or field is missing or has the wrong type.
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
4585-4629: Cycle guard implemented - addresses past review concern.The recursive traversal now has proper protection with
maxDepth := 3and the loop conditiondepth < maxDepth. This prevents infinite loops if the resolver returns cycles or excessive depth.
There was a problem hiding this comment.
Actionable comments posted: 4
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/fetch.go (1)
41-48: Fix pointer-to-range-variable when storing RPCCall.Using
&callin a range loop takes the address of the loop variable, so everyFetchItem.Planends up pointing at the last call. Use an index loop and take the address of the slice element.🐛 Proposed fix
- for _, call := range executionPlan.Calls { - graph.nodes[call.ID] = call.DependentCalls - graph.fetches[call.ID] = FetchItem{ - ID: call.ID, - Plan: &call, + for i := range executionPlan.Calls { + call := &executionPlan.Calls[i] + graph.nodes[call.ID] = call.DependentCalls + graph.fetches[call.ID] = FetchItem{ + ID: call.ID, + Plan: call, ServiceCall: nil, - DependentFetches: call.DependentCalls, + DependentFetches: call.DependentCalls, } }
🤖 Fix all issues with AI agents
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go`:
- Around line 2476-2478: The inline comment "This test case fails - execution
plan generation is incorrect for sibling field resolvers with nested resolvers"
above the test t.Run("Should create an execution plan for a query with both
childCategories and optionalCategories with nested field resolvers", ...) is
misleading; either remove it if the test now passes, or replace it with a clear
TODO that references an issue ID/URL, or mark the test to skip with t.Skip and a
brief rationale; update the comment to reflect the chosen action so future
readers know whether the failure is known and tracked or resolved.
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`:
- Around line 143-151: The code sets parentCallID from len(r.plan.Calls) which
references plan index instead of the RPCCall ID, causing mis-wired dependencies;
change the logic so parentCallID is set to the current call's ID (use
r.currentCall.ID or r.callIndex before increment) for top-level parent and, for
nested resolvers, preserve/use the ancestor resolver's RPCCall ID (the
ancestor's r.currentCall.ID or a stored ancestorResolverID) so dependentCalls
reference call IDs rather than plan indexes; apply the same change to the other
occurrence that mirrors this logic (the block at the other location noted in the
review).
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go`:
- Around line 450-461: The code uses parentID := r.operatonIndex for top-level
resolver calls which can diverge from the actual RPCCall.ID when r.callIndex has
advanced; change the logic so that when r.fieldResolverAncestors.len() == 0 you
set parentID to the current RPC call ID (use r.currentCall.ID or the struct
field that holds the active call's ID) instead of r.operatonIndex, so
resolverField.callerRef is the correct dependency; keep the existing branch that
reads from r.resolverFields[r.fieldResolverAncestors.peek()] when there are
resolver ancestors.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go`:
- Around line 4850-4901: Two test cases share the identical "name" value which
makes test output ambiguous; open the test cases in grpc_datasource_test.go that
set the name string "Query with both childCategories and optionalCategories with
nested field resolvers" and rename one to a distinct, descriptive string (for
example append " - nested optionalCategories in childCategories" or " - nested
childCategories in optionalCategories") so each subtest name is unique; update
only the name field in the corresponding test case struct(s) and keep the rest
of the validate/validateError blocks unchanged.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`:
- Around line 403-412: The federation visitor is incorrectly inheriting the
grandparent callerRef for nested resolvers; in the block that computes fieldPath
and callerRef (using r.fieldResolverAncestors and r.resolverFields) set
callerRef to the parent resolver's id
(r.resolverFields[r.fieldResolverAncestors.peek()].id) instead of its callerRef
so the new resolverField (constructed with id: r.callIndex, callerRef:
callerRef) mirrors the non-federation wiring; update the assignment near
resolverField creation to use the parent id to ensure correct dependency
chaining.
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go`:
- Around line 476-481: The code is storing zero-copy byte slices produced by
unsafebytes.StringToBytes from a local variable fieldName into
resolvedField.contextPath and r.fieldPath, which risks use-after-free; change
both assignments that call unsafebytes.StringToBytes (the ones assigning
resolvedField.contextPath and r.fieldPath) to make a safe copy of the bytes
(e.g., use append([]byte(nil), fieldName...) or bytes.Clone) so the stored
slices own their memory; apply the same replacement at the identical locations
in execution_plan_visitor_federation.go as well.
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
50-54: Typo in field name:operatonIndexshould beoperationIndex.The field name
operatonIndexis missing the letter 'i'. While this doesn't affect functionality, it impacts code readability and maintainability.🔧 Suggested fix
- operatonIndex int // The index of the current root operation in the plan. + operationIndex int // The index of the current root operation in the plan.Note: This rename would need to be applied consistently throughout the file (lines 308, 446).
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
54-55: Naming inconsistency with non-federation visitor.This file correctly uses
operationIndex, butexecution_plan_visitor.gousesoperatonIndex(missing 'i'). Consider aligning the naming across both files for consistency.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go`:
- Around line 223-238: The current early-return skips nested selection sets when
the ancestor is an inline fragment (FieldSelectionSet); update the guard in the
visitor (around r.planInfo.currentResponseMessage.Fields and
r.walker.Ancestor().Kind) to allow inline-fragment ancestors in addition to
NodeKindField (e.g., treat ast.NodeKindInlineFragment or the FieldSelectionSet
kind as a valid ancestor), and keep the subsequent logic that creates/assigns
the nested message on
r.planInfo.currentResponseMessage.Fields[lastIndex].Message (ensure you
reference r.planInfo.currentResponseMessage.Fields, r.walker.Ancestor().Kind,
r.planCtx.newMessageFromSelectionSet and the lastIndex computation so the nested
message is created when a field was added via an inline fragment).
Noroth
deleted the
ludwig/eng-8789-recursive-call-with-field-resolvers-causes-issues
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.244](v2.0.0-rc.243...v2.0.0-rc.244) (2026-01-26) ### Bug Fixes * incorrect field resolver path ([#1363](#1363)) ([46544dc](46544dc)) --- 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 incorrect field resolver path <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
fixes #1361
Field resolvers and nested field resolver are reading the context data from an incorrect path. As we retrieve the data from protobuf messages, we need to reference the path in the generated proto.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist