From f42670c2349911838f79e6215ac1edce0287df7f Mon Sep 17 00:00:00 2001 From: Oryan Date: Mon, 12 Oct 2026 15:47:32 -0400 Subject: [PATCH 1/2] Keep coerced custom scalar values in input objects and Map arguments Arguments that need converting go through ObjectMapper.convertValue, which serializes the values graphql-java already coerced and deserializes them again. Custom scalar values Jackson can't do that for, such as an upload or a LocalDate without the jsr310 module, failed inside input objects (#436, #466), and a Map argument got bean-like scalar values as a nested Map (#424). Arguments are still converted by the ObjectMapper first, so anything that converts today gets the same result. If that fails, the values that don't convert to their own class are written as embedded objects, which Jackson's deserializers return as they are, with a problem handler for interfaces such as Part. If that fails too, its error is thrown, with the original one suppressed. Nothing that reads an untyped value, such as those of a Map, depends on the serialized form, so there the scalar values are kept unless the mapper has a serializer for them, e.g. UUID, or java.time with the jsr310 module. Co-Authored-By: Claude Opus 5.5 --- .../tools/resolver/ArgumentConverter.kt | 140 ++++++ .../tools/resolver/MethodFieldResolver.kt | 6 +- .../kickstart/tools/CustomScalarInputTest.kt | 466 ++++++++++++++++++ 3 files changed, 608 insertions(+), 4 deletions(-) create mode 100644 src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt create mode 100644 src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt new file mode 100644 index 00000000..318d2d48 --- /dev/null +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt @@ -0,0 +1,140 @@ +package graphql.kickstart.tools.resolver + +import com.fasterxml.jackson.core.JsonGenerator +import com.fasterxml.jackson.core.JsonParser +import com.fasterxml.jackson.core.JsonToken +import com.fasterxml.jackson.databind.DeserializationContext +import com.fasterxml.jackson.databind.DeserializationFeature +import com.fasterxml.jackson.databind.JavaType +import com.fasterxml.jackson.databind.JsonMappingException +import com.fasterxml.jackson.databind.JsonSerializable +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.databind.SerializationFeature +import com.fasterxml.jackson.databind.SerializerProvider +import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler +import com.fasterxml.jackson.databind.deser.ValueInstantiator +import com.fasterxml.jackson.databind.jsontype.TypeSerializer +import com.fasterxml.jackson.databind.ser.impl.UnknownSerializer +import com.fasterxml.jackson.databind.ser.impl.UnsupportedTypeSerializer +import com.fasterxml.jackson.databind.ser.std.BeanSerializerBase +import com.fasterxml.jackson.databind.util.TokenBuffer +import java.io.IOException +import java.lang.reflect.Type +import java.util.concurrent.ConcurrentHashMap + +/** + * Converts an argument value that graphql-java already coerced to the type of the resolver method's parameter. + * + * Jackson converts a value by serializing it and deserializing it again, which the coerced values of custom scalars + * often don't survive, e.g. an upload, or a LocalDate without the jsr310 module. Those are handed to Jackson as embedded + * objects instead, which its deserializers return as they are if they have the type being read. Where a String is read, + * they're converted with toString(). + */ +internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) { + + private val javaType = mapper.typeFactory.constructType(type) + private val isUntyped = javaType.isUntyped() + private val convertsToOwnClassCache = ConcurrentHashMap, Boolean>() + + fun convert(value: Any?): Any? { + if (value == null) { + return mapper.convertValue(value, javaType) + } + + if (isUntyped) { + // nothing that reads an untyped value depends on its serialized form, so the scalar values are kept, + // unless the mapper has a serializer for them that defines that form + val embedded = embedValues(value) { !hasSerializerOfItsOwn(it) } + return if (embedded != null) readEmbedded(embedded) else mapper.convertValue(value, javaType) + } + + // a deserializer may depend on the serialized form, so values are only embedded if converting fails + return try { + mapper.convertValue(value, javaType) + } catch (e: IllegalArgumentException) { + val embedded = embedValues(value) { !convertsToOwnClass(it) } ?: throw e + try { + readEmbedded(embedded) + } catch (embeddedError: IllegalArgumentException) { + // the values Jackson couldn't convert likely hid the actual problem + throw embeddedError.apply { addSuppressed(e) } + } + } + } + + /** + * Returns a copy of the value with the scalar values [isEmbedded] matches wrapped in [EmbeddedValue], or null if there are none. + */ + private fun embedValues(value: Any, isEmbedded: (Any) -> Boolean): Any? { + var embedsAny = false + fun embed(value: Any?): Any? = when (value) { + null, is String, is Number, is Boolean, is Enum<*> -> value + is Map<*, *> -> value.mapValuesTo(LinkedHashMap()) { embed(it.value) } + is Collection<*> -> value.map { embed(it) } + else -> if (isEmbedded(value)) EmbeddedValue(value).also { embedsAny = true } else value + } + + return embed(value).takeIf { embedsAny } + } + + // a bean serializer writes the value's properties, which an untyped value would get as a Map, the other two fail + private fun hasSerializerOfItsOwn(value: Any): Boolean { + val serializer = try { + mapper.serializerProviderInstance.findValueSerializer(value.javaClass) + } catch (e: JsonMappingException) { + return false + } + return serializer !is BeanSerializerBase && serializer !is UnknownSerializer && serializer !is UnsupportedTypeSerializer + } + + // the values of a scalar usually all convert or all don't, and converting one can be expensive, e.g. an upload + private fun convertsToOwnClass(value: Any): Boolean = convertsToOwnClassCache.getOrPut(value.javaClass) { + try { + mapper.convertValue(value, value.javaClass) + true + } catch (e: IllegalArgumentException) { + false + } + } + + // like ObjectMapper.convertValue, but reads embedded values + private fun readEmbedded(value: Any): Any? { + val buffer = TokenBuffer(mapper, false).forceUseOfBigDecimal(mapper.isEnabled(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)) + return try { + mapper.writer().without(SerializationFeature.WRAP_ROOT_VALUE).writeValue(buffer, value) + mapper.readerFor(javaType) + .without(DeserializationFeature.UNWRAP_ROOT_VALUE) + .withHandler(embeddedValueHandler) + .readValue(buffer.asParser()) + } catch (e: IOException) { + throw IllegalArgumentException(e.message, e) + } + } + + private val embeddedValueHandler = object : DeserializationProblemHandler() { + // an interface or abstract class, e.g. Part, has no deserializer that returns embedded values + override fun handleMissingInstantiator( + context: DeserializationContext, + instClass: Class<*>, + valueInstantiator: ValueInstantiator?, + p: JsonParser, + msg: String? + ): Any? { + val value = if (p.hasToken(JsonToken.VALUE_EMBEDDED_OBJECT)) p.embeddedObject else null + return if (instClass.isInstance(value)) value else NOT_HANDLED + } + + // a bean of another class, which the value is converted to on its own + override fun handleWeirdNativeValue(context: DeserializationContext, targetType: JavaType, valueToConvert: Any, p: JsonParser): Any? = + mapper.convertValue(valueToConvert, targetType) + } + + private class EmbeddedValue(private val value: Any) : JsonSerializable { + override fun serialize(gen: JsonGenerator, serializers: SerializerProvider) = gen.writeEmbeddedObject(value) + + override fun serializeWithType(gen: JsonGenerator, serializers: SerializerProvider, typeSer: TypeSerializer) = gen.writeEmbeddedObject(value) + } +} + +// Object, or a Map, Collection, array or Optional of it, is read by UntypedObjectDeserializer, which returns embedded values as they are +private fun JavaType.isUntyped(): Boolean = isJavaLangObject || (isContainerType || isReferenceType) && contentType.isUntyped() diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index 0a9e65d6..5d7df72f 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -1,6 +1,5 @@ package graphql.kickstart.tools.resolver -import com.fasterxml.jackson.core.type.TypeReference import graphql.GraphQLContext import graphql.kickstart.tools.* import graphql.kickstart.tools.SchemaParserOptions.GenericWrapper @@ -77,6 +76,7 @@ internal class MethodFieldResolver( val isNonNull = definition.type is NonNullType val isOptional = this.typeResolver.getRawClass(parameterType) == Optional::class.java + val argumentConverter = ArgumentConverter(mapper, parameterType) args.add { environment -> val argumentPresent = environment.arguments.containsKey(definition.name) @@ -94,9 +94,7 @@ internal class MethodFieldResolver( } if (value == null || shouldValueBeConverted(value, definition, parameterType, environment)) { - return@add mapper.convertValue(value, object : TypeReference() { - override fun getType() = parameterType - }) + return@add argumentConverter.convert(value) } return@add value diff --git a/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt b/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt new file mode 100644 index 00000000..7e16176c --- /dev/null +++ b/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt @@ -0,0 +1,466 @@ +package graphql.kickstart.tools + +import com.fasterxml.jackson.annotation.JsonInclude +import com.fasterxml.jackson.core.JsonGenerator +import com.fasterxml.jackson.core.JsonParser +import com.fasterxml.jackson.databind.DeserializationContext +import com.fasterxml.jackson.databind.JsonNode +import com.fasterxml.jackson.databind.SerializerProvider +import com.fasterxml.jackson.databind.annotation.JsonDeserialize +import com.fasterxml.jackson.databind.deser.std.StdDeserializer +import com.fasterxml.jackson.databind.module.SimpleModule +import com.fasterxml.jackson.databind.node.ObjectNode +import com.fasterxml.jackson.databind.node.POJONode +import com.fasterxml.jackson.databind.ser.std.StdSerializer +import com.fasterxml.jackson.databind.ser.std.ToStringSerializer +import graphql.ExceptionWhileDataFetching +import graphql.ExecutionInput +import graphql.GraphQL +import graphql.GraphQLContext +import graphql.execution.CoercedVariables +import graphql.language.StringValue +import graphql.language.Value +import graphql.schema.Coercing +import graphql.schema.GraphQLScalarType +import kotlinx.coroutines.ExperimentalCoroutinesApi +import org.junit.Test +import java.time.LocalDate +import java.util.* +import javax.servlet.http.Part + +@OptIn(ExperimentalCoroutinesApi::class) +class CustomScalarInputTest { + + @Test + fun `custom scalars in input objects are passed to the resolver`() { + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + scalar Opaque + + type Query { + event(input: EventInput!): String! + events(inputs: [EventInput!]!): [String!]! + optionalEvent(input: EventInput): String! + eventNode(input: EventNodeInput!): String! + } + + input EventInput { + date: LocalDate + opaque: Opaque + } + + input EventNodeInput { + date: LocalDate + } + """) + .scalars(localDateScalar, opaqueScalar) + .resolvers(EventQueryResolver()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + val data = assertNoGraphQlErrors(gql, mapOf("input" to mapOf("date" to "2021-03-04", "opaque" to "variable"))) { + """ + query(${'$'}input: EventInput!) { + literal: event(input: { date: "2020-01-02", opaque: "literal" }) + variable: event(input: ${'$'}input) + events(inputs: [{ date: "2020-01-02" }, { opaque: "second" }]) + optionalEvent(input: { date: "2020-01-02", opaque: "optional" }) + eventNode(input: { date: "2020-01-02" }) + } + """ + } + + assertEquals(data, mapOf( + "literal" to "2020-01-02 literal", + "variable" to "2021-03-04 variable", + "events" to listOf("2020-01-02 null", "null second"), + "optionalEvent" to "2020-01-02 optional", + "eventNode" to "LocalDate 2020-01-02" + )) + } + + @Test + fun `uploads in input objects are passed to the resolver`() { + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar Upload + + type Query { + upload(input: UploadInput!): String! + javaUpload(input: JavaUploadInput!): String! + } + + input UploadInput { + name: String! + file: Upload + files: [Upload!] + } + + input JavaUploadInput { + file: Upload + } + """) + .scalars(uploadScalar) + .resolvers(UploadQueryResolver()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + val files = mapOf("file" to MockPart("a.txt", "Hello"), "files" to listOf(MockPart("b.txt", "big"), MockPart("c.txt", "world"))) + val data = assertNoGraphQlErrors(gql, files) { + """ + query(${'$'}file: Upload, ${'$'}files: [Upload!]) { + upload(input: { name: "greeting", file: ${'$'}file, files: ${'$'}files }) + javaUpload(input: { file: ${'$'}file }) + } + """ + } + + assertEquals(data, mapOf( + "upload" to "greeting: Hello big world", + "javaUpload" to "a.txt: Hello" + )) + } + + @Test + fun `custom scalars in Map arguments are passed as coerced values`() { + val resolver = MapQueryResolver() + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + scalar Money + scalar UUID + + type Query { + event(input: EventInput!): String! + eventMap(input: EventMapInput!): Boolean! + } + + input EventInput { + date: LocalDate + price: Money + id: UUID + } + + input EventMapInput { + name: String + date: LocalDate + price: Money + id: UUID + next: EventMapInput + } + """) + .scalars(localDateScalar, moneyScalar, uuidScalar) + .resolvers(resolver) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + assertNoGraphQlErrors(gql) { + """ + query { + eventMap(input: { price: "5" }) + } + """ + } + + assertEquals(resolver.input, mapOf("price" to Money(5, "CAD"))) + + assertNoGraphQlErrors(gql) { + """ + query { + eventMap(input: { + name: null + date: "2020-01-02" + price: "5" + id: "4dbd9a3d-4a26-4d10-9bb2-5b2dc4c8a0f0" + next: { date: "2021-03-04" } + }) + } + """ + } + + // the ObjectMapper has a serializer for UUID, so that still arrives in its serialized form + assertEquals(resolver.input, mapOf( + "name" to null, + "date" to LocalDate.of(2020, 1, 2), + "price" to Money(5, "CAD"), + "id" to "4dbd9a3d-4a26-4d10-9bb2-5b2dc4c8a0f0", + "next" to mapOf("date" to LocalDate.of(2021, 3, 4)) + )) + } + + @Test + fun `custom scalars the ObjectMapper can convert are still converted by it`() { + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + scalar Money + scalar Opaque + scalar GeoPoint + + type Query { + payment(input: PaymentInput!): String! + paymentNode(input: PaymentNodeInput!): String! + price(price: Money!): String! + } + + input PaymentInput { + price: Money + reference: Opaque + location: GeoPoint + date: LocalDate + } + + input PaymentNodeInput { + price: Money + } + """) + .scalars(localDateScalar, moneyScalar, opaqueScalar, geoPointScalar) + .resolvers(PaymentQueryResolver()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + val data = assertNoGraphQlErrors(gql) { + """ + query { + payment(input: { price: "5", reference: "ref", location: "1.5,2.5" }) + withDate: payment(input: { price: "5", reference: "ref", location: "1.5,2.5", date: "2020-01-02" }) + paymentNode(input: { price: "5" }) + price(price: "5") + } + """ + } + + assertEquals(data, mapOf( + "payment" to "MoneyDto(amount=5, currency=CAD) OpaqueDto(value=ref) GeoPoint(lat=1.5, lng=2.5) null", + "withDate" to "MoneyDto(amount=5, currency=CAD) OpaqueDto(value=ref) GeoPoint(lat=1.5, lng=2.5) 2020-01-02", + "paymentNode" to """{"price":{"amount":5,"currency":"CAD"}}""", + "price" to "MoneyDto(amount=5, currency=CAD)" + )) + } + + @Test + fun `custom scalars in inputs are converted by a configured ObjectMapper`() { + val resolver = MapQueryResolver() + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + scalar Upload + + type Query { + event(input: EventInput!): String! + eventMap(input: EventMapInput!): Boolean! + javaUpload(input: JavaUploadInput!): String! + } + + input EventInput { + date: LocalDate + } + + input EventMapInput { + name: String + date: LocalDate + } + + input JavaUploadInput { + file: Upload + } + """) + .scalars(localDateScalar, uploadScalar) + .resolvers(resolver, UploadQueryResolver()) + .options(SchemaParserOptions.newOptions() + .objectMapperConfigurer { mapper, _ -> + mapper.registerModule(SimpleModule() + .addSerializer(LocalDate::class.java, ToStringSerializer.instance) + .addDeserializer(LocalDate::class.java, NextDayDeserializer()) + .addSerializer(Part::class.java, PartSerializer()) + .addDeserializer(Part::class.java, PartDeserializer())) + mapper.setDefaultPropertyInclusion(JsonInclude.Include.NON_NULL) + } + .build()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + val data = assertNoGraphQlErrors(gql, mapOf("file" to MockPart("a.txt", "Hello"))) { + """ + query(${'$'}file: Upload) { + event(input: { date: "2020-01-02" }) + eventMap(input: { name: null, date: "2020-01-02" }) + javaUpload(input: { file: ${'$'}file }) + } + """ + } + + assertEquals(data, mapOf("event" to "2020-01-03 null", "eventMap" to true, "javaUpload" to "copy of a.txt: Hello")) + assertEquals(resolver.input, mapOf("date" to "2020-01-02")) + } + + @Test + fun `errors converting inputs with custom scalars name the actual problem`() { + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + + type Query { + defaulted(input: DefaultedInput!): String! + } + + input DefaultedInput { + name: String + date: LocalDate + } + """) + .scalars(localDateScalar) + .resolvers(DefaultedQueryResolver()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + val result = gql.execute(ExecutionInput.newExecutionInput().query( + """ + query { + defaulted(input: { name: null, date: "2020-01-02" }) + } + """)) + + assertEquals(result.errors.size, 1) + val exception = (result.errors[0] as ExceptionWhileDataFetching).exception + assert(exception.message.orEmpty().contains("JSON property name due to missing")) { exception.message.orEmpty() } + val conversionError = exception.suppressed.single() + assert(conversionError.message.orEmpty().startsWith("Java 8 date/time type `java.time.LocalDate` not supported")) { conversionError.message.orEmpty() } + } + + class EventQueryResolver : GraphQLQueryResolver { + fun event(input: EventInput): String = "${input.date} ${input.opaque?.value}" + fun events(inputs: List): List = inputs.map { event(it) } + fun optionalEvent(input: Optional): String = input.map { event(it) }.orElse("empty") + fun eventNode(input: ObjectNode): String = (input["date"] as POJONode).pojo.let { "${it.javaClass.simpleName} $it" } + } + + class EventInput { + var date: LocalDate? = null + var opaque: Opaque? = null + var price: Money? = null + var id: UUID? = null + } + + class UploadQueryResolver : GraphQLQueryResolver { + fun upload(input: UploadInput): String = "${input.name}: ${(listOfNotNull(input.file) + input.files).joinToString(" ") { it.content() }}" + fun javaUpload(input: JavaUploadInput): String = input.file?.let { "${it.name}: ${it.content()}" }.orEmpty() + + private fun Part.content() = String(inputStream.readBytes()) + } + + data class UploadInput(val name: String, val file: Part?, val files: List = emptyList()) + + class JavaUploadInput { + var file: Part? = null + } + + class MapQueryResolver : GraphQLQueryResolver { + var input: Map? = null + + // the scalars are only found through the fields of input classes, not of Map inputs + fun event(input: EventInput): String = "${input.date} ${input.price}" + + fun eventMap(input: Map): Boolean { + this.input = input + return true + } + } + + class PaymentQueryResolver : GraphQLQueryResolver { + fun payment(input: PaymentInput): String = "${input.price} ${input.reference} ${input.location} ${input.date}" + fun paymentNode(input: ObjectNode): String = input.toString() + fun price(price: MoneyDto): String = price.toString() + } + + class PaymentInput { + var price: MoneyDto? = null + var reference: OpaqueDto? = null + + @field:JsonDeserialize(using = GeoPointDeserializer::class) + var location: GeoPoint? = null + var date: LocalDate? = null + } + + class DefaultedQueryResolver : GraphQLQueryResolver { + fun defaulted(input: DefaultedInput): String = "${input.name} ${input.date}" + } + + data class DefaultedInput(val name: String = "default", val date: LocalDate? = null) + + /** + * Has no creator Jackson could use, like the coerced values of many custom scalars. + */ + class Opaque private constructor(private val internalValue: String) { + val value get() = internalValue + + companion object { + fun of(value: String) = Opaque(value) + } + } + + data class OpaqueDto(val value: String) + + data class Money(val amount: Int, val currency: String) + + data class MoneyDto(val amount: Int, val currency: String) + + data class GeoPoint(val lat: Double, val lng: Double) + + // reads the serialized form, and an embedded GeoPoint wouldn't fail it + class GeoPointDeserializer : StdDeserializer(GeoPoint::class.java) { + override fun deserialize(p: JsonParser, context: DeserializationContext): GeoPoint { + val node = p.codec.readTree(p) + return GeoPoint(node.path("lat").asDouble(), node.path("lng").asDouble()) + } + } + + class NextDayDeserializer : StdDeserializer(LocalDate::class.java) { + override fun deserialize(p: JsonParser, context: DeserializationContext): LocalDate = LocalDate.parse(p.text).plusDays(1) + } + + // the workaround from https://github.com/graphql-java-kickstart/graphql-java-tools/issues/466 + class PartSerializer : StdSerializer(Part::class.java) { + override fun serialize(value: Part, gen: JsonGenerator, provider: SerializerProvider) { + gen.writeStartObject() + gen.writeStringField("name", value.name) + gen.writeBinaryField("content", value.inputStream.readBytes()) + gen.writeEndObject() + } + } + + class PartDeserializer : StdDeserializer(Part::class.java) { + override fun deserialize(p: JsonParser, context: DeserializationContext): Part { + val node = p.codec.readTree(p) + return MockPart("copy of ${node["name"].textValue()}", String(node["content"].binaryValue())) + } + } + + private val localDateScalar = scalar("LocalDate") { LocalDate.parse(it) } + private val opaqueScalar = scalar("Opaque") { Opaque.of(it) } + private val moneyScalar = scalar("Money") { Money(it.toInt(), "CAD") } + private val uuidScalar = scalar("UUID") { UUID.fromString(it) } + private val geoPointScalar = scalar("GeoPoint") { value -> value.split(",").map { it.toDouble() }.let { GeoPoint(it[0], it[1]) } } + + private fun scalar(name: String, parse: (String) -> T): GraphQLScalarType = GraphQLScalarType.newScalar() + .name(name) + .coercing(object : Coercing { + override fun serialize(dataFetcherResult: Any, context: GraphQLContext, locale: Locale) = dataFetcherResult.toString() + override fun parseValue(input: Any, context: GraphQLContext, locale: Locale): T = parse(input as String) + override fun parseLiteral(input: Value<*>, variables: CoercedVariables, context: GraphQLContext, locale: Locale): T = parse((input as StringValue).value!!) + }) + .build() +} From 7c3750454dc16d2511c8ad4eb61caaa3860c66d9 Mon Sep 17 00:00:00 2001 From: Oryan Date: Mon, 12 Oct 2026 16:05:33 -0400 Subject: [PATCH 2/2] Pass every coerced scalar value to Map arguments A Map, Object or other untyped argument kept a scalar value only if the ObjectMapper had no serializer of its own for it, so a UUID still arrived as a String, a Date as a Long, and a LocalDate as an array once the jsr310 module was registered. Nothing that reads an untyped value depends on its serialized form, so these now get every coerced value other than strings, numbers, booleans and enums, whatever modules the mapper has (#424). Only the classes whose values failed to convert to their own class are remembered now. Whether a value converts can depend on what it holds, so a value that converted no longer stops a later one of the same class from being embedded. Co-Authored-By: Claude Opus 5.5 --- .../tools/resolver/ArgumentConverter.kt | 31 ++-- .../kickstart/tools/CustomScalarInputTest.kt | 160 +++++++++++++++++- 2 files changed, 165 insertions(+), 26 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt index 318d2d48..c946c61a 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/ArgumentConverter.kt @@ -6,7 +6,6 @@ import com.fasterxml.jackson.core.JsonToken import com.fasterxml.jackson.databind.DeserializationContext import com.fasterxml.jackson.databind.DeserializationFeature import com.fasterxml.jackson.databind.JavaType -import com.fasterxml.jackson.databind.JsonMappingException import com.fasterxml.jackson.databind.JsonSerializable import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.databind.SerializationFeature @@ -14,9 +13,6 @@ import com.fasterxml.jackson.databind.SerializerProvider import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler import com.fasterxml.jackson.databind.deser.ValueInstantiator import com.fasterxml.jackson.databind.jsontype.TypeSerializer -import com.fasterxml.jackson.databind.ser.impl.UnknownSerializer -import com.fasterxml.jackson.databind.ser.impl.UnsupportedTypeSerializer -import com.fasterxml.jackson.databind.ser.std.BeanSerializerBase import com.fasterxml.jackson.databind.util.TokenBuffer import java.io.IOException import java.lang.reflect.Type @@ -29,12 +25,15 @@ import java.util.concurrent.ConcurrentHashMap * often don't survive, e.g. an upload, or a LocalDate without the jsr310 module. Those are handed to Jackson as embedded * objects instead, which its deserializers return as they are if they have the type being read. Where a String is read, * they're converted with toString(). + * + * An untyped parameter, e.g. a Map, gets the coerced values as they are, apart from strings, numbers, + * booleans and enums, which are converted as before. */ internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) { private val javaType = mapper.typeFactory.constructType(type) private val isUntyped = javaType.isUntyped() - private val convertsToOwnClassCache = ConcurrentHashMap, Boolean>() + private val nonConvertingClasses: MutableSet> = ConcurrentHashMap.newKeySet() fun convert(value: Any?): Any? { if (value == null) { @@ -42,9 +41,8 @@ internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) { } if (isUntyped) { - // nothing that reads an untyped value depends on its serialized form, so the scalar values are kept, - // unless the mapper has a serializer for them that defines that form - val embedded = embedValues(value) { !hasSerializerOfItsOwn(it) } + // nothing that reads an untyped value depends on its serialized form, so the coerced values are kept + val embedded = embedValues(value) { true } return if (embedded != null) readEmbedded(embedded) else mapper.convertValue(value, javaType) } @@ -77,22 +75,17 @@ internal class ArgumentConverter(private val mapper: ObjectMapper, type: Type) { return embed(value).takeIf { embedsAny } } - // a bean serializer writes the value's properties, which an untyped value would get as a Map, the other two fail - private fun hasSerializerOfItsOwn(value: Any): Boolean { - val serializer = try { - mapper.serializerProviderInstance.findValueSerializer(value.javaClass) - } catch (e: JsonMappingException) { + // failing to convert can be expensive, e.g. an upload, so those classes are remembered. Whether a value converts + // can depend on what it holds, so values of the other classes are checked every time + private fun convertsToOwnClass(value: Any): Boolean { + if (value.javaClass in nonConvertingClasses) { return false } - return serializer !is BeanSerializerBase && serializer !is UnknownSerializer && serializer !is UnsupportedTypeSerializer - } - - // the values of a scalar usually all convert or all don't, and converting one can be expensive, e.g. an upload - private fun convertsToOwnClass(value: Any): Boolean = convertsToOwnClassCache.getOrPut(value.javaClass) { - try { + return try { mapper.convertValue(value, value.javaClass) true } catch (e: IllegalArgumentException) { + nonConvertingClasses.add(value.javaClass) false } } diff --git a/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt b/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt index 7e16176c..5a90a5c4 100644 --- a/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/CustomScalarInputTest.kt @@ -8,6 +8,8 @@ import com.fasterxml.jackson.databind.JsonNode import com.fasterxml.jackson.databind.SerializerProvider import com.fasterxml.jackson.databind.annotation.JsonDeserialize import com.fasterxml.jackson.databind.deser.std.StdDeserializer +import com.fasterxml.jackson.databind.exc.InvalidDefinitionException +import com.fasterxml.jackson.databind.exc.MismatchedInputException import com.fasterxml.jackson.databind.module.SimpleModule import com.fasterxml.jackson.databind.node.ObjectNode import com.fasterxml.jackson.databind.node.POJONode @@ -65,6 +67,7 @@ class CustomScalarInputTest { """ query(${'$'}input: EventInput!) { literal: event(input: { date: "2020-01-02", opaque: "literal" }) + opaqueOnly: event(input: { opaque: "alone" }) variable: event(input: ${'$'}input) events(inputs: [{ date: "2020-01-02" }, { opaque: "second" }]) optionalEvent(input: { date: "2020-01-02", opaque: "optional" }) @@ -75,6 +78,7 @@ class CustomScalarInputTest { assertEquals(data, mapOf( "literal" to "2020-01-02 literal", + "opaqueOnly" to "null alone", "variable" to "2021-03-04 variable", "events" to listOf("2020-01-02 null", "null second"), "optionalEvent" to "2020-01-02 optional", @@ -139,6 +143,9 @@ class CustomScalarInputTest { type Query { event(input: EventInput!): String! eventMap(input: EventMapInput!): Boolean! + rawMap(input: RawMapInput!): Boolean! + listMap(inputs: [ListMapInput!]!): Boolean! + optionalMap(input: OptionalMapInput): Boolean! } input EventInput { @@ -154,6 +161,18 @@ class CustomScalarInputTest { id: UUID next: EventMapInput } + + input RawMapInput { + price: Money + } + + input ListMapInput { + price: Money + } + + input OptionalMapInput { + price: Money + } """) .scalars(localDateScalar, moneyScalar, uuidScalar) .resolvers(resolver) @@ -181,18 +200,24 @@ class CustomScalarInputTest { id: "4dbd9a3d-4a26-4d10-9bb2-5b2dc4c8a0f0" next: { date: "2021-03-04" } }) + rawMap(input: { price: "6" }) + listMap(inputs: [{ price: "7" }, { price: "8" }]) + optionalMap(input: { price: "9" }) } """ } - // the ObjectMapper has a serializer for UUID, so that still arrives in its serialized form + // including those the ObjectMapper has a serializer for, e.g. UUID assertEquals(resolver.input, mapOf( "name" to null, "date" to LocalDate.of(2020, 1, 2), "price" to Money(5, "CAD"), - "id" to "4dbd9a3d-4a26-4d10-9bb2-5b2dc4c8a0f0", + "id" to UUID.fromString("4dbd9a3d-4a26-4d10-9bb2-5b2dc4c8a0f0"), "next" to mapOf("date" to LocalDate.of(2021, 3, 4)) )) + assertEquals(resolver.rawInput, mapOf("price" to Money(6, "CAD"))) + assertEquals(resolver.listInput, listOf(mapOf("price" to Money(7, "CAD")), mapOf("price" to Money(8, "CAD")))) + assertEquals(resolver.optionalInput, Optional.of(mapOf("price" to Money(9, "CAD")))) } @Test @@ -302,7 +327,8 @@ class CustomScalarInputTest { } assertEquals(data, mapOf("event" to "2020-01-03 null", "eventMap" to true, "javaUpload" to "copy of a.txt: Hello")) - assertEquals(resolver.input, mapOf("date" to "2020-01-02")) + // a Map gets the coerced value, not the serialized one, but the inclusion setting still applies + assertEquals(resolver.input, mapOf("date" to LocalDate.of(2020, 1, 2))) } @Test @@ -336,9 +362,89 @@ class CustomScalarInputTest { assertEquals(result.errors.size, 1) val exception = (result.errors[0] as ExceptionWhileDataFetching).exception - assert(exception.message.orEmpty().contains("JSON property name due to missing")) { exception.message.orEmpty() } - val conversionError = exception.suppressed.single() - assert(conversionError.message.orEmpty().startsWith("Java 8 date/time type `java.time.LocalDate` not supported")) { conversionError.message.orEmpty() } + assertEquals((exception.cause as MismatchedInputException).path.map { it.fieldName }, listOf("name")) + val conversionError = exception.suppressed.single().cause as InvalidDefinitionException + assertEquals(conversionError.type.rawClass, LocalDate::class.java) + } + + @Test + fun `custom scalars whose values only sometimes convert are passed to the resolver`() { + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + scalar Box + + type Query { + box(input: BoxInput!): String! + } + + input BoxInput { + box: Box + date: LocalDate + } + """) + .scalars(localDateScalar, boxScalar) + .resolvers(BoxQueryResolver()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + // a box of text converts, so only the date is embedded, but a box of a date has to be embedded too + val text = assertNoGraphQlErrors(gql) { + """ + query { + box(input: { box: "text", date: "2020-01-02" }) + } + """ + } + val date = assertNoGraphQlErrors(gql) { + """ + query { + box(input: { box: "date:2021-03-04" }) + } + """ + } + + assertEquals(text, mapOf("box" to "text 2020-01-02")) + assertEquals(date, mapOf("box" to "2021-03-04 null")) + } + + @Test + fun `custom scalars read as a String are converted with toString`() { + val schema = SchemaParser.newParser() + .schemaString( + """ + scalar LocalDate + scalar Opaque + + type Query { + label(input: LabelInput!): String! + } + + input LabelInput { + date: LocalDate + opaque: Opaque + } + """) + .scalars(localDateScalar, opaqueScalar) + .resolvers(LabelQueryResolver()) + .build() + .makeExecutableSchema() + val gql = GraphQL.newGraphQL(schema).build() + + val data = assertNoGraphQlErrors(gql) { + """ + query { + date: label(input: { date: "2020-01-02" }) + opaque: label(input: { opaque: "secret" }) + } + """ + } + + // a LocalDate gives its ISO date, but a class that doesn't override toString() gives a meaningless one + assertEquals(data["date"], "2020-01-02 null") + assert((data["opaque"] as String).startsWith("null ${Opaque::class.java.name}@")) { data["opaque"].toString() } } class EventQueryResolver : GraphQLQueryResolver { @@ -370,6 +476,9 @@ class CustomScalarInputTest { class MapQueryResolver : GraphQLQueryResolver { var input: Map? = null + var rawInput: LinkedHashMap<*, *>? = null + var listInput: List>? = null + var optionalInput: Optional>? = null // the scalars are only found through the fields of input classes, not of Map inputs fun event(input: EventInput): String = "${input.date} ${input.price}" @@ -378,6 +487,22 @@ class CustomScalarInputTest { this.input = input return true } + + // the signature from https://github.com/graphql-java-kickstart/graphql-java-tools/issues/424 + fun rawMap(input: LinkedHashMap<*, *>): Boolean { + rawInput = input + return true + } + + fun listMap(inputs: List>): Boolean { + listInput = inputs + return true + } + + fun optionalMap(input: Optional>): Boolean { + optionalInput = input + return true + } } class PaymentQueryResolver : GraphQLQueryResolver { @@ -401,8 +526,28 @@ class CustomScalarInputTest { data class DefaultedInput(val name: String = "default", val date: LocalDate? = null) + class BoxQueryResolver : GraphQLQueryResolver { + fun box(input: BoxInput): String = "${input.box?.content} ${input.date}" + } + + class BoxInput { + var box: Box? = null + var date: LocalDate? = null + } + + data class Box(val content: Any) + + class LabelQueryResolver : GraphQLQueryResolver { + fun label(input: LabelInput): String = "${input.date} ${input.opaque}" + } + + class LabelInput { + var date: String? = null + var opaque: String? = null + } + /** - * Has no creator Jackson could use, like the coerced values of many custom scalars. + * Can't be rebuilt from its serialized properties, like the coerced values of many custom scalars. */ class Opaque private constructor(private val internalValue: String) { val value get() = internalValue @@ -454,6 +599,7 @@ class CustomScalarInputTest { private val moneyScalar = scalar("Money") { Money(it.toInt(), "CAD") } private val uuidScalar = scalar("UUID") { UUID.fromString(it) } private val geoPointScalar = scalar("GeoPoint") { value -> value.split(",").map { it.toDouble() }.let { GeoPoint(it[0], it[1]) } } + private val boxScalar = scalar("Box") { Box(if (it.startsWith("date:")) LocalDate.parse(it.removePrefix("date:")) else it) } private fun scalar(name: String, parse: (String) -> T): GraphQLScalarType = GraphQLScalarType.newScalar() .name(name)