Polish gh-3047 "Correct inconsistencies in the Cache-Control..."

spencergibb · spencergibb · commit 29ec0728a8e0 · 2023-10-04T15:18:00.000-04:00
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/GlobalLocalResponseCacheGatewayFilter.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/GlobalLocalResponseCacheGatewayFilter.java
@@ -40,8 +40,15 @@ public class GlobalLocalResponseCacheGatewayFilter implements GlobalFilter, Orde
 
 	private final ResponseCacheGatewayFilter responseCacheGatewayFilter;
 
+	@Deprecated
 	public GlobalLocalResponseCacheGatewayFilter(ResponseCacheManagerFactory cacheManagerFactory, Cache globalCache,
-			Duration configuredTimeToLive, LocalResponseCacheRequestOptions requestOptions) {
+			Duration configuredTimeToLive) {
+		responseCacheGatewayFilter = new ResponseCacheGatewayFilter(
+				cacheManagerFactory.create(globalCache, configuredTimeToLive));
+	}
+
+	public GlobalLocalResponseCacheGatewayFilter(ResponseCacheManagerFactory cacheManagerFactory, Cache globalCache,
+			Duration configuredTimeToLive, LocalResponseCacheProperties.RequestOptions requestOptions) {
 		responseCacheGatewayFilter = new ResponseCacheGatewayFilter(
 				cacheManagerFactory.create(globalCache, configuredTimeToLive, requestOptions));
 	}
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheGatewayFilterFactory.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheGatewayFilterFactory.java
@@ -24,6 +24,7 @@
 import org.springframework.cloud.gateway.config.LocalResponseCacheAutoConfiguration;
 import org.springframework.cloud.gateway.filter.GatewayFilter;
 import org.springframework.cloud.gateway.filter.factory.AbstractGatewayFilterFactory;
+import org.springframework.cloud.gateway.filter.factory.cache.LocalResponseCacheProperties.RequestOptions;
 import org.springframework.cloud.gateway.support.HasRouteId;
 import org.springframework.util.unit.DataSize;
 import org.springframework.validation.annotation.Validated;
@@ -53,10 +54,16 @@ public class LocalResponseCacheGatewayFilterFactory
 
 	private final DataSize defaultSize;
 
-	private final LocalResponseCacheRequestOptions requestOptions;
+	private final RequestOptions requestOptions;
 
