From 5cdf5db28758a38afaf02a8e4f841335ea3eefd1 Mon Sep 17 00:00:00 2001 From: oryan Date: Tue, 5 Jan 2021 09:52:29 -0500 Subject: [PATCH 1/3] Rework input parsing in MethodFieldResolver to handle scalars within input objects --- .../graphql/kickstart/tools/SchemaParser.kt | 4 +- .../tools/resolver/MethodFieldResolver.kt | 188 +++++++++++++++--- .../kickstart/tools/EndToEndSpec.groovy | 14 ++ .../MethodFieldResolverDataFetcherSpec.groovy | 19 ++ .../kickstart/tools/EndToEndSpecHelper.kt | 8 + .../tools/MethodFieldResolverTest.kt | 9 +- 6 files changed, 206 insertions(+), 36 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index b53d6c24..e6a167d2 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -363,8 +363,8 @@ class SchemaParser internal constructor( private fun buildDefaultValue(value: Value<*>?): Any? { return when (value) { null -> null - is IntValue -> value.value - is FloatValue -> value.value + is IntValue -> value.value.toInt() + is FloatValue -> value.value.toDouble() is StringValue -> value.value is EnumValue -> value.name is BooleanValue -> value.isValue diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index fa601737..f43f7876 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -1,6 +1,6 @@ package graphql.kickstart.tools.resolver -import com.fasterxml.jackson.core.type.TypeReference +import com.fasterxml.jackson.annotation.JsonIgnore import graphql.TrivialDataFetcher import graphql.kickstart.tools.* import graphql.kickstart.tools.SchemaParserOptions.GenericWrapper @@ -9,13 +9,16 @@ import graphql.kickstart.tools.util.coroutineScope import graphql.kickstart.tools.util.isTrivialDataFetcher import graphql.kickstart.tools.util.unwrap import graphql.language.* -import graphql.schema.DataFetcher -import graphql.schema.DataFetchingEnvironment -import graphql.schema.GraphQLTypeUtil.isScalar +import graphql.schema.* +import graphql.schema.GraphQLTypeUtil.* import kotlinx.coroutines.future.future +import java.lang.reflect.Field 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.KClass import kotlin.reflect.full.valueParameters import kotlin.reflect.jvm.javaType import kotlin.reflect.jvm.kotlinFunction @@ -39,7 +42,6 @@ internal class MethodFieldResolver( override fun createDataFetcher(): DataFetcher<*> { val args = mutableListOf() - val mapper = options.objectMapperProvider.provide(field) // Add source argument if this is a resolver (but not a root resolver) if (this.search.requiredFirstParameterType != null) { @@ -80,13 +82,8 @@ internal class MethodFieldResolver( return@add Optional.empty() } - if (value == null || shouldValueBeConverted(value, definition, parameterType, environment)) { - return@add mapper.convertValue(value, object : TypeReference() { - override fun getType() = parameterType - }) - } - return@add value + return@add parseInput(value, definition.type, parameterType, environment) } } @@ -106,33 +103,160 @@ 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) + private fun parseInput( + value: Any?, + type: Type<*>, + javaType: JavaType, + environment: DataFetchingEnvironment + ): Any? { + return when (type) { + is NonNullType -> parseInput(value, type.type, javaType, environment) + is TypeName -> { + if (javaType is ParameterizedType && Optional::class.isAssignableFrom(javaType.rawType)) { + // parse and wrap with optional + return Optional.ofNullable(parseInput(value, type, javaType.actualTypeArguments[0], environment)) + } + + when (val graphQLType = environment.graphQLSchema?.getType(type.name)) { + is GraphQLInputObjectType -> { + if (value == null) return value + return if ((javaType as Class<*>).constructors.any { it.parameters.isEmpty() }) { + parseInputObjectWithNoArgsConstructor(javaType, graphQLType, value, environment) + } else { + parseInputObjectWithAllArgsConstructor(javaType, graphQLType, value, environment) + } + } + is GraphQLScalarType -> when { + type.name == "ID" -> parseIdInput(value, javaType) + javaType is Class<*> && javaType.isPrimitive && value == null -> getPrimitiveDefault(javaType) + else -> value + } + is GraphQLEnumType -> when (value) { + is String -> (javaType as Class<*>) + .getMethod("valueOf", String::class.java) + .invoke(null, value) + else -> value + } + else -> value + } + } + is ListType -> when { + value == null -> value + javaType is ParameterizedType && Optional::class.isAssignableFrom(javaType.rawType) -> { + // parse and wrap with optional + Optional.ofNullable(parseInput(value, type, javaType.actualTypeArguments[0], environment)) + } + javaType is ParameterizedType && Collection::class.isAssignableFrom(javaType.rawType) -> { + val collection = (value as Collection<*>).map { + parseInput(it, type.type, javaType.actualTypeArguments[0], environment) + } + + return when { + List::class.isAssignableFrom(javaType.rawType) -> collection.toList() + Set::class.isAssignableFrom(javaType.rawType) -> collection.toSet() + else -> collection + } + } + javaType is WildcardType -> parseInput(value, type, javaType.upperBounds[0], environment) + else -> value + } + else -> value + } } - /** - * 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)) - && isConcreteScalarType(environment, type.type, this.genericType.unwrapGenericType(genericParameterType)) - is TypeName -> environment.graphQLSchema?.getType(type.name)?.let { isScalar(it) && type.name != "ID" } - ?: false - is NonNullType -> isConcreteScalarType(environment, type.type, genericParameterType) - else -> false + private fun parseInputObjectWithAllArgsConstructor( + javaType: Class<*>, + graphQLType: GraphQLInputObjectType, + value: Any?, + environment: DataFetchingEnvironment + ): Any? { + val fields = parseInputObjectFields(javaType, graphQLType, value, environment) + + return javaType + .getDeclaredConstructor(*fields.map { it.first.type }.toTypedArray()) + .newInstance(*fields.map { it.second }.toTypedArray()) + } + + private fun parseInputObjectWithNoArgsConstructor( + javaType: Class<*>, + graphQLType: GraphQLInputObjectType, + value: Any?, + environment: DataFetchingEnvironment + ): Any? { + val inputObject = javaType.getDeclaredConstructor().newInstance() + + parseInputObjectFields(javaType, graphQLType, value, environment) + .forEach { + val field = it.first + field.isAccessible = true + field.set(inputObject, it.second) + } + + return inputObject + } + + private fun parseInputObjectFields( + javaType: Class<*>, + graphQLType: GraphQLInputObjectType, + value: Any?, + environment: DataFetchingEnvironment + ): List> { + return javaType.declaredFields + .filterNot { it.isSynthetic } + // TODO use an annotation specific to graphql (i.e. GraphQLIgnore?) + .filterNot { it.isAnnotationPresent(JsonIgnore::class.java) } + .map { + val graphQLField = graphQLType.fields.find { t -> t.definition.name == it.name } + ?: throw IllegalArgumentException("Could not construct input object: missing field '${it.name}' in '${graphQLType.name}' ") + val fieldValue = (value as Map<*, *>)[graphQLField.definition.name] + val parsedValue = parseInput(fieldValue, graphQLField.definition.type, it.genericType, environment) + Pair(it, parsedValue) + } + } + + private fun parseIdInput(value: Any?, javaType: JavaType) = when { + value !is String // if value was already coerced + || String::class.isAssignableFrom(javaType) -> value + Int::class.isAssignableFrom(javaType) + || Integer::class.isAssignableFrom(javaType) -> Integer.parseInt(value) + Long::class.isAssignableFrom(javaType) + || java.lang.Long::class.isAssignableFrom(javaType) -> java.lang.Long.parseLong(value) + UUID::class.isAssignableFrom(javaType) -> UUID.fromString(value) + else -> value + } + + private fun KClass<*>.isAssignableFrom(javaType: JavaType): Boolean { + return this.java.isAssignableFrom(javaType.unwrap()) + } + + private fun getPrimitiveDefault(javaType: Class<*>): Any? { + return when (javaType) { + Boolean::class.java -> false + Byte::class.java -> 0 + Char::class.java -> '\u0000' + Int::class.java -> 0 + Short::class.java -> 0 + Long::class.java -> 0L + Float::class.java -> 0.0f + Double::class.java -> 0.0 + else -> null } } override fun scanForMatches(): List { - val unwrappedGenericType = genericType.unwrapGenericType(try { - method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType - } catch (e: InternalError) { - method.genericReturnType - }) - val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue(field.type, unwrappedGenericType, genericType, SchemaClassScanner.ReturnValueReference(method)) + val unwrappedGenericType = genericType.unwrapGenericType( + try { + method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType + } catch (e: InternalError) { + method.genericReturnType + } + ) + val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue( + field.type, + unwrappedGenericType, + genericType, + SchemaClassScanner.ReturnValueReference(method) + ) return field.inputValueDefinitions.mapIndexed { i, inputDefinition -> TypeClassMatcher.PotentialMatch.parameterType(inputDefinition.type, getMethodParameterType(i)!!, genericType, SchemaClassScanner.MethodParameterReference(method, i)) diff --git a/src/test/groovy/graphql/kickstart/tools/EndToEndSpec.groovy b/src/test/groovy/graphql/kickstart/tools/EndToEndSpec.groovy index e654d455..c0686c07 100644 --- a/src/test/groovy/graphql/kickstart/tools/EndToEndSpec.groovy +++ b/src/test/groovy/graphql/kickstart/tools/EndToEndSpec.groovy @@ -203,6 +203,20 @@ class EndToEndSpec extends Specification { (data["echoFiles"] as ArrayList).join(",") == "Hello,World" } + def "generated schema should handle input types with scalar fields"() { + when: + def part = new MockPart("test.doc", "Hello") + def input = [name: "filename", part: part] + def args = ["input": input] + def data = Utils.assertNoGraphQlErrors(gql, args) { + ''' + mutation ($input: FileInput) { echoFile(input: $input)} + ''' + } + then: + (data["echoFile"] as String) == "Hello" + } + def "generated schema should handle any java.util.Map (using HashMap) types as property maps"() { when: def data = Utils.assertNoGraphQlErrors(gql) { diff --git a/src/test/groovy/graphql/kickstart/tools/MethodFieldResolverDataFetcherSpec.groovy b/src/test/groovy/graphql/kickstart/tools/MethodFieldResolverDataFetcherSpec.groovy index c7ca1c34..fd5bea74 100644 --- a/src/test/groovy/graphql/kickstart/tools/MethodFieldResolverDataFetcherSpec.groovy +++ b/src/test/groovy/graphql/kickstart/tools/MethodFieldResolverDataFetcherSpec.groovy @@ -228,12 +228,25 @@ class MethodFieldResolverDataFetcherSpec extends Specification { } } ExecutionId executionId = ExecutionId.from("executionId123") + + def schema = SchemaParser.newParser() + .schemaString(""" + type Query { test(input: InputClass): Boolean } + input InputClass { + name: String + } + """) + .resolvers(new Query()) + .build() + .makeExecutableSchema() + ExecutionContextBuilder.newExecutionContextBuilder() .instrumentation(SimpleInstrumentation.INSTANCE) .executionId(executionId) .queryStrategy(executionStrategy) .mutationStrategy(executionStrategy) .subscriptionStrategy(executionStrategy) + .graphQLSchema(schema) .build() } @@ -251,4 +264,10 @@ class MethodFieldResolverDataFetcherSpec extends Specification { class ContextClass { } + + static class Query implements GraphQLQueryResolver { + static boolean test(InputClass input) { + return input != null + } + } } diff --git a/src/test/kotlin/graphql/kickstart/tools/EndToEndSpecHelper.kt b/src/test/kotlin/graphql/kickstart/tools/EndToEndSpecHelper.kt index 8b9801ec..ddfa3933 100644 --- a/src/test/kotlin/graphql/kickstart/tools/EndToEndSpecHelper.kt +++ b/src/test/kotlin/graphql/kickstart/tools/EndToEndSpecHelper.kt @@ -110,6 +110,7 @@ input ComplexInputTypeTwo { type Mutation { addItem(newItem: NewItemInput!): Item! echoFiles(fileParts: [Upload!]!): [String!]! + echoFile(input: FileInput): String saveUser(input: UserInput!): String } @@ -127,6 +128,11 @@ extend input UserInput { password: String } +input FileInput { + name: String + part: Upload +} + input ItemSearchInput { # The item name to look for name: String! @@ -370,6 +376,7 @@ class ItemResolver : GraphQLResolver { class EchoFilesResolver : GraphQLMutationResolver { fun echoFiles(fileParts: List): List = fileParts.map { String(it.inputStream.readBytes()) } + fun echoFile(input: FileInput): String? = input.part?.inputStream?.readBytes()?.let { String(it) } } interface ItemInterface { @@ -388,6 +395,7 @@ data class NestedComplexMapItem(val item: UndiscoveredItem) data class Tag(val id: Int, val name: String) data class ItemSearchInput(val name: String) data class NewItemInput(val name: String, val type: Type) +data class FileInput(var name: String = "", var part: Part? = null) data class ComplexNullable(val first: String, val second: String, val third: String) data class ComplexInputType(val first: String, val second: List?>?) data class ComplexInputTypeTwo(val first: String) diff --git a/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt b/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt index d2d69bd7..dcf40cab 100644 --- a/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverTest.kt @@ -22,6 +22,7 @@ class MethodFieldResolverTest { testValue(input: String): String testOmitted(input: String): String testNull(input: String): String + testList(input: [String]): String } """ ) @@ -30,6 +31,7 @@ class MethodFieldResolverTest { fun testValue(input: Optional) = input.toString() fun testOmitted(input: Optional) = input.toString() fun testNull(input: Optional) = input.toString() + fun testList(input: Optional>) = input.toString() }) .build() .makeExecutableSchema() @@ -38,11 +40,13 @@ class MethodFieldResolverTest { val result = gql .execute(ExecutionInput.newExecutionInput() - .query(""" + .query( + """ query { testValue(input: "test-value") testOmitted testNull(input: null) + testList(input: ["list", "value"]) } """) .context(Object()) @@ -51,7 +55,8 @@ class MethodFieldResolverTest { val expected = mapOf( "testValue" to "Optional[test-value]", "testOmitted" to "Optional.empty", - "testNull" to "Optional.empty" + "testNull" to "Optional.empty", + "testList" to "Optional[[list, value]]" ) Assert.assertEquals(expected, result.getData()) From 655cd3489f7f007dc944c1f83c61928de07d74dc Mon Sep 17 00:00:00 2001 From: oryan Date: Tue, 5 Jan 2021 10:04:18 -0500 Subject: [PATCH 2/3] Formatting --- .../graphql/kickstart/tools/resolver/MethodFieldResolver.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index f43f7876..f06dc3da 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -216,11 +216,11 @@ internal class MethodFieldResolver( private fun parseIdInput(value: Any?, javaType: JavaType) = when { value !is String // if value was already coerced - || String::class.isAssignableFrom(javaType) -> value + || String::class.isAssignableFrom(javaType) -> value Int::class.isAssignableFrom(javaType) - || Integer::class.isAssignableFrom(javaType) -> Integer.parseInt(value) + || Integer::class.isAssignableFrom(javaType) -> Integer.parseInt(value) Long::class.isAssignableFrom(javaType) - || java.lang.Long::class.isAssignableFrom(javaType) -> java.lang.Long.parseLong(value) + || java.lang.Long::class.isAssignableFrom(javaType) -> java.lang.Long.parseLong(value) UUID::class.isAssignableFrom(javaType) -> UUID.fromString(value) else -> value } From b97b6f0dbd3c1ae5697172060f56c012796a6541 Mon Sep 17 00:00:00 2001 From: oryan Date: Tue, 5 Jan 2021 10:18:34 -0500 Subject: [PATCH 3/3] Revert formatting --- .../tools/resolver/MethodFieldResolver.kt | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index f06dc3da..66ed017c 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -244,19 +244,12 @@ internal class MethodFieldResolver( } override fun scanForMatches(): List { - val unwrappedGenericType = genericType.unwrapGenericType( - try { - method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType - } catch (e: InternalError) { - method.genericReturnType - } - ) - val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue( - field.type, - unwrappedGenericType, - genericType, - SchemaClassScanner.ReturnValueReference(method) - ) + val unwrappedGenericType = genericType.unwrapGenericType(try { + method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType + } catch (e: InternalError) { + method.genericReturnType + }) + val returnValueMatch = TypeClassMatcher.PotentialMatch.returnValue(field.type, unwrappedGenericType, genericType, SchemaClassScanner.ReturnValueReference(method)) return field.inputValueDefinitions.mapIndexed { i, inputDefinition -> TypeClassMatcher.PotentialMatch.parameterType(inputDefinition.type, getMethodParameterType(i)!!, genericType, SchemaClassScanner.MethodParameterReference(method, i))