Fix bugs with generating arguments (#319)

smyrick · web-flow · commit 148c983a298b · 2019-09-05T10:03:02.000-07:00
Fixes #249 Fixes #318 Resolve the two bugs and refactor the function to its own ArgumentBuilder so we can more easily test the bugs
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
@@ -3,6 +3,7 @@ package com.expedia.graphql.generator
 import com.expedia.graphql.SchemaGeneratorConfig
 import com.expedia.graphql.TopLevelObject
 import com.expedia.graphql.generator.state.SchemaGeneratorState
+import com.expedia.graphql.generator.types.ArgumentBuilder
 import com.expedia.graphql.generator.types.DirectiveBuilder
 import com.expedia.graphql.generator.types.EnumBuilder
 import com.expedia.graphql.generator.types.FunctionBuilder
@@ -16,13 +17,15 @@ import com.expedia.graphql.generator.types.QueryBuilder
 import com.expedia.graphql.generator.types.ScalarBuilder
 import com.expedia.graphql.generator.types.SubscriptionBuilder
 import com.expedia.graphql.generator.types.UnionBuilder
+import graphql.schema.GraphQLArgument
 import graphql.schema.GraphQLCodeRegistry
 import graphql.schema.GraphQLDirective
 import graphql.schema.GraphQLSchema
 import java.lang.reflect.Field
 import kotlin.reflect.KAnnotatedElement
 import kotlin.reflect.KClass
 import kotlin.reflect.KFunction
+import kotlin.reflect.KParameter
 import kotlin.reflect.KProperty
 import kotlin.reflect.KType
 
@@ -45,6 +48,7 @@ open class SchemaGenerator(val config: SchemaGeneratorConfig) {
     private val enumTypeBuilder = EnumBuilder(this)
     private val scalarTypeBuilder = ScalarBuilder(this)
     private val directiveTypeBuilder = DirectiveBuilder(this)
+    private val argumentBuilder = ArgumentBuilder(this)
 
     open fun generate(
         queries: List<TopLevelObject>,
@@ -98,4 +102,7 @@ open class SchemaGenerator(val config: SchemaGeneratorConfig) {
 
     open fun fieldDirectives(field: Field): List<GraphQLDirective> =
         directiveTypeBuilder.fieldDirectives(field)
+
+    open fun argument(parameter: KParameter): GraphQLArgument =
+        argumentBuilder.argument(parameter)
 }
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/extensions/kParameterExtensions.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/extensions/kParameterExtensions.kt
@@ -16,7 +16,7 @@ internal fun KParameter.isDataFetchingEnvironment() = this.type.classifier == Da
 
 @Throws(CouldNotGetNameOfKParameterException::class)
 internal fun KParameter.getName(): String =
-    this.name ?: throw CouldNotGetNameOfKParameterException(this)
+    this.getGraphQLName() ?: this.name ?: throw CouldNotGetNameOfKParameterException(this)
 
 @Throws(CouldNotCastArgumentException::class)
 internal fun KParameter.javaTypeClass(): Class<*> =
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/extensions/kPropertyExtensions.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/extensions/kPropertyExtensions.kt
@@ -3,6 +3,11 @@ package com.expedia.graphql.generator.extensions
 import kotlin.reflect.KClass
 import kotlin.reflect.KProperty
 
+/**
+ * KProperty Extensions:
+ * For properties we need to check both the property for annotations and the constructor argument
+ */
+
 internal fun KProperty<*>.isPropertyGraphQLID(parentClass: KClass<*>): Boolean = when {
     this.isGraphQLID() -> true
     getConstructorParameter(parentClass)?.isGraphQLID().isTrue() -> true
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/ArgumentBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/ArgumentBuilder.kt
@@ -0,0 +1,37 @@
+package com.expedia.graphql.generator.types
+
+import com.expedia.graphql.exceptions.InvalidInputFieldTypeException
+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.getName
+import com.expedia.graphql.generator.extensions.isGraphQLID
+import com.expedia.graphql.generator.extensions.isInterface
+import com.expedia.graphql.generator.extensions.safeCast
+import graphql.schema.GraphQLArgument
+import kotlin.reflect.KParameter
+
+internal class ArgumentBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
+
+    @Throws(InvalidInputFieldTypeException::class)
+    internal fun argument(parameter: KParameter): GraphQLArgument {
+
+        if (parameter.isInterface()) {
+            throw InvalidInputFieldTypeException(parameter)
+        }
+
+        val graphQLType = graphQLTypeOf(parameter.type, inputType = true, annotatedAsID = parameter.isGraphQLID())
+
+        // Deprecation of arguments is currently unsupported: https://github.com/facebook/graphql/issues/197
+        val builder = GraphQLArgument.newArgument()
+            .name(parameter.getName())
+            .description(parameter.getGraphQLDescription())
+            .type(graphQLType.safeCast())
+
+        generator.directives(parameter).forEach {
+            builder.withDirective(it)
+        }
+
+        return config.hooks.onRewireGraphQLType(builder.build()).safeCast()
+    }
+}
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
@@ -1,26 +1,21 @@
 package com.expedia.graphql.generator.types
 
 import com.expedia.graphql.directives.deprecatedDirectiveWithReason
-import com.expedia.graphql.exceptions.InvalidInputFieldTypeException
 import com.expedia.graphql.generator.SchemaGenerator
 import com.expedia.graphql.generator.TypeBuilder
 import com.expedia.graphql.generator.extensions.getDeprecationReason
 import com.expedia.graphql.generator.extensions.getFunctionName
 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.getValidArguments
-import com.expedia.graphql.generator.extensions.isInterface
 import com.expedia.graphql.generator.extensions.safeCast
 import graphql.schema.FieldCoordinates
-import graphql.schema.GraphQLArgument
 import graphql.schema.GraphQLFieldDefinition
 import graphql.schema.GraphQLOutputType
 import org.reactivestreams.Publisher
 import java.util.concurrent.CompletableFuture
 import kotlin.reflect.KFunction
-import kotlin.reflect.KParameter
 import kotlin.reflect.KType
 import kotlin.reflect.full.isSubclassOf
 
@@ -41,11 +36,9 @@ internal class FunctionBuilder(generator: SchemaGenerator) : TypeBuilder(generat
             builder.withDirective(it)
         }
 
-        fn.getValidArguments()
-            .forEach {
-                // deprecation of arguments is currently unsupported: https://github.com/facebook/graphql/issues/197
-                builder.argument(argument(it))
-            }
+        fn.getValidArguments().forEach {
+            builder.argument(generator.argument(it))
+        }
 
         val typeFromHooks = config.hooks.willResolveMonad(fn.returnType)
         val returnType = getWrappedReturnType(typeFromHooks)
@@ -67,23 +60,4 @@ internal class FunctionBuilder(generator: SchemaGenerator) : TypeBuilder(generat
             returnType.classifier == CompletableFuture::class -> returnType.getTypeOfFirstArgument()
             else -> returnType
         }
-
-    @Throws(InvalidInputFieldTypeException::class)
-    private fun argument(parameter: KParameter): GraphQLArgument {
-
-        if (parameter.isInterface()) {
-            throw InvalidInputFieldTypeException(parameter)
-        }
-
-        val builder = GraphQLArgument.newArgument()
-            .name(parameter.getName())
-            .description(parameter.getGraphQLDescription())
-            .type(graphQLTypeOf(parameter.type, true).safeCast())
-
-        generator.directives(parameter).forEach {
-            builder.withDirective(it)
-        }
-
-        return config.hooks.onRewireGraphQLType(builder.build()).safeCast()
-    }
 }
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/extensions/GraphQLSchemaExtensionsTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/extensions/GraphQLSchemaExtensionsTest.kt
@@ -87,7 +87,7 @@ class GraphQLSchemaExtensionsTest {
         val sdl = schema.print(includeDefaultSchemaDefinition = false, includeDirectives = false).trim()
         val expected = """
             type Query {
-              queryById(id: String!): TypeWithId!
+              queryById(id: ID!): TypeWithId!
             }
 
             type TypeWithId {
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/extensions/KParameterExtensionsKtTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/extensions/KParameterExtensionsKtTest.kt
@@ -56,6 +56,7 @@ internal class KParameterExtensionsKtTest {
     @Test
     fun getNameException() {
         val mockParam: KParameter = mockk()
+        every { mockParam.annotations } returns emptyList()
         every { mockParam.name } returns null
         assertFailsWith(CouldNotGetNameOfKParameterException::class) {
             mockParam.getName()
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/ArgumentBuilderTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/ArgumentBuilderTest.kt
@@ -0,0 +1,89 @@
+package com.expedia.graphql.generator.types
+
+import com.expedia.graphql.annotations.GraphQLDescription
+import com.expedia.graphql.annotations.GraphQLID
+import com.expedia.graphql.annotations.GraphQLName
+import com.expedia.graphql.exceptions.InvalidInputFieldTypeException
+import com.expedia.graphql.test.utils.SimpleDirective
+import graphql.schema.GraphQLNonNull
+import org.junit.jupiter.api.Test
+import kotlin.reflect.full.findParameterByName
+import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
+import kotlin.test.assertNotNull
+
+internal class ArgumentBuilderTest : TypeTestHelper() {
+
+    private lateinit var builder: ArgumentBuilder
+
+    override fun beforeTest() {
+        builder = ArgumentBuilder(generator)
+    }
+
+    internal interface MyInterface {
+        val id: String
+    }
+
+    internal class ArgumentTestClass {
+        fun description(@GraphQLDescription("Argument description") input: String) = input
+
+        fun directive(@SimpleDirective input: String) = input
+
+        fun changeName(@GraphQLName("newName") input: String) = input
+
+        fun id(@GraphQLID idArg: Int) = "Your id is $idArg"
+
+        fun interfaceArg(input: MyInterface) = input.id
+    }
+
+    @Test
+    fun `Description is set on arguments`() {
+        val kParameter = ArgumentTestClass::description.findParameterByName("input")
+        assertNotNull(kParameter)
+        val result = builder.argument(kParameter)
+
+        assertEquals("String", (result.type as? GraphQLNonNull)?.wrappedType?.name)
+        assertEquals("Argument description", result.description)
+    }
+
+    @Test
+    fun `Directives are included on arguments`() {
+        val kParameter = ArgumentTestClass::directive.findParameterByName("input")
+        assertNotNull(kParameter)
+        val result = builder.argument(kParameter)
+
+        assertEquals("String", (result.type as? GraphQLNonNull)?.wrappedType?.name)
+        assertEquals(1, result.directives.size)
+        assertEquals("simpleDirective", result.directives.firstOrNull()?.name)
+    }
+
+    @Test
+    fun `Argument names can be changed with @GraphQLName`() {
+        val kParameter = ArgumentTestClass::changeName.findParameterByName("input")
+        assertNotNull(kParameter)
+        val result = builder.argument(kParameter)
+
+        assertEquals("String", (result.type as? GraphQLNonNull)?.wrappedType?.name)
+        assertEquals("newName", result.name)
+    }
+
+    @Test
+    fun `ID argument type is valid`() {
+        val kParameter = ArgumentTestClass::id.findParameterByName("idArg")
+        assertNotNull(kParameter)
+        val result = builder.argument(kParameter)
+
+        assertEquals(expected = "idArg", actual = result.name)
+        assertEquals("ID", (result.type as? GraphQLNonNull)?.wrappedType?.name)
+    }
+
+    @Test
+    fun `Interface argument type throws exception`() {
+        val kParameter = ArgumentTestClass::interfaceArg.findParameterByName("input")
+        assertNotNull(kParameter)
+
+        assertFailsWith(InvalidInputFieldTypeException::class) {
+            builder.argument(kParameter)
+        }
+    }
+}
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/FunctionBuilderTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/FunctionBuilderTest.kt
@@ -39,26 +39,18 @@ internal class FunctionBuilderTest : TypeTestHelper() {
     @GraphQLDirective(locations = [Introspection.DirectiveLocation.FIELD_DEFINITION])
     internal annotation class FunctionDirective(val arg: String)
 
-    @GraphQLDirective
-    internal annotation class ArgumentDirective(val arg: String)
-
     private class Happy {
 
         @GraphQLDescription("By bob")
         @FunctionDirective("happy")
         fun littleTrees() = UUID.randomUUID().toString()
 
-        fun paint(@GraphQLDescription("brush color") @ArgumentDirective("red") color: String) = color.reversed()
-
-        @Deprecated("Should paint instead")
+        @Deprecated("Should paint instead", ReplaceWith("paint"))
         fun sketch(tree: String) = tree
 
         @GraphQLName("renamedFunction")
         fun originalName(input: String) = input
 
-        @Deprecated("No saw, just paint", replaceWith = ReplaceWith("paint"))
-        fun saw(tree: String) = tree
-
         fun context(@GraphQLContext context: String, string: String) = "$context and $string"
 
         fun ignoredParameter(color: String, @GraphQLIgnore ignoreMe: String) = "$color and $ignoreMe"
@@ -73,46 +65,31 @@ internal class FunctionBuilderTest : TypeTestHelper() {
     }
 
     @Test
-    fun `test description`() {
+    fun `Function description can be set`() {
         val kFunction = Happy::littleTrees
         val result = builder.function(kFunction, "Query")
         assertEquals("By bob", result.description)
     }
 
     @Test
-    fun `test description on argument`() {
-        val kFunction = Happy::paint
-        val result = builder.function(kFunction, "Query").arguments[0]
-        assertEquals("brush color", result.description)
+    fun `Function names can be changed`() {
+        val kFunction = Happy::originalName
+        val result = builder.function(kFunction, "Query")
+        assertEquals("renamedFunction", result.name)
     }
 
     @Test
-    fun `test deprecation`() {
+    fun `Functions can be deprecated`() {
         val kFunction = Happy::sketch
         val result = builder.function(kFunction, "Query")
         assertTrue(result.isDeprecated)
-        assertEquals("Should paint instead", result.deprecationReason)
+        assertEquals("Should paint instead, replace with paint", result.deprecationReason)
 
         val fieldDirectives = result.directives
         assertEquals(1, fieldDirectives.size)
         assertEquals("deprecated", fieldDirectives.first().name)
     }
 
-    @Test
-    fun `test changing the name of a function`() {
-        val kFunction = Happy::originalName
-        val result = builder.function(kFunction, "Query")
-        assertEquals("renamedFunction", result.name)
-    }
-
-    @Test
-    fun `test deprecation with replacement`() {
-        val kFunction = Happy::saw
-        val result = builder.function(kFunction, "Query")
-        assertTrue(result.isDeprecated)
-        assertEquals("No saw, just paint, replace with paint", result.deprecationReason)
-    }
-
     @Test
     fun `test custom directive on function`() {
         val kFunction = Happy::littleTrees
@@ -130,19 +107,6 @@ internal class FunctionBuilderTest : TypeTestHelper() {
         )
     }
 
-    @Test
-    fun `test custom directive on function argument`() {
-        val kFunction = Happy::paint
-        val result = builder.function(kFunction, "Query").arguments[0]
-
-        assertEquals(1, result.directives.size)
-        val directive = result.directives[0]
-        assertEquals("argumentDirective", directive.name)
-        assertEquals("red", directive.arguments[0].value)
-        assertEquals("arg", directive.arguments[0].name)
-        assertEquals(GraphQLNonNull(Scalars.GraphQLString), directive.arguments[0].type)
-    }
-
     @Test
     fun `test context on argument`() {
         val kFunction = Happy::context
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

Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ class GraphQLSchemaExtensionsTest {`
`87`	`87`	`val sdl = schema.print(includeDefaultSchemaDefinition = false, includeDirectives = false).trim()`
`88`	`88`	`val expected = """`
`89`	`89`	`type Query {`
`90`		`- queryById(id: String!): TypeWithId!`
	`90`	`+ queryById(id: ID!): TypeWithId!`
`91`	`91`	`}`
`92`	`92`
`93`	`93`	`type TypeWithId {`