Let input types share a dictionary class

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

@@ -1,5 +1,6 @@
 package graphql.kickstart.tools
 
+import com.fasterxml.jackson.databind.JsonNode
 import graphql.kickstart.tools.resolver.FieldResolver
 import graphql.kickstart.tools.resolver.FieldResolverScanner
 import graphql.kickstart.tools.util.*
@@ -8,13 +9,14 @@ import graphql.schema.GraphQLScalarType
 import graphql.schema.idl.ScalarInfo
 import org.apache.commons.lang3.ClassUtils
 import org.slf4j.LoggerFactory
+import java.lang.reflect.Member
 import java.lang.reflect.Method
 
 /**
  * @author Andrew Potter
  */
 internal class SchemaClassScanner(
-    initialDictionary: BiMap<String, Class<*>>,
+    initialDictionary: Map<String, Class<*>>,
     allDefinitions: List<Definition<*>>,
     resolvers: List<GraphQLResolver<*>>,
     private val scalars: CustomScalarMap,
@@ -45,6 +47,7 @@ internal class SchemaClassScanner(
     private val typeClassMatcher = TypeClassMatcher(definitionsByName)
     private val dictionary = mutableMapOf<TypeDefinition<*>, DictionaryEntry>()
     private val unvalidatedTypes = mutableSetOf<TypeDefinition<*>>()
+    private val enumsWithoutClass = mutableMapOf<EnumTypeDefinition, String>()
     private val queue = linkedSetOf<QueueItem>()
 
     private val fieldResolversByType = mutableMapOf<ObjectTypeDefinition, MutableMap<FieldDefinition, FieldResolver>>()
@@ -56,6 +59,18 @@ internal class SchemaClassScanner(
             }
         }
 
+        // Input types never go into the type => class bimap, so several of them can share a class, e.g. ObjectNode. Every
+        // other kind still gets one class per type, as before. Most of them end up in the bimap, which interface and union
+        // type resolution use to find the type of an object.
+        initialDictionary
+            .filterKeys { definitionsByName[it] !is InputObjectTypeDefinition }
+            .entries.groupBy({ it.value }, { it.key })
+            .filterValues { it.size > 1 }
+            .entries.firstOrNull()
+            ?.let { (clazz, names) ->
+                throw SchemaClassScannerError("Class in supplied dictionary '${clazz.name}' specified type names ${names.joinToString { "'$it'" }}, but only input object types can share a class!")
+            }
+
         if (options.allowUnimplementedResolvers) {
             log.warn("Option 'allowUnimplementedResolvers' should only be set to true during development, as it can cause schema errors to be moved to query time instead of schema creation time.  Make sure this is turned off in production.")
         }
@@ -181,6 +196,11 @@ internal class SchemaClassScanner(
     }
 
     private fun validateAndCreateResult(rootTypeHolder: RootTypesHolder): ScannedSchemaObjects {
+        enumsWithoutClass
+            .filter { dictionary[it.key]?.javaType == null }
+            .values.firstOrNull()
+            ?.let { throw SchemaClassScannerError(it) }
+
         initialDictionary
             .filter { !it.value.accessed }
             .forEach {
@@ -412,31 +432,83 @@ internal class SchemaClassScanner(
                 scanInterfacesOfType(graphQLType)
             }
 
-            is InputObjectTypeDefinition -> {
-                val inputObjectTypes = listOf(graphQLType) + inputExtensionDefinitions.filter { it.name == graphQLType.name }
-                inputObjectTypes
-                    .flatMap { it.inputValueDefinitions }
-                    .forEach { inputValueDefinition ->
-                        val inputGraphQLType = inputValueDefinition.type.unwrap()
-                        if (inputGraphQLType is TypeName && !ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS.containsKey(inputGraphQLType.name)) {
-                            val inputValueJavaType = findInputValueType(inputValueDefinition.name, inputGraphQLType, javaType.unwrap())
-                            if (inputValueJavaType != null) {
-                                handleFoundType(typeClassMatcher.match(TypeClassMatcher.PotentialMatch.parameterType(
-                                    inputValueDefinition.type,
-                                    inputValueJavaType,
-                                    GenericTypeResolver(javaType, options),
-                                    InputObjectReference(inputValueDefinition)
-                                )))
-                            } else {
-                                var mappingAdvice = "Try adding it manually to the dictionary"
-                                if (javaType.unwrap().name.contains("Map")) {
-                                    mappingAdvice = " or add a class to represent your input type instead of a Map."
-                                }
-                                log.warn("Cannot find definition for field '${inputValueDefinition.name}: ${inputGraphQLType.name}' on input type '${graphQLType.name}' -> ${javaType.unwrap().name}. $mappingAdvice")
-                            }
-                        }
+            is InputObjectTypeDefinition -> handleInputObjectFields(graphQLType, javaType)
+        }
+    }
+
+    /**
+     * Finds the types of the input object's fields through the properties of its class. A null [javaType] means the input object
+     * has no known class, e.g. because it's only reachable through a Map.
+     */
+    private fun handleInputObjectFields(graphQLType: InputObjectTypeDefinition, javaType: JavaType?) {
+        val inputObjectTypes = listOf(graphQLType) + inputExtensionDefinitions.filter { it.name == graphQLType.name }
+        inputObjectTypes
+            .flatMap { it.inputValueDefinitions }
+            .forEach { inputValueDefinition ->
+                val inputGraphQLType = inputValueDefinition.type.unwrap()
+                if (inputGraphQLType is TypeName && !ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS.containsKey(inputGraphQLType.name)) {
+                    val propertyType = javaType?.unwrap()?.let { findInputValueTypeInType(inputValueDefinition.name, it) }
+                    val inputValueJavaType = propertyType ?: initialDictionary[inputGraphQLType.name]?.get()
+                    if (inputValueJavaType != null) {
+                        val genericContext = javaType ?: inputValueJavaType
+                        handleFoundType(typeClassMatcher.match(TypeClassMatcher.PotentialMatch.parameterType(
+                            // a class from the dictionary is for the unwrapped type, not for its list or non-null wrapper
+                            if (propertyType != null) inputValueDefinition.type else inputGraphQLType,
+                            inputValueJavaType,
+                            GenericTypeResolver(genericContext, options),
+                            InputObjectReference(inputValueDefinition)
+                        )))
+                    } else {
+                        handleInputValueWithoutJavaType(graphQLType, inputValueDefinition, inputGraphQLType, javaType?.unwrap())
                     }
+                }
+            }
+    }
+
+    /**
+     * Without a Java property for an input field its type can still be built, as long as it doesn't need a class of its own.
+     * At runtime the value is whatever the input object's class (or the Map) deserializes it into.
+     */
+    private fun handleInputValueWithoutJavaType(graphQLType: InputObjectTypeDefinition, inputValueDefinition: InputValueDefinition, inputGraphQLType: TypeName, clazz: Class<*>?) {
+        if (clazz != null && !isPropertyMap(clazz)) {
+            log.warn("Cannot find a public getter or field for field '${inputValueDefinition.name}: ${inputGraphQLType.name}' of input type '${graphQLType.name}' on ${clazz.name}. " +
+                "Unless Jackson maps it some other way, e.g. through @JsonProperty, check that the property name matches the schema, " +
+                "otherwise the field's value is rejected or dropped when the input is deserialized.")
+        }
+
+        when (val definition = definitionsByName[inputGraphQLType.name]) {
+            is ScalarTypeDefinition -> handleFoundScalarType(definition)
+            is InputObjectTypeDefinition -> {
+                val isNewType = !dictionary.containsKey(definition)
+                // registered without a class, so a class found for it later still gets set and scanned
+                handleFoundType(definition, null, InputObjectReference(inputValueDefinition))
+                if (isNewType) {
+                    handleInputObjectFields(definition, null)
+                }
             }
+            // the enum class may still be found through another field, so this is only reported once scanning is done
+            is EnumTypeDefinition -> enumsWithoutClass.putIfAbsent(definition, enumWithoutClassMessage(definition, graphQLType, inputValueDefinition, clazz))
+            // undefined types and output types are reported by the SchemaParser
+            else -> {}
+        }
+    }
+
+    private fun isPropertyMap(clazz: Class<*>) =
+        java.util.Map::class.java.isAssignableFrom(clazz) || JsonNode::class.java.isAssignableFrom(clazz) || clazz == Object::class.java
+
+    private fun isLibraryClass(clazz: Class<*>) =
+        listOf("java.", "javax.", "kotlin.", "com.fasterxml.jackson.").any { clazz.name.startsWith(it) }
+
+    private fun enumWithoutClassMessage(enum: EnumTypeDefinition, graphQLType: InputObjectTypeDefinition, inputValueDefinition: InputValueDefinition, clazz: Class<*>?): String {
+        val field = inputValueDefinition.name
+        val problem = "Enum type '${enum.name}' is used by field '$field' of input type '${graphQLType.name}', but its Java enum can't be determined"
+        return when {
+            clazz == null -> "$problem because no class was found for input type '${graphQLType.name}'. " +
+                "Pass the enum class for type '${enum.name}' in the parser's dictionary, or a class for '${graphQLType.name}' with a public getter or field '$field'."
+            isPropertyMap(clazz) && isLibraryClass(clazz) -> "$problem because '${graphQLType.name}' is bound to ${clazz.name}, which has no type information for its fields. " +
+                "Pass the enum class for type '${enum.name}' in the parser's dictionary, or use a class for '${graphQLType.name}' with a public getter or field '$field'."
+            else -> "$problem because ${clazz.name} has no public getter or field named '$field'. " +
+                "Add a public getter or field '$field' to the class used for '${graphQLType.name}', or pass the enum class for type '${enum.name}' in the parser's dictionary."
         }
     }
 
@@ -453,25 +525,19 @@ internal class SchemaClassScanner(
         queue.add(QueueItem(graphQLType, javaType))
     }
 
-    private fun findInputValueType(name: String, inputGraphQLType: TypeName, clazz: Class<out Any>): JavaType? {
-        val inputValueType = findInputValueTypeInType(name, clazz)
-        if (inputValueType != null) {
-            return inputValueType
-        }
-
-        return initialDictionary[inputGraphQLType.name]?.get()
-    }
-
     private fun findInputValueTypeInType(name: String, clazz: Class<*>): JavaType? {
-        val methods = clazz.methods
+        // a Map or JsonNode has no properties for the input's fields, only methods that may share their names, e.g. ObjectNode.fields(),
+        // but a subclass of one may still declare typed getters or fields of its own
+        val isProperty = { member: Member -> !isPropertyMap(clazz) || !isLibraryClass(member.declaringClass) }
+        val methods = clazz.methods.filter(isProperty)
 
         val (getters, methodsWithParameters) = methods.filter {
             it.name == name || it.name == "get${name.replaceFirstChar(Char::titlecase)}"
         }.sortedBy { it.name.length }.partition { it.parameterCount == 0 }
 
         return getters.find { !it.isSynthetic }?.genericReturnType
             ?: getters.firstOrNull()?.genericReturnType
-            ?: clazz.fields.find { it.name == name }?.genericType
+            ?: clazz.fields.find { it.name == name && isProperty(it) }?.genericType
             ?: methodsWithParameters.find { !it.isSynthetic }?.genericReturnType
             ?: methodsWithParameters.firstOrNull()?.genericReturnType
     }

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

@@ -506,8 +506,7 @@ class SchemaParser internal constructor(
                     scalarType
                 } else {
                     if (!allowedTypeReferences.contains(typeDefinition.name)) {
-                        throw SchemaError("Expected type '${typeDefinition.name}' to be a ${expectedType.simpleName}, but it wasn't!  " +
-                            "Was a type only permitted for object types incorrectly used as an input type, or vice-versa?")
+                        throw unexpectedTypeError(typeDefinition, expectedType)
                     }
                     inputObjects.find { it.name == typeDefinition.name } ?: GraphQLTypeReference(typeDefinition.name)
                 }
@@ -538,8 +537,7 @@ class SchemaParser internal constructor(
                     scalarType
                 } else {
                     if (!allowedTypeReferences.contains(typeDefinition.name)) {
-                        throw SchemaError("Expected type '${typeDefinition.name}' to be a ${expectedType.simpleName}, but it wasn't!  " +
-                            "Was a type only permitted for object types incorrectly used as an input type, or vice-versa?")
+                        throw unexpectedTypeError(typeDefinition, expectedType)
                     }
                     val found = inputObjects.filter { it.name == typeDefinition.name }
                     if (found.size == 1) {
@@ -565,6 +563,65 @@ class SchemaParser internal constructor(
             else -> throw SchemaError("Unknown type: $typeDefinition")
         }
 
+    private fun unexpectedTypeError(typeName: TypeName, expectedType: KClass<*>): SchemaError {
+        val name = typeName.name
+        val subject = "Type '$name'" +
+            (findUsage(typeName)?.let { " used by $it" } ?: "") +
+            (typeName.sourceLocation?.let { " at line ${it.line}, column ${it.column}" + (it.sourceName?.let { source -> " of $source" } ?: "") } ?: "")
+        // unused ones first, since the extensions of an unused type are still among the definitions
+        val definition = unusedDefinitions.find { it.name == name }
+            ?: definitions.filterIsInstance<TypeDefinition<*>>().find { it.name == name }
+        val isInput = expectedType == GraphQLInputType::class
+        val kind = when (definition) {
+            is InputObjectTypeDefinition -> "an input object type"
+            is InterfaceTypeDefinition -> "an interface"
+            is UnionTypeDefinition -> "a union"
+            else -> "an object type"
+        }
+
+        return SchemaError(when {
+            definition == null -> "$subject is not defined in the schema."
+            isInput && definition !is InputObjectTypeDefinition && definition !is EnumTypeDefinition && definition !is ScalarTypeDefinition ->
+                "$subject is $kind, which can't be used as an input type. Input types are input objects, enums and scalars."
+            !isInput && definition is InputObjectTypeDefinition ->
+                "$subject is $kind, which can't be used as an output type. Output types are object types, interfaces, unions, enums and scalars."
+            definition in unusedDefinitions ->
+                "$subject is defined in the schema, but was not reached while scanning the resolvers and the classes they use, so it wasn't built. " +
+                    unreachedTypeHint(definition)
+            else -> "$subject can't be used as ${if (isInput) "an input" else "an output"} type."
+        })
+    }
+
+    // what makes the scanner reach the type depends on its kind, see SchemaClassScanner.canIncludeUnusedType
+    private fun unreachedTypeHint(definition: TypeDefinition<*>): String = when (definition) {
+        is InterfaceTypeDefinition, is UnionTypeDefinition ->
+            "Return it from a resolver method or one of those classes, or set SchemaParserOptions.includeUnusedTypes."
+        is EnumTypeDefinition ->
+            "Use it in a resolver method or one of those classes, or set SchemaParserOptions.includeUnusedTypes and pass its enum class in the parser's dictionary."
+        is InputObjectTypeDefinition ->
+            "Use it in an argument of a resolver method, directly or through another input type. SchemaParserOptions.includeUnusedTypes doesn't include input types."
+        is ScalarTypeDefinition ->
+            "Use it in a resolver method or one of those classes. SchemaParserOptions.includeUnusedTypes doesn't include scalars."
+        else ->
+            "Return it from a resolver method or one of those classes, or set SchemaParserOptions.includeUnusedTypes and pass a class for it in the parser's dictionary."
+    }
+
+    // determineType only gets the type reference, so the definition using it is looked up for the error message
+    private fun findUsage(typeName: TypeName): String? = definitions.asSequence().flatMap { definition ->
+        when (definition) {
+            is ObjectTypeDefinition -> fieldUsages(definition.name, definition.fieldDefinitions)
+            is InterfaceTypeDefinition -> fieldUsages(definition.name, definition.fieldDefinitions)
+            is InputObjectTypeDefinition -> definition.inputValueDefinitions.map { "input field '${definition.name}.${it.name}'" to it.type }
+            is DirectiveDefinition -> definition.inputValueDefinitions.map { "argument '${it.name}' of directive '@${definition.name}'" to it.type }
+            else -> emptyList()
+        }
+    }.find { (_, type) -> type.unwrap() === typeName }?.first
+
+    private fun fieldUsages(typeName: String, fields: List<FieldDefinition>): List<Pair<String, Type<*>>> = fields.flatMap { field ->
+        listOf("field '$typeName.${field.name}'" to field.type) +
+            field.inputValueDefinitions.map { "argument '${it.name}' of field '$typeName.${field.name}'" to it.type }
+    }
+
     /**
      * Returns an optional [String] describing a deprecated field/enum.
      * If a deprecation directive was defined using the @deprecated directive,

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

@@ -5,9 +5,14 @@ import kotlin.reflect.KClass
 
 class SchemaParserDictionary {
 
-    private val dictionary: BiMap<String, Class<*>> = BiMap.create()
+    // Not a BiMap, since several input types can share a class, e.g. ObjectNode. The scanner checks that other types don't.
+    private val dictionary = linkedMapOf<String, Class<*>>()
 
-    fun getDictionary(): BiMap<String, Class<*>> = BiMap.unmodifiableBiMap(dictionary)
+    /**
+     * Returns a copy of the entries added so far. If several types share a class, the inverse of the copy only maps that class
+     * back to the first of them.
+     */
+    fun getDictionary(): BiMap<String, Class<*>> = BiMap.unmodifiableCopyOf(dictionary)
 
     /**
      * Add arbitrary classes to the parser's dictionary, overriding the generated type name.

src/main/kotlin/graphql/kickstart/tools/util/BiMap.java

@@ -1,6 +1,7 @@
 package graphql.kickstart.tools.util;
 
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -21,6 +22,17 @@ public static <K, V> BiMap<K, V> unmodifiableBiMap(BiMap<K, V> biMap) {
         return new BiMap<>(unmodifiableMap(biMap.delegate), unmodifiableMap(biMap.inverse));
     }
 
+    /**
+     * Returns an unmodifiable bimap with the entries of the given map. Unlike {@link #put(Object, Object)}, several keys may
+     * have the same value. The inverse only maps such a value back to the first of its keys, so the inverse and
+     * {@link #values()} can have fewer entries than the bimap.
+     */
+    public static <K, V> BiMap<K, V> unmodifiableCopyOf(Map<K, V> map) {
+        Map<V, K> inverse = new HashMap<>();
+        map.forEach((key, value) -> inverse.putIfAbsent(value, key));
+        return new BiMap<>(unmodifiableMap(new LinkedHashMap<>(map)), unmodifiableMap(inverse));
+    }
+
     private BiMap(Map<K, V> delegate, Map<V, K> inverse) {
         this.delegate = delegate;
         this.inverse = inverse;