Skip to content

Commit 46cbdff

Browse files
jhoellerjhoeller
committed
Consistent ControllerAdvice applicability against user-declared class
Issue: SPR-16496
1 parent 766e602 commit 46cbdff

File tree

2 files changed

+63
-23
lines changed

2 files changed

+63
-23
lines changed

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolver.java

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2018 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.
@@ -17,6 +17,7 @@
1717
package org.springframework.web.servlet.mvc.method.annotation;
1818

1919
import java.lang.reflect.Method;
20+
import java.lang.reflect.Proxy;
2021
import java.util.ArrayList;
2122
import java.util.Collections;
2223
import java.util.LinkedHashMap;
@@ -27,6 +28,7 @@
2728
import javax.servlet.http.HttpServletRequest;
2829
import javax.servlet.http.HttpServletResponse;
2930

31+
import org.springframework.aop.support.AopUtils;
3032
import org.springframework.beans.factory.InitializingBean;
3133
import org.springframework.context.ApplicationContext;
3234
import org.springframework.context.ApplicationContextAware;
@@ -440,13 +442,18 @@ protected ModelAndView doResolveHandlerMethodException(HttpServletRequest reques
440442
* Spring-managed beans were detected.
441443
* @param handlerMethod the method where the exception was raised (may be {@code null})
442444
* @param exception the raised exception
443-
* @return a method to handle the exception, or {@code null}
445+
* @return a method to handle the exception, or {@code null} if none
444446
*/
445447
@Nullable
446-
protected ServletInvocableHandlerMethod getExceptionHandlerMethod(@Nullable HandlerMethod handlerMethod, Exception exception) {
447-
Class<?> handlerType = (handlerMethod != null ? handlerMethod.getBeanType() : null);
448+
protected ServletInvocableHandlerMethod getExceptionHandlerMethod(
449+
@Nullable HandlerMethod handlerMethod, Exception exception) {
450+
451+
Class<?> handlerType = null;
448452

449453
if (handlerMethod != null) {
454+
// Local exception handler methods on the controller class itself.
455+
// To be invoked through the proxy, even in case of an interface-based proxy.
456+
handlerType = handlerMethod.getBeanType();
450457
ExceptionHandlerMethodResolver resolver = this.exceptionHandlerCache.get(handlerType);
451458
if (resolver == null) {
452459
resolver = new ExceptionHandlerMethodResolver(handlerType);
@@ -456,14 +463,20 @@ protected ServletInvocableHandlerMethod getExceptionHandlerMethod(@Nullable Hand
456463
if (method != null) {
457464
return new ServletInvocableHandlerMethod(handlerMethod.getBean(), method);
458465
}
466+
// For advice applicability check below (involving base packages, assignable types
467+
// and annotation presence), use target class instead of interface-based proxy.
468+
if (Proxy.isProxyClass(handlerType)) {
469+
handlerType = AopUtils.getTargetClass(handlerMethod.getBean());
470+
}
459471
}
460472

461473
for (Entry<ControllerAdviceBean, ExceptionHandlerMethodResolver> entry : this.exceptionHandlerAdviceCache.entrySet()) {
462-
if (entry.getKey().isApplicableToBeanType(handlerType)) {
474+
ControllerAdviceBean advice = entry.getKey();
475+
if (advice.isApplicableToBeanType(handlerType)) {
463476
ExceptionHandlerMethodResolver resolver = entry.getValue();
464477
Method method = resolver.resolveMethod(exception);
465478
if (method != null) {
466-
return new ServletInvocableHandlerMethod(entry.getKey().resolveBean(), method);
479+
return new ServletInvocableHandlerMethod(advice.resolveBean(), method);
467480
}
468481
}
469482
}

‎spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ExceptionHandlerExceptionResolverTests.java

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2016 the original author or authors.
2+
* Copyright 2002-2018 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.
@@ -25,6 +25,7 @@
2525
import org.junit.BeforeClass;
2626
import org.junit.Test;
2727

28+
import org.springframework.aop.framework.ProxyFactory;
2829
import org.springframework.beans.FatalBeanException;
2930
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
3031
import org.springframework.context.annotation.Bean;
@@ -38,6 +39,7 @@
3839
import org.springframework.web.bind.annotation.ExceptionHandler;
3940
import org.springframework.web.bind.annotation.ResponseBody;
4041
import org.springframework.web.bind.annotation.RestControllerAdvice;
42+
import org.springframework.web.context.support.WebApplicationObjectSupport;
4143
import org.springframework.web.method.HandlerMethod;
4244
import org.springframework.web.method.annotation.ModelMethodProcessor;
4345
import org.springframework.web.method.support.HandlerMethodArgumentResolver;
@@ -213,8 +215,8 @@ public void resolveRedirectAttributesAtArgument() throws Exception {
213215

214216
@Test
215217
public void resolveExceptionGlobalHandler() throws Exception {
216-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyConfig.class);
217-
this.resolver.setApplicationContext(cxt);
218+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyConfig.class);
219+
this.resolver.setApplicationContext(ctx);
218220
this.resolver.afterPropertiesSet();
219221

220222
IllegalAccessException ex = new IllegalAccessException();
@@ -228,8 +230,8 @@ public void resolveExceptionGlobalHandler() throws Exception {
228230

229231
@Test
230232
public void resolveExceptionGlobalHandlerOrdered() throws Exception {
231-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyConfig.class);
232-
this.resolver.setApplicationContext(cxt);
233+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyConfig.class);
234+
this.resolver.setApplicationContext(ctx);
233235
this.resolver.afterPropertiesSet();
234236

235237
IllegalStateException ex = new IllegalStateException();
@@ -243,8 +245,8 @@ public void resolveExceptionGlobalHandlerOrdered() throws Exception {
243245

244246
@Test // SPR-12605
245247
public void resolveExceptionWithHandlerMethodArg() throws Exception {
246-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyConfig.class);
247-
this.resolver.setApplicationContext(cxt);
248+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyConfig.class);
249+
this.resolver.setApplicationContext(ctx);
248250
this.resolver.afterPropertiesSet();
249251

250252
ArrayIndexOutOfBoundsException ex = new ArrayIndexOutOfBoundsException();
@@ -258,8 +260,8 @@ public void resolveExceptionWithHandlerMethodArg() throws Exception {
258260

259261
@Test
260262
public void resolveExceptionWithAssertionError() throws Exception {
261-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyConfig.class);
262-
this.resolver.setApplicationContext(cxt);
263+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyConfig.class);
264+
this.resolver.setApplicationContext(ctx);
263265
this.resolver.afterPropertiesSet();
264266

265267
AssertionError err = new AssertionError("argh");
@@ -274,8 +276,8 @@ public void resolveExceptionWithAssertionError() throws Exception {
274276

275277
@Test
276278
public void resolveExceptionWithAssertionErrorAsRootCause() throws Exception {
277-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyConfig.class);
278-
this.resolver.setApplicationContext(cxt);
279+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyConfig.class);
280+
this.resolver.setApplicationContext(ctx);
279281
this.resolver.afterPropertiesSet();
280282

281283
AssertionError err = new AssertionError("argh");
@@ -290,8 +292,8 @@ public void resolveExceptionWithAssertionErrorAsRootCause() throws Exception {
290292

291293
@Test
292294
public void resolveExceptionControllerAdviceHandler() throws Exception {
293-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class);
294-
this.resolver.setApplicationContext(cxt);
295+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class);
296+
this.resolver.setApplicationContext(ctx);
295297
this.resolver.afterPropertiesSet();
296298

297299
IllegalStateException ex = new IllegalStateException();
@@ -305,8 +307,8 @@ public void resolveExceptionControllerAdviceHandler() throws Exception {
305307

306308
@Test
307309
public void resolveExceptionControllerAdviceNoHandler() throws Exception {
308-
AnnotationConfigApplicationContext cxt = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class);
309-
this.resolver.setApplicationContext(cxt);
310+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class);
311+
this.resolver.setApplicationContext(ctx);
310312
this.resolver.afterPropertiesSet();
311313

312314
IllegalStateException ex = new IllegalStateException();
@@ -317,6 +319,21 @@ public void resolveExceptionControllerAdviceNoHandler() throws Exception {
317319
assertEquals("DefaultTestExceptionResolver: IllegalStateException", this.response.getContentAsString());
318320
}
319321

322+
@Test // SPR-16496
323+
public void resolveExceptionControllerAdviceAgainstProxy() throws Exception {
324+
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(MyControllerAdviceConfig.class);
325+
this.resolver.setApplicationContext(ctx);
326+
this.resolver.afterPropertiesSet();
327+
328+
IllegalStateException ex = new IllegalStateException();
329+
HandlerMethod handlerMethod = new HandlerMethod(new ProxyFactory(new ResponseBodyController()).getProxy(), "handle");
330+
ModelAndView mav = this.resolver.resolveException(this.request, this.response, handlerMethod, ex);
331+
332+
assertNotNull("Exception was not handled", mav);
333+
assertTrue(mav.isEmpty());
334+
assertEquals("BasePackageTestExceptionResolver: IllegalStateException", this.response.getContentAsString());
335+
}
336+
320337

321338
private void assertMethodProcessorCount(int resolverCount, int handlerCount) {
322339
assertEquals(resolverCount, this.resolver.getArgumentResolvers().getResolvers().size());
@@ -348,8 +365,18 @@ public void handleException(Exception ex, Writer writer) throws IOException {
348365
}
349366

350367

368+
interface ResponseBodyInterface {
369+
370+
void handle();
371+
372+
@ExceptionHandler
373+
@ResponseBody
374+
String handleException(IllegalArgumentException ex);
375+
}
376+
377+
351378
@Controller
352-
static class ResponseBodyController {
379+
static class ResponseBodyController extends WebApplicationObjectSupport implements ResponseBodyInterface {
353380

354381
public void handle() {}
355382

@@ -454,7 +481,7 @@ public String handleException(IllegalStateException ex) {
454481
}
455482

456483

457-
@RestControllerAdvice("org.springframework.web.servlet.mvc.method.annotation")
484+
@RestControllerAdvice(assignableTypes = WebApplicationObjectSupport.class)
458485
@Order(2)
459486
static class BasePackageTestExceptionResolver {
460487

0 commit comments

Comments
 (0)