Merge pull request #890 from graphql-java-kickstart/feature/418-warn-argument-on-context

Warn at build when a resolver method is missing a parameter for an argument

Author: oryan-block

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

@@ -9,6 +9,7 @@ import graphql.kickstart.tools.util.*
 import graphql.language.*
 import graphql.schema.GraphQLScalarType
 import graphql.schema.idl.ScalarInfo
+import graphql.schema.idl.TypeUtil
 import org.apache.commons.lang3.ClassUtils
 import org.apache.commons.lang3.reflect.TypeUtils
 import org.slf4j.LoggerFactory
@@ -528,6 +529,7 @@ internal class SchemaClassScanner(
             fieldResolver.scanForMatches().forEach { potentialMatch ->
                 handleFoundType(typeClassMatcher.match(potentialMatch))
             }
+            warnIfParameterMissing(type, fieldResolver)
         }
     }
 
@@ -552,6 +554,32 @@ internal class SchemaClassScanner(
         )
     }
 
+    // TODO throw a FieldResolverError instead in 15.0.0 (#889)
+    private fun warnIfParameterMissing(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
+        if (fieldResolver !is MethodFieldResolver) {
+            return
+        }
+
+        fieldResolver.field.inputValueDefinitions.forEachIndexed { index, argument ->
+            val parameter = fieldResolver.getMethodParameterType(index)?.let(fieldResolver.typeResolver::getRawClass) ?: return@forEachIndexed
+            if (!isContextParameter(parameter, argument)) {
+                return@forEachIndexed
+            }
+
+            val method = fieldResolver.method
+            log.warn(
+                "Field '${type.name}.${fieldResolver.field.name}' has no parameter for its argument '${argument.name}: " +
+                    "${TypeUtil.simplePrint(argument.type)}' in ${method.declaringClass.name}.${method.name}, so the ${parameter.simpleName} " +
+                    "parameter gets the argument instead. Add a parameter for each argument, in order, before the ${parameter.simpleName} parameter."
+            )
+        }
+    }
+
+    // the context class can also be an argument's type (e.g. a Map for an input object), so only spec scalars count for it
+    private fun isContextParameter(parameter: Class<*>, argument: InputValueDefinition): Boolean =
+        parameter in fieldResolverScanner.allowedTrailingParameterTypes &&
+            (parameter != options.contextClass || argument.type.unwrap().name in ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS)
+
     private fun handleFoundType(match: TypeClassMatcher.Match) = when (match) {
         is TypeClassMatcher.ScalarMatch -> handleFoundScalarType(match.type)
         is TypeClassMatcher.ValidMatch -> handleFoundType(match.type, match.javaType, match.reference)

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

@@ -25,7 +25,7 @@ internal class FieldResolverScanner(val options: SchemaParserOptions) {
 
     private val log = LoggerFactory.getLogger(javaClass)
 
-    private val allowedTrailingParameterTypes = listOfNotNull(DataFetchingEnvironment::class.java, GraphQLContext::class.java, options.contextClass)
+    val allowedTrailingParameterTypes = listOfNotNull(DataFetchingEnvironment::class.java, GraphQLContext::class.java, options.contextClass)
 
     private val methodsByNameCache = mutableMapOf<Pair<Class<out Any>, Boolean>, Map<String, List<Method>>>()
 

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

@@ -10,6 +10,7 @@ import graphql.language.Value
 import graphql.schema.Coercing
 import graphql.schema.CoercingParseLiteralException
 import graphql.schema.CoercingSerializeException
+import graphql.schema.DataFetchingEnvironment
 import graphql.schema.GraphQLScalarType
 import org.junit.Test
 import java.lang.reflect.InvocationHandler
@@ -283,6 +284,60 @@ class MethodFieldResolverTest {
         ))
     }
 
+    @Test
+    fun `should warn when a resolver method is missing a parameter for an argument`() {
+        val (_, warnings) = withWarnings {
+            SchemaParser.newParser()
+                .schemaString(
+                    """
+                    type Query {
+                        user(id: ID!, active: Boolean!): String
+                        users(active: Boolean, first: Int): String
+                        image: Image
+                        search(filter: Filter): String
+                        count(active: Boolean): String
+                    }
+
+                    type Image {
+                        thumbnail(size: Int): String
+                    }
+
+                    input Filter {
+                        name: String
+                    }
+                    """)
+                .resolvers(MissingParameterQuery(), ImageResolver())
+                .options(SchemaParserOptions.newOptions().contextClass(Map::class).build())
+                .build()
+                .makeExecutableSchema()
+        }
+
+        val query = MissingParameterQuery::class.java.name
+        assertEquals(warnings, listOf(
+            "Field 'Query.user' has no parameter for its argument 'active: Boolean!' in $query.user, so the Map parameter gets the " +
+                "argument instead. Add a parameter for each argument, in order, before the Map parameter.",
+            "Field 'Query.users' has no parameter for its argument 'active: Boolean' in $query.users, so the GraphQLContext parameter " +
+                "gets the argument instead. Add a parameter for each argument, in order, before the GraphQLContext parameter.",
+            "Field 'Image.thumbnail' has no parameter for its argument 'size: Int' in ${ImageResolver::class.java.name}.thumbnail, so the " +
+                "DataFetchingEnvironment parameter gets the argument instead. Add a parameter for each argument, in order, before the " +
+                "DataFetchingEnvironment parameter."
+        ))
+    }
+
+    class MissingParameterQuery : GraphQLQueryResolver {
+        fun user(id: String, context: Map<String, Any>) = "$id $context"
+        fun users(context: GraphQLContext?, first: Int?) = "$context $first"
+        fun image() = Image()
+        fun search(filter: Map<String, Any>?) = filter.toString()
+        fun count(active: Boolean?, context: Map<String, Any>) = "$active $context"
+    }
+
+    class Image
+
+    class ImageResolver : GraphQLResolver<Image> {
+        fun thumbnail(image: Image, env: DataFetchingEnvironment?) = env.toString()
+    }
+
     sealed class Maybe<out T> {
         data class Some<T>(val value: T) : Maybe<T>()
         object None : Maybe<Nothing>()