GeezerVison is showing github.com — Consolidate built-in directive handling and move helpers to Directives class by andimarek · Pull Request #4229 · graphql-java/graphql-java
Conversation
andimarek
commented
Jan 28, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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:
DirectiveInfo.isGraphqlSpecifiedDirective(name) → Directives.isBuiltInDirective(name)DirectiveInfo.isGraphqlSpecifiedDirective(directive) → Directives.isBuiltInDirective(directive)DirectiveInfo.GRAPHQL_SPECIFICATION_DIRECTIVES → Directives.BUILT_IN_DIRECTIVESDirectiveInfo.GRAPHQL_SPECIFICATION_DIRECTIVE_MAP → Directives.BUILT_IN_DIRECTIVES_MAP
-
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
- Replaced
"clear directives works as expected" test with "all built-in directives are always present"
- Updated directive ordering expectations in
IntrospectionWithDirectivesSupportTest
- Added SchemaTransformer test for modifying a built-in directive
- Full test suite passes (5337 tests, 0 failures)
🤖 Generated with Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…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 <[email protected]>
Test Results
327 files ±0 327 suites ±0 5m 8s ⏱️ ±0s
5 250 tests +1 5 242 ✅ +2 8 💤 - 1 0 ❌ ±0
5 339 runs +1 5 331 ✅ +2 8 💤 - 1 0 ❌ ±0
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.
Uh oh!
There was an error while loading. Please reload this page.
…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 <[email protected]>
DirectiveInfo is fully replaced by methods in Directives:
BUILT_IN_DIRECTIVES, BUILT_IN_DIRECTIVES_MAP, isBuiltInDirective().
Co-Authored-By: Claude Opus 4.5 <[email protected]>
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 <[email protected]>
andimarek
changed the title
Remove clearDirectives and fix @specifiedBy in SchemaPrinter
Consolidate built-in directive handling and move helpers to Directives class
Jan 28, 2026
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 <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
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 <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
