From b2dd1f4dc62f936edc911362b75a80a290677c2a Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Tue, 7 May 2024 15:36:47 -0700 Subject: [PATCH 1/7] Convert graphql-java validation errors to TypedGraphQL error to be consistent with our other error handling. --- .../dgs/autoconfig/DgsAutoConfiguration.kt | 5 + .../ValidationErrorInstrumentation.kt | 34 +++++ .../dgs/ValidationErrorInstrumentationTest.kt | 118 ++++++++++++++++++ 3 files changed, 157 insertions(+) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt create mode 100644 graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt diff --git a/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt b/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt index 389fccf13..2388075fc 100644 --- a/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt +++ b/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt @@ -107,6 +107,11 @@ open class DgsAutoConfiguration( return GraphQLContextContributorInstrumentation(graphQLContextContributors.orderedStream().toList()) } + @Bean + open fun validationErrorInstrumentation(): Instrumentation { + return ValidationErrorInstrumentation() + } + @Bean @ConditionalOnMissingBean open fun dgsQueryExecutor( diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt new file mode 100644 index 000000000..b0f47de7d --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt @@ -0,0 +1,34 @@ +package com.netflix.graphql.dgs.internal + +import com.netflix.graphql.dgs.exceptions.DgsException +import com.netflix.graphql.types.errors.ErrorDetail +import com.netflix.graphql.types.errors.ErrorType +import com.netflix.graphql.types.errors.TypedGraphQLError +import graphql.ExecutionResult +import graphql.GraphQLError +import graphql.execution.instrumentation.InstrumentationState +import graphql.execution.instrumentation.SimplePerformantInstrumentation +import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters +import graphql.validation.ValidationError +import java.util.concurrent.CompletableFuture + +class ValidationErrorInstrumentation : SimplePerformantInstrumentation() { + override fun instrumentExecutionResult(executionResult: ExecutionResult?, parameters: InstrumentationExecutionParameters?, state: InstrumentationState?): CompletableFuture { + if (executionResult != null && executionResult.errors.isNotEmpty()) { + val newExecutionResult = ExecutionResult.newExecutionResult().from(executionResult) + val graphqlErrors: MutableList = mutableListOf() + executionResult.errors.filterIsInstance().forEach { validationError -> + val graphqlError = TypedGraphQLError + .newBuilder() + .errorType(ErrorType.BAD_REQUEST) + .errorDetail(ErrorDetail.Common.FIELD_NOT_FOUND) + .message(validationError.message) + .extensions(mapOf(DgsException.EXTENSION_CLASS_KEY to "ValidationError")) + .build() + graphqlErrors.add(graphqlError) + } + return CompletableFuture.completedFuture(newExecutionResult.errors(graphqlErrors).build()) + } + return super.instrumentExecutionResult(executionResult, parameters, state) + } +} diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt new file mode 100644 index 000000000..94555b5b8 --- /dev/null +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt @@ -0,0 +1,118 @@ +/* + * Copyright 2024 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.netflix.graphql.dgs + +import com.netflix.graphql.dgs.internal.ValidationErrorInstrumentation +import graphql.GraphQL +import graphql.execution.instrumentation.Instrumentation +import graphql.schema.StaticDataFetcher +import graphql.schema.idl.RuntimeWiring +import graphql.schema.idl.SchemaGenerator +import graphql.schema.idl.SchemaParser +import graphql.schema.idl.TypeRuntimeWiring +import org.assertj.core.api.Assertions +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test + +class ValidationErrorInstrumentationTest { + + private lateinit var validationErrorInstrumentation: Instrumentation + + private val schema = """ + type Query{ + hello: String + } + """.trimMargin() + + @BeforeEach + fun setup() { + validationErrorInstrumentation = ValidationErrorInstrumentation() + } + + @Test + fun `Validation errors are not added for valid queries`() { + val graphQL: GraphQL = buildGraphQL(schema) + val result = graphQL.execute( + """ + { + hello + } + """.trimIndent() + ) + + Assertions.assertThat(result.isDataPresent).isTrue + val data = result.getData>() + Assertions.assertThat(data["hello"]).isEqualTo("hello there!") + } + + @Test + fun `Validation errors contain errorDetail and errorType in the extensions for invalid fields`() { + val graphQL: GraphQL = buildGraphQL(schema) + val result = graphQL.execute( + """ + { + InvalidField + } + """.trimIndent() + ) + + Assertions.assertThat(result.isDataPresent).isFalse + Assertions.assertThat(result.errors.size).isEqualTo(1) + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("class", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["class"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") + Assertions.assertThat(result.errors[0].extensions["errorDetail"]).isEqualTo("FIELD_NOT_FOUND") + } + + @Test + fun `Multiple validation errors contain errorDetail and errorType in the extensions for invalid fields`() { + val graphQL: GraphQL = buildGraphQL(schema) + val result = graphQL.execute( + """ + { + InvalidField + helloInvalid + } + """.trimIndent() + ) + + Assertions.assertThat(result.isDataPresent).isFalse + Assertions.assertThat(result.errors.size).isEqualTo(2) + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("class", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["class"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") + Assertions.assertThat(result.errors[0].extensions["errorDetail"]).isEqualTo("FIELD_NOT_FOUND") + Assertions.assertThat(result.errors[1].extensions.keys.containsAll(listOf("class", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[1].extensions["class"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[1].extensions["errorType"]).isEqualTo("BAD_REQUEST") + Assertions.assertThat(result.errors[1].extensions["errorDetail"]).isEqualTo("FIELD_NOT_FOUND") + } + + private fun buildGraphQL(schema: String): GraphQL { + val schemaParser = SchemaParser() + val typeDefinitionRegistry = schemaParser.parse(schema) + val runtimeWiring = RuntimeWiring.newRuntimeWiring() + .type("Query") { builder: TypeRuntimeWiring.Builder -> + builder.dataFetcher("hello", StaticDataFetcher("hello there!")) + } + .build() + val schemaGenerator = SchemaGenerator() + val graphQLSchema = schemaGenerator.makeExecutableSchema(typeDefinitionRegistry, runtimeWiring) + + return GraphQL.newGraphQL(graphQLSchema).instrumentation(validationErrorInstrumentation).build() + } +} From 01d2f2805ef46cb2c1dbce563547b03e798ab5bc Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Thu, 9 May 2024 11:32:38 -0700 Subject: [PATCH 2/7] Handle more cases of validation errors and other graphql-java errors. --- .../dgs/autoconfig/DgsAutoConfiguration.kt | 4 +- .../GraphQLJavaErrorInstrumentation.kt | 70 +++++++++++++++++++ .../ValidationErrorInstrumentation.kt | 34 --------- ...=> GraphQLJavaErrorInstrumentationTest.kt} | 60 ++++++++++++---- 4 files changed, 120 insertions(+), 48 deletions(-) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt delete mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt rename graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/{ValidationErrorInstrumentationTest.kt => GraphQLJavaErrorInstrumentationTest.kt} (66%) diff --git a/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt b/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt index 2388075fc..995e87077 100644 --- a/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt +++ b/graphql-dgs-spring-boot-oss-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/autoconfig/DgsAutoConfiguration.kt @@ -108,8 +108,8 @@ open class DgsAutoConfiguration( } @Bean - open fun validationErrorInstrumentation(): Instrumentation { - return ValidationErrorInstrumentation() + open fun graphqlJavaErrorInstrumentation(): Instrumentation { + return GraphQLJavaErrorInstrumentation() } @Bean diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt new file mode 100644 index 000000000..8b68c2408 --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt @@ -0,0 +1,70 @@ +package com.netflix.graphql.dgs.internal + +import com.netflix.graphql.types.errors.ErrorDetail +import com.netflix.graphql.types.errors.ErrorType +import com.netflix.graphql.types.errors.TypedGraphQLError +import graphql.ExecutionResult +import graphql.GraphQLError +import graphql.execution.instrumentation.InstrumentationState +import graphql.execution.instrumentation.SimplePerformantInstrumentation +import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters +import graphql.validation.ValidationError +import graphql.validation.ValidationErrorType +import java.util.concurrent.CompletableFuture + +class GraphQLJavaErrorInstrumentation : SimplePerformantInstrumentation() { + override fun instrumentExecutionResult(executionResult: ExecutionResult, parameters: InstrumentationExecutionParameters?, state: InstrumentationState?): CompletableFuture { + if (executionResult.errors.isNotEmpty()) { + val newExecutionResult = ExecutionResult.newExecutionResult().from(executionResult) + val graphqlErrors: MutableList = mutableListOf() + + executionResult.errors.forEach { error -> + // put in the classification unless it's already there since graphql-java errors contain this field + val extensions = (if (error.extensions != null) error.extensions else emptyMap()).toMutableMap() + if (!extensions.containsKey("classification")) { + val errorClassification = error.errorType + extensions["classification"] = errorClassification.toSpecification(error) + } + + if (error.errorType == graphql.ErrorType.ValidationError || error.errorType == graphql.ErrorType.InvalidSyntax || + error.errorType == graphql.ErrorType.NullValueInNonNullableField + ) { + val graphqlErrorBuilder = TypedGraphQLError + .newBadRequestBuilder() + .locations(error.locations) + .message(error.message) + .extensions(extensions) + if (error is ValidationError) { + if (error.validationErrorType == ValidationErrorType.NullValueForNonNullArgument) { + graphqlErrorBuilder.errorDetail(ErrorDetail.Common.INVALID_ARGUMENT) + } else if (error.validationErrorType == ValidationErrorType.FieldUndefined) { + graphqlErrorBuilder.errorDetail(ErrorDetail.Common.FIELD_NOT_FOUND) + } + } + + graphqlErrors.add(graphqlErrorBuilder.build()) + } else if (error.errorType == graphql.ErrorType.OperationNotSupported) { + val graphqlErrorBuilder = TypedGraphQLError + .newBuilder() + .errorType(ErrorType.UNAVAILABLE) + .locations(error.locations) + .message(error.message) + .extensions(error.extensions) + graphqlErrors.add(graphqlErrorBuilder.build()) + } else if (error.errorType == graphql.ErrorType.DataFetchingException || error.errorType == graphql.ErrorType.ExecutionAborted) { + val graphqlErrorBuilder = TypedGraphQLError + .newBuilder() + .errorType(ErrorType.UNKNOWN) + .locations(error.locations) + .message(error.message) + .extensions(error.extensions) + graphqlErrors.add(graphqlErrorBuilder.build()) + } else { + graphqlErrors.add(error) + } + } + return CompletableFuture.completedFuture(newExecutionResult.errors(graphqlErrors).build()) + } + return super.instrumentExecutionResult(executionResult, parameters, state) + } +} diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt deleted file mode 100644 index b0f47de7d..000000000 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/ValidationErrorInstrumentation.kt +++ /dev/null @@ -1,34 +0,0 @@ -package com.netflix.graphql.dgs.internal - -import com.netflix.graphql.dgs.exceptions.DgsException -import com.netflix.graphql.types.errors.ErrorDetail -import com.netflix.graphql.types.errors.ErrorType -import com.netflix.graphql.types.errors.TypedGraphQLError -import graphql.ExecutionResult -import graphql.GraphQLError -import graphql.execution.instrumentation.InstrumentationState -import graphql.execution.instrumentation.SimplePerformantInstrumentation -import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters -import graphql.validation.ValidationError -import java.util.concurrent.CompletableFuture - -class ValidationErrorInstrumentation : SimplePerformantInstrumentation() { - override fun instrumentExecutionResult(executionResult: ExecutionResult?, parameters: InstrumentationExecutionParameters?, state: InstrumentationState?): CompletableFuture { - if (executionResult != null && executionResult.errors.isNotEmpty()) { - val newExecutionResult = ExecutionResult.newExecutionResult().from(executionResult) - val graphqlErrors: MutableList = mutableListOf() - executionResult.errors.filterIsInstance().forEach { validationError -> - val graphqlError = TypedGraphQLError - .newBuilder() - .errorType(ErrorType.BAD_REQUEST) - .errorDetail(ErrorDetail.Common.FIELD_NOT_FOUND) - .message(validationError.message) - .extensions(mapOf(DgsException.EXTENSION_CLASS_KEY to "ValidationError")) - .build() - graphqlErrors.add(graphqlError) - } - return CompletableFuture.completedFuture(newExecutionResult.errors(graphqlErrors).build()) - } - return super.instrumentExecutionResult(executionResult, parameters, state) - } -} diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt similarity index 66% rename from graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt rename to graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt index 94555b5b8..fdd8255a6 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/ValidationErrorInstrumentationTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt @@ -16,7 +16,7 @@ package com.netflix.graphql.dgs -import com.netflix.graphql.dgs.internal.ValidationErrorInstrumentation +import com.netflix.graphql.dgs.internal.GraphQLJavaErrorInstrumentation import graphql.GraphQL import graphql.execution.instrumentation.Instrumentation import graphql.schema.StaticDataFetcher @@ -30,17 +30,17 @@ import org.junit.jupiter.api.Test class ValidationErrorInstrumentationTest { - private lateinit var validationErrorInstrumentation: Instrumentation + private lateinit var graphqlJavaErrorInstrumentation: Instrumentation private val schema = """ type Query{ - hello: String + hello(name: String!): String } """.trimMargin() @BeforeEach fun setup() { - validationErrorInstrumentation = ValidationErrorInstrumentation() + graphqlJavaErrorInstrumentation = GraphQLJavaErrorInstrumentation() } @Test @@ -49,7 +49,7 @@ class ValidationErrorInstrumentationTest { val result = graphQL.execute( """ { - hello + hello(name: "xyz") } """.trimIndent() ) @@ -72,14 +72,50 @@ class ValidationErrorInstrumentationTest { Assertions.assertThat(result.isDataPresent).isFalse Assertions.assertThat(result.errors.size).isEqualTo(1) - Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("class", "errorDetail", "errorType"))) - Assertions.assertThat(result.errors[0].extensions["class"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("classification", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["classification"]).isEqualTo("ValidationError") Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") Assertions.assertThat(result.errors[0].extensions["errorDetail"]).isEqualTo("FIELD_NOT_FOUND") } @Test - fun `Multiple validation errors contain errorDetail and errorType in the extensions for invalid fields`() { + fun `Validation errors contain errorDetail and errorType in the extensions for invalid input`() { + val graphQL: GraphQL = buildGraphQL(schema) + val result = graphQL.execute( + """ + { + hello + } + """.trimIndent() + ) + + Assertions.assertThat(result.isDataPresent).isFalse + Assertions.assertThat(result.errors.size).isEqualTo(1) + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("classification", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["classification"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") + } + + @Test + fun `Validation errors contain errorDetail and errorType in the extensions for invalid syntax`() { + val graphQL: GraphQL = buildGraphQL(schema) + val result = graphQL.execute( + """ + { + hello( + } + """.trimIndent() + ) + + Assertions.assertThat(result.isDataPresent).isFalse + Assertions.assertThat(result.errors.size).isEqualTo(1) + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("classification", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["classification"]).isEqualTo("InvalidSyntax") + Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") + } + + @Test + fun `Multiple validation errors contain errorDetail and errorType in the extensions for multiple invalid fields`() { val graphQL: GraphQL = buildGraphQL(schema) val result = graphQL.execute( """ @@ -92,12 +128,12 @@ class ValidationErrorInstrumentationTest { Assertions.assertThat(result.isDataPresent).isFalse Assertions.assertThat(result.errors.size).isEqualTo(2) - Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("class", "errorDetail", "errorType"))) - Assertions.assertThat(result.errors[0].extensions["class"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("classification", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["classification"]).isEqualTo("ValidationError") Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") Assertions.assertThat(result.errors[0].extensions["errorDetail"]).isEqualTo("FIELD_NOT_FOUND") Assertions.assertThat(result.errors[1].extensions.keys.containsAll(listOf("class", "errorDetail", "errorType"))) - Assertions.assertThat(result.errors[1].extensions["class"]).isEqualTo("ValidationError") + Assertions.assertThat(result.errors[1].extensions["classification"]).isEqualTo("ValidationError") Assertions.assertThat(result.errors[1].extensions["errorType"]).isEqualTo("BAD_REQUEST") Assertions.assertThat(result.errors[1].extensions["errorDetail"]).isEqualTo("FIELD_NOT_FOUND") } @@ -113,6 +149,6 @@ class ValidationErrorInstrumentationTest { val schemaGenerator = SchemaGenerator() val graphQLSchema = schemaGenerator.makeExecutableSchema(typeDefinitionRegistry, runtimeWiring) - return GraphQL.newGraphQL(graphQLSchema).instrumentation(validationErrorInstrumentation).build() + return GraphQL.newGraphQL(graphQLSchema).instrumentation(graphqlJavaErrorInstrumentation).build() } } From cdb80fc34edcaa214037fa3a0df804edd4eb8f71 Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Thu, 9 May 2024 12:13:45 -0700 Subject: [PATCH 3/7] Fix lint errors. --- .../netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt index fdd8255a6..0f0a185de 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt @@ -28,7 +28,7 @@ import org.assertj.core.api.Assertions import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test -class ValidationErrorInstrumentationTest { +class GraphQLJavaErrorInstrumentationTest { private lateinit var graphqlJavaErrorInstrumentation: Instrumentation From ae47d46cdc9d7dffe41ed66b26e8a5b54518dbad Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Thu, 9 May 2024 13:17:03 -0700 Subject: [PATCH 4/7] Use path instead of queryPath for validation error metric. --- .../dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt | 2 +- .../graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt b/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt index a376dbb5a..4388eb57e 100644 --- a/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt +++ b/graphql-dgs-spring-boot-micrometer/src/main/kotlin/com/netflix/graphql/dgs/metrics/micrometer/DgsGraphQLMetricsInstrumentation.kt @@ -340,7 +340,7 @@ class DgsGraphQLMetricsInstrumentation( val errorDetail = errorDetailExtension(error) when (error) { is ValidationError -> { - errorPath = error.queryPath.orEmpty() + errorPath = error.path.orEmpty() errorType = ErrorType.BAD_REQUEST.name } is InvalidSyntaxError -> { diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt index 8b68c2408..35609c912 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt @@ -29,9 +29,11 @@ class GraphQLJavaErrorInstrumentation : SimplePerformantInstrumentation() { if (error.errorType == graphql.ErrorType.ValidationError || error.errorType == graphql.ErrorType.InvalidSyntax || error.errorType == graphql.ErrorType.NullValueInNonNullableField ) { + val path = if (error is ValidationError) error.queryPath else error.path val graphqlErrorBuilder = TypedGraphQLError .newBadRequestBuilder() .locations(error.locations) + .path(path) .message(error.message) .extensions(extensions) if (error is ValidationError) { From 623c65ffe3ac732c70cc7b817bbb3973df03cca7 Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Thu, 9 May 2024 14:06:52 -0700 Subject: [PATCH 5/7] Address feedback. --- .../GraphQLJavaErrorInstrumentation.kt | 23 ++++++++----------- .../GraphQLJavaErrorInstrumentationTest.kt | 19 +++++++++++++++ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt index 35609c912..d6f9412b4 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt @@ -27,7 +27,7 @@ class GraphQLJavaErrorInstrumentation : SimplePerformantInstrumentation() { } if (error.errorType == graphql.ErrorType.ValidationError || error.errorType == graphql.ErrorType.InvalidSyntax || - error.errorType == graphql.ErrorType.NullValueInNonNullableField + error.errorType == graphql.ErrorType.NullValueInNonNullableField || error.errorType == graphql.ErrorType.OperationNotSupported ) { val path = if (error is ValidationError) error.queryPath else error.path val graphqlErrorBuilder = TypedGraphQLError @@ -36,27 +36,24 @@ class GraphQLJavaErrorInstrumentation : SimplePerformantInstrumentation() { .path(path) .message(error.message) .extensions(extensions) + if (error is ValidationError) { - if (error.validationErrorType == ValidationErrorType.NullValueForNonNullArgument) { - graphqlErrorBuilder.errorDetail(ErrorDetail.Common.INVALID_ARGUMENT) - } else if (error.validationErrorType == ValidationErrorType.FieldUndefined) { + if (error.validationErrorType == ValidationErrorType.FieldUndefined) { graphqlErrorBuilder.errorDetail(ErrorDetail.Common.FIELD_NOT_FOUND) + } else { + graphqlErrorBuilder.errorDetail(ErrorDetail.Common.INVALID_ARGUMENT) } } - graphqlErrors.add(graphqlErrorBuilder.build()) - } else if (error.errorType == graphql.ErrorType.OperationNotSupported) { - val graphqlErrorBuilder = TypedGraphQLError - .newBuilder() - .errorType(ErrorType.UNAVAILABLE) - .locations(error.locations) - .message(error.message) - .extensions(error.extensions) + if (error.errorType == graphql.ErrorType.OperationNotSupported) { + graphqlErrorBuilder.errorDetail(ErrorDetail.Common.INVALID_ARGUMENT) + } graphqlErrors.add(graphqlErrorBuilder.build()) } else if (error.errorType == graphql.ErrorType.DataFetchingException || error.errorType == graphql.ErrorType.ExecutionAborted) { val graphqlErrorBuilder = TypedGraphQLError .newBuilder() - .errorType(ErrorType.UNKNOWN) + .errorType(ErrorType.INTERNAL) + .errorDetail(ErrorDetail.Common.SERVICE_ERROR) .locations(error.locations) .message(error.message) .extensions(error.extensions) diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt index 0f0a185de..59387e447 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt @@ -114,6 +114,25 @@ class GraphQLJavaErrorInstrumentationTest { Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") } + @Test + fun `Error contains errorDetail and errorType in the extensions for invalid operation`() { + val graphQL: GraphQL = buildGraphQL(schema) + val result = graphQL.execute( + """ + mutation { + hell + } + """.trimIndent() + ) + + Assertions.assertThat(result.isDataPresent).isFalse + Assertions.assertThat(result.errors.size).isEqualTo(1) + Assertions.assertThat(result.errors[0].extensions.keys.containsAll(listOf("classification", "errorDetail", "errorType"))) + Assertions.assertThat(result.errors[0].extensions["classification"]).isEqualTo("OperationNotSupported") + Assertions.assertThat(result.errors[0].extensions["errorType"]).isEqualTo("BAD_REQUEST") + Assertions.assertThat(result.errors[0].extensions["errorDetail"]).isEqualTo("INVALID_ARGUMENT") + } + @Test fun `Multiple validation errors contain errorDetail and errorType in the extensions for multiple invalid fields`() { val graphQL: GraphQL = buildGraphQL(schema) From 0b1356b2745de2ec826714116d5ab96fd1c82884 Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Thu, 9 May 2024 14:38:46 -0700 Subject: [PATCH 6/7] Formatting fixes. --- .../netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt index 59387e447..73d0f557f 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/GraphQLJavaErrorInstrumentationTest.kt @@ -118,7 +118,7 @@ class GraphQLJavaErrorInstrumentationTest { fun `Error contains errorDetail and errorType in the extensions for invalid operation`() { val graphQL: GraphQL = buildGraphQL(schema) val result = graphQL.execute( - """ + """ mutation { hell } From 58eca505c7e3ae7e1291ee43511a3db7df1faa1f Mon Sep 17 00:00:00 2001 From: "$(git --no-pager log --format=format:'%an' -n 1)" Date: Thu, 9 May 2024 14:52:09 -0700 Subject: [PATCH 7/7] Fix test. --- .../dgs/metrics/micrometer/MicrometerServletSmokeTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt b/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt index 2afeb3f71..2b8d295d6 100644 --- a/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt +++ b/graphql-dgs-spring-boot-micrometer/src/test/kotlin/com/netflix/graphql/dgs/metrics/micrometer/MicrometerServletSmokeTest.kt @@ -462,7 +462,7 @@ class MicrometerServletSmokeTest { .and("gql.operation.name", "anonymous") .and("gql.query.complexity", "none") .and("gql.query.sig.hash", "none") - .and("gql.errorDetail", "none") + .and("gql.errorDetail", "INVALID_ARGUMENT") .and("gql.errorCode", "BAD_REQUEST") .and("gql.path", "[hello]") .and("outcome", "failure")