Skip to content

Commit 6dc9508

Browse files
oryan-blockclaude
oryan-block
and
claude
committed
Honor null generic wrapper results and await plain futures
A generic wrapper transformer that returned null was ignored, because transformWithGenericWrapper fell back to the original object with an elvis operator. Option-like wrappers could therefore never map their empty case to null: a scalar field serialized the wrapper's toString and an object or list field failed at runtime. The transformer result is now returned as is, including null, and the original object is only kept when no wrapper matches. The default Future wrapper only unwraps the type at scan time. At runtime graphql-java only awaits a CompletionStage, so any other Future (a FutureTask, or the observer returned by RxJava's toFuture()) reached graphql-java as the field value and child fields failed with a source type mismatch. When a resolver method is declared to return Future, a returned future that is not a CompletionStage is now waited on with a blocking get() on the fetching thread, as if the resolver had called get() itself. The cause of an ExecutionException is rethrown so the resolver's own error is reported, and the interrupt flag is restored before an InterruptedException is rethrown. The check uses the method's declared raw return type, so a returned object that merely implements Future is left alone. Because the wait blocks, a plain future that only completes after graphql-java moves on, such as one backed by a DataLoader load, now hangs instead of failing with a type mismatch. Such resolvers should return a CompletionStage. A plain future returned by a custom generic wrapper transformer is still not waited on unless the method itself is declared to return Future. Fixes #371 Fixes #203 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 62c5146 commit 6dc9508

3 files changed

Lines changed: 164 additions & 20 deletions

File tree

‎src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ import org.slf4j.LoggerFactory
2222
import java.lang.reflect.InvocationTargetException
2323
import java.lang.reflect.Method
2424
import java.util.*
25+
import java.util.concurrent.CompletionStage
26+
import java.util.concurrent.ExecutionException
27+
import java.util.concurrent.Future
2528
import java.util.function.Supplier
2629
import kotlin.coroutines.intrinsics.suspendCoroutineUninterceptedOrReturn
2730
import kotlin.reflect.full.valueParameters
@@ -122,12 +125,15 @@ internal class MethodFieldResolver(
122125
}
123126
}
124127

128+
// a returned future is only waited on if the method's declared raw return type is Future, so a returned object that merely implements Future is left alone
129+
val returnsFuture = TypeUtils.getRawType(getReturnType(), null) == Future::class.java
130+
125131
return if (numberOfParameters > 0 || isSuspendFunction) {
126132
// requires arguments and environment or is a suspend function
127-
MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options, isSuspendFunction)
133+
MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options, isSuspendFunction, returnsFuture)
128134
} else {
129135
// if there are no parameters an optimized version of the data fetcher can be used
130-
LightMethodFieldResolverDataFetcher(createSourceResolver(), this.method, options)
136+
LightMethodFieldResolverDataFetcher(createSourceResolver(), this.method, options, returnsFuture)
131137
}
132138
}
133139

@@ -154,11 +160,7 @@ internal class MethodFieldResolver(
154160
}
155161

