From bac74616780e83ca9b75014510c51c9d667a7cbf Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 5 Jul 2021 19:35:04 +0100 Subject: [PATCH] Add DataFetcherExceptionResolverAdapter The adapter aims to: - simplify the common case of synchronous resolution to a single error - support ThreadLocal context propagation on an opt-in basis This replaces the SyncDataFetcherExceptionResolver and removes the need to propagate ThreadLocal context to every resolver. --- .../SecurityDataFetcherExceptionResolver.java | 37 +++--- .../src/docs/asciidoc/index.adoc | 5 + .../DataFetcherExceptionResolver.java | 22 ++-- .../DataFetcherExceptionResolverAdapter.java | 114 ++++++++++++++++++ .../ExceptionResolversExceptionHandler.java | 15 +-- .../SyncDataFetcherExceptionResolver.java | 46 ------- ...ceptionResolversExceptionHandlerTests.java | 18 ++- .../graphql/web/WebGraphQlHandlerTests.java | 21 +++- 8 files changed, 186 insertions(+), 92 deletions(-) create mode 100644 spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java delete mode 100644 spring-graphql/src/main/java/org/springframework/graphql/execution/SyncDataFetcherExceptionResolver.java diff --git a/samples/webmvc-http-security/src/main/java/io/spring/sample/graphql/SecurityDataFetcherExceptionResolver.java b/samples/webmvc-http-security/src/main/java/io/spring/sample/graphql/SecurityDataFetcherExceptionResolver.java index 1c357a48..e9b52a8b 100644 --- a/samples/webmvc-http-security/src/main/java/io/spring/sample/graphql/SecurityDataFetcherExceptionResolver.java +++ b/samples/webmvc-http-security/src/main/java/io/spring/sample/graphql/SecurityDataFetcherExceptionResolver.java @@ -1,14 +1,11 @@ package io.spring.sample.graphql; -import java.util.Collections; -import java.util.List; - import graphql.GraphQLError; import graphql.GraphqlErrorBuilder; import graphql.schema.DataFetchingEnvironment; +import org.springframework.graphql.execution.DataFetcherExceptionResolverAdapter; import org.springframework.graphql.execution.ErrorType; -import org.springframework.graphql.execution.SyncDataFetcherExceptionResolver; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.authentication.AuthenticationTrustResolver; import org.springframework.security.authentication.AuthenticationTrustResolverImpl; @@ -20,22 +17,22 @@ import org.springframework.stereotype.Component; import org.springframework.util.Assert; @Component -public class SecurityDataFetcherExceptionResolver implements SyncDataFetcherExceptionResolver { +public class SecurityDataFetcherExceptionResolver extends DataFetcherExceptionResolverAdapter { private AuthenticationTrustResolver authenticationTrustResolver = new AuthenticationTrustResolverImpl(); @Override - public List doResolveException(Throwable exception, DataFetchingEnvironment environment) { - if (exception instanceof AuthenticationException) { - return unauthorized(environment); + protected GraphQLError resolveToSingleError(Throwable ex, DataFetchingEnvironment env) { + if (ex instanceof AuthenticationException) { + return unauthorized(env); } - if (exception instanceof AccessDeniedException) { + if (ex instanceof AccessDeniedException) { SecurityContext context = SecurityContextHolder.getContext(); Authentication authentication = context.getAuthentication(); if (this.authenticationTrustResolver.isAnonymous(authentication)) { - return unauthorized(environment); + return unauthorized(env); } - return forbidden(environment); + return forbidden(env); } return null; } @@ -45,20 +42,18 @@ public class SecurityDataFetcherExceptionResolver implements SyncDataFetcherExce this.authenticationTrustResolver = authenticationTrustResolver; } - private List unauthorized(DataFetchingEnvironment environment) { - return Collections.singletonList( - GraphqlErrorBuilder.newError(environment) - .errorType(ErrorType.UNAUTHORIZED) - .message("Unauthorized") - .build()); + private GraphQLError unauthorized(DataFetchingEnvironment environment) { + return GraphqlErrorBuilder.newError(environment) + .errorType(ErrorType.UNAUTHORIZED) + .message("Unauthorized") + .build(); } - private List forbidden(DataFetchingEnvironment environment) { - return Collections.singletonList( - GraphqlErrorBuilder.newError(environment) + private GraphQLError forbidden(DataFetchingEnvironment environment) { + return GraphqlErrorBuilder.newError(environment) .errorType(ErrorType.FORBIDDEN) .message("Forbidden") - .build()); + .build(); } } diff --git a/spring-graphql-docs/src/docs/asciidoc/index.adoc b/spring-graphql-docs/src/docs/asciidoc/index.adoc index 6b1fa6db..fce3bdff 100644 --- a/spring-graphql-docs/src/docs/asciidoc/index.adoc +++ b/spring-graphql-docs/src/docs/asciidoc/index.adoc @@ -271,6 +271,11 @@ by the <> builder. It enables applications to register more Spring `DataFetcherExceptionResolver` components that are invoked sequentially until one resolves the `Exception` to a 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 GraphQL defines an `ErrorType` enum with common, error classification categories: diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolver.java index a80af1bd..c859e361 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolver.java @@ -23,27 +23,33 @@ import graphql.schema.DataFetchingEnvironment; import reactor.core.publisher.Mono; /** - * Contract for resolving exceptions from {@link graphql.schema.DataFetcher}'s - * to {@code GraphQLError}'s to be added to the GraphQL response, possibly also - * using Spring's {@link graphql.ErrorType} for the error category. + * Contract to resolve exceptions from {@link graphql.schema.DataFetcher}s. + * Implementations are typically declared as beans in Spring configuration and + * are invoked sequentially until one emits a List of {@link GraphQLError}s. * - *

Implementations are typically declared as beans in Spring configuration - * and invoked in order until one emits a List. + *

Most resolver implementations can extend + * {@link DataFetcherExceptionResolverAdapter} and override one of its + * {@link DataFetcherExceptionResolverAdapter#resolveToSingleError resolveToSingleError} or + * {@link DataFetcherExceptionResolverAdapter#resolveToMultipleErrors resolveToMultipleErrors} + * methods that resolve the exception synchronously. + * + *

Resolver implementations can use {@link ErrorType} to classify errors + * using one of several common categories. * * @author Rossen Stoyanchev * @since 1.0.0 - * @see SyncDataFetcherExceptionResolver + * @see ErrorType + * @see DataFetcherExceptionResolverAdapter + * @see ExceptionResolversExceptionHandler */ public interface DataFetcherExceptionResolver { /** * Resolve the given exception and return the error(s) to add to the response. - * *

Implementations can use * {@link graphql.GraphqlErrorBuilder#newError(DataFetchingEnvironment)} to * create an error with the coordinates of the target field, and use * {@link ErrorType} to specify a category for the error. - * * @param exception the exception to resolve * @param environment the environment for the invoked {@code DataFetcher} * @return a {@code Mono} with errors to add to the GraphQL response; diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java new file mode 100644 index 00000000..09b569e4 --- /dev/null +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java @@ -0,0 +1,114 @@ +/* + * Copyright 2002-2021 the original author or authors. + * + * 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 + * + * https://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 org.springframework.graphql.execution; + +import java.util.Collections; +import java.util.List; + +import graphql.GraphQLError; +import graphql.schema.DataFetchingEnvironment; +import reactor.core.publisher.Mono; +import reactor.util.context.ContextView; + +import org.springframework.lang.Nullable; + +/** + * Adapter for {@link DataFetcherExceptionResolver} that pre-implements the + * asynchronous contract and exposes the following synchronous methods: + *

    + *
  • {@link #resolveToSingleError} + *
  • {@link #resolveToMultipleErrors} + *
+ * + *

Implementations can also express interest in ThreadLocal context + * propagation, from the underlying transport thread, via + * {@link #setThreadLocalContextAware(boolean)}. + * + * @author Rossen Stoyanchev + */ +public class DataFetcherExceptionResolverAdapter implements DataFetcherExceptionResolver { + + private boolean threadLocalContextAware; + + + /** + * Sub-classes can set this to indicate that ThreadLocal context from the + * transport handler (e.g. HTTP handler) should be restored when resolving + * exceptions. + *

Note: This property is applicable only if transports + * use ThreadLocal's' (e.g. Spring MVC) and if a {@link ThreadLocalAccessor} + * is registered to extract ThreadLocal values of interest. There is no + * impact from setting this property otherwise. + *

By default this is set to "false" in which case there is no attempt + * to propagate ThreadLocal context. + * @param threadLocalContextAware whether this resolver needs access to + * ThreadLocal context or not. + */ + public void setThreadLocalContextAware(boolean threadLocalContextAware) { + this.threadLocalContextAware = threadLocalContextAware; + } + + /** + * Whether ThreadLocal context needs to be restored for this resolver. + */ + public boolean isThreadLocalContextAware() { + return this.threadLocalContextAware; + } + + @Override + public final Mono> resolveException(Throwable ex, DataFetchingEnvironment env) { + return Mono.defer(() -> Mono.justOrEmpty(resolveInternal(ex, env))); + } + + @Nullable + private List resolveInternal(Throwable ex, DataFetchingEnvironment env) { + if (!this.threadLocalContextAware) { + return resolveToMultipleErrors(ex, env); + } + ContextView contextView = ReactorContextManager.getReactorContext(env); + try { + ReactorContextManager.restoreThreadLocalValues(contextView); + return resolveToMultipleErrors(ex, env); + } + finally { + ReactorContextManager.resetThreadLocalValues(contextView); + } + } + + /** + * Override this method to resolve an Exception to multiple GraphQL errors. + * @param ex the exception to resolve + * @param env the environment for the invoked {@code DataFetcher} + * @return the resolved errors or {@code null} if unresolved + */ + @Nullable + protected List resolveToMultipleErrors(Throwable ex, DataFetchingEnvironment env) { + GraphQLError error = resolveToSingleError(ex, env); + return (error != null ? Collections.singletonList(error) : null); + } + + /** + * Override this method to resolve an Exception to a single GraphQL error. + * @param ex the exception to resolve + * @param env the environment for the invoked {@code DataFetcher} + * @return the resolved error or {@code null} if unresolved + */ + @Nullable + protected GraphQLError resolveToSingleError(Throwable ex, DataFetchingEnvironment env) { + return null; + } + +} 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 4a1dd645..c154aacd 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 @@ -70,7 +70,7 @@ class ExceptionResolversExceptionHandler implements DataFetcherExceptionHandler // https://github.com/graphql-java/graphql-java/issues/2356 try { return Flux.fromIterable(this.resolvers) - .flatMap((resolver) -> resolveErrors(ex, env, resolver)) + .flatMap((resolver) -> resolver.resolveException(ex, env)) .next() .map((errors) -> DataFetcherExceptionHandlerResult.newResult().errors(errors).build()) .switchIfEmpty(Mono.fromCallable(() -> applyDefaultHandling(ex, env))) @@ -89,19 +89,6 @@ class ExceptionResolversExceptionHandler implements DataFetcherExceptionHandler } } - private Mono> resolveErrors( - Throwable ex, DataFetchingEnvironment environment, DataFetcherExceptionResolver resolver) { - - ContextView contextView = ReactorContextManager.getReactorContext(environment); - try { - ReactorContextManager.restoreThreadLocalValues(contextView); - return resolver.resolveException(ex, environment); - } - finally { - ReactorContextManager.resetThreadLocalValues(contextView); - } - } - private DataFetcherExceptionHandlerResult applyDefaultHandling(Throwable ex, DataFetchingEnvironment env) { GraphQLError error = GraphqlErrorBuilder.newError(env) .message(ex.getMessage()) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/SyncDataFetcherExceptionResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SyncDataFetcherExceptionResolver.java deleted file mode 100644 index 7804e44f..00000000 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/SyncDataFetcherExceptionResolver.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright 2002-2021 the original author or authors. - * - * 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 - * - * https://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 org.springframework.graphql.execution; - -import java.util.List; - -import graphql.GraphQLError; -import graphql.schema.DataFetchingEnvironment; -import reactor.core.publisher.Mono; - -/** - * {@link DataFetcherExceptionResolver} that resolves exceptions synchronously. - * - * @author Rossen Stoyanchev - * @since 1.0.0 - */ -public interface SyncDataFetcherExceptionResolver extends DataFetcherExceptionResolver { - - @Override - default Mono> resolveException(Throwable exception, DataFetchingEnvironment env) { - return Mono.just(doResolveException(exception, env)); - } - - /** - * Implement this method to resolve exceptions. - * @param exception the exception to resolve - * @param env the environment for the invoked {@code DataFetcher} - * @return the list of resolved GraphQL errors - */ - List doResolveException(Throwable exception, DataFetchingEnvironment env); - -} 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 1b693859..5b4f9c7e 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 @@ -20,12 +20,14 @@ import java.time.Duration; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.BiFunction; import graphql.ExecutionInput; import graphql.ExecutionResult; import graphql.GraphQL; import graphql.GraphQLError; import graphql.GraphqlErrorBuilder; +import graphql.schema.DataFetchingEnvironment; import org.junit.jupiter.api.Test; import reactor.core.publisher.Mono; import reactor.util.context.Context; @@ -96,7 +98,7 @@ public class ExceptionResolversExceptionHandlerTests { (env) -> { throw new IllegalArgumentException("Invalid greeting"); }, - (SyncDataFetcherExceptionResolver) (ex, env) -> Collections.singletonList( + threadLocalContextAwareExceptionResolver((ex, env) -> GraphqlErrorBuilder.newError(env) .message("Resolved error: " + ex.getMessage() + ", name=" + nameThreadLocal.get()) .errorType(ErrorType.BAD_REQUEST) @@ -155,4 +157,18 @@ public class ExceptionResolversExceptionHandlerTests { assertThat(result.getErrors()).hasSize(0); } + private static DataFetcherExceptionResolver threadLocalContextAwareExceptionResolver( + BiFunction resolver) { + + DataFetcherExceptionResolverAdapter adapter = new DataFetcherExceptionResolverAdapter() { + + @Override + protected GraphQLError resolveToSingleError(Throwable ex, DataFetchingEnvironment env) { + return resolver.apply(ex, env); + } + }; + adapter.setThreadLocalContextAware(true); + return adapter; + } + } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/web/WebGraphQlHandlerTests.java b/spring-graphql/src/test/java/org/springframework/graphql/web/WebGraphQlHandlerTests.java index 0a10fdf6..cda2694b 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/web/WebGraphQlHandlerTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/web/WebGraphQlHandlerTests.java @@ -21,10 +21,12 @@ import java.time.Duration; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.BiFunction; import graphql.GraphQL; import graphql.GraphQLError; import graphql.GraphqlErrorBuilder; +import graphql.schema.DataFetchingEnvironment; import org.junit.jupiter.api.Test; import reactor.core.publisher.Mono; @@ -32,9 +34,10 @@ import org.springframework.graphql.GraphQlService; import org.springframework.graphql.GraphQlTestUtils; import org.springframework.graphql.TestGraphQlSource; import org.springframework.graphql.TestThreadLocalAccessor; +import org.springframework.graphql.execution.DataFetcherExceptionResolver; +import org.springframework.graphql.execution.DataFetcherExceptionResolverAdapter; import org.springframework.graphql.execution.ErrorType; import org.springframework.graphql.execution.ExecutionGraphQlService; -import org.springframework.graphql.execution.SyncDataFetcherExceptionResolver; import org.springframework.http.HttpHeaders; import static org.assertj.core.api.Assertions.assertThat; @@ -127,7 +130,7 @@ public class WebGraphQlHandlerTests { (env) -> { throw new IllegalArgumentException("Invalid greeting"); }, - (SyncDataFetcherExceptionResolver) (ex, env) -> Collections.singletonList( + threadLocalContextAwareExceptionResolver((ex, env) -> GraphqlErrorBuilder.newError(env) .message("Resolved error: " + ex.getMessage() + ", name=" + nameThreadLocal.get()) .errorType(ErrorType.BAD_REQUEST).build())); @@ -149,4 +152,18 @@ public class WebGraphQlHandlerTests { } } + private static DataFetcherExceptionResolver threadLocalContextAwareExceptionResolver( + BiFunction resolver) { + + DataFetcherExceptionResolverAdapter adapter = new DataFetcherExceptionResolverAdapter() { + + @Override + protected GraphQLError resolveToSingleError(Throwable ex, DataFetchingEnvironment env) { + return resolver.apply(ex, env); + } + }; + adapter.setThreadLocalContextAware(true); + return adapter; + } + }