Update error message for invalid field selection (#936)

Co-authored-by: Shane Myrick <[email protected]>
ExpediaGroup · Nov 12, 2020 · 1371cbf · 1371cbf
1 parent f553dd2
commit 1371cbf
Show file tree

Hide file tree

Showing 6 changed files with 132 additions and 14 deletions.
diff --git a/.../src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSelection.kt b/.../src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSelection.kt
@@ -42,7 +42,7 @@ internal fun validateFieldSelection(
                 }
             }
             "}" -> return
-            else -> validateFieldSet(fields[currentField], extendedType, errors, validatedDirective)
+            else -> validateFieldSet(currentField, fields[currentField], extendedType, errors, validatedDirective)
         }
         previousField = currentField
     }

diff --git a/...ration/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSet.kt b/...ration/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSet.kt
@@ -23,7 +23,7 @@ import graphql.schema.GraphQLList
 import graphql.schema.GraphQLTypeUtil
 import graphql.schema.GraphQLUnionType
 
-internal fun validateFieldSet(targetField: GraphQLFieldDefinition?, extendedType: Boolean, errors: MutableList<String>, validatedDirective: DirectiveInfo) {
+internal fun validateFieldSet(fieldName: String, targetField: GraphQLFieldDefinition?, extendedType: Boolean, errors: MutableList<String>, validatedDirective: DirectiveInfo) {
     val errorMessage = validatedDirective.getErrorString()
     if (null != targetField) {
         val externalField = targetField.getDirective(EXTERNAL_DIRECTIVE_NAME) != null
@@ -45,6 +45,6 @@ internal fun validateFieldSet(targetField: GraphQLFieldDefinition?, extendedType
             }
         }
     } else {
-        errors.add("$errorMessage specifies invalid field set - field set specifies fields that do not exist")
+        errors.add("$errorMessage specifies invalid field set - field set specifies field that does not exist, field=$fieldName")
     }
 }
diff --git a/.../src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateFieldSetKtTest.kt b/.../src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateFieldSetKtTest.kt
@@ -25,8 +25,8 @@ import graphql.schema.GraphQLNonNull
 import graphql.schema.GraphQLObjectType
 import graphql.schema.GraphQLTypeReference
 import graphql.schema.GraphQLUnionType
-import org.junit.jupiter.api.Test
 import kotlin.test.assertEquals
