From 29ec0728a8e07e783293b914ca7fb26645263798 Mon Sep 17 00:00:00 2001 From: sgibb Date: Wed, 4 Oct 2023 15:12:19 -0400 Subject: [PATCH] Polish gh-3047 "Correct inconsistencies in the Cache-Control..." --- ...GlobalLocalResponseCacheGatewayFilter.java | 9 +++- ...ocalResponseCacheGatewayFilterFactory.java | 11 ++++- .../cache/LocalResponseCacheProperties.java | 45 +++++++++++++++++-- .../LocalResponseCacheRequestOptions.java | 31 ------------- .../cache/LocalResponseCacheUtils.java | 7 ++- .../RequestNoCacheDirectiveStrategy.java | 37 --------------- .../factory/cache/ResponseCacheManager.java | 15 +++++-- .../cache/ResponseCacheManagerFactory.java | 7 ++- ...itional-spring-configuration-metadata.json | 3 +- ...esponseCacheGatewayFilterFactoryTests.java | 9 ++-- ...geHeaderAfterCacheExchangeMutatorTest.java | 5 ++- ...atusCodeAfterCacheExchangeMutatorTest.java | 3 +- 12 files changed, 89 insertions(+), 93 deletions(-) delete mode 100644 spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheRequestOptions.java delete mode 100644 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/GlobalLocalResponseCacheGatewayFilter.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/GlobalLocalResponseCacheGatewayFilter.java index e2667b7b..75f74ca3 100644 --- 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 index bd91ddbb..652de96f 100644 --- 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.cache.Cache; 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) { + this(cacheManagerFactory, defaultTimeToLive, defaultSize, new RequestOptions()); + } public LocalResponseCacheGatewayFilterFactory(ResponseCacheManagerFactory cacheManagerFactory, - Duration defaultTimeToLive, DataSize defaultSize, LocalResponseCacheRequestOptions requestOptions) { + 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 index 71827b2d..33a27b03 100644 --- 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 class LocalResponseCacheProperties { 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 class LocalResponseCacheProperties { + '}'; } + 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 deleted file mode 100644 index 36c30a2e..00000000 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/LocalResponseCacheRequestOptions.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright 2013-2023 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. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.cloud.gateway.filter.factory.cache; - -public class LocalResponseCacheRequestOptions { - - private RequestNoCacheDirectiveStrategy noCache = RequestNoCacheDirectiveStrategy.SKIP_UPDATE_CACHE_ENTRY; - - public RequestNoCacheDirectiveStrategy getNoCache() { - return noCache; - } - - public void setNoCache(RequestNoCacheDirectiveStrategy noCache) { - this.noCache = noCache; - } - -} 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 index 9850a4fc..6de54ac5 100644 --- 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,9 +16,8 @@ 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 { @@ -26,8 +25,8 @@ public final class 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 deleted file mode 100644 index ef4b07bb..00000000 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/RequestNoCacheDirectiveStrategy.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright 2013-2022 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. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.cloud.gateway.filter.factory.cache; - -/** - * 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 RequestNoCacheDirectiveStrategy { - - /** - * 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/ResponseCacheManager.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/factory/cache/ResponseCacheManager.java index 53b126e7..90d8a42d 100644 --- 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.Flux; 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 class ResponseCacheManager { 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 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 index 82752618..1030070b 100644 --- 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 class ResponseCacheManagerFactory { 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 index 4921a1fb..753c5efa 100644 --- 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 index 6a133b81..a161ec54 100644 --- 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 class LocalResponseCacheGatewayFilterFactoryTests extends BaseWebClientTe } @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 class LocalResponseCacheGatewayFilterFactoryTests extends BaseWebClientTe @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 index 780ea111..fab93d26 100644 --- 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.MockServerHttpRequest; 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 @@ class SetMaxAgeHeaderAfterCacheExchangeMutatorTest { 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 index d052ad20..9ff03362 100644 --- 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 @@ class SetStatusCodeAfterCacheExchangeMutatorTest { } @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 @@ class SetStatusCodeAfterCacheExchangeMutatorTest { assertThat(inputExchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.valueOf(statusCode)); } + }