Consolidate built-in directive handling and move helpers to Directives class#4229
Merged
Consolidate built-in directive handling and move helpers to Directives class#4229
Conversation
…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]>
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.♻️ This comment has been updated with latest results. |
…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
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]>
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.DirectiveInfoclass 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_MAPDirective 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()viaensureBuiltInDirectives(). Previously@include/@skipwere pre-populated in the Builder field initializer while the other 5 were force-added inbuildImpl(), which could cause duplicate directives whenSchemaTransformerreplaced a built-in directive with a new object (identity-basedLinkedHashSetvs name-based dedup).Move directive helpers to
Directivesclass — AddedBUILT_IN_DIRECTIVES(ordered set),BUILT_IN_DIRECTIVES_MAP, andisBuiltInDirective()methods toDirectives. RemovedDirectiveInfo. Updated all internal callers.Fix SchemaPrinter
@specifiedByhandling —@specifiedByis now printed directly from the first-classspecifiedByUrlproperty onGraphQLScalarType, matching how@deprecatedusesdeprecationReason. Previously@specifiedByrelied on applied directives while@deprecatedwas 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 viaSchemaTransformer(adding a new argument), and that all other built-in directives remain unchanged after the transformation.Test plan
"clear directives works as expected"test with"all built-in directives are always present"IntrospectionWithDirectivesSupportTest🤖 Generated with Claude Code