From a59af427d5461c516b713f70485a8ef0e7867c18 Mon Sep 17 00:00:00 2001 From: Alexandre Baron Date: Fri, 23 Oct 2020 13:55:41 +0200 Subject: [PATCH 1/3] Always convert scalar ID values to the method parameter type fix #367 --- .../tools/resolver/MethodFieldResolver.kt | 42 +++--- .../kickstart/tools/BuiltInIdSpec.groovy | 134 ++++++++++++++++++ .../kickstart/tools/BuiltInLongIdSpec.groovy | 81 ----------- 3 files changed, 155 insertions(+), 102 deletions(-) create mode 100644 src/test/groovy/graphql/kickstart/tools/BuiltInIdSpec.groovy delete mode 100644 src/test/groovy/graphql/kickstart/tools/BuiltInLongIdSpec.groovy diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index b76548dc..5d9aad62 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -14,8 +14,6 @@ import graphql.schema.DataFetchingEnvironment import graphql.schema.GraphQLTypeUtil.isScalar import kotlinx.coroutines.future.future 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.full.valueParameters @@ -82,15 +80,13 @@ internal class MethodFieldResolver( return@add Optional.empty() } - if (value != null - && parameterType.unwrap().isAssignableFrom(value.javaClass) - && isScalarType(environment, definition.type, parameterType)) { - return@add value + if (isValueMustBeConvert(value, definition, parameterType, environment)) { + return@add mapper.convertValue(value, object : TypeReference() { + override fun getType() = parameterType + }) } - return@add mapper.convertValue(value, object : TypeReference() { - override fun getType() = parameterType - }) + return@add value } } @@ -110,23 +106,27 @@ internal class MethodFieldResolver( } } - private fun isScalarType(environment: DataFetchingEnvironment, type: Type<*>, genericParameterType: JavaType): Boolean = + private fun isValueMustBeConvert(value: Any?, definition: InputValueDefinition, parameterType: JavaType, environment: DataFetchingEnvironment): Boolean { + return value != null + && (!parameterType.unwrap().isAssignableFrom(value.javaClass) + || !isConcreteScalarType(environment, definition.type, parameterType)) + } + + /** + * A concrete scalar type is a scalar type where values are always coerce to the same Java type. + * The ID scalar type is not concrete because values can be coerce to many Java types (eg. String, Long, UUID). + * All values of a non concrete scalar type must be convert to the method field type. + */ + private fun isConcreteScalarType(environment: DataFetchingEnvironment, type: Type<*>, genericParameterType: JavaType): Boolean = when (type) { is ListType -> List::class.java.isAssignableFrom(this.genericType.getRawClass(genericParameterType)) - && isScalarType(environment, type.type, this.genericType.unwrapGenericType(genericParameterType)) - is TypeName -> environment.graphQLSchema?.getType(type.name)?.let { isScalar(it) && !isJavaLanguageType(genericParameterType) } - ?: false - is NonNullType -> isScalarType(environment, type.type, 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 isJavaLanguageType(type: JavaType): Boolean = - when (type) { - is ParameterizedType -> isJavaLanguageType(type.actualTypeArguments[0]) - is WildcardType -> isJavaLanguageType(type.upperBounds[0]) - else -> genericType.getRawClass(type).`package`.name == "java.lang" - } - override fun scanForMatches(): List { val unwrappedGenericType = genericType.unwrapGenericType(try { method.kotlinFunction?.returnType?.javaType ?: method.genericReturnType diff --git a/src/test/groovy/graphql/kickstart/tools/BuiltInIdSpec.groovy b/src/test/groovy/graphql/kickstart/tools/BuiltInIdSpec.groovy new file mode 100644 index 00000000..a9bfb62f --- /dev/null +++ b/src/test/groovy/graphql/kickstart/tools/BuiltInIdSpec.groovy @@ -0,0 +1,134 @@ +package graphql.kickstart.tools + +import graphql.GraphQL +import graphql.execution.AsyncExecutionStrategy +import graphql.schema.GraphQLSchema +import spock.lang.Shared +import spock.lang.Specification + +class BuiltInIdSpec extends Specification { + + @Shared + GraphQL gql + + def setupSpec() { + GraphQLSchema schema = SchemaParser.newParser().schemaString('''\ + type Query { + itemByLongId(id: ID!): Item1! + itemsByLongIds(ids: [ID!]!): [Item1!]! + itemByUuidId(id: ID!): Item2! + itemsByUuidIds(ids: [ID!]!): [Item2!]! + } + + type Item1 { + id: ID! + } + + type Item2 { + id: ID! + } + '''.stripIndent()) + .resolvers(new QueryWithLongItemResolver()) + .build() + .makeExecutableSchema() + gql = GraphQL.newGraphQL(schema) + .queryExecutionStrategy(new AsyncExecutionStrategy()) + .build() + } + + def "supports Long as ID as input"() { + when: + def data = Utils.assertNoGraphQlErrors(gql) { + ''' + { + itemByLongId(id: 1) { + id + } + } + ''' + } + + then: + data.itemByLongId != null + data.itemByLongId.id == "1" + } + + def "supports list of Long as ID as input"() { + when: + def data = Utils.assertNoGraphQlErrors(gql) { + ''' + { + itemsByLongIds(ids: [1,2,3]) { + id + } + } + ''' + } + + then: + data.itemsByLongIds != null + data.itemsByLongIds.size == 3 + data.itemsByLongIds[0].id == "1" + } + + def "supports UUID as ID as input"() { + when: + def data = Utils.assertNoGraphQlErrors(gql) { + ''' + { + itemByUuidId(id: "00000000-0000-0000-0000-000000000000") { + id + } + } + ''' + } + + then: + data.itemByUuidId != null + data.itemByUuidId.id == "00000000-0000-0000-0000-000000000000" + } + + def "supports list of UUID as ID as input"() { + when: + def data = Utils.assertNoGraphQlErrors(gql) { + ''' + { + itemsByUuidIds(ids: ["00000000-0000-0000-0000-000000000000","11111111-1111-1111-1111-111111111111","22222222-2222-2222-2222-222222222222"]) { + id + } + } + ''' + } + + then: + data.itemsByUuidIds != null + data.itemsByUuidIds.size == 3 + data.itemsByUuidIds[0].id == "00000000-0000-0000-0000-000000000000" + } + + class QueryWithLongItemResolver implements GraphQLQueryResolver { + Item1 itemByLongId(Long id) { + new Item1(id: id) + } + + List itemsByLongIds(List ids) { + ids.collect { new Item1(id: it) } + } + + Item2 itemByUuidId(UUID id) { + new Item2(id: id) + } + + List itemsByUuidIds(List ids) { + ids.collect { new Item2(id: it) } + } + } + + class Item1 { + Long id + } + + class Item2 { + UUID id + } +} diff --git a/src/test/groovy/graphql/kickstart/tools/BuiltInLongIdSpec.groovy b/src/test/groovy/graphql/kickstart/tools/BuiltInLongIdSpec.groovy deleted file mode 100644 index 1382467a..00000000 --- a/src/test/groovy/graphql/kickstart/tools/BuiltInLongIdSpec.groovy +++ /dev/null @@ -1,81 +0,0 @@ -package graphql.kickstart.tools - -import graphql.GraphQL -import graphql.execution.AsyncExecutionStrategy -import graphql.schema.GraphQLSchema -import spock.lang.Shared -import spock.lang.Specification - -class BuiltInLongIdSpec extends Specification { - - @Shared - GraphQL gql - - def setupSpec() { - GraphQLSchema schema = SchemaParser.newParser().schemaString('''\ - type Query { - itemByLongId(id: ID!): Item! - itemsByLongIds(ids: [ID!]!): [Item!]! - } - - type Item { - id: ID! - } - '''.stripIndent()) - .resolvers(new QueryWithLongItemResolver()) - .build() - .makeExecutableSchema() - gql = GraphQL.newGraphQL(schema) - .queryExecutionStrategy(new AsyncExecutionStrategy()) - .build() - } - - def "supports Long as ID as input"() { - when: - def data = Utils.assertNoGraphQlErrors(gql) { - ''' - { - itemByLongId(id: 1) { - id - } - } - ''' - } - - then: - data.itemByLongId != null - data.itemByLongId.id == "1" - } - - def "supports list of Long as ID as input"() { - when: - def data = Utils.assertNoGraphQlErrors(gql) { - ''' - { - itemsByLongIds(ids: [1,2,3]) { - id - } - } - ''' - } - - then: - data.itemsByLongIds != null - data.itemsByLongIds.size == 3 - data.itemsByLongIds[0].id == "1" - } - - class QueryWithLongItemResolver implements GraphQLQueryResolver { - Item itemByLongId(Long id) { - new Item(id: id) - } - - List itemsByLongIds(List ids) { - ids.collect { new Item(id: it) } - } - } - - class Item { - Long id - } -} From 3262bc38b446e4786e0c598dc48350f4a84dfcc2 Mon Sep 17 00:00:00 2001 From: Vojtech Polivka Date: Tue, 17 Nov 2020 14:36:25 -0800 Subject: [PATCH 2/3] Clean up --- pom.xml | 2 +- .../tools/resolver/MethodFieldResolver.kt | 23 +++++++++---------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/pom.xml b/pom.xml index 03146bf8..3500f809 100644 --- a/pom.xml +++ b/pom.xml @@ -4,7 +4,7 @@ com.graphql-java-kickstart graphql-java-tools - 6.2.1-SNAPSHOT + 6.2.1-SNAPSHOT-unused-type jar GraphQL Java Tools diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt index 5d9aad62..c7a9920e 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/MethodFieldResolver.kt @@ -80,7 +80,7 @@ internal class MethodFieldResolver( return@add Optional.empty() } - if (isValueMustBeConvert(value, definition, parameterType, environment)) { + if (value != null && shouldValueBeConverted(value, definition, parameterType, environment)) { return@add mapper.convertValue(value, object : TypeReference() { override fun getType() = parameterType }) @@ -106,26 +106,25 @@ internal class MethodFieldResolver( } } - private fun isValueMustBeConvert(value: Any?, definition: InputValueDefinition, parameterType: JavaType, environment: DataFetchingEnvironment): Boolean { - return value != null - && (!parameterType.unwrap().isAssignableFrom(value.javaClass) - || !isConcreteScalarType(environment, definition.type, parameterType)) + 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 are always coerce to the same Java type. - * The ID scalar type is not concrete because values can be coerce to many Java types (eg. String, Long, UUID). - * All values of a non concrete scalar type must be convert to the method field type. + * 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 = - when (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)) + && isConcreteScalarType(environment, type.type, this.genericType.unwrapGenericType(genericParameterType)) is TypeName -> environment.graphQLSchema?.getType(type.name)?.let { isScalar(it) && type.name != "ID" } - ?: false + ?: false is NonNullType -> isConcreteScalarType(environment, type.type, genericParameterType) else -> false } + } override fun scanForMatches(): List { val unwrappedGenericType = genericType.unwrapGenericType(try { From f6928c2aa4bdc7082dce01ff5a9bcea6736908dc Mon Sep 17 00:00:00 2001 From: Vojtech Polivka Date: Tue, 17 Nov 2020 14:38:28 -0800 Subject: [PATCH 3/3] Remove version tag --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 3500f809..03146bf8 100644 --- a/pom.xml +++ b/pom.xml @@ -4,7 +4,7 @@ com.graphql-java-kickstart graphql-java-tools - 6.2.1-SNAPSHOT-unused-type + 6.2.1-SNAPSHOT jar GraphQL Java Tools