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 86f189fa6eb975015a7dd446d3deb7edaebeed00..cd39a07f6bde7276eef61aecd43718aa04fd0e2e 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 @@ -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 pathComparator = pathMatcher.getPatternComparator(lookupPath); Map targetHandlerMethods = new LinkedHashMap(); - Map targetPathMatches = new LinkedHashMap(); Set allowedMethods = new LinkedHashSet(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 matchedPaths = new ArrayList(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 (targetPathMatches.size() == 1) { - extractHandlerMethodUriTemplates(targetPathMatches.values().iterator().next(), lookupPath, request); - } - return targetHandlerMethods.values().iterator().next(); - } - else if (!targetHandlerMethods.isEmpty()) { - 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); + if (!targetHandlerMethods.isEmpty()) { + List matches = new ArrayList(targetHandlerMethods.keySet()); + RequestMappingInfoComparator requestMappingInfoComparator = new RequestMappingInfoComparator(pathComparator); + Collections.sort(matches, requestMappingInfoComparator); + RequestMappingInfo bestMappingMatch = matches.get(0); + if (bestMappingMatch.matchedPaths.length > 0) { + extractHandlerMethodUriTemplates(bestMappingMatch.matchedPaths[0], 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 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: + *
    + *
  • 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.
    • + *
  • RHIs with one single {@linkplain RequestMappingInfo#methods request method} will be ordered before those + * without a method, or with more than one method.
    • + *
  • RHIs with more {@linkplain RequestMappingInfo#params request parameters} will be ordered before those with + * less parameters
    • + * + */ + static class RequestMappingInfoComparator implements Comparator { + + private final Comparator pathComparator; + + RequestMappingInfoComparator(Comparator 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)); + } + } + } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTest.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/RequestMappingInfoComparatorTest.java new file mode 100644 index 0000000000000000000000000000000000000000..ce946354c2616007b156b973f6a0230ca0a550f2 --- /dev/null +++ b/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 infos = new ArrayList(); + 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 { + + public int compare(String s1, String s2) { + return 0; + } + } + +} diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java index 76526cb12755f3ac458d15c254fdaa53b230fecb..500ec6678be2f46eec19cfc717ffa9221036fb40 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java +++ b/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"); + } + } + } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java index db6d15b6da8d3725ef258fe46314371d4286538b..eb5a32f319225883673a81de149394bb9d6032f4 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/UriTemplateServletAnnotationControllerTests.java +++ b/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/new") + public void handleSpecific(Writer writer) throws IOException { + writer.write("specific"); + } + +/* @RequestMapping("/hotels/{hotel}") - public void handleVars(Writer writer) throws IOException { + public void handleVars(@PathVariable("hotel") String hotel, Writer writer) throws IOException { + assertEquals("Invalid path variable value", "42", hotel); writer.write("variables"); } +*/ - @RequestMapping("/hotels/12345") - public void handleSpecific(Writer writer) throws IOException { - writer.write("specific"); + @RequestMapping("/hotels/*") + public void handleWildCard(Writer writer) throws IOException { + writer.write("wildcard"); } + } }