Skip to content

Commit f78bef9

Browse files
rstoyanchevrstoyanchev
committed
Improve no-match handling for @RequestMapping methods
Issue: SPR-9603
1 parent 7704d76 commit f78bef9

File tree

2 files changed

+97
-38
lines changed

2 files changed

+97
-38
lines changed

‎org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java

Lines changed: 59 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,10 @@ protected Set<String> getMappingPathPatterns(RequestMappingInfo info) {
5454
}
5555

5656
/**
57-
* Check if the given RequestMappingInfo matches the current request and
58-
* return a (potentially new) instance with conditions that match the
57+
* Check if the given RequestMappingInfo matches the current request and
58+
* return a (potentially new) instance with conditions that match the
5959
* current request -- for example with a subset of URL patterns.
60-
* @returns an info in case of a match; or {@code null} otherwise.
60+
* @returns an info in case of a match; or {@code null} otherwise.
6161
*/
6262
@Override
6363
protected RequestMappingInfo getMatchingMapping(RequestMappingInfo info, HttpServletRequest request) {
@@ -88,7 +88,7 @@ protected void handleMatch(RequestMappingInfo info, String lookupPath, HttpServl
8888
Set<String> patterns = info.getPatternsCondition().getPatterns();
8989
String bestPattern = patterns.isEmpty() ? lookupPath : patterns.iterator().next();
9090
request.setAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE, bestPattern);
91-
91+
9292
Map<String, String> uriTemplateVariables = getPathMatcher().extractUriTemplateVariables(bestPattern, lookupPath);
9393
request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVariables);
9494

