feat: generate proto from operations and sdl #2302
Conversation
WalkthroughAdds an operations-based GraphQL→Proto generation path (compileOperationsToProto) with CLI flags, operation-file processing, field-number manager, type/message/request builders, proto-text rendering, language-specific proto options, many tests and docs, and expanded protographic public exports; SDL-based flow remains. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas needing extra attention:
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 |
asoorm
requested review from
JivusAyrus,
Noroth,
StarpTech,
endigma and
jensneuse
as code owners
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
cb28579 to
e284194
Compare
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
2 times, most recently
from
efd48b7 to
9ff4661
Compare
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
5 times, most recently
from
c1fa72f to
51d2da7
Compare
Maps GraphQL types to Protocol Buffer types with full type system support: - Scalars: nullable → wrapper types, non-null → direct types - Lists: mapped to repeated fields - Complex types: objects, enums, input objects mapped by name - Custom scalar mappings and configuration options
Manages field number assignment with collision avoidance. Tracks numbers per-message, supports manual assignment and reset.
Builds request messages from operation variables with input object and enum support. Handles variable type conversion and field numbering.
Converts protobuf AST to formatted proto text with proper syntax, package declarations, imports, and indentation.
Wires together all modules to provide complete operation-to-proto conversion. Adds input object processing and updates exports.
Import Kind from graphql and use Kind.SELECTION_SET instead of 'SelectionSet' string to fix type error in test.
…eration-to-proto Add support for named fragments (fragment spreads) and inline fragments. Fragments are expanded inline to produce self-contained proto messages with deterministic field ordering, ensuring wire compatibility across router restarts. - Collect fragment definitions from GraphQL documents - Recursively resolve fragment spreads and inline fragments - Handle interfaces, unions, and nested fragment compositions - Prevent duplicate field additions
Adds a new queryNoSideEffects option to compileOperationsToProto that marks Query operations with 'option idempotency_level = NO_SIDE_EFFECTS;' in the generated proto definition. Mutations are not affected by this option.
Add support for rendering reserved field declarations in messageToProtoText() and enumToProtoText(). Reserved declarations are now properly formatted and placed after nested types but before fields, preserving proto3 reserved syntax for both numbers (with range compression) and field names.
Initialize options.createdEnums when missing to persist the Set across processFieldSelection calls, preventing duplicate enum definitions in generated proto files
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
protographic/src/operations/proto-text-generator.ts (1)
1-434: Run Prettier on this fileThe CI pipeline reports a Prettier style issue here. Please run your standard formatter (e.g.
prettier --write protographic/src/operations/proto-text-generator.ts) so spacing and wrapping match project conventions.
🧹 Nitpick comments (3)
protographic/src/operations/proto-text-generator.ts (1)
251-414: HardenformatReservedagainst string-based reservations
formatReservedcurrently assumesreservedentries are either numeric ranges (number[]) or field-name strings and treats all strings as names:if (typeof item === 'string') { names.push(item); }In protobufjs, reserved numbers and ranges can also appear as strings (e.g.
'5'or'5 to 10') when parsed from existing.protofiles. In that case this printer would incorrectly emit them as reserved names instead of numeric ranges.To make this more robust and align with the
sdl-to-proto-visitorbehavior, consider parsing string entries and routing them to numbers vs names, e.g.:- for (const item of reserved) { - if (typeof item === 'string') { - names.push(item); - } else if (Array.isArray(item) && item.length >= 2) { + for (const item of reserved) { + if (Array.isArray(item) && item.length >= 2) { const [start, end] = item; for (let i = start; i <= end; i++) { numbers.push(i); } + } else if (typeof item === 'string') { + if (item.includes('to')) { + // e.g. "5 to 10" + const [start, end] = item.split('to').map((s) => Number.parseInt(s.trim(), 10)); + if (!Number.isNaN(start) && !Number.isNaN(end)) { + for (let i = start; i <= end; i++) numbers.push(i); + continue; + } + } else if (/^\d+$/.test(item)) { + numbers.push(Number.parseInt(item, 10)); + continue; + } + names.push(item); } }This keeps current behavior for our lock-driven ranges while also correctly handling reservations coming from parsed proto definitions.
protographic/src/operations/message-builder.ts (1)
386-490: Consider removing or wiring up unused fragment helpers
processInlineFragmentandprocessFragmentSpreadare defined but only call each other;buildMessageFromSelectionSetnow uses thecollectFields/processFieldSelectiontwo‑pass flow and never invokes these helpers.If they’re no longer part of the active path, you can safely remove them (or make them private utilities in tests) to reduce cognitive load. If you plan to reintroduce them later, a brief comment explaining their intended usage would help future readers.
cli/src/commands/grpc-service/commands/generate.ts (1)
180-337: Operations mode merging looks solid; watch for name collisions across rootsThe new operations-based flow (
readOperationFiles→compileOperationsToProtoper file →mergeProtoRoots→rootToProtoText) is well-structured and keeps lock data flowing between operations. The generated file list and result metadata (isOperationsMode,generatedFiles,query idempotency) make the CLI output easy to reason about.One thing to keep in mind for future hardening:
mergeProtoRootsdeduplicates messages and enums purely by name and unconditionally adds all RPC methods to a singleService. If two operation files accidentally define incompatible messages or RPCs with the same name, the first definition “wins” without an explicit error. If that scenario is realistic for your users, a follow-up enhancement to detect and report such conflicts (instead of silently choosing the first) would make failures easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/src/commands/grpc-service/commands/generate.ts(6 hunks)protographic/OPERATIONS_TO_PROTO.md(1 hunks)protographic/src/operations/message-builder.ts(1 hunks)protographic/src/operations/proto-text-generator.ts(1 hunks)protographic/tests/operations/enum-support.test.ts(1 hunks)protographic/tests/operations/proto-text-generator.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protographic/tests/operations/proto-text-generator.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/message-builder.tscli/src/commands/grpc-service/commands/generate.ts
📚 Learning: 2025-09-22T11:13:45.617Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/keyfunc/keyfunc_test.go:142-154
Timestamp: 2025-09-22T11:13:45.617Z
Learning: When reviewing forked code, especially test files with test fixtures like JWT tokens, avoid suggesting modifications to maintain alignment with upstream and preserve the original author's structure. Test fixtures that are clearly marked as such (not real secrets) should generally be left unchanged in forked implementations.
Applied to files:
protographic/src/operations/message-builder.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:
cli/src/commands/grpc-service/commands/generate.tsprotographic/src/operations/proto-text-generator.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/src/operations/proto-text-generator.ts
🧬 Code graph analysis (4)
protographic/src/operations/message-builder.ts (4)
protographic/src/operations/field-numbering.ts (1)
FieldNumberManager(14-68)protographic/src/naming-conventions.ts (1)
graphqlFieldToProtoField(18-20)protographic/src/operations/type-mapper.ts (1)
mapGraphQLTypeToProto(74-191)protographic/src/operations/request-builder.ts (1)
buildEnumType(275-299)
cli/src/commands/grpc-service/commands/generate.ts (3)
protographic/src/index.ts (5)
validateGraphQLSDL(89-92)compileOperationsToProto(100-100)rootToProtoText(127-127)compileGraphQLToMapping(17-33)compileGraphQLToProto(54-80)protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(93-147)protographic/src/operations/proto-text-generator.ts (1)
rootToProtoText(58-86)
protographic/tests/operations/enum-support.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(93-147)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/operations/proto-text-generator.ts (2)
protographic/src/sdl-to-proto-visitor.ts (2)
formatIndent(2256-2258)formatReservedNumbers(2158-2199)protographic/src/index.ts (6)
ProtoTextOptions(133-133)rootToProtoText(127-127)serviceToProtoText(128-128)messageToProtoText(129-129)enumToProtoText(130-130)formatField(131-131)
🪛 GitHub Actions: Protographic CI
protographic/src/operations/message-builder.ts
[warning] 1-1: Code style issue found by Prettier. Run 'prettier --write' to fix.
protographic/tests/operations/enum-support.test.ts
[warning] 1-1: Code style issue found by Prettier. Run 'prettier --write' to fix.
protographic/src/operations/proto-text-generator.ts
[warning] 1-1: Code style issue found by Prettier. Run 'prettier --write' to fix.
🪛 markdownlint-cli2 (0.18.1)
protographic/OPERATIONS_TO_PROTO.md
10-10: Link fragments should be valid
(MD051, link-fragments)
25-25: Link fragments should be valid
(MD051, link-fragments)
61-61: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
599-599: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
612-612: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
638-638: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
2131db1 to
e8574f0
Compare
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
e8574f0 to
60ef256
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
protographic/src/operations/message-builder.ts (1)
473-477: Consider supporting interfaces and unions in fragment spreads.Currently, line 474 only checks
isObjectType(type)for fragment spread resolution. However, fragments can also be defined on interface types per the GraphQL spec. Consider updating the type check:if (!type || !(isObjectType(type) || isInterfaceType(type))) { // Type not found or not a supported type - skip return; }This aligns with the handling in
collectFields(line 156) andprocessInlineFragment(line 412).Based on past review feedback
protographic/src/operations/proto-text-generator.ts (2)
7-9: Consider documenting the interface extension pattern.The
MethodWithIdempotencyinterface extendsprotobuf.Methodwith a custom property. While this works (the property is attached at runtime), consider adding a brief comment explaining this is a runtime extension and may not be type-safe ifprotobuf.Methodchanges.
106-107: Consider conditional wrapper import based on usage.Currently,
google/protobuf/wrappers.protois always imported (line 107). Based on past review feedback, you might want to track whether wrapper types are actually used and only import when necessary, similar to howGraphQLToProtoTextVisitortracksusesWrapperTypes.However, for operations-based generation, wrapper types are commonly used for nullable fields, so the always-import approach may be acceptable. Consider documenting this decision.
Based on past review feedback
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cli/src/commands/grpc-service/commands/generate.ts(5 hunks)protographic/src/operations/message-builder.ts(1 hunks)protographic/src/operations/proto-text-generator.ts(1 hunks)protographic/src/operations/request-builder.ts(1 hunks)protographic/src/proto-options.ts(1 hunks)protographic/src/sdl-to-proto-visitor.ts(5 hunks)protographic/tests/operations/enum-support.test.ts(1 hunks)protographic/tests/operations/proto-text-generator.test.ts(1 hunks)protographic/tests/util.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- protographic/tests/operations/enum-support.test.ts
- protographic/src/operations/request-builder.ts
- protographic/src/proto-options.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/message-builder.tsprotographic/src/sdl-to-proto-visitor.tscli/src/commands/grpc-service/commands/generate.ts
📚 Learning: 2025-09-22T11:13:45.617Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/keyfunc/keyfunc_test.go:142-154
Timestamp: 2025-09-22T11:13:45.617Z
Learning: When reviewing forked code, especially test files with test fixtures like JWT tokens, avoid suggesting modifications to maintain alignment with upstream and preserve the original author's structure. Test fixtures that are clearly marked as such (not real secrets) should generally be left unchanged in forked implementations.
Applied to files:
protographic/src/operations/message-builder.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/src/sdl-to-proto-visitor.tsprotographic/src/operations/proto-text-generator.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/sdl-to-proto-visitor.tscli/src/commands/grpc-service/commands/generate.tsprotographic/src/operations/proto-text-generator.ts
🧬 Code graph analysis (5)
protographic/tests/operations/proto-text-generator.test.ts (2)
protographic/src/operations/proto-text-generator.ts (5)
rootToProtoText(59-87)serviceToProtoText(155-201)messageToProtoText(206-246)enumToProtoText(251-280)formatField(285-298)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/operations/message-builder.ts (4)
protographic/src/operations/field-numbering.ts (1)
FieldNumberManager(14-68)protographic/src/naming-conventions.ts (1)
graphqlFieldToProtoField(18-20)protographic/src/operations/type-mapper.ts (1)
mapGraphQLTypeToProto(74-191)protographic/src/operations/request-builder.ts (1)
buildEnumType(271-293)
protographic/src/sdl-to-proto-visitor.ts (2)
protographic/src/proto-options.ts (1)
buildProtoOptions(24-71)protographic/src/operations/list-type-utils.ts (3)
unwrapNonNullType(9-11)isNestedListType(20-24)calculateNestingLevel(37-53)
cli/src/commands/grpc-service/commands/generate.ts (4)
protographic/src/index.ts (5)
compileOperationsToProto(100-100)rootToProtoText(127-127)compileGraphQLToMapping(17-33)compileGraphQLToProto(54-80)validateGraphQLSDL(89-92)cli/src/commands/router/commands/plugin/helper.ts (2)
renderResultTree(13-67)renderValidationResults(75-134)protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(93-147)protographic/src/operations/proto-text-generator.ts (1)
rootToProtoText(59-87)
protographic/src/operations/proto-text-generator.ts (3)
protographic/src/sdl-to-proto-visitor.ts (2)
formatIndent(2230-2232)formatReservedNumbers(2132-2173)protographic/src/index.ts (6)
ProtoTextOptions(133-133)rootToProtoText(127-127)serviceToProtoText(128-128)messageToProtoText(129-129)enumToProtoText(130-130)formatField(131-131)protographic/src/proto-options.ts (1)
buildProtoOptions(24-71)
⏰ 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 (31)
protographic/src/sdl-to-proto-visitor.ts (4)
48-49: LGTM! Importing shared utilities improves consistency.The extraction of list-type utilities and proto options builder into shared modules reduces duplication and ensures consistent behavior across SDL-based and operations-based generation modes.
95-103: LGTM! Language-specific options expansion.The addition of Java, C#, Ruby, PHP, Objective-C, and Swift options provides comprehensive multi-language support for proto generation, aligning with the operations-to-proto feature goals.
239-254: LGTM! Centralized options handling via shared utility.Using
buildProtoOptionsensures consistent option emission across both SDL-based and operations-based generation paths, reducing code duplication and maintenance burden.
2002-2028: LGTM! Refactored to use external list-type utilities.The migration from private helper methods (
unwrapNonNullType,isNestedListType,calculateNestingLevel) to external shared utilities maintains the same logic while enabling reuse across the operations-to-proto workflow.protographic/tests/util.ts (3)
52-67: LGTM! Enhanced error handling with helpful debugging context.The try-catch block with
getAllNestedTypeNamesprovides clear error messages when a message path is not found, significantly improving the developer experience when tests fail due to incorrect message paths.
69-87: LGTM! Useful debugging helper for error messages.The recursive
getAllNestedTypeNameshelper efficiently collects all nested type names from a protobufjs Root, enabling comprehensive error messages that list available types when lookups fail.
145-167: LGTM! Comprehensive reserved numbers handling addresses past review feedback.The implementation now correctly handles:
- Single numeric reserved tags (e.g.,
reserved 5;)- Numeric ranges as arrays
[start, end]- String field names (skipped as expected)
This addresses the past review comment about silently dropping bare numeric entries.
Based on past review feedback
protographic/src/operations/message-builder.ts (4)
32-37: LGTM! Well-documented recursion limits.The
DEFAULT_MAX_DEPTHconstant (50) andDEFAULT_STARTING_DEPTH(0) provide clear recursion protection with reasonable defaults. The value 50 is appropriate for most GraphQL queries while protecting against circular references.Based on past review feedback
94-100: LGTM! Comprehensive depth limit check with helpful error message.The depth check at message entry provides:
- Clear error message explaining the issue
- Guidance on increasing the limit via
maxDepthoption- Protection against stack overflow from deeply nested or circular references
108-164: LGTM! Well-structured field collection with recursion protection.The
collectFieldsfunction correctly:
- Implements depth-based recursion protection
- Handles Field, InlineFragment, and FragmentSpread cases
- Validates type conditions before processing fragments
- Deduplicates fields by proto field name
The stop condition at line 114 prevents excessive recursion even within the collection phase.
327-340: LGTM! Enum deduplication fix properly implemented.The fix ensures
options.createdEnumsis initialized when missing and persists across calls:if (!options.createdEnums) { options.createdEnums = new Set<string>(); }This addresses the past review comment about enum de-duplication breaking when
options.createdEnumsis not provided.Based on past review feedback
protographic/tests/operations/proto-text-generator.test.ts (4)
15-17: LGTM! Test-scoped interface extension for idempotency.The
MethodWithIdempotencyinterface cleanly extendsprotobuf.Methodto support testing the idempotency_level option. This test-scoped approach is appropriate and doesn't pollute production code.
156-169: LGTM! Comprehensive idempotency level testing.The test correctly validates:
- RPC methods with
idempotencyLevelgenerate block syntax with options- The
option idempotency_level = NO_SIDE_EFFECTS;statement is emitted- The formatting uses correct indentation (2 spaces for option)
This aligns with the
--query-idempotencyCLI flag requirements.
374-456: LGTM! Thorough reserved fields testing validates past fix.The tests validate correct handling of:
- Reserved field numbers (single and ranges)
- Reserved field names
- Combined reserved numbers and names
This confirms the fix from past review comments is working correctly.
Based on past review feedback
458-536: LGTM! Comprehensive integration test.The end-to-end test validates:
- Complete proto file generation with service, messages, and options
- Consistent formatting and ordering
- Valid proto syntax (via
expectValidProto)- Language-specific options (go_package)
The inline snapshot ensures any changes to output are explicitly reviewed.
protographic/src/operations/proto-text-generator.ts (5)
123-137: LGTM! Centralized options handling via shared utility.Using
buildProtoOptionsensures consistent option emission across SDL-based and operations-based generation paths, reducing duplication and maintenance burden.
184-194: LGTM! Clean idempotency level handling.The implementation correctly:
- Checks for the
idempotencyLevelproperty via type assertion- Emits block syntax with option when present:
rpc Method(...) returns (...) { option idempotency_level = ...; }- Falls back to inline syntax when absent:
rpc Method(...) returns (...) {}
227-231: LGTM! Reserved declarations properly rendered.The code correctly emits reserved declarations after nested types but before fields, matching proto3 best practices. This addresses the past review comment about preserving reserved field numbers from the lock data.
Based on past review feedback
311-343: LGTM! Comprehensive reserved declaration formatting.The
formatReservedfunction correctly:
- Separates numeric and string reserved entries
- Expands ranges
[start, end]into individual numbers- Formats numbers with range compaction via
formatReservedNumbers- Formats names with proper quoting
This ensures proto lock data is properly preserved in generated output.
Based on past review feedback
349-390: LGTM! Efficient range compaction for reserved numbers.The
formatReservedNumbersfunction efficiently:
- Sorts and deduplicates numbers
- Identifies contiguous ranges
- Formats as compact proto3 syntax:
2, 5 to 10, 15This matches the existing implementation in
sdl-to-proto-visitor.tsand provides readable reserved declarations.cli/src/commands/grpc-service/commands/generate.ts (11)
18-29: LGTM! Well-structured language options type.The
LanguageOptionstype consolidates all language-specific proto options in a single location, improving maintainability and addressing past review feedback about grouping options.Based on past review feedback
36-41: LGTM! CLI options expanded for operations mode.The new flags support the operations-to-proto workflow:
withOperations: Directory path for operation filesqueryIdempotency: Idempotency level for queriescustomScalarMapping: Custom scalar type mappingsmaxDepth: Recursion depth controlprefixOperationType: RPC naming control
137-151: LGTM! Comprehensive validation of query-idempotency flag.The validation correctly:
- Warns when used without
--with-operations- Validates against allowed values
['NO_SIDE_EFFECTS', 'DEFAULT']- Normalizes to uppercase for consistent handling
154-163: LGTM! Flexible custom scalar mapping input.Supporting both inline JSON and file paths (prefixed with
@) provides flexibility for users. The error handling properly wraps parsing errors with context.
165-173: LGTM! Proper validation of maxDepth parameter.The validation ensures:
- Parsed value is a valid integer
- Value is positive (>= 1)
- Clear error message on validation failure
266-286: LGTM! Comprehensive operation file discovery.The
readOperationFilesfunction correctly:
- Supports multiple GraphQL file extensions:
.graphql,.gql,.graphqls,.gqls- Validates at least one operation file exists
- Reads all operation files into memory
- Provides clear error messages
292-338: LGTM! Clean proto AST merging logic.The
mergeProtoRootsfunction correctly:
- Handles edge cases (empty list, single root)
- Deduplicates messages and enums by name
- Merges all RPC methods from all services
- Preserves the service name across operations
This resolves the past review concern about merging proto outputs.
Based on past review feedback
374-403: LGTM! Proper lock data propagation across operations.The loop correctly:
- Loads initial lock data before processing operations
- Passes current lock data to each
compileOperationsToProtocall- Updates lock data after each operation for field number stability
- Collects AST roots instead of strings for proper merging
This ensures field number stability across multiple operation files.
405-413: LGTM! Single proto text generation after AST merge.Generating proto text once from the merged AST (instead of merging proto strings) ensures:
- Imports are not duplicated or lost
- RPC method options (like
idempotency_level) are preserved- Proper formatting and structure throughout
This addresses the past critical review comment about merged proto output being invalid.
Based on past review feedback
489-506: LGTM! Clean branching between generation modes.The generation logic cleanly branches based on
operationsDirpresence:
- Operations mode: processes operation files and merges ASTs
- SDL mode: generates from schema types and mapping
Both paths properly handle lock data and return consistent
GenerationResultstructures.
521-534: LGTM! Robust custom scalar mapping parser.The
parseCustomScalarMappingfunction:
- Supports file paths (prefixed with
@) for reusable configurations- Validates file existence before reading
- Falls back to inline JSON parsing
- Provides clear error messages
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
1949cba to
ed62b6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
protographic/OPERATIONS_TO_PROTO.md (1)
596-599: Add language identifiers to fenced code blocks in Troubleshooting section.This was flagged in the previous review but remains unresolved. All fenced code blocks should specify a language; these error examples should use
text.Apply this diff:
#### No Operation Files Found **Error:** -``` +```text No GraphQL operation files (.graphql, .gql) found in ./operations -``` +``` **Solution:** @@ -607,7 +607,7 @@ Note: Only files in the top-level directory are read; subdirectories are not tr #### Anonymous Operations Not Supported **Error:** -``` +```text Operations must be named -``` +``` **Solution:** @@ -633,7 +633,7 @@ Name all your operations: #### Field Number Conflicts **Error:** -``` +```text Field number conflict in message X -``` +```Also applies to: 610-612, 636-638
🧹 Nitpick comments (1)
protographic/src/operations/message-builder.ts (1)
399-499:processInlineFragment/processFragmentSpreadappear unused and duplicate traversal logicThese helpers aren't referenced within this file and are not exported, while their behavior overlaps with the
collectFields+ second-pass pipeline. Consider either wiring them into the main flow or removing them to avoid dead code and potential divergence from the primary fragment-handling logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
protographic/OPERATIONS_TO_PROTO.md(1 hunks)protographic/src/operations/message-builder.ts(1 hunks)protographic/tests/operations/fragment-spreads-interface-union.test.ts(1 hunks)protographic/tests/operations/fragments.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/tests/operations/fragments.test.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/src/operations/message-builder.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/message-builder.ts
🧬 Code graph analysis (3)
protographic/tests/operations/fragments.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(93-147)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/fragment-spreads-interface-union.test.ts (1)
protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/operations/message-builder.ts (4)
protographic/src/index.ts (8)
MessageBuilderOptions(121-121)FieldNumberManager(114-114)buildMessageFromSelectionSet(117-117)ProtoTypeInfo(111-111)buildEnumType(123-123)mapGraphQLTypeToProto(105-105)buildFieldDefinition(118-118)buildNestedMessage(119-119)protographic/src/operations/field-numbering.ts (1)
FieldNumberManager(14-68)protographic/src/naming-conventions.ts (1)
graphqlFieldToProtoField(18-20)protographic/src/operations/type-mapper.ts (2)
ProtoTypeInfo(42-55)mapGraphQLTypeToProto(74-191)
🪛 markdownlint-cli2 (0.18.1)
protographic/OPERATIONS_TO_PROTO.md
59-59: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
protographic/src/operations/message-builder.ts (4)
75-190: Field collection, depth limiting, and lock-based ordering look solidThe two-pass
collectFieldsapproach withmaxDepth/_depthchecks, fragment/inline-fragment handling, and reconciliation viaFieldNumberManager.reconcileFieldOrder+ lock pre-assignment forms a coherent pipeline; I don't see correctness gaps here for the supported GraphQL object/interface/union cases.
209-247: Helper utilities cleanly separate field-numbering and nested-list wrapper concerns
getOrAssignFieldNumber,resolveTypeNameAndRepetition, andcreateProtoFieldkeep numbering, wrapper resolution, and comment attachment out of the main traversal, which should make future changes to lock semantics or nested list wrappers localized and safer.
273-295: Enum de-duplication via persistentcreatedEnumsset is correctInitializing
options.createdEnumswhen missing and then reusing it across calls ensures each enum is only added once tooptions.root, avoiding duplicate enum definitions when callers don't pass a pre-populated set.
300-393: Field processing correctly handles__typename, unions, nested selections, and custom scalarsThe early-return for
__typename, union-type guard, nested message creation for object/interface/union named types, and scalar/enum mapping viamapGraphQLTypeToProto(withensureEnumCreatedwhen needed) all align with the intended GraphQL-to-proto mapping; I don't see functional issues in this core path.protographic/tests/operations/fragments.test.ts (1)
7-1962: Fragment tests give comprehensive coverage of operations-to-proto behaviorThis suite exercises a wide range of fragment patterns (simple/nested spreads, fragment-on-fragment, interface/union usages, nested resolvers, duplicates,
__typename, and alias cases) and asserts full proto snapshots viacompileOperationsToProto, which should provide strong regression protection for the new operations-based flow.protographic/tests/operations/fragment-spreads-interface-union.test.ts (1)
5-951: Focused coverage for fragment spreads on interfaces, unions, and mixed graphs looks strongThese tests specifically target interface- and union-based fragment spreads (including nested and mixed interface+union shapes) and validate the resulting proto messages, which rounds out coverage for complex polymorphic selections in the operations-to-proto pipeline.
|
|
||
| All operations must have a name. The operation name becomes the RPC method name in the generated proto. | ||
|
|
||
| **✅ Correct: Named operation** |
There was a problem hiding this comment.
Resolve MD036 linting violations: emphasis used instead of heading.
Lines 59 and 68 use bold emphasis for example labels, which markdownlint flags as places where headings would be more appropriate. Consider removing the emphasis and relying on emoji/plain text labels instead.
Apply this diff to satisfy the linting rule while maintaining clarity:
### Named Operations Requirement
All operations must have a name. The operation name becomes the RPC method name in the generated proto.
-**✅ Correct: Named operation**
+✅ **Correct: Named operation**
```graphql
query GetUser($id: ID!) {
user(id: $id) {
@@ -65,7 +65,7 @@ query GetUser($id: ID!) {
}-❌ Incorrect: Anonymous operation
+❌ Incorrect: Anonymous operation
query {
user(id: "123") {This keeps the visual distinction (emoji + inline emphasis) without triggering the linting rule.
Also applies to: 68-68
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
59-59: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In protographic/OPERATIONS_TO_PROTO.md around lines 59 and 68, the lines start
with emphasis which triggers MD036; update those lines so they do not begin with
bold/italic. For example, move the emoji to the start of the line and place the
bold text after it (e.g., "❌ **Incorrect: Anonymous operation**") or remove the
emphasis entirely, ensuring the line no longer begins with an emphasis marker
while preserving the emoji and label wording.
…ions-to-proto # Conflicts: # cli/src/commands/grpc-service/commands/generate.ts # protographic/src/index.ts # protographic/src/sdl-to-proto-visitor.ts
There was a problem hiding this comment.
Actionable comments posted: 2
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)
450-452: CRITICAL: Undefined variablegoPackagebreaks build.The variable
goPackageis not defined in scope. After the refactoring to uselanguageOptions, this should referencelanguageOptions.goPackageinstead.Apply this diff to fix the build error:
const protoOptions: ProtoOption[] = []; - if (goPackage) { - protoOptions.push(getGoModulePathProtoOption(goPackage!)); + if (languageOptions.goPackage) { + protoOptions.push(getGoModulePathProtoOption(languageOptions.goPackage)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/src/commands/grpc-service/commands/generate.ts(6 hunks)protographic/src/index.ts(3 hunks)protographic/src/sdl-to-proto-visitor.ts(2 hunks)protographic/tests/sdl-to-proto/10-options.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/src/index.tsprotographic/tests/sdl-to-proto/10-options.test.tsprotographic/src/sdl-to-proto-visitor.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/sdl-to-proto-visitor.tscli/src/commands/grpc-service/commands/generate.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:
cli/src/commands/grpc-service/commands/generate.ts
🧬 Code graph analysis (3)
protographic/tests/sdl-to-proto/10-options.test.ts (2)
protographic/src/index.ts (1)
compileGraphQLToProto(54-80)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/sdl-to-proto-visitor.ts (1)
protographic/src/operations/list-type-utils.ts (3)
unwrapNonNullType(9-11)isNestedListType(20-24)calculateNestingLevel(37-53)
cli/src/commands/grpc-service/commands/generate.ts (3)
protographic/src/index.ts (3)
compileOperationsToProto(100-100)rootToProtoText(127-127)validateGraphQLSDL(89-92)protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(93-147)protographic/src/operations/proto-text-generator.ts (1)
rootToProtoText(59-87)
🪛 GitHub Actions: Protographic CI
protographic/tests/sdl-to-proto/10-options.test.ts
[error] 1-1: SDL to Proto Options tests failing: generated proto text does not contain expected language options (e.g., java_package, java_outer_classname, java_multiple_files, csharp_namespace, ruby_package, php_namespace, php_metadata_namespace, objc_class_prefix, swift_prefix, go_package, and option ordering). This indicates option generation logic is not applying the requested options or tests are asserting on a mismatch with the current output.
🪛 GitHub Actions: Query Planner CI
cli/src/commands/grpc-service/commands/generate.ts
[error] 450-450: TypeScript error TS2304: Cannot find name 'goPackage'.
🪛 GitHub Actions: wgc CI
cli/src/commands/grpc-service/commands/generate.ts
[error] 450-450: Cannot find name 'goPackage'.
🪛 GitHub Check: build_test
protographic/tests/sdl-to-proto/10-options.test.ts
[failure] 180-180: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with swift_prefix option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option swift_prefix = "MyService";'
- Expected
- Received
- option swift_prefix = "MyService";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:180:23
[failure] 171-171: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with objc_class_prefix option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option objc_class_prefix = "MS";'
- Expected
- Received
- option objc_class_prefix = "MS";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:171:23
[failure] 162-162: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with php_metadata_namespace option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option php_metadata_namespace = "Exam…'
- Expected
- Received
- option php_metadata_namespace = "Example\MyService\Metadata";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:162:23
[failure] 153-153: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with php_namespace option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option php_namespace = "Example\MySer…'
- Expected
- Received
- option php_namespace = "Example\MyService";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:153:23
[failure] 144-144: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with ruby_package option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option ruby_package = "MyService::Pro…'
- Expected
- Received
- option ruby_package = "MyService::Proto";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:144:23
[failure] 135-135: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with csharp_namespace option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option csharp_namespace = "Example.My…'
- Expected
- Received
- option csharp_namespace = "Example.MyService";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:135:23
[failure] 124-124: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with all Java options
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option java_package = "com.example.my…'
- Expected
- Received
- option java_package = "com.example.myservice";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:124:23
[failure] 113-113: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with java_multiple_files option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option java_multiple_files = true;'
- Expected
- Received
- option java_multiple_files = true;
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:113:23
[failure] 104-104: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with java_outer_classname option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option java_outer_classname = "MyServ…'
- Expected
- Received
- option java_outer_classname = "MyServiceProto";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:104:23
[failure] 95-95: tests/sdl-to-proto/10-options.test.ts > SDL to Proto Options > should generate proto with java_package option
AssertionError: expected 'syntax = "proto3";\npackage service.v…' to contain 'option java_package = "com.example.my…'
- Expected
- Received
- option java_package = "com.example.myservice";
- syntax = "proto3";
- package service.v1;
- import "google/protobuf/wrappers.proto";
- // Service definition for DefaultService
- service DefaultService {
- rpc QueryHello(QueryHelloRequest) returns (QueryHelloResponse) {}
- }
- // Request message for hello operation.
- message QueryHelloRequest {
- }
- // Response message for hello operation.
- message QueryHelloResponse {
- google.protobuf.StringValue hello = 1;
- }
❯ tests/sdl-to-proto/10-options.test.ts:95:23
⏰ 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 (12)
protographic/src/sdl-to-proto-visitor.ts (1)
1967-1992: LGTM! Successful refactoring to centralize list-type utilities.The refactoring to use external utilities (
unwrapNonNullType,isNestedListType,calculateNestingLevel) fromlist-type-utils.tssuccessfully centralizes previously duplicated logic. This improves maintainability and addresses the past review comment about having logic in a single place.protographic/src/index.ts (1)
8-8: LGTM! Clean expansion of the public API surface.The import of
protobufand the extensive new exports for operations-to-proto functionality align well with the PR objectives. The additions are well-organized and consistently expose the new operation-based generation path.Also applies to: 99-151
cli/src/commands/grpc-service/commands/generate.ts (8)
268-288: LGTM! Clean implementation of operation file reading.The function properly uses an array of valid extensions with
includes()and provides clear error messages. The implementation is straightforward and handles edge cases appropriately.
294-340: LGTM! Solid implementation of proto AST merging.The function correctly merges multiple protobuf roots by deduplicating messages/enums by name and combining all RPC methods into a single service. The early returns for edge cases (empty array, single root) improve efficiency.
357-423: LGTM! Well-structured operations-based generation flow.The function properly orchestrates the operations-to-proto compilation:
- Processes each operation file separately to maintain reversibility
- Preserves field number stability through incremental lock data updates
- Collects AST roots before merging (good for avoiding string manipulation)
- Includes clear error messages with file context
529-542: LGTM! Clean implementation of custom scalar mapping parser.The function correctly handles both file paths (with
@prefix) and inline JSON strings. The CLI option description (lines 78-80) already provides clear guidance on the@prefix usage for file paths.
128-180: LGTM! Comprehensive validation with clear error messages.The validation logic properly:
- Verifies operations directory exists and is a directory
- Validates query idempotency values (NO_SIDE_EFFECTS, DEFAULT)
- Parses and validates custom scalar mappings with helpful error handling
- Ensures maxDepth is a positive integer
- Warns users about flag misuse when not using operations mode
20-31: LGTM! Clean type definition for language-specific options.The
LanguageOptionstype properly encapsulates all language-specific proto options as suggested in past reviews. This improves parameter management compared to passing individual options.
53-62: LGTM! Clear documentation of disabled alpha features.The comment clearly indicates that language-specific options (Java, C#, Ruby, PHP, ObjC, Swift) are disabled for the alpha release, addressing the past review feedback.
211-226: LGTM! Conditional file generation with clear output tracking.The implementation correctly:
- Writes mapping.json only when present (operations mode doesn't generate mapping)
- Always writes service.proto
- Conditionally writes lock data
- Builds a dynamic list of generated files for user feedback
This addresses the past review feedback about always including service.proto.
protographic/tests/sdl-to-proto/10-options.test.ts (2)
6-10: Good refactoring: shared test SDL reduces duplication.Extracting
simpleSDLas a constant is a good practice that keeps the tests focused on the option configurations rather than the SDL structure.
208-225: Review comment is incorrect — no issues found in the code.The review makes two false claims:
Property naming inconsistency claim is incorrect. All properties use consistent camelCase:
goPackage,javaPackage,javaOuterClassname,javaMultipleFiles,csharpNamespace,rubyPackage,phpNamespace,objcClassPrefix,swiftPrefix. There is no mixed naming convention.Option generation issue claim is incorrect. Tests earlier in the same file (lines 183–206) verify that all options ARE being generated and present in the proto output. The implementation (line 435–436 in
sdl-to-proto-visitor.ts) correctly sorts options.The test at lines 208–225 is well-structured: it filters option lines, verifies at least one exists, and compares sorted order. The logic is sound and should work as intended.
Likely an incorrect or invalid review comment.
Updated tests to use the new protoOptions array format instead of individual language-specific parameters (javaPackage, goPackage, etc.) to align with the simplified API from main branch.
Fixed ReferenceError where goPackage and other language options were referenced directly instead of from the languageOptions parameter. Now properly builds protoOptions array from all language-specific options in the languageOptions object.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/src/commands/grpc-service/commands/generate.ts (1)
290-340: LGTM! Consider documenting deduplication behavior.The merge logic correctly handles edge cases and deduplicates messages/enums by name. The first-seen strategy is appropriate since all operations should reference the same schema types.
Consider adding a comment explaining that message/enum deduplication assumes all operations are compiled against the same schema, so identically-named types will have identical structures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/commands/grpc-service/commands/generate.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
cli/src/commands/grpc-service/commands/generate.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:
cli/src/commands/grpc-service/commands/generate.ts
🧬 Code graph analysis (1)
cli/src/commands/grpc-service/commands/generate.ts (4)
protographic/src/index.ts (5)
compileOperationsToProto(100-100)rootToProtoText(127-127)compileGraphQLToMapping(17-33)ProtoOption(135-135)validateGraphQLSDL(89-92)protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(93-147)protographic/src/operations/proto-text-generator.ts (1)
rootToProtoText(59-87)protographic/src/sdl-to-proto-visitor.ts (1)
ProtoOption(104-107)
⏰ 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: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
cli/src/commands/grpc-service/commands/generate.ts (13)
20-31: LGTM! Good refactoring.The
LanguageOptionstype consolidates language-specific proto options as suggested in past review comments, improving parameter management.
33-43: LGTM!The CLI options are appropriately extended to support operations-based generation with all new fields properly optional.
45-95: LGTM!The CLI command setup is well-documented with clear help text for all new flags. Language-specific options are appropriately disabled for the alpha release per past review feedback.
97-102: LGTM!The
GenerationResulttype appropriately reflects that mapping is optional in operations mode and includes a flag to distinguish generation modes.
128-180: LGTM! Solid validation logic.The validation comprehensively checks:
- Operations directory existence and type
- Query idempotency values and usage context
- Custom scalar mapping parsing
- Max depth as a positive integer
- Appropriate warnings for misused flags
182-246: LGTM! Well-structured generation flow.The command action properly:
- Aggregates language options into a single object
- Conditionally writes files based on generation mode
- Builds a dynamic list of generated files
- Includes relevant metadata in the result output
248-261: LGTM!The
GenerationOptionstype appropriately includes all necessary fields for both SDL and operations-based generation modes.
263-288: LGTM! Clean implementation.The function properly:
- Uses a validExtensions array for clarity
- Includes all relevant GraphQL file extensions per past review feedback
- Provides clear error messages
- Reads operation files with proper error handling
342-423: LGTM! Well-structured operations processing.The function properly:
- Threads lock data through operations for field number stability
- Collects AST roots before merging (efficient approach)
- Provides clear error context for operation-specific failures
- Returns appropriate values for operations mode
425-494: LGTM! Clean SDL generation refactoring.The function properly:
- Separates SDL-specific generation logic
- Builds proto options from language options
- Returns appropriate values for SDL mode
496-542: LGTM! Clean mode dispatch.The function properly:
- Validates the schema before processing
- Branches between operations and SDL modes based on configuration
- Threads all options through to the appropriate generation function
553-569: LGTM! Clean parsing implementation.The function properly handles both file paths (with @ prefix) and inline JSON, with appropriate file existence checking.
571-579: LGTM!The helper correctly uses
accessinstead of the deprecatedexistsfrom node:fs, with a helpful comment explaining the choice.
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
2 times, most recently
from
39be098 to
b817019
Compare
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
b817019 to
e47cb2f
Compare
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)
protographic/src/sdl-to-proto-visitor.ts (1)
1989-2088: List wrapper implementation and documentation are out of syncThe docstrings for
handleListType/createNestedListWrapperdescribe a level‑1 wrapper like:message ListOfString { repeated string items = 1; }but
buildWrapperMessagealways generates an innerListmessage:message ListOfString { message List { repeated string items = 1; } List list = 1; }This is functionally fine, but the comments no longer match what’s emitted, which is confusing when reasoning about nullable vs. nested list shapes. Either:
- Special‑case
level === 1inbuildWrapperMessageto match the documented “simple wrapper” shape; or- Update the examples in the comments to show the actual
Listnested message structure for both single‑ and multi‑level wrappers.I’d also briefly confirm that the operations path’s nested list wrappers (
ensureNestedListWrapper/list-type-utils) produce the same message names and shapes so SDL‑based and operations‑based generation stay consistent.
🧹 Nitpick comments (11)
protographic/src/sdl-to-proto-visitor.ts (1)
224-250: Simplify proto option wiring and double‑check go_package default behaviorYou’re manually reconstructing the
ProtoOptionsbag before callingbuildProtoOptions. SinceGraphQLToProtoTextVisitorOptionsalready extendsProtoOptions, you can avoid drift by passingoptionsdirectly:- const protoOptionsFromLanguageProps = buildProtoOptions( - { - goPackage: options.goPackage, - javaPackage: options.javaPackage, - javaOuterClassname: options.javaOuterClassname, - javaMultipleFiles: options.javaMultipleFiles, - csharpNamespace: options.csharpNamespace, - rubyPackage: options.rubyPackage, - phpNamespace: options.phpNamespace, - phpMetadataNamespace: options.phpMetadataNamespace, - objcClassPrefix: options.objcClassPrefix, - swiftPrefix: options.swiftPrefix, - }, - packageName, - ); + const protoOptionsFromLanguageProps = buildProtoOptions(options, packageName);Also note that
buildProtoOptionscurrently only emitsgo_packagewhenoptions.goPackageis truthy, so the “Generate default go_package if not provided” behavior isn’t actually triggered whengoPackageis omitted. If you intended to derive a default frompackageName, consider relaxing that guard inbuildProtoOptionsin a follow‑up.protographic/tests/operations/field-numbering.test.ts (1)
1-224: FieldNumberManager behavior is well coveredThese tests exercise all the important behaviors of
createFieldNumberManager(per‑message sequencing, manual assignment, resets, and mixed usage), which should make regressions obvious. If you ever wire aProtoLockManagerinto this flow, consider an additional test that passes a stub lock manager and assertsreconcileFieldOrder/getLockManagerinteractions, but that’s optional for now.protographic/src/types.ts (1)
1-14: Use a type‑only namespace import for protobuf typesSince
protobufis only used for typing and the rest of the codebase uses a namespace import, it’s cleaner and safer to switch to a type‑only namespace import:-import protobuf from 'protobufjs'; +import type * as protobuf from 'protobufjs';This avoids relying on a synthetic default export, prevents an unnecessary runtime import when compiling with
importsNotUsedAsValues = 'remove', and keeps the style consistent with other modules usingprotobufjs.protographic/tests/operations/request-builder.test.ts (1)
241-299: Prefer GraphQL type guards over constructor name checksIn multiple places you validate the looked‑up type with:
if (!inputType || inputType.constructor.name !== 'GraphQLInputObjectType') { throw new Error('Invalid input type'); }Since you already import
GraphQLInputObjectType, or could importisInputObjectType, this can be made more robust and intention‑revealing:import { /* ..., */ isInputObjectType } from 'graphql'; const inputType = schema.getType('UserInput'); if (!inputType || !isInputObjectType(inputType)) { throw new Error('Invalid input type'); }This avoids relying on
constructor.name(which can be brittle under minification or across multiple GraphQL instances) and keeps the tests aligned with GraphQL’s own helpers.protographic/tests/operations/fragments.test.ts (1)
324-353: Tighten fragment test naming and expectationsTwo small nits:
- The test
should handle fragments that reference non-existent fragmentsactually uses a single defined fragment (UserFields) and doesn’t exercise missing/undefined fragments. If the intent is simply “basic fragment usage without cycles”, consider renaming to avoid confusion for future readers.- In
should handle __typename field in fragments, the snapshot implicitly asserts that__typenameis dropped, but nothing explicitly checks for its absence. You could make the intent clearer and guard against regressions by also asserting thatprotodoes not contain__typename(or the corresponding field name) in addition to the snapshot.These are purely clarity/coverage tweaks; the existing assertions already exercise the core behavior.
Also applies to: 1849-1905
protographic/tests/operations/proto-text-generator.test.ts (1)
6-11: Reuse the exported MethodWithIdempotency type to avoid driftYou’re redefining a local
MethodWithIdempotencyinterface that matches the production type:interface MethodWithIdempotency extends protobuf.Method { idempotencyLevel?: 'NO_SIDE_EFFECTS' | 'DEFAULT'; }Now that
MethodWithIdempotency(andIdempotencyLevel) are exported fromsrc/types.ts/src/index.ts, you could import and use that instead:import type { MethodWithIdempotency } from '../../src';This keeps the tests in sync if you ever extend the allowed idempotency levels or adjust the interface.
protographic/tests/operations/recursion-protection.test.ts (1)
294-392: Clarify edge‑case test names and consider softening the timing assertionTwo minor suggestions:
should handle empty fragments gracefullyandshould handle fragments that reference non-existent fragmentsdon’t match their implementations: the former’s fragment isn’t empty, and the latter doesn’t reference any missing fragment. Renaming them to reflect what they actually test (e.g., “simple fragment spread without cycles”) would make the suite easier to read.- In
should handle reasonable depth efficiently, assertingendTime - startTime < 1000is probably fine, but it does introduce a small risk of flakiness on slow or contended CI runners. If this ever becomes noisy, you could either relax the threshold further or drop the timing assertion and rely on the fact that the test will naturally fail if compilation becomes pathologically slow.Functionally everything here looks correct; these are just resilience/clarity tweaks.
Also applies to: 395-431
protographic/src/operation-to-proto.ts (3)
181-212: Type fieldNumberManager explicitly to align with FieldNumberManager interface
fieldNumberManageris declared without a type annotation and only assigned in the constructor viacreateFieldNumberManager(this.lockManager). This effectively makes itanyunder typical TypeScript settings, which loses the structural contract (e.g.,reconcileFieldOrder,getLockManager, etc.) and weakens IDE support.Consider importing the type and annotating the field:
import type { FieldNumberManager } from './operations/field-numbering.js'; class OperationsToProtoVisitor { // ... private readonly fieldNumberManager: FieldNumberManager; // ... }This keeps the implementation unchanged while tightening type-safety around the field-number integration.
281-291: Reuse createOperationMethodName to keep RPC naming conventions centralized
processOperationcurrently derives the method name viaupperFirst(camelCase(operationName)), even though a dedicatedcreateOperationMethodNamehelper exists and is already imported. Duplicating the naming logic here can lead to drift if naming rules evolve (e.g., handling of punctuation, acronyms, or legacy conventions used elsewhere in protographic).You can delegate to the shared helper and then apply the optional operation-type prefix on top, e.g.:
let methodName = createOperationMethodName(operationName); if (this.prefixOperationType) { const operationTypePrefix = upperFirst(node.operation.toLowerCase()); methodName = `${operationTypePrefix}${methodName}`; }This keeps operations-based generation aligned with the rest of the codebase’s RPC naming semantics.
439-448: Consider a clearer error when the schema lacks the root operation type
getRootTypeuses non-null assertions onschema.getQueryType(),getMutationType(), andgetSubscriptionType(). If a caller passes a schema without a matching root type for the operation (e.g., amutationoperation against a schema withoutmutationroot), this will blow up with a generic runtime error rather than a targeted message.Since you already validate the operations, this is likely rare, but you could improve diagnostics by explicitly checking for
nulland throwing a more descriptive error:private getRootType(operationType: OperationTypeNode): GraphQLObjectType { switch (operationType) { case OperationTypeNode.QUERY: { const type = this.schema.getQueryType(); if (!type) throw new Error('Schema does not define a query root type.'); return type; } // similar for MUTATION/SUBSCRIPTION } }This is non-blocking but makes failures easier to debug when the schema and operations get out of sync.
protographic/src/operations/proto-text-generator.ts (1)
148-188: Avoid duplicating method formatting logic between serviceToProtoText and formatMethodRight now
serviceToProtoTexthand-rolls RPC formatting (including idempotency handling), whileformatMethodindependently formats a singlerpcline without idempotency support. This creates two slightly different code paths for essentially the same concern.Consider either:
- Making
serviceToProtoTextdelegate to an enhancedformatMethodthat also understandsMethodWithIdempotency, or- Updating
formatMethodto mirror the idempotency/streaming logic so callers using it directly get behaviour consistent withserviceToProtoText.This would reduce drift if you later tweak RPC formatting (e.g., adding more per-method options).
Also applies to: 425-440
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
protographic/src/index.ts(3 hunks)protographic/src/operation-to-proto.ts(1 hunks)protographic/src/operations/proto-text-generator.ts(1 hunks)protographic/src/sdl-to-proto-visitor.ts(4 hunks)protographic/src/types.ts(1 hunks)protographic/tests/operations/enum-support.test.ts(1 hunks)protographic/tests/operations/field-numbering.test.ts(1 hunks)protographic/tests/operations/field-ordering-stability.test.ts(1 hunks)protographic/tests/operations/fragment-spreads-interface-union.test.ts(1 hunks)protographic/tests/operations/fragments.test.ts(1 hunks)protographic/tests/operations/message-builder.test.ts(1 hunks)protographic/tests/operations/operation-to-proto.test.ts(1 hunks)protographic/tests/operations/operation-validation.test.ts(1 hunks)protographic/tests/operations/proto-text-generator.test.ts(1 hunks)protographic/tests/operations/recursion-protection.test.ts(1 hunks)protographic/tests/operations/request-builder.test.ts(1 hunks)protographic/tests/operations/type-mapper.test.ts(1 hunks)protographic/tests/operations/validation.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- protographic/tests/operations/validation.test.ts
- protographic/tests/operations/operation-validation.test.ts
- protographic/tests/operations/fragment-spreads-interface-union.test.ts
- protographic/tests/operations/message-builder.test.ts
- protographic/tests/operations/enum-support.test.ts
- protographic/tests/operations/type-mapper.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/tests/operations/fragments.test.tsprotographic/tests/operations/field-ordering-stability.test.tsprotographic/src/sdl-to-proto-visitor.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/src/operation-to-proto.tsprotographic/src/sdl-to-proto-visitor.tsprotographic/src/operations/proto-text-generator.tsprotographic/src/index.ts
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
protographic/tests/operations/operation-to-proto.test.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/sdl-to-proto-visitor.ts
🧬 Code graph analysis (11)
protographic/tests/operations/fragments.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(87-141)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/types.ts (1)
protographic/src/index.ts (2)
IdempotencyLevel(135-135)MethodWithIdempotency(135-135)
protographic/tests/operations/field-numbering.test.ts (2)
protographic/src/index.ts (1)
createFieldNumberManager(113-113)protographic/src/operations/field-numbering.ts (1)
createFieldNumberManager(76-204)
protographic/tests/operations/request-builder.test.ts (2)
protographic/src/operations/request-builder.ts (3)
buildRequestMessage(48-104)buildInputObjectMessage(180-262)buildEnumType(271-293)protographic/src/operations/field-numbering.ts (1)
createFieldNumberManager(76-204)
protographic/src/operation-to-proto.ts (7)
protographic/src/types.ts (2)
IdempotencyLevel(7-7)MethodWithIdempotency(12-14)protographic/src/operations/field-numbering.ts (1)
createFieldNumberManager(76-204)protographic/src/naming-conventions.ts (2)
createRequestMessageName(43-45)createResponseMessageName(50-52)protographic/src/operations/request-builder.ts (3)
buildRequestMessage(48-104)buildInputObjectMessage(180-262)buildEnumType(271-293)protographic/src/operations/message-builder.ts (1)
buildMessageFromSelectionSet(75-204)protographic/src/operations/proto-text-generator.ts (1)
rootToProtoText(53-78)protographic/src/operations/type-mapper.ts (1)
mapGraphQLTypeToProto(74-191)
protographic/tests/operations/field-ordering-stability.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(87-141)protographic/tests/util.ts (3)
expectValidProto(29-31)loadProtoFromText(39-43)getFieldNumbersFromMessage(52-67)
protographic/tests/operations/operation-to-proto.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(87-141)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/proto-text-generator.test.ts (3)
protographic/src/types.ts (1)
MethodWithIdempotency(12-14)protographic/src/operations/proto-text-generator.ts (5)
rootToProtoText(53-78)serviceToProtoText(148-194)messageToProtoText(199-239)enumToProtoText(244-273)formatField(278-291)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/recursion-protection.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(87-141)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/src/sdl-to-proto-visitor.ts (2)
protographic/src/proto-options.ts (2)
ProtoOptions(4-15)buildProtoOptions(24-71)protographic/src/operations/list-type-utils.ts (3)
unwrapNonNullType(9-11)isNestedListType(20-24)calculateNestingLevel(37-53)
protographic/src/operations/proto-text-generator.ts (4)
protographic/src/sdl-to-proto-visitor.ts (2)
formatIndent(2217-2219)formatReservedNumbers(2119-2160)protographic/src/index.ts (7)
ProtoTextOptions(133-133)rootToProtoText(127-127)serviceToProtoText(128-128)messageToProtoText(129-129)enumToProtoText(130-130)MethodWithIdempotency(135-135)formatField(131-131)protographic/src/proto-options.ts (1)
buildProtoOptions(24-71)protographic/src/types.ts (1)
MethodWithIdempotency(12-14)
⏰ 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 (5)
protographic/src/sdl-to-proto-visitor.ts (1)
91-99: Exposing language proto options via visitor options looks goodExtending
GraphQLToProtoTextVisitorOptionsfromProtoOptionscleanly surfaces the language-specific options to SDL-based generation and keeps the public surface aligned with the operations-based flow. No issues from a typing or backwards-compatibility perspective.protographic/tests/operations/operation-to-proto.test.ts (1)
1-1822: Comprehensive operation‑to‑proto integration coverageThis suite does a good job exercising the public
compileOperationsToProtoAPI across operation types, language options, idempotency behavior, error conditions, and determinism (field numbering, repeated compilation). Assertions are scoped to stable substrings or snapshots, so future internal refactors should be reasonably safe as long as the surface behavior stays the same.protographic/tests/operations/field-ordering-stability.test.ts (1)
5-1999: Thorough coverage of field/variable/input ordering and lockData behaviourThis suite exercises the critical permutations (reordering, add/remove/re-add, nested inputs, fragments, inline fragments, multiple operations, mutations, deep nesting, edge cases) against
compileOperationsToProtowith and withoutlockData. Assertions consistently use proto-level names (in_stock, nested message paths likeGetUserResponse.User, etc.), and the expectations around re-used vs. reserved tag numbers line up with the field-number manager semantics. I don’t see gaps or logical issues here; this is a solid safety net for the new operations-to-proto flow.protographic/src/index.ts (1)
8-154: Public API surface for operations-based generation looks consistentThe additional exports (operations-to-proto entrypoint, type-mapper/request/message builders, proto-text generation helpers, idempotency types, and protobufjs itself) are wired through
index.tscoherently and match the intended usage described in the PR. I don’t see any correctness or compatibility issues in how these are surfaced.protographic/src/operations/proto-text-generator.ts (1)
83-143: Header and reserved-handling logic align with protobufjs/lock semanticsThe updated header generation and reserved rendering look solid:
- Header now conditionally imports
google/protobuf/wrappers.protobased on actualgoogle.protobuf.*usage in theRoot, and merges user-specified imports/options on top of language-specific options frombuildProtoOptions.messageToProtoText/enumToProtoTextemitreservedclauses before fields/values, usingformatReserved/formatReservedNumbersto normalize(number[]|string)[]from protobufjs into compact ranges plus quoted names.This matches the intended behaviour for preserving removed field numbers/names while keeping the emitted proto clean and deterministic.
Also applies to: 199-239, 244-273, 304-383
This adds the ability to collect GraphQL operations (queries/mutations) and emit .proto files so we can auto-generate servers and clients (via gRPC/Buf Connect) to consume the graph.
--with-operationsthat selects operations-based generation mode.Backwards compatibility
Existing schema-type-based generation unchanged. New mode opt-in.
Summary by CodeRabbit
New Features
Documentation
Quality
Tests
✏️ Tip: You can customize this high-level summary in your review settings.