Directives on constructor args with no prefix (#320)

smyrick · web-flow · commit 0c87da38bd2c · 2019-09-05T10:03:35.000-07:00
Fixes #278 Property (input/output) directives are now resolved by checking the property and the constructor argument for any directives. This is the same pattern for checking any other annotations we have
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
@@ -8,6 +8,7 @@ import com.expedia.graphql.generator.types.DirectiveBuilder
 import com.expedia.graphql.generator.types.EnumBuilder
 import com.expedia.graphql.generator.types.FunctionBuilder
 import com.expedia.graphql.generator.types.InputObjectBuilder
+import com.expedia.graphql.generator.types.InputPropertyBuilder
 import com.expedia.graphql.generator.types.InterfaceBuilder
 import com.expedia.graphql.generator.types.ListBuilder
 import com.expedia.graphql.generator.types.MutationBuilder
@@ -43,6 +44,7 @@ open class SchemaGenerator(val config: SchemaGeneratorConfig) {
     private val interfaceTypeBuilder = InterfaceBuilder(this)
     private val propertyTypeBuilder = PropertyBuilder(this)
     private val inputObjectTypeBuilder = InputObjectBuilder(this)
+    private val inputPropertyBuilder = InputPropertyBuilder(this)
     private val listTypeBuilder = ListBuilder(this)
     private val functionTypeBuilder = FunctionBuilder(this)
     private val enumTypeBuilder = EnumBuilder(this)
@@ -85,6 +87,9 @@ open class SchemaGenerator(val config: SchemaGeneratorConfig) {
     open fun inputObjectType(kClass: KClass<*>) =
         inputObjectTypeBuilder.inputObjectType(kClass)
 
+    open fun inputProperty(prop: KProperty<*>, parentClass: KClass<*>) =
+        inputPropertyBuilder.inputProperty(prop, parentClass)
+
     open fun interfaceType(kClass: KClass<*>) =
         interfaceTypeBuilder.interfaceType(kClass)
 
@@ -97,8 +102,8 @@ open class SchemaGenerator(val config: SchemaGeneratorConfig) {
     open fun scalarType(type: KType, annotatedAsID: Boolean = false) =
         scalarTypeBuilder.scalarType(type, annotatedAsID)
 
-    open fun directives(element: KAnnotatedElement): List<GraphQLDirective> =
-        directiveTypeBuilder.directives(element)
+    open fun directives(element: KAnnotatedElement, parentClass: KClass<*>? = null): List<GraphQLDirective> =
+        directiveTypeBuilder.directives(element, parentClass)
 
     open fun fieldDirectives(field: Field): List<GraphQLDirective> =
         directiveTypeBuilder.fieldDirectives(field)
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
@@ -29,4 +29,7 @@ internal fun KProperty<*>.getPropertyDescription(parentClass: KClass<*>): String
 internal fun KProperty<*>.getPropertyName(parentClass: KClass<*>): String? =
     this.getGraphQLName() ?: getConstructorParameter(parentClass)?.getGraphQLName() ?: this.name
 
+internal fun KProperty<*>.getPropertyAnnotations(parentClass: KClass<*>): List<Annotation> =
+    this.annotations.union(getConstructorParameter(parentClass)?.annotations.orEmpty()).toList()
+
 private fun KProperty<*>.getConstructorParameter(parentClass: KClass<*>) = parentClass.findConstructorParamter(this.name)
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/DirectiveBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/DirectiveBuilder.kt
@@ -2,6 +2,7 @@ package com.expedia.graphql.generator.types
 
 import com.expedia.graphql.generator.SchemaGenerator
 import com.expedia.graphql.generator.TypeBuilder
+import com.expedia.graphql.generator.extensions.getPropertyAnnotations
 import com.expedia.graphql.generator.extensions.getSimpleName
 import com.expedia.graphql.generator.extensions.getValidProperties
 import com.expedia.graphql.generator.extensions.safeCast
@@ -10,14 +11,22 @@ import graphql.schema.GraphQLArgument
 import graphql.schema.GraphQLDirective
 import java.lang.reflect.Field
 import kotlin.reflect.KAnnotatedElement
+import kotlin.reflect.KClass
+import kotlin.reflect.KProperty
 import com.expedia.graphql.annotations.GraphQLDirective as GraphQLDirectiveAnnotation
 
 internal class DirectiveBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
 
-    internal fun directives(element: KAnnotatedElement): List<GraphQLDirective> =
-        element.annotations
+    internal fun directives(element: KAnnotatedElement, parentClass: KClass<*>?): List<GraphQLDirective> {
+        val annotations = when {
+            element is KProperty<*> && parentClass != null -> element.getPropertyAnnotations(parentClass)
+            else -> element.annotations
+        }
+
+        return annotations
             .mapNotNull { it.getDirectiveInfo() }
             .map(this::getDirective)
+    }
 
     internal fun fieldDirectives(field: Field): List<GraphQLDirective> =
         field.annotations
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InputObjectBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InputObjectBuilder.kt
@@ -3,17 +3,11 @@ package com.expedia.graphql.generator.types
 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.getPropertyDescription
-import com.expedia.graphql.generator.extensions.getPropertyName
 import com.expedia.graphql.generator.extensions.getSimpleName
 import com.expedia.graphql.generator.extensions.getValidProperties
-import com.expedia.graphql.generator.extensions.isPropertyGraphQLID
 import com.expedia.graphql.generator.extensions.safeCast
-import graphql.schema.GraphQLInputObjectField
 import graphql.schema.GraphQLInputObjectType
-import graphql.schema.GraphQLInputType
 import kotlin.reflect.KClass
-import kotlin.reflect.KProperty
 
 internal class InputObjectBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
     internal fun inputObjectType(kClass: KClass<*>): GraphQLInputObjectType {
@@ -27,21 +21,8 @@ internal class InputObjectBuilder(generator: SchemaGenerator) : TypeBuilder(gene
         }
 
         // It does not make sense to run functions against the input types so we only process the properties
-        kClass.getValidProperties(config.hooks)
-            .forEach { builder.field(inputProperty(it, kClass)) }
-
-        return config.hooks.onRewireGraphQLType(builder.build()).safeCast()
-    }
-
-    private fun inputProperty(prop: KProperty<*>, parentClass: KClass<*>): GraphQLInputObjectField {
-        val builder = GraphQLInputObjectField.newInputObjectField()
-
-        builder.description(prop.getPropertyDescription(parentClass))
-        builder.name(prop.getPropertyName(parentClass))
-        builder.type(graphQLTypeOf(prop.returnType, true, prop.isPropertyGraphQLID(parentClass)).safeCast<GraphQLInputType>())
-
-        generator.directives(prop).forEach {
-            builder.withDirective(it)
+        kClass.getValidProperties(config.hooks).forEach {
+            builder.field(generator.inputProperty(it, kClass))
         }
 
         return config.hooks.onRewireGraphQLType(builder.build()).safeCast()
diff --git a/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InputPropertyBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/InputPropertyBuilder.kt
@@ -0,0 +1,29 @@
+package com.expedia.graphql.generator.types
+
+import com.expedia.graphql.generator.SchemaGenerator
+import com.expedia.graphql.generator.TypeBuilder
+import com.expedia.graphql.generator.extensions.getPropertyDescription
+import com.expedia.graphql.generator.extensions.getPropertyName
+import com.expedia.graphql.generator.extensions.isPropertyGraphQLID
+import com.expedia.graphql.generator.extensions.safeCast
+import graphql.schema.GraphQLInputObjectField
+import graphql.schema.GraphQLInputType
+import kotlin.reflect.KClass
+import kotlin.reflect.KProperty
+
+internal class InputPropertyBuilder(generator: SchemaGenerator) : TypeBuilder(generator) {
+
+    internal fun inputProperty(prop: KProperty<*>, parentClass: KClass<*>): GraphQLInputObjectField {
+        val builder = GraphQLInputObjectField.newInputObjectField()
+
+        builder.description(prop.getPropertyDescription(parentClass))
+        builder.name(prop.getPropertyName(parentClass))
+        builder.type(graphQLTypeOf(prop.returnType, true, prop.isPropertyGraphQLID(parentClass)).safeCast<GraphQLInputType>())
+
+        generator.directives(prop, parentClass).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/PropertyBuilder.kt b/graphql-kotlin-schema-generator/src/main/kotlin/com/expedia/graphql/generator/types/PropertyBuilder.kt
@@ -30,7 +30,7 @@ internal class PropertyBuilder(generator: SchemaGenerator) : TypeBuilder(generat
             fieldBuilder.withDirective(deprecatedDirectiveWithReason(it))
         }
 
-        generator.directives(prop).forEach {
+        generator.directives(prop, parentClass).forEach {
             fieldBuilder.withDirective(it)
         }
 
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/DirectiveBuilderTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/DirectiveBuilderTest.kt
@@ -6,6 +6,7 @@ import com.expedia.graphql.directives.DeprecatedDirective
 import com.expedia.graphql.generator.SchemaGenerator
 import com.expedia.graphql.generator.extensions.isTrue
 import com.expedia.graphql.getTestSchemaConfigWithMockedDirectives
+import com.expedia.graphql.test.utils.SimpleDirective
 import graphql.Directives
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
@@ -15,9 +16,6 @@ import kotlin.test.assertTrue
 
 internal class DirectiveBuilderTest {
 
-    @GraphQLDirective
-    internal annotation class SimpleDirective
-
     @GraphQLDirective
     internal annotation class DirectiveWithString(val string: String)
 
@@ -59,6 +57,12 @@ internal class DirectiveBuilderTest {
         fun directiveWithClass(string: String) = string
     }
 
+    internal data class MyClassWithConstructorArgs(
+        @SimpleDirective val noPrefix: String,
+        @property:SimpleDirective val propertyPrefix: String,
+        val noDirective: String
+    )
+
     private lateinit var basicGenerator: SchemaGenerator
 
     @BeforeEach
@@ -148,4 +152,24 @@ internal class DirectiveBuilderTest {
 
         assertEquals(initialCount + 1, basicGenerator.state.directives.size)
     }
+
+    @Test
+    fun `directives on constructor arguments can be used with or without annotation prefix`() {
+        val noDirectiveResult = basicGenerator.directives(MyClassWithConstructorArgs::noDirective)
+        assertEquals(expected = 0, actual = noDirectiveResult.size)
+
+        val propertyPrefixResult = basicGenerator.directives(MyClassWithConstructorArgs::propertyPrefix)
+        assertEquals(expected = 1, actual = propertyPrefixResult.size)
+        assertEquals(expected = "simpleDirective", actual = propertyPrefixResult.first().name)
+
+        val noPrefixResult = basicGenerator.directives(MyClassWithConstructorArgs::noPrefix, MyClassWithConstructorArgs::class)
+        assertEquals(expected = 1, actual = noPrefixResult.size)
+        assertEquals(expected = "simpleDirective", actual = noPrefixResult.first().name)
+    }
+
+    @Test
+    fun `directives on constructor arguments only works with parent class`() {
+        val noPrefixResult = basicGenerator.directives(MyClassWithConstructorArgs::noPrefix, null)
+        assertEquals(expected = 0, actual = noPrefixResult.size)
+    }
 }
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/InputPropertyBuilderTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/InputPropertyBuilderTest.kt
@@ -0,0 +1,53 @@
+package com.expedia.graphql.generator.types
+
+import com.expedia.graphql.annotations.GraphQLDescription
+import com.expedia.graphql.annotations.GraphQLName
+import com.expedia.graphql.test.utils.SimpleDirective
+import org.junit.jupiter.api.Test
+import kotlin.test.assertEquals
+
+internal class InputPropertyBuilderTest : TypeTestHelper() {
+
+    private lateinit var builder: InputPropertyBuilder
+
+    override fun beforeTest() {
+        builder = InputPropertyBuilder(generator)
+    }
+
+    private data class InputPropertyTestClass(
+        @GraphQLDescription("Custom description")
+        val description: String,
+
+        @GraphQLName("newName")
+        val changeMe: String,
+
+        @SimpleDirective
+        val directiveWithNoPrefix: String,
+
+        @property:SimpleDirective
+        val directiveWithPrefix: String
+    )
+
+    @Test
+    fun `Input property can have a description`() {
+        val result = builder.inputProperty(InputPropertyTestClass::description, InputPropertyTestClass::class)
+        assertEquals("Custom description", result.description)
+    }
+
+    @Test
+    fun `Input property names can change`() {
+        val result = builder.inputProperty(InputPropertyTestClass::changeMe, InputPropertyTestClass::class)
+        assertEquals("newName", result.name)
+    }
+
+    @Test
+    fun `Input property can have directives`() {
+        val resultWithNoPrefix = builder.inputProperty(InputPropertyTestClass::directiveWithNoPrefix, InputPropertyTestClass::class)
+        assertEquals(1, resultWithNoPrefix.directives.size)
+        assertEquals("simpleDirective", resultWithNoPrefix.directives.first().name)
+
+        val resultWithPrefix = builder.inputProperty(InputPropertyTestClass::directiveWithPrefix, InputPropertyTestClass::class)
+        assertEquals(1, resultWithPrefix.directives.size)
+        assertEquals("simpleDirective", resultWithPrefix.directives.first().name)
+    }
+}
diff --git a/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/PropertyBuilderTest.kt b/graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/PropertyBuilderTest.kt
@@ -10,6 +10,7 @@ import com.expedia.graphql.execution.KotlinDataFetcherFactoryProvider
 import com.expedia.graphql.generator.SchemaGenerator
 import com.expedia.graphql.generator.extensions.getSimpleName
 import com.expedia.graphql.hooks.SchemaGeneratorHooks
+import com.expedia.graphql.test.utils.SimpleDirective
 import graphql.Scalars
 import graphql.introspection.Introspection
 import graphql.schema.DataFetcher
@@ -51,7 +52,13 @@ internal class PropertyBuilderTest : TypeTestHelper() {
         val fooBar: String,
 
         @GraphQLID
-        val myId: String
+        val myId: String,
+
+        @SimpleDirective
+        val directiveWithNoPrefix: String,
+
+        @property:SimpleDirective
+        val directiveWithPrefix: String
     )
 
     private lateinit var builder: PropertyBuilder
@@ -127,6 +134,23 @@ internal class PropertyBuilderTest : TypeTestHelper() {
         )
     }
 
+    @Test
+    fun `Properties with no directives are not set`() {
+        val resultWithPrefix = builder.property(DataClassWithProperties::myId, DataClassWithProperties::class)
+        assertEquals(0, resultWithPrefix.directives.size)
+    }
+
+    @Test
+    fun `Properties can have directives on the constructor args`() {
+        val resultWithPrefix = builder.property(DataClassWithProperties::directiveWithPrefix, DataClassWithProperties::class)
+        assertEquals(1, resultWithPrefix.directives.size)
+        assertEquals("simpleDirective", resultWithPrefix.directives.first().name)
+
+        val resultWithNoPrefix = builder.property(DataClassWithProperties::directiveWithNoPrefix, DataClassWithProperties::class)
+        assertEquals(1, resultWithNoPrefix.directives.size)
+        assertEquals("simpleDirective", resultWithNoPrefix.directives.first().name)
+    }
+
     @Test
     fun `Test nullable property`() {
         val prop = ClassWithProperties::nullableCake
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
@@ -48,6 +48,7 @@ internal open class TypeTestHelper {
     private var directiveBuilder: DirectiveBuilder? = null
     private var functionBuilder: FunctionBuilder? = null
     private var propertyBuilder: PropertyBuilder? = null
+    private var inputPropertyBuilder: InputPropertyBuilder? = null
     private var unionBuilder: UnionBuilder? = null
     private var argumentBuilder: ArgumentBuilder? = null
     private var inputObjectBuilder: InputObjectBuilder? = null
@@ -81,6 +82,11 @@ internal open class TypeTestHelper {
             propertyBuilder!!.property(it.invocation.args[0] as KProperty<*>, it.invocation.args[1] as KClass<*>)
         }
 
+        inputPropertyBuilder = spyk(InputPropertyBuilder(generator))
+        every { generator.inputProperty(any(), any()) } answers {
+            inputPropertyBuilder!!.inputProperty(it.invocation.args[0] as KProperty<*>, it.invocation.args[1] as KClass<*>)
+        }
+
         scalarBuilder = spyk(ScalarBuilder(generator))
         every { generator.scalarType(any(), any()) } answers {
             scalarBuilder!!.scalarType(it.invocation.args[0] as KType, it.invocation.args[1] as Boolean)
@@ -117,8 +123,8 @@ internal open class TypeTestHelper {
         }
 
         directiveBuilder = spyk(DirectiveBuilder(generator))
-        every { generator.directives(any()) } answers {
-            val directives = directiveBuilder!!.directives(it.invocation.args[0] as KAnnotatedElement)
+        every { generator.directives(any(), any()) } answers {
+            val directives = directiveBuilder!!.directives(it.invocation.args[0] as KAnnotatedElement, it.invocation.args[1] as? KClass<*>)
             for (directive in directives) {
                 state.directives[directive.name] = directive
             }

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ internal class PropertyBuilder(generator: SchemaGenerator) : TypeBuilder(generat`
`30`	`30`	`fieldBuilder.withDirective(deprecatedDirectiveWithReason(it))`
`31`	`31`	`}`
`32`	`32`
`33`		`- generator.directives(prop).forEach {`
	`33`	`+ generator.directives(prop, parentClass).forEach {`
`34`	`34`	`fieldBuilder.withDirective(it)`
`35`	`35`	`}`
`36`	`36`