Tighten FlashMapManager for use with alternative storage options

Ensure that both FlashMapManager methods - the one invoked at the
start of a request and the one invoked before a redirect, update
the underlying storage fully since it's not guaranteed that both
will be invoked on any given request.

Also move the logic to remove expired FlashMap instances to the
metohd invoked at the start of a request to ensure the check is
made frequently enough.

SPR-8997

Author: rstoyanchev

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

@@ -840,13 +840,13 @@ protected void doService(HttpServletRequest request, HttpServletResponse respons
 		request.setAttribute(LOCALE_RESOLVER_ATTRIBUTE, this.localeResolver);
 		request.setAttribute(THEME_RESOLVER_ATTRIBUTE, this.themeResolver);
 		request.setAttribute(THEME_SOURCE_ATTRIBUTE, getThemeSource());
-		request.setAttribute(OUTPUT_FLASH_MAP_ATTRIBUTE, new FlashMap());
-		request.setAttribute(FLASH_MAP_MANAGER_ATTRIBUTE, this.flashMapManager);
 
-		Map<String, ?> flashMap = this.flashMapManager.getFlashMapForRequest(request);
-		if (flashMap != null) {
-			request.setAttribute(INPUT_FLASH_MAP_ATTRIBUTE, flashMap);
+		FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(request, response);
+		if (inputFlashMap != null) {
+			request.setAttribute(INPUT_FLASH_MAP_ATTRIBUTE, Collections.unmodifiableMap(inputFlashMap));
 		}
+		request.setAttribute(OUTPUT_FLASH_MAP_ATTRIBUTE, new FlashMap());
+		request.setAttribute(FLASH_MAP_MANAGER_ATTRIBUTE, this.flashMapManager);
 
 		try {
 			doDispatch(request, response);

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/FlashMap.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.
@@ -32,12 +32,11 @@
  * <p>A FlashMap can be set up with a request path and request parameters to 
  * help identify the target request. Without this information, a FlashMap is
  * made available to the next request, which may or may not be the intended 
- * recipient. On a redirect, the target URL is known and for example
- * {@code org.springframework.web.servlet.view.RedirectView} has the
- * opportunity to automatically update the current FlashMap with target
- * URL information.
+ * recipient. On a redirect, the target URL is known and a FlashMap can be
+ * updated with that information. This is done automatically when the
+ * {@code org.springframework.web.servlet.view.RedirectView} is used.
  * 
- * <p>Annotated controllers will usually not use this type directly.
+ * <p>Note: annotated controllers will usually not use FlashMap directly.
  * See {@code org.springframework.web.servlet.mvc.support.RedirectAttributes}
  * for an overview of using flash attributes in annotated controllers. 
  * 
@@ -58,25 +57,6 @@ public final class FlashMap extends HashMap<String, Object> implements Comparabl
 
 	private int timeToLive;
 
-	private final int createdBy;
-
-	/**
-	 * Create a new instance with an id uniquely identifying the creator of 
-	 * this FlashMap.
-	 * @param createdBy identifies the FlashMapManager instance that created
-	 * 	and will manage this FlashMap instance (e.g. via a hashCode)  
-	 */
-	public FlashMap(int createdBy) {
-		this.createdBy = createdBy;
-	}
-	
-	/**
-	 * Create a new instance.
-	 */
-	public FlashMap() {
-		this.createdBy = 0;
-	}
-
 	/**
 	 * Provide a URL path to help identify the target request for this FlashMap.
 	 * The path may be absolute (e.g. /application/resource) or relative to the
@@ -96,7 +76,6 @@ public String getTargetRequestPath() {
 
 	/**
 	 * Provide request parameters identifying the request for this FlashMap.
-	 * Null or empty keys and values are skipped.
 	 * @param params a Map with the names and values of expected parameters.
 	 */
 	public FlashMap addTargetRequestParams(MultiValueMap<String, String> params) {
@@ -112,8 +91,8 @@ public FlashMap addTargetRequestParams(MultiValueMap<String, String> params) {
 
 	/**
 	 * Provide a request parameter identifying the request for this FlashMap.
-	 * @param name the expected parameter name, skipped if {@code null}
-	 * @param value the expected parameter value, skipped if {@code null}
+	 * @param name the expected parameter name, skipped if empty or {@code null}
+	 * @param value the expected value, skipped if empty or {@code null}
 	 */
 	public FlashMap addTargetRequestParam(String name, String value) {
 		if (StringUtils.hasText(name) && StringUtils.hasText(value)) {
@@ -151,13 +130,6 @@ public boolean isExpired() {
 		}
 	}
 
-	/**
-	 * Whether the given id matches the id of the creator of this FlashMap.
-	 */
-	public boolean isCreatedBy(int createdBy) {
-		return this.createdBy == createdBy;
-	}
-
 	/**
 	 * Compare two FlashMaps and prefer the one that specifies a target URL 
 	 * path or has more target URL parameters. Before comparing FlashMap

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

@@ -16,44 +16,43 @@
 
 package org.springframework.web.servlet;
 
-import java.util.Map;
-
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 /**
  * A strategy interface for retrieving and saving FlashMap instances.
  * See {@link FlashMap} for a general overview of flash attributes.
- * 
+ *
  * @author Rossen Stoyanchev
  * @since 3.1
- * 
+ *
  * @see FlashMap
  */
 public interface FlashMapManager {
 
 	/**
-	 * Get a Map with flash attributes saved by a previous request.
-	 * See {@link FlashMap} for details on how FlashMap instances
-	 * identifies the target requests they're saved for.
-	 * If found, the Map is removed from the underlying storage.
+	 * Find a FlashMap saved by a previous request that matches to the current
+	 * request, remove it from underlying storage, and also remove other
+	 * expired FlashMap instances.
+	 * <p>This method is invoked in the beginning of every request in contrast
+	 * to {@link #saveOutputFlashMap}, which is invoked only when there are
+	 * flash attributes to be saved - i.e. before a redirect.
 	 * @param request the current request
-	 * @return a read-only Map with flash attributes or {@code null}
+	 * @param response the current response
+	 * @return a FlashMap matching the current request or {@code null}
 	 */
-	Map<String, ?> getFlashMapForRequest(HttpServletRequest request);
+	FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response);
 
 	/**
-	 * Save the given FlashMap, in some underlying storage, mark the beginning
-	 * of its expiration period, and remove other expired FlashMap instances.
-	 * The method has no impact if the FlashMap is empty and there are no
-	 * expired FlashMap instances to be removed.
+	 * Save the given FlashMap, in some underlying storage and set the start
+	 * of its expiration period.
 	 * <p><strong>Note:</strong> Invoke this method prior to a redirect in order
-	 * to allow saving the FlashMap in the HTTP session or perhaps in a response
+	 * to allow saving the FlashMap in the HTTP session or in a response
 	 * cookie before the response is committed.
 	 * @param flashMap the FlashMap to save
 	 * @param request the current request
 	 * @param response the current response
 	 */
-	void save(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response);
+	void saveOutputFlashMap(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response);
 
 }

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

@@ -19,7 +19,6 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.CopyOnWriteArrayList;
 
 import javax.servlet.http.HttpServletRequest;
@@ -50,6 +49,8 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 
 	private UrlPathHelper urlPathHelper = new UrlPathHelper();
 
+	private static final Object writeLock = new Object();
+
 	/**
 	 * Set the amount of time in seconds after a {@link FlashMap} is saved
 	 * (at request completion) and before it expires.
@@ -81,20 +82,59 @@ public UrlPathHelper getUrlPathHelper() {
 		return this.urlPathHelper;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 * <p>Does not cause an HTTP session to be created.
-	 */
-	public final Map<String, ?> getFlashMapForRequest(HttpServletRequest request) {
-		List<FlashMap> flashMaps = retrieveFlashMaps(request);
-		if (CollectionUtils.isEmpty(flashMaps)) {
+	public final FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response) {
+		List<FlashMap> allMaps = retrieveFlashMaps(request);
+		if (CollectionUtils.isEmpty(allMaps)) {
 			return null;
 		}
 		if (logger.isDebugEnabled()) {
-			logger.debug("Retrieved FlashMap(s): " + flashMaps);
+			logger.debug("Retrieved FlashMap(s): " + allMaps);
+		}
+		List<FlashMap> mapsToRemove = getExpiredFlashMaps(allMaps);
+		FlashMap match = getMatchingFlashMap(allMaps, request);
+		if (match != null) {
+			mapsToRemove.add(match);
+		}
+		if (!mapsToRemove.isEmpty()) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Removing FlashMap(s): " + allMaps);
+			}
+			synchronized (writeLock) {
+				allMaps = retrieveFlashMaps(request);
+				allMaps.removeAll(mapsToRemove);
+				updateFlashMaps(allMaps, request, response);
+			}
+		}
+		return match;
+	}
+
+	/**
+	 * Retrieve saved FlashMap instances from underlying storage.
+	 * @param request the current request
+	 * @return a List with FlashMap instances or {@code null}
+	 */
+	protected abstract List<FlashMap> retrieveFlashMaps(HttpServletRequest request);
+
+	/**
+	 * Return a list of expired FlashMap instances contained in the given list.
+	 */
+	private List<FlashMap> getExpiredFlashMaps(List<FlashMap> allMaps) {
+		List<FlashMap> result = new ArrayList<FlashMap>();
+		for (FlashMap map : allMaps) {
+			if (map.isExpired()) {
+				result.add(map);
+			}
 		}
+		return result;
+	}
+
+	/**
+	 * Return a FlashMap contained in the given list that matches the request.
+	 * @return a matching FlashMap or {@code null}
+	 */
+	private FlashMap getMatchingFlashMap(List<FlashMap> allMaps, HttpServletRequest request) {
 		List<FlashMap> result = new ArrayList<FlashMap>();
-		for (FlashMap flashMap : flashMaps) {
+		for (FlashMap flashMap : allMaps) {
 			if (isFlashMapForRequest(flashMap, request)) {
 				result.add(flashMap);
 			}
@@ -104,24 +144,15 @@ public UrlPathHelper getUrlPathHelper() {
 			if (logger.isDebugEnabled()) {
 				logger.debug("Found matching FlashMap(s): " + result);
 			}
-			FlashMap match = result.remove(0);
-			flashMaps.remove(match);
-			return Collections.unmodifiableMap(match);
+			return result.get(0);
 		}
 		return null;
 	}
 
-	/**
-	 * Retrieve saved FlashMap instances from underlying storage.
-	 * @param request the current request
-	 * @return a List with FlashMap instances or {@code null}
-	 */
-	protected abstract List<FlashMap> retrieveFlashMaps(HttpServletRequest request);
-
 	/**
 	 * Whether the given FlashMap matches the current request.
-	 * The default implementation uses the target request path and query params
-	 * saved in the FlashMap.
+	 * The default implementation uses the target request path and query
+	 * parameters saved in the FlashMap.
 	 */
 	protected boolean isFlashMapForRequest(FlashMap flashMap, HttpServletRequest request) {
 		if (flashMap.getTargetRequestPath() != null) {
@@ -142,37 +173,21 @@ protected boolean isFlashMapForRequest(FlashMap flashMap, HttpServletRequest req
 		return true;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 * <p>The FlashMap, if not empty, is saved to the HTTP session.
-	 */
-	public final void save(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response) {
-		Assert.notNull(flashMap, "FlashMap must not be null");
-
-		List<FlashMap> flashMaps = retrieveFlashMaps(request);
-		if (flashMap.isEmpty() && (flashMaps == null)) {
+	public final void saveOutputFlashMap(FlashMap flashMap, HttpServletRequest request, HttpServletResponse response) {
+		if (CollectionUtils.isEmpty(flashMap)) {
 			return;
 		}
-		synchronized (this) {
-			boolean update = false;
-			flashMaps = retrieveFlashMaps(request);
-			if (!CollectionUtils.isEmpty(flashMaps)) {
-				update = removeExpired(flashMaps);
-			}
-			if (!flashMap.isEmpty()) {
-				String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request);
-				flashMap.setTargetRequestPath(path);
-				flashMap.startExpirationPeriod(this.flashMapTimeout);
-				if (logger.isDebugEnabled()) {
-					logger.debug("Saving FlashMap=" + flashMap);
-				}
-				flashMaps = (flashMaps == null) ? new CopyOnWriteArrayList<FlashMap>() : flashMaps;
-				flashMaps.add(flashMap);
-				update = true;
-			}
-			if (update) {
-				updateFlashMaps(flashMaps, request, response);
-			}
+		String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request);
+		flashMap.setTargetRequestPath(path);
+		flashMap.startExpirationPeriod(this.flashMapTimeout);
+		if (logger.isDebugEnabled()) {
+			logger.debug("Saving FlashMap=" + flashMap);
+		}
+		synchronized (writeLock) {
+			List<FlashMap> allMaps = retrieveFlashMaps(request);
+			allMaps = (allMaps == null) ? new CopyOnWriteArrayList<FlashMap>() : allMaps;
+			allMaps.add(flashMap);
+			updateFlashMaps(allMaps, request, response);
 		}
 	}
 
@@ -197,25 +212,4 @@ private String decodeAndNormalizePath(String path, HttpServletRequest request) {
 	protected abstract void updateFlashMaps(List<FlashMap> flashMaps, HttpServletRequest request,
 			HttpServletResponse response);
 
-	/**
-	 * Remove expired FlashMap instances from the given List.
-	 */
-	protected boolean removeExpired(List<FlashMap> flashMaps) {
-		List<FlashMap> expired = new ArrayList<FlashMap>();
-		for (FlashMap flashMap : flashMaps) {
-			if (flashMap.isExpired()) {
-				if (logger.isTraceEnabled()) {
-					logger.trace("Removing expired FlashMap: " + flashMap);
-				}
-				expired.add(flashMap);
-			}
-		}
-		if (expired.isEmpty()) {
-			return false;
-		}
-		else {
-			return flashMaps.removeAll(expired);
-		}
-	}
-
 }

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

@@ -25,7 +25,7 @@
 import org.springframework.web.servlet.FlashMap;
 
 /**
- * Stores {@link FlashMap} instances in the HTTP session.
+ * Store and retrieve {@link FlashMap} instances to and from the HTTP session.
  *
  * @author Rossen Stoyanchev
  * @since 3.1.1
@@ -35,9 +35,10 @@ public class SessionFlashMapManager extends AbstractFlashMapManager{
 	private static final String FLASH_MAPS_SESSION_ATTRIBUTE = SessionFlashMapManager.class.getName() + ".FLASH_MAPS";
 
 	/**
-	 * Retrieve saved FlashMap instances from the HTTP session.
-	 * @param request the current request
-	 * @return a List with FlashMap instances or {@code null}
+	 * Retrieve saved FlashMap instances from the HTTP Session.
+	 * <p>Does not cause an HTTP session to be created but may update it if a
+	 * FlashMap matching the current request is found or there are expired
+	 * FlashMap to be removed.
 	 */
 	@SuppressWarnings("unchecked")
 	protected List<FlashMap> retrieveFlashMaps(HttpServletRequest request) {
@@ -46,7 +47,7 @@ protected List<FlashMap> retrieveFlashMaps(HttpServletRequest request) {
 	}
 
 	/**
-	 * Save the given FlashMap instances in the HTTP session.
+	 * Save the given FlashMap instance, if not empty, in the HTTP session.
 	 */
 	protected void updateFlashMaps(List<FlashMap> flashMaps, HttpServletRequest request, HttpServletResponse response) {
 		request.getSession().setAttribute(FLASH_MAPS_SESSION_ATTRIBUTE, flashMaps);

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

@@ -276,7 +276,7 @@ protected void renderMergedOutputModel(
 		}
 
 		FlashMapManager flashMapManager = RequestContextUtils.getFlashMapManager(request);
-		flashMapManager.save(flashMap, request, response);
+		flashMapManager.saveOutputFlashMap(flashMap, request, response);
 
 		sendRedirect(request, response, targetUrl.toString(), this.http10Compatible);
 	}