Warn about directives applied where their definition doesn't allow them

src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt

@@ -118,7 +118,7 @@
         val types = (additionalObjects.toSet() as Set<GraphQLType>) + inputObjects + enums + interfaces + unions
         // Directives applied to the schema itself, e.g. `extend schema @link(...)`. Only GraphQLAppliedDirectives are built here, not
         // the deprecated GraphQLDirective ones: graphql-java doesn't resolve type references in those, so enum arguments would fail.
-        val schemaAppliedDirectives = buildAppliedDirectives(rootInfo.getDirectives()).toList()
+        val schemaAppliedDirectives = buildAppliedDirectives(rootInfo.getDirectives(), Introspection.DirectiveLocation.SCHEMA, "the schema").toList()
         return SchemaObjects(query, mutation, subscription, types, schemaDirectives.all, codeRegistryBuilder, rootInfo.getDescription(), schemaAppliedDirectives)
     }
 
@@ -142,8 +142,7 @@
             .definition(objectDefinition)
             .extensionDefinitions(extensions)
             .description(getDocumentation(objectDefinition, options))
-            .withAppliedDirectives(*buildAppliedDirectives(directives))
-            .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.OBJECT))
+            .withDirectivesAt(directives, Introspection.DirectiveLocation.OBJECT, "type '$name'")
             .apply {
                 objectDefinition.withExtensions(typeExtensions).flatMap { it.implements }.forEach { implementsDefinition ->
                     val interfaceName = (implementsDefinition as TypeName).name
@@ -154,7 +153,7 @@
             .apply {
                 objectDefinition.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }.forEach { fieldDefinition ->
                     field { field ->
-                        createField(field, fieldDefinition, inputObjects)
+                        createField(field, name, fieldDefinition, inputObjects)
                         codeRegistryBuilder.dataFetcher(
                             FieldCoordinates.coordinates(objectDefinition.name, fieldDefinition.name),
                             fieldResolversByType[objectDefinition]?.get(fieldDefinition)?.createDataFetcher()
@@ -184,8 +183,7 @@
             .definition(definition)
             .extensionDefinitions(extensionDefinitions)
             .description(getDocumentation(definition, options))
-            .withAppliedDirectives(*buildAppliedDirectives(directives))
-            .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.INPUT_OBJECT))
+            .withDirectivesAt(directives, Introspection.DirectiveLocation.INPUT_OBJECT, "type '${definition.name}'")
             .apply {
                 (extensionDefinitions + definition).forEach { typeDefinition ->
                     typeDefinition.inputValueDefinitions.forEach { fieldDefinition ->
@@ -197,8 +195,7 @@
                                 .apply { fieldDefinition.defaultValue?.let { v -> defaultValueLiteral(v) } }
                                 .apply { getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } }
                                 .type(determineInputType(fieldDefinition.type, inputObjects, referencingInputObjects))
-                                .withAppliedDirectives(*buildAppliedDirectives(fieldDefinition.directives))
-                                .withDirectives(*buildDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION))
+                                .withDirectivesAt(fieldDefinition.directives, INPUT_FIELD_DEFINITION, "input field '${definition.name}.${fieldDefinition.name}'")
                                 .build()
                         )
                     }
@@ -224,8 +221,7 @@
             .definition(definition)
             .extensionDefinitions(extensions)
             .description(getDocumentation(definition, options))
-            .withAppliedDirectives(*buildAppliedDirectives(directives))
-            .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.ENUM))
+            .withDirectivesAt(directives, Introspection.DirectiveLocation.ENUM, "type '$name'")
             .apply {
                 values.distinctBy { it.name }.forEach { valueDefinition ->
                     val enumName = valueDefinition.name
@@ -242,8 +238,7 @@
                             .description(getDocumentation(valueDefinition, options))
                             .value(enumValue)
                             .apply { getDeprecated(valueDefinition.directives)?.let { deprecationReason(it) } }
-                            .withAppliedDirectives(*buildAppliedDirectives(valueDefinition.directives))
-                            .withDirectives(*buildDirectives(valueDefinition.directives, Introspection.DirectiveLocation.ENUM_VALUE))
+                            .withDirectivesAt(valueDefinition.directives, Introspection.DirectiveLocation.ENUM_VALUE, "enum value '$name.$enumName'")
                             .definition(valueDefinition)
                             .build()
                     )
@@ -263,11 +258,10 @@
             .definition(interfaceDefinition)
             .extensionDefinitions(extensions)
             .description(getDocumentation(interfaceDefinition, options))
-            .withAppliedDirectives(*buildAppliedDirectives(directives))
-            .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.INTERFACE))
+            .withDirectivesAt(directives, Introspection.DirectiveLocation.INTERFACE, "type '${interfaceDefinition.name}'")
             .apply {
                 fieldDefinitions.distinctBy { it.name }.forEach { fieldDefinition ->
-                    field { field -> createField(field, fieldDefinition, inputObjects) }
+                    field { field -> createField(field, interfaceDefinition.name, fieldDefinition, inputObjects) }
                 }
             }
             .apply {
@@ -289,8 +283,7 @@
             .definition(definition)
             .extensionDefinitions(extensions)
             .description(getDocumentation(definition, options))
-            .withAppliedDirectives(*buildAppliedDirectives(directives))
-            .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.UNION))
+            .withDirectivesAt(directives, Introspection.DirectiveLocation.UNION, "type '${definition.name}'")
             .apply {
                 getLeafUnionObjects(definition, types).forEach { possibleType(it) }
             }
