feat: generate proto from operations and sdl #2302
Open
+13,527
−99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis PR introduces GraphQL Operations-to-Protocol Buffer compilation support. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
StarpTech
requested changes
Oct 25, 2025
❌ 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 GraphQL spec validation to operation compilation to catch invalid operations including circular fragment references before proto generation. Implements two-layer defense: GraphQL validation (primary) catches spec violations like circular fragments, while depth limiting (default: 50, configurable via maxDepth) provides safety net against stack overflow from deeply nested operations. - Add validate() with specifiedRules to compileOperationsToProto() - Add comprehensive validation tests (circular fragments, unused vars, etc.) - Add inline snapshot test showing nested message structure from recursion - Fix tests using invalid GraphQL (unused variables, circular fragments) Addresses PR feedback about recursion stop conditions.
Previously, nested GraphQL lists like [[String]] were flattened to repeated string, losing structure. Now generates proper wrapper messages matching SDL-to-proto behavior. - Add nested list detection and wrapper message generation - Extract shared list utilities to eliminate code duplication
ability to prefix operation type to rpc method names and request and response messages.
Prefixes RPC method names with operation type (Query/Mutation) when generating proto from operations. Only applies with --with-operations.
Add --prefix-operation-type flag to prefix RPC method names with operation type (Query/Mutation/Subscription). Also fix proto generation issues: - Allow unknown directives in GraphQL validation - Fix nested message handling in proto merging
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
5384feb to
38b2c4b
Compare
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
616fee1 to
a294dfb
Compare
asoorm
force-pushed
the
ahmet/eng-8163-operations-to-proto
branch
from
a5043d5 to
1d46577
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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)
2108-2123: Restore wrapper support for nullable list elementsUsing
getProtoTypeFromGraphQL(baseType, true)strips wrapper types from the list payload, so the generated wrapper message encodes nullable scalar lists asrepeated string. Proto3 cannot representnullentries in a repeated primitive field, so GraphQL inputs like[ "a", null, "b" ]collapse thenullelement and violate the schema contract. The previous implementation relied on wrapper types (google.protobuf.StringValue, etc.) to preserve element-level nullability—this change regresses that behavior.Please fetch the inner type without forcing
ignoreWrapperTypes, and reuse itstypeNameso wrapper messages are still emitted when needed. For example:- } else { - innerWrapperName = this.getProtoTypeFromGraphQL(baseType, true).typeName; - } + } else { + const innerType = this.getProtoTypeFromGraphQL(baseType); + innerWrapperName = innerType.typeName; + }This keeps nullable list elements faithfully encodable while still sharing the same wrapper construction for nested lists.
🧹 Nitpick comments (1)
protographic/OPERATIONS_TO_PROTO.md (1)
600-647: Add language identifiers to error-output code fences.The fenced blocks showing CLI errors (e.g., the “No GraphQL operation files found” snippet) omit a language tag, which trips markdownlint MD040. Annotate them with something like ```text to keep lint green.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
cli/src/commands/grpc-service/commands/generate.ts(7 hunks)protographic/OPERATIONS_TO_PROTO.md(1 hunks)protographic/README.md(2 hunks)protographic/src/index.ts(1 hunks)protographic/src/operation-to-proto.ts(1 hunks)protographic/src/operations/field-numbering.ts(1 hunks)protographic/src/operations/list-type-utils.ts(1 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/operations/type-mapper.ts(1 hunks)protographic/src/sdl-to-proto-visitor.ts(5 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/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)protographic/tests/sdl-to-proto/10-options.test.ts(2 hunks)protographic/tests/util.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/tests/operations/field-ordering-stability.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/tests/operations/field-ordering-stability.test.tsprotographic/tests/operations/fragments.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/operations/field-numbering.tsprotographic/tests/sdl-to-proto/10-options.test.tsprotographic/src/sdl-to-proto-visitor.tsprotographic/src/index.tsprotographic/src/operations/message-builder.ts
📚 Learning: 2025-10-17T16:35:24.723Z
Learnt from: Aenimus
Repo: wundergraph/cosmo PR: 2287
File: composition/tests/v1/federation-factory.test.ts:569-583
Timestamp: 2025-10-17T16:35:24.723Z
Learning: In the wundergraph/cosmo repository's Vitest test suite, `toHaveLength()` works with Map objects and `expect(map.has(key))` without an explicit matcher correctly asserts the boolean result in the test assertions for `composition/tests/v1/federation-factory.test.ts`.
Applied to files:
protographic/tests/operations/type-mapper.test.ts
🧬 Code graph analysis (21)
protographic/tests/operations/operation-validation.test.ts (1)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)
protographic/src/operations/request-builder.ts (3)
protographic/src/operations/field-numbering.ts (1)
FieldNumberManager(14-68)protographic/src/naming-conventions.ts (2)
graphqlArgumentToProtoField(25-27)graphqlFieldToProtoField(18-20)protographic/src/operations/type-mapper.ts (1)
mapGraphQLTypeToProto(74-191)
protographic/tests/operations/request-builder.test.ts (2)
protographic/src/operations/request-builder.ts (3)
buildRequestMessage(43-101)buildInputObjectMessage(177-261)buildEnumType(270-290)protographic/src/operations/field-numbering.ts (1)
createFieldNumberManager(76-204)
protographic/tests/operations/operation-to-proto.test.ts (3)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)protographic/src/index.ts (1)
compileOperationsToProto(99-99)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/field-ordering-stability.test.ts (3)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)protographic/src/index.ts (1)
compileOperationsToProto(99-99)protographic/tests/util.ts (3)
expectValidProto(29-31)loadProtoFromText(39-43)getFieldNumbersFromMessage(52-67)
protographic/src/operation-to-proto.ts (6)
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(43-101)buildInputObjectMessage(177-261)buildEnumType(270-290)protographic/src/operations/message-builder.ts (1)
buildMessageFromSelectionSet(65-188)protographic/src/operations/proto-text-generator.ts (1)
rootToProtoText(51-79)protographic/src/operations/type-mapper.ts (1)
mapGraphQLTypeToProto(74-191)
protographic/src/operations/field-numbering.ts (1)
protographic/src/index.ts (3)
FieldNumberManager(113-113)ProtoLockManager(95-95)createFieldNumberManager(112-112)
protographic/tests/operations/message-builder.test.ts (2)
protographic/src/operations/message-builder.ts (3)
buildFieldDefinition(485-503)buildNestedMessage(513-539)buildMessageFromSelectionSet(65-188)protographic/src/operations/field-numbering.ts (1)
createFieldNumberManager(76-204)
protographic/tests/operations/fragments.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/proto-text-generator.test.ts (2)
protographic/src/operations/proto-text-generator.ts (5)
rootToProtoText(51-79)serviceToProtoText(172-232)messageToProtoText(237-272)enumToProtoText(277-301)formatField(306-329)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/recursion-protection.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/type-mapper.test.ts (2)
protographic/src/operations/type-mapper.ts (5)
mapGraphQLTypeToProto(74-191)getProtoTypeName(264-267)isGraphQLScalarType(275-277)requiresWrapperType(286-289)getRequiredImports(298-309)protographic/src/index.ts (5)
mapGraphQLTypeToProto(104-104)getProtoTypeName(105-105)isGraphQLScalarType(106-106)requiresWrapperType(107-107)getRequiredImports(108-108)
protographic/src/operations/proto-text-generator.ts (1)
protographic/src/index.ts (6)
ProtoTextOptions(132-132)rootToProtoText(126-126)serviceToProtoText(127-127)messageToProtoText(128-128)enumToProtoText(129-129)formatField(130-130)
protographic/tests/sdl-to-proto/10-options.test.ts (2)
protographic/src/index.ts (1)
compileGraphQLToProto(53-79)protographic/tests/util.ts (1)
expectValidProto(29-31)
cli/src/commands/grpc-service/commands/generate.ts (4)
protographic/src/index.ts (5)
ProtoLock(135-135)validateGraphQLSDL(88-91)compileOperationsToProto(99-99)compileGraphQLToMapping(16-32)compileGraphQLToProto(53-79)cli/src/commands/router/commands/plugin/toolchain.ts (1)
generateProtoAndMapping(360-405)cli/src/commands/router/commands/plugin/helper.ts (2)
renderResultTree(13-67)renderValidationResults(75-134)protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)
protographic/tests/operations/validation.test.ts (1)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)
protographic/src/operations/type-mapper.ts (3)
protographic/src/index.ts (7)
ProtoTypeInfo(110-110)TypeMapperOptions(110-110)mapGraphQLTypeToProto(104-104)getProtoTypeName(105-105)isGraphQLScalarType(106-106)requiresWrapperType(107-107)getRequiredImports(108-108)protographic/src/sdl-to-proto-visitor.ts (1)
handleListType(2028-2054)protographic/src/operations/list-type-utils.ts (3)
unwrapNonNullType(9-11)isNestedListType(20-24)calculateNestingLevel(37-53)
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)
protographic/tests/operations/enum-support.test.ts (2)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(92-146)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(270-290)
protographic/tests/operations/field-numbering.test.ts (1)
protographic/src/operations/field-numbering.ts (1)
createFieldNumberManager(76-204)
🪛 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)
30-30: Link fragments should be valid
(MD051, link-fragments)
63-63: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
72-72: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
601-601: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
614-614: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
640-640: 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). (1)
- GitHub Check: Analyze (go)
Comment on lines
+289
to
+502
| function mergeProtoResults( | ||
| results: Array<{ proto: string; lockData: ProtoLock }>, | ||
| options: { | ||
| serviceName: string; | ||
| packageName: string; | ||
| goPackage?: string; | ||
| javaPackage?: string; | ||
| javaOuterClassname?: string; | ||
| javaMultipleFiles?: boolean; | ||
| csharpNamespace?: string; | ||
| rubyPackage?: string; | ||
| phpNamespace?: string; | ||
| phpMetadataNamespace?: string; | ||
| objcClassPrefix?: string; | ||
| swiftPrefix?: string; | ||
| }, | ||
| ): string { | ||
| if (results.length === 0) { | ||
| throw new Error('No proto results to merge'); | ||
| } | ||
|
|
||
| if (results.length === 1) { | ||
| return results[0].proto; | ||
| } | ||
|
|
||
| // Parse each proto file to extract components | ||
| const allMessages = new Set<string>(); | ||
| const allEnums = new Set<string>(); | ||
| const allRpcMethods: string[] = []; | ||
|
|
||
| for (const result of results) { | ||
| const lines = result.proto.split('\n'); | ||
| let inService = false; | ||
| let inMessage = false; | ||
| let inEnum = false; | ||
| let braceCount = 0; | ||
| let currentBlock: string[] = []; | ||
|
|
||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
|
|
||
| // Track service block | ||
| if (trimmed.startsWith('service ')) { | ||
| inService = true; | ||
| braceCount = 0; | ||
| continue; | ||
| } | ||
|
|
||
| if (inService) { | ||
| if (trimmed.includes('{')) { | ||
| braceCount++; | ||
| } | ||
| if (trimmed.includes('}')) { | ||
| braceCount--; | ||
| } | ||
|
|
||
| // Extract RPC methods | ||
| if (trimmed.startsWith('rpc ')) { | ||
| allRpcMethods.push(line); | ||
| } | ||
|
|
||
| if (braceCount === 0) { | ||
| inService = false; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // Track message blocks - only start a new message if not already in one | ||
| if (!inMessage && !inEnum && trimmed.startsWith('message ')) { | ||
| inMessage = true; | ||
| braceCount = 0; | ||
| currentBlock = [line]; | ||
| // Count braces on the message declaration line | ||
| if (trimmed.includes('{')) { | ||
| braceCount++; | ||
| } | ||
| if (trimmed.includes('}')) { | ||
| braceCount--; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (inMessage) { | ||
| currentBlock.push(line); | ||
| if (trimmed.includes('{')) { | ||
| braceCount++; | ||
| } | ||
| if (trimmed.includes('}')) { | ||
| braceCount--; | ||
| } | ||
|
|
||
| if (braceCount === 0) { | ||
| const messageText = currentBlock.join('\n'); | ||
| allMessages.add(messageText); | ||
| inMessage = false; | ||
| currentBlock = []; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // Track enum blocks - only start a new enum if not already in one | ||
| if (!inMessage && !inEnum && trimmed.startsWith('enum ')) { | ||
| inEnum = true; | ||
| braceCount = 0; | ||
| currentBlock = [line]; | ||
| // Count braces on the enum declaration line | ||
| if (trimmed.includes('{')) { | ||
| braceCount++; | ||
| } | ||
| if (trimmed.includes('}')) { | ||
| braceCount--; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (inEnum) { | ||
| currentBlock.push(line); | ||
| if (trimmed.includes('{')) { | ||
| braceCount++; | ||
| } | ||
| if (trimmed.includes('}')) { | ||
| braceCount--; | ||
| } | ||
|
|
||
| if (braceCount === 0) { | ||
| const enumText = currentBlock.join('\n'); | ||
| allEnums.add(enumText); | ||
| inEnum = false; | ||
| currentBlock = []; | ||
| } | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Build the merged proto file | ||
| const parts: string[] = []; | ||
|
|
||
| // Add syntax | ||
| parts.push('syntax = "proto3";'); | ||
| parts.push(''); | ||
|
|
||
| // Add package | ||
| parts.push(`package ${options.packageName};`); | ||
| parts.push(''); | ||
|
|
||
| // Add language-specific options | ||
| if (options.goPackage) { | ||
| parts.push(`option go_package = "${options.goPackage}";`); | ||
| } | ||
| if (options.javaPackage) { | ||
| parts.push(`option java_package = "${options.javaPackage}";`); | ||
| } | ||
| if (options.javaOuterClassname) { | ||
| parts.push(`option java_outer_classname = "${options.javaOuterClassname}";`); | ||
| } | ||
| if (options.javaMultipleFiles) { | ||
| parts.push('option java_multiple_files = true;'); | ||
| } | ||
| if (options.csharpNamespace) { | ||
| parts.push(`option csharp_namespace = "${options.csharpNamespace}";`); | ||
| } | ||
| if (options.rubyPackage) { | ||
| parts.push(`option ruby_package = "${options.rubyPackage}";`); | ||
| } | ||
| if (options.phpNamespace) { | ||
| parts.push(`option php_namespace = "${options.phpNamespace}";`); | ||
| } | ||
| if (options.phpMetadataNamespace) { | ||
| parts.push(`option php_metadata_namespace = "${options.phpMetadataNamespace}";`); | ||
| } | ||
| if (options.objcClassPrefix) { | ||
| parts.push(`option objc_class_prefix = "${options.objcClassPrefix}";`); | ||
| } | ||
| if (options.swiftPrefix) { | ||
| parts.push(`option swift_prefix = "${options.swiftPrefix}";`); | ||
| } | ||
|
|
||
| if ( | ||
| options.goPackage || | ||
| options.javaPackage || | ||
| options.javaOuterClassname || | ||
| options.javaMultipleFiles || | ||
| options.csharpNamespace || | ||
| options.rubyPackage || | ||
| options.phpNamespace || | ||
| options.phpMetadataNamespace || | ||
| options.objcClassPrefix || | ||
| options.swiftPrefix | ||
| ) { | ||
| parts.push(''); | ||
| } | ||
|
|
||
| // Add all unique messages | ||
| for (const message of allMessages) { | ||
| parts.push(message); | ||
| parts.push(''); | ||
| } | ||
|
|
||
| // Add all unique enums | ||
| for (const enumDef of allEnums) { | ||
| parts.push(enumDef); | ||
| parts.push(''); | ||
| } | ||
|
|
||
| // Add service with all RPC methods | ||
| parts.push(`service ${options.serviceName} {`); | ||
| for (const rpcMethod of allRpcMethods) { | ||
| parts.push(rpcMethod); | ||
| } | ||
| parts.push('}'); | ||
|
|
||
| return parts.join('\n'); | ||
| } |
There was a problem hiding this comment.
Merged proto output is invalid (drops imports & RPC options).
mergeProtoResults reconstructs the final proto from raw strings but never copies import statements or the contents of RPC method blocks. Any operation using wrapper scalars (importing google/protobuf/wrappers.proto) or the new --query-idempotency flag (emitting option idempotency_level = … inside the RPC) will lose those lines in the merged output, producing proto files that both fail to compile and silently drop user-selected options. Please preserve the full header (including imports) and each RPC block instead of stripping them away—e.g. parse each proto via protobufjs.parse or keep the entire service block text while merging.
Comment on lines
+5
to
+31
| ## Table of Contents | ||
|
|
||
| - [Overview](#overview) | ||
| - [Concepts](#concepts) | ||
| - [Named Operations Requirement](#named-operations-requirement) | ||
| - [Generation Modes](#generation-modes) | ||
| - [Field Number Stability](#field-number-stability) | ||
| - [Idempotency Levels](#idempotency-levels) | ||
| - [CLI Reference](#cli-reference) | ||
| - [Command Options](#command-options) | ||
| - [Examples](#examples) | ||
| - [API Reference](#api-reference) | ||
| - [compileOperationsToProto](#compileoperationstoproto) | ||
| - [Options Interface](#options-interface) | ||
| - [Tutorial](#tutorial) | ||
| - [Basic Usage](#basic-usage) | ||
| - [Working with Fragments](#working-with-fragments) | ||
| - [Handling Subscriptions](#handling-subscriptions) | ||
| - [Maintaining Field Stability](#maintaining-field-stability) | ||
| - [Advanced Topics](#advanced-topics) | ||
| - [Multi-Language Support](#multi-language-support) | ||
| - [Custom Scalar Mappings](#custom-scalar-mappings) | ||
| - [Proto Lock Files](#proto-lock-files) | ||
| - [Troubleshooting](#troubleshooting) | ||
| - [Best Practices](#best-practices) | ||
| - [FAQ](#faq) | ||
|
|
There was a problem hiding this comment.
Fix broken FAQ link in table of contents.
The ToC links to #faq, but the document never defines an “FAQ” section. Please either add the missing ## FAQ section or drop the entry so the ToC remains accurate and markdownlint (MD051) passes.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
10-10: Link fragments should be valid
(MD051, link-fragments)
25-25: Link fragments should be valid
(MD051, link-fragments)
30-30: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
In protographic/OPERATIONS_TO_PROTO.md around lines 5 to 31 the table of
contents includes a "- [FAQ](#faq)" entry but the document has no corresponding
"## FAQ" section; either remove the FAQ line from the ToC or add a "## FAQ"
heading with the expected content below the Advanced Topics/Troubleshooting
sections so the anchor resolves and markdownlint MD051 stops failing.
Comment on lines
+524
to
+553
| private createNestedListWrapperCallback(graphqlType: any): string { | ||
| // Import mapGraphQLTypeToProto to get type info | ||
| const { mapGraphQLTypeToProto } = require('./operations/type-mapper.js'); | ||
|
|
||
| const typeInfo = mapGraphQLTypeToProto(graphqlType, { | ||
| customScalarMappings: this.customScalarMappings, | ||
| }); | ||
|
|
||
| if (!typeInfo.requiresNestedWrapper) { | ||
| // This shouldn't happen, but return the type name as fallback | ||
| return typeInfo.typeName; | ||
| } | ||
|
|
||
| // Create the wrapper message | ||
| const wrapperName = typeInfo.typeName; | ||
| const nestingLevel = typeInfo.nestingLevel || 1; | ||
|
|
||
| // Extract base type name from wrapper name | ||
| // e.g., "ListOfListOfString" -> "String" | ||
| const baseTypeName = wrapperName.replace(/^(ListOf)+/, ''); | ||
|
|
||
| // Ensure all wrapper levels are created | ||
| if (!this.nestedListWrappers.has(wrapperName) && !this.createdMessages.has(wrapperName)) { | ||
| for (let i = 1; i <= nestingLevel; i++) { | ||
| this.createNestedListWrapper(i, baseTypeName); | ||
| } | ||
| } | ||
|
|
||
| return wrapperName; | ||
| } |
There was a problem hiding this comment.
require inside an ES module will crash nested-list generation.
This file is emitted as ESM (all other imports use import ... from), so require('./operations/type-mapper.js') throws ReferenceError: require is not defined the first time we hit a nested list. That blocks all operations containing multi-dimensional lists or nullable list wrappers.
Import mapGraphQLTypeToProto at the top and reuse it in the callback instead of calling require.
-import {
- createRequestMessageName,
- createResponseMessageName,
- createOperationMethodName,
-} from './naming-conventions.js';
+import {
+ createRequestMessageName,
+ createResponseMessageName,
+ createOperationMethodName,
+} from './naming-conventions.js';
+import { mapGraphQLTypeToProto } from './operations/type-mapper.js';
...
- private createNestedListWrapperCallback(graphqlType: any): string {
- // Import mapGraphQLTypeToProto to get type info
- const { mapGraphQLTypeToProto } = require('./operations/type-mapper.js');
-
+ private createNestedListWrapperCallback(graphqlType: any): string {
const typeInfo = mapGraphQLTypeToProto(graphqlType, {
customScalarMappings: this.customScalarMappings,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private createNestedListWrapperCallback(graphqlType: any): string { | |
| // Import mapGraphQLTypeToProto to get type info | |
| const { mapGraphQLTypeToProto } = require('./operations/type-mapper.js'); | |
| const typeInfo = mapGraphQLTypeToProto(graphqlType, { | |
| customScalarMappings: this.customScalarMappings, | |
| }); | |
| if (!typeInfo.requiresNestedWrapper) { | |
| // This shouldn't happen, but return the type name as fallback | |
| return typeInfo.typeName; | |
| } | |
| // Create the wrapper message | |
| const wrapperName = typeInfo.typeName; | |
| const nestingLevel = typeInfo.nestingLevel || 1; | |
| // Extract base type name from wrapper name | |
| // e.g., "ListOfListOfString" -> "String" | |
| const baseTypeName = wrapperName.replace(/^(ListOf)+/, ''); | |
| // Ensure all wrapper levels are created | |
| if (!this.nestedListWrappers.has(wrapperName) && !this.createdMessages.has(wrapperName)) { | |
| for (let i = 1; i <= nestingLevel; i++) { | |
| this.createNestedListWrapper(i, baseTypeName); | |
| } | |
| } | |
| return wrapperName; | |
| } | |
| private createNestedListWrapperCallback(graphqlType: any): string { | |
| const typeInfo = mapGraphQLTypeToProto(graphqlType, { | |
| customScalarMappings: this.customScalarMappings, | |
| }); | |
| if (!typeInfo.requiresNestedWrapper) { | |
| // This shouldn't happen, but return the type name as fallback | |
| return typeInfo.typeName; | |
| } | |
| // Create the wrapper message | |
| const wrapperName = typeInfo.typeName; | |
| const nestingLevel = typeInfo.nestingLevel || 1; | |
| // Extract base type name from wrapper name | |
| // e.g., "ListOfListOfString" -> "String" | |
| const baseTypeName = wrapperName.replace(/^(ListOf)+/, ''); | |
| // Ensure all wrapper levels are created | |
| if (!this.nestedListWrappers.has(wrapperName) && !this.createdMessages.has(wrapperName)) { | |
| for (let i = 1; i <= nestingLevel; i++) { | |
| this.createNestedListWrapper(i, baseTypeName); | |
| } | |
| } | |
| return wrapperName; | |
| } |
🤖 Prompt for AI Agents
In protographic/src/operation-to-proto.ts around lines 524 to 553, the function
createNestedListWrapperCallback uses require('./operations/type-mapper.js')
which fails in ESM (ReferenceError: require is not defined); replace the dynamic
require by importing mapGraphQLTypeToProto at the top of the file (e.g., add a
named import from './operations/type-mapper.js') and then use that imported
function inside createNestedListWrapperCallback, removing the require call so
nested-list generation works in ESM.
Comment on lines
+248
to
+262
| // First, add nested types (messages and enums) | ||
| for (const nested of Object.values(message.nestedArray)) { | ||
| if (nested instanceof protobuf.Type) { | ||
| const nestedLines = messageToProtoText(nested, options, indent + 1); | ||
| lines.push(...nestedLines); | ||
| } else if (nested instanceof protobuf.Enum) { | ||
| const nestedLines = enumToProtoText(nested, options, indent + 1); | ||
| lines.push(...nestedLines); | ||
| } | ||
| } | ||
|
|
||
| // Then, add fields | ||
| for (const field of message.fieldsArray) { | ||
| lines.push(...formatField(field, options, indent + 1)); | ||
| } |
There was a problem hiding this comment.
Emit reserved declarations to preserve removed field numbers
ProtoLockManager populates Type.reserved so we keep gaps for removed fields and names. Because messageToProtoText ignores that array, every reserved number/name disappears from the emitted proto, meaning the generated artifact no longer advertises which tags are off-limits. That regresses the compatibility guarantees we get from the lock: consumers editing the proto (or regenerating it without the lock) can now accidentally reuse wire tags that should stay blocked. Please render the reserved clauses before the field list.
@@
- // Then, add fields
+ // Emit reserved field numbers / names
+ if (Array.isArray(message.reserved) && message.reserved.length > 0) {
+ const reservedNumberClauses: string[] = [];
+ const reservedNameClauses: string[] = [];
+
+ for (const reservation of message.reserved) {
+ if (Array.isArray(reservation)) {
+ const [start, end] = reservation;
+ reservedNumberClauses.push(start === end ? `${start}` : `${start} to ${end}`);
+ } else if (typeof reservation === 'string') {
+ if (reservation.includes('to')) {
+ reservedNumberClauses.push(reservation);
+ } else if (/^\d+$/.test(reservation)) {
+ reservedNumberClauses.push(reservation);
+ } else {
+ reservedNameClauses.push(`"${reservation}"`);
+ }
+ }
+ }
+
+ if (reservedNumberClauses.length > 0) {
+ lines.push(`${indentStr} reserved ${reservedNumberClauses.join(', ')};`);
+ }
+
+ if (reservedNameClauses.length > 0) {
+ lines.push(`${indentStr} reserved ${reservedNameClauses.join(', ')};`);
+ }
+ }
+
+ // Then, add fields
for (const field of message.fieldsArray) {
lines.push(...formatField(field, options, indent + 1));
}🤖 Prompt for AI Agents
In protographic/src/operations/proto-text-generator.ts around lines 248 to 262,
messageToProtoText currently emits nested types first and then fields, but it
omits rendering message.reserved/reservedNames so reserved tag numbers and names
are lost; update the function to render reserved declarations immediately before
emitting the field list: inspect message.reserved (and message.reservedNames if
present), format them into proper "reserved" lines (single numbers, ranges, and
quoted names as appropriate) using the same indentation logic as fields, push
those lines into the output array after nested types and before iterating
message.fieldsArray, and ensure formatting matches existing helper conventions
so the emitted proto preserves reserved declarations.
Comment on lines
+145
to
+161
| for (const range of type.reserved) { | ||
| if (typeof range === 'string') { | ||
| // Skip string reserved fields (field names) | ||
| continue; | ||
| } | ||
| if (Array.isArray(range)) { | ||
| // Handle number arrays [start, end] | ||
| if (range.length === 2) { | ||
| const [start, end] = range; | ||
| for (let i = start; i <= end; i++) { | ||
| numbers.push(i); | ||
| } | ||
| } else { | ||
| numbers.push(...range); | ||
| } | ||
| // For ranges, just return the start for simplicity | ||
| return range.start; | ||
| }) || [] | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Reserved single numbers are silently dropped.
type.reserved can contain bare numbers in protobufjs. The current loop skips them, so callers never see reserved tags like reserved 5;, and downstream tests risk reusing those numbers. Please treat numeric entries explicitly.
Apply this diff:
- for (const range of type.reserved) {
- if (typeof range === 'string') {
- // Skip string reserved fields (field names)
- continue;
- }
- if (Array.isArray(range)) {
+ for (const range of type.reserved) {
+ if (typeof range === 'string') {
+ continue;
+ }
+ if (typeof range === 'number') {
+ numbers.push(range);
+ continue;
+ }
+ if (Array.isArray(range)) {
// Handle number arrays [start, end]
if (range.length === 2) {
const [start, end] = range;
for (let i = start; i <= end; i++) {
numbers.push(i);
}
} else {
numbers.push(...range);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const range of type.reserved) { | |
| if (typeof range === 'string') { | |
| // Skip string reserved fields (field names) | |
| continue; | |
| } | |
| if (Array.isArray(range)) { | |
| // Handle number arrays [start, end] | |
| if (range.length === 2) { | |
| const [start, end] = range; | |
| for (let i = start; i <= end; i++) { | |
| numbers.push(i); | |
| } | |
| } else { | |
| numbers.push(...range); | |
| } | |
| // For ranges, just return the start for simplicity | |
| return range.start; | |
| }) || [] | |
| ); | |
| } | |
| } | |
| for (const range of type.reserved) { | |
| if (typeof range === 'string') { | |
| continue; | |
| } | |
| if (typeof range === 'number') { | |
| numbers.push(range); | |
| continue; | |
| } | |
| if (Array.isArray(range)) { | |
| // Handle number arrays [start, end] | |
| if (range.length === 2) { | |
| const [start, end] = range; | |
| for (let i = start; i <= end; i++) { | |
| numbers.push(i); | |
| } | |
| } else { | |
| numbers.push(...range); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In protographic/tests/util.ts around lines 145 to 161, the loop over
type.reserved currently skips non-string, non-array entries (silently dropping
bare numeric reserved tags); update the branch so that when typeof range ===
'number' you push that number into numbers (e.g., numbers.push(range)), and keep
the existing handling for arrays and strings; ensure no other reserved entry
types are ignored so single numeric reserved tags like `reserved 5;` are
preserved.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Release Notes
New Features
--with-operationsflag, enabling proto generation from GraphQL operation files.Documentation
Tests