156162
override fun scanForMatches(): List<TypeClassMatcher.PotentialMatch> {
157-
val returnType = try {
158-
method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType
159-
} catch (e: InternalError) {
160-
method.genericReturnType
161-
}
163+
val returnType = getReturnType()
162164
val unwrappedGenericType = genericType.unwrapGenericType(if (search.isSubscription) returnType.asPublisher() else returnType)
163165
val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue(field.type, unwrappedGenericType, genericType, SchemaClassScanner.ReturnValueReference(method))
164166

@@ -174,6 +176,13 @@ internal class MethodFieldResolver(
174176
?.let { TypeUtils.parameterize(Publisher::class.java, it) }
175177
?: this
176178

179+
private fun getReturnType(): JavaType =
180+
try {
181+
method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType
182+
} catch (e: InternalError) {
183+
method.genericReturnType
184+
}
185+
177186
private fun getIndexOffset(): Int {
178187
return if (resolverInfo is DataClassTypeResolverInfo && !method.declaringClass.isAssignableFrom(resolverInfo.dataClassType)) {
179188
1
@@ -203,7 +212,8 @@ internal class MethodFieldResolverDataFetcher(
203212
private val method: Method,
204213
private val args: List<ArgumentPlaceholder>,
205214
private val options: SchemaParserOptions,
206-
private val isSuspendFunction: Boolean
215+
private val isSuspendFunction: Boolean,
216+
private val returnsFuture: Boolean
207217
) : DataFetcher<Any> {
208218

209219
override fun get(environment: DataFetchingEnvironment): Any? {
@@ -212,10 +222,10 @@ internal class MethodFieldResolverDataFetcher(
212222

213223
return if (isSuspendFunction) {
214224
environment.coroutineScope().future(options.coroutineContextProvider.provide()) {
215-
invokeSuspend(source, method, args)?.transformWithGenericWrapper(options.genericWrappers) { environment }
225+
invokeSuspend(source, method, args)?.transformWithGenericWrapper(options.genericWrappers, returnsFuture) { environment }
216226
}
217227
} else {
218-
invoke(method, source, args)?.transformWithGenericWrapper(options.genericWrappers) { environment }
228+
invoke(method, source, args)?.transformWithGenericWrapper(options.genericWrappers, returnsFuture) { environment }
219229
}
220230
}
221231

@@ -234,13 +244,14 @@ internal class MethodFieldResolverDataFetcher(
234244
internal class LightMethodFieldResolverDataFetcher(
235245
private val sourceResolver: SourceResolver,
236246
private val method: Method,
237-
private val options: SchemaParserOptions
247+
private val options: SchemaParserOptions,
248+
private val returnsFuture: Boolean
238249
) : LightDataFetcher<Any?> {
239250

240251
override fun get(fieldDefinition: GraphQLFieldDefinition?, sourceObject: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
241252
val source = sourceResolver.resolve(null, sourceObject)
242253

243-
return invoke(method, source, emptyArray())?.transformWithGenericWrapper(options.genericWrappers, environmentSupplier)
254+
return invoke(method, source, emptyArray())?.transformWithGenericWrapper(options.genericWrappers, returnsFuture, environmentSupplier)
244255
}
245256

246257
override fun get(environment: DataFetchingEnvironment): Any? {
@@ -258,14 +269,34 @@ internal class LightMethodFieldResolverDataFetcher(
258269

259270
private fun Any.transformWithGenericWrapper(
260271
genericWrappers: List<GenericWrapper>,
272+
returnsFuture: Boolean,
261273
environmentSupplier: Supplier<DataFetchingEnvironment>
262-
): Any {
263-
return genericWrappers
274+
): Any? {
275+
val genericWrapper = genericWrappers
264276
.asSequence()
265277
.filter { it.type.isInstance(this) }
266278
.sortedWith(CompareGenericWrappers)
267279
.firstOrNull()
268-
?.transformer?.invoke(this, environmentSupplier.get()) ?: this
280+
?: return this
281+
282+
val value = genericWrapper.transformer(this, environmentSupplier.get())
283+
return if (returnsFuture) value.awaitFuture() else value
284+
}
285+
286+
// graphql-java only awaits completion stages, so any other future is waited on by blocking the fetching thread
287+
private fun Any?.awaitFuture(): Any? {
288+
if (this !is Future<*> || this is CompletionStage<*>) {
289+
return this
290+
}
291+
292+
return try {
293+
get()
294+
} catch (e: ExecutionException) {
295+
throw e.cause ?: e
296+
} catch (e: InterruptedException) {
297+
Thread.currentThread().interrupt()
298+
throw e
299+
}
269300
}
270301

271302
private class CompareGenericWrappers {

‎src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,51 @@ class MethodFieldResolverTest {
239239
assertEquals(exceptionWhileDataFetching.exception.message, "Whoops")
240240
}
241241

242+
@Test
243+
fun `should return null when generic wrapper transformer returns null`() {
244+
val schema = SchemaParser.newParser()
245+
.schemaString(
246+
"""
247+
type Query {
248+
name: String
249+
names: [String!]
250+
nameWithArgument(input: String): String
251+
}
252+
""")
253+
.resolvers(object : GraphQLQueryResolver {
254+
fun name(): Maybe<String> = Maybe.None
255+
fun names(): Maybe<List<String>> = Maybe.None
256+
fun nameWithArgument(input: String?): Maybe<String> = Maybe.None
257+
})
258+
.options(SchemaParserOptions.newOptions()
259+
.genericWrappers(SchemaParserOptions.GenericWrapper.withTransformer(Maybe::class, 0, { maybe -> (maybe as? Maybe.Some)?.value }))
260+
.build())
261+
.build()
262+
.makeExecutableSchema()
263+
264+
val gql = GraphQL.newGraphQL(schema).build()
265+
val result = gql.execute(ExecutionInput.newExecutionInput().query(
266+
"""
267+
query {
268+
name
269+
names
270+
nameWithArgument(input: "test")
271+
}
272+
"""))
273+
274+
assertEquals(result.errors, listOf())
275+
assertEquals(result.getData(), mapOf(
276+
"name" to null,
277+
"names" to null,
278+
"nameWithArgument" to null
279+
))
280+
}
281+
282+
sealed class Maybe<out T> {
283+
data class Some<T>(val value: T) : Maybe<T>()
284+
object None : Maybe<Nothing>()
285+
}
286+
242287
/**
243288
* Custom Scalar Class type that doesn't work with Jackson serialization/deserialization
244289
*/

‎src/test/kotlin/graphql/kickstart/tools/ReactiveTest.kt‎

Lines changed: 73 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package graphql.kickstart.tools
22

3+
import graphql.ExceptionWhileDataFetching
34
import graphql.GraphQL
45
import graphql.execution.AsyncExecutionStrategy
56
import graphql.kickstart.tools.SchemaParser.Companion.newParser
@@ -9,6 +10,7 @@ import org.junit.Test
910
import java.util.*
1011
import java.util.concurrent.CompletableFuture
1112
import java.util.concurrent.Future
13+
import java.util.concurrent.FutureTask
1214

1315
//import io.reactivex.Single;
1416
//import io.reactivex.internal.operators.single.SingleJust;
@@ -39,6 +41,73 @@ class ReactiveTest {
3941
}
4042
}
4143

44+
@Test
45+
fun `future that is not a completion stage succeeds`() {
46+
val schema = newParser().file("Reactive.graphqls")
47+
.resolvers(object : GraphQLQueryResolver {
48+
fun organization(organizationId: Int): Future<Organization> =
49+
FutureTask { Organization(User(organizationId.toLong())) }.also { Thread(it).start() }
50+
})
51+
.build()
52+
.makeExecutableSchema()
53+
54+
val gql = GraphQL.newGraphQL(schema).build()
55+
56+
val data = assertNoGraphQlErrors(gql) {
57+
"query { organization(organizationId: 1) { user { id } } }"
58+
}
59+
60+
assertEquals(data, mapOf("organization" to mapOf("user" to mapOf("id" to "1"))))
61+
}
62+
63+
@Test
64+
fun `future that is not a completion stage rethrows its failure`() {
65+
val schema = newParser().file("Reactive.graphqls")
66+
.resolvers(object : GraphQLQueryResolver {
67+
fun organization(organizationId: Int): Future<Organization> =
68+
FutureTask<Organization> { throw IllegalStateException("Whoops") }.also { it.run() }
69+
})
70+
.build()
71+
.makeExecutableSchema()
72+
73+
val gql = GraphQL.newGraphQL(schema).build()
74+
val result = gql.execute("query { organization(organizationId: 1) { user { id } } }")
75+
76+
assertEquals(result.errors.size, 1)
77+
val exceptionWhileDataFetching = result.errors[0] as ExceptionWhileDataFetching
78+
assertEquals(exceptionWhileDataFetching.exception.message, "Whoops")
79+
}
80+
81+
@Test
82+
fun `future is only waited on if the resolver declares a future`() {
83+
val schema = newParser()
84+
.schemaString(
85+
"""
86+
type Query {
87+
job: Job
88+
futureJob: Job
89+
}
90+
91+
type Job {
92+
id: ID
93+
}
94+
""")
95+
.resolvers(object : GraphQLQueryResolver {
96+
fun job(): Job = Job(1)
97+
fun futureJob(): Future<Job> = FutureTask { Job(2) }.also { it.run() }
98+
})
99+
.build()
100+
.makeExecutableSchema()
101+
102+
val gql = GraphQL.newGraphQL(schema).build()
103+
104+
val data = assertNoGraphQlErrors(gql) {
105+
"query { job { id } futureJob { id } }"
106+
}
107+
108+
assertEquals(data, mapOf("job" to mapOf("id" to "1"), "futureJob" to mapOf("id" to "2")))
109+
}
110+
42111
private class Query : GraphQLQueryResolver {
43112
// Single<Optional<Organization>> organization(int organizationid) {
44113
// return Single.just(Optional.empty()); //CompletableFuture.completedFuture(null);
@@ -48,12 +117,11 @@ class ReactiveTest {
48117
}
49118
}
50119

51-
private class Organization {
52-
private val user: User? = null
53-
}
120+
private class Organization(private val user: User? = null)
54121

55-
private class User {
56-
private val id: Long? = null
122+
private class User(private val id: Long? = null) {
57123
private val name: String? = null
58124
}
125+
126+
private class Job(private val id: Long) : Future<String> by CompletableFuture.completedFuture("done")
59127
}

0 commit comments

Comments
 (0)