From f3b9aebb541bebd054155ad2eddbdc390ccc30f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernd=20St=C3=BCbinger?= <41049452+stuebingerb@users.noreply.github.com> Date: Thu, 7 Nov 2024 18:25:47 +0100 Subject: [PATCH] refactor: Make valueNode of ScalarCoercion required The `valueNode` of `ScalarCoercion` was nullable, leading to explicit `null` cases in various places. However, the code always calls it with a non-null `ValueNode`, cf. https://github.com/stuebingerb/KGraphQL/blob/main/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/Coercion.kt#L25 --- .../kgraphql/schema/builtin/BUILT_IN_TYPE.kt | 50 +++---------------- .../schema/dsl/types/BooleanScalarDSL.kt | 2 +- .../schema/dsl/types/DoubleScalarDSL.kt | 2 +- .../kgraphql/schema/dsl/types/IntScalarDSL.kt | 2 +- .../schema/dsl/types/LongScalarDSL.kt | 2 +- .../schema/dsl/types/ShortScalarDSL.kt | 2 +- .../schema/dsl/types/StringScalarDSL.kt | 2 +- .../kgraphql/schema/scalar/Coercion.kt | 2 +- .../kgraphql/schema/scalar/ScalarCoercion.kt | 2 +- .../com/apurebase/kgraphql/TestClasses.kt | 2 +- .../kgraphql/schema/SchemaBuilderTest.kt | 21 +------- .../language/InputValuesSpecificationTest.kt | 23 +++++++-- .../typesystem/ScalarsSpecificationTest.kt | 24 +++++---- 13 files changed, 51 insertions(+), 85 deletions(-) diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/builtin/BUILT_IN_TYPE.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/builtin/BUILT_IN_TYPE.kt index cb6f3990..b8adc443 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/builtin/BUILT_IN_TYPE.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/builtin/BUILT_IN_TYPE.kt @@ -4,8 +4,6 @@ package com.apurebase.kgraphql.schema.builtin import com.apurebase.kgraphql.GraphQLError import com.apurebase.kgraphql.defaultKQLTypeName -import com.apurebase.kgraphql.dropQuotes -import com.apurebase.kgraphql.isLiteral import com.apurebase.kgraphql.schema.model.TypeDef import com.apurebase.kgraphql.schema.model.ast.ValueNode import com.apurebase.kgraphql.schema.model.ast.ValueNode.BooleanValueNode @@ -43,7 +41,7 @@ object BUILT_IN_TYPE { val INT = TypeDef.Scalar(Int::class.defaultKQLTypeName(), Int::class, INT_COERCION, INT_DESCRIPTION) - //GraphQL does not differ float and double, treat double like float + // GraphQL does not differ float and double, treat double like float val DOUBLE = TypeDef.Scalar(Float::class.defaultKQLTypeName(), Double::class, DOUBLE_COERCION, FLOAT_DESCRIPTION) val FLOAT = TypeDef.Scalar(Float::class.defaultKQLTypeName(), Float::class, FLOAT_COERCION, FLOAT_DESCRIPTION) @@ -57,8 +55,7 @@ object BUILT_IN_TYPE { object STRING_COERCION : StringScalarCoercion { override fun serialize(instance: String): String = instance - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> raw.dropQuotes() + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is StringValueNode -> valueNode.value else -> throw GraphQLError( "Cannot coerce ${valueNode.valueNodeName} to string constant", @@ -70,12 +67,7 @@ object STRING_COERCION : StringScalarCoercion { object DOUBLE_COERCION : StringScalarCoercion { override fun serialize(instance: Double): String = instance.toString() - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> { - if (!raw.isLiteral()) raw.toDouble() - else throw GraphQLError("Cannot coerce string literal, expected numeric string constant") - } - + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is DoubleValueNode -> valueNode.value is NumberValueNode -> valueNode.value.toDouble() else -> throw GraphQLError( @@ -88,8 +80,7 @@ object DOUBLE_COERCION : StringScalarCoercion { object FLOAT_COERCION : StringScalarCoercion { override fun serialize(instance: Float): String = instance.toDouble().toString() - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> DOUBLE_COERCION.deserialize(raw).toFloat() + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is DoubleValueNode -> DOUBLE_COERCION.deserialize(raw, valueNode).toFloat() is NumberValueNode -> DOUBLE_COERCION.deserialize(raw, valueNode).toFloat() else -> throw GraphQLError( @@ -102,12 +93,7 @@ object FLOAT_COERCION : StringScalarCoercion { object INT_COERCION : StringScalarCoercion { override fun serialize(instance: Int): String = instance.toString() - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> { - if (!raw.isLiteral()) raw.toInt() - else throw GraphQLError("Cannot coerce string literal, expected numeric string constant") - } - + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is NumberValueNode -> when { valueNode.value > Integer.MAX_VALUE -> throw GraphQLError( "Cannot coerce to type of Int as '${valueNode.value}' is greater than (2^-31)-1", @@ -132,12 +118,7 @@ object INT_COERCION : StringScalarCoercion { object SHORT_COERCION : StringScalarCoercion { override fun serialize(instance: Short): String = instance.toString() - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> { - if (!raw.isLiteral()) raw.toShort() - else throw GraphQLError("Cannot coerce string literal, expected numeric string constant") - } - + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is NumberValueNode -> when { valueNode.value > Short.MAX_VALUE -> throw GraphQLError( "Cannot coerce to type of Int as '${valueNode.value}' is greater than (2^-15)-1", @@ -162,12 +143,7 @@ object SHORT_COERCION : StringScalarCoercion { object LONG_COERCION : StringScalarCoercion { override fun serialize(instance: Long): String = instance.toString() - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> { - if (!raw.isLiteral()) raw.toLong() - else throw GraphQLError("Cannot coerce string literal, expected numeric string constant") - } - + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is NumberValueNode -> valueNode.value else -> throw GraphQLError( "Cannot coerce ${valueNode.valueNodeName} to expected numeric constant", @@ -180,17 +156,7 @@ object LONG_COERCION : StringScalarCoercion { object BOOLEAN_COERCION : StringScalarCoercion { override fun serialize(instance: Boolean): String = instance.toString() - override fun deserialize(raw: String, valueNode: ValueNode?) = when (valueNode) { - null -> { - if (raw.isLiteral()) throw GraphQLError("Cannot coerce string literal, expected numeric string constant") - when { - //custom parsing, because String#toBoolean() returns false for any input != true - raw.equals("true", true) -> true - raw.equals("false", true) -> false - else -> throw IllegalArgumentException("$raw does not represent valid Boolean value") - } - } - + override fun deserialize(raw: String, valueNode: ValueNode) = when (valueNode) { is BooleanValueNode -> valueNode.value is StringValueNode -> when { valueNode.value.equals("true", true) -> true diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/BooleanScalarDSL.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/BooleanScalarDSL.kt index ca5d37ab..8863b9fc 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/BooleanScalarDSL.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/BooleanScalarDSL.kt @@ -17,7 +17,7 @@ class BooleanScalarDSL(kClass: KClass) : ScalarDSL(kClas override fun serialize(instance: T): Boolean = serializeImpl(instance) - override fun deserialize(raw: Boolean, valueNode: ValueNode?): T = deserializeImpl(raw) + override fun deserialize(raw: Boolean, valueNode: ValueNode): T = deserializeImpl(raw) } } } diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/DoubleScalarDSL.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/DoubleScalarDSL.kt index b8c41a6a..12087631 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/DoubleScalarDSL.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/DoubleScalarDSL.kt @@ -17,7 +17,7 @@ class DoubleScalarDSL(kClass: KClass) : ScalarDSL(kClass) override fun serialize(instance: T): Double = serializeImpl(instance) - override fun deserialize(raw: Double, valueNode: ValueNode?): T = deserializeImpl(raw) + override fun deserialize(raw: Double, valueNode: ValueNode): T = deserializeImpl(raw) } } } diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/IntScalarDSL.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/IntScalarDSL.kt index ee624626..89f9499a 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/IntScalarDSL.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/IntScalarDSL.kt @@ -17,7 +17,7 @@ class IntScalarDSL(kClass: KClass) : ScalarDSL(kClass) { override fun serialize(instance: T): Int = serializeImpl(instance) - override fun deserialize(raw: Int, valueNode: ValueNode?): T = deserializeImpl(raw) + override fun deserialize(raw: Int, valueNode: ValueNode): T = deserializeImpl(raw) } } } diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/LongScalarDSL.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/LongScalarDSL.kt index 5f2e79a8..e58cd812 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/LongScalarDSL.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/LongScalarDSL.kt @@ -17,7 +17,7 @@ class LongScalarDSL(kClass: KClass) : ScalarDSL(kClass) { override fun serialize(instance: T): Long = serializeImpl(instance) - override fun deserialize(raw: Long, valueNode: ValueNode?): T = deserializeImpl(raw) + override fun deserialize(raw: Long, valueNode: ValueNode): T = deserializeImpl(raw) } } } diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/ShortScalarDSL.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/ShortScalarDSL.kt index 0f2ed3fa..defb5e8b 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/ShortScalarDSL.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/ShortScalarDSL.kt @@ -17,7 +17,7 @@ class ShortScalarDSL(kClass: KClass) : ScalarDSL(kClass) { override fun serialize(instance: T): Short = serializeImpl(instance) - override fun deserialize(raw: Short, valueNode: ValueNode?): T = deserializeImpl(raw) + override fun deserialize(raw: Short, valueNode: ValueNode): T = deserializeImpl(raw) } } } diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/StringScalarDSL.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/StringScalarDSL.kt index 3a12665f..a6843f5b 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/StringScalarDSL.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/dsl/types/StringScalarDSL.kt @@ -17,7 +17,7 @@ class StringScalarDSL(kClass: KClass) : ScalarDSL(kClass) override fun serialize(instance: T): String = serializeImpl(instance) - override fun deserialize(raw: String, valueNode: ValueNode?): T = deserializeImpl(raw) + override fun deserialize(raw: String, valueNode: ValueNode): T = deserializeImpl(raw) } } } diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/Coercion.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/Coercion.kt index c6f4eed9..5d7de2fc 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/Coercion.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/Coercion.kt @@ -25,7 +25,7 @@ private typealias JsonValueNode = com.fasterxml.jackson.databind.node.ValueNode fun deserializeScalar(scalar: Type.Scalar, value: ValueNode): T { try { return when (scalar.coercion) { - //built in scalars + // built-in scalars STRING_COERCION -> STRING_COERCION.deserialize(value.valueNodeName, value as StringValueNode) as T FLOAT_COERCION -> FLOAT_COERCION.deserialize(value.valueNodeName, value) as T DOUBLE_COERCION -> DOUBLE_COERCION.deserialize(value.valueNodeName, value) as T diff --git a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/ScalarCoercion.kt b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/ScalarCoercion.kt index 215aea89..ce622fda 100644 --- a/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/ScalarCoercion.kt +++ b/kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/scalar/ScalarCoercion.kt @@ -16,6 +16,6 @@ interface ScalarCoercion { /** * strategy for scalar deserialization */ - fun deserialize(raw: Raw, valueNode: ValueNode? = null): Scalar + fun deserialize(raw: Raw, valueNode: ValueNode): Scalar } diff --git a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestClasses.kt b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestClasses.kt index 20223507..a6f06a9a 100644 --- a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestClasses.kt +++ b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestClasses.kt @@ -25,7 +25,7 @@ class Id(val literal: String, val numeric: Int) class IdScalarSupport : StringScalarCoercion { override fun serialize(instance: Id): String = "${instance.literal}:${instance.numeric}" - override fun deserialize(raw: String, valueNode: ValueNode?) = Id(raw.split(':')[0], raw.split(':')[1].toInt()) + override fun deserialize(raw: String, valueNode: ValueNode) = Id(raw.split(':')[0], raw.split(':')[1].toInt()) } enum class FilmType { FULL_LENGTH, SHORT_LENGTH } diff --git a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/schema/SchemaBuilderTest.kt b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/schema/SchemaBuilderTest.kt index ea311ab1..5f28cadf 100644 --- a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/schema/SchemaBuilderTest.kt +++ b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/schema/SchemaBuilderTest.kt @@ -17,6 +17,7 @@ import com.apurebase.kgraphql.schema.dsl.SchemaBuilder import com.apurebase.kgraphql.schema.dsl.types.TypeDSL import com.apurebase.kgraphql.schema.execution.DefaultGenericTypeResolver import com.apurebase.kgraphql.schema.introspection.TypeKind +import com.apurebase.kgraphql.schema.model.ast.ValueNode import com.apurebase.kgraphql.schema.scalar.StringScalarCoercion import com.apurebase.kgraphql.schema.structure.Field import kotlinx.coroutines.Dispatchers @@ -41,22 +42,6 @@ import kotlin.reflect.typeOf * Tests for SchemaBuilder behaviour, not request execution */ class SchemaBuilderTest { - @Suppress("UNCHECKED_CAST") - @Test - fun `DSL created UUID scalar support`() { - val testedSchema = defaultSchema { - stringScalar { - description = "unique identifier of object" - deserialize = { uuid: String -> UUID.fromString(uuid) } - serialize = UUID::toString - } - } - - val uuidScalar = testedSchema.model.scalars[UUID::class]!!.coercion as StringScalarCoercion - val testUuid = UUID.randomUUID() - assertThat(uuidScalar.serialize(testUuid), equalTo(testUuid.toString())) - assertThat(uuidScalar.deserialize(testUuid.toString()), equalTo(testUuid)) - } @Test fun `ignored property DSL`() { @@ -107,7 +92,6 @@ class SchemaBuilderTest { resolver { -> Scenario(Id("GKalus", 234234), "Gamil Kalus", "TOO LONG") } } type { - transformation(Scenario::content) { content: String, capitalized: Boolean? -> if (capitalized == true) content.replaceFirstChar { it.uppercase() } else content } @@ -140,7 +124,6 @@ class SchemaBuilderTest { assertThat(scenarioType.kind, equalTo(TypeKind.OBJECT)) assertThat(scenarioType["pdf"], notNullValue()) - } @Test @@ -239,7 +222,7 @@ class SchemaBuilderTest { } @Test - fun ` _ is allowed as receiver argument name`() { + fun ` _ should be allowed as receiver argument name`() { val schema = defaultSchema { query("actor") { resolver { -> Actor("BoguĊ› Linda", 4343) } diff --git a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/language/InputValuesSpecificationTest.kt b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/language/InputValuesSpecificationTest.kt index 8e392adb..825e0b8d 100644 --- a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/language/InputValuesSpecificationTest.kt +++ b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/language/InputValuesSpecificationTest.kt @@ -15,6 +15,7 @@ import org.hamcrest.CoreMatchers.equalTo import org.hamcrest.MatcherAssert.assertThat import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource import org.junit.jupiter.params.provider.ValueSource @Specification("2.9 Input Values") @@ -76,12 +77,26 @@ class InputValuesSpecificationTest { assertThat(response.extract("data/Double"), equalTo(input)) } - @Test + @ParameterizedTest + @CsvSource( + value = [ + "true, true", + "false, false", + "\"true\", true", + "\"false\", false", + "\"TRUE\", true", + "\"FALSE\", false", + "\"tRuE\", true", + "\"faLSe\", false", + "1, true", + "0, false", + "-1, false" + ] + ) @Specification("2.9.3 Boolean Value") - fun `Boolean input value`() { - val input = true + fun `Boolean input value`(input: String, expected: Boolean) { val response = deserialize(schema.executeBlocking("{ Boolean(value: $input) }")) - assertThat(response.extract("data/Boolean"), equalTo(input)) + assertThat(response.extract("data/Boolean"), equalTo(expected)) } @ParameterizedTest diff --git a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/typesystem/ScalarsSpecificationTest.kt b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/typesystem/ScalarsSpecificationTest.kt index f5ca25d9..03b0796e 100644 --- a/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/typesystem/ScalarsSpecificationTest.kt +++ b/kgraphql/src/test/kotlin/com/apurebase/kgraphql/specification/typesystem/ScalarsSpecificationTest.kt @@ -33,7 +33,7 @@ class ScalarsSpecificationTest { coercion = object : StringScalarCoercion { override fun serialize(instance: UUID): String = instance.toString() - override fun deserialize(raw: String, valueNode: ValueNode?): UUID = UUID.fromString(raw) + override fun deserialize(raw: String, valueNode: ValueNode): UUID = UUID.fromString(raw) } } query("person") { @@ -88,7 +88,7 @@ class ScalarsSpecificationTest { stringScalar { name = "ID" description = "the unique identifier of object" - deserialize = { uuid: String -> UUID.fromString(uuid) } + deserialize = UUID::fromString serialize = UUID::toString } query("personById") { @@ -285,8 +285,6 @@ class ScalarsSpecificationTest { mutation("addPart") { description = "Adds a new part in the parts inventory database" resolver { newPart: NewPart -> - println(newPart) - newPart } } @@ -295,21 +293,25 @@ class ScalarsSpecificationTest { } val manufacturer = """Joe Bloggs""" + val addedDate = "2001-09-01" val response = deserialize( schema.executeBlocking( - "mutation Mutation(\$newPart : NewPart!){ addPart(newPart: \$newPart) {manufacturer} }", + "mutation Mutation(\$newPart: NewPart!) { addPart(newPart: \$newPart) { addedDate manufacturer } }", """ - { "newPart" : { - "manufacturer":"$manufacturer", - "name":"Front bumper", - "oem":true, - "addedDate":"2001-09-01" - }} + { + "newPart": { + "manufacturer": "$manufacturer", + "name": "Front bumper", + "oem": true, + "addedDate": "$addedDate" + } + } """.trimIndent() ) ) assertThat(response.extract("data/addPart/manufacturer"), equalTo(manufacturer)) + assertThat(response.extract("data/addPart/addedDate"), equalTo(addedDate)) } }