SPR-6378 - RC2: Issue with RequestMethod.GET differs from M2

git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@2507 50f2f4bb-b051-0410-bef5-90022cba6387

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/HandlerMapping.java

@@ -64,6 +64,16 @@ public interface HandlerMapping {
 	 */
 	String PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE = HandlerMapping.class.getName() + ".pathWithinHandlerMapping";
 
+	/**
+	 * Name of the {@link HttpServletRequest} attribute that contains the
+	 * best matching pattern within the handler mapping.
+	 * <p>Note: This attribute is not required to be supported by all
+	 * HandlerMapping implementations. URL-based HandlerMappings will
+	 * typically support it, but handlers should not necessarily expect
+	 * this request attribute to be present in all scenarios.
+	 */
+	String BEST_MATCHING_PATTERN_ATTRIBUTE = HandlerMapping.class.getName() + ".bestMatchingPattern";
+
 	/**
 	 * Name of the {@link HttpServletRequest} attribute that contains the URI
 	 * templates map, mapping variable names to values.
@@ -74,7 +84,6 @@ public interface HandlerMapping {
 	 */
 	String URI_TEMPLATE_VARIABLES_ATTRIBUTE = HandlerMapping.class.getName() + ".uriTemplateVariables";
 
-
 	/**
 	 * Return a handler and any interceptors for this request. The choice may be made
 	 * on request URL, session state, or any factor the implementing class chooses.

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/handler/AbstractUrlHandlerMapping.java

@@ -178,7 +178,7 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping {
 					rawHandler = getApplicationContext().getBean(handlerName);
 				}
 				validateHandler(rawHandler, request);
-				handler = buildPathExposingHandler(rawHandler, lookupPath, null);
+				handler = buildPathExposingHandler(rawHandler, lookupPath, lookupPath, null);
 			}
 		}
 		if (handler != null && logger.isDebugEnabled()) {
@@ -213,35 +213,35 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping {
 				handler = getApplicationContext().getBean(handlerName);
 			}
 			validateHandler(handler, request);
-			return buildPathExposingHandler(handler, urlPath, null);
+			return buildPathExposingHandler(handler, urlPath, urlPath, null);
 		}
 		// Pattern match?
-		List<String> matchingPaths = new ArrayList<String>();
-		for (String registeredPath : this.handlerMap.keySet()) {
-			if (getPathMatcher().match(registeredPath, urlPath)) {
-				matchingPaths.add(registeredPath);
+		List<String> matchingPatterns = new ArrayList<String>();
+		for (String registeredPattern : this.handlerMap.keySet()) {
+			if (getPathMatcher().match(registeredPattern, urlPath)) {
+				matchingPatterns.add(registeredPattern);
 			}
 		}
-		String bestPathMatch = null;
-		if (!matchingPaths.isEmpty()) {
-			Collections.sort(matchingPaths, getPathMatcher().getPatternComparator(urlPath));
+		String bestPatternMatch = null;
+		if (!matchingPatterns.isEmpty()) {
+			Collections.sort(matchingPatterns, getPathMatcher().getPatternComparator(urlPath));
 			if (logger.isDebugEnabled()) {
-				logger.debug("Matching path for request [" + urlPath + "] are " + matchingPaths);
+				logger.debug("Matching patterns for request [" + urlPath + "] are " + matchingPatterns);
 			}
-			bestPathMatch = matchingPaths.get(0);
+			bestPatternMatch = matchingPatterns.get(0);
 		}
-		if (bestPathMatch != null) {
-			handler = this.handlerMap.get(bestPathMatch);
+		if (bestPatternMatch != null) {
+			handler = this.handlerMap.get(bestPatternMatch);
 			// Bean name or resolved handler?
 			if (handler instanceof String) {
 				String handlerName = (String) handler;
 				handler = getApplicationContext().getBean(handlerName);
 			}
 			validateHandler(handler, request);
-			String pathWithinMapping = getPathMatcher().extractPathWithinPattern(bestPathMatch, urlPath);
+			String pathWithinMapping = getPathMatcher().extractPathWithinPattern(bestPatternMatch, urlPath);
 			Map<String, String> uriTemplateVariables =
-					getPathMatcher().extractUriTemplateVariables(bestPathMatch, urlPath);
-			return buildPathExposingHandler(handler, pathWithinMapping, uriTemplateVariables);
+					getPathMatcher().extractUriTemplateVariables(bestPatternMatch, urlPath);
+			return buildPathExposingHandler(handler, bestPatternMatch, pathWithinMapping, uriTemplateVariables);
 		}
 		// No handler found...
 		return null;
@@ -269,11 +269,13 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping {
 	 * @param uriTemplateVariables the URI template variables, can be <code>null</code> if no variables found
 	 * @return the final handler object
 	 */
-	protected Object buildPathExposingHandler(
-			Object rawHandler, String pathWithinMapping, Map<String, String> uriTemplateVariables) {
+	protected Object buildPathExposingHandler(Object rawHandler,
+			String bestMatchingPattern,
+			String pathWithinMapping,
+			Map<String, String> uriTemplateVariables) {
 
 		HandlerExecutionChain chain = new HandlerExecutionChain(rawHandler);
-		chain.addInterceptor(new PathExposingHandlerInterceptor(pathWithinMapping));
+		chain.addInterceptor(new PathExposingHandlerInterceptor(bestMatchingPattern, pathWithinMapping));
 		if (!CollectionUtils.isEmpty(uriTemplateVariables)) {
 			chain.addInterceptor(new UriTemplateVariablesHandlerInterceptor(uriTemplateVariables));
 		}
@@ -286,7 +288,8 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping {
 	 * @param request the request to expose the path to
 	 * @see #PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE
 	 */
-	protected void exposePathWithinMapping(String pathWithinMapping, HttpServletRequest request) {
+	protected void exposePathWithinMapping(String bestMatchingPattern, String pathWithinMapping, HttpServletRequest request) {
+		request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, bestMatchingPattern);
 		request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, pathWithinMapping);
 	}
 
@@ -384,15 +387,18 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping {
 	 */
 	private class PathExposingHandlerInterceptor extends HandlerInterceptorAdapter {
 
+		private final String bestMatchingPattern;
+
 		private final String pathWithinMapping;
 
-		private PathExposingHandlerInterceptor(String pathWithinMapping) {
+		private PathExposingHandlerInterceptor(String bestMatchingPattern, String pathWithinMapping) {
+			this.bestMatchingPattern = bestMatchingPattern;
 			this.pathWithinMapping = pathWithinMapping;
 		}
 
 		@Override
 		public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
-			exposePathWithinMapping(this.pathWithinMapping, request);
+			exposePathWithinMapping(this.bestMatchingPattern, this.pathWithinMapping, request);
 			return true;
 		}
 	}

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

@@ -432,7 +432,7 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 				if (mappingInfo.paths.length > 0) {
 					List<String> matchedPaths = new ArrayList<String>(mappingInfo.paths.length);
 					for (String methodLevelPattern : mappingInfo.paths) {
-						String matchedPattern = getMatchedPattern(methodLevelPattern, lookupPath);
+						String matchedPattern = getMatchedPattern(methodLevelPattern, lookupPath, request);
 						if (matchedPattern != null) {
 							if (mappingInfo.matches(request)) {
 								match = true;
@@ -518,12 +518,23 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 			}
 		}
 
-		private String getMatchedPattern(String methodLevelPattern, String lookupPath) {
-			if ((!hasTypeLevelMapping() || ObjectUtils.isEmpty(getTypeLevelMapping().value())) &&
-					isPathMatchInternal(methodLevelPattern, lookupPath)) {
-				return methodLevelPattern;
-			}
-			if (hasTypeLevelMapping()) {
+		/**
+		 * Determines the matched pattern for the given methodLevelPattern and path.
+		 *
+		 * <p>Uses the following algorithm:
+		 * <ol>
+		 * <li>If there is a type-level mapping with path information, it is
+		 * {@linkplain PathMatcher#combine(String, String) combined} with the method-level pattern.
+		 * <li>If there is a {@linkplain HandlerMapping#BEST_MATCHING_PATTERN_ATTRIBUTE best matching pattern} in the
+		 * request, it is combined with the method-level pattern.
+		 * <li>Otherwise, 
+		 * @param methodLevelPattern
+		 * @param lookupPath
+		 * @param request
+		 * @return
+		 */
+		private String getMatchedPattern(String methodLevelPattern, String lookupPath, HttpServletRequest request) {
+			if (hasTypeLevelMapping() && (!ObjectUtils.isEmpty(getTypeLevelMapping().value()))) {
 				String[] typeLevelPatterns = getTypeLevelMapping().value();
 				for (String typeLevelPattern : typeLevelPatterns) {
 					if (!typeLevelPattern.startsWith("/")) {
@@ -534,7 +545,17 @@ public class AnnotationMethodHandlerAdapter extends WebContentGenerator implemen
 						return combinedPattern;
 					}
 				}
-
+				return null;
+			}
+			String bestMatchingPattern = (String) request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE);
+			if (StringUtils.hasText(bestMatchingPattern)) {
+				String combinedPattern = pathMatcher.combine(bestMatchingPattern, methodLevelPattern);
+				if (!combinedPattern.equals(bestMatchingPattern) && (isPathMatchInternal(combinedPattern, lookupPath))) {
+					return combinedPattern;
+				}
+			}
+			if (isPathMatchInternal(methodLevelPattern, lookupPath)) {
+				return methodLevelPattern;
 			}
 			return null;
 		}

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

@@ -0,0 +1,52 @@
+/*
+ * 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.io.IOException;
+import java.io.Writer;
+
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.RequestMapping;
+import org.springframework.web.bind.annotation.RequestMethod;
+import org.springframework.web.bind.annotation.RequestParam;
+
+/**
+ * Used for testing the combination of ControllerClassNameHandlerMapping/SimpleUrlHandlerMapping with @RequestParam in
+ * {@link ServletAnnotationControllerTests}. Implemented as a top-level class (rather than an inner class) to make the
+ * ControllerClassNameHandlerMapping work.
+ *
+ * @author Arjen Poutsma
+ */
+@Controller
+public class BookController {
+
+	@RequestMapping("list")
+	public void list(Writer writer) throws IOException {
+		writer.write("list");
+	}
+
+	@RequestMapping("show")
+	public void show(@RequestParam(required = true) Long id, Writer writer) throws IOException {
+		writer.write("show-id=" + id);
+	}
+
+	@RequestMapping(method = RequestMethod.POST)
+	public void create(Writer writer) throws IOException {
+		writer.write("create");
+	}
+
+}

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

@@ -55,6 +55,7 @@ import org.springframework.aop.support.DefaultPointcutAdvisor;
 import org.springframework.beans.DerivedTestBean;
 import org.springframework.beans.ITestBean;
 import org.springframework.beans.TestBean;
+import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.beans.factory.config.PropertyPlaceholderConfigurer;
@@ -107,6 +108,7 @@ import org.springframework.web.servlet.DispatcherServlet;
 import org.springframework.web.servlet.ModelAndView;
 import org.springframework.web.servlet.View;
 import org.springframework.web.servlet.ViewResolver;
+import org.springframework.web.servlet.handler.SimpleUrlHandlerMapping;
 import org.springframework.web.servlet.mvc.AbstractController;
 import org.springframework.web.servlet.mvc.multiaction.InternalPathMethodNameResolver;
 import org.springframework.web.servlet.mvc.support.ControllerClassNameHandlerMapping;
@@ -1159,6 +1161,75 @@ public class ServletAnnotationControllerTests {
 		assertEquals("Content-Type=[text/html],Custom-Header=[value21,value22]", response.getContentAsString());
 	}
 
+	@Test
+	public void controllerClassNameNoTypeLevelAnn() throws Exception {
+		servlet = new DispatcherServlet() {
+			@Override
+			protected WebApplicationContext createWebApplicationContext(WebApplicationContext parent)
+					throws BeansException {
+				GenericWebApplicationContext wac = new GenericWebApplicationContext();
+				wac.registerBeanDefinition("controller", new RootBeanDefinition(BookController.class));
+				RootBeanDefinition mapping = new RootBeanDefinition(ControllerClassNameHandlerMapping.class);
+				mapping.getPropertyValues().add("excludedPackages", null);
+				wac.registerBeanDefinition("handlerMapping", mapping);
+				wac.refresh();
+				return wac;
+			}
+		};
+		servlet.init(new MockServletConfig());
+
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/book/list");
+		MockHttpServletResponse response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("list", response.getContentAsString());
+
+		request = new MockHttpServletRequest("GET", "/book/show");
+		request.addParameter("id", "12");
+		response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("show-id=12", response.getContentAsString());
+
+		request = new MockHttpServletRequest("POST", "/book");
+		response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("create", response.getContentAsString());
+	}
+
+	@Test
+	public void simpleUrlHandlerMapping() throws Exception {
+		servlet = new DispatcherServlet() {
+			@Override
+			protected WebApplicationContext createWebApplicationContext(WebApplicationContext parent)
+					throws BeansException {
+				GenericWebApplicationContext wac = new GenericWebApplicationContext();
+				wac.registerBeanDefinition("controller", new RootBeanDefinition(BookController.class));
+				RootBeanDefinition hmDef = new RootBeanDefinition(SimpleUrlHandlerMapping.class);
+				hmDef.getPropertyValues().add("mappings", "/book/*=controller\n/book=controller");
+				wac.registerBeanDefinition("handlerMapping", hmDef);
+				wac.refresh();
+				return wac;
+			}
+		};
+		servlet.init(new MockServletConfig());
+
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/book/list");
+		MockHttpServletResponse response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("list", response.getContentAsString());
+
+		request = new MockHttpServletRequest("GET", "/book/show");
+		request.addParameter("id", "12");
+		response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("show-id=12", response.getContentAsString());
+
+		request = new MockHttpServletRequest("POST", "/book");
+		response = new MockHttpServletResponse();
+		servlet.service(request, response);
+		assertEquals("create", response.getContentAsString());
+	}
+
+
 
 	/*
 	 * Controllers
@@ -2033,5 +2104,5 @@ public class ServletAnnotationControllerTests {
 
 	}
 
-
+	
 }