feat(cli,protographic): improvements to operation validation and proto generation #2436
Conversation
Add wg.connectrpc.graphql_variable_name field option to map proto fields to non-standard GraphQL variable names. Auto-imports annotations.proto and generates proper field option syntax in proto output.
Use full dot-notation paths to prevent field number conflicts between nested messages with the same name in different parent contexts.
Relaxes validation to accept names like GETUSER, HRService, APIHandler. Operation names must start with uppercase letter and contain only letters/numbers.
asoorm
requested review from
JivusAyrus,
Noroth,
StarpTech,
endigma and
jensneuse
as code owners
WalkthroughRemoved the CLI flag Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2436 +/- ##
===========================================
- Coverage 62.75% 43.36% -19.39%
===========================================
Files 296 144 -152
Lines 41353 14401 -26952
Branches 4244 1139 -3105
===========================================
- Hits 25951 6245 -19706
+ Misses 15381 8154 -7227
+ Partials 21 2 -19
🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protographic/OPERATIONS_TO_PROTO.md (1)
158-158: CLI does not expose--prefix-operation-typeoption despite documentation reference.The
prefixOperationTypeoption exists in the underlying protographic library with full implementation and test coverage, but the CLI command atcli/src/commands/grpc-service/commands/generate.tsdoes not pass this option tocompileOperationsToProto. The documentation at lines 158 and 179-187 should either be removed or the option should be exposed in the CLI.
🧹 Nitpick comments (2)
protographic/src/operation-to-proto.ts (1)
287-289: Consider simplifying the error message.The error message is quite verbose with three sentences explaining the same constraint. Consider consolidating for clarity:
🔎 Proposed simplification
throw new Error( `Root-level field alias "${selection.alias.value}: ${selection.name.value}" is not supported. ` + - 'Field aliases at the root level break proto schema generation consistency. ' + - 'Each GraphQL field must map to exactly one proto field name. ' + - 'Please remove the alias or use it only on nested fields.', + 'Each GraphQL field must map to exactly one proto field name for deterministic schema generation. ' + + 'Please remove the alias or use it only on nested fields.', );protographic/tests/operations/operation-validation.test.ts (1)
262-275: Consider adding a test for names starting with numbers.While the regex
/^[A-Z][a-zA-Z0-9]*$/correctly rejects names starting with numbers, there's no explicit test case for this scenario (e.g.,query 123GetUser). Although GraphQL itself may not allow this, adding a test would make the validation boundary explicit.🔎 Suggested additional test
test('should reject operation names starting with numbers', () => { const operation = ` query 1GetUser { user { id name } } `; expect(() => { compileOperationsToProto(operation, schema); }).toThrow(/Operation name.*must start with an uppercase letter/); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cli/src/commands/grpc-service/commands/generate.tsprotographic/OPERATIONS_TO_PROTO.mdprotographic/src/naming-conventions.tsprotographic/src/operation-to-proto.tsprotographic/src/operations/message-builder.tsprotographic/src/operations/proto-field-options.tsprotographic/src/operations/proto-text-generator.tsprotographic/src/operations/request-builder.tsprotographic/tests/operations/fragments.test.tsprotographic/tests/operations/nested-message-field-numbering.test.tsprotographic/tests/operations/operation-validation.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
protographic/tests/operations/operation-validation.test.tsprotographic/OPERATIONS_TO_PROTO.mdprotographic/src/operation-to-proto.ts
📚 Learning: 2025-10-27T09:45:41.622Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.
Applied to files:
protographic/tests/operations/nested-message-field-numbering.test.ts
📚 Learning: 2025-08-20T10:38:49.191Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.
Applied to files:
protographic/src/operations/proto-field-options.tsprotographic/src/operations/proto-text-generator.ts
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
protographic/src/operations/proto-text-generator.ts
🧬 Code graph analysis (5)
protographic/tests/operations/operation-validation.test.ts (1)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(87-141)
protographic/tests/operations/nested-message-field-numbering.test.ts (1)
protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/operations/request-builder.ts (2)
protographic/src/naming-conventions.ts (1)
protoFieldToProtoJSON(33-35)protographic/src/operations/proto-field-options.ts (1)
GRAPHQL_VARIABLE_NAME(16-19)
protographic/src/operations/proto-text-generator.ts (2)
protographic/src/sdl-to-proto-visitor.ts (1)
formatIndent(2217-2219)protographic/src/operations/proto-field-options.ts (1)
GRAPHQL_VARIABLE_NAME(16-19)
protographic/src/operations/message-builder.ts (2)
protographic/src/operations/field-numbering.ts (2)
assignFieldNumbersFromLockData(212-229)FieldNumberManager(14-68)protographic/src/index.ts (2)
FieldNumberManager(113-113)buildMessageFromSelectionSet(116-116)
⏰ 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). (3)
- GitHub Check: codecov/patch
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
cli/src/commands/grpc-service/commands/generate.ts (1)
347-348: LGTM!The comment update accurately reflects the shift from "reversibility" to "proto schema consistency," which aligns with the broader PR objectives around deterministic proto generation.
protographic/src/naming-conventions.ts (1)
29-36: LGTM!The new
protoFieldToProtoJSONutility correctly implements the inverse transformation needed for proto JSON field naming. The implementation usingcamelCasefrom lodash-es is appropriate and follows the established patterns in this module.protographic/tests/operations/fragments.test.ts (1)
1264-1267: LGTM!The updated inline snapshot correctly reflects the new path-based field numbering for nested messages. The
valuefield now has tag2(followingid = 1) because nestedChildmessages are tracked independently with their own field number counters, which aligns with the PR's objective for deterministic proto schema generation.protographic/OPERATIONS_TO_PROTO.md (1)
719-721: LGTM!The updated limitation wording accurately reflects the shift toward deterministic proto schema generation and clarifies the one-to-one mapping requirement between GraphQL fields and proto field names.
protographic/src/operations/request-builder.ts (2)
21-23: LGTM!The new imports correctly bring in the naming utility and field option constant needed for GraphQL variable name preservation.
160-171: LGTM!The implementation correctly detects when a GraphQL variable name diverges from the expected proto JSON naming (camelCase of snake_case) and stores the original name as a custom field option. This ensures proper mapping during request handling while maintaining proto schema determinism.
The lazy initialization of
field.optionsand the use of the centralizedGRAPHQL_VARIABLE_NAME.optionNameconstant are both well-implemented.protographic/src/operations/proto-text-generator.ts (4)
4-4: LGTM!Import correctly added for the field option constant.
98-130: LGTM!The conditional logic correctly detects usage of
graphql_variable_nameand emits the required import (google/protobuf/descriptor.proto) and extension block only when needed. This avoids unnecessary proto file bloat when the option isn't used. Based on learnings, inlineextend google.protobuf.FieldOptionsdeclarations work correctly with protoc.
305-322: LGTM!The field formatting logic correctly handles options by:
- Rendering with brackets
[key = value]when options exist- Properly quoting string values
- Falling back to the plain syntax when no options are present
The extension option keys (like
(graphql_variable_name)) are already parenthesized and will render correctly.
456-491: LGTM!The detection helpers follow the same pattern as the existing
detectWrapperTypeUsageandmessageUsesWrapperTypesfunctions, maintaining consistency. The recursive traversal correctly checks both direct fields and nested messages for thegraphql_variable_nameoption.protographic/src/operations/proto-field-options.ts (1)
1-19: LGTM!Well-structured module that centralizes the proto field option definition. Key observations:
- Field number 50001 is correctly within the user-defined extension range (50000-99999)
- The
optionNameincludes parentheses for proper extension option syntax- Using
as constensures the constant is immutable- The
ProtoFieldOptioninterface provides good typing for future extensibilityprotographic/tests/operations/nested-message-field-numbering.test.ts (1)
1-501: LGTM!Excellent test coverage for the new nested message field numbering feature. The test suite comprehensively validates:
- Basic numbering - Fields start at 1 in nested messages
- Independent counters - Sibling messages with identical names have separate field number spaces
- Field stability - Existing field numbers are preserved when adding new fields
- Reserved handling - Removed field numbers are properly reserved for backward compatibility
- Deep nesting - 5+ level hierarchies work correctly with full dot-notation paths
The inline snapshots serve as clear documentation of expected behavior, and the
lockDataassertions verify the internal tracking mechanism using full path notation (e.g.,GetEmployeeResponse.GetEmployee.Employee.Details).protographic/src/operation-to-proto.ts (2)
267-279: LGTM! Clear validation with helpful error messaging.The operation name validation correctly enforces protobuf RPC naming conventions while remaining flexible (allowing both PascalCase and all-UPPERCASE). The error message provides concrete examples and a suggested name based on the invalid input.
295-303: Operation names are already validated to the required format; using the name as-is is safe.The validation at lines 270–274 enforces that operation names match
/^[A-Z][a-zA-Z0-9]*$/, rejecting camelCase and snake_case with a helpful error message. Since invalid names are rejected upfront, usingoperationNamedirectly at line 297 is correct—no transformation is needed. This is not a breaking change; it's stricter validation that guides users toward valid names (PascalCase or UPPERCASE).protographic/tests/operations/operation-validation.test.ts (1)
201-361: Excellent test coverage for operation name validation!The test suite comprehensively validates the new operation name requirements:
- ✅ Accepts valid patterns (PascalCase, UPPERCASE, alphanumeric combinations)
- ✅ Rejects invalid patterns (camelCase, snake_case)
- ✅ Validates helpful error messages
- ✅ Covers all operation types (query, mutation, subscription)
The tests align well with the implementation and provide clear regression protection.
protographic/src/operations/message-builder.ts (3)
81-81: LGTM! Clear hierarchical path tracking for nested messages.The addition of
parentPathand computation offullPathcorrectly implements hierarchical tracking to prevent field number collisions between nested messages with identical names in different parent contexts.Also applies to: 86-88
361-362: The currentPath computation and hierarchical path tracking for nested messages is correct and well-tested.At line 361,
currentPathcorrectly captures either the passedmessagePath(parent's full path) or the currentmessage.name. This is properly propagated tobuildMessageFromSelectionSetat line 369, where line 88 constructs the full hierarchical path via${parentPath}.${messageName}.The test suite comprehensively validates this structure across multiple scenarios: simple nesting, sibling messages with identical names (confirming independent field counters), field preservation during schema evolution, and deeply nested hierarchies (5+ levels). Lock file tracking uses full dot-notation paths (e.g.,
GetEmployeeResponse.GetEmployee.Details) to uniquely identify each nested message context.
211-230: Function signature change correctly implemented across all call sites.The
getOrAssignFieldNumberfunction signature has been properly updated with a single call site inprocessFieldSelection(line 400) correctly passing all arguments in the right order:currentPath(derived frommessagePath || message.name),protoFieldName,fieldNumberManager, andmessage. No compatibility issues found.
Ensures that messagePath parameter is properly propagated through processInlineFragment and processFragmentSpread functions to maintain correct field number tracking for nested messages created within fragments. This fixes an issue where nested messages in fragments would lose their path context, potentially causing field number collisions in the proto lock file mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
protographic/src/operations/message-builder.ts (2)
211-230: Signature change correctly supports path-based numbering.The refactoring from type-based to path-based field number lookup is essential for the nested message tracking feature. The implementation correctly uses
messagePathfor all field number operations.Minor consideration: Line 229's fallback
return message ? message.fieldsArray.length + 1 : 1returns 1 when bothfieldNumberManagerandmessageare absent, which would cause conflicts if called repeatedly. Current usage (line 400) always passesmessage, so this is safe, but the API surface could be more defensive.Optional: Consider making `message` required when `fieldNumberManager` is absent
function getOrAssignFieldNumber( messagePath: string, protoFieldName: string, fieldNumberManager?: FieldNumberManager, - message?: protobuf.Type, + message: protobuf.Type | undefined, ): number { const existingFieldNumber = fieldNumberManager?.getFieldNumber(messagePath, protoFieldName); if (existingFieldNumber !== undefined) { return existingFieldNumber; } if (fieldNumberManager) { const fieldNumber = fieldNumberManager.getNextFieldNumber(messagePath); fieldNumberManager.assignFieldNumber(messagePath, protoFieldName, fieldNumber); return fieldNumber; } + if (!message) { + throw new Error( + `Cannot assign field number without fieldNumberManager or message reference for path "${messagePath}"` + ); + } return message.fieldsArray.length + 1; }
65-82: Update JSDoc to document theparentPathparameter.The function signature now includes
parentPath?: stringfor hierarchical path tracking, but the JSDoc comment doesn't document this parameter.📝 Proposed JSDoc update
/** * Builds a Protocol Buffer message type from a GraphQL selection set * * @param messageName - The name for the proto message * @param selectionSet - The GraphQL selection set to convert * @param parentType - The GraphQL type that contains these selections * @param typeInfo - TypeInfo for resolving field types * @param options - Optional configuration + * @param parentPath - Optional parent message path for nested message field number tracking * @returns A protobuf Type object */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protographic/src/operations/message-builder.ts
⏰ 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 (9)
protographic/src/operations/message-builder.ts (9)
32-37: LGTM! Depth guard constants are well-defined.The constants provide clear defaults for recursion depth tracking, with 50 being a reasonable limit for typical GraphQL operations while preventing stack overflow.
81-88: Excellent addition of hierarchical path tracking.The
parentPathparameter andfullPathconstruction correctly implement nested message path tracking for field number collision prevention. The logic properly handles both root-level messages (no parent) and nested messages (concatenated path).
99-125: Robust depth guarding implementation.The two-level depth checking (at message building entrance and during recursive field collection) provides strong protection against stack overflow. The incremental depth tracking through
collectFieldsrecursive calls (lines 147, 151, 162) and nested message creation (line 357) is correct and comprehensive.
176-184: Critical fix: Path-based field numbering prevents collisions.Using
fullPathinstead ofmessageNameforreconcileFieldOrderandassignFieldNumbersFromLockDatacorrectly enables hierarchical field number tracking, preventing collisions between identically-named messages in different nesting contexts.
193-201: Fragment processing fix confirmed - path propagation is correct.The
fullPathis now properly passed toprocessFieldSelection, ensuring nested messages created through fragments have correct path tracking. This directly addresses the previous review concern about field number collisions in fragment-based nested messages.
310-310: Path propagation in processFieldSelection is correct.The optional
messagePathparameter and the fallback logic at line 399 (currentPath = messagePath || message.name) correctly handle both root and nested contexts for field number assignment.Also applies to: 398-400
351-370: Nested message path handling is well-designed.The separation between the clean nested message name (PascalCase field name) and the full hierarchical path for lock file tracking is elegant. The
currentPathderivation at line 361 and its propagation asparentPathensures correct path construction for field number isolation.
418-418: Inline fragment path propagation correctly implemented.All three selection types (Field, InlineFragment, FragmentSpread) now receive the
messagePath, ensuring nested messages created within inline fragments maintain proper path context for field numbering.Also applies to: 449-454
471-471: Fragment spread path propagation completes the fix.Path propagation through all selection types in
processFragmentSpread, including recursive fragment spreads (line 510), ensures comprehensive path tracking across complex fragment compositions.Also applies to: 505-510
…eration Use service name as-is when generating proto from operations to preserve acronyms like HRService, APIService, etc.
…ased generation" This reverts commit afe5d94.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/commands/grpc-service/commands/generate.ts (1)
21-30: RemoveprefixOperationTypefrom underlying library or restore in CLI.Removal of the
prefixOperationTypeflag is incomplete. While it was removed from this CLI file, the flag still exists and is actively used in theprotographiclibrary:
protographic/src/operation-to-proto.ts(lines 67, 165, 205, 300)protographic/tests/operations/operation-to-proto.test.ts(multiple test cases)protographic/OPERATIONS_TO_PROTO.mdandprotographic/README.md(documentation)Either remove the flag completely from the entire codebase including the library, or restore the CLI option if this flag should remain supported.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/commands/grpc-service/commands/generate.ts
⏰ 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 (1)
cli/src/commands/grpc-service/commands/generate.ts (1)
347-349: LGTM! Comment accurately reflects the generation approach.The updated comment clearly explains that operations are processed separately to maintain proto schema consistency, which aligns with the PR's goal of improving deterministic proto generation.
The comment referenced a non-existent proto file path. Updated to accurately describe that field options are defined using 'extend google.protobuf.FieldOptions' in generated proto files, as seen in the actual implementation.
asoorm
force-pushed
the
ahmet/cli-protographic-improvements
branch
from
7ad7f37 to
03fa914
Compare
This PR is split from #2379 to enable independent review and merging of improvements to proto generation and operation validation. Required before implementing the larger features in #2379.
Ultimately, this PR permits us to have non-standard graphQL variable names / graphql variable names which cannot be derived from proto field names making it simpler to onboard non-standard GraphQL servers.
Changes
Protographic
graphql_variable_namedoesn't match protojson equivalent of proto message field.Consider:
Before: If multiple
Detailsmessages existed in different contexts, they sharedthe same lock file entry. This caused field numbering conflicts when fields were added
or removed in one context but not others:
After: The lock file now stores the full hierarchical path
(FindEmployeesByCriteriaResponse.FindEmployees.Details), ensuring each Details
message maintains independent field numbering.
CLI
prefixOperationTypeCLI option (breaking change, but OK, because nobody using yet) - it's not actually necessary, because we don't need to derive the operation type from the proto, we only need to execute the trusted documents / Named Operations.Context
Split from #2379 to simplify review and get these changes merged independently.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.