fix: directives with arguments should be created per declaration (ExpediaGroup#287)

Directive definitions should only be added to the schema once - while building the schema we are caching the directive definitions and add them after processing all the objects. Our current logic relies on this cache to not-rebuild the directives per each one of its declarations. Directives can have arguments which means that we have to rebuild the directive definition if they accept any arguments.

Author: dariuszkuc

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

@@ -63,7 +63,7 @@ internal class SchemaGenerator(internal val config: SchemaGeneratorConfig) {
             builder.additionalType(it)
         }
 
-        builder.additionalDirectives(state.directives)
+        builder.additionalDirectives(state.directives.values.toSet())
         builder.codeRegistry(codeRegistry.build())
         return config.hooks.willBuildSchema(builder).build()
     }
@@ -95,15 +95,9 @@ internal class SchemaGenerator(internal val config: SchemaGeneratorConfig) {
     internal fun scalarType(type: KType, annotatedAsID: Boolean = false) =
         scalarTypeBuilder.scalarType(type, annotatedAsID)
 
-    internal fun directives(element: KAnnotatedElement): List<GraphQLDirective> {
-        val directives = directiveTypeBuilder.directives(element)
-        state.directives.addAll(directives)
-        return directives
-    }
+    internal fun directives(element: KAnnotatedElement): List<GraphQLDirective> =
+        directiveTypeBuilder.directives(element)
 
-    internal fun fieldDirectives(field: Field): List<GraphQLDirective> {
-        val directives = directiveTypeBuilder.fieldDirectives(field)
-        state.directives.addAll(directives)
-        return directives
-    }
+    internal fun fieldDirectives(field: Field): List<GraphQLDirective> =
+        directiveTypeBuilder.fieldDirectives(field)
 }

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

@@ -4,21 +4,24 @@ import com.expedia.graphql.directives.DeprecatedDirective
 import graphql.Directives
 import graphql.schema.GraphQLDirective
 import graphql.schema.GraphQLType
+import java.util.concurrent.ConcurrentHashMap
 
 internal class SchemaGeneratorState(supportedPackages: List<String>) {
     val cache = TypesCache(supportedPackages)
     val additionalTypes = mutableSetOf<GraphQLType>()
-    val directives = mutableSetOf<GraphQLDirective>()
+    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
-        directives.add(Directives.IncludeDirective)
-        directives.add(Directives.SkipDirective)
+        directives[Directives.IncludeDirective.name] = Directives.IncludeDirective
+        directives[Directives.SkipDirective.name] = Directives.SkipDirective
 
         // graphql-kotlin default directives
-        directives.add(DeprecatedDirective)
+        // @deprecated directive is a built-in directive that each GraphQL server should provide bu currently it is not added by graphql-java
+        //   see https://github.com/graphql-java/graphql-java/issues/1598
+        directives[DeprecatedDirective.name] = DeprecatedDirective
     }
 }

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

