Fix ControllerAdvice ordering support
Prior to this commit, the `ControllerAdvice` support would detect all beans and exception handler methods, to put them in an ordered `TreeMap` using the `OrderComparator.INSTANCE`. Doing so would naturally consider beans with the same order as duplicates and would write a single entry in the map. This effectively ignored many `ControllerAdvice` beans with the same order (but one). This commit removes the use of a `TreeMap` and instead uses a `LinkedHashMap` and the insertion order for proper ordering. Fixes gh-901
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
/*
|
||||
* Copyright 2002-2023 the original author or authors.
|
||||
* Copyright 2002-2024 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.
|
||||
@@ -21,9 +21,9 @@ import java.util.Arrays;
|
||||
import java.util.Collection;
|
||||
import java.util.Collections;
|
||||
import java.util.HashMap;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.TreeMap;
|
||||
import java.util.concurrent.ConcurrentHashMap;
|
||||
|
||||
import graphql.GraphQLError;
|
||||
@@ -71,6 +71,7 @@ import org.springframework.web.method.ControllerAdviceBean;
|
||||
* non-controller {@link graphql.schema.DataFetcher}s.
|
||||
*
|
||||
* @author Rossen Stoyanchev
|
||||
* @author Brian Clozel
|
||||
* @since 1.2.0
|
||||
*/
|
||||
final class AnnotatedControllerExceptionResolver {
|
||||
@@ -82,7 +83,7 @@ final class AnnotatedControllerExceptionResolver {
|
||||
|
||||
private final Map<Class<?>, MethodResolver> controllerCache = new ConcurrentHashMap<>(64);
|
||||
|
||||
private final Map<ControllerAdviceBean, MethodResolver> controllerAdviceCache = new TreeMap<>(OrderComparator.INSTANCE);
|
||||
private final Map<ControllerAdviceBean, MethodResolver> controllerAdviceCache = new LinkedHashMap<>();
|
||||
|
||||
|
||||
AnnotatedControllerExceptionResolver(HandlerMethodArgumentResolverComposite resolvers) {
|
||||
@@ -110,15 +111,19 @@ final class AnnotatedControllerExceptionResolver {
|
||||
* @param context the context to look into
|
||||
*/
|
||||
public void registerControllerAdvice(ApplicationContext context) {
|
||||
Map<ControllerAdviceBean, MethodResolver> detectedControllerAdvice = new HashMap<>();
|
||||
for (ControllerAdviceBean bean : ControllerAdviceBean.findAnnotatedBeans(context)) {
|
||||
Class<?> beanType = bean.getBeanType();
|
||||
if (beanType != null) {
|
||||
Map<Class<? extends Throwable>, Method> methods = findExceptionHandlers(beanType);
|
||||
if (!methods.isEmpty()) {
|
||||
this.controllerAdviceCache.put(bean, new MethodResolver(methods));
|
||||
detectedControllerAdvice.put(bean, new MethodResolver(methods));
|
||||
}
|
||||
}
|
||||
}
|
||||
detectedControllerAdvice.keySet().stream().sorted(OrderComparator.INSTANCE).forEach(bean -> {
|
||||
this.controllerAdviceCache.put(bean, detectedControllerAdvice.get(bean));
|
||||
});
|
||||
if (logger.isDebugEnabled()) {
|
||||
logger.debug("@GraphQlException methods in ControllerAdvice beans: " +
|
||||
(this.controllerAdviceCache.size() == 0 ? "none" : this.controllerAdviceCache.size()));
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
/*
|
||||
* Copyright 2002-2023 the original author or authors.
|
||||
* Copyright 2002-2024 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.
|
||||
@@ -47,7 +47,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
|
||||
* Unit tests for {@link AnnotatedControllerExceptionResolver}.
|
||||
*
|
||||
* @author Rossen Stoyanchev
|
||||
* @since 1.2.0
|
||||
* @author Brian Clozel
|
||||
*/
|
||||
public class AnnotatedControllerExceptionResolverTests {
|
||||
|
||||
@@ -137,6 +137,25 @@ public class AnnotatedControllerExceptionResolverTests {
|
||||
assertThat(actual.get(0).getMessage()).isEqualTo("ordered handle: Bad input");
|
||||
}
|
||||
|
||||
@Test
|
||||
void resolveWithMultipleControllerAdviceAtSameOrder() {
|
||||
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
|
||||
context.register(TestControllerAdvice.class);
|
||||
context.register(OtherTestControllerAdvice.class);
|
||||
context.refresh();
|
||||
|
||||
Exception ex = new IllegalArgumentException("Bad input");
|
||||
List<GraphQLError> actual = exceptionResolver(context).resolveException(ex, this.environment, null).block();
|
||||
assertThat(actual).hasSize(1);
|
||||
assertThat(actual.get(0).getMessage()).isEqualTo("handle: Bad input");
|
||||
|
||||
ex = new IllegalStateException("Bad state");
|
||||
actual = exceptionResolver(context).resolveException(ex, this.environment, null).block();
|
||||
assertThat(actual).hasSize(1);
|
||||
assertThat(actual.get(0).getMessage()).isEqualTo("handle: Bad state");
|
||||
}
|
||||
|
||||
|
||||
@Test
|
||||
void invalidReturnType() {
|
||||
assertThatIllegalStateException().isThrownBy(() ->
|
||||
@@ -250,6 +269,17 @@ public class AnnotatedControllerExceptionResolverTests {
|
||||
|
||||
}
|
||||
|
||||
@SuppressWarnings("unused")
|
||||
@ControllerAdvice
|
||||
private static class OtherTestControllerAdvice {
|
||||
|
||||
@GraphQlExceptionHandler
|
||||
GraphQLError handle(IllegalStateException ex) {
|
||||
return GraphQLError.newError().message("handle: " + ex.getMessage()).build();
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
|
||||
@SuppressWarnings("unused")
|
||||
@ControllerAdvice
|
||||
|
||||
Reference in New Issue
Block a user