Merge pull request #911 from graphql-java-kickstart/feature/889-throw-missing-parameter

Throw when a resolver method is missing a parameter for an argument

Author: oryan-block

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

@@ -6,6 +6,7 @@ import graphql.kickstart.tools.ScanDictionary.InitialDictionaryEntry
 import graphql.kickstart.tools.ScanDictionary.UnionMemberReference
 import graphql.kickstart.tools.ScanDictionary.UnusedTypeReference
 import graphql.kickstart.tools.resolver.FieldResolver
+import graphql.kickstart.tools.resolver.FieldResolverError
 import graphql.kickstart.tools.resolver.FieldResolverScanner
 import graphql.kickstart.tools.resolver.MethodFieldResolver
 import graphql.kickstart.tools.resolver.PropertyFieldResolver
@@ -457,7 +458,7 @@ internal class SchemaClassScanner(
             fieldResolver.scanForMatches().forEach { potentialMatch ->
                 dictionary.handleFoundType(typeClassMatcher.match(potentialMatch))
             }
-            warnIfParameterMissing(type, fieldResolver)
+            requireParameterForEachArgument(type, fieldResolver)
         }
     }
 
@@ -482,8 +483,7 @@ internal class SchemaClassScanner(
         )
     }
 
-    // TODO throw a FieldResolverError instead in 15.0.0 (#889)
-    private fun warnIfParameterMissing(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
+    private fun requireParameterForEachArgument(type: ObjectTypeDefinition, fieldResolver: FieldResolver) {
         if (fieldResolver !is MethodFieldResolver) {
             return
         }
@@ -495,7 +495,7 @@ internal class SchemaClassScanner(
             }
 
             val method = fieldResolver.method
-            log.warn(
+            throw FieldResolverError(
                 "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."

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

@@ -11,8 +11,12 @@ import graphql.language.Value
 import graphql.schema.Coercing
 import graphql.schema.CoercingParseLiteralException
 import graphql.schema.CoercingSerializeException
+import graphql.kickstart.tools.resolver.FieldResolverError
+import graphql.kickstart.tools.resolver.MissingResolverDataFetcherProvider
+import graphql.schema.DataFetcher
 import graphql.schema.DataFetchingEnvironment
 import graphql.schema.GraphQLScalarType
+import org.junit.Assert.assertThrows
 import org.junit.Test
 import java.lang.reflect.InvocationHandler
 import java.lang.reflect.Method
@@ -311,45 +315,77 @@ 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()
-        }
-
+    fun `should fail the build when a resolver method is missing a parameter for an argument`() {
         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 " +
+        val messagesByQueryField = mapOf(
+            "user(id: ID!, active: Boolean!): String" to
+                "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 " +
+            "users(active: Boolean, first: Int): String" to
+                "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 " +
+            "image: Image" to
+                "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."
-        ))
+        )
+
+        messagesByQueryField.forEach { (queryField, message) ->
+            val error = assertThrows(FieldResolverError::class.java) {
+                buildMissingParameterSchema(queryField, SchemaParserOptions.newOptions())
+            }
+
+            assertEquals(error.message, message)
+        }
+    }
+
+    @Test
+    fun `should fail the build for a missing parameter when unimplemented resolvers are allowed`() {
+        val options = listOf(
+            SchemaParserOptions.newOptions().allowUnimplementedResolvers(true),
+            SchemaParserOptions.newOptions().missingResolverDataFetcherProvider(MissingResolverDataFetcherProvider { _, _ -> DataFetcher { "provided" } })
+        )
+
+        options.forEach {
+            val error = assertThrows(FieldResolverError::class.java) {
+                buildMissingParameterSchema("users(active: Boolean, first: Int): String", it)
+            }
+
+            assert("Field 'Query.users' has no parameter for its argument 'active: Boolean'" in error.message!!) { error.message!! }
+        }
+    }
+
+    @Test
+    fun `should build when the context class is an argument type`() {
+        val schema = buildMissingParameterSchema(
+            "search(filter: Filter): String\ncount(active: Boolean): String",
+            SchemaParserOptions.newOptions()
+        )
+
+        assertEquals(schema.queryType.fieldDefinitions.map { it.name }, listOf("search", "count"))
     }
 
+    private fun buildMissingParameterSchema(queryField: String, options: SchemaParserOptions.Builder) =
+        SchemaParser.newParser()
+            .schemaString(
+                """
+                type Query {
+                    $queryField
+                }
+
+                type Image {
+                    thumbnail(size: Int): String
+                }
+
+                input Filter {
+                    name: String
+                }
+                """)
+            .resolvers(MissingParameterQuery(), ImageResolver())
+            .options(options.contextClass(Map::class).build())
+            .build()
+            .makeExecutableSchema()
+
     class MissingParameterQuery : GraphQLQueryResolver {
         fun user(id: String, context: Map<String, Any>) = "$id $context"
         fun users(context: GraphQLContext?, first: Int?) = "$context $first"

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

@@ -432,29 +432,27 @@ class SourcelessResolverTest {
     }
 
     @Test
-    fun `no argument warning for a method without the source`() {
-        val (_, warnings) = withWarnings {
-            SchemaParser.newParser()
-                .schemaString(
-                    """
-                    type Query {
-                        folder: Folder!
-                    }
+    fun `a method without the source needs no parameter for the source`() {
+        val schema = SchemaParser.newParser()
+            .schemaString(
+                """
+                type Query {
+                    folder: Folder!
+                }
 
-                    type Folder {
-                        images: [Image!]!
-                    }
+                type Folder {
+                    images: [Image!]!
+                }
 
-                    type Image {
-                        thumbnail(size: Int!): String!
-                    }
-                    """)
-                .resolvers(FolderQuery(), FolderResolver(), SourcelessImageResolver())
-                .build()
-                .makeExecutableSchema()
-        }
+                type Image {
+                    thumbnail(size: Int!): String!
+                }
+                """)
+            .resolvers(FolderQuery(), FolderResolver(), SourcelessImageResolver())
+            .build()
+            .makeExecutableSchema()
 
-        assertEquals(warnings.filter { "has no parameter for its argument" in it }, listOf())
+        assertEquals(schema.getObjectType("Image")!!.getFieldDefinition("thumbnail").arguments.map { it.name }, listOf("size"))
     }
 
     @Test