Repository navigation
Resolve nested input objects and input field directives before directive wiring - #843
Merged
Merged
Conversation
An input object that another input object only referenced through a
list or non-null wrapper, like `subs: [Sub!]` or `other: Sub2!`, was
still a GraphQLTypeReference when the directive wirings ran, unless
the SDL happened to declare it first. determineInputType handed list
and non-null types to determineType, the output path, which never
builds input objects and falls back to a type reference. Wirings that
walk nested input types, like graphql-java-extended-validation's
DirectivesAndTypeWalker, then failed on GraphQLTypeUtil.unwrapAll with
a ClassCastException or skipped the nested directives. Those branches
now recurse into determineInputType, so the wrapped type gets built on
the spot. A field only stays a type reference where it points back at
an input object that is still being built, i.e. a cycle.
That changes which field of a cycle stays a reference. Types are now
built depth-first through the wrappers, so it's the field pointing
back. Before, it was the list or non-null field, so in
`input A { b: B! } input B { a: A }` the wiring of B.a used to see A
and now sees a type reference, while A.b is now resolved. Unlike
graphql-java, which wires directives on the finished schema, wirings
here still see that reference, so DirectivesAndTypeWalker still fails
on recursive or cyclic input types, as it did before.
Directive definition arguments go through the same path. Building an
input object needs the directive definitions for its own applied
directives, and the definitions weren't assigned yet, so a directive
argument whose input object carried any directive failed with an
UninitializedPropertyAccessException. Nullable arguments already
failed this way, and the change above would have made list and
non-null arguments fail too. There's now a first set of definitions
that only refers to input objects by type reference, and each one is
replaced by the real definition the first time it's looked up. So an
input object built for a directive argument sees the real definitions
of the directives applied to it, and their input object arguments
aren't type references at wiring time. Only a directive applied
within its own argument types sees its first definition there.
Input fields also took their legacy directives from the parent input
object's definition instead of their own (a regression from 2a33e6b,
which fixed this for arguments only). So inputField.getDirective()
and env.directive inside onInputObjectField were null while the
applied directive was set, and every input field carried the input
object's own directives. The wiring of input fields and enum values
now also records INPUT_FIELD_DEFINITION and ENUM_VALUE as their
location instead of FIELD_DEFINITION.
Fixes #321, fixes #385, fixes #731.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 12, 2026
schemaDirectives is now a map by name, so finishing a directive replaces its entry instead of rebuilding the whole set, and findDirective is a plain lookup. The comments say in plainer words why directives are built twice. The WiringWrapper location change for input fields and enum values is reverted. directiveLocation isn't read anywhere, so it changed nothing. Tests: the #731 case is now an onInputObjectField override in the existing EmailDirective test. The two SchemaParserTest cases are one test with a list, a non-null and a back-pointing field. The two tests for input objects used by directive arguments are one, which records argument types at wiring time, in both declaration orders. Dropped the query-executing validation test: the type and legacy directive checks already cover what it walked, and the probe runs the issue's schema through extended-validation itself. None of the tests call deprecated API anymore, so the build passes with -Werror. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SchemaDirectives holds the directives by name, the ones not rebuilt with their input objects yet, and the input objects being built for them. find() rebuilds a directive the first time it's looked up. This replaces the finishDirective function that createDirectives had to set and reset, and a new object per parse means nothing carries over between parses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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
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.



