Skip to content

Commit 96da1ff

Browse files
sbrannensbrannen
committed
Support @RestControllerAdvice in Standalone MockMvc again
Since Spring Framework 5.2, @RestControllerAdvice registered with MockMvc when using MockMvcBuilders.standaloneSetup() has no longer been properly supported if annotation attributes were declared in the @RestControllerAdvice annotation. Prior to 5.2, this was not an issue. The cause for this regression is two-fold. 1. Commit 50c2577 refactored DefaultListableBeanFactory so that findAnnotationOnBean() supports merged annotations; however, that commit did not refactor StaticListableBeanFactory#findAnnotationOnBean() to support merged annotations. 2. Commit 978adbd refactored ControllerAdviceBean so that a merged @ControllerAdvice annotation is only looked up via ApplicationContext#findAnnotationOnBean(). The latter relies on the fact that findAnnotationOnBean() supports merged annotations (e.g., @RestControllerAdvice as a merged instance of @ControllerAdvice). Behind the scenes, MockMvcBuilders.standaloneSetup() creates a StubWebApplicationContext which internally uses a StubBeanFactory which extends StaticListableBeanFactory. Consequently, since the implementation of findAnnotationOnBean() in StaticListableBeanFactory was not updated to support merged annotations like it was in DefaultListableBeanFactory, we only see this regression with the standalone MockMvc support and not with MockMvc support for an existing WebApplicationContext or with standard Spring applications using an ApplicationContext that uses DefaultListableBeanFactory. This commit fixes this regression by supporting merged annotations in StaticListableBeanFactory#findAnnotationOnBean() as well. Closes gh-25520
1 parent 40fc472 commit 96da1ff

File tree

3 files changed

+204
-15
lines changed

3 files changed

+204
-15
lines changed

‎spring-beans/src/main/java/org/springframework/beans/factory/support/StaticListableBeanFactory.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
import org.springframework.beans.factory.SmartFactoryBean;
3838
import org.springframework.core.OrderComparator;
3939
import org.springframework.core.ResolvableType;
40-
import org.springframework.core.annotation.AnnotationUtils;
40+
import org.springframework.core.annotation.AnnotatedElementUtils;
4141
import org.springframework.lang.Nullable;
4242
import org.springframework.util.Assert;
4343
import org.springframework.util.ObjectUtils;
@@ -450,7 +450,7 @@ public <A extends Annotation> A findAnnotationOnBean(String beanName, Class<A> a
450450
throws NoSuchBeanDefinitionException {
451451

452452
Class<?> beanType = getType(beanName);
453-
return (beanType != null ? AnnotationUtils.findAnnotation(beanType, annotationType) : null);
453+
return (beanType != null ? AnnotatedElementUtils.findMergedAnnotation(beanType, annotationType) : null);
454454
}
455455

456456
}

‎spring-beans/src/test/java/org/springframework/beans/factory/BeanFactoryUtilsTests.java

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package org.springframework.beans.factory;
1818

19+
import java.lang.annotation.Retention;
20+
import java.lang.annotation.RetentionPolicy;
1921
import java.util.Arrays;
2022
import java.util.List;
2123
import java.util.Map;
@@ -33,6 +35,7 @@
3335
import org.springframework.beans.testfixture.beans.TestBean;
3436
import org.springframework.beans.testfixture.beans.factory.DummyFactory;
3537
import org.springframework.cglib.proxy.NoOp;
38+
import org.springframework.core.annotation.AliasFor;
3639
import org.springframework.core.io.Resource;
3740
import org.springframework.util.ObjectUtils;
3841

@@ -324,6 +327,33 @@ public void testIntDependencies() {
324327
assertThat(Arrays.equals(new String[] { "buffer" }, deps)).isTrue();
325328
}
326329

