feat: add initial support for @requires in gRPC#1362
Merged
Conversation
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:
📝 WalkthroughWalkthroughAdds required-field ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as GraphQL Client
participant Planner as Planner
participant Compiler as RPCCompiler
participant DataSource as GRPC DataSource
participant Service as MockService (gRPC)
Client->>Planner: submit query with `@requires` fields
Planner->>Compiler: buildRequiredFieldsMessage / request message defs
Compiler-->>Planner: message definitions
Planner->>DataSource: plan and execute CallKindRequired RPCs
DataSource->>Service: gRPC Require/Resolve RPC requests
Service-->>DataSource: RPC responses
DataSource->>Planner: mergeWithPath required-field results
Planner-->>Client: assembled GraphQL response
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 |
… ludwig/eng-8642-add-support-for-plain-fields
Noroth
requested review from
a team,
StarpTech,
devsergiy,
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/pkg/astvisitor/visitor.go (1)
4031-4045:⚠️ Potential issue | 🟡 MinorUpdate ResolveInlineFragment docs to match the new sentinel return.
The comment still describes a
(ref, bool)return, but the method now returns only an int and usesast.InvalidRefas the sentinel.✏️ Suggested doc update
-// It returns the inline fragment ref and true if the current field is inside of an inline fragment. -// It returns -1 and false if the current field is not inside of an inline fragment. +// It returns the inline fragment ref if the current field is inside of an inline fragment. +// It returns ast.InvalidRef if the current field is not inside of an inline fragment.v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
1374-1437:⚠️ Potential issue | 🟠 MajorUse TrimPrefix instead of TrimLeft when stripping the package prefix.
TrimLefttreats the second argument as a set of characters to trim from the left side of the string, not as a prefix. This can incorrectly drop leading characters from message names if they appear in the package name, causing invalid message names and breaking lookups. UseTrimPrefixto remove the exact package prefix string.🔧 Proposed fix
-func (p *RPCCompiler) fullMessageName(m protoref.MessageDescriptor) string { - return strings.TrimLeft(string(m.FullName()), p.doc.Package+".") -} +func (p *RPCCompiler) fullMessageName(m protoref.MessageDescriptor) string { + return strings.TrimPrefix(string(m.FullName()), p.doc.Package+".") +}
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan.go`:
- Around line 409-415: In parseGraphQLType, when r.definition.NodeByName(dt)
returns found == false and currently returns DataTypeUnknown, add a fallback
call to fromGraphQLType(dt) to resolve built-in scalars (String, Int, Float,
Boolean, ID) before giving up; specifically, in the parseGraphQLType function
replace the immediate return DataTypeUnknown with logic that calls
fromGraphQLType(dt) and returns its result if it yields a known type, otherwise
return DataTypeUnknown—this keeps NodeByName as the primary lookup but uses
fromGraphQLType as a fallback for schema-missing scalars.
In `@v2/pkg/grpctest/mockservice_resolve.go`:
- Around line 890-918: The allocation and loops in ResolveStorageLinkedStorages
use depth (int32) directly which causes a compile error and allows negatives;
change handling to read depth32 := req.GetFieldArgs().GetDepth() (or use
existing depth variable), clamp it to >=0, convert to int (e.g. intDepth :=
int(depth32)), use intDepth as the capacity in make([]*productv1.Storage, 0,
intDepth) and iterate with for j := 0; j < intDepth; j++ (or use int32 loop with
conversion), ensuring all uses reference the converted/clamped int variable.
- Around line 920-966: The make() call for storages uses an int32 capacity and
preallocates up to radius which both prevents compilation and defeats the "cap
at 5"; in ResolveStorageNearbyStorages convert the radius when creating slices
and cap it to 5 before preallocating/looping: compute cap := int(radius); if cap
> 5 { cap = 5 } then use make([]*productv1.Storage, 0, cap) and loop j from 0 to
cap (using int indices), keeping radius as int32 for proto reads but always cast
to int for slice/loop operations in the ResolveStorageNearbyStorages function.
🧹 Nitpick comments (3)
v2/pkg/engine/datasource/grpc_datasource/compiler_test.go (1)
469-507: Avoid order-dependent assertions for nested message lists.
If compiler message ordering changes, this test may become flaky. Prefer set-based assertions.Suggested refactor
- require.Equal(t, 5, len(compiler.doc.Messages)) - require.Equal(t, "MyMessage", compiler.doc.Messages[0].Name) - require.Equal(t, "MyMessage.NestedMessage", compiler.doc.Messages[1].Name) - require.Equal(t, "SecondMessage", compiler.doc.Messages[2].Name) - require.Equal(t, "SecondMessage.MyMessage", compiler.doc.Messages[3].Name) - require.Equal(t, "SecondMessage.MyMessage.NestedMessage", compiler.doc.Messages[4].Name) + require.Equal(t, 5, len(compiler.doc.Messages)) + names := make([]string, 0, len(compiler.doc.Messages)) + for _, msg := range compiler.doc.Messages { + names = append(names, msg.Name) + } + require.ElementsMatch(t, []string{ + "MyMessage", + "MyMessage.NestedMessage", + "SecondMessage", + "SecondMessage.MyMessage", + "SecondMessage.MyMessage.NestedMessage", + }, names)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (1)
17-208: Optional: factor shared graphqlError/graphqlResponse helpers.
These structs are duplicated across multiple test functions; a small helper could reduce repetition.v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
395-445: TODO: nested field resolvers in required fields remain unsupported.
If this scenario is expected, consider a follow-up task to either implement support or guard against it explicitly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@v2/pkg/engine/datasource/grpc_datasource/compiler.go`:
- Around line 1397-1401: The current RPCCompiler.fullMessageName uses
strings.TrimLeft which treats p.doc.Package+"." as a cutset and can corrupt
message names; update the implementation of RPCCompiler.fullMessageName to use
strings.TrimPrefix on string(m.FullName()) with the prefix p.doc.Package+"." so
only the exact package prefix is removed (refer to RPCCompiler.fullMessageName,
m.FullName(), and p.doc.Package).
- Around line 688-763: The function buildRequiredFieldsMessage currently
proceeds when data.Get("representations") is missing or empty, producing an
empty context list; change it to fail fast by checking representations
immediately after retrieval (the representations :=
data.Get("representations").Array() line) and returning a clear error if the key
is absent or the slice length is zero (e.g., fmt.Errorf("representations missing
or empty for message %s", inputMessage.Name) or referencing rpcMessage.Name),
mirroring the behavior of buildProtoMessageWithContext; implement the check
before the loop so no empty RPC message is returned.
… ludwig/eng-8642-add-support-for-plain-fields
… ludwig/eng-8642-add-support-for-plain-fields
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (2)
146-148: Variable shadowing: loop variableentityis reused for type assertion.The loop variable
entityis shadowed by the type-asserted variable on the next line. While this works, it can cause confusion. Consider using a distinct name likeentityMapfor the type-asserted value.♻️ Suggested improvement
for index, entity := range entities { - entity, ok := entity.(map[string]interface{}) - require.True(t, ok, "entity should be an object") + entityMap, ok := entity.(map[string]interface{}) + require.True(t, ok, "entity should be an object") productID := index + 1 - require.Equal(t, fmt.Sprintf("%d", productID), entity["id"]) + require.Equal(t, fmt.Sprintf("%d", productID), entityMap["id"])This pattern repeats throughout the file (lines 255-256, 329-330, 387-388, etc.).
🤖 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 146 - 148, The test loop is shadowing the loop variable entity by reusing the name in the type assertion; update the type-assertion to use a distinct name (e.g., entityMap) wherever you see patterns like "for index, entity := range entities { entity, ok := entity.(map[string]interface{})" (appears in grpc_datasource_federation_test.go and repeats at similar spots such as the blocks around the earlier referenced lines); change the asserted variable name and its uses within the loop body (references to the map) so you no longer shadow the loop variable.
20-26: Consider extracting common test types to package level.The
graphqlErrorandgraphqlResponsestructs are defined identically in four test functions (lines 20-26, 212-218, 512-518, 1211-1217). Additionally, the test execution loop pattern (lines 164-205, 464-505, 1163-1204, 1578-1619) is nearly identical across all functions.Consider extracting these to reduce duplication:
♻️ Suggested refactor
// Package-level test types type graphqlError struct { Message string `json:"message"` } type graphqlResponse struct { Data map[string]interface{} `json:"data"` errors []graphqlError `json:"errors,omitempty"` } // Helper function for running federation test cases func runFederationTestCases(t *testing.T, conn grpc.ClientConnInterface, testCases []federationTestCase) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { schemaDoc := grpctest.MustGraphQLSchema(t) queryDoc, report := astparser.ParseGraphqlDocumentString(tc.query) if report.HasErrors() { t.Fatalf("failed to parse query: %s", report.Error()) } // ... rest of common setup and execution }) } }🤖 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 20 - 26, Extract the duplicated test types and execution loop into shared package-level helpers: move the repeating structs graphqlError and graphqlResponse to top-level in this test file, and implement a helper function (e.g. runFederationTestCases) that accepts testing.T, a grpc.ClientConnInterface, and a slice of federationTestCase to run each tc with the common setup/teardown and assertions; replace the nearly identical test loops in the four test functions with calls to runFederationTestCases, referencing the same federationTestCase type and keeping per-test specifics only in the individual test case slices.
🤖 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 1308-1366: The response construction is using the old
requiredField.resultField (built earlier without TargetName) instead of the
newly built field whose Name was set to rpcConfig.TargetName; update the
RPCCall.Response -> Message.Fields to use the local variable field (the RPC
field built by r.buildField and assigned field.Name = rpcConfig.TargetName)
instead of requiredField.resultField so the protobuf lookup uses the mapped
TargetName; locate the block building RPCCall.Response (the RPCMessage with Name
rpcConfig.RPC + "Result") and replace requiredField.resultField with the newly
constructed field variable.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go`:
- Around line 541-546: The JSON string assigned to vars in
grpc_datasource_federation_test.go contains a trailing comma after the last
object in the "representations" array which makes it invalid; locate the vars
string in the test (the `vars` literal in grpc_datasource_federation_test.go)
and remove the trailing comma after the final representation object so the JSON
is valid (then run the test to confirm json.Unmarshal no longer fails).
- Around line 118-132: The JSON assigned to the test variable "vars" in
grpc_datasource_federation_test.go is missing the final closing brace for the
outer object; fix by appending the missing "}" to the end of the JSON string so
the object that starts at the top-level (the one containing "variables") is
properly closed—locate the "vars" string literal in the test and add the missing
closing brace to make the JSON well-formed.
---
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go`:
- Around line 146-148: The test loop is shadowing the loop variable entity by
reusing the name in the type assertion; update the type-assertion to use a
distinct name (e.g., entityMap) wherever you see patterns like "for index,
entity := range entities { entity, ok := entity.(map[string]interface{})"
(appears in grpc_datasource_federation_test.go and repeats at similar spots such
as the blocks around the earlier referenced lines); change the asserted variable
name and its uses within the loop body (references to the map) so you no longer
shadow the loop variable.
- Around line 20-26: Extract the duplicated test types and execution loop into
shared package-level helpers: move the repeating structs graphqlError and
graphqlResponse to top-level in this test file, and implement a helper function
(e.g. runFederationTestCases) that accepts testing.T, a
grpc.ClientConnInterface, and a slice of federationTestCase to run each tc with
the common setup/teardown and assertions; replace the nearly identical test
loops in the four test functions with calls to runFederationTestCases,
referencing the same federationTestCase type and keeping per-test specifics only
in the individual test case slices.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/pkg/astvisitor/visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/grpctest/mockservice_resolve.go
💤 Files with no reviewable changes (1)
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go
- v2/pkg/astvisitor/visitor.go
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
1348-1359:⚠️ Potential issue | 🟠 MajorEnsure required-field response uses TargetName-mapped field.
The response currently uses
requiredField.resultFielddirectly; ifrpcConfig.TargetNamediffers, protobuf field lookup can fail. Consider mapping the field name before embedding it in the response.🛠️ Suggested fix
- call := RPCCall{ + resultField := requiredField.resultField + if rpcConfig.TargetName != "" { + resultField.Name = rpcConfig.TargetName + } + call := RPCCall{ ... Response: RPCMessage{ Name: rpcConfig.Response, Fields: RPCFields{ { Name: resultFieldName, ProtoTypeName: DataTypeMessage, Repeated: true, JSONPath: resultFieldName, Message: &RPCMessage{ Name: rpcConfig.RPC + "Result", - Fields: RPCFields{requiredField.resultField}, + Fields: RPCFields{resultField}, }, }, }, }, }🤖 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.go` around lines 1348 - 1359, The response message construction is embedding requiredField.resultField directly which can fail when rpcConfig.TargetName remaps the field; modify the code that builds the RPCMessage (around RPCMessage{Name: rpcConfig.RPC + "Result", Fields: RPCFields{requiredField.resultField}}) to lookup or clone the required field using rpcConfig.TargetName (or map resultFieldName -> rpcConfig.TargetName) so the Field.Name / JSONPath matches the target mapping before inserting into the response Fields; ensure the inserted field uses the mapped name and any related Proto/JSON properties are adjusted accordingly.
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (2)
21-27: DRY up duplicated GraphQL response/error structs.
graphqlErrorandgraphqlResponseare repeated in every test function in this file. Consider lifting them to package-level (or a small test helper) to avoid duplication and keep changes centralized.🤖 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 21 - 27, graphqlError and graphqlResponse types are duplicated across test functions; define them once at package scope (or in a small test helper) and remove the per-test definitions so all tests reuse the single definitions (e.g., add package-level types named graphqlError and graphqlResponse and update tests that currently redeclare these types to reference the package-level versions).
162-165: Apply JSON validity checks consistently across the other test loops.Only this test suite validates
tc.varswithgjson.Valid, which already caught malformed JSON once. Consider applying the same check in the other loops (or extract a tiny helper) so invalidvarsare caught uniformly.🤖 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 162 - 165, The other test loops iterate over testCases but only the federation loop validates JSON with gjson.Valid(tc.vars); add the same gjson.Valid check in each t.Run loop (or extract a small helper like validateVars(t, tc.vars) that calls require.True(t, gjson.Valid(vars)) and use it in every test loop) so malformed tc.vars is consistently caught; update all t.Run closures that reference tc.vars (the testCases iterations) to call the helper or gjson.Valid before further parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan.go`:
- Around line 1348-1359: The response message construction is embedding
requiredField.resultField directly which can fail when rpcConfig.TargetName
remaps the field; modify the code that builds the RPCMessage (around
RPCMessage{Name: rpcConfig.RPC + "Result", Fields:
RPCFields{requiredField.resultField}}) to lookup or clone the required field
using rpcConfig.TargetName (or map resultFieldName -> rpcConfig.TargetName) so
the Field.Name / JSONPath matches the target mapping before inserting into the
response Fields; ensure the inserted field uses the mapped name and any related
Proto/JSON properties are adjusted accordingly.
---
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go`:
- Around line 21-27: graphqlError and graphqlResponse types are duplicated
across test functions; define them once at package scope (or in a small test
helper) and remove the per-test definitions so all tests reuse the single
definitions (e.g., add package-level types named graphqlError and
graphqlResponse and update tests that currently redeclare these types to
reference the package-level versions).
- Around line 162-165: The other test loops iterate over testCases but only the
federation loop validates JSON with gjson.Valid(tc.vars); add the same
gjson.Valid check in each t.Run loop (or extract a small helper like
validateVars(t, tc.vars) that calls require.True(t, gjson.Valid(vars)) and use
it in every test loop) so malformed tc.vars is consistently caught; update all
t.Run closures that reference tc.vars (the testCases iterations) to call the
helper or gjson.Valid before further parsing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go
devsergiy
requested changes
Mar 2, 2026
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
devsergiy
approved these changes
Mar 4, 2026
… ludwig/eng-8642-add-support-for-plain-fields
dkorittki
approved these changes
Mar 10, 2026
This was referenced Mar 10, 2026
Noroth
pushed a commit
that referenced
this pull request
Mar 10, 2026
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.262](v2.0.0-rc.261...v2.0.0-rc.262) (2026-03-10) ### Features * add initial support for [@requires](https://github.com/requires) in gRPC ([#1362](#1362)) ([07ae75e](07ae75e)) --- 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 initial support for @requires in gRPC. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 tasks
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
3 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 introduces initial support for
@requiresin gRPC. It translates incoming entity calls to corresponding RPC calls in the gRPC service and invokes an additional rpc for each required field providing the variable data from the representation variables.Summary by CodeRabbit
New Features
Tests
Chores
Checklist