SPR-6464 Use the redirect URL and its request parameters (if any), instead of a synthetic key, to match a FlashMap instance to its intended recepient request. This should help to prevent conflicts in most cases transparently. Only if necessary a controller can add extra request parameters to distinguish the request even more uniquely.

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

@@ -16,8 +16,15 @@
 
 package org.springframework.web.servlet;
 
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import javax.servlet.http.HttpServletRequest;
+
+import org.springframework.beans.BeanUtils;
 import org.springframework.ui.ModelMap;
-import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
+import org.springframework.web.util.UrlPathHelper;
 
 /**
  * Stores attributes that need to be made available in the next request.
@@ -25,52 +32,108 @@ import org.springframework.util.Assert;
  * @author Rossen Stoyanchev
  * @since 3.1
  */
-public class FlashMap extends ModelMap {
+public class FlashMap extends ModelMap implements Comparable<FlashMap> {
 
 	private static final long serialVersionUID = 1L;
-
-	private final String key;
-
-	private final String keyParameterName;
-
+	
+	private String expectedUrlPath;
+	
+	private Map<String, String> expectedRequestParameters = new LinkedHashMap<String, String>();
+	
+	private UrlPathHelper urlPathHelper = new UrlPathHelper();
+	
 	private long expirationStartTime;
 	
 	private int timeToLive;
-
+	
 	/**
-	 * Create a FlashMap with a unique key.
+	 * Provide a URL path to help identify the request this FlashMap should be
+	 * made available to. This will usually be the target URL of a redirect. 
+	 * If not set, this FlashMap will match to requests with any URL path.
+	 * 
+	 * <p>If the {@code url} parameter is not a URL path but is an absolute 
+	 * or a relative URL, the unnecessary parts of the URL are removed the 
+	 * resulting URL path is normalized.
+	 * 
+	 * @param request the current request
+	 * @param url a URL path, an absolute URL, or a relative URL 
 	 */
-	public FlashMap(String key, String keyParameterName) {
-		Assert.notNull("The key is required", key);
-		Assert.notNull("The key parameter name is required", keyParameterName);
-		this.key = key;
-		this.keyParameterName = keyParameterName;
+	public void setExpectedUrlPath(HttpServletRequest request, String url) {
+		this.expectedUrlPath = (url != null) ? normalizeRelativeUrlPath(request, extractUrlPath(url)) : null;
 	}
 
-	/**
-	 * Create a FlashMap without a key. 
-	 */
-	public FlashMap() {
-		this.key = null;
-		this.keyParameterName = null;
+	private String extractUrlPath(String url) {
+		int index = url.indexOf("://");
+		if (index != -1) {
+			index = url.indexOf("/", index + 3);
+			url = (index != -1) ? url.substring(index) : "";
+		}
+		index = url.indexOf("?");
+		return (index != -1) ? url.substring(0, index) : url;  
 	}
 
-	/**
-	 * Return the key assigned to this FlashMap instance; 
-	 * or {@code null} if a unique key has not been assigned.
-	 */
-	public String getKey() {
-		return this.key;
+	private String normalizeRelativeUrlPath(HttpServletRequest request, String path) {
+		if (!path.startsWith("/")) {
+			String requestUri = this.urlPathHelper.getRequestUri(request);
+			path = requestUri.substring(0, requestUri.lastIndexOf('/') + 1) + path;
+			path = StringUtils.cleanPath(path);
+		}
+		return path;
 	}
-
+	
 	/**
-	 * Return the name of the request parameter to use when appending the flash 
-	 * key to a redirect URL. 
+	 * Provide request parameter pairs to help identify the request this FlashMap 
+	 * should be made available to. If expected parameters are not set, this 
+	 * FlashMap instance will match to requests with any parameters. 
+	 * 
+	 * <p>Although the provided map contain any Object values, only non-"simple" 
+	 * value types as defined in {@link BeanUtils#isSimpleValueType} are used.
+	 * 
+	 * @param params a Map with the names and values of expected parameters.
 	 */
-	public String getKeyParameterName() {
-		return keyParameterName;
+	public void setExpectedRequestParameters(Map<String, ?> params) {
+		this.expectedRequestParameters = new LinkedHashMap<String, String>();
+		if (params != null) {
+			for (String name : params.keySet()) {
+				Object value = params.get(name);
+				if ((value != null) && BeanUtils.isSimpleValueType(value.getClass())) {
+					this.expectedRequestParameters.put(name, value.toString());
+				}
+			}
+		}
 	}
-
+	
+	/**
+	 * Whether this FlashMap matches to the given request by checking 
+	 * expectations provided via {@link #setExpectedUrlPath} and 
+	 * {@link #setExpectedRequestParameters}.
+	 * 
+	 * @param request the current request
+	 * 
+	 * @return "true" if the expectations match or there are no expectations.
+	 */
+	public boolean matches(HttpServletRequest request) {
+		if (this.expectedUrlPath != null) {
+			if (!matchPathsIgnoreTrailingSlash(this.urlPathHelper.getRequestUri(request), this.expectedUrlPath)) {
+				return false;
+			}
+		}
+		if (this.expectedRequestParameters != null) {
+			for (Map.Entry<String, String> entry : this.expectedRequestParameters.entrySet()) {
+				if (!entry.getValue().equals(request.getParameter(entry.getKey()))) {
+					return false;
+				}
+			}
+		}
+		return true;
+	}
+	
+	private boolean matchPathsIgnoreTrailingSlash(String path1, String path2) {
+		path1 = path1.endsWith("/") ? path1.substring(0, path1.length() - 1) : path1;
+		path2 = path2.endsWith("/") ? path2.substring(0, path2.length() - 1) : path2;
+		return path1.equals(path2);
+	}
+	
 	/**
 	 * Start the expiration period for this instance. After the given number of 
 	 * seconds calls to {@link #isExpired()} will return "true".
@@ -94,4 +157,32 @@ public class FlashMap extends ModelMap {
 		}
 	}
 
+	/**
+	 * Compare two FlashMap instances. One instance is preferred over the other
+	 * if it has an expected URL path or if it has a greater number of expected 
+	 * request parameters.
+	 * 
+	 * <p>It is expected that both instances have been matched against the 
+	 * current request via {@link FlashMap#matches}.
+	 */
+	public int compareTo(FlashMap other) {
+		int thisUrlPath = (this.expectedUrlPath != null) ? 1 : 0;
+		int otherUrlPath = (other.expectedUrlPath != null) ? 1 : 0;
+		if (thisUrlPath != otherUrlPath) {
+			return otherUrlPath - thisUrlPath;
+		}
+		else {
+			return other.expectedRequestParameters.size() - this.expectedRequestParameters.size();
+		}
+	}
+
+	@Override
+	public String toString() {
+		StringBuilder result = new StringBuilder();
+		result.append("[Attributes=").append(super.toString());
+		result.append(", expecteUrlPath=").append(this.expectedUrlPath);
+		result.append(", expectedRequestParameters=" + this.expectedRequestParameters.toString()).append("]");
+		return result.toString();
+	}
+
 }

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

@@ -22,11 +22,11 @@ import org.springframework.web.servlet.support.RequestContextUtils;
 
 /**
  * A strategy interface for maintaining {@link FlashMap} instances in some 
- * underlying storage between two requests. This is typically used when
- * redirecting from one URL to another.
+ * underlying storage until the next request. The most common use case is
+ * a redirect. For example redirecting from a POST that creates a resource 
+ * to the page that shows the created resource and passing along a 
+ * success message that needs to be shown once only.
  * 
- * TODO ...
- *
  * @author Rossen Stoyanchev
  * @since 3.1
  * 
@@ -35,13 +35,18 @@ import org.springframework.web.servlet.support.RequestContextUtils;
 public interface FlashMapManager {
 	
 	/**
-	 * Request attribute to hold the current request FlashMap.
+	 * Request attribute to hold the current request FlashMap. 
 	 * @see RequestContextUtils#getFlashMap
 	 */
 	public static final String CURRENT_FLASH_MAP_ATTRIBUTE = DispatcherServlet.class.getName() + ".CURRENT_FLASH_MAP";
 	
 	/**
-	 * Request attribute to hold the FlashMap from the previous request.
+	 * Request attribute to hold the FlashMap from the previous request. 
+	 * Access to the previous FlashMap should generally not be needed 
+	 * since its content is exposed as attributes of the current 
+	 * request. However, it may be useful to expose previous request
+	 * flash attributes in other ways such as in the model of annotated 
+	 * controllers.
 	 */
 	public static final String PREVIOUS_FLASH_MAP_ATTRIBUTE = DispatcherServlet.class.getName() + ".PREVIOUS_FLASH_MAP";
 
@@ -51,21 +56,27 @@ public interface FlashMapManager {
 	 * 	<li>Create a new FlashMap and make it available to the current request 
 	 * 	under the request attribute {@link #CURRENT_FLASH_MAP_ATTRIBUTE}.
 	 * 	<li>Locate the FlashMap saved on the previous request and expose its 
-	 * 	contents as attributes in the current request.
-	 * 	<li>Remove expired flash map instances.
+	 * 	contents as attributes in the current request, also exposing the 
+	 *  previous FlashMap under {@link #PREVIOUS_FLASH_MAP_ATTRIBUTE}.
+	 * 	<li>Check for and remove expired FlashMap instances.
 	 * </ul>
 	 * 
+	 * <p>If the {@link #CURRENT_FLASH_MAP_ATTRIBUTE} request attribute exists
+	 * in the current request, this method should return "false" immediately.
+	 * 
 	 * @param request the current request
 	 * 
-	 * @return "true" if flash storage tasks were performed; "false" otherwise
-	 * if the {@link #CURRENT_FLASH_MAP_ATTRIBUTE} request attribute exists.
+	 * @return "true" if flash storage tasks were performed; "false" otherwise.
 	 */
 	boolean requestStarted(HttpServletRequest request);
 
 	/**
-	 * Access the current FlashMap through the {@link #CURRENT_FLASH_MAP_ATTRIBUTE}
-	 * request attribute and if not empty, save it in the underlying storage. This 
-	 * method should be invoked after {@link #requestStarted} and if it returned "true". 
+	 * Access the current FlashMap through the request attribute
+	 * {@link #CURRENT_FLASH_MAP_ATTRIBUTE} and if it is not empty, save it 
+	 * in the underlying storage. 
+	 * 
+	 * <p>If the call to {@link #requestStarted} returned "false", this 
+	 * method is not invoked.
 	 */
 	void requestCompleted(HttpServletRequest request);
 

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/support/DefaultFlashMapManager.java

@@ -16,58 +16,35 @@
 
 package org.springframework.web.servlet.support;
 
-import java.util.Iterator;
-import java.util.Map;
-import java.util.Random;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.springframework.util.CollectionUtils;
 import org.springframework.web.servlet.FlashMap;
 import org.springframework.web.servlet.FlashMapManager;
+import org.springframework.web.util.WebUtils;
 
 /**
- * A default implementation that saves and retrieves FlashMap instances to and 
- * from the HTTP session.
+ * A {@link FlashMapManager} that saves and retrieves FlashMap instances in the 
+ * HTTP session.
  * 
  * @author Rossen Stoyanchev
  * @since 3.1
  */
 public class DefaultFlashMapManager implements FlashMapManager {
 
-	static final String FLASH_MAPS_SESSION_ATTRIBUTE = DefaultFlashMapManager.class + ".FLASH_MAPS";
+	private static final String FLASH_MAPS_SESSION_ATTRIBUTE = DefaultFlashMapManager.class + ".FLASH_MAPS";
 
-	private boolean useUniqueFlashKey = true;
-	
-	private String flashKeyParameterName = "_flashKey";
+	private static final Log logger = LogFactory.getLog(DefaultFlashMapManager.class);
 	
 	private int flashTimeout = 180;
-	
-	private static final Random random = new Random();
-
-	/**
-	 * Whether each FlashMap instance should be stored with a unique key.
-	 * The unique key needs to be passed as a parameter in the redirect URL 
-	 * and then used to look up the FlashMap instance avoiding potential 
-	 * issues with concurrent requests.
-	 * <p>The default setting is "true".
-	 * <p>When set to "false" only one FlashMap is maintained making it
-	 * possible for a second concurrent request (e.g. via Ajax) to "consume" 
-	 * the FlashMap inadvertently.
-	 */
-	public void setUseUniqueFlashKey(boolean useUniqueFlashKey) {
-		this.useUniqueFlashKey = useUniqueFlashKey;
-	}
-
-	/**
-	 * Customize the name of the request parameter to be appended to the 
-	 * redirect URL when using a unique flash key.
-	 * <p>The default value is "_flashKey".
-	 */
-	public void setFlashKeyParameterName(String parameterName) {
-		this.flashKeyParameterName = parameterName;
-	}
 
 	/**
 	 * The amount of time in seconds after a request has completed processing 
@@ -90,92 +67,91 @@ public class DefaultFlashMapManager implements FlashMapManager {
 			return false;
 		}
 
-		FlashMap currentFlashMap = 
-			this.useUniqueFlashKey ?
-				new FlashMap(createFlashKey(request), this.flashKeyParameterName) : new FlashMap();
+		FlashMap currentFlashMap = new FlashMap();
 		request.setAttribute(CURRENT_FLASH_MAP_ATTRIBUTE, currentFlashMap);
 		
 		FlashMap previousFlashMap = lookupPreviousFlashMap(request);
 		if (previousFlashMap != null) {
-			for (String name : previousFlashMap.keySet()) {
-				if (request.getAttribute(name) == null) {
-					request.setAttribute(name, previousFlashMap.get(name));
-				}
-			}
-			// For exposing flash attributes in other places (e.g. annotated controllers)
+			WebUtils.exposeRequestAttributes(request, previousFlashMap);
 			request.setAttribute(PREVIOUS_FLASH_MAP_ATTRIBUTE, previousFlashMap);
 		}
 		
-		// Check and remove expired instances
-		Map<String, FlashMap> allFlashMaps = retrieveAllFlashMaps(request, false);
-		if (allFlashMaps != null && !allFlashMaps.isEmpty()) {
-			Iterator<FlashMap> iterator = allFlashMaps.values().iterator();
-			while (iterator.hasNext()) {
-				if (iterator.next().isExpired()) {
-					iterator.remove();
+		// Remove expired flash maps
+		List<FlashMap> allMaps = retrieveFlashMaps(request, false);
+		if (allMaps != null && !allMaps.isEmpty()) {
+			List<FlashMap> expiredMaps = new ArrayList<FlashMap>();
+			for (FlashMap flashMap : allMaps) {
+				if (flashMap.isExpired()) {
+					if (logger.isDebugEnabled()) {
+						logger.debug("Removing expired FlashMap: " + flashMap);
+					}
+					expiredMaps.add(flashMap);
 				}
 			}
+			allMaps.removeAll(expiredMaps);
 		}
 		
 		return true;
 	}
 
 	/**
-	 * Create a unique flash key. The default implementation uses {@link Random}.
-	 * @return the unique key; never {@code null}.
-	 */
-	protected String createFlashKey(HttpServletRequest request) {
-		return String.valueOf(random.nextInt());
-	}
-	
-	/**
-	 * Return the FlashMap from the previous request, if available. 
-	 * If {@link #useUniqueFlashKey} is "true", a flash key parameter is 
-	 * expected to be in the request. Otherwise there can be only one  
-	 * FlashMap instance to return.
+	 * Return the FlashMap from the previous request.
 	 * 
 	 * @return the FlashMap from the previous request; or {@code null} if none.
 	 */
 	private FlashMap lookupPreviousFlashMap(HttpServletRequest request) {
-		Map<String, FlashMap> flashMaps = retrieveAllFlashMaps(request, false);
-		if (flashMaps != null && !flashMaps.isEmpty()) {
-			if (this.useUniqueFlashKey) {
-				String key = request.getParameter(this.flashKeyParameterName);
-				if (key != null) {
-					return flashMaps.remove(key);
+		List<FlashMap> allMaps = retrieveFlashMaps(request, false);
+		if (CollectionUtils.isEmpty(allMaps)) {
+			return null;
+		}
+		
+		if (logger.isDebugEnabled()) {
+			logger.debug("Looking up previous FlashMap among available FlashMaps: " + allMaps);
+		}
+		
+		List<FlashMap> matches = new ArrayList<FlashMap>();
+		for (FlashMap flashMap : allMaps) {
+			if (flashMap.matches(request)) {
+				if (logger.isDebugEnabled()) {
+					logger.debug("Matched " + flashMap);
 				}
-			}
-			else {
-				String key = flashMaps.keySet().iterator().next();
-				return flashMaps.remove(key);
+				matches.add(flashMap);
 			}
 		}
+		
+		if (!matches.isEmpty()) {
+			Collections.sort(matches);
+			return matches.remove(0);
+		}
+		
 		return null;
 	}
 
 	/**
-	 * Retrieve all FlashMap instances from the HTTP session in a thread-safe way. 
+	 * Retrieve the list of all FlashMap instances from the HTTP session. 
 	 * @param request the current request
 	 * @param allowCreate whether to create and the FlashMap container if not found
 	 * @return a Map with all stored FlashMap instances; or {@code null}
 	 */
 	@SuppressWarnings("unchecked")
-	private Map<String, FlashMap> retrieveAllFlashMaps(HttpServletRequest request, boolean allowCreate) {
+	private List<FlashMap> retrieveFlashMaps(HttpServletRequest request, boolean allowCreate) {
 		HttpSession session = request.getSession(allowCreate);
 		if (session == null) {
 			return null;
 		} 
-		Map<String, FlashMap> result = (Map<String, FlashMap>) session.getAttribute(FLASH_MAPS_SESSION_ATTRIBUTE);
-		if (result == null && allowCreate) {
+		
+		List<FlashMap> allMaps = (List<FlashMap>) session.getAttribute(FLASH_MAPS_SESSION_ATTRIBUTE);
+		if (allMaps == null && allowCreate) {
 			synchronized (DefaultFlashMapManager.class) {
-				result = (Map<String, FlashMap>) session.getAttribute(FLASH_MAPS_SESSION_ATTRIBUTE);
-				if (result == null) {
-					result = new ConcurrentHashMap<String, FlashMap>(5);
-					session.setAttribute(FLASH_MAPS_SESSION_ATTRIBUTE, result);
+				allMaps = (List<FlashMap>) session.getAttribute(FLASH_MAPS_SESSION_ATTRIBUTE);
+				if (allMaps == null) {
+					allMaps = new CopyOnWriteArrayList<FlashMap>();
+					session.setAttribute(FLASH_MAPS_SESSION_ATTRIBUTE, allMaps);
 				}
 			}
 		}
-		return result;
+		
+		return allMaps;
 	}
 
 	/**
@@ -187,13 +163,16 @@ public class DefaultFlashMapManager implements FlashMapManager {
 		FlashMap flashMap = (FlashMap) request.getAttribute(CURRENT_FLASH_MAP_ATTRIBUTE);
 		if (flashMap == null) {
 			throw new IllegalStateException(
-					"Did not find current FlashMap exposed as request attribute " + CURRENT_FLASH_MAP_ATTRIBUTE);
+					"Did not find a FlashMap exposed as the request attribute " + CURRENT_FLASH_MAP_ATTRIBUTE);
 		}
+		
 		if (!flashMap.isEmpty()) {
-			Map<String, FlashMap> allFlashMaps = retrieveAllFlashMaps(request, true);
+			if (logger.isDebugEnabled()) {
+				logger.debug("Saving FlashMap=" + flashMap);
+			}
+			List<FlashMap> allFlashMaps = retrieveFlashMaps(request, true);
 			flashMap.startExpirationPeriod(this.flashTimeout);
-			String key = this.useUniqueFlashKey ? flashMap.getKey() : "flashMap";
-			allFlashMaps.put(key, flashMap);
+			allFlashMaps.add(flashMap);
 		}
 	}
 

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/view/RedirectView.java

@@ -36,7 +36,6 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.springframework.beans.BeanUtils;
 import org.springframework.http.HttpStatus;
-import org.springframework.ui.ModelMap;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.ObjectUtils;
 import org.springframework.web.servlet.FlashMap;
@@ -44,7 +43,6 @@ import org.springframework.web.servlet.HandlerMapping;
 import org.springframework.web.servlet.View;
 import org.springframework.web.servlet.support.RequestContextUtils;
 import org.springframework.web.util.UriTemplate;
-import org.springframework.web.util.UriUtils;
 import org.springframework.web.util.WebUtils;
 
 /**
@@ -263,19 +261,17 @@ public class RedirectView extends AbstractUrlBasedView {
 			targetUrl = new StringBuilder(uriTemplate.expand(vars).toString());
 			model = removeKeys(model, uriTemplate.getVariableNames());
 		}
+
+		FlashMap flashMap = RequestContextUtils.getFlashMap(request);
+		if (!CollectionUtils.isEmpty(flashMap)) {
+			flashMap.setExpectedUrlPath(request, targetUrl.toString());
+			flashMap.setExpectedRequestParameters(model);
+		}
 		
 		if (this.exposeModelAttributes) {
 			appendQueryProperties(targetUrl, model, enc);
 		}
-		
-		FlashMap flashMap = RequestContextUtils.getFlashMap(request);
-		if (flashMap != null && !flashMap.isEmpty()) {
-			if (flashMap.getKey() != null) {
-				ModelMap queryParam = new ModelMap(flashMap.getKeyParameterName(), flashMap.getKey());
-				appendQueryProperties(targetUrl, queryParam, enc);
-			}
-		}
-		
+
 		return targetUrl.toString();
 	}
 
@@ -295,11 +291,7 @@ public class RedirectView extends AbstractUrlBasedView {
 			@Override
 			protected URI encodeUri(String uri) {
 				try {
-					String encoded = UriUtils.encodeUri(uri, encoding);
-					return new URI(encoded);
-				}
-				catch (UnsupportedEncodingException ex) {
-					throw new IllegalStateException(ex);
+					return new URI(uri);
 				}
 				catch (URISyntaxException ex) {
 					throw new IllegalArgumentException("Could not create URI from [" + uri + "]: " + ex, ex);