Skip to content

Commit f203700

Browse files
authored
Merge pull request #839 from graphql-java-kickstart/bugfix/203
Reject futures that graphql-java can't wait on
2 parents acc0cdf + 3723635 commit f203700

2 files changed

Lines changed: 254 additions & 11 deletions

File tree

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

Lines changed: 48 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ import org.slf4j.LoggerFactory
2626
import java.lang.reflect.InvocationTargetException
2727
import java.lang.reflect.Method
2828
import java.util.*
29+
import java.util.concurrent.CompletionStage
30+
import java.util.concurrent.Future
2931
import java.util.function.Supplier
3032
import kotlin.coroutines.intrinsics.suspendCoroutineUninterceptedOrReturn
3133
import kotlin.reflect.jvm.javaType
@@ -126,12 +128,17 @@ internal class MethodFieldResolver(
126128
}
127129
}
128130

131+
// the class the return type unwraps to, so a value that happens to implement Future isn't mistaken for a wrapper.
132+
// Object (e.g. for a union) can't tell them apart, so then every future of a method declared to return Future is checked
133+
val valueClass = (TypeUtils.getRawType(getUnwrappedReturnType(), null) ?: Any::class.java)
134+
.takeUnless { it == Any::class.java && TypeUtils.getRawType(getReturnType(), null) == Future::class.java }
135+
129136
return if (numberOfParameters > 0 || isSuspendFunction) {
130137
// requires arguments and environment or is a suspend function
131-
MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options, isSuspendFunction)
138+
MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options, isSuspendFunction, valueClass)
132139
} else {
133140
// if there are no parameters an optimized version of the data fetcher can be used
134-
LightMethodFieldResolverDataFetcher(createSourceResolver(), this.method, options)
141+
LightMethodFieldResolverDataFetcher(createSourceResolver(), this.method, options, valueClass)
135142
}
136143
}
137144

@@ -158,13 +165,7 @@ internal class MethodFieldResolver(
158165
}
159166

