Merge pull request #453 from alexandreBaronZnk/bugfix/367-list-ids-not-resolved-correctly

Always convert scalar ID values to the method parameter type fix #367

Author: vojtapol

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

@@ -14,8 +14,6 @@ import graphql.schema.DataFetchingEnvironment
 import graphql.schema.GraphQLTypeUtil.isScalar
 import kotlinx.coroutines.future.future
 import java.lang.reflect.Method
-import java.lang.reflect.ParameterizedType
-import java.lang.reflect.WildcardType
 import java.util.*
 import kotlin.coroutines.intrinsics.suspendCoroutineUninterceptedOrReturn
 import kotlin.reflect.full.valueParameters
@@ -82,15 +80,13 @@ internal class MethodFieldResolver(
                     return@add Optional.empty<Any>()
                 }
 
-                if (value != null
-                    && parameterType.unwrap().isAssignableFrom(value.javaClass)
-                    && isScalarType(environment, definition.type, parameterType)) {
-                    return@add value
+                if (value != null && shouldValueBeConverted(value, definition, parameterType, environment)) {
+                    return@add mapper.convertValue(value, object : TypeReference<Any>() {
+                        override fun getType() = parameterType
+                    })
                 }
 
-                return@add mapper.convertValue(value, object : TypeReference<Any>() {
-                    override fun getType() = parameterType
-                })
+                return@add value
             }
         }
 
@@ -110,22 +106,25 @@ internal class MethodFieldResolver(
         }
     }
 
-    private fun isScalarType(environment: DataFetchingEnvironment, type: Type<*>, genericParameterType: JavaType): Boolean =
-        when (type) {
+    private fun shouldValueBeConverted(value: Any, definition: InputValueDefinition, parameterType: JavaType, environment: DataFetchingEnvironment): Boolean {
+        return !parameterType.unwrap().isAssignableFrom(value.javaClass) || !isConcreteScalarType(environment, definition.type, parameterType)
+    }
+
+    /**
+     * A concrete scalar type is a scalar type where values always coerce to the same Java type. The ID scalar type is not concrete
+     * because values can be coerced to multiple different Java types (eg. String, Long, UUID). All values of a non-concrete scalar
+     * type must be converted to the target method parameter type.
+     */
+    private fun isConcreteScalarType(environment: DataFetchingEnvironment, type: Type<*>, genericParameterType: JavaType): Boolean {
+        return when (type) {
             is ListType -> List::class.java.isAssignableFrom(this.genericType.getRawClass(genericParameterType))
-                && isScalarType(environment, type.type, this.genericType.unwrapGenericType(genericParameterType))
-            is TypeName -> environment.graphQLSchema?.getType(type.name)?.let { isScalar(it) && !isJavaLanguageType(genericParameterType) }
+                && isConcreteScalarType(environment, type.type, this.genericType.unwrapGenericType(genericParameterType))
+            is TypeName -> environment.graphQLSchema?.getType(type.name)?.let { isScalar(it) && type.name != "ID" }
                 ?: false
-            is NonNullType -> isScalarType(environment, type.type, genericParameterType)
+            is NonNullType -> isConcreteScalarType(environment, type.type, genericParameterType)
             else -> false
         }
-
-    private fun isJavaLanguageType(type: JavaType): Boolean =
-        when (type) {
-            is ParameterizedType -> isJavaLanguageType(type.actualTypeArguments[0])
-            is WildcardType -> isJavaLanguageType(type.upperBounds[0])
-            else -> genericType.getRawClass(type).`package`.name == "java.lang"
-        }
+    }
 
     override fun scanForMatches(): List<TypeClassMatcher.PotentialMatch> {
         val unwrappedGenericType = genericType.unwrapGenericType(try {

src/test/groovy/graphql/kickstart/tools/BuiltInIdSpec.groovy

@@ -0,0 +1,134 @@
+package graphql.kickstart.tools
+
+import graphql.GraphQL
+import graphql.execution.AsyncExecutionStrategy
+import graphql.schema.GraphQLSchema
+import spock.lang.Shared
+import spock.lang.Specification
+
+class BuiltInIdSpec extends Specification {
+
+    @Shared
+    GraphQL gql
+
+    def setupSpec() {
+        GraphQLSchema schema = SchemaParser.newParser().schemaString('''\
+                        type Query {
+                            itemByLongId(id: ID!): Item1!
+                            itemsByLongIds(ids: [ID!]!): [Item1!]!
+                            itemByUuidId(id: ID!): Item2!
+                            itemsByUuidIds(ids: [ID!]!): [Item2!]!
+                        }
+                        
+                        type Item1 {
+                          id: ID!
+                        }
+                        
+                        type Item2 {
+                          id: ID!
+                        }
+                    '''.stripIndent())
+                .resolvers(new QueryWithLongItemResolver())
+                .build()
+                .makeExecutableSchema()
+        gql = GraphQL.newGraphQL(schema)
+                .queryExecutionStrategy(new AsyncExecutionStrategy())
+                .build()
+    }
+
+    def "supports Long as ID as input"() {
+        when:
+        def data = Utils.assertNoGraphQlErrors(gql) {
+            '''
+                {
+                    itemByLongId(id: 1) {
+                        id
+                    }
+                }
+                '''
+        }
+
+        then:
+        data.itemByLongId != null
+        data.itemByLongId.id == "1"
+    }
+
+    def "supports list of Long as ID as input"() {
+        when:
+        def data = Utils.assertNoGraphQlErrors(gql) {
+            '''
+                {
+                    itemsByLongIds(ids: [1,2,3]) {
+                        id
+                    }
+                }
+                '''
+        }
+
+        then:
+        data.itemsByLongIds != null
+        data.itemsByLongIds.size == 3
+        data.itemsByLongIds[0].id == "1"
+    }
+
+    def "supports UUID as ID as input"() {
+        when:
+        def data = Utils.assertNoGraphQlErrors(gql) {
+            '''
+                {
+                    itemByUuidId(id: "00000000-0000-0000-0000-000000000000") {
+                        id
+                    }
+                }
+                '''
+        }
+
+        then:
+        data.itemByUuidId != null
+        data.itemByUuidId.id == "00000000-0000-0000-0000-000000000000"
+    }
+
+    def "supports list of UUID as ID as input"() {
+        when:
+        def data = Utils.assertNoGraphQlErrors(gql) {
+            '''
+                {
+                    itemsByUuidIds(ids: ["00000000-0000-0000-0000-000000000000","11111111-1111-1111-1111-111111111111","22222222-2222-2222-2222-222222222222"]) {
+                        id
+                    }
+                }
+                '''
+        }
+
+        then:
+        data.itemsByUuidIds != null
+        data.itemsByUuidIds.size == 3
+        data.itemsByUuidIds[0].id == "00000000-0000-0000-0000-000000000000"
+    }
+
+    class QueryWithLongItemResolver implements GraphQLQueryResolver {
+        Item1 itemByLongId(Long id) {
+            new Item1(id: id)
+        }
+
+        List<Item1> itemsByLongIds(List<Long> ids) {
+            ids.collect { new Item1(id: it) }
+        }
+
+        Item2 itemByUuidId(UUID id) {
+            new Item2(id: id)
+        }
+
+        List<Item2> itemsByUuidIds(List<UUID> ids) {
+            ids.collect { new Item2(id: it) }
+        }
+    }
+
+    class Item1 {
+        Long id
+    }
+
+    class Item2 {
+        UUID id
+    }
+}