feat: add support for requires in protographic #2439
+7,194
−2,312
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
Contributor
WalkthroughAdds GraphQL federation Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2439 +/- ##
===========================================
- Coverage 61.63% 41.91% -19.73%
===========================================
Files 229 998 +769
Lines 23894 138473 +114579
Branches 0 7993 +7993
===========================================
+ Hits 14728 58038 +43310
- Misses 7924 78810 +70886
- Partials 1242 1625 +383
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
Noroth
requested review from
Aenimus,
JivusAyrus,
StarpTech,
devsergiy,
endigma,
jensneuse and
wilsonrivera
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@protographic/src/abstract-selection-rewriter.ts`:
- Line 57: The currentType property is only set in the constructor and never
updated during AST traversal, so findNamedTypeForField always resolves against
the root type; update currentType as the visitor descends and ascends into
fields: when entering a field (or selection set) look up the field's type from
the previous currentType using the same logic as findNamedTypeForField and set
currentType to the named/unwrapped type, and when leaving the field restore the
previous currentType (e.g., push/pop or store parent type); modify the visitor
methods in the AbstractSelectionRewriter class that handle entering/leaving
fields/selections to maintain currentType correctly so nested selections resolve
to their actual object types.
In `@protographic/src/naming-conventions.ts`:
- Around line 88-110: The doc examples for createRequiredFieldsMethodName are
out of sync with createMethodSuffixFromEntityKey which sorts keys; update the
examples or remove the sort—prefer updating the examples: change the example
showing "name,id" to reflect the sorted suffix (i.e.,
createMethodSuffixFromEntityKey('name,id') => 'ByIdAndName' and
createRequiredFieldsMethodName('User','post','name,id') =>
'RequireUserPostByIdAndName'); reference createRequiredFieldsMethodName and
createMethodSuffixFromEntityKey when making the change.
In `@protographic/src/required-fields-visitor.ts`:
- Around line 496-519: The code in handleCompositeType overwrites
this.current.compositeType for each interface/union field so only the last
composite is kept; change the message model to hold an array (e.g.,
this.current.compositeTypes) and update handleCompositeType to push a new
composite metadata object instead of assigning to compositeType (use
CompositeMessageKind.INTERFACE/UNION, implementingTypes/memberTypes and typeName
as in the diff); also update any consumers/readers of current.compositeType to
iterate the new compositeTypes array (and adjust types/interfaces for the
message object accordingly).
- Around line 439-477: The nested message naming uses the object type name which
can collide when the same object type is used by multiple fields; update
onEnterSelectionSet to derive a unique nested message identifier (e.g., include
the parent field name or path) instead of solely using this.currentType.name,
then check this.current.nestedMessages for an existing nested by that identifier
and reuse it (push existing into stack and set this.current) instead of always
creating a new one; reference functions/vars to change: onEnterSelectionSet,
findObjectTypeForField / findObjectType, this.current, this.currentType,
this.current.nestedMessages, nested (ProtoMessage), this.stack, and
this.current.name to ensure no duplicate nested message definitions.
- Around line 141-152: The visitor currently iterates raw key directive strings
and emits duplicate RPC/messages when keys differ only by order/spacing; update
visit() to normalize and deduplicate key field sets (use the same normalization
that createRequiredFieldsMethodName uses or normalize by parsing, trimming and
sorting field names) by computing a normalizedKey (e.g., from
getKeyFieldsString(keyDirective) then normalize) and maintain a Set of
seenNormalizedKeys; if normalizedKey is already seen skip emitting (skip setting
this.mapping[this.currentKeyFieldsString] and calling visit(this.fieldSetDoc,
this.visitor)), otherwise add to seen and proceed, ensuring you still reference
this.currentKeyFieldsString/getKeyFieldsString and the mapping/visit calls when
emitting the first occurrence.
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 397-424: validateDisallowAbstractTypesForRequires currently calls
parse(fieldSelections.value) which can throw and abort validation; wrap the
parse call in a try/catch inside validateDisallowAbstractTypesForRequires, and
on parse failure convert the exception into a validation error via this.addError
(include parse error message and ctx.node.loc) instead of rethrowing; only
proceed to construct SelectionSetValidationVisitor, call visit(), and relay its
errors/warnings if parse succeeds.
In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 110-181: getFieldDefinition currently throws when a field isn't
found which aborts validation; change it to return null (or undefined) instead
and push a validation error into this.validationResult.errors (include field
name and this.currentType.name for context). Update the getFieldDefinition
signature to allow null and remove the throw; then update callers — onEnterField
and onEnterSelectionSet — to check the returned value (e.g., if (!fieldDef) {
return; } or push errors as needed) before using fieldDef.type or accessing
properties. Ensure error messages match existing validation style and keep
traversal/validation continuing instead of throwing.
In `@protographic/tests/field-set/01-basics.test.ts`:
- Around line 69-122: The test's `@requires` field set never contains duplicates
so the deduplication logic isn't exercised; update the SDL in this test (the sdl
string used to build the schema in the 'should visit a field set for a scalar
type and deduplicate fields' test) to include a duplicate selection (e.g. change
`@requires`(fields: "name") to `@requires`(fields: "name name") or otherwise repeat
"name") so RequiredFieldsVisitor receives a duplicated field in fieldSet and the
dedupe path is tested.
🧹 Nitpick comments (5)
protographic/src/abstract-selection-rewriter.ts (1)
173-175: Unused private method.The
fieldDefinitionmethod is defined but never called anywhere in this class.♻️ Remove unused method
- /** - * Retrieves the field definition for a given field name from the current type. - * - * `@param` fieldName - The name of the field to look up - * `@returns` The GraphQL field definition, or undefined if not found - */ - private fieldDefinition(fieldName: string): GraphQLField<any, any, any> | undefined { - return this.currentType.getFields()[fieldName]; - }protographic/tests/sdl-to-proto/01-basic-types.test.ts (1)
1-1: Unused import:itis imported but not used in this file.All tests in this file use
test(...)syntax. Theitimport appears unused.♻️ Remove unused import
-import { describe, expect, it, test } from 'vitest'; +import { describe, expect, test } from 'vitest';protographic/tests/sdl-to-mapping/03-federation.test.ts (2)
1008-1008: Stale comment - appears to be a leftover from development.The comment
// RequireWarehouseStockHealthScoreByIdFields.RestockDatadoesn't match the test context (this test usesProduct, notWarehouse).🧹 Remove stale comment
const mapping = compileGraphQLToMapping(sdl, 'ProductService'); - // RequireWarehouseStockHealthScoreByIdFields.RestockData expect(mapping.toJson()).toMatchInlineSnapshot(`
1103-1103: Stale comment - same leftover as above.🧹 Remove stale comment
const mapping = compileGraphQLToMapping(sdl, 'ProductService'); - // RequireWarehouseStockHealthScoreByIdFields.RestockData expect(mapping.toJson()).toMatchInlineSnapshot(`protographic/src/sdl-to-mapping-visitor.ts (1)
131-141: Remove stale TODO to avoid confusion.Required-field mapping is now implemented below, so this TODO is misleading.
💡 Suggested cleanup
- // todo: add required fields mappingWould you like me to open a small cleanup issue or prep a patch?
| private readonly visitor: ASTVisitor; | ||
| private readonly fieldSetDoc: DocumentNode; | ||
| public readonly schema: GraphQLSchema; | ||
| private currentType: GraphQLObjectType; |
Contributor
There was a problem hiding this comment.
currentType is never updated during traversal.
The currentType field is set once in the constructor but never updated as the visitor traverses nested fields. This means findNamedTypeForField will always look up fields from the root object type, which may cause incorrect type resolution for deeply nested selections.
🤖 Prompt for AI Agents
In `@protographic/src/abstract-selection-rewriter.ts` at line 57, The currentType
property is only set in the constructor and never updated during AST traversal,
so findNamedTypeForField always resolves against the root type; update
currentType as the visitor descends and ascends into fields: when entering a
field (or selection set) look up the field's type from the previous currentType
using the same logic as findNamedTypeForField and set currentType to the
named/unwrapped type, and when leaving the field restore the previous
currentType (e.g., push/pop or store parent type); modify the visitor methods in
the AbstractSelectionRewriter class that handle entering/leaving
fields/selections to maintain currentType correctly so nested selections resolve
to their actual object types.
Contributor
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-mapping-visitor.ts (1)
130-141:⚠️ Potential issue | 🟡 MinorRemove the stale TODO comment.
The required-fields mapping is now implemented below, so this TODO is misleading.🧹 Suggested cleanup
- // todo: add required fields mapping
Contributor
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)
.github/workflows/protographic.yaml (1)
26-36:⚠️ Potential issue | 🔴 CriticalInstall step should include all packages targeted by the build.
pnpm install --filter ./protographicinstalls./protographicand its dependencies, but it may not reliably install the devDependencies of workspace packages like./compositionand./connectthat are required for their build scripts to run. The build step runspnpm runon all three packages, so ensure all their devDependencies are installed.✅ Suggested fix
- - name: Install dependencies - run: pnpm install --filter ./protographic + - name: Install dependencies + run: pnpm install --filter ./connect --filter ./composition --filter ./protographic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@protographic/src/abstract-selection-rewriter.ts`:
- Around line 122-142: The code replaces ctx.node.selections with
inlineFragments even when inlineFragments is empty, which drops interface-level
fields; add an early return (or skip the replacement) right after computing
inlineFragments (check inlineFragments.length === 0) to avoid mutating
ctx.node.selections when there are no inline fragments, then only proceed with
distributing fields into fragments (using fields, inlineFragments, this.hasField
and fragment.selectionSet.selections) when fragments exist.
In `@protographic/src/required-fields-visitor.ts`:
- Around line 445-494: The enter/leave imbalance happens because
onEnterSelectionSet sometimes returns before pushing, but onLeaveSelectionSet
always pops; fix by tracking whether a push happened and only pop when it did:
in onEnterSelectionSet compute a local pushed = false and set pushed = true
right before you push this.currentType onto this.ancestors and this.current onto
this.stack (and when you add nested message), then push that pushed flag onto a
new boolean stack (e.g., this.selectionSetPushStack) so the state is paired; in
onLeaveSelectionSet pop the flag from this.selectionSetPushStack and only pop
this.ancestors/this.stack and restore this.currentType/this.current when the
flag is true, leaving them unchanged when false. Ensure the new stack is
initialized on the class and used consistently with the existing ancestors/stack
fields.
In `@protographic/src/sdl-to-mapping-visitor.ts`:
- Around line 187-196: getRequiredFieldSet assumes the directive argument is a
StringValueNode and casts without checking, which yields undefined node.value
for non-string kinds; update getRequiredFieldSet to first inspect the directive
argument's kind (compare to Kind.STRING) before casting to StringValueNode,
throw a clear error when the argument is missing or not Kind.STRING, and ensure
Kind is imported from 'graphql' so the code safely accesses node.value for the
REQUIRES_DIRECTIVE_NAME lookup.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@protographic/src/selection-set-validation-visitor.ts`:
- Around line 110-115: When getFieldDefinition returns null in onEnterField,
return false (not undefined) so traversal into that field's subtree is skipped;
similarly, when onEnterSelectionSet cannot resolve the field/type, return false
instead of undefined. Update onEnterField (uses getFieldDefinition) and
onEnterSelectionSet to return false on failed lookups, and modify
onLeaveSelectionSet to only pop from the ancestors/currentType stacks when an
entry was actually pushed (e.g., check ancestors length or a pushed-flag) to
avoid corrupting currentType.
- Around line 110-123: The validator in SelectionSetValidationVisitor (methods
onEnterField and onEnterSelectionSet) wrongly rejects abstract types and inline
fragments for `@requires` field sets; remove or bypass the checks that call
isAbstractType(namedType) and that reject InlineFragment nodes so that abstract
return types (interfaces/unions) and inline fragments are allowed to pass
through to RequiredFieldsVisitor/AbstractSelectionRewriter for normalization.
Specifically, update onEnterField (remove the block that pushes the "Abstract
types are not allowed..." error and returns BREAK for abstract namedType) and
update onEnterSelectionSet (remove or guard the rejection of InlineFragment
nodes) — or alternatively wrap those checks so they are skipped when processing
`@requires` field sets handled by AbstractSelectionRewriter/RequiredFieldsVisitor.
In `@protographic/tests/field-set/01-basics.test.ts`:
- Around line 23-24: The docstring for the entityName field incorrectly claims a
default to the `@key` entity; update the comment for the entityName property (the
entityName field) to remove the "defaults to finding the type with `@key`
directive" phrase and clearly state that entityName is required (or,
alternatively, implement an actual fallback to derive entityName from the `@key`
directive in the code that constructs/validates entityName such as the
factory/parser that populates entityName); prefer the doc-only change: edit the
comment above the entityName declaration to indicate it is required and does not
default to `@key`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@protographic/src/proto-utils.ts`:
- Around line 289-315: The JSDoc for createNestedListWrapper is incorrect: it
says the function returns the wrapper message name but the implementation
(createNestedListWrapper -> builds wrapperName and returns
buildWrapperMessage(...).join('\n')) actually returns the rendered proto message
string; update the `@returns` description to state it returns the generated
wrapper message definition as a string (e.g., the full proto message text), and
optionally mention that it uses buildWrapperMessage(includeComments,
wrapperName, level, baseType) to render it; keep references to
createNestedListWrapper, wrapperName, and buildWrapperMessage so future readers
can locate the logic.
- Around line 104-141: The oneof field numbers in buildCompositeTypeMessage are
assigned based on the original compositeTypes array order (from memberTypes or
implementingTypes) which is non-deterministic; before iterating, sort
compositeTypes deterministically (e.g., lexicographically by the type string) so
the loop that calls graphqlFieldToProtoField(compositeType) and assigns =
${index + 1} produces stable, repeatable field numbers and avoids
wire-compatibility breaks.
- Around line 149-171: The description can contain "/*" or "*/" which will break
the generated block comment; before trimming/splitting the description in
formatComment, sanitize the description string (e.g., replace "/*" with "/ *"
and "*/" with "* /" or another safe replacement) so the block opener/closer are
not present in the content, then proceed to compute lines and render single-line
(LINE_COMMENT_PREFIX) or multi-line (BLOCK_COMMENT_START/BLOCK_COMMENT_END)
comments as before; update formatComment to perform this replacement on the
description variable prior to using it.
🧹 Nitpick comments (1)
protographic/src/sdl-to-mapping-visitor.ts (1)
135-140: Remove the stale TODO comment.
The required-fields mapping is now implemented, so this TODO is misleading.🧹 Suggested cleanup
- // todo: add required fields mapping
Comment on lines
104
to
141
| function buildCompositeTypeMessage( | ||
| includeComments: boolean, | ||
| compositeType: CompositeMessageDefinition, | ||
| indent: number, | ||
| ): string[] { | ||
| const lines: string[] = []; | ||
|
|
||
| if (includeComments && compositeType.description) { | ||
| lines.push(...formatComment(includeComments, compositeType.description, indent)); | ||
| } | ||
|
|
||
| let oneOfName = ''; | ||
| let compositeTypes: string[] = []; | ||
|
|
||
| if (isUnionMessageDefinition(compositeType)) { | ||
| oneOfName = 'value'; | ||
| compositeTypes = compositeType.memberTypes; | ||
| } else { | ||
| oneOfName = 'instance'; | ||
| compositeTypes = compositeType.implementingTypes; | ||
| } | ||
|
|
||
| lines.push( | ||
| indentContent(indent, `message ${compositeType.typeName} {`), | ||
| indentContent(indent + 1, `oneof ${oneOfName} {`), | ||
| ); | ||
|
|
||
| compositeTypes.forEach((compositeType, index) => { | ||
| lines.push( | ||
| indentContent(indent + 2, `${compositeType} ${graphqlFieldToProtoField(compositeType)} = ${index + 1};`), | ||
| ); | ||
| }); | ||
|
|
||
| lines.push(indentContent(indent + 1, '}')); | ||
| lines.push(indentContent(indent, '}')); | ||
|
|
||
| return lines; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the definition of CompositeMessageDefinition
rg "CompositeMessageDefinition" -n .Repository: wundergraph/cosmo
Length of output: 631
🏁 Script executed:
sed -n '1,200p' protographic/src/types.tsRepository: wundergraph/cosmo
Length of output: 3967
Ensure deterministic oneof field numbering
Field numbers currently derive from the unsorted memberTypes/implementingTypes array order; sort these arrays before iteration to guarantee stable numbering and avoid wire compatibility breaks.
Proposed diff
- compositeTypes.forEach((compositeType, index) => {
+ const orderedTypes = [...compositeTypes].sort((a, b) => a.localeCompare(b));
+ orderedTypes.forEach((compositeType, index) => {
lines.push(
indentContent(indent + 2, `${compositeType} ${graphqlFieldToProtoField(compositeType)} = ${index + 1};`),
);
});🤖 Prompt for AI Agents
In `@protographic/src/proto-utils.ts` around lines 104 - 141, The oneof field
numbers in buildCompositeTypeMessage are assigned based on the original
compositeTypes array order (from memberTypes or implementingTypes) which is
non-deterministic; before iterating, sort compositeTypes deterministically
(e.g., lexicographically by the type string) so the loop that calls
graphqlFieldToProtoField(compositeType) and assigns = ${index + 1} produces
stable, repeatable field numbers and avoids wire-compatibility breaks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protographic/tests/sdl-to-proto/07-argument-ordering.test.ts (1)
375-378:⚠️ Potential issue | 🟡 MinorPotential bug:
visitor3usesmodifiedSchemainstead ofmodifiedSchema3.Line 376 creates
visitor3withmodifiedSchema(the second schema), but the test comment andmodifiedSchema3definition at lines 356-373 suggest it should usemodifiedSchema3(with re-addedidfield and newcreatedfield). This means the test isn't actually testing the third schema evolution scenario described in the comments.🐛 Proposed fix
// Create a third visitor using the same lock data - const visitor3 = new GraphQLToProtoTextVisitor(modifiedSchema, { + const visitor3 = new GraphQLToProtoTextVisitor(modifiedSchema3, { serviceName: 'UserService', lockData: lockData || undefined, });
🤖 Fix all issues with AI agents
In `@protographic/src/required-fields-visitor.ts`:
- Around line 133-138: The loop over this.keyDirectives is currently treating
every `@key` equally; update the iteration in required-fields-visitor (where
keyDirective is examined and getKeyFieldsString is called) to first inspect
keyDirective.arguments for an argument named "resolvable" and skip (continue) if
that argument exists and its value is the BooleanValue false, so non-resolvable
`@key` directives are not used to generate required-field RPCs; keep using
getKeyFieldsString, and throw the existing error only after confirming the
directive is resolvable.
🧹 Nitpick comments (8)
protographic/.eslintrc (2)
3-5: Normalize indentation in the plugins array.
Current spacing is inconsistent and hurts readability; align it with the rest of the JSON.
6-27: Avoid disablingno-undefand unused-vars globally.
These rules catch real runtime defects; consider scoping them to specific files viaoverridesinstead of turning them off globally.protographic/src/operations/message-builder.ts (2)
451-469: Inconsistent switch statement formatting.This switch statement has different formatting than the one in
collectFields(lines 128-170):
casestatements appear to lack indentation relative toswitch- Blank lines before
breakstatements (lines 454, 460, 465)breakappears at reduced indentationCompare with
collectFieldswhich has properly indented cases and no blank lines beforebreak. Since the commit mentions ESLint compliance, consider making this consistent throughout the file.🔧 Suggested formatting to match collectFields style
switch (selection.kind) { - case 'Field': { + case 'Field': { processFieldSelection(selection, message, fragmentType, typeInfo, options, fieldNumberManager, messagePath); - - break; - } - case 'InlineFragment': { + break; + } + case 'InlineFragment': { // Nested inline fragment processInlineFragment(selection, message, fragmentType, typeInfo, options, fieldNumberManager, messagePath); - - break; - } - case 'FragmentSpread': { + break; + } + case 'FragmentSpread': { processFragmentSpread(selection, message, fragmentType, typeInfo, options, fieldNumberManager, messagePath); - - break; - } - // No default + break; + } }
518-537: Same formatting inconsistency as above.This switch statement has the same formatting issues as
processInlineFragment: inconsistent indentation and blank lines beforebreakstatements. Apply the same fix pattern for consistency.protographic/src/proto-lock.ts (1)
106-136: Dead code: the reserved number reuse logic can never execute.The code at lines 110-114 searches
removedItemsfor a matching item name, but this condition can never be true:
removedItemsis populated at lines 67-71 with items that are not inavailableItems- The outer loop at line 106 iterates over
availableItems- Therefore, any
iteminavailableItemscannot exist inremovedItemsThe condition
removedItem === itemwill always be false, making the entireif (reservedNumber === undefined)branch at lines 116-124 always execute, and theelsebranch at lines 125-133 never execute.This matches the test expectation that re-added fields get new numbers (not reusing reserved ones), so no bug in behavior. However, the code suggests an intention to reuse reserved numbers that doesn't work.
Consider either:
- Removing the dead code (lines 109-114 and 125-133) for clarity
- Fixing the logic if reserved number reuse is actually desired
♻️ Simplified version without dead code
// Assign numbers to items that don't have one for (const item of availableItems) { if (newFieldsMap[item] === undefined) { - // Check if the item was previously removed (exists in our reservedNumbers) - let reservedNumber: number | undefined; - for (const [removedItem, number] of Object.entries(removedItems)) { - if (removedItem === item && reservedNumbers.includes(number)) { - reservedNumber = number; - } - } - - if (reservedNumber === undefined) { - // Find next available number - let nextNumber = maxNumber + 1; - while (reservedNumbers.includes(nextNumber)) { - nextNumber++; - } - - newFieldsMap[item] = nextNumber; - maxNumber = nextNumber; - } else { - // Reuse the reserved number for this item - newFieldsMap[item] = reservedNumber; - - // Remove from reserved list - const index = reservedNumbers.indexOf(reservedNumber); - if (index !== -1) { - reservedNumbers.splice(index, 1); - } - } + // Find next available number + let nextNumber = maxNumber + 1; + while (reservedNumbers.includes(nextNumber)) { + nextNumber++; + } + + newFieldsMap[item] = nextNumber; + maxNumber = nextNumber; } }protographic/tests/sdl-to-proto/06-field-ordering.test.ts (1)
3-3: Unused import:isNullfromlodash-es.The
isNullfunction is imported but never used in this file. This appears to be a leftover from refactoring.🧹 Suggested fix
-import { isNull } from 'lodash-es';protographic/src/sdl-to-mapping-visitor.ts (2)
2-2: Unused import:ArgumentNode.The
ArgumentNodetype is imported but doesn't appear to be used in this file.🧹 Suggested fix
import { - ArgumentNode, DirectiveNode, GraphQLEnumType,
139-139: Stale TODO comment.The TODO comment "todo: add required fields mapping" appears to be outdated since
createRequiredFieldsMapping(type)is called immediately after the key directive loop at line 143.🧹 Suggested fix
if (key) { // Create entity mapping for each key combination this.createEntityMapping(typeName, key); - // todo: add required fields mapping }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protographic/src/operations/message-builder.ts (1)
129-169:⚠️ Potential issue | 🟠 MajorFail fast on proto-field name collisions to avoid silent drops.
graphqlFieldToProtoFieldcan map distinct GraphQL fields to the same proto name (e.g.,fooBarvsfoo_bar). The current logic silently keeps the first and drops the rest, which can corrupt the generated schema. Consider detecting collisions and throwing with a clear error.Suggested fix
if (isObjectType(currentType) || isInterfaceType(currentType)) { const fieldName = selection.name.value; const protoFieldName = graphqlFieldToProtoField(fieldName); - if (!fieldNames.includes(protoFieldName)) { + const existing = fieldSelections.get(protoFieldName); + if (existing && existing.selection.name.value !== fieldName) { + throw new Error( + `Field name collision after proto mapping: "${existing.selection.name.value}" and ` + + `"${fieldName}" both map to "${protoFieldName}".`, + ); + } + if (!existing) { fieldNames.push(protoFieldName); fieldSelections.set(protoFieldName, { selection, type: currentType }); } }
🤖 Fix all issues with AI agents
In `@protographic/src/sdl-to-proto-visitor.ts`:
- Around line 1555-1564: The field-filtering logic in the Sdl-to-Proto visitor
is incorrectly excluding fields annotated with REQUIRES_DIRECTIVE_NAME; update
the filter that builds validFields (which currently uses
allFields.filter(...).filter(...)) to only exclude fields with the
EXTERNAL_DIRECTIVE_NAME (remove REQUIRES_DIRECTIVE_NAME from the directive
check), so fields with `@requires` remain included in the object message
generation (refer to variables allFields and validFields and the constants
EXTERNAL_DIRECTIVE_NAME and REQUIRES_DIRECTIVE_NAME to locate the code).
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.
Summary by CodeRabbit
New Features
Improvements
Validation
Tests
Checklist