diff --git a/src/main/java/graphql/schema/GraphQLSchema.java b/src/main/java/graphql/schema/GraphQLSchema.java index 1cc2f341d..1ba9ed962 100644 --- a/src/main/java/graphql/schema/GraphQLSchema.java +++ b/src/main/java/graphql/schema/GraphQLSchema.java @@ -96,7 +96,7 @@ private GraphQLSchema(Builder builder) { this.introspectionSchemaType = builder.introspectionSchemaType; this.introspectionSchemaField = Introspection.buildSchemaField(builder.introspectionSchemaType); this.introspectionTypeField = Introspection.buildTypeField(builder.introspectionSchemaType); - this.directiveDefinitionsHolder = new DirectivesUtil.DirectivesHolder(builder.additionalDirectives, emptyList()); + this.directiveDefinitionsHolder = new DirectivesUtil.DirectivesHolder(builder.additionalDirectives.values(), emptyList()); this.schemaAppliedDirectivesHolder = new DirectivesUtil.DirectivesHolder(builder.schemaDirectives, builder.schemaAppliedDirectives); this.definition = builder.definition; this.extensionDefinitions = nonNullCopyOf(builder.extensionDefinitions); @@ -763,7 +763,7 @@ public static Builder newSchema(GraphQLSchema existingSchema) { .introspectionSchemaType(existingSchema.getIntrospectionSchemaType()) .codeRegistry(existingSchema.getCodeRegistry()) .clearAdditionalTypes() - .additionalDirectives(new LinkedHashSet<>(existingSchema.getDirectives())) + .additionalDirectives(existingSchema.getDirectives()) .clearSchemaDirectives() .withSchemaDirectives(schemaDirectivesArray(existingSchema)) .withSchemaAppliedDirectives(schemaAppliedDirectivesArray(existingSchema)) @@ -813,7 +813,7 @@ public static class Builder { private List extensionDefinitions; private String description; - private final Set additionalDirectives = new LinkedHashSet<>(); + private final Map additionalDirectives = new LinkedHashMap<>(); private final Set additionalTypes = new LinkedHashSet<>(); private final List schemaDirectives = new ArrayList<>(); private final List schemaAppliedDirectives = new ArrayList<>(); @@ -921,19 +921,49 @@ public Builder clearAdditionalTypes() { return this; } + /** + * Adds multiple directive definitions to the schema. + * + * @param additionalDirectives the directive definitions to add + * + * @return this builder + * + * @deprecated use {@link #additionalDirectives(Collection)} instead + */ + @Deprecated(since = "2026-05-20") public Builder additionalDirectives(Set additionalDirectives) { - this.additionalDirectives.addAll(additionalDirectives); + return additionalDirectives((Collection) additionalDirectives); + } + + /** + * Adds multiple directive definitions to the schema. + * + * @param additionalDirectives the directive definitions to add + * + * @return this builder + */ + public Builder additionalDirectives(Collection additionalDirectives) { + for (GraphQLDirective additionalDirective : additionalDirectives) { + additionalDirective(additionalDirective); + } return this; } public Builder additionalDirective(GraphQLDirective additionalDirective) { - this.additionalDirectives.add(additionalDirective); + String name = additionalDirective.getName(); + GraphQLDirective existing = additionalDirectives.get(name); + if (existing != null && existing != additionalDirective) { + throw new AssertException(String.format("Directive '%s' already exists with a different instance", name)); + } + if (existing == null) { + additionalDirectives.put(name, additionalDirective); + } return this; } /** * Clears all directives from this builder, including any that were previously added - * via {@link #additionalDirective(GraphQLDirective)} or {@link #additionalDirectives(Set)}. + * via {@link #additionalDirective(GraphQLDirective)} or {@link #additionalDirectives(Collection)}. * Built-in directives ({@code @include}, {@code @skip}, {@code @deprecated}, etc.) will * always be added back automatically at build time by {@code ensureBuiltInDirectives()}. *

@@ -1073,19 +1103,15 @@ private GraphQLSchema buildImpl() { private void ensureBuiltInDirectives() { // put built-in directives first, preserving user-supplied overrides by name - Set userDirectiveNames = new LinkedHashSet<>(); - for (GraphQLDirective d : additionalDirectives) { - userDirectiveNames.add(d.getName()); - } - LinkedHashSet ordered = new LinkedHashSet<>(); + Map ordered = new LinkedHashMap<>(); for (GraphQLDirective builtIn : Directives.BUILT_IN_DIRECTIVES) { - if (!userDirectiveNames.contains(builtIn.getName())) { - ordered.add(builtIn); + if (!additionalDirectives.containsKey(builtIn.getName())) { + ordered.put(builtIn.getName(), builtIn); } } - ordered.addAll(additionalDirectives); + ordered.putAll(additionalDirectives); additionalDirectives.clear(); - additionalDirectives.addAll(ordered); + additionalDirectives.putAll(ordered); } private GraphQLSchema validateSchema(GraphQLSchema graphQLSchema) { diff --git a/src/main/java/graphql/schema/SchemaTransformer.java b/src/main/java/graphql/schema/SchemaTransformer.java index 96761b7d1..d0b98912b 100644 --- a/src/main/java/graphql/schema/SchemaTransformer.java +++ b/src/main/java/graphql/schema/SchemaTransformer.java @@ -735,7 +735,7 @@ public GraphQLSchema rebuildSchema(GraphQLCodeRegistry.Builder codeRegistry, Set .mutation(this.mutation) .subscription(this.subscription) .additionalTypes(this.additionalTypes) - .additionalDirectives(this.directives) + .additionalDirectives(new ArrayList<>(this.directives)) .introspectionSchemaType(this.introspectionSchemaType) .withSchemaDirectives(this.schemaDirectives) .withSchemaAppliedDirectives(this.schemaAppliedDirectives) diff --git a/src/main/java/graphql/schema/idl/SchemaGenerator.java b/src/main/java/graphql/schema/idl/SchemaGenerator.java index 050458dc6..d0211989b 100644 --- a/src/main/java/graphql/schema/idl/SchemaGenerator.java +++ b/src/main/java/graphql/schema/idl/SchemaGenerator.java @@ -11,6 +11,7 @@ import graphql.schema.GraphQLType; import graphql.schema.idl.errors.SchemaProblem; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Set; @@ -128,7 +129,7 @@ private GraphQLSchema makeExecutableSchemaImpl(ImmutableTypeDefinitionRegistry t GraphQLSchema.Builder schemaBuilder = GraphQLSchema.newSchema(); - Set additionalDirectives = schemaGeneratorHelper.buildAdditionalDirectiveDefinitions(buildCtx); + Collection additionalDirectives = schemaGeneratorHelper.buildAdditionalDirectiveDefinitions(buildCtx); schemaBuilder.additionalDirectives(additionalDirectives); schemaGeneratorHelper.buildSchemaDirectivesAndExtensions(buildCtx, schemaBuilder); diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index f3cafb9ab..e04e8e2a6 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -187,6 +187,84 @@ class GraphQLSchemaTest extends Specification { schema.getDirective("custom") != null } + def "duplicate directive with different instance throws error"() { + given: + def directive1 = directive("duplicate", DirectiveLocation.FIELD) + def directive2 = directive("duplicate", DirectiveLocation.OBJECT) + + when: + basicSchemaBuilder() + .additionalDirective(directive1) + .additionalDirective(directive2) + + then: + def exception = thrown(AssertException) + exception.message == "Directive 'duplicate' already exists with a different instance" + } + + def "same directive instance can be added multiple times"() { + given: + def directive = directive("myDir", DirectiveLocation.FIELD) + + when: + def schema = basicSchemaBuilder() + .additionalDirective(directive) + .additionalDirective(directive) + .build() + + then: + schema.getDirective("myDir") == directive + schema.directives.findAll { it.name == "myDir" }.size() == 1 + } + + def "additionalDirectives accepts collection"() { + given: + def directive1 = directive("dir1", DirectiveLocation.FIELD) + def directive2 = directive("dir2", DirectiveLocation.OBJECT) + + when: + def schema = basicSchemaBuilder() + .additionalDirectives([directive1, directive2]) + .build() + + then: + schema.getDirective("dir1") == directive1 + schema.getDirective("dir2") == directive2 + } + + @SuppressWarnings("deprecation") + def "additionalDirectives accepts deprecated set overload"() { + given: + def directive1 = directive("setDir1", DirectiveLocation.FIELD) + def directive2 = directive("setDir2", DirectiveLocation.OBJECT) + Set directives = new LinkedHashSet<>() + directives.add(directive1) + directives.add(directive2) + + when: + def schema = basicSchemaBuilder() + .additionalDirectives(directives) + .build() + + then: + schema.getDirective("setDir1") == directive1 + schema.getDirective("setDir2") == directive2 + } + + def "additionalDirectives rejects duplicate directive names in collection"() { + given: + def directive1 = directive("duplicate", DirectiveLocation.FIELD) + def directive2 = directive("duplicate", DirectiveLocation.OBJECT) + + when: + basicSchemaBuilder() + .additionalDirectives([directive1, directive2]) + + then: + def exception = thrown(AssertException) + exception.message == "Directive 'duplicate' already exists with a different instance" + } + def "clearDirectives supports replacing non-built-in directives in a schema transform"() { given: "a schema with a custom directive" def originalDirective = GraphQLDirective.newDirective() @@ -209,7 +287,7 @@ class GraphQLSchemaTest extends Specification { def nonBuiltIns = schema.getDirectives().findAll { !Directives.isBuiltInDirective(it) } .collect { it.getName() == "custom" ? replacementDirective : it } builder.clearDirectives() - .additionalDirectives(new LinkedHashSet<>(nonBuiltIns)) + .additionalDirectives(nonBuiltIns) }) then: "all 7 built-in directives are still present" @@ -248,6 +326,13 @@ class GraphQLSchemaTest extends Specification { schema.getDirective("skip").description == "custom skip description" } + private static GraphQLDirective directive(String name, DirectiveLocation directiveLocation) { + GraphQLDirective.newDirective() + .name(name) + .validLocations(directiveLocation) + .build() + } + def "clear additional types works as expected"() { setup: def schemaBuilder = basicSchemaBuilder() diff --git a/src/test/groovy/graphql/schema/validation/NoDirectiveRedefinitionTest.groovy b/src/test/groovy/graphql/schema/validation/NoDirectiveRedefinitionTest.groovy new file mode 100644 index 000000000..e1fe34b69 --- /dev/null +++ b/src/test/groovy/graphql/schema/validation/NoDirectiveRedefinitionTest.groovy @@ -0,0 +1,59 @@ +package graphql.schema.validation + +import graphql.AssertException +import graphql.TestUtil +import graphql.schema.GraphQLDirective +import spock.lang.Specification + +import static graphql.Scalars.GraphQLString +import static graphql.introspection.Introspection.DirectiveLocation.FIELD_DEFINITION +import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition +import static graphql.schema.GraphQLObjectType.newObject +import static graphql.schema.GraphQLSchema.newSchema + +class NoDirectiveRedefinitionTest extends Specification { + + def "directive cannot be redefined in SDL schema"() { + given: + def sdl = ''' + directive @exampleDirective on FIELD_DEFINITION + directive @exampleDirective on FIELD_DEFINITION + + type Query { + hello: String @exampleDirective + } + ''' + + when: + TestUtil.schema(sdl) + + then: + def schemaProblem = thrown(AssertionError) + schemaProblem.message.contains("tried to redefine existing directive 'exampleDirective'") + } + + def "programmatically redefined directive is rejected"() { + when: + newSchema() + .query(newObject() + .name("Query") + .field(newFieldDefinition() + .name("hello") + .type(GraphQLString)) + .build()) + .additionalDirective(exampleDirective()) + .additionalDirective(exampleDirective()) + .build() + + then: + def exception = thrown(AssertException) + exception.message == "Directive 'exampleDirective' already exists with a different instance" + } + + private static GraphQLDirective exampleDirective() { + GraphQLDirective.newDirective() + .name("exampleDirective") + .validLocation(FIELD_DEFINITION) + .build() + } +}