@@ -101,40 +101,57 @@ protected void handleMatch(RequestMappingInfo info, String lookupPath, HttpServl
101101
/**
102102
* Iterate all RequestMappingInfos once again, look if any match by URL at
103103
* least and raise exceptions accordingly.
104-
*
105-
* @throws HttpRequestMethodNotSupportedException
104+
*
105+
* @throws HttpRequestMethodNotSupportedException
106106
* if there are matches by URL but not by HTTP method
107-
* @throws HttpMediaTypeNotAcceptableException
107+
* @throws HttpMediaTypeNotAcceptableException
108108
* if there are matches by URL but not by consumable media types
109-
* @throws HttpMediaTypeNotAcceptableException
109+
* @throws HttpMediaTypeNotAcceptableException
110110
* if there are matches by URL but not by producible media types
111111
*/
112112
@Override
113-
protected HandlerMethod handleNoMatch(Set<RequestMappingInfo> requestMappingInfos,
114-
String lookupPath,
115-
HttpServletRequest request) throws ServletException {
113+
protected HandlerMethod handleNoMatch(Set<RequestMappingInfo> requestMappingInfos,
114+
String lookupPath, HttpServletRequest request) throws ServletException {
115+
116116
Set<String> allowedMethods = new HashSet<String>(6);
117-
Set<MediaType> consumableMediaTypes = new HashSet<MediaType>();
118-
Set<MediaType> producibleMediaTypes = new HashSet<MediaType>();
117+
118+
Set<RequestMappingInfo> patternMatches = new HashSet<RequestMappingInfo>();
119+
Set<RequestMappingInfo> patternAndMethodMatches = new HashSet<RequestMappingInfo>();
120+
119121
for (RequestMappingInfo info : requestMappingInfos) {
120122
if (info.getPatternsCondition().getMatchingCondition(request) != null) {
121-
if (info.getMethodsCondition().getMatchingCondition(request) == null) {
123+
patternMatches.add(info);
124+
if (info.getMethodsCondition().getMatchingCondition(request) != null) {
125+
patternAndMethodMatches.add(info);
126+
}
127+
else {
122128
for (RequestMethod method : info.getMethodsCondition().getMethods()) {
123129
allowedMethods.add(method.name());
124130
}
125131
}
126-
if (info.getConsumesCondition().getMatchingCondition(request) == null) {
127-
consumableMediaTypes.addAll(info.getConsumesCondition().getConsumableMediaTypes());
128-
}
129-
if (info.getProducesCondition().getMatchingCondition(request) == null) {
130-
producibleMediaTypes.addAll(info.getProducesCondition().getProducibleMediaTypes());
131-
}
132132
}
133133
}
134-
if (!allowedMethods.isEmpty()) {
134+
135+
if (patternMatches.isEmpty()) {
136+
return null;
137+
}
138+
else if (patternAndMethodMatches.isEmpty() && !allowedMethods.isEmpty()) {
135139
throw new HttpRequestMethodNotSupportedException(request.getMethod(), allowedMethods);
136140
}
137-
else if (!consumableMediaTypes.isEmpty()) {
141+
142+
Set<MediaType> consumableMediaTypes;
143+
Set<MediaType> producibleMediaTypes;
144+
145+
if (patternAndMethodMatches.isEmpty()) {
146+
consumableMediaTypes = getConsumableMediaTypes(request, patternMatches);
147+
producibleMediaTypes = getProdicubleMediaTypes(request, patternMatches);
148+
}
149+
else {
150+
consumableMediaTypes = getConsumableMediaTypes(request, patternAndMethodMatches);
151+
producibleMediaTypes = getProdicubleMediaTypes(request, patternAndMethodMatches);
152+
}
153+
154+
if (!consumableMediaTypes.isEmpty()) {
138155
MediaType contentType = null;
139156
if (StringUtils.hasLength(request.getContentType())) {
140157
contentType = MediaType.parseMediaType(request.getContentType());
@@ -149,4 +166,24 @@ else if (!producibleMediaTypes.isEmpty()) {
149166
}
150167
}
151168

169+
private Set<MediaType> getConsumableMediaTypes(HttpServletRequest request, Set<RequestMappingInfo> partialMatches) {
170+
Set<MediaType> result = new HashSet<MediaType>();
171+
for (RequestMappingInfo partialMatch : partialMatches) {
172+
if (partialMatch.getConsumesCondition().getMatchingCondition(request) == null) {
173+
result.addAll(partialMatch.getConsumesCondition().getConsumableMediaTypes());
174+
}
175+
}
176+
return result;
177+
}
178+
179+
private Set<MediaType> getProdicubleMediaTypes(HttpServletRequest request, Set<RequestMappingInfo> partialMatches) {
180+
Set<MediaType> result = new HashSet<MediaType>();
181+
for (RequestMappingInfo partialMatch : partialMatches) {
182+
if (partialMatch.getProducesCondition().getMatchingCondition(request) == null) {
183+
result.addAll(partialMatch.getProducesCondition().getProducibleMediaTypes());
184+
}
185+
}
186+
return result;
187+
}
188+
152189
}

‎org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ public void bestMatch() throws Exception {
131131
HandlerMethod hm = (HandlerMethod) this.mapping.getHandler(request).getHandler();
132132
assertEquals(this.fooParamMethod.getMethod(), hm.getMethod());
133133
}
134-
134+
135135
@Test
136136
public void requestMethodNotAllowed() throws Exception {
137137
try {
@@ -143,7 +143,18 @@ public void requestMethodNotAllowed() throws Exception {
143143
assertArrayEquals("Invalid supported methods", new String[]{"GET", "HEAD"}, ex.getSupportedMethods());
144144
}
145145
}
146-
146+
147+
// SPR-9603
148+
149+
@Test(expected=HttpMediaTypeNotAcceptableException.class)
150+
public void requestMethodMatchFalsePositive() throws Exception {
151+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/users");
152+
request.addHeader("Accept", "application/xml");
153+
154+
this.mapping.registerHandler(new UserController());
155+
this.mapping.getHandler(request);
156+
}
157+
147158
@Test
148159
public void mediaTypeNotSupported() throws Exception {
149160
testMediaTypeNotSupported("/person/1");
@@ -159,11 +170,11 @@ private void testMediaTypeNotSupported(String url) throws Exception {
159170
fail("HttpMediaTypeNotSupportedException expected");
160171
}
161172
catch (HttpMediaTypeNotSupportedException ex) {
162-
assertEquals("Invalid supported consumable media types",
173+
assertEquals("Invalid supported consumable media types",
163174
Arrays.asList(new MediaType("application", "xml")), ex.getSupportedMediaTypes());
164175
}
165176
}
166-
177+
167178
@Test
168179
public void mediaTypeNotAccepted() throws Exception {
169180
testMediaTypeNotAccepted("/persons");
@@ -179,7 +190,7 @@ private void testMediaTypeNotAccepted(String url) throws Exception {
179190
fail("HttpMediaTypeNotAcceptableException expected");
180191
}
181192
catch (HttpMediaTypeNotAcceptableException ex) {
182-
assertEquals("Invalid supported producible media types",
193+
assertEquals("Invalid supported producible media types",
183194
Arrays.asList(new MediaType("application", "xml")), ex.getSupportedMediaTypes());
184195
}
185196
}
@@ -191,12 +202,12 @@ public void uriTemplateVariables() {
191202
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
192203
String lookupPath = new UrlPathHelper().getLookupPathForRequest(request);
193204
this.mapping.handleMatch(key, lookupPath, request);
194-
205+
195206
@SuppressWarnings("unchecked")
196-
Map<String, String> uriVariables =
207+
Map<String, String> uriVariables =
197208
(Map<String, String>) request.getAttribute(
198209
HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE);
199-
210+
200211
assertNotNull(uriVariables);
201212
assertEquals("1", uriVariables.get("path1"));
202213
assertEquals("2", uriVariables.get("path2"));
@@ -209,7 +220,7 @@ public void bestMatchingPatternAttribute() {
209220
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
210221

211222
this.mapping.handleMatch(key, "/1/2", request);
212-
223+
213224
assertEquals("/{path1}/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
214225
}
215226

@@ -220,7 +231,7 @@ public void bestMatchingPatternAttributeNoPatternsDefined() {
220231
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
221232

222233
this.mapping.handleMatch(key, "/1/2", request);
223-
234+
224235
assertEquals("/1/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
225236
}
226237

@@ -237,10 +248,10 @@ public void producibleMediaTypesAttribute() throws Exception {
237248
request.addHeader("Accept", "application/json");
238249
this.mapping.getHandler(request);
239250

240-
assertNull("Negated expression should not be listed as a producible type",
251+
assertNull("Negated expression should not be listed as a producible type",
241252
request.getAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE));
242253
}
243-
254+
244255
@Test
245256
public void mappedInterceptors() throws Exception {
246257
String path = "/foo";
@@ -261,14 +272,13 @@ public void mappedInterceptors() throws Exception {
261272
assertNull(chain);
262273
}
263274

264-
@SuppressWarnings("unused")
265275
@Controller
266276
private static class Handler {
267277

268278
@RequestMapping(value = "/foo", method = RequestMethod.GET)
269279
public void foo() {
270280
}
271-
281+
272282
@RequestMapping(value = "/foo", method = RequestMethod.GET, params="p")
273283
public void fooParam() {
274284
}
@@ -301,12 +311,24 @@ public String nonXmlContent() {
301311
}
302312
}
303313

314+
@Controller
315+
private static class UserController {
316+
317+
@RequestMapping(value = "/users", method = RequestMethod.GET, produces = "application/json")
318+
public void getUser() {
319+
}
320+
321+
@RequestMapping(value = "/users", method = RequestMethod.PUT)
322+
public void saveUser() {
323+
}
324+
}
325+
304326
private static class TestRequestMappingInfoHandlerMapping extends RequestMappingInfoHandlerMapping {
305327

306328
public void registerHandler(Object handler) {
307329
super.detectHandlerMethods(handler);
308330
}
309-
331+
310332
@Override
311333
protected boolean isHandler(Class<?> beanType) {
312334
return AnnotationUtils.findAnnotation(beanType, RequestMapping.class) != null;
@@ -329,5 +351,5 @@ protected RequestMappingInfo getMappingForMethod(Method method, Class<?> handler
329351
}
330352
}
331353
}
332-
354+
333355
}

0 commit comments

Comments
 (0)