From 304fb45fb7b7096e474a4dba8c1855238b7c92f0 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 13 Apr 2022 17:09:10 +0100 Subject: [PATCH] Refine handling of unresolved exceptions Closes gh-352 --- .../src/docs/asciidoc/index.adoc | 24 ++++++++++++------- .../ExceptionResolversExceptionHandler.java | 7 +++--- ...ceptionResolversExceptionHandlerTests.java | 4 ++-- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/spring-graphql-docs/src/docs/asciidoc/index.adoc b/spring-graphql-docs/src/docs/asciidoc/index.adoc index a08b66db..b4d81f8f 100644 --- a/spring-graphql-docs/src/docs/asciidoc/index.adoc +++ b/spring-graphql-docs/src/docs/asciidoc/index.adoc @@ -514,21 +514,23 @@ added by <> components. [[execution-exceptions]] === Exception Resolution -GraphQL Java applications can register a `DataFetcherExceptionHandler` to decide how to +A GraphQL Java application can register a `DataFetcherExceptionHandler` to decide how to represent exceptions from the data layer in the "errors" section of the GraphQL response. Spring for GraphQL has a built-in `DataFetcherExceptionHandler` that is configured for use -by the <> builder. It enables applications to register one or -more Spring `DataFetcherExceptionResolver` components that are invoked sequentially -until one resolves the `Exception` to a list of `graphql.GraphQLError` objects. +by the default <> builder. It allows applications to register +one or more Spring `DataFetcherExceptionResolver` components that are invoked sequentially +until one resolves the `Exception` to a (possibly empty) list of `graphql.GraphQLError` +objects. `DataFetcherExceptionResolver` is an asynchronous contract. For most implementations, it would be sufficient to extend `DataFetcherExceptionResolverAdapter` and override one of its `resolveToSingleError` or `resolveToMultipleErrors` methods that resolve exceptions synchronously. -A `GraphQLError` can be assigned an `graphql.ErrorClassification`. Spring for GraphQL -defines an `ErrorType` enum with common, error classification categories: +A `GraphQLError` can be assigned to a category via `graphql.ErrorClassification`. +In Spring GraphQL, you can also assign via `ErrorType` which has the following common +classifications that applications can use to categorize errors: - `BAD_REQUEST` - `UNAUTHORIZED` @@ -536,8 +538,14 @@ defines an `ErrorType` enum with common, error classification categories: - `NOT_FOUND` - `INTERNAL_ERROR` -Applications can use this to classify errors. If an error remains unresolved, by -default it is marked as `INTERNAL_ERROR`. +If an exception remains unresolved, by default it is categorized as an `INTERNAL_ERROR` +with a generic message that includes the category name and the `executionId` from +`DataFetchingEnvironment`. The message is intentionally opaque to avoid leaking +implementation details. Applications can use a `DataFetcherExceptionResolver` to customize +error details. + +Unresolved exception are logged at ERROR level along with the `executionId` to correlate +to the error sent to the client. Resolved exceptions are logged at DEBUG level. diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandler.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandler.java index e3cd38df..024f9312 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandler.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandler.java @@ -35,7 +35,6 @@ import reactor.core.publisher.Mono; import reactor.util.context.ContextView; import org.springframework.util.Assert; -import org.springframework.util.StringUtils; /** * {@link DataFetcherExceptionHandler} that invokes {@link DataFetcherExceptionResolver}'s @@ -111,14 +110,14 @@ class ExceptionResolversExceptionHandler implements DataFetcherExceptionHandler } private DataFetcherExceptionHandlerResult createInternalError(Throwable ex, DataFetchingEnvironment environment) { + ExecutionId executionId = environment.getExecutionId(); if (logger.isErrorEnabled()) { - ExecutionId id = environment.getExecutionId(); - logger.error("Unresolved " + ex.getClass().getSimpleName() + ", executionId= " + id, ex); + logger.error("Unresolved " + ex.getClass().getSimpleName() + " for executionId " + executionId, ex); } return DataFetcherExceptionHandlerResult .newResult(GraphqlErrorBuilder.newError(environment) .errorType(ErrorType.INTERNAL_ERROR) - .message((StringUtils.hasText(ex.getMessage()) ? ex.getMessage() : ex.getClass().getSimpleName())) + .message(ErrorType.INTERNAL_ERROR + " for " + executionId) .build()) .build(); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandlerTests.java b/spring-graphql/src/test/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandlerTests.java index 39668c3c..dae423be 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandlerTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandlerTests.java @@ -27,8 +27,8 @@ import reactor.core.publisher.Mono; import reactor.util.context.Context; import reactor.util.context.ContextView; -import org.springframework.graphql.ResponseHelper; import org.springframework.graphql.GraphQlSetup; +import org.springframework.graphql.ResponseHelper; import org.springframework.graphql.TestThreadLocalAccessor; import static org.assertj.core.api.Assertions.assertThat; @@ -126,7 +126,7 @@ public class ExceptionResolversExceptionHandlerTests { ResponseHelper response = ResponseHelper.forResult(result); assertThat(response.errorCount()).isEqualTo(1); - assertThat(response.error(0).message()).isEqualTo("Invalid greeting"); + assertThat(response.error(0).message()).startsWith("INTERNAL_ERROR for "); assertThat(response.error(0).errorType()).isEqualTo("INTERNAL_ERROR"); String greeting = response.rawValue("greeting");