Skip to content

Fix ShallowTypeRefCollector: resolve type refs in applied directive arguments#4288

Merged
andimarek merged 5 commits intographql-java:masterfrom
rstata:fix-fast
Mar 9, 2026
Merged

Fix ShallowTypeRefCollector: resolve type refs in applied directive arguments#4288
andimarek merged 5 commits intographql-java:masterfrom
rstata:fix-fast

Conversation

@rstata
Copy link
Copy Markdown

@rstata rstata commented Mar 3, 2026

Summary

ShallowTypeRefCollector (introduced with FastBuilder in #4196) performs
shallow scans of types and directives to collect GraphQLTypeReference objects
for later replacement. In three places it called scanArgumentType(arg) on a
GraphQLArgument but never called scanAppliedDirectives(arg.getAppliedDirectives()).
It also had no handling to descend into GraphQLEnumValueDefinition objects to
scan their applied directives.

The result: when FastBuilder is used with programmatically-constructed types
carrying GraphQLTypeReference objects in applied directive argument types on
field arguments, enum values, or directive definition arguments, those references
are never resolved. The AppliedDirectiveArgumentsAreValid validator then sees a
GraphQLTypeReference where it expects a real input type and throws:

InvalidSchemaException: Invalid argument 'x' for applied directive of name 'y'

(This does not manifest when types are built via FastSchemaGenerator + SDL,
because SchemaGeneratorHelper.buildInputType resolves types at construction
time. It manifests when FastBuilder is used directly with programmatically-built
types.)

Changes

Three locations in ShallowTypeRefCollector fixed:

  • handleObjectType / handleInterfaceType — added
    scanAppliedDirectives(arg.getAppliedDirectives()) inside the field-argument
    loop of both methods, alongside the existing scanArgumentType(arg) call.

  • handleTypeDef — added handleEnumType() which iterates over
    GraphQLEnumValueDefinition values and scans their applied directives. (Applied
    directives on the enum type itself were already covered by the existing
    GraphQLDirectiveContainer branch.)

  • handleDirective — added scanAppliedDirectives(argument.getAppliedDirectives())
    inside the directive-argument loop alongside the existing scanArgumentType call.

Adds four regression tests in FastBuilderTest covering all three fix locations:
applied directive on object type field argument, interface field argument, enum
value, and directive definition argument.

Related

Bug in the FastBuilder / ShallowTypeRefCollector introduced in #4196.

…ontexts

ShallowTypeRefCollector called scanArgumentType(arg) for GraphQLArgument objects
in several places but never scanAppliedDirectives(arg.getAppliedDirectives()),
and never descended into enum values to scan their applied directives. This left
GraphQLTypeReference objects in applied directive argument types unresolved,
causing the AppliedDirectiveArgumentsAreValid validator to spuriously fail with
"Invalid argument 'x' for applied directive of name 'y'".

Three locations fixed:

1. handleObjectType / handleInterfaceType — field argument applied directives
   were not scanned. Added scanAppliedDirectives(arg.getAppliedDirectives())
   inside the field-argument loop of both methods.

2. handleTypeDef — already scanned applied directives on the enum type itself
   (via the GraphQLDirectiveContainer branch), but never descended into enum
   values. Added handleEnumType() which scans applied directives on each
   GraphQLEnumValueDefinition.

3. handleDirective — directive definition argument applied directives were not
   scanned. Added scanAppliedDirectives(argument.getAppliedDirectives()) inside
   the argument loop.

SchemaUtil.replaceTypeReferences (used by the standard Builder) does a full
deep traversal via getChildrenWithTypeReferences() and handles all of these
correctly; this change brings ShallowTypeRefCollector in line with that behavior.

Adds four regression tests in FastBuilderTest covering:
- Applied directive on object type field argument
- Applied directive on interface field argument
- Applied directive on enum value
- Applied directive on directive definition argument

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rstata rstata marked this pull request as draft March 3, 2026 11:52
@rstata
Copy link
Copy Markdown
Author

rstata commented Mar 3, 2026

(more internal testing has failed -- stand by...)

@rstata rstata marked this pull request as ready for review March 4, 2026 01:45
@andimarek
Copy link
Copy Markdown
Member

@rstata is that ready or are we waiting for more?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2026

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 5675 (+4 🟢) 5618 (+4 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 17 5675 (+4 🟢) 5617 (+4 🟢) 0 (±0) 0 (±0) 58 (±0)
Java 21 5675 (+4 🟢) 5617 (+4 🟢) 0 (±0) 0 (±0) 58 (±0)
Java 25 5675 (+4 🟢) 5617 (+4 🟢) 0 (±0) 0 (±0) 58 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 22732 (+16 🟢) 22501 (+16 🟢) 0 (±0) 0 (±0) 231 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 28707 3126 90.2% ±0.0%
Branches 8335 1511 84.7% ±0.0%
Methods 7682 1224 86.3% ±0.0%

Changed Class Coverage (1 class)

Class Line Branch Method
g.s.ShallowTypeRefCollector +0.2% 🟢 +0.4% 🟢 ±0.0%

Full HTML report: build artifact jacoco-html-report

Updated: 2026-03-06 22:51:00 UTC

@rstata
Copy link
Copy Markdown
Author

rstata commented Mar 9, 2026

oops -- i forgot about this! turns out this version did fix all the problems, viaduct builds and passes all its tests with this fix in place.

@andimarek andimarek merged commit cf536bd into graphql-java:master Mar 9, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants