From ec2603de63341a1a3bba756c8e767c4bd4ea50bd Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Sat, 6 Oct 2012 22:03:31 -0400 Subject: [PATCH] Fix issue in AnnotationMethodHandlerExceptionResolver Caching of resovled exceptions introduced in SPR-7703 also introduced a side effect whereby if exactly one exception was previously cached, any other exception would appear as a match to the previously matched @ExceptionHandler method. This change ensures use of a fresh map when determining matching @ExceptionHandler methods while also updating the cache. Issue: SPR-9209 --- ...otationMethodHandlerExceptionResolver.java | 11 +++++----- ...onMethodHandlerExceptionResolverTests.java | 21 +++++++++++++++++-- ...otationMethodHandlerExceptionResolver.java | 11 +++++----- ...onMethodHandlerExceptionResolverTests.java | 19 +++++++++++++++-- 4 files changed, 48 insertions(+), 14 deletions(-) diff --git a/org.springframework.web.portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java b/org.springframework.web.portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java index 13955c415d..35eb460a22 100644 --- a/org.springframework.web.portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java +++ b/org.springframework.web.portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java @@ -25,6 +25,7 @@ import java.lang.reflect.Method; import java.security.Principal; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -147,7 +148,7 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc exceptionHandlerCache.put(handlerType, handlers); } - final Map, Method> resolverMethods = handlers; + final Map, Method> matchedHandlers = new HashMap, Method>(); ReflectionUtils.doWithMethods(handlerType, new ReflectionUtils.MethodCallback() { public void doWith(Method method) { @@ -155,11 +156,11 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc List> handledExceptions = getHandledExceptions(method); for (Class handledException : handledExceptions) { if (handledException.isAssignableFrom(thrownExceptionType)) { - if (!resolverMethods.containsKey(handledException)) { - resolverMethods.put(handledException, method); + if (!matchedHandlers.containsKey(handledException)) { + matchedHandlers.put(handledException, method); } else { - Method oldMappedMethod = resolverMethods.get(handledException); + Method oldMappedMethod = matchedHandlers.get(handledException); if (!oldMappedMethod.equals(method)) { throw new IllegalStateException( "Ambiguous exception handler mapped for " + handledException + "]: {" + @@ -171,7 +172,7 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc } }); - handlerMethod = getBestMatchingMethod(resolverMethods, thrownException); + handlerMethod = getBestMatchingMethod(matchedHandlers, thrownException); handlers.put(thrownExceptionType, (handlerMethod == null ? NO_METHOD_FOUND : handlerMethod)); return handlerMethod; } diff --git a/org.springframework.web.portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java b/org.springframework.web.portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java index 4dfe1f8681..eb41b5be3f 100644 --- a/org.springframework.web.portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java +++ b/org.springframework.web.portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java @@ -16,17 +16,20 @@ package org.springframework.web.portlet.mvc.annotation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + import java.io.FileNotFoundException; import java.io.IOException; import java.net.BindException; import java.net.SocketException; + import javax.portlet.PortletRequest; import javax.portlet.PortletResponse; -import static org.junit.Assert.*; import org.junit.Before; import org.junit.Test; - import org.springframework.mock.web.portlet.MockRenderRequest; import org.springframework.mock.web.portlet.MockRenderResponse; import org.springframework.stereotype.Controller; @@ -106,6 +109,20 @@ public class AnnotationMethodHandlerExceptionResolverTests { exceptionResolver.resolveException(request, response, controller, ex); } + // SPR-9209 + + @Test + public void cachingSideEffect() { + IllegalArgumentException ex = new IllegalArgumentException(); + SimpleController controller = new SimpleController(); + + ModelAndView mav = exceptionResolver.resolveException(request, response, controller, ex); + assertNotNull("No ModelAndView returned", mav); + + mav = exceptionResolver.resolveException(request, response, controller, new NullPointerException()); + assertNull(mav); + } + @Controller private static class SimpleController { diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java index 2ff7987c9f..012b3c85b8 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java @@ -27,6 +27,7 @@ import java.security.Principal; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -171,7 +172,7 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc exceptionHandlerCache.put(handlerType, handlers); } - final Map, Method> resolverMethods = handlers; + final Map, Method> matchedHandlers = new HashMap, Method>(); ReflectionUtils.doWithMethods(handlerType, new ReflectionUtils.MethodCallback() { public void doWith(Method method) { @@ -179,11 +180,11 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc List> handledExceptions = getHandledExceptions(method); for (Class handledException : handledExceptions) { if (handledException.isAssignableFrom(thrownExceptionType)) { - if (!resolverMethods.containsKey(handledException)) { - resolverMethods.put(handledException, method); + if (!matchedHandlers.containsKey(handledException)) { + matchedHandlers.put(handledException, method); } else { - Method oldMappedMethod = resolverMethods.get(handledException); + Method oldMappedMethod = matchedHandlers.get(handledException); if (!oldMappedMethod.equals(method)) { throw new IllegalStateException( "Ambiguous exception handler mapped for " + handledException + "]: {" + @@ -195,7 +196,7 @@ public class AnnotationMethodHandlerExceptionResolver extends AbstractHandlerExc } }); - handlerMethod = getBestMatchingMethod(resolverMethods, thrownException); + handlerMethod = getBestMatchingMethod(matchedHandlers, thrownException); handlers.put(thrownExceptionType, (handlerMethod == null ? NO_METHOD_FOUND : handlerMethod)); return handlerMethod; } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java index 412ed1b029..b0ffa84762 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java @@ -26,6 +26,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import static org.junit.Assert.*; + import org.junit.Before; import org.junit.Test; @@ -110,7 +111,7 @@ public class AnnotationMethodHandlerExceptionResolverTests { assertEquals("Invalid view name returned", "GenericError", mav.getViewName()); assertEquals("Invalid status code returned", 500, response.getStatus()); } - + @Test(expected = IllegalStateException.class) public void ambiguous() { IllegalArgumentException ex = new IllegalArgumentException(); @@ -127,7 +128,7 @@ public class AnnotationMethodHandlerExceptionResolverTests { assertTrue("ModelAndView not empty", mav.isEmpty()); assertEquals("Invalid response written", "IllegalArgumentException", response.getContentAsString()); } - + @Test public void responseBody() throws UnsupportedEncodingException { IllegalArgumentException ex = new IllegalArgumentException(); @@ -139,6 +140,20 @@ public class AnnotationMethodHandlerExceptionResolverTests { assertEquals("Invalid response written", "IllegalArgumentException", response.getContentAsString()); } + // SPR-9209 + + @Test + public void cachingSideEffect() { + IllegalArgumentException ex = new IllegalArgumentException(); + SimpleController controller = new SimpleController(); + + ModelAndView mav = exceptionResolver.resolveException(request, response, controller, ex); + assertNotNull("No ModelAndView returned", mav); + + mav = exceptionResolver.resolveException(request, response, controller, new NullPointerException()); + assertNull(mav); + } + @Controller private static class SimpleController {