From 1371cbffef3a5a2f218d46cc3a9f46ecfa9cf65d Mon Sep 17 00:00:00 2001 From: Shane Myrick Date: Thu, 12 Nov 2020 12:08:23 -0800 Subject: [PATCH] Update error message for invalid field selection (#936) Co-authored-by: Shane Myrick --- .../validation/validateFieldSelection.kt | 2 +- .../federation/validation/validateFieldSet.kt | 4 +- .../validation/ValidateFieldSetKtTest.kt | 13 +- .../ValidateRequiresDirectiveKtTest.kt | 113 +++++++++++++++++- .../integration/FederatedKeyDirectiveIT.kt | 8 +- .../FederatedRequiresDirectiveIT.kt | 6 +- 6 files changed, 132 insertions(+), 14 deletions(-) diff --git a/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSelection.kt b/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSelection.kt index 12493496c1..a0296567fb 100644 --- a/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSelection.kt +++ b/graphql-kotlin-federation/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/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSet.kt b/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSet.kt index aafecd488e..e7b6334f1e 100644 --- a/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/federation/validation/validateFieldSet.kt +++ b/graphql-kotlin-federation/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, validatedDirective: DirectiveInfo) { +internal fun validateFieldSet(fieldName: String, targetField: GraphQLFieldDefinition?, extendedType: Boolean, errors: MutableList, 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/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateFieldSetKtTest.kt b/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateFieldSetKtTest.kt index ca5ca78cb4..b7bd551ad3 100644 --- a/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateFieldSetKtTest.kt +++ b/graphql-kotlin-federation/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,6 +52,7 @@ class ValidateFieldSetKtTest { fun `@key returns an error on null targetField`() { val errors = mutableListOf() validateFieldSet( + fieldName = "foo", targetField = null, extendedType = false, errors = errors, @@ -59,7 +60,7 @@ class ValidateFieldSetKtTest { ) 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/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateRequiresDirectiveKtTest.kt b/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateRequiresDirectiveKtTest.kt index f70d6165ec..b3d6a4c00a 100644 --- a/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/ValidateRequiresDirectiveKtTest.kt +++ b/graphql-kotlin-federation/src/test/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/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/integration/FederatedKeyDirectiveIT.kt b/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/integration/FederatedKeyDirectiveIT.kt index 77f62d1903..b569cbd7ed 100644 --- a/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/integration/FederatedKeyDirectiveIT.kt +++ b/graphql-kotlin-federation/src/test/kotlin/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/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/integration/FederatedRequiresDirectiveIT.kt b/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/integration/FederatedRequiresDirectiveIT.kt index 711e8df7b0..f4f116c232 100644 --- a/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/federation/validation/integration/FederatedRequiresDirectiveIT.kt +++ b/graphql-kotlin-federation/src/test/kotlin/com/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) }