refactor: improve entity validation and mapping#1472
Merged
Conversation
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
|
Caution Review failedPull request was closed or merged during review 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 (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThreads requested entity type through planning, adds package-private helpers to extract/validate federated entity representations and build per-call entity index maps, stores AST definition on DataSource, refactors JSON merge to consume resultData with entityIndexMap, and updates tests and RPCCall fixtures to include RequestedEntityType. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant DS as DataSource
participant Validator as Validator
participant JSON as JSONBuilder
Client->>DS: Load(variables, operation)
DS->>Validator: getRepresentations(variables)
Validator-->>DS: representations[]
DS->>DS: Issue gRPC RPCCall (RequestedEntityType)
DS-->>DS: Receive gRPC response (data)
DS->>Validator: validateEntityResponse(data, requestedType, representations)
Validator-->>DS: nil / error
DS->>Validator: newEntityIndexMap(requestedType, representations)
Validator-->>DS: entityIndexMap
DS->>JSON: mergeValues(root, resultData{response, entityIndexMap, kind})
JSON->>JSON: mergeEntities using resultData.entityIndexMap
JSON-->>DS: merged result
DS-->>Client: Return merged response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
113-114: Consider printingRequestedEntityTypeinRPCExecutionPlan.String()Great addition to
RPCCall. Including it in the plan string output would make entity-call debugging easier.Suggested diff
func (r *RPCExecutionPlan) String() string { var result strings.Builder @@ fmt.Fprintf(&result, " Service: %s\n", call.ServiceName) fmt.Fprintf(&result, " Method: %s\n", call.MethodName) + if call.RequestedEntityType != "" { + fmt.Fprintf(&result, " RequestedEntityType: %s\n", call.RequestedEntityType) + } result.WriteString(" Request:\n") formatRPCMessage(&result, call.Request, 8)🤖 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 113 - 114, RPCExecutionPlan.String() should include the RequestedEntityType field to make entity-targeted RPCs easier to debug: update the RPCExecutionPlan.String() implementation to append the RequestedEntityType (when non-empty) into the returned plan string—e.g. add "requestedEntity=..." or similar near where RPCCall details are formatted—so callers inspecting RPCExecutionPlan.String() will see the entity type alongside existing RPCCall information.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
562-562: Remove redundant staleerrassertionAt this line,
errstill references earlier setup work, so this check doesn’t validate new behavior.Suggested diff
- 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` at line 562, The line asserting require.NoError(t, err) is stale because `err` refers to earlier setup, so remove that redundant assertion and instead assert the error returned by the specific operation under test (or reassign `err` from that operation before asserting). Locate the stale check in the test (the require.NoError call referencing `err` in grpc_datasource_test.go) and either delete it or replace it with an assertion against the actual error variable produced by the function/method being exercised in this test.v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
571-577: Fail fast if an interface entity has no concrete implementors.At Line 574-577,
OneOfTypeInterfaceis set even if implementor lookup fails (ok == false), which can leave an under-specified response message. Consider stopping planning with an internal error in that branch.Suggested hardening
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 - } + memberTypes, ok := r.definition.InterfaceTypeDefinitionImplementedByObjectWithNames(node.Ref) + if !ok || len(memberTypes) == 0 { + r.walker.StopWithInternalErr(fmt.Errorf("interface entity type %s has no concrete implementors", typeName)) + return + } + entityMessage.OneOfType = OneOfTypeInterface + entityMessage.MemberTypes = memberTypes } }🤖 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 571 - 577, The code sets entityMessage.OneOfType = OneOfTypeInterface even when InterfaceTypeDefinitionImplementedByObjectWithNames(node.Ref) returns ok == false; change the logic in the block that checks r.definition.NodeByNameStr(typeName) so you first call InterfaceTypeDefinitionImplementedByObjectWithNames(node.Ref) and if ok is false return/raise an internal planning error (fail-fast) instead of setting OneOfType, otherwise set entityMessage.OneOfType = OneOfTypeInterface and assign entityMessage.MemberTypes = memberTypes; update the containing function to propagate that error (or abort planning) so under-specified interface entities are not allowed.v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
39-42: The constructor comment still describes removed behavior.
newJSONBuilderno longer builds or stores the federation index map itself; that moved intoentity.go/grpc_datasource.go. Updating this comment will make the new ownership model much easier to follow.📝 Comment-only cleanup
-// newJSONBuilder creates a new JSON builder instance with the provided mapping -// and variables. The builder automatically creates an index map for proper -// federation entity ordering if representations are present in the variables. +// newJSONBuilder creates a new JSON builder instance with the provided mapping +// and variables. Federation entity ordering is handled outside the builder via +// per-call entityIndexMap construction in the datasource/entity helpers. func newJSONBuilder(a arena.Arena, mapping *GRPCMapping, variables gjson.Result) (*jsonBuilder, error) {🤖 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 - 42, Update the doc comment for newJSONBuilder to remove the outdated statement that the builder "automatically creates an index map for proper federation entity ordering" and instead state that newJSONBuilder only creates a JSON builder with the provided mapping and variables, while federation index map construction/ownership has been moved to entity.go/grpc_datasource.go; reference the function name newJSONBuilder in the comment so readers know where the behavior changed.
🤖 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/entity.go`:
- Around line 57-59: The current guard in validateEntityResponse treats an empty
representations slice as an error; change it to only error when the
representations value is missing (nil or not present) rather than when its
length is zero—i.e., detect absence via a presence boolean (or compare to nil)
so that representations == nil triggers the error but representations != nil &&
len(representations) == 0 is accepted; update the corresponding expectation in
v2/pkg/engine/datasource/grpc_datasource/entity_test.go to allow an explicit
empty list case.
---
Nitpick comments:
In
`@v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go`:
- Around line 571-577: The code sets entityMessage.OneOfType =
OneOfTypeInterface even when
InterfaceTypeDefinitionImplementedByObjectWithNames(node.Ref) returns ok ==
false; change the logic in the block that checks
r.definition.NodeByNameStr(typeName) so you first call
InterfaceTypeDefinitionImplementedByObjectWithNames(node.Ref) and if ok is false
return/raise an internal planning error (fail-fast) instead of setting
OneOfType, otherwise set entityMessage.OneOfType = OneOfTypeInterface and assign
entityMessage.MemberTypes = memberTypes; update the containing function to
propagate that error (or abort planning) so under-specified interface entities
are not allowed.
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan.go`:
- Around line 113-114: RPCExecutionPlan.String() should include the
RequestedEntityType field to make entity-targeted RPCs easier to debug: update
the RPCExecutionPlan.String() implementation to append the RequestedEntityType
(when non-empty) into the returned plan string—e.g. add "requestedEntity=..." or
similar near where RPCCall details are formatted—so callers inspecting
RPCExecutionPlan.String() will see the entity type alongside existing RPCCall
information.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go`:
- Line 562: The line asserting require.NoError(t, err) is stale because `err`
refers to earlier setup, so remove that redundant assertion and instead assert
the error returned by the specific operation under test (or reassign `err` from
that operation before asserting). Locate the stale check in the test (the
require.NoError call referencing `err` in grpc_datasource_test.go) and either
delete it or replace it with an assertion against the actual error variable
produced by the function/method being exercised in this test.
In `@v2/pkg/engine/datasource/grpc_datasource/json_builder.go`:
- Around line 39-42: Update the doc comment for newJSONBuilder to remove the
outdated statement that the builder "automatically creates an index map for
proper federation entity ordering" and instead state that newJSONBuilder only
creates a JSON builder with the provided mapping and variables, while federation
index map construction/ownership has been moved to entity.go/grpc_datasource.go;
reference the function name newJSONBuilder in the comment so readers know where
the behavior changed.
🪄 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: 8f6c8727-b16c-49a0-b7c8-d7b048d09f73
📒 Files selected for processing (10)
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_test.gov2/pkg/engine/datasource/grpc_datasource/json_builder.go
… ludwig/improve-entity-mapping
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/grpc_datasource.go`:
- Line 140: The function name getRepesentations contains a spelling mistake;
rename the function to getRepresentations at its declaration and update the
accompanying comment, then update every call site (the 11 usages) to use
getRepresentations (including the usage in grpc_datasource.go where it’s
currently invoked as getRepesentations(variables)) so the symbol name is
consistent across the codebase and the tests compile.
- Line 51: The DataSource struct contains an unused field named definition (type
*ast.Document); remove this field from the DataSource struct and delete any
assignment to it in the constructor/initializer (currently assigning from
config.Definition), and then run go build to ensure there are no remaining
references; also remove the ast import if it becomes unused. Locate symbols:
DataSource (struct) and config.Definition (assignment in the
NewDataSource/constructor) to find and remove the dead field and its
initialization.
🪄 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: 22b33442-072f-4c43-9a81-c4d985de7906
📒 Files selected for processing (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
dkorittki
approved these changes
Apr 20, 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 simplifies the entity validation and mapping. Instead of having a global list of all representations, will only validate against the variables that are required for a specific call. We retrieve the expected entity type during the planning and use this information to extract the representations for one call
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.