From d60c85202e7303e2588c4368ae13c1616244c51c Mon Sep 17 00:00:00 2001 From: Oryan Date: Thu, 29 Oct 2026 18:34:47 -0400 Subject: [PATCH 1/2] Unwrap a DataFetcherResult inside an Optional Optional> bound T's type to the DataFetcherResult class, so graphql-java got the DataFetcherResult as the field value. DataFetcherResult and Optional are now peeled in either order, each once per value, and the Optional is unwrapped before graphql-java sees the DataFetcherResult. A DataFetcherResult or an Optional twice in one value, such as Optional>>, now fails the build. --- .../kickstart/tools/TypeClassMatcher.kt | 50 ++++++------- .../tools/resolver/ReturnValueUnwrapper.kt | 14 ++-- .../tools/NestedGenericWrapperTest.kt | 70 ++++++++++++++++++- 3 files changed, 101 insertions(+), 33 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt b/src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt index 44f7c64a..fff48337 100644 --- a/src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt +++ b/src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt @@ -60,36 +60,36 @@ internal class TypeClassMatcher(private val definitionsByName: Map): Unwrapping { val onUnwrap = { wrapper: GenericWrapper -> unwrapped += Unwrapped.Wrapper(wrapper) } var realType = typeResolver.resolve(javaType, onUnwrap) - - if (realType is ParameterizedType && realType.isSubtypeOf(DataFetcherResult::class.java)) { - if (location != Location.RETURN_TYPE) { - throw error("${DataFetcherResult::class.java.name} can only be used as a return type") - } - - unwrapped += Unwrapped.Data - realType = typeResolver.resolve(realType.actualTypeArguments.first(), onUnwrap) - - if (realType is ParameterizedType && realType.isSubtypeOf(DataFetcherResult::class.java)) { - throw error("${DataFetcherResult::class.java.name} cannot be nested within itself") - } - } - + var data = false var optional = false - // Handle jdk8 Optionals - if (realType is ParameterizedType && realType.isSubtypeOf(Optional::class.java)) { - optional = true - - if (location == Location.RETURN_TYPE && !root) { - throw error("${Optional::class.java.name} can only be used at the top level of a return type") - } + while (realType is ParameterizedType) { + when { + realType.isSubtypeOf(DataFetcherResult::class.java) -> { + if (location != Location.RETURN_TYPE) { + throw error("${DataFetcherResult::class.java.name} can only be used as a return type") + } + if (data) { + throw error("${DataFetcherResult::class.java.name} cannot be nested within itself") + } + data = true + unwrapped += Unwrapped.Data + } - unwrapped += Unwrapped.Present - realType = typeResolver.resolve(realType.actualTypeArguments.first(), onUnwrap) + realType.isSubtypeOf(Optional::class.java) -> { + if (location == Location.RETURN_TYPE && !root) { + throw error("${Optional::class.java.name} can only be used at the top level of a return type") + } + if (optional) { + throw error("${Optional::class.java.name} cannot be nested within itself") + } + optional = true + unwrapped += Unwrapped.Present + } - if (realType is ParameterizedType && realType.isSubtypeOf(Optional::class.java)) { - throw error("${Optional::class.java.name} cannot be nested within itself") + else -> break } + realType = typeResolver.resolve(realType.actualTypeArguments.first(), onUnwrap) } // Match graphql type to java type. diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt index 705316dd..4ddefd73 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt @@ -88,20 +88,24 @@ private class DataOf(private val data: Step) : Step { } } -private class ValueOf(private val present: Step) : Step { - override fun unwrap(value: Any?, environmentSupplier: Supplier): Any? = - if (value is Optional<*>) present.unwrap(value.orElse(null), environmentSupplier) else value +private class ValueOf(private val present: Step?) : Step { + override fun unwrap(value: Any?, environmentSupplier: Supplier): Any? = when { + value !is Optional<*> -> value + present == null -> value.orElse(null) + else -> present.unwrap(value.orElse(null), environmentSupplier) + } } private val Unwrapped.Wrapper.isFuture get() = Future::class.java.isAssignableFrom(wrapper.type) || CompletionStage::class.java.isAssignableFrom(wrapper.type) private fun steps(unwrapped: List, awaits: Awaitable, peels: Awaitable): Step? = - unwrapped.foldRight(null as Step?) { it, next -> + unwrapped.foldRightIndexed(null as Step?) { i, it, next -> when (it) { is Unwrapped.Wrapper -> if (it.isFuture) Await(it.wrapper, awaits, next) else Peel(it.wrapper, peels, next) Unwrapped.Data -> next?.let(::DataOf) - Unwrapped.Present -> next?.let(::ValueOf) + // graphql-java reads a DataFetcherResult only as the value itself, never inside an Optional + Unwrapped.Present -> if (next != null || unwrapped.getOrNull(i + 1) == Unwrapped.Data) ValueOf(next) else null Unwrapped.Elements -> next?.let(::EachElement) } } diff --git a/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt b/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt index 79d1bb87..aad0a9d2 100644 --- a/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/NestedGenericWrapperTest.kt @@ -93,6 +93,25 @@ class NestedGenericWrapperTest { @Test fun `unwraps wrappers in a list in an Optional`() = assertResolves("optionalListOfWrapped", ab) + @Test + fun `resolves a DataFetcherResult in an Optional and keeps its errors`() { + val result = execute("optionalResult") + + assertEquals(result.errors.map { it.message }, listOf("partial")) + assertEquals(result.getData(), mapOf("optionalResult" to mapOf("data" to "a"))) + } + + @Test + fun `resolves an empty Optional of a DataFetcherResult to null`() = assertResolves("emptyOptionalResult", null) + + @Test + fun `unwraps wrappers in a list in a DataFetcherResult in an Optional and keeps its errors`() { + val result = execute("optionalResultOfListOfWrapped") + + assertEquals(result.errors.map { it.message }, listOf("partial")) + assertEquals(result.getData(), mapOf("optionalResultOfListOfWrapped" to ab)) + } + @Test fun `unwraps wrappers in a list from a suspend function`() = assertResolves("suspendListOfWrapped", ab) @@ -178,6 +197,7 @@ class NestedGenericWrapperTest { assertSame(fetch(schema, "plainList"), query.plainList) assertSame(fetch(schema, "futureOfList"), query.futureOfList) assertSame(fetch(schema, "wrappedFuture"), query.wrappedFuture.get()) + assertSame(fetch(schema, "optionalPayload"), query.optionalPayload) } @Test @@ -202,18 +222,22 @@ class NestedGenericWrapperTest { val shapes = (1..4).fold(listOf(listOf>())) { shapes, _ -> shapes + shapes.filter { it.size == shapes.last().size }.flatMap { shape -> containers.map { shape + it } } } val accepted = graphQLTypes.flatMap { graphQLType -> - shapes.flatMap { shape -> listOf(false, true).map { shape to javaType(shape, leaf(graphQLType).javaClass, wildcardElements = it) } }.mapNotNull { (shape, javaType) -> + val shapesOfTheSameListDepth = shapes.filter { shape -> shape.count { it == List::class.java || it == Page::class.java } == graphQLType.listDepth() } + shapesOfTheSameListDepth.flatMap { shape -> listOf(false, true).map { shape to javaType(shape, leaf(graphQLType).javaClass, wildcardElements = it) } }.mapNotNull { (shape, javaType) -> val potentialMatch = TypeClassMatcher.PotentialMatch.returnValue(graphQLType, javaType, typeResolver, ScanDictionary.ReturnValueReference(method)) - val match = try { + try { matcher.match(potentialMatch) + shape to potentialMatch } catch (e: SchemaClassScannerError) { null } - (shape to potentialMatch).takeIf { match != null && potentialMatch.unwrapping.javaType == leaf(graphQLType).javaClass } } } val failures = accepted.mapNotNull { (shape, potentialMatch) -> val graphQLType = potentialMatch.graphQLType + if (potentialMatch.unwrapping.javaType != leaf(graphQLType).javaClass) { + return@mapNotNull "${TypeUtils.toString(potentialMatch.javaType)} as ${AstPrinter.printAst(graphQLType)}: bound to ${TypeUtils.toString(potentialMatch.unwrapping.javaType)}" + } val unwrapper = ReturnValueUnwrapper.create( method, potentialMatch.unwrapping, TypeUtils.getRawType(typeResolver.resolve(potentialMatch.javaType), null), isSuspendFunction = false, isSubscription = false, options.genericWrappers ) @@ -232,6 +256,38 @@ class NestedGenericWrapperTest { assertEquals(failures, listOf()) } + @Test + fun `rejects a DataFetcherResult or an Optional twice in one value`() { + val typeResolver = GenericTypeResolver(Query::class.java, SchemaParserOptions.newOptions().build()) + val matcher = TypeClassMatcher(mapOf("Payload" to ObjectTypeDefinition("Payload"))) + val method = Query::class.java.getMethod("listOfWrapped") + val result = DataFetcherResult::class.java + val optional = Optional::class.java + val rejections = listOf( + listOf(optional, result, result) to "${result.name} cannot be nested within itself", + listOf(result, optional, result) to "${result.name} cannot be nested within itself", + listOf(optional, result, optional) to "${optional.name} cannot be nested within itself", + ) + + val messages = rejections.map { (shape, _) -> + val potentialMatch = TypeClassMatcher.PotentialMatch.returnValue(TypeName("Payload"), javaType(shape, Payload::class.java, wildcardElements = false), typeResolver, ScanDictionary.ReturnValueReference(method)) + try { + matcher.match(potentialMatch) + "accepted" + } catch (e: SchemaClassScannerError) { + e.message.orEmpty().substringAfterLast("): ") + } + } + + assertEquals(messages, rejections.map { it.second }) + } + + private fun Type<*>.listDepth(): Int = when (this) { + is ListType -> 1 + type.listDepth() + is NonNullType -> type.listDepth() + else -> 0 + } + private fun leaf(graphQLType: Type<*>): Any = if (graphQLType.unwrap().name == "Leaf") FutureLeaf("a") else Payload("a") private fun javaType(shape: List>, leafClass: Class<*>, wildcardElements: Boolean): java.lang.reflect.Type = @@ -284,6 +340,10 @@ class NestedGenericWrapperTest { pageOfWrapped: [Payload!]! resultOfListOfWrapped: [Payload!]! listOfResultsOfWrapped: [Payload!]! + optionalResult: Payload + emptyOptionalResult: Payload + optionalResultOfListOfWrapped: [Payload!] + optionalPayload: Payload optionalListOfWrapped: [Payload!] suspendListOfWrapped: [Payload!]! suspendListOfFutures: [Payload!]! @@ -387,6 +447,7 @@ class NestedGenericWrapperTest { val plainList = listOf(Payload("a")) val futureOfList: CompletableFuture> = completedFuture(plainList) val wrappedFuture: Try> = Try.success(completedFuture(Payload("a"))) + val optionalPayload: Optional = Optional.of(Payload("a")) private fun result(data: T, vararg errors: GraphQLError): DataFetcherResult = DataFetcherResult.newResult().data(data).errors(errors.toList()).build() @@ -406,6 +467,9 @@ class NestedGenericWrapperTest { fun resultOfListOfWrapped(): DataFetcherResult>> = result(listOfWrapped(), GraphQLError.newError().message("partial").build()) fun listOfResultsOfWrapped(): List>> = listOfWrapped().map { result(it) } fun optionalListOfWrapped(): Optional>> = Optional.of(listOfWrapped()) + fun optionalResult(): Optional> = Optional.of(result(Payload("a"), GraphQLError.newError().message("partial").build())) + fun emptyOptionalResult(): Optional> = Optional.empty() + fun optionalResultOfListOfWrapped(): Optional>>> = Optional.of(resultOfListOfWrapped()) suspend fun suspendListOfWrapped(): List> = listOfWrapped() suspend fun suspendListOfFutures(): List> = listOfFutures() suspend fun suspendNullableListOfFutures(): List> = listOfFutures() From 2c002004e29b0596c55205aa36c13f293a2c9968 Mon Sep 17 00:00:00 2001 From: Oryan Date: Thu, 29 Oct 2026 18:56:44 -0400 Subject: [PATCH 2/2] Look at the entry inside an Optional instead of indexing into the unwrapped list --- .../tools/resolver/ReturnValueUnwrapper.kt | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt b/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt index 4ddefd73..5a0473d3 100644 --- a/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt +++ b/src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt @@ -99,16 +99,18 @@ private class ValueOf(private val present: Step?) : Step { private val Unwrapped.Wrapper.isFuture get() = Future::class.java.isAssignableFrom(wrapper.type) || CompletionStage::class.java.isAssignableFrom(wrapper.type) -private fun steps(unwrapped: List, awaits: Awaitable, peels: Awaitable): Step? = - unwrapped.foldRightIndexed(null as Step?) { i, it, next -> - when (it) { - is Unwrapped.Wrapper -> if (it.isFuture) Await(it.wrapper, awaits, next) else Peel(it.wrapper, peels, next) - Unwrapped.Data -> next?.let(::DataOf) - // graphql-java reads a DataFetcherResult only as the value itself, never inside an Optional - Unwrapped.Present -> if (next != null || unwrapped.getOrNull(i + 1) == Unwrapped.Data) ValueOf(next) else null - Unwrapped.Elements -> next?.let(::EachElement) - } +private fun steps(unwrapped: List, awaits: Awaitable, peels: Awaitable): Step? { + val outermost = unwrapped.firstOrNull() ?: return null + val inner = unwrapped.subList(1, unwrapped.size) + val next = steps(inner, awaits, peels) + return when (outermost) { + is Unwrapped.Wrapper -> if (outermost.isFuture) Await(outermost.wrapper, awaits, next) else Peel(outermost.wrapper, peels, next) + Unwrapped.Data -> next?.let(::DataOf) + // graphql-java reads a DataFetcherResult and then an Optional in its data, so an Optional around one is ours to unwrap + Unwrapped.Present -> if (next != null || inner.firstOrNull() == Unwrapped.Data) ValueOf(next) else null + Unwrapped.Elements -> next?.let(::EachElement) } +} private fun Any?.thenUnlessNextAwaits(next: Step?, environmentSupplier: Supplier): Any? = if (next is Await) next.unwrap(this, environmentSupplier) else then(next, environmentSupplier)