Skip to content

Commit 7ba1647

Browse files
andimarekandimarek
authored
Merge pull request #4229 from graphql-java/investigate-directive-handling
Consolidate built-in directive handling and move helpers to Directives class
2 parents 9f18071 + f361f4f commit 7ba1647

File tree

14 files changed

+171
-121
lines changed

14 files changed

+171
-121
lines changed

‎src/main/java/graphql/Directives.java‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@
77
import graphql.language.StringValue;
88
import graphql.schema.GraphQLDirective;
99

10+
import java.util.Collections;
11+
import java.util.LinkedHashMap;
12+
import java.util.LinkedHashSet;
13+
import java.util.Map;
14+
import java.util.Set;
1015
import java.util.concurrent.atomic.AtomicBoolean;
1116

1217
import static graphql.Scalars.GraphQLBoolean;
@@ -249,6 +254,57 @@ public class Directives {
249254
.definition(EXPERIMENTAL_DISABLE_ERROR_PROPAGATION_DIRECTIVE_DEFINITION)
250255
.build();
251256

257+
/**
258+
* The set of all built-in directives that are always present in a graphql schema.
259+
* The iteration order is stable and meaningful.
260+
*/
261+
public static final Set<GraphQLDirective> BUILT_IN_DIRECTIVES;
262+
263+
/**
264+
* A map from directive name to directive for all built-in directives.
265+
*/
266+
public static final Map<String, GraphQLDirective> BUILT_IN_DIRECTIVES_MAP;
267+
268+
static {
269+
LinkedHashSet<GraphQLDirective> directives = new LinkedHashSet<>();
270+
directives.add(IncludeDirective);
271+
directives.add(SkipDirective);
272+
directives.add(DeprecatedDirective);
273+
directives.add(SpecifiedByDirective);
274+
directives.add(OneOfDirective);
275+
directives.add(DeferDirective);
276+
directives.add(ExperimentalDisableErrorPropagationDirective);
277+
BUILT_IN_DIRECTIVES = Collections.unmodifiableSet(directives);
278+
279+
LinkedHashMap<String, GraphQLDirective> map = new LinkedHashMap<>();
280+
for (GraphQLDirective d : BUILT_IN_DIRECTIVES) {
281+
map.put(d.getName(), d);
282+
}
283+
BUILT_IN_DIRECTIVES_MAP = Collections.unmodifiableMap(map);
284+
}
285+
286+
/**
287+
* Returns true if a directive with the provided name is a built-in directive.
288+
*
289+
* @param directiveName the name of the directive in question
290+
*
291+
* @return true if the directive is built-in, false otherwise
292+
*/
293+
public static boolean isBuiltInDirective(String directiveName) {
294+
return BUILT_IN_DIRECTIVES_MAP.containsKey(directiveName);
295+
}
296+
297+
/**
298+
* Returns true if the provided directive is a built-in directive.
299+
*
300+
* @param directive the directive in question
301+
*
302+
* @return true if the directive is built-in, false otherwise
303+
*/
304+
public static boolean isBuiltInDirective(GraphQLDirective directive) {
305+
return isBuiltInDirective(directive.getName());
306+
}
307+
252308
private static Description createDescription(String s) {
253309
return new Description(s, null, false);
254310
}

‎src/main/java/graphql/introspection/IntrospectionResultToSchema.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
import static graphql.Assert.assertShouldNeverHappen;
4242
import static graphql.Assert.assertTrue;
4343
import static graphql.collect.ImmutableKit.map;
44-
import static graphql.schema.idl.DirectiveInfo.isGraphqlSpecifiedDirective;
44+
import static graphql.Directives.isBuiltInDirective;
4545

4646
@SuppressWarnings("unchecked")
4747
@PublicApi
@@ -131,7 +131,7 @@ public Document createSchemaDefinition(Map<String, Object> introspectionResult)
131131

132132
private DirectiveDefinition createDirective(Map<String, Object> input) {
133133
String directiveName = (String) input.get("name");
134-
if (isGraphqlSpecifiedDirective(directiveName)) {
134+
if (isBuiltInDirective(directiveName)) {
135135
return null;
136136
}
137137

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

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
import static graphql.collect.ImmutableKit.nonNullCopyOf;
3838
import static graphql.schema.GraphqlTypeComparators.byNameAsc;
3939
import static graphql.schema.GraphqlTypeComparators.sortTypes;
40-
import static java.util.Arrays.asList;
40+
4141

4242
/**
4343
* The schema represents the combined type system of the graphql engine. This is how the engine knows
@@ -692,7 +692,6 @@ public static Builder newSchema(GraphQLSchema existingSchema) {
692692
.introspectionSchemaType(existingSchema.getIntrospectionSchemaType())
693693
.codeRegistry(existingSchema.getCodeRegistry())
694694
.clearAdditionalTypes()
695-
.clearDirectives()
696695
.additionalDirectives(new LinkedHashSet<>(existingSchema.getDirectives()))
697696
.clearSchemaDirectives()
698697
.withSchemaDirectives(schemaDirectivesArray(existingSchema))
@@ -741,10 +740,7 @@ public static class Builder {
741740
private List<SchemaExtensionDefinition> extensionDefinitions;
742741
private String description;
743742

744-
// we default these in
745-
private final Set<GraphQLDirective> additionalDirectives = new LinkedHashSet<>(
746-
asList(Directives.IncludeDirective, Directives.SkipDirective)
747-
);
743+
private final Set<GraphQLDirective> additionalDirectives = new LinkedHashSet<>();
748744
private final Set<GraphQLNamedType> additionalTypes = new LinkedHashSet<>();
749745
private final List<GraphQLDirective> schemaDirectives = new ArrayList<>();
750746
private final List<GraphQLAppliedDirective> schemaAppliedDirectives = new ArrayList<>();
@@ -862,12 +858,6 @@ public Builder additionalDirective(GraphQLDirective additionalDirective) {
862858
return this;
863859
}
864860

865-
public Builder clearDirectives() {
866-
this.additionalDirectives.clear();
867-
return this;
868-
}
869-
870-
871861
public Builder withSchemaDirectives(GraphQLDirective... directives) {
872862
for (GraphQLDirective directive : directives) {
873863
withSchemaDirective(directive);
@@ -960,13 +950,8 @@ private GraphQLSchema buildImpl() {
960950
assertNotNull(additionalTypes, "additionalTypes can't be null");
961951
assertNotNull(additionalDirectives, "additionalDirectives can't be null");
962952

963-
// schemas built via the schema generator have the deprecated directive BUT we want it present for hand built
964-
// schemas - it's inherently part of the spec!
965-
addBuiltInDirective(Directives.DeprecatedDirective, additionalDirectives);
966-
addBuiltInDirective(Directives.SpecifiedByDirective, additionalDirectives);
967-
addBuiltInDirective(Directives.OneOfDirective, additionalDirectives);
968-
addBuiltInDirective(Directives.DeferDirective, additionalDirectives);
969-
addBuiltInDirective(Directives.ExperimentalDisableErrorPropagationDirective, additionalDirectives);
953+
// built-in directives are always present in a schema and come first
954+
ensureBuiltInDirectives();
970955

971956
// quick build - no traversing
972957
final GraphQLSchema partiallyBuiltSchema = new GraphQLSchema(this);
@@ -989,10 +974,21 @@ private GraphQLSchema buildImpl() {
989974
return validateSchema(finalSchema);
990975
}
991976

992-
private void addBuiltInDirective(GraphQLDirective qlDirective, Set<GraphQLDirective> additionalDirectives1) {
993-
if (additionalDirectives1.stream().noneMatch(d -> d.getName().equals(qlDirective.getName()))) {
994-
additionalDirectives1.add(qlDirective);
977+
private void ensureBuiltInDirectives() {
978+
// put built-in directives first, preserving user-supplied overrides by name
979+
Set<String> userDirectiveNames = new LinkedHashSet<>();
980+
for (GraphQLDirective d : additionalDirectives) {
981+
userDirectiveNames.add(d.getName());
982+
}
983+
LinkedHashSet<GraphQLDirective> ordered = new LinkedHashSet<>();
984+
for (GraphQLDirective builtIn : Directives.BUILT_IN_DIRECTIVES) {
985+
if (!userDirectiveNames.contains(builtIn.getName())) {
986+
ordered.add(builtIn);
987+
}
995988
}
989+
ordered.addAll(additionalDirectives);
990+
additionalDirectives.clear();
991+
additionalDirectives.addAll(ordered);
996992
}
997993

998994
private GraphQLSchema validateSchema(GraphQLSchema graphQLSchema) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import graphql.Assert;
44
import graphql.PublicApi;
55
import graphql.introspection.Introspection;
6-
import graphql.schema.idl.DirectiveInfo;
6+
import graphql.Directives;
77
import graphql.schema.idl.ScalarInfo;
88

99
import java.util.Stack;
@@ -293,7 +293,7 @@ public static Predicate<GraphQLNamedSchemaElement> isSystemElement() {
293293
return ScalarInfo.isGraphqlSpecifiedScalar((GraphQLScalarType) schemaElement);
294294
}
295295
if (schemaElement instanceof GraphQLDirective) {
296-
return DirectiveInfo.isGraphqlSpecifiedDirective((GraphQLDirective) schemaElement);
296+
return Directives.isBuiltInDirective((GraphQLDirective) schemaElement);
297297
}
298298
if (schemaElement instanceof GraphQLNamedType) {
299299
return Introspection.isIntrospectionTypes((GraphQLNamedType) schemaElement);

‎src/main/java/graphql/schema/diffing/SchemaGraphFactory.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import graphql.introspection.Introspection;
77
import graphql.language.AstPrinter;
88
import graphql.schema.*;
9-
import graphql.schema.idl.DirectiveInfo;
9+
import graphql.Directives;
1010
import graphql.schema.idl.ScalarInfo;
1111
import graphql.util.TraversalControl;
1212
import graphql.util.Traverser;
@@ -390,7 +390,7 @@ private void newDirective(GraphQLDirective directive, SchemaGraph schemaGraph) {
390390
directiveVertex.add("name", directive.getName());
391391
directiveVertex.add("repeatable", directive.isRepeatable());
392392
directiveVertex.add("locations", directive.validLocations());
393-
boolean graphqlSpecified = DirectiveInfo.isGraphqlSpecifiedDirective(directive.getName());
393+
boolean graphqlSpecified = Directives.isBuiltInDirective(directive.getName());
394394
directiveVertex.setBuiltInType(graphqlSpecified);
395395
directiveVertex.add("description", desc(directive.getDescription()));
396396
for (GraphQLArgument argument : directive.getArguments()) {

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

Lines changed: 0 additions & 61 deletions
This file was deleted.

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package graphql.schema.idl;
22

33
import graphql.Assert;
4+
import graphql.Directives;
45
import graphql.DirectivesUtil;
56
import graphql.GraphQLContext;
67
import graphql.PublicApi;
@@ -64,6 +65,7 @@
6465
import java.util.stream.Stream;
6566

6667
import static graphql.Directives.DeprecatedDirective;
68+
import static graphql.Directives.SpecifiedByDirective;
6769
import static graphql.Scalars.GraphQLString;
6870
import static graphql.schema.visibility.DefaultGraphqlFieldVisibility.DEFAULT_FIELD_VISIBILITY;
6971
import static graphql.util.EscapeUtil.escapeJsonString;
@@ -80,7 +82,7 @@ public class SchemaPrinter {
8082
* This predicate excludes all directives which are specified by the GraphQL Specification.
8183
* Printing these directives is optional.
8284
*/
83-
public static final Predicate<String> ExcludeGraphQLSpecifiedDirectivesPredicate = d -> !DirectiveInfo.isGraphqlSpecifiedDirective(d);
85+
public static final Predicate<String> ExcludeGraphQLSpecifiedDirectivesPredicate = d -> !Directives.isBuiltInDirective(d);
8486

8587
/**
8688
* Options to use when printing a schema
@@ -559,7 +561,12 @@ private SchemaElementPrinter<GraphQLScalarType> scalarPrinter() {
559561
printAsAst(out, type.getDefinition(), type.getExtensionDefinitions());
560562
} else {
561563
printComments(out, type, "");
562-
out.format("scalar %s%s\n\n", type.getName(), directivesString(GraphQLScalarType.class, type));
564+
List<GraphQLAppliedDirective> directives = DirectivesUtil.toAppliedDirectives(type).stream()
565+
.filter(d -> !d.getName().equals(SpecifiedByDirective.getName()))
566+
.collect(toList());
567+
out.format("scalar %s%s%s\n\n", type.getName(),
568+
directivesString(GraphQLScalarType.class, directives),
569+
specifiedByUrlString(type));
563570
}
564571
}
565572
};
@@ -1103,6 +1110,14 @@ private String getDeprecationReason(GraphQLDirectiveContainer directiveContainer
11031110
}
11041111
}
11051112

1113+
private String specifiedByUrlString(GraphQLScalarType scalarType) {
1114+
String url = scalarType.getSpecifiedByUrl();
1115+
if (url == null || !options.getIncludeDirective().test(SpecifiedByDirective.getName())) {
1116+
return "";
1117+
}
1118+
return " @specifiedBy(url : \"" + escapeJsonString(url) + "\")";
1119+
}
1120+
11061121
private String directiveDefinition(GraphQLDirective directive) {
11071122
StringBuilder sb = new StringBuilder();
11081123

‎src/main/java/graphql/schema/usage/SchemaUsage.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import graphql.schema.GraphQLNamedType;
1212
import graphql.schema.GraphQLObjectType;
1313
import graphql.schema.GraphQLSchema;
14-
import graphql.schema.idl.DirectiveInfo;
14+
import graphql.Directives;
1515
import graphql.schema.idl.ScalarInfo;
1616

1717
import java.util.HashSet;
@@ -180,7 +180,7 @@ private boolean isReferencedImpl(GraphQLSchema schema, String elementName, Set<S
180180
GraphQLDirective directive = schema.getDirective(elementName);
181181
if (directive != null) {
182182
String directiveName = directive.getName();
183-
if (DirectiveInfo.isGraphqlSpecifiedDirective(directiveName)) {
183+
if (Directives.isBuiltInDirective(directiveName)) {
184184
return true;
185185
}
186186
if (isNamedElementReferenced(schema, directiveName, pathSoFar)) {

‎src/main/java/graphql/util/Anonymizer.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@
7171
import graphql.schema.GraphQLUnionType;
7272
import graphql.schema.SchemaTransformer;
7373
import graphql.schema.TypeResolver;
74-
import graphql.schema.idl.DirectiveInfo;
7574
import graphql.schema.idl.ScalarInfo;
7675
import graphql.schema.idl.TypeUtil;
7776
import graphql.schema.impl.SchemaUtil;
@@ -259,7 +258,7 @@ public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition graph
259258

260259
@Override
261260
public TraversalControl visitGraphQLDirective(GraphQLDirective graphQLDirective, TraverserContext<GraphQLSchemaElement> context) {
262-
if (DirectiveInfo.isGraphqlSpecifiedDirective(graphQLDirective.getName())) {
261+
if (Directives.isBuiltInDirective(graphQLDirective.getName())) {
263262
return TraversalControl.ABORT;
264263
}
265264
String newName = assertNotNull(newNameMap.get(graphQLDirective));
@@ -282,7 +281,7 @@ public TraversalControl visitGraphQLAppliedDirective(GraphQLAppliedDirective gra
282281
changeNode(context, newElement);
283282
return TraversalControl.ABORT;
284283
}
285-
if (DirectiveInfo.isGraphqlSpecifiedDirective(graphQLDirective.getName())) {
284+
if (Directives.isBuiltInDirective(graphQLDirective.getName())) {
286285
return TraversalControl.ABORT;
287286
}
288287
String newName = assertNotNull(newNameMap.get(graphQLDirective));
@@ -516,7 +515,7 @@ public TraversalControl visitGraphQLAppliedDirectiveArgument(GraphQLAppliedDirec
516515

517516
@Override
518517
public TraversalControl visitGraphQLDirective(GraphQLDirective graphQLDirective, TraverserContext<GraphQLSchemaElement> context) {
519-
if (DirectiveInfo.isGraphqlSpecifiedDirective(graphQLDirective)) {
518+
if (Directives.isBuiltInDirective(graphQLDirective)) {
520519
return TraversalControl.ABORT;
521520
}
522521
recordDirectiveName.accept(graphQLDirective);
@@ -525,7 +524,7 @@ public TraversalControl visitGraphQLDirective(GraphQLDirective graphQLDirective,
525524

526525
@Override
527526
public TraversalControl visitGraphQLAppliedDirective(GraphQLAppliedDirective graphQLAppliedDirective, TraverserContext<GraphQLSchemaElement> context) {
528-
if (DirectiveInfo.isGraphqlSpecifiedDirective(graphQLAppliedDirective.getName())) {
527+
if (Directives.isBuiltInDirective(graphQLAppliedDirective.getName())) {
529528
return TraversalControl.ABORT;
530529
}
531530
recordDirectiveName.accept(graphQLAppliedDirective);

‎src/test/groovy/graphql/archunit/JSpecifyAnnotationsCheck.groovy‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,6 @@ class JSpecifyAnnotationsCheck extends Specification {
286286
"graphql.schema.diff.reporting.PrintStreamReporter",
287287
"graphql.schema.diffing.SchemaGraph",
288288
"graphql.schema.idl.CombinedWiringFactory",
289-
"graphql.schema.idl.DirectiveInfo",
290289
"graphql.schema.idl.MapEnumValuesProvider",
291290
"graphql.schema.idl.NaturalEnumValuesProvider",
292291
"graphql.schema.idl.RuntimeWiring",

0 commit comments

Comments
 (0)