Conversation
dkorittki
changed the title
@requires directive
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEmits Args proto messages for required fields with arguments, centralizes directive lookup into a generic helper, updates field-guarding to skip Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. Comment |
dkorittki
changed the title
@requires directive@requires directive
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protographic/tests/sdl-to-proto/04-federation.test.ts`:
- Around line 1321-1402: The snapshot is stale: the generator moved
field-argument handling for required fields into a dedicated RPC
(ResolveUserPost with ResolveUserPostArgs) and RequireUserPostByIdRequest should
only include the external context; update the inline snapshot so
RequireUserPostByIdRequest no longer contains field_args, add the
ResolveUserPost RPC and its ResolveUserPostArgs message, and ensure
RequireUserPostByIdContext still contains LookupUserByIdRequestKey and
RequireUserPostByIdFields; reference the symbols RequireUserPostByIdRequest,
RequireUserPostByIdContext, RequireUserPostByIdFields, ResolveUserPost, and
ResolveUserPostArgs when making the snapshot changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1edfb924-4a1f-44f6-8c7c-be2c25aed00f
📒 Files selected for processing (1)
protographic/tests/sdl-to-proto/04-federation.test.ts
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
protographic/src/sdl-to-proto-visitor.ts (1)
1138-1141: Consider skipping all@requiresfields in resolver RPC collection (not only arg-bearing ones).Line 1139 currently gates on
field.args.length > 0. A field with@requiresand@connect__fieldResolverbut no args can still generate a resolver RPC, creating a parallel resolution path.💡 Suggested change
- if (field.args.length > 0 && this.findDirective(field, REQUIRES_DIRECTIVE_NAME)) { + if (this.findDirective(field, REQUIRES_DIRECTIVE_NAME)) { continue }Based on learnings:
@requiresfields are intended to be resolved by dedicated RPCs to prevent bypassing dependency requirements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/sdl-to-proto-visitor.ts` around lines 1138 - 1141, The resolver-RPC collection currently only skips fields with `@requires` if they have arguments (the condition `field.args.length > 0 && this.findDirective(field, REQUIRES_DIRECTIVE_NAME)`); update the logic to skip any field that has the `@requires` directive regardless of args by using only the `this.findDirective(field, REQUIRES_DIRECTIVE_NAME)` check (and update the comment accordingly) so fields with `@connect__fieldResolver` but no args do not generate a parallel resolver RPC; reference the local variable `field`, the helper `findDirective`, and the constant `REQUIRES_DIRECTIVE_NAME` when making the change.protographic/tests/sdl-to-proto/04-federation.test.ts (1)
1296-1414: Add one list-argument variant to lockrepeatedbehavior inRequire...Args.This case validates scalar args well; adding something like
post(tags: [String!]!)would guard proto cardinality for generated args fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/tests/sdl-to-proto/04-federation.test.ts` around lines 1296 - 1414, Update the test SDL to include a list-argument variant so the generator emits a repeated field for args: change the User.post field signature to include a list arg (e.g., post(slug: String!, tags: [String!]!): Post!) and re-run/generate the expected proto snapshot so RequireUserPostByIdArgs contains a repeated string tags field (referencing RequireUserPostByIdArgs and RequireUserPostByIdArgs.tags) and adjust RequireUserPostByIdRequest/RequireUserPostByIdResponse expectations if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protographic/src/required-fields-visitor.ts`:
- Around line 339-344: The loop that maps this.requiredField.args into
requireArgsMessage.fields currently only copies typeName from
getProtoTypeFromGraphQL, causing list arguments to lose their repeated
cardinality; update the mapping inside the forEach (where
this.requiredField.args is iterated and objects are pushed onto
requireArgsMessage.fields) to capture the full result of
getProtoTypeFromGraphQL(false, d.type, true) (e.g., assign to a local variable
like protoType) and include its repeated flag (or equivalent property) in the
pushed object so list arguments are emitted with repeated: true while preserving
fieldName, typeName and fieldNumber.
---
Nitpick comments:
In `@protographic/src/sdl-to-proto-visitor.ts`:
- Around line 1138-1141: The resolver-RPC collection currently only skips fields
with `@requires` if they have arguments (the condition `field.args.length > 0 &&
this.findDirective(field, REQUIRES_DIRECTIVE_NAME)`); update the logic to skip
any field that has the `@requires` directive regardless of args by using only the
`this.findDirective(field, REQUIRES_DIRECTIVE_NAME)` check (and update the
comment accordingly) so fields with `@connect__fieldResolver` but no args do not
generate a parallel resolver RPC; reference the local variable `field`, the
helper `findDirective`, and the constant `REQUIRES_DIRECTIVE_NAME` when making
the change.
In `@protographic/tests/sdl-to-proto/04-federation.test.ts`:
- Around line 1296-1414: Update the test SDL to include a list-argument variant
so the generator emits a repeated field for args: change the User.post field
signature to include a list arg (e.g., post(slug: String!, tags: [String!]!):
Post!) and re-run/generate the expected proto snapshot so
RequireUserPostByIdArgs contains a repeated string tags field (referencing
RequireUserPostByIdArgs and RequireUserPostByIdArgs.tags) and adjust
RequireUserPostByIdRequest/RequireUserPostByIdResponse expectations if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7510e347-95f4-4d9d-b14d-c7b88e53f5ab
📒 Files selected for processing (4)
protographic/src/required-fields-visitor.tsprotographic/src/sdl-to-mapping-visitor.tsprotographic/src/sdl-to-proto-visitor.tsprotographic/tests/sdl-to-proto/04-federation.test.ts
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protographic/tests/sdl-to-mapping/03-federation.test.ts (1)
1326-1414: Prefer focused assertions over a full snapshot for this contract.The current snapshot is valid, but it is broader than the test intent (“no separate resolve mapping”). Narrow assertions would reduce unrelated snapshot churn.
♻️ Suggested assertion-focused rewrite
- expect(mapping.toJson()).toMatchInlineSnapshot(` - { ...large snapshot... } - `); + const json = mapping.toJson(); + + expect(json.entityMappings).toHaveLength(1); + expect(json.entityMappings[0]?.requiredFieldMappings).toHaveLength(1); + expect( + json.entityMappings[0]?.requiredFieldMappings?.[0]?.fieldMapping?.original, + ).toBe('post'); + + expect(json.operationMappings.some((m) => m.original === 'post')).toBe(false); + + const userType = json.typeFieldMappings.find((t) => t.type === 'User'); + const postField = userType?.fieldMappings.find((f) => f.original === 'post'); + expect(postField?.argumentMappings?.[0]?.original).toBe('slug');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/tests/sdl-to-mapping/03-federation.test.ts` around lines 1326 - 1414, The test currently asserts the entire mapping snapshot via expect(mapping.toJson()).toMatchInlineSnapshot(...) which is too broad; instead replace it with focused assertions that validate the specific contract "no separate resolve mapping": call const json = mapping.toJson() and assert json.entityMappings has the expected single entry (e.g. expect(json.entityMappings).toHaveLength(1)), assert that none of json.entityMappings has kind === 'resolve' (e.g. expect(json.entityMappings.some(e => e.kind === 'resolve')).toBe(false)), and then assert only the specific fields you care about in json.operationMappings and json.typeFieldMappings using expect.objectContaining or targeted length checks (e.g. expect(json.operationMappings).toEqual(expect.arrayContaining([expect.objectContaining({ mapped: 'QueryUser' })])) rather than matching the entire snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@protographic/tests/sdl-to-mapping/03-federation.test.ts`:
- Around line 1326-1414: The test currently asserts the entire mapping snapshot
via expect(mapping.toJson()).toMatchInlineSnapshot(...) which is too broad;
instead replace it with focused assertions that validate the specific contract
"no separate resolve mapping": call const json = mapping.toJson() and assert
json.entityMappings has the expected single entry (e.g.
expect(json.entityMappings).toHaveLength(1)), assert that none of
json.entityMappings has kind === 'resolve' (e.g.
expect(json.entityMappings.some(e => e.kind === 'resolve')).toBe(false)), and
then assert only the specific fields you care about in json.operationMappings
and json.typeFieldMappings using expect.objectContaining or targeted length
checks (e.g.
expect(json.operationMappings).toEqual(expect.arrayContaining([expect.objectContaining({
mapped: 'QueryUser' })])) rather than matching the entire snapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11f89669-ca2e-4afc-96bc-5d3b444260c4
📒 Files selected for processing (1)
protographic/tests/sdl-to-mapping/03-federation.test.ts
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protographic/src/sdl-to-proto-visitor.ts (1)
1138-1140: Tighten comment wording to match the actual guard condition.Line 1138 says all
@requiresfields are skipped, but Line 1139 skips only arg-bearing@requiresfields. Please make the comment precise to avoid future misreads.Suggested wording fix
- // Skip fields with `@requires` directive, they are managed by the required fields visitor + // Skip arg-bearing `@requires` fields; they are managed by the required fields visitor if (field.args.length > 0 && this.findDirective(field, REQUIRES_DIRECTIVE_NAME)) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/sdl-to-proto-visitor.ts` around lines 1138 - 1140, The comment above the guard is misleading: it claims all `@requires` fields are skipped but the code only skips fields that both have arguments and an `@requires` directive; update the comment to precisely state that only arg-bearing `@requires` fields are skipped because those are handled by the required fields visitor (referencing the `field` variable, `REQUIRES_DIRECTIVE_NAME`, and `findDirective` call so reviewers can locate the guard).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@protographic/src/sdl-to-proto-visitor.ts`:
- Around line 1138-1140: The comment above the guard is misleading: it claims
all `@requires` fields are skipped but the code only skips fields that both have
arguments and an `@requires` directive; update the comment to precisely state that
only arg-bearing `@requires` fields are skipped because those are handled by the
required fields visitor (referencing the `field` variable,
`REQUIRES_DIRECTIVE_NAME`, and `findDirective` call so reviewers can locate the
guard).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8338ca6-8481-4276-af1f-8d855d3b09dc
📒 Files selected for processing (2)
protographic/src/required-fields-visitor.tsprotographic/src/sdl-to-proto-visitor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- protographic/src/required-fields-visitor.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
protographic/src/required-fields-visitor.ts (1)
336-341:⚠️ Potential issue | 🟠 MajorPreserve
isRepeatedwhen mapping required-field arguments.On Line 338, only
typeNameis copied fromgetProtoTypeFromGraphQL(...). List-typed GraphQL arguments will be emitted as singular proto fields.💡 Proposed fix
- const requireArgsMessage: ProtoMessage = { - messageName: requireArgsMessageName, - fields: this.requiredField.args.map((d, i): ProtoMessageField => ({ - fieldName: graphqlArgumentToProtoField(d.name), - typeName: getProtoTypeFromGraphQL(false, d.type, true).typeName, - fieldNumber: i + 1, - })) - }; + const requireArgsMessage: ProtoMessage = { + messageName: requireArgsMessageName, + fields: this.requiredField.args.map((d, i): ProtoMessageField => { + const protoType = getProtoTypeFromGraphQL(false, d.type, true); + return { + fieldName: graphqlArgumentToProtoField(d.name), + typeName: protoType.typeName, + isRepeated: protoType.isRepeated, + fieldNumber: i + 1, + }; + }), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/src/required-fields-visitor.ts` around lines 336 - 341, The mapping for required-field arguments loses the repeated-ness because only typeName is taken from getProtoTypeFromGraphQL; update the mapping in the this.requiredField.args -> ProtoMessageField creation to call getProtoTypeFromGraphQL(...) once (e.g., const protoType = getProtoTypeFromGraphQL(false, d.type, true)) and include protoType.isRepeated as the isRepeated property on the produced ProtoMessageField (alongside fieldName, typeName and fieldNumber) so list-typed GraphQL arguments emit as repeated proto fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@protographic/src/required-fields-visitor.ts`:
- Around line 336-341: The mapping for required-field arguments loses the
repeated-ness because only typeName is taken from getProtoTypeFromGraphQL;
update the mapping in the this.requiredField.args -> ProtoMessageField creation
to call getProtoTypeFromGraphQL(...) once (e.g., const protoType =
getProtoTypeFromGraphQL(false, d.type, true)) and include protoType.isRepeated
as the isRepeated property on the produced ProtoMessageField (alongside
fieldName, typeName and fieldNumber) so list-typed GraphQL arguments emit as
repeated proto fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d143cd1-0fcf-4bf5-bcc9-051b9cc151e1
📒 Files selected for processing (1)
protographic/src/required-fields-visitor.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2653 +/- ##
===========================================
+ Coverage 63.14% 86.20% +23.05%
===========================================
Files 249 20 -229
Lines 26661 4493 -22168
Branches 0 997 +997
===========================================
- Hits 16835 3873 -12962
+ Misses 8449 599 -7850
+ Partials 1377 21 -1356
🚀 New features to boost your workflow:
|
…vers-with-requires
Contributor
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
…vers-with-requires
Noroth
requested changes
Mar 18, 2026
…vers-with-requires
…vers-with-requires
Add a guard to skip @requires fields with arguments before trying to validate them as resolver fields.
…vers-with-requires
…vers-with-requires
Noroth
reviewed
Apr 1, 2026
Co-authored-by: Ludwig Bedacht <ludwig.bedacht@gmail.com>
Noroth
approved these changes
Apr 1, 2026
dkorittki
deleted the
dominik/eng-8643-add-support-for-field-resolvers-with-requires
branch
5 tasks
dkorittki
added a commit
to wundergraph/graphql-go-tools
that referenced
this pull request
Apr 7, 2026
…1452) ## Context A [change in protographic](wundergraph/cosmo#2653) allows to treat fields with arguments to be treated as a required fields alongside needed protobuf rpc's and message to retrieve field_arguments. A necessary change to allow field arguments on `@requires` fields. When giving this new proto spec and mapping to the engine such fields were silently ignoring the arguments — the gRPC call received the context (key + required fields) but no `field_args`, so the service always saw zero-value arguments. ## Implementation Wire field arguments through the `@requires` execution path (grpc datasource, federation visitor, compiler), doing it similar to how field resolvers already handle them. ## Tests Three new integration + execution-plan test scenarios covering: | Scenario | Args | |---|---| | Single required string arg | `prefix: String!` | | Two args, one repeated (list) | `prefixes: [String!]!, maxResults: Int!` | | Nullable arg (maps to `google.protobuf.StringValue`) | `prefix: String` | ## Checklist - [x] I have discussed my proposed changes in an issue and have received approval to proceed. - [x] I have followed the coding standards of the project. - [x] Tests or benchmarks have been added or updated.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Adds support for protographic to generate proper protobuf messages and RPC´s for fields with arguments and the
@requiresdirective. Instead of treating these fields as resolvables they are treated as required fields, so we generateRequire*protobuf messages / RPC´s instead ofResolve*.Checklist
Summary by CodeRabbit