From 2b1bec70676d526395500e026a656bf0a4ad38b2 Mon Sep 17 00:00:00 2001 From: Oryan Date: Sat, 3 Oct 2026 13:50:46 -0400 Subject: [PATCH] Fix subscription resolver return type validation The Publisher return type check for subscriptions was applied whenever the resolver instance implemented GraphQLSubscriptionResolver, so a class that is also a query or mutation resolver lost all of its non-Publisher methods and failed with "No method found". The check and the missing method note now depend on the root type being scanned, which RootResolverInfo records. The checks were also inverted: they accepted supertypes of Publisher and CompletableFuture rather than subtypes, so Publisher implementations such as Flux were rejected. Accept Publisher subtypes, futures of them (Future or CompletionStage, with class, parameterized or wildcard type arguments) and ReceiveChannel subtypes that have a wrapper registered for that exact type. Methods erased to Object, such as suspend functions, are still accepted since their return type can't be checked here. Generic wrappers only unwrap their exact type, so subscription return types are now matched against the schema as a Publisher of their events. Otherwise a Flux return, or a covariant override of a Publisher method, would map the Item type to the Flux class. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../graphql/kickstart/tools/ResolverInfo.kt | 3 +- .../kickstart/tools/SchemaClassScanner.kt | 2 +- .../tools/resolver/FieldResolverScanner.kt | 27 +- .../tools/resolver/MethodFieldResolver.kt | 24 +- .../graphql/kickstart/tools/util/Utils.kt | 9 + .../kickstart/tools/SchemaParserTest.kt | 290 ++++++++++++++++++ 6 files changed, 333 insertions(+), 22 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/ResolverInfo.kt b/src/main/kotlin/graphql/kickstart/tools/ResolverInfo.kt index 267feb21..4d734af7 100644 --- a/src/main/kotlin/graphql/kickstart/tools/ResolverInfo.kt +++ b/src/main/kotlin/graphql/kickstart/tools/ResolverInfo.kt @@ -68,7 +68,8 @@ internal class MultiResolverInfo( internal class RootResolverInfo( val resolvers: List, - private val options: SchemaParserOptions + private val options: SchemaParserOptions, + val isSubscription: Boolean = false ) : ResolverInfo() { override fun getFieldSearches() = resolvers.map { FieldResolverScanner.Search(getRealResolverClass(it, options), this, it) } diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt index 8d678863..d2215367 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt @@ -543,7 +543,7 @@ internal class SchemaClassScanner( private val queryResolverInfo = RootResolverInfo(queryResolvers, options) private val mutationResolverInfo = RootResolverInfo(mutationResolvers, options) - private val subscriptionResolverInfo = RootResolverInfo(subscriptionResolvers, options) + private val subscriptionResolverInfo = RootResolverInfo(subscriptionResolvers, options, isSubscription = true) val query = createRootType("query", queryDefinition, queryName, true, queryResolvers, GraphQLQueryResolver::class.java, queryResolverInfo) val mutation = createRootType("mutation", mutationDefinition, mutationName, rootInfo.isMutationRequired(), mutationResolvers, GraphQLMutationResolver::class.java, mutationResolverInfo) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt index 6adaf414..8939bd00 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/FieldResolverScanner.kt @@ -2,7 +2,6 @@ package graphql.kickstart.tools.resolver import graphql.GraphQLContext import graphql.Scalars -import graphql.kickstart.tools.GraphQLSubscriptionResolver import graphql.kickstart.tools.ResolverInfo import graphql.kickstart.tools.RootResolverInfo import graphql.kickstart.tools.SchemaParserOptions @@ -13,10 +12,10 @@ import graphql.schema.DataFetchingEnvironment import kotlinx.coroutines.channels.ReceiveChannel import org.apache.commons.lang3.ClassUtils import org.apache.commons.lang3.reflect.FieldUtils +import org.apache.commons.lang3.reflect.TypeUtils import org.reactivestreams.Publisher import org.slf4j.LoggerFactory import java.lang.reflect.* -import java.util.concurrent.CompletableFuture import kotlin.reflect.full.valueParameters import kotlin.reflect.jvm.javaType import kotlin.reflect.jvm.kotlinFunction @@ -124,27 +123,27 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { // to avoid issues with duplicate method declarations .filter { it.declaringClass != Object::class.java } // subscription resolvers must return a publisher - .filter { search.source !is GraphQLSubscriptionResolver || resolverMethodReturnsPublisher(it) } + .filter { !search.isSubscription || resolverMethodReturnsPublisher(it) } .toList() } private fun resolverMethodReturnsPublisher(method: Method) = - method.returnType.isAssignableFrom(Publisher::class.java) + // suspend functions and unbounded type variables are erased to Object, so the actual return type is unknown here + method.returnType == Any::class.java + || Publisher::class.java.isAssignableFrom(method.returnType) || resolverMethodReturnsPublisherFuture(method) || receiveChannelToPublisherWrapper(method) private fun resolverMethodReturnsPublisherFuture(method: Method) = - method.returnType.isAssignableFrom(CompletableFuture::class.java) - && method.genericReturnType is ParameterizedType - && (method.genericReturnType as ParameterizedType).actualTypeArguments - .any { - it is ParameterizedType && it.unwrap().isAssignableFrom(Publisher::class.java) - } + method.genericReturnType.futureValueType() + ?.let { if (it is WildcardType) it.upperBounds.first() else it } + ?.let { TypeUtils.isAssignable(it, Publisher::class.java) } == true private fun receiveChannelToPublisherWrapper(method: Method) = - method.returnType.isAssignableFrom(ReceiveChannel::class.java) + ReceiveChannel::class.java.isAssignableFrom(method.returnType) && options.genericWrappers.any { wrapper -> val isReceiveChannelWrapper = wrapper.type == method.returnType + // lambdas are erased to return Object, so this only checks that the transformer can return a Publisher val hasPublisherTransformer = wrapper .transformer.javaClass .declaredMethods @@ -199,7 +198,7 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { searches.forEach { search -> signatures.addAll(getMissingMethodSignatures(field, search, isBoolean, scannedProperties)) - isSubscription = isSubscription || search.source is GraphQLSubscriptionResolver + isSubscription = isSubscription || search.isSubscription } val sourceName = field.sourceLocation?.sourceName ?: "" @@ -241,7 +240,9 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) { val resolverInfo: ResolverInfo, val source: Any?, val requiredFirstParameterType: Class<*>? = null - ) + ) { + val isSubscription get() = resolverInfo is RootResolverInfo && resolverInfo.isSubscription + } } internal class FieldResolverError(msg: String) : RuntimeException(msg) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index 3a0ccd11..926ebee4 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -6,6 +6,8 @@ import graphql.kickstart.tools.* import graphql.kickstart.tools.SchemaParserOptions.GenericWrapper import graphql.kickstart.tools.util.JavaType import graphql.kickstart.tools.util.coroutineScope +import graphql.kickstart.tools.util.futureValueType +import graphql.kickstart.tools.util.typeArgument import graphql.kickstart.tools.util.unwrap import graphql.language.* import graphql.schema.DataFetcher @@ -14,6 +16,8 @@ import graphql.schema.GraphQLFieldDefinition import graphql.schema.GraphQLTypeUtil.isScalar import graphql.schema.LightDataFetcher import kotlinx.coroutines.future.future +import org.apache.commons.lang3.reflect.TypeUtils +import org.reactivestreams.Publisher import org.slf4j.LoggerFactory import java.lang.reflect.InvocationTargetException import java.lang.reflect.Method @@ -150,13 +154,12 @@ internal class MethodFieldResolver( } override fun scanForMatches(): List { - val unwrappedGenericType = genericType.unwrapGenericType( - try { - method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType - } catch (e: InternalError) { - method.genericReturnType - } - ) + val returnType = try { + method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType + } catch (e: InternalError) { + method.genericReturnType + } + val unwrappedGenericType = genericType.unwrapGenericType(if (search.isSubscription) returnType.asPublisher() else returnType) val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue(field.type, unwrappedGenericType, genericType, SchemaClassScanner.ReturnValueReference(method)) return field.inputValueDefinitions.mapIndexed { i, inputDefinition -> @@ -164,6 +167,13 @@ internal class MethodFieldResolver( } + listOf(returnValueMatch) } + // generic wrappers only match their exact type, so a Publisher implementation (e.g. Flux), or a future of one, + // is matched as a plain Publisher of its events + private fun JavaType.asPublisher(): JavaType = + (futureValueType() ?: this).typeArgument(Publisher::class.java) + ?.let { TypeUtils.parameterize(Publisher::class.java, it) } + ?: this + private fun getIndexOffset(): Int { return if (resolverInfo is DataClassTypeResolverInfo && !method.declaringClass.isAssignableFrom(resolverInfo.dataClassType)) { 1 diff --git a/src/main/kotlin/graphql/kickstart/tools/util/Utils.kt b/src/main/kotlin/graphql/kickstart/tools/util/Utils.kt index 7a646182..58c6ef40 100644 --- a/src/main/kotlin/graphql/kickstart/tools/util/Utils.kt +++ b/src/main/kotlin/graphql/kickstart/tools/util/Utils.kt @@ -6,9 +6,12 @@ import graphql.language.* import graphql.schema.DataFetchingEnvironment import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import org.apache.commons.lang3.reflect.TypeUtils import java.lang.reflect.Method import java.lang.reflect.ParameterizedType import java.lang.reflect.Proxy +import java.util.concurrent.CompletionStage +import java.util.concurrent.Future /** * @author Andrew Potter @@ -37,6 +40,12 @@ internal fun JavaType.unwrap(): Class = this as Class<*> } +internal fun JavaType.typeArgument(type: Class<*>): JavaType? = + TypeUtils.getTypeArguments(this, type)?.get(type.typeParameters.first()) + +internal fun JavaType.futureValueType(): JavaType? = + typeArgument(CompletionStage::class.java) ?: typeArgument(Future::class.java) + internal fun DataFetchingEnvironment.coroutineScope(): CoroutineScope { val context: Any? = this.getContext() return if (context is CoroutineScope) context else CoroutineScope(Dispatchers.Default) diff --git a/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt b/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt index 3904b380..66a53abe 100644 --- a/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/SchemaParserTest.kt @@ -1,16 +1,25 @@ package graphql.kickstart.tools +import graphql.ExecutionResult +import graphql.GraphQL import graphql.kickstart.tools.resolver.FieldResolverError import graphql.schema.* import graphql.schema.idl.SchemaDirectiveWiring import graphql.schema.idl.SchemaDirectiveWiringEnvironment import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.reactive.publish import org.junit.Assert.assertThrows 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 +import java.util.concurrent.CompletableFuture.completedStage +import java.util.concurrent.CompletionStage import java.util.concurrent.Future @OptIn(ExperimentalCoroutinesApi::class) @@ -743,4 +752,285 @@ class SchemaParserTest { assertEquals(error.message, expected) } + + @Test + fun `parser should find mutation methods on a resolver that is also a subscription resolver`() { + class MutationAndSubscription : GraphQLMutationResolver, GraphQLSubscriptionResolver { + fun addItem(name: String) = name + fun onItemAdded(): Publisher = StringPublisher("added") + } + + val gql = GraphQL.newGraphQL( + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: String + } + + type Mutation { + addItem(name: String!): String! + } + + type Subscription { + onItemAdded: String! + } + """ + ) + .resolvers( + MutationAndSubscription(), + object : GraphQLQueryResolver { fun test() = "test" } + ) + .build() + .makeExecutableSchema() + ).build() + + val data = assertNoGraphQlErrors(gql) { """mutation { addItem(name: "item") }""" } + + assertEquals(data["addItem"], "item") + assertEquals(gql.firstSubscriptionEvent("subscription { onItemAdded }"), mapOf("onItemAdded" to "added")) + } + + @Test + fun `parser should find query methods on a resolver that is also a subscription resolver`() { + class QueryAndSubscription : GraphQLQueryResolver, GraphQLSubscriptionResolver { + fun item() = "item" + fun onItemAdded(): Publisher = StringPublisher("added") + } + + val gql = GraphQL.newGraphQL( + SchemaParser.newParser() + .schemaString( + """ + type Query { + item: String! + } + + type Subscription { + onItemAdded: String! + } + """ + ) + .resolvers(QueryAndSubscription()) + .build() + .makeExecutableSchema() + ).build() + + val data = assertNoGraphQlErrors(gql) { "{ item }" } + + assertEquals(data["item"], "item") + assertEquals(gql.firstSubscriptionEvent("subscription { onItemAdded }"), mapOf("onItemAdded" to "added")) + } + + @Test + fun `parser should verify subscription return type on a resolver that is also a mutation resolver`() { + class MutationAndSubscription : GraphQLMutationResolver, GraphQLSubscriptionResolver { + fun addItem(name: String) = name + fun onItemAdded() = "added" + } + + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: String + } + + type Mutation { + addItem(name: String!): String! + } + + type Subscription { + onItemAdded: String! + } + """ + ) + .resolvers( + MutationAndSubscription(), + object : GraphQLQueryResolver { fun test() = "test" } + ) + .build() + .makeExecutableSchema() + } + + assert(error.message!!.contains("onItemAdded()")) + assert(error.message!!.endsWith("Note that a Subscription data fetcher must return a Publisher of events")) + } + + @Test + fun `parser should not mention subscriptions for a missing mutation method on a resolver that is also a subscription resolver`() { + class MutationAndSubscription : GraphQLMutationResolver, GraphQLSubscriptionResolver { + fun onItemAdded(): Publisher = StringPublisher("added") + } + + val error = assertThrows(FieldResolverError::class.java) { + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: String + } + + type Mutation { + addItem(name: String!): String! + } + + type Subscription { + onItemAdded: String! + } + """ + ) + .resolvers( + MutationAndSubscription(), + object : GraphQLQueryResolver { fun test() = "test" } + ) + .build() + .makeExecutableSchema() + } + + assert(error.message!!.contains("addItem(~name)")) + assert(!error.message!!.contains("Subscription data fetcher")) { error.message!! } + } + + @Test + fun `parser should accept subscription resolvers returning publisher subtypes`() { + class Subscription : OverriddenPublisherSubscription { + fun onCustomPublisher(): EventPublisher = SingleEventPublisher("onCustomPublisher") + fun onConcretePublisher() = SingleEventPublisher("onConcretePublisher") + override fun onOverriddenPublisher() = SingleEventPublisher("onOverriddenPublisher") + } + + val gql = GraphQL.newGraphQL( + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: String + } + + type Subscription { + onCustomPublisher: Event! + onConcretePublisher: Event! + onOverriddenPublisher: Event! + } + + type Event { + name: String! + } + """ + ) + .resolvers( + Subscription(), + object : GraphQLQueryResolver { fun test() = "test" } + ) + .build() + .makeExecutableSchema() + ).build() + + listOf("onCustomPublisher", "onConcretePublisher", "onOverriddenPublisher").forEach { field -> + assertEquals(gql.firstSubscriptionEvent("subscription { $field { name } }"), mapOf(field to mapOf("name" to field))) + } + } + + @Test + fun `parser should accept subscription resolvers returning futures of publishers`() { + class Subscription : GraphQLSubscriptionResolver { + fun onCompletableFuture(): CompletableFuture> = completedFuture(SingleEventPublisher("onCompletableFuture")) + fun onCompletionStage(): CompletionStage> = completedStage(SingleEventPublisher("onCompletionStage")) + fun onFuture(): Future> = completedFuture(SingleEventPublisher("onFuture")) + fun onConcretePublisherFuture(): CompletableFuture = completedFuture(SingleEventPublisher("onConcretePublisherFuture")) + fun onWildcardPublisherFuture(): CompletionStage> = completedStage(SingleEventPublisher("onWildcardPublisherFuture")) + } + + val gql = GraphQL.newGraphQL( + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: String + } + + type Subscription { + onCompletableFuture: Event! + onCompletionStage: Event! + onFuture: Event! + onConcretePublisherFuture: Event! + onWildcardPublisherFuture: Event! + } + + type Event { + name: String! + } + """ + ) + .resolvers( + Subscription(), + object : GraphQLQueryResolver { fun test() = "test" } + ) + .build() + .makeExecutableSchema() + ).build() + + listOf("onCompletableFuture", "onCompletionStage", "onFuture", "onConcretePublisherFuture", "onWildcardPublisherFuture").forEach { field -> + assertEquals(gql.firstSubscriptionEvent("subscription { $field { name } }"), mapOf(field to mapOf("name" to field))) + } + } + + @Test + fun `parser should accept subscription resolvers returning a receive channel subtype with a wrapper for it`() { + class Subscription : GraphQLSubscriptionResolver { + fun onChannel(): Channel = Channel(1).also { it.trySend("onChannel") } + } + + val gql = GraphQL.newGraphQL( + SchemaParser.newParser() + .schemaString( + """ + type Query { + test: String + } + + type Subscription { + onChannel: String! + } + """ + ) + .resolvers( + Subscription(), + object : GraphQLQueryResolver { fun test() = "test" } + ) + .options( + SchemaParserOptions.newOptions() + .genericWrappers(SchemaParserOptions.GenericWrapper.withTransformer(Channel::class, 0, { channel: Channel<*> -> + publish { for (item in channel) send(item) } + })) + .build() + ) + .build() + .makeExecutableSchema() + ).build() + + assertEquals(gql.firstSubscriptionEvent("subscription { onChannel }"), mapOf("onChannel" to "onChannel")) + } + + private fun GraphQL.firstSubscriptionEvent(query: String): Map? { + val result = execute(query) + assert(result.errors.isEmpty()) { result.errors.toString() } + + val subscriber = TestEnvironment().newManualSubscriber(result.getData>()) + return subscriber.requestNextElement().getData>() + } + + interface EventPublisher : Publisher + + class StringPublisher(value: String) : Publisher by publish(block = { send(value) }) + + data class Event(val name: String) + + class SingleEventPublisher(name: String) : EventPublisher, Publisher by publish(block = { send(Event(name)) }) + + interface OverriddenPublisherSubscription : GraphQLSubscriptionResolver { + fun onOverriddenPublisher(): Publisher + } }