feat: redesign list handling in protographic #2058
Conversation
WalkthroughThe changes overhaul the handling of GraphQL list types in the SDL-to-Proto conversion logic by introducing a formalized wrapper message mechanism for lists. The conversion code is refactored to generate wrapper messages for nullable and nested lists, and all test snapshots are updated to reflect this new structure. A comprehensive new test suite for list scenarios is added, and the test workspace configuration is updated to include the protographic module. Documentation on list handling is expanded with detailed explanations and examples. Additional tests verify field number preservation for generated wrapper messages. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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: 1
🔭 Outside diff range comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1-1: Use theformatscript or run Prettier directly in protographicThe
npm run prettierscript doesn’t exist in protographic. To fix formatting insrc/sdl-to-proto-visitor.ts, either:
- From the repo root:
cd protographic npm run format -- src/sdl-to-proto-visitor.ts- Or run Prettier via npx:
npx prettier --write protographic/src/sdl-to-proto-visitor.ts
🧹 Nitpick comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1526-1595: Solid implementation of wrapper message generation.The wrapper message generation correctly handles different nesting levels and properly caches results. The generated message structures align with Protocol Buffer best practices for representing nullable and nested lists.
Consider using the existing
getIndent()method instead of the inlineformatIndentfunction for consistency:- const formatIndent = (indent: number, content: string) => { - return ' '.repeat(indent) + content; - } + const formatIndent = (indent: number, content: string) => { + const savedIndent = this.indent; + this.indent = indent; + const result = this.getIndent() + content; + this.indent = savedIndent; + return result; + }Or simply use template literals with manual spacing since it's a local helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
protographic/src/sdl-to-proto-visitor.ts(7 hunks)protographic/tests/sdl-to-proto/01-basic-types.test.ts(8 hunks)protographic/tests/sdl-to-proto/02-complex-types.test.ts(4 hunks)protographic/tests/sdl-to-proto/05-edge-cases.test.ts(5 hunks)protographic/tests/sdl-to-proto/11-lists.test.ts(1 hunks)vitest.workspace.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Protographic CI
protographic/src/sdl-to-proto-visitor.ts
[warning] 1-1: Prettier formatting check failed. Code style issues found. Run 'prettier --write' to fix code style issues.
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
vitest.workspace.ts (1)
1-1: LGTM! Workspace configuration updated correctly.The addition of 'protographic' to the workspace modules is necessary for including the new module in the test infrastructure.
protographic/tests/sdl-to-proto/02-complex-types.test.ts (1)
39-50: Wrapper message pattern correctly applied for nullable lists.The changes consistently introduce wrapper messages for nullable lists following the
ListOfXpattern. This aligns with the PR's objective to redesign list handling in protographic.Also applies to: 232-257, 306-329
protographic/tests/sdl-to-proto/05-edge-cases.test.ts (1)
367-381: Edge case handling follows the established wrapper message pattern.The wrapper messages are correctly applied for nullable lists across various GraphQL constructs including object types and input types. The implementation maintains consistency throughout the complex schema.
Also applies to: 570-626
protographic/tests/sdl-to-proto/11-lists.test.ts (1)
1-1183: Comprehensive test suite for list handling redesign.This new test file provides excellent coverage of the list handling functionality with test cases covering:
- Various nullability combinations
- Different nesting levels
- All GraphQL type categories (scalars, objects, enums, interfaces, unions)
- Input types and mutations
- Edge cases
The tests properly validate the core design principle: non-nullable lists use
repeatedfields directly, while nullable lists use wrapper messages.protographic/tests/sdl-to-proto/01-basic-types.test.ts (2)
384-384: Verify: GraphQL schema change makes list argument non-nullable.The
tagsparameter changed from[String]to[String]!, making it a required non-nullable list. This is a functional change to the GraphQL schema, not just a proto generation update. Please confirm this change is intentional as it could be a breaking change for existing clients.
143-153: List handling pattern correctly applied across various scenarios.The changes consistently apply the wrapper message pattern:
- Nullable lists (
[T]) useListOfTwrapper messages- Non-nullable lists (
[T]!and[T!]!) userepeatedfields directly- Nested nullable lists create appropriately nested wrapper messages
- The pattern works correctly with recursive types
Also applies to: 480-540, 688-746
protographic/src/sdl-to-proto-visitor.ts (9)
9-9: Import addition looks good.The
GraphQLListimport is necessary for the new list handling functionality.
88-95: Good design choice for the ProtoType interface.Encapsulating the type name and repeated flag in a single interface improves type safety and makes the API more explicit.
894-898: Clean refactoring to use ProtoType.The updated code properly uses the
ProtoTypeinterface, making the repeated field handling more explicit and maintainable.
960-964: Consistent ProtoType usage in response messages.The refactoring maintains consistency with the request message handling.
1124-1128: Consistent ProtoType adoption in object type processing.Good consistency in applying the ProtoType pattern throughout the codebase.
1195-1199: ProtoType pattern correctly applied to input object types.Maintains consistency with other type processing methods.
1420-1456: Well-structured refactoring of getProtoTypeFromGraphQL.The method now properly delegates list handling to a dedicated method and consistently returns ProtoType objects. The logic flow is preserved for non-list types.
1458-1496: Excellent implementation of list type handling.The method provides a comprehensive solution for converting GraphQL list types to Protocol Buffers, with clear distinction between simple repeated fields and cases requiring wrapper messages. The documentation with examples is particularly helpful.
1498-1524: Well-implemented helper methods for list analysis.The
isNestedListTypeandcalculateNestingLevelmethods correctly handle the GraphQL type tree traversal, including proper handling of NonNull wrappers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1422-1458: Good refactoring with improved separation of concerns.The refactored method has better structure with list handling delegated to
handleListType. TheProtoTypereturn type improves type safety and clarity.Note: The existing review comment about renaming
typetographqlTypeto avoid shadowing the type keyword is still valid and should be addressed.
🧹 Nitpick comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1460-1616: Comprehensive implementation of list handling with good separation of concerns.The new list handling methods provide a robust solution for converting GraphQL lists to Protocol Buffer representations. The logic correctly handles:
- Simple repeated fields for non-nullable single-level lists
- Wrapper messages for nullable or nested lists
- Proper nesting level calculations
- Type guards for nested list detection
The code is well-structured with clear method responsibilities.
Consider extracting the
formatIndentfunction (lines 1594-1596) to a class method since it's a utility that could be reused elsewhere:+ private formatIndent(indent: number, content: string): string { + return ' '.repeat(indent) + content; + } + private buildWrapperMessage(wrapperName: string, level: number, baseType: GraphQLNamedType): string[] { const lines: string[] = []; - const formatIndent = (indent: number, content: string) => { - return ' '.repeat(indent) + content; - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protographic/src/sdl-to-proto-visitor.ts(7 hunks)
⏰ 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: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
protographic/src/sdl-to-proto-visitor.ts (3)
90-96: Well-designed interface for type information encapsulation.The
ProtoTypeinterface cleanly encapsulates both the type name and repetition information, which improves code organization and makes the list handling logic more explicit.
883-883: Consistent and improved usage of ProtoType interface.The usage of
ProtoTypethroughout the file follows a consistent pattern:
- Extract
isRepeatedandtypeNameproperties- Generate appropriate field declarations based on repetition
- Use explicit type information instead of ad-hoc checks
This improves code maintainability and makes the list handling logic more explicit and predictable.
Also applies to: 896-900, 952-952, 962-966, 1115-1115, 1126-1130, 1186-1186, 1197-1201
1565-1579: Improved wrapper creation with better separation of concerns.The refactored
createNestedListWrappermethod now properly delegates message building tobuildWrapperMessagewhile maintaining the caching logic. The explicit parameters make the method's purpose clearer.
…ture-in-protographic
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1-1: Fix prettier formatting issues to pass CI.The pipeline is failing due to prettier formatting issues. This needs to be resolved before the PR can be merged.
Please run the following command to fix the formatting issues:
prettier --write protographic/src/sdl-to-proto-visitor.tsAlternatively, if you have prettier configured in your package.json scripts:
npm run format # or yarn format
♻️ Duplicate comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1476-1540: Previous review feedback addressed.The refactoring successfully addresses the previous review comment about extracting type flattening logic into helper functions. The new
handleListType,unwrapNonNullType,isNestedListType, andcalculateNestingLevelmethods provide clear separation of concerns and make the type handling logic more maintainable.
🧹 Nitpick comments (1)
protographic/tests/sdl-to-proto/06-field-ordering.test.ts (1)
13-13: Remove unused import.The
isNullfunction from lodash-es is imported but never used in the file.-import { isNull } from 'lodash-es';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
protographic/src/sdl-to-proto-visitor.ts(7 hunks)protographic/tests/sdl-to-proto/06-field-ordering.test.ts(2 hunks)protographic/tests/sdl-to-proto/11-lists.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Protographic CI
protographic/tests/sdl-to-proto/06-field-ordering.test.ts
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
protographic/src/sdl-to-proto-visitor.ts
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
protographic/tests/sdl-to-proto/06-field-ordering.test.ts (1)
1128-1442: Comprehensive test coverage for list wrapper types.The new test suite provides excellent coverage of the list wrapper functionality:
- Simple nullable lists generating wrapper messages with consistent field numbering
- Nested lists with appropriate naming conventions (ListOfListOfX)
- Field number preservation across schema modifications
- Proper exclusion of wrapper types from lock data
- Mixed scenarios with additions and removals
The test logic correctly validates that wrapper types use deterministic field numbers (field 1) and appropriate field names ('items' for simple wrappers, 'list' for nested wrappers).
protographic/tests/sdl-to-proto/11-lists.test.ts (1)
5-2011: Excellent comprehensive test coverage for list handling.This test suite provides outstanding coverage of the new list wrapper approach:
- Complete scenario coverage: Tests cover all GraphQL types (scalars, objects, enums, interfaces, unions, input types) with various list configurations
- Nullability handling: Proper distinction between nullable lists (wrapper messages) and non-nullable lists (direct repeated fields)
- Nesting patterns: Correct handling of nested lists with appropriate wrapper message structures
- Edge cases: Comprehensive coverage of various nullability combinations and edge cases
- Complex scenarios: Recursive types, deeply nested structures, and mixed type scenarios
- Mutation support: Input types and mutations with list parameters
- Documentation: Inline snapshots provide clear documentation of expected protobuf output
The test logic correctly validates:
- Non-nullable lists generate
repeatedfields directly- Nullable lists generate wrapper messages with
itemsfield (field number 1)- Nested nullable lists generate wrapper messages with
listfield (field number 1)- Proper naming conventions (ListOfX, ListOfListOfX, etc.)
- Consistent field numbering across all wrapper types
protographic/src/sdl-to-proto-visitor.ts (4)
90-97: Good abstraction with the new ProtoType interface.The
ProtoTypeinterface cleanly encapsulates both the type name and repetition flag, which should make the code more maintainable and reduce the complexity of handling list types throughout the codebase.
1422-1458: Approve the refactored type conversion method.The refactored
getProtoTypeFromGraphQLmethod properly separates concerns by handling list types first, then delegating to appropriate handlers. The consistent return ofProtoTypeobjects throughout all branches is well-implemented.The comment on line 1423 suggests there might be edge cases where nullable lists could be mishandled as scalar types. Please verify that all GraphQL list type combinations are properly handled by running comprehensive tests.
896-900: Excellent consistent refactoring to use ProtoType.isRepeated.The refactoring successfully centralizes the logic for determining repeated fields through the
ProtoType.isRepeatedflag. This eliminates the need for ad-hoc list type checking throughout the codebase and makes the implementation more maintainable.Also applies to: 962-966, 1126-1130, 1197-1201
1476-1540: List handling logic is well-tested and can be approved.The existing tests in
protographic/tests/sdl-to-proto/11-lists.test.ts, along with the basic-types and field-ordering suites, cover:
- Deeply nested lists with mixed nullability
- Lists of complex types (objects, interfaces, unions)
- Mixed nullable/non-nullable and nested/non-nested combinations
No further edge cases remain untested. Great work!
Noroth
deleted the
ludwig/eng-7580-implement-nullable-lists-feature-in-protographic
branch
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Checklist