160167
override fun scanForMatches(): List<TypeClassMatcher.PotentialMatch> {
161-
val returnType = try {
162-
method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType
163-
} catch (e: InternalError) {
164-
method.genericReturnType
165-
}
166-
val unwrappedGenericType = genericType.unwrapGenericType(if (search.isSubscription) returnType.asPublisher() else returnType)
167-
val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue(field.type, unwrappedGenericType, genericType, SchemaClassScanner.ReturnValueReference(method))
168+
val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue(field.type, getUnwrappedReturnType(), genericType, SchemaClassScanner.ReturnValueReference(method))
168169

169170
return field.inputValueDefinitions.mapIndexed { i, inputDefinition ->
170171
TypeClassMatcher.PotentialMatch.parameterType(inputDefinition.type, getMethodParameterType(i)!!, genericType, SchemaClassScanner.MethodParameterReference(method, i))
@@ -178,6 +179,16 @@ internal class MethodFieldResolver(
178179
?.let { TypeUtils.parameterize(Publisher::class.java, it) }
179180
?: this
180181

182+
private fun getReturnType(): JavaType =
183+
try {
184+
method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType
185+
} catch (e: InternalError) {
186+
method.genericReturnType
187+
}
188+
189+
private fun getUnwrappedReturnType(): JavaType =
190+
getReturnType().let { genericType.unwrapGenericType(if (search.isSubscription) it.asPublisher() else it) }
191+
181192
private fun getIndexOffset(): Int {
182193
return if (resolverInfo is DataClassTypeResolverInfo && !method.declaringClass.isAssignableFrom(resolverInfo.dataClassType)) {
183194
1
@@ -207,7 +218,8 @@ internal class MethodFieldResolverDataFetcher(
207218
private val method: Method,
208219
private val args: List<ArgumentPlaceholder>,
209220
private val options: SchemaParserOptions,
210-
private val isSuspendFunction: Boolean
221+
private val isSuspendFunction: Boolean,
222+
private val valueClass: Class<*>?
211223
) : DataFetcher<Any> {
212224

213225
override fun get(environment: DataFetchingEnvironment): Any? {
@@ -220,9 +232,11 @@ internal class MethodFieldResolverDataFetcher(
220232
environment.coroutineScope().future(options.coroutineContextProvider.provide(), CoroutineStart.UNDISPATCHED) {
221233
ensureActive()
222234
invokeSuspend(source, method, args)?.transformWithGenericWrapper(options.genericWrappers) { environment }
235+
.requireAwaitable(valueClass, method, isSuspendFunction = true)
223236
}
224237
} else {
225238
invoke(method, source, args)?.transformWithGenericWrapper(options.genericWrappers) { environment }
239+
.requireAwaitable(valueClass, method, isSuspendFunction = false)
226240
}
227241
}
228242

@@ -241,13 +255,15 @@ internal class MethodFieldResolverDataFetcher(
241255
internal class LightMethodFieldResolverDataFetcher(
242256
private val sourceResolver: SourceResolver,
243257
private val method: Method,
244-
private val options: SchemaParserOptions
258+
private val options: SchemaParserOptions,
259+
private val valueClass: Class<*>?
245260
) : LightDataFetcher<Any?> {
246261

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

250265
return invoke(method, source, emptyArray())?.transformWithGenericWrapper(options.genericWrappers, environmentSupplier)
266+
.requireAwaitable(valueClass, method, isSuspendFunction = false)
251267
}
252268

253269
override fun get(environment: DataFetchingEnvironment): Any? {
@@ -278,6 +294,27 @@ private fun Any.transformWithGenericWrapper(
278294
return genericWrapper.transformer(this, environmentSupplier.get())
279295
}
280296

297+
// graphql-java only waits on a CompletionStage returned by a data fetcher, so any other future, or a future
298+
// that a suspend function completes with, would reach it as the field value
299+
private fun Any?.requireAwaitable(valueClass: Class<*>?, method: Method, isSuspendFunction: Boolean): Any? {
300+
if (this !is Future<*> || valueClass?.isInstance(this) == true) {
301+
return this
302+
}
303+
304+
val resolverName = "${method.declaringClass.name}.${method.name}"
305+
if (isSuspendFunction) {
306+
throw ResolverError("Suspend function '$resolverName' resolved to a ${javaClass.name}, which graphql-java can't wait on. Return its value instead.")
307+
}
308+
if (this !is CompletionStage<*>) {
309+
throw ResolverError(
310+
"Resolver method '$resolverName' resolved to a ${javaClass.name}, which graphql-java can't wait on. " +
311+
"Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead."
312+
)
313+
}
314+
315+
return this
316+
}
317+
281318
private class CompareGenericWrappers {
282319
companion object : Comparator<GenericWrapper> {
283320
override fun compare(w1: GenericWrapper, w2: GenericWrapper): Int = when {

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

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,18 @@
11
package graphql.kickstart.tools
22

3+
import graphql.ExceptionWhileDataFetching
4+
import graphql.ExecutionResult
35
import graphql.GraphQL
46
import graphql.execution.AsyncExecutionStrategy
57
import graphql.kickstart.tools.SchemaParser.Companion.newParser
68
import graphql.kickstart.tools.SchemaParserOptions.Companion.newOptions
9+
import graphql.kickstart.tools.SchemaParserOptions.GenericWrapper
710
import kotlinx.coroutines.ExperimentalCoroutinesApi
811
import org.junit.Test
912
import java.util.*
1013
import java.util.concurrent.CompletableFuture
1114
import java.util.concurrent.Future
15+
import java.util.concurrent.FutureTask
1216

1317
//import io.reactivex.Single;
1418
//import io.reactivex.internal.operators.single.SingleJust;
@@ -39,6 +43,208 @@ class ReactiveTest {
3943
}
4044
}
4145

46+
@Test
47+
fun `future that is not a completion stage fails with a clear error`() {
48+
val gql = GraphQL.newGraphQL(itemSchema(PlainFutureQuery())).build()
49+
50+
val result = gql.execute(
51+
"""
52+
query {
53+
item { id }
54+
itemWithArgument(id: 1) { id }
55+
name
56+
}
57+
""")
58+
59+
assertEquals(result.getData(), mapOf("item" to null, "itemWithArgument" to null, "name" to null))
60+
assertEquals(result.errorMessagesByPath(), mapOf(
61+
"item" to "Resolver method '${PlainFutureQuery::class.java.name}.item' resolved to a java.util.concurrent.FutureTask, " +
62+
"which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead.",
63+
"itemWithArgument" to "Resolver method '${PlainFutureQuery::class.java.name}.itemWithArgument' resolved to a java.util.concurrent.FutureTask, " +
64+
"which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead.",
65+
"name" to "Resolver method '${PlainFutureQuery::class.java.name}.name' resolved to a java.util.concurrent.FutureTask, " +
66+
"which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead."
67+
))
68+
}
69+
70+
@Test
71+
fun `suspend function that resolves to a future fails with a clear error`() {
72+
val gql = GraphQL.newGraphQL(itemSchema(PlainFutureQuery())).build()
73+
74+
val result = gql.execute(
75+
"""
76+
query {
77+
suspendItem { id }
78+
suspendCompletableItem { id }
79+
}
80+
""")
81+
82+
assertEquals(result.getData(), mapOf("suspendItem" to null, "suspendCompletableItem" to null))
83+
assertEquals(result.errorMessagesByPath(), mapOf(
84+
"suspendItem" to "Suspend function '${PlainFutureQuery::class.java.name}.suspendItem' resolved to a java.util.concurrent.FutureTask, " +
85+
"which graphql-java can't wait on. Return its value instead.",
86+
"suspendCompletableItem" to "Suspend function '${PlainFutureQuery::class.java.name}.suspendCompletableItem' resolved to a java.util.concurrent.CompletableFuture, " +
87+
"which graphql-java can't wait on. Return its value instead."
88+
))
89+
}
90+
91+
@Test
92+
fun `future that is not a completion stage can be unwrapped by a generic wrapper`() {
93+
val options = newOptions()
94+
.genericWrappers(GenericWrapper.withTransformer(FutureTask::class, 0, { task -> task.get() }))
95+
.build()
96+
val gql = GraphQL.newGraphQL(itemSchema(PlainFutureQuery(), options)).build()
97+
98+
val data = assertNoGraphQlErrors(gql) {
99+
"""
100+
query {
101+
item { id }
102+
itemWithArgument(id: 2) { id }
103+
name
104+
suspendItem { id }
105+
nullItem { id }
106+
}
107+
"""
108+
}
109+
110+
assertEquals(data, mapOf(
111+
"item" to mapOf("id" to "1"),
112+
"itemWithArgument" to mapOf("id" to "2"),
113+
"name" to "name",
114+
"suspendItem" to mapOf("id" to "3"),
115+
"nullItem" to null
116+
))
117+
}
118+
119+
@Test
120+
fun `future that a generic wrapper, a type variable or a union resolves to fails with a clear error`() {
121+
val schema = newParser()
122+
.schemaString(
123+
"""
124+
type Query {
125+
item: Item
126+
boxedItem: Item
127+
search: SearchResult
128+
}
129+
130+
union SearchResult = Item
131+
132+
type Item {
133+
id: ID
134+
}
135+
""")
136+
.resolvers(FutureItemQuery())
137+
.options(newOptions()
138+
.genericWrappers(GenericWrapper.withTransformer(Box::class, 0, { box -> FutureTask { box.value }.also { it.run() } }))
139+
.build())
140+
.build()
141+
.makeExecutableSchema()
142+
val gql = GraphQL.newGraphQL(schema).build()
143+
144+
val result = gql.execute(
145+
"""
146+
query {
147+
item { id }
148+
boxedItem { id }
149+
search {
150+
... on Item { id }
151+
}
152+
}
153+
""")
154+
155+
assertEquals(result.getData(), mapOf("item" to null, "boxedItem" to null, "search" to null))
156+
assertEquals(result.errorMessagesByPath(), mapOf(
157+
"item" to "Resolver method '${ItemQuery::class.java.name}.item' resolved to a java.util.concurrent.FutureTask, " +
158+
"which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead.",
159+
"boxedItem" to "Resolver method '${FutureItemQuery::class.java.name}.boxedItem' resolved to a java.util.concurrent.FutureTask, " +
160+
"which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead.",
161+
"search" to "Resolver method '${FutureItemQuery::class.java.name}.search' resolved to a java.util.concurrent.FutureTask, " +
162+
"which graphql-java can't wait on. Return a CompletionStage, such as a CompletableFuture, from the method or its generic wrapper transformer instead."
163+
))
164+
}
165+
166+
@Test
167+
fun `value that implements Future resolves normally`() {
168+
val schema = newParser()
169+
.schemaString(
170+
"""
171+
type Query {
172+
job: Job
173+
}
174+
175+
type Job {
176+
id: ID
177+
}
178+
""")
179+
.resolvers(object : GraphQLQueryResolver {
180+
fun job(): Job = Job(1)
181+
})
182+
.build()
183+
.makeExecutableSchema()
184+
185+
val gql = GraphQL.newGraphQL(schema).build()
186+
187+
val data = assertNoGraphQlErrors(gql) {
188+
"""
189+
query {
190+
job { id }
191+
}
192+
"""
193+
}
194+
195+
assertEquals(data, mapOf("job" to mapOf("id" to "1")))
196+
}
197+
198+
private fun itemSchema(query: GraphQLQueryResolver, options: SchemaParserOptions = newOptions().build()) = newParser()
199+
.schemaString(
200+
"""
201+
type Query {
202+
item: Item
203+
itemWithArgument(id: ID): Item
204+
name: String
205+
suspendItem: Item
206+
suspendCompletableItem: Item
207+
nullItem: Item
208+
}
209+
210+
type Item {
211+
id: ID
212+
}
213+
""")
214+
.resolvers(query)
215+
.options(options)
216+
.build()
217+
.makeExecutableSchema()
218+
219+
private fun ExecutionResult.errorMessagesByPath() =
220+
errors.associate { it.path?.joinToString(".") to (it as ExceptionWhileDataFetching).exception.message }
221+
222+
// the futures are already done, so even a future that could be read without blocking is rejected
223+
private class PlainFutureQuery : GraphQLQueryResolver {
224+
fun item(): Future<Item> = FutureTask { Item(1) }.also { it.run() }
225+
fun itemWithArgument(id: Long): Future<Item> = FutureTask { Item(id) }.also { it.run() }
226+
fun name(): Future<String> = FutureTask { "name" }.also { it.run() }
227+
suspend fun suspendItem(): Future<Item> = FutureTask { Item(3) }.also { it.run() }
228+
suspend fun suspendCompletableItem(): Future<Item> = CompletableFuture.completedFuture(Item(4))
229+
fun nullItem(): Future<Item?> = FutureTask<Item?> { null }.also { it.run() }
230+
}
231+
232+
open class ItemQuery<R>(private val item: () -> R) {
233+
fun item(): R = item.invoke()
234+
}
235+
236+
private class FutureItemQuery : ItemQuery<Future<Item>>({ FutureTask { Item(1) }.also { it.run() } }), GraphQLQueryResolver {
237+
fun boxedItem(): Box<Item> = Box(Item(2))
238+
fun search(): Future<Any> = FutureTask<Any> { Item(3) }.also { it.run() }
239+
}
240+
241+
private class Box<T>(val value: T)
242+
243+
private class Item(private val id: Long)
244+
245+
// implements Future, but is the schema type itself rather than a wrapper around one
246+
private class Job(private val id: Long) : Future<String> by CompletableFuture.completedFuture("done")
247+
42248
private class Query : GraphQLQueryResolver {
43249
// Single<Optional<Organization>> organization(int organizationid) {
44250
// return Single.just(Optional.empty()); //CompletableFuture.completedFuture(null);

0 commit comments

Comments
 (0)