Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds interface-based entity federation support to the gRPC datasource: new entity helper utilities and tests, RPCCall tracking of requested entity type, execution-plan visitor and planner changes for interface fragments, json-builder/merge refactor using per-call entity index maps, and proto/mapping/mock/test updates for Resource/Subresource lookups. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as GraphQL Client
participant DataSource as gRPC DataSource
participant Service as gRPC Service
participant Validator as Entity Helper
participant Merger as JSON Merger
Client->>DataSource: Load(query with _entities representations)
DataSource->>Validator: getRepesentations(variables)
Validator-->>DataSource: representations[]
DataSource->>Validator: newEntityIndexMap(requestedEntityType, representations)
Validator-->>DataSource: entityIndexMap (indices for matching __typename)
DataSource->>Service: RPCCall Lookup*ById (RequestedEntityType="Resource"/"Subresource")
Service-->>DataSource: response (_entities array with concrete oneof instances)
DataSource->>Validator: validateEntityResponse(response, requestedEntityType, representations)
Validator-->>DataSource: validation OK / error
DataSource->>Merger: mergeEntities(leftResult, rightResult with entityIndexMap)
Merger->>Merger: place each right entity at entityIndexMap[index]
Merger-->>DataSource: merged _entities
DataSource-->>Client: returned GraphQL response with resolved entities
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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 |
Noroth
requested review from
StarpTech,
devsergiy,
dkorittki,
jensneuse and
ysmolski
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
Author
|
@claude review |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
![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: 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/execution_plan_visitor_federation.go (1)
559-579:⚠️ Potential issue | 🟠 MajorDon't hard-code
__typenamefor interface entities.Line 563 seeds
__typenamewithtypeNamebefore Lines 573-576 convert the message into an interface. For_entities { ... on Resource { __typename ... } }, that will surface"Resource"for every row instead of the concrete member type, so interface-object responses return the wrong runtime typename. Keep the static field only for concrete object entities and let interface entity responses derive__typenamefrom the resolved oneof member.Based on learnings: In `v2/pkg/engine/datasource/grpc_datasource`, `message.Fields` only defines which fields are surfaced in the JSON response.💡 Suggested fix
- entityMessage := &RPCMessage{ - Name: typeName, - Fields: []RPCField{ - { - Name: "__typename", - ProtoTypeName: DataTypeString, - JSONPath: "__typename", - StaticValue: typeName, - }, - }, - } + entityMessage := &RPCMessage{ + Name: typeName, + } // Check if the entity type is an interface and set oneof type and member types. if node, found := r.definition.NodeByNameStr(typeName); found { if node.Kind == ast.NodeKindInterfaceTypeDefinition { entityMessage.OneOfType = OneOfTypeInterface if memberTypes, ok := r.definition.InterfaceTypeDefinitionImplementedByObjectWithNames(node.Ref); ok { entityMessage.MemberTypes = memberTypes } + } else { + entityMessage.Fields = []RPCField{ + { + Name: "__typename", + ProtoTypeName: DataTypeString, + JSONPath: "__typename", + StaticValue: typeName, + }, + } } - }🤖 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_visitor_federation.go` around lines 559 - 579, The code currently always seeds a static "__typename" RPCField with StaticValue typeName on the RPCMessage (entityMessage), which causes interface responses to report the interface name instead of the concrete member type; change the logic so that you only add the static "__typename" field when the type is a concrete object (i.e., when r.definition.NodeByNameStr(typeName) does not yield a NodeKindInterfaceTypeDefinition) and do not set entityMessage.Fields with StaticValue for interface types—leave "__typename" off for interfaces so the resolved oneof member supplies the runtime typename; adjust the entityMessage construction (or move the RPCField creation) and retain the existing branch that sets entityMessage.OneOfType = OneOfTypeInterface and entityMessage.MemberTypes when the node is an interface (using r.definition.InterfaceTypeDefinitionImplementedByObjectWithNames).
🧹 Nitpick comments (5)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
562-564: Line 562 has a redundanterrassertion.
erris already handled right afterNewProtoCompiler, so the extrarequire.NoError(t, err)beforenewJSONBuildercan be removed for clarity.♻️ Suggested cleanup
- require.NoError(t, err) jsonBuilder, err := newJSONBuilder(nil, testMapping(), gjson.Result{}) require.NoError(t, err)🤖 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_test.go` around lines 562 - 564, Remove the redundant error check before calling newJSONBuilder: the variable err was already asserted immediately after NewProtoCompiler, so delete the extra require.NoError(t, err) that appears directly above the newJSONBuilder invocation (referencing newJSONBuilder and the prior NewProtoCompiler block) and keep only the require.NoError(t, err) that verifies the compiler creation; this cleans up the test by avoiding a duplicate assertion on the same err variable.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (1)
93-133: Good interface-entity coverage; consider asserting__typenameexplicitly.Since
Line 95requests__typename, asserting it in validation would better lock down concrete-type dispatch (Product/Storage/Warehouse) in this path.Suggested test hardening
validate: func(t *testing.T, data map[string]interface{}) { entities, ok := data["_entities"].([]interface{}) require.True(t, ok, "_entities should be an array") require.Len(t, entities, 3, "Should return 3 entities") // Entity 0: Product (i%3 == 0) e0, ok := entities[0].(map[string]interface{}) require.True(t, ok) + require.Equal(t, "Product", e0["__typename"]) require.Equal(t, "1", e0["id"]) require.Equal(t, "Product 1", e0["name"]) // Entity 1: Storage (i%3 == 1) e1, ok := entities[1].(map[string]interface{}) require.True(t, ok) + require.Equal(t, "Storage", e1["__typename"]) require.Equal(t, "2", e1["id"]) require.Equal(t, "Storage 2", e1["name"]) // Entity 2: Warehouse (i%3 == 2) e2, ok := entities[2].(map[string]interface{}) require.True(t, ok) + require.Equal(t, "Warehouse", e2["__typename"]) require.Equal(t, "3", e2["id"]) require.Equal(t, "Warehouse 3", e2["name"]) },🤖 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 93 - 133, The test validates entity ids and names returned by the _entities query but omits asserting the requested __typename; update the validate function in this test case ("Query interface entity Resource") to also assert each entity's "__typename" equals the expected concrete type ("Product" for e0, "Storage" for e1, "Warehouse" for e2) immediately after casting each entity (e0, e1, e2) to map[string]interface{} so the selection/dispatch behavior is explicitly verified.v2/pkg/grpctest/mockservice_lookup.go (1)
64-97: Make the concrete variant stable per key.Line 66 picks the
Resourcesubtype from the loop index, so the sameidcan becomeProduct,Storage, orWarehousedepending on batch order. That makes these federation fixtures harder to reason about. Deriving the variant fromresourceIdinstead keeps the mock deterministic per entity.♻️ Suggested refactor
+import ( + "context" + "fmt" + "hash/crc32" + "math/rand" + + "github.com/wundergraph/graphql-go-tools/v2/pkg/grpctest/productv1" +) - -import ( - "context" - "fmt" - "math/rand" - - "github.com/wundergraph/graphql-go-tools/v2/pkg/grpctest/productv1" -) func (s *MockService) LookupResourceById(ctx context.Context, in *productv1.LookupResourceByIdRequest) (*productv1.LookupResourceByIdResponse, error) { var results []*productv1.Resource - for i, input := range in.GetKeys() { + for _, input := range in.GetKeys() { resourceId := input.GetId() - switch i % 3 { + switch crc32.ChecksumIEEE([]byte(resourceId)) % 3 { case 0: results = append(results, &productv1.Resource{ Instance: &productv1.Resource_Product{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/grpctest/mockservice_lookup.go` around lines 64 - 97, The current loop in the lookup mock uses the loop index (i % 3) to choose a Resource subtype, causing the same resourceId to map to different variants depending on batch order; change the selection to be deterministic per key by hashing or otherwise deriving a stable value from resourceId (the input.GetKeys() loop, resourceId variable) and use that (e.g., stableHash(resourceId) % 3) instead of i%3 when choosing between Resource_Product, Resource_Storage and Resource_Warehouse so each id always yields the same concrete variant in the results slice.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
142-142: Typo:getRepesentationsshould begetRepresentations.The function name is missing an 'r' in "Representations".
✏️ Suggested fix
- representations := getRepesentations(variables) + representations := getRepresentations(variables)This also requires renaming the function in
entity.go.🤖 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.go` at line 142, Refactor the misspelled function call getRepesentations to getRepresentations: rename the function declaration in entity.go from getRepesentations to getRepresentations and update all call sites (including the call in grpc_datasource.go where representations := getRepesentations(variables)) to the corrected name so references compile and match naming; ensure tests/build run after renaming to catch any remaining references.v2/pkg/engine/datasource/grpc_datasource/entity.go (1)
32-41: Typo:getRepesentationsshould begetRepresentations.The function name is missing an 'r' in "Representations".
✏️ Suggested fix
-// getRepesentations gets the representations from the variables. +// getRepresentations gets the representations from the variables. // If no representations are found, it returns nil. -func getRepesentations(variables gjson.Result) []gjson.Result { +func getRepresentations(variables gjson.Result) []gjson.Result { r := variables.Get("representations") if !r.Exists() { return nil } return r.Array() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/entity.go` around lines 32 - 41, Rename the misnamed function getRepesentations to getRepresentations and update all references/call sites to use the corrected identifier; keep the signature func getRepresentations(variables gjson.Result) []gjson.Result and preserve the implementation (checking variables.Get("representations").Exists() and returning r.Array() or nil). Search for any usages (tests, other functions in the package) and update imports/refs to the new name to avoid build errors.
🤖 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/json_builder.go`:
- Around line 83-89: The code currently overwrites the entire root object by
assigning entities := left and then unconditionally replacing it with a new
astjson.ObjectValue when the _entities path is missing; instead only allocate a
new root when left is nil so existing top-level fields are preserved. Change the
logic in json_builder.go so entities = left if left != nil, and only create
entities = astjson.ObjectValue(j.jsonArena) when left == nil; then ensure you
Set the entityPath to an ArrayValue on entities only when arr is nil (creating
arr from entities.Get(entityPath) as now). Keep references to entityPath,
j.jsonArena, left, entities, arr and use astjson.ObjectValue /
astjson.ArrayValue as needed.
---
Outside diff comments:
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`:
- Around line 559-579: The code currently always seeds a static "__typename"
RPCField with StaticValue typeName on the RPCMessage (entityMessage), which
causes interface responses to report the interface name instead of the concrete
member type; change the logic so that you only add the static "__typename" field
when the type is a concrete object (i.e., when
r.definition.NodeByNameStr(typeName) does not yield a
NodeKindInterfaceTypeDefinition) and do not set entityMessage.Fields with
StaticValue for interface types—leave "__typename" off for interfaces so the
resolved oneof member supplies the runtime typename; adjust the entityMessage
construction (or move the RPCField creation) and retain the existing branch that
sets entityMessage.OneOfType = OneOfTypeInterface and entityMessage.MemberTypes
when the node is an interface (using
r.definition.InterfaceTypeDefinitionImplementedByObjectWithNames).
---
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/entity.go`:
- Around line 32-41: Rename the misnamed function getRepesentations to
getRepresentations and update all references/call sites to use the corrected
identifier; keep the signature func getRepresentations(variables gjson.Result)
[]gjson.Result and preserve the implementation (checking
variables.Get("representations").Exists() and returning r.Array() or nil).
Search for any usages (tests, other functions in the package) and update
imports/refs to the new name to avoid build errors.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go`:
- Around line 93-133: The test validates entity ids and names returned by the
_entities query but omits asserting the requested __typename; update the
validate function in this test case ("Query interface entity Resource") to also
assert each entity's "__typename" equals the expected concrete type ("Product"
for e0, "Storage" for e1, "Warehouse" for e2) immediately after casting each
entity (e0, e1, e2) to map[string]interface{} so the selection/dispatch behavior
is explicitly verified.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go`:
- Around line 562-564: Remove the redundant error check before calling
newJSONBuilder: the variable err was already asserted immediately after
NewProtoCompiler, so delete the extra require.NoError(t, err) that appears
directly above the newJSONBuilder invocation (referencing newJSONBuilder and the
prior NewProtoCompiler block) and keep only the require.NoError(t, err) that
verifies the compiler creation; this cleans up the test by avoiding a duplicate
assertion on the same err variable.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go`:
- Line 142: Refactor the misspelled function call getRepesentations to
getRepresentations: rename the function declaration in entity.go from
getRepesentations to getRepresentations and update all call sites (including the
call in grpc_datasource.go where representations :=
getRepesentations(variables)) to the corrected name so references compile and
match naming; ensure tests/build run after renaming to catch any remaining
references.
In `@v2/pkg/grpctest/mockservice_lookup.go`:
- Around line 64-97: The current loop in the lookup mock uses the loop index (i
% 3) to choose a Resource subtype, causing the same resourceId to map to
different variants depending on batch order; change the selection to be
deterministic per key by hashing or otherwise deriving a stable value from
resourceId (the input.GetKeys() loop, resourceId variable) and use that (e.g.,
stableHash(resourceId) % 3) instead of i%3 when choosing between
Resource_Product, Resource_Storage and Resource_Warehouse so each id always
yields the same concrete variant in the results slice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9708479-28ef-4a01-aa6d-4ef9613469e4
⛔ 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 (16)
v2/pkg/engine/datasource/grpc_datasource/entity.gov2/pkg/engine/datasource/grpc_datasource/entity_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.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_requires_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.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/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/grpctest/mockservice_lookup.gov2/pkg/grpctest/product.protov2/pkg/grpctest/testdata/products.graphqls
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/entity.go (1)
32-41: RenamegetRepesentationstogetRepresentationsto avoid API typo propagation.The current misspelling hurts discoverability and can spread to call sites/tests.
✏️ Proposed rename in this file
-// getRepesentations gets the representations from the variables. +// getRepresentations gets the representations from the variables. // If no representations are found, it returns nil. -func getRepesentations(variables gjson.Result) []gjson.Result { +func getRepresentations(variables gjson.Result) []gjson.Result { r := variables.Get("representations") if !r.Exists() { return nil } return r.Array() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/entity.go` around lines 32 - 41, Rename the misspelled function getRepesentations to getRepresentations and update all references and tests accordingly: change the function declaration from getRepesentations(variables gjson.Result) to getRepresentations(variables gjson.Result), update any call sites that invoke getRepesentations to call getRepresentations, and run/update unit tests or imports that reference the old name to ensure compilation succeeds; keep the implementation identical (reading variables.Get("representations") and returning r.Array()) so behavior is unchanged.
🤖 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/entity.go`:
- Around line 32-41: Rename the misspelled function getRepesentations to
getRepresentations and update all references and tests accordingly: change the
function declaration from getRepesentations(variables gjson.Result) to
getRepresentations(variables gjson.Result), update any call sites that invoke
getRepesentations to call getRepresentations, and run/update unit tests or
imports that reference the old name to ensure compilation succeeds; keep the
implementation identical (reading variables.Get("representations") and returning
r.Array()) so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea29704a-9535-407a-95c9-ff268c0130a9
📒 Files selected for processing (2)
v2/pkg/engine/datasource/grpc_datasource/entity.gov2/pkg/engine/datasource/grpc_datasource/entity_test.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/datasource/grpc_datasource/entity_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (1)
107-129: Assert__typenamein the interface-only test too.This case requests
__typenamebut never verifies it. Adding typenames here would make the interface-resolution assertion complete and catch regressions earlier.Patch suggestion
validate: func(t *testing.T, data map[string]interface{}) { entities, ok := data["_entities"].([]interface{}) require.True(t, ok, "_entities should be an array") require.Len(t, entities, 3, "Should return 3 entities") // Entity 0: Product (i%3 == 0) e0, ok := entities[0].(map[string]interface{}) require.True(t, ok) + require.Equal(t, "Product", e0["__typename"]) require.Equal(t, "1", e0["id"]) require.Equal(t, "Product 1", e0["name"]) // Entity 1: Storage (i%3 == 1) e1, ok := entities[1].(map[string]interface{}) require.True(t, ok) + require.Equal(t, "Storage", e1["__typename"]) require.Equal(t, "2", e1["id"]) require.Equal(t, "Storage 2", e1["name"]) // Entity 2: Warehouse (i%3 == 2) e2, ok := entities[2].(map[string]interface{}) require.True(t, ok) + require.Equal(t, "Warehouse", e2["__typename"]) require.Equal(t, "3", e2["id"]) require.Equal(t, "Warehouse 3", e2["name"]) },🤖 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 107 - 129, The test's validate function in grpc_datasource_federation_test.go currently checks id and name for each entity but omits verifying the requested __typename; update the validate closure (the validate func used in the test) to assert that each entity map (entities[0], entities[1], entities[2]) contains the correct "__typename" value ("Product", "Storage", "Warehouse" respectively) in addition to the existing id and name checks so interface-resolution is fully validated.
🤖 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/grpc_datasource_federation_test.go`:
- Around line 107-129: The test's validate function in
grpc_datasource_federation_test.go currently checks id and name for each entity
but omits verifying the requested __typename; update the validate closure (the
validate func used in the test) to assert that each entity map (entities[0],
entities[1], entities[2]) contains the correct "__typename" value ("Product",
"Storage", "Warehouse" respectively) in addition to the existing id and name
checks so interface-resolution is fully validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9d25011-7a2c-4221-ab9d-6828fa79334d
⛔ 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 (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/grpctest/mockservice_lookup.gov2/pkg/grpctest/product.protov2/pkg/grpctest/testdata/products.graphqls
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/grpctest/mapping/mapping.go
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go
- v2/pkg/grpctest/mockservice_lookup.go
- v2/pkg/grpctest/testdata/products.graphqls
- v2/pkg/grpctest/product.proto
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
39-47: Update the constructor comment to match the new flow.
newJSONBuilderno longer creates federation index maps itself, but the docstring above still says it does. Please align the comment with the new per-callresultData.entityIndexMappath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/json_builder.go` around lines 39 - 47, Update the doc comment on newJSONBuilder to reflect that it no longer creates federation index maps; instead note that index maps are now constructed per call via resultData.entityIndexMap and that newJSONBuilder only initializes a jsonBuilder with mapping, variables and jsonArena. Mention the jsonBuilder type and the shift of index map responsibility to the per-call resultData to make the comment accurate.
🤖 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/json_builder.go`:
- Around line 39-47: Update the doc comment on newJSONBuilder to reflect that it
no longer creates federation index maps; instead note that index maps are now
constructed per call via resultData.entityIndexMap and that newJSONBuilder only
initializes a jsonBuilder with mapping, variables and jsonArena. Mention the
jsonBuilder type and the shift of index map responsibility to the per-call
resultData to make the comment accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee43b460-abd8-434c-bbaa-777c44551764
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go
devsergiy
approved these changes
Apr 15, 2026
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 interface objects in the grpc datasource. It also refactors entity mapping and validation to a separate file with proper unit tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.