+	@Deprecated
 	public LocalResponseCacheGatewayFilterFactory(ResponseCacheManagerFactory cacheManagerFactory,
-			Duration defaultTimeToLive, DataSize defaultSize, LocalResponseCacheRequestOptions requestOptions) {
+			Duration defaultTimeToLive, DataSize defaultSize) {
+		this(cacheManagerFactory, defaultTimeToLive, defaultSize, new RequestOptions());
+	}
+
+	public LocalResponseCacheGatewayFilterFactory(ResponseCacheManagerFactory cacheManagerFactory,
+			Duration defaultTimeToLive, DataSize defaultSize, RequestOptions requestOptions) {
 		super(RouteCacheConfiguration.class);
 		this.cacheManagerFactory = cacheManagerFactory;
 		this.defaultTimeToLive = defaultTimeToLive;
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheProperties.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheProperties.java
@@ -40,7 +40,7 @@ public class LocalResponseCacheProperties {
 
 	private Duration timeToLive;
 
-	private LocalResponseCacheRequestOptions request = new LocalResponseCacheRequestOptions();
+	private RequestOptions request = new RequestOptions();
 
 	public DataSize getSize() {
 		return size;
@@ -66,11 +66,11 @@ public void setTimeToLive(Duration timeToLive) {
 		this.timeToLive = timeToLive;
 	}
 
-	public LocalResponseCacheRequestOptions getRequest() {
+	public RequestOptions getRequest() {
 		return request;
 	}
 
-	public void setRequest(LocalResponseCacheRequestOptions request) {
+	public void setRequest(RequestOptions request) {
 		this.request = request;
 	}
 
@@ -80,4 +80,43 @@ public String toString() {
 				+ '}';
 	}
 
+	public static class RequestOptions {
+
+		private NoCacheStrategy noCacheStrategy = NoCacheStrategy.SKIP_UPDATE_CACHE_ENTRY;
+
+		public NoCacheStrategy getNoCacheStrategy() {
+			return noCacheStrategy;
+		}
+
+		public void setNoCacheStrategy(NoCacheStrategy noCacheStrategy) {
+			this.noCacheStrategy = noCacheStrategy;
+		}
+
+		@Override
+		public String toString() {
+			return "RequestOptions{" + "noCacheStrategy=" + noCacheStrategy + '}';
+		}
+
+	}
+
+	/**
+	 * When client sends "no-cache" directive in "Cache-Control" header, the response
+	 * should be re-validated from upstream. There are several strategies that indicates
+	 * what to do with the new fresh response.
+	 */
+	public enum NoCacheStrategy {
+
+		/**
+		 * Update the cache entry by the fresh response coming from upstream with a new
+		 * time to live.
+		 */
+		UPDATE_CACHE_ENTRY,
+		/**
+		 * Skip the update. The client will receive the fresh response, other clients will
+		 * receive the old entry in cache.
+		 */
+		SKIP_UPDATE_CACHE_ENTRY
+
+	}
+
 }
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheRequestOptions.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheRequestOptions.java
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheUtils.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheUtils.java
@@ -16,18 +16,17 @@
 
 package org.springframework.cloud.gateway.filter.factory.cache;
 
-import java.util.Optional;
-
 import org.springframework.http.server.reactive.ServerHttpRequest;
+import org.springframework.util.StringUtils;
 
 public final class LocalResponseCacheUtils {
 
 	private LocalResponseCacheUtils() {
 	}
 
 	public static boolean isNoCacheRequest(ServerHttpRequest request) {
-		return Optional.ofNullable(request.getHeaders().getCacheControl())
-				.filter(cc -> cc.matches(".*(\s|,|^)no-cache(\\s|,|$).*")).isPresent();
+		String cacheControl = request.getHeaders().getCacheControl();
+		return StringUtils.hasText(cacheControl) && cacheControl.matches(".*(\s|,|^)no-cache(\\s|,|$).*");
 	}
 
 }
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/RequestNoCacheDirectiveStrategy.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/RequestNoCacheDirectiveStrategy.java
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/ResponseCacheManager.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/ResponseCacheManager.java
@@ -30,6 +30,8 @@
 import reactor.core.publisher.Mono;
 
 import org.springframework.cache.Cache;
+import org.springframework.cloud.gateway.filter.factory.cache.LocalResponseCacheProperties.NoCacheStrategy;
+import org.springframework.cloud.gateway.filter.factory.cache.LocalResponseCacheProperties.RequestOptions;
 import org.springframework.cloud.gateway.filter.factory.cache.keygenerator.CacheKeyGenerator;
 import org.springframework.cloud.gateway.filter.factory.cache.postprocessor.AfterCacheExchangeMutator;
 import org.springframework.cloud.gateway.filter.factory.cache.postprocessor.SetCacheDirectivesByMaxAgeAfterCacheExchangeMutator;
@@ -66,8 +68,13 @@ public class ResponseCacheManager {
 
 	private final boolean ignoreNoCacheUpdate;
 
+	@Deprecated
+	public ResponseCacheManager(CacheKeyGenerator cacheKeyGenerator, Cache cache, Duration configuredTimeToLive) {
+		this(cacheKeyGenerator, cache, configuredTimeToLive, new RequestOptions());
+	}
+
 	public ResponseCacheManager(CacheKeyGenerator cacheKeyGenerator, Cache cache, Duration configuredTimeToLive,
-			LocalResponseCacheRequestOptions requestOptions) {
+			RequestOptions requestOptions) {
 		this.cacheKeyGenerator = cacheKeyGenerator;
 		this.cache = cache;
 		this.ignoreNoCacheUpdate = isSkipNoCacheUpdateActive(requestOptions);
@@ -78,9 +85,9 @@ public ResponseCacheManager(CacheKeyGenerator cacheKeyGenerator, Cache cache, Du
 				new SetCacheDirectivesByMaxAgeAfterCacheExchangeMutator());
 	}
 
-	private static boolean isSkipNoCacheUpdateActive(LocalResponseCacheRequestOptions requestOptions) {
-		return Optional.ofNullable(requestOptions).map(LocalResponseCacheRequestOptions::getNoCache)
-				.filter(RequestNoCacheDirectiveStrategy.SKIP_UPDATE_CACHE_ENTRY::equals).isPresent();
+	private static boolean isSkipNoCacheUpdateActive(RequestOptions requestOptions) {
+		return requestOptions != null
+				&& requestOptions.getNoCacheStrategy().equals(NoCacheStrategy.SKIP_UPDATE_CACHE_ENTRY);
 	}
 
 	private static final List<HttpStatusCode> statusesToCache = Arrays.asList(HttpStatus.OK, HttpStatus.PARTIAL_CONTENT,
diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/ResponseCacheManagerFactory.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/ResponseCacheManagerFactory.java
@@ -33,8 +33,13 @@ public ResponseCacheManagerFactory(CacheKeyGenerator cacheKeyGenerator) {
 		this.cacheKeyGenerator = cacheKeyGenerator;
 	}
 
+	@Deprecated
+	public ResponseCacheManager create(Cache cache, Duration timeToLive) {
+		return new ResponseCacheManager(cacheKeyGenerator, cache, timeToLive);
+	}
+
 	public ResponseCacheManager create(Cache cache, Duration timeToLive,
-			LocalResponseCacheRequestOptions requestOptions) {
+			LocalResponseCacheProperties.RequestOptions requestOptions) {
 		return new ResponseCacheManager(cacheKeyGenerator, cache, timeToLive, requestOptions);
 	}
 
diff --git a/spring-cloud-gateway-server/src/main/resources/META-INF/additional-spring-configuration-metadata.json b/spring-cloud-gateway-server/src/main/resources/META-INF/additional-spring-configuration-metadata.json
@@ -45,8 +45,7 @@
     {
       "name": "spring.cloud.gateway.filter.local-response-cache.size",
       "type": "org.springframework.util.unit.DataSize",
-      "description": "Maximum size of the cache to evict entries for this route (in KB, MB and GB).",
-      "defaultValue": "null"
+      "description": "Maximum size of the cache to evict entries for this route (in KB, MB and GB)."
     },
     {
       "name": "spring.cloud.gateway.filter.local-response-cache.time-to-live",
diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheGatewayFilterFactoryTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheGatewayFilterFactoryTests.java
@@ -348,10 +348,9 @@ public RouteLocator testRouteLocator(RouteLocatorBuilder builder) {
 	}
 
 	@Nested
-	@SpringBootTest(
-			properties = { "spring.cloud.gateway.filter.local-response-cache.enabled=true",
-					"spring.cloud.gateway.filter.local-response-cache.time-to-live=2m",
-					"spring.cloud.gateway.filter.local-response-cache.request.no-cache=skip-update-cache-entry" },
+	@SpringBootTest(properties = { "spring.cloud.gateway.filter.local-response-cache.enabled=true",
+			"spring.cloud.gateway.filter.local-response-cache.time-to-live=2m",
+			"spring.cloud.gateway.filter.local-response-cache.request.no-cache-strategy=skip-update-cache-entry" },
 			webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
 	public class DirectiveNoCacheSkippingUpdate extends BaseWebClientTests {
 
@@ -425,7 +424,7 @@ public RouteLocator testRouteLocator(RouteLocatorBuilder builder) {
 	@SpringBootTest(
 			properties = { "spring.cloud.gateway.filter.local-response-cache.enabled=true",
 					"spring.cloud.gateway.filter.local-response-cache.time-to-live=2m",
-					"spring.cloud.gateway.filter.local-response-cache.request.no-cache=update-cache-entry" },
+					"spring.cloud.gateway.filter.local-response-cache.request.no-cache-strategy=update-cache-entry" },
 			webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
 	public class DirectiveNoCacheWithUpdate extends BaseWebClientTests {
 
diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/factory/cache/postprocessor/SetMaxAgeHeaderAfterCacheExchangeMutatorTest.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/factory/cache/postprocessor/SetMaxAgeHeaderAfterCacheExchangeMutatorTest.java
@@ -35,6 +35,7 @@
 import org.springframework.mock.http.server.reactive.MockServerHttpResponse;
 import org.springframework.mock.web.server.MockServerWebExchange;
 import org.springframework.util.CollectionUtils;
+import org.springframework.util.StringUtils;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -142,8 +143,8 @@ void otherCacheControlValuesAreNotRemoved_whenMaxAgeIsModified() {
 				clock, false);
 		toTest.accept(inputExchange, inputCachedResponse);
 
-		String[] cacheControlValues = Optional.ofNullable(inputExchange.getResponse().getHeaders().getCacheControl())
-				.map(s -> s.split("\\s*,\\s*")).orElse(null);
+		String[] cacheControlValues = StringUtils
+				.tokenizeToStringArray(inputExchange.getResponse().getHeaders().getCacheControl(), ",");
 		assertThat(cacheControlValues).contains("max-stale=12", "min-stale=1");
 	}
 
diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/factory/cache/postprocessor/SetStatusCodeAfterCacheExchangeMutatorTest.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/filter/factory/cache/postprocessor/SetStatusCodeAfterCacheExchangeMutatorTest.java
@@ -49,7 +49,7 @@ void setUp() {
 	}
 
 	@ParameterizedTest
-	@ValueSource(ints = {200, 400, 404, 500})
+	@ValueSource(ints = { 200, 400, 404, 500 })
 	void statusCodeIsSetFromCachedResponse(int statusCode) {
 		CachedResponse cachedResponse = CachedResponse.create(HttpStatus.valueOf(statusCode)).body("some-data").build();
 		MockServerHttpRequest httpRequest = MockServerHttpRequest.get("https://this").build();
@@ -61,4 +61,5 @@ void statusCodeIsSetFromCachedResponse(int statusCode) {
 
 		assertThat(inputExchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.valueOf(statusCode));
 	}
+
 }

Original file line number	Diff line number	Diff line change
`@@ -16,18 +16,17 @@`
`16`	`16`
`17`	`17`	`package org.springframework.cloud.gateway.filter.factory.cache;`
`18`	`18`
`19`		`-import java.util.Optional;`
`20`		`-`
`21`	`19`	`import org.springframework.http.server.reactive.ServerHttpRequest;`
	`20`	`+import org.springframework.util.StringUtils;`
`22`	`21`
`23`	`22`	`public final class LocalResponseCacheUtils {`
`24`	`23`
`25`	`24`	`private LocalResponseCacheUtils() {`
`26`	`25`	`}`
`27`	`26`
`28`	`27`	`public static boolean isNoCacheRequest(ServerHttpRequest request) {`
`29`		`- return Optional.ofNullable(request.getHeaders().getCacheControl())`
`30`		`- .filter(cc -> cc.matches(".(\s\|,\|^)no-cache(\\s\|,\|$).")).isPresent();`
	`28`	`+ String cacheControl = request.getHeaders().getCacheControl();`
	`29`	`+ return StringUtils.hasText(cacheControl) && cacheControl.matches(".(\s\|,\|^)no-cache(\\s\|,\|$).");`
`31`	`30`	`}`
`32`	`31`
`33`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,8 +33,13 @@ public ResponseCacheManagerFactory(CacheKeyGenerator cacheKeyGenerator) {`
`33`	`33`	`this.cacheKeyGenerator = cacheKeyGenerator;`
`34`	`34`	`}`
`35`	`35`
	`36`	`+ @Deprecated`
	`37`	`+ public ResponseCacheManager create(Cache cache, Duration timeToLive) {`
	`38`	`+ return new ResponseCacheManager(cacheKeyGenerator, cache, timeToLive);`
	`39`	`+ }`
	`40`	`+`
`36`	`41`	`public ResponseCacheManager create(Cache cache, Duration timeToLive,`
`37`		`- LocalResponseCacheRequestOptions requestOptions) {`
	`42`	`+ LocalResponseCacheProperties.RequestOptions requestOptions) {`
`38`	`43`	`return new ResponseCacheManager(cacheKeyGenerator, cache, timeToLive, requestOptions);`
`39`	`44`	`}`
`40`	`45`
Original file line number	Diff line number	Diff line change
`@@ -45,8 +45,7 @@`
`45`	`45`	`{`
`46`	`46`	`"name": "spring.cloud.gateway.filter.local-response-cache.size",`
`47`	`47`	`"type": "org.springframework.util.unit.DataSize",`
`48`		`- "description": "Maximum size of the cache to evict entries for this route (in KB, MB and GB).",`
`49`		`- "defaultValue": "null"`
	`48`	`+ "description": "Maximum size of the cache to evict entries for this route (in KB, MB and GB)."`
`50`	`49`	`},`
`51`	`50`	`{`
`52`	`51`	`"name": "spring.cloud.gateway.filter.local-response-cache.time-to-live",`