diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt index 7012e5f4..5a63f30d 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt @@ -67,6 +67,7 @@ internal class SchemaClassScanner( private val sealedSubclassMatcher = SealedSubclassMatcher(dictionary, typeExtensions) private val fieldResolversByType = mutableMapOf>() + private val notCallableOnResolver = mutableListOf() init { initialDictionary.forEach { (name, clazz) -> @@ -146,6 +147,8 @@ internal class SchemaClassScanner( } } + throwIfNotCallableOnResolver() + return validateAndCreateResult(rootTypeHolder) } @@ -451,7 +454,7 @@ internal class SchemaClassScanner( private fun scanResolverInfoForPotentialMatches(type: ObjectTypeDefinition, resolverInfo: ResolverInfo) { type.withExtensions(typeExtensions).flatMap { it.fieldDefinitions }.forEach { field -> val fieldResolver = fieldResolverScanner.findFieldResolver(field, resolverInfo) - warnIfNotCallableOnResolver(type, fieldResolver) + collectIfNotCallableOnResolver(type, fieldResolver) fieldResolversByType.getOrPut(type) { mutableMapOf() }[fieldResolver.field] = fieldResolver @@ -462,8 +465,7 @@ internal class SchemaClassScanner( } } - // TODO throw a FieldResolverError instead in 15.0.0 (#868) - private fun warnIfNotCallableOnResolver(type: ObjectTypeDefinition, fieldResolver: FieldResolver) { + private fun collectIfNotCallableOnResolver(type: ObjectTypeDefinition, fieldResolver: FieldResolver) { val source = fieldResolver.search.source ?: return val member: Member = when (fieldResolver) { is MethodFieldResolver -> fieldResolver.method @@ -474,12 +476,22 @@ internal class SchemaClassScanner( return } - // class-based proxies don't delegate field reads to the target, so a field needs a getter too - val fix = if (member is Field) "Add a getter for the field and use class-based proxies" else "Use class-based proxies instead" - log.warn( + notCallableOnResolver += NotCallableOnResolver( "Field '${type.name}.${fieldResolver.field.name}' resolves to ${member.declaringClass.name}.${member.name}, but the resolver is a " + - "${source.javaClass.name}, which isn't a ${member.declaringClass.name}, so every query of the field will fail. This happens " + - "with JDK dynamic proxies, which only implement interfaces. $fix, e.g. proxyTargetClass = true in Spring." + "${source.javaClass.name}, which isn't a ${member.declaringClass.name}, so every query of the field will fail.", + member is Field + ) + } + + private fun throwIfNotCallableOnResolver() { + if (notCallableOnResolver.isEmpty()) { + return + } + + val fix = "Use class-based proxies instead, e.g. proxyTargetClass = true in Spring." + + if (notCallableOnResolver.any { it.isField }) " For a public field, also add a getter, since class-based proxies don't pass field reads to the target." else "" + throw FieldResolverError( + notCallableOnResolver.joinToString("\n", postfix = "\nThis happens with JDK dynamic proxies, which only implement interfaces. $fix") { it.message } ) } @@ -528,6 +540,8 @@ internal class SchemaClassScanner( private data class QueueItem(val type: ObjectTypeDefinition, val clazz: JavaType) + private data class NotCallableOnResolver(val message: String, val isField: Boolean) + private class RootTypesHolder( options: SchemaParserOptions, rootInfo: RootTypeInfo, diff --git a/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt b/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt index 49cea21f..61d1d457 100644 --- a/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt @@ -9,7 +9,9 @@ import graphql.schema.idl.SchemaDirectiveWiring import graphql.schema.idl.SchemaDirectiveWiringEnvironment import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.reactive.publish +import org.junit.Assert.assertFalse import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.reactivestreams.Publisher @@ -410,9 +412,124 @@ class SchemaParserTest { } @Test - fun `parser handles spring AOP proxied resolvers by default`() { + fun `parser rejects spring AOP proxied resolvers that are JDK dynamic proxies`() { val resolver = ProxyFactory(ProxiedResolver()).proxy as GraphQLQueryResolver + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: [String] + } + """) + .resolvers(resolver) + .build() + } + + assertTrue(error.message!!.contains("Field 'Query.test' resolves to ${ProxiedResolver::class.java.name}.test, but the resolver is a ")) + assertTrue(error.message!!.contains("Use class-based proxies instead")) + } + + @Test + fun `parser rejects JDK dynamic proxied resolvers even when unimplemented resolvers are allowed`() { + val resolver = ProxyFactory(ProxiedResolver()).proxy as GraphQLQueryResolver + + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: [String] + missing: String + } + """) + .resolvers(resolver) + .options(SchemaParserOptions.newOptions().allowUnimplementedResolvers(true).build()) + .build() + } + + assertTrue(error.message!!.contains("Field 'Query.test'")) + } + + @Test + fun `parser rejects JDK dynamic proxied resolvers even with a missing resolver data fetcher provider`() { + val resolver = ProxyFactory(ProxiedResolver()).proxy as GraphQLQueryResolver + + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: [String] + missing: String + } + """) + .resolvers(resolver) + .options( + SchemaParserOptions.newOptions() + .missingResolverDataFetcherProvider { _, _ -> DataFetcher { null } } + .build() + ) + .build() + } + + assertTrue(error.message!!.contains("Field 'Query.test'")) + } + + @Test + fun `parser names every field of every JDK dynamic proxied resolver in one error`() { + val query = ProxyFactory(ProxiedResolver()).proxy as GraphQLQueryResolver + val mutation = ProxyFactory(ProxiedMutationResolver()).proxy as GraphQLMutationResolver + + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: [String] + total: Int! + } + + type Mutation { + save: Boolean! + } + """) + .resolvers(query, mutation) + .build() + } + + val message = error.message!! + assertTrue(message, message.contains("Field 'Query.test'")) + assertTrue(message, message.contains("Field 'Query.total'")) + assertTrue(message, message.contains("Field 'Mutation.save'")) + assertFalse(message, message.contains("add a getter")) + } + + @Test + fun `parser suggests a getter when a JDK dynamic proxied resolver is read through a field`() { + val resolver = ProxyFactory(ProxiedFieldResolver()).proxy as GraphQLQueryResolver + + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + count: Int! + } + """) + .resolvers(resolver) + .build() + } + + assertTrue(error.message!!.contains("Field 'Query.count' resolves to ${ProxiedFieldResolver::class.java.name}.count")) + assertTrue(error.message!!.contains("add a getter")) + } + + @Test + fun `parser handles spring AOP proxied resolvers that are class-based proxies`() { + val factory = ProxyFactory(ProxiedResolver()).apply { setProxyTargetClass(true) } + SchemaParser.newParser() .schemaString( """ @@ -420,7 +537,7 @@ class SchemaParserTest { test: [String] } """) - .resolvers(resolver) + .resolvers(factory.proxy as GraphQLQueryResolver) .build() } @@ -1127,8 +1244,18 @@ class SchemaParserTest { data class Movie(val title: String) - class ProxiedResolver : GraphQLQueryResolver { - fun test(): List = listOf() + open class ProxiedResolver : GraphQLQueryResolver { + open fun test(): List = listOf() + open fun total(): Int = 2 + } + + open class ProxiedMutationResolver : GraphQLMutationResolver { + open fun save(): Boolean = true + } + + open class ProxiedFieldResolver : GraphQLQueryResolver { + @JvmField + val count: Int = 1 } enum class CustomEnum {