feat: support nullable base types #1212
Conversation
WalkthroughThis change introduces comprehensive support for nullable scalar fields in GraphQL-to-gRPC execution. It expands the protobuf schema, GraphQL schema, mapping configurations, and mock service to handle nullable fields using protobuf wrapper types. Compiler, execution plan, and marshalling logic are updated to recognize and process optional fields. Extensive new tests validate correct handling of nullable fields in queries and mutations. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsv2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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 (
|
Noroth
changed the title
![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 (7)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
253-261: Document the protobuf wrapper type conventionThe method correctly handles protobuf wrapper types by extracting the "value" field. Consider adding a comment documenting this convention for clarity.
+// resolveOptionalField extracts the inner value from protobuf wrapper types +// (e.g., google.protobuf.StringValue, Int32Value) which contain a single "value" field func (d *DataSource) resolveOptionalField(arena *astjson.Arena, root *astjson.Value, name string, data protoref.Message) error {v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (2)
1670-1670: Consider using approximate equality for float comparisonsUsing
math.Roundfor float comparisons works but could be more idiomatic. Consider using a tolerance-based comparison or the testifyInDeltaassertion.-require.Equal(t, float64(3.14), math.Round(firstEntry["optionalFloat"].(float64)*100)/100) // round to 2 decimal places +require.InDelta(t, 3.14, firstEntry["optionalFloat"].(float64), 0.001, "optionalFloat should be approximately 3.14")Also applies to: 1730-1730
1542-1878: Comprehensive test coverage for nullable fieldsThe test thoroughly covers all nullable field scenarios with well-structured table-driven tests. Consider extracting the common setup code (schema parsing, datasource creation) into a helper function to reduce duplication across test cases.
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
93-93: Enhance comment clarity for the Imports field.Consider making the comment more descriptive to clarify these are protobuf file import paths.
- Imports []string // The imports of the protobuf document + Imports []string // Import paths of other protobuf files referenced by this document
492-492: Remove or update misleading TODO comment.The TODO comment about handling optional fields appears outdated since optional scalar fields are already handled in the nested message section above (lines 448-464).
- // TODO handle optional fieldsv2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
176-196: Consider adding validation for scalar field types.The
ToOptionalTypeMessagemethod correctly wraps fields in a message, but it could benefit from validation to ensure it's only called on scalar fields.Consider adding a check to ensure this method is only used with scalar types:
// ToOptionalTypeMessage returns a message that wraps the scalar value in a message // as protobuf scalar types are not nullable. func (r *RPCField) ToOptionalTypeMessage(protoName string) *RPCMessage { if r == nil { return nil } // Ensure this is only used for scalar types if r.Message != nil { // Field already has a message type, shouldn't wrap again return r.Message } return &RPCMessage{ Name: protoName, Fields: RPCFields{ RPCField{ Name: knownTypeOptionalFieldValueName, JSONPath: r.JSONPath, TypeName: r.TypeName, Repeated: r.Repeated, EnumName: r.EnumName, }, }, } }v2/pkg/grpctest/mockservice.go (1)
21-50: Consider more robust ID generation for production use.The implementation correctly handles nullable fields. However, the ID generation using
rand.Intn(1000)could lead to collisions. While acceptable for a mock service, consider using UUID or a monotonic counter for production implementations.
📜 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 (13)
execution/engine/execution_engine_grpc_test.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/compiler.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(4 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(4 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(5 hunks)v2/pkg/grpctest/mapping/mapping.go(7 hunks)v2/pkg/grpctest/mockservice.go(2 hunks)v2/pkg/grpctest/product.proto(4 hunks)v2/pkg/grpctest/schema.go(3 hunks)v2/pkg/grpctest/testdata/products.graphqls(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
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 (1)
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
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: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
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/mockservice.go (1)
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
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_test.go (1)
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
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)
execution/engine/execution_engine_grpc_test.go (1)
v2/pkg/grpctest/mapping/mapping.go (1)
DefaultGRPCMapping(10-619)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
Message(117-121)
⏰ 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: Linters
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (26)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (1)
821-823: LGTM!The helper method cleanly encapsulates the nullable scalar check logic and is consistently used across the visitor methods.
v2/pkg/grpctest/mapping/mapping.go (1)
74-616: Mapping additions are well-structured and consistentThe new RPC and field mappings for nullable fields follow the established patterns and naming conventions. All required mapping fields are properly configured.
execution/engine/execution_engine_grpc_test.go (1)
502-811: Comprehensive test coverage for nullable fields functionality.The test suite thoroughly covers various scenarios for nullable fields including queries with default values, by ID with varying data completeness, filters, and mutations. The test structure is consistent and assertions are appropriate.
v2/pkg/engine/datasource/grpc_datasource/compiler.go (2)
275-316: Well-structured refactoring for multi-file protobuf support.The refactoring properly handles imported protobuf files by processing them before the main schema file, ensuring correct dependency resolution. The new
processFilemethod effectively encapsulates the parsing logic.
446-467: Correct implementation of optional field handling.The logic properly handles optional scalar fields by wrapping them in protobuf wrapper types, correctly skipping fields without values to represent null.
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
14-17: Well-documented additions for nullable field support.The constant and struct field additions are properly documented and provide the necessary infrastructure for handling optional scalar fields wrapped in protobuf messages.
Also applies to: 86-88, 169-173
v2/pkg/grpctest/testdata/products.graphqls (1)
167-194: Well-designed GraphQL schema additions for nullable fields.The schema additions follow GraphQL best practices with clear type definitions, appropriate use of nullable vs non-nullable fields, and comprehensive query/mutation operations for testing nullable field scenarios.
Also applies to: 223-227, 240-242
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (4)
17-22: Good test structure improvement!The introduction of the
testCasestruct standardizes test case definitions and improves test maintainability.
23-53: Excellent refactoring to reduce test duplication!The
runTesthelper function effectively consolidates common test logic, making tests more maintainable and consistent.
1745-1746: Correct implementation of optional field marking.The addition of
Optional: trueto thelimitfield inSearchInputproperly indicates nullable field support in the execution plan.Also applies to: 1937-1938
3161-3797: Comprehensive test coverage for nullable fields!The
TestNullableFieldsExecutionPlanfunction provides excellent coverage of nullable field scenarios including queries, mutations, filters, and various field selection patterns. The tests properly validate that optional fields are marked withOptional: truein the execution plan.v2/pkg/grpctest/schema.go (3)
199-262: Well-structured field configuration additions.The new field configurations for nullable fields queries and mutations follow the established pattern and properly define argument sources.
297-308: Consistent metadata updates for new schema fields.The root node metadata properly includes all new nullable fields queries and mutations.
Also applies to: 314-316
448-583: Comprehensive type definitions for nullable fields support.The child node metadata additions include all necessary types and their fields for the nullable fields functionality, maintaining consistency with the existing schema structure.
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (3)
87-106: Proper RPC configuration for nullable fields operations.The new RPC configurations correctly map GraphQL operations to their corresponding gRPC methods with appropriate request/response types.
Also applies to: 119-128
232-249: Consistent field mappings for nullable operations.The Query and Mutation field mappings properly define target names and argument mappings following the snake_case convention.
Also applies to: 264-276
537-596: Complete field mappings for nullable types.The field mappings for
NullableFieldsType,NullableFieldsInput, andNullableFieldsFiltercorrectly map all fields with proper snake_case naming for protobuf compatibility.v2/pkg/grpctest/mockservice.go (6)
12-12: LGTM! Correct import for protobuf wrapper types.The addition of the wrapperspb import is necessary for handling nullable scalar fields using protobuf wrapper types.
52-89: Well-implemented update method with proper error case handling.The method correctly handles nullable fields and includes test case support for non-existent IDs, which is valuable for testing error scenarios.
91-136: Excellent test data coverage for nullable fields!The method provides comprehensive test scenarios:
- Full data with all optional fields populated
- Partial data with mixed null/non-null optional fields
- Minimal data with all optional fields null
The clear comments make the test cases easy to understand.
138-155: Good default test case implementation.The method provides a useful default test case with mixed null/non-null optional fields, which is helpful for testing nullable field handling.
157-222: Comprehensive test scenarios with clever dynamic data generation!The implementation provides excellent test coverage:
- Special cases for null responses ("not-found", "null-test")
- Named test cases for different nullable field combinations
- Dynamic data generation based on ID characteristics in the default case
The use of ID length for generating consistent test data is particularly clever.
224-300: Well-designed filtering implementation with nullable field support.The method handles filtering comprehensively:
- Graceful handling of nil filters
- Proper extraction of filter values with nil checks
- Sophisticated logic for varying nullable fields based on
includeNullsflag- Good test data diversity using index-based variations
v2/pkg/grpctest/product.proto (3)
4-4: Proper protobuf wrapper import and well-structured RPC definitions.The import of
google/protobuf/wrappers.protois correctly placed, and the new RPCs follow the established naming conventions with clear documentation.Also applies to: 32-38
248-294: Well-structured request/response messages following protobuf conventions.All message definitions are properly structured with:
- Consistent naming patterns
- Appropriate field types and numbering
- Clear comments for each operation
473-499: Correct implementation of nullable fields using protobuf wrapper types.The message definitions properly distinguish between:
- Optional fields using wrapper types (
google.protobuf.StringValue, etc.)- Required fields using regular scalar types
This follows protobuf best practices for representing nullable scalar fields in proto3.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.204](v2.0.0-rc.203...v2.0.0-rc.204) (2025-07-10) ### Features * support nullable base types ([#1212](#1212)) ([b45b92c](b45b92c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for nullable base types. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.204, including the new feature. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Checklist