diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index f99a1413..d8de056d 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -118,7 +118,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(), Introspection.DirectiveLocation.SCHEMA, "the schema").toList() return SchemaObjects(query, mutation, subscription, types, schemaDirectives.all, codeRegistryBuilder, rootInfo.getDescription(), schemaAppliedDirectives) } @@ -142,8 +142,7 @@ class SchemaParser internal constructor( .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 @@ class SchemaParser internal constructor( .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 @@ class SchemaParser internal constructor( .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 @@ 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)) - .withDirectives(*buildDirectives(fieldDefinition.directives, INPUT_FIELD_DEFINITION)) + .withDirectivesAt(fieldDefinition.directives, INPUT_FIELD_DEFINITION, "input field '${definition.name}.${fieldDefinition.name}'") .build() ) } @@ -224,8 +221,7 @@ class SchemaParser internal constructor( .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 @@ 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)) + .withDirectivesAt(valueDefinition.directives, Introspection.DirectiveLocation.ENUM_VALUE, "enum value '$name.$enumName'") .definition(valueDefinition) .build() ) @@ -263,11 +258,10 @@ class SchemaParser internal constructor( .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 @@ class SchemaParser internal constructor( .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,23 +331,23 @@ class SchemaParser internal constructor( 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): GraphQLFieldDefinition.Builder { + private fun createField(field: GraphQLFieldDefinition.Builder, typeName: String, fieldDefinition: FieldDefinition, inputObjects: List): 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): GraphQLArgument { + private fun createArgument(definition: InputValueDefinition, owner: String, inputObjects: List): GraphQLArgument { return GraphQLArgument.newArgument() .name(definition.name) .definition(definition) @@ -362,8 +355,7 @@ 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)) + .withDirectivesAt(definition.directives, Introspection.DirectiveLocation.ARGUMENT_DEFINITION, "argument '${definition.name}' of $owner") .build() } @@ -373,8 +365,7 @@ 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.withDirectivesAt(a.definition!!.directives, Introspection.DirectiveLocation.ARGUMENT_DEFINITION, "argument '${a.name}' of directive '@${d.name}'") } } d.transform { it.replaceArguments(arguments) } } @@ -435,10 +426,20 @@ class SchemaParser internal constructor( .build() } - private fun buildAppliedDirectives(directives: List): Array { + // element is how warnings name the schema element, e.g. "field 'Query.name'" + @Suppress("DEPRECATION") // legacy directives, see buildDirectives + private fun > B.withDirectivesAt( + directives: List, + location: Introspection.DirectiveLocation, + element: String + ): B = withAppliedDirectives(*buildAppliedDirectives(directives, location, element)) + .withDirectives(*buildDirectives(directives, location)) + + private fun buildAppliedDirectives(directives: List, location: Introspection.DirectiveLocation, element: String): Array { + // 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 @@ class SchemaParser internal constructor( }.toTypedArray() } + private fun directiveProblems(directives: List, location: Introspection.DirectiveLocation, element: String): 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 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 @@ 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.") diff --git a/src/test/kotlin/graphql/kickstart/tools/DirectiveTest.kt b/src/test/kotlin/graphql/kickstart/tools/DirectiveTest.kt index 39f4fe4b..d1c2c519 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) + } + """) + .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" 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")