Skip to content

Commit 26648a3

Browse files
andimarekandimarek
authored
Reject duplicate additional directive definitions (#4394)
* Reject duplicate additional directive definitions * Cover deprecated directive set overload
1 parent fd0fcb9 commit 26648a3

5 files changed

Lines changed: 189 additions & 18 deletions

File tree

‎src/main/java/graphql/schema/GraphQLSchema.java‎

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ private GraphQLSchema(Builder builder) {
9696
this.introspectionSchemaType = builder.introspectionSchemaType;
9797
this.introspectionSchemaField = Introspection.buildSchemaField(builder.introspectionSchemaType);
9898
this.introspectionTypeField = Introspection.buildTypeField(builder.introspectionSchemaType);
99-
this.directiveDefinitionsHolder = new DirectivesUtil.DirectivesHolder(builder.additionalDirectives, emptyList());
99+
this.directiveDefinitionsHolder = new DirectivesUtil.DirectivesHolder(builder.additionalDirectives.values(), emptyList());
100100
this.schemaAppliedDirectivesHolder = new DirectivesUtil.DirectivesHolder(builder.schemaDirectives, builder.schemaAppliedDirectives);
101101
this.definition = builder.definition;
102102
this.extensionDefinitions = nonNullCopyOf(builder.extensionDefinitions);
@@ -763,7 +763,7 @@ public static Builder newSchema(GraphQLSchema existingSchema) {
763763
.introspectionSchemaType(existingSchema.getIntrospectionSchemaType())
764764
.codeRegistry(existingSchema.getCodeRegistry())
765765
.clearAdditionalTypes()
766-
.additionalDirectives(new LinkedHashSet<>(existingSchema.getDirectives()))
766+
.additionalDirectives(existingSchema.getDirectives())
767767
.clearSchemaDirectives()
768768
.withSchemaDirectives(schemaDirectivesArray(existingSchema))
769769
.withSchemaAppliedDirectives(schemaAppliedDirectivesArray(existingSchema))
@@ -813,7 +813,7 @@ public static class Builder {
813813
private List<SchemaExtensionDefinition> extensionDefinitions;
814814
private String description;
815815

816-
private final Set<GraphQLDirective> additionalDirectives = new LinkedHashSet<>();
816+
private final Map<String, GraphQLDirective> additionalDirectives = new LinkedHashMap<>();
817817
private final Set<GraphQLNamedType> additionalTypes = new LinkedHashSet<>();
818818
private final List<GraphQLDirective> schemaDirectives = new ArrayList<>();
819819
private final List<GraphQLAppliedDirective> schemaAppliedDirectives = new ArrayList<>();
@@ -921,19 +921,49 @@ public Builder clearAdditionalTypes() {
921921
return this;
922922
}
923923

924+
/**
925+
* Adds multiple directive definitions to the schema.
926+
*
927+
* @param additionalDirectives the directive definitions to add
928+
*
929+
* @return this builder
930+
*
931+
* @deprecated use {@link #additionalDirectives(Collection)} instead
932+
*/
933+
@Deprecated(since = "2026-05-20")
924934
public Builder additionalDirectives(Set<GraphQLDirective> additionalDirectives) {
925-
this.additionalDirectives.addAll(additionalDirectives);
935+
return additionalDirectives((Collection<? extends GraphQLDirective>) additionalDirectives);
936+
}
937+
938+
/**
939+
* Adds multiple directive definitions to the schema.
940+
*
941+
* @param additionalDirectives the directive definitions to add
942+
*
943+
* @return this builder
944+
*/
945+
public Builder additionalDirectives(Collection<? extends GraphQLDirective> additionalDirectives) {
946+
for (GraphQLDirective additionalDirective : additionalDirectives) {
947+
additionalDirective(additionalDirective);
948+
}
926949
return this;
927950
}
928951

929952
public Builder additionalDirective(GraphQLDirective additionalDirective) {
930-
this.additionalDirectives.add(additionalDirective);
953+
String name = additionalDirective.getName();
954+
GraphQLDirective existing = additionalDirectives.get(name);
955+
if (existing != null && existing != additionalDirective) {
956+
throw new AssertException(String.format("Directive '%s' already exists with a different instance", name));
957+
}
958+
if (existing == null) {
959+
additionalDirectives.put(name, additionalDirective);
960+
}
931961
return this;
932962
}
933963

934964
/**
935965
* Clears all directives from this builder, including any that were previously added
936-
* via {@link #additionalDirective(GraphQLDirective)} or {@link #additionalDirectives(Set)}.
966+
* via {@link #additionalDirective(GraphQLDirective)} or {@link #additionalDirectives(Collection)}.
937967
* Built-in directives ({@code @include}, {@code @skip}, {@code @deprecated}, etc.) will
938968
* always be added back automatically at build time by {@code ensureBuiltInDirectives()}.
939969
* <p>
@@ -1073,19 +1103,15 @@ private GraphQLSchema buildImpl() {
10731103

10741104
private void ensureBuiltInDirectives() {
10751105
// put built-in directives first, preserving user-supplied overrides by name
1076-
Set<String> userDirectiveNames = new LinkedHashSet<>();
1077-
for (GraphQLDirective d : additionalDirectives) {
1078-
userDirectiveNames.add(d.getName());
1079-
}
1080-
LinkedHashSet<GraphQLDirective> ordered = new LinkedHashSet<>();
1106+
Map<String, GraphQLDirective> ordered = new LinkedHashMap<>();
10811107
for (GraphQLDirective builtIn : Directives.BUILT_IN_DIRECTIVES) {
1082-
if (!userDirectiveNames.contains(builtIn.getName())) {
1083-
ordered.add(builtIn);
1108+
if (!additionalDirectives.containsKey(builtIn.getName())) {
1109+
ordered.put(builtIn.getName(), builtIn);
10841110
}
10851111
}
1086-
ordered.addAll(additionalDirectives);
1112+
ordered.putAll(additionalDirectives);
10871113
additionalDirectives.clear();
1088-
additionalDirectives.addAll(ordered);
1114+
additionalDirectives.putAll(ordered);
10891115
}
10901116

10911117
private GraphQLSchema validateSchema(GraphQLSchema graphQLSchema) {

‎src/main/java/graphql/schema/SchemaTransformer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -735,7 +735,7 @@ public GraphQLSchema rebuildSchema(GraphQLCodeRegistry.Builder codeRegistry, Set
735735
.mutation(this.mutation)
736736
.subscription(this.subscription)
737737
.additionalTypes(this.additionalTypes)
738-
.additionalDirectives(this.directives)
738+
.additionalDirectives(new ArrayList<>(this.directives))
739739
.introspectionSchemaType(this.introspectionSchemaType)
740740
.withSchemaDirectives(this.schemaDirectives)
741741
.withSchemaAppliedDirectives(this.schemaAppliedDirectives)

‎src/main/java/graphql/schema/idl/SchemaGenerator.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import graphql.schema.GraphQLType;
1212
import graphql.schema.idl.errors.SchemaProblem;
1313

14+
import java.util.Collection;
1415
import java.util.List;
1516
import java.util.Map;
1617
import java.util.Set;
@@ -128,7 +129,7 @@ private GraphQLSchema makeExecutableSchemaImpl(ImmutableTypeDefinitionRegistry t
128129

129130
GraphQLSchema.Builder schemaBuilder = GraphQLSchema.newSchema();
130131

131-
Set<GraphQLDirective> additionalDirectives = schemaGeneratorHelper.buildAdditionalDirectiveDefinitions(buildCtx);
132+
Collection<GraphQLDirective> additionalDirectives = schemaGeneratorHelper.buildAdditionalDirectiveDefinitions(buildCtx);
132133
schemaBuilder.additionalDirectives(additionalDirectives);
133134

134135
schemaGeneratorHelper.buildSchemaDirectivesAndExtensions(buildCtx, schemaBuilder);

‎src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy‎

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,84 @@ class GraphQLSchemaTest extends Specification {
187187
schema.getDirective("custom") != null
188188
}
189189

190+
def "duplicate directive with different instance throws error"() {
191+
given:
192+
def directive1 = directive("duplicate", DirectiveLocation.FIELD)
193+
def directive2 = directive("duplicate", DirectiveLocation.OBJECT)
194+
195+
when:
196+
basicSchemaBuilder()
197+
.additionalDirective(directive1)
198+
.additionalDirective(directive2)
199+
200+
then:
201+
def exception = thrown(AssertException)
202+
exception.message == "Directive 'duplicate' already exists with a different instance"
203+
}
204+
205+
def "same directive instance can be added multiple times"() {
206+
given:
207+
def directive = directive("myDir", DirectiveLocation.FIELD)
208+
209+
when:
210+
def schema = basicSchemaBuilder()
211+
.additionalDirective(directive)
212+
.additionalDirective(directive)
213+
.build()
214+
215+
then:
216+
schema.getDirective("myDir") == directive
217+
schema.directives.findAll { it.name == "myDir" }.size() == 1
218+
}
219+
220+
def "additionalDirectives accepts collection"() {
221+
given:
222+
def directive1 = directive("dir1", DirectiveLocation.FIELD)
223+
def directive2 = directive("dir2", DirectiveLocation.OBJECT)
224+
225+
when:
226+
def schema = basicSchemaBuilder()
227+
.additionalDirectives([directive1, directive2])
228+
.build()
229+
230+
then:
231+
schema.getDirective("dir1") == directive1
232+
schema.getDirective("dir2") == directive2
233+
}
234+
235+
@SuppressWarnings("deprecation")
236+
def "additionalDirectives accepts deprecated set overload"() {
237+
given:
238+
def directive1 = directive("setDir1", DirectiveLocation.FIELD)
239+
def directive2 = directive("setDir2", DirectiveLocation.OBJECT)
240+
Set<GraphQLDirective> directives = new LinkedHashSet<>()
241+
directives.add(directive1)
242+
directives.add(directive2)
243+
244+
when:
245+
def schema = basicSchemaBuilder()
246+
.additionalDirectives(directives)
247+
.build()
248+
249+
then:
250+
schema.getDirective("setDir1") == directive1
251+
schema.getDirective("setDir2") == directive2
252+
}
253+
254+
def "additionalDirectives rejects duplicate directive names in collection"() {
255+
given:
256+
def directive1 = directive("duplicate", DirectiveLocation.FIELD)
257+
def directive2 = directive("duplicate", DirectiveLocation.OBJECT)
258+
259+
when:
260+
basicSchemaBuilder()
261+
.additionalDirectives([directive1, directive2])
262+
263+
then:
264+
def exception = thrown(AssertException)
265+
exception.message == "Directive 'duplicate' already exists with a different instance"
266+
}
267+
190268
def "clearDirectives supports replacing non-built-in directives in a schema transform"() {
191269
given: "a schema with a custom directive"
192270
def originalDirective = GraphQLDirective.newDirective()
@@ -209,7 +287,7 @@ class GraphQLSchemaTest extends Specification {
209287
def nonBuiltIns = schema.getDirectives().findAll { !Directives.isBuiltInDirective(it) }
210288
.collect { it.getName() == "custom" ? replacementDirective : it }
211289
builder.clearDirectives()
212-
.additionalDirectives(new LinkedHashSet<>(nonBuiltIns))
290+
.additionalDirectives(nonBuiltIns)
213291
})
214292

215293
then: "all 7 built-in directives are still present"
@@ -248,6 +326,13 @@ class GraphQLSchemaTest extends Specification {
248326
schema.getDirective("skip").description == "custom skip description"
249327
}
250328

329+
private static GraphQLDirective directive(String name, DirectiveLocation directiveLocation) {
330+
GraphQLDirective.newDirective()
331+
.name(name)
332+
.validLocations(directiveLocation)
333+
.build()
334+
}
335+
251336
def "clear additional types works as expected"() {
252337
setup:
253338
def schemaBuilder = basicSchemaBuilder()

‎src/test/groovy/graphql/schema/validation/NoDirectiveRedefinitionTest.groovy‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package graphql.schema.validation
2+
3+
import graphql.AssertException
4+
import graphql.TestUtil
5+
import graphql.schema.GraphQLDirective
6+
import spock.lang.Specification
7+
8+
import static graphql.Scalars.GraphQLString
9+
import static graphql.introspection.Introspection.DirectiveLocation.FIELD_DEFINITION
10+
import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition
11+
import static graphql.schema.GraphQLObjectType.newObject
12+
import static graphql.schema.GraphQLSchema.newSchema
13+
14+
class NoDirectiveRedefinitionTest extends Specification {
15+
16+
def "directive cannot be redefined in SDL schema"() {
17+
given:
18+
def sdl = '''
19+
directive @exampleDirective on FIELD_DEFINITION
20+
directive @exampleDirective on FIELD_DEFINITION
21+
22+
type Query {
23+
hello: String @exampleDirective
24+
}
25+
'''
26+
27+
when:
28+
TestUtil.schema(sdl)
29+
30+
then:
31+
def schemaProblem = thrown(AssertionError)
32+
schemaProblem.message.contains("tried to redefine existing directive 'exampleDirective'")
33+
}
34+
35+
def "programmatically redefined directive is rejected"() {
36+
when:
37+
newSchema()
38+
.query(newObject()
39+
.name("Query")
40+
.field(newFieldDefinition()
41+
.name("hello")
42+
.type(GraphQLString))
43+
.build())
44+
.additionalDirective(exampleDirective())
45+
.additionalDirective(exampleDirective())
46+
.build()
47+
48+
then:
49+
def exception = thrown(AssertException)
50+
exception.message == "Directive 'exampleDirective' already exists with a different instance"
51+
}
52+
53+
private static GraphQLDirective exampleDirective() {
54+
GraphQLDirective.newDirective()
55+
.name("exampleDirective")
56+
.validLocation(FIELD_DEFINITION)
57+
.build()
58+
}
59+
}

0 commit comments

Comments
 (0)