Throw at build for resolvers that can't be called on their JDK dynamic proxy

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<ObjectTypeDefinition, MutableMap<FieldDefinition, FieldResolver>>()
+    private val notCallableOnResolver = mutableListOf<NotCallableOnResolver>()
 
     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,

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,17 +412,132 @@ 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<Any?> { 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(
                 """
                 type Query {
                     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<String> = listOf()
+    open class ProxiedResolver : GraphQLQueryResolver {
+        open fun test(): List<String> = 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 {