Skip to content

Commit ec2603d

Browse files
rstoyanchevrstoyanchev
committed
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
1 parent 470c85a commit ec2603d

4 files changed

Lines changed: 48 additions & 14 deletions

File tree

‎org.springframework.web.portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.security.Principal;
2626
import java.util.ArrayList;
2727
import java.util.Arrays;
28+
import java.util.HashMap;
2829
import java.util.List;
2930
import java.util.Locale;
3031
import java.util.Map;
@@ -147,19 +148,19 @@ private Method findBestExceptionHandlerMethod(Object handler, final Exception th
147148
exceptionHandlerCache.put(handlerType, handlers);
148149
}
149150

150-
final Map<Class<? extends Throwable>, Method> resolverMethods = handlers;
151+
final Map<Class<? extends Throwable>, Method> matchedHandlers = new HashMap<Class<? extends Throwable>, Method>();
151152

152153
ReflectionUtils.doWithMethods(handlerType, new ReflectionUtils.MethodCallback() {
153154
public void doWith(Method method) {
154155
method = ClassUtils.getMostSpecificMethod(method, handlerType);
155156
List<Class<? extends Throwable>> handledExceptions = getHandledExceptions(method);
156157
for (Class<? extends Throwable> handledException : handledExceptions) {
157158
if (handledException.isAssignableFrom(thrownExceptionType)) {
158-
if (!resolverMethods.containsKey(handledException)) {
159-
resolverMethods.put(handledException, method);
159+
if (!matchedHandlers.containsKey(handledException)) {
160+
matchedHandlers.put(handledException, method);
160161
}
161162
else {
162-
Method oldMappedMethod = resolverMethods.get(handledException);
163+
Method oldMappedMethod = matchedHandlers.get(handledException);
163164
if (!oldMappedMethod.equals(method)) {
164165
throw new IllegalStateException(
165166
"Ambiguous exception handler mapped for " + handledException + "]: {" +
@@ -171,7 +172,7 @@ public void doWith(Method method) {
171172
}
172173
});
173174

174-
handlerMethod = getBestMatchingMethod(resolverMethods, thrownException);
175+
handlerMethod = getBestMatchingMethod(matchedHandlers, thrownException);
175176
handlers.put(thrownExceptionType, (handlerMethod == null ? NO_METHOD_FOUND : handlerMethod));
176177
return handlerMethod;
177178
}

‎org.springframework.web.portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,20 @@
1616

1717
package org.springframework.web.portlet.mvc.annotation;
1818

19+
import static org.junit.Assert.assertEquals;
20+
import static org.junit.Assert.assertNotNull;
21+
import static org.junit.Assert.assertNull;
22+
1923
import java.io.FileNotFoundException;
2024
import java.io.IOException;
2125
import java.net.BindException;
2226
import java.net.SocketException;
27+
2328
import javax.portlet.PortletRequest;
2429
import javax.portlet.PortletResponse;
2530

26-
import static org.junit.Assert.*;
2731
import org.junit.Before;
2832
import org.junit.Test;
29-
3033
import org.springframework.mock.web.portlet.MockRenderRequest;
3134
import org.springframework.mock.web.portlet.MockRenderResponse;
3235
import org.springframework.stereotype.Controller;
@@ -106,6 +109,20 @@ public void ambiguous() {
106109
exceptionResolver.resolveException(request, response, controller, ex);
107110
}
108111

112+
// SPR-9209
113+
114+
@Test
115+
public void cachingSideEffect() {
116+
IllegalArgumentException ex = new IllegalArgumentException();
117+
SimpleController controller = new SimpleController();
118+
119+
ModelAndView mav = exceptionResolver.resolveException(request, response, controller, ex);
120+
assertNotNull("No ModelAndView returned", mav);
121+
122+
mav = exceptionResolver.resolveException(request, response, controller, new NullPointerException());
123+
assertNull(mav);
124+
}
125+
109126

110127
@Controller
111128
private static class SimpleController {

‎org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolver.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.ArrayList;
2828
import java.util.Arrays;
2929
import java.util.Collections;
30+
import java.util.HashMap;
3031
import java.util.List;
3132
import java.util.Locale;
3233
import java.util.Map;
@@ -171,19 +172,19 @@ private Method findBestExceptionHandlerMethod(Object handler, final Exception th
171172
exceptionHandlerCache.put(handlerType, handlers);
172173
}
173174

174-
final Map<Class<? extends Throwable>, Method> resolverMethods = handlers;
175+
final Map<Class<? extends Throwable>, Method> matchedHandlers = new HashMap<Class<? extends Throwable>, Method>();
175176

176177
ReflectionUtils.doWithMethods(handlerType, new ReflectionUtils.MethodCallback() {
177178
public void doWith(Method method) {
178179
method = ClassUtils.getMostSpecificMethod(method, handlerType);
179180
List<Class<? extends Throwable>> handledExceptions = getHandledExceptions(method);
180181
for (Class<? extends Throwable> handledException : handledExceptions) {
181182
if (handledException.isAssignableFrom(thrownExceptionType)) {
182-
if (!resolverMethods.containsKey(handledException)) {
183-
resolverMethods.put(handledException, method);
183+
if (!matchedHandlers.containsKey(handledException)) {
184+
matchedHandlers.put(handledException, method);
184185
}
185186
else {
186-
Method oldMappedMethod = resolverMethods.get(handledException);
187+
Method oldMappedMethod = matchedHandlers.get(handledException);
187188
if (!oldMappedMethod.equals(method)) {
188189
throw new IllegalStateException(
189190
"Ambiguous exception handler mapped for " + handledException + "]: {" +
@@ -195,7 +196,7 @@ public void doWith(Method method) {
195196
}
196197
});
197198

198-
handlerMethod = getBestMatchingMethod(resolverMethods, thrownException);
199+
handlerMethod = getBestMatchingMethod(matchedHandlers, thrownException);
199200
handlers.put(thrownExceptionType, (handlerMethod == null ? NO_METHOD_FOUND : handlerMethod));
200201
return handlerMethod;
201202
}

‎org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerExceptionResolverTests.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import javax.servlet.http.HttpServletResponse;
2727

2828
import static org.junit.Assert.*;
29+
2930
import org.junit.Before;
3031
import org.junit.Test;
3132

@@ -110,7 +111,7 @@ public void inherited() {
110111
assertEquals("Invalid view name returned", "GenericError", mav.getViewName());
111112
assertEquals("Invalid status code returned", 500, response.getStatus());
112113
}
113-
114+
114115
@Test(expected = IllegalStateException.class)
115116
public void ambiguous() {
116117
IllegalArgumentException ex = new IllegalArgumentException();
@@ -127,7 +128,7 @@ public void noModelAndView() throws UnsupportedEncodingException {
127128
assertTrue("ModelAndView not empty", mav.isEmpty());
128129
assertEquals("Invalid response written", "IllegalArgumentException", response.getContentAsString());
129130
}
130-
131+
131132
@Test
132133
public void responseBody() throws UnsupportedEncodingException {
133134
IllegalArgumentException ex = new IllegalArgumentException();
@@ -139,6 +140,20 @@ public void responseBody() throws UnsupportedEncodingException {
139140
assertEquals("Invalid response written", "IllegalArgumentException", response.getContentAsString());
140141
}
141142

143+
// SPR-9209
144+
145+
@Test
146+
public void cachingSideEffect() {
147+
IllegalArgumentException ex = new IllegalArgumentException();
148+
SimpleController controller = new SimpleController();
149+
150+
ModelAndView mav = exceptionResolver.resolveException(request, response, controller, ex);
151+
assertNotNull("No ModelAndView returned", mav);
152+
153+
mav = exceptionResolver.resolveException(request, response, controller, new NullPointerException());
154+
assertNull(mav);
155+
}
156+
142157

143158
@Controller
144159
private static class SimpleController {

0 commit comments

Comments
 (0)