root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions 2 Packages Projects Releases Wiki Activity

SPR-5367 - PathVariable mappings are greedy over hard coded mappings

This commit is contained in:
Arjen Poutsma 2010-10-13 12:03:26 +00:00
parent ac5b1bcfab
commit 4108927b28
2 changed files with 105 additions and 66 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

121
org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/annotation/AnnotationMethodHandlerAdapter.java
View File

@ -511,17 +511,20 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
} }
RequestMapping mapping = AnnotationUtils.findAnnotation(method, RequestMapping.class); RequestMapping mapping = AnnotationUtils.findAnnotation(method, RequestMapping.class);
if (mapping != null) { if (mapping != null) {
RequestMappingInfo mappingInfo = new RequestMappingInfo(); String[] patterns = mapping.value();
mappingInfo.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())) { if (!hasTypeLevelMapping() || !Arrays.equals(mapping.method(), getTypeLevelMapping().method())) {
mappingInfo.methods = mapping.method(); methods = mapping.method();
} }
if (!hasTypeLevelMapping() || !Arrays.equals(mapping.params(), getTypeLevelMapping().params())) { if (!hasTypeLevelMapping() || !Arrays.equals(mapping.params(), getTypeLevelMapping().params())) {
mappingInfo.params = mapping.params(); params = mapping.params();
} }
if (!hasTypeLevelMapping() || !Arrays.equals(mapping.headers(), getTypeLevelMapping().headers())) { 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); this.mappings.put(method, mappingInfo);
return true; return true;
} }
@ -531,15 +534,14 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
public Method resolveHandlerMethod(HttpServletRequest request) throws ServletException { public Method resolveHandlerMethod(HttpServletRequest request) throws ServletException {
String lookupPath = urlPathHelper.getLookupPathForRequest(request); String lookupPath = urlPathHelper.getLookupPathForRequest(request);
Comparator<String> pathComparator = pathMatcher.getPatternComparator(lookupPath); Comparator<String> pathComparator = pathMatcher.getPatternComparator(lookupPath);
Map<RequestMappingInfo, Method> targetHandlerMethods = new LinkedHashMap<RequestMappingInfo, Method>(); Map<RequestSpecificMappingInfo, Method> targetHandlerMethods = new LinkedHashMap<RequestSpecificMappingInfo, Method>();
Set<String> allowedMethods = new LinkedHashSet<String>(7); Set<String> allowedMethods = new LinkedHashSet<String>(7);
String resolvedMethodName = null; String resolvedMethodName = null;
for (Method handlerMethod : getHandlerMethods()) { for (Method handlerMethod : getHandlerMethods()) {
RequestMappingInfo mappingInfo = this.mappings.get(handlerMethod); RequestSpecificMappingInfo mappingInfo = new RequestSpecificMappingInfo(this.mappings.get(handlerMethod));
boolean match = false; boolean match = false;
if (mappingInfo.hasPatterns()) { if (mappingInfo.hasPatterns()) {
List<String> matchingPatterns = new ArrayList<String>(mappingInfo.patterns.length); for (String pattern : mappingInfo.getPatterns()) {
for (String pattern : mappingInfo.patterns) {
if (!hasTypeLevelMapping() && !pattern.startsWith("/")) { if (!hasTypeLevelMapping() && !pattern.startsWith("/")) {
pattern = "/" + pattern; pattern = "/" + pattern;
} }
@ -547,7 +549,7 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
if (combinedPattern != null) { if (combinedPattern != null) {
if (mappingInfo.matches(request)) { if (mappingInfo.matches(request)) {
match = true; match = true;
matchingPatterns.add(combinedPattern); mappingInfo.addMatchedPattern(combinedPattern);
} }
else { else {
if (!mappingInfo.matchesRequestMethod(request)) { if (!mappingInfo.matchesRequestMethod(request)) {
@ -557,13 +559,12 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
} }
} }
} }
Collections.sort(matchingPatterns, pathComparator); mappingInfo.sortMatchedPatterns(pathComparator);
mappingInfo.matchedPatterns = matchingPatterns;
} }
else { else {
// No paths specified: parameter match sufficient. // No paths specified: parameter match sufficient.
match = mappingInfo.matches(request); 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())) { resolvedMethodName != null && !resolvedMethodName.equals(handlerMethod.getName())) {
match = false; match = false;
} }
@ -576,7 +577,7 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
if (match) { if (match) {
Method oldMappedMethod = targetHandlerMethods.put(mappingInfo, handlerMethod); Method oldMappedMethod = targetHandlerMethods.put(mappingInfo, handlerMethod);
if (oldMappedMethod != null && oldMappedMethod != 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 (!oldMappedMethod.getName().equals(handlerMethod.getName())) {
if (resolvedMethodName == null) { if (resolvedMethodName == null) {
resolvedMethodName = methodNameResolver.getHandlerMethodName(request); resolvedMethodName = methodNameResolver.getHandlerMethodName(request);
@ -606,11 +607,11 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
} }
} }
if (!targetHandlerMethods.isEmpty()) { if (!targetHandlerMethods.isEmpty()) {
List<RequestMappingInfo> matches = new ArrayList<RequestMappingInfo>(targetHandlerMethods.keySet()); List<RequestSpecificMappingInfo> matches = new ArrayList<RequestSpecificMappingInfo>(targetHandlerMethods.keySet());
RequestMappingInfoComparator requestMappingInfoComparator = RequestSpecificMappingInfoComparator requestMappingInfoComparator =
new RequestMappingInfoComparator(pathComparator, request); new RequestSpecificMappingInfoComparator(pathComparator, request);
Collections.sort(matches, requestMappingInfoComparator); Collections.sort(matches, requestMappingInfoComparator);
RequestMappingInfo bestMappingMatch = matches.get(0); RequestSpecificMappingInfo bestMappingMatch = matches.get(0);
String bestMatchedPath = bestMappingMatch.bestMatchedPattern(); String bestMatchedPath = bestMappingMatch.bestMatchedPattern();
if (bestMatchedPath != null) { if (bestMatchedPath != null) {
extractHandlerMethodUriTemplates(bestMatchedPath, lookupPath, request); 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 { static class RequestMappingInfo {
String[] patterns = new String[0]; private final String[] patterns;
List<String> 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() { public boolean hasPatterns() {
return patterns.length > 0; return patterns.length > 0;
} }
public String bestMatchedPattern() { public String[] getPatterns() {
return (!this.matchedPatterns.isEmpty() ? this.matchedPatterns.get(0) : null); 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) { 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<String> matchedPatterns = new ArrayList<String>();
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<String> 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: * result in:
* <ul> * <ul>
* <li>RHIs with {@linkplain RequestMappingInfo#matchedPatterns better matched paths} take prescedence * <li>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 * 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 * pattern comparator}.) Typically, this means that patterns without wild cards and uri templates will be ordered
* before those without.</li> * before those without.</li>
@ -1090,29 +1137,29 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
* less parameters</li> * less parameters</li>
* </ol> * </ol>
*/ */
static class RequestMappingInfoComparator implements Comparator<RequestMappingInfo> { static class RequestSpecificMappingInfoComparator implements Comparator<RequestSpecificMappingInfo> {
private final Comparator<String> pathComparator; private final Comparator<String> pathComparator;
private final ServerHttpRequest request; private final ServerHttpRequest request;
RequestMappingInfoComparator(Comparator<String> pathComparator, HttpServletRequest request) { RequestSpecificMappingInfoComparator(Comparator<String> pathComparator, HttpServletRequest request) {
this.pathComparator = pathComparator; this.pathComparator = pathComparator;
this.request = new ServletServerHttpRequest(request); 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()); int pathComparison = pathComparator.compare(info1.bestMatchedPattern(), info2.bestMatchedPattern());
if (pathComparison != 0) { if (pathComparison != 0) {
return pathComparison; return pathComparison;
} }
int info1ParamCount = info1.params.length; int info1ParamCount = info1.getParamCount();
int info2ParamCount = info2.params.length; int info2ParamCount = info2.getParamCount();
if (info1ParamCount != info2ParamCount) { if (info1ParamCount != info2ParamCount) {
return info2ParamCount - info1ParamCount; return info2ParamCount - info1ParamCount;
} }
int info1HeaderCount = info1.headers.length; int info1HeaderCount = info1.getHeaderCount();
int info2HeaderCount = info2.headers.length; int info2HeaderCount = info2.getHeaderCount();
if (info1HeaderCount != info2HeaderCount) { if (info1HeaderCount != info2HeaderCount) {
return info2HeaderCount - info1HeaderCount; return info2HeaderCount - info1HeaderCount;
} }
@ -1120,8 +1167,8 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator
if (acceptComparison != 0) { if (acceptComparison != 0) {
return acceptComparison; return acceptComparison;
} }
int info1MethodCount = info1.methods.length; int info1MethodCount = info1.getMethodCount();
int info2MethodCount = info2.methods.length; int info2MethodCount = info2.getMethodCount();
if (info1MethodCount == 0 && info2MethodCount > 0) { if (info1MethodCount == 0 && info2MethodCount > 0) {
return 1; return 1;
} }

50
org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTests.java → org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestSpecificMappingInfoComparatorTests.java
View File

@ -31,45 +31,39 @@ import org.springframework.web.bind.annotation.RequestMethod;
/** /**
* @author Arjen Poutsma * @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 @Before
public void setUp() throws NoSuchMethodException { 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 = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, null, null);
oneMethodInfo.methods = new RequestMethod[]{RequestMethod.GET};
twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET, RequestMethod.POST}, null, null);
twoMethodsInfo.methods = new RequestMethod[]{RequestMethod.GET, RequestMethod.POST};
oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, new String[]{"param"}, null);
oneMethodOneParamInfo.methods = new RequestMethod[]{RequestMethod.GET};
oneMethodOneParamInfo.params = new String[]{"param"};
oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, new String[]{"param1", "param2"}, null);
oneMethodTwoParamsInfo.methods = new RequestMethod[]{RequestMethod.GET};
oneMethodTwoParamsInfo.params = new String[]{"param1", "param2"};
} }
@Test @Test
public void sort() { public void sort() {
List<AnnotationMethodHandlerAdapter.RequestMappingInfo> infos = new ArrayList<AnnotationMethodHandlerAdapter.RequestMappingInfo>(); List<AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo> infos = new ArrayList<AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo>();
infos.add(emptyInfo); infos.add(emptyInfo);
infos.add(oneMethodInfo); infos.add(oneMethodInfo);
infos.add(twoMethodsInfo); infos.add(twoMethodsInfo);
@ -88,17 +82,15 @@ public class RequestMappingInfoComparatorTests {
@Test @Test
public void acceptHeaders() { public void acceptHeaders() {
AnnotationMethodHandlerAdapter.RequestMappingInfo html = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo html = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, new String[] {"accept=text/html"});
html.headers = new String[] {"accept=text/html"};
AnnotationMethodHandlerAdapter.RequestMappingInfo xml = new AnnotationMethodHandlerAdapter.RequestMappingInfo(); AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo xml = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, new String[] {"accept=application/xml"});
xml.headers = 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(); MockHttpServletRequest request = new MockHttpServletRequest();
request.addHeader("Accept", "application/xml, text/html"); 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(html, xml) > 0);
assertTrue(comparator.compare(xml, html) < 0); assertTrue(comparator.compare(xml, html) < 0);
@ -109,14 +101,14 @@ public class RequestMappingInfoComparatorTests {
request = new MockHttpServletRequest(); request = new MockHttpServletRequest();
request.addHeader("Accept", "application/xml, text/*"); 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(html, xml) > 0);
assertTrue(comparator.compare(xml, html) < 0); assertTrue(comparator.compare(xml, html) < 0);
request = new MockHttpServletRequest(); request = new MockHttpServletRequest();
request.addHeader("Accept", "application/pdf"); 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(html, xml) == 0);
assertTrue(comparator.compare(xml, html) == 0); assertTrue(comparator.compare(xml, html) == 0);
@ -124,7 +116,7 @@ public class RequestMappingInfoComparatorTests {
// See SPR-7000 // See SPR-7000
request = new MockHttpServletRequest(); request = new MockHttpServletRequest();
request.addHeader("Accept", "text/html;q=0.9,application/xml"); 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(html, xml) > 0);
assertTrue(comparator.compare(xml, html) < 0); assertTrue(comparator.compare(xml, html) < 0);