fix: properly cache GraphQL object/interfaces/union types

graphql-java requires all GraphQL object/interfaces/union types in the graph to have unique names but underlying objects do not implement hashcode and equals. This means that if we are generating schemas based on the reflections we had to ensure that we would only build objects once - we implemented caching logic in the builders. Unfortunately existing logic was putting all objects/interfaces/unions as "under construction" but it only marked objects as completed and only if they were generated from interface builder.

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())
+    }
 }