Fixes #321
Fixes #385
Fixes #731
Checklist
Description
If an input object is only referenced from another input object through a list or non-null wrapper, it's still a
GraphQLTypeReferencewhile theSchemaDirectiveWirings run, unless the SDL happens to declare it first. Here's PhilippS93's schema from #385 with the nested input wrapped:Anything that walks argument types from
onFieldand callsGraphQLTypeUtil.unwrapAllon those fields getsClassCastException: GraphQLTypeReference cannot be cast to GraphQLUnmodifiedType. That includes the samples'RangeDirectiveand graphql-java-extended-validation'sDirectivesAndTypeWalker(#321). With extended-validation it happens at schema build. Then there's #731: insideonInputObjectField, the legacyenv.directiveandfield.getDirective("range")are null on the@rangeinput fields whileenv.appliedDirectiveis set. So a walker that reads the legacy directive off input fields skips them even when the type is resolved.SchemaParser.determineInputTypepassedListTypeandNonNullTypetodetermineType, which is the output path. That function never builds input objects. It only looks through the ones already built and otherwise returns a type reference. #731 is a separate bug:createInputObjectbuilt each input field's legacy directives fromdefinition.directives(the input object's) instead offieldDefinition.directives. It's a regression from 2a33e6b (#708), which fixed the same thing for arguments only.Now both branches call
determineInputTypeagain, so the wrapped input object gets built right there and wirings see aGraphQLInputObjectTypewhatever the SDL order. A field only stays a reference where it points back at an input object that's still being built, i.e. a cycle. Input fields now get their legacy directives from their own definition.Directive definition arguments take the same path, and they needed one more change. To build an input object we need the definitions of the directives applied to it, but
schemaDirectives(a lateinit) isn't assigned yet whilecreateDirectivesbuilds the arguments. So on master,directive @meta(info: MetaInput)already failed withUninitializedPropertyAccessExceptiononceMetaInputhad any applied directive, even@deprecated. The change above would have brokeninfo: MetaInput!and[MetaInput!]the same way. NowcreateDirectivesfirst builds every directive with input objects only as type references.SchemaDirectives.findthen rebuilds each one with the real input objects the first time it's looked up. So input objects built for directive arguments see the real definitions of the directives applied to them. Only a directive applied inside its own argument types still sees its first build there. Most of thecreateDirectivesdiff is the existing code moving intocreateDirective.SchemaParserTesthas a new case that records the unwrapped type of each input field inonInputObjectField.subs: [SubFilter!]andother: OtherFilter!are declared after their parent, andSubFilter.parentpoints back at it.subsandotherare resolved andparentstays a reference. On master it's the other way round. For #731, the existingEmailDirectivetest now also records the input field's message inonInputObjectField, in both the applied and the legacy view. On master the legacy directive there is null. TwoDirectiveTestcases cover input objects used by directive arguments, each in both declaration orders. The first checks that the argument types areGraphQLInputObjectTypeat wiring time, for@meta(info: ConfigInput!)and for the@optionapplied insideConfigInput. It fails on master. The second applies@limit(bound: BoundInput!)insideBoundInputand checks thatBoundInputis only wired once. It passes on master too. It's there for the new lookup: with a freshreferencingInputObjectsper directive it fails with graphql-java'sAssertException. None of the tests call deprecated API, so they build with-Werror.Outside the tests, PhilippS93's schema went through the real extended-validation 24.0
ValidationSchemaWiringon graphql-java 26.1, using the installed jar. Master throws theClassCastExceptionabove at schema build. This branch builds, and an invalid mutation gets all three nested errors (/changeUser/input/name/forename,/changeUser/input/aliases[1]/forenameand/changeUser/input/guardian/name/forename size must be between 3 and 25). A valid one gets none.I didn't fix cycles. Types are immutable, so one field of a cycle has to stay a reference. graphql-java wires directives on the finished schema, so its wirings never see that reference, but ours do. That means
DirectivesAndTypeWalkerstill throws on a recursive input likeinput Tree { and: [Tree!] }, same as on master. Fixing it means holding off all directive wiring until the schema is assembled, which also touches objects, interfaces, unions and enums. That should be a separate issue. The samples repo needs a follow-up too before #385 has a working example. ItsRangeDirectiveonly goes one level deep. Against current graphql-java it also fails withFloatValue cannot be cast to Double, because it callsgetArgumentValue().getValue()on a literal. TheAn output field must be in contexterror PhilippS93 got from callinggetFieldDataFetcher()inonInputObjectFieldis how graphql-java's API behaves, so there's nothing to fix here.#434 (closed) had the same symptom for a top-level non-null argument and was fixed back then. Its test still passes. dnagarajan89's
BookInput!case at the end of #385 is the same case. What still broke for them is that the samples'RangeDirectivereads the legacygetDirectiveoff input fields, and that's #731.Behaviour change: input objects behind a list or non-null wrapper (input fields,
extend inputfields, directive arguments) are nowGraphQLInputObjectTypeat wiring time. They're also built and wired inside their parent instead of later in SDL order, so a wiring that depends on call order could notice. In a cycle, a different field now stays a reference. Ininput A { b: B! } input B { a: A }, the wiring ofB.aused to seeAand now sees a type reference, whileA.bis now resolved.inputField.getDirective(...),inputField.directivesandenv.directiveinonInputObjectFieldnow return only the field's own directives, so code that read the input object's directives off its fields will get null. Input fields with@deprecatednow have it in the legacy list too, as arguments already did. Schemas where a directive argument's input object has directives now build instead of throwing. Because of these changes this should go in a minor release.🤖 Generated with Claude Code