diff --git a/src/main/kotlin/graphql/kickstart/tools/DictionaryTypeResolver.kt b/src/main/kotlin/graphql/kickstart/tools/DictionaryTypeResolver.kt index b2e6a6cf..40a52802 100644 --- a/src/main/kotlin/graphql/kickstart/tools/DictionaryTypeResolver.kt +++ b/src/main/kotlin/graphql/kickstart/tools/DictionaryTypeResolver.kt @@ -1,7 +1,6 @@ package graphql.kickstart.tools import graphql.TypeResolutionEnvironment -import graphql.kickstart.tools.util.BiMap import graphql.kickstart.tools.util.JavaType import graphql.kickstart.tools.util.unwrap import graphql.language.TypeDefinition @@ -15,7 +14,7 @@ import java.lang.reflect.ParameterizedType * @author Andrew Potter */ internal abstract class DictionaryTypeResolver( - private val dictionary: BiMap> + private val dictionary: Map> ) : TypeResolver { // Raw class of each parameterized entry, e.g. Box for Box, unless several entries share it private val rawClassDictionary by lazy { @@ -46,7 +45,7 @@ internal abstract class DictionaryTypeResolver( } internal class InterfaceTypeResolver( - dictionary: BiMap>, + dictionary: Map>, private val thisInterface: GraphQLInterfaceType ) : DictionaryTypeResolver( dictionary @@ -55,7 +54,7 @@ internal class InterfaceTypeResolver( } internal class UnionTypeResolver( - dictionary: BiMap>, + dictionary: Map>, private val thisUnion: GraphQLUnionType ) : DictionaryTypeResolver( dictionary diff --git a/src/main/kotlin/graphql/kickstart/tools/ScannedSchemaObjects.kt b/src/main/kotlin/graphql/kickstart/tools/ScannedSchemaObjects.kt index 5009c117..1a60dce9 100644 --- a/src/main/kotlin/graphql/kickstart/tools/ScannedSchemaObjects.kt +++ b/src/main/kotlin/graphql/kickstart/tools/ScannedSchemaObjects.kt @@ -1,7 +1,6 @@ package graphql.kickstart.tools import graphql.kickstart.tools.resolver.FieldResolver -import graphql.kickstart.tools.util.BiMap import graphql.kickstart.tools.util.JavaType import graphql.language.EnumTypeDefinition import graphql.language.FieldDefinition @@ -24,6 +23,15 @@ internal data class ScannedSchemaObjects( val directiveOnlyEnums: Set ) -internal typealias TypeClassDictionary = BiMap, JavaType> +internal class TypeClassDictionary(val classByType: Map, JavaType>) { + val typeByClass: Map> = classByType.entries + .groupBy({ it.value }, { it.key }) + .mapValues { (javaType, types) -> + types.singleOrNull() ?: throw SchemaClassScannerError( + "Class '${javaType.typeName}' is used for types ${types.joinToString { "'${it.name}'" }}, but only input object types can share a class!" + ) + } +} + internal typealias CustomScalarMap = Map internal typealias TypeExtensions = Map>> diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt index fa2d1a77..5b90d7c2 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt @@ -264,23 +264,18 @@ internal class SchemaClassScanner( val observedDefinitions = dictionary.keys.toSet() + unvalidatedTypes // The dictionary doesn't need to know what classes are used with scalars. - // In addition, scalars can have duplicate classes so that breaks the bi-map. // Input types can also be excluded from the dictionary, since it's only used for interfaces, unions, and enums. // Union types can also be excluded, as their possible types are resolved recursively later - val dictionary = try { - BiMap.unmodifiableBiMap(BiMap.create, JavaType>().also { - dictionary - .filter { - it.value.javaType != null - && it.value.typeClass() != Any::class.java - && !Map::class.java.isAssignableFrom(it.value.typeClass()) - && it.key !is InputObjectTypeDefinition - && it.key !is UnionTypeDefinition - }.mapValuesTo(it) { it.value.javaType } - }) - } catch (t: Throwable) { - throw SchemaClassScannerError("Error creating bimap of type => class", t) - } + val dictionary = TypeClassDictionary( + dictionary + .filter { + it.value.javaType != null + && it.value.typeClass() != Any::class.java + && !Map::class.java.isAssignableFrom(it.value.typeClass()) + && it.key !is InputObjectTypeDefinition + && it.key !is UnionTypeDefinition + }.mapValues { it.value.javaType!! } + ) // SchemaParser builds the scalars, this only checks that each one was provided val scalars = observedDefinitions.filterIsInstance() .filter { diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt index 160c220b..9c8dfedf 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt @@ -104,8 +104,8 @@ class SchemaParser internal constructor( val enums = enumDefinitions.map { createEnumObject(it) } // Assign type resolver to interfaces now that we know all of the object types - interfaces.forEach { codeRegistryBuilder.typeResolver(it, InterfaceTypeResolver(dictionary.inverse(), it)) } - unions.forEach { codeRegistryBuilder.typeResolver(it, UnionTypeResolver(dictionary.inverse(), it)) } + interfaces.forEach { codeRegistryBuilder.typeResolver(it, InterfaceTypeResolver(dictionary.typeByClass, it)) } + unions.forEach { codeRegistryBuilder.typeResolver(it, UnionTypeResolver(dictionary.typeByClass, it)) } // Find query type and mutation/subscription type (if mutation/subscription type exists) val queryName = rootInfo.getQueryName() @@ -240,7 +240,7 @@ class SchemaParser internal constructor( private fun createEnumObject(definition: EnumTypeDefinition): GraphQLEnumType { val name = definition.name // enums only used by directives can do without a class, see SchemaClassScanner.handleDirectiveEnum - val enumClass = dictionary[definition]?.unwrap() + val enumClass = dictionary.classByType[definition]?.unwrap() if (enumClass == null && definition !in directiveOnlyEnums) throw SchemaError("Expected enum with name '$name' but found none!") if (enumClass != null && !enumClass.isEnum) throw SchemaError("Type '$name' is declared as an enum in the GraphQL schema but is not a Java enum!") diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParserBuilder.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParserBuilder.kt index 18a0ed38..df8f7c4a 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParserBuilder.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParserBuilder.kt @@ -165,7 +165,7 @@ class SchemaParserBuilder { val definitions = appendDynamicDefinitions(parseDefinitions()) val customScalars = scalars.associateBy { it.name } - return SchemaClassScanner(dictionary.getDictionary(), definitions, resolvers, customScalars, options) + return SchemaClassScanner(dictionary.toMap(), definitions, resolvers, customScalars, options) .scanForClasses() } diff --git a/src/main/kotlin/graphql/kickstart/tools/SchemaParserDictionary.kt b/src/main/kotlin/graphql/kickstart/tools/SchemaParserDictionary.kt index 56224fa1..1012dc0e 100644 --- a/src/main/kotlin/graphql/kickstart/tools/SchemaParserDictionary.kt +++ b/src/main/kotlin/graphql/kickstart/tools/SchemaParserDictionary.kt @@ -1,17 +1,25 @@ package graphql.kickstart.tools -import graphql.kickstart.tools.util.BiMap import kotlin.reflect.KClass class SchemaParserDictionary { private val dictionary = linkedMapOf>() + /** + * Returns a copy of the entries added so far, in the order they were added. Several names may share a class. + */ + fun toMap(): Map> = dictionary.toMap() + + // TODO remove in 15.0.0 (#900) /** * 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> = BiMap.unmodifiableCopyOf(dictionary) + @Deprecated("Use toMap() instead", ReplaceWith("toMap()")) + @Suppress("DEPRECATION") + fun getDictionary(): graphql.kickstart.tools.util.BiMap> = + graphql.kickstart.tools.util.BiMap.unmodifiableCopyOf(dictionary) /** * Add arbitrary classes to the parser's dictionary, overriding the generated type name. diff --git a/src/main/kotlin/graphql/kickstart/tools/util/BiMap.java b/src/main/kotlin/graphql/kickstart/tools/util/BiMap.java index e1ec9603..70343ba7 100644 --- a/src/main/kotlin/graphql/kickstart/tools/util/BiMap.java +++ b/src/main/kotlin/graphql/kickstart/tools/util/BiMap.java @@ -8,6 +8,11 @@ import static java.util.Collections.unmodifiableMap; +// TODO remove in 15.0.0 (#900) +/** + * @deprecated Use a plain {@link java.util.Map}. + */ +@Deprecated @SuppressWarnings("SuspiciousMethodCalls") public class BiMap implements Map { diff --git a/src/test/kotlin/graphql/kickstart/tools/SchemaClassScannerTest.kt b/src/test/kotlin/graphql/kickstart/tools/SchemaClassScannerTest.kt index 563a1556..59cd7c61 100644 --- a/src/test/kotlin/graphql/kickstart/tools/SchemaClassScannerTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/SchemaClassScannerTest.kt @@ -7,6 +7,7 @@ import graphql.Scalars import graphql.execution.CoercedVariables import graphql.language.Value import graphql.schema.* +import org.junit.Assert.assertThrows import org.junit.Test import java.util.* import java.util.concurrent.CompletableFuture @@ -561,6 +562,136 @@ class SchemaClassScannerTest { var name: String? = null } + @Test + fun `scanner allows input object types to share a class`() { + val schema = SchemaParser.newParser() + .resolvers(object : GraphQLQueryResolver { + fun test(a: Pojo, b: Pojo): String = "${a.name} ${b.name}" + }) + .schemaString( + """ + type Query { + test(a: AInput, b: BInput): String + } + + input AInput { + name: String + } + + input BInput { + name: String + } + """) + .build() + .makeExecutableSchema() + .assertEveryFieldResolves() + + val data = assertNoGraphQlErrors(GraphQL.newGraphQL(schema).build()) { + """ + query { + test(a: { name: "x" }, b: { name: "y" }) + } + """ + } + + assertEquals(data["test"], "x y") + } + + @Test + fun `scanner rejects two object types that share a class`() { + val error = assertThrows(SchemaClassScannerError::class.java) { + SchemaParser.newParser() + .resolvers(object : GraphQLQueryResolver { + fun a(): SharedObject? = null + fun b(): SharedObject? = null + }) + .schemaString( + """ + type Query { + a: A + b: B + } + + type A { + id: Int + } + + type B { + id: Int + } + """) + .build() + } + + assertEquals(error.message, "Class '${SharedObject::class.java.name}' is used for types 'A', 'B', " + + "but only input object types can share a class!") + } + + @Test + fun `scanner rejects an interface type and an object type that share a class`() { + val error = assertThrows(SchemaClassScannerError::class.java) { + SchemaParser.newParser() + .resolvers(object : GraphQLQueryResolver { + fun node(): SharedObject? = null + }) + .schemaString( + """ + type Query { + node: Node + } + + interface Node { + id: Int + } + + type Item implements Node { + id: Int + } + """) + .dictionary("Item", SharedObject::class) + .build() + } + + assertEquals(error.message, "Class '${SharedObject::class.java.name}' is used for types 'Node', 'Item', " + + "but only input object types can share a class!") + } + + @Test + fun `scanner rejects two enum types that share a class`() { + val error = assertThrows(SchemaClassScannerError::class.java) { + SchemaParser.newParser() + .resolvers(object : GraphQLQueryResolver { + fun a(): SharedEnum? = null + fun b(): SharedEnum? = null + }) + .schemaString( + """ + type Query { + a: EnumA + b: EnumB + } + + enum EnumA { + X + } + + enum EnumB { + X + } + """) + .build() + } + + assertEquals(error.message, "Class '${SharedEnum::class.java.name}' is used for types 'EnumA', 'EnumB', " + + "but only input object types can share a class!") + } + + class SharedObject { + var id: Int? = null + } + + enum class SharedEnum { X } + @Test fun `scanner should handle nested types in input types`() { val schema = SchemaParser.newParser() diff --git a/src/test/kotlin/graphql/kickstart/tools/SchemaParserDictionaryTest.kt b/src/test/kotlin/graphql/kickstart/tools/SchemaParserDictionaryTest.kt index 9a5f70c1..69e66021 100644 --- a/src/test/kotlin/graphql/kickstart/tools/SchemaParserDictionaryTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/SchemaParserDictionaryTest.kt @@ -67,7 +67,7 @@ class SchemaParserDictionaryTest { .add("AInput", Map::class) .add("BInput", Map::class) - assertEquals(dictionary.getDictionary().toMap(), mapOf("AInput" to Map::class.java, "BInput" to Map::class.java)) + assertEquals(dictionary.toMap(), mapOf("AInput" to Map::class.java, "BInput" to Map::class.java)) val schema = SchemaParser.newParser() .schemaString( @@ -90,7 +90,7 @@ class SchemaParserDictionaryTest { .resolvers(object : GraphQLQueryResolver { fun test(): String? = null }) - .dictionary(dictionary.getDictionary()) + .dictionary(dictionary.toMap()) .build() .makeExecutableSchema() .assertEveryFieldResolves() @@ -100,6 +100,30 @@ class SchemaParserDictionaryTest { assertEquals(field.getAppliedDirective("b").getArgument("arg")?.getValue>(), mapOf("y" to "b")) } + @Test + fun `toMap returns the entries in insertion order and keeps every name of a shared class`() { + val dictionary = SchemaParserDictionary() + .add("B", Map::class) + .add("A", String::class) + .add("C", Map::class) + + assertEquals(dictionary.toMap().entries.map { it.key to it.value }, listOf("B" to Map::class.java, "A" to String::class.java, "C" to Map::class.java)) + } + + @Test + @Suppress("DEPRECATION") + fun `getDictionary keeps working and only maps a shared class back to its first name`() { + val dictionary = SchemaParserDictionary() + .add("B", Map::class) + .add("A", String::class) + .add("C", Map::class) + + val deprecated = dictionary.getDictionary() + + assertEquals(deprecated.toMap(), dictionary.toMap()) + assertEquals(deprecated.inverse().toMap(), mapOf, String>(Map::class.java to "B", String::class.java to "A")) + } + @Test fun `input type and object type can share a dictionary class`() { val schema = SchemaParser.newParser() diff --git a/src/test/kotlin/graphql/kickstart/tools/util/BiMapTest.kt b/src/test/kotlin/graphql/kickstart/tools/util/BiMapTest.kt index fdc56f30..8cbd3fe4 100644 --- a/src/test/kotlin/graphql/kickstart/tools/util/BiMapTest.kt +++ b/src/test/kotlin/graphql/kickstart/tools/util/BiMapTest.kt @@ -1,3 +1,5 @@ +@file:Suppress("DEPRECATION") + package graphql.kickstart.tools.util import graphql.kickstart.tools.assertEquals