Consolidate built-in directive handling and move helpers to Directives class - #4229
Merged
andimarek merged 5 commits intoJan 28, 2026
Merged
Consolidate built-in directive handling and move helpers to Directives class#4229andimarek merged 5 commits into
andimarek merged 5 commits into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
…y in SchemaPrinter Remove GraphQLSchema.Builder.clearDirectives() as it allows removing spec-required built-in directives, which arguably violates the spec. All 7 built-in directives are now always present. Change SchemaPrinter to synthesize @specifiedBy from the first-class specifiedByUrl property on GraphQLScalarType, matching how @deprecated already uses the first-class deprecationReason property. This ensures both directives rely on their first-class properties as the source of truth rather than applied directives. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor
Test Results 327 files ±0 327 suites ±0 5m 8s ⏱️ ±0s Results for commit f361f4f. ± Comparison against base commit 9f18071. This pull request removes 197 and adds 174 tests. Note that renamed tests count towards both. ?
, expected: combo-\"\\\b\f\n\r\t, #4]
__schema { types { fields { args { type { name fields { name }}}}}}
__schema { types { fields { type { name fields { name }}}}}
__schema { types { inputFields { type { inputFields { name }}}}}
__schema { types { interfaces { fields { type { interfaces { name } } } } } }
__schema { types { name} }
__type(name : "t") { name }
a1: __schema { types { name} }
a1: __type(name : "t") { name }
…
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@37c41ec0 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0] graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@4fe64d23 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1] graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@58437801 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2] graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@6c6017b9 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0] graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@4730e0f0 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1] graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@506a1372 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2] graphql.AssertTest ‑ assertNotNull with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure3@235d29d6 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0] graphql.AssertTest ‑ assertNotNull with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@22ebccb9 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1] graphql.AssertTest ‑ assertNotNull with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@35a60674 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2] graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@4784efd9 delegate=graphql.AssertTest@1d12e953 owner=graphql.AssertTest@1d12e953 thisObject=graphql.AssertTest@1d12e953 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0] … ♻️ This comment has been updated with latest results. |
Sorry, something went wrong.
…s class All 7 built-in directives are now handled uniformly in buildImpl() via ensureBuiltInDirectives(), fixing the inconsistency where @include/@Skip were pre-populated in the Builder field initializer while the other 5 were force-added in buildImpl(). This eliminates a potential duplicate directive bug when SchemaTransformer replaces a built-in directive. Add BUILT_IN_DIRECTIVES set, BUILT_IN_DIRECTIVES_MAP, and isBuiltInDirective() methods to the Directives class. Deprecate DirectiveInfo in favor of these new Directives methods. Update all internal callers to use Directives directly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
DirectiveInfo is fully replaced by methods in Directives: BUILT_IN_DIRECTIVES, BUILT_IN_DIRECTIVES_MAP, isBuiltInDirective(). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Instead of creating a synthetic applied directive object just to feed it into the directive printing pipeline, print @specifiedBy directly from the first-class specifiedByUrl property as a simple string. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Verifies that a built-in directive (e.g. @deprecated) can be modified by adding a new argument through SchemaTransformer, and that all other built-in directives remain unchanged after the transformation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
rstata
pushed a commit
to rstata/graphql-java
that referenced
this pull request
Mar 2, 2026
PR graphql-java#4229 removed `clearDirectives()` from `GraphQLSchema.Builder` as part of unifying built-in directive handling. However, as noted in issue graphql-java#4259, this method is useful when using `GraphQLSchema.transform` to replace all non-built-in directives: the typical pattern is to collect the non-built-in directives from the source schema, call `clearDirectives()` on the builder, then add back transformed versions of those directives. The new implementation clears `additionalDirectives` entirely. Built-in directives are always re-added automatically at build time by `ensureBuiltInDirectives()`, so they cannot be permanently removed through this method. This treats all built-ins uniformly, consistent with the approach introduced in graphql-java#4229. Co-Authored-By: Claude Opus 4.6 <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
1 participant
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.
Breaking Changes
GraphQLSchema.Builder.clearDirectives() removed — This method allowed removing spec-required built-in directives. Callers should remove any .clearDirectives() calls; all 7 built-in directives are now always present.
DirectiveInfo class removed — Replace usages as follows:
Directive ordering changed — Built-in directives now consistently appear first in schema.getDirectives() and introspection results, followed by user-defined directives. Previously the order was inconsistent depending on how the schema was built.
Summary
Consolidate built-in directive handling — All 7 built-in directives are now handled uniformly in buildImpl() via ensureBuiltInDirectives(). Previously @include/@skip were pre-populated in the Builder field initializer while the other 5 were force-added in buildImpl(), which could cause duplicate directives when SchemaTransformer replaced a built-in directive with a new object (identity-based LinkedHashSet vs name-based dedup).
Move directive helpers to Directives class — Added BUILT_IN_DIRECTIVES (ordered set), BUILT_IN_DIRECTIVES_MAP, and isBuiltInDirective() methods to Directives. Removed DirectiveInfo. Updated all internal callers.
Fix SchemaPrinter @specifiedBy handling — @specifiedBy is now printed directly from the first-class specifiedByUrl property on GraphQLScalarType, matching how @deprecated uses deprecationReason. Previously @specifiedBy relied on applied directives while @deprecated was synthesized from its first-class property.
Add SchemaTransformer test for built-in directive modification — New test verifies that a built-in directive (e.g. @deprecated) can be modified via SchemaTransformer (adding a new argument), and that all other built-in directives remain unchanged after the transformation.
Test plan
🤖 Generated with Claude Code