Simplify lightweight data fetcher heuristic

Author: vojtapol

pom.xml

@@ -4,7 +4,7 @@
 
     <groupId>com.graphql-java-kickstart</groupId>
     <artifactId>graphql-java-tools</artifactId>
-    <version>14.0.0-LOCAL</version>
+    <version>14.1.0-LOCAL</version>
     <packaging>jar</packaging>
 
     <name>GraphQL Java Tools</name>

src/main/kotlin/graphql/kickstart/tools/resolver/MapFieldResolver.kt

@@ -55,7 +55,7 @@ internal class MapFieldResolverDataFetcher(
     private val key: String,
 ) : LightDataFetcher<Any> {
 
-    override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
+    override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
         if (sourceObject is Map<*, *>) {
             return sourceObject[key]
         } else {

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

@@ -6,7 +6,6 @@ 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.isTrivialDataFetcher
 import graphql.kickstart.tools.util.unwrap
 import graphql.language.*
 import graphql.schema.DataFetcher
@@ -37,13 +36,9 @@ internal class MethodFieldResolver(
 
     private val log = LoggerFactory.getLogger(javaClass)
 
-    private val additionalLastArgument =
-        try {
-            (method.kotlinFunction?.valueParameters?.size
-                ?: method.parameterCount) == (field.inputValueDefinitions.size + getIndexOffset() + 1)
-        } catch (e: InternalError) {
-            method.parameterCount == (field.inputValueDefinitions.size + getIndexOffset() + 1)
-        }
+    private val isSuspendFunction = method.isSuspendFunction()
+    private val numberOfDeclaredParameters = method.kotlinFunction?.valueParameters?.size ?: method.parameterCount
+    private val hasAdditionalParameter = numberOfDeclaredParameters == (field.inputValueDefinitions.size + getIndexOffset() + 1)
 
     override fun createDataFetcher(): DataFetcher<*> {
         val args = mutableListOf<ArgumentPlaceholder>()
@@ -100,7 +95,7 @@ internal class MethodFieldResolver(
         }
 
         // Add DataFetchingEnvironment/Context argument
-        if (this.additionalLastArgument) {
+        if (this.hasAdditionalParameter) {
             when (this.method.parameterTypes.last()) {
                 null -> throw ResolverError("Expected at least one argument but got none, this is most likely a bug with graphql-java-tools")
                 options.contextClass -> args.add { environment ->
@@ -123,10 +118,10 @@ internal class MethodFieldResolver(
             }
         }
 
-        return if (args.isEmpty() && isTrivialDataFetcher(this.method)) {
+        return if (numberOfDeclaredParameters == 0 && !isSuspendFunction) {
             LightMethodFieldResolverDataFetcher(createSourceResolver(), this.method, options)
         } else {
-            MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options)
+            MethodFieldResolverDataFetcher(createSourceResolver(), this.method, args, options, isSuspendFunction)
         }
     }
 
@@ -196,10 +191,9 @@ internal class MethodFieldResolverDataFetcher(
     private val method: Method,
     private val args: List<ArgumentPlaceholder>,
     private val options: SchemaParserOptions,
+    private val isSuspendFunction: Boolean
 ) : DataFetcher<Any> {
 
-    private val isSuspendFunction = method.isSuspendFunction()
-
     override fun get(environment: DataFetchingEnvironment): Any? {
         val source = sourceResolver.resolve(environment, null)
         val args = this.args.map { it(environment) }.toTypedArray()
@@ -223,27 +217,18 @@ internal class MethodFieldResolverDataFetcher(
 }
 
 /**
- * Similar to [MethodFieldResolverDataFetcher] but for light data fetchers which do not require the environment to be supplied unless suspend functions or
- * generic wrappers are used.
+ * Similar to [MethodFieldResolverDataFetcher] but for light data fetchers which do not require the environment to be supplied unless generic wrappers are used.
  */
 internal class LightMethodFieldResolverDataFetcher(
     private val sourceResolver: SourceResolver,
     private val method: Method,
     private val options: SchemaParserOptions,
 ) : LightDataFetcher<Any?> {
 
-    private val isSuspendFunction = method.isSuspendFunction()
-
-    override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
+    override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
         val source = sourceResolver.resolve(null, sourceObject)
 
-        return if (isSuspendFunction) {
-            environmentSupplier.get().coroutineScope().future(options.coroutineContextProvider.provide()) {
-                invokeSuspend(source, method, emptyArray())?.transformWithGenericWrapper(options.genericWrappers, environmentSupplier)
-            }
-        } else {
-            invoke(method, source, emptyArray())?.transformWithGenericWrapper(options.genericWrappers, environmentSupplier)
-        }
+        return invoke(method, source, emptyArray())?.transformWithGenericWrapper(options.genericWrappers, environmentSupplier)
     }
 
     override fun get(environment: DataFetchingEnvironment): Any? {

src/main/kotlin/graphql/kickstart/tools/resolver/PropertyFieldResolver.kt

@@ -44,7 +44,7 @@ internal class PropertyFieldResolverDataFetcher(
     private val field: Field,
 ) : LightDataFetcher<Any> {
 
-    override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
+    override fun get(fieldDefinition: GraphQLFieldDefinition, sourceObject: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): Any? {
         return field.get(sourceResolver.resolve(null, sourceObject))
     }
 

src/main/kotlin/graphql/kickstart/tools/util/Utils.kt

@@ -61,22 +61,4 @@ internal fun getDocumentation(node: AbstractNode<*>, options: SchemaParserOption
             .trimIndent()
     }
 
-/**
- * Simple heuristic to check if a method is a trivial data fetcher.
- *
- * Requirements are:
- * - prefixed with get
- * - must have zero parameters
- */
-internal fun isTrivialDataFetcher(method: Method): Boolean {
-    return (method.parameterCount == 0
-        && (
-        method.name.startsWith("get")
-            || isBooleanGetter(method)))
-}
-
-private fun isBooleanGetter(method: Method) = (method.name.startsWith("is")
-    && (method.returnType == java.lang.Boolean::class.java)
-    || method.returnType == Boolean::class.java)
-
 internal fun String.snakeToCamelCase(): String = split("_").joinToString(separator = "") { it.replaceFirstChar(Char::titlecase) }

src/test/kotlin/graphql/kickstart/tools/util/UtilsTest.kt

@@ -1,8 +1,5 @@
 package graphql.kickstart.tools.util
 
-import org.junit.Assert
-import org.junit.Test
-
 class UtilsTest {
 
     @Suppress("unused")
@@ -26,19 +23,4 @@ class UtilsTest {
     private class UtilsTestTrivialDataFetcherBean {
         val isBooleanPrimitive = false
     }
-
-    @Test
-    fun isTrivialDataFetcher() {
-        val clazz = Bean::class.java
-
-        Assert.assertTrue(isTrivialDataFetcher(clazz.getMethod("getterValid")))
-        Assert.assertFalse(isTrivialDataFetcher(clazz.getMethod("getterWithArgument", String::class.java)))
-        Assert.assertFalse(isTrivialDataFetcher(clazz.getMethod("notAGetter")))
-
-        Assert.assertFalse(isTrivialDataFetcher(clazz.getMethod("isString")))
-        Assert.assertTrue(isTrivialDataFetcher(clazz.getMethod("isJavaBoolean")))
-        Assert.assertTrue(isTrivialDataFetcher(clazz.getMethod("isKotlinBoolean")))
-
-        Assert.assertTrue(isTrivialDataFetcher(UtilsTestTrivialDataFetcherBean::class.java.getMethod("isBooleanPrimitive")))
-    }
 }