Fix backwards compatibility in WebContentInterceptor

As of SPR-11792, WebContentGenerator and WebContentInterceptor offer
new APIs and new behavior regarding HTTP caching, including the use of a
new CacheControl class.

Those changes broke part of the behavior in WebContentInterceptor. This
class allows to override the global Cache configuration at the Generator
level, using specific mappings. Prior to this change, those mappings
would not properly apply the HTTP caching configuration when using
deprecated configuration settings in WebContentGenerator.

This change fixes those backwards compatibility issues for
WebContentInterceptor users.

Issue: SPR-13207

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

@@ -55,7 +55,9 @@ public class WebContentInterceptor extends WebContentGenerator implements Handle
 
 	private PathMatcher pathMatcher = new AntPathMatcher();
 
-	private Map<String, CacheControl> cacheMappings = new HashMap<String, CacheControl>();
+	private Map<String, Integer> cacheMappings = new HashMap<String, Integer>();
+
+	private Map<String, CacheControl> cacheControlMappings = new HashMap<String, CacheControl>();
 
 	public WebContentInterceptor() {
 		// no restriction of HTTP methods by default,
@@ -124,15 +126,7 @@ public class WebContentInterceptor extends WebContentGenerator implements Handle
 		while (propNames.hasMoreElements()) {
 			String path = (String) propNames.nextElement();
 			int cacheSeconds = Integer.valueOf(cacheMappings.getProperty(path));
-			if (cacheSeconds > 0) {
-				this.cacheMappings.put(path, CacheControl.maxAge(cacheSeconds, TimeUnit.SECONDS));
-			}
-			else if (cacheSeconds == 0) {
-				this.cacheMappings.put(path, CacheControl.noStore());
-			}
-			else {
-				this.cacheMappings.put(path, CacheControl.empty());
-			}
+			this.cacheMappings.put(path, cacheSeconds);
 		}
 	}
 
@@ -154,7 +148,7 @@ public class WebContentInterceptor extends WebContentGenerator implements Handle
 	 */
 	public void addCacheMapping(CacheControl cacheControl, String... paths) {
 		for (String path : paths) {
-			this.cacheMappings.put(path, cacheControl);
+			this.cacheControlMappings.put(path, cacheControl);
 		}
 	}
 
@@ -182,13 +176,20 @@ public class WebContentInterceptor extends WebContentGenerator implements Handle
 			logger.debug("Looking up cache seconds for [" + lookupPath + "]");
 		}
 
-		CacheControl cacheControl = lookupCacheSeconds(lookupPath);
+		CacheControl cacheControl = lookupCacheControl(lookupPath);
+		Integer cacheSeconds = lookupCacheSeconds(lookupPath);
 		if (cacheControl != null) {
 			if (logger.isDebugEnabled()) {
 				logger.debug("Applying CacheControl to [" + lookupPath + "]");
 			}
 			checkAndPrepare(request, response, cacheControl);
 		}
