From ef0eb01f93d6c485cf37692fd193833a6821272a Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Wed, 8 Jul 2015 00:49:59 +0200 Subject: [PATCH] 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 --- .../servlet/mvc/WebContentInterceptor.java | 56 ++++++++++---- .../servlet/support/WebContentGenerator.java | 51 ++++++++----- .../mvc/WebContentInterceptorTests.java | 75 +++++++++++++------ 3 files changed, 124 insertions(+), 58 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/WebContentInterceptor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/WebContentInterceptor.java index be33c5f07b3..67d08abcf3c 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/WebContentInterceptor.java +++ b/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 cacheMappings = new HashMap(); + private Map cacheMappings = new HashMap(); + + private Map cacheControlMappings = new HashMap(); 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. + *

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. diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/support/WebContentGenerator.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/support/WebContentGenerator.java index 5db3572a827..ddda844c5f4 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/support/WebContentGenerator.java +++ b/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); } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/WebContentInterceptorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/WebContentInterceptorTests.java index acbce4a2bc1..19340357ec1 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/WebContentInterceptorTests.java +++ b/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 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 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 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 expiresHeaders = response.getHeaders("Expires"); + assertThat(expiresHeaders, Matchers.emptyIterable()); + Iterable 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 expiresHeaders = response.getHeaders("Expires"); + assertThat(expiresHeaders, Matchers.iterableWithSize(1)); + Iterable cacheControlHeaders = response.getHeaders("Cache-Control"); + assertThat(cacheControlHeaders, Matchers.contains("no-cache", "no-store")); + Iterable 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); } }