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

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

@@ -22,9 +22,13 @@ import java.io.Reader;
 import java.io.Writer;
 import java.lang.reflect.Method;
 import java.security.Principal;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
@@ -434,8 +438,8 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 
 		public Method resolveHandlerMethod(HttpServletRequest request) throws ServletException {
 			String lookupPath = urlPathHelper.getLookupPathForRequest(request);
+			Comparator<String> pathComparator = pathMatcher.getPatternComparator(lookupPath);
 			Map<RequestMappingInfo, Method> targetHandlerMethods = new LinkedHashMap<RequestMappingInfo, Method>();
-			Map<RequestMappingInfo, String> targetPathMatches = new LinkedHashMap<RequestMappingInfo, String>();
 			Set<String> allowedMethods = new LinkedHashSet<String>(7);
 			String resolvedMethodName = null;
 			for (Method handlerMethod : getHandlerMethods()) {
@@ -450,11 +454,12 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 				}
 				boolean match = false;
 				if (mappingInfo.paths.length > 0) {
+					List<String> matchedPaths = new ArrayList<String>(mappingInfo.paths.length);					
 					for (String mappedPath : mappingInfo.paths) {
 						if (isPathMatch(mappedPath, lookupPath)) {
 							if (checkParameters(mappingInfo, request)) {
 								match = true;
-								targetPathMatches.put(mappingInfo, mappedPath);
+								matchedPaths.add(mappedPath);
 							}
 							else {
 								for (RequestMethod requestMethod : mappingInfo.methods) {
@@ -464,6 +469,8 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 							}
 						}
 					}
+					Collections.sort(matchedPaths, pathComparator);
+					mappingInfo.matchedPaths = matchedPaths.toArray(new String[matchedPaths.size()]);					
 				}
 				else {
 					// No paths specified: parameter match sufficient.
@@ -504,34 +511,13 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 					}
 				}
 			}
