From b352287a46e3d7c8ad09bc9cff7d455f60cd54b3 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 28 Sep 2022 22:18:15 -0500 Subject: [PATCH 01/11] Added "DgsException" for easier error handling * Removed explicit empty query check so instrumentation can catch InvalidSyntax * Mapped ValidationError and SyntaxError to BAD_REQUEST for micrometer * Re-introduced micrometer to the example project --- graphql-dgs-example-java/build.gradle.kts | 2 + .../src/main/resources/application.yml | 7 +++ .../DgsGraphQLMetricsInstrumentation.kt | 9 ++-- .../micrometer/MicrometerServletSmokeTest.kt | 38 ++++++++-------- .../dgs/webflux/MalformedQueryContentTest.kt | 7 ++- .../dgs/webmvc/MalformedQueryContentTest.kt | 16 ++++--- .../graphql/dgs/mvc/DgsRestController.kt | 1 - .../DefaultDataFetcherExceptionHandler.kt | 26 +++++------ .../dgs/exceptions/DgsBadRequestException.kt | 10 ++++- .../exceptions/DgsEntityNotFoundException.kt | 7 ++- .../graphql/dgs/exceptions/DgsException.kt | 37 ++++++++++++++++ .../DgsInvalidInputArgumentException.kt | 7 ++- .../dgs/internal/BaseDgsQueryExecutor.kt | 44 +++++++++---------- .../dgs/DefaultDgsFederationResolverTest.kt | 4 +- .../DefaultDataFetcherExceptionHandlerTest.kt | 20 +++++++++ .../internal/DefaultDgsQueryExecutorTest.kt | 19 ++++++-- .../graphql/dgs/internal/InputArgumentTest.kt | 2 +- 17 files changed, 170 insertions(+), 86 deletions(-) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt diff --git a/graphql-dgs-example-java/build.gradle.kts b/graphql-dgs-example-java/build.gradle.kts index b9b655aa5..1253db924 100644 --- a/graphql-dgs-example-java/build.gradle.kts +++ b/graphql-dgs-example-java/build.gradle.kts @@ -25,9 +25,11 @@ dependencies { // please review the Spring Boot Docs in the link below for additional information. // https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html#production-ready-metrics-export-simple implementation("org.springframework.boot:spring-boot-starter-actuator") + // Adding the DGS Micrometer integration that provides timers for gql.* // For additional information go to the link below: // https://netflix.github.io/dgs/advanced/instrumentation/ + implementation(project(":graphql-dgs-spring-boot-micrometer")) implementation("com.github.ben-manes.caffeine:caffeine") testImplementation(project(":graphql-dgs-client")) diff --git a/graphql-dgs-example-java/src/main/resources/application.yml b/graphql-dgs-example-java/src/main/resources/application.yml index 5558589aa..db8118336 100644 --- a/graphql-dgs-example-java/src/main/resources/application.yml +++ b/graphql-dgs-example-java/src/main/resources/application.yml @@ -2,6 +2,13 @@ spring: application: name: dgs-example +management: + endpoints: + web: + exposure: + include: + - metrics + logging: level: com.netflix.graphql.dgs.example: DEBUG \ No newline at end of file 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 66e8f31a0..21fef6d82 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 @@ -6,6 +6,7 @@ import com.netflix.graphql.dgs.metrics.DgsMetrics.GqlMetric import com.netflix.graphql.dgs.metrics.DgsMetrics.GqlTag import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsGraphQLMetricsTagsProvider import com.netflix.graphql.dgs.metrics.micrometer.utils.QuerySignatureRepository +import com.netflix.graphql.types.errors.ErrorType import graphql.ExecutionInput import graphql.ExecutionResult import graphql.GraphQLError @@ -344,11 +345,11 @@ class DgsGraphQLMetricsInstrumentation( when (error) { is ValidationError -> { errorPath = error.queryPath ?: emptyList() - errorType = errorType(error) + errorType = ErrorType.BAD_REQUEST.name } is InvalidSyntaxError -> { errorPath = emptyList() - errorType = errorType(error) + errorType = ErrorType.BAD_REQUEST.name } else -> { errorPath = error.path ?: emptyList() @@ -368,10 +369,6 @@ class DgsGraphQLMetricsInstrumentation( return dedupeErrorPaths.values } - private fun errorType(error: T): String { - return error.errorType?.toString() ?: TagUtils.TAG_VALUE_UNKNOWN - } - private fun errorTypeExtension(error: T): String { return extension(error, "errorType", TagUtils.TAG_VALUE_UNKNOWN) } 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 e58e840f4..e16343c47 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 @@ -27,11 +27,13 @@ import com.netflix.graphql.dgs.DgsTypeDefinitionRegistry import com.netflix.graphql.dgs.InputArgument import com.netflix.graphql.dgs.exceptions.DefaultDataFetcherExceptionHandler import com.netflix.graphql.dgs.exceptions.DgsBadRequestException +import com.netflix.graphql.dgs.exceptions.DgsException import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsContextualTagCustomizer import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsExecutionTagCustomizer import com.netflix.graphql.dgs.metrics.micrometer.tagging.DgsFieldFetchTagCustomizer import com.netflix.graphql.dgs.metrics.micrometer.utils.QuerySignatureRepository import com.netflix.graphql.types.errors.ErrorDetail +import com.netflix.graphql.types.errors.ErrorType import com.netflix.graphql.types.errors.TypedGraphQLError import graphql.GraphQLError import graphql.execution.DataFetcherExceptionHandler @@ -388,7 +390,7 @@ class MicrometerServletSmokeTest { .and("gql.query.complexity", "none") .and("gql.query.sig.hash", "none") .and("gql.errorDetail", "none") - .and("gql.errorCode", "InvalidSyntax") + .and("gql.errorCode", "BAD_REQUEST") .and("gql.path", "[]") .and("outcome", "failure") ) @@ -448,7 +450,7 @@ class MicrometerServletSmokeTest { .and("gql.query.complexity", "none") .and("gql.query.sig.hash", "none") .and("gql.errorDetail", "none") - .and("gql.errorCode", "ValidationError") + .and("gql.errorCode", "BAD_REQUEST") .and("gql.path", "[hello]") .and("outcome", "failure") ) @@ -479,8 +481,12 @@ class MicrometerServletSmokeTest { """ |{ | "errors":[ - | {"message":"java.lang.IllegalStateException: Exception triggered.","locations":[], - | "path":["triggerInternalFailure"],"extensions":{"errorType":"INTERNAL"}} + | { + | "message":"java.lang.IllegalStateException: Exception triggered.", + | "locations": [], + | "path": ["triggerInternalFailure"], + | "extensions": {"errorType":"INTERNAL"} + | } | ], | "data":{"triggerInternalFailure":null} |} @@ -550,7 +556,7 @@ class MicrometerServletSmokeTest { """ |{ | "errors":[ - | {"message":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException: Exception triggered.", + | {"message":"Exception triggered.", | "locations":[],"path":["triggerBadRequestFailure"], | "extensions":{"errorType":"BAD_REQUEST"}} | ], @@ -688,18 +694,12 @@ class MicrometerServletSmokeTest { .andExpect( content().json( """ - |{ - | "errors":[ - | {"message":"java.lang.IllegalStateException: Exception triggered.","locations":[], - | "path":["triggerInternalFailure"],"extensions":{"errorType":"INTERNAL"}}, - | {"message":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException: Exception triggered.","locations":[], - | "path":["triggerBadRequestFailure"],"extensions":{"errorType":"BAD_REQUEST"}}, - | {"message":"Exception triggered.","locations":[], - | "path":["triggerCustomFailure"], - | "extensions":{"errorType":"UNAVAILABLE","errorDetail":"ENHANCE_YOUR_CALM"}} - | ], - | "data":{"triggerInternalFailure":null,"triggerBadRequestFailure":null,"triggerCustomFailure":null} - |} + | {"errors":[ + | {"message":"java.lang.IllegalStateException: Exception triggered.","locations":[],"path":["triggerInternalFailure"],"extensions":{"errorType":"INTERNAL"}}, + | {"message":"Exception triggered.","locations":[],"path":["triggerBadRequestFailure"],"extensions":{"class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException","errorType":"BAD_REQUEST"}}, + | {"message":"Exception triggered.","locations":[],"path":["triggerCustomFailure"],"extensions":{"errorType":"UNAVAILABLE","errorDetail":"ENHANCE_YOUR_CALM"}} + | ], + | "data":{"triggerInternalFailure":null,"triggerBadRequestFailure":null,"triggerCustomFailure":null}} """.trimMargin(), false ) @@ -937,7 +937,7 @@ class MicrometerServletSmokeTest { val executor = ThreadPoolTaskExecutor() executor.corePoolSize = 1 executor.maxPoolSize = 1 - executor.threadNamePrefix = "${MicrometerServletSmokeTest::class.java.simpleName}-test-" + executor.setThreadNamePrefix("${MicrometerServletSmokeTest::class.java.simpleName}-test-") executor.setQueueCapacity(10) executor.initialize() return executor @@ -969,5 +969,5 @@ class MicrometerServletSmokeTest { } } - class CustomException(message: String?) : java.lang.IllegalStateException(message) + class CustomException(message: String) : DgsException(message = message, errorType = ErrorType.INTERNAL) } diff --git a/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt b/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt index 978d7f3b0..9382e4e4c 100644 --- a/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt +++ b/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt @@ -84,9 +84,12 @@ class MalformedQueryContentTest { { "errors":[ { - "message": "The query is null or empty.", + "message": "GraphQL operations must contain a non-empty `query`.", "locations":[], - "extensions":{"errorType":"BAD_REQUEST"} + "extensions": { + "errorType":"BAD_REQUEST", + "class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException" + } } ] } diff --git a/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt b/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt index e9acdc6f7..f22152dd4 100644 --- a/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt +++ b/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt @@ -95,12 +95,16 @@ class MalformedQueryContentTest { """ { "errors":[ - { - "message": "The query is null or empty.", - "locations": [], - "extensions": {"errorType":"BAD_REQUEST"} - } - ] + { + "message":"GraphQL operations must contain a non-empty `query`.", + "locations":[], + "path":[], + "extensions":{ + "class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException", + "errorType":"BAD_REQUEST" + } + } + ] } """.trimIndent() ) diff --git a/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt b/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt index 562251e6b..d8a4b48bc 100644 --- a/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt +++ b/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt @@ -259,7 +259,6 @@ open class DgsRestController( logger.error(errorMessage, ex) mapper.writeValueAsBytes(errorResponse.toSpecification()) } - return ResponseEntity(result, responseHeaders, HttpStatus.OK) } } diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt index eb1dcf91b..fea1431ec 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt @@ -46,23 +46,17 @@ class DefaultDataFetcherExceptionHandler : DataFetcherExceptionHandler { "Exception while executing data fetcher for ${handlerParameters.path}: ${exception.message}", exception ) - val graphqlError = if (springSecurityAvailable && isSpringSecurityAccessException(exception)) { - TypedGraphQLError.newPermissionDeniedBuilder() - .message("%s: %s", exception::class.java.name, exception.message) - .path(handlerParameters.path).build() - } else if (exception is DgsEntityNotFoundException) { - TypedGraphQLError.newNotFoundBuilder() - .message("%s: %s", exception::class.java.name, exception.message) - .path(handlerParameters.path).build() - } else if (exception is DgsBadRequestException) { - TypedGraphQLError.newBadRequestBuilder() - .message("%s: %s", exception::class.java.name, exception.message) - .path(handlerParameters.path).build() - } else { - TypedGraphQLError.newInternalErrorBuilder() - .message("%s: %s", exception::class.java.name, exception.message) - .path(handlerParameters.path).build() + + val graphqlError = when (exception) { + is DgsException -> exception.toGraphQlError(handlerParameters.path) + else -> when { + springSecurityAvailable && isSpringSecurityAccessException(exception) -> TypedGraphQLError.newPermissionDeniedBuilder() + else -> TypedGraphQLError.newInternalErrorBuilder() + }.message("%s: %s", exception::class.java.name, exception.message) + .path(handlerParameters.path) + .build() } + return DataFetcherExceptionHandlerResult.newResult() .error(graphqlError) .build() diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt index 55dd4a9f8..beb20c3c9 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,5 +15,11 @@ */ package com.netflix.graphql.dgs.exceptions +import com.netflix.graphql.types.errors.ErrorType -open class DgsBadRequestException(override val message: String = "Malformed or incorrect request") : RuntimeException(message) +open class DgsBadRequestException(override val message: String = "Malformed or incorrect request") : + DgsException(message = message, errorType = ErrorType.BAD_REQUEST) { + companion object { + val NULL_OR_EMPTY_QUERY_EXCEPTION = DgsBadRequestException("GraphQL operations must contain a non-empty `query`.") + } +} diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsEntityNotFoundException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsEntityNotFoundException.kt index ec3e82467..d4fa36425 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsEntityNotFoundException.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsEntityNotFoundException.kt @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,4 +16,7 @@ package com.netflix.graphql.dgs.exceptions -class DgsEntityNotFoundException(override val message: String = "Requested entity not found") : RuntimeException(message) +import com.netflix.graphql.types.errors.ErrorType + +class DgsEntityNotFoundException(override val message: String = "Requested entity not found") : + DgsException(message = message, errorType = ErrorType.NOT_FOUND) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt new file mode 100644 index 000000000..a6dcb2e8c --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt @@ -0,0 +1,37 @@ +/* + * Copyright 2022 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.exceptions + +import com.netflix.graphql.types.errors.ErrorType +import com.netflix.graphql.types.errors.TypedGraphQLError +import graphql.execution.ResultPath + +abstract class DgsException( + override val message: String, + override val cause: Exception? = null, + val errorType: ErrorType = ErrorType.UNKNOWN +) : RuntimeException(message, cause) { + fun toGraphQlError(path: ResultPath = ResultPath.rootPath()): TypedGraphQLError { + return TypedGraphQLError + .newBuilder() + .errorType(errorType) + .path(path) + .message(message) + .extensions(mapOf("class" to this::class.java.name)) + .build() + } +} diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsInvalidInputArgumentException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsInvalidInputArgumentException.kt index ac1155d64..5261841cb 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsInvalidInputArgumentException.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsInvalidInputArgumentException.kt @@ -1,5 +1,5 @@ /* - * Copyright 2021 Netflix, Inc. + * Copyright 2022 Netflix, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,4 +16,7 @@ package com.netflix.graphql.dgs.exceptions -class DgsInvalidInputArgumentException(override val message: String, override val cause: Exception? = null) : RuntimeException(message, cause) +import com.netflix.graphql.types.errors.ErrorType + +class DgsInvalidInputArgumentException(override val message: String, override val cause: Exception? = null) : + DgsException(message = message, cause = cause, errorType = ErrorType.BAD_REQUEST) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt index fb134529c..975b569cc 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt @@ -27,9 +27,12 @@ import com.jayway.jsonpath.ParseContext import com.jayway.jsonpath.spi.json.JacksonJsonProvider import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider import com.netflix.graphql.dgs.context.DgsContext -import com.netflix.graphql.types.errors.ErrorType -import com.netflix.graphql.types.errors.TypedGraphQLError -import graphql.* +import com.netflix.graphql.dgs.exceptions.DgsBadRequestException +import graphql.ExecutionInput +import graphql.ExecutionResult +import graphql.ExecutionResultImpl +import graphql.GraphQL +import graphql.GraphQLError import graphql.execution.ExecutionIdProvider import graphql.execution.ExecutionStrategy import graphql.execution.instrumentation.Instrumentation @@ -72,19 +75,14 @@ object BaseDgsQueryExecutor { idProvider: Optional, preparsedDocumentProvider: PreparsedDocumentProvider? ): CompletableFuture { - var inputVariables = variables ?: Collections.emptyMap() + val inputVariables = variables ?: Collections.emptyMap() if (!StringUtils.hasText(query)) { return CompletableFuture.completedFuture( ExecutionResultImpl .newExecutionResult() - .addError( - TypedGraphQLError - .newBadRequestBuilder() - .message("The query is null or empty.") - .errorType(ErrorType.BAD_REQUEST) - .build() - ).build() + .addError(DgsBadRequestException.NULL_OR_EMPTY_QUERY_EXCEPTION.toGraphQlError()) + .build() ) } @@ -101,19 +99,19 @@ object BaseDgsQueryExecutor { val dataLoaderRegistry = dataLoaderProvider.buildRegistryWithContextSupplier { dgsContext } - @Suppress("DEPRECATION") - val executionInputBuilder: ExecutionInput.Builder = - ExecutionInput - .newExecutionInput() - .query(query) - .operationName(operationName) - .variables(inputVariables) - .dataLoaderRegistry(dataLoaderRegistry) - .context(dgsContext) - .graphQLContext(dgsContext) - .extensions(extensions.orEmpty()) - return try { + @Suppress("DEPRECATION") + val executionInputBuilder: ExecutionInput.Builder = + ExecutionInput + .newExecutionInput() + .query(query) + .operationName(operationName) + .variables(inputVariables) + .dataLoaderRegistry(dataLoaderRegistry) + .context(dgsContext) + .graphQLContext(dgsContext) + .extensions(extensions.orEmpty()) + graphQL.executeAsync(executionInputBuilder.build()) } catch (e: Exception) { logger.error("Encountered an exception while handling query $query", e) diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt index dc76c2736..45683260b 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt @@ -198,7 +198,7 @@ class DefaultDgsFederationResolverTest { .satisfies( Consumer { assertThat(it) - .endsWith("MissingFederatedQueryArgument: The federated query is missing field(s) __typename") + .endsWith("The federated query is missing field(s) __typename") } ) } @@ -414,7 +414,7 @@ class DefaultDgsFederationResolverTest { .satisfies( Consumer { assertThat(it) - .contains("com.netflix.graphql.dgs.exceptions.DgsInvalidInputArgumentException: Invalid input argument exception") + .contains("Invalid input argument exception") } ) assertThat(customExceptionHandler.invocationCounter).isEqualTo(1) diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandlerTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandlerTest.kt index 4e302c8c3..970b55f9b 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandlerTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandlerTest.kt @@ -16,6 +16,7 @@ package com.netflix.graphql.dgs.exceptions +import com.netflix.graphql.types.errors.ErrorType import graphql.execution.DataFetcherExceptionHandlerParameters import graphql.execution.ResultPath import io.mockk.MockKAnnotations @@ -106,4 +107,23 @@ internal class DefaultDataFetcherExceptionHandlerTest { // We return null here because we don't want graphql-java to write classification field assertThat(result.errors[0].errorType).isNull() } + + @Test + fun `custom DGS exception should return custom error`() { + val customDgsExceptionMessage = "Studio Search Who" + val customDgsExceptionType = ErrorType.FAILED_PRECONDITION + class CustomDgsException() : + DgsException(message = customDgsExceptionMessage, errorType = customDgsExceptionType) + + every { dataFetcherExceptionHandlerParameters.exception }.returns(CustomDgsException()) + + val result = DefaultDataFetcherExceptionHandler().handleException(dataFetcherExceptionHandlerParameters).get() + assertThat(result.errors.size).isEqualTo(1) + + val extensions = result.errors[0].extensions + assertThat(extensions["errorType"]).isEqualTo(customDgsExceptionType.name) + + // We return null here because we don't want graphql-java to write classification field + assertThat(result.errors[0].errorType).isNull() + } } diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt index fbfdc6eee..a504bff38 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt @@ -23,7 +23,8 @@ import com.netflix.graphql.dgs.exceptions.DgsQueryExecutionDataExtractionExcepti import com.netflix.graphql.dgs.exceptions.QueryException import com.netflix.graphql.dgs.internal.method.InputArgumentResolver import com.netflix.graphql.dgs.internal.method.MethodDataFetcherFactory -import com.netflix.graphql.types.errors.ErrorType +import com.netflix.graphql.types.errors.TypedGraphQLError +import graphql.InvalidSyntaxError import graphql.execution.AsyncExecutionStrategy import graphql.execution.AsyncSerialExecutionStrategy import graphql.execution.instrumentation.SimpleInstrumentation @@ -42,6 +43,7 @@ import java.time.LocalDateTime import java.util.* import java.util.function.Supplier +@Suppress("GraphQLUnresolvedReference") @ExtendWith(MockKExtension::class) internal class DefaultDgsQueryExecutorTest { @@ -152,12 +154,21 @@ internal class DefaultDgsQueryExecutorTest { } @Test - fun `Returns a GraphQL Error wth BAD_REQUEST described in the extensions`() { + fun `Empty query returns a GraphQL Error wth type SyntaxError`() { val result = dgsQueryExecutor.execute(" ") assertThat(result) .isNotNull - .extracting { it.errors.first().extensions["errorType"] } - .isEqualTo(ErrorType.BAD_REQUEST.name) + .extracting { it.errors.first() } + .isInstanceOf(TypedGraphQLError::class.java) + } + + @Test + fun `Invalid Syntax query returns a GraphQL Error wth type SyntaxError`() { + val result = dgsQueryExecutor.execute("a") + assertThat(result) + .isNotNull + .extracting { it.errors.first() } + .isInstanceOf(InvalidSyntaxError::class.java) } @Test diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/InputArgumentTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/InputArgumentTest.kt index e0c987a20..98dd85b66 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/InputArgumentTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/InputArgumentTest.kt @@ -73,7 +73,7 @@ import java.time.LocalDateTime import java.time.format.DateTimeFormatter import java.util.* -@Suppress("unused") +@Suppress("unused", "GraphQLUnresolvedReference") @ExtendWith(MockKExtension::class) internal class InputArgumentTest { @MockK From e1939560c90e8b74c1f4a9d39c4810eeb29b6aa3 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 02/11] added tests for DgsException --- .../graphql/dgs/exceptions/DgsException.kt | 14 +++- .../dgs/exceptions/DgsExceptionTest.kt | 70 +++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DgsExceptionTest.kt diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt index a6dcb2e8c..ed9a33dbf 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsException.kt @@ -25,13 +25,21 @@ abstract class DgsException( override val cause: Exception? = null, val errorType: ErrorType = ErrorType.UNKNOWN ) : RuntimeException(message, cause) { - fun toGraphQlError(path: ResultPath = ResultPath.rootPath()): TypedGraphQLError { + companion object { + const val EXTENSION_CLASS_KEY = "class" + } + + fun toGraphQlError(path: ResultPath? = null): TypedGraphQLError { return TypedGraphQLError .newBuilder() + .apply { + if (path != null) { + path(path) + } + } .errorType(errorType) - .path(path) .message(message) - .extensions(mapOf("class" to this::class.java.name)) + .extensions(mapOf(EXTENSION_CLASS_KEY to this::class.java.name)) .build() } } diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DgsExceptionTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DgsExceptionTest.kt new file mode 100644 index 000000000..57c9ecbd3 --- /dev/null +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/exceptions/DgsExceptionTest.kt @@ -0,0 +1,70 @@ +/* + * Copyright 2022 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.exceptions + +import graphql.execution.ResultPath +import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.entry +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test + +class DgsExceptionTest { + companion object { + const val FAKE_ERROR_MESSAGE = "dgs message" + } + + @Nested + inner class ToGraphQlError { + @Test + fun `should default to not include path`() { + assertThat( + CustomDgsException() + .toGraphQlError() // no path + .path + ).isNull() + } + + @Test + fun `should include path if explicitly asked`() { + val pathSegments = listOf("path", "pathObj") + + assertThat( + CustomDgsException() + .toGraphQlError(path = ResultPath.fromList(pathSegments)) + .path + ).containsAll(pathSegments) + } + + @Test + fun `should contain error class name in extensions`() { + assertThat( + CustomDgsException() + .toGraphQlError() + .extensions + ).contains( + entry( + DgsException.EXTENSION_CLASS_KEY, + CustomDgsException::class.java.name + ) + ) + } + } + + private inner class CustomDgsException : DgsException( + message = FAKE_ERROR_MESSAGE + ) +} From 33934a4c427d67be80fe5f74b3476a2c0a425ed4 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 03/11] added DGS execution result + tests --- .../dgs/internal/DgsExecutionResult.kt | 91 +++++++++++++++++++ .../dgs/internal/DgsExecutionResultTest.kt | 83 +++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt create mode 100644 graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt new file mode 100644 index 000000000..43915e579 --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt @@ -0,0 +1,91 @@ +/* + * Copyright 2022 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.internal + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.databind.exc.InvalidDefinitionException +import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper +import com.netflix.graphql.dgs.internal.utils.TimeTracer +import graphql.ExecutionResult +import graphql.ExecutionResultImpl +import graphql.GraphQLError +import graphql.GraphqlErrorBuilder +import org.slf4j.Logger +import org.slf4j.LoggerFactory +import org.springframework.http.HttpHeaders +import org.springframework.http.HttpStatus +import org.springframework.http.ResponseEntity + +class DgsExecutionResult @JvmOverloads constructor( + val executionResult: ExecutionResult, + val headers: HttpHeaders = HttpHeaders(), + val status: HttpStatus = HttpStatus.OK +) : ExecutionResult by executionResult { + companion object { + private val sentinelObject = Any() + private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) + } + + constructor( + status: HttpStatus = HttpStatus.OK, + headers: HttpHeaders = HttpHeaders.EMPTY, + errors: List = listOf(), + extensions: Map? = null, + + // By default, assign data as a sentinel object. + // If we were to default to null here, this constructor + // would be unable to discriminate between an intentionally null + // response and one that the user left default. + data: Any? = sentinelObject + ) : this( + headers = headers, + status = status, + executionResult = ExecutionResultImpl + .newExecutionResult() + .errors(errors) + .extensions(extensions) + .apply { + if (data != sentinelObject) { + data(data) + } + } + .build() + ) + + fun toSpringResponse( + mapper: ObjectMapper = jacksonObjectMapper() + ): ResponseEntity { + val result = try { + TimeTracer.logTime( + { mapper.writeValueAsBytes(this.toSpecification()) }, + logger, + "Serialized JSON result in {}ms" + ) + } catch (ex: InvalidDefinitionException) { + val errorMessage = "Error serializing response: ${ex.message}" + val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build()) + logger.error(errorMessage, ex) + mapper.writeValueAsBytes(errorResponse.toSpecification()) + } + + return ResponseEntity( + result, + headers, + status + ) + } +} diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt new file mode 100644 index 000000000..5a00741ac --- /dev/null +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResultTest.kt @@ -0,0 +1,83 @@ +/* + * Copyright 2022 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.internal + +import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.entry +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.springframework.http.HttpHeaders +import org.springframework.http.HttpStatus + +class DgsExecutionResultTest { + @Test + fun `should be able to pass null for data`() { + assertThat( + DgsExecutionResult( + data = null + ).toSpecification() + ).contains(entry("data", null)) + } + + @Test + fun `should default to not having data`() { + assertThat( + DgsExecutionResult() + .toSpecification() + ).doesNotContainKey("data") + } + + @Test + fun `should be able to pass in custom data`() { + val data = "Check under your chair" + + assertThat( + DgsExecutionResult(data = data) + .toSpecification() + ).contains(entry("data", data)) + } + + @Nested + inner class ToSpringResponse { + @Test + fun `should create a spring response with specified headers`() { + val headers = HttpHeaders() + headers.add("We can add headers now??", "Yes we can") + + assertThat( + DgsExecutionResult( + headers = headers + ).toSpringResponse() + .headers + .toMap() + ).containsAllEntriesOf(headers.toMap()) + } + + @Test + fun `should create a spring response with specified http status code`() { + val httpStatusCode = HttpStatus.ALREADY_REPORTED + + assertThat( + DgsExecutionResult( + status = httpStatusCode + ).toSpringResponse() + .statusCode + .value() + ).isEqualTo(httpStatusCode.value()) + } + } +} From 1b99812b788486294883e116fbf018d21ec37952 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 04/11] teach HttpHandlers about dgsExecutionResult --- .../handlers/DefaultDgsWebfluxHttpHandler.kt | 11 ++++- .../graphql/dgs/mvc/DgsRestController.kt | 44 +++---------------- 2 files changed, 16 insertions(+), 39 deletions(-) diff --git a/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt b/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt index 2af9cae3a..e43181e97 100644 --- a/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt +++ b/graphql-dgs-spring-webflux-autoconfigure/src/main/kotlin/com/netflix/graphql/dgs/webflux/handlers/DefaultDgsWebfluxHttpHandler.kt @@ -20,6 +20,7 @@ import com.fasterxml.jackson.core.JsonParseException import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.databind.exc.MismatchedInputException import com.fasterxml.jackson.module.kotlin.readValue +import com.netflix.graphql.dgs.internal.DgsExecutionResult import com.netflix.graphql.dgs.reactive.DgsReactiveQueryExecutor import graphql.ExecutionResult import org.slf4j.Logger @@ -74,9 +75,15 @@ class DefaultDgsWebfluxHttpHandler( } return executionResult.flatMap { result -> + val dgsExecutionResult = when (result) { + is DgsExecutionResult -> result + else -> DgsExecutionResult(result) + } + ServerResponse - .ok() - .bodyValue(result.toSpecification()) + .status(dgsExecutionResult.status) + .headers { it.addAll(dgsExecutionResult.headers) } + .bodyValue(dgsExecutionResult.toSpecification()) }.onErrorResume { ex -> when (ex) { is JsonParseException -> diff --git a/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt b/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt index d8a4b48bc..13e149009 100644 --- a/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt +++ b/graphql-dgs-spring-webmvc/src/main/kotlin/com/netflix/graphql/dgs/mvc/DgsRestController.kt @@ -18,15 +18,13 @@ package com.netflix.graphql.dgs.mvc import com.fasterxml.jackson.core.JsonParseException import com.fasterxml.jackson.databind.ObjectMapper -import com.fasterxml.jackson.databind.exc.InvalidDefinitionException import com.fasterxml.jackson.databind.exc.MismatchedInputException import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper import com.fasterxml.jackson.module.kotlin.readValue import com.netflix.graphql.dgs.DgsQueryExecutor +import com.netflix.graphql.dgs.internal.DgsExecutionResult import com.netflix.graphql.dgs.internal.utils.MultipartVariableMapper import com.netflix.graphql.dgs.internal.utils.TimeTracer -import graphql.ExecutionResultImpl -import graphql.GraphqlErrorBuilder import graphql.execution.reactive.SubscriptionPublisher import org.slf4j.Logger import org.slf4j.LoggerFactory @@ -75,7 +73,9 @@ open class DgsRestController( ) { companion object { - const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers" + // defined in here and DgsExecutionResult, for backwards compatibility. + // keep these two variables synced. + const val DGS_RESPONSE_HEADERS_KEY = DgsExecutionResult.DGS_RESPONSE_HEADERS_KEY private val logger: Logger = LoggerFactory.getLogger(DgsRestController::class.java) } @@ -226,39 +226,9 @@ open class DgsRestController( .body("Trying to execute subscription on /graphql. Use /subscriptions instead!") } - val responseHeaders = if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) { - val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY] - val responseHeaders = HttpHeaders() - if (dgsResponseHeaders is Map<*, *>) { - dgsResponseHeaders.forEach { - if (it.key != null) { - responseHeaders.add(it.key.toString(), it.value?.toString()) - } - } - } else { - logger.warn( - "{} must be of type java.util.Map, but was {}", - DGS_RESPONSE_HEADERS_KEY, - dgsResponseHeaders?.javaClass?.name - ) - } - - executionResult.extensions.remove(DGS_RESPONSE_HEADERS_KEY) - responseHeaders - } else HttpHeaders() - - val result = try { - TimeTracer.logTime( - { mapper.writeValueAsBytes(executionResult.toSpecification()) }, - logger, - "Serialized JSON result in {}ms" - ) - } catch (ex: InvalidDefinitionException) { - val errorMessage = "Error serializing response: ${ex.message}" - val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build()) - logger.error(errorMessage, ex) - mapper.writeValueAsBytes(errorResponse.toSpecification()) + return when (executionResult) { + is DgsExecutionResult -> executionResult.toSpringResponse() + else -> DgsExecutionResult(executionResult).toSpringResponse() } - return ResponseEntity(result, responseHeaders, HttpStatus.OK) } } From a7beb8fad88161a53156bd17e167e2c84dc83f61 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 05/11] empty http request now returns 400 --- .../dgs/internal/BaseDgsQueryExecutor.kt | 13 ++++--- .../internal/DefaultDgsQueryExecutorTest.kt | 35 ++++++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt index 975b569cc..79e6e3811 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt @@ -40,6 +40,7 @@ import graphql.execution.preparsed.PreparsedDocumentProvider import graphql.schema.GraphQLSchema import org.slf4j.Logger import org.slf4j.LoggerFactory +import org.springframework.http.HttpStatus import org.springframework.util.StringUtils import java.util.* import java.util.concurrent.CompletableFuture @@ -79,10 +80,14 @@ object BaseDgsQueryExecutor { if (!StringUtils.hasText(query)) { return CompletableFuture.completedFuture( - ExecutionResultImpl - .newExecutionResult() - .addError(DgsBadRequestException.NULL_OR_EMPTY_QUERY_EXCEPTION.toGraphQlError()) - .build() + DgsExecutionResult( + status = HttpStatus.BAD_REQUEST, + errors = listOf( + DgsBadRequestException + .NULL_OR_EMPTY_QUERY_EXCEPTION + .toGraphQlError() + ) + ) ) } diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt index a504bff38..ce070e1b8 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/internal/DefaultDgsQueryExecutorTest.kt @@ -19,11 +19,11 @@ package com.netflix.graphql.dgs.internal import com.jayway.jsonpath.TypeRef import com.jayway.jsonpath.spi.mapper.MappingException import com.netflix.graphql.dgs.* +import com.netflix.graphql.dgs.exceptions.DgsBadRequestException import com.netflix.graphql.dgs.exceptions.DgsQueryExecutionDataExtractionException import com.netflix.graphql.dgs.exceptions.QueryException import com.netflix.graphql.dgs.internal.method.InputArgumentResolver import com.netflix.graphql.dgs.internal.method.MethodDataFetcherFactory -import com.netflix.graphql.types.errors.TypedGraphQLError import graphql.InvalidSyntaxError import graphql.execution.AsyncExecutionStrategy import graphql.execution.AsyncSerialExecutionStrategy @@ -154,12 +154,39 @@ internal class DefaultDgsQueryExecutorTest { } @Test - fun `Empty query returns a GraphQL Error wth type SyntaxError`() { + fun `Empty query returns DgsExecutionResult with NULL_OR_EMPTY_QUERY_EXCEPTION`() { val result = dgsQueryExecutor.execute(" ") + assertThat(result) .isNotNull - .extracting { it.errors.first() } - .isInstanceOf(TypedGraphQLError::class.java) + .isInstanceOf(DgsExecutionResult::class.java) + + assertThat( + result + .errors + .first() + .extensions["errorType"] + // default bad request error type + ).isEqualTo( + DgsBadRequestException() + .errorType + .name + ) + + assertThat( + result + .errors + .first() + .message + ).isEqualTo(DgsBadRequestException.NULL_OR_EMPTY_QUERY_EXCEPTION.message) + + val springResponse = (result as DgsExecutionResult).toSpringResponse() + + assertThat( + springResponse + .statusCode + .value() + ).isEqualTo(400) } @Test From 3b1bf4a58b8f08b1e56cea3f24195e6a95ab44b6 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 06/11] updated instrumentation examples --- .../datafetcher/MyInstrumentation.java | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java b/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java index 072984520..4b4c7db30 100644 --- a/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java +++ b/graphql-dgs-example-java/src/main/java/com/netflix/graphql/dgs/example/datafetcher/MyInstrumentation.java @@ -16,30 +16,25 @@ package com.netflix.graphql.dgs.example.datafetcher; -import com.netflix.graphql.dgs.mvc.DgsRestController; +import com.netflix.graphql.dgs.internal.DgsExecutionResult; import graphql.ExecutionResult; -import graphql.ExecutionResultImpl; import graphql.execution.instrumentation.SimpleInstrumentation; import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters; +import org.springframework.http.HttpHeaders; import org.springframework.stereotype.Component; -import java.util.HashMap; -import java.util.Map; import java.util.concurrent.CompletableFuture; @Component public class MyInstrumentation extends SimpleInstrumentation { @Override public CompletableFuture instrumentExecutionResult(ExecutionResult executionResult, InstrumentationExecutionParameters parameters) { - HashMap extensions = new HashMap<>(); - if(executionResult.getExtensions() != null) { - extensions.putAll(executionResult.getExtensions()); - } + HttpHeaders responseHeaders = new HttpHeaders(); + responseHeaders.add("myHeader", "hello"); - Map responseHeaders = new HashMap<>(); - responseHeaders.put("myHeader", "hello"); - extensions.put(DgsRestController.DGS_RESPONSE_HEADERS_KEY, responseHeaders); - - return super.instrumentExecutionResult(new ExecutionResultImpl(executionResult.getData(), executionResult.getErrors(), extensions), parameters); + return super.instrumentExecutionResult(new DgsExecutionResult( + executionResult, + responseHeaders + ), parameters); } } From 15e5139799cf5ac90f37410f107cde948ece52d9 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 07/11] added regression test for extension on response --- .../graphql/dgs/mvc/DgsRestControllerTest.kt | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt b/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt index 69f884b72..859e19dca 100644 --- a/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt +++ b/graphql-dgs-spring-webmvc/src/test/kotlin/com/netflix/graphql/dgs/mvc/DgsRestControllerTest.kt @@ -203,9 +203,22 @@ class DgsRestControllerTest { val result = DgsRestController(dgsQueryExecutor).graphql(requestBody.toByteArray(), null, null, null, httpHeaders, webRequest) assertThat(result.headers["myHeader"]).contains("hello") + + assertThat(result.body).isInstanceOfSatisfying(ByteArray::class.java) { body -> + val mapper = jacksonObjectMapper() + val (_, _, extensions) = mapper.readValue(body, GraphQLResponse::class.java) + assertThat(extensions).isNull() + } } - data class GraphQLResponse(val data: Map = emptyMap(), val errors: List = emptyList()) + data class GraphQLResponse( + val data: Map = emptyMap(), + val errors: List = emptyList(), + + // should not be on the response; only here so we can + // test that it does not exist. + val extensions: Map? + ) @JsonIgnoreProperties(ignoreUnknown = true) data class GraphQLError(val message: String) From 0d72c99140099c97080f6c42f097ee56ec30e78c Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 08/11] fixed expected status code in empty query test --- .../netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt b/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt index 9382e4e4c..e948f1569 100644 --- a/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt +++ b/graphql-dgs-spring-webflux-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webflux/MalformedQueryContentTest.kt @@ -77,7 +77,7 @@ class MalformedQueryContentTest { .bodyValue("{ }") .exchange() .expectStatus() - .isOk + .isBadRequest .expectBody() .json( """ From 8580ecde7b5893573bb33823b5d63b85845c5dde Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 09/11] added backwards compat for old method of adding headers --- .../dgs/internal/DgsExecutionResult.kt | 153 +++++++++++------- 1 file changed, 99 insertions(+), 54 deletions(-) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt index 43915e579..67c83a50c 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt @@ -31,61 +31,106 @@ import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity class DgsExecutionResult @JvmOverloads constructor( - val executionResult: ExecutionResult, - val headers: HttpHeaders = HttpHeaders(), - val status: HttpStatus = HttpStatus.OK -) : ExecutionResult by executionResult { - companion object { - private val sentinelObject = Any() - private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) - } + val executionResult: ExecutionResult, + val headers: HttpHeaders = HttpHeaders(), + val status: HttpStatus = HttpStatus.OK + ) : ExecutionResult by executionResult { + companion object { + // defined in here and DgsRestController, for backwards compatibility. + // keep these two variables synced. + const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers" + private val sentinelObject = Any() + private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) + } - constructor( - status: HttpStatus = HttpStatus.OK, - headers: HttpHeaders = HttpHeaders.EMPTY, - errors: List = listOf(), - extensions: Map? = null, + init { + addExtensionsHeaderKeyToHeader() + } - // By default, assign data as a sentinel object. - // If we were to default to null here, this constructor - // would be unable to discriminate between an intentionally null - // response and one that the user left default. - data: Any? = sentinelObject - ) : this( - headers = headers, - status = status, - executionResult = ExecutionResultImpl - .newExecutionResult() - .errors(errors) - .extensions(extensions) - .apply { - if (data != sentinelObject) { - data(data) - } - } - .build() - ) + constructor( + status: HttpStatus = HttpStatus.OK, + headers: HttpHeaders = HttpHeaders.EMPTY, + errors: List = listOf(), + extensions: Map? = null, - fun toSpringResponse( - mapper: ObjectMapper = jacksonObjectMapper() - ): ResponseEntity { - val result = try { - TimeTracer.logTime( - { mapper.writeValueAsBytes(this.toSpecification()) }, - logger, - "Serialized JSON result in {}ms" - ) - } catch (ex: InvalidDefinitionException) { - val errorMessage = "Error serializing response: ${ex.message}" - val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build()) - logger.error(errorMessage, ex) - mapper.writeValueAsBytes(errorResponse.toSpecification()) - } + // By default, assign data as a sentinel object. + // If we were to default to null here, this constructor + // would be unable to discriminate between an intentionally null + // response and one that the user left default. + data: Any? = sentinelObject + ) : this( + headers = headers, + status = status, + executionResult = ExecutionResultImpl + .newExecutionResult() + .errors(errors) + .extensions(extensions) + .apply { + if (data != sentinelObject) { + data(data) + } + } + .build() + ) - return ResponseEntity( - result, - headers, - status - ) - } -} + // for backwards compat with https://github.com/Netflix/dgs-framework/pull/1261. + private fun addExtensionsHeaderKeyToHeader() { + if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) { + val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY] + + if (dgsResponseHeaders is Map<*, *>) { + dgsResponseHeaders.forEach { + if (it.key != null) { + headers.add(it.key.toString(), it.value?.toString()) + } + } + } else { + logger.warn( + "{} must be of type java.util.Map, but was {}", + DGS_RESPONSE_HEADERS_KEY, + dgsResponseHeaders?.javaClass?.name + ) + } + } + } + + fun toSpringResponse( + mapper: ObjectMapper = jacksonObjectMapper() + ): ResponseEntity { + val result = try { + TimeTracer.logTime( + { mapper.writeValueAsBytes(this.toSpecification()) }, + logger, + "Serialized JSON result in {}ms" + ) + } catch (ex: InvalidDefinitionException) { + val errorMessage = "Error serializing response: ${ex.message}" + val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build()) + logger.error(errorMessage, ex) + mapper.writeValueAsBytes(errorResponse.toSpecification()) + } + + return ResponseEntity( + result, + headers, + status + ) + } + + // overridden for compatibility with https://github.com/Netflix/dgs-framework/pull/1261. + override fun toSpecification(): MutableMap { + val spec = executionResult.toSpecification() + + if (spec["extensions"] != null && extensions.containsKey(DGS_RESPONSE_HEADERS_KEY)) { + val extensions = spec["extensions"] as Map<*, *> + + if (extensions.size != 1) { + spec["extensions"] = extensions.minus(DGS_RESPONSE_HEADERS_KEY) + } else { + spec.remove("extensions") + } + } + + return spec + } + } \ No newline at end of file From 18e73f553becd3641c8635548d99bd9fc0bdeb3e Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 13:35:05 -0500 Subject: [PATCH 10/11] ran formatter --- .../dgs/internal/DgsExecutionResult.kt | 184 +++++++++--------- 1 file changed, 92 insertions(+), 92 deletions(-) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt index 67c83a50c..e2177c107 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsExecutionResult.kt @@ -31,106 +31,106 @@ import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity class DgsExecutionResult @JvmOverloads constructor( - val executionResult: ExecutionResult, - val headers: HttpHeaders = HttpHeaders(), - val status: HttpStatus = HttpStatus.OK - ) : ExecutionResult by executionResult { - companion object { - // defined in here and DgsRestController, for backwards compatibility. - // keep these two variables synced. - const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers" - private val sentinelObject = Any() - private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) - } + val executionResult: ExecutionResult, + val headers: HttpHeaders = HttpHeaders(), + val status: HttpStatus = HttpStatus.OK +) : ExecutionResult by executionResult { + companion object { + // defined in here and DgsRestController, for backwards compatibility. + // keep these two variables synced. + const val DGS_RESPONSE_HEADERS_KEY = "dgs-response-headers" + private val sentinelObject = Any() + private val logger: Logger = LoggerFactory.getLogger(DgsExecutionResult::class.java) + } - init { - addExtensionsHeaderKeyToHeader() - } + init { + addExtensionsHeaderKeyToHeader() + } - constructor( - status: HttpStatus = HttpStatus.OK, - headers: HttpHeaders = HttpHeaders.EMPTY, - errors: List = listOf(), - extensions: Map? = null, + constructor( + status: HttpStatus = HttpStatus.OK, + headers: HttpHeaders = HttpHeaders.EMPTY, + errors: List = listOf(), + extensions: Map? = null, - // By default, assign data as a sentinel object. - // If we were to default to null here, this constructor - // would be unable to discriminate between an intentionally null - // response and one that the user left default. - data: Any? = sentinelObject - ) : this( - headers = headers, - status = status, - executionResult = ExecutionResultImpl - .newExecutionResult() - .errors(errors) - .extensions(extensions) - .apply { - if (data != sentinelObject) { - data(data) - } - } - .build() - ) + // By default, assign data as a sentinel object. + // If we were to default to null here, this constructor + // would be unable to discriminate between an intentionally null + // response and one that the user left default. + data: Any? = sentinelObject + ) : this( + headers = headers, + status = status, + executionResult = ExecutionResultImpl + .newExecutionResult() + .errors(errors) + .extensions(extensions) + .apply { + if (data != sentinelObject) { + data(data) + } + } + .build() + ) - // for backwards compat with https://github.com/Netflix/dgs-framework/pull/1261. - private fun addExtensionsHeaderKeyToHeader() { - if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) { - val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY] + // for backwards compat with https://github.com/Netflix/dgs-framework/pull/1261. + private fun addExtensionsHeaderKeyToHeader() { + if (executionResult.extensions?.containsKey(DGS_RESPONSE_HEADERS_KEY) == true) { + val dgsResponseHeaders = executionResult.extensions[DGS_RESPONSE_HEADERS_KEY] - if (dgsResponseHeaders is Map<*, *>) { - dgsResponseHeaders.forEach { - if (it.key != null) { - headers.add(it.key.toString(), it.value?.toString()) - } - } - } else { - logger.warn( - "{} must be of type java.util.Map, but was {}", - DGS_RESPONSE_HEADERS_KEY, - dgsResponseHeaders?.javaClass?.name - ) - } - } - } + if (dgsResponseHeaders is Map<*, *>) { + dgsResponseHeaders.forEach { + if (it.key != null) { + headers.add(it.key.toString(), it.value?.toString()) + } + } + } else { + logger.warn( + "{} must be of type java.util.Map, but was {}", + DGS_RESPONSE_HEADERS_KEY, + dgsResponseHeaders?.javaClass?.name + ) + } + } + } - fun toSpringResponse( - mapper: ObjectMapper = jacksonObjectMapper() - ): ResponseEntity { - val result = try { - TimeTracer.logTime( - { mapper.writeValueAsBytes(this.toSpecification()) }, - logger, - "Serialized JSON result in {}ms" - ) - } catch (ex: InvalidDefinitionException) { - val errorMessage = "Error serializing response: ${ex.message}" - val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build()) - logger.error(errorMessage, ex) - mapper.writeValueAsBytes(errorResponse.toSpecification()) - } + fun toSpringResponse( + mapper: ObjectMapper = jacksonObjectMapper() + ): ResponseEntity { + val result = try { + TimeTracer.logTime( + { mapper.writeValueAsBytes(this.toSpecification()) }, + logger, + "Serialized JSON result in {}ms" + ) + } catch (ex: InvalidDefinitionException) { + val errorMessage = "Error serializing response: ${ex.message}" + val errorResponse = ExecutionResultImpl(GraphqlErrorBuilder.newError().message(errorMessage).build()) + logger.error(errorMessage, ex) + mapper.writeValueAsBytes(errorResponse.toSpecification()) + } - return ResponseEntity( - result, - headers, - status - ) - } + return ResponseEntity( + result, + headers, + status + ) + } - // overridden for compatibility with https://github.com/Netflix/dgs-framework/pull/1261. - override fun toSpecification(): MutableMap { - val spec = executionResult.toSpecification() + // overridden for compatibility with https://github.com/Netflix/dgs-framework/pull/1261. + override fun toSpecification(): MutableMap { + val spec = executionResult.toSpecification() - if (spec["extensions"] != null && extensions.containsKey(DGS_RESPONSE_HEADERS_KEY)) { - val extensions = spec["extensions"] as Map<*, *> + if (spec["extensions"] != null && extensions.containsKey(DGS_RESPONSE_HEADERS_KEY)) { + val extensions = spec["extensions"] as Map<*, *> - if (extensions.size != 1) { - spec["extensions"] = extensions.minus(DGS_RESPONSE_HEADERS_KEY) - } else { - spec.remove("extensions") - } - } + if (extensions.size != 1) { + spec["extensions"] = extensions.minus(DGS_RESPONSE_HEADERS_KEY) + } else { + spec.remove("extensions") + } + } - return spec - } - } \ No newline at end of file + return spec + } +} From 046c132720a24ab8d05bc22e0258cf39d59d3e11 Mon Sep 17 00:00:00 2001 From: Anthony Oleinik Date: Wed, 19 Oct 2022 23:37:19 -0500 Subject: [PATCH 11/11] fixed test expecting 200 when should be 400 --- .../netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt b/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt index f22152dd4..a41026533 100644 --- a/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt +++ b/graphql-dgs-spring-webmvc-autoconfigure/src/test/kotlin/com/netflix/graphql/dgs/webmvc/MalformedQueryContentTest.kt @@ -89,7 +89,7 @@ class MalformedQueryContentTest { .content("{ }") mvc.perform(uriBuilder) - .andExpect(status().isOk) + .andExpect(status().isBadRequest) .andExpect( content().json( """ @@ -98,7 +98,6 @@ class MalformedQueryContentTest { { "message":"GraphQL operations must contain a non-empty `query`.", "locations":[], - "path":[], "extensions":{ "class":"com.netflix.graphql.dgs.exceptions.DgsBadRequestException", "errorType":"BAD_REQUEST"