Compare encoded params in AbstractFlashMapManager

AbstractFlashMapManager no longer decodes the target query parameters
it needs to use to match to the request after the redirect.

Instead it stores query parameters as-is adn then relies on parsing the
encoded query string after the redirect.

Issue: SPR-12569

spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -16,11 +16,11 @@
 
 package org.springframework.web.servlet.support;
 
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
+
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
@@ -30,12 +30,13 @@ import org.apache.commons.logging.LogFactory;
 import org.springframework.util.Assert;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.MultiValueMap;
-import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
 import org.springframework.web.servlet.FlashMap;
 import org.springframework.web.servlet.FlashMapManager;
+import org.springframework.web.util.UriComponents;
 import org.springframework.web.util.UrlPathHelper;
 
+
 /**
  * A base class for {@link FlashMapManager} implementations.
  *
@@ -172,10 +173,16 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 				return false;
 			}
 		}
-		MultiValueMap<String, String> targetParams = flashMap.getTargetRequestParams();
-		for (String expectedName : targetParams.keySet()) {
-			for (String expectedValue : targetParams.get(expectedName)) {
-				if (!ObjectUtils.containsElement(request.getParameterValues(expectedName), expectedValue)) {
+		UriComponents uriComponents = ServletUriComponentsBuilder.fromRequest(request).build();
+		MultiValueMap<String, String> actualParams = uriComponents.getQueryParams();
+		MultiValueMap<String, String> expectedParams = flashMap.getTargetRequestParams();
+		for (String expectedName : expectedParams.keySet()) {
+			List<String> actualValues = actualParams.get(expectedName);
+			if (actualValues == null) {
+				return false;
+			}
+			for (String expectedValue : expectedParams.get(expectedName)) {
+				if (!actualValues.contains(expectedValue)) {
 					return false;
 				}
 			}
@@ -191,7 +198,6 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 
 		String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request);
 		flashMap.setTargetRequestPath(path);
-		decodeParameters(flashMap.getTargetRequestParams(), request);
 
 		if (logger.isDebugEnabled()) {
 			logger.debug("Saving FlashMap=" + flashMap);
@@ -227,17 +233,6 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 		return path;
 	}
 
-	private void decodeParameters(MultiValueMap<String, String> params, HttpServletRequest request) {
-		for (String name : new ArrayList<String>(params.keySet())) {
-			for (String value : new ArrayList<String>(params.remove(name))) {
-				name = getUrlPathHelper().decodeRequestString(request, name);
-				value = getUrlPathHelper().decodeRequestString(request, value);
-				params.add(name, value);
-			}
-		}
-	}
-
-
 	/**
 	 * Retrieve saved FlashMap instances from the underlying storage.
 	 * @param request the current request

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -16,6 +16,8 @@
 
 package org.springframework.web.servlet.mvc.method.annotation;
 
+import static org.junit.Assert.*;
+
 import java.beans.PropertyEditorSupport;
 import java.io.IOException;
 import java.io.Serializable;
@@ -42,6 +44,7 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
+
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
@@ -137,8 +140,6 @@ import org.springframework.web.servlet.mvc.support.RedirectAttributes;
 import org.springframework.web.servlet.support.RequestContextUtils;
 import org.springframework.web.servlet.view.InternalResourceViewResolver;
 
-import static org.junit.Assert.*;
-
 /**
  * The origin of this test class is {@link ServletAnnotationControllerHandlerMethodTests}.
  *
@@ -1532,7 +1533,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl
 
 		// GET after POST
 		request = new MockHttpServletRequest("GET", "/messages/1");
-		request.addParameter("name", "value");
+		request.setQueryString("name=value");
 		request.setSession(session);
 		response = new MockHttpServletResponse();
 		getServlet().service(request, response);

spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java

@@ -18,6 +18,7 @@ package org.springframework.web.servlet.support;
 
 import static org.junit.Assert.*;
 
+import java.net.URLEncoder;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -31,7 +32,6 @@ import org.junit.Test;
 
 import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.mock.web.test.MockHttpServletResponse;
-import org.springframework.util.MultiValueMap;
 import org.springframework.web.servlet.FlashMap;
 import org.springframework.web.util.WebUtils;
 
@@ -113,19 +113,19 @@ public class FlashMapManagerTests {
 
 		this.flashMapManager.setFlashMaps(Arrays.asList(flashMap));
 
-		this.request.setParameter("number", (String) null);
+		this.request.setQueryString("number=");
 		FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response);
 
 		assertNull(inputFlashMap);
 		assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size());
 
-		this.request.setParameter("number", "two");
+		this.request.setQueryString("number=two");
 		inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response);
 
 		assertNull(inputFlashMap);
 		assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size());
 
-		this.request.setParameter("number", "one");
+		this.request.setQueryString("number=one");
 		inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response);
 
 		assertEquals(flashMap, inputFlashMap);
@@ -143,13 +143,13 @@ public class FlashMapManagerTests {
 
 		this.flashMapManager.setFlashMaps(Arrays.asList(flashMap));
 
-		this.request.setParameter("id", "1");
+		this.request.setQueryString("id=1");
 		FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response);
 
 		assertNull(inputFlashMap);
 		assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size());
 
-		this.request.addParameter("id", "2");
+		this.request.setQueryString("id=1&id=2");
 		inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response);
 
 		assertEquals(flashMap, inputFlashMap);
@@ -268,20 +268,54 @@ public class FlashMapManagerTests {
 
 	@Test
 	public void saveOutputFlashMapDecodeParameters() throws Exception {
+
+		FlashMap flashMap = new FlashMap();
+		flashMap.put("key", "value");
+		flashMap.setTargetRequestPath("/path");
+		flashMap.addTargetRequestParam("param", "%D0%90%D0%90");
+		flashMap.addTargetRequestParam("param", "%D0%91%D0%91");
+		flashMap.addTargetRequestParam("param", "%D0%92%D0%92");
+		flashMap.addTargetRequestParam("%3A%2F%3F%23%5B%5D%40", "value");
+
 		this.request.setCharacterEncoding("UTF-8");
+		this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response);
+
+		MockHttpServletRequest requestAfterRedirect = new MockHttpServletRequest("GET", "/path");
+		requestAfterRedirect.setQueryString("param=%D0%90%D0%90&param=%D0%91%D0%91&param=%D0%92%D0%92&%3A%2F%3F%23%5B%5D%40=value");
+		requestAfterRedirect.addParameter("param", "\u0410\u0410");
+		requestAfterRedirect.addParameter("param", "\u0411\u0411");
+		requestAfterRedirect.addParameter("param", "\u0412\u0412");
+		requestAfterRedirect.addParameter(":/?#[]@", "value");
+
+		flashMap = this.flashMapManager.retrieveAndUpdate(requestAfterRedirect, new MockHttpServletResponse());
+		assertNotNull(flashMap);
+		assertEquals(1, flashMap.size());
+		assertEquals("value", flashMap.get("key"));
+	}
+
+	// SPR-12569
+
+	@Test
+	public void flashAttributesWithQueryParamsWithSpace() throws Exception {
+
+		String encodedValue = URLEncoder.encode("1 2", "UTF-8");
 
 		FlashMap flashMap = new FlashMap();
-		flashMap.put("anyKey", "anyValue");
+		flashMap.put("key", "value");
+		flashMap.setTargetRequestPath("/path");
+		flashMap.addTargetRequestParam("param", encodedValue);
 
-		flashMap.addTargetRequestParam("key", "%D0%90%D0%90");
-		flashMap.addTargetRequestParam("key", "%D0%91%D0%91");
-		flashMap.addTargetRequestParam("key", "%D0%92%D0%92");
-		flashMap.addTargetRequestParam("%3A%2F%3F%23%5B%5D%40", "value");
+		this.request.setCharacterEncoding("UTF-8");
 		this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response);
 
-		MultiValueMap<String,String> targetRequestParams = flashMap.getTargetRequestParams();
-		assertEquals(Arrays.asList("\u0410\u0410", "\u0411\u0411", "\u0412\u0412"), targetRequestParams.get("key"));
-		assertEquals(Arrays.asList("value"), targetRequestParams.get(":/?#[]@"));
+		MockHttpServletRequest requestAfterRedirect = new MockHttpServletRequest("GET", "/path");
+		requestAfterRedirect.setQueryString("param=" + encodedValue);
+		requestAfterRedirect.addParameter("param", "1 2");
+
+		flashMap = this.flashMapManager.retrieveAndUpdate(requestAfterRedirect, new MockHttpServletResponse());
+		assertNotNull(flashMap);
+		assertEquals(1, flashMap.size());
+		assertEquals("value", flashMap.get("key"));
 	}