Skip to content

Commit 4108927

Browse files
poutsmapoutsma
committed
SPR-5367 - PathVariable mappings are greedy over hard coded mappings
1 parent ac5b1bc commit 4108927

File tree

2 files changed

+105
-66
lines changed

2 files changed

+105
-66
lines changed

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

Lines changed: 84 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -511,17 +511,20 @@ protected boolean isHandlerMethod(Method method) {
511511
}
512512
RequestMapping mapping = AnnotationUtils.findAnnotation(method, RequestMapping.class);
513513
if (mapping != null) {
514-
RequestMappingInfo mappingInfo = new RequestMappingInfo();
515-
mappingInfo.patterns = mapping.value();
514+
String[] patterns = mapping.value();
515+
RequestMethod[] methods = new RequestMethod[0];
516+
String[] params = new String[0];
517+
String[] headers = new String[0];
516518
if (!hasTypeLevelMapping() || !Arrays.equals(mapping.method(), getTypeLevelMapping().method())) {
517-
mappingInfo.methods = mapping.method();
519+
methods = mapping.method();
518520
}
519521
if (!hasTypeLevelMapping() || !Arrays.equals(mapping.params(), getTypeLevelMapping().params())) {
520-
mappingInfo.params = mapping.params();
522+
params = mapping.params();
521523
}
522524
if (!hasTypeLevelMapping() || !Arrays.equals(mapping.headers(), getTypeLevelMapping().headers())) {
523-
mappingInfo.headers = mapping.headers();
525+
headers = mapping.headers();
524526
}
527+
RequestMappingInfo mappingInfo = new RequestMappingInfo(patterns, methods, params, headers);
525528
this.mappings.put(method, mappingInfo);
526529
return true;
527530
}
@@ -531,23 +534,22 @@ protected boolean isHandlerMethod(Method method) {
531534
public Method resolveHandlerMethod(HttpServletRequest request) throws ServletException {
532535
String lookupPath = urlPathHelper.getLookupPathForRequest(request);
533536
Comparator<String> pathComparator = pathMatcher.getPatternComparator(lookupPath);
534-
Map<RequestMappingInfo, Method> targetHandlerMethods = new LinkedHashMap<RequestMappingInfo, Method>();
537+
Map<RequestSpecificMappingInfo, Method> targetHandlerMethods = new LinkedHashMap<RequestSpecificMappingInfo, Method>();
535538
Set<String> allowedMethods = new LinkedHashSet<String>(7);
536539
String resolvedMethodName = null;
537540
for (Method handlerMethod : getHandlerMethods()) {
538-
RequestMappingInfo mappingInfo = this.mappings.get(handlerMethod);
541+
RequestSpecificMappingInfo mappingInfo = new RequestSpecificMappingInfo(this.mappings.get(handlerMethod));
539542
boolean match = false;
540543
if (mappingInfo.hasPatterns()) {
541-
List<String> matchingPatterns = new ArrayList<String>(mappingInfo.patterns.length);
542-
for (String pattern : mappingInfo.patterns) {
544+
for (String pattern : mappingInfo.getPatterns()) {
543545
if (!hasTypeLevelMapping() && !pattern.startsWith("/")) {
544546
pattern = "/" + pattern;
545547
}
546548
String combinedPattern = getCombinedPattern(pattern, lookupPath, request);
547549
if (combinedPattern != null) {
548550
if (mappingInfo.matches(request)) {
549551
match = true;
550-
matchingPatterns.add(combinedPattern);
552+
mappingInfo.addMatchedPattern(combinedPattern);
551553
}
552554
else {
553555
if (!mappingInfo.matchesRequestMethod(request)) {
@@ -557,13 +559,12 @@ public Method resolveHandlerMethod(HttpServletRequest request) throws ServletExc
557559
}
558560
}
559561
}
560-
Collections.sort(matchingPatterns, pathComparator);
561-
mappingInfo.matchedPatterns = matchingPatterns;
562+
mappingInfo.sortMatchedPatterns(pathComparator);
562563
}
563564
else {
564565
// No paths specified: parameter match sufficient.
565566
match = mappingInfo.matches(request);
566-
if (match && mappingInfo.methods.length == 0 && mappingInfo.params.length == 0 &&
567+
if (match && mappingInfo.getMethodCount() == 0 && mappingInfo.getParamCount() == 0 &&
567568
resolvedMethodName != null && !resolvedMethodName.equals(handlerMethod.getName())) {
568569
match = false;
569570
}
@@ -576,7 +577,7 @@ public Method resolveHandlerMethod(HttpServletRequest request) throws ServletExc
576577
if (match) {
577578
Method oldMappedMethod = targetHandlerMethods.put(mappingInfo, handlerMethod);
578579
if (oldMappedMethod != null && oldMappedMethod != handlerMethod) {
579-
if (methodNameResolver != null && mappingInfo.patterns.length == 0) {
580+
if (methodNameResolver != null && !mappingInfo.hasPatterns()) {
580581
if (!oldMappedMethod.getName().equals(handlerMethod.getName())) {
581582
if (resolvedMethodName == null) {
582583
resolvedMethodName = methodNameResolver.getHandlerMethodName(request);
@@ -606,11 +607,11 @@ public Method resolveHandlerMethod(HttpServletRequest request) throws ServletExc
606607
}
607608
}
608609
if (!targetHandlerMethods.isEmpty()) {
609-
List<RequestMappingInfo> matches = new ArrayList<RequestMappingInfo>(targetHandlerMethods.keySet());
610-
RequestMappingInfoComparator requestMappingInfoComparator =
611-
new RequestMappingInfoComparator(pathComparator, request);
610+
List<RequestSpecificMappingInfo> matches = new ArrayList<RequestSpecificMappingInfo>(targetHandlerMethods.keySet());
611+
RequestSpecificMappingInfoComparator requestMappingInfoComparator =
612+
new RequestSpecificMappingInfoComparator(pathComparator, request);
612613
Collections.sort(matches, requestMappingInfoComparator);
613-
RequestMappingInfo bestMappingMatch = matches.get(0);
614+
RequestSpecificMappingInfo bestMappingMatch = matches.get(0);
614615
String bestMatchedPath = bestMappingMatch.bestMatchedPattern();
615616
if (bestMatchedPath != null) {
616617
extractHandlerMethodUriTemplates(bestMatchedPath, lookupPath, request);
@@ -996,26 +997,43 @@ private void writeWithMessageConverters(Object returnValue,
996997

997998

998999
/**
999-
* Holder for request mapping metadata. Allows for finding a best matching candidate.
1000+
* Holder for request mapping metadata.
10001001
*/
10011002
static class RequestMappingInfo {
10021003

1003-
String[] patterns = new String[0];
1004+
private final String[] patterns;
10041005

1005-
List<String> matchedPatterns = Collections.emptyList();
1006+
private final RequestMethod[] methods;
10061007

1007-
RequestMethod[] methods = new RequestMethod[0];
1008+
private final String[] params;
10081009

1009-
String[] params = new String[0];
1010+
private final String[] headers;
10101011

1011-
String[] headers = new String[0];
1012+
RequestMappingInfo(String[] patterns, RequestMethod[] methods, String[] params, String[] headers) {
1013+
this.patterns = patterns != null ? patterns : new String[0];
1014+
this.methods = methods != null ? methods : new RequestMethod[0];
1015+
this.params = params != null ? params : new String[0];
1016+
this.headers = headers != null ? headers : new String[0];
1017+
}
10121018

10131019
public boolean hasPatterns() {
10141020
return patterns.length > 0;
10151021
}
10161022

1017-
public String bestMatchedPattern() {
1018-
return (!this.matchedPatterns.isEmpty() ? this.matchedPatterns.get(0) : null);
1023+
public String[] getPatterns() {
1024+
return patterns;
1025+
}
1026+
1027+
public int getMethodCount() {
1028+
return methods.length;
1029+
}
1030+
1031+
public int getParamCount() {
1032+
return params.length;
1033+
}
1034+
1035+
public int getHeaderCount() {
1036+
return headers.length;
10191037
}
10201038

10211039
public boolean matches(HttpServletRequest request) {
@@ -1075,12 +1093,41 @@ public String toString() {
10751093
}
10761094
}
10771095

1096+
/**
1097+
* Subclass of {@link RequestMappingInfo} that holds request-specific data.
1098+
*/
1099+
static class RequestSpecificMappingInfo extends RequestMappingInfo {
1100+
1101+
private final List<String> matchedPatterns = new ArrayList<String>();
1102+
1103+
RequestSpecificMappingInfo(String[] patterns, RequestMethod[] methods, String[] params, String[] headers) {
1104+
super(patterns, methods, params, headers);
1105+
}
1106+
1107+
RequestSpecificMappingInfo(RequestMappingInfo other) {
1108+
super(other.patterns, other.methods, other.params, other.headers);
1109+
}
1110+
1111+
public void addMatchedPattern(String matchedPattern) {
1112+
matchedPatterns.add(matchedPattern);
1113+
}
1114+
1115+
public void sortMatchedPatterns(Comparator<String> pathComparator) {
1116+
Collections.sort(matchedPatterns, pathComparator);
1117+
}
1118+
1119+
public String bestMatchedPattern() {
1120+
return (!this.matchedPatterns.isEmpty() ? this.matchedPatterns.get(0) : null);
1121+
}
1122+
1123+
}
1124+
10781125

10791126
/**
1080-
* Comparator capable of sorting {@link RequestMappingInfo}s (RHIs) so that sorting a list with this comparator will
1127+
* Comparator capable of sorting {@link RequestSpecificMappingInfo}s (RHIs) so that sorting a list with this comparator will
10811128
* result in:
10821129
* <ul>
1083-
* <li>RHIs with {@linkplain RequestMappingInfo#matchedPatterns better matched paths} take prescedence
1130+
* <li>RHIs with {@linkplain org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo#matchedPatterns better matched paths} take prescedence
10841131
* over those with a weaker match (as expressed by the {@linkplain PathMatcher#getPatternComparator(String) path
10851132
* pattern comparator}.) Typically, this means that patterns without wild cards and uri templates will be ordered
10861133
* before those without.</li>
@@ -1090,38 +1137,38 @@ public String toString() {
10901137
* less parameters</li>
10911138
* </ol>
10921139
*/
1093-
static class RequestMappingInfoComparator implements Comparator<RequestMappingInfo> {
1140+
static class RequestSpecificMappingInfoComparator implements Comparator<RequestSpecificMappingInfo> {
10941141

10951142
private final Comparator<String> pathComparator;
10961143

10971144
private final ServerHttpRequest request;
10981145

1099-
RequestMappingInfoComparator(Comparator<String> pathComparator, HttpServletRequest request) {
1146+
RequestSpecificMappingInfoComparator(Comparator<String> pathComparator, HttpServletRequest request) {
11001147
this.pathComparator = pathComparator;
11011148
this.request = new ServletServerHttpRequest(request);
11021149
}
11031150

1104-
public int compare(RequestMappingInfo info1, RequestMappingInfo info2) {
1151+
public int compare(RequestSpecificMappingInfo info1, RequestSpecificMappingInfo info2) {
11051152
int pathComparison = pathComparator.compare(info1.bestMatchedPattern(), info2.bestMatchedPattern());
11061153
if (pathComparison != 0) {
11071154
return pathComparison;
11081155
}
1109-
int info1ParamCount = info1.params.length;
1110-
int info2ParamCount = info2.params.length;
1156+
int info1ParamCount = info1.getParamCount();
1157+
int info2ParamCount = info2.getParamCount();
11111158
if (info1ParamCount != info2ParamCount) {
11121159
return info2ParamCount - info1ParamCount;
11131160
}
1114-
int info1HeaderCount = info1.headers.length;
1115-
int info2HeaderCount = info2.headers.length;
1161+
int info1HeaderCount = info1.getHeaderCount();
1162+
int info2HeaderCount = info2.getHeaderCount();
11161163
if (info1HeaderCount != info2HeaderCount) {
11171164
return info2HeaderCount - info1HeaderCount;
11181165
}
11191166
int acceptComparison = compareAcceptHeaders(info1, info2);
11201167
if (acceptComparison != 0) {
11211168
return acceptComparison;
11221169
}
1123-
int info1MethodCount = info1.methods.length;
1124-
int info2MethodCount = info2.methods.length;
1170+
int info1MethodCount = info1.getMethodCount();
1171+
int info2MethodCount = info2.getMethodCount();
11251172
if (info1MethodCount == 0 && info2MethodCount > 0) {
11261173
return 1;
11271174
}

‎org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTests.java‎ renamed to ‎org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestSpecificMappingInfoComparatorTests.java‎

Lines changed: 21 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -31,45 +31,39 @@
3131
/**
3232
* @author Arjen Poutsma
3333
*/
34-
public class RequestMappingInfoComparatorTests {
34+
public class RequestSpecificMappingInfoComparatorTests {
3535

36-
private AnnotationMethodHandlerAdapter.RequestMappingInfoComparator comparator;
36+
private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator comparator;
3737

38-
private AnnotationMethodHandlerAdapter.RequestMappingInfo emptyInfo;
38+
private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo emptyInfo;
3939

40-
private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodInfo;
40+
private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo oneMethodInfo;
4141

42-
private AnnotationMethodHandlerAdapter.RequestMappingInfo twoMethodsInfo;
42+
private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo twoMethodsInfo;
4343

44-
private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodOneParamInfo;
44+
private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo oneMethodOneParamInfo;
4545

46-
private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodTwoParamsInfo;
46+
private AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo oneMethodTwoParamsInfo;
4747

4848

4949
@Before
5050
public void setUp() throws NoSuchMethodException {
51-
comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), new MockHttpServletRequest());
51+
comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), new MockHttpServletRequest());
5252

53-
emptyInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
53+
emptyInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[0],null, null);
5454

55-
oneMethodInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
56-
oneMethodInfo.methods = new RequestMethod[]{RequestMethod.GET};
55+
oneMethodInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, null, null);
5756

58-
twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
59-
twoMethodsInfo.methods = new RequestMethod[]{RequestMethod.GET, RequestMethod.POST};
57+
twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET, RequestMethod.POST}, null, null);
6058

61-
oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
62-
oneMethodOneParamInfo.methods = new RequestMethod[]{RequestMethod.GET};
63-
oneMethodOneParamInfo.params = new String[]{"param"};
59+
oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, new String[]{"param"}, null);
6460

65-
oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
66-
oneMethodTwoParamsInfo.methods = new RequestMethod[]{RequestMethod.GET};
67-
oneMethodTwoParamsInfo.params = new String[]{"param1", "param2"};
61+
oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, new RequestMethod[]{RequestMethod.GET}, new String[]{"param1", "param2"}, null);
6862
}
6963

7064
@Test
7165
public void sort() {
72-
List<AnnotationMethodHandlerAdapter.RequestMappingInfo> infos = new ArrayList<AnnotationMethodHandlerAdapter.RequestMappingInfo>();
66+
List<AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo> infos = new ArrayList<AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo>();
7367
infos.add(emptyInfo);
7468
infos.add(oneMethodInfo);
7569
infos.add(twoMethodsInfo);
@@ -88,17 +82,15 @@ public void sort() {
8882

8983
@Test
9084
public void acceptHeaders() {
91-
AnnotationMethodHandlerAdapter.RequestMappingInfo html = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
92-
html.headers = new String[] {"accept=text/html"};
85+
AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo html = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, new String[] {"accept=text/html"});
9386

94-
AnnotationMethodHandlerAdapter.RequestMappingInfo xml = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
95-
xml.headers = new String[] {"accept=application/xml"};
87+
AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo xml = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, new String[] {"accept=application/xml"});
9688

97-
AnnotationMethodHandlerAdapter.RequestMappingInfo none = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
89+
AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo none = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfo(null, null, null, null);
9890

9991
MockHttpServletRequest request = new MockHttpServletRequest();
10092
request.addHeader("Accept", "application/xml, text/html");
101-
comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request);
93+
comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request);
10294

10395
assertTrue(comparator.compare(html, xml) > 0);
10496
assertTrue(comparator.compare(xml, html) < 0);
@@ -109,22 +101,22 @@ public void acceptHeaders() {
109101

110102
request = new MockHttpServletRequest();
111103
request.addHeader("Accept", "application/xml, text/*");
112-
comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request);
104+
comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request);
113105

114106
assertTrue(comparator.compare(html, xml) > 0);
115107
assertTrue(comparator.compare(xml, html) < 0);
116108

117109
request = new MockHttpServletRequest();
118110
request.addHeader("Accept", "application/pdf");
119-
comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request);
111+
comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request);
120112

121113
assertTrue(comparator.compare(html, xml) == 0);
122114
assertTrue(comparator.compare(xml, html) == 0);
123115

124116
// See SPR-7000
125117
request = new MockHttpServletRequest();
126118
request.addHeader("Accept", "text/html;q=0.9,application/xml");
127-
comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator(), request);
119+
comparator = new AnnotationMethodHandlerAdapter.RequestSpecificMappingInfoComparator(new MockComparator(), request);
128120

129121
assertTrue(comparator.compare(html, xml) > 0);
130122
assertTrue(comparator.compare(xml, html) < 0);

0 commit comments

Comments
 (0)