Skip to content

Change scope of FunctionDataFetcher methods #588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Change scope of FunctionDataFetcher methods #588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions ...chema-generator/src/main/kotlin/com/expediagroup/graphql/execution/FunctionDataFetcher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ open class FunctionDataFetcher(
/**
* Iterate over all the function parameters and map them to the proper input values from the environment
*/
protected open fun getParameterValues(fn: KFunction<*>, environment: DataFetchingEnvironment): Array<Any?> = fn.valueParameters
protected fun getParameterValues(fn: KFunction<*>, environment: DataFetchingEnvironment): Array<Any?> = fn.valueParameters
.map { param -> mapParameterToValue(param, environment) }
.toTypedArray()

Expand All @@ -86,7 +86,7 @@ open class FunctionDataFetcher(
*
* - The entire environment is returned if the parameter is of type [DataFetchingEnvironment]
*/
protected open fun mapParameterToValue(param: KParameter, environment: DataFetchingEnvironment): Any? =
private fun mapParameterToValue(param: KParameter, environment: DataFetchingEnvironment): Any? =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by keeping it open one could technically implement some other autowired object besides GraphQLContext and DataFetchingEnvironment

Copy link
Contributor Author

@smyrick smyrick Feb 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting. Do we need to leave this method in particular open or do we just have them re-implement get and call their own custom method?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since other methods are already open I'd keep those open as well. Once (or IF) we implement Kotlin native way of executing the queries (and drop the dependency on CompletableFuture) we could change the interface method signatures to something more appropriate.

when {
param.isGraphQLContext() -> environment.getContext()
param.isDataFetchingEnvironment() -> environment
Expand All @@ -98,7 +98,7 @@ open class FunctionDataFetcher(
*
* This is currently achieved by using a Jackson [ObjectMapper].
*/
protected open fun convertParameterValue(param: KParameter, environment: DataFetchingEnvironment): Any? {
private fun convertParameterValue(param: KParameter, environment: DataFetchingEnvironment): Any? {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by keeping it open we could allow others to implement some custom logic here, e.g. how to handle file uploads or skip converting the type if underlying object is already a correct type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not think about that possibility. Let's discuss in more about both on the thread below.

val name = param.getName()
val klazz = param.javaTypeClass()
val argument = environment.arguments[name]
Expand All @@ -111,7 +111,7 @@ open class FunctionDataFetcher(
* If you need to override the exception handling you can override the entire method.
* You can also call it from [get] with different values to override the default corotuine context or start parameter.
*/
protected open fun runSuspendingFunction(
protected fun runSuspendingFunction(
instance: Any,
parameterValues: Array<Any?>,
coroutineContext: CoroutineContext = EmptyCoroutineContext,
Expand All @@ -130,7 +130,7 @@ 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.
*/
protected open fun runBlockingFunction(instance: Any, parameterValues: Array<Any?>): Any? {
protected fun runBlockingFunction(instance: Any, parameterValues: Array<Any?>): Any? {
try {
return fn.call(instance, *parameterValues)
} catch (exception: InvocationTargetException) {
Expand Down
29 changes: 29 additions & 0 deletions ...a-generator/src/test/kotlin/com/expediagroup/graphql/execution/FunctionDataFetcherTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import io.mockk.mockk
import kotlinx.coroutines.coroutineScope
import org.junit.jupiter.api.Test
import java.util.concurrent.CompletableFuture
import kotlin.reflect.KFunction
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNotNull
Expand Down Expand Up @@ -66,6 +67,34 @@ internal class FunctionDataFetcherTest {
val field1: String
)

class CustomFunctionDataFetcher(private val target: Any?, private val fn: KFunction<*>) : FunctionDataFetcher(target, fn) {
internal var ranCustomMethod = false

override fun get(environment: DataFetchingEnvironment): Any? {
ranCustomMethod = true
val instance = target ?: environment.getSource<Any>()

return instance?.let {
val parameterValues = getParameterValues(fn, environment)

if (fn.isSuspend) {
runSuspendingFunction(it, parameterValues)
} else {
runBlockingFunction(it, parameterValues)
}
}
}
}

@Test
fun `get method can be overriden`() {
val dataFetcher = CustomFunctionDataFetcher(target = null, fn = MyClass::print)
val mockEnvironmet: DataFetchingEnvironment = mockk()
every { mockEnvironmet.getSource<Any>() } returns null
assertNull(dataFetcher.get(mockEnvironmet))
assertTrue(dataFetcher.ranCustomMethod)
}

@Test
fun `null target and null source returns null`() {
val dataFetcher = FunctionDataFetcher(target = null, fn = MyClass::print)
Expand Down