From e43ef3a709402001867d6720bd53e5bb13f28af0 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 25 Jun 2024 16:00:45 +0100 Subject: [PATCH] Polishing in exception handling See gh-996 --- .../AnnotatedControllerExceptionResolver.java | 47 ++++++++++--------- .../DataFetcherExceptionResolverAdapter.java | 13 +++-- 2 files changed, 30 insertions(+), 30 deletions(-) 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 0b814055..6a9709a0 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 @@ -175,7 +175,7 @@ final class AnnotatedControllerExceptionResolver { Throwable ex, DataFetchingEnvironment environment, @Nullable Object controller) { Object controllerOrAdvice = null; - MethodHolder methodHolder = null; + MethodReturnValueAdapter methodReturnValueAdapter = null; Class controllerType = null; if (controller != null) { @@ -183,19 +183,19 @@ final class AnnotatedControllerExceptionResolver { MethodResolver methodResolver = this.controllerCache.get(controllerType); if (methodResolver != null) { controllerOrAdvice = controller; - methodHolder = methodResolver.resolveMethod(ex); + methodReturnValueAdapter = methodResolver.resolveMethod(ex); } else if (logger.isWarnEnabled()) { logger.warn("No registration for controller type: " + controllerType.getName()); } } - if (methodHolder == null) { + if (methodReturnValueAdapter == null) { for (Map.Entry entry : this.controllerAdviceCache.entrySet()) { ControllerAdviceBean advice = entry.getKey(); if (controller == null || advice.isApplicableToBeanType(controllerType)) { - methodHolder = entry.getValue().resolveMethod(ex); - if (methodHolder != null) { + methodReturnValueAdapter = entry.getValue().resolveMethod(ex); + if (methodReturnValueAdapter != null) { controllerOrAdvice = advice.resolveBean(); break; } @@ -203,18 +203,19 @@ final class AnnotatedControllerExceptionResolver { } } - if (methodHolder == null) { + if (methodReturnValueAdapter == null) { return Mono.empty(); } - return invokeExceptionHandler(ex, environment, controllerOrAdvice, methodHolder); + return invokeExceptionHandler(ex, environment, controllerOrAdvice, methodReturnValueAdapter); } private Mono> invokeExceptionHandler( - Throwable exception, DataFetchingEnvironment env, Object controllerOrAdvice, MethodHolder methodHolder) { + Throwable exception, DataFetchingEnvironment env, Object controllerOrAdvice, + MethodReturnValueAdapter methodReturnValueAdapter) { DataFetcherHandlerMethod exceptionHandler = new DataFetcherHandlerMethod( - new HandlerMethod(controllerOrAdvice, methodHolder.getMethod()), this.argumentResolvers, + new HandlerMethod(controllerOrAdvice, methodReturnValueAdapter.getMethod()), this.argumentResolvers, null, null, false); List exceptions = new ArrayList<>(); @@ -236,7 +237,7 @@ final class AnnotatedControllerExceptionResolver { Object result = exceptionHandler.invoke(env, arguments); - return methodHolder.adapt(result, exception); + return methodReturnValueAdapter.adapt(result, exception); } catch (Throwable invocationEx) { // Any other than the original exception (or a cause) is unintended here, @@ -256,17 +257,17 @@ final class AnnotatedControllerExceptionResolver { private static final class MethodResolver { @SuppressWarnings("DataFlowIssue") - private static final MethodHolder NO_MATCH = - new MethodHolder(ReflectionUtils.findMethod(MethodResolver.class, "noMatch")); + private static final MethodReturnValueAdapter NO_MATCH = + new MethodReturnValueAdapter(ReflectionUtils.findMethod(MethodResolver.class, "noMatch")); - private final Map, MethodHolder> exceptionMappings = new HashMap<>(16); + private final Map, MethodReturnValueAdapter> exceptionMappings = new HashMap<>(16); - private final Map, MethodHolder> resolvedExceptionCache = new ConcurrentReferenceHashMap<>(16); + private final Map, MethodReturnValueAdapter> resolvedExceptionCache = new ConcurrentReferenceHashMap<>(16); MethodResolver(Map, Method> methodMap) { methodMap.forEach((exceptionType, method) -> - this.exceptionMappings.put(exceptionType, new MethodHolder(method))); + this.exceptionMappings.put(exceptionType, new MethodReturnValueAdapter(method))); } /** @@ -276,8 +277,8 @@ final class AnnotatedControllerExceptionResolver { * @return the exception handler to use, or {@code null} if no match */ @Nullable - MethodHolder resolveMethod(Throwable exception) { - MethodHolder method = resolveMethodByExceptionType(exception.getClass()); + MethodReturnValueAdapter resolveMethod(Throwable exception) { + MethodReturnValueAdapter method = resolveMethodByExceptionType(exception.getClass()); if (method == null) { Throwable cause = exception.getCause(); if (cause != null) { @@ -288,8 +289,8 @@ final class AnnotatedControllerExceptionResolver { } @Nullable - private MethodHolder resolveMethodByExceptionType(Class exceptionType) { - MethodHolder method = this.resolvedExceptionCache.get(exceptionType); + private MethodReturnValueAdapter resolveMethodByExceptionType(Class exceptionType) { + MethodReturnValueAdapter method = this.resolvedExceptionCache.get(exceptionType); if (method == null) { method = getMappedMethod(exceptionType); this.resolvedExceptionCache.put(exceptionType, method); @@ -297,7 +298,7 @@ final class AnnotatedControllerExceptionResolver { return (method != NO_MATCH) ? method : null; } - private MethodHolder getMappedMethod(Class exceptionType) { + private MethodReturnValueAdapter getMappedMethod(Class exceptionType) { List> matches = new ArrayList<>(); for (Class mappedException : this.exceptionMappings.keySet()) { if (mappedException.isAssignableFrom(exceptionType)) { @@ -323,9 +324,9 @@ final class AnnotatedControllerExceptionResolver { /** - * Container for an exception handler method, and an adapter for its return values. + * Helps to adapt the return value of an exception handler method. */ - private static class MethodHolder { + private static class MethodReturnValueAdapter { private final Method method; @@ -333,7 +334,7 @@ final class AnnotatedControllerExceptionResolver { private final ReturnValueAdapter adapter; - MethodHolder(Method method) { + MethodReturnValueAdapter(Method method) { Assert.notNull(method, "Method is required"); this.method = method; this.returnType = new MethodParameter(method, -1); 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 index 0c60bdac..a8e74d12 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/DataFetcherExceptionResolverAdapter.java @@ -96,16 +96,15 @@ public abstract class DataFetcherExceptionResolverAdapter implements DataFetcher @Nullable @SuppressWarnings("deprecation") private List resolveInternal(Throwable exception, DataFetchingEnvironment env) { - if (!this.threadLocalContextAware) { - return resolveToMultipleErrors(exception, env); - } try { - return ContextSnapshot.captureFrom(env.getGraphQlContext()) - .wrap(() -> resolveToMultipleErrors(exception, env)) - .call(); + return (this.threadLocalContextAware) ? + ContextSnapshot.captureFrom(env.getGraphQlContext()) + .wrap(() -> resolveToMultipleErrors(exception, env)) + .call() : + resolveToMultipleErrors(exception, env); } catch (Exception ex2) { - this.logger.warn("Failed to resolve " + exception, ex2); + this.logger.warn("Failure while resolving " + exception.getMessage(), ex2); return null; } }