Merge pull request #940 from graphql-java-kickstart/bugfix/optional-data-fetcher-result

Unwrap a DataFetcherResult inside an Optional

Author: oryan-block

src/main/kotlin/graphql/kickstart/tools/TypeClassMatcher.kt

@@ -60,36 +60,36 @@ internal class TypeClassMatcher(private val definitionsByName: Map<String, TypeD
         private fun unwrap(graphQLType: GraphQLLangType, javaType: JavaType, root: Boolean, unwrapped: MutableList<Unwrapped>): 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.

src/main/kotlin/graphql/kickstart/tools/resolver/ReturnValueUnwrapper.kt

@@ -88,23 +88,29 @@ private class DataOf(private val data: Step) : Step {
         }
 }
 
-private class ValueOf(private val present: Step) : Step {
-    override fun unwrap(value: Any?, environmentSupplier: Supplier<DataFetchingEnvironment>): 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<DataFetchingEnvironment>): 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<Unwrapped>, awaits: Awaitable, peels: Awaitable): Step? =
-    unwrapped.foldRight(null as Step?) { 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)
-            Unwrapped.Elements -> next?.let(::EachElement)
-        }
+private fun steps(unwrapped: List<Unwrapped>, 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<DataFetchingEnvironment>): Any? =
     if (next is Await) next.unwrap(this, environmentSupplier) else then(next, environmentSupplier)

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<Class<*>>())) { 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<String>())
     }
 
+    @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<Class<*>>, 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<List<Payload>> = completedFuture(plainList)
         val wrappedFuture: Try<CompletableFuture<Payload>> = Try.success(completedFuture(Payload("a")))
+        val optionalPayload: Optional<Payload> = Optional.of(Payload("a"))
 
         private fun <T : Any> result(data: T, vararg errors: GraphQLError): DataFetcherResult<T> = DataFetcherResult.newResult<T>().data(data).errors(errors.toList()).build()
 
@@ -406,6 +467,9 @@ class NestedGenericWrapperTest {
         fun resultOfListOfWrapped(): DataFetcherResult<List<Try<Payload>>> = result(listOfWrapped(), GraphQLError.newError().message("partial").build())
         fun listOfResultsOfWrapped(): List<DataFetcherResult<Try<Payload>>> = listOfWrapped().map { result(it) }
         fun optionalListOfWrapped(): Optional<List<Try<Payload>>> = Optional.of(listOfWrapped())
+        fun optionalResult(): Optional<DataFetcherResult<Payload>> = Optional.of(result(Payload("a"), GraphQLError.newError().message("partial").build()))
+        fun emptyOptionalResult(): Optional<DataFetcherResult<Payload>> = Optional.empty()
+        fun optionalResultOfListOfWrapped(): Optional<DataFetcherResult<List<Try<Payload>>>> = Optional.of(resultOfListOfWrapped())
         suspend fun suspendListOfWrapped(): List<Try<Payload>> = listOfWrapped()
         suspend fun suspendListOfFutures(): List<CompletableFuture<Payload>> = listOfFutures()
         suspend fun suspendNullableListOfFutures(): List<CompletableFuture<Payload>> = listOfFutures()