fix: *generateGraphQLType hooks are now called on interfaces (#303)

smyrick · dariuszkuc · commit 5812fa04ab18 · 2019-08-15T22:19:24.000-05:00
* 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.
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/SchemaGenerator.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/SchemaGenerator.kt
@@ -18,7 +18,6 @@ import com.expedia.graphql.generator.types.SubscriptionBuilder
 import com.expedia.graphql.generator.types.UnionBuilder
 import graphql.schema.GraphQLCodeRegistry
 import graphql.schema.GraphQLDirective
-import graphql.schema.GraphQLInterfaceType
 import graphql.schema.GraphQLSchema
 import java.lang.reflect.Field
 import kotlin.reflect.KAnnotatedElement
@@ -76,8 +75,8 @@ open class SchemaGenerator(val config: SchemaGeneratorConfig) {
     open fun listType(type: KType, inputType: Boolean) =
         listTypeBuilder.listType(type, inputType)
 
-    open fun objectType(kClass: KClass<*>, interfaceType: GraphQLInterfaceType? = null) =
-        objectTypeBuilder.objectType(kClass, interfaceType)
+    open fun objectType(kClass: KClass<*>) =
+        objectTypeBuilder.objectType(kClass)
 
     open fun inputObjectType(kClass: KClass<*>) =
         inputObjectTypeBuilder.inputObjectType(kClass)
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/SubTypeMapper.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/SubTypeMapper.kt
@@ -7,5 +7,5 @@ internal class SubTypeMapper(supportedPackages: List<String>) {
 
     private val reflections = Reflections(supportedPackages)
 
-    fun getSubTypesOf(kclass: KClass<*>): List<Class<*>> = reflections.getSubTypesOf(kclass.java).filterNot { it.kotlin.isAbstract }
+    fun getSubTypesOf(kclass: KClass<*>): List<KClass<*>> = reflections.getSubTypesOf(kclass.java).filterNot { it.kotlin.isAbstract }.map { it.kotlin }
 }
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/TypeBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/TypeBuilder.kt
@@ -13,6 +13,7 @@ import com.expedia.graphql.generator.state.TypesCacheKey
 import graphql.schema.GraphQLCodeRegistry
 import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeReference
+import graphql.schema.GraphQLTypeUtil
 import kotlin.reflect.KClass
 import kotlin.reflect.KType
 
@@ -28,11 +29,17 @@ internal open class TypeBuilder constructor(protected val generator: SchemaGener
             ?: generator.scalarType(type, annotatedAsID)
             ?: objectFromReflection(type, inputType)
 
-        val typeWithNullability = graphQLType.wrapInNonNull(type)
-        return config.hooks.didGenerateGraphQLType(type, typeWithNullability)
+        // Do not call the hook on GraphQLTypeReference as we have not generated the type yet
+        val unwrappedType = GraphQLTypeUtil.unwrapNonNull(graphQLType)
+        if (unwrappedType !is GraphQLTypeReference) {
+            val typeWithNullability = graphQLType.wrapInNonNull(type)
+            return config.hooks.didGenerateGraphQLType(type, typeWithNullability)
+        }
+
+        return graphQLType
     }
 
-    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)
 
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/extensions/kFunctionExtensions.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/extensions/kFunctionExtensions.kt
@@ -0,0 +1,11 @@
+package com.expedia.graphql.generator.extensions
+
+import kotlin.reflect.KFunction
+import kotlin.reflect.KParameter
+import kotlin.reflect.full.valueParameters
+
+internal fun KFunction<*>.getValidArguments(): List<KParameter> =
+    this.valueParameters
+        .filterNot { it.isGraphQLContext() }
+        .filterNot { it.isGraphQLIgnored() }
+        .filterNot { it.isDataFetchingEnvironment() }
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/state/TypesCache.kt b/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
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/FunctionBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/FunctionBuilder.kt
@@ -10,9 +10,7 @@ import com.expedia.graphql.generator.extensions.getGraphQLDescription
 import com.expedia.graphql.generator.extensions.getKClass
 import com.expedia.graphql.generator.extensions.getName
 import com.expedia.graphql.generator.extensions.getTypeOfFirstArgument
-import com.expedia.graphql.generator.extensions.isDataFetchingEnvironment
-import com.expedia.graphql.generator.extensions.isGraphQLContext
-import com.expedia.graphql.generator.extensions.isGraphQLIgnored
+import com.expedia.graphql.generator.extensions.getValidArguments
 import com.expedia.graphql.generator.extensions.isInterface
 import com.expedia.graphql.generator.extensions.safeCast
 import graphql.schema.FieldCoordinates
@@ -25,7 +23,6 @@ import kotlin.reflect.KFunction
 import kotlin.reflect.KParameter
 import kotlin.reflect.KType
 import kotlin.reflect.full.isSubclassOf
-import kotlin.reflect.full.valueParameters
 
 internal class FunctionBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
 
@@ -44,10 +41,7 @@ internal class FunctionBuilder(generator: SchemaGenerator) : TypeBuilder(generat
             builder.withDirective(it)
         }
 
-        fn.valueParameters
-            .filterNot { it.isGraphQLContext() }
-            .filterNot { it.isGraphQLIgnored() }
-            .filterNot { it.isDataFetchingEnvironment() }
+        fn.getValidArguments()
             .forEach {
                 // deprecation of arguments is currently unsupported: https://github.com/facebook/graphql/issues/197
                 builder.argument(argument(it))
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InterfaceBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InterfaceBuilder.kt
@@ -11,7 +11,9 @@ import graphql.TypeResolutionEnvironment
 import graphql.schema.GraphQLInterfaceType
 import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeReference
+import graphql.schema.GraphQLTypeUtil
 import kotlin.reflect.KClass
+import kotlin.reflect.full.createType
 
 internal class InterfaceBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
     internal fun interfaceType(kClass: KClass<*>): GraphQLType {
@@ -31,15 +33,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)
-                // skip objects currently under construction
-                if (objectType !is GraphQLTypeReference) {
-                    state.additionalTypes.add(objectType)
+            subTypeMapper.getSubTypesOf(kClass)
+                .map { graphQLTypeOf(it.createType()) }
+                .forEach {
+                    // Do not add objects currently under construction to the additional types
+                    val unwrappedType = GraphQLTypeUtil.unwrapNonNull(it)
+                    if (unwrappedType !is GraphQLTypeReference) {
+                        state.additionalTypes.add(it)
+                    }
                 }
-            }
+
+            val interfaceType = builder.build()
             codeRegistry.typeResolver(interfaceType) { env: TypeResolutionEnvironment -> env.schema.getObjectType(env.getObject<Any>().javaClass.kotlin.getSimpleName()) }
             config.hooks.onRewireGraphQLType(interfaceType).safeCast()
         }
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/ObjectBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/ObjectBuilder.kt
@@ -12,12 +12,13 @@ import graphql.schema.GraphQLInterfaceType
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeReference
+import graphql.schema.GraphQLTypeUtil
 import kotlin.reflect.KClass
 import kotlin.reflect.full.createType
 
 internal class ObjectBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
 
-    internal fun objectType(kClass: KClass<*>, interfaceType: GraphQLInterfaceType? = null): GraphQLType {
+    internal fun objectType(kClass: KClass<*>): GraphQLType {
         return state.cache.buildIfNotUnderConstruction(kClass) {
             val builder = GraphQLObjectType.newObject()
 
@@ -29,18 +30,14 @@ internal class ObjectBuilder(generator: SchemaGenerator) : TypeBuilder(generator
                 builder.withDirective(it)
             }
 
-            if (interfaceType != null) {
-                // invoked from the interface builder which can still be modified by the hooks
-                builder.withInterface(GraphQLTypeReference(interfaceType.name))
-            } else {
-                kClass.getValidSuperclasses(config.hooks)
-                    .map { objectFromReflection(it.createType(), false) }
-                    .forEach {
-                        when (it) {
-                            is GraphQLInterfaceType -> builder.withInterface(it)
-                        }
+            kClass.getValidSuperclasses(config.hooks)
+                .map { graphQLTypeOf(it.createType()) }
+                .forEach {
+                    when (val unwrappedType = GraphQLTypeUtil.unwrapNonNull(it)) {
+                        is GraphQLTypeReference -> builder.withInterface(unwrappedType)
+                        is GraphQLInterfaceType -> builder.withInterface(unwrappedType)
                     }
-            }
+                }
 
             kClass.getValidProperties(config.hooks)
                 .forEach { builder.field(generator.property(it, kClass)) }
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/UnionBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/UnionBuilder.kt
@@ -4,11 +4,11 @@ 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 graphql.TypeResolutionEnvironment
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLType
 import graphql.schema.GraphQLTypeReference
+import graphql.schema.GraphQLTypeUtil
 import graphql.schema.GraphQLUnionType
 import kotlin.reflect.KClass
 import kotlin.reflect.full.createType
@@ -24,16 +24,15 @@ internal class UnionBuilder(generator: SchemaGenerator) : TypeBuilder(generator)
                 builder.withDirective(it)
             }
 
-            val implementations = subTypeMapper.getSubTypesOf(kClass)
-            implementations.forEach {
-                val objectType = state.cache.get(TypesCacheKey(it.kotlin.createType(), false))
-                    ?: generator.objectType(it.kotlin)
-
-                when (objectType) {
-                    is GraphQLTypeReference -> builder.possibleType(objectType)
-                    is GraphQLObjectType -> builder.possibleType(objectType)
+            subTypeMapper.getSubTypesOf(kClass)
+                .map { graphQLTypeOf(it.createType()) }
+                .forEach {
+                    when (val unwrappedType = GraphQLTypeUtil.unwrapNonNull(it)) {
+                        is GraphQLTypeReference -> builder.possibleType(unwrappedType)
+                        is GraphQLObjectType -> builder.possibleType(unwrappedType)
+                    }
                 }
-            }
+
             val unionType = builder.build()
             codeRegistry.typeResolver(unionType) { env: TypeResolutionEnvironment -> env.schema.getObjectType(env.getObject<Any>().javaClass.kotlin.getSimpleName()) }
             config.hooks.onRewireGraphQLType(unionType)
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/extensions/KFunctionExtensionsKtTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/extensions/KFunctionExtensionsKtTest.kt
@@ -0,0 +1,48 @@
+package com.expedia.graphql.generator.extensions
+
+import com.expedia.graphql.annotations.GraphQLContext
+import com.expedia.graphql.annotations.GraphQLIgnore
+import graphql.schema.DataFetchingEnvironment
+import org.junit.jupiter.api.Test
+import kotlin.test.assertEquals
+
+internal class KFunctionExtensionsKtTest {
+
+    @Test
+    fun getValidArguments() {
+        val args = TestingClass::happyPath.getValidArguments()
+        assertEquals(expected = 1, actual = args.size)
+        assertEquals(expected = "color", actual = args.first().getName())
+    }
+
+    @Test
+    fun `getValidArguments should ignore @GraphQLIgnore`() {
+        val args = TestingClass::ignored.getValidArguments()
+        assertEquals(expected = 1, actual = args.size)
+        assertEquals(expected = "notIgnored", actual = args.first().getName())
+    }
+
+    @Test
+    fun `getValidArguments should ignore @GraphQLContext`() {
+        val args = TestingClass::context.getValidArguments()
+        assertEquals(expected = 1, actual = args.size)
+        assertEquals(expected = "notContext", actual = args.first().getName())
+    }
+
+    @Test
+    fun `getValidArguments should ignore DataFetchingEnvironment`() {
+        val args = TestingClass::dataFetchingEnvironment.getValidArguments()
+        assertEquals(expected = 1, actual = args.size)
+        assertEquals(expected = "notEnvironment", actual = args.first().getName())
+    }
+
+    private class TestingClass {
+        fun happyPath(color: String) = "You're color is $color"
+
+        fun ignored(@GraphQLIgnore ignoredArg: String, notIgnored: String) = "$ignoredArg and $notIgnored"
+
+        fun context(@GraphQLContext contextArg: String, notContext: String) = "$contextArg and $notContext"
+
+        fun dataFetchingEnvironment(environment: DataFetchingEnvironment, notEnvironment: String): String = "${environment.field.name} and $notEnvironment"
+    }
+}
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/TypeTestHelper.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/TypeTestHelper.kt
@@ -11,7 +11,6 @@ import com.expedia.graphql.generator.state.SchemaGeneratorState
 import com.expedia.graphql.generator.state.TypesCache
 import com.expedia.graphql.hooks.SchemaGeneratorHooks
 import graphql.schema.GraphQLCodeRegistry
-import graphql.schema.GraphQLInterfaceType
 import io.mockk.every
 import io.mockk.mockk
 import io.mockk.spyk
@@ -48,6 +47,7 @@ internal open class TypeTestHelper {
     private var directiveBuilder: DirectiveBuilder? = null
     private var functionBuilder: FunctionBuilder? = null
     private var propertyBuilder: PropertyBuilder? = null
+    private var unionBuilder: UnionBuilder? = null
 
     @BeforeTest
     fun setup() {
@@ -84,8 +84,8 @@ internal open class TypeTestHelper {
         }
 
         objectBuilder = spyk(ObjectBuilder(generator))
-        every { generator.objectType(any(), any()) } answers {
-            objectBuilder!!.objectType(it.invocation.args[0] as KClass<*>, it.invocation.args[1] as GraphQLInterfaceType?)
+        every { generator.objectType(any()) } answers {
+            objectBuilder!!.objectType(it.invocation.args[0] as KClass<*>)
         }
 
         listBuilder = spyk(ListBuilder(generator))
@@ -98,6 +98,11 @@ internal open class TypeTestHelper {
             interfaceBuilder!!.interfaceType(it.invocation.args[0] as KClass<*>)
         }
 
+        unionBuilder = spyk(UnionBuilder(generator))
+        every { generator.unionType(any()) } answers {
+            unionBuilder!!.unionType(it.invocation.args[0] as KClass<*>)
+        }
+
         directiveBuilder = spyk(DirectiveBuilder(generator))
         every { generator.directives(any()) } answers {
             val directives = directiveBuilder!!.directives(it.invocation.args[0] as KAnnotatedElement)
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/UnionBuilderTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/UnionBuilderTest.kt
@@ -33,6 +33,15 @@ internal class UnionBuilderTest : TypeTestHelper() {
     @GraphQLName("StrawBerryCakeRenamed")
     private class StrawBerryCakeCustomName : CakeCustomName
 
+    private interface NestedUnionA
+
+    private interface NestedUnionB
+
+    private class NestedClass : NestedUnionA, NestedUnionB {
+        fun getUnionA(): NestedUnionA = NestedClass()
+        fun getUnionB(): NestedUnionB = NestedClass()
+    }
+
     @Test
     fun `Test naming`() {
         val result = builder.unionType(Cake::class) as? GraphQLUnionType
@@ -84,4 +93,14 @@ internal class UnionBuilderTest : TypeTestHelper() {
         assertNotNull(second)
         assertEquals(first.hashCode(), second.hashCode())
     }
+
+    @Test
+    fun `verify nested classes resovle the type reference in the gererator`() {
+        val cache = generator.state.cache
+        assertTrue(cache.doesNotContain(NestedUnionA::class))
+
+        val unionType = builder.unionType(NestedUnionA::class) as? GraphQLUnionType
+        assertNotNull(unionType)
+        assertFalse(cache.doesNotContain(NestedUnionA::class))
+    }
 }
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/hooks/SchemaGeneratorHooksTest.kt b/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 {
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/test/integration/RecursiveInterfaceTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/test/integration/RecursiveInterfaceTest.kt

Original file line number	Diff line number	Diff line change
`@@ -7,5 +7,5 @@ internal class SubTypeMapper(supportedPackages: List<String>) {`
`7`	`7`
`8`	`8`	`private val reflections = Reflections(supportedPackages)`
`9`	`9`
`10`		`- fun getSubTypesOf(kclass: KClass<>): List<Class<>> = reflections.getSubTypesOf(kclass.java).filterNot { it.kotlin.isAbstract }`
	`10`	`+ fun getSubTypesOf(kclass: KClass<>): List<KClass<>> = reflections.getSubTypesOf(kclass.java).filterNot { it.kotlin.isAbstract }.map { it.kotlin }`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -39,8 +39,8 @@ internal class TypesCache(private val supportedPackages: List<String>) {`
`39`	`39`	`val cacheKey = getCacheKeyString(key)`
`40`	`40`
`41`	`41`	`if (cacheKey != null) {`
`42`		`- typeUnderConstruction.remove(kGraphQLType.kClass)`
`43`		`- return cache.put(cacheKey, kGraphQLType)`
	`42`	`+ cache[cacheKey] = kGraphQLType`
	`43`	`+ return kGraphQLType`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`return null`