fix: Improve list input coercion (#113)

Resolves #112 along with two minor refactorings: * ListsSpecificationTest now directly writes variables as string without mapping them via Jackson * `ArgumentsHandler` has been merged into `ArgumentTransformer` and cleaned up
stuebingerb · Dec 5, 2024 · 50f7933 · 50f7933
2 parents 9fbb525 + 84280b4
commit 50f7933
Show file tree

Hide file tree

Showing 9 changed files with 204 additions and 147 deletions.
diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ArgumentTransformer.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ArgumentTransformer.kt
@@ -1,10 +1,13 @@
 package com.apurebase.kgraphql.schema.execution
 
-import com.apurebase.kgraphql.ExecutionException
+import com.apurebase.kgraphql.Context
 import com.apurebase.kgraphql.InvalidInputValueException
 import com.apurebase.kgraphql.request.Variables
 import com.apurebase.kgraphql.schema.DefaultSchema
+import com.apurebase.kgraphql.schema.introspection.TypeKind
+import com.apurebase.kgraphql.schema.model.ast.ArgumentNodes
 import com.apurebase.kgraphql.schema.model.ast.ValueNode
+import com.apurebase.kgraphql.schema.model.ast.ValueNode.ObjectValueNode
 import com.apurebase.kgraphql.schema.scalar.deserializeScalar
 import com.apurebase.kgraphql.schema.structure.InputValue
 import com.apurebase.kgraphql.schema.structure.Type
@@ -14,20 +17,80 @@ import kotlin.reflect.jvm.jvmErasure
 
 open class ArgumentTransformer(val schema: DefaultSchema) {
 
-    private fun transformValue(type: Type, value: ValueNode, variables: Variables): Any? {
+    fun transformArguments(
+        funName: String,
+        inputValues: List<InputValue<*>>,
+        args: ArgumentNodes?,
+        variables: Variables,
+        executionNode: Execution,
+        requestContext: Context
+    ): List<Any?> {
+        val unsupportedArguments = args?.filter { arg ->
+            inputValues.none { value -> value.name == arg.key }
+        }
+
+        if (unsupportedArguments?.isNotEmpty() == true) {
+            throw InvalidInputValueException(
+                "$funName does support arguments ${inputValues.map { it.name }}. Found arguments ${args.keys}",
+                executionNode.selectionNode
+            )
+        }
+
+        return inputValues.map { parameter ->
+            val value = args?.get(parameter.name)
+
+            when {
+                // inject request context
+                parameter.type.isInstance(requestContext) -> requestContext
+                parameter.type.isInstance(executionNode) -> executionNode
+                value == null && parameter.type.kind != TypeKind.NON_NULL -> parameter.default
+                value == null && parameter.type.kind == TypeKind.NON_NULL -> {
+                    parameter.default ?: throw InvalidInputValueException(
+                        "argument '${parameter.name}' of type ${schema.typeReference(parameter.type)} on field '$funName' is not nullable, value cannot be null",
+                        executionNode.selectionNode
+                    )
+                }
+
+                else -> {
+                    val transformedValue = transformValue(parameter.type, value!!, variables, true)
+                    if (transformedValue == null && parameter.type.isNotNullable()) {
+                        throw InvalidInputValueException(
+                            "argument ${parameter.name} is not optional, value cannot be null",
+                            executionNode.selectionNode
+                        )
+                    }
+                    transformedValue
+                }
+            }
+        }
+    }
+
+    private fun transformValue(
+        type: Type,
+        value: ValueNode,
+        variables: Variables,
+        // Normally, single values should be coerced to a list with one element. But not if that single value is a
+        // list of a nested list. Seems strange but cf. https://spec.graphql.org/October2021/#sec-List.Input-Coercion
+        // This parameter is used to track if we have seen a ListValueNode in the recursive call chain.
+        coerceSingleValueAsList: Boolean
+    ): Any? {
         val kType = type.toKType()
         val typeName = type.unwrapped().name
 
         return when {
             value is ValueNode.VariableNode -> {
                 variables.get(kType.jvmErasure, kType, typeName, value) { subValue ->
-                    transformValue(type, subValue, variables)
+                    transformValue(type, subValue, variables, coerceSingleValueAsList)
                 }
             }
 
-            type.isList() && value !is ValueNode.ListValueNode -> {
-                if (type.isNullable() && value is ValueNode.NullValueNode) {
-                    null
+            // https://spec.graphql.org/October2021/#sec-List.Input-Coercion
+            // If the value passed as an input to a list type is not a list and not the null value, then the result
+            // of input coercion is a list of size one, where the single item value is the result of input coercion
+            // for the list's item type on the provided value (note this may apply recursively for nested lists).
+            type.isList() && value !is ValueNode.ListValueNode && value !is ValueNode.NullValueNode -> {
+                if (coerceSingleValueAsList) {
+                    listOf(transformValue(type.unwrapList(), value, variables, true))
                 } else {
                     throw InvalidInputValueException(
                         "argument '${value.valueNodeName}' is not valid value of type List",
@@ -36,7 +99,7 @@ open class ArgumentTransformer(val schema: DefaultSchema) {
                 }
             }
 
-            value is ValueNode.ObjectValueNode -> {
+            value is ObjectValueNode -> {
                 // SchemaCompilation ensures that input types have a primaryConstructor
                 val constructor = checkNotNull(type.unwrapped().kClass?.primaryConstructor)
                 val params = constructor.parameters.associateBy { it.name }
@@ -56,7 +119,7 @@ open class ArgumentTransformer(val schema: DefaultSchema) {
                             value
                         )
 
-                    params.getValue(valueField.name.value) to transformValue(paramType, valueField.value, variables)
+                    params.getValue(valueField.name.value) to transformValue(paramType, valueField.value, variables, coerceSingleValueAsList)
                 }
 
                 val missingNonOptionalInputs = params.values.filter { !it.isOptional && !valueMap.containsKey(it) }
@@ -88,7 +151,7 @@ open class ArgumentTransformer(val schema: DefaultSchema) {
                     )
                 } else {
                     value.values.map { valueNode ->
-                        transformValue(type.unwrapList(), valueNode, variables)
+                        transformValue(type.unwrapList(), valueNode, variables, false)
                     }
                 }
             }
@@ -124,24 +187,4 @@ open class ArgumentTransformer(val schema: DefaultSchema) {
             value
         )
     }
-
-    fun transformCollectionElementValue(inputValue: InputValue<*>, value: ValueNode, variables: Variables): Any? {
-        assert(inputValue.type.isList())
-        val elementType = inputValue.type.unwrapList().ofType as Type?
-            ?: throw ExecutionException("Unable to handle value of element of collection without type", value)
-
-        return transformValue(elementType, value, variables)
-    }
-
-    fun transformPropertyValue(parameter: InputValue<*>, value: ValueNode, variables: Variables): Any? {
-        return transformValue(parameter.type, value, variables)
-    }
-
-    fun transformPropertyObjectValue(
-        parameter: InputValue<*>,
-        value: ValueNode.ObjectValueNode,
-        variables: Variables
-    ): Any? {
-        return transformValue(parameter.type, value, variables)
-    }
 }
diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ArgumentsHandler.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ArgumentsHandler.kt
diff --git a/.../main/kotlin/com/apurebase/kgraphql/schema/execution/DataLoaderPreparedRequestExecutor.kt b/.../main/kotlin/com/apurebase/kgraphql/schema/execution/DataLoaderPreparedRequestExecutor.kt
@@ -28,7 +28,7 @@ import kotlin.reflect.KProperty1
 
 class DataLoaderPreparedRequestExecutor(val schema: DefaultSchema) : RequestExecutor {
 
-    private val argumentsHandler = ArgumentsHandler(schema)
+    private val argumentsHandler = ArgumentTransformer(schema)
 
     inner class ExecutionContext(
         val variables: Variables,

diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ParallelRequestExecutor.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ParallelRequestExecutor.kt
@@ -30,7 +30,7 @@ class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecutor {
         val requestContext: Context
     )
 
-    private val argumentsHandler = ArgumentsHandler(schema)
+    private val argumentsHandler = ArgumentTransformer(schema)
 
     private val jsonNodeFactory = JsonNodeFactory.instance
 

diff --git a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestUtils.kt b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestUtils.kt
@@ -84,6 +84,3 @@ fun Any.getResourceAsFile(name: String): File = this::class.java.classLoader.get
 object ResourceFiles {
     val kitchenSinkQuery = getResourceAsFile("kitchen-sink.graphql").readText()
 }
-
-
-const val d = '$'
diff --git a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/request/ParserTest.kt b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/request/ParserTest.kt
@@ -2,7 +2,6 @@ package com.apurebase.kgraphql.request
 
 import com.apurebase.kgraphql.GraphQLError
 import com.apurebase.kgraphql.ResourceFiles.kitchenSinkQuery
-import com.apurebase.kgraphql.d
 import com.apurebase.kgraphql.schema.model.ast.DefinitionNode.ExecutableDefinitionNode.OperationDefinitionNode
 import com.apurebase.kgraphql.schema.model.ast.DocumentNode
 import com.apurebase.kgraphql.schema.model.ast.OperationTypeNode.QUERY
@@ -217,7 +216,7 @@ class ParserTest {
                 |  ... on $keyword { field }
                 |}
                 |fragment $fragmentName on Type {
-                |  $keyword($keyword: ${d}$keyword)
+                |  $keyword($keyword: ${'$'}$keyword)
                 |    @$keyword($keyword: $keyword)
                 |}
             """.trimMargin()

diff --git a/...test/kotlin/com/apurebase/kgraphql/specification/language/InputValuesSpecificationTest.kt b/...test/kotlin/com/apurebase/kgraphql/specification/language/InputValuesSpecificationTest.kt
@@ -2,7 +2,6 @@ package com.apurebase.kgraphql.specification.language
 
 import com.apurebase.kgraphql.GraphQLError
 import com.apurebase.kgraphql.Specification
-import com.apurebase.kgraphql.d
 import com.apurebase.kgraphql.defaultSchema
 import com.apurebase.kgraphql.deserialize
 import com.apurebase.kgraphql.extract
@@ -174,13 +173,13 @@ class InputValuesSpecificationTest {
     }
 
     @ParameterizedTest
-    @ValueSource(strings = ["null", "true", "42", "\"foo\""])
+    @ValueSource(strings = ["true", "\"foo\""])
     @Specification("2.9.7 List Value")
     fun `Invalid List input value`(value: String) {
         invoking {
             deserialize(schema.executeBlocking("{ List(value: $value) }"))
         } shouldThrow GraphQLError::class with {
-            message shouldBeEqualTo "argument '$value' is not valid value of type List"
+            message shouldBeEqualTo "Cannot coerce $value to numeric constant"
             extensionsErrorType shouldBeEqualTo "BAD_USER_INPUT"
         }
     }
@@ -195,13 +194,13 @@ class InputValuesSpecificationTest {
     }
 
     @ParameterizedTest
-    @ValueSource(strings = ["null", "true", "42", "\"foo\""])
+    @ValueSource(strings = ["null", "true", "42"])
     @Specification("2.9.8 Object Value")
     fun `Invalid Literal object input value`(value: String) {
         invoking {
             schema.executeBlocking("{ Object(value: { number: 232, description: \"little number\", list: $value }) }")
         } shouldThrow GraphQLError::class with {
-            message shouldBeEqualTo "argument '$value' is not valid value of type List"
+            message shouldBeEqualTo "argument '$value' is not valid value of type String"
             extensionsErrorType shouldBeEqualTo "BAD_USER_INPUT"
         }
     }
@@ -273,11 +272,11 @@ class InputValuesSpecificationTest {
     fun `Input object value mixed with variables`() {
         val response = schema.executeBlocking(
             """
-            query ObjectVariablesMixed(${d}description: String!, ${d}number: Int! = 25) {
+            query ObjectVariablesMixed(${'$'}description: String!, ${'$'}number: Int! = 25) {
                 ObjectList(value: {
-                    number: ${d}number,
-                    description: ${d}description,
-                    list: ["number", ${d}description, "little number"]
+                    number: ${'$'}number,
+                    description: ${'$'}description,
+                    list: ["number", ${'$'}description, "little number"]
                 })
             }
         """.trimIndent(), """{ "description": "Custom description" }"""