+		else if (cacheSeconds != null) {
+			if (logger.isDebugEnabled()) {
+				logger.debug("Applying CacheControl to [" + lookupPath + "]");
+			}
+			checkAndPrepare(request, response, cacheSeconds);
+		}
 		else {
 			if (logger.isDebugEnabled()) {
 				logger.debug("Applying default cache seconds to [" + lookupPath + "]");
@@ -208,20 +209,43 @@ public class WebContentInterceptor extends WebContentGenerator implements Handle
 	 * @return the associated {@code CacheControl}, or {@code null} if not found
 	 * @see org.springframework.util.AntPathMatcher
 	 */
-	protected CacheControl lookupCacheSeconds(String urlPath) {
+	protected CacheControl lookupCacheControl(String urlPath) {
 		// direct match?
-		CacheControl cacheControl = this.cacheMappings.get(urlPath);
+		CacheControl cacheControl = this.cacheControlMappings.get(urlPath);
 		if (cacheControl == null) {
 			// pattern match?
-			for (String registeredPath : this.cacheMappings.keySet()) {
+			for (String registeredPath : this.cacheControlMappings.keySet()) {
 				if (this.pathMatcher.match(registeredPath, urlPath)) {
-					cacheControl = this.cacheMappings.get(registeredPath);
+					cacheControl = this.cacheControlMappings.get(registeredPath);
 				}
 			}
 		}
 		return cacheControl;
 	}
 
+	/**
+	 * Look up a cacheSeconds integer value for the given URL path.
+	 * <p>Supports direct matches, e.g. a registered "/test" matches "/test",
+	 * and various Ant-style pattern matches, e.g. a registered "/t*" matches
+	 * both "/test" and "/team". For details, see the AntPathMatcher class.
+	 * @param urlPath URL the bean is mapped to
+	 * @return the cacheSeconds integer value, or {@code null} if not found
+	 * @see org.springframework.util.AntPathMatcher
+	 */
+	protected Integer lookupCacheSeconds(String urlPath) {
+		// direct match?
+		Integer cacheSeconds = this.cacheMappings.get(urlPath);
+		if (cacheSeconds == null) {
+			// pattern match?
+			for (String registeredPath : this.cacheMappings.keySet()) {
+				if (this.pathMatcher.match(registeredPath, urlPath)) {
+					cacheSeconds = this.cacheMappings.get(registeredPath);
+				}
+			}
+		}
+		return cacheSeconds;
+	}
+
 
 	/**
 	 * This implementation is empty.

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

@@ -347,7 +347,32 @@ public abstract class WebContentGenerator extends WebApplicationObjectSupport {
 		else {
 			cControl = CacheControl.empty();
 		}
-		checkAndPrepare(request, response, cControl);
+		checkRequest(request);
+		if (this.usePreviousHttpCachingBehavior) {
+			addHttp10CacheHeaders(cacheSeconds, response);
+		}
+		else {
+			String ccValue = cControl.getHeaderValue();
+			if (ccValue != null) {
+				response.setHeader(HEADER_CACHE_CONTROL, ccValue);
+			}
+		}
+	}
+
+	protected final void checkRequest(HttpServletRequest request) throws ServletException {
+		// Check whether we should support the request method.
+		String method = request.getMethod();
+		if (this.supportedMethods != null && !this.supportedMethods.contains(method)) {
+			throw new HttpRequestMethodNotSupportedException(
+					method, StringUtils.toStringArray(this.supportedMethods));
+		}
+
+		// Check whether a session is required.
+		if (this.requireSession) {
+			if (request.getSession(false) == null) {
+				throw new HttpSessionRequiredException("Pre-existing session required but none found");
+			}
+		}
 	}
 
 	/**
@@ -366,22 +391,10 @@ public abstract class WebContentGenerator extends WebApplicationObjectSupport {
 			HttpServletRequest request, HttpServletResponse response, CacheControl cacheControl)
 			throws ServletException {
 
-		// Check whether we should support the request method.
-		String method = request.getMethod();
-		if (this.supportedMethods != null && !this.supportedMethods.contains(method)) {
-			throw new HttpRequestMethodNotSupportedException(
-					method, StringUtils.toStringArray(this.supportedMethods));
-		}
-
-		// Check whether a session is required.
-		if (this.requireSession) {
-			if (request.getSession(false) == null) {
-				throw new HttpSessionRequiredException("Pre-existing session required but none found");
-			}
-		}
+		checkRequest(request);
 
 		if (this.usePreviousHttpCachingBehavior) {
-			addHttp10CacheHeaders(response);
+			addHttp10CacheHeaders(this.cacheSeconds, response);
 		}
 		else if (cacheControl != null) {
 			String ccValue = cacheControl.getHeaderValue();
@@ -391,11 +404,11 @@ public abstract class WebContentGenerator extends WebApplicationObjectSupport {
 		}
 	}
 
-	protected void addHttp10CacheHeaders(HttpServletResponse response) {
-		if (this.cacheSeconds > 0) {
-			cacheForSeconds(response, this.cacheSeconds, this.alwaysMustRevalidate);
+	protected final void addHttp10CacheHeaders(int cacheSeconds, HttpServletResponse response) {
+		if (cacheSeconds > 0) {
+			cacheForSeconds(response, cacheSeconds, this.alwaysMustRevalidate);
 		}
-		else if (this.cacheSeconds == 0) {
+		else if (cacheSeconds == 0) {
 			preventCaching(response);
 		}
 	}

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

@@ -16,9 +16,12 @@
 
 package org.springframework.web.servlet.mvc;
 
+import static org.junit.Assert.*;
+
 import java.util.List;
 import java.util.Properties;
 
+import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -26,10 +29,9 @@ import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.mock.web.test.MockHttpServletResponse;
 import org.springframework.web.servlet.support.WebContentGenerator;
 
-import static org.junit.Assert.*;
-
 /**
  * @author Rick Evans
+ * @author Brian Clozel
  */
 public class WebContentInterceptorTests {
 
@@ -47,19 +49,18 @@ public class WebContentInterceptorTests {
 
 
 	@Test
-	public void preHandleSetsCacheSecondsOnMatchingRequest() throws Exception {
+	public void cacheResourcesConfiguration() throws Exception {
 		WebContentInterceptor interceptor = new WebContentInterceptor();
 		interceptor.setCacheSeconds(10);
 
 		interceptor.preHandle(request, response, null);
 
-		List cacheControlHeaders = response.getHeaders("Cache-Control");
-		assertNotNull("'Cache-Control' header not set (must be) : null", cacheControlHeaders);
-		assertTrue("'Cache-Control' header not set (must be) : empty", cacheControlHeaders.size() > 0);
+		Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
+		assertThat(cacheControlHeaders, Matchers.hasItem("max-age=10"));
 	}
 
 	@Test
-	public void preHandleSetsCacheSecondsOnMatchingRequestWithCustomCacheMapping() throws Exception {
+	public void mappedCacheConfigurationOverridesGlobal() throws Exception {
 		Properties mappings = new Properties();
 		mappings.setProperty("**/*handle.vm", "-1");
 
@@ -70,46 +71,74 @@ public class WebContentInterceptorTests {
 		request.setRequestURI("http://localhost:7070/example/adminhandle.vm");
 		interceptor.preHandle(request, response, null);
 
-		List cacheControlHeaders = response.getHeaders("Cache-Control");
-		assertSame("'Cache-Control' header set must be empty", 0, cacheControlHeaders.size());
+		Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
+		assertThat(cacheControlHeaders, Matchers.emptyIterable());
 
 		request.setRequestURI("http://localhost:7070/example/bingo.html");
 		interceptor.preHandle(request, response, null);
 
 		cacheControlHeaders = response.getHeaders("Cache-Control");
-		assertNotNull("'Cache-Control' header not set (must be) : null", cacheControlHeaders);
-		assertTrue("'Cache-Control' header not set (must be) : empty", cacheControlHeaders.size() > 0);
+		assertThat(cacheControlHeaders, Matchers.hasItem("max-age=10"));
 	}
 
 	@Test
-	public void preHandleSetsCacheSecondsOnMatchingRequestWithNoCaching() throws Exception {
+	public void preventCacheConfiguration() throws Exception {
 		WebContentInterceptor interceptor = new WebContentInterceptor();
 		interceptor.setCacheSeconds(0);
 
 		interceptor.preHandle(request, response, null);
 
-		List cacheControlHeaders = response.getHeaders("Cache-Control");
-		assertNotNull("'Cache-Control' header not set (must be) : null", cacheControlHeaders);
-		assertTrue("'Cache-Control' header not set (must be) : empty", cacheControlHeaders.size() > 0);
+		Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
+		assertThat(cacheControlHeaders, Matchers.contains("no-store"));
 	}
 
 	@Test
-	public void preHandleSetsCacheSecondsOnMatchingRequestWithCachingDisabled() throws Exception {
+	public void emptyCacheConfiguration() throws Exception {
 		WebContentInterceptor interceptor = new WebContentInterceptor();
 		interceptor.setCacheSeconds(-1);
 
 		interceptor.preHandle(request, response, null);
 
-		List expiresHeaders = response.getHeaders("Expires");
-		assertSame("'Expires' header set (must not be) : empty", 0, expiresHeaders.size());
-		List cacheControlHeaders = response.getHeaders("Cache-Control");
-		assertSame("'Cache-Control' header set (must not be) : empty", 0, cacheControlHeaders.size());
+		Iterable<String> expiresHeaders = response.getHeaders("Expires");
+		assertThat(expiresHeaders, Matchers.emptyIterable());
+		Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
+		assertThat(cacheControlHeaders, Matchers.emptyIterable());
+	}
+
+	@SuppressWarnings("deprecation")
+	@Test
+	public void http10CachingConfigAndSpecificMapping() throws Exception {
+		WebContentInterceptor interceptor = new WebContentInterceptor();
+		interceptor.setCacheSeconds(0);
+		interceptor.setAlwaysMustRevalidate(true);
+		Properties mappings = new Properties();
+		mappings.setProperty("**/*.cache.html", "10");
+		interceptor.setCacheMappings(mappings);
+
+		request.setRequestURI("http://example.org/foo/page.html");
+		interceptor.preHandle(request, response, null);
+
+		Iterable<String> expiresHeaders = response.getHeaders("Expires");
+		assertThat(expiresHeaders, Matchers.iterableWithSize(1));
+		Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
+		assertThat(cacheControlHeaders, Matchers.contains("no-cache", "no-store"));
+		Iterable<String> pragmaHeaders = response.getHeaders("Pragma");
+		assertThat(pragmaHeaders, Matchers.contains("no-cache"));
+
+		response = new MockHttpServletResponse();
+		request.setRequestURI("http://example.org/page.cache.html");
+		interceptor.preHandle(request, response, null);
+
+		expiresHeaders = response.getHeaders("Expires");
+		assertThat(expiresHeaders, Matchers.iterableWithSize(1));
+		cacheControlHeaders = response.getHeaders("Cache-Control");
+		assertThat(cacheControlHeaders, Matchers.contains("max-age=10, must-revalidate"));
 	}
 
 	@Test(expected = IllegalArgumentException.class)
-	public void testSetPathMatcherToNull() throws Exception {
-				WebContentInterceptor interceptor = new WebContentInterceptor();
-				interceptor.setPathMatcher(null);
+	public void throwsExceptionWithNullPathMatcher() throws Exception {
+		WebContentInterceptor interceptor = new WebContentInterceptor();
+		interceptor.setPathMatcher(null);
 	}
 
 }