Fix directive wirings discarding earlier changes to the same element - #830
Open
oryan-block wants to merge 1 commit into
Open
oryan-block wants to merge 1 commit into
oryan-block wants to merge 1 commit into
Conversation
DirectiveWiringHelper built every SchemaDirectiveWiringEnvironment from the original element instead of the output of the previous wiring. When more than one wiring ran on an element (several named directives, or a named directive plus a static or factory wiring), each wiring started over from the original element and only the last one's changes, such as a new description, ended up in the schema. Pass the running output to buildEnvironment as the environment's element, the way graphql-java's SchemaGeneratorDirectiveHelper does. The directives, parent trees and env.fieldDefinition still come from the original element, as in graphql-java, so a wiring that builds its result from env.fieldDefinition instead of env.element still drops earlier changes. Since a wiring's result is now handed to the next one, also reject a null result with graphql-java's message naming the element, instead of failing later with an unrelated assertion. Fixes #738 Co-Authored-By: Claude Opus 5.5 (1M context) <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 #738
Checklist
Description
A directive wiring that changes the element it's given, like the
@authwiring in #738 that appends the required scopes to the field description, only sticks if it's the last wiring to run on that element.DirectiveWiringHelper#wireDirectivesbuilt everySchemaDirectiveWiringEnvironmentfrom the original element (wrapper.graphQlType) instead of the previous wiring's output. So each named, static and factory wiring started over from the original, and whatever the last one returned ended up in the schema. In the issue, the named@authwiring changed the description and then a static wiring returnedenv.element, which was still the original field. Two named directives on the same field have the same problem: only the second one's change survives.buildEnvironmentnow takes the running output and uses it as the environment's element, which is what graphql-java 26.1'sSchemaGeneratorDirectiveHelper#wireDirectivesdoes. The directives, applied directives, parent trees and params still come from the original element, same as graphql-java.Since a wiring's result is now handed to the next one, a wiring that returns null (which
SchemaDirectiveWiringdoesn't allow) would fail later withAssertException: fieldDefinition can't be null, which doesn't say which wiring or field it was. So each result is now checked, with graphql-java's own message:The SchemaDirectiveWiring MUST return a non null return value for element 'name'.env.fieldDefinitionstill comes from the original element, as it does in graphql-java. So a wiring that builds its result fromenv.fieldDefinitioninstead ofenv.elementstill drops earlier changes. graphql-java-extended-validation'sValidationSchemaWiringdoes this for example, so registering it as a static wiring after a wiring like the one in the issue still loses the description. Changing that would mean diverging from graphql-java, so I left it. The reporter's wiring readsenv.fieldDefinitiontoo, but that's fine since it runs first. The wiring factory path has no test becauseSchemaParserBuilderhas no way to set one.Behaviour change: when several wirings run on the same element, each one now gets the previous wiring's output as
env.elementinstead of the original, so changes made by earlier wirings (description, arguments, type, etc.) are kept. Setups with one wiring per element, or wirings that only touch data fetchers through the code registry, behave as before. A wiring that relies onenv.elementbeing the untouched original will now see the changed element. A wiring that returns null now fails right away with anIllegalStateExceptionnaming the element. Before, it failed later with theAssertExceptionabove, or went unnoticed if a static wiring ran after it.🤖 Generated with Claude Code