diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index f99a1413..002eb3cc 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -3,7 +3,16 @@ package graphql.kickstart.tools import graphql.Directives import graphql.Scalars import graphql.introspection.Introspection +import graphql.introspection.Introspection.DirectiveLocation.ARGUMENT_DEFINITION +import graphql.introspection.Introspection.DirectiveLocation.ENUM +import graphql.introspection.Introspection.DirectiveLocation.ENUM_VALUE +import graphql.introspection.Introspection.DirectiveLocation.FIELD_DEFINITION import graphql.introspection.Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION +import graphql.introspection.Introspection.DirectiveLocation.INPUT_OBJECT +import graphql.introspection.Introspection.DirectiveLocation.INTERFACE +import graphql.introspection.Introspection.DirectiveLocation.OBJECT +import graphql.introspection.Introspection.DirectiveLocation.SCHEMA +import graphql.introspection.Introspection.DirectiveLocation.UNION import graphql.kickstart.tools.directive.DirectiveWiringHelper import graphql.kickstart.tools.resolver.MethodFieldResolver import graphql.kickstart.tools.util.extensionsOf @@ -118,7 +127,7 @@ class SchemaParser internal constructor( val types = (additionalObjects.toSet() as Set) + 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(), SCHEMA).toList() return SchemaObjects(query, mutation, subscription, types, schemaDirectives.all, codeRegistryBuilder, rootInfo.getDescription(), schemaAppliedDirectives) } @@ -142,8 +151,8 @@ class SchemaParser internal constructor( .definition(objectDefinition) .extensionDefinitions(extensions) .description(getDocumentation(objectDefinition, options)) - .withAppliedDirectives(*buildAppliedDirectives(directives)) - .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.OBJECT)) + .withAppliedDirectives(*buildAppliedDirectives(directives, OBJECT)) + .withDirectives(*buildDirectives(directives, OBJECT)) .apply { objectDefinition.withExtensions(typeExtensions).flatMap { it.implements }.forEach { implementsDefinition -> val interfaceName = (implementsDefinition as TypeName).name @@ -184,8 +193,8 @@ class SchemaParser internal constructor( .definition(definition) .extensionDefinitions(extensionDefinitions) .description(getDocumentation(definition, options)) - .withAppliedDirectives(*buildAppliedDirectives(directives)) - .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.INPUT_OBJECT)) + .withAppliedDirectives(*buildAppliedDirectives(directives, INPUT_OBJECT)) + .withDirectives(*buildDirectives(directives, INPUT_OBJECT)) .apply { (extensionDefinitions + definition).forEach { typeDefinition -> typeDefinition.inputValueDefinitions.forEach { fieldDefinition -> @@ -197,7 +206,7 @@ class SchemaParser internal constructor( .apply { fieldDefinition.defaultValue?.let { v -> defaultValueLiteral(v) } } .apply { getDeprecated(fieldDefinition.directives)?.let { deprecate(it) } } .type(determineInputType(fieldDefinition.type, inputObjects, referencingInputObjects)) - .withAppliedDirectives(*buildAppliedDirectives(fieldDefinition.directives)) + .withAppliedDirectives(*buildAppliedDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION)) .withDirectives(*buildDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION)) .build() ) @@ -224,8 +233,8 @@ class SchemaParser internal constructor( .definition(definition) .extensionDefinitions(extensions) .description(getDocumentation(definition, options)) - .withAppliedDirectives(*buildAppliedDirectives(directives)) - .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.ENUM)) + .withAppliedDirectives(*buildAppliedDirectives(directives, ENUM)) + .withDirectives(*buildDirectives(directives, ENUM)) .apply { values.distinctBy { it.name }.forEach { valueDefinition -> val enumName = valueDefinition.name @@ -242,8 +251,8 @@ class SchemaParser internal constructor( .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)) + .withAppliedDirectives(*buildAppliedDirectives(valueDefinition.directives, ENUM_VALUE)) + .withDirectives(*buildDirectives(valueDefinition.directives, ENUM_VALUE)) .definition(valueDefinition) .build() ) @@ -263,8 +272,8 @@ class SchemaParser internal constructor( .definition(interfaceDefinition) .extensionDefinitions(extensions) .description(getDocumentation(interfaceDefinition, options)) - .withAppliedDirectives(*buildAppliedDirectives(directives)) - .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.INTERFACE)) + .withAppliedDirectives(*buildAppliedDirectives(directives, INTERFACE)) + .withDirectives(*buildDirectives(directives, INTERFACE)) .apply { fieldDefinitions.distinctBy { it.name }.forEach { fieldDefinition -> field { field -> createField(field, fieldDefinition, inputObjects) } @@ -289,8 +298,8 @@ class SchemaParser internal constructor( .definition(definition) .extensionDefinitions(extensions) .description(getDocumentation(definition, options)) - .withAppliedDirectives(*buildAppliedDirectives(directives)) - .withDirectives(*buildDirectives(directives, Introspection.DirectiveLocation.UNION)) + .withAppliedDirectives(*buildAppliedDirectives(directives, UNION)) + .withDirectives(*buildDirectives(directives, UNION)) .apply { getLeafUnionObjects(definition, types).forEach { possibleType(it) } } @@ -345,8 +354,8 @@ class SchemaParser internal constructor( .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)) + .withAppliedDirectives(*buildAppliedDirectives(fieldDefinition.directives, FIELD_DEFINITION)) + .withDirectives(*buildDirectives(fieldDefinition.directives, FIELD_DEFINITION)) .apply { fieldDefinition.inputValueDefinitions.forEach { argumentDefinition -> argument(createArgument(argumentDefinition, inputObjects)) @@ -362,8 +371,8 @@ class SchemaParser internal constructor( .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)) + .withAppliedDirectives(*buildAppliedDirectives(definition.directives, ARGUMENT_DEFINITION)) + .withDirectives(*buildDirectives(definition.directives, ARGUMENT_DEFINITION)) .build() } @@ -373,8 +382,8 @@ class SchemaParser internal constructor( // 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.withAppliedDirectives(*buildAppliedDirectives(a.definition!!.directives, ARGUMENT_DEFINITION)) + .withDirectives(*buildDirectives(a.definition!!.directives, ARGUMENT_DEFINITION)) } } d.transform { it.replaceArguments(arguments) } } @@ -435,10 +444,11 @@ class SchemaParser internal constructor( .build() } - private fun buildAppliedDirectives(directives: List): Array { + private fun buildAppliedDirectives(directives: List, location: Introspection.DirectiveLocation): Array { + // TODO throw a SchemaError instead in 15.0.0 (#884), like graphql-java does + directiveProblems(directives, location).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 +489,27 @@ class SchemaParser internal constructor( }.toTypedArray() } + private fun directiveProblems(directives: List, location: Introspection.DirectiveLocation): List = + 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${applied.first().sourceLocation.describe()} is applied to $location, but it's declared on ${definition.validLocations().joinToString(" | ")}.") + } + if (definition.isNonRepeatable && applied.size > 1) { + add("Directive @$name${applied[1].sourceLocation.describe()} isn't repeatable but is applied more than once.") + } + applied.forEach { directive -> + definition.arguments + .filter { it.type is GraphQLNonNull && !it.hasSetDefaultValue() && directive.getArgument(it.name) == null } + .forEach { add("Directive @$name${directive.sourceLocation.describe()} is missing 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 +553,7 @@ class SchemaParser internal constructor( 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.") @@ -670,10 +700,7 @@ class SchemaParser internal constructor( else -> "can't be used as ${if (isInput) "an input" else "an output"} type." } val usage = findUsage(typeName)?.let { " used by $it" }.orEmpty() - val location = typeName.sourceLocation - ?.let { " at line ${it.line}, column ${it.column}" + it.sourceName?.let { name -> " in $name" }.orEmpty() } - .orEmpty() - return SchemaError("Type '${typeName.name}'$usage$location $problem") + return SchemaError("Type '${typeName.name}'$usage${typeName.sourceLocation.describe()} $problem") } // what makes the scanner reach the type depends on its kind, see SchemaClassScanner.canIncludeUnusedType @@ -737,3 +764,6 @@ private val BUILT_IN_DIRECTIVES = listOf( ).associateBy { it.name } const val DEFAULT_DEPRECATION_MESSAGE = "No longer supported" + +private fun SourceLocation?.describe(): String = + this?.let { " at line $line, column $column" + sourceName?.let { name -> " in $name" }.orEmpty() }.orEmpty() diff --git a/src/test/kotlin/graphql/kickstart/tools/DirectiveTest.kt b/src/test/kotlin/graphql/kickstart/tools/DirectiveTest.kt index 39f4fe4b..a3367c0d 100644 --- a/src/test/kotlin/graphql/kickstart/tools/DirectiveTest.kt +++ b/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(), "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) + } + """, "directives.graphqls") + .resolvers(QueryResolver()) + .build() + .makeExecutableSchema() + .assertEveryFieldResolves() + } + + assertEquals(warnings, listOf( + "Directive @meta at line 4, column 47 in directives.graphqls is missing its required argument 'info'.", + "Directive @deprecated at line 16, column 31 in directives.graphqls is applied to OBJECT, but it's declared on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION.", + "Directive @limit at line 18, column 46 in directives.graphqls isn't repeatable but is applied more than once.", + "Directive @entity at line 13, column 40 in directives.graphqls is applied to FIELD_DEFINITION, but it's declared on OBJECT.", + "Directive @meta at line 13, column 48 in directives.graphqls is missing its required argument 'info'.", + "Directive @meta at line 10, column 35 in directives.graphqls isn't repeatable but is applied more than once." + )) + 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" diff --git a/src/test/resources/RelayConnection.graphqls b/src/test/resources/RelayConnection.graphqls index e08d5099..6ba1c3eb 100644 --- a/src/test/resources/RelayConnection.graphqls +++ b/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")