fix: *generateGraphQLType hooks are now called on interfaces

Fixes #294

Interfaces now go through the internal TypeBuilder method 'graphQLTypeOf' instead of 'objectFromReflection'. This means we can make 'objectFromReflection' private. But the type may be (if not always) wrapped in a GraphQLNonNull which means we have to cast it back to the interface type before adding it to the ObjectBuilder.

Author: smyrick

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

@@ -32,7 +32,7 @@ internal open class TypeBuilder constructor(protected val generator: SchemaGener
         return config.hooks.didGenerateGraphQLType(type, typeWithNullability)
     }
 
-    internal fun objectFromReflection(type: KType, inputType: Boolean): GraphQLType {
+    private fun objectFromReflection(type: KType, inputType: Boolean): GraphQLType {
         val cacheKey = TypesCacheKey(type, inputType)
         val cachedType = state.cache.get(cacheKey)
 

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

@@ -39,3 +39,18 @@ internal fun GraphQLDirectiveContainer.getAllDirectives(): List<GraphQLDirective
 
     return mutableList
 }
+
+/**
+ * If the GraphQLType is of type T or GraphQLNonNull<T>, return the class as T.
+ * Otherwise return null.
+ */
+internal inline fun <reified T : GraphQLType> GraphQLType.unwrapNonNullCast(): T? {
+    return when (this) {
+        is T -> this
+        is GraphQLNonNull -> when (val wrappedType = this.wrappedType) {
+            is T -> wrappedType
+            else -> null
+        }
+        else -> null
+    }
+}

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

@@ -39,8 +39,8 @@ internal class TypesCache(private val supportedPackages: List<String>) {
         val cacheKey = getCacheKeyString(key)
 
         if (cacheKey != null) {
-            typeUnderConstruction.remove(kGraphQLType.kClass)
-            return cache.put(cacheKey, kGraphQLType)
+            cache[cacheKey] = kGraphQLType
+            return kGraphQLType
         }
 
         return null

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

@@ -12,6 +12,7 @@ import graphql.schema.GraphQLInterfaceType
 import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeReference
 import kotlin.reflect.KClass
+import kotlin.reflect.full.createType
 
 internal class InterfaceBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
     internal fun interfaceType(kClass: KClass<*>): GraphQLType {
@@ -31,15 +32,17 @@ internal class InterfaceBuilder(generator: SchemaGenerator) : TypeBuilder(genera
             kClass.getValidFunctions(config.hooks)
                 .forEach { builder.field(generator.function(it, kClass.getSimpleName(), abstract = true)) }
 
-            val interfaceType = builder.build()
             val implementations = subTypeMapper.getSubTypesOf(kClass)
             implementations.forEach { implementation ->
-                val objectType = generator.objectType(implementation.kotlin, interfaceType)
+                val kType = implementation.kotlin.createType()
+                val objectType = graphQLTypeOf(kType)
                 // skip objects currently under construction
                 if (objectType !is GraphQLTypeReference) {
                     state.additionalTypes.add(objectType)
                 }
             }
+
+            val interfaceType = builder.build()
             codeRegistry.typeResolver(interfaceType) { env: TypeResolutionEnvironment -> env.schema.getObjectType(env.getObject<Any>().javaClass.kotlin.getSimpleName()) }
             config.hooks.onRewireGraphQLType(interfaceType).safeCast()
         }

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

@@ -8,6 +8,7 @@ import com.expedia.graphql.generator.extensions.getValidFunctions
 import com.expedia.graphql.generator.extensions.getValidProperties
 import com.expedia.graphql.generator.extensions.getValidSuperclasses
 import com.expedia.graphql.generator.extensions.safeCast
+import com.expedia.graphql.generator.extensions.unwrapNonNullCast
 import graphql.schema.GraphQLInterfaceType
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLType
@@ -34,10 +35,14 @@ internal class ObjectBuilder(generator: SchemaGenerator) : TypeBuilder(generator
                 builder.withInterface(GraphQLTypeReference(interfaceType.name))
             } else {
                 kClass.getValidSuperclasses(config.hooks)
-                    .map { objectFromReflection(it.createType(), false) }
+                    .map { graphQLTypeOf(type = it.createType(), inputType = false, annotatedAsID = false) }
                     .forEach {
-                        when (it) {
-                            is GraphQLInterfaceType -> builder.withInterface(it)
+                        val possibleTypeReferece = it.unwrapNonNullCast<GraphQLTypeReference>()
+                        val possibleInterface = it.unwrapNonNullCast<GraphQLInterfaceType>()
+
+                        when {
+                            possibleTypeReferece != null -> builder.withInterface(possibleTypeReferece)
+                            possibleInterface != null -> builder.withInterface(possibleInterface)
                         }
                     }
             }

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

@@ -4,7 +4,7 @@ import com.expedia.graphql.generator.SchemaGenerator
 import com.expedia.graphql.generator.TypeBuilder
 import com.expedia.graphql.generator.extensions.getGraphQLDescription
 import com.expedia.graphql.generator.extensions.getSimpleName
-import com.expedia.graphql.generator.state.TypesCacheKey
+import com.expedia.graphql.generator.extensions.unwrapNonNullCast
 import graphql.TypeResolutionEnvironment
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLType
@@ -25,15 +25,20 @@ internal class UnionBuilder(generator: SchemaGenerator) : TypeBuilder(generator)
             }
 
             val implementations = subTypeMapper.getSubTypesOf(kClass)
-            implementations.forEach {
-                val objectType = state.cache.get(TypesCacheKey(it.kotlin.createType(), false))
-                    ?: generator.objectType(it.kotlin)
+            implementations.map {
+                val kType = it.kotlin.createType()
+                graphQLTypeOf(kType, inputType = false, annotatedAsID = false)
+            }
+            .forEach {
+                val possibleTypeReferece = it.unwrapNonNullCast<GraphQLTypeReference>()
+                val possibleObjectType = it.unwrapNonNullCast<GraphQLObjectType>()
 
-                when (objectType) {
-                    is GraphQLTypeReference -> builder.possibleType(objectType)
-                    is GraphQLObjectType -> builder.possibleType(objectType)
+                when {
+                    possibleTypeReferece != null -> builder.possibleType(possibleTypeReferece)
+                    possibleObjectType != null -> builder.possibleType(possibleObjectType)
                 }
             }
+
             val unionType = builder.build()
             codeRegistry.typeResolver(unionType) { env: TypeResolutionEnvironment -> env.schema.getObjectType(env.getObject<Any>().javaClass.kotlin.getSimpleName()) }
             config.hooks.onRewireGraphQLType(unionType)

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/extensions/GraphQLExtensionsTest.kt

@@ -9,6 +9,7 @@ import graphql.schema.GraphQLFieldDefinition
 import graphql.schema.GraphQLInterfaceType
 import graphql.schema.GraphQLNonNull
 import graphql.schema.GraphQLObjectType
+import graphql.schema.GraphQLScalarType
 import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeVisitor
 import graphql.util.TraversalControl
@@ -20,6 +21,8 @@ import kotlin.reflect.KType
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 import kotlin.test.assertFalse
+import kotlin.test.assertNotNull
+import kotlin.test.assertNull
 import kotlin.test.assertTrue
 
 @Suppress("Detekt.LargeClass")
@@ -127,4 +130,34 @@ internal class GraphQLExtensionsTest {
             type.safeCast<GraphQLInterfaceType>()
         }
     }
+
+    @Test
+    fun `unwrapNonNullCast returns the type on valid types`() {
+        val objectType: GraphQLType = GraphQLObjectType.newObject().name("name").description("description").build()
+        val unwrappedType = objectType.unwrapNonNullCast<GraphQLObjectType>()
+        assertNotNull(unwrappedType)
+    }
+
+    @Test
+    fun `unwrapNonNullCast returns the wrapped type on GraphQLNonNull types`() {
+        val objectType: GraphQLType = GraphQLObjectType.newObject().name("name").description("description").build()
+        val nonNull = GraphQLNonNull.nonNull(objectType)
+        val unwrappedType = nonNull.unwrapNonNullCast<GraphQLObjectType>()
+        assertNotNull(unwrappedType)
+    }
+
+    @Test
+    fun `unwrapNonNullCast returns null on mismatched types`() {
+        val objectType: GraphQLType = GraphQLObjectType.newObject().name("name").description("description").build()
+        val unwrappedType = objectType.unwrapNonNullCast<GraphQLScalarType>()
+        assertNull(unwrappedType)
+    }
+
+    @Test
+    fun `unwrapNonNullCast returns null on mismatched GraphQLNonNull types`() {
+        val objectType: GraphQLType = GraphQLObjectType.newObject().name("name").description("description").build()
+        val nonNull = GraphQLNonNull.nonNull(objectType)
+        val unwrappedType = nonNull.unwrapNonNullCast<GraphQLScalarType>()
+        assertNull(unwrappedType)
+    }
 }

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/hooks/SchemaGeneratorHooksTest.kt

@@ -95,13 +95,12 @@ class SchemaGeneratorHooksTest {
 
         val hooks = MockSchemaGeneratorHooks()
         toSchema(
-            queries = listOf(TopLevelObject(TestQuery())),
+            queries = listOf(TopLevelObject(TestInterfaceQuery())),
             config = getTestSchemaConfigWithHooks(hooks)
         )
         assertTrue(hooks.seenTypes.contains(RandomData::class.createType()))
         assertTrue(hooks.seenTypes.contains(SomeData::class.createType()))
-        // TODO bug: didGenerateGraphQLType hook is not applied on the object types build from the interface
-//        assertTrue(hooks.seenTypes.contains(OtherData::class.createType()))
+        assertTrue(hooks.seenTypes.contains(OtherData::class.createType()))
     }
 
     @Test
@@ -199,6 +198,9 @@ class SchemaGeneratorHooksTest {
 
     class TestQuery {
         fun query(): SomeData = SomeData("someData", 0)
+    }
+
+    class TestInterfaceQuery {
         fun randomQuery(): RandomData = if (Random.nextBoolean()) {
             SomeData("random", 1)
         } else {

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/test/integration/RecursiveInterfaceTest.kt

@@ -28,29 +28,29 @@ class RecursiveInterfaceTest {
 }
 
 class RecursiveInterfaceQuery {
-    fun getRoot() = RecursiveInterfaceA()
+    fun getRoot() = RecursiveClassA()
 }
 
 class InterfaceWithSelfFieldQuery {
     fun getInterface() = InterfaceWithSelfFieldB()
 }
 
-interface SomeInterface {
+interface SomeInterfaceWithId {
     val id: String
 }
 
 interface InterfaceWithSelfField {
     val parent: InterfaceWithSelfField?
 }
 
-class RecursiveInterfaceA : SomeInterface {
+class RecursiveClassA : SomeInterfaceWithId {
     override val id = "A"
-    fun getB() = RecursiveInterfaceB()
+    fun getB() = RecursiveClassB()
 }
 
-class RecursiveInterfaceB : SomeInterface {
+class RecursiveClassB : SomeInterfaceWithId {
     override val id = "B"
-    fun getA() = RecursiveInterfaceA()
+    fun getA() = RecursiveClassA()
 }
 
 class InterfaceWithSelfFieldA : InterfaceWithSelfField {