From cca89f8fb8285e924926deeb9da564930637df57 Mon Sep 17 00:00:00 2001 From: Douglas Robert Wallin Date: Mon, 22 Apr 2024 14:23:05 -0400 Subject: [PATCH 1/2] 1965: adjust `FunctionDataFetcher` to wrap all results in `CompleteableFuture` This makes the result where `runBlockingFunction` ends in a thrown `Error` (or descendant of `Error`) be handled as expected. Also for consistency makes `runBlockingFunction` wrap the result in a `CompletableFuture` even for the successful case. Lastly improves grammar a bit adding some commas where needed. --- .../execution/FunctionDataFetcher.kt | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcher.kt b/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcher.kt index 645ea39718..db3c3c5e4e 100644 --- a/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcher.kt +++ b/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcher.kt @@ -36,8 +36,8 @@ import kotlin.reflect.full.valueParameters /** * Simple DataFetcher that invokes target function on the given object. * - * @param target The target object that performs the data fetching, if not specified then this data fetcher will attempt - * to use source object from the environment + * @param target The target object that performs the data fetching, if not specified, then this data fetcher will attempt + * to use a source object from the environment * @param fn The Kotlin function being invoked */ @Suppress("Detekt.SpreadOperator") @@ -49,7 +49,7 @@ open class FunctionDataFetcher( /** * Invoke a suspend function or blocking function, passing in the [target] if not null or default to using the source from the environment. */ - override fun get(environment: DataFetchingEnvironment): Any? { + override fun get(environment: DataFetchingEnvironment): CompletableFuture { val instance: Any? = target ?: environment.getSource() val instanceParameter = fn.instanceParameter @@ -63,7 +63,7 @@ open class FunctionDataFetcher( runBlockingFunction(parameterValues) } } else { - null + CompletableFuture.completedFuture(null) } } @@ -82,10 +82,10 @@ open class FunctionDataFetcher( * Retrieves the provided parameter value in the operation input to pass to the function to execute. * * If the argument is missing in the input, and the type is not an [OptionalInput], do not return a mapping. - * This allows for default Kotlin values to be used when executing the function. If you need logic when a value + * This allows for default Kotlin values to be used when executing the function. Otherwise, if you need logic when a value * is missing, use the [OptionalInput] wrapper instead. * - * If the parameter is of a special type then we do not read the input and instead just pass on that value. + * If the parameter is of a special type, then we do not read the input and instead just pass on that value. * The special values include: * - The entire environment is returned if the parameter is of type [DataFetchingEnvironment] */ @@ -120,12 +120,15 @@ open class FunctionDataFetcher( } /** - * Once all parameters values are properly converted, this function will be called to run a simple blocking function. - * If you need to override the exception handling you can override this method. + * Once all parameter values are properly converted, this function will be called to run a simple blocking function. + * If you need to override the exception handling, you can override this method. */ - protected open fun runBlockingFunction(parameterValues: Map): Any? = try { - fn.callBy(parameterValues) - } catch (exception: InvocationTargetException) { - throw exception.cause ?: exception - } + protected open fun runBlockingFunction(parameterValues: Map): CompletableFuture = + CompletableFuture().apply { + try { + complete(fn.callBy(parameterValues)) + } catch (exception: InvocationTargetException) { + completeExceptionally(exception.cause ?: exception) + } + } } From eeec25b8fab776e66e6d4f311d7a6b8b39140522 Mon Sep 17 00:00:00 2001 From: Douglas Robert Wallin Date: Mon, 22 Apr 2024 14:25:31 -0400 Subject: [PATCH 2/2] 1965: update `FunctionDataFetcherTest` to work with changes to always return `CompletableFuture` --- .../execution/FunctionDataFetcherTest.kt | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/generator/graphql-kotlin-schema-generator/src/test/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcherTest.kt b/generator/graphql-kotlin-schema-generator/src/test/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcherTest.kt index 1907788742..a7d4869d7a 100644 --- a/generator/graphql-kotlin-schema-generator/src/test/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcherTest.kt +++ b/generator/graphql-kotlin-schema-generator/src/test/kotlin/com/expediagroup/graphql/generator/execution/FunctionDataFetcherTest.kt @@ -24,7 +24,6 @@ import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.coroutineScope import org.junit.jupiter.api.Test -import java.util.concurrent.CompletableFuture import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNotNull @@ -91,7 +90,7 @@ class FunctionDataFetcherTest { val mockEnvironment: DataFetchingEnvironment = mockk { every { getSource() } returns null } - assertNull(dataFetcher.get(mockEnvironment)) + assertNull(dataFetcher.get(mockEnvironment).get()) } @Test @@ -102,7 +101,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("string" to "hello") every { containsArgument("string") } returns true } - assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -112,7 +111,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("string" to "hello") every { containsArgument("string") } returns true } - assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -127,7 +126,7 @@ class FunctionDataFetcherTest { every { arguments } returns emptyMap() every { containsArgument(any()) } returns false } - assertEquals(expected = "foo", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "foo", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -138,7 +137,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("string" to "hello") every { containsArgument("string") } returns true } - assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -149,7 +148,7 @@ class FunctionDataFetcherTest { every { arguments } returns emptyMap() every { containsArgument(any()) } returns false } - assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "hello", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -160,7 +159,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("string" to "foo") every { containsArgument("string") } returns true } - assertEquals(expected = "foo", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "foo", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -171,7 +170,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("string" to null) every { containsArgument("string") } returns true } - assertNull(dataFetcher.get(mockEnvironment)) + assertNull(dataFetcher.get(mockEnvironment).get()) } @Test @@ -182,7 +181,7 @@ class FunctionDataFetcherTest { every { containsArgument("items") } returns true } - assertEquals(expected = "foo:bar", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "foo:bar", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -195,7 +194,7 @@ class FunctionDataFetcherTest { every { name } returns "fooBarBaz" } } - assertEquals(expected = "fooBarBaz", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "fooBarBaz", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -209,12 +208,11 @@ class FunctionDataFetcherTest { val result = dataFetcher.get(mockEnvironment) - assertTrue(result is CompletableFuture<*>) assertEquals(expected = "hello", actual = result.get()) } @Test - fun `throwException function propagates the original exception`() { + fun `throwException function returns CompletableFuture with exception`() { val dataFetcher = FunctionDataFetcher(target = MyClass(), fn = MyClass::throwException) val mockEnvironment: DataFetchingEnvironment = mockk { every { arguments } returns emptyMap() @@ -225,7 +223,7 @@ class FunctionDataFetcherTest { dataFetcher.get(mockEnvironment) assertFalse(true, "Should not be here") } catch (e: Exception) { - assertEquals(e.message, "Test Exception") + assertEquals("graphql.GraphQLException: Test Exception", e.message) } } @@ -240,7 +238,6 @@ class FunctionDataFetcherTest { try { val result = dataFetcher.get(mockEnvironment) - assertTrue(result is CompletableFuture<*>) result.get() assertFalse(true, "Should not be here") } catch (e: Exception) { @@ -257,7 +254,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("myCustomArgument" to mapOf("jacksonField" to "foo")) every { containsArgument("myCustomArgument") } returns true } - assertEquals(expected = "You sent foo", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "You sent foo", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -267,7 +264,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("input" to "hello") every { containsArgument("input") } returns true } - assertEquals(expected = "input was hello", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "input was hello", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -277,7 +274,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("input" to null) every { containsArgument("input") } returns true } - assertEquals(expected = "input was null", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "input was null", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -287,7 +284,7 @@ class FunctionDataFetcherTest { every { arguments } returns emptyMap() every { containsArgument(any()) } returns false } - assertEquals(expected = "input was UNDEFINED", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "input was UNDEFINED", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -297,7 +294,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("input" to listOf(linkedMapOf("jacksonField" to "foo"))) every { containsArgument("input") } returns true } - val result = dataFetcher.get(mockEnvironment) + val result = dataFetcher.get(mockEnvironment).get() assertEquals(expected = "first input was foo", actual = result) } @@ -308,7 +305,7 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("input" to mapOf("required" to "hello", "optional" to "hello")) every { containsArgument("input") } returns true } - assertEquals(expected = "optional was hello", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "optional was hello", actual = dataFetcher.get(mockEnvironment).get()) } @Test @@ -318,6 +315,6 @@ class FunctionDataFetcherTest { every { arguments } returns mapOf("input" to mapOf("required" to "hello")) every { containsArgument("input") } returns true } - assertEquals(expected = "optional was UNDEFINED", actual = dataFetcher.get(mockEnvironment)) + assertEquals(expected = "optional was UNDEFINED", actual = dataFetcher.get(mockEnvironment).get()) } }