Merge branch 'master' into bugfix/308

Author: oryan-block

.github/workflows/release.yml

@@ -1,5 +1,7 @@
 name: Release
 on: [ workflow_dispatch ]
+permissions:
+  contents: read
 jobs:
   build:
     runs-on: ubuntu-latest
@@ -17,6 +19,9 @@ jobs:
   publish:
     runs-on: ubuntu-latest
     needs: build
+    permissions:
+      contents: write
+      pull-requests: write
     steps:
       - name: Checkout
         uses: actions/checkout@v7

github-build.sh

@@ -5,6 +5,14 @@ getVersion() {
   grep -m1 -o "<version>.*</version>$" pom.xml | awk -F'[><]' '{print $3}'
 }
 
+requireSnapshot() {
+  local APP_VERSION=$(getVersion)
+  if [[ ${APP_VERSION} != *-SNAPSHOT ]]; then
+    echo "Version in pom.xml must end with -SNAPSHOT to be released: '${APP_VERSION}'"
+    exit 1
+  fi
+}
+
 removeSnapshots() {
   sed -i 's/-SNAPSHOT//' pom.xml
 }
@@ -15,6 +23,15 @@ commitRelease() {
   git tag -a "v${APP_VERSION}" -m "Tag release version"
 }
 
+publishTag() {
+  local APP_VERSION=$(getVersion)
+  git push origin "v${APP_VERSION}"
+  # Release tags aren't on master's first-parent history, so GitHub can't find the previous one on its own
+  local PREVIOUS_TAG=$(gh release view --json tagName --jq .tagName)
+  gh release create "v${APP_VERSION}" --verify-tag --draft --generate-notes --notes-start-tag "${PREVIOUS_TAG}" --title "v${APP_VERSION}" \
+    || echo "::warning::Draft release for v${APP_VERSION} was not created, create it manually"
+}
+
 bumpVersion() {
   echo "Bump version number"
   local APP_VERSION=$(getVersion | xargs)
@@ -43,11 +60,14 @@ commitNextVersion() {
 git config --global user.email "actions@github.com"
 git config --global user.name "GitHub Actions"
 
+requireSnapshot
+
 echo "Deploying release to Maven Central"
 removeSnapshots
 
 mvn --batch-mode -Prelease deploy
 
 commitRelease
+publishTag
 bumpVersion
 commitNextVersion

src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt

@@ -437,13 +437,15 @@ internal class SchemaClassScanner(
     private fun findInputValueTypeInType(name: String, clazz: Class<*>): JavaType? {
         val methods = clazz.methods
 
-        val filteredMethods = methods.filter {
+        val (getters, methodsWithParameters) = methods.filter {
             it.name == name || it.name == "get${name.replaceFirstChar(Char::titlecase)}"
-        }.sortedBy { it.name.length }
+        }.sortedBy { it.name.length }.partition { it.parameterCount == 0 }
 
-        return filteredMethods.find { !it.isSynthetic }?.genericReturnType
-            ?: filteredMethods.firstOrNull()?.genericReturnType
+        return getters.find { !it.isSynthetic }?.genericReturnType
+            ?: getters.firstOrNull()?.genericReturnType
             ?: clazz.fields.find { it.name == name }?.genericType
+            ?: methodsWithParameters.find { !it.isSynthetic }?.genericReturnType
+            ?: methodsWithParameters.firstOrNull()?.genericReturnType
     }
 
     private data class QueueItem(val type: ObjectTypeDefinition, val clazz: JavaType)

src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt

@@ -65,14 +65,16 @@ internal class TypeClassMatcher(private val definitionsByName: Map<String, TypeD
                 match(potentialMatch, graphQLType.type, realType)
             }
 
-            is ListType -> {
-                if (realType is ParameterizedType && isListType(realType, potentialMatch)) {
+            is ListType -> when {
+                realType is ParameterizedType && isListType(realType, potentialMatch) ->
                     match(potentialMatch, graphQLType.type, realType.actualTypeArguments.first())
-                } else if (realType is Class<*> && realType.isArray) {
+                realType is Class<*> && realType.isArray ->
                     match(potentialMatch, graphQLType.type, realType.componentType)
-                } else {
+                // e.g. a property map value: the element type is unknown, so match it as Object too
+                realType == Object::class.java ->
+                    match(potentialMatch, graphQLType.type, realType)
+                else ->
                     throw error(potentialMatch, "Java class is not a List or generic type information was lost: $realType")
-                }
             }
 
             is TypeName -> {

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

@@ -16,8 +16,7 @@ import org.apache.commons.lang3.reflect.TypeUtils
 import org.reactivestreams.Publisher
 import org.slf4j.LoggerFactory
 import java.lang.reflect.*
-import kotlin.reflect.full.valueParameters
-import kotlin.reflect.jvm.javaType
+import kotlin.reflect.full.extensionReceiverParameter
 import kotlin.reflect.jvm.kotlinFunction
 
 /**
@@ -29,6 +28,8 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) {
 
     private val allowedLastArgumentTypes = listOfNotNull(DataFetchingEnvironment::class.java, GraphQLContext::class.java, options.contextClass)
 
+    private val methodsByNameCache = mutableMapOf<Pair<Class<out Any>, Boolean>, Map<String, List<Method>>>()
+
     fun findFieldResolver(field: FieldDefinition, resolverInfo: ResolverInfo): FieldResolver {
         val searches = resolverInfo.getFieldSearches()
 
@@ -86,29 +87,34 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) {
     }
 
     private fun findResolverMethod(field: FieldDefinition, search: Search): Method? {
-        val methods = getAllMethods(search)
+        val methodsByName = getMethodsByName(search)
         val argumentCount = field.inputValueDefinitions.size + if (search.requiredFirstParameterType != null) 1 else 0
         val name = field.name
+        val capitalizedName = name.replaceFirstChar(Char::titlecase)
 
         // Check for the following one by one:
         //   1. Method with exact field name
         //   2. Method that returns a boolean with "is" style getter
         //   3. Method with "get" style getter
         //   4. Method with "getField" style getter
         //   5. Method with "get" style getter with the field name converted from snake_case to camelCased. ex: key_ops -> getKeyOps()
-        return methods.find {
-            it.name == name && verifyMethodArguments(it, argumentCount, search)
-        } ?: methods.find {
-            (isBoolean(field.type) && it.name == "is${name.replaceFirstChar(Char::titlecase)}") && verifyMethodArguments(it, argumentCount, search)
-        } ?: methods.find {
-            it.name == "get${name.replaceFirstChar(Char::titlecase)}" && verifyMethodArguments(it, argumentCount, search)
-        } ?: methods.find {
-            it.name == "getField${name.replaceFirstChar(Char::titlecase)}" && verifyMethodArguments(it, argumentCount, search)
-        } ?: methods.find {
-            it.name == "get${name.snakeToCamelCase()}" && verifyMethodArguments(it, argumentCount, search)
+        return listOfNotNull(
+            name,
+            if (isBoolean(field.type)) "is$capitalizedName" else null,
+            "get$capitalizedName",
+            "getField$capitalizedName",
+            "get${name.snakeToCamelCase()}"
+        ).firstNotNullOfOrNull { methodName ->
+            methodsByName[methodName]?.find { verifyMethodArguments(it, argumentCount, search) }
         }
     }
 
+    // root fields are searched on every root resolver, so each class's methods are only indexed once
+    private fun getMethodsByName(search: Search): Map<String, List<Method>> =
+        methodsByNameCache.getOrPut(search.type.unwrap() to search.isSubscription) {
+            getAllMethods(search).groupBy { it.name }
+        }
+
     private fun getAllMethods(search: Search): List<Method> {
         val type = search.type.unwrap()
         val declaredMethods = type.declaredNonProxyMethods
@@ -160,31 +166,23 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) {
                 it.eraseUnboundedWildcards() == search.requiredFirstParameterType || method.declaringClass.typeParameters.contains(it)
             } ?: false
         } else {
-            true
+            // an extension receiver can only take the source object
+            !isExtensionFunction(method)
         }
 
-        val methodParameterCount = getMethodParameterCount(method)
-        val methodLastParameter = getMethodLastParameter(method)
+        val methodParameterCount = method.parameterCountWithoutContinuation()
+        val methodLastParameter = method.parameterTypes.getOrNull(methodParameterCount - 1)
 
         val correctParameterCount = methodParameterCount == requiredCount ||
             (methodParameterCount == (requiredCount + 1) && allowedLastArgumentTypes.contains(methodLastParameter))
         return correctParameterCount && appropriateFirstParameter
     }
 
-    private fun getMethodParameterCount(method: Method): Int {
-        return try {
-            method.kotlinFunction?.valueParameters?.size ?: method.parameterCount
-        } catch (e: InternalError) {
-            method.parameterCount
-        }
-    }
-
-    private fun getMethodLastParameter(method: Method): Type? {
+    private fun isExtensionFunction(method: Method): Boolean {
         return try {
-            method.kotlinFunction?.valueParameters?.lastOrNull()?.type?.javaType
-                ?: method.parameterTypes.lastOrNull()
+            method.kotlinFunction?.extensionReceiverParameter != null
         } catch (e: InternalError) {
-            method.parameterTypes.lastOrNull()
+            false
         }
     }
 

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

@@ -7,6 +7,8 @@ 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.isSuspendFunction
+import graphql.kickstart.tools.util.parameterCountWithoutContinuation
 import graphql.kickstart.tools.util.typeArgument
 import graphql.kickstart.tools.util.unwrap
 import graphql.language.*
@@ -15,6 +17,8 @@ import graphql.schema.DataFetchingEnvironment
 import graphql.schema.GraphQLFieldDefinition
 import graphql.schema.GraphQLTypeUtil.isScalar
 import graphql.schema.LightDataFetcher
+import kotlinx.coroutines.CoroutineStart
+import kotlinx.coroutines.ensureActive
 import kotlinx.coroutines.future.future
 import org.apache.commons.lang3.reflect.TypeUtils
 import org.reactivestreams.Publisher
@@ -24,7 +28,6 @@ import java.lang.reflect.Method
 import java.util.*
 import java.util.function.Supplier
 import kotlin.coroutines.intrinsics.suspendCoroutineUninterceptedOrReturn
-import kotlin.reflect.full.valueParameters
 import kotlin.reflect.jvm.javaType
 import kotlin.reflect.jvm.kotlinFunction
 
@@ -41,7 +44,7 @@ internal class MethodFieldResolver(
     private val log = LoggerFactory.getLogger(javaClass)
 
     private val isSuspendFunction = method.isSuspendFunction()
-    private val numberOfParameters = method.kotlinFunction?.valueParameters?.size ?: method.parameterCount
+    private val numberOfParameters = method.parameterCountWithoutContinuation()
     private val hasAdditionalParameter = numberOfParameters == (field.inputValueDefinitions.size + getIndexOffset() + 1)
 
     override fun createDataFetcher(): DataFetcher<*> {
@@ -100,7 +103,8 @@ internal class MethodFieldResolver(
 
         // Add DataFetchingEnvironment/Context argument
         if (this.hasAdditionalParameter) {
-            when (this.method.parameterTypes.last()) {
+            // suspend functions have a trailing Continuation parameter
+            when (this.method.parameterTypes[numberOfParameters - 1]) {
                 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 ->
                     val context: Any? = environment.graphQlContext[options.contextClass]
@@ -211,7 +215,10 @@ internal class MethodFieldResolverDataFetcher(
         val args = this.args.map { it(environment) }.toTypedArray()
 
         return if (isSuspendFunction) {
-            environment.coroutineScope().future(options.coroutineContextProvider.provide()) {
+            // start undispatched so DataLoader loads are queued before graphql-java dispatches them,
+            // which runs the block even if the context is already cancelled, hence ensureActive
+            environment.coroutineScope().future(options.coroutineContextProvider.provide(), CoroutineStart.UNDISPATCHED) {
+                ensureActive()
                 invokeSuspend(source, method, args)?.transformWithGenericWrapper(options.genericWrappers) { environment }
             }
         } else {
@@ -277,14 +284,6 @@ private class CompareGenericWrappers {
     }
 }
 
-private fun Method.isSuspendFunction(): Boolean {
-    return try {
-        this.kotlinFunction?.isSuspend == true
-    } catch (e: InternalError) {
-        false
-    }
-}
-
 private suspend inline fun invokeSuspend(target: Any, resolverMethod: Method, args: Array<Any?>): Any? {
     return suspendCoroutineUninterceptedOrReturn { continuation ->
         invoke(resolverMethod, target, args + continuation)

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

@@ -12,6 +12,7 @@ import java.lang.reflect.ParameterizedType
 import java.lang.reflect.Proxy
 import java.util.concurrent.CompletionStage
 import java.util.concurrent.Future
+import kotlin.reflect.jvm.kotlinFunction
 
 /**
  * @author Andrew Potter
@@ -69,6 +70,18 @@ internal val Class<*>.declaredNonProxyMethods: List<JavaMethod>
         }
     }
 
+internal fun JavaMethod.isSuspendFunction(): Boolean {
+    return try {
+        this.kotlinFunction?.isSuspend == true
+    } catch (e: InternalError) {
+        false
+    }
+}
+
+// the trailing Continuation of a suspend function isn't a resolver argument
+internal fun JavaMethod.parameterCountWithoutContinuation(): Int =
+    if (isSuspendFunction()) parameterCount - 1 else parameterCount
+
 internal fun getDocumentation(node: AbstractNode<*>, options: SchemaParserOptions): String? =
     when {
         node is AbstractDescribedNode<*> && node.description != null -> node.description?.content

src/test/kotlin/graphql/kickstart/tools/FieldResolverScannerTest.kt

@@ -6,10 +6,12 @@ import graphql.kickstart.tools.resolver.FieldResolverScanner
 import graphql.kickstart.tools.resolver.MethodFieldResolver
 import graphql.kickstart.tools.resolver.PropertyFieldResolver
 import graphql.language.FieldDefinition
+import graphql.language.InputValueDefinition
 import graphql.language.TypeName
 import graphql.relay.Connection
 import graphql.relay.DefaultConnection
 import graphql.relay.DefaultPageInfo
+import graphql.schema.DataFetchingEnvironment
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import org.junit.Test
 import java.util.*
@@ -98,6 +100,34 @@ class FieldResolverScannerTest {
         Locale.setDefault(default)
     }
 
+    @Test
+    fun `scanner ignores extension functions on root resolvers`() {
+        val resolver = RootResolverInfo(listOf(UserQuery(), ExtensionQuery()), options)
+        val field = FieldDefinition.newFieldDefinition()
+            .name("user")
+            .type(TypeName("String"))
+            .inputValueDefinition(InputValueDefinition("id", TypeName("ID")))
+            .build()
+
+        val user = scanner.findFieldResolver(field, resolver)
+
+        assert(user is MethodFieldResolver)
+        assertEquals((user as MethodFieldResolver).method.declaringClass, UserQuery::class.java)
+    }
+
+    @Test
+    fun `scanner finds field resolver methods in priority order`() {
+        val resolverInfo = RootResolverInfo(listOf(PriorityQuery()), options)
+
+        fun methodName(name: String, type: String) =
+            (scanner.findFieldResolver(FieldDefinition(name, TypeName(type)), resolverInfo) as MethodFieldResolver).method.name
+
+        assertEquals(methodName("name", "String"), "name")
+        assertEquals(methodName("active", "Boolean"), "isActive")
+        assertEquals(methodName("enabled", "String"), "getEnabled")
+        assertEquals(methodName("count", "Int"), "getFieldCount")
+    }
+
     class RootQuery1 : GraphQLQueryResolver {
         fun field1() {}
     }
@@ -110,6 +140,14 @@ class FieldResolverScannerTest {
         fun field1() {}
     }
 
+    class UserQuery : GraphQLQueryResolver {
+        fun user(id: String): String = id
+    }
+
+    class ExtensionQuery : GraphQLQueryResolver {
+        fun DataFetchingEnvironment.user(): String = field.name
+    }
+
     class CamelCaseQuery1 : GraphQLQueryResolver {
         fun getHullType(): HullType = HullType()
     }
@@ -118,6 +156,18 @@ class FieldResolverScannerTest {
         fun getId(): HullType = HullType()
     }
 
+    class PriorityQuery : GraphQLQueryResolver {
+        fun name(): String = "name"
+        fun getName(): String = "name"
+        fun getFieldName(): String = "name"
+        fun active(flag: Boolean): Boolean = flag
+        fun isActive(): Boolean = true
+        fun getActive(): Boolean = true
+        fun isEnabled(): Boolean = true
+        fun getEnabled(): String = "enabled"
+        fun getFieldCount(): Int = 1
+    }
+
     class HullType
 
     open class ParentPropertyQuery {