fix(protographic): use proto wrapper for nullable scalars#2032
Conversation
WalkthroughThe changes introduce support for Protocol Buffer wrapper types for nullable GraphQL scalar fields in the Protographic project. Documentation, source code, and test cases were updated to map nullable GraphQL scalars to Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
protographic/src/sdl-to-proto-visitor.ts (2)
1440-1489: Unused innerMostType parameter in createNestedListWrapper.The method signature was updated to accept
innerMostTypefor preserving nullability information, but this parameter is not used in the method body. The proto type is still derived frombaseTypeon line 1478.Consider using
innerMostTypeinstead ofbaseTypewhen determining the proto type to properly handle nullability for nested lists.- const protoType = this.getProtoTypeFromGraphQL(baseType, true); + const protoType = this.getProtoTypeFromGraphQL(innerMostType, true);
1398-1426: Fix nested-list nullability handlingThe
innerMostTypeyou compute in getProtoTypeFromGraphQL is never actually used when building the wrapper. As a result, the null-vs-non-null semantics for nested lists aren’t preserved.Locations to update:
- protographic/src/sdl-to-proto-visitor.ts:
• getProtoTypeFromGraphQL (≈ lines 1398–1409) – you trackinnerMostTypebut don’t apply it
• createNestedListWrapper signature (line 1440) –innerMostType: GraphQLTypeis passed in but never referencedRecommended changes:
- In createNestedListWrapper, inspect
innerMostType(e.g. viaisNonNullType(innerMostType)) and use it to decide whether the wrapper or itsresultfield should be optional vs. required in the proto- If nullability shouldn’t affect the wrapper, remove the unused
innerMostTypeparameter and simplify the logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
protographic/README.md(1 hunks)protographic/SDL_PROTO_RULES.md(4 hunks)protographic/src/sdl-to-proto-visitor.ts(8 hunks)protographic/tests/sdl-to-proto/01-basic-types.test.ts(11 hunks)protographic/tests/sdl-to-proto/02-complex-types.test.ts(7 hunks)protographic/tests/sdl-to-proto/04-federation.test.ts(2 hunks)protographic/tests/sdl-to-proto/05-edge-cases.test.ts(16 hunks)protographic/tests/sdl-to-proto/09-comments.test.ts(7 hunks)protographic/tests/sdl-to-proto/10-options.test.ts(2 hunks)protographic/tests/util.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
protographic/tests/sdl-to-proto/01-basic-types.test.ts (2)
protographic/src/index.ts (1)
compileGraphQLToProto(52-78)protographic/tests/util.ts (1)
expectValidProto(30-32)
🔇 Additional comments (23)
protographic/README.md (1)
21-21: Documentation accurately describes the new wrapper type feature.The added line clearly communicates the value of using Protocol Buffer wrappers to distinguish semantic nulls from zero values, which aligns with the PR objectives.
protographic/tests/sdl-to-proto/04-federation.test.ts (2)
402-402: Correctly imports wrapper types when needed.The conditional import of
google/protobuf/wrappers.protois properly added to support the wrapper types used in the proto definition.
430-432: Proper use of wrapper types for nullable scalar fields.The nullable scalar fields (
endTime,metadata,attachment) are correctly mapped togoogle.protobuf.StringValuewrapper types, allowing proper distinction between null and empty string values.protographic/tests/sdl-to-proto/10-options.test.ts (2)
29-29: Correctly imports wrapper types for comprehensive scalar support.The import statement is properly added to support the various wrapper types used throughout the proto definition.
45-73: Comprehensive wrapper type mapping for all scalar types.All GraphQL scalar types are correctly mapped to their corresponding protobuf wrapper types:
- String →
google.protobuf.StringValue- Int →
google.protobuf.Int32Value- Float →
google.protobuf.DoubleValue- Boolean →
google.protobuf.BoolValue- ID →
google.protobuf.StringValueThis ensures consistent nullability handling across all scalar types.
protographic/tests/util.ts (1)
11-22: Properly supports wrapper type validation.The function correctly preloads the protobuf wrapper definitions before parsing proto text, ensuring that wrapper type references are resolved during validation. The implementation follows protobufjs best practices by:
- Creating a new Root instance
- Loading wrapper definitions first
- Parsing with the pre-loaded root
- Calling resolveAll() for complete resolution
protographic/tests/sdl-to-proto/09-comments.test.ts (2)
85-85: Consistent wrapper type imports across all test cases.The import statements for
google/protobuf/wrappers.protoare correctly added in each test case that uses wrapper types, ensuring proper proto compilation.Also applies to: 236-236, 379-379
117-119: Comprehensive wrapper type usage for nullable fields.The nullable scalar fields are consistently mapped to their appropriate wrapper types across different scenarios:
- Pagination parameters (
offset,limit) →google.protobuf.Int32Value- User fields (
name,age) →google.protobuf.StringValueandgoogle.protobuf.Int32Value- Optional descriptions →
google.protobuf.StringValueThis demonstrates thorough coverage of nullable field handling with wrapper types.
Also applies to: 144-145, 314-314, 511-511
protographic/SDL_PROTO_RULES.md (3)
56-63: LGTM! Clear documentation of nullable scalar type mappings.The table clearly distinguishes between non-null and nullable GraphQL scalar types and their corresponding Protocol Buffer types. The wrapper types are correctly mapped according to protobuf specifications.
289-299: Example correctly demonstrates wrapper type usage.The example properly shows:
- The required import statement for wrapper types
- Correct application of wrapper types to nullable fields only
- Non-null fields remaining as primitive types
317-351: Field reservation examples properly updated for wrapper types.The examples correctly show how nullable fields with wrapper types work alongside the field reservation mechanism, maintaining backward compatibility during schema evolution.
protographic/tests/sdl-to-proto/01-basic-types.test.ts (5)
23-74: Test correctly validates wrapper type conversion for nullable scalars.The test properly expects wrapper types for all nullable scalar fields and includes the required import statement.
186-211: Correctly handles mixed nullable and non-null fields.The test properly expects wrapper types only for nullable fields while keeping primitive types for non-null fields.
231-267: Request arguments properly use wrapper types for nullable fields.The test correctly expects wrapper types for nullable query arguments (offset, nameFilter).
287-308: Import placement correctly respects existing options.The test validates that the wrapper import is properly placed after package options.
520-588: Excellent comprehensive test coverage for nullable scalar conversions.This new test thoroughly validates the wrapper type conversion for all GraphQL scalar types, ensuring complete coverage of the feature.
protographic/tests/sdl-to-proto/02-complex-types.test.ts (2)
92-133: Input types correctly use wrapper types for nullable fields.The test properly validates that nullable fields in both input types and regular types use wrapper types.
287-327: Nested input types properly handle nullable fields with wrapper types.The test correctly validates wrapper type usage in complex nested input structures.
protographic/tests/sdl-to-proto/05-edge-cases.test.ts (2)
6-6: Verify the removal of the empty schema test case.The AI summary indicates an empty schema test was removed. Please confirm this removal was intentional and doesn't reduce test coverage for edge cases.
25-643: Edge case tests properly updated for wrapper types.All edge case scenarios correctly use wrapper types for nullable fields, maintaining comprehensive test coverage.
protographic/src/sdl-to-proto-visitor.ts (3)
52-65: Wrapper type mappings are correctly defined.The
SCALAR_WRAPPER_TYPE_MAPcorrectly maps GraphQL scalar types to their corresponding Protocol Buffer wrapper types according to the protobuf specification.
444-462: Import insertion logic correctly handles proto file structure.The implementation properly places the wrapper import after the package declaration and any existing options, maintaining valid protobuf syntax.
1368-1390: Nullability handling logic is correctly implemented.The method properly distinguishes between nullable and non-null scalar types, using wrapper types only for nullable scalars. The
ignoreWrapperTypesparameter provides necessary flexibility for special cases.
2 participants
Use
google/protobuf/wrappers.protowrappers to accurately represent semantic nulls vs zero values.Corresponding engine changes are in this PR by @Noroth - wundergraph/graphql-go-tools#1212
This change only supports Nullable Scalar types. Support for Nullable fields within Lists/Nest lists will follow. Currently in discussion.
Summary by CodeRabbit
Documentation
New Features
Tests
Chores
Checklist