fix: stop validating @requires field set contents (#2165) (#2166)

cherry-pick
a955e0d

Co-authored-by: David Glasser <glasser@apollographql.com>

Author: samuelAndalon

generator/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/generator/federation/validation/FederatedSchemaValidator.kt

@@ -37,11 +37,10 @@ internal class FederatedSchemaValidator {
      * Validates target GraphQLType whether it is a valid federated object.
      *
      * Verifies:
-     * - @key, @provides and @requires field sets reference existing fields
-     * - @requires references @external fields
+     * - @key, @provides and @requires field sets are non-empty
+     * - @key and @provides field sets reference existing fields
      * - @provides references an object
-     * - field sets cannot reference unions
-     * - list and interfaces can only be referenced from `@requires` and `@provides`
+     * - @key field sets cannot reference unions, lists, or interfaces
      */
     internal fun validateGraphQLType(type: GraphQLType): List<String> {
         val unwrappedType = GraphQLTypeUtil.unwrapAll(type)
@@ -61,7 +60,9 @@ internal class FederatedSchemaValidator {
         errors.addAll(validateDirective(federatedType, KEY_DIRECTIVE_NAME, directiveMap, fieldMap))
         for (field in fields) {
             if (field.getAppliedDirective(REQUIRES_DIRECTIVE_NAME) != null) {
-                errors.addAll(validateDirective("$federatedType.${field.name}", REQUIRES_DIRECTIVE_NAME, field.allAppliedDirectivesByName, fieldMap))
+                // Don't validate @requires field set contents — the naive parser does not
+                // support inline fragments or __typename, which are valid in @requires.
+                errors.addAll(validateDirective("$federatedType.${field.name}", REQUIRES_DIRECTIVE_NAME, field.allAppliedDirectivesByName, null))
             }
 
             if (field.getAppliedDirective(PROVIDES_DIRECTIVE_NAME) != null) {

generator/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/generator/federation/validation/validateDirective.kt

@@ -26,7 +26,7 @@ internal fun validateDirective(
     validatedType: String,
     targetDirective: String,
     directiveMap: Map<String, List<GraphQLAppliedDirective>>,
-    fieldMap: Map<String, GraphQLFieldDefinition>
+    fieldMap: Map<String, GraphQLFieldDefinition>?
 ): List<String> {
     val validationErrors = mutableListOf<String>()
     val directives = directiveMap[targetDirective]
@@ -39,7 +39,7 @@ internal fun validateDirective(
             val fieldSet = fieldSetValue.split(" ").filter { it.isNotEmpty() }
             if (fieldSet.isEmpty()) {
                 validationErrors.add("@$targetDirective directive on $validatedType is missing field information")
-            } else {
+            } else if (fieldMap != null) {
                 val directiveInfo = DirectiveInfo(
                     directiveName = targetDirective,
                     fieldSet = fieldSetValue,

generator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/data/integration/requires/failure/_2/RequiresNonExistentField.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 Expedia, Inc
+ * Copyright 2024 Expedia, Inc
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,26 +14,44 @@
  * limitations under the License.
  */
 
-package com.expediagroup.graphql.generator.federation.data.integration.requires.failure._1
+package com.expediagroup.graphql.generator.federation.data.integration.requires.success._8
 
+import com.expediagroup.graphql.generator.federation.directives.ExternalDirective
 import com.expediagroup.graphql.generator.federation.directives.FieldSet
 import com.expediagroup.graphql.generator.federation.directives.KeyDirective
 import com.expediagroup.graphql.generator.federation.directives.RequiresDirective
+import kotlin.properties.Delegates
 
 /*
-# example of invalid usage of @requires directive when it references local field
-type RequiresLocalField @key(fields : "id") {
-  description: String!
-  id: String!
-  shippingCost: String! @requires(fields : "weight")
-  weight: Float!
+# example of @requires directive with inline fragment on union type (issue #1939)
+type RequiresInlineFragmentOnUnion @key(fields : "id") {
+  id: Int!
+  shippingLabel: String! @requires(fields : "animal { ... on Dog { breed } }")
+  animal: Animal! @external
+}
+
+union Animal = Dog | Cat
+
+type Dog {
+  breed: String!
+}
+
+type Cat {
+  color: String!
 }
  */
 @KeyDirective(fields = FieldSet("id"))
-class RequiresLocalField(val id: String, val description: String) {
+data class RequiresInlineFragmentOnUnion(val id: Int) {
 
-    var weight: Double = 0.0
+    @ExternalDirective
+    var animal: Animal by Delegates.notNull()
 
-    @RequiresDirective(FieldSet("weight"))
-    fun shippingCost(): String = "$${weight * 9.99}"
+    @RequiresDirective(FieldSet("animal { ... on Dog { breed } }"))
+    fun shippingLabel(): String = "label"
 }
+
+interface Animal
+
+data class Dog(val breed: String) : Animal
+
+data class Cat(val color: String) : Animal

…equires/failure/_1/RequiresLocalField.kt …cess/_8/RequiresInlineFragmentOnUnion.ktgenerator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/data/integration/requires/failure/_1/RequiresLocalField.kt renamed to generator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/data/integration/requires/success/_8/RequiresInlineFragmentOnUnion.kt generator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/data/integration/requires/failure/_1/RequiresLocalField.kt renamed to generator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/data/integration/requires/success/_8/RequiresInlineFragmentOnUnion.kt

@@ -270,4 +270,29 @@ internal class ValidateDirectiveKtTest {
             actual = exception.message
         )
     }
+
+    /**
+     * @foo(fields: "bar")
+     * Validates that when fieldMap is null, field set contents are not validated.
+     */
+    @Test
+    fun `if fieldMap is null, non-empty field set is accepted without validation`() {
+        val directive: GraphQLAppliedDirective = mockk {
+            every { name } returns "foo"
+            every { getArgument(eq("fields")) } returns mockk {
+                every { argumentValue.value } returns mockk<FieldSet> {
+                    every { value } returns "bar"
+                }
+            }
+        }
+
+        val validationErrors = validateDirective(
+            validatedType = "",
+            targetDirective = "foo",
+            directiveMap = mapOf("foo" to listOf(directive)),
+            fieldMap = null
+        )
+
+        assertTrue(validationErrors.isEmpty())
+    }
 }

generator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/validation/ValidateDirectiveKtTest.kt

@@ -19,14 +19,11 @@ package com.expediagroup.graphql.generator.federation.validation.integration
 import com.expediagroup.graphql.generator.federation.directives.EXTERNAL_DIRECTIVE_NAME
 import com.expediagroup.graphql.generator.federation.directives.KEY_DIRECTIVE_NAME
 import com.expediagroup.graphql.generator.federation.directives.REQUIRES_DIRECTIVE_NAME
-import com.expediagroup.graphql.generator.federation.exception.InvalidFederatedSchema
 import com.expediagroup.graphql.generator.federation.toFederatedSchema
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLTypeUtil
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertDoesNotThrow
-import kotlin.test.assertEquals
-import kotlin.test.assertFailsWith
 import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
@@ -93,32 +90,6 @@ class FederatedRequiresDirectiveIT {
         }
     }
 
-    @Test
-    fun `@requires directive cannot reference local fields`() {
-        val exception = assertFailsWith<InvalidFederatedSchema> {
-            toFederatedSchema(config = federatedTestConfig("com.expediagroup.graphql.generator.federation.data.integration.requires.failure._1"))
-        }
-        val expected =
-            """
-                Invalid federated schema:
-                 - @requires(fields = "weight") directive on RequiresLocalField.shippingCost specifies invalid field set - @requires should reference only @external fields, field=weight
-            """.trimIndent()
-        assertEquals(expected, exception.message)
-    }
-
-    @Test
-    fun `@requires directive should reference valid fields`() {
-        val exception = assertFailsWith<InvalidFederatedSchema> {
-            toFederatedSchema(config = federatedTestConfig("com.expediagroup.graphql.generator.federation.data.integration.requires.failure._2"))
-        }
-        val expected =
-            """
-                Invalid federated schema:
-                 - @requires(fields = "zipCode") directive on RequiresNonExistentField.shippingCost specifies invalid field set - field set specifies field that does not exist, field=zipCode
-            """.trimIndent()
-        assertEquals(expected, exception.message)
-    }
-
     @Test
     fun `verifies @requires needs @external leaf fields only`() {
         assertDoesNotThrow {
@@ -159,6 +130,23 @@ class FederatedRequiresDirectiveIT {
         }
     }
 
+    // Regression test for #1939: inline fragments in @requires field sets were
+    // rejected by the naive space-splitting parser.
+    @Test
+    fun `verifies @requires directive can use inline fragments on union type`() {
+        assertDoesNotThrow {
+            val schema = toFederatedSchema(config = federatedTestConfig("com.expediagroup.graphql.generator.federation.data.integration.requires.success._8"))
+            val validatedType = schema.getObjectType("RequiresInlineFragmentOnUnion")
+            assertTrue(validatedType.hasAppliedDirective(KEY_DIRECTIVE_NAME))
+            val externalField = validatedType.getFieldDefinition("animal")
+            assertNotNull(externalField)
+            assertTrue(externalField.hasAppliedDirective(EXTERNAL_DIRECTIVE_NAME))
+            val requiresField = validatedType.getFieldDefinition("shippingLabel")
+            assertNotNull(requiresField)
+            assertTrue(requiresField.hasAppliedDirective(REQUIRES_DIRECTIVE_NAME))
+        }
+    }
+
     @Test
     fun `verifies @external is applied on all fields within external type`() {
         assertDoesNotThrow {