From b472d192f43c5d827cfc49115c22c83acaebf2f0 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 23 May 2018 09:10:07 -0400 Subject: [PATCH] Improve support for caching encoded resources The key in CachingResourceResolver now includes the "Accept-Encoding" request header cleaned to exclude "*", "identity", and parameters, and also sorted alphabetically. For encoded resources the response now includes a response header with "Vary: Accept-Encoding". Issue: SPR-16381 --- .../resource/CachingResourceResolver.java | 28 +++++++++++++--- .../resource/EncodedResourceResolver.java | 1 + .../resource/GzipResourceResolver.java | 1 + .../CachingResourceResolverTests.java | 32 +++++++++++-------- .../EncodedResourceResolverTests.java | 5 +++ .../resource/CachingResourceResolver.java | 29 ++++++++++++++--- .../resource/EncodedResourceResolver.java | 1 + .../resource/GzipResourceResolver.java | 1 + .../CachingResourceResolverTests.java | 16 ++++++---- .../EncodedResourceResolverTests.java | 5 +++ 10 files changed, 91 insertions(+), 28 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/CachingResourceResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/CachingResourceResolver.java index 8d25e1f6af..c64c39efbd 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/CachingResourceResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/CachingResourceResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2018 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. @@ -16,7 +16,9 @@ package org.springframework.web.reactive.resource; +import java.util.Arrays; import java.util.List; +import java.util.stream.Collectors; import reactor.core.publisher.Mono; @@ -25,6 +27,7 @@ import org.springframework.cache.CacheManager; import org.springframework.core.io.Resource; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; /** @@ -94,14 +97,31 @@ public class CachingResourceResolver extends AbstractResourceResolver { StringBuilder key = new StringBuilder(RESOLVED_RESOURCE_CACHE_KEY_PREFIX); key.append(requestPath); if (exchange != null) { - String encoding = exchange.getRequest().getHeaders().getFirst("Accept-Encoding"); - if (encoding != null && encoding.contains("gzip")) { - key.append("+encoding=gzip"); + String codingKey = getContentCodingKey(exchange); + if (codingKey != null) { + key.append("+encoding=").append(codingKey); } } return key.toString(); } + @Nullable + private static String getContentCodingKey(ServerWebExchange exchange) { + String header = exchange.getRequest().getHeaders().getFirst("Accept-Encoding"); + if (!StringUtils.hasText(header)) { + return null; + } + return Arrays.stream(StringUtils.tokenizeToStringArray(header, ",")) + .map(token -> { + int index = token.indexOf(';'); + return (index >= 0 ? token.substring(0, index) : token).trim().toLowerCase(); + }) + .filter(coding -> !coding.equals("*")) + .filter(coding -> !coding.equals("identity")) + .sorted() + .collect(Collectors.joining(",")); + } + @Override protected Mono resolveUrlPathInternal(String resourceUrlPath, List locations, ResourceResolverChain chain) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/EncodedResourceResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/EncodedResourceResolver.java index a1eb0a12f3..fc40acc0e3 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/EncodedResourceResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/EncodedResourceResolver.java @@ -268,6 +268,7 @@ public class EncodedResourceResolver extends AbstractResourceResolver { headers = new HttpHeaders(); } headers.add(HttpHeaders.CONTENT_ENCODING, this.coding); + headers.add(HttpHeaders.VARY, HttpHeaders.ACCEPT_ENCODING); return headers; } } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java index 9a11fd2844..c9fbb560a5 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/GzipResourceResolver.java @@ -166,6 +166,7 @@ public class GzipResourceResolver extends AbstractResourceResolver { headers = new HttpHeaders(); } headers.add(HttpHeaders.CONTENT_ENCODING, "gzip"); + headers.add(HttpHeaders.VARY, HttpHeaders.ACCEPT_ENCODING); return headers; } } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/CachingResourceResolverTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/CachingResourceResolverTests.java index b297d5f014..abc01f4e35 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/CachingResourceResolverTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/CachingResourceResolverTests.java @@ -28,10 +28,10 @@ import org.springframework.cache.Cache; import org.springframework.cache.concurrent.ConcurrentMapCache; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; -import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.web.test.server.MockServerWebExchange; import static org.junit.Assert.*; +import static org.springframework.mock.http.server.reactive.test.MockServerHttpRequest.*; /** * Unit tests for {@link CachingResourceResolver}. @@ -68,7 +68,7 @@ public class CachingResourceResolverTests { @Test public void resolveResourceInternal() { Resource expected = new ClassPathResource("test/bar.css", getClass()); - MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("")); + MockServerWebExchange exchange = MockServerWebExchange.from(get("")); Resource actual = this.chain.resolveResource(exchange, "bar.css", this.locations).block(TIMEOUT); assertNotSame(expected, actual); @@ -78,9 +78,9 @@ public class CachingResourceResolverTests { @Test public void resolveResourceInternalFromCache() { Resource expected = Mockito.mock(Resource.class); - this.cache.put(CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + "bar.css", expected); + this.cache.put(getCacheKey("bar.css"), expected); - MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("")); + MockServerWebExchange exchange = MockServerWebExchange.from(get("")); Resource actual = this.chain.resolveResource(exchange, "bar.css", this.locations).block(TIMEOUT); assertSame(expected, actual); @@ -88,7 +88,7 @@ public class CachingResourceResolverTests { @Test public void resolveResourceInternalNoMatch() { - MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("")); + MockServerWebExchange exchange = MockServerWebExchange.from(get("")); assertNull(this.chain.resolveResource(exchange, "invalid.css", this.locations).block(TIMEOUT)); } @@ -117,11 +117,11 @@ public class CachingResourceResolverTests { @Test public void resolveResourceAcceptEncodingInCacheKey() { String file = "bar.css"; - MockServerHttpRequest request = MockServerHttpRequest.get(file).header("Accept-Encoding", "gzip").build(); - MockServerWebExchange exchange = MockServerWebExchange.from(request); + MockServerWebExchange exchange = MockServerWebExchange.from(get(file) + .header("Accept-Encoding", "gzip ; a=b , deflate , brotli ; c=d ")); Resource expected = this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT); - String cacheKey = CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + file + "+encoding=gzip"; + String cacheKey = getCacheKey(file + "+encoding=brotli,deflate,gzip"); Object actual = this.cache.get(cacheKey).get(); assertSame(expected, actual); @@ -130,10 +130,10 @@ public class CachingResourceResolverTests { @Test public void resolveResourceNoAcceptEncoding() { String file = "bar.css"; - MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(file)); + MockServerWebExchange exchange = MockServerWebExchange.from(get(file)); Resource expected = this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT); - String cacheKey = CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + file; + String cacheKey = getCacheKey(file); Object actual = this.cache.get(cacheKey).get(); assertEquals(expected, actual); @@ -143,15 +143,19 @@ public class CachingResourceResolverTests { public void resolveResourceMatchingEncoding() { Resource resource = Mockito.mock(Resource.class); Resource gzipped = Mockito.mock(Resource.class); - this.cache.put(CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + "bar.css", resource); - this.cache.put(CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + "bar.css+encoding=gzip", gzipped); + this.cache.put(getCacheKey("bar.css"), resource); + this.cache.put(getCacheKey("bar.css+encoding=gzip"), gzipped); String file = "bar.css"; - MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(file)); + MockServerWebExchange exchange = MockServerWebExchange.from(get(file)); assertSame(resource, this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT)); - exchange = MockServerWebExchange.from(MockServerHttpRequest.get(file).header("Accept-Encoding", "gzip")); + exchange = MockServerWebExchange.from(get(file).header("Accept-Encoding", "gzip")); assertSame(gzipped, this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT)); } + private static String getCacheKey(String key) { + return CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + key; + } + } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/EncodedResourceResolverTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/EncodedResourceResolverTests.java index f84892b3c6..3328c185aa 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/EncodedResourceResolverTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/EncodedResourceResolverTests.java @@ -37,6 +37,7 @@ import org.springframework.cache.concurrent.ConcurrentMapCache; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; +import org.springframework.http.HttpHeaders; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.web.test.server.MockServerWebExchange; import org.springframework.util.FileCopyUtils; @@ -109,7 +110,11 @@ public class EncodedResourceResolverTests { assertEquals(getResource(file + ".gz").getDescription(), actual.getDescription()); assertEquals(getResource(file).getFilename(), actual.getFilename()); + assertTrue(actual instanceof HttpResource); + HttpHeaders headers = ((HttpResource) actual).getResponseHeaders(); + assertEquals("gzip", headers.getFirst(HttpHeaders.CONTENT_ENCODING)); + assertEquals("Accept-Encoding", headers.getFirst(HttpHeaders.VARY)); } @Test diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CachingResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CachingResourceResolver.java index e35cddde09..d78d5a578f 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CachingResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CachingResourceResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 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. @@ -16,14 +16,18 @@ package org.springframework.web.servlet.resource; +import java.util.Arrays; import java.util.List; +import java.util.stream.Collectors; import javax.servlet.http.HttpServletRequest; import org.springframework.cache.Cache; import org.springframework.cache.CacheManager; import org.springframework.core.io.Resource; +import org.springframework.http.HttpHeaders; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; /** * A {@link org.springframework.web.servlet.resource.ResourceResolver} that @@ -95,14 +99,31 @@ public class CachingResourceResolver extends AbstractResourceResolver { StringBuilder key = new StringBuilder(RESOLVED_RESOURCE_CACHE_KEY_PREFIX); key.append(requestPath); if (request != null) { - String encoding = request.getHeader("Accept-Encoding"); - if (encoding != null && encoding.contains("gzip")) { - key.append("+encoding=gzip"); + String codingKey = getContentCodingKey(request); + if (codingKey != null) { + key.append("+encoding=").append(codingKey); } } return key.toString(); } + @Nullable + private static String getContentCodingKey(HttpServletRequest request) { + String header = request.getHeader(HttpHeaders.ACCEPT_ENCODING); + if (!StringUtils.hasText(header)) { + return null; + } + return Arrays.stream(StringUtils.tokenizeToStringArray(header, ",")) + .map(token -> { + int index = token.indexOf(';'); + return (index >= 0 ? token.substring(0, index) : token).trim().toLowerCase(); + }) + .filter(coding -> !coding.equals("*")) + .filter(coding -> !coding.equals("identity")) + .sorted() + .collect(Collectors.joining(",")); + } + @Override protected String resolveUrlPathInternal(String resourceUrlPath, List locations, ResourceResolverChain chain) { diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/EncodedResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/EncodedResourceResolver.java index f87cfefe2e..9809671c24 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/EncodedResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/EncodedResourceResolver.java @@ -263,6 +263,7 @@ public class EncodedResourceResolver extends AbstractResourceResolver { headers = new HttpHeaders(); } headers.add(HttpHeaders.CONTENT_ENCODING, this.coding); + headers.add(HttpHeaders.VARY, HttpHeaders.ACCEPT_ENCODING); return headers; } } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java index 640fc1dbed..5866d68b41 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/GzipResourceResolver.java @@ -167,6 +167,7 @@ public class GzipResourceResolver extends AbstractResourceResolver { headers = new HttpHeaders(); } headers.add(HttpHeaders.CONTENT_ENCODING, "gzip"); + headers.add(HttpHeaders.VARY, HttpHeaders.ACCEPT_ENCODING); return headers; } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CachingResourceResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CachingResourceResolverTests.java index be721a835e..a6a063c7cb 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CachingResourceResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CachingResourceResolverTests.java @@ -73,7 +73,7 @@ public class CachingResourceResolverTests { @Test public void resolveResourceInternalFromCache() { Resource expected = Mockito.mock(Resource.class); - this.cache.put(CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + "bar.css", expected); + this.cache.put(getCacheKey("bar.css"), expected); Resource actual = this.chain.resolveResource(null, "bar.css", this.locations); assertSame(expected, actual); @@ -110,10 +110,10 @@ public class CachingResourceResolverTests { public void resolveResourceAcceptEncodingInCacheKey() { String file = "bar.css"; MockHttpServletRequest request = new MockHttpServletRequest("GET", file); - request.addHeader("Accept-Encoding", "gzip"); + request.addHeader("Accept-Encoding", "gzip ; a=b , deflate , brotli ; c=d "); Resource expected = this.chain.resolveResource(request, file, this.locations); - String cacheKey = CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + file + "+encoding=gzip"; + String cacheKey = getCacheKey(file + "+encoding=brotli,deflate,gzip"); Object actual = this.cache.get(cacheKey).get(); assertSame(expected, actual); @@ -125,7 +125,7 @@ public class CachingResourceResolverTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", file); Resource expected = this.chain.resolveResource(request, file, this.locations); - String cacheKey = CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + file; + String cacheKey = getCacheKey(file); Object actual = this.cache.get(cacheKey).get(); assertEquals(expected, actual); @@ -135,8 +135,8 @@ public class CachingResourceResolverTests { public void resolveResourceMatchingEncoding() { Resource resource = Mockito.mock(Resource.class); Resource gzipped = Mockito.mock(Resource.class); - this.cache.put(CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + "bar.css", resource); - this.cache.put(CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + "bar.css+encoding=gzip", gzipped); + this.cache.put(getCacheKey("bar.css"), resource); + this.cache.put(getCacheKey("bar.css+encoding=gzip"), gzipped); MockHttpServletRequest request = new MockHttpServletRequest("GET", "bar.css"); assertSame(resource, this.chain.resolveResource(request,"bar.css", this.locations)); @@ -146,4 +146,8 @@ public class CachingResourceResolverTests { assertSame(gzipped, this.chain.resolveResource(request, "bar.css", this.locations)); } + private static String getCacheKey(String key) { + return CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + key; + } + } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/EncodedResourceResolverTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/EncodedResourceResolverTests.java index 3ee7afb381..cde55ae020 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/EncodedResourceResolverTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/EncodedResourceResolverTests.java @@ -36,6 +36,7 @@ import org.springframework.cache.concurrent.ConcurrentMapCache; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; +import org.springframework.http.HttpHeaders; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.util.FileCopyUtils; @@ -105,7 +106,11 @@ public class EncodedResourceResolverTests { assertEquals(getResource(file + ".gz").getDescription(), actual.getDescription()); assertEquals(getResource(file).getFilename(), actual.getFilename()); + assertTrue(actual instanceof HttpResource); + HttpHeaders headers = ((HttpResource) actual).getResponseHeaders(); + assertEquals("gzip", headers.getFirst(HttpHeaders.CONTENT_ENCODING)); + assertEquals("Accept-Encoding", headers.getFirst(HttpHeaders.VARY)); } @Test -- GitLab