From 4108927b28a00d4918e2852f09c8bde10c3fd0bb Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Wed, 13 Oct 2010 12:03:26 +0000 Subject: [PATCH] SPR-5367 - PathVariable mappings are greedy over hard coded mappings --- .../AnnotationMethodHandlerAdapter.java | 121 ++++++++++++------ ...stSpecificMappingInfoComparatorTests.java} | 50 +++----- 2 files changed, 105 insertions(+), 66 deletions(-) rename org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/{RequestMappingInfoComparatorTests.java => RequestSpecificMappingInfoComparatorTests.java} (54%) diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerAdapter.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerAdapter.java index 039f8a04ed..e6c22dd7aa 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerAdapter.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerAdapter.java @@ -511,17 +511,20 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator } RequestMapping mapping = AnnotationUtils.findAnnotation(method, RequestMapping.class); if (mapping != null) { - RequestMappingInfo mappingInfo = new RequestMappingInfo(); - mappingInfo.patterns = mapping.value(); + String[] patterns = mapping.value(); + RequestMethod[] methods = new RequestMethod[0]; + String[] params = new String[0]; + String[] headers = new String[0]; if (!hasTypeLevelMapping() || !Arrays.equals(mapping.method(), getTypeLevelMapping().method())) { - mappingInfo.methods = mapping.method(); + methods = mapping.method(); } if (!hasTypeLevelMapping() || !Arrays.equals(mapping.params(), getTypeLevelMapping().params())) { - mappingInfo.params = mapping.params(); + params = mapping.params(); } if (!hasTypeLevelMapping() || !Arrays.equals(mapping.headers(), getTypeLevelMapping().headers())) { - mappingInfo.headers = mapping.headers(); + headers = mapping.headers(); } + RequestMappingInfo mappingInfo = new RequestMappingInfo(patterns, methods, params, headers); this.mappings.put(method, mappingInfo); return true; } @@ -531,15 +534,14 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator public Method resolveHandlerMethod(HttpServletRequest request) throws ServletException { String lookupPath = urlPathHelper.getLookupPathForRequest(request); Comparator pathComparator = pathMatcher.getPatternComparator(lookupPath); - Map targetHandlerMethods = new LinkedHashMap(); + Map targetHandlerMethods = new LinkedHashMap(); Set allowedMethods = new LinkedHashSet(7); String resolvedMethodName = null; for (Method handlerMethod : getHandlerMethods()) { - RequestMappingInfo mappingInfo = this.mappings.get(handlerMethod); + RequestSpecificMappingInfo mappingInfo = new RequestSpecificMappingInfo(this.mappings.get(handlerMethod)); boolean match = false; if (mappingInfo.hasPatterns()) { - List matchingPatterns = new ArrayList(mappingInfo.patterns.length); - for (String pattern : mappingInfo.patterns) { + for (String pattern : mappingInfo.getPatterns()) { if (!hasTypeLevelMapping() && !pattern.startsWith("/")) { pattern = "/" + pattern; } @@ -547,7 +549,7 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator if (combinedPattern != null) { if (mappingInfo.matches(request)) { match = true; - matchingPatterns.add(combinedPattern); + mappingInfo.addMatchedPattern(combinedPattern); } else { if (!mappingInfo.matchesRequestMethod(request)) { @@ -557,13 +559,12 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator } } } - Collections.sort(matchingPatterns, pathComparator); - mappingInfo.matchedPatterns = matchingPatterns; + mappingInfo.sortMatchedPatterns(pathComparator); } else { // No paths specified: parameter match sufficient. match = mappingInfo.matches(request); - if (match && mappingInfo.methods.length == 0 && mappingInfo.params.length == 0 && + if (match && mappingInfo.getMethodCount() == 0 && mappingInfo.getParamCount() == 0 && resolvedMethodName != null && !resolvedMethodName.equals(handlerMethod.getName())) { match = false; } @@ -576,7 +577,7 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator if (match) { Method oldMappedMethod = targetHandlerMethods.put(mappingInfo, handlerMethod); if (oldMappedMethod != null && oldMappedMethod != handlerMethod) { - if (methodNameResolver != null && mappingInfo.patterns.length == 0) { + if (methodNameResolver != null && !mappingInfo.hasPatterns()) { if (!oldMappedMethod.getName().equals(handlerMethod.getName())) { if (resolvedMethodName == null) { resolvedMethodName = methodNameResolver.getHandlerMethodName(request); @@ -606,11 +607,11 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator } } if (!targetHandlerMethods.isEmpty()) { - List matches = new ArrayList(targetHandlerMethods.keySet()); - RequestMappingInfoComparator requestMappingInfoComparator = - new RequestMappingInfoComparator(pathComparator, request); + List matches = new ArrayList(targetHandlerMethods.keySet()); + RequestSpecificMappingInfoComparator requestMappingInfoComparator = + new RequestSpecificMappingInfoComparator(pathComparator, request); Collections.sort(matches, requestMappingInfoComparator); - RequestMappingInfo bestMappingMatch = matches.get(0); + RequestSpecificMappingInfo bestMappingMatch = matches.get(0); String bestMatchedPath = bestMappingMatch.bestMatchedPattern(); if (bestMatchedPath != null) { extractHandlerMethodUriTemplates(bestMatchedPath, lookupPath, request); @@ -996,26 +997,43 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator /** - * Holder for request mapping metadata. Allows for finding a best matching candidate. + * Holder for request mapping metadata. */ static class RequestMappingInfo { - String[] patterns = new String[0]; + private final String[] patterns; - List matchedPatterns = Collections.emptyList(); + private final RequestMethod[] methods; - RequestMethod[] methods = new RequestMethod[0]; + private final String[] params; - String[] params = new String[0]; + private final String[] headers; - String[] headers = new String[0]; + RequestMappingInfo(String[] patterns, RequestMethod[] methods, String[] params, String[] headers) { + this.patterns = patterns != null ? patterns : new String[0]; + this.methods = methods != null ? methods : new RequestMethod[0]; + this.params = params != null ? params : new String[0]; + this.headers = headers != null ? headers : new String[0]; + } public boolean hasPatterns() { return patterns.length > 0; } - public String bestMatchedPattern() { - return (!this.matchedPatterns.isEmpty() ? this.matchedPatterns.get(0) : null); + public String[] getPatterns() { + return patterns; + } + + public int getMethodCount() { + return methods.length; + } + + public int getParamCount() { + return params.length; + } + + public int getHeaderCount() { + return headers.length; } public boolean matches(HttpServletRequest request) { @@ -1075,12 +1093,41 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator } } + /** + * Subclass of {@link RequestMappingInfo} that holds request-specific data. + */ + static class RequestSpecificMappingInfo extends RequestMappingInfo { + + private final List matchedPatterns = new ArrayList(); + + RequestSpecificMappingInfo(String[] patterns, RequestMethod[] methods, String[] params, String[] headers) { + super(patterns, methods, params, headers); + } + + RequestSpecificMappingInfo(RequestMappingInfo other) { + super(other.patterns, other.methods, other.params, other.headers); + } + + public void addMatchedPattern(String matchedPattern) { + matchedPatterns.add(matchedPattern); + } + + public void sortMatchedPatterns(Comparator pathComparator) { + Collections.sort(matchedPatterns, pathComparator); + } + + public String bestMatchedPattern() { + return (!this.matchedPatterns.isEmpty() ? this.matchedPatterns.get(0) : null); + } + + } + /** - * Comparator capable of sorting {@link RequestMappingInfo}s (RHIs) so that sorting a list with this comparator will + * Comparator capable of sorting {@link RequestSpecificMappingInfo}s (RHIs) so that sorting a list with this comparator will * result in: *
    - *
  • RHIs with {@linkplain RequestMappingInfo#matchedPatterns better matched paths} take prescedence + *
  • RHIs with {@linkplain org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo#matchedPatterns better matched paths} take prescedence * over those with a weaker match (as expressed by the {@linkplain PathMatcher#getPatternComparator(String) path * pattern comparator}.) Typically, this means that patterns without wild cards and uri templates will be ordered * before those without.
    • @@ -1090,29 +1137,29 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator * less parameters * */ - static class RequestMappingInfoComparator implements Comparator { + static class RequestSpecificMappingInfoComparator implements Comparator { private final Comparator pathComparator; private final ServerHttpRequest request; - RequestMappingInfoComparator(Comparator pathComparator, HttpServletRequest request) { + RequestSpecificMappingInfoComparator(Comparator pathComparator, HttpServletRequest request) { this.pathComparator = pathComparator; this.request = new ServletServerHttpRequest(request); } - public int compare(RequestMappingInfo info1, RequestMappingInfo info2) { + public int compare(RequestSpecificMappingInfo info1, RequestSpecificMappingInfo info2) { int pathComparison = pathComparator.compare(info1.bestMatchedPattern(), info2.bestMatchedPattern()); if (pathComparison != 0) { return pathComparison; } - int info1ParamCount = info1.params.length; - int info2ParamCount = info2.params.length; + int info1ParamCount = info1.getParamCount(); + int info2ParamCount = info2.getParamCount(); if (info1ParamCount != info2ParamCount) { return info2ParamCount - info1ParamCount; } - int info1HeaderCount = info1.headers.length; - int info2HeaderCount = info2.headers.length; + int info1HeaderCount = info1.getHeaderCount(); + int info2HeaderCount = info2.getHeaderCount(); if (info1HeaderCount != info2HeaderCount) { return info2HeaderCount - info1HeaderCount; } @@ -1120,8 +1167,8 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator if (acceptComparison != 0) { return acceptComparison; } - int info1MethodCount = info1.methods.length; - int info2MethodCount = info2.methods.length; + int info1MethodCount = info1.getMethodCount(); + int info2MethodCount = info2.getMethodCount(); if (info1MethodCount == 0 && info2MethodCount > 0) { return 1; } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestSpecificMappingInfoComparatorTests.java similarity index 54% rename from org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTests.java rename to org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestSpecificMappingInfoComparatorTests.java index 7e095f459c..539feaef22 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestSpecificMappingInfoComparatorTests.java @@ -31,45 +31,39 @@ import org.springframework.web.bind.annotation.RequestMethod; /** * @author Arjen Poutsma */ -public class RequestMappingInfoComparatorTests { +public class RequestSpecificMappingInfoComparatorTests { - private AnnotationMethodHandlerAdapter.RequestMappingInfoComparator comparator; + private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator comparator; - private AnnotationMethodHandlerAdapter.RequestMappingInfo emptyInfo; + private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo emptyInfo; - private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodInfo; + private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo oneMethodInfo; - private AnnotationMethodHandlerAdapter.RequestMappingInfo twoMethodsInfo; + private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo twoMethodsInfo; - private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodOneParamInfo; + private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo oneMethodOneParamInfo; - private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodTwoParamsInfo; + private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo oneMethodTwoParamsInfo; @Before public void setUp() throws NoSuchMethodException { - comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), new MockHttpServletRequest()); + comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), new MockHttpServletRequest()); - emptyInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); + emptyInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[0],null, null); - oneMethodInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); - oneMethodInfo.methods = new RequestMethod[]{RequestMethod.GET}; + oneMethodInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, null, null); - twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); - twoMethodsInfo.methods = new RequestMethod[]{RequestMethod.GET, RequestMethod.POST}; + twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET, RequestMethod.POST}, null, null); - oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); - oneMethodOneParamInfo.methods = new RequestMethod[]{RequestMethod.GET}; - oneMethodOneParamInfo.params = new String[]{"param"}; + oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, new String[]{"param"}, null); - oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); - oneMethodTwoParamsInfo.methods = new RequestMethod[]{RequestMethod.GET}; - oneMethodTwoParamsInfo.params = new String[]{"param1", "param2"}; + oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, new String[]{"param1", "param2"}, null); } @Test public void sort() { - List infos = new ArrayList(); + List infos = new ArrayList(); infos.add(emptyInfo); infos.add(oneMethodInfo); infos.add(twoMethodsInfo); @@ -88,17 +82,15 @@ public class RequestMappingInfoComparatorTests { @Test public void acceptHeaders() { - AnnotationMethodHandlerAdapter.RequestMappingInfo html = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); - html.headers = new String[] {"accept=text/html"}; + AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo html = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, new String[] {"accept=text/html"}); - AnnotationMethodHandlerAdapter.RequestMappingInfo xml = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); - xml.headers = new String[] {"accept=application/xml"}; + AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo xml = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, new String[] {"accept=application/xml"}); - AnnotationMethodHandlerAdapter.RequestMappingInfo none = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); + AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo none = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, null); MockHttpServletRequest request = new MockHttpServletRequest(); request.addHeader("Accept", "application/xml, text/html"); - comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request); + comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request); assertTrue(comparator.compare(html, xml) > 0); assertTrue(comparator.compare(xml, html) < 0); @@ -109,14 +101,14 @@ public class RequestMappingInfoComparatorTests { request = new MockHttpServletRequest(); request.addHeader("Accept", "application/xml, text/*"); - comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request); + comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request); assertTrue(comparator.compare(html, xml) > 0); assertTrue(comparator.compare(xml, html) < 0); request = new MockHttpServletRequest(); request.addHeader("Accept", "application/pdf"); - comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request); + comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request); assertTrue(comparator.compare(html, xml) == 0); assertTrue(comparator.compare(xml, html) == 0); @@ -124,7 +116,7 @@ public class RequestMappingInfoComparatorTests { // See SPR-7000 request = new MockHttpServletRequest(); request.addHeader("Accept", "text/html;q=0.9,application/xml"); - comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request); + comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request); assertTrue(comparator.compare(html, xml) > 0); assertTrue(comparator.compare(xml, html) < 0);