Avoid cache collisions in CachingResourceResolver

Prior to this commit, there could be cache collisions in the
`CachingResourceResolver` because the cache key generation was
incomplete. This commit expands the cache key generation to avoid such
cases.

Fixes gh-36713

spring-webflux/src/main/java/org/springframework/web/reactive/resource/CachingResourceResolver.java

@@ -107,7 +107,7 @@ public class CachingResourceResolver extends AbstractResourceResolver {
 	protected Mono<Resource> resolveResourceInternal(@Nullable ServerWebExchange exchange,
 			String requestPath, List<? extends Resource> locations, ResourceResolverChain chain) {
 
-		String key = computeKey(exchange, requestPath);
+		String key = computeKey(exchange, requestPath, locations);
 		Resource cachedResource = this.cache.get(key, Resource.class);
 
 		if (cachedResource != null) {
@@ -120,14 +120,31 @@ public class CachingResourceResolver extends AbstractResourceResolver {
 				.doOnNext(resource -> this.cache.put(key, resource));
 	}
 
+	/**
+	 * Compute the caching key for the given exchange and resource request path.
+	 * @deprecated since 6.2.19 in favor of {@link #computeKey(ServerWebExchange, String, List)}.
+	 */
+	@Deprecated(since = "6.2.19", forRemoval = true)
 	protected String computeKey(@Nullable ServerWebExchange exchange, String requestPath) {
+		return computeKey(exchange, requestPath, Collections.emptyList());
+	}
+
+	/**
+	 * Compute the caching key for the given exchange and resource request path in the configured locations.
+	 */
+	protected String computeKey(@Nullable ServerWebExchange exchange, String requestPath, List<? extends Resource> locations) {
+		StringBuilder builder = new StringBuilder(RESOLVED_RESOURCE_CACHE_KEY_PREFIX);
+		if (!locations.isEmpty()) {
+			builder.append(Integer.toHexString(locations.hashCode())).append(":");
+		}
+		builder.append(requestPath);
 		if (exchange != null) {
 			String codingKey = getContentCodingKey(exchange);
 			if (StringUtils.hasText(codingKey)) {
-				return RESOLVED_RESOURCE_CACHE_KEY_PREFIX + requestPath + "+encoding=" + codingKey;
+				builder.append("+encoding=").append(codingKey);
 			}
 		}
-		return RESOLVED_RESOURCE_CACHE_KEY_PREFIX + requestPath;
+		return builder.toString();
 	}
 
 	@Nullable

spring-webflux/src/test/java/org/springframework/web/reactive/resource/CachingResourceResolverTests.java

@@ -16,6 +16,7 @@
 
 package org.springframework.web.reactive.resource;
 
+import java.io.IOException;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
@@ -39,6 +40,7 @@ import static org.springframework.web.testfixture.http.server.reactive.MockServe
  * Tests for {@link CachingResourceResolver}.
  *
  * @author Rossen Stoyanchev
+ * @author Brian Clozel
  */
 @ExtendWith(GzipSupport.class)
 class CachingResourceResolverTests {
@@ -48,6 +50,8 @@ class CachingResourceResolverTests {
 
 	private Cache cache;
 
+	private CachingResourceResolver cachingResolver;
+
 	private ResourceResolverChain chain;
 
 	private List<Resource> locations;
@@ -59,7 +63,9 @@ class CachingResourceResolverTests {
 		this.cache = new ConcurrentMapCache("resourceCache");
 
 		List<ResourceResolver> resolvers = new ArrayList<>();
-		resolvers.add(new CachingResourceResolver(this.cache));
+		this.cachingResolver = new CachingResourceResolver(this.cache);
+		resolvers.add(this.cachingResolver);
+		resolvers.add(new EncodedResourceResolver());
 		resolvers.add(new PathResourceResolver());
 		this.chain = new DefaultResourceResolverChain(resolvers);
 
@@ -81,7 +87,7 @@ class CachingResourceResolverTests {
 	@Test
 	void resolveResourceInternalFromCache() {
 		Resource expected = mock();
-		this.cache.put(resourceKey("bar.css"), expected);
+		this.cache.put(this.cachingResolver.computeKey(null, "bar.css", this.locations), expected);
 
 		MockServerWebExchange exchange = MockServerWebExchange.from(get(""));
 		Resource actual = this.chain.resolveResource(exchange, "bar.css", this.locations).block(TIMEOUT);
@@ -118,7 +124,7 @@ class CachingResourceResolverTests {
 	}
 
 	@Test
-	void resolveResourceAcceptEncodingInCacheKey(GzippedFiles gzippedFiles) {
+	void resolveResourceAcceptEncodingInCacheKey(GzippedFiles gzippedFiles) throws IOException {
 
 		String file = "bar.css";
 		gzippedFiles.create(file);
@@ -126,59 +132,70 @@ class CachingResourceResolverTests {
 		// 1. Resolve plain resource
 
 		MockServerWebExchange exchange = MockServerWebExchange.from(get(file));
-		Resource expected = this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
-
-		String cacheKey = resourceKey(file);
-		assertThat(this.cache.get(cacheKey).get()).isSameAs(expected);
+		this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
 
+		Resource actual = getFromResourceCache(exchange, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css");
 
 		// 2. Resolve with Accept-Encoding
 
 		exchange = MockServerWebExchange.from(get(file)
 				.header("Accept-Encoding", "gzip ; a=b  , deflate ,  br  ; c=d "));
-		expected = this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
+		this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
 
-		cacheKey = resourceKey(file + "+encoding=br,gzip");
-		assertThat(this.cache.get(cacheKey).get()).isSameAs(expected);
+		actual = getFromResourceCache(exchange, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css.gz");
 
 		// 3. Resolve with Accept-Encoding but no matching codings
 
 		exchange = MockServerWebExchange.from(get(file).header("Accept-Encoding", "deflate"));
-		expected = this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
+		this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
 
-		cacheKey = resourceKey(file);
-		assertThat(this.cache.get(cacheKey).get()).isSameAs(expected);
+		actual = getFromResourceCache(exchange, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css");
 	}
 
 	@Test
-	void resolveResourceNoAcceptEncoding() {
+	void resolveResourceNoAcceptEncoding() throws IOException {
 		String file = "bar.css";
 		MockServerWebExchange exchange = MockServerWebExchange.from(get(file));
-		Resource expected = this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
+		this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT);
 
-		String cacheKey = resourceKey(file);
-		Object actual = this.cache.get(cacheKey).get();
-
-		assertThat(actual).isEqualTo(expected);
+		Resource actual = getFromResourceCache(exchange, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css");
 	}
 
 	@Test
 	void resolveResourceMatchingEncoding() {
 		Resource resource = mock();
 		Resource gzipped = mock();
-		this.cache.put(resourceKey("bar.css"), resource);
-		this.cache.put(resourceKey("bar.css+encoding=gzip"), gzipped);
 
-		String file = "bar.css";
-		MockServerWebExchange exchange = MockServerWebExchange.from(get(file));
-		assertThat(this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT)).isSameAs(resource);
+		MockServerWebExchange exchange = MockServerWebExchange.from(get("bar.css"));
+		this.cache.put(this.cachingResolver.computeKey(exchange, "bar.css", this.locations), resource);
 
-		exchange = MockServerWebExchange.from(get(file).header("Accept-Encoding", "gzip"));
-		assertThat(this.chain.resolveResource(exchange, file, this.locations).block(TIMEOUT)).isSameAs(gzipped);
+		MockServerWebExchange gzipExchange = MockServerWebExchange.from(get("bar.css").header("Accept-Encoding", "gzip"));
+		this.cache.put(this.cachingResolver.computeKey(gzipExchange, "bar.css", this.locations), gzipped);
+
+		assertThat(this.chain.resolveResource(exchange, "bar.css", this.locations).block(TIMEOUT)).isSameAs(resource);
+		assertThat(this.chain.resolveResource(gzipExchange, "bar.css", this.locations).block(TIMEOUT)).isSameAs(gzipped);
 	}
 
-	private static String resourceKey(String key) {
-		return CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + key;
+	@Test
+	void shareCacheBetweenResourceLocations() {
+		MockServerWebExchange exchange = MockServerWebExchange.from(get("bar.css"));
+
+		List<Resource> firstLocations = List.of(new ClassPathResource("testalternatepath/", getClass()));
+		Resource firstResource = this.chain.resolveResource(exchange, "bar.css", firstLocations).block(TIMEOUT);
+
+		List<Resource> secondLocations = List.of(new ClassPathResource("test/", getClass()));
+		Resource secondResource = this.chain.resolveResource(exchange, "bar.css", secondLocations).block(TIMEOUT);
+
+		assertThat(firstResource).isNotSameAs(secondResource);
+	}
+
+	private Resource getFromResourceCache(MockServerWebExchange exchange, String file) {
+		String cacheKey = this.cachingResolver.computeKey(exchange, file, this.locations);
+		return this.cache.get(cacheKey, Resource.class);
 	}
 
 }

spring-webmvc/src/main/java/org/springframework/web/servlet/resource/CachingResourceResolver.java

@@ -108,7 +108,7 @@ public class CachingResourceResolver extends AbstractResourceResolver {
 	protected Resource resolveResourceInternal(@Nullable HttpServletRequest request, String requestPath,
 			List<? extends Resource> locations, ResourceResolverChain chain) {
 
-		String key = computeKey(request, requestPath);
+		String key = computeKey(request, requestPath, locations);
 		Resource resource = this.cache.get(key, Resource.class);
 
 		if (resource != null) {
@@ -126,14 +126,31 @@ public class CachingResourceResolver extends AbstractResourceResolver {
 		return resource;
 	}
 
+	/**
+	 * Compute the caching key for the given request and resource request path.
+	 * @deprecated since 6.2.19 in favor of {@link #computeKey(HttpServletRequest, String, List)}.
+	 */
+	@Deprecated(since = "6.2.19", forRemoval = true)
 	protected String computeKey(@Nullable HttpServletRequest request, String requestPath) {
+		return computeKey(request, requestPath, Collections.emptyList());
+	}
+
+	/**
+	 * Compute the caching key for the given request and resource request path in the configured locations.
+	 */
+	protected String computeKey(@Nullable HttpServletRequest request, String requestPath, List<? extends Resource> locations) {
+		StringBuilder builder = new StringBuilder(RESOLVED_RESOURCE_CACHE_KEY_PREFIX);
+		if (!locations.isEmpty()) {
+			builder.append(Integer.toHexString(locations.hashCode())).append(":");
+		}
+		builder.append(requestPath);
 		if (request != null) {
 			String codingKey = getContentCodingKey(request);
 			if (StringUtils.hasText(codingKey)) {
-				return RESOLVED_RESOURCE_CACHE_KEY_PREFIX + requestPath + "+encoding=" + codingKey;
+				builder.append("+encoding=").append(codingKey);
 			}
 		}
-		return RESOLVED_RESOURCE_CACHE_KEY_PREFIX + requestPath;
+		return builder.toString();
 	}
 
 	@Nullable

spring-webmvc/src/test/java/org/springframework/web/servlet/resource/CachingResourceResolverTests.java

@@ -16,6 +16,7 @@
 
 package org.springframework.web.servlet.resource;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -37,12 +38,15 @@ import static org.mockito.Mockito.mock;
  * Tests for {@link CachingResourceResolver}.
  *
  * @author Rossen Stoyanchev
+ * @author Brian Clozel
  */
 @ExtendWith(GzipSupport.class)
 class CachingResourceResolverTests {
 
 	private Cache cache;
 
+	private CachingResourceResolver cachingResolver;
+
 	private ResourceResolverChain chain;
 
 	private List<Resource> locations;
@@ -54,7 +58,9 @@ class CachingResourceResolverTests {
 		this.cache = new ConcurrentMapCache("resourceCache");
 
 		List<ResourceResolver> resolvers = new ArrayList<>();
-		resolvers.add(new CachingResourceResolver(this.cache));
+		this.cachingResolver = new CachingResourceResolver(this.cache);
+		resolvers.add(this.cachingResolver);
+		resolvers.add(new EncodedResourceResolver());
 		resolvers.add(new PathResourceResolver());
 		this.chain = new DefaultResourceResolverChain(resolvers);
 
@@ -75,7 +81,7 @@ class CachingResourceResolverTests {
 	@Test
 	void resolveResourceInternalFromCache() {
 		Resource expected = mock();
-		this.cache.put(resourceKey("bar.css"), expected);
+		this.cache.put(this.cachingResolver.computeKey(null, "bar.css", this.locations), expected);
 		Resource actual = this.chain.resolveResource(null, "bar.css", this.locations);
 
 		assertThat(actual).isSameAs(expected);
@@ -109,7 +115,7 @@ class CachingResourceResolverTests {
 	}
 
 	@Test
-	void resolveResourceAcceptEncodingInCacheKey(GzippedFiles gzippedFiles) {
+	void resolveResourceAcceptEncodingInCacheKey(GzippedFiles gzippedFiles) throws IOException {
 
 		String file = "bar.css";
 		gzippedFiles.create(file);
@@ -117,59 +123,72 @@ class CachingResourceResolverTests {
 		// 1. Resolve plain resource
 
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", file);
-		Resource expected = this.chain.resolveResource(request, file, this.locations);
+		this.chain.resolveResource(request, file, this.locations);
 
-		String cacheKey = resourceKey(file);
-		assertThat(this.cache.get(cacheKey).get()).isSameAs(expected);
+		Resource actual = getFromResourceCache(request, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css");
 
 		// 2. Resolve with Accept-Encoding
 
 		request = new MockHttpServletRequest("GET", file);
 		request.addHeader("Accept-Encoding", "gzip ; a=b  , deflate ,  br  ; c=d ");
-		expected = this.chain.resolveResource(request, file, this.locations);
+		this.chain.resolveResource(request, file, this.locations);
 
-		cacheKey = resourceKey(file + "+encoding=br,gzip");
-		assertThat(this.cache.get(cacheKey).get()).isSameAs(expected);
+		actual = getFromResourceCache(request, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css.gz");
 
 		// 3. Resolve with Accept-Encoding but no matching codings
 
 		request = new MockHttpServletRequest("GET", file);
 		request.addHeader("Accept-Encoding", "deflate");
-		expected = this.chain.resolveResource(request, file, this.locations);
+		this.chain.resolveResource(request, file, this.locations);
 
-		cacheKey = resourceKey(file);
-		assertThat(this.cache.get(cacheKey).get()).isSameAs(expected);
+		actual = getFromResourceCache(request, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css");
 	}
 
 	@Test
-	void resolveResourceNoAcceptEncoding() {
+	void resolveResourceNoAcceptEncoding() throws IOException {
 		String file = "bar.css";
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", file);
-		Resource expected = this.chain.resolveResource(request, file, this.locations);
+		this.chain.resolveResource(request, file, this.locations);
 
-		String cacheKey = resourceKey(file);
-		Object actual = this.cache.get(cacheKey).get();
-
-		assertThat(actual).isEqualTo(expected);
+		Resource actual = getFromResourceCache(request, file);
+		assertThat(actual.getFile().getName()).isEqualTo("bar.css");
 	}
 
 	@Test
 	void resolveResourceMatchingEncoding() {
 		Resource resource = mock();
 		Resource gzipped = mock();
-		this.cache.put(resourceKey("bar.css"), resource);
-		this.cache.put(resourceKey("bar.css+encoding=gzip"), gzipped);
 
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "bar.css");
-		assertThat(this.chain.resolveResource(request, "bar.css", this.locations)).isSameAs(resource);
+		this.cache.put(this.cachingResolver.computeKey(request, "bar.css", this.locations), resource);
 
-		request = new MockHttpServletRequest("GET", "bar.css");
-		request.addHeader("Accept-Encoding", "gzip");
-		assertThat(this.chain.resolveResource(request, "bar.css", this.locations)).isSameAs(gzipped);
+		MockHttpServletRequest gzipRequest = new MockHttpServletRequest("GET", "bar.css");
+		gzipRequest.addHeader("Accept-Encoding", "gzip");
+		this.cache.put(this.cachingResolver.computeKey(gzipRequest, "bar.css", this.locations), gzipped);
+
+		assertThat(this.chain.resolveResource(request, "bar.css", this.locations)).isSameAs(resource);
+		assertThat(this.chain.resolveResource(gzipRequest, "bar.css", this.locations)).isSameAs(gzipped);
 	}
 
-	private static String resourceKey(String key) {
-		return CachingResourceResolver.RESOLVED_RESOURCE_CACHE_KEY_PREFIX + key;
+	@Test
+	void shareCacheBetweenResourceLocations() {
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "bar.css");
+
+		List<Resource> firstLocations = List.of(new ClassPathResource("testalternatepath/", getClass()));
+		Resource firstResource = this.chain.resolveResource(request, "bar.css", firstLocations);
+
+		List<Resource> secondLocations = List.of(new ClassPathResource("test/", getClass()));
+		Resource secondResource = this.chain.resolveResource(request, "bar.css", secondLocations);
+
+		assertThat(firstResource).isNotSameAs(secondResource);
+	}
+
+	private Resource getFromResourceCache(MockHttpServletRequest request, String file) {
+		String cacheKey = this.cachingResolver.computeKey(request, file, this.locations);
+		return this.cache.get(cacheKey, Resource.class);
 	}
 
 }