+import org.junit.jupiter.api.Test
 
 class ValidateFieldSetKtTest {
 
@@ -52,14 +52,15 @@ class ValidateFieldSetKtTest {
     fun `@key returns an error on null targetField`() {
         val errors = mutableListOf<String>()
         validateFieldSet(
+            fieldName = "foo",
             targetField = null,
             extendedType = false,
             errors = errors,
             validatedDirective = mockKeyDirectiveInfo
         )
 
         assertEquals(1, errors.size)
-        assertEquals("@key(fields = foo) directive on Parent specifies invalid field set - field set specifies fields that do not exist", errors.first())
+        assertEquals("@key(fields = foo) directive on Parent specifies invalid field set - field set specifies field that does not exist, field=foo", errors.first())
     }
 
     /**
@@ -76,6 +77,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,
@@ -101,6 +103,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = false,
             errors = errors,
@@ -127,6 +130,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,
@@ -163,6 +167,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,
@@ -188,6 +193,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,
@@ -219,6 +225,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,
@@ -250,6 +257,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,
@@ -275,6 +283,7 @@ class ValidateFieldSetKtTest {
             .build()
 
         validateFieldSet(
+            fieldName = "foo",
             targetField = target,
             extendedType = true,
             errors = errors,

diff --git a/.../kotlin/com/expediagroup/graphql/federation/validation/ValidateRequiresDirectiveKtTest.kt b/.../kotlin/com/expediagroup/graphql/federation/validation/ValidateRequiresDirectiveKtTest.kt
@@ -17,13 +17,14 @@
 package com.expediagroup.graphql.federation.validation
 
 import com.expediagroup.graphql.federation.externalDirective
+import com.expediagroup.graphql.federation.getKeyDirective
 import com.expediagroup.graphql.federation.getRequiresDirective
 import graphql.Scalars.GraphQLFloat
 import graphql.Scalars.GraphQLString
 import graphql.schema.GraphQLFieldDefinition
 import graphql.schema.GraphQLObjectType
-import org.junit.jupiter.api.Test
 import kotlin.test.assertEquals
+import org.junit.jupiter.api.Test
 
 class ValidateRequiresDirectiveKtTest {
 
@@ -32,6 +33,12 @@ class ValidateRequiresDirectiveKtTest {
         .type(GraphQLFloat)
         .build()
 
+    private val idExternalField = GraphQLFieldDefinition.newFieldDefinition()
+        .name("id")
+        .type(GraphQLString)
+        .withDirective(externalDirective)
+        .build()
+
     /**
      * type Foo {
      *   shippingCost: String
@@ -107,7 +114,7 @@ class ValidateRequiresDirectiveKtTest {
         )
 
         assertEquals(1, errors.size)
-        assertEquals("@requires(fields = bar) directive on Foo.shippingCost specifies invalid field set - field set specifies fields that do not exist", errors.first())
+        assertEquals("@requires(fields = bar) directive on Foo.shippingCost specifies invalid field set - field set specifies field that does not exist, field=bar", errors.first())
     }
 
     /**
@@ -173,4 +180,106 @@ class ValidateRequiresDirectiveKtTest {
 
         assertEquals(0, errors.size)
     }
+
+    /**
+     * type Foo @extends {
+     *   shippingCost: String @requires(fields: "bar { foo }")
+     *   bar: Bar @external
+     * }
+     *
+     * type Bar @extends @key(fields = "weight") {
+     *   weight: Float
+     * }
+     */
+    @Test
+    fun `Verify valid requires directive, but invalid nested field set selection`() {
+        val shippingCost = GraphQLFieldDefinition.newFieldDefinition()
+            .name("shippingCost")
+            .type(GraphQLString)
+            .withDirective(getRequiresDirective("bar { foo }"))
+            .build()
+
+        val barObject = GraphQLObjectType.newObject()
+            .name("Bar")
+            .field(weight)
+            .withDirective(getKeyDirective("weight"))
+            .build()
+
+        val barField = GraphQLFieldDefinition.newFieldDefinition()
+            .name("bar")
+            .type(barObject)
+            .withDirective(externalDirective)
+            .build()
+
+        val validatedType = GraphQLObjectType.newObject()
+            .name("Foo")
+            .field(shippingCost)
+            .field(barField)
+            .build()
+
+        val errors = validateRequiresDirective(
+            validatedType = validatedType.name,
+            fieldMap = validatedType.fieldDefinitions.map { it.name to it }.toMap(),
+            validatedField = shippingCost,
+            extendedType = true
+        )
+
+        assertEquals(1, errors.size)
+        assertEquals("@requires(fields = bar { foo }) directive on Foo.shippingCost specifies invalid field set - field set specifies field that does not exist, field=foo", errors.first())
+    }
+
+    /**
+     * type Foo @extends @key(fields = "id") {
+     *   id: String @external
+     *   bar: Bar @external
+     *   shippingCost: String @requires(fields: "bar { weight }")
+     * }
+     *
+     * type Bar @extends @key(fields = "weight") {
+     *   weight: Float @external
+     * }
+     */
+    @Test
+    fun `Verify valid requires directive with valid nested field set selection`() {
+        val shippingCostField = GraphQLFieldDefinition.newFieldDefinition()
+            .name("shippingCost")
+            .type(GraphQLString)
+            .withDirective(getRequiresDirective("bar { weight }"))
+            .build()
+
+        val externalWeightField = GraphQLFieldDefinition.newFieldDefinition()
+            .name("weight")
+            .type(GraphQLFloat)
+            .withDirective(externalDirective)
+            .build()
+
+        val barObject = GraphQLObjectType.newObject()
+            .name("Bar")
+            .field(externalWeightField)
+            .withDirective(getKeyDirective("weight"))
+            .build()
+
+        val barField = GraphQLFieldDefinition.newFieldDefinition()
+            .name("bar")
+            .type(barObject)
+            .withDirective(externalDirective)
+            .build()
+
+        val validatedType = GraphQLObjectType.newObject()
+            .name("Foo")
+            .field(idExternalField)
+            .field(shippingCostField)
+            .field(barField)
+            .withDirective(getKeyDirective("id"))
+            .build()
+
+        val errors = validateRequiresDirective(
+            validatedType = validatedType.name,
+            fieldMap = validatedType.fieldDefinitions.map { it.name to it }.toMap(),
+            validatedField = shippingCostField,
+            extendedType = true
+        )
+
+        assertEquals(0, errors.size)
+    }
 }
diff --git a/...lin/com/expediagroup/graphql/federation/validation/integration/FederatedKeyDirectiveIT.kt b/...lin/com/expediagroup/graphql/federation/validation/integration/FederatedKeyDirectiveIT.kt
@@ -30,11 +30,11 @@ import com.expediagroup.graphql.federation.data.integration.key.success._5.KeyWi
 import com.expediagroup.graphql.federation.exception.InvalidFederatedSchema
 import com.expediagroup.graphql.federation.toFederatedSchema
 import graphql.schema.GraphQLSchema
-import org.junit.jupiter.api.Test
-import org.junit.jupiter.api.assertDoesNotThrow
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 import kotlin.test.assertNotNull
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertDoesNotThrow
 
 class FederatedKeyDirectiveIT {
 
@@ -130,7 +130,7 @@ class FederatedKeyDirectiveIT {
                 queries = listOf(TopLevelObject(InvalidKeyQuery()))
             )
         }
-        val expected = "Invalid federated schema:\n - @key(fields = id) directive on InvalidKey specifies invalid field set - field set specifies fields that do not exist"
+        val expected = "Invalid federated schema:\n - @key(fields = id) directive on InvalidKey specifies invalid field set - field set specifies field that does not exist, field=id"
         assertEquals(expected, exception.message)
     }
 
