Skip to content

Commit 53108d0

Browse files
committed
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
1 parent 43a76c4 commit 53108d0

File tree

2 files changed

+41
-6
lines changed

2 files changed

+41
-6
lines changed

spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolver.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -21,9 +21,9 @@
2121
import java.util.Collection;
2222
import java.util.Collections;
2323
import java.util.HashMap;
24+
import java.util.LinkedHashMap;
2425
import java.util.List;
2526
import java.util.Map;
26-
import java.util.TreeMap;
2727
import java.util.concurrent.ConcurrentHashMap;
2828

2929
import graphql.GraphQLError;
@@ -71,6 +71,7 @@
7171
* non-controller {@link graphql.schema.DataFetcher}s.
7272
*
7373
* @author Rossen Stoyanchev
74+
* @author Brian Clozel
7475
* @since 1.2.0
7576
*/
7677
final class AnnotatedControllerExceptionResolver {
@@ -82,7 +83,7 @@ final class AnnotatedControllerExceptionResolver {
8283

8384
private final Map<Class<?>, MethodResolver> controllerCache = new ConcurrentHashMap<>(64);
8485

85-
private final Map<ControllerAdviceBean, MethodResolver> controllerAdviceCache = new TreeMap<>(OrderComparator.INSTANCE);
86+
private final Map<ControllerAdviceBean, MethodResolver> controllerAdviceCache = new LinkedHashMap<>();
8687

8788

8889
AnnotatedControllerExceptionResolver(HandlerMethodArgumentResolverComposite resolvers) {
@@ -110,15 +111,19 @@ public void registerController(Class<?> controllerType) {
110111
* @param context the context to look into
111112
*/
112113
public void registerControllerAdvice(ApplicationContext context) {
114+
Map<ControllerAdviceBean, MethodResolver> detectedControllerAdvice = new HashMap<>();
113115
for (ControllerAdviceBean bean : ControllerAdviceBean.findAnnotatedBeans(context)) {
114116
Class<?> beanType = bean.getBeanType();
115117
if (beanType != null) {
116118
Map<Class<? extends Throwable>, Method> methods = findExceptionHandlers(beanType);
117119
if (!methods.isEmpty()) {
118-
this.controllerAdviceCache.put(bean, new MethodResolver(methods));
120+
detectedControllerAdvice.put(bean, new MethodResolver(methods));
119121
}
120122
}
121123
}
124+
detectedControllerAdvice.keySet().stream().sorted(OrderComparator.INSTANCE).forEach(bean -> {
125+
this.controllerAdviceCache.put(bean, detectedControllerAdvice.get(bean));
126+
});
122127
if (logger.isDebugEnabled()) {
123128
logger.debug("@GraphQlException methods in ControllerAdvice beans: " +
124129
(this.controllerAdviceCache.size() == 0 ? "none" : this.controllerAdviceCache.size()));

spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerExceptionResolverTests.java

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2024 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -47,7 +47,7 @@
4747
* Unit tests for {@link AnnotatedControllerExceptionResolver}.
4848
*
4949
* @author Rossen Stoyanchev
50-
* @since 1.2.0
50+
* @author Brian Clozel
5151
*/
5252
public class AnnotatedControllerExceptionResolverTests {
5353

@@ -137,6 +137,25 @@ void resolveWithOrderedControllerAdvice() {
137137
assertThat(actual.get(0).getMessage()).isEqualTo("ordered handle: Bad input");
138138
}
139139

140+
@Test
141+
void resolveWithMultipleControllerAdviceAtSameOrder() {
142+
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
143+
context.register(TestControllerAdvice.class);
144+
context.register(OtherTestControllerAdvice.class);
145+
context.refresh();
146+
147+
Exception ex = new IllegalArgumentException("Bad input");
148+
List<GraphQLError> actual = exceptionResolver(context).resolveException(ex, this.environment, null).block();
149+
assertThat(actual).hasSize(1);
150+
assertThat(actual.get(0).getMessage()).isEqualTo("handle: Bad input");
151+
152+
ex = new IllegalStateException("Bad state");
153+
actual = exceptionResolver(context).resolveException(ex, this.environment, null).block();
154+
assertThat(actual).hasSize(1);
155+
assertThat(actual.get(0).getMessage()).isEqualTo("handle: Bad state");
156+
}
157+
158+
140159
@Test
141160
void invalidReturnType() {
142161
assertThatIllegalStateException().isThrownBy(() ->
@@ -250,6 +269,17 @@ GraphQLError handle(IllegalArgumentException ex) {
250269

251270
}
252271

272+
@SuppressWarnings("unused")
273+
@ControllerAdvice
274+
private static class OtherTestControllerAdvice {
275+
276+
@GraphQlExceptionHandler
277+
GraphQLError handle(IllegalStateException ex) {
278+
return GraphQLError.newError().message("handle: " + ex.getMessage()).build();
279+
}
280+
281+
}
282+
253283

254284
@SuppressWarnings("unused")
255285
@ControllerAdvice

0 commit comments

Comments
 (0)