@@ -338,32 +331,31 @@
     private fun isNonRepeatable(directiveName: String): Boolean =
         directiveDefinitions.find { it.name == directiveName }?.let { !it.isRepeatable } ?: BUILT_IN_DIRECTIVES.containsKey(directiveName)
 
-    private fun createField(field: GraphQLFieldDefinition.Builder, fieldDefinition: FieldDefinition, inputObjects: List<GraphQLInputObjectType>): GraphQLFieldDefinition.Builder {
+    private fun createField(field: GraphQLFieldDefinition.Builder, typeName: String, fieldDefinition: FieldDefinition, inputObjects: List<GraphQLInputObjectType>): GraphQLFieldDefinition.Builder {
+        val element = "field '$typeName.${fieldDefinition.name}'"
         return field
             .name(fieldDefinition.name)
             .description(getDocumentation(fieldDefinition, options))
             .definition(fieldDefinition)
             .apply { getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } }
             .type(determineOutputType(fieldDefinition.type, inputObjects))
-            .withAppliedDirectives(*buildAppliedDirectives(fieldDefinition.directives))
-            .withDirectives(*buildDirectives(fieldDefinition.directives, Introspection.DirectiveLocation.FIELD_DEFINITION))
+            .withDirectivesAt(fieldDefinition.directives, Introspection.DirectiveLocation.FIELD_DEFINITION, element)
             .apply {
                 fieldDefinition.inputValueDefinitions.forEach { argumentDefinition ->
-                    argument(createArgument(argumentDefinition, inputObjects))
+                    argument(createArgument(argumentDefinition, element, inputObjects))
                 }
             }
     }
 
-    private fun createArgument(definition: InputValueDefinition, inputObjects: List<GraphQLInputObjectType>): GraphQLArgument {
+    private fun createArgument(definition: InputValueDefinition, owner: String, inputObjects: List<GraphQLInputObjectType>): GraphQLArgument {
         return GraphQLArgument.newArgument()
             .name(definition.name)
             .definition(definition)
             .description(getDocumentation(definition, options))
             .type(determineInputType(definition.type, inputObjects, mutableSetOf()))
             .apply { getDeprecated(definition.directives)?.let { deprecate(it) } }
             .apply { definition.defaultValue?.let { defaultValueLiteral(it) } }
-            .withAppliedDirectives(*buildAppliedDirectives(definition.directives))
-            .withDirectives(*buildDirectives(definition.directives, Introspection.DirectiveLocation.ARGUMENT_DEFINITION))
+            .withDirectivesAt(definition.directives, Introspection.DirectiveLocation.ARGUMENT_DEFINITION, "argument '${definition.name}' of $owner")
             .build()
     }
 
@@ -373,8 +365,7 @@
         // because the arguments can have directives too, we attach them only after the directives themselves are created
         schemaDirectives.update { d ->
             val arguments = d.arguments.map { a -> a.transform {
-                it.withAppliedDirectives(*buildAppliedDirectives(a.definition!!.directives))
-                    .withDirectives(*buildDirectives(a.definition!!.directives, Introspection.DirectiveLocation.OBJECT))
+                it.withDirectivesAt(a.definition!!.directives, Introspection.DirectiveLocation.ARGUMENT_DEFINITION, "argument '${a.name}' of directive '@${d.name}'")
             } }
             d.transform { it.replaceArguments(arguments) }
         }
@@ -435,10 +426,20 @@
             .build()
     }
 
