feat: add support for multiple key directives #1262
Conversation
… ludwig/eng-7007-support-of-multiple-and-nested-keys-for-entities
WalkthroughThis change set introduces significant enhancements to the gRPC datasource, focusing on federation support, improved execution planning, and robust test coverage. Key updates include new and refactored planning logic for entity lookups, composite types, and required fields; expanded configuration and mapping structures; concurrent gRPC call execution with improved JSON response construction; and comprehensive federation-aware testing. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…sted-keys-for-entities
…sted-keys-for-entities
Noroth
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
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: 5
🧹 Nitpick comments (3)
v2/pkg/astvisitor/visitor.go (1)
4009-4024: Handle nested inline fragments, not just the first level
ResolveInlineFragmentwalks two levels up (len-2) and returns that ancestor if it is anInlineFragment. This misses valid cases where a field is nested deeper, e.g.:... on A { <- InlineFragment 1 ... on B { <- InlineFragment 2 id <- current field } }Here
len-2points to the innermost SelectionSet, so the function returns-1.Consider scanning upwards until the first
InlineFragmentis found:-func (w *Walker) ResolveInlineFragment() (int, bool) { - if len(w.Ancestors) < 2 { - return -1, false - } - node := w.Ancestors[len(w.Ancestors)-2] - if node.Kind != ast.NodeKindInlineFragment { - return -1, false - } - return node.Ref, true +func (w *Walker) ResolveInlineFragment() (int, bool) { + for i := len(w.Ancestors) - 2; i >= 0; i-- { + if w.Ancestors[i].Kind == ast.NodeKindInlineFragment { + return w.Ancestors[i].Ref, true + } + } + return -1, false }This remains O(depth) (rarely > 5) and makes the helper robust for any nesting depth.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (2)
99-101: Remove or implement commented codeThere's commented out code that checks for entity inline fragments. This should either be removed if not needed or properly implemented with a clear comment explaining why it's disabled.
// EnterInlineFragment implements astvisitor.InlineFragmentVisitor. func (r *rpcPlanVisitorFederation) EnterInlineFragment(ref int) { - // if !r.IsEntityInlineFragment(ref) { - // return - // } - fragmentName := r.operation.InlineFragmentTypeConditionNameString(ref)
405-409: Clarify or remove TODO commentThe TODO comment questions whether the
DisableEntityResolvercheck is needed. This should be clarified with proper documentation or removed if the check is confirmed to be necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
execution/engine/execution_engine_grpc_test.go(2 hunks)v2/pkg/astvisitor/visitor.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/configuration.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/configuration_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(5 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(22 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(9 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(5 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go(1 hunks)v2/pkg/grpctest/mapping/mapping.go(1 hunks)v2/pkg/grpctest/testdata/products.graphqls(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: in the graphql-go-tools grpc datasource, only non-null lists use protobuf's repeated field syntax di...
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/engine/datasource/grpc_datasource/configuration.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
📚 Learning: in the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.buf...
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go
🧬 Code Graph Analysis (3)
execution/engine/execution_engine_grpc_test.go (1)
v2/pkg/grpctest/mapping/mapping.go (1)
MustDefaultGRPCMapping(1026-1029)
v2/pkg/astvisitor/visitor.go (2)
pkg/ast/ast_node_kind.go (3)
NodeKindField(32-32)NodeKindOperationDefinition(30-30)NodeKindInlineFragment(35-35)v2/pkg/engine/plan/plan.go (1)
Kind(7-7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
v2/pkg/astvisitor/visitor.go (1)
Walker(65-97)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)v2/pkg/engine/datasource/grpc_datasource/configuration.go (1)
GRPCMapping(16-31)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
RPCCall(58-69)ListMetadata(179-184)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (48)
v2/pkg/grpctest/testdata/products.graphqls (1)
14-20: LGTM! Well-structured federation entity with multiple keys.The new
Warehousetype correctly implements GraphQL federation patterns with multiple@keydirectives on different fields (idandslug). This enables flexible entity resolution in federated schemas. The corresponding update to the_Entityunion ensures proper federation support.Also applies to: 403-403
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
376-376: LGTM! Properly extends gRPC datasource with federation configuration.The addition of
FederationConfigsfield initialized withp.dataSourcePlannerConfig.RequiredFieldscorrectly extends the gRPC datasource configuration to support federation-aware planning and execution. This aligns with the broader federation support being added to the gRPC datasource package.v2/pkg/grpctest/mapping/mapping.go (1)
228-249: LGTM! EntityRPCs restructuring properly supports multiple key directives.The change from
map[string]grpcdatasource.EntityRPCConfigtomap[string][]grpcdatasource.EntityRPCConfigcorrectly enables multiple entity RPC configurations per type. This supports the federation feature where entities can be resolved through different keys. The existing entries are properly wrapped in slices, maintaining compatibility while enabling the new functionality.execution/engine/execution_engine_grpc_test.go (2)
237-237: LGTM! Proper test configuration update.The addition of explicit gRPC mapping configuration aligns with the federation support enhancements and ensures the test exercises the updated execution logic with proper mapping context.
258-258: LGTM! Consistent test configuration.This change maintains consistency with the other test subcase and properly provides the required gRPC mapping configuration.
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (2)
221-242: LGTM! Proper federation multi-key support implementation.The EntityRPCs structure change from
map[string]EntityRPCConfigtomap[string][]EntityRPCConfigcorrectly enables support for multiple key directives per entity type. Wrapping existing configurations in slices maintains compatibility while enabling the new federation capabilities.
631-638: LGTM! Consistent field mapping addition.The new FilterTypeInput field mapping properly extends the mapping coverage with consistent field name translation from GraphQL camelCase to protobuf snake_case format.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (7)
20-20: LGTM!The import addition is necessary for the new
plan.FederationFieldConfigurationstype used in the federation support.
40-40: LGTM!The
federationConfigsfield addition properly integrates federation support into the DataSource struct with appropriate visibility and type.
54-54: LGTM!The exported
FederationConfigsfield appropriately exposes federation configuration in the public API for DataSource creation.
60-60: LGTM!The federation configuration is properly passed to the planner to enable federation-aware execution plan creation.
71-71: LGTM!The federation configuration is properly stored in the DataSource instance, following the same pattern as other configuration fields.
104-104: LGTM!The blank line addition improves code readability by visually separating the method name construction from the gRPC invocation.
238-238: LGTM!The change to
field.AliasOrPath()improves alias handling for optional scalar fields, which is important for federation support where field aliases play a key role in entity resolution.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (7)
128-135: LGTM - Proper GRPCMapping structure usageThe explicit definition of the
GRPCMappingwithServiceandQueryRPCsfields correctly follows the updated structure that supports multiple entity RPC configurations. This change aligns well with the federation support mentioned in the PR objectives.
187-194: LGTM - Consistent mapping configurationThe mapping configuration is consistent with the pattern established in other tests, properly defining the service name and RPC method configurations.
199-201: LGTM - Argument mappings properly configuredThe addition of
ArgumentMappingsfield correctly maps the GraphQL argument"filter"to the gRPC field"filter", which is necessary for proper argument passing between GraphQL and gRPC layers.
277-284: LGTM - Consistent service configurationThe service configuration follows the same pattern as previous test cases, maintaining consistency across the test suite.
289-291: LGTM - Proper argument mappingThe argument mapping configuration is correctly set up to handle the filter parameter mapping between GraphQL and gRPC.
379-403: LGTM - Comprehensive user query mappingThe mapping configuration for the user query test case is properly structured with:
- Service name set to "Products"
- QueryRPCs defining the user query RPC method
- Field mappings for both Query and User types
This provides good test coverage for different entity types.
1861-1901: LGTM - Good test coverage for aliased nullable fieldsThe new test case "Query nullable fields type with all fields and aliases" provides valuable coverage for:
- Testing nullable field handling with GraphQL aliases
- Verifying that required fields have non-empty values when aliased
- Ensuring optional fields are present but may be null when aliased
- Proper type validation for aliased fields
The test logic correctly validates that:
- Required fields (
requiredString1,requiredInt1) have non-empty values- Optional fields (
optionalString1,optionalInt1, etc.) are present but may be null- Type assertions are performed for non-null optional fields
This test case complements the existing nullable fields tests and adds important coverage for alias handling.
v2/pkg/engine/datasource/grpc_datasource/configuration.go (4)
131-145: LGTM! The entity RPC resolution logic handles multiple configurations correctly.The method properly iterates through all entity RPC configs for a given type and uses the new key comparison logic to find the matching configuration.
147-170: Well-implemented set operations for key comparison.The
keySettype and its methods provide an efficient way to compare unordered key collections. The implementation correctly handles edge cases like empty strings and whitespace.
172-191: Correct implementation of federation key comparison logic.The function properly handles the comparison of federation keys by:
- Stripping nested selection sets to focus on top-level fields
- Comparing keys as unordered sets (appropriate for GraphQL field selection)
- Handling the early return optimization for identical strings
193-223: Robust implementation of selection set stripping with good edge case handling.The function correctly:
- Handles nested braces with depth tracking
- Gracefully handles unmatched closing braces by ignoring them
- Deduplicates consecutive spaces
- Converts commas to spaces for uniform parsing
This implementation is resilient to various input formats and edge cases.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (2)
1349-1349: Planner initialization correctly updated for federation support.The addition of
nilas the third parameter toNewPlanneraligns with the updated constructor signature that now accepts federation configurations. Passingnilindicates these tests don't require federation features.Also applies to: 1452-1452
209-210: Consistent field naming updates to match protobuf conventions.All field names have been correctly updated from camelCase to snake_case, which aligns with protobuf naming conventions. This ensures consistency between the GraphQL schema and the underlying gRPC service definitions.
Also applies to: 392-393, 397-398, 415-416, 435-436, 493-494, 498-499, 516-517, 536-537, 582-583, 588-589, 601-601, 748-749, 830-831, 847-848, 859-860, 876-877
v2/pkg/engine/datasource/grpc_datasource/configuration_test.go (3)
5-239: Excellent test coverage for key field comparison logic.The test suite comprehensively covers:
- Basic equality and inequality cases
- Order independence of fields
- Various separator formats (spaces, commas, mixed)
- Whitespace handling and normalization
- Duplicate field handling
- Selection set stripping
- Edge cases (empty strings, unbalanced braces, case sensitivity)
This provides strong confidence in the correctness of the key comparison implementation.
241-309: Thorough testing of keySet operations.The tests properly validate:
- The
addmethod's handling of empty strings and whitespace- The
equalsmethod's correctness for various scenarios- Edge cases like empty sets
311-398: Comprehensive test coverage for selection set stripping.Tests effectively validate:
- Simple and nested selection sets
- Multiple selection sets in one string
- Edge cases (unbalanced braces, empty sets, consecutive spaces)
- Error resilience (extra closing braces, malformed input)
v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go (4)
46-58: Well-structured required fields visitation logic.The method properly creates a fragment document and walks it with appropriate error handling at each step.
60-69: Good defensive programming with nil message check.The early validation ensures the visitor has required state before proceeding.
92-102: Correct management of message ancestor stack.The method properly maintains the stack of ancestor messages with appropriate bounds checking.
130-164: Well-implemented composite type handling for interfaces and unions.The method correctly:
- Distinguishes between interface and union types
- Retrieves member types from the schema
- Provides clear error messages for missing implementations
- Handles recursive field references appropriately
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
287-337: Good use of the Strategy pattern for visitor selectionThe introduction of the
PlanVisitorinterface and conditional visitor selection based on federation configurations is a clean implementation of the Strategy pattern. This allows for better separation of concerns between standard and federation-aware planning.v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (13)
3-9: LGTM!The new imports are appropriate for the refactored functionality -
errorsfor explicit error creation andplanfor federation configuration types.
29-44: LGTM!The struct refactoring properly centralizes planning logic through
planCtxand adds federation support while removing entity-specific fields that are now handled by the dedicated federation visitor.
46-71: LGTM!The config struct and constructor properly handle the new federation configurations and maintain consistent naming conventions.
73-76: LGTM!Clean getter method following Go conventions.
77-83: LGTM!Proper initialization of the planning context with all required dependencies.
120-135: LGTM!Proper delegation of message creation to the planning context.
212-234: LGTM!Good improvement with proper error handling for RPC method resolution and clean delegation to the planning context.
236-244: LGTM!Improved readability with
InRootField()check and proper error propagation.
246-249: LGTM!Clear error message for unsupported
_entitiesfield, properly directing to the federation-specific visitor.
265-280: LGTM!Proper delegation to planning context with comprehensive error handling for field building.
285-302: LGTM!Cleaner implementation with improved readability using
InRootField()check.
304-361: LGTM!Consistent delegation of data type conversion to the planning context.
418-449: LGTM!Proper delegation to planning context for list type detection and metadata creation with appropriate error handling.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go
Show resolved
Hide resolved
… ludwig/eng-7007-support-of-multiple-and-nested-keys-for-entities
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go
Outdated
Show resolved
Hide resolved
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_federation.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_federation.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: 3
♻️ Duplicate comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (2)
259-263: Clarify field duplication prevention logic.The duplicate field prevention is good, but it would be helpful to understand how
Fields.Exists()handles the comparison between field names and aliases.Line 260: Will we populate both the field and the alias in response?
378-384: Use explicit comparison with ast.InvalidRef.Following the existing comment about using explicit checks, this method should compare explicitly with
ast.InvalidRef.Better explicit check == ast.InvalidRef
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (2)
15-34: Consider adding documentation for exported types.The
entityInfoandfederationConfigDatatypes lack documentation comments. Adding godoc comments would improve code maintainability.+// entityInfo contains the information about the entity that is being looked up. +// It tracks the AST references for the root field and inline fragment associated with an entity. type entityInfo struct { typeName string entityRootFieldRef int entityInlineFragmentRef int } +// federationConfigData holds the parsed federation configuration for a specific entity type, +// including key fields and required fields selection sets. type federationConfigData struct { entityTypeName string keyFields string requiredFields string }
125-147: Consider extracting reset logic to a helper method.The planInfo reset logic in
LeaveInlineFragmentis quite verbose. Consider extracting it to a helper method for better readability.+func (r *rpcPlanVisitorFederation) resetPlanningInfo() { + r.planInfo = planningInfo{ + operationType: r.planInfo.operationType, + operationFieldName: r.planInfo.operationFieldName, + currentRequestMessage: &RPCMessage{}, + currentResponseMessage: &RPCMessage{}, + currentResponseFieldIndex: 0, + responseMessageAncestors: []*RPCMessage{}, + responseFieldIndexAncestors: []int{}, + } +} + func (r *rpcPlanVisitorFederation) LeaveInlineFragment(ref int) { if r.entityInfo.entityInlineFragmentRef != ref { // We only handle the entity inline fragment return } r.plan.Calls = append(r.plan.Calls, *r.currentCall) r.currentCall = &RPCCall{} r.currentCallIndex++ - r.planInfo = planningInfo{ - operationType: r.planInfo.operationType, - operationFieldName: r.planInfo.operationFieldName, - currentRequestMessage: &RPCMessage{}, - currentResponseMessage: &RPCMessage{}, - currentResponseFieldIndex: 0, - responseMessageAncestors: []*RPCMessage{}, - responseFieldIndexAncestors: []int{}, - } + r.resetPlanningInfo() r.entityInfo.entityInlineFragmentRef = ast.InvalidRef }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/astvisitor/visitor.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/astvisitor/visitor.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (7)
1-13: LGTM: Clean imports and package declaration.The imports are well-organized and include all necessary dependencies for AST manipulation, text processing, and error handling.
76-84: LGTM: Clean planning method with proper error handling.The method correctly uses the AST walker and handles errors appropriately by returning them wrapped with context.
95-102: Good restriction to query operations only.The check for query-only operations is appropriate for federation scenarios, and the error message is clear and descriptive.
185-219: Robust composite type handling with good error messages.The method correctly handles interfaces and unions, with clear error messages when definitions are missing. The recursive call for field nodes is a smart approach.
279-288: Good handling of inline fragment field selection.The logic for handling fields inside non-entity inline fragments is well-structured, properly checking for the entity inline fragment and building field selection sets.
386-418: Well-structured federation config parsing.The parsing logic correctly handles multiple configurations per entity type and distinguishes between key fields and required fields based on the presence of
FieldName.
331-366: Fields Alignment with Federation Spec Confirmed
The JSON paths"representations"and"_entities"correctly map to the_entities(representations: …)query per the GraphQL Federation spec. The RPC field names"keys"(for inputs) and"result"(for outputs) are internal to the gRPC service and don’t need to mirror the GraphQL field names. No changes required here.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
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_federation.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go (1)
457-463: Fix loop-variable capture in sub-tests
ttis reused by the range loop; witht.Parallel()the closure may run aftertthas advanced, leading to flaky tests. Capture the current value before spawning the goroutine.-for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { +for _, tc := range tests { + tt := tc // capture + t.Run(tt.name, func(t *testing.T) {
♻️ Duplicate comments (2)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
382-405: Large block of duplicated mapping – see earlier refactor noteSame duplication issue as above; please consolidate.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
54-55: Document magic constant48The slice-capacity hint
astvisitor.NewWalker(48)still lacks context. Add a brief comment (or make it a namedconst) explaining why 48 is appropriate here.
🧹 Nitpick comments (5)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
796-817: Minor: avoid linear search on every element
slices.Containsruns a linear scan for each array element inbuildProtoMessage.
For large lists this is O(N²). Consider amap[string]struct{}for constant-time look-ups.Nit only; no action required if list sizes are known small.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (3)
130-137: Stop repeating the same GRPCMapping in every testThese seven lines are repeated verbatim in several test cases. Factor them out into a helper (you already have
testMapping()for most tests) to keep the spec single-sourced and avoid future drift.
201-204: ArgumentMappings can move to the schema-level helper tooThe
filter -> filtermapping is also duplicated. Once you introduce the shared helper (previous comment) include these argument mappings there as well to prevent silent inconsistencies later.
1862-1902: Minor: long alias list – add table-driven helperThe 40-line validation block is almost identical to the previous test except that every field is aliased. A tiny helper that takes a slice of expected keys would shrink the test body and make intent clearer.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
325-328: Include key details in error messageWhen no RPC config is found, include the key fields searched for to aid debugging, e.g.:
- return fmt.Errorf("entity type %s not found in mapping", fc.entityTypeName) + return fmt.Errorf("no RPC config for entity %s with key %q", fc.entityTypeName, fc.keyFields)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
v2/pkg/engine/datasource/grpc_datasource/compiler.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(8 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_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/execution_plan.go
📚 Learning: 2025-07-18T11:18:37.946Z
Learnt from: jensneuse
PR: wundergraph/graphql-go-tools#1237
File: v2/pkg/astparser/tokenizer.go:54-120
Timestamp: 2025-07-18T11:18:37.946Z
Learning: In v2/pkg/astparser/tokenizer.go, the depth tracking logic using globalDepth, localDepth, and localDepthPeak is intentional and serves a specific design purpose. The combination of these variables and the calculation of total depth as globalDepth + localDepthPeak is by design, not a bug.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
471-490: Optional-flag may be computed on wrong level
createListMetadatarecordsOptionalbefore advancing to the inner list
type. For nested lists this writes the same flag for every depth.If the outer list is non-null but the inner is nullable (or vice-versa) the
metadata becomes incorrect. Capture the flag after resolving each nested
type:for i := 0; i < nestingLevel; i++ { - md.LevelInfo[i] = LevelInfo{ - Optional: !r.definition.TypeIsNonNull(typeRef), - } typeRef = r.definition.ResolveNestedListOrListType(typeRef) + md.LevelInfo[i] = LevelInfo{ + Optional: !r.definition.TypeIsNonNull(typeRef), + }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
501-502: Nil passed tonewJSONBuilderrisks a panic
newJSONBuilder(nil, …)suggests the first arg is a non-nullable dependency (likely a logger, codec or plan).
If the implementation dereferences it the test will panic once code paths change.
Consider passing an explicit stub/mock instead ofnil.
3513-3521:SelectionSetshould include braces
SelectionSet: "id"is passed to the federation config.
The planner expects a GraphQL selection set (e.g." { id name } ").
A bareidmay fail during parsing or at runtime.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
1862-1902: Rename test for clarity; matches an earlier reviewer’s concernThe name "Query nullable fields type with all aliased fields" is misleading. It reads like “all fields then all aliases”. Consider something like "Query nullable fields type with aliased required and optional fields".
- name: "Query nullable fields type with all aliased fields", + name: "Query nullable fields type with aliased required and optional fields",
🧹 Nitpick comments (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go (1)
54-57: Add context for the magic constant48
astvisitor.NewWalker(48)is used across the codebase but without explanation.
A one-line comment here (e.g. “// 48 = expected max AST depth”) preserves institutional knowledge for future maintainers.v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
796-817: Minor: optimise membership test
isAllowedForTypenamedoes a linearslices.Containsfor every element.
IfMemberTypescan be large or the loop hot, convert once to amap[string]struct{}and do O(1) look-ups.
Purely a micro-optimisation—consider only if profiling shows this as a hotspot.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (3)
279-286: Reduce duplication: factor repeated GRPCMapping into a helperThe Service/QueryRPCs mapping for complexFilterType is duplicated across tests. Consider extracting a shared helper (e.g., mappingForComplexFilterType()) to keep tests DRY and easier to maintain.
3504-3504: Rename the test case for accuracyThe case name mentions "nullable fields type" but it's testing federation entities. Rename for clarity.
- name: "Query nullable fields type with all fields", + name: "Entities lookup returns Product and Storage in input order",
3523-3532: Nit: assert map type before Contains for clearer failuresrequire.Contains works on maps, but asserting the type first yields clearer error messages.
- require.Contains(t, entities[0], "id") - require.Contains(t, entities[0], "name") + e0, ok := entities[0].(map[string]interface{}) + require.True(t, ok, "entities[0] should be an object") + require.Contains(t, e0, "id") + require.Contains(t, e0, "name")Repeat similarly for entities[1], entities[2], entities[3].
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
121-129: Gracefully handle missing_entitiesfieldIf either side of the merge has no
_entitiesarray,Array()returns an error and the whole operation fails.
Consider treating a missing field as an empty array instead of an error so partial results can still be merged.v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go (1)
457-476: Tests become unmaintainable – extract a shared plan builderThe huge literal
RPCExecutionPlanstructs duplicated across test cases make the file ~1.1 k lines.
A single mapping or field change forces manual updates in many places and obscures the intent of each test.Suggest introducing a small DSL / helper such as
planBuilder().entity("Product").key("id").field("name")…or compare against a golden JSON dump produced byplan.String().
This keeps the focus on what differs between cases (keys, order, nested fields) and avoids brittle copy-paste errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
v2/pkg/engine/datasource/grpc_datasource/compiler.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(8 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-18T11:18:37.946Z
Learnt from: jensneuse
PR: wundergraph/graphql-go-tools#1237
File: v2/pkg/astparser/tokenizer.go:54-120
Timestamp: 2025-07-18T11:18:37.946Z
Learning: In v2/pkg/astparser/tokenizer.go, the depth tracking logic using globalDepth, localDepth, and localDepthPeak is intentional and serves a specific design purpose. The combination of these variables and the calculation of total depth as globalDepth + localDepthPeak is by design, not a bug.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
🧬 Code Graph Analysis (2)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
RPCMessage(71-85)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (5)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (7)
DataTypeString(21-21)Document(92-99)DataType(17-17)DataTypeUnknown(31-31)DataTypeMessage(30-30)DataTypeEnum(29-29)Service(102-106)v2/pkg/engine/datasource/grpc_datasource/configuration.go (2)
GRPCMapping(16-31)RPCConfig(45-52)v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)v2/pkg/ast/ast_type.go (5)
Type(21-29)TypeKind(12-12)TypeKindNamed(16-16)TypeKindList(17-17)TypeKindNonNull(18-18)pkg/ast/ast_node_kind.go (5)
NodeKindInterfaceTypeDefinition(16-16)NodeKindUnionTypeDefinition(18-18)NodeKindObjectTypeDefinition(14-14)NodeKindInputObjectTypeDefinition(24-24)NodeKindEnumTypeDefinition(21-21)
🔇 Additional comments (11)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
3-7:slicesrequires Go 1.21 – verify toolchainThe std-lib
slicespackage was introduced in Go 1.21. If the project still targets 1.20 or lower the build will fail.
Confirm the minimum Go version ingo.modand CI images, or switch togolang.org/x/exp/slices.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
80-90: SharedjsonBuilderused concurrently – confirm thread-safety
builder.marshalResponseJSONis executed inside goroutines whilejsonBuilderis shared. IfjsonBuilderholds mutable state this is unsafe.Either:
- Instantiate a new builder per goroutine, or
- Guard all builder access with the existing mutex.
Please verify the builder implementation.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (7)
14-17: LGTM: new imports are correct and usedgjson is used by jsonBuilder; plan is used for FederationFieldConfigurations. No issues.
130-137: Explicit GRPCMapping for complexFilterType looks goodService and RPC wiring align with the mock Invoke check on method suffix. OK.
189-196: Align test with the TODO (avoid mappings) or update the TODOThis test still configures Mapping despite the TODO (Line 159) stating it should not use mappings and expect no response. Please either remove Mapping here (and assert empty data) or update the TODO accordingly to avoid confusion.
201-203: Verify ArgumentMappings vs variables shapeYou map GraphQL arg "filter" to request field "filter". The variables payload nests "filter" twice (filter.filter...). Please confirm this aligns with the expected proto request shape; otherwise, simplify the variables to a single "filter".
291-293: LGTM: ArgumentMappings are consistentThe "filter" → "filter" mapping matches the field naming; no issues.
381-405: LGTM: user QueryRPC mappingThe mapping for the user query is clear and pairs with the gRPC error test. Looks good.
501-503: LGTM: jsonBuilder-based marshalingSwitching to jsonBuilder with an empty gjson.Result is consistent with the refactor. The expected JSON output matches the configured JSONPaths.
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
386-391: Potential int64 overflow on 32-bit Go builds
I searched the CI workflows (.github/workflows), Makefiles, and README for anyGOARCH=386or 32-bit build targets and found none. It appears the project only tests 64-bit binaries.
If 32-bit platforms are not a supported target, the int→int64 overflow risk on 32-bit Go is moot. Otherwise, please confirm whether we intend to support 32-bit builds so we can update these cases to useNewNumberString(strconv.FormatInt(…))forInt64Kind(and the analogous change in arrays) as proposed.v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go (1)
1046-1092:runFederationTestdoes not close overttsafely when used in loopThe helper is invoked inside the loop after taking the address of
tt, but the variable is reused on each iteration; only the last value could leak into goroutines spawned byt.Parallel(). Passttby value:for _, tc := range tests { tc := tc // shadow runFederationTest(t, tc) }Although the current helper immediately copies fields, guarding now avoids subtle future races.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
302-328: Constructor retains behaviour; minor nit on nil mapping guard
NewPlannerdefensively replaces a nil mapping with an empty struct, preventing nil-pointer panics—good.
As the function already does this, the earlierif mapping == nil {}guard scattered in helpers may now be redundant; consider pruning to keep the codebase DRY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
107-118: 👍 Duplicate-__typenamedefense looks good
AppendTypeNameFieldnow checks for an existing__typenamebefore appending, eliminating the duplicate-field bug raised in earlier reviews. Implementation is concise and safe for a nil slice.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (2)
370-372: Safer base case for nested list traversal
if level > md.NestingLevelis odd given 0-based traversal. Use>=to prevent accidental out-of-bounds access onLevelInfo[level].- if level > md.NestingLevel { + if level >= md.NestingLevel { return current, nil }
556-561: Data envelope allocates a fresh arena; consider using caller arena to avoid cross-arena references
toDataObjectcreates a newastjson.Arenaand insertsroot(allocated elsewhere). This mixes arenas. Prefer building the envelope in the same arena asroot(or accept an arena parameter).Example:
-func (j *jsonBuilder) toDataObject(root *astjson.Value) *astjson.Value { - a := astjson.Arena{} - data := a.NewObject() - data.Set(dataPath, root) - return data -} +func (j *jsonBuilder) toDataObject(arena *astjson.Arena, root *astjson.Value) *astjson.Value { + data := arena.NewObject() + data.Set(dataPath, root) + return data +}If call-site refactors are non-trivial, confirm cross-arena safety of
astjson.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (4)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (4)
485-486: Verify bytes encoding aligns with GraphQL clients
BytesKindis serialized viaNewStringBytes. Ensure this produces the expected (typically base64) encoding for GraphQL binary data.
- If it writes raw bytes as UTF-8, encode to base64 first (or map to a custom scalar).
- If
NewStringBytesalready base64-encodes, ignore this comment.Also applies to: 533-534
566-593: LGTM: GraphQL error envelope with gRPC code in extensionsError formatting looks solid:
errorsarray withmessageandextensions.codepopulated from gRPC status, defaulting to INTERNAL. Returning bytes afterMarshalTo(nil)makesdefer a.Reset()safe.
96-106: Lists/nullability handling aligns with our protobuf-wrapper patternThe use of wrapper messages for nested/nullable lists and the nullability checks in
flattenListStructurematch our established approach (only non-null lists use repeated fields directly; others use wrappers).Also applies to: 342-364
151-183: Confirm astjson cross-arena safety or refactor mergeEntitiesCurrently,
mergeEntitiesdoes:
- Creates a fresh
astjson.Arena{}- Inserts
*astjson.Valueitems (lr,rr) allocated in other arenasUnless
astjsonguarantees that values from one arena remain valid when stored in another—and won’t be freed or invalidated when the source arena is garbage-collected—this risks dangling references.Please verify whether
astjsonsupports cross-arena value reuse. If not, adopt one of these refactors:
- Allocate in a caller-provided or shared arena (e.g. pass
*astjson.ArenaintomergeEntities).- Clone each item into the destination arena using an
astjson.Cloneor equivalent method.- Change the signature to
mergeEntities(arena *astjson.Arena, left, right *astjson.Value) (*astjson.Value, error)and build the result inside that arena.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (5)
552-559: Same arena-lifetime bug intoDataObject
a := astjson.Arena{}is stack-bound; the returned JSON value holds a dangling pointer.
Create / pass a long-lived arena instead.
213-217: Slice aliasing mutates sharedmessage.Fieldsslice
validFields := message.Fieldsfollowed byappend(validFields, …)can corruptmessage.Fieldsfor future calls.
Make a copy before appending:validFields := append([]*FieldDescriptor{}, message.Fields...)
477-479: Int64 cast tointrisks overflow & violates GraphQL 32-bit IntSplit the cases: keep
Int32cast toint, but encodeInt64as string (or range-check).
See prior review for full diff.
524-526: Same Int64 overflow issue for array itemsMirror the scalar fix here to prevent overflow.
152-180: Use-after-free: returning a value bound to a stack-allocated Arena
root := astjson.Arena{}is a stack value; every*astjson.Valueallocated inside keeps a pointer to that arena.
When the function returns, the arena object goes out of scope and the returned pointer dereferences invalid memory – exactly the bug highlighted in previous reviews.Allocate/receive an arena that outlives the result:
-func (j *jsonBuilder) mergeEntities(left *astjson.Value, right *astjson.Value) (*astjson.Value, error) { - root := astjson.Arena{} +func (j *jsonBuilder) mergeEntities(arena *astjson.Arena, left, right *astjson.Value) (*astjson.Value, error) {…and create objects via
arena, or make the caller pass one in.
Failing to fix this will crash as soon as the merged value is read.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
37-58: Linear search ingetResultIndexis O(n²) overall
getResultIndexscans the slice on every call; with large entity sets this becomes quadratic.
Consider pre-building amap[int]intper type (representationIndex ➜ resultIndex) for O(1) look-ups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
Noroth
deleted the
ludwig/eng-7007-support-of-multiple-and-nested-keys-for-entities
branch
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.219](v2.0.0-rc.218...v2.0.0-rc.219) (2025-08-08) ### Features * add support for multiple key directives ([#1262](#1262)) ([8535a92](8535a92)) --- 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 support for multiple key directives. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.219. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores
Checklist