feat: redesign handling for lists in gRPC #1246
Conversation
Noroth
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughThis update introduces extensive support for complex nested list types and new domain entities (BlogPost and Author) across the gRPC datasource, protobuf schema, test suites, and mapping/configuration layers. It refactors internal list handling, enhances nullable and repeated field management, and adds comprehensive tests for queries and mutations involving these new structures. Numerous new RPCs, message types, and mapping rules are defined to support CRUD operations and filtering for BlogPost and Author entities. Changes
Estimated code review effort5 (Critical) | ⏱️ ~180+ minutes This is a large, high-complexity update with extensive new features, refactoring, and comprehensive test coverage across many files and domains (schema, proto, mapping, core logic, and tests). Review will require careful attention to list/nullability handling, new entity support, and correctness of mapping and test logic. Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsv2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (2)Learnt from: Noroth Learnt from: SkArchon 🧬 Code Graph Analysis (1)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
⏰ 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)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go (1)
1-327: Well-structured test for entity lookup RPC plan generation.The test effectively validates the generation of RPC execution plans for GraphQL entity lookups, with proper request/response message structures and field mappings.
Note: The TODO comment at line 105 indicates planned support for multiple entity lookup types. Consider creating a GitHub issue to track this enhancement.
Would you like me to create an issue to track the implementation of multiple entity lookup types?
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
278-281: Consider making the field number configurable.The implementation assumes list wrappers always use field number 1. This could be a maintenance issue if the protobuf schema changes.
Consider adding a field number parameter to ListMetadata or making it configurable through the mapping configuration to improve flexibility and maintainability.
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
512-604: Complex but correct nested list handlingThe
buildListMessageandtraverseListfunctions correctly implement the recursive logic for handling nested list structures. The code properly manages optional lists and different data types.Consider adding more detailed documentation explaining the protobuf wrapper convention (field number 1) and the traversal algorithm for future maintainers.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
590-607: Well-implemented list metadata extractionThe
createListMetadatafunction correctly analyzes GraphQL type structures to extract nesting levels and optionality information.Consider adding a comment explaining the relationship between GraphQL's type wrapping and the resulting metadata structure for future maintainers.
v2/pkg/grpctest/mockservice.go (1)
1874-1904: Consider if UpdateBlogPost should update complex fields.The current implementation only updates simple fields and lists but doesn't update complex fields like RelatedCategories, Contributors, MentionedProducts, etc.
If this is intentional for testing partial updates, it's fine. Otherwise, you might want to add conversion logic similar to the create method.
Would you like me to add the missing field updates to match the create operation's behavior, or is this partial update intentional for testing purposes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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 (18)
execution/engine/execution_engine_grpc_test.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/compiler.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/compiler_test.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_composite_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_nullable_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(8 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(6 hunks)v2/pkg/grpctest/Makefile(1 hunks)v2/pkg/grpctest/mapping/mapping.go(6 hunks)v2/pkg/grpctest/mockservice.go(10 hunks)v2/pkg/grpctest/product.proto(10 hunks)v2/pkg/grpctest/schema.go(4 hunks)v2/pkg/grpctest/testdata/products.graphqls(4 hunks)
🧠 Learnings (6)
v2/pkg/engine/datasource/grpc_datasource/compiler_test.go (1)
Learnt from: SkArchon
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_nullable_test.go (1)
Learnt from: SkArchon
PR: #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.
execution/engine/execution_engine_grpc_test.go (1)
Learnt from: SkArchon
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
Learnt from: Noroth
PR: #1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.568Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
Learnt from: SkArchon
PR: #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.
v2/pkg/grpctest/product.proto (1)
Learnt from: Noroth
PR: #1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.568Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
v2/pkg/grpctest/mockservice.go (2)
Learnt from: SkArchon
PR: #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.
Learnt from: SkArchon
PR: #1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
🧬 Code Graph Analysis (6)
execution/engine/execution_engine_grpc_test.go (1)
v2/pkg/grpctest/mapping/mapping.go (1)
DefaultGRPCMapping(10-954)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (7)
RPCExecutionPlan(49-54)RPCCall(58-71)RPCMessage(75-89)RPCFields(231-231)ListMetadata(181-186)LevelInfo(189-192)RPCField(150-178)v2/pkg/engine/datasource/grpc_datasource/compiler.go (7)
DataTypeMessage(29-29)Message(117-121)DataTypeString(20-20)DataTypeEnum(28-28)DataTypeInt32(21-21)DataTypeDouble(26-26)DataTypeBool(27-27)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
Message(117-121)DataTypeMessage(29-29)pkg/introspection/introspection.go (1)
TypeName(42-44)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
ListMetadata(181-186)RPCField(150-178)LevelInfo(189-192)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
ListMetadata(181-186)RPCMessage(75-89)LevelInfo(189-192)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
Message(117-121)v2/pkg/graphqljsonschema/jsonschema.go (1)
NewNull(426-430)
v2/pkg/grpctest/mockservice.go (2)
v2/pkg/grpctest/productv1/product.pb.go (69)
CategoryInput(6979-6985)CategoryInput(6998-6998)CategoryInput(7013-7015)Category(4861-4868)Category(4881-4881)Category(4896-4898)ListOfCategoryInput(216-221)ListOfCategoryInput(234-234)ListOfCategoryInput(249-251)ListOfUserInput(711-716)ListOfUserInput(729-729)ListOfUserInput(744-746)User(4345-4351)User(4364-4364)User(4379-4381)ListOfListOfUserInput(486-491)ListOfListOfUserInput(504-504)ListOfListOfUserInput(519-521)ListOfListOfUser(441-446)ListOfListOfUser(459-459)ListOfListOfUser(474-476)ListOfListOfUser_List(7163-7168)ListOfListOfUser_List(7181-7181)ListOfListOfUser_List(7196-7198)ListOfUser(666-671)ListOfUser(684-684)ListOfUser(699-701)ListOfListOfCategoryInput(351-356)ListOfListOfCategoryInput(369-369)ListOfListOfCategoryInput(384-386)ListOfListOfCategory(306-311)ListOfListOfCategory(324-324)ListOfListOfCategory(339-341)ListOfListOfCategory_List(7031-7036)ListOfListOfCategory_List(7049-7049)ListOfListOfCategory_List(7064-7066)ListOfCategory(171-176)ListOfCategory(189-189)ListOfCategory(204-206)BlogPost(5365-5389)BlogPost(5402-5402)BlogPost(5417-5419)ListOfString(621-626)ListOfString(639-639)ListOfString(654-656)ListOfFloat(261-266)ListOfFloat(279-279)ListOfFloat(294-296)ListOfBoolean(126-131)ListOfBoolean(144-144)ListOfBoolean(159-161)ListOfListOfString(396-401)ListOfListOfString(414-414)ListOfListOfString(429-431)CategoryKind_CATEGORY_KIND_ELECTRONICS(30-30)CategoryKind_CATEGORY_KIND_BOOK(29-29)ListOfProduct(576-581)ListOfProduct(594-594)ListOfProduct(609-611)Product(4225-4232)Product(4245-4245)Product(4260-4262)CategoryKind_CATEGORY_KIND_OTHER(32-32)Author(5621-5640)Author(5653-5653)Author(5668-5670)ListOfBlogPost(81-86)ListOfBlogPost(99-99)ListOfBlogPost(114-116)execution/federationtesting/accounts/graph/model/models_gen.go (2)
Name(43-46)Title(74-77)
🪛 checkmake (0.2.2)
v2/pkg/grpctest/Makefile
[warning] 12-12: Target "PHONY" should be declared PHONY.
(phonydeclared)
[warning] 16-16: Target "PHONY" should be declared PHONY.
(phonydeclared)
🧰 Additional context used
🧠 Learnings (6)
v2/pkg/engine/datasource/grpc_datasource/compiler_test.go (1)
Learnt from: SkArchon
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_nullable_test.go (1)
Learnt from: SkArchon
PR: #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.
execution/engine/execution_engine_grpc_test.go (1)
Learnt from: SkArchon
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
Learnt from: Noroth
PR: #1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.568Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
Learnt from: SkArchon
PR: #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.
v2/pkg/grpctest/product.proto (1)
Learnt from: Noroth
PR: #1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.568Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
v2/pkg/grpctest/mockservice.go (2)
Learnt from: SkArchon
PR: #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.
Learnt from: SkArchon
PR: #1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
🧬 Code Graph Analysis (6)
execution/engine/execution_engine_grpc_test.go (1)
v2/pkg/grpctest/mapping/mapping.go (1)
DefaultGRPCMapping(10-954)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (7)
RPCExecutionPlan(49-54)RPCCall(58-71)RPCMessage(75-89)RPCFields(231-231)ListMetadata(181-186)LevelInfo(189-192)RPCField(150-178)v2/pkg/engine/datasource/grpc_datasource/compiler.go (7)
DataTypeMessage(29-29)Message(117-121)DataTypeString(20-20)DataTypeEnum(28-28)DataTypeInt32(21-21)DataTypeDouble(26-26)DataTypeBool(27-27)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (2)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
Message(117-121)DataTypeMessage(29-29)pkg/introspection/introspection.go (1)
TypeName(42-44)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
ListMetadata(181-186)RPCField(150-178)LevelInfo(189-192)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
ListMetadata(181-186)RPCMessage(75-89)LevelInfo(189-192)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
Message(117-121)v2/pkg/graphqljsonschema/jsonschema.go (1)
NewNull(426-430)
v2/pkg/grpctest/mockservice.go (2)
v2/pkg/grpctest/productv1/product.pb.go (69)
CategoryInput(6979-6985)CategoryInput(6998-6998)CategoryInput(7013-7015)Category(4861-4868)Category(4881-4881)Category(4896-4898)ListOfCategoryInput(216-221)ListOfCategoryInput(234-234)ListOfCategoryInput(249-251)ListOfUserInput(711-716)ListOfUserInput(729-729)ListOfUserInput(744-746)User(4345-4351)User(4364-4364)User(4379-4381)ListOfListOfUserInput(486-491)ListOfListOfUserInput(504-504)ListOfListOfUserInput(519-521)ListOfListOfUser(441-446)ListOfListOfUser(459-459)ListOfListOfUser(474-476)ListOfListOfUser_List(7163-7168)ListOfListOfUser_List(7181-7181)ListOfListOfUser_List(7196-7198)ListOfUser(666-671)ListOfUser(684-684)ListOfUser(699-701)ListOfListOfCategoryInput(351-356)ListOfListOfCategoryInput(369-369)ListOfListOfCategoryInput(384-386)ListOfListOfCategory(306-311)ListOfListOfCategory(324-324)ListOfListOfCategory(339-341)ListOfListOfCategory_List(7031-7036)ListOfListOfCategory_List(7049-7049)ListOfListOfCategory_List(7064-7066)ListOfCategory(171-176)ListOfCategory(189-189)ListOfCategory(204-206)BlogPost(5365-5389)BlogPost(5402-5402)BlogPost(5417-5419)ListOfString(621-626)ListOfString(639-639)ListOfString(654-656)ListOfFloat(261-266)ListOfFloat(279-279)ListOfFloat(294-296)ListOfBoolean(126-131)ListOfBoolean(144-144)ListOfBoolean(159-161)ListOfListOfString(396-401)ListOfListOfString(414-414)ListOfListOfString(429-431)CategoryKind_CATEGORY_KIND_ELECTRONICS(30-30)CategoryKind_CATEGORY_KIND_BOOK(29-29)ListOfProduct(576-581)ListOfProduct(594-594)ListOfProduct(609-611)Product(4225-4232)Product(4245-4245)Product(4260-4262)CategoryKind_CATEGORY_KIND_OTHER(32-32)Author(5621-5640)Author(5653-5653)Author(5668-5670)ListOfBlogPost(81-86)ListOfBlogPost(99-99)ListOfBlogPost(114-116)execution/federationtesting/accounts/graph/model/models_gen.go (2)
Name(43-46)Title(74-77)
🪛 checkmake (0.2.2)
v2/pkg/grpctest/Makefile
[warning] 12-12: Target "PHONY" should be declared PHONY.
(phonydeclared)
[warning] 16-16: Target "PHONY" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (50)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
171-192: LGTM! Well-structured list metadata implementation.The additions provide a clean way to represent nested list structures with optionality at each level. The
IsOptionalScalarmethod correctly checks for optional non-message types.Also applies to: 224-227
v2/pkg/engine/datasource/grpc_datasource/compiler_test.go (1)
296-449: Excellent test coverage for nested list compilation.The test thoroughly validates the compilation of nested lists with metadata, including proper verification of the protobuf message structure using reflection. The test case with modifiers field demonstrates the list metadata functionality well.
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (1)
107-146: Comprehensive and consistent mapping additions.The new mappings for BlogPost and Author entities are well-structured and follow the established pattern. All field mappings correctly use snake_case conversion for gRPC compatibility.
Also applies to: 169-188, 310-345, 373-398, 659-671, 732-944
execution/engine/execution_engine_grpc_test.go (2)
537-537: Verify the expected float value change.The expected float value was changed from
12.345to12.34. Please confirm this matches the actual test data returned by the mock service.
813-1441: Excellent comprehensive test coverage for nested list features.The new tests thoroughly validate the BlogPost and Author entities with:
- Scalar and nested scalar lists
- Complex object lists and nested complex lists
- Query operations (by ID, filtered, all)
- Mutation operations with complex input structures
The tests follow consistent patterns and provide proper assertions for JSON responses.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (3)
428-430: Good addition of existence check for proto message.This change improves test robustness by verifying that the "Product" message exists before accessing its descriptor, preventing potential nil pointer dereference.
436-438: Consistent error handling for response message.Good to see the same defensive check applied here for the response message, maintaining consistency with the previous check.
2092-2941: Excellent comprehensive test coverage for nested lists functionality.This test thoroughly validates the gRPC datasource's ability to handle complex nested list structures with various nullability combinations. The table-driven approach with clear test cases makes it easy to understand and maintain. Good coverage of:
- Different list nullability patterns (required/optional lists with required/optional items)
- Scalar and complex type lists
- Nested list structures
- Query and mutation operations
- Proper validation of response data
v2/pkg/engine/datasource/grpc_datasource/execution_plan_nullable_test.go (1)
7-645: LGTM! Comprehensive test coverage for nullable fields.The test suite provides thorough coverage of nullable field handling in execution plans, including:
- Nullable fields in responses and inputs
- Queries with filters containing nullable fields
- Mutations for creating and updating entities with nullable fields
- Partial field selections
The test structure follows best practices with parallel execution and clear test case naming.
v2/pkg/grpctest/mapping/mapping.go (1)
114-951: LGTM! Well-structured mapping additions.The new mappings for BlogPost and Author entities are comprehensive and follow the established patterns:
- Query, mutation, and field mappings are properly defined
- Naming conventions are consistent (camelCase to snake_case)
- All necessary CRUD operations are covered
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (1)
5-1012: LGTM! Excellent test coverage for list handling.The test suite thoroughly covers list handling scenarios including:
- Simple repeated fields
- Nullable lists with proper metadata
- Nested lists with multiple nesting levels
- List parameters in queries and mutations
- Complex nested input objects
The ListMetadata structure is well-tested with proper nesting levels and optionality information.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_composite_test.go (1)
13-896: LGTM! Comprehensive composite type testing.The test suite excellently covers interface and union type handling:
- Interface types with inline fragments and member type selections
- Union types with partial and complete member selections
- Both query and mutation scenarios
- Proper handling of FieldSelectionSet for type-specific fields
The use of
cmp.Diffprovides clear test failure messages.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (3)
221-230: LGTM! Clean integration of list type handling.The new list handling is well-integrated into the existing flow, properly checking for
IsListTypebefore invoking the flattening logic.
272-331: Well-implemented recursive list traversal.The
traverseListmethod correctly handles:
- Multiple nesting levels with proper bounds checking
- Optional list levels with appropriate null/empty array returns
- Both scalar and message type elements
- Recursive traversal for nested structures
The implementation is robust and handles edge cases appropriately.
336-336: Good error message improvement.Adding quotes around the field name in the error message improves clarity.
v2/pkg/grpctest/testdata/products.graphqls (3)
179-212: Well-structured type definition with comprehensive list handlingThe
BlogPosttype demonstrates excellent handling of various list nullability patterns with clear documentation. The nested list structures are properly defined and the comments effectively explain the nullability semantics.
213-239: Clean implementation of Author type with team structuresThe
Authortype effectively models team organization through nested lists, with appropriate nullability choices for different fields. The structure aligns well with the domain model.
344-354: Standard CRUD operations following GraphQL conventionsThe new queries and mutations for
BlogPostandAuthorentities follow consistent naming patterns and properly utilize input types and filters.Also applies to: 371-377
v2/pkg/engine/datasource/grpc_datasource/compiler.go (4)
215-223: Good improvement to follow Go idiomsChanging
MessageByNameto return(Message, bool)follows Go best practices for existence checking, allowing callers to properly handle missing messages.
339-347: Proper error handling for missing messagesThe code correctly uses the new
MessageByNamesignature to check for message existence and provides clear error messages.
448-487: Comprehensive handling of wrapped message typesThe enhanced switch statement properly handles nested lists, optional scalars, and standard messages with appropriate error checking for required fields and metadata.
606-621: Clean enum value extractionThe
getEnumValuehelper method provides a clean abstraction for enum value lookup with proper error handling.v2/pkg/grpctest/schema.go (2)
263-350: Consistent field configuration additionsThe new field configurations for BlogPost and Author operations follow the established pattern with proper argument source type specifications.
396-403: Complete metadata registration for new typesAll new types (BlogPost, Author, and related input/filter types) are properly registered in both root nodes and child nodes metadata, maintaining consistency with the GraphQL schema.
Also applies to: 413-416, 684-787
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
327-340: Good refactoring with proper inline fragment handlingExtracting the field building logic to
buildFieldimproves code organization. The inline fragment handling correctly separates fragment fields into a dedicated selection set.
342-379: Comprehensive field construction logicThe
buildFieldmethod properly handles all field types including nullable/nested lists, enums, and special GraphQL fields like__typename.
457-487: Consistent list handling for input fieldsThe input field construction properly mirrors the output field logic, maintaining consistency in how nullable and nested lists are handled throughout the codebase.
Also applies to: 562-584
586-588: Clean type checking abstractionsThe helper methods (
typeIsNonNullList,unwrapListNesting,typeIsNullableOrNestedList) provide clear abstractions for GraphQL type system navigation.Also applies to: 609-623, 902-912
v2/pkg/grpctest/product.proto (5)
14-50: Well-organized RPC service extensionsThe new RPC methods follow consistent naming conventions and provide comprehensive CRUD operations for the new domain entities.
52-140: Consistent wrapper message pattern for list handlingThe list wrapper messages follow a uniform pattern that enables proper nullable and nested list support in protobuf. The use of field number 1 for "items" is consistent throughout.
584-711: Well-structured domain message typesThe new message types correctly implement nullable fields using protobuf wrappers and maintain consistency between output types, input types, and filters. The use of
DoubleValuefor GraphQL Float fields is appropriate.
779-782: Appropriate simplification of CategoryInputThe removal of nullable wrappers and addition of the
kindfield aligns CategoryInput with its GraphQL schema definition where all fields are required.
550-550: Breaking changes to existing protobuf fieldsThe following fields were changed from direct types to wrapper types, which is a breaking change for existing clients. Please ensure backward compatibility is handled or document a clear migration path:
v2/pkg/grpctest/product.proto:550
Order.order_lines:repeated OrderLine→ListOfOrderLinev2/pkg/grpctest/product.proto:573
SearchInput.limit:int32→google.protobuf.Int32Valuev2/pkg/grpctest/product.proto:738
OrderLineInput.modifiers:repeated string→ListOfStringv2/pkg/grpctest/product.proto:744
OrderLine.modifiers:repeated string→ListOfString– Ensure clients depending on these fields are updated accordingly or provide a documented migration strategy.
v2/pkg/grpctest/mockservice.go (17)
21-122: LGTM! Well-structured helper functions for input-to-output type conversion.These helper functions effectively handle the conversion from input types to output types with proper nil checks and consistent ID generation patterns. The implementation aligns well with the mock service's purpose of testing complex nested list structures.
143-143: Good precision improvement for float fields.Upgrading from
wrapperspb.FloatValuetowrapperspb.DoubleValueprovides better precision for floating-point values, which is particularly important for financial or scientific calculations in the mock service.
182-182: Consistent precision upgrade maintained.The change to
wrapperspb.DoubleValuehere maintains consistency with the create operation, ensuring uniform float precision handling across all nullable field operations.
205-205: Complete and consistent float precision upgrade across all query methods.The systematic upgrade to
wrapperspb.DoubleValueacross all query methods ensures uniform handling of optional float fields throughout the mock service. This maintains consistency with the mutation methods and provides better precision for all nullable float operations.Also applies to: 280-280, 314-314, 358-358, 375-375
502-509: Good defensive programming for limit parameter handling.The addition of limit validation and default value handling prevents potential issues with invalid or missing limit values. Using
GetValue()consistently ensures proper access to the wrapped nullable int32 value.
955-958: Correct wrapping of order lines in the expected protobuf type.The change properly wraps the
orderLinesslice in aListOfOrderLinestruct, aligning with the expected protobuf message structure for theOrdertype.
966-1062: Comprehensive BlogPost query implementation with excellent test data coverage.The
QueryBlogPostimplementation provides a thorough example of all supported list types including:
- Simple scalar lists (tags, viewCounts)
- Optional wrapped lists (OptionalTags, Keywords, Ratings)
- Nested lists of strings (TagGroups, RelatedTopics)
- Lists of complex types (RelatedCategories, Contributors)
- Deeply nested structures (CategoryGroups, ContributorTeams)
This serves as an excellent reference implementation for testing the full capabilities of the new nested list support.
1064-1279: Well-designed QueryBlogPostById with comprehensive test scenarios.The implementation provides excellent test coverage with:
- Proper null handling for "not-found" IDs
- "simple" case with minimal required fields
- "complex" case showcasing all optional fields
- Generic fallback for other IDs
The consistent initialization of required fields across all cases ensures the mock service won't cause unexpected null pointer issues during testing.
1281-1376: Robust filter implementation with proper null handling.The
QueryBlogPostsWithFilterimplementation correctly:
- Returns empty results for nil filters
- Implements multi-criteria filtering (title, hasCategories, minTags)
- Generates appropriate test data based on filter values
- Ensures all required fields are populated in filtered results
1378-1464: Good variety in QueryAllBlogPosts for testing optional field handling.The implementation effectively uses modulo operations to create variety in optional field population, which is excellent for testing GraphQL's handling of nullable fields across different blog posts in a single query.
1466-1544: Comprehensive Author query with excellent field variety.The
QueryAuthorimplementation showcases all Author field types including:
- Optional email field using
wrapperspb.StringValue- Simple string arrays (Skills, Languages)
- Nested string lists (TeamsByProject, Collaborations)
- Complex type references (WrittenPosts, FavoriteCategories)
- Deeply nested user and category structures
This provides thorough test coverage for the Author entity's complex schema.
1546-1674: Well-structured QueryAuthorById with appropriate test scenarios.The implementation provides good coverage with:
- "minimal" case ensuring only required fields
- "experienced" case with comprehensive optional data
- Proper handling of empty vs populated optional lists (e.g., Collaborations)
- Consistent required field initialization across all cases
1676-1754: Clean filter implementation with conditional field population.The
QueryAuthorsWithFiltercorrectly implements:
- Conditional TeamsByProject population based on
hasTeamsfilter- Dynamic skill count generation
- Proper empty list initialization when teams are not requested
1756-1823: Good optional field variation in QueryAllAuthors.The implementation effectively demonstrates:
- Conditional email population (odd indices only)
- Sparse socialLinks (every 3rd author)
- Variable collaborations (only author 2)
- Consistent handling of required fields
This variety is excellent for testing GraphQL's nullable field handling.
1826-1872: CreateBlogPost properly uses helper functions for type conversion.The implementation correctly:
- Uses the helper functions to convert input types to output types
- Generates appropriate mock data for fields not in the input (MentionedProducts, MentionedUsers, ContributorTeams)
- Preserves all scalar and list fields from the input
The mixing of converted input data with mock data is appropriate for a test service.
1907-1953: CreateAuthor implementation correctly handles complex type conversions.The implementation properly:
- Uses helper functions to convert FavoriteCategories, AuthorGroups, and ProjectTeams from input types
- Provides sensible mock data for fields not included in the simplified input (WrittenPosts, RelatedAuthors, etc.)
- Maintains consistency with the BlogPost create pattern
1955-1979: UpdateAuthor follows the same partial update pattern as UpdateBlogPost.Like UpdateBlogPost, this only updates simple fields. This consistent behavior across both entity types suggests it's an intentional design choice for the mock service, possibly to test GraphQL's handling of partial updates.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
264-283: Add validation for message parameter.The method should validate the
messageparameter to prevent potential nil pointer dereferences when it's used in themarshalResponseJSONcall withintraverseList.func (d *DataSource) flattenListStructure(arena *astjson.Arena, md *ListMetadata, data protoref.Message, message *RPCMessage) (*astjson.Value, error) { if md == nil { return arena.NewNull(), errors.New("unable to flatten list structure: list metadata not found") } + if message == nil { + return arena.NewNull(), errors.New("unable to flatten list structure: message parameter is nil") + } if len(md.LevelInfo) < md.NestingLevel { return arena.NewNull(), errors.New("unable to flatten list structure: nesting level data does not match the number of levels in the list metadata") }
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
517-585: Add bounds checking for level parameter.While the current implementation should work correctly with valid data, adding defensive bounds checking would prevent potential panics.
func (p *RPCCompiler) traverseList(rootMsg protoref.Message, level int, field Field, rpcField *RPCField, data gjson.Result) protoref.Message { + if level <= 0 || level > len(rpcField.ListMetadata.LevelInfo) { + p.report.AddInternalError(fmt.Errorf("invalid nesting level %d for list metadata", level)) + return nil + } + listFieldDesc := rootMsg.Descriptor().Fields().ByNumber(1)Also, the recursive list building correctly mirrors the flattening logic and handles all data types appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
v2/pkg/grpctest/productv1/product.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
v2/pkg/engine/datasource/grpc_datasource/compiler.go(6 hunks)v2/pkg/engine/datasource/grpc_datasource/compiler_test.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(3 hunks)v2/pkg/grpctest/Makefile(1 hunks)v2/pkg/grpctest/mockservice.go(10 hunks)v2/pkg/grpctest/product.proto(10 hunks)
🧰 Additional context used
🧠 Learnings (4)
v2/pkg/engine/datasource/grpc_datasource/compiler_test.go (1)
Learnt from: SkArchon
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
Learnt from: SkArchon
PR: #1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
v2/pkg/grpctest/product.proto (1)
Learnt from: Noroth
PR: #1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.568Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
v2/pkg/grpctest/mockservice.go (2)
Learnt from: SkArchon
PR: #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.
Learnt from: SkArchon
PR: #1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
🧬 Code Graph Analysis (3)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
ListMetadata(181-186)RPCMessage(75-89)LevelInfo(189-192)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
Message(117-121)v2/pkg/graphqljsonschema/jsonschema.go (1)
NewNull(426-430)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
ListMetadata(181-186)RPCField(150-178)LevelInfo(189-192)
v2/pkg/grpctest/mockservice.go (1)
v2/pkg/grpctest/productv1/product.pb.go (84)
CategoryInput(6979-6985)CategoryInput(6998-6998)CategoryInput(7013-7015)Category(4861-4868)Category(4881-4881)Category(4896-4898)ListOfCategoryInput(216-221)ListOfCategoryInput(234-234)ListOfCategoryInput(249-251)ListOfUserInput(711-716)ListOfUserInput(729-729)ListOfUserInput(744-746)User(4345-4351)User(4364-4364)User(4379-4381)ListOfListOfUserInput(486-491)ListOfListOfUserInput(504-504)ListOfListOfUserInput(519-521)ListOfListOfUser(441-446)ListOfListOfUser(459-459)ListOfListOfUser(474-476)ListOfUser(666-671)ListOfUser(684-684)ListOfUser(699-701)ListOfListOfCategoryInput(351-356)ListOfListOfCategoryInput(369-369)ListOfListOfCategoryInput(384-386)ListOfListOfCategory(306-311)ListOfListOfCategory(324-324)ListOfListOfCategory(339-341)ListOfCategory(171-176)ListOfCategory(189-189)ListOfCategory(204-206)BlogPost(5365-5389)BlogPost(5402-5402)BlogPost(5417-5419)ListOfString(621-626)ListOfString(639-639)ListOfString(654-656)ListOfFloat(261-266)ListOfFloat(279-279)ListOfFloat(294-296)ListOfBoolean(126-131)ListOfBoolean(144-144)ListOfBoolean(159-161)ListOfListOfString(396-401)ListOfListOfString(414-414)ListOfListOfString(429-431)ListOfProduct(576-581)ListOfProduct(594-594)ListOfProduct(609-611)Product(4225-4232)Product(4245-4245)Product(4260-4262)Author(5621-5640)Author(5653-5653)Author(5668-5670)ListOfBlogPost(81-86)ListOfBlogPost(99-99)ListOfBlogPost(114-116)MutationCreateBlogPostRequest(3850-3855)MutationCreateBlogPostRequest(3868-3868)MutationCreateBlogPostRequest(3883-3885)MutationCreateBlogPostResponse(3895-3900)MutationCreateBlogPostResponse(3913-3913)MutationCreateBlogPostResponse(3928-3930)MutationUpdateBlogPostRequest(3940-3946)MutationUpdateBlogPostRequest(3959-3959)MutationUpdateBlogPostRequest(3974-3976)MutationUpdateBlogPostResponse(3993-3998)MutationUpdateBlogPostResponse(4011-4011)MutationUpdateBlogPostResponse(4026-4028)MutationCreateAuthorRequest(4038-4043)MutationCreateAuthorRequest(4056-4056)MutationCreateAuthorRequest(4071-4073)MutationCreateAuthorResponse(4083-4088)MutationCreateAuthorResponse(4101-4101)MutationCreateAuthorResponse(4116-4118)MutationUpdateAuthorRequest(4128-4134)MutationUpdateAuthorRequest(4147-4147)MutationUpdateAuthorRequest(4162-4164)MutationUpdateAuthorResponse(4181-4186)MutationUpdateAuthorResponse(4199-4199)MutationUpdateAuthorResponse(4214-4216)
🪛 checkmake (0.2.2)
v2/pkg/grpctest/Makefile
[warning] 16-16: Missing required phony target "all"
(minphony)
[warning] 16-16: Missing required phony target "clean"
(minphony)
[warning] 16-16: Missing required phony target "test"
(minphony)
⏰ 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 (10)
v2/pkg/grpctest/Makefile (1)
12-19: LGTM!The new Makefile targets are well-structured and serve clear purposes. The
build-plugintarget compiles the plugin service, andregenerate-protogenerates gRPC service code from the GraphQL schema, both supporting the PR's comprehensive list handling enhancements.v2/pkg/engine/datasource/grpc_datasource/compiler_test.go (1)
296-451: LGTM! Comprehensive test coverage for nested list compilation.The test thoroughly validates the new nested list handling capabilities by:
- Testing multiple nesting levels (orders → lines → modifiers)
- Verifying optional list metadata handling
- Using protobuf reflection to ensure correct message structure
- Validating that list wrappers are properly constructed with repeated "items" fields
This provides excellent coverage for the compiler's new
buildListMessageandtraverseListmethods.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
285-352: LGTM! Well-structured recursive list traversal.The
traverseListmethod correctly handles:
- Multi-level nesting with recursive traversal
- Optionality checks at each level
- Different processing for intermediate vs final levels
- Proper error handling with descriptive messages
The use of field number 1 for list wrappers is consistent with the protobuf schema design where all list wrapper messages use this convention.
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
215-223: Good improvement to error handling.Returning a boolean presence indicator follows Go idioms and enables clearer error messages when messages are not found in the document.
448-486: LGTM! Clear separation of message field handling.The switch statement effectively distinguishes between:
- Nested/nullable lists requiring special wrapper handling
- Optional scalars using well-known wrapper types
- Standard nested messages
Each case includes appropriate validation and error handling.
v2/pkg/grpctest/product.proto (2)
52-156: Excellent schema design for nested list support.The introduction of list wrapper messages provides a consistent and flexible approach for handling:
- Nullable lists (e.g.,
ListOfString optional_tags)- Multi-level nesting (e.g.,
ListOfListOfString tag_groups)- Complex type lists (e.g.,
ListOfBlogPost written_posts)This design enables the proper representation of GraphQL's nullable and nested list semantics in protobuf.
616-661: LGTM! Comprehensive domain entities for testing list handling.The
BlogPostandAuthormessages effectively demonstrate:
- Direct repeated fields for non-nullable lists (e.g.,
repeated string tags)- List wrappers for nullable lists (e.g.,
ListOfString optional_tags)- Multi-level nesting (e.g.,
ListOfListOfString tag_groups)- Complex type lists (e.g.,
ListOfBlogPost written_posts)These entities provide excellent test coverage for the new list handling capabilities.
v2/pkg/grpctest/mockservice.go (3)
151-151: LGTM! Correct type alignment for nullable float fields.The consistent use of
wrapperspb.DoubleValueinstead ofwrapperspb.FloatValueproperly aligns with the protobuf field definitions.Also applies to: 190-190, 213-213, 288-288, 322-322, 366-366, 383-383
963-968: LGTM! Consistent list handling pattern.The wrapping of
OrderLinesin theListOfOrderLinestructure aligns with the PR's objective to redesign list handling in gRPC, following the same pattern as other list types in the codebase.
976-1092: LGTM! Comprehensive mock implementation for BlogPost queries.The
QueryBlogPostimplementation provides excellent test coverage for the new nested list handling features, including:
- Scalar lists (tags, categories, viewCounts)
- Nullable/optional lists (optionalTags, keywords, ratings)
- Nested string lists (tagGroups, relatedTopics, commentThreads)
- Complex object lists (relatedCategories, contributors)
- Deeply nested lists (categoryGroups, contributorTeams)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
execution/engine/execution_engine_grpc_test.go (1)
814-1434: Excellent comprehensive test coverage for nested list handling.The test cases thoroughly cover all scenarios for BlogPost and Author entities including scalar lists, nested lists, complex object lists, and mutations. The assertions are detailed and verify the correct handling of the new functionality.
Consider using goldie fixtures for large response assertions as suggested in the past review comment. This would make the tests more maintainable when dealing with complex JSON responses:
// Example using goldie g := goldie.New(t) g.Assert(t, "blogpost_complex_lists_query", []byte(response))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
execution/engine/execution_engine_grpc_test.go(6 hunks)v2/pkg/grpctest/mockservice.go(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
v2/pkg/grpctest/mockservice.go (2)
Learnt from: SkArchon
PR: #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.
Learnt from: SkArchon
PR: #1218
File: v2/pkg/engine/resolve/loader.go:58-60
Timestamp: 2025-07-09T09:34:51.269Z
Learning: In Go's bytes package, the String() method on *bytes.Buffer handles nil receivers gracefully by returning an empty string rather than panicking, making additional nil checks unnecessary when calling String() on potentially nil *bytes.Buffer instances.
execution/engine/execution_engine_grpc_test.go (1)
Learnt from: SkArchon
PR: #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.
🧬 Code Graph Analysis (2)
v2/pkg/grpctest/mockservice.go (2)
v2/pkg/grpctest/productv1/product.pb.go (60)
CategoryInput(6979-6985)CategoryInput(6998-6998)CategoryInput(7013-7015)Category(4861-4868)Category(4881-4881)Category(4896-4898)ListOfCategoryInput(216-221)ListOfCategoryInput(234-234)ListOfCategoryInput(249-251)ListOfUserInput(711-716)ListOfUserInput(729-729)ListOfUserInput(744-746)User(4345-4351)User(4364-4364)User(4379-4381)ListOfListOfUserInput(486-491)ListOfListOfUserInput(504-504)ListOfListOfUserInput(519-521)ListOfListOfUser(441-446)ListOfListOfUser(459-459)ListOfListOfUser(474-476)ListOfUser(666-671)ListOfUser(684-684)ListOfUser(699-701)ListOfListOfCategoryInput(351-356)ListOfListOfCategoryInput(369-369)ListOfListOfCategoryInput(384-386)ListOfListOfCategory(306-311)ListOfListOfCategory(324-324)ListOfListOfCategory(339-341)ListOfCategory(171-176)ListOfCategory(189-189)ListOfCategory(204-206)BlogPost(5365-5389)BlogPost(5402-5402)BlogPost(5417-5419)ListOfString(621-626)ListOfString(639-639)ListOfString(654-656)ListOfFloat(261-266)ListOfFloat(279-279)ListOfFloat(294-296)ListOfBoolean(126-131)ListOfBoolean(144-144)ListOfBoolean(159-161)ListOfListOfString(396-401)ListOfListOfString(414-414)ListOfListOfString(429-431)ListOfProduct(576-581)ListOfProduct(594-594)ListOfProduct(609-611)Product(4225-4232)Product(4245-4245)Product(4260-4262)Author(5621-5640)Author(5653-5653)Author(5668-5670)ListOfBlogPost(81-86)ListOfBlogPost(99-99)ListOfBlogPost(114-116)execution/federationtesting/accounts/graph/model/models_gen.go (2)
Name(43-46)Title(74-77)
execution/engine/execution_engine_grpc_test.go (1)
v2/pkg/grpctest/mapping/mapping.go (1)
DefaultGRPCMapping(10-954)
⏰ 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 (6)
execution/engine/execution_engine_grpc_test.go (1)
9-9: Good use of math constants for clarity.Using
math.MaxFloat64instead of hardcoded values improves test readability and makes the intent clear.Also applies to: 214-214, 289-289
v2/pkg/grpctest/mockservice.go (5)
22-130: Well-designed helper functions for type conversions.The helper functions properly handle null checks at multiple levels and generate appropriate IDs for converted entities. The consistent approach to handling nested structures and returning empty lists (rather than nil) for empty inputs aligns well with GraphQL's non-null list semantics.
152-152: Correct use of DoubleValue for GraphQL Float type.The consistent change from
wrapperspb.FloatValuetowrapperspb.DoubleValuealigns with the GraphQL specification where Float maps to double precision, as discussed in previous reviews.Also applies to: 191-191, 214-214, 289-289, 323-323, 367-367, 384-384
511-512: Proper handling of wrapper types and list containers.The changes correctly use
GetValue()for the limit wrapper type and properly wrap order lines in theListOfOrderLinecontainer, aligning with the new protobuf schema structure for repeated fields.Also applies to: 964-968
977-1549: Comprehensive BlogPost query and mutation implementations.The BlogPost methods provide excellent test coverage with varied data patterns for different list types. The mutations properly use the helper functions for type conversions, addressing the concerns raised in previous reviews.
Also applies to: 1967-2080
1552-1964: Well-implemented Author query and mutation methods.The Author implementations mirror the comprehensive approach used for BlogPost, with proper type conversions in mutations and rich test data for various scenarios. This consistency aids in testing and maintenance.
Also applies to: 2083-2200
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
562-584: Consider extracting common list handling logic.While this method improves code organization, there's significant duplication of list handling logic between
buildFieldandbuildInputMessageField. The identical logic for nullable/nested list detection and metadata creation could be extracted into a shared helper method.Consider extracting the list handling logic:
+func (r *rpcPlanVisitor) applyListHandling(field *RPCField, typeRef int) { + if r.definition.TypeIsList(typeRef) { + switch { + case r.typeIsNullableOrNestedList(typeRef): + field.ListMetadata = r.createListMetadata(typeRef) + field.IsListType = true + default: + field.Repeated = true + } + } +}Then use this helper in both methods to eliminate duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/ast/ast_type.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(8 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/ast/ast_type.go
🧰 Additional context used
🧠 Learnings (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
Learnt from: Noroth
PR: #1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.568Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
Learnt from: SkArchon
PR: #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.
🧬 Code Graph Analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
RPCField(150-178)ListMetadata(181-186)LevelInfo(189-192)pkg/ast/ast_field_alias.go (1)
Alias(8-12)pkg/ast/ast_node_kind.go (1)
NodeKindInputObjectTypeDefinition(24-24)v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
DataTypeMessage(29-29)Message(117-121)DataType(16-16)
⏰ 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 (9)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (4)
8-8: LGTM: Math import added for improved float testing.The math import is correctly added to support the use of
math.MaxFloat64in the test assertions with proper delta tolerance.
429-431: Good improvement: Added explicit proto message existence validation.The explicit checks for proto message existence before using them improves test reliability and provides clearer error messages if the messages are not found in the compiled schema.
Also applies to: 437-439
883-883: Excellent fix: Using delta tolerance for float comparison.Using
math.MaxFloat64withInDeltais much better than exact float comparison, which can be unreliable due to floating-point precision issues.
2093-2942: Excellent comprehensive test coverage for nested lists functionality.This substantial test function provides thorough coverage of the redesigned list handling in gRPC datasource. The implementation demonstrates several strengths:
Well-structured test design:
- Table-driven tests with clear, descriptive names
- Comprehensive validation functions for each scenario
- Proper separation of concerns between query execution and response validation
Comprehensive scenario coverage:
- Single lists with different nullability patterns (
[String!]!,[String],[String!])- Nested lists with complex nullability (
[[String!]!]!,[[String]], etc.)- Scalar type lists (integers, floats, booleans)
- Complex object lists with input/output type conversions
- Filtered queries, ID-based queries, and mutations
- Both BlogPost and Author entities with their respective list structures
Robust validation:
- Proper type checking with
require.IsType- Null handling validation for optional fields
- Array length and content verification
- Complex nested structure validation
The test aligns perfectly with the PR objective of redesigning list handling for gRPC and provides confidence that the new nested list functionality works correctly across various scenarios.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (5)
327-340: LGTM! Good refactoring of field building logic.The extraction of field building into a dedicated
buildFieldmethod and the separation of inline fragment handling improves code organization and readability. The early return pattern for inline fragments is clean and avoids nested conditions.
342-379: Well-structured field building method with comprehensive list handling.The centralized field building logic is well-organized and handles the complex scenarios for list types appropriately. The distinction between nullable/nested lists (using wrapper messages) and non-nullable single lists (using protobuf repeated syntax) aligns with the design objectives.
586-603: Well-implemented metadata creation for nested lists.The method correctly handles the complex nested list scenarios by:
- Determining the nesting level using
TypeNumberOfListWraps- Creating appropriate metadata structure with level information
- Properly traversing the type hierarchy to capture nullability at each level
This implementation supports the PR's objective of redesigning list handling in gRPC.
527-527: Consistent use of refactored field building methods.The changes properly utilize the new
buildInputMessageFieldhelper method, maintaining the existing logic flow while benefiting from the centralized field creation logic.Also applies to: 533-533, 548-551
882-892: Correct implementation of nullable/nested list detection.The helper method properly identifies the two cases that require wrapper message handling:
- Nullable lists - because protobuf repeated fields cannot be null
- Nested lists - because protobuf doesn't directly support nested repeated fields
This aligns with the gRPC list handling redesign objectives.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.211](v2.0.0-rc.210...v2.0.0-rc.211) (2025-07-28) ### Features * redesign handling for lists in gRPC ([#1246](#1246)) ([a06c9db](a06c9db)) ### Bug Fixes * disable minifier for gRPC datasource ([#1249](#1249)) ([9a26e5c](9a26e5c)) * test v2 benchmarks on ci ([#1238](#1238)) ([d9cfb21](d9cfb21)) --- 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** * Redesigned handling of lists in the gRPC datasource. * **Bug Fixes** * Disabled the minifier for the gRPC datasource. * Enabled testing of version 2 benchmarks on continuous integration (CI). * **Documentation** * Added a changelog entry for version 2.0.0-rc.211. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
Checklist