fix: properly cache GraphQL object/interfaces/union types (#291)

`graphql-java` requires all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types to have unique names within a schema. While generating the schemas based on the reflections we have to ensure that each one of those objects is build only once. Since underlying objects do not implement hashcode it leads to problems if we attempt to update the fields through some hooks (e.g. use `newBuilder(existingObject)`). In order to make sure we only build those objects once we implemented basic caching logic in our builders - when we start building the target object we put it under construction and after it is build we add it to the cache and mark it completed.

Unfortunately, existing logic was flawed - all objects were being put under construction but only `GraphQLObjectType`s generated when processing their `GraphQLInterfaceType` were marked as completed and added to the cache. This change fixes the caching logic to properly complete all `GraphQLObjectType`, `GraphQLInterfaceType` and `GraphQLUnionType` types and add them to the underlying cache.

Author: dariuszkuc

graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/SchemaGenerator.kt

@@ -58,8 +58,8 @@ internal class SchemaGenerator(internal val config: SchemaGeneratorConfig) {
         builder.mutation(mutationBuilder.getMutationObject(mutations))
         builder.subscription(subscriptionBuilder.getSubscriptionObject(subscriptions))
 
-        // add interface/union implementations
-        state.getValidAdditionalTypes().forEach {
+        // add interface implementations
+        state.additionalTypes.forEach {
             builder.additionalType(it)
         }
 

graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/state/SchemaGeneratorState.kt

@@ -6,13 +6,12 @@ import graphql.schema.GraphQLDirective
 import graphql.schema.GraphQLType
 import java.util.concurrent.ConcurrentHashMap
 
+@Suppress("UseDataClass")
 internal class SchemaGeneratorState(supportedPackages: List<String>) {
     val cache = TypesCache(supportedPackages)
     val additionalTypes = mutableSetOf<GraphQLType>()
     val directives = ConcurrentHashMap<String, GraphQLDirective>()
 
-    fun getValidAdditionalTypes(): List<GraphQLType> = additionalTypes.filter { cache.doesNotContainGraphQLType(it) }
-
     init {
         // NOTE: @include and @defer query directives are added by graphql-java by default
         // adding them explicitly here to keep it consistent with missing deprecated directive

graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/state/TypesCache.kt

@@ -10,6 +10,7 @@ import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeReference
 import kotlin.reflect.KClass
 import kotlin.reflect.KType
+import kotlin.reflect.full.createType
 import kotlin.reflect.full.isSubclassOf
 
 internal class TypesCache(private val supportedPackages: List<String>) {
@@ -68,16 +69,19 @@ internal class TypesCache(private val supportedPackages: List<String>) {
 
     private fun isTypeNotSupported(type: KType): Boolean = supportedPackages.none { type.qualifiedName.startsWith(it) }
 
-    private fun putTypeUnderConstruction(kClass: KClass<*>) = typeUnderConstruction.add(kClass)
-
-    fun removeTypeUnderConstruction(kClass: KClass<*>) = typeUnderConstruction.remove(kClass)
-
     fun buildIfNotUnderConstruction(kClass: KClass<*>, build: (KClass<*>) -> GraphQLType): GraphQLType {
-        return if (typeUnderConstruction.contains(kClass)) {
-            GraphQLTypeReference.typeRef(kClass.getSimpleName())
-        } else {
-            putTypeUnderConstruction(kClass)
-            build(kClass)
+        val cachedType = cache[kClass.getSimpleName()]
+        return when {
+            cachedType != null -> cachedType.graphQLType
+            typeUnderConstruction.contains(kClass) -> GraphQLTypeReference.typeRef(kClass.getSimpleName())
+            else -> {
+                typeUnderConstruction.add(kClass)
+                val newType = build(kClass)
+                val key = TypesCacheKey(kClass.createType(), false)
+                put(key, KGraphQLType(kClass, newType))
+                typeUnderConstruction.remove(kClass)
+                newType
+            }
         }
     }
 }

graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InterfaceBuilder.kt

@@ -33,13 +33,12 @@ internal class InterfaceBuilder(generator: SchemaGenerator) : TypeBuilder(genera
 
             val interfaceType = builder.build()
             val implementations = subTypeMapper.getSubTypesOf(kClass)
-            implementations.forEach {
-                val objectType = generator.objectType(it.kotlin, interfaceType)
+            implementations.forEach { implementation ->
+                val objectType = generator.objectType(implementation.kotlin, interfaceType)
 
                 // Only update the state if the object is fully constructed and not a reference
                 if (objectType !is GraphQLTypeReference) {
                     state.additionalTypes.add(objectType)
-                    state.cache.removeTypeUnderConstruction(it.kotlin)
                 }
             }
 

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/InterfaceBuilderTest.kt

@@ -6,6 +6,9 @@ import com.expedia.graphql.test.utils.SimpleDirective
 import graphql.schema.GraphQLInterfaceType
 import org.junit.jupiter.api.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlin.test.assertNotNull
+import kotlin.test.assertTrue
 
 internal class InterfaceBuilderTest : TypeTestHelper() {
 
@@ -48,4 +51,17 @@ internal class InterfaceBuilderTest : TypeTestHelper() {
         assertEquals(1, result?.directives?.size)
         assertEquals("simpleDirective", result?.directives?.first()?.name)
     }
+
+    @Test
+    fun `verify interface is build only once`() {
+        val cache = generator.state.cache
+        assertTrue(cache.doesNotContain(HappyInterface::class))
+
+        val first = builder.interfaceType(HappyInterface::class) as? GraphQLInterfaceType
+        assertNotNull(first)
+        assertFalse(cache.doesNotContain(HappyInterface::class))
+        val second = builder.interfaceType(HappyInterface::class) as? GraphQLInterfaceType
+        assertNotNull(second)
+        assertEquals(first.hashCode(), second.hashCode())
+    }
 }

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/ObjectBuilderTest.kt

@@ -9,7 +9,9 @@ import graphql.schema.GraphQLNonNull
 import graphql.schema.GraphQLObjectType
 import org.junit.jupiter.api.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
+import kotlin.test.assertTrue
 
 @Suppress("Detekt.UnusedPrivateClass")
 internal class ObjectBuilderTest : TypeTestHelper() {
@@ -67,4 +69,17 @@ internal class ObjectBuilderTest : TypeTestHelper() {
             setOf(Introspection.DirectiveLocation.OBJECT)
         )
     }
+
+    @Test
+    fun `verify object is build only once`() {
+        val cache = generator.state.cache
+        assertTrue(cache.doesNotContain(BeHappy::class))
+
+        val first = builder.objectType(BeHappy::class) as? GraphQLObjectType
+        assertNotNull(first)
+        assertFalse(cache.doesNotContain(BeHappy::class))
+        val second = builder.objectType(BeHappy::class) as? GraphQLObjectType
+        assertNotNull(second)
+        assertEquals(first.hashCode(), second.hashCode())
+    }
 }

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/UnionBuilderTest.kt

@@ -7,7 +7,9 @@ import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLUnionType
 import org.junit.jupiter.api.Test
 import kotlin.test.assertEquals
+import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
+import kotlin.test.assertTrue
 
 @Suppress("Detekt.UnusedPrivateClass")
 internal class UnionBuilderTest : TypeTestHelper() {
@@ -69,4 +71,17 @@ internal class UnionBuilderTest : TypeTestHelper() {
         assertEquals(1, result.directives.size)
         assertEquals("simpleDirective", result.directives.first().name)
     }
+
+    @Test
+    fun `verify union is build only once`() {
+        val cache = generator.state.cache
+        assertTrue(cache.doesNotContain(Cake::class))
+
+        val first = builder.unionType(Cake::class) as? GraphQLUnionType
+        assertNotNull(first)
+        assertFalse(cache.doesNotContain(Cake::class))
+        val second = builder.unionType(Cake::class) as? GraphQLUnionType
+        assertNotNull(second)
+        assertEquals(first.hashCode(), second.hashCode())
+    }
 }