-			if (targetHandlerMethods.size() == 1) {
+			if (!targetHandlerMethods.isEmpty()) {
-				if (targetPathMatches.size() == 1) {
+				List<RequestMappingInfo> matches = new ArrayList<RequestMappingInfo>(targetHandlerMethods.keySet());
-					extractHandlerMethodUriTemplates(targetPathMatches.values().iterator().next(), lookupPath, request);
+				RequestMappingInfoComparator requestMappingInfoComparator = new RequestMappingInfoComparator(pathComparator);
-				}
+				Collections.sort(matches, requestMappingInfoComparator);
-				return targetHandlerMethods.values().iterator().next();
+				RequestMappingInfo bestMappingMatch = matches.get(0);
-			}
+				if (bestMappingMatch.matchedPaths.length > 0) {
-			else if (!targetHandlerMethods.isEmpty()) {
+					extractHandlerMethodUriTemplates(bestMappingMatch.matchedPaths[0], lookupPath, request);
-				RequestMappingInfo bestMappingMatch = null;
-				String bestPathMatch = null;
-				for (RequestMappingInfo mapping : targetHandlerMethods.keySet()) {
-					String mappedPath = targetPathMatches.get(mapping);
-					if (bestMappingMatch == null) {
-						bestMappingMatch = mapping;
-						bestPathMatch = mappedPath;
-					}
-					else {
-						if (isBetterPathMatch(mappedPath, bestPathMatch, lookupPath) ||
-								(!isBetterPathMatch(bestPathMatch, mappedPath, lookupPath) &&
-										(isBetterMethodMatch(mapping, bestMappingMatch) ||
-										(!isBetterMethodMatch(bestMappingMatch, mapping) &&
-												isBetterParamMatch(mapping, bestMappingMatch))))) {
-							bestMappingMatch = mapping;
-							bestPathMatch = mappedPath;
-						}
-					}
-				}
-				if (bestPathMatch != null) {
-					extractHandlerMethodUriTemplates(bestPathMatch, lookupPath, request);
 				}
 				return targetHandlerMethods.get(bestMappingMatch);
 			}
@@ -565,20 +551,6 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 					ServletAnnotationMappingUtils.checkParameters(mapping.params, request);
 		}
 
-		private boolean isBetterPathMatch(String mappedPath, String mappedPathToCompare, String lookupPath) {
-			return (mappedPath != null &&
-					(mappedPathToCompare == null || mappedPathToCompare.length() < mappedPath.length() ||
-							(mappedPath.equals(lookupPath) && !mappedPathToCompare.equals(lookupPath))));
-		}
-
-		private boolean isBetterMethodMatch(RequestMappingInfo mapping, RequestMappingInfo mappingToCompare) {
-			return (mappingToCompare.methods.length == 0 && mapping.methods.length > 0);
-		}
-
-		private boolean isBetterParamMatch(RequestMappingInfo mapping, RequestMappingInfo mappingToCompare) {
-			return (mappingToCompare.params.length < mapping.params.length);
-		}
-
 		@SuppressWarnings("unchecked")
 		private void extractHandlerMethodUriTemplates(String mappedPath, String lookupPath, HttpServletRequest request) {
 			Map<String, String> variables = null;
@@ -770,14 +742,20 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 	}
 
 
-	private static class RequestMappingInfo {
+	static class RequestMappingInfo {
 
-		public String[] paths = new String[0];
+		String[] paths = new String[0];
 
-		public RequestMethod[] methods = new RequestMethod[0];
+		String[] matchedPaths = new String[0];
 
-		public String[] params = new String[0];
+		RequestMethod[] methods = new RequestMethod[0];
 
+		String[] params = new String[0];
+
+		String bestMatchedPath() {
+			return matchedPaths.length > 0 ? matchedPaths[0] : null;
+		}
+		
 		@Override
 		public boolean equals(Object obj) {
 			RequestMappingInfo other = (RequestMappingInfo) obj;
@@ -792,4 +770,51 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 		}
 	}
 
+	/**
+	 * Comparator capable of sorting {@link RequestMappingInfo}s (RHIs) so that sorting a list with this comparator will
+	 * result in:
+	 * <ul>
+	 * <li>RHIs with {@linkplain RequestMappingInfo#matchedPaths 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 chards and uri templates will be ordered before those without.</li>
+	 * <li>RHIs with one single {@linkplain RequestMappingInfo#methods request method} will be ordered before those
+	 * without a method, or with more than one method.</li>
+	 * <li>RHIs with more {@linkplain RequestMappingInfo#params request parameters} will be ordered before those with
+	 * less parameters</li>
+	 * </ol>
+	 */
+	static class RequestMappingInfoComparator implements Comparator<RequestMappingInfo> {
+
+		private final Comparator<String> pathComparator;
+
+		RequestMappingInfoComparator(Comparator<String> pathComparator) {
+			this.pathComparator = pathComparator;
+		}
+
+		public int compare(RequestMappingInfo info1, RequestMappingInfo info2) {
+			int pathComparison = pathComparator.compare(info1.bestMatchedPath(), info2.bestMatchedPath());
+			if (pathComparison != 0) {
+				return pathComparison;
+			}
+			int info1MethodCount = info1.methods.length;
+			int info2MethodCount = info2.methods.length;
+			if (info1MethodCount == 0 && info2MethodCount > 0) {
+				return 1;
+			}
+			else if (info2MethodCount == 0 && info1MethodCount > 0) {
+				return -1;
+			}
+			else if (info1MethodCount == 1 & info2MethodCount > 1) {
+				return -1;
+
+			}
+			else if (info2MethodCount == 1 & info1MethodCount > 1) {
+				return 1;
+			}
+			int info1ParamCount = info1.params.length;
+			int info2ParamCount = info2.params.length;
+			return (info1ParamCount < info2ParamCount ? 1 : (info1ParamCount == info2ParamCount ? 0 : -1));
+		}
+	}
+	
 }

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTest.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright 2002-2009 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.web.servlet.mvc.annotation;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.springframework.web.bind.annotation.RequestMethod;
+
+/**
+ * @author Arjen Poutsma
+ */
+public class RequestMappingInfoComparatorTest {
+
+	private AnnotationMethodHandlerAdapter.RequestMappingInfoComparator comparator;
+
+	private AnnotationMethodHandlerAdapter.RequestMappingInfo emptyInfo;
+
+	private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodInfo;
+
+	private AnnotationMethodHandlerAdapter.RequestMappingInfo twoMethodsInfo;
+
+	private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodOneParamInfo;
+
+	private AnnotationMethodHandlerAdapter.RequestMappingInfo oneMethodTwoParamsInfo;
+
+	@Before
+	public void setUp() throws NoSuchMethodException {
+		comparator = new AnnotationMethodHandlerAdapter.RequestMappingInfoComparator(new MockComparator());
+
+		emptyInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
+
+		oneMethodInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
+		oneMethodInfo.methods = new RequestMethod[]{RequestMethod.GET};
+
+		twoMethodsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
+		twoMethodsInfo.methods = new RequestMethod[]{RequestMethod.GET, RequestMethod.POST};
+
+		oneMethodOneParamInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
+		oneMethodOneParamInfo.methods = new RequestMethod[]{RequestMethod.GET};
+		oneMethodOneParamInfo.params = new String[]{"param"};
+
+		oneMethodTwoParamsInfo = new AnnotationMethodHandlerAdapter.RequestMappingInfo();
+		oneMethodTwoParamsInfo.methods = new RequestMethod[]{RequestMethod.GET};
+		oneMethodTwoParamsInfo.params = new String[]{"param1", "param2"};
+	}
+
+	@Test
+	public void sort() {
+		List<AnnotationMethodHandlerAdapter.RequestMappingInfo> infos = new ArrayList<AnnotationMethodHandlerAdapter.RequestMappingInfo>();
+		infos.add(emptyInfo);
+		infos.add(oneMethodInfo);
+		infos.add(twoMethodsInfo);
+		infos.add(oneMethodOneParamInfo);
+		infos.add(oneMethodTwoParamsInfo);
+
+		Collections.shuffle(infos);
+		Collections.sort(infos, comparator);
+
+		assertEquals(oneMethodTwoParamsInfo, infos.get(0));
+		assertEquals(oneMethodOneParamInfo, infos.get(1));
+		assertEquals(oneMethodInfo, infos.get(2));
+		assertEquals(twoMethodsInfo, infos.get(3));
+		assertEquals(emptyInfo, infos.get(4));
+	}
+
+	private static class MockComparator implements Comparator<String> {
+
+		public int compare(String s1, String s2) {
+			return 0;
+		}
+	}
+
+}

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java

@@ -30,6 +30,7 @@ import java.util.Map;
 import java.util.Set;
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletContext;
+import javax.servlet.ServletException;
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -801,6 +802,24 @@ public class ServletAnnotationControllerTests {
 		}
 	}
 
