Decode path vars when url decoding is turned off

When URL decoding is turned off in AbstractHandlerMapping, the
extracted path variables are also not encoded. Turning off URL decoding
may be necessary for request mapping to work correctly when the path
may contain the (encoded) special character '/'. At the same time there
is no good reason not to leave path variables encoded. This change
ensures path variables are encoded when URL decoding is turned off.

Issue: SPR-9098

Author: rstoyanchev

spring-web/src/main/java/org/springframework/web/util/UrlPathHelper.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2010 the original author or authors.
+ * Copyright 2002-2012 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.
@@ -18,12 +18,15 @@
 
 import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Properties;
+
 import javax.servlet.http.HttpServletRequest;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.util.StringUtils;
 
 /**
@@ -37,6 +40,7 @@
  *
  * @author Juergen Hoeller
  * @author Rob Harrop
+ * @author Rossen Stoyanchev
  * @since 14.01.2004
  */
 public class UrlPathHelper {
@@ -301,7 +305,7 @@ public String getOriginatingServletPath(HttpServletRequest request) {
 	 * @return the query string
 	 */
 	public String getOriginatingQueryString(HttpServletRequest request) {
-		if ((request.getAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE) != null) || 
+		if ((request.getAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE) != null) ||
 			(request.getAttribute(WebUtils.ERROR_REQUEST_URI_ATTRIBUTE) != null)) {
 			return (String) request.getAttribute(WebUtils.FORWARD_QUERY_STRING_ATTRIBUTE);
 		}
@@ -333,21 +337,25 @@ private String decodeAndCleanUriString(HttpServletRequest request, String uri) {
 	 */
 	public String decodeRequestString(HttpServletRequest request, String source) {
 		if (this.urlDecode) {
-			String enc = determineEncoding(request);
-			try {
-				return UriUtils.decode(source, enc);
-			}
-			catch (UnsupportedEncodingException ex) {
-				if (logger.isWarnEnabled()) {
-					logger.warn("Could not decode request string [" + source + "] with encoding '" + enc +
-							"': falling back to platform default encoding; exception message: " + ex.getMessage());
-				}
-				return URLDecoder.decode(source);
-			}
+			return decodeInternal(request, source);
 		}
 		return source;
 	}
 
+	private String decodeInternal(HttpServletRequest request, String source) {
+		String enc = determineEncoding(request);
+		try {
+			return UriUtils.decode(source, enc);
+		}
+		catch (UnsupportedEncodingException ex) {
+			if (logger.isWarnEnabled()) {
+				logger.warn("Could not decode request string [" + source + "] with encoding '" + enc +
+						"': falling back to platform default encoding; exception message: " + ex.getMessage());
+			}
+			return URLDecoder.decode(source);
+		}
+	}
+
 	/**
 	 * Determine the encoding for the given request.
 	 * Can be overridden in subclasses.
@@ -366,6 +374,27 @@ protected String determineEncoding(HttpServletRequest request) {
 		return enc;
 	}
 
+	/**
+	 * Decode the given URI path variables via {@link #decodeRequestString(HttpServletRequest, String)}
+	 * unless {@link #setUrlDecode(boolean)} is set to {@code true} in which case
+	 * it is assumed the URL path from which the variables were extracted is
+	 * already decoded through a call to {@link #getLookupPathForRequest(HttpServletRequest)}.
+	 * @param request current HTTP request
+	 * @param vars URI variables extracted from the URL path
+	 * @return the same Map or a new Map instance
+	 */
+	public Map<String, String> decodePathVariables(HttpServletRequest request, Map<String, String> vars) {
+		if (this.urlDecode) {
+			return vars;
+		}
+		else {
+			Map<String, String> decodedVars = new LinkedHashMap<String, String>(vars.size());
+			for (Entry<String, String> entry : vars.entrySet()) {
+				decodedVars.put(entry.getKey(), decodeInternal(request, entry.getValue()));
+			}
+			return decodedVars;
+		}
+	}
 
 	private boolean shouldRemoveTrailingServletPathSlash(HttpServletRequest request) {
 		if (request.getAttribute(WEBSPHERE_URI_ATTRIBUTE) == null) {

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java

@@ -88,9 +88,10 @@ protected void handleMatch(RequestMappingInfo info, String lookupPath, HttpServl
 		Set<String> patterns = info.getPatternsCondition().getPatterns();
 		String bestPattern = patterns.isEmpty() ? lookupPath : patterns.iterator().next();
 		request.setAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE, bestPattern);
-		
-		Map<String, String> uriTemplateVariables = getPathMatcher().extractUriTemplateVariables(bestPattern, lookupPath);
-		request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVariables);
+
+		Map<String, String> vars = getPathMatcher().extractUriTemplateVariables(bestPattern, lookupPath);
+		Map<String, String> decodedVars = getUrlPathHelper().decodePathVariables(request, vars);
+		request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, decodedVars);
 
 		if (!info.getProducesCondition().getProducibleMediaTypes().isEmpty()) {
 			Set<MediaType> mediaTypes = info.getProducesCondition().getProducibleMediaTypes();

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 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.
@@ -131,7 +131,7 @@ public void bestMatch() throws Exception {
 		HandlerMethod hm = (HandlerMethod) this.mapping.getHandler(request).getHandler();
 		assertEquals(this.fooParamMethod.getMethod(), hm.getMethod());
 	}
-	
+
 	@Test
 	public void requestMethodNotAllowed() throws Exception {
 		try {
@@ -143,7 +143,7 @@ public void requestMethodNotAllowed() throws Exception {
 			assertArrayEquals("Invalid supported methods", new String[]{"GET", "HEAD"}, ex.getSupportedMethods());
 		}
 	}
-	
+
 	@Test
 	public void mediaTypeNotSupported() throws Exception {
 		testMediaTypeNotSupported("/person/1");
@@ -159,11 +159,11 @@ private void testMediaTypeNotSupported(String url) throws Exception {
 			fail("HttpMediaTypeNotSupportedException expected");
 		}
 		catch (HttpMediaTypeNotSupportedException ex) {
-			assertEquals("Invalid supported consumable media types", 
+			assertEquals("Invalid supported consumable media types",
 					Arrays.asList(new MediaType("application", "xml")), ex.getSupportedMediaTypes());
 		}
 	}
-	
+
 	@Test
 	public void mediaTypeNotAccepted() throws Exception {
 		testMediaTypeNotAccepted("/persons");
@@ -179,7 +179,7 @@ private void testMediaTypeNotAccepted(String url) throws Exception {
 			fail("HttpMediaTypeNotAcceptableException expected");
 		}
 		catch (HttpMediaTypeNotAcceptableException ex) {
-			assertEquals("Invalid supported producible media types", 
+			assertEquals("Invalid supported producible media types",
 					Arrays.asList(new MediaType("application", "xml")), ex.getSupportedMediaTypes());
 		}
 	}
@@ -191,25 +191,49 @@ public void uriTemplateVariables() {
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
 		String lookupPath = new UrlPathHelper().getLookupPathForRequest(request);
 		this.mapping.handleMatch(key, lookupPath, request);
-		
+
 		@SuppressWarnings("unchecked")
-		Map<String, String> uriVariables = 
+		Map<String, String> uriVariables =
 			(Map<String, String>) request.getAttribute(
 					HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE);
-		
+
 		assertNotNull(uriVariables);
 		assertEquals("1", uriVariables.get("path1"));
 		assertEquals("2", uriVariables.get("path2"));
 	}
 
+	// SPR-9098
+
+	@Test
+	public void uriTemplateVariablesDecode() {
+		PatternsRequestCondition patterns = new PatternsRequestCondition("/{group}/{identifier}");
+		RequestMappingInfo key = new RequestMappingInfo(patterns, null, null, null, null, null, null);
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/group/a%2Fb");
+
+		UrlPathHelper pathHelper = new UrlPathHelper();
+		pathHelper.setUrlDecode(false);
+		String lookupPath = pathHelper.getLookupPathForRequest(request);
+
+		this.mapping.setUrlPathHelper(pathHelper);
+		this.mapping.handleMatch(key, lookupPath, request);
+
+		@SuppressWarnings("unchecked")
+		Map<String, String> uriVariables =
+			(Map<String, String>) request.getAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE);
+
+		assertNotNull(uriVariables);
+		assertEquals("group", uriVariables.get("group"));
+		assertEquals("a/b", uriVariables.get("identifier"));
+	}
+
 	@Test
 	public void bestMatchingPatternAttribute() {
 		PatternsRequestCondition patterns = new PatternsRequestCondition("/{path1}/2", "/**");
 		RequestMappingInfo key = new RequestMappingInfo(patterns, null, null, null, null, null, null);
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
 
 		this.mapping.handleMatch(key, "/1/2", request);
-		
+
 		assertEquals("/{path1}/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
 	}
 
@@ -220,7 +244,7 @@ public void bestMatchingPatternAttributeNoPatternsDefined() {
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
 
 		this.mapping.handleMatch(key, "/1/2", request);
-		
+
 		assertEquals("/1/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
 	}
 
@@ -237,10 +261,10 @@ public void producibleMediaTypesAttribute() throws Exception {
 		request.addHeader("Accept", "application/json");
 		this.mapping.getHandler(request);
 
-		assertNull("Negated expression should not be listed as a producible type", 
+		assertNull("Negated expression should not be listed as a producible type",
 				request.getAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE));
 	}
-	
+
 	@Test
 	public void mappedInterceptors() throws Exception {
 		String path = "/foo";
@@ -268,7 +292,7 @@ private static class Handler {
 		@RequestMapping(value = "/foo", method = RequestMethod.GET)
 		public void foo() {
 		}
-		
+
 		@RequestMapping(value = "/foo", method = RequestMethod.GET, params="p")
 		public void fooParam() {
 		}
@@ -306,7 +330,7 @@ private static class TestRequestMappingInfoHandlerMapping extends RequestMapping
 		public void registerHandler(Object handler) {
 			super.detectHandlerMethods(handler);
 		}
-		
+
 		@Override
 		protected boolean isHandler(Class<?> beanType) {
 			return AnnotationUtils.findAnnotation(beanType, RequestMapping.class) != null;
@@ -329,5 +353,5 @@ protected RequestMappingInfo getMappingForMethod(Method method, Class<?> handler
 			}
 		}
 	}
-	
+
 }

src/dist/changelog.txt

@@ -27,6 +27,7 @@ Changes in version 3.2 M1
 * add CompositeRequestCondition for use with multiple custom request mapping conditions
 * add ability to configure custom MessageCodesResolver through the MVC Java config
 * add option in MappingJacksonJsonView for setting the Content-Length header
+* decode path variables when url decoding is turned off in AbstractHandlerMapping
 
 Changes in version 3.1.1 (2012-02-16)
 -------------------------------------