Merge pull request #851 from graphql-java-kickstart/bugfix/465

Let input types share a dictionary class

Author: oryan-block

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

@@ -16,7 +16,7 @@ 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,
@@ -59,6 +59,17 @@ internal class SchemaClassScanner(
             }
         }
 
+        // Input object types never go into the type => class bimap that interface and union type resolution use, so several of
+        // them can share a class, e.g. ObjectNode
+        initialDictionary
+            .filterKeys { definitionsByName[it] !is InputObjectTypeDefinition }
+            .entries.groupBy({ it.value }, { it.key })
+            .forEach { (clazz, names) ->
+                if (names.size > 1) {
+                    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.")
         }

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;

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

@@ -0,0 +1,190 @@
+package graphql.kickstart.tools
+
+import com.fasterxml.jackson.databind.node.ObjectNode
+import graphql.GraphQL
+import org.junit.Assert.assertThrows
+import org.junit.Test
+
+class SchemaParserDictionaryTest {
+
+    @Test
+    fun `input types can share a dictionary class`() {
+        val schema = SchemaParser.newParser()
+            .schemaString(
+                """
+                type Query {
+                    create(input: EntityCreateInput!): String
+                    update(input: EntityUpdateInput!): String
+                }
+
+                input EntityCreateInput {
+                    name: String
+                    a: NestedA
+                }
+
+                input EntityUpdateInput {
+                    id: ID!
+                    b: [NestedB!]
+                }
+
+                input NestedA {
+                    x: Int
+                }
+
+                input NestedB {
+                    y: Int
+                }
+                """)
+            .resolvers(object : GraphQLQueryResolver {
+                fun create(input: ObjectNode): String = input.toString()
+                fun update(input: ObjectNode): String = input.toString()
+            })
+            .dictionary("EntityCreateInput", ObjectNode::class.java)
+            .dictionary("EntityUpdateInput", ObjectNode::class.java)
+            // ObjectNode has no properties, so the classes of the nested inputs come from the dictionary
+            .dictionary("NestedA", ObjectNode::class.java)
+            .dictionary("NestedB", ObjectNode::class.java)
+            .build()
+            .makeExecutableSchema()
+        val gql = GraphQL.newGraphQL(schema).build()
+
+        val data = assertNoGraphQlErrors(gql) {
+            """
+            query {
+                create(input: { name: "n", a: { x: 1 } })
+                update(input: { id: "2", b: [{ y: 3 }] })
+            }
+            """
+        }
+
+        assertEquals(data["create"], """{"name":"n","a":{"x":1}}""")
+        assertEquals(data["update"], """{"id":"2","b":[{"y":3}]}""")
+    }
+
+    @Test
+    fun `directive input types sharing a class can be passed to the builder from a SchemaParserDictionary`() {
+        val dictionary = SchemaParserDictionary()
+            .add("AInput", Map::class)
+            .add("BInput", Map::class)
+
+        assertEquals(dictionary.getDictionary().toMap(), mapOf("AInput" to Map::class.java, "BInput" to Map::class.java))
+
+        // graphql-spring-boot reads a SchemaParserDictionary bean this way
+        val schema = SchemaParser.newParser()
+            .schemaString(
+                """
+                directive @a(arg: AInput) on FIELD_DEFINITION
+                directive @b(arg: BInput) on FIELD_DEFINITION
+
+                type Query {
+                    test: String @a(arg: { x: 1 }) @b(arg: { y: "b" })
+                }
+
+                input AInput {
+                    x: Int
+                }
+
+                input BInput {
+                    y: String
+                }
+                """)
+            .resolvers(object : GraphQLQueryResolver {
+                fun test(): String? = null
+            })
+            .dictionary(dictionary.getDictionary())
+            .build()
+            .makeExecutableSchema()
+
+        val field = schema.queryType.getFieldDefinition("test")
+        assertEquals(field.getAppliedDirective("a").getArgument("arg")?.getValue<Map<*, *>>(), mapOf("x" to 1))
+        assertEquals(field.getAppliedDirective("b").getArgument("arg")?.getValue<Map<*, *>>(), mapOf("y" to "b"))
+    }
+
+    @Test
+    fun `input type and object type can share a dictionary class`() {
+        // DogInput is only used by a directive, so its class can only come from the dictionary
+        val schema = SchemaParser.newParser()
+            .schemaString(
+                """
+                directive @d(arg: DogInput) on FIELD_DEFINITION
+
+                type Query {
+                    dog: Dog @d(arg: { name: "z" })
+                }
+
+                interface Dog {
+                    name: String
+                }
+
+                type LabradorRetriever implements Dog {
+                    name: String
+                }
+
+                input DogInput {
+                    name: String
+                }
+                """)
+            .resolvers(object : GraphQLQueryResolver {
+                fun dog(): Dog = LabradorRetriever(name = "a", color = "black", fluffiness = 1, speed = 1f)
+            })
+            .dictionary("LabradorRetriever", LabradorRetriever::class)
+            .dictionary("DogInput", LabradorRetriever::class)
+            .build()
+            .makeExecutableSchema()
+
+        val field = schema.queryType.getFieldDefinition("dog")
+        assertEquals(field.getAppliedDirective("d").getArgument("arg")?.getValue<Map<*, *>>(), mapOf("name" to "z"))
+
+        val gql = GraphQL.newGraphQL(schema).build()
+        val data = assertNoGraphQlErrors(gql) {
+            """
+            query {
+                dog {
+                    __typename
+                    name
+                }
+            }
+            """
+        }
+
+        assertEquals(data["dog"], mapOf("__typename" to "LabradorRetriever", "name" to "a"))
+    }
+
+    @Test
+    fun `object types can't share a dictionary class`() {
+        val error = assertThrows(SchemaClassScannerError::class.java) {
+            SchemaParser.newParser()
+                .schemaString(
+                    """
+                    type Query {
+                        a: A
+                        b: B
+                    }
+
+                    type A {
+                        id: Int
+                    }
+
+                    type B {
+                        id: Int
+                    }
+
+                    input AInput {
+                        id: Int
+                    }
+                    """)
+                .resolvers(object : GraphQLQueryResolver {
+                    fun a(): ThirdItem? = null
+                    fun b(): ThirdItem? = null
+                })
+                .dictionary("A", ThirdItem::class)
+                .dictionary("AInput", ThirdItem::class)
+                .dictionary("B", ThirdItem::class)
+                .build()
+        }
+
+        // the input type sharing the class isn't part of the conflict
+        assertEquals(error.message, "Class in supplied dictionary '${ThirdItem::class.java.name}' specified type names 'A', 'B', " +
+            "but only input object types can share a class!")
+    }
+}

src/test/kotlin/graphql/kickstart/tools/util/BiMapTest.kt

@@ -67,4 +67,19 @@ class BiMapTest {
         val expected = setOf(1, 2, 3)
         assertEquals(expected, values)
     }
+
+    @Test
+    fun `unmodifiable copy should keep keys with the same value`() {
+        val copy = BiMap.unmodifiableCopyOf(linkedMapOf("foo" to 1, "bar" to 2, "baz" to 1))
+
+        assertEquals(mapOf("foo" to 1, "bar" to 2, "baz" to 1), copy.toMap())
+        assertEquals(listOf("foo", "bar", "baz"), copy.keys.toList())
+        assertEquals(setOf(1, 2), copy.values)
+        assertEquals(mapOf(1 to "foo", 2 to "bar"), copy.inverse().toMap())
+    }
+
+    @Test(expected = UnsupportedOperationException::class)
+    fun `unmodifiable copy should not permit changes`() {
+        BiMap.unmodifiableCopyOf(mapOf("foo" to 1))["bar"] = 2
+    }
 }