Skip to content

Commit ef0eb01

Browse files
bclozelbclozel
committed
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
1 parent 3c2efee commit ef0eb01

File tree

3 files changed

+124
-58
lines changed

3 files changed

+124
-58
lines changed

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

+40-16
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ public class WebContentInterceptor extends WebContentGenerator implements Handle
5555

5656
private PathMatcher pathMatcher = new AntPathMatcher();
5757

58-
private Map<String, CacheControl> cacheMappings = new HashMap<String, CacheControl>();
58+
private Map<String, Integer> cacheMappings = new HashMap<String, Integer>();
59+
60+
private Map<String, CacheControl> cacheControlMappings = new HashMap<String, CacheControl>();
5961

6062
public WebContentInterceptor() {
6163
// no restriction of HTTP methods by default,
@@ -124,15 +126,7 @@ public void setCacheMappings(Properties cacheMappings) {
124126
while (propNames.hasMoreElements()) {
125127
String path = (String) propNames.nextElement();
126128
int cacheSeconds = Integer.valueOf(cacheMappings.getProperty(path));
127-
if (cacheSeconds > 0) {
128-
this.cacheMappings.put(path, CacheControl.maxAge(cacheSeconds, TimeUnit.SECONDS));
129-
}
130-
else if (cacheSeconds == 0) {
131-
this.cacheMappings.put(path, CacheControl.noStore());
132-
}
133-
else {
134-
this.cacheMappings.put(path, CacheControl.empty());
135-
}
129+
this.cacheMappings.put(path, cacheSeconds);
136130
}
137131
}
138132

@@ -154,7 +148,7 @@ else if (cacheSeconds == 0) {
154148
*/
155149
public void addCacheMapping(CacheControl cacheControl, String... paths) {
156150
for (String path : paths) {
157-
this.cacheMappings.put(path, cacheControl);
151+
this.cacheControlMappings.put(path, cacheControl);
158152
}
159153
}
160154

@@ -182,13 +176,20 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons
182176
logger.debug("Looking up cache seconds for [" + lookupPath + "]");
183177
}
184178

185-
CacheControl cacheControl = lookupCacheSeconds(lookupPath);
179+
CacheControl cacheControl = lookupCacheControl(lookupPath);
180+
Integer cacheSeconds = lookupCacheSeconds(lookupPath);
186181
if (cacheControl != null) {
187182
if (logger.isDebugEnabled()) {
188183
logger.debug("Applying CacheControl to [" + lookupPath + "]");
189184
}
190185
checkAndPrepare(request, response, cacheControl);
191186
}
187+
else if (cacheSeconds != null) {
188+
if (logger.isDebugEnabled()) {
189+
logger.debug("Applying CacheControl to [" + lookupPath + "]");
190+
}
191+
checkAndPrepare(request, response, cacheSeconds);
192+
}
192193
else {
193194
if (logger.isDebugEnabled()) {
194195
logger.debug("Applying default cache seconds to [" + lookupPath + "]");
@@ -208,20 +209,43 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons
208209
* @return the associated {@code CacheControl}, or {@code null} if not found
209210
* @see org.springframework.util.AntPathMatcher
210211
*/
211-
protected CacheControl lookupCacheSeconds(String urlPath) {
212+
protected CacheControl lookupCacheControl(String urlPath) {
212213
// direct match?
213-
CacheControl cacheControl = this.cacheMappings.get(urlPath);
214+
CacheControl cacheControl = this.cacheControlMappings.get(urlPath);
214215
if (cacheControl == null) {
215216
// pattern match?
216-
for (String registeredPath : this.cacheMappings.keySet()) {
217+
for (String registeredPath : this.cacheControlMappings.keySet()) {
217218
if (this.pathMatcher.match(registeredPath, urlPath)) {
218-
cacheControl = this.cacheMappings.get(registeredPath);
219+
cacheControl = this.cacheControlMappings.get(registeredPath);
219220
}
220221
}
221222
}
222223
return cacheControl;
223224
}
224225

226+
/**
227+
* Look up a cacheSeconds integer value for the given URL path.
228+
* <p>Supports direct matches, e.g. a registered "/test" matches "/test",
229+
* and various Ant-style pattern matches, e.g. a registered "/t*" matches
230+
* both "/test" and "/team". For details, see the AntPathMatcher class.
231+
* @param urlPath URL the bean is mapped to
232+
* @return the cacheSeconds integer value, or {@code null} if not found
233+
* @see org.springframework.util.AntPathMatcher
234+
*/
235+
protected Integer lookupCacheSeconds(String urlPath) {
236+
// direct match?
237+
Integer cacheSeconds = this.cacheMappings.get(urlPath);
238+
if (cacheSeconds == null) {
239+
// pattern match?
240+
for (String registeredPath : this.cacheMappings.keySet()) {
241+
if (this.pathMatcher.match(registeredPath, urlPath)) {
242+
cacheSeconds = this.cacheMappings.get(registeredPath);
243+
}
244+
}
245+
}
246+
return cacheSeconds;
247+
}
248+
225249

226250
/**
227251
* This implementation is empty.

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

+32-19
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,32 @@ else if (cacheSeconds == 0) {
347347
else {
348348
cControl = CacheControl.empty();
349349
}
350-
checkAndPrepare(request, response, cControl);
350+
checkRequest(request);
351+
if (this.usePreviousHttpCachingBehavior) {
352+
addHttp10CacheHeaders(cacheSeconds, response);
353+
}
354+
else {
355+
String ccValue = cControl.getHeaderValue();
356+
if (ccValue != null) {
357+
response.setHeader(HEADER_CACHE_CONTROL, ccValue);
358+
}
359+
}
360+
}
361+
362+
protected final void checkRequest(HttpServletRequest request) throws ServletException {
363+
// Check whether we should support the request method.
364+
String method = request.getMethod();
365+
if (this.supportedMethods != null && !this.supportedMethods.contains(method)) {
366+
throw new HttpRequestMethodNotSupportedException(
367+
method, StringUtils.toStringArray(this.supportedMethods));
368+
}
369+
370+
// Check whether a session is required.
371+
if (this.requireSession) {
372+
if (request.getSession(false) == null) {
373+
throw new HttpSessionRequiredException("Pre-existing session required but none found");
374+
}
375+
}
351376
}
352377

353378
/**
@@ -366,22 +391,10 @@ protected final void checkAndPrepare(
366391
HttpServletRequest request, HttpServletResponse response, CacheControl cacheControl)
367392
throws ServletException {
368393

369-
// Check whether we should support the request method.
370-
String method = request.getMethod();
371-
if (this.supportedMethods != null && !this.supportedMethods.contains(method)) {
372-
throw new HttpRequestMethodNotSupportedException(
373-
method, StringUtils.toStringArray(this.supportedMethods));
374-
}
375-
376-
// Check whether a session is required.
377-
if (this.requireSession) {
378-
if (request.getSession(false) == null) {
379-
throw new HttpSessionRequiredException("Pre-existing session required but none found");
380-
}
381-
}
394+
checkRequest(request);
382395

383396
if (this.usePreviousHttpCachingBehavior) {
384-
addHttp10CacheHeaders(response);
397+
addHttp10CacheHeaders(this.cacheSeconds, response);
385398
}
386399
else if (cacheControl != null) {
387400
String ccValue = cacheControl.getHeaderValue();
@@ -391,11 +404,11 @@ else if (cacheControl != null) {
391404
}
392405
}
393406

394-
protected void addHttp10CacheHeaders(HttpServletResponse response) {
395-
if (this.cacheSeconds > 0) {
396-
cacheForSeconds(response, this.cacheSeconds, this.alwaysMustRevalidate);
407+
protected final void addHttp10CacheHeaders(int cacheSeconds, HttpServletResponse response) {
408+
if (cacheSeconds > 0) {
409+
cacheForSeconds(response, cacheSeconds, this.alwaysMustRevalidate);
397410
}
398-
else if (this.cacheSeconds == 0) {
411+
else if (cacheSeconds == 0) {
399412
preventCaching(response);
400413
}
401414
}

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

+52-23
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,22 @@
1616

1717
package org.springframework.web.servlet.mvc;
1818

19+
import static org.junit.Assert.*;
20+
1921
import java.util.List;
2022
import java.util.Properties;
2123

24+
import org.hamcrest.Matchers;
2225
import org.junit.Before;
2326
import org.junit.Test;
2427

2528
import org.springframework.mock.web.test.MockHttpServletRequest;
2629
import org.springframework.mock.web.test.MockHttpServletResponse;
2730
import org.springframework.web.servlet.support.WebContentGenerator;
2831

29-
import static org.junit.Assert.*;
30-
3132
/**
3233
* @author Rick Evans
34+
* @author Brian Clozel
3335
*/
3436
public class WebContentInterceptorTests {
3537

@@ -47,19 +49,18 @@ public void setUp() throws Exception {
4749

4850

4951
@Test
50-
public void preHandleSetsCacheSecondsOnMatchingRequest() throws Exception {
52+
public void cacheResourcesConfiguration() throws Exception {
5153
WebContentInterceptor interceptor = new WebContentInterceptor();
5254
interceptor.setCacheSeconds(10);
5355

5456
interceptor.preHandle(request, response, null);
5557

56-
List cacheControlHeaders = response.getHeaders("Cache-Control");
57-
assertNotNull("'Cache-Control' header not set (must be) : null", cacheControlHeaders);
58-
assertTrue("'Cache-Control' header not set (must be) : empty", cacheControlHeaders.size() > 0);
58+
Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
59+
assertThat(cacheControlHeaders, Matchers.hasItem("max-age=10"));
5960
}
6061

6162
@Test
62-
public void preHandleSetsCacheSecondsOnMatchingRequestWithCustomCacheMapping() throws Exception {
63+
public void mappedCacheConfigurationOverridesGlobal() throws Exception {
6364
Properties mappings = new Properties();
6465
mappings.setProperty("**/*handle.vm", "-1");
6566

@@ -70,46 +71,74 @@ public void preHandleSetsCacheSecondsOnMatchingRequestWithCustomCacheMapping() t
7071
request.setRequestURI("http://localhost:7070/example/adminhandle.vm");
7172
interceptor.preHandle(request, response, null);
7273

73-
List cacheControlHeaders = response.getHeaders("Cache-Control");
74-
assertSame("'Cache-Control' header set must be empty", 0, cacheControlHeaders.size());
74+
Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
75+
assertThat(cacheControlHeaders, Matchers.emptyIterable());
7576

7677
request.setRequestURI("http://localhost:7070/example/bingo.html");
7778
interceptor.preHandle(request, response, null);
7879

7980
cacheControlHeaders = response.getHeaders("Cache-Control");
80-
assertNotNull("'Cache-Control' header not set (must be) : null", cacheControlHeaders);
81-
assertTrue("'Cache-Control' header not set (must be) : empty", cacheControlHeaders.size() > 0);
81+
assertThat(cacheControlHeaders, Matchers.hasItem("max-age=10"));
8282
}
8383

8484
@Test
85-
public void preHandleSetsCacheSecondsOnMatchingRequestWithNoCaching() throws Exception {
85+
public void preventCacheConfiguration() throws Exception {
8686
WebContentInterceptor interceptor = new WebContentInterceptor();
8787
interceptor.setCacheSeconds(0);
8888

8989
interceptor.preHandle(request, response, null);
9090

91-
List cacheControlHeaders = response.getHeaders("Cache-Control");
92-
assertNotNull("'Cache-Control' header not set (must be) : null", cacheControlHeaders);
93-
assertTrue("'Cache-Control' header not set (must be) : empty", cacheControlHeaders.size() > 0);
91+
Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
92+
assertThat(cacheControlHeaders, Matchers.contains("no-store"));
9493
}
9594

9695
@Test
97-
public void preHandleSetsCacheSecondsOnMatchingRequestWithCachingDisabled() throws Exception {
96+
public void emptyCacheConfiguration() throws Exception {
9897
WebContentInterceptor interceptor = new WebContentInterceptor();
9998
interceptor.setCacheSeconds(-1);
10099

101100
interceptor.preHandle(request, response, null);
102101

103-
List expiresHeaders = response.getHeaders("Expires");
104-
assertSame("'Expires' header set (must not be) : empty", 0, expiresHeaders.size());
105-
List cacheControlHeaders = response.getHeaders("Cache-Control");
106-
assertSame("'Cache-Control' header set (must not be) : empty", 0, cacheControlHeaders.size());
102+
Iterable<String> expiresHeaders = response.getHeaders("Expires");
103+
assertThat(expiresHeaders, Matchers.emptyIterable());
104+
Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
105+
assertThat(cacheControlHeaders, Matchers.emptyIterable());
106+
}
107+
108+
@SuppressWarnings("deprecation")
109+
@Test
110+
public void http10CachingConfigAndSpecificMapping() throws Exception {
111+
WebContentInterceptor interceptor = new WebContentInterceptor();
112+
interceptor.setCacheSeconds(0);
113+
interceptor.setAlwaysMustRevalidate(true);
114+
Properties mappings = new Properties();
115+
mappings.setProperty("**/*.cache.html", "10");
116+
interceptor.setCacheMappings(mappings);
117+
118+
request.setRequestURI("http://example.org/foo/page.html");
119+
interceptor.preHandle(request, response, null);
120+
121+
Iterable<String> expiresHeaders = response.getHeaders("Expires");
122+
assertThat(expiresHeaders, Matchers.iterableWithSize(1));
123+
Iterable<String> cacheControlHeaders = response.getHeaders("Cache-Control");
124+
assertThat(cacheControlHeaders, Matchers.contains("no-cache", "no-store"));
125+
Iterable<String> pragmaHeaders = response.getHeaders("Pragma");
126+
assertThat(pragmaHeaders, Matchers.contains("no-cache"));
127+
128+
response = new MockHttpServletResponse();
129+
request.setRequestURI("http://example.org/page.cache.html");
130+
interceptor.preHandle(request, response, null);
131+
132+
expiresHeaders = response.getHeaders("Expires");
133+
assertThat(expiresHeaders, Matchers.iterableWithSize(1));
134+
cacheControlHeaders = response.getHeaders("Cache-Control");
135+
assertThat(cacheControlHeaders, Matchers.contains("max-age=10, must-revalidate"));
107136
}
108137

109138
@Test(expected = IllegalArgumentException.class)
110-
public void testSetPathMatcherToNull() throws Exception {
111-
WebContentInterceptor interceptor = new WebContentInterceptor();
112-
interceptor.setPathMatcher(null);
139+
public void throwsExceptionWithNullPathMatcher() throws Exception {
140+
WebContentInterceptor interceptor = new WebContentInterceptor();
141+
interceptor.setPathMatcher(null);
113142
}
114143

115144
}

0 commit comments

Comments
 (0)