Merge pull request #3552 from jbellenger/jbellenger-validate-dir-loc

require non-empty directive locations

src/main/java/graphql/schema/GraphQLDirective.java

@@ -15,6 +15,7 @@
 import java.util.function.Consumer;
 import java.util.function.UnaryOperator;
 
+import static graphql.Assert.assertNotEmpty;
 import static graphql.Assert.assertNotNull;
 import static graphql.Assert.assertValidName;
 import static graphql.introspection.Introspection.DirectiveLocation;
@@ -52,6 +53,7 @@ private GraphQLDirective(String name,
                              DirectiveDefinition definition) {
         assertValidName(name);
         assertNotNull(arguments, () -> "arguments can't be null");
+        assertNotEmpty(locations, () -> "locations can't be empty");
         this.name = name;
         this.description = description;
         this.repeatable = repeatable;

src/test/groovy/graphql/TestUtil.groovy

@@ -2,6 +2,7 @@ package graphql
 
 import graphql.execution.MergedField
 import graphql.execution.MergedSelectionSet
+import graphql.introspection.Introspection.DirectiveLocation
 import graphql.language.Document
 import graphql.language.Field
 import graphql.language.NullValue
@@ -12,8 +13,8 @@ import graphql.language.Type
 import graphql.parser.Parser
 import graphql.schema.Coercing
 import graphql.schema.DataFetcher
-import graphql.schema.GraphQLAppliedDirectiveArgument
 import graphql.schema.GraphQLAppliedDirective
+import graphql.schema.GraphQLAppliedDirectiveArgument
 import graphql.schema.GraphQLArgument
 import graphql.schema.GraphQLDirective
 import graphql.schema.GraphQLInputType
@@ -194,13 +195,19 @@ class TestUtil {
                 .name(definition.getName())
                 .description(definition.getDescription() == null ? null : definition.getDescription().getContent())
                 .coercing(mockCoercing())
-                .replaceDirectives(definition.getDirectives().stream().map({ mockDirective(it.getName()) }).collect(Collectors.toList()))
+                .replaceDirectives(
+                        definition.getDirectives()
+                                .stream()
+                                .map({ mkDirective(it.getName(), DirectiveLocation.SCALAR) })
+                                .collect(Collectors.toList()))
                 .definition(definition)
                 .build()
     }
 
-    static GraphQLDirective mockDirective(String name) {
-        newDirective().name(name).description(name).build()
+    static GraphQLDirective mkDirective(String name, DirectiveLocation location, GraphQLArgument arg = null) {
+        def b = newDirective().name(name).description(name).validLocation(location)
+        if (arg != null) b.argument(arg)
+        b.build()
     }
 
     static TypeRuntimeWiring mockTypeRuntimeWiring(String typeName, boolean withResolver) {

src/test/groovy/graphql/schema/GraphQLArgumentTest.groovy

@@ -1,6 +1,7 @@
 package graphql.schema
 
 import graphql.collect.ImmutableKit
+import static graphql.introspection.Introspection.DirectiveLocation.ARGUMENT_DEFINITION
 import graphql.language.FloatValue
 import graphql.schema.validation.InvalidSchemaException
 import spock.lang.Specification
@@ -9,10 +10,10 @@ import static graphql.Scalars.GraphQLFloat
 import static graphql.Scalars.GraphQLInt
 import static graphql.Scalars.GraphQLString
 import static graphql.schema.GraphQLArgument.newArgument
-import static graphql.schema.GraphQLDirective.newDirective
 import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition
 import static graphql.schema.GraphQLObjectType.newObject
 import static graphql.schema.GraphQLSchema.newSchema
+import static graphql.TestUtil.mkDirective
 
 class GraphQLArgumentTest extends Specification {
 
@@ -22,15 +23,15 @@ class GraphQLArgumentTest extends Specification {
                 .description("A1_description")
                 .type(GraphQLInt)
                 .deprecate("custom reason")
-                .withDirective(newDirective().name("directive1"))
+                .withDirective(mkDirective("directive1", ARGUMENT_DEFINITION))
                 .build()
         when:
         def transformedArgument = startingArgument.transform({
             it
                     .name("A2")
                     .description("A2_description")
                     .type(GraphQLString)
-                    .withDirective(newDirective().name("directive3"))
+                    .withDirective(mkDirective("directive3", ARGUMENT_DEFINITION))
                     .value("VALUE") // Retain deprecated for test coverage
                     .deprecate(null)
                     .defaultValue("DEFAULT") // Retain deprecated for test coverage
@@ -79,9 +80,9 @@ class GraphQLArgumentTest extends Specification {
         def argument
 
         given:
-        def builder = GraphQLArgument.newArgument().name("A1")
+        def builder = newArgument().name("A1")
                 .type(GraphQLInt)
-                .withDirective(newDirective().name("directive1"))
+                .withDirective(mkDirective("directive1", ARGUMENT_DEFINITION))
 
         when:
         argument = builder.build()
@@ -96,8 +97,8 @@ class GraphQLArgumentTest extends Specification {
         when:
         argument = builder
                 .clearDirectives()
-                .withDirective(newDirective().name("directive2"))
-                .withDirective(newDirective().name("directive3"))
+                .withDirective(mkDirective("directive2", ARGUMENT_DEFINITION))
+                .withDirective(mkDirective("directive3", ARGUMENT_DEFINITION))
                 .build()
 
         then:
@@ -109,9 +110,9 @@ class GraphQLArgumentTest extends Specification {
         when:
         argument = builder
                 .replaceDirectives([
-                        newDirective().name("directive1").build(),
-                        newDirective().name("directive2").build(),
-                        newDirective().name("directive3").build()]) // overwrite
+                        mkDirective("directive1", ARGUMENT_DEFINITION),
+                        mkDirective("directive2", ARGUMENT_DEFINITION),
+                        mkDirective("directive3", ARGUMENT_DEFINITION)]) // overwrite
                 .build()
 
         then:
@@ -198,7 +199,7 @@ class GraphQLArgumentTest extends Specification {
     def "Applied schema directives arguments are validated for programmatic schemas"() {
         given:
         def arg = newArgument().name("arg").type(GraphQLInt).valueProgrammatic(ImmutableKit.emptyMap()).build() // Retain for test coverage
-        def directive = GraphQLDirective.newDirective().name("cached").argument(arg).build()
+        def directive = mkDirective("cached", ARGUMENT_DEFINITION, arg)
         def field = newFieldDefinition()
                 .name("hello")
                 .type(GraphQLString)

src/test/groovy/graphql/schema/GraphQLDirectiveTest.groovy

@@ -1,6 +1,8 @@
 package graphql.schema
 
+import graphql.AssertException
 import graphql.TestUtil
+import graphql.introspection.Introspection
 import graphql.language.Node
 import spock.lang.Specification
 
@@ -168,9 +170,38 @@ class GraphQLDirectiveTest extends Specification {
 
         then:
         assertDirectiveContainer(scalarType)
+    }
+
+    def "throws an error on missing required properties"() {
+        given:
+        def validDirective = GraphQLDirective.newDirective()
+                .name("dir")
+                .validLocation(Introspection.DirectiveLocation.SCALAR)
+                .build()
+
+        when:
+        validDirective.transform { it.name(null) }
+
+        then:
+        def e = thrown(AssertException)
+        e.message.contains("Name must be non-null, non-empty")
 
+        when:
+        validDirective.transform { it.replaceArguments(null) }
+
+        then:
+        def e2 = thrown(AssertException)
+        e2.message.contains("arguments must not be null")
+
+        when:
+        validDirective.transform { it.clearValidLocations() }
+
+        then:
+        def e3 = thrown(AssertException)
+        e3.message.contains("locations can't be empty")
     }
 
+
     static boolean assertDirectiveContainer(GraphQLDirectiveContainer container) {
         assert container.hasDirective("d1") // Retain for test coverage
         assert container.hasAppliedDirective("d1")

src/test/groovy/graphql/schema/GraphQLEnumValueDefinitionTest.groovy

@@ -1,25 +1,25 @@
 package graphql.schema
 
+import static graphql.introspection.Introspection.DirectiveLocation
 import spock.lang.Specification
 
-import static graphql.schema.GraphQLDirective.newDirective
 import static graphql.schema.GraphQLEnumValueDefinition.newEnumValueDefinition
+import static graphql.TestUtil.mkDirective
 
 class GraphQLEnumValueDefinitionTest extends Specification {
     def "object can be transformed"() {
         given:
         def startEnumValue = newEnumValueDefinition().name("EV1")
                 .description("EV1_description")
                 .value("A")
-                .withDirective(newDirective().name("directive1"))
+                .withDirective(mkDirective("directive1", DirectiveLocation.ENUM_VALUE))
                 .build()
         when:
         def transformedEnumValue = startEnumValue.transform({
             it
                     .name("EV2")
                     .value("X")
-                    .withDirective(newDirective().name("directive2"))
-
+                    .withDirective(mkDirective("directive2", DirectiveLocation.ENUM_VALUE))
         })
 
         then:

src/test/groovy/graphql/schema/GraphQLFieldDefinitionTest.groovy

@@ -2,6 +2,7 @@ package graphql.schema
 
 import graphql.AssertException
 import graphql.TestUtil
+import graphql.introspection.Introspection
 import graphql.schema.idl.SchemaPrinter
 import spock.lang.Specification
 
@@ -10,9 +11,9 @@ import static graphql.Scalars.GraphQLFloat
 import static graphql.Scalars.GraphQLInt
 import static graphql.Scalars.GraphQLString
 import static graphql.TestUtil.mockArguments
+import static graphql.TestUtil.mkDirective
 import static graphql.schema.DefaultGraphqlTypeComparatorRegistry.newComparators
 import static graphql.schema.GraphQLArgument.newArgument
-import static graphql.schema.GraphQLDirective.newDirective
 import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition
 import static graphql.schema.idl.SchemaPrinter.Options.defaultOptions
 
@@ -35,8 +36,8 @@ class GraphQLFieldDefinitionTest extends Specification {
                 .deprecate("F1_deprecated")
                 .argument(newArgument().name("argStr").type(GraphQLString))
                 .argument(newArgument().name("argInt").type(GraphQLInt))
-                .withDirective(newDirective().name("directive1"))
-                .withDirective(newDirective().name("directive2"))
+                .withDirective(mkDirective("directive1", Introspection.DirectiveLocation.FIELD_DEFINITION))
+                .withDirective(mkDirective("directive2", Introspection.DirectiveLocation.FIELD_DEFINITION))
                 .build()
 
         when:
@@ -47,13 +48,10 @@ class GraphQLFieldDefinitionTest extends Specification {
                     .argument(newArgument().name("argStr").type(GraphQLString))
                     .argument(newArgument().name("argInt").type(GraphQLBoolean))
                     .argument(newArgument().name("argIntAdded").type(GraphQLInt))
-                    .withDirective(newDirective().name("directive3"))
-
+                    .withDirective(mkDirective("directive3", Introspection.DirectiveLocation.FIELD_DEFINITION))
         })
 
-
         then:
-
         startingField.name == "F1"
         startingField.type == GraphQLFloat
         startingField.description == "F1_description"

src/test/groovy/graphql/schema/GraphQLInputObjectFieldTest.groovy

@@ -1,12 +1,13 @@
 package graphql.schema
 
+import graphql.introspection.Introspection
 import graphql.language.FloatValue
 import spock.lang.Specification
 
 import static graphql.Scalars.GraphQLFloat
 import static graphql.Scalars.GraphQLInt
-import static graphql.schema.GraphQLDirective.newDirective
 import static graphql.schema.GraphQLInputObjectField.newInputObjectField
+import static graphql.TestUtil.mkDirective
 
 class GraphQLInputObjectFieldTest extends Specification {
 
@@ -16,8 +17,8 @@ class GraphQLInputObjectFieldTest extends Specification {
                 .name("F1")
                 .type(GraphQLFloat)
                 .description("F1_description")
-                .withDirective(newDirective().name("directive1"))
-                .withDirective(newDirective().name("directive2"))
+                .withDirective(mkDirective("directive1", Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION))
+                .withDirective(mkDirective("directive2", Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION))
                 .deprecate("No longer useful")
                 .build()
 
@@ -26,13 +27,10 @@ class GraphQLInputObjectFieldTest extends Specification {
             builder.name("F2")
                     .type(GraphQLInt)
                     .deprecate(null)
-                    .withDirective(newDirective().name("directive3"))
-
+                    .withDirective(mkDirective("directive3", Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION))
         })
 
-
         then:
-
         startingField.name == "F1"
         startingField.type == GraphQLFloat
         startingField.description == "F1_description"

src/test/groovy/graphql/schema/GraphQLScalarTypeTest.groovy

@@ -1,8 +1,9 @@
 package graphql.schema
 
+import graphql.introspection.Introspection
 import spock.lang.Specification
 
-import static graphql.schema.GraphQLDirective.newDirective
+import static graphql.TestUtil.mkDirective
 
 class GraphQLScalarTypeTest extends Specification {
     Coercing<String, String> coercing = new Coercing<String, String>() {
@@ -28,14 +29,14 @@ class GraphQLScalarTypeTest extends Specification {
                 .name("S1")
                 .description("S1_description")
                 .coercing(coercing)
-                .withDirective(newDirective().name("directive1"))
-                .withDirective(newDirective().name("directive2"))
+                .withDirective(mkDirective("directive1", Introspection.DirectiveLocation.SCALAR))
+                .withDirective(mkDirective("directive2", Introspection.DirectiveLocation.SCALAR))
                 .build()
         when:
         def transformedScalar = startingScalar.transform({ builder ->
             builder.name("S2")
                     .description("S2_description")
-                    .withDirective(newDirective().name("directive3"))
+                    .withDirective(mkDirective("directive3", Introspection.DirectiveLocation.SCALAR))
         })
 
         then:
@@ -55,6 +56,5 @@ class GraphQLScalarTypeTest extends Specification {
         transformedScalar.getDirective("directive1") != null
         transformedScalar.getDirective("directive2") != null
         transformedScalar.getDirective("directive3") != null
-
     }
 }