From 6dba688cec2b98beaa2785a310bfe1623e43232f Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 14 Mar 2023 07:15:54 +0000 Subject: [PATCH] Annotated exception handlers yield correctly Closes gh-160 --- .../AnnotatedControllerConfigurer.java | 13 +++-- .../AnnotatedControllerExceptionResolver.java | 14 +++--- ...tatedControllerExceptionResolverTests.java | 5 +- .../support/SchemaMappingInvocationTests.java | 49 ++++++++++++++++--- 4 files changed, 60 insertions(+), 21 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java index f6e12842..3f5a4206 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java @@ -62,7 +62,6 @@ import org.springframework.format.FormatterRegistrar; import org.springframework.format.support.DefaultFormattingConversionService; import org.springframework.format.support.FormattingConversionService; import org.springframework.graphql.data.GraphQlArgumentBinder; -import org.springframework.graphql.execution.SelfDescribingDataFetcher; import org.springframework.graphql.data.method.HandlerMethod; import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; import org.springframework.graphql.data.method.HandlerMethodArgumentResolverComposite; @@ -71,6 +70,7 @@ import org.springframework.graphql.data.method.annotation.SchemaMapping; import org.springframework.graphql.execution.BatchLoaderRegistry; import org.springframework.graphql.execution.DataFetcherExceptionResolver; import org.springframework.graphql.execution.RuntimeWiringConfigurer; +import org.springframework.graphql.execution.SelfDescribingDataFetcher; import org.springframework.graphql.execution.SubscriptionPublisherException; import org.springframework.lang.Nullable; import org.springframework.stereotype.Controller; @@ -617,18 +617,21 @@ public class AnnotatedControllerConfigurer return result; } - private Mono> handleException( + private Mono> handleException( Throwable ex, DataFetchingEnvironment env, DataFetcherHandlerMethod handlerMethod) { return this.exceptionResolver.resolveException(ex, env, handlerMethod.getBean()) - .map(errors -> DataFetcherResult.newResult().errors(errors).build()); + .map(errors -> DataFetcherResult.newResult().errors(errors).build()) + .switchIfEmpty(Mono.error(ex)); } + @SuppressWarnings("unchecked") private Publisher handleSubscriptionError( Throwable ex, DataFetchingEnvironment env, DataFetcherHandlerMethod handlerMethod) { - return this.exceptionResolver.resolveException(ex, env, handlerMethod.getBean()) - .flatMap(errors -> Mono.error(new SubscriptionPublisherException(errors, ex))); + return (Publisher) this.exceptionResolver.resolveException(ex, env, handlerMethod.getBean()) + .flatMap(errors -> Mono.error(new SubscriptionPublisherException(errors, ex))) + .switchIfEmpty(Mono.error(ex)); } @Override diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolver.java index 60e2fbe3..53f672f4 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolver.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolver.java @@ -160,7 +160,7 @@ final class AnnotatedControllerExceptionResolver { * @param ex the exception to resolve * @param environment the environment for the invoked {@code DataFetcher} * @param controller the controller that raised the exception, if applicable - * @return a {@code Mono} with errors as specified in + * @return a {@code Mono} with resolved {@code GraphQLError}s as specified in * {@link DataFetcherExceptionResolver#resolveException(Throwable, DataFetchingEnvironment)} */ public Mono> resolveException( @@ -194,7 +194,7 @@ final class AnnotatedControllerExceptionResolver { } if (methodHolder == null) { - return Mono.error(ex); + return Mono.empty(); } return invokeExceptionHandler(ex, environment, controllerOrAdvice, methodHolder); @@ -405,13 +405,13 @@ final class AnnotatedControllerExceptionResolver { /** Adapter for a single GraphQLError */ ReturnValueAdapter forSingleError = (result, returnType, ex) -> (result == null ? - Mono.error(ex) : + Mono.empty() : Mono.just(Collections.singletonList((GraphQLError) result))); /** Adapter for a collection of GraphQLError's */ ReturnValueAdapter forCollection = (result, returnType, ex) -> (result == null ? - Mono.error(ex) : + Mono.empty() : Mono.just((result instanceof List ? (List) result : new ArrayList<>((Collection) result)))); @@ -419,7 +419,7 @@ final class AnnotatedControllerExceptionResolver { /** Adapter for Object */ ReturnValueAdapter forObject = (result, returnType, ex) -> { if (result == null) { - return Mono.error(ex); + return Mono.empty(); } else if (result instanceof GraphQLError) { return forSingleError.adapt(result, returnType, ex); @@ -438,12 +438,12 @@ final class AnnotatedControllerExceptionResolver { /** Adapter for {@code Mono} */ ReturnValueAdapter forMonoVoid = (result, returnType, ex) -> - (result == null ? Mono.error(ex) : Mono.just(Collections.emptyList())); + (result == null ? Mono.empty() : Mono.just(Collections.emptyList())); /** Adapter for a {@code Mono} wrapping any of the other synchronous return value types */ ReturnValueAdapter forMono = (result, returnType, ex) -> (result == null ? - Mono.error(ex) : + Mono.empty() : ((Mono) result).flatMap(o -> forObject.adapt(o, returnType, ex)).switchIfEmpty(Mono.error(ex))); } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolverTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolverTests.java index 910858e6..a6bdd1b3 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolverTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolverTests.java @@ -102,9 +102,8 @@ public class AnnotatedControllerExceptionResolverTests { AnnotatedControllerExceptionResolver resolver = exceptionResolver(); resolver.registerController(controller.getClass()); - StepVerifier.create(resolver.resolveException(ex, this.environment, controller)) - .expectErrorSatisfies(actualEx -> assertThat(actualEx).isSameAs(ex)) - .verify(); + StepVerifier.create(resolver.resolveException(ex, this.environment, controller)).verifyComplete(); + StepVerifier.create(resolver.resolveException(ex, this.environment, controller)).verifyComplete(); } @Test diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingInvocationTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingInvocationTests.java index bc9f8c18..22e4c5a6 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingInvocationTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingInvocationTests.java @@ -15,10 +15,12 @@ */ package org.springframework.graphql.data.method.annotation.support; +import java.util.Collections; import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BiConsumer; import graphql.GraphQLContext; import graphql.GraphQLError; @@ -49,11 +51,11 @@ import org.springframework.graphql.data.method.annotation.QueryMapping; import org.springframework.graphql.data.method.annotation.SchemaMapping; import org.springframework.graphql.data.method.annotation.SubscriptionMapping; import org.springframework.graphql.execution.BatchLoaderRegistry; +import org.springframework.graphql.execution.DataFetcherExceptionResolver; import org.springframework.graphql.execution.DefaultBatchLoaderRegistry; import org.springframework.graphql.execution.ErrorType; import org.springframework.graphql.execution.SubscriptionPublisherException; import org.springframework.stereotype.Controller; -import org.springframework.util.Assert; import static org.assertj.core.api.Assertions.assertThat; @@ -229,6 +231,34 @@ public class SchemaMappingInvocationTests { assertThat(responseHelper.error(0).message()).isEqualTo("Rejected: Bad input"); } + @Test + void handleExceptionWithResolverWhenNoAnnotatedExceptionHandlerMatches() { + String document = "{ " + + " booksByCriteria(criteria: {author:\"Heller\"}) { " + + " id" + + " name" + + " }" + + "}"; + + DataFetcherExceptionResolver resolver = (ex, env) -> + Mono.just(Collections.singletonList( + GraphQLError.newError().errorType(ErrorType.INTERNAL_ERROR) + .message("Rejected: " + ex.getMessage()) + .build())); + + ExecutionGraphQlService service = graphQlService((configurer, setup) -> { + setup.exceptionResolver(configurer.getExceptionResolver()); // First @ControllerAdvice (no match) + setup.exceptionResolver(resolver); // Then resolver + }); + + Mono responseMono = service.execute(TestExecutionRequest.forDocument(document)); + + ResponseHelper responseHelper = ResponseHelper.forResponse(responseMono); + assertThat(responseHelper.errorCount()).isEqualTo(1); + assertThat(responseHelper.error(0).errorType()).isEqualTo("INTERNAL_ERROR"); + assertThat(responseHelper.error(0).message()).isEqualTo("Rejected: Fetch failure"); + } + @Test void handleExceptionFromSubscription() { String document = "subscription { " + @@ -257,6 +287,10 @@ public class SchemaMappingInvocationTests { private ExecutionGraphQlService graphQlService() { + return graphQlService((configurer, setup) -> {}); + } + + private ExecutionGraphQlService graphQlService(BiConsumer consumer) { BatchLoaderRegistry registry = new DefaultBatchLoaderRegistry(); AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); @@ -269,10 +303,10 @@ public class SchemaMappingInvocationTests { configurer.setApplicationContext(context); configurer.afterPropertiesSet(); - return GraphQlSetup.schemaResource(BookSource.schema) - .runtimeWiring(configurer) - .dataLoaders(registry) - .toGraphQlService(); + GraphQlSetup setup = GraphQlSetup.schemaResource(BookSource.schema).runtimeWiring(configurer); + consumer.accept(configurer, setup); + + return setup.dataLoaders(registry).toGraphQlService(); } @@ -292,7 +326,10 @@ public class SchemaMappingInvocationTests { @QueryMapping public List booksByCriteria(@Argument BookCriteria criteria) { - Assert.isTrue(!criteria.getAuthor().equalsIgnoreCase("Fitzgerald"), "Bad input"); + switch (criteria.getAuthor()) { + case "Fitzgerald" -> throw new IllegalArgumentException("Bad input"); + case "Heller" -> throw new IllegalStateException("Fetch failure"); + } return BookSource.findBooksByAuthor(criteria.getAuthor()); }