@@ -207,7 +207,7 @@ class FederatedKeyDirectiveIT {
         }
         val expected = "Invalid federated schema:\n" +
             " - @key(fields = id { uuid }) directive on NestedKeyReferencingScalar specifies invalid field set - field set defines nested selection set on unsupported type\n" +
-            " - @key(fields = id { uuid }) directive on NestedKeyReferencingScalar specifies invalid field set - field set specifies fields that do not exist"
+            " - @key(fields = id { uuid }) directive on NestedKeyReferencingScalar specifies invalid field set - field set specifies field that does not exist, field=uuid"
         assertEquals(expected, exception.message)
     }
 }
diff --git a/...om/expediagroup/graphql/federation/validation/integration/FederatedRequiresDirectiveIT.kt b/...om/expediagroup/graphql/federation/validation/integration/FederatedRequiresDirectiveIT.kt
@@ -20,11 +20,11 @@ import com.expediagroup.graphql.TopLevelObject
 import com.expediagroup.graphql.federation.data.integration.requires.failure._3.RequiresOnLocalTypeQuery
 import com.expediagroup.graphql.federation.exception.InvalidFederatedSchema
 import com.expediagroup.graphql.federation.toFederatedSchema
-import org.junit.jupiter.api.Test
-import org.junit.jupiter.api.assertDoesNotThrow
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 import kotlin.test.assertNotNull
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertDoesNotThrow
 
 class FederatedRequiresDirectiveIT {
 
@@ -64,7 +64,7 @@ class FederatedRequiresDirectiveIT {
         val expected =
             """
                 Invalid federated schema:
-                 - @requires(fields = zipCode) directive on RequiresNonExistentField.shippingCost specifies invalid field set - field set specifies fields that do not exist
+                 - @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)
     }