feat: allow field resolvers on fields without arguments#2629
Merged
Conversation
Contributor
WalkthroughAdds support for field resolvers without arguments, updates visitor and validation logic to recognize a CONNECT_FIELD_RESOLVER directive, emits resolver RPCs/messages selectively, expands tests for no-arg resolvers and argument-list semantics, and applies documentation and formatting edits across protographic docs and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2629 +/- ##
===========================================
+ Coverage 64.58% 89.30% +24.71%
===========================================
Files 301 21 -280
Lines 42900 4527 -38373
Branches 4601 1248 -3353
===========================================
- Hits 27709 4043 -23666
+ Misses 15170 484 -14686
+ Partials 21 0 -21
🚀 New features to boost your workflow:
|
![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: 2
🤖 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/sdl-to-proto-visitor.ts`:
- Around line 1300-1313: The synthetic args message generation drops list-ness
because the mapping only forwards typeName; update the field mapping in the
hasArgs branch (the call that builds the message for argsMessageName inside
buildMessage) to include the isRepeated flag returned by getProtoTypeFromGraphQL
so repeated GraphQL argument types become "repeated" in the proto; i.e., when
iterating field.args use const protoType =
this.getProtoTypeFromGraphQL(arg.type) and pass protoType.typeName and
protoType.isRepeated (alongside fieldNumber and fieldName from
graphqlFieldToProtoField) into buildMessage so list-valued resolver args are
emitted correctly.
In `@protographic/src/sdl-validation-visitor.ts`:
- Around line 353-358: The validation reports "No `@connect__fieldResolver`
directive found..." even for zero-arg fields that rely on the implicit ID
fallback when only the directive's context is omitted; update the check that
emits that warning to only run when the directive is actually absent (use the
existing hasResolverDirective variable) and, if the directive exists but has an
empty/missing context, emit a different message that mentions a missing/empty
context rather than "directive not found"; locate and modify the warning
emission near where hasArgs and hasResolverDirective are computed (references:
hasArgs, hasResolverDirective, CONNECT_FIELD_RESOLVER) so the warning is gated
by !hasResolverDirective or replaced with a context-specific warning when the
directive is present but context is empty.
🪄 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: 8188a894-5648-42bf-92f3-ac607d5c3574
📒 Files selected for processing (15)
protographic/OPERATIONS_TO_PROTO.mdprotographic/README.mdprotographic/SDL_MAPPING_RULES.mdprotographic/SDL_PROTO_RULES.mdprotographic/plugin_development.mdprotographic/src/sdl-to-mapping-visitor.tsprotographic/src/sdl-to-proto-visitor.tsprotographic/src/sdl-validation-visitor.tsprotographic/tests/abstract-selection-rewriter/01-basics.test.tsprotographic/tests/abstract-selection-rewriter/02-advanced.test.tsprotographic/tests/operations/proto-text-generator.test.tsprotographic/tests/operations/request-builder.test.tsprotographic/tests/sdl-to-mapping/04-field-resolvers.test.tsprotographic/tests/sdl-to-proto/13-field-arguments.test.tsprotographic/vite.config.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protographic/tests/sdl-to-proto/13-field-arguments.test.ts (1)
966-1262: Add one lock-data regression around the new resolver path.The new snapshots validate emitted proto text, but they do not exercise the schema-evolution case where an existing object field becomes a
@connect__fieldResolver. SincecompileGraphQLToProto()already returnslockData, a round-trip with previous lock data would catch two easy-to-miss regressions: the removed field number should become reserved on the parent message, and the generatedResolve*resolver messages should stay out oflockData. Based on learnings, resolver-related messages (created bycreateResolverRequestMessageandcreateResolverResponseMessage) are special messages that should not be tracked in the proto lock file, unlike other message types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/tests/sdl-to-proto/13-field-arguments.test.ts` around lines 966 - 1262, Add a lock-data regression test: call compileGraphQLToProto twice simulating prior lockData and a new schema where a field becomes `@connect__fieldResolver`, then verify lockData reserves the removed field number on the parent message and that resolver messages created by createResolverRequestMessage and createResolverResponseMessage are not written into lockData; update the test to use the returned lockData from compileGraphQLToProto for the first run and pass it back into the second run to assert the reserved field behavior and absence of resolver-related messages in lockData.
🤖 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-proto/13-field-arguments.test.ts`:
- Around line 966-1262: Add a lock-data regression test: call
compileGraphQLToProto twice simulating prior lockData and a new schema where a
field becomes `@connect__fieldResolver`, then verify lockData reserves the removed
field number on the parent message and that resolver messages created by
createResolverRequestMessage and createResolverResponseMessage are not written
into lockData; update the test to use the returned lockData from
compileGraphQLToProto for the first run and pass it back into the second run to
assert the reserved field behavior and absence of resolver-related messages in
lockData.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622a4c7c-d73a-4405-b401-e9539fe57bc1
📒 Files selected for processing (2)
protographic/src/sdl-to-proto-visitor.tsprotographic/tests/sdl-to-proto/13-field-arguments.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protographic/tests/sdl-validation/01-basic-validation.test.ts (1)
241-262: Consider adding a test for field without arguments and without explicit context.The new test covers the case where a field without arguments has
@connect__fieldResolverwith an explicit context. For completeness, consider adding a complementary test for the fallback behavior when no context is provided:test('should fall back to ID field for field without arguments when no context is provided', () => { const sdl = ` directive `@connect__fieldResolver`(context: openfed__FieldSet!) on FIELD_DEFINITION scalar openfed__FieldSet type Query { user(id: ID!): User! } type User { id: ID! name: String! avatar: String! `@connect__fieldResolver` } `; const visitor = new SDLValidationVisitor(sdl); const result = visitor.visit(); expect(result.errors).toHaveLength(0); expect(result.warnings).toHaveLength(1); expect(result.warnings[0]).toContain( '@connect__fieldResolver directive on the field avatar has no context provided - falling back to ID field', ); });This would ensure consistent behavior between fields with and without arguments when the context fallback is triggered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protographic/tests/sdl-validation/01-basic-validation.test.ts` around lines 241 - 262, Add a complementary test to cover the fallback behavior when a field without arguments uses `@connect__fieldResolver` but no context is provided: create a test (e.g., "should fall back to ID field for field without arguments when no context is provided") in the same suite that builds an SDL where User.avatar has `@connect__fieldResolver` (no context), instantiate SDLValidationVisitor(sdl) and call visit(), then assert result.errors.length === 0 and result.warnings.length === 1 and that the warning message contains text indicating the directive on field "avatar" has no context and is falling back to the ID field (so the test references SDLValidationVisitor, the avatar field, and the `@connect__fieldResolver` directive).
🤖 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-validation/01-basic-validation.test.ts`:
- Around line 241-262: Add a complementary test to cover the fallback behavior
when a field without arguments uses `@connect__fieldResolver` but no context is
provided: create a test (e.g., "should fall back to ID field for field without
arguments when no context is provided") in the same suite that builds an SDL
where User.avatar has `@connect__fieldResolver` (no context), instantiate
SDLValidationVisitor(sdl) and call visit(), then assert result.errors.length ===
0 and result.warnings.length === 1 and that the warning message contains text
indicating the directive on field "avatar" has no context and is falling back to
the ID field (so the test references SDLValidationVisitor, the avatar field, and
the `@connect__fieldResolver` directive).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 155dd836-a6ad-41c2-bc72-3d173f266b0e
📒 Files selected for processing (2)
protographic/src/sdl-validation-visitor.tsprotographic/tests/sdl-validation/01-basic-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- protographic/src/sdl-validation-visitor.ts
devsergiy
approved these changes
Mar 11, 2026
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
3 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.
This PR enables protographic to generate field resolvers for fields without arguments. This is useful when you have expensive fields that would otherwise be always resolved by the operation to be excluded and resolved separately.
Summary by CodeRabbit
New Features
Documentation
Tests
Checklist