330+
@Test
331+
public void findAnnotationOnBean() {
332+
this.listableBeanFactory.registerSingleton("controllerAdvice", new ControllerAdviceClass());
333+
this.listableBeanFactory.registerSingleton("restControllerAdvice", new RestControllerAdviceClass());
334+
testFindAnnotationOnBean(this.listableBeanFactory);
335+
}
336+
337+
@Test // gh-25520
338+
public void findAnnotationOnBeanWithStaticFactory() {
339+
StaticListableBeanFactory lbf = new StaticListableBeanFactory();
340+
lbf.addBean("controllerAdvice", new ControllerAdviceClass());
341+
lbf.addBean("restControllerAdvice", new RestControllerAdviceClass());
342+
testFindAnnotationOnBean(lbf);
343+
}
344+
345+
private void testFindAnnotationOnBean(ListableBeanFactory lbf) {
346+
assertControllerAdvice(lbf, "controllerAdvice");
347+
assertControllerAdvice(lbf, "restControllerAdvice");
348+
}
349+
350+
private void assertControllerAdvice(ListableBeanFactory lbf, String beanName) {
351+
ControllerAdvice controllerAdvice = lbf.findAnnotationOnBean(beanName, ControllerAdvice.class);
352+
assertThat(controllerAdvice).isNotNull();
353+
assertThat(controllerAdvice.value()).isEqualTo("com.example");
354+
assertThat(controllerAdvice.basePackage()).isEqualTo("com.example");
355+
}
356+
327357
@Test
328358
public void isSingletonAndIsPrototypeWithStaticFactory() {
329359
StaticListableBeanFactory lbf = new StaticListableBeanFactory();
@@ -393,6 +423,35 @@ public void isSingletonAndIsPrototypeWithStaticFactory() {
393423
}
394424

395425

426+
@Retention(RetentionPolicy.RUNTIME)
427+
@interface ControllerAdvice {
428+
429+
@AliasFor("basePackage")
430+
String value() default "";
431+
432+
@AliasFor("value")
433+
String basePackage() default "";
434+
}
435+
436+
@Retention(RetentionPolicy.RUNTIME)
437+
@ControllerAdvice
438+
@interface RestControllerAdvice {
439+
440+
@AliasFor(annotation = ControllerAdvice.class)
441+
String value() default "";
442+
443+
@AliasFor(annotation = ControllerAdvice.class)
444+
String basePackage() default "";
445+
}
446+
447+
@ControllerAdvice("com.example")
448+
static class ControllerAdviceClass {
449+
}
450+
451+
@RestControllerAdvice("com.example")
452+
static class RestControllerAdviceClass {
453+
}
454+
396455
static class TestBeanSmartFactoryBean implements SmartFactoryBean<TestBean> {
397456

398457
private final TestBean testBean = new TestBean("enigma", 42);

‎spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/ExceptionHandlerTests.java

Lines changed: 143 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,23 @@
1616

1717
package org.springframework.test.web.servlet.samples.standalone;
1818

19+
import org.junit.jupiter.api.Nested;
1920
import org.junit.jupiter.api.Test;
2021

22+
import org.springframework.core.Ordered;
23+
import org.springframework.core.annotation.Order;
24+
import org.springframework.http.MediaType;
2125
import org.springframework.stereotype.Controller;
2226
import org.springframework.web.bind.annotation.ControllerAdvice;
2327
import org.springframework.web.bind.annotation.ExceptionHandler;
2428
import org.springframework.web.bind.annotation.GetMapping;
2529
import org.springframework.web.bind.annotation.PathVariable;
30+
import org.springframework.web.bind.annotation.RestController;
31+
import org.springframework.web.bind.annotation.RestControllerAdvice;
2632

2733
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
2834
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.forwardedUrl;
35+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
2936
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
3037
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup;
3138

@@ -37,28 +44,32 @@
3744
*/
3845
class ExceptionHandlerTests {
3946

40-
@Test
41-
void localExceptionHandlerMethod() throws Exception {
42-
standaloneSetup(new PersonController()).build()
43-
.perform(get("/person/Clyde"))
47+
@Nested
48+
class MvcTests {
49+
50+
@Test
51+
void localExceptionHandlerMethod() throws Exception {
52+
standaloneSetup(new PersonController()).build()
53+
.perform(get("/person/Clyde"))
4454
.andExpect(status().isOk())
4555
.andExpect(forwardedUrl("errorView"));
46-
}
56+
}
4757

48-
@Test
49-
void globalExceptionHandlerMethod() throws Exception {
50-
standaloneSetup(new PersonController()).setControllerAdvice(new GlobalExceptionHandler()).build()
58+
@Test
59+
void globalExceptionHandlerMethod() throws Exception {
60+
standaloneSetup(new PersonController()).setControllerAdvice(new GlobalExceptionHandler()).build()
5161
.perform(get("/person/Bonnie"))
5262
.andExpect(status().isOk())
5363
.andExpect(forwardedUrl("globalErrorView"));
54-
}
64+
}
5565

56-
@Test
57-
void globalExceptionHandlerMethodUsingClassArgument() throws Exception {
58-
standaloneSetup(PersonController.class).setControllerAdvice(GlobalExceptionHandler.class).build()
66+
@Test
67+
void globalExceptionHandlerMethodUsingClassArgument() throws Exception {
68+
standaloneSetup(PersonController.class).setControllerAdvice(GlobalExceptionHandler.class).build()
5969
.perform(get("/person/Bonnie"))
6070
.andExpect(status().isOk())
6171
.andExpect(forwardedUrl("globalErrorView"));
72+
}
6273
}
6374

6475

@@ -82,7 +93,6 @@ String handleException(IllegalArgumentException exception) {
8293
}
8394
}
8495

85-
8696
@ControllerAdvice
8797
private static class GlobalExceptionHandler {
8898

@@ -92,4 +102,124 @@ String handleException(IllegalStateException exception) {
92102
}
93103
}
94104

105+
106+
@Nested
107+
class RestTests {
108+
109+
@Test
110+
void noException() throws Exception {
111+
standaloneSetup(RestPersonController.class)
112+
.setControllerAdvice(RestGlobalExceptionHandler.class, RestPersonControllerExceptionHandler.class).build()
113+
.perform(get("/person/Yoda").accept(MediaType.APPLICATION_JSON))
114+
.andExpect(status().isOk())
115+
.andExpect(jsonPath("$.name").value("Yoda"));
116+
}
117+
118+
@Test
119+
void localExceptionHandlerMethod() throws Exception {
120+
standaloneSetup(RestPersonController.class)
121+
.setControllerAdvice(RestGlobalExceptionHandler.class, RestPersonControllerExceptionHandler.class).build()
122+
.perform(get("/person/Luke").accept(MediaType.APPLICATION_JSON))
123+
.andExpect(status().isOk())
124+
.andExpect(jsonPath("$.error").value("local - IllegalArgumentException"));
125+
}
126+
127+
@Test
128+
void globalExceptionHandlerMethod() throws Exception {
129+
standaloneSetup(RestPersonController.class)
130+
.setControllerAdvice(RestGlobalExceptionHandler.class).build()
131+
.perform(get("/person/Leia").accept(MediaType.APPLICATION_JSON))
132+
.andExpect(status().isOk())
133+
.andExpect(jsonPath("$.error").value("global - IllegalStateException"));
134+
}
135+
136+
@Test
137+
void globalRestPersonControllerExceptionHandlerTakesPrecedenceOverGlobalExceptionHandler() throws Exception {
138+
standaloneSetup(RestPersonController.class)
139+
.setControllerAdvice(RestGlobalExceptionHandler.class, RestPersonControllerExceptionHandler.class).build()
140+
.perform(get("/person/Leia").accept(MediaType.APPLICATION_JSON))
141+
.andExpect(status().isOk())
142+
.andExpect(jsonPath("$.error").value("globalPersonController - IllegalStateException"));
143+
}
144+
145+
@Test // gh-25520
146+
void noHandlerFound() throws Exception {
147+
standaloneSetup(RestPersonController.class)
148+
.setControllerAdvice(RestGlobalExceptionHandler.class, RestPersonControllerExceptionHandler.class)
149+
.addDispatcherServletCustomizer(dispatcherServlet -> dispatcherServlet.setThrowExceptionIfNoHandlerFound(true))
150+
.build()
151+
.perform(get("/bogus").accept(MediaType.APPLICATION_JSON))
152+
.andExpect(status().isOk())
153+
.andExpect(jsonPath("$.error").value("global - NoHandlerFoundException"));
154+
}
155+
}
156+
157+
158+
@RestController
159+
private static class RestPersonController {
160+
161+
@GetMapping("/person/{name}")
162+
Person get(@PathVariable String name) {
163+
switch (name) {
164+
case "Luke":
165+
throw new IllegalArgumentException();
166+
case "Leia":
167+
throw new IllegalStateException();
168+
default:
169+
return new Person("Yoda");
170+
}
171+
}
172+
173+
@ExceptionHandler
174+
Error handleException(IllegalArgumentException exception) {
175+
return new Error("local - " + exception.getClass().getSimpleName());
176+
}
177+
}
178+
179+
@RestControllerAdvice(assignableTypes = RestPersonController.class)
180+
@Order(Ordered.HIGHEST_PRECEDENCE)
181+
private static class RestPersonControllerExceptionHandler {
182+
183+
@ExceptionHandler
184+
Error handleException(Throwable exception) {
185+
return new Error("globalPersonController - " + exception.getClass().getSimpleName());
186+
}
187+
}
188+
189+
@RestControllerAdvice
190+
@Order(Ordered.LOWEST_PRECEDENCE)
191+
private static class RestGlobalExceptionHandler {
192+
193+
@ExceptionHandler
194+
Error handleException(Throwable exception) {
195+
return new Error( "global - " + exception.getClass().getSimpleName());
196+
}
197+
}
198+
199+
static class Person {
200+
201+
private final String name;
202+
203+
Person(String name) {
204+
this.name = name;
205+
}
206+
207+
public String getName() {
208+
return name;
209+
}
210+
}
211+
212+
static class Error {
213+
214+
private final String error;
215+
216+
Error(String error) {
217+
this.error = error;
218+
}
219+
220+
public String getError() {
221+
return error;
222+
}
223+
}
224+
95225
}

0 commit comments

Comments
 (0)