Merge pull request #915 from graphql-java-kickstart/feature/846-remove-missing-resolver-data-fetcher

Remove missingResolverDataFetcher and the legacy context fallbacks

Author: oryan-block

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

@@ -6,7 +6,6 @@ import graphql.kickstart.tools.relay.RelayConnectionFactory
 import graphql.kickstart.tools.resolver.MissingResolverDataFetcherProvider
 import graphql.kickstart.tools.util.JavaType
 import graphql.kickstart.tools.util.ParameterizedTypeImpl
-import graphql.schema.DataFetcher
 import graphql.schema.DataFetchingEnvironment
 import graphql.schema.visibility.GraphqlFieldVisibility
 import kotlinx.coroutines.Dispatchers
@@ -24,8 +23,6 @@ data class SchemaParserOptions internal constructor(
     val contextClass: Class<*>?,
     val genericWrappers: List<GenericWrapper>,
     val allowUnimplementedResolvers: Boolean,
-    @Deprecated("Use missingResolverDataFetcherProvider instead.")
-    val missingResolverDataFetcher: DataFetcher<Any?>?,
     val missingResolverDataFetcherProvider: MissingResolverDataFetcherProvider?,
     val objectMapperProvider: PerFieldObjectMapperProvider,
     val proxyHandlers: List<ProxyHandler>,
@@ -55,7 +52,6 @@ data class SchemaParserOptions internal constructor(
         private val genericWrappers: MutableList<GenericWrapper> = mutableListOf()
         private var useDefaultGenericWrappers = true
         private var allowUnimplementedResolvers = false
-        private var missingResolverDataFetcher: DataFetcher<Any?>? = null
         private var missingResolverDataFetcherProvider: MissingResolverDataFetcherProvider? = null
         private var objectMapperProvider: PerFieldObjectMapperProvider = PerFieldConfiguringObjectMapperProvider()
         private val proxyHandlers: MutableList<ProxyHandler> = mutableListOf(Spring4AopProxyHandler(), GuiceAopProxyHandler(), JavassistProxyHandler(), WeldProxyHandler())
@@ -93,11 +89,6 @@ data class SchemaParserOptions internal constructor(
             this.allowUnimplementedResolvers = allowUnimplementedResolvers
         }
 
-        @Deprecated("Use missingResolverDataFetcherProvider instead.")
-        fun missingResolverDataFetcher(missingResolverDataFetcher: DataFetcher<Any?>?) = this.apply {
-            this.missingResolverDataFetcher = missingResolverDataFetcher
-        }
-
         fun missingResolverDataFetcherProvider(missingResolverDataFetcherProvider: MissingResolverDataFetcherProvider?) = this.apply {
             this.missingResolverDataFetcherProvider = missingResolverDataFetcherProvider
         }
@@ -205,7 +196,6 @@ data class SchemaParserOptions internal constructor(
                 contextClass,
                 wrappers,
                 allowUnimplementedResolvers,
-                missingResolverDataFetcher,
                 missingResolverDataFetcherProvider,
                 objectMapperProvider,
                 proxyHandlers,

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

@@ -77,10 +77,7 @@ internal class FieldResolverScanner(val options: SchemaParserOptions, private va
     }
 
     private fun missingFieldResolver(field: FieldDefinition): FieldResolver? {
-        @Suppress("DEPRECATION")
-        return if (options.allowUnimplementedResolvers
-            || options.missingResolverDataFetcher != null
-            || options.missingResolverDataFetcherProvider != null) {
+        return if (options.allowUnimplementedResolvers || options.missingResolverDataFetcherProvider != null) {
             if (options.allowUnimplementedResolvers) {
                 log.warn("Missing resolver for field: $field")
             }

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

@@ -20,7 +20,6 @@ import kotlinx.coroutines.ensureActive
 import kotlinx.coroutines.future.future
 import org.apache.commons.lang3.reflect.TypeUtils
 import org.reactivestreams.Publisher
-import org.slf4j.LoggerFactory
 import java.lang.reflect.InvocationTargetException
 import java.lang.reflect.Method
 import java.util.*
@@ -41,8 +40,6 @@ internal class MethodFieldResolver(
     private val scalarNames: Set<String>
 ) : FieldResolver(field, search, options) {
 
-    private val log = LoggerFactory.getLogger(javaClass)
-
     private val isSuspendFunction = method.isSuspendFunction()
     private val numberOfParameters = method.parameterCountWithoutContinuation()
 
@@ -105,23 +102,17 @@ internal class MethodFieldResolver(
         val firstTrailingParameter = field.inputValueDefinitions.size + search.firstArgumentIndex
         method.parameterTypes.slice(firstTrailingParameter until numberOfParameters).forEach { parameterType ->
             when (parameterType) {
+                GraphQLContext::class.java -> args.add { environment -> environment.graphQlContext }
+
                 options.contextClass -> args.add { environment ->
-                    val context: Any? = environment.graphQlContext[options.contextClass]
-                    if (context != null) {
-                        context
-                    } else {
-                        log.warn(
-                            "Generic context class has been deprecated by graphql-java. " +
-                                "To continue using a custom context class as the last parameter in resolver methods " +
-                                "please insert it into the GraphQLContext map when building the ExecutionInput. " +
-                                "This warning will become an error in the future."
-                        )
-                        @Suppress("DEPRECATION")
-                        environment.getContext() // TODO: remove deprecated use in next major release
-                    }
+                    environment.graphQlContext.get<Any>(parameterType) ?: throw ResolverError(
+                        "Resolver method '${method.declaringClass.name}.${method.name}' has a parameter of the context class " +
+                            "'${parameterType.name}', but the GraphQLContext has no value for it. Put it in the GraphQLContext under " +
+                            "the class key when building the ExecutionInput, for example " +
+                            "graphQLContext(mapOf(${parameterType.simpleName}::class.java to context))."
+                    )
                 }
 
-                GraphQLContext::class.java -> args.add { environment -> environment.graphQlContext }
                 else -> args.add { environment -> environment }
             }
         }

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

@@ -11,11 +11,8 @@ internal class MissingFieldResolver(
     options: SchemaParserOptions
 ) : FieldResolver(field, FieldResolverScanner.Search(Any::class.java, MissingResolverInfo(), null), options) {
 
-    @Suppress("DEPRECATION")
     private val missingResolverDataFetcherProvider: MissingResolverDataFetcherProvider = options.missingResolverDataFetcherProvider
-            ?: MissingResolverDataFetcherProvider {
-                _, options -> options.missingResolverDataFetcher ?: DataFetcher<Any> { TODO("Schema resolver not implemented") }
-            }
+            ?: MissingResolverDataFetcherProvider { _, _ -> DataFetcher<Any> { TODO("Schema resolver not implemented") } }
     override fun scanForMatches(): List<TypeClassMatcher.PotentialMatch> = listOf()
     override fun createDataFetcher(): DataFetcher<*> = missingResolverDataFetcherProvider.createDataFetcher(field, options)
 }

src/main/kotlin/graphql/kickstart/tools/util/Utils.kt

@@ -9,8 +9,6 @@ import graphql.schema.idl.TypeUtil
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import org.apache.commons.lang3.reflect.TypeUtils
-import org.slf4j.LoggerFactory
-import java.lang.invoke.MethodHandles
 import java.lang.reflect.Method
 import java.lang.reflect.ParameterizedType
 import java.lang.reflect.Proxy
@@ -22,8 +20,6 @@ import kotlin.reflect.jvm.kotlinFunction
  * @author Andrew Potter
  */
 
-private val log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass())
-
 internal typealias GraphQLRootResolver = GraphQLResolver<Void>
 
 internal typealias JavaType = java.lang.reflect.Type
@@ -70,17 +66,6 @@ internal fun DataFetchingEnvironment.coroutineScope(): CoroutineScope {
         return scope
     }
 
-    @Suppress("DEPRECATION")
-    val context: Any? = this.getContext()
-    if (context is CoroutineScope) {
-        log.warn(
-            "Passing a CoroutineScope as the context object has been deprecated by graphql-java. " +
-                "Please insert it into the GraphQLContext map under the CoroutineScope class key when building the ExecutionInput. " +
-                "Support for the context object will be removed in the next major release."
-        )
-        return context
-    }
-
     return CoroutineScope(Dispatchers.Default)
 }
 

src/test/kotlin/graphql/kickstart/tools/MethodFieldResolverDataFetcherTest.kt

@@ -19,6 +19,8 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.channels.ReceiveChannel
+import org.junit.Assert.assertThrows
+import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.reactivestreams.Publisher
 import org.reactivestreams.tck.TestEnvironment
@@ -98,7 +100,7 @@ class MethodFieldResolverDataFetcherTest {
     }
 
     @Test
-    fun `data fetcher executes suspend function in CoroutineScope from legacy context`() {
+    fun `data fetcher does not execute suspend function in CoroutineScope from legacy context`() {
         // setup
         val scopeClass = ScopeClass()
 
@@ -107,7 +109,7 @@ class MethodFieldResolverDataFetcherTest {
         // expect
         @Suppress("UNCHECKED_CAST")
         val future = resolver.get(createEnvironment(DataClass(), legacyContext = scopeClass.scope)) as CompletableFuture<Boolean>
-        assert(future.get())
+        assertEquals(future.get(), false)
     }
 
     class ScopeClass : GraphQLResolver<DataClass> {
@@ -116,7 +118,7 @@ class MethodFieldResolverDataFetcherTest {
 
         @Suppress("UNUSED_PARAMETER")
         suspend fun isActive(data: DataClass): Boolean {
-            return coroutineContext[Job] == job.children.first()
+            return coroutineContext[Job] == job.children.firstOrNull()
         }
     }
 
@@ -275,6 +277,47 @@ class MethodFieldResolverDataFetcherTest {
         assertEquals(resolver.get(createEnvironment(DataClass(), context = context)), true)
     }
 
+    @Test
+    fun `data fetcher fails if custom context is missing from GraphQLContext`() {
+        val options = SchemaParserOptions.newOptions().contextClass(ContextClass::class).build()
+        val resolver = createFetcher("active", options = options, resolver = object : GraphQLResolver<DataClass> {
+            fun isActive(dataClass: DataClass, ctx: ContextClass?): Boolean {
+                return ctx != null
+            }
+        })
+
+        val error = assertThrows(ResolverError::class.java) { resolver.get(createEnvironment(DataClass())) }
+
+        assertTrue(error.message!!.contains("'${ContextClass::class.java.name}'"))
+        assertTrue(error.message!!.contains(".isActive'"))
+        assertTrue(error.message!!.contains("GraphQLContext"))
+    }
+
+    @Test
+    fun `data fetcher fails if custom context is only passed as the legacy context object`() {
+        val options = SchemaParserOptions.newOptions().contextClass(ContextClass::class).build()
+        val resolver = createFetcher("active", options = options, resolver = object : GraphQLResolver<DataClass> {
+            fun isActive(dataClass: DataClass, ctx: ContextClass): Boolean {
+                return true
+            }
+        })
+
+        assertThrows(ResolverError::class.java) { resolver.get(createEnvironment(DataClass(), legacyContext = ContextClass())) }
+    }
+
+    @Test
+    fun `data fetcher passes GraphQLContext if it is the custom context class`() {
+        val context = GraphQLContext.newContext().build()
+        val options = SchemaParserOptions.newOptions().contextClass(GraphQLContext::class).build()
+        val resolver = createFetcher("active", options = options, resolver = object : GraphQLResolver<DataClass> {
+            fun isActive(dataClass: DataClass, ctx: GraphQLContext): Boolean {
+                return ctx == context
+            }
+        })
+
+        assertEquals(resolver.get(createEnvironment(DataClass(), context = context)), true)
+    }
+
     @Test
     fun `data fetcher passes custom context and environment if method has both as extra arguments`() {
         val customContext = ContextClass()

src/test/kotlin/graphql/kickstart/tools/MissingFieldResolverTest.kt

@@ -58,46 +58,6 @@ class MissingFieldResolverTest {
         ))
     }
 
-    @Test
-    @Suppress("DEPRECATION")
-    fun `should call missing resolver data fetcher if provided`() {
-        val schema = SchemaParser.newParser()
-            .schemaString(
-                """
-                type Query {
-                    implementedField(input: String): String
-                    missingField(input: Int): Int
-                }
-                """
-            )
-            .resolvers(object : GraphQLQueryResolver {
-                fun implementedField(input: Optional<String>) = input.toString()
-            })
-            .options(SchemaParserOptions.newOptions()
-                .missingResolverDataFetcher(TestMissingResolverDataFetcher())
-                .build())
-            .build()
-            .makeExecutableSchema()
-            .assertEveryFieldResolves()
-
-        val gql = GraphQL.newGraphQL(schema).build()
-
-        val result = gql.execute(
-            """
-            query {
-                implementedField(input: "test-value")
-                missingField(input: 1)
-            }
-            """)
-
-        val expected = mapOf(
-            "implementedField" to "Optional[test-value]",
-            "missingField" to 1
-        )
-
-        assertEquals(result.getData(), expected)
-    }
-
     @Test
     fun `should call missing resolver data fetcher provider if provided`() {
         val missingResolverDataFetcherProvider = TestMissingResolverDataFetcherProvider();