Polishing contribution

See gh-398

Author: rstoyanchev

spring-graphql/src/main/java/org/springframework/graphql/execution/AbstractGraphQlSourceBuilder.java

@@ -16,6 +16,12 @@
 
 package org.springframework.graphql.execution;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Consumer;
+
 import graphql.GraphQL;
 import graphql.execution.instrumentation.ChainedInstrumentation;
 import graphql.execution.instrumentation.Instrumentation;
@@ -24,9 +30,6 @@
 import graphql.schema.GraphQLTypeVisitor;
 import graphql.schema.SchemaTraverser;
 
-import java.util.*;
-import java.util.function.Consumer;
-
 
 /**
  * Implementation of {@link GraphQlSource.Builder} that leaves it to subclasses
@@ -57,8 +60,8 @@ public B exceptionResolvers(List<DataFetcherExceptionResolver> resolvers) {
 	}
 
 	@Override
-	public B subscriptionExceptionResolvers(List<SubscriptionExceptionResolver> subscriptionExceptionResolvers) {
-		this.subscriptionExceptionResolvers.addAll(subscriptionExceptionResolvers);
+	public B subscriptionExceptionResolvers(List<SubscriptionExceptionResolver> resolvers) {
+		this.subscriptionExceptionResolvers.addAll(resolvers);
 		return self();
 	}
 
@@ -110,10 +113,7 @@ public GraphQlSource build() {
 	protected abstract GraphQLSchema initGraphQlSchema();
 
 	private GraphQLSchema applyTypeVisitors(GraphQLSchema schema) {
-		SubscriptionExceptionResolver subscriptionExceptionResolver = new DelegatingSubscriptionExceptionResolver(
-				subscriptionExceptionResolvers);
-		GraphQLTypeVisitor visitor = ContextDataFetcherDecorator.createVisitor(subscriptionExceptionResolver);
-
+		GraphQLTypeVisitor visitor = ContextDataFetcherDecorator.createVisitor(this.subscriptionExceptionResolvers);
 		List<GraphQLTypeVisitor> visitors = new ArrayList<>(this.typeVisitors);
 		visitors.add(visitor);
 

spring-graphql/src/main/java/org/springframework/graphql/execution/DelegatingSubscriptionExceptionResolver.java renamed to spring-graphql/src/main/java/org/springframework/graphql/execution/CompositeSubscriptionExceptionResolver.java

@@ -16,59 +16,66 @@
 
 package org.springframework.graphql.execution;
 
-import graphql.ErrorType;
+import java.util.Collections;
+import java.util.List;
+
 import graphql.GraphQLError;
 import graphql.GraphqlErrorBuilder;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.springframework.util.Assert;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
 
-import java.util.Collections;
-import java.util.List;
+import org.springframework.util.Assert;
 
 /**
- * An implementation of {@link SubscriptionExceptionResolver} that is trying to map exception to GraphQL error
- * using provided implementation of {@link SubscriptionExceptionResolver}.
- * <br/>
- * If none of provided implementations resolve exception to error or if any of implementation throw an exception,
- * this {@link SubscriptionExceptionResolver} will return a default error.
+ * Implementation of {@link SubscriptionExceptionResolver} that is given a list
+ * of other {@link SubscriptionExceptionResolver}'s to invoke in turn until one
+ * returns a list of {@link GraphQLError}'s.
+ *
+ * <p>If the exception remains unresolved, it is mapped to a default error of
+ * type {@link ErrorType#INTERNAL_ERROR} with a generic message.
  *
  * @author Mykyta Ivchenko
- * @see SubscriptionExceptionResolver
+ * @author Rossen Stoyanchev
+ * @since 1.0.1
  */
