From 159ce34abe4887efd1eee974d180ba291ac6bc64 Mon Sep 17 00:00:00 2001 From: spencergibb Date: Tue, 11 Feb 2025 19:39:39 -0500 Subject: [PATCH] Adds support for retry with request body. Adds MvcUtils.getOrCacheBody() used by RetryFilterFunctions.retry(). Moves retry tests into RetryFilterFunctionTests.java Fixes gh-3336 --- .../filters/retry.adoc | 10 +- .../gateway/server/mvc/common/MvcUtils.java | 8 + .../mvc/filter/RetryFilterFunctions.java | 15 ++ .../server/mvc/ServerMvcIntegrationTests.java | 71 ------- .../mvc/filter/RetryFilterFunctionTests.java | 180 ++++++++++++++++++ 5 files changed, 212 insertions(+), 72 deletions(-) create mode 100644 spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctionTests.java diff --git a/docs/modules/ROOT/pages/spring-cloud-gateway-server-mvc/filters/retry.adoc b/docs/modules/ROOT/pages/spring-cloud-gateway-server-mvc/filters/retry.adoc index fa6529fa..96165f0d 100644 --- a/docs/modules/ROOT/pages/spring-cloud-gateway-server-mvc/filters/retry.adoc +++ b/docs/modules/ROOT/pages/spring-cloud-gateway-server-mvc/filters/retry.adoc @@ -9,6 +9,7 @@ The `Retry` filter supports the following parameters: * `methods`: The HTTP methods that should be retried, represented by using `org.springframework.http.HttpMethod`. * `series`: The series of status codes to be retried, represented by using `org.springframework.http.HttpStatus.Series`. * `exceptions`: A list of thrown exceptions that should be retried. +* `cacheBody`: A flag to signal if the request body should be cached. If set to `true`, the `adaptCacheBody` filter must be used to send the cached body downstream. //* `backoff`: The configured exponential backoff for the retries. //Retries are performed after a backoff interval of `firstBackoff * (factor ^ n)`, where `n` is the iteration. //If `maxBackoff` is configured, the maximum backoff applied is limited to `maxBackoff`. @@ -20,8 +21,11 @@ The following defaults are configured for `Retry` filter, if enabled: * `series`: 5XX series * `methods`: GET method * `exceptions`: `IOException`, `TimeoutException` and `RetryException` +* `cacheBody`: `false` //* `backoff`: disabled +WARNING: Setting `cacheBody` to `true` causes the gateway to read the whole body into memory. This should be used with caution. + The following listing configures a Retry filter: .application.yml @@ -42,11 +46,14 @@ spring: retries: 3 series: SERVER_ERROR methods: GET,POST + cacheBody: true + - name: AdaptCachedBody ---- .GatewaySampleApplication.java [source,java] ---- +import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.adaptCachedBody; import static org.springframework.cloud.gateway.server.mvc.filter.RetryFilterFunctions.retry; import static org.springframework.cloud.gateway.server.mvc.handler.GatewayRouterFunctions.route; import static org.springframework.cloud.gateway.server.mvc.handler.HandlerFunctions.http; @@ -59,7 +66,8 @@ class RouteConfiguration { public RouterFunction gatewayRouterFunctionsAddReqHeader() { return route("add_request_parameter_route") .route(host("*.retry.com"), http("https://example.org")) - .filter(retry(config -> config.setRetries(3).setSeries(Set.of(HttpStatus.Series.SERVER_ERROR)).setMethods(Set.of(HttpMethod.GET, HttpMethod.POST)))) + .filter(retry(config -> config.setRetries(3).setSeries(Set.of(HttpStatus.Series.SERVER_ERROR)).setMethods(Set.of(HttpMethod.GET, HttpMethod.POST)).setCacheBody(true))) + .filter(adaptCachedBody()) .build(); } } diff --git a/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtils.java b/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtils.java index 5e635b71..72f30289 100644 --- a/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtils.java +++ b/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtils.java @@ -116,6 +116,14 @@ public abstract class MvcUtils { } } + public static ByteArrayInputStream getOrCacheBody(ServerRequest request) { + ByteArrayInputStream body = getAttribute(request, MvcUtils.CACHED_REQUEST_BODY_ATTR); + if (body != null) { + return body; + } + return cacheBody(request); + } + public static String expand(ServerRequest request, String template) { Assert.notNull(request, "request may not be null"); Assert.notNull(template, "template may not be null"); diff --git a/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctions.java b/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctions.java index 8a550ce2..272b1e6c 100644 --- a/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctions.java +++ b/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctions.java @@ -27,6 +27,7 @@ import java.util.concurrent.TimeoutException; import java.util.function.Consumer; import org.springframework.cloud.gateway.server.mvc.common.Configurable; +import org.springframework.cloud.gateway.server.mvc.common.MvcUtils; import org.springframework.cloud.gateway.server.mvc.common.Shortcut; import org.springframework.core.NestedRuntimeException; import org.springframework.http.HttpMethod; @@ -71,6 +72,9 @@ public abstract class RetryFilterFunctions { .setPolicies(Arrays.asList(simpleRetryPolicy, new HttpRetryPolicy(config)).toArray(new RetryPolicy[0])); RetryTemplate retryTemplate = retryTemplateBuilder.customPolicy(compositeRetryPolicy).build(); return (request, next) -> retryTemplate.execute(context -> { + if (config.isCacheBody()) { + MvcUtils.getOrCacheBody(request); + } ServerResponse serverResponse = next.handle(request); if (isRetryableStatusCode(serverResponse.statusCode(), config) @@ -121,6 +125,8 @@ public abstract class RetryFilterFunctions { private Set methods = new HashSet<>(List.of(HttpMethod.GET)); + private boolean cacheBody = false; + // TODO: individual statuses // TODO: backoff // TODO: support more Spring Retry policies @@ -176,6 +182,15 @@ public abstract class RetryFilterFunctions { return this; } + public boolean isCacheBody() { + return cacheBody; + } + + public RetryConfig setCacheBody(boolean cacheBody) { + this.cacheBody = cacheBody; + return this; + } + } private static class RetryException extends NestedRuntimeException { diff --git a/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/ServerMvcIntegrationTests.java b/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/ServerMvcIntegrationTests.java index 4af00c8b..9deff0ed 100644 --- a/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/ServerMvcIntegrationTests.java +++ b/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/ServerMvcIntegrationTests.java @@ -27,8 +27,6 @@ import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; import com.github.benmanes.caffeine.cache.Caffeine; @@ -41,8 +39,6 @@ import jakarta.servlet.ServletException; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; import jakarta.servlet.http.HttpServletRequest; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; @@ -80,10 +76,8 @@ import org.springframework.test.context.ContextConfiguration; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.StreamUtils; -import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; -import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.servlet.function.HandlerFunction; import org.springframework.web.servlet.function.RouterFunction; @@ -118,7 +112,6 @@ import static org.springframework.cloud.gateway.server.mvc.filter.CircuitBreaker import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.addRequestHeader; import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.addRequestHeadersIfNotPresent; import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.addRequestParameter; -import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.prefixPath; import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.redirectTo; import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.removeRequestHeader; import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.rewritePath; @@ -127,7 +120,6 @@ import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunction import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.setRequestHostHeader; import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.stripPrefix; import static org.springframework.cloud.gateway.server.mvc.filter.LoadBalancerFilterFunctions.lb; -import static org.springframework.cloud.gateway.server.mvc.filter.RetryFilterFunctions.retry; import static org.springframework.cloud.gateway.server.mvc.handler.GatewayRouterFunctions.route; import static org.springframework.cloud.gateway.server.mvc.handler.HandlerFunctions.forward; import static org.springframework.cloud.gateway.server.mvc.handler.HandlerFunctions.http; @@ -394,20 +386,6 @@ public class ServerMvcIntegrationTests { // @formatter:on } - @Test - public void retryWorks() { - restClient.get().uri("/retry?key=get").exchange().expectStatus().isOk().expectBody(String.class).isEqualTo("3"); - // test for: java.lang.IllegalArgumentException: You have already selected another - // retry policy - restClient.get() - .uri("/retry?key=get2") - .exchange() - .expectStatus() - .isOk() - .expectBody(String.class) - .isEqualTo("3"); - } - @Test public void rateLimitWorks() { restClient.get().uri("/anything/ratelimit").exchange().expectStatus().isOk(); @@ -1005,11 +983,6 @@ public class ServerMvcIntegrationTests { return new TestHandler(); } - @Bean - RetryController retryController() { - return new RetryController(); - } - @Bean EventController eventController() { return new EventController(); @@ -1194,19 +1167,6 @@ public class ServerMvcIntegrationTests { // @formatter:on } - @Bean - public RouterFunction gatewayRouterFunctionsRetry() { - // @formatter:off - return route("testretry") - .route(path("/retry"), http()) - .before(new LocalServerPortUriResolver()) - .filter(retry(3)) - //.filter(retry(config -> config.setRetries(3).setSeries(Set.of(HttpStatus.Series.SERVER_ERROR)).setMethods(Set.of(HttpMethod.GET, HttpMethod.POST)))) - .filter(prefixPath("/do")) - .build(); - // @formatter:on - } - @Bean public RouterFunction gatewayRouterFunctionsRateLimit() { // @formatter:off @@ -1690,37 +1650,6 @@ public class ServerMvcIntegrationTests { } - @RestController - protected static class RetryController { - - Log log = LogFactory.getLog(getClass()); - - ConcurrentHashMap map = new ConcurrentHashMap<>(); - - @GetMapping("/do/retry") - public ResponseEntity retry(@RequestParam("key") String key, - @RequestParam(name = "count", defaultValue = "3") int count, - @RequestParam(name = "failStatus", required = false) Integer failStatus) { - AtomicInteger num = getCount(key); - int i = num.incrementAndGet(); - log.warn("Retry count: " + i); - String body = String.valueOf(i); - if (i < count) { - HttpStatus httpStatus = HttpStatus.INTERNAL_SERVER_ERROR; - if (failStatus != null) { - httpStatus = HttpStatus.resolve(failStatus); - } - return ResponseEntity.status(httpStatus).header("X-Retry-Count", body).body("temporarily broken"); - } - return ResponseEntity.status(HttpStatus.OK).header("X-Retry-Count", body).body(body); - } - - AtomicInteger getCount(String key) { - return map.computeIfAbsent(key, s -> new AtomicInteger()); - } - - } - protected static class TestHandler implements HandlerFunction { @Override diff --git a/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctionTests.java b/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctionTests.java new file mode 100644 index 00000000..edae11e4 --- /dev/null +++ b/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/RetryFilterFunctionTests.java @@ -0,0 +1,180 @@ +/* + * Copyright 2013-2025 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.server.mvc.filter; + +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.context.SpringBootTest.WebEnvironment; +import org.springframework.boot.test.web.server.LocalServerPort; +import org.springframework.cloud.gateway.server.mvc.test.HttpbinTestcontainers; +import org.springframework.cloud.gateway.server.mvc.test.LocalServerPortUriResolver; +import org.springframework.cloud.gateway.server.mvc.test.TestLoadBalancerConfig; +import org.springframework.cloud.gateway.server.mvc.test.client.TestRestClient; +import org.springframework.cloud.loadbalancer.annotation.LoadBalancerClient; +import org.springframework.context.annotation.Bean; +import org.springframework.core.log.LogMessage; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.util.StringUtils; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.servlet.function.RouterFunction; +import org.springframework.web.servlet.function.ServerResponse; + +import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.adaptCachedBody; +import static org.springframework.cloud.gateway.server.mvc.filter.FilterFunctions.prefixPath; +import static org.springframework.cloud.gateway.server.mvc.filter.RetryFilterFunctions.retry; +import static org.springframework.cloud.gateway.server.mvc.handler.GatewayRouterFunctions.route; +import static org.springframework.cloud.gateway.server.mvc.handler.HandlerFunctions.http; + +@SuppressWarnings("unchecked") +@SpringBootTest(properties = {}, webEnvironment = WebEnvironment.RANDOM_PORT) +@ContextConfiguration(initializers = HttpbinTestcontainers.class) +public class RetryFilterFunctionTests { + + @LocalServerPort + int port; + + @Autowired + TestRestClient restClient; + + @Test + public void retryWorks() { + restClient.get().uri("/retry?key=get").exchange().expectStatus().isOk().expectBody(String.class).isEqualTo("3"); + // test for: java.lang.IllegalArgumentException: You have already selected another + // retry policy + restClient.get() + .uri("/retry?key=get2") + .exchange() + .expectStatus() + .isOk() + .expectBody(String.class) + .isEqualTo("3"); + } + + @Test + public void retryBodyWorks() { + restClient.post() + .uri("/retrybody?key=post") + .bodyValue("thebody") + .exchange() + .expectStatus() + .isOk() + .expectBody(String.class) + .isEqualTo("3"); + } + + @SpringBootConfiguration + @EnableAutoConfiguration + @LoadBalancerClient(name = "httpbin", configuration = TestLoadBalancerConfig.Httpbin.class) + protected static class TestConfiguration { + + @Bean + public RouterFunction gatewayRouterFunctionsRetry() { + // @formatter:off + return route("testretry") + .GET("/retry", http()) + .before(new LocalServerPortUriResolver()) + .filter(retry(3)) + .filter(prefixPath("/do")) + .build(); + // @formatter:on + } + + @Bean + public RouterFunction gatewayRouterFunctionsRetryBody() { + // @formatter:off + return route("testretrybody") + .POST("/retrybody", http()) + .before(new LocalServerPortUriResolver()) + .filter(retry(config -> config.setRetries(3).setSeries(Set.of(HttpStatus.Series.SERVER_ERROR)) + .setMethods(Set.of(HttpMethod.GET, HttpMethod.POST)).setCacheBody(true))) + .filter(adaptCachedBody()) + .filter(prefixPath("/do")) + .build(); + // @formatter:on + } + + @RestController + protected static class RetryController { + + Log log = LogFactory.getLog(getClass()); + + ConcurrentHashMap map = new ConcurrentHashMap<>(); + + @GetMapping("/do/retry") + public ResponseEntity retry(@RequestParam("key") String key, + @RequestParam(name = "count", defaultValue = "3") int count, + @RequestParam(name = "failStatus", required = false) Integer failStatus) { + AtomicInteger num = getCount(key); + int i = num.incrementAndGet(); + log.warn("Retry count: " + i); + String body = String.valueOf(i); + if (i < count) { + HttpStatus httpStatus = HttpStatus.INTERNAL_SERVER_ERROR; + if (failStatus != null) { + httpStatus = HttpStatus.resolve(failStatus); + } + return ResponseEntity.status(httpStatus).header("X-Retry-Count", body).body("temporarily broken"); + } + return ResponseEntity.status(HttpStatus.OK).header("X-Retry-Count", body).body(body); + } + + @PostMapping("/do/retrybody") + public ResponseEntity retryBody(@RequestParam("key") String key, + @RequestParam(name = "count", defaultValue = "3") int count, @RequestBody String requestBody) { + AtomicInteger num = getCount(key); + int i = num.incrementAndGet(); + log.warn(LogMessage.format("Retry count: %s, body: %s", i, requestBody)); + String body = String.valueOf(i); + if (!StringUtils.hasText(requestBody)) { + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .header("X-Retry-Count", body) + .body("missing body"); + } + if (i < count) { + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .header("X-Retry-Count", body) + .body("temporarily broken"); + } + return ResponseEntity.status(HttpStatus.OK).header("X-Retry-Count", body).body(body); + } + + AtomicInteger getCount(String key) { + return map.computeIfAbsent(key, s -> new AtomicInteger()); + } + + } + + } + +}