+	@Test
+	public void pathOrdering() throws ServletException, IOException {
+		@SuppressWarnings("serial") DispatcherServlet servlet = new DispatcherServlet() {
+			@Override
+			protected WebApplicationContext createWebApplicationContext(WebApplicationContext parent) {
+				GenericWebApplicationContext wac = new GenericWebApplicationContext();
+				wac.registerBeanDefinition("controller", new RootBeanDefinition(PathOrderingController.class));
+				wac.refresh();
+				return wac;
+			}
+		};
+		servlet.init(new MockServletConfig());
+
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/dir/myPath1.do");
+		MockHttpServletResponse response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("method1", response.getContentAsString());
+	}
 
 	/*
 	 * Controllers
@@ -1350,4 +1369,19 @@ public class ServletAnnotationControllerTests {
 		}
 	}
 
+	@Controller
+	public static class PathOrderingController {
+
+
+		@RequestMapping(value = {"/dir/myPath1.do", "/**/*.do"})
+		public void method1(Writer writer) throws IOException {
+			writer.write("method1");
+		}
+
+		@RequestMapping("/dir/*.do")
+		public void method2(Writer writer) throws IOException {
+			writer.write("method2");
+		}
+	}
+
 }

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java

@@ -65,7 +65,7 @@ public class UriTemplateServletAnnotationControllerTests {
 	public void ambiguous() throws Exception {
 		initServlet(AmbiguousUriTemplateController.class);
 
-		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels/12345");
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels/new");
 		MockHttpServletResponse response = new MockHttpServletResponse();
 		servlet.service(request, response);
 		assertEquals("specific", response.getContentAsString());
@@ -162,16 +162,25 @@ public class UriTemplateServletAnnotationControllerTests {
 	@Controller
 	public static class AmbiguousUriTemplateController {
 
-		@RequestMapping("/hotels/{hotel}")
+		@RequestMapping("/hotels/new")
-		public void handleVars(Writer writer) throws IOException {
-			writer.write("variables");
-		}
-
-		@RequestMapping("/hotels/12345")
 		public void handleSpecific(Writer writer) throws IOException {
 			writer.write("specific");
 		}
 
+/*
+		@RequestMapping("/hotels/{hotel}")
+		public void handleVars(@PathVariable("hotel") String hotel, Writer writer) throws IOException {
+			assertEquals("Invalid path variable value", "42", hotel);
+			writer.write("variables");
+		}
+*/
+
+		@RequestMapping("/hotels/*")
+		public void handleWildCard(Writer writer) throws IOException {
+			writer.write("wildcard");
+		}
+
+
 	}
 
 }