-public class DelegatingSubscriptionExceptionResolver implements SubscriptionExceptionResolver {
-    private static final Log logger = LogFactory.getLog(DelegatingSubscriptionExceptionResolver.class);
+class CompositeSubscriptionExceptionResolver implements SubscriptionExceptionResolver {
+
+    private static final Log logger = LogFactory.getLog(CompositeSubscriptionExceptionResolver.class);
+
     private final List<SubscriptionExceptionResolver> resolvers;
 
-    public DelegatingSubscriptionExceptionResolver(List<SubscriptionExceptionResolver> resolvers) {
-        Assert.notNull(resolvers, "'resolvers' list must be not null.");
+
+    CompositeSubscriptionExceptionResolver(List<SubscriptionExceptionResolver> resolvers) {
+        Assert.notNull(resolvers, "'resolvers' is required");
         this.resolvers = resolvers;
     }
 
+
     @Override
     public Mono<List<GraphQLError>> resolveException(Throwable exception) {
-        return Flux.fromIterable(resolvers)
+        return Flux.fromIterable(this.resolvers)
                 .flatMap(resolver -> resolver.resolveException(exception))
                 .next()
-                .onErrorResume(error -> Mono.just(handleMappingException(error, exception)))
-                .defaultIfEmpty(createDefaultErrors());
+                .onErrorResume(error -> Mono.just(handleResolverException(error, exception)))
+                .defaultIfEmpty(createDefaultError());
     }
 
-    private List<GraphQLError> handleMappingException(Throwable resolverException, Throwable originalException) {
+    private List<GraphQLError> handleResolverException(
+            Throwable resolverException, Throwable originalException) {
+
         if (logger.isWarnEnabled()) {
             logger.warn("Failure while resolving " + originalException.getClass().getName(), resolverException);
         }
-        return createDefaultErrors();
+        return createDefaultError();
     }
 
-    private List<GraphQLError> createDefaultErrors() {
-        GraphQLError error = GraphqlErrorBuilder.newError()
-                .message("Unknown error")
-                .errorType(ErrorType.DataFetchingException)
-                .build();
-
-        return Collections.singletonList(error);
+    private List<GraphQLError> createDefaultError() {
+        return Collections.singletonList(GraphqlErrorBuilder.newError()
+                .message("Subscription error")
+                .errorType(ErrorType.INTERNAL_ERROR)
+                .build());
     }
+
 }

spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java

@@ -16,17 +16,25 @@
 
 package org.springframework.graphql.execution;
 
+import java.util.List;
+
 import graphql.ExecutionInput;
-import graphql.schema.*;
+import graphql.schema.DataFetcher;
+import graphql.schema.DataFetchingEnvironment;
+import graphql.schema.GraphQLCodeRegistry;
+import graphql.schema.GraphQLFieldDefinition;
+import graphql.schema.GraphQLFieldsContainer;
+import graphql.schema.GraphQLSchemaElement;
+import graphql.schema.GraphQLTypeVisitor;
+import graphql.schema.GraphQLTypeVisitorStub;
 import graphql.util.TraversalControl;
 import graphql.util.TraverserContext;
 import org.reactivestreams.Publisher;
-import org.springframework.util.Assert;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
 import reactor.util.context.ContextView;
 
-import java.util.function.Function;
+import org.springframework.util.Assert;
 
 /**
  * Wrap a {@link DataFetcher} to enable the following:
@@ -35,6 +43,7 @@
  * <li>Support {@link Flux} return value as a shortcut to {@link Flux#collectList()}.
  * <li>Re-establish Reactor Context passed via {@link ExecutionInput}.
  * <li>Re-establish ThreadLocal context passed via {@link ExecutionInput}.
+ * <li>Resolve exceptions from a GraphQL subscription {@link Publisher}.
  * </ul>
  *
  * @author Rossen Stoyanchev
@@ -50,6 +59,7 @@ final class ContextDataFetcherDecorator implements DataFetcher<Object> {
 	private ContextDataFetcherDecorator(
 			DataFetcher<?> delegate, boolean subscription,
 			SubscriptionExceptionResolver subscriptionExceptionResolver) {
+
 		Assert.notNull(delegate, "'delegate' DataFetcher is required");
 		Assert.notNull(subscriptionExceptionResolver, "'subscriptionExceptionResolver' is required");
 		this.delegate = delegate;
@@ -66,8 +76,11 @@ public Object get(DataFetchingEnvironment environment) throws Exception {
 		ContextView contextView = ReactorContextManager.getReactorContext(environment.getGraphQlContext());
 
 		if (this.subscription) {
-			Publisher<?> publisher = interceptSubscriptionPublisherWithExceptionHandler((Publisher<?>) value);
-			return (!contextView.isEmpty() ? Flux.from(publisher).contextWrite(contextView) : publisher);
+			Assert.state(value instanceof Publisher, "Expected Publisher for a subscription");
+			Flux<?> flux = Flux.from((Publisher<?>) value).onErrorResume(exception ->
+					this.subscriptionExceptionResolver.resolveException(exception)
+							.flatMap(errors -> Mono.error(new SubscriptionPublisherException(errors, exception))));
+			return (!contextView.isEmpty() ? flux.contextWrite(contextView) : flux);
 		}
 
 		if (value instanceof Flux) {
@@ -85,29 +98,15 @@ public Object get(DataFetchingEnvironment environment) throws Exception {
 		return value;
 	}
 
-	@SuppressWarnings("unchecked")
-	private Publisher<?> interceptSubscriptionPublisherWithExceptionHandler(Publisher<?> publisher) {
-		Function<? super Throwable, Mono<?>> onErrorResumeFunction = e ->
-				subscriptionExceptionResolver.resolveException(e)
-						.flatMap(errors -> Mono.error(new SubscriptionStreamException(errors)));
-
-		if (publisher instanceof Flux) {
-			return ((Flux<Object>) publisher).onErrorResume(onErrorResumeFunction);
-		}
-
-		if (publisher instanceof Mono) {
-			return ((Mono<Object>) publisher).onErrorResume(onErrorResumeFunction);
-		}
-
-		throw new IllegalArgumentException("Unknown publisher type: '" + publisher.getClass().getName() +"'. " +
-				"Expected reactor.core.publisher.Mono or reactor.core.publisher.Flux");
-	}
 
 	/**
-	 * {@link GraphQLTypeVisitor} that wraps non-GraphQL data fetchers and adapts them if
-	 * they return {@link Flux} or {@link Mono}.
+	 * Static factory method to create {@link GraphQLTypeVisitor} that wraps
+	 * data fetchers with the {@link ContextDataFetcherDecorator}.
 	 */
-	static GraphQLTypeVisitor createVisitor(SubscriptionExceptionResolver subscriptionExceptionResolver) {
+	static GraphQLTypeVisitor createVisitor(List<SubscriptionExceptionResolver> resolvers) {
+
+		SubscriptionExceptionResolver compositeResolver = new CompositeSubscriptionExceptionResolver(resolvers);
+
 		return new GraphQLTypeVisitorStub() {
 			@Override
 			public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition fieldDefinition,
@@ -122,7 +121,7 @@ public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition field
 				}
 
 				boolean handlesSubscription = parent.getName().equals("Subscription");
-				dataFetcher = new ContextDataFetcherDecorator(dataFetcher, handlesSubscription, subscriptionExceptionResolver);
+				dataFetcher = new ContextDataFetcherDecorator(dataFetcher, handlesSubscription, compositeResolver);
 				codeRegistry.dataFetcher(parent, fieldDefinition, dataFetcher);
 				return TraversalControl.CONTINUE;
 			}

spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolver.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2021 the original author or authors.
+ * Copyright 2002-2022 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.
@@ -17,6 +17,7 @@
 package org.springframework.graphql.execution;
 
 import java.util.List;
+import java.util.function.BiFunction;
 
 import graphql.GraphQLError;
 import graphql.schema.DataFetchingEnvironment;
@@ -55,9 +56,31 @@ public interface DataFetcherExceptionResolver {
 	 * @return a {@code Mono} with errors to add to the GraphQL response;
 	 * if the {@code Mono} completes with an empty List, the exception is resolved
 	 * without any errors added to the response; if the {@code Mono} completes
-	 * empty, without emitting a List, the exception remains unresolved and gives
-	 * other resolvers a chance.
+	 * empty, without emitting a List, the exception remains unresolved and that
+	 * allows other resolvers to resolve it.
 	 */
 	Mono<List<GraphQLError>> resolveException(Throwable exception, DataFetchingEnvironment environment);
 
+
+	/**
+	 * Factory method to create a {@link DataFetcherExceptionResolver} to resolve
+	 * an exception to a single GraphQL error. Effectively, a shortcut
+	 * for creating {@link DataFetcherExceptionResolverAdapter} and overriding
+	 * its {@code resolveToSingleError} method.
+	 * @param resolver the resolver function to use
+	 * @return the created instance
+	 * @since 1.0.1
+	 */
+	static DataFetcherExceptionResolverAdapter forSingleError(
+			BiFunction<Throwable, DataFetchingEnvironment, GraphQLError> resolver) {
+
+		return new DataFetcherExceptionResolverAdapter() {
+
+			@Override
+			protected GraphQLError resolveToSingleError(Throwable ex, DataFetchingEnvironment env) {
+				return resolver.apply(ex, env);
+			}
+		};
+	}
+
 }

spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java

@@ -34,14 +34,16 @@
  * <li>{@link #resolveToMultipleErrors}
  * </ul>
  *
- * <p>Use {@link #from(BiFunction)} to create an instance or extend this class
- * and override one of its resolve methods.
+ * <p>Applications may also use
+ * {@link DataFetcherExceptionResolver#forSingleError(BiFunction)} as a shortcut
+ * for {@link #resolveToSingleError(Throwable, DataFetchingEnvironment)}.
  *
  * <p>Implementations can also express interest in ThreadLocal context
  * propagation, from the underlying transport thread, via
  * {@link #setThreadLocalContextAware(boolean)}.
  *
  * @author Rossen Stoyanchev
+ * @since 1.0.0
  */
 public abstract class DataFetcherExceptionResolverAdapter implements DataFetcherExceptionResolver {
 
@@ -57,7 +59,7 @@ protected DataFetcherExceptionResolverAdapter() {
 
 
 	/**
-	 * Sub-classes can set this to indicate that ThreadLocal context from the
+	 * Subclasses can set this to indicate that ThreadLocal context from the
 	 * transport handler (e.g. HTTP handler) should be restored when resolving
 	 * exceptions.
 	 * <p><strong>Note:</strong> This property is applicable only if transports
@@ -129,7 +131,9 @@ protected GraphQLError resolveToSingleError(Throwable ex, DataFetchingEnvironmen
 	 * resolves exceptions with the given {@code BiFunction}.
 	 * @param resolver the resolver function to use
 	 * @return the created instance
+	 * @deprecated as of 1.0.1, please use {@link DataFetcherExceptionResolver#forSingleError(BiFunction)}
 	 */
+	@Deprecated
 	public static DataFetcherExceptionResolverAdapter from(
 			BiFunction<Throwable, DataFetchingEnvironment, GraphQLError> resolver) {
 

spring-graphql/src/main/java/org/springframework/graphql/execution/ExceptionResolversExceptionHandler.java

@@ -41,6 +41,7 @@
  * in a sequence until one returns a list of {@link GraphQLError}'s.
  *
  * @author Rossen Stoyanchev
+ * @since 1.0.0
  */
 class ExceptionResolversExceptionHandler implements DataFetcherExceptionHandler {