@@ -25,38 +25,48 @@ internal class DirectiveBuilder(generator: SchemaGenerator) : TypeBuilder(genera
             .map(this::getDirective)
 
     private fun getDirective(directiveInfo: DirectiveInfo): GraphQLDirective {
-
-        val existingDirective = state.directives.find { it.name == directiveInfo.effectiveName }
-
-        if (existingDirective != null) {
-            return existingDirective
+        val directiveName = directiveInfo.effectiveName
+        val directive = state.directives.computeIfAbsent(directiveName) {
+            val builder = GraphQLDirective.newDirective()
+                .name(directiveInfo.effectiveName)
+                .description(directiveInfo.directiveAnnotation.description)
+
+            directiveInfo.directiveAnnotation.locations.forEach {
+                builder.validLocation(it)
+            }
+
+            val directiveClass = directiveInfo.directive.annotationClass
+            directiveClass.getValidProperties(config.hooks).forEach { prop ->
+                val propertyName = prop.name
+                val value = prop.call(directiveInfo.directive)
+                val type = graphQLTypeOf(prop.returnType)
+
+                val argument = GraphQLArgument.newArgument()
+                    .name(propertyName)
+                    .value(value)
+                    .type(type.safeCast())
+                    .build()
+
+                builder.argument(argument)
+            }
+            builder.build()
         }
 
-        val directiveClass = directiveInfo.directive.annotationClass
-
-        val builder = GraphQLDirective.newDirective()
-            .name(directiveInfo.effectiveName)
-            .description(directiveInfo.directiveAnnotation.description)
-
-        directiveInfo.directiveAnnotation.locations.forEach {
-            builder.validLocation(it)
+        return if (directive.arguments.isNotEmpty()) {
+            // update args for this instance
+            val builder = GraphQLDirective.newDirective(directive)
+            directiveInfo.directive.annotationClass.getValidProperties(config.hooks).forEach { prop ->
+                val defaultArgument = directive.getArgument(prop.name)
+                val value = prop.call(directiveInfo.directive)
+                val argument = GraphQLArgument.newArgument(defaultArgument)
+                    .value(value)
+                    .build()
+                builder.argument(argument)
+            }
+            builder.build()
+        } else {
+            directive
         }
-
-        directiveClass.getValidProperties(config.hooks).forEach { prop ->
-            val propertyName = prop.name
-            val value = prop.call(directiveInfo.directive)
-            val type = graphQLTypeOf(prop.returnType)
-
-            val argument = GraphQLArgument.newArgument()
-                .name(propertyName)
-                .value(value)
-                .type(type.safeCast())
-                .build()
-
-            builder.argument(argument)
-        }
-
-        return builder.build()
     }
 }
 

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/DirectiveBuilderTest.kt

@@ -7,6 +7,7 @@ import com.expedia.graphql.generator.SchemaGenerator
 import com.expedia.graphql.generator.extensions.isTrue
 import com.expedia.graphql.getTestSchemaConfigWithMockedDirectives
 import graphql.Directives
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import kotlin.reflect.KClass
 import kotlin.test.assertEquals
@@ -48,14 +49,22 @@ internal class DirectiveBuilderTest {
         @DirectiveWithString(string = "foo")
         fun directiveWithString(string: String) = string
 
+        @DirectiveWithString(string = "bar")
+        fun directiveWithAnotherString(string: String) = string
+
         @DirectiveWithEnum(type = Type.TWO)
         fun directiveWithEnum(string: String) = string
 
         @DirectiveWithClass(kclass = Type::class)
         fun directiveWithClass(string: String) = string
     }
 
-    private val basicGenerator = SchemaGenerator(getTestSchemaConfigWithMockedDirectives())
+    private lateinit var basicGenerator: SchemaGenerator
+
+    @BeforeEach
+    fun setUp() {
+        basicGenerator = SchemaGenerator(getTestSchemaConfigWithMockedDirectives())
+    }
 
     @Test
     fun `no annotation`() {
@@ -88,14 +97,17 @@ internal class DirectiveBuilderTest {
     }
 
     @Test
-    fun `directives are not duplicated in the schema`() {
+    fun `directives are only added to the schema once`() {
         val initialCount = basicGenerator.state.directives.size
-        assertTrue(basicGenerator.state.directives.contains(Directives.IncludeDirective))
-        assertTrue(basicGenerator.state.directives.contains(Directives.SkipDirective))
-        assertTrue(basicGenerator.state.directives.contains(DeprecatedDirective))
-
-        basicGenerator.directives(MyClass::simpleDirective)
-        basicGenerator.directives(MyClass::simpleDirective)
+        assertTrue(basicGenerator.state.directives.containsKey(Directives.IncludeDirective.name))
+        assertTrue(basicGenerator.state.directives.containsKey(Directives.SkipDirective.name))
+        assertTrue(basicGenerator.state.directives.containsKey(DeprecatedDirective.name))
+
+        val firstInvocation = basicGenerator.directives(MyClass::simpleDirective)
+        assertEquals(1, firstInvocation.size)
+        val secondInvocation = basicGenerator.directives(MyClass::simpleDirective)
+        assertEquals(1, secondInvocation.size)
+        assertEquals(firstInvocation.first(), secondInvocation.first())
         assertEquals(initialCount + 1, basicGenerator.state.directives.size)
     }
 
@@ -118,4 +130,22 @@ internal class DirectiveBuilderTest {
 
         assertEquals(0, directives.size)
     }
+
+    @Test
+    fun `directives are created per each declaration`() {
+        val initialCount = basicGenerator.state.directives.size
+        val directivesOnFirstField = basicGenerator.directives(MyClass::directiveWithString)
+        val directivesOnSecondField = basicGenerator.directives(MyClass::directiveWithAnotherString)
+        assertEquals(expected = 1, actual = directivesOnFirstField.size)
+        assertEquals(expected = 1, actual = directivesOnSecondField.size)
+
+        val firstDirective = directivesOnFirstField.first()
+        val seconDirective = directivesOnSecondField.first()
+        assertEquals("directiveWithString", firstDirective.name)
+        assertEquals("directiveWithString", seconDirective.name)
+        assertEquals("foo", firstDirective.getArgument("string")?.value)
+        assertEquals("bar", seconDirective.getArgument("string")?.value)
+
+        assertEquals(initialCount + 1, basicGenerator.state.directives.size)
+    }
 }

graphql-kotlin-schema-generator/src/test/kotlin/com/expedia/graphql/generator/types/TypeTestHelper.kt

@@ -101,12 +101,16 @@ internal open class TypeTestHelper {
         directiveBuilder = spyk(DirectiveBuilder(generator))
         every { generator.directives(any()) } answers {
             val directives = directiveBuilder!!.directives(it.invocation.args[0] as KAnnotatedElement)
-            state.directives.addAll(directives)
+            for (directive in directives) {
+                state.directives[directive.name] = directive
+            }
             directives
         }
         every { generator.fieldDirectives(any()) } answers {
             val directives = directiveBuilder!!.fieldDirectives(it.invocation.args[0] as Field)
-            state.directives.addAll(directives)
+            for (directive in directives) {
+                state.directives[directive.name] = directive
+            }
             directives
         }
 

graphql-kotlin-spring-example/src/main/kotlin/com/expedia/graphql/sample/directives/CustomDirectiveWiringFactory.kt

@@ -10,11 +10,11 @@ import kotlin.reflect.KClass
 class CustomDirectiveWiringFactory : KotlinDirectiveWiringFactory(manualWiring = mapOf<String, KotlinSchemaDirectiveWiring>("lowercase" to LowercaseSchemaDirectiveWiring())) {
 
     private val stringEvalDirectiveWiring = StringEvalSchemaDirectiveWiring()
-    private val caleOnlyDirectiveWiring = CakeOnlySchemaDirectiveWiring()
+    private val caleOnlyDirectiveWiring = SpecificValueOnlySchemaDirectiveWiring()
 
     override fun getSchemaDirectiveWiring(environment: KotlinSchemaDirectiveEnvironment<GraphQLDirectiveContainer>): KotlinSchemaDirectiveWiring? = when {
         environment.directive.name == getDirectiveName(StringEval::class) -> stringEvalDirectiveWiring
-        environment.directive.name == getDirectiveName(CakeOnly::class) -> caleOnlyDirectiveWiring
+        environment.directive.name == getDirectiveName(SpecificValueOnly::class) -> caleOnlyDirectiveWiring
         else -> null
     }
 }

graphql-kotlin-spring-example/src/main/kotlin/com/expedia/graphql/sample/directives/CakeOnly.kt renamed to graphql-kotlin-spring-example/src/main/kotlin/com/expedia/graphql/sample/directives/SpecificValueOnly.kt

@@ -2,5 +2,5 @@ package com.expedia.graphql.sample.directives
 
 import com.expedia.graphql.annotations.GraphQLDirective
 
-@GraphQLDirective(description = "This validates inputted string is equal to cake")
-annotation class CakeOnly
+@GraphQLDirective(description = "This validates inputted string is equal to specified argument")
+annotation class SpecificValueOnly(val value: String)

graphql-kotlin-spring-example/src/main/kotlin/com/expedia/graphql/sample/directives/CakeOnlySchemaDirectiveWiring.kt renamed to graphql-kotlin-spring-example/src/main/kotlin/com/expedia/graphql/sample/directives/SpecificValueOnlySchemaDirectiveWiring.kt

@@ -5,17 +5,19 @@ import com.expedia.graphql.directives.KotlinSchemaDirectiveWiring
 import graphql.schema.DataFetcher
 import graphql.schema.GraphQLFieldDefinition
 
-class CakeOnlySchemaDirectiveWiring : KotlinSchemaDirectiveWiring {
+class SpecificValueOnlySchemaDirectiveWiring : KotlinSchemaDirectiveWiring {
 
     @Throws(RuntimeException::class)
     override fun onField(environment: KotlinFieldDirectiveEnvironment): GraphQLFieldDefinition {
         val field = environment.element
         val originalDataFetcher: DataFetcher<Any> = environment.getDataFetcher()
 
+        val supportedValue = environment.directive.getArgument("value")?.value?.toString() ?: ""
+
         val cakeOnlyFetcher = DataFetcher<Any> { dataEnv ->
             val strArg: String? = dataEnv.getArgument(environment.element.arguments[0].name) as String?
-            if (!"cake".equals(other = strArg, ignoreCase = true)) {
-                throw RuntimeException("The cake is a lie!")
+            if (!supportedValue.equals(other = strArg, ignoreCase = true)) {
+                throw RuntimeException("Unsupported value, expected=$supportedValue actual=$strArg")
             }
             originalDataFetcher.get(dataEnv)
         }

graphql-kotlin-spring-example/src/main/kotlin/com/expedia/graphql/sample/query/CustomDirectiveQuery.kt

@@ -1,8 +1,8 @@
 package com.expedia.graphql.sample.query
 
 import com.expedia.graphql.annotations.GraphQLDescription
-import com.expedia.graphql.sample.directives.CakeOnly
 import com.expedia.graphql.sample.directives.LowercaseDirective
+import com.expedia.graphql.sample.directives.SpecificValueOnly
 import com.expedia.graphql.sample.directives.StringEval
 import org.springframework.stereotype.Component
 
@@ -13,10 +13,14 @@ class CustomDirectiveQuery : Query {
     fun justWhisper(@StringEval(default = "default string", lowerCase = true) msg: String?): String? = msg
 
     @GraphQLDescription("This will only accept 'Cake' as input")
-    @CakeOnly
+    @SpecificValueOnly("cake")
     fun onlyCake(msg: String): String = "<3"
 
+    @GraphQLDescription("This will only accept 'IceCream' as input")
+    @SpecificValueOnly("icecream")
+    fun onlyIceCream(msg: String): String = "<3"
+
     @GraphQLDescription("Returns message modified by the manually wired directive to force lowercase")
     @LowercaseDirective
     fun forceLowercaseEcho(msg: String) = msg
-}
+}