-    private fun buildAppliedDirectives(directives: List<Directive>): Array<GraphQLAppliedDirective> {
+    // element is how warnings name the schema element, e.g. "field 'Query.name'"
+    @Suppress("DEPRECATION") // legacy directives, see buildDirectives
+    private fun <B : GraphqlDirectivesContainerTypeBuilder<B, *>> B.withDirectivesAt(
+        directives: List<Directive>,
+        location: Introspection.DirectiveLocation,
+        element: String
+    ): B = withAppliedDirectives(*buildAppliedDirectives(directives, location, element))
+        .withDirectives(*buildDirectives(directives, location))
+
+    private fun buildAppliedDirectives(directives: List<Directive>, location: Introspection.DirectiveLocation, element: String): Array<GraphQLAppliedDirective> {
+        // TODO throw a SchemaError instead in 15.0.0 (#884), like graphql-java does
+        directiveProblems(directives, location, element).forEach { log.warn(it) }
         return directives.map { directive ->
-            val graphQLDirective = schemaDirectives.find(directive.name)
-                ?: BUILT_IN_DIRECTIVES[directive.name]
+            val graphQLDirective = findDirective(directive.name)
                 ?: if (options.allowUndeclaredDirectives) return@map buildUndeclaredAppliedDirective(directive)
                 else throw SchemaError("Found applied directive ${directive.name} without corresponding directive definition.")
             val graphQLArguments = graphQLDirective.arguments.associateBy { it.name }
@@ -479,6 +480,27 @@
         }.toTypedArray()
     }
 
+    private fun directiveProblems(directives: List<Directive>, location: Introspection.DirectiveLocation, element: String): List<String> =
+        directives.groupBy { it.name }.flatMap { (name, applied) ->
+            // undeclared directives have no definition to check against
+            val definition = findDirective(name) ?: return@flatMap emptyList()
+            buildList {
+                if (location !in definition.validLocations()) {
+                    add("Directive @$name is applied to $element, but it's declared on ${definition.validLocations().joinToString(" | ")}, not $location.")
+                }
+                if (definition.isNonRepeatable && applied.size > 1) {
+                    add("Directive @$name isn't repeatable but is applied more than once to $element.")
+                }
+                applied.forEach { directive ->
+                    definition.arguments
+                        .filter { it.type is GraphQLNonNull && !it.hasSetDefaultValue() && directive.getArgument(it.name) == null }
+                        .forEach { add("Directive @$name is applied to $element without its required argument '${it.name}'.") }
+                }
+            }
+        }
+
+    private fun findDirective(name: String): GraphQLDirective? = schemaDirectives.find(name) ?: BUILT_IN_DIRECTIVES[name]
+
     private fun buildUndeclaredAppliedDirective(directive: Directive): GraphQLAppliedDirective {
         return GraphQLAppliedDirective.newDirective()
             .name(directive.name)
@@ -522,8 +544,7 @@
             val repeatable = directiveDefinitions.find { it.name.equals(directive.name) }?.isRepeatable ?: false
             if (repeatable || !names.contains(directive.name)) {
                 names.add(directive.name)
-                val graphQLDirective = schemaDirectives.find(directive.name)
-                    ?: BUILT_IN_DIRECTIVES[directive.name]
+                val graphQLDirective = findDirective(directive.name)
                     // graphql-java rejects legacy directives without a definition, so undeclared ones are only kept as applied directives
                     ?: if (options.allowUndeclaredDirectives) continue
                     else throw SchemaError("Found applied directive ${directive.name} without corresponding directive definition.")

src/test/kotlin/graphql/kickstart/tools/DirectiveTest.kt

@@ -283,7 +283,7 @@ class DirectiveTest {
         val schema = SchemaParser.newParser()
             .schemaString(
                 """
-                directive @uppercase on OBJECT
+                directive @uppercase on FIELD_DEFINITION
 
                 type Query {
                     name: String @uppercase
@@ -388,37 +388,40 @@ class DirectiveTest {
     @Test
     fun `should fill in default values of directive arguments that weren't supplied`() {
         val emailDirective = EmailDirective()
-        val schema = SchemaParser.newParser()
-            .schemaString(
-                """
-                directive @email(message: String = "{path} must be a valid email") on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION
-                directive @owner(team: String = "books-team") on SCHEMA | ENUM_VALUE
+        val (schema, warnings) = withWarnings {
+            SchemaParser.newParser()
+                .schemaString(
+                    """
+                    directive @email(message: String = "{path} must be a valid email") on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION
+                    directive @owner(team: String = "books-team") on SCHEMA | ENUM_VALUE
 
-                schema @owner {
-                    query: Query
-                }
+                    schema @owner {
+                        query: Query
+                    }
 
-                enum AllowedState {
-                    ALLOWED @owner
-                    DISALLOWED
-                }
+                    enum AllowedState {
+                        ALLOWED @owner
+                        DISALLOWED
+                    }
 
-                input PersonInput {
-                    email: String @email
-                }
+                    input PersonInput {
+                        email: String @email
+                    }
 
-                type Query {
-                    contactEmail: String @email
-                    updatePersonEmail(primaryEmail: String @email, backupEmail: String @email(message: "invalid backup email")): String
-                    updatePerson(person: PersonInput, state: AllowedState): String
-                }
-                """)
-            .resolvers(PersonQueryResolver())
-            .directive("email", emailDirective)
-            .build()
-            .makeExecutableSchema()
-            .assertEveryFieldResolves()
+                    type Query {
+                        contactEmail: String @email
+                        updatePersonEmail(primaryEmail: String @email, backupEmail: String @email(message: "invalid backup email")): String
+                        updatePerson(person: PersonInput, state: AllowedState): String
+                    }
+                    """)
+                .resolvers(PersonQueryResolver())
+                .directive("email", emailDirective)
+                .build()
+                .makeExecutableSchema()
+                .assertEveryFieldResolves()
+        }
 
+        assertEquals(warnings, emptyList())
         val expectedMessages = mapOf(
             "contactEmail" to "{path} must be a valid email",
             "primaryEmail" to "{path} must be a valid email",
@@ -434,6 +437,49 @@ class DirectiveTest {
         assertEquals(enumValue.getAppliedDirective("owner").getArgument("team")?.getValue<String>(), "books-team")
     }
 
+    @Test
+    fun `should warn about invalid applied directives and keep them`() {
+        val (schema, warnings) = withWarnings {
+            SchemaParser.newParser()
+                .schemaString(
+                    """
+                    directive @entity on OBJECT
+                    directive @meta(info: String!, level: Int! = 1) on SCHEMA | FIELD_DEFINITION | ARGUMENT_DEFINITION
+                    directive @limit(max: Int @meta) on FIELD_DEFINITION
+
+                    schema @meta(info: "a") {
+                        query: Query
+                    }
+
+                    extend schema @meta(info: "b")
+
+                    type Query {
+                        books: [Book!] @entity @meta
+                    }
+
+                    type Book @deprecated {
+                        id: Int!
+                        name: String! @limit @limit(max: 10)
+                    }
+                    """)
+                .resolvers(QueryResolver())
+                .build()
+                .makeExecutableSchema()
+                .assertEveryFieldResolves()
+        }
+
+        assertEquals(warnings, listOf(
+            "Directive @meta is applied to argument 'max' of directive '@limit' without its required argument 'info'.",
+            "Directive @deprecated is applied to type 'Book', but it's declared on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION, not OBJECT.",
+            "Directive @limit isn't repeatable but is applied more than once to field 'Book.name'.",
+            "Directive @entity is applied to field 'Query.books', but it's declared on OBJECT, not FIELD_DEFINITION.",
+            "Directive @meta is applied to field 'Query.books' without its required argument 'info'.",
+            "Directive @meta isn't repeatable but is applied more than once to the schema."
+        ))
+        assertEquals(schema.getObjectType("Book")!!.getField("name").getAppliedDirectives("limit").size, 2)
+        assertEquals(schema.getSchemaAppliedDirectives("meta").size, 2)
+    }
+
     @Test
     fun `should build input objects used by directive arguments before wiring`() {
         val meta = "directive @meta(info: ConfigInput!) on FIELD_DEFINITION"

src/test/resources/RelayConnection.graphqls

@@ -1,4 +1,4 @@
-directive @connection(for: String!) on FIELD
+directive @connection(for: String!) on FIELD_DEFINITION
 
 type Query {
     users(first: Int, after: String): UserRelayConnection @connection(for: "User")