fix: handle nullable and nested argument lists properly #1254
Conversation
Noroth
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughBulk operation support for authors and blog posts was introduced across the GraphQL schema, gRPC service definitions, mock service implementations, and test suites. This includes new bulk search and bulk mutation fields, corresponding gRPC RPCs and message types, extended mapping configurations, comprehensive test coverage for nullable list parameters, and internal refactoring to support mapped input field names and improved error messages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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: 0
🧹 Nitpick comments (2)
v2/pkg/grpctest/mockservice.go (2)
2464-2548: Consider refactoring to reduce duplication with MutationBulkCreateAuthors.The implementation is correct and handles nullable lists properly. However, it's nearly identical to
MutationBulkCreateAuthorsexcept for ID generation. Consider extracting a shared helper function to reduce code duplication.
2634-2716: Consider refactoring to reduce duplication with MutationBulkCreateBlogPosts.The implementation correctly handles nullable lists. Like the author operations, this could benefit from extracting common logic into a helper function to reduce duplication between create and update operations.
📜 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 (10)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(5 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(4 hunks)v2/pkg/grpctest/mapping/mapping.go(4 hunks)v2/pkg/grpctest/mockservice.go(2 hunks)v2/pkg/grpctest/product.proto(5 hunks)v2/pkg/grpctest/testdata/products.graphqls(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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.
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.
v2/pkg/grpctest/product.proto (1)
Learnt from: Noroth
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
Learnt from: Noroth
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
Learnt from: Noroth
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
Learnt from: Noroth
PR: #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.
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_list_test.go (1)
Learnt from: Noroth
PR: #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.
🧬 Code Graph Analysis (4)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
v2/pkg/graphqljsonschema/jsonschema.go (1)
NewNull(426-430)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
DataTypeMessage(29-29)Message(117-121)DataTypeEnum(28-28)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (6)
RPCExecutionPlan(49-54)RPCCall(58-71)RPCMessage(75-89)RPCField(150-178)ListMetadata(181-186)LevelInfo(189-192)v2/pkg/engine/datasource/grpc_datasource/compiler.go (5)
DataTypeMessage(29-29)Message(117-121)DataTypeString(20-20)DataTypeBool(27-27)DataTypeInt32(21-21)
v2/pkg/grpctest/mockservice.go (1)
v2/pkg/grpctest/productv1/product.pb.go (90)
ListOfListOfString(576-581)ListOfListOfString(594-594)ListOfListOfString(609-611)ListOfListOfString_List(8235-8240)ListOfListOfString_List(8253-8253)ListOfListOfString_List(8268-8270)QueryBulkSearchAuthorsRequest(3662-3667)QueryBulkSearchAuthorsRequest(3680-3680)QueryBulkSearchAuthorsRequest(3695-3697)QueryBulkSearchAuthorsResponse(3707-3712)QueryBulkSearchAuthorsResponse(3725-3725)QueryBulkSearchAuthorsResponse(3740-3742)Author(6341-6360)Author(6373-6373)Author(6388-6390)ListOfString(801-806)ListOfString(819-819)ListOfString(834-836)ListOfString_List(8455-8460)ListOfString_List(8473-8473)ListOfString_List(8488-8490)Category(5581-5588)Category(5601-5601)Category(5616-5618)CategoryKind_CATEGORY_KIND_OTHER(32-32)ListOfListOfCategory(486-491)ListOfListOfCategory(504-504)ListOfListOfCategory(519-521)ListOfListOfCategory_List(8147-8152)ListOfListOfCategory_List(8165-8165)ListOfListOfCategory_List(8180-8182)ListOfCategory(351-356)ListOfCategory(369-369)ListOfCategory(384-386)ListOfCategory_List(8015-8020)ListOfCategory_List(8033-8033)ListOfCategory_List(8048-8050)CategoryKind_CATEGORY_KIND_BOOK(29-29)QueryBulkSearchBlogPostsRequest(3752-3757)QueryBulkSearchBlogPostsRequest(3770-3770)QueryBulkSearchBlogPostsRequest(3785-3787)QueryBulkSearchBlogPostsResponse(3797-3802)QueryBulkSearchBlogPostsResponse(3815-3815)QueryBulkSearchBlogPostsResponse(3830-3832)BlogPost(6085-6109)BlogPost(6122-6122)BlogPost(6137-6139)User(5065-5071)User(5084-5084)User(5099-5101)MutationBulkCreateAuthorsRequest(4586-4591)MutationBulkCreateAuthorsRequest(4604-4604)MutationBulkCreateAuthorsRequest(4619-4621)MutationBulkCreateAuthorsResponse(4631-4636)MutationBulkCreateAuthorsResponse(4649-4649)MutationBulkCreateAuthorsResponse(4664-4666)ListOfListOfUser(621-626)ListOfListOfUser(639-639)ListOfListOfUser(654-656)ListOfBlogPost(171-176)ListOfBlogPost(189-189)ListOfBlogPost(204-206)ListOfUser(846-851)ListOfUser(864-864)ListOfUser(879-881)ListOfProduct(756-761)ListOfProduct(774-774)ListOfProduct(789-791)Product(4945-4952)Product(4965-4965)Product(4980-4982)CategoryKind_CATEGORY_KIND_ELECTRONICS(30-30)MutationBulkUpdateAuthorsRequest(4676-4681)MutationBulkUpdateAuthorsRequest(4694-4694)MutationBulkUpdateAuthorsRequest(4709-4711)MutationBulkUpdateAuthorsResponse(4721-4726)MutationBulkUpdateAuthorsResponse(4739-4739)MutationBulkUpdateAuthorsResponse(4754-4756)MutationBulkCreateBlogPostsRequest(4766-4771)MutationBulkCreateBlogPostsRequest(4784-4784)MutationBulkCreateBlogPostsRequest(4799-4801)MutationBulkCreateBlogPostsResponse(4811-4816)MutationBulkCreateBlogPostsResponse(4829-4829)MutationBulkCreateBlogPostsResponse(4844-4846)MutationBulkUpdateBlogPostsRequest(4856-4861)MutationBulkUpdateBlogPostsRequest(4874-4874)MutationBulkUpdateBlogPostsRequest(4889-4891)MutationBulkUpdateBlogPostsResponse(4901-4906)MutationBulkUpdateBlogPostsResponse(4919-4919)MutationBulkUpdateBlogPostsResponse(4934-4936)
🔇 Additional comments (37)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
43-44: LGTM! Enhanced test error messages improve debugging.The formatted error messages will provide better context when test failures occur, making it easier to diagnose issues with execution plan validation. The changes maintain the original test logic while improving failure diagnostics.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
225-225: LGTM! Enhanced error context for list structure flattening.The addition of field name context and proper error wrapping with
%wverb provides better debugging information when list structure flattening fails. This will be particularly helpful when processing the complex nested lists introduced in the bulk operations.
266-266: LGTM! Simplified error messages improve clarity.Removing the redundant "unable to flatten list structure:" prefix makes the error messages more concise while maintaining their descriptive value. The core error information is preserved and the messages remain clear for debugging purposes.
Also applies to: 270-270, 293-293, 297-297, 312-312
v2/pkg/grpctest/testdata/products.graphqls (2)
356-360: LGTM! Well-designed bulk search operations.The bulk search operations follow consistent naming patterns and properly use non-null list types for both input filters and return values. The reuse of existing
AuthorFilterandBlogPostFiltertypes maintains schema consistency.
385-391: LGTM! Consistent bulk mutation operations.The bulk create and update mutations follow the same design principles as the query operations, using existing input types and maintaining consistent return type patterns. The naming convention aligns well with the bulk search operations.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (1)
1000-1092: Excellent test coverage for nullable list parameters handling.This test case provides comprehensive validation for the bulk search authors functionality, correctly implementing:
- Nullable list parameter handling with proper
IsListType: trueandListMetadataconfiguration- Correct nesting level (1) with appropriate optional flags for the nullable
[AuthorFilter!]type- Well-structured nested message definitions for
AuthorFilterwith proper field mappings- Consistent JSON path mappings and data type specifications
The test aligns perfectly with the retrieved learning about nullable list handling in the gRPC datasource, where wrapper types are used instead of protobuf's repeated field syntax for nullable lists.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (5)
443-445: Good refactoring to centralize input argument name resolution.The introduction of
mappedInputNameensures consistent field naming by properly resolving input argument mappings using the base type and ancestor reference. This improvement centralizes the naming resolution logic and ensures that all input arguments use the correctly mapped names in the protobuf message structure.
455-457: Excellent consolidation of field creation logic.Replacing inline field construction with a call to
buildInputMessageFieldimproves code maintainability and ensures consistent field creation. Using themappedInputNameensures that input object types use the correctly resolved field mappings in the protobuf message structure.
471-472: Consistent field creation for scalar and enum types.The consolidation of field creation logic for scalar and enum types maintains consistency with the input object type handling. This reduces code duplication and ensures that all input argument types follow the same field creation pattern with proper name mapping.
512-513: Consistent field creation in buildMessageField method.Applying the same consolidation pattern to
buildMessageFieldensures consistency across all methods that construct input message fields. This maintains the same centralized field creation logic and reduces the potential for inconsistencies between different code paths.
557-559: Important addition of enum name handling.The addition of enum name handling in
buildInputMessageFieldensures that enum fields have theirEnumNameproperty correctly set when using the centralized field creation method. This is essential for proper protobuf message generation and maintains consistency with enum handling in other parts of the codebase.v2/pkg/grpctest/mapping/mapping.go (4)
154-163: Well-structured bulk search query RPC additions.The new bulk search RPC configurations follow the established naming conventions perfectly:
- Consistent
Queryprefix for query operations- Descriptive operation names (
BulkSearchAuthors,BulkSearchBlogPosts)- Complete request/response message name mappings
- Follows the same pattern as existing query RPCs
206-225: Comprehensive bulk mutation RPC configurations.The bulk mutation RPC additions provide complete coverage for bulk operations:
- Consistent
Mutationprefix following established patterns- Complete set of operations: create and update for both authors and blog posts
- Proper request/response message name mappings
- Descriptive and consistent naming throughout
383-394: Proper field mappings for bulk search queries.The bulk search field mappings are well-structured:
- Target names use consistent snake_case convention (
bulk_search_authors,bulk_search_blog_posts)- Proper argument mappings for
filtersparameter- Follows the same pattern as existing query field mappings
- Complete and consistent with the RPC configurations
448-471: Complete bulk mutation field mappings.The bulk mutation field mappings provide comprehensive coverage:
- Consistent snake_case target names following gRPC conventions
- Appropriate argument mappings reflecting the specific parameter names for each operation
- Complete mappings for all four bulk mutation operations
- Maintains consistency with existing mutation field mapping patterns
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (9)
2891-2953: LGTM: Comprehensive test for bulk search with nullable list parametersThis test case effectively validates the handling of nullable list parameters in bulk search operations. The test properly:
- Tests different filter criteria producing appropriate results
- Validates complex nested structures (teamsByProject, categoryPreferences)
- Checks that filter parameters correctly influence the generated data
- Uses proper assertions for different data types
The test aligns well with the PR objective of handling nullable and nested argument lists properly.
2954-2968: LGTM: Important edge case test for null parameter handlingThis test correctly validates the behavior when a nullable list parameter is explicitly set to null. The expectation of an empty array response is logical and the test is concise and focused on this specific edge case.
2970-3049: LGTM: Well-structured test for blog post bulk search operationsThis test case effectively mirrors the author bulk search test pattern while testing blog post-specific functionality. It properly validates:
- Filter parameter effects on response data (minTags, hasCategories)
- Complex nested structures (tagGroups, categoryGroups, relatedCategories)
- Appropriate response structure and data types
The consistency with the author test pattern makes the test suite easier to maintain.
3051-3124: LGTM: Comprehensive bulk create mutation testThis test effectively validates bulk create operations with complex input structures. The test properly:
- Uses realistic input data with various list types (scalar, nested, complex)
- Validates proper ID generation for bulk created items
- Checks that input data is correctly processed and returned in the response
- Tests nested complex structures like favoriteCategories and authorGroups
The test provides good coverage for bulk create functionality with nullable list parameters.
3126-3182: LGTM: Appropriate bulk update mutation testThis test case effectively validates bulk update operations with nullable list parameters. The test correctly:
- Uses appropriate input data for update scenarios
- Validates that updates are properly applied
- Checks distinctive ID generation for updated items (bulk-updated-author prefix)
- Tests complex list structures in update contexts
The test provides good coverage for bulk update functionality.
3184-3264: LGTM: Extensive bulk create test for blog postsThis test case provides comprehensive coverage of bulk create operations for blog posts with complex input data. The test effectively:
- Uses extensive input data covering all list types (scalar, nested, complex)
- Validates proper handling of nullable list parameters
- Checks creation of complex nested structures
- Tests realistic blog post creation scenarios
The comprehensive input data and thorough validation make this a robust test for the bulk create functionality.
3266-3318: LGTM: Focused bulk update test for blog postsThis test case appropriately validates bulk update operations for blog posts. The test correctly:
- Uses focused input data suitable for update scenarios
- Validates proper application of updates
- Checks distinctive ID generation for updated posts (bulk-updated-post prefix)
- Tests key list structures in update contexts
The focused approach makes the test clear and maintainable while providing adequate coverage.
3320-3341: LGTM: Important edge case test for empty arraysThis test case effectively validates the behavior when nullable list parameters are provided as empty arrays rather than null. The test correctly:
- Tests both bulk search operations with empty filter arrays
- Expects appropriate empty array responses
- Covers an important edge case that complements the null parameter test
This edge case testing ensures robust handling of different nullable list parameter scenarios.
2891-3341: Excellent comprehensive test coverage for bulk operations with nullable listsThis substantial addition to the test suite provides excellent coverage for the new bulk operations functionality. The test cases effectively validate:
Comprehensive Coverage:
- Both query (bulk search) and mutation (bulk create/update) operations
- Multiple entity types (authors and blog posts)
- Various list types: scalar lists, nested lists, and complex object lists
- Important edge cases: null parameters and empty arrays
Quality Implementation:
- Clear, descriptive test names that explain the validation purpose
- Comprehensive input data covering realistic usage scenarios
- Thorough validation functions checking response structure and data correctness
- Consistent patterns with existing test structure in the file
Alignment with PR Objectives:
The tests directly address the PR goal of "handle nullable and nested argument lists properly" by specifically testing nullable list parameters in various bulk operation contexts.This addition significantly strengthens the test coverage for nullable and nested list handling in gRPC datasources.
v2/pkg/grpctest/mockservice.go (5)
1859-1859: Good defensive initialization practice.Initializing with an empty List struct instead of nil prevents potential nil pointer dereferences downstream.
2206-2279: Excellent implementation of bulk search with proper null handling.The method correctly handles nullable list parameters at multiple levels (nil filters and nil list), which aligns perfectly with the PR's objective to handle nullable argument lists properly.
2281-2376: Consistent implementation with comprehensive test data.The implementation follows the same robust null-handling pattern and generates blog posts with all required nested list structures properly initialized.
2378-2462: Well-structured bulk creation with proper type conversions.Good use of helper functions for converting input types to output types. The implementation handles nullable lists correctly and ensures all required fields are populated.
2550-2632: Consistent implementation for blog post bulk creation.The implementation correctly handles nullable lists and follows the established pattern from author operations.
v2/pkg/grpctest/product.proto (4)
14-17: Well-organized RPC declarations for bulk operations.The new bulk operation RPCs are properly grouped with related operations and follow consistent naming conventions.
Also applies to: 36-37
58-92: Correct implementation of nullable list wrappers.These wrapper types properly implement the pattern for nullable lists in protobuf, consistent with the existing codebase pattern and the learning that "only non-null lists use protobuf's repeated field syntax directly."
471-486: Proper use of nullable wrappers for requests and repeated fields for responses.The message definitions correctly use nullable wrapper types for input filters while using direct repeated fields for response lists, which aligns with GraphQL's type system requirements.
554-585: Consistent implementation of bulk mutation messages.The mutation messages correctly implement the nullable input pattern and follow the established conventions for field naming and structure.
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (4)
147-156: LGTM: Bulk query RPC configurations follow established patterns.The new bulk search operations for authors and blog posts are correctly configured with consistent naming conventions and structure matching existing query RPCs.
199-218: LGTM: Bulk mutation RPC configurations follow established patterns.The new bulk create and update operations for authors and blog posts are correctly configured with consistent naming conventions and structure matching existing mutation RPCs.
376-387: LGTM: Query field mappings for bulk operations are correctly configured.The field mappings follow established naming conventions with proper snake_case target names and consistent argument mappings using "filters" parameter, which aligns with the bulk operation pattern for handling multiple filter inputs.
441-464: No action needed: argument mappings align with proto definitionsThe mappings in
mapping_test_helper.gocorrectly translate GraphQL arguments to the gRPC request fields as defined inv2/pkg/grpctest/product.proto:
MutationBulkCreateAuthorsRequest declares
ListOfAuthorInput authors = 1;
→ mapping"authors": "authors"is accurate (snake_case equals CamelCase here).MutationBulkCreateBlogPostsRequest declares
ListOfBlogPostInput blog_posts = 1;
→ mapping"blogPosts": "blog_posts"correctly converts camelCase to snake_case.The convention—converting camelCase GraphQL names to snake_case proto fields, or leaving them unchanged when identical—is properly followed. No changes are required.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/grpctest/mockservice.go (1)
1859-1860: Consider using nil for empty teams list.When
hasTeamsis false, consider settingteamsByProjectto nil instead of an empty wrapper to maintain consistency with how other optional fields are handled in the codebase.- teamsByProject = &productv1.ListOfListOfString{List: &productv1.ListOfListOfString_List{}} + teamsByProject = nil
📜 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 (10)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(5 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(4 hunks)v2/pkg/grpctest/mapping/mapping.go(4 hunks)v2/pkg/grpctest/mockservice.go(2 hunks)v2/pkg/grpctest/product.proto(5 hunks)v2/pkg/grpctest/testdata/products.graphqls(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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.
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.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
Learnt from: Noroth
PR: #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.
v2/pkg/grpctest/product.proto (1)
Learnt from: Noroth
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
Learnt from: Noroth
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (1)
Learnt from: Noroth
PR: #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.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
Learnt from: Noroth
PR: #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.
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/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (3)
DataTypeMessage(29-29)Message(117-121)DataTypeEnum(28-28)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
v2/pkg/graphqljsonschema/jsonschema.go (1)
NewNull(426-430)
🔇 Additional comments (24)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
43-44: LGTM: Improved test failure diagnostics.The enhanced error messages in the test assertions provide valuable context when tests fail, making it easier to understand what went wrong by showing both expected and actual error strings.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
225-225: LGTM: Enhanced error context for list flattening failures.Adding the field alias/path to the error message provides valuable context when list structure flattening fails, making debugging easier by identifying exactly which field caused the issue.
266-266: LGTM: Improved error message clarity.Removing the redundant "unable to flatten list structure:" prefix from error messages makes them more concise and direct while maintaining their descriptive value. The core error information is preserved without unnecessary verbosity.
Also applies to: 270-270, 293-293, 297-297, 312-312
v2/pkg/engine/datasource/grpc_datasource/execution_plan_list_test.go (1)
1000-1092: LGTM! Well-structured test for nullable list parameters.This test case effectively validates the execution plan generation for bulk search operations with nullable list parameters. The test correctly demonstrates:
- Proper handling of optional list parameter
[AuthorFilter!]withIsListType: trueandOptional: true- Correct list metadata configuration with nesting level and optionality information
- Appropriate message structure for both request and response
The test aligns perfectly with the PR's objective to handle nullable and nested argument lists properly.
v2/pkg/grpctest/mapping/mapping.go (4)
154-163: LGTM! Consistent bulk search RPC configuration.The new QueryRPCs entries for bulk search operations follow the established naming conventions and structure perfectly. Both
bulkSearchAuthorsandbulkSearchBlogPostsuse appropriate RPC, Request, and Response message naming patterns.
206-225: LGTM! Complete bulk mutation RPC configuration.The new MutationRPCs entries comprehensively cover bulk operations for both authors and blog posts. The naming conventions are consistent with existing mutations, using the "Mutation" prefix appropriately and maintaining parallel structure for create/update operations.
383-394: LGTM! Proper GraphQL to gRPC field mappings.The Query field mappings for bulk search operations correctly implement the camelCase to snake_case conversion and include appropriate argument mappings for the
filtersparameter. The structure is consistent with existing query field mappings.
448-471: LGTM! Comprehensive bulk mutation field mappings.The Mutation field mappings cover all bulk operations and correctly handle different argument types. The mappings properly convert GraphQL field names to snake_case gRPC targets and appropriately map different parameter names (
authorsvsblogPosts→blog_posts).v2/pkg/grpctest/testdata/products.graphqls (2)
355-360: LGTM! Well-designed bulk search operations.The bulk search query fields properly use nullable list parameters
[AuthorFilter!]and[BlogPostFilter!], which allows clients to optionally provide filters while ensuring any provided filter is non-null. The return types appropriately guarantee non-null results, which is consistent with search operation expectations.
384-391: LGTM! Consistent bulk mutation operations.The bulk mutation fields follow a coherent design pattern with nullable list parameters that accept non-null input objects. This design provides flexibility for clients while maintaining type safety. The parameter naming is appropriately descriptive and the return types ensure predictable results.
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (1)
147-156: LGTM! Properly synchronized test helper mappings.The test helper mappings correctly mirror the main mapping configuration, ensuring test consistency. All bulk operation RPCs and field mappings are properly replicated with identical structure and naming conventions. This synchronization is essential for accurate testing of the new bulk operations.
Also applies to: 199-218, 376-387, 441-464
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (5)
443-446: LGTM! Proper input argument mapping implementation.The introduction of
mappedInputNameusing the base type from the walker's type definitions stack and ancestor node reference correctly implements input argument name mapping. This aligns with the PR objective to handle nullable and nested argument lists properly.
455-458: Good refactoring to centralize field construction.The replacement of manual
RPCFieldconstruction with thebuildInputMessageFieldhelper method improves code consistency and maintainability. The mapped input name is correctly passed to ensure proper field naming.
471-472: Consistent refactoring for scalar and enum types.The use of
buildInputMessageFieldhelper for scalar and enum types maintains consistency with the input object type handling above. This consolidates the field construction logic as intended.
512-513: Proper consolidation of field construction logic.The refactoring in
buildMessageFieldto usebuildInputMessageFieldmaintains consistency across the codebase and ensures all input fields are constructed using the same centralized logic.
557-559: Essential enhancement for enum field handling.The addition of
EnumNameproperty setting for enum types is crucial for proper protobuf message generation. UsingResolveTypeNameStringensures the correct enum type name is captured.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
2891-3341: Comprehensive test coverage for nullable list parameters.The new test cases thoroughly cover bulk operations with nullable list parameters, including:
- Null parameters (
filters: null)- Empty lists (
filters: [])- Populated lists with complex nested structures
- Both query and mutation operations
The tests align well with the PR objective of handling nullable and nested argument lists properly.
v2/pkg/grpctest/product.proto (3)
14-17: LGTM! Well-structured bulk operation RPCs.The new bulk operation RPCs follow consistent naming patterns and are properly organized within the service definition.
Also applies to: 36-37
58-92: Consistent implementation of list wrapper types.The new wrapper message types follow the established pattern for nullable lists, correctly using nested List messages as per the codebase conventions.
471-486: Proper use of wrapper types for nullable inputs.The request/response messages correctly distinguish between nullable inputs (using wrapper types) and non-nullable outputs (using repeated fields), maintaining consistency with the gRPC datasource patterns.
Also applies to: 554-585
v2/pkg/grpctest/mockservice.go (4)
2207-2279: Well-implemented bulk search with proper null handling.The method correctly handles nullable filter lists and generates appropriate mock data for each filter, maintaining consistency with the single-filter query methods.
2281-2376: Consistent implementation of bulk blog post search.The method follows the same robust pattern as
QueryBulkSearchAuthors, properly handling nullable lists and generating comprehensive mock data.
2378-2462: Comprehensive bulk create implementation.Excellent use of helper functions for type conversions and proper handling of nullable inputs. All required fields are populated with appropriate mock data.
2464-2716: Consistent and well-structured bulk mutation implementations.All bulk mutation methods follow the same robust pattern:
- Proper null handling for input lists
- Consistent use of helper functions for type conversions
- Comprehensive mock data generation for all required fields
The implementation maintains high code quality throughout.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.214](v2.0.0-rc.213...v2.0.0-rc.214) (2025-07-29) ### Bug Fixes * handle nullable and nested argument lists properly ([#1254](#1254)) ([67af556](67af556)) --- 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 * **Bug Fixes** * Resolved an issue with handling nullable and nested argument lists. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.214. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
Checklist