Merge pull request #891 from graphql-java-kickstart/feature/859-scalar-args-at-build

Check scalar arguments when building the schema

Author: oryan-block

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

@@ -58,7 +58,8 @@ internal class SchemaClassScanner(
     private val objectDefinitionsByName = objectDefinitions.associateBy { it.name }
     private val interfaceDefinitionsByName = definitionsByName.values.filterIsInstance<InterfaceTypeDefinition>().associateBy { it.name }
 
-    private val fieldResolverScanner = FieldResolverScanner(options)
+    private val scalarNames = ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS.keys + scalarDefinitions.map { it.name }
+    private val fieldResolverScanner = FieldResolverScanner(options, scalarNames)
     private val typeClassMatcher = TypeClassMatcher(definitionsByName)
     private val dictionary = mutableMapOf<TypeDefinition<*>, DictionaryEntry>()
     private val unvalidatedTypes = mutableSetOf<TypeDefinition<*>>()

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

@@ -21,7 +21,7 @@ import kotlin.reflect.jvm.kotlinFunction
 /**
  * @author Andrew Potter
  */
-internal class FieldResolverScanner(val options: SchemaParserOptions) {
+internal class FieldResolverScanner(val options: SchemaParserOptions, private val scalarNames: Set<String>) {
 
     private val log = LoggerFactory.getLogger(javaClass)
 
@@ -45,7 +45,7 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) {
     private fun findFieldResolver(field: FieldDefinition, search: Search, scanProperties: Boolean): FieldResolver? {
         val method = findResolverMethod(field, search)
         if (method != null) {
-            return MethodFieldResolver(field, search, options, method.apply(trySetAccessible(field, search.type)))
+            return MethodFieldResolver(field, search, options, method.apply(trySetAccessible(field, search.type)), scalarNames)
         }
 
         if (scanProperties) {

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

@@ -15,7 +15,6 @@ import graphql.language.*
 import graphql.schema.DataFetcher
 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
@@ -40,7 +39,8 @@ internal class MethodFieldResolver(
     field: FieldDefinition,
     search: FieldResolverScanner.Search,
     options: SchemaParserOptions,
-    val method: Method
+    val method: Method,
+    private val scalarNames: Set<String>
 ) : FieldResolver(field, search, options) {
 
     private val log = LoggerFactory.getLogger(javaClass)
@@ -78,6 +78,8 @@ internal class MethodFieldResolver(
             val isNonNull = definition.type is NonNullType
             val isOptional = this.typeResolver.getRawClass(parameterType) == Optional::class.java
             val argumentConverter = ArgumentConverter(mapper, parameterType)
+            val parameterClass = parameterType.unwrap()
+            val isConcreteScalar = isConcreteScalarType(definition.type, parameterType)
 
             args.add { environment ->
                 val argumentPresent = environment.arguments.containsKey(definition.name)
@@ -94,7 +96,7 @@ internal class MethodFieldResolver(
                     return@add Optional.empty<Any>()
                 }
 
-                if (value == null || shouldValueBeConverted(value, definition, parameterType, environment)) {
+                if (value == null || !isConcreteScalar || !parameterClass.isInstance(value)) {
                     return@add argumentConverter.convert(value)
                 }
 
@@ -141,24 +143,18 @@ internal class MethodFieldResolver(
         }
     }
 
-    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 {
+    private fun isConcreteScalarType(type: Type<*>, genericParameterType: JavaType): Boolean {
         return when (type) {
-            is ListType -> List::class.java.isAssignableFrom(this.typeResolver.getRawClass(genericParameterType))
-                && isConcreteScalarType(environment, type.type, this.typeResolver.resolve(genericParameterType))
-
-            is TypeName -> environment.graphQLSchema.getType(type.name!!)?.let { isScalar(it) && type.name != "ID" }
-                ?: false
+            is ListType -> genericParameterType.typeArgument(List::class.java)
+                ?.let { isConcreteScalarType(type.type, it) } ?: false
 
-            is NonNullType -> isConcreteScalarType(environment, type.type, genericParameterType)
+            is TypeName -> type.name in scalarNames && type.name != "ID"
+            is NonNullType -> isConcreteScalarType(type.type, genericParameterType)
             else -> false
         }
     }

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

@@ -18,7 +18,7 @@ import java.util.*
 class FieldResolverScannerTest {
 
     private val options = defaultOptions()
-    private val scanner = FieldResolverScanner(options)
+    private val scanner = FieldResolverScanner(options, GRAPHQL_SCALARS.keys)
 
     @Test
     fun `scanner finds fields on multiple root types`() {

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

@@ -2,7 +2,6 @@ package graphql.kickstart.tools
 
 import graphql.ExecutionResult
 import graphql.GraphQLContext
-import graphql.Scalars
 import graphql.execution.*
 import graphql.execution.instrumentation.SimplePerformantInstrumentation
 import graphql.kickstart.tools.resolver.FieldResolverError
@@ -14,8 +13,6 @@ import graphql.language.TypeName
 import graphql.schema.DataFetcher
 import graphql.schema.DataFetchingEnvironment
 import graphql.schema.DataFetchingEnvironmentImpl
-import graphql.schema.GraphQLObjectType
-import graphql.schema.GraphQLSchema
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.DelicateCoroutinesApi
 import kotlinx.coroutines.Dispatchers
@@ -421,7 +418,7 @@ class MethodFieldResolverDataFetcherTest {
         } else {
             NormalResolverInfo(resolver, options)
         }
-        return FieldResolverScanner(options).findFieldResolver(field, resolverInfo).createDataFetcher()
+        return FieldResolverScanner(options, GRAPHQL_SCALARS.keys).findFieldResolver(field, resolverInfo).createDataFetcher()
     }
 
     private fun createEnvironment(
@@ -446,11 +443,7 @@ class MethodFieldResolverDataFetcherTest {
             }
         }
         val executionId = ExecutionId.from("executionId123")
-        val schema = GraphQLSchema.newSchema()
-            .query(GraphQLObjectType.newObject().name("Query").field { it.name("active").type(Scalars.GraphQLBoolean) })
-            .build()
         return ExecutionContextBuilder.newExecutionContextBuilder()
-            .graphQLSchema(schema)
             .graphQLContext(context)
             .instrumentation(SimplePerformantInstrumentation.INSTANCE)
             .executionId(executionId)

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

@@ -4,6 +4,7 @@ import graphql.ExceptionWhileDataFetching
 import graphql.ExecutionInput
 import graphql.GraphQL
 import graphql.GraphQLContext
+import graphql.GraphQLError
 import graphql.execution.CoercedVariables
 import graphql.language.StringValue
 import graphql.language.Value
@@ -167,6 +168,31 @@ class MethodFieldResolverTest {
         assertEquals(result.getData(), mapOf("test" to 6))
     }
 
+    @Test
+    fun `should handle nested lists of scalar types`() {
+        val schema = SchemaParser.newParser()
+            .schemaString(
+                """
+                scalar CustomScalar
+                type Query {
+                    count(tags: [[String]]): Int
+                    test(input: [[CustomScalar]]): Int
+                }
+                """)
+            .scalars(customScalarType)
+            .resolvers(object : GraphQLQueryResolver {
+                fun count(tags: List<Set<String>>) = tags.sumOf { it.size }
+                fun test(scalars: List<Set<CustomScalar>>) = scalars.sumOf { set -> set.sumOf { it.value.length } }
+            })
+            .build()
+            .makeExecutableSchema()
+
+        val result = GraphQL.newGraphQL(schema).build().execute("""{ count(tags: [["a", "a", "b"]]) test(input: [["Foo"], ["Bar"]]) }""")
+
+        assertEquals(result.errors, emptyList<GraphQLError>())
+        assertEquals(result.getData(), mapOf("count" to 2, "test" to 6))
+    }
+
     @Test
     fun `should handle proxies`() {
         val invocationHandler = object : InvocationHandler {

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

@@ -47,7 +47,7 @@ class TypeClassMatcherTest {
             ) { x -> x }
         ).build()
 
-        private val scanner: FieldResolverScanner = FieldResolverScanner(options)
+        private val scanner: FieldResolverScanner = FieldResolverScanner(options, GRAPHQL_SCALARS.keys)
         private val resolver = RootResolverInfo(listOf(QueryMethods()), options)
 
         private fun createPotentialMatch(methodName: String, graphQLType: Type<*>): TypeClassMatcher.PotentialMatch {