diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt index d787ef6d..4b2e3bc5 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt @@ -34,7 +34,11 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { val searches = resolverInfo.getFieldSearches() val scanProperties = field.inputValueDefinitions.isEmpty() - val found = searches.mapNotNull { search -> findFieldResolver(field, search, scanProperties) } + val found = searches.asSequence() + .mapNotNull { search -> findFieldResolver(field, search, scanProperties) } + // only root fields must match a single resolver. Other fields use the first match, so the methods it shadows, + // e.g. a supertype resolver's, are never called and aren't checked + .let { if (resolverInfo is RootResolverInfo) it.toList() else it.take(1).toList() } if (resolverInfo is RootResolverInfo && found.size > 1) { throw FieldResolverError("Found more than one matching resolver for field '$field': $found") @@ -46,7 +50,7 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { private fun findFieldResolver(field: FieldDefinition, search: Search, scanProperties: Boolean): FieldResolver? { val method = findResolverMethod(field, search) if (method != null) { - return MethodFieldResolver(field, search, options, method.apply(trySetAccessible(field, search.type))) + return MethodFieldResolver(field, search, options, method.apply(trySetAccessible(field, search.type)), findInvocableMethod(field, search, method)) } if (scanProperties) { @@ -63,6 +67,45 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { return null } + /** + * A resolver behind a JDK dynamic proxy, e.g. Spring AOP without proxyTargetClass, is scanned as its target class, but the + * proxy only implements the target's interfaces. So its methods are called through the interface method they implement, + * which also runs the proxy's interceptors. + */ + private fun findInvocableMethod(field: FieldDefinition, search: Search, method: Method): Method { + val source = search.source + if (source == null || Modifier.isStatic(method.modifiers) || method.declaringClass.isInstance(source)) { + return method + } + + val type = search.type.unwrap() + return ClassUtils.getAllInterfaces(type) + .filter { it.isInstance(source) } + .flatMap { it.methods.asList() } + .find { !Modifier.isStatic(it.modifiers) && method.implements(it, type) } + ?.apply(trySetAccessible(field, search.type)) + ?: throw FieldResolverError( + "Resolver method '${method.declaringClass.name}.${method.name}' for field '${field.name}' as defined in schema ${getSourceLocation(field)} " + + "can't be called on ${source.javaClass.name}, which is not an instance of ${method.declaringClass.name}. " + + "This happens with JDK dynamic proxies, which only implement interfaces. " + + "Declare the method on an interface the resolver implements, or use class-based proxies (e.g. proxyTargetClass = true in Spring)." + ) + } + + // the implementation of a generic interface method takes the type arguments, e.g. echo(String) for echo(T) of Api, + // and the compiler bridges the erased echo(Object) to it. Kotlin also unboxes them, e.g. increment(int) for Api. + // A method inherited from a generic superclass is erased itself, e.g. getValue(Object) for getValue(T), so both sides are + // resolved against the target class + private fun Method.implements(interfaceMethod: Method, type: Class<*>): Boolean = + name == interfaceMethod.name + && parameterCount == interfaceMethod.parameterCount + && parameterTypes.indices.all { i -> + val interfaceParameterType = interfaceMethod.genericParameterTypes[i] + val resolvedInterfaceType = TypeUtils.getRawType(interfaceParameterType, type) ?: interfaceMethod.parameterTypes[i] + val resolvedType = TypeUtils.getRawType(genericParameterTypes[i], type) ?: parameterTypes[i] + resolvedInterfaceType == if (interfaceParameterType is TypeVariable<*>) ClassUtils.primitiveToWrapper(resolvedType) else resolvedType + } + private fun trySetAccessible(field: FieldDefinition, type: JavaType): AccessibleObject.() -> Unit = { try { isAccessible = true @@ -200,14 +243,16 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { isSubscription = isSubscription || search.isSubscription } - val sourceName = field.sourceLocation?.sourceName ?: "" - val sourceLocation = field.sourceLocation?.let { "$sourceName:${it.line}" } ?: "" - - return "No method${if (scannedProperties) " or field" else ""} found as defined in schema $sourceLocation with any of the following signatures " + + return "No method${if (scannedProperties) " or field" else ""} found as defined in schema ${getSourceLocation(field)} with any of the following signatures " + "(with or without one of $allowedLastArgumentTypes as the last argument), in priority order:\n${signatures.joinToString("\n ")}" + if (isSubscription) "\n\nNote that a Subscription data fetcher must return a Publisher of events" else "" } + private fun getSourceLocation(field: FieldDefinition): String { + val sourceName = field.sourceLocation?.sourceName ?: "" + return field.sourceLocation?.let { "$sourceName:${it.line}" } ?: "" + } + private fun getMissingMethodSignatures(field: FieldDefinition, search: Search, isBoolean: Boolean, scannedProperties: Boolean): List { val baseType = search.type.unwrap() val signatures = mutableListOf() diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index 0a9e65d6..71063ffe 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -40,7 +40,9 @@ internal class MethodFieldResolver( field: FieldDefinition, search: FieldResolverScanner.Search, options: SchemaParserOptions, - val method: Method + val method: Method, + // the method that's called, which is the interface method that [method] implements if the resolver is a JDK dynamic proxy + private val invocableMethod: Method ) : FieldResolver(field, search, options) { private val log = LoggerFactory.getLogger(javaClass) @@ -136,10 +138,10 @@ internal class MethodFieldResolver( return if (numberOfParameters > 0 || isSuspendFunction) { // requires arguments and environment or is a suspend function - MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options, isSuspendFunction, valueClass) + MethodFieldResolverDataFetcher(createSourceResolver(), invocableMethod, args, options, isSuspendFunction, valueClass) } else { // if there are no parameters an optimized version of the data fetcher can be used - LightMethodFieldResolverDataFetcher(createSourceResolver(), this.method, options, valueClass) + LightMethodFieldResolverDataFetcher(createSourceResolver(), invocableMethod, options, valueClass) } } diff --git a/src/test/kotlin/graphql/kickstart/tools/ProxiedResolverTest.kt b/src/test/kotlin/graphql/kickstart/tools/ProxiedResolverTest.kt new file mode 100644 index 00000000..3c11fd1b --- /dev/null +++ b/src/test/kotlin/graphql/kickstart/tools/ProxiedResolverTest.kt @@ -0,0 +1,471 @@ +package graphql.kickstart.tools + +import graphql.ExceptionWhileDataFetching +import graphql.GraphQL +import graphql.kickstart.tools.resolver.FieldResolverError +import graphql.schema.DataFetchingEnvironment +import kotlinx.coroutines.delay +import org.aopalliance.intercept.MethodInterceptor +import org.junit.Assert.assertThrows +import org.junit.Test +import org.springframework.aop.framework.ProxyFactory +import java.lang.reflect.Proxy + +class ProxiedResolverTest { + + private val invocations = mutableListOf() + + // stands in for the interceptors Spring adds to a proxy, e.g. for @PreAuthorize or @Transactional + private val recordingInterceptor = MethodInterceptor { invocation -> + invocations.add("${invocation.method.declaringClass.simpleName}.${invocation.method.name}") + invocation.proceed() + } + + private val querySchema = """ + type Query { + test: String + greeting(name: String!): String + suspendTest: String + } + """ + + @Test + fun `JDK proxied resolver methods are called through the interface the proxy implements`() { + val gql = graphQL(querySchema, jdkProxy(Query())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + test + } + """ + } + + assertEquals(data, mapOf("test" to "ok")) + assertEquals(invocations, listOf("QueryApi.test")) + } + + @Test + fun `JDK proxied resolver methods with arguments are called through the interface the proxy implements`() { + val gql = graphQL(querySchema, jdkProxy(Query())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + greeting(name: "Rex") + } + """ + } + + assertEquals(data, mapOf("greeting" to "Hello, Rex, from greeting")) + assertEquals(invocations, listOf("QueryApi.greeting")) + } + + @Test + fun `JDK proxied resolver suspend functions are called through the interface the proxy implements`() { + val gql = graphQL(querySchema, jdkProxy(Query())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + suspendTest + } + """ + } + + assertEquals(data, mapOf("suspendTest" to "ok")) + assertEquals(invocations, listOf("QueryApi.suspendTest")) + } + + @Test + fun `JDK proxied resolver interceptors can reject the call`() { + val gql = graphQL(querySchema, jdkProxy(Query()) { throw IllegalStateException("Access denied") }) + + val result = gql.execute( + """ + query { + test + } + """) + + assertEquals(result.getData(), mapOf("test" to null)) + assertEquals(result.errors.map { (it as ExceptionWhileDataFetching).exception.message }, listOf("Access denied")) + } + + interface QueryApi { + fun test(): String + fun greeting(name: String, env: DataFetchingEnvironment): String + suspend fun suspendTest(): String + } + + class Query : GraphQLQueryResolver, QueryApi { + override fun test() = "ok" + override fun greeting(name: String, env: DataFetchingEnvironment) = "Hello, $name, from ${env.field.name}" + override suspend fun suspendTest(): String { + // suspends, so the result reaches graphql-java through the continuation instead of the proxy's return value + delay(1) + return "ok" + } + } + + private val petSchema = """ + type Query { + pet: Pet + } + + type Pet { + name: String + greeting: String + } + """ + + @Test + fun `JDK proxied field resolver methods are called through the interface the proxy implements`() { + val gql = graphQL(petSchema, PetQuery(), jdkProxy(PetResolver())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + pet { + name + greeting + } + } + """ + } + + assertEquals(data, mapOf("pet" to mapOf("name" to "Rex", "greeting" to "Hello, Rex"))) + assertEquals(invocations, listOf("GreetingApi.greeting")) + } + + @Test + fun `JDK proxied supertype resolver methods are called through the interface the proxy implements`() { + val gql = graphQL(petSchema, PetQuery(), jdkProxy(NamedResolver())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + pet { + greeting + } + } + """ + } + + assertEquals(data, mapOf("pet" to mapOf("greeting" to "Hello, Rex"))) + assertEquals(invocations, listOf("NamedGreetingApi.greeting")) + } + + @Test + fun `parser ignores JDK proxied supertype resolver methods when a more specific resolver takes precedence`() { + val gql = graphQL(petSchema, PetQuery(), PetResolver(), jdkProxy(ClassOnlyNamedResolver())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + pet { + greeting + } + } + """ + } + + assertEquals(data, mapOf("pet" to mapOf("greeting" to "Hello, Rex"))) + } + + interface Named { + val name: String + } + + class Pet(override val name: String) : Named + + class PetQuery : GraphQLQueryResolver { + fun pet() = Pet("Rex") + } + + interface GreetingApi { + fun greeting(pet: Pet): String + } + + class PetResolver : GraphQLResolver, GreetingApi { + override fun greeting(pet: Pet) = "Hello, ${pet.name}" + } + + interface NamedGreetingApi { + fun greeting(named: Named): String + } + + class NamedResolver : GraphQLResolver, NamedGreetingApi { + override fun greeting(named: Named) = "Hello, ${named.name}" + } + + class ClassOnlyNamedResolver : GraphQLResolver { + fun greeting(named: Named) = "Hi, ${named.name}" + } + + @Test + fun `JDK proxied resolver methods implementing a generic interface are called through the interface the proxy implements`() { + val gql = graphQL( + """ + type Query { + echo(value: String!): String + increment(value: Int!): Int + } + """, + jdkProxy(GenericQuery())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + echo(value: "ok") + increment(value: 1) + } + """ + } + + assertEquals(data, mapOf("echo" to "ok", "increment" to 2)) + assertEquals(invocations, listOf("EchoApi.echo", "IncrementApi.increment")) + } + + interface EchoApi { + fun echo(value: T): T + } + + interface IncrementApi { + fun increment(value: T): T + } + + // compiled to echo(String) and increment(int), which the interfaces' echo(Object) and increment(Object) are bridged to + class GenericQuery : GraphQLQueryResolver, EchoApi, IncrementApi { + override fun echo(value: String) = value + override fun increment(value: Int) = value + 1 + } + + @Test + fun `JDK proxied field resolver methods inherited from a generic superclass are called through the interface the proxy implements`() { + val gql = graphQL(petSchema, PetQuery(), jdkProxy(InheritedPetResolver())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + pet { + greeting + } + } + """ + } + + assertEquals(data, mapOf("pet" to mapOf("greeting" to "Hello, Rex"))) + assertEquals(invocations, listOf("GenericGreetingApi.greeting")) + } + + interface GenericGreetingApi { + fun greeting(pet: T): String + } + + abstract class GenericGreetingResolver : GraphQLResolver, GenericGreetingApi { + override fun greeting(pet: T) = "Hello, ${pet.name}" + } + + // inherits the erased greeting(Named) + class InheritedPetResolver : GenericGreetingResolver() + + @Test + fun `JDK proxied field resolver methods inherited from a generic superclass are called through a non-generic interface the proxy implements`() { + val gql = graphQL(petSchema, PetQuery(), jdkProxy(MixedPetResolver())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + pet { + greeting + } + } + """ + } + + assertEquals(data, mapOf("pet" to mapOf("greeting" to "Hello, Rex"))) + assertEquals(invocations, listOf("GreetingApi.greeting")) + } + + abstract class PlainGreetingResolver : GraphQLResolver { + fun greeting(pet: T) = "Hello, ${pet.name}" + } + + // the inherited greeting(Named) implements GreetingApi.greeting(Pet) through a synthetic bridge, which isn't scanned + class MixedPetResolver : PlainGreetingResolver(), GreetingApi + + @Test + fun `JDK proxied resolver methods declared on a non-public interface are called through the interface the proxy implements`() { + val gql = graphQL( + """ + type Query { + test: String + } + """, + jdkProxy(PrivateInterfaceQuery())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + test + } + """ + } + + assertEquals(data, mapOf("test" to "ok")) + assertEquals(invocations, listOf("PrivateQueryApi.test")) + } + + private interface PrivateQueryApi { + fun test(): String + } + + private class PrivateInterfaceQuery : GraphQLQueryResolver, PrivateQueryApi { + override fun test() = "ok" + } + + @Test + fun `JDK proxied resolver static methods are called directly`() { + val gql = graphQL( + """ + type Query { + test: String + } + """, + jdkProxy(StaticQuery())) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + test + } + """ + } + + assertEquals(data, mapOf("test" to "static")) + } + + class StaticQuery : GraphQLQueryResolver { + companion object { + @JvmStatic + fun test() = "static" + } + } + + @Test + fun `parser should throw descriptive exception when a JDK proxied resolver method isn't declared on an interface`() { + val resolver = jdkProxy(ClassOnlyQuery()) + + val error = assertThrows(FieldResolverError::class.java) { + parse( + """ + type Query { + test: String + } + """, + resolver) + } + + val queryClass = ClassOnlyQuery::class.java.name + assertEquals(error.message, "Resolver method '$queryClass.test' for field 'test' as defined in schema :3 can't be called on " + + "${resolver.javaClass.name}, which is not an instance of $queryClass. This happens with JDK dynamic proxies, which only implement " + + "interfaces. Declare the method on an interface the resolver implements, or use class-based proxies (e.g. proxyTargetClass = true in Spring).") + } + + class ClassOnlyQuery : GraphQLQueryResolver { + fun test() = "ok" + } + + @Test + fun `parser should throw descriptive exception when a JDK proxied resolver method is only declared on an interface the proxy doesn't implement`() { + // the target implements QueryApi, but the proxy only exposes the interfaces it's given + val resolver = ProxyFactory().apply { + setTarget(Query()) + setInterfaces(GraphQLQueryResolver::class.java) + }.proxy as GraphQLQueryResolver + + val error = assertThrows(FieldResolverError::class.java) { + parse( + """ + type Query { + test: String + } + """, + resolver) + } + + assertEquals(error.message?.substringBefore(" as defined"), "Resolver method '${Query::class.java.name}.test' for field 'test'") + } + + @Test + fun `parser should throw descriptive exception when a JDK proxied resolver method is only declared as a static interface method`() { + val error = assertThrows(FieldResolverError::class.java) { + parse( + """ + type Query { + test: String + } + """, + jdkProxy(StaticInterfaceQuery())) + } + + assertEquals(error.message?.substringBefore(" as defined"), "Resolver method '${StaticInterfaceQuery::class.java.name}.test' for field 'test'") + } + + interface StaticApi { + companion object { + @JvmStatic + fun test() = "static" + } + } + + // calling the static StaticApi.test() would skip both the proxy and the resolver's own test() + class StaticInterfaceQuery : GraphQLQueryResolver, StaticApi { + fun test() = "ok" + } + + @Test + fun `class based proxied resolver methods are called through the proxy`() { + val resolver = ProxyFactory(OpenQuery()).apply { + isProxyTargetClass = true + addAdvice(recordingInterceptor) + }.proxy as GraphQLQueryResolver + + val gql = graphQL( + """ + type Query { + test: String + } + """, + resolver) + + val data = assertNoGraphQlErrors(gql) { + """ + query { + test + } + """ + } + + assertEquals(data, mapOf("test" to "ok")) + assertEquals(invocations, listOf("OpenQuery.test")) + } + + // CGLIB can only intercept methods it can override + open class OpenQuery : GraphQLQueryResolver { + open fun test() = "ok" + } + + private fun jdkProxy(resolver: GraphQLResolver<*>, interceptor: MethodInterceptor = recordingInterceptor) = + (ProxyFactory(resolver).apply { addAdvice(interceptor) }.proxy as GraphQLResolver<*>) + .also { assert(Proxy.isProxyClass(it.javaClass)) } + + private fun graphQL(schema: String, vararg resolvers: GraphQLResolver<*>) = + GraphQL.newGraphQL(parse(schema, *resolvers).makeExecutableSchema()).build() + + private fun parse(schema: String, vararg resolvers: GraphQLResolver<*>) = SchemaParser.newParser() + .schemaString(schema) + .resolvers(*resolvers) + .build() +} diff --git a/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt b/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt index e5e2ba30..e4d7835f 100644 --- a/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt @@ -14,7 +14,6 @@ import org.junit.Before import org.junit.Test import org.reactivestreams.Publisher import org.reactivestreams.tck.TestEnvironment -import org.springframework.aop.framework.ProxyFactory import java.io.FileNotFoundException import java.util.concurrent.CompletableFuture import java.util.concurrent.CompletableFuture.completedFuture @@ -220,21 +219,6 @@ class SchemaParserTest { } } - @Test - fun `parser handles spring AOP proxied resolvers by default`() { - val resolver = ProxyFactory(ProxiedResolver()).proxy as GraphQLQueryResolver - - SchemaParser.newParser() - .schemaString( - """ - type Query { - test: [String] - } - """) - .resolvers(resolver) - .build() - } - @Test fun `parser handles enums with overridden toString method`() { SchemaParser.newParser() @@ -702,10 +686,6 @@ class SchemaParserTest { class RootObj - class ProxiedResolver : GraphQLQueryResolver { - fun test(): List = listOf() - } - enum class CustomEnum { FOO { override fun toString(): String {