Merge pull request #1672 from sklv-max/fix_x_ratelimit_headers_handling

Fix X-RateLimit headers handling when using CompositeDelayFunction
zalando · Nov 21, 2024 · 83e2365 · 83e2365
2 parents e4caf83 + d44f0e0
commit 83e2365
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 8 deletions.
diff --git a/riptide-failsafe/src/main/java/org/zalando/riptide/failsafe/CompositeDelayFunction.java b/riptide-failsafe/src/main/java/org/zalando/riptide/failsafe/CompositeDelayFunction.java
@@ -29,8 +29,9 @@ public Duration get(final ExecutionContext<R> context) throws Throwable {
                     }
                 })
                 .filter(Objects::nonNull)
+                .filter(delay -> !Duration.ofMinutes(-1).equals(delay))
                 .findFirst()
-                .orElse(null);
+                .orElse(Duration.ofMinutes(-1));
     }
 
     @SafeVarargs

diff --git a/riptide-failsafe/src/test/java/org/zalando/riptide/failsafe/CompositeDelayFunctionTest.java b/riptide-failsafe/src/test/java/org/zalando/riptide/failsafe/CompositeDelayFunctionTest.java
@@ -11,7 +11,6 @@
 import java.time.Duration;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrowsExactly;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
@@ -43,8 +42,8 @@ final class CompositeDelayFunctionTest {
     @BeforeEach
     void defaultBehavior() throws Throwable {
         // starting with Mockito 3.4.4, mocks will return Duration.ZERO instead of null, by default
-        when(first.get(any())).thenReturn(null);
-        when(first.get(any())).thenReturn(null);
+        when(first.get(any())).thenReturn(Duration.ofMinutes(-1));
+        when(first.get(any())).thenReturn(Duration.ofMinutes(-1));
     }
 
     @Test
@@ -73,11 +72,18 @@ void shouldIgnoreNullDelay() throws Throwable {
     }
 
     @Test
-    void shouldFallbackToNullDelay() throws Throwable {
+    void shouldFallbackToDefaultDelay() throws Throwable {
         when(first.get(eq(firstContext))).thenReturn(Duration.ofSeconds(1));
         when(second.get(eq(secondContext))).thenReturn(Duration.ofSeconds(2));
 
-        assertNull(unit.get(mock(ExecutionContext.class)));
+        assertEquals(Duration.ofMinutes(-1), unit.get(mock(ExecutionContext.class)));
+    }
+
+    @Test
+    void shouldUseFirstNonDefaultDelay() throws Throwable {
+        when(second.get(eq(secondContext))).thenReturn(Duration.ofSeconds(2));
+
+        assertEquals(Duration.ofSeconds(2), unit.get(secondContext));
     }
 
 }
diff --git a/...utoconfigure/src/test/java/org/zalando/riptide/autoconfigure/retry/FailsafeRetryTest.java b/...utoconfigure/src/test/java/org/zalando/riptide/autoconfigure/retry/FailsafeRetryTest.java
@@ -1,11 +1,13 @@
 package org.zalando.riptide.autoconfigure.retry;
 
+import com.google.common.base.Stopwatch;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
 import org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.http.HttpStatus;
 import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.web.client.MockRestServiceServer;
 import org.zalando.logbook.autoconfigure.LogbookAutoConfiguration;
@@ -15,17 +17,27 @@
 import org.zalando.riptide.autoconfigure.RiptideClientTest;
 import org.zalando.riptide.failsafe.RetryException;
 
+import java.time.Duration;
 import java.util.concurrent.CompletionException;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.springframework.http.HttpStatus.Series.SERVER_ERROR;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
 import static org.springframework.http.HttpStatus.Series.CLIENT_ERROR;
+import static org.springframework.http.HttpStatus.Series.SERVER_ERROR;
+import static org.springframework.http.HttpStatus.Series.SUCCESSFUL;
 import static org.springframework.test.web.client.ExpectedCount.times;
 import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo;
-import static org.springframework.test.web.client.response.MockRestResponseCreators.withServerError;
 import static org.springframework.test.web.client.response.MockRestResponseCreators.withBadRequest;
+import static org.springframework.test.web.client.response.MockRestResponseCreators.withServerError;
+import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess;
+import static org.springframework.test.web.client.response.MockRestResponseCreators.withTooManyRequests;
+import static org.zalando.riptide.Bindings.anySeries;
 import static org.zalando.riptide.Bindings.on;
 import static org.zalando.riptide.Navigators.series;
+import static org.zalando.riptide.Navigators.status;
+import static org.zalando.riptide.PassRoute.pass;
 import static org.zalando.riptide.failsafe.RetryRoute.retry;
 
 @RiptideClientTest
@@ -70,4 +82,34 @@ void shouldRetryForCustomRetryException() {
 
         server.verify();
     }
+
+    @Test
+    void shouldRetryWithDelayEpochSeconds() {
+        server.expect(times(1), requestTo("http://retry-test"))
+                .andRespond(withTooManyRequests().header("X-RateLimit-Reset", "2"));
+        server.expect(times(1), requestTo("http://retry-test")).andRespond(withSuccess());
+
+        assertTimeout(Duration.ofMillis(3000), () -> {
+            atLeast(Duration.ofSeconds(2), () -> retryClient.get()
+                    .dispatch(series(),
+                            on(SUCCESSFUL).call(pass()),
+                            anySeries().dispatch(status(),
+                                    on(HttpStatus.TOO_MANY_REQUESTS).call(retry())))
+                    .join());
+        });
+
+        server.verify();
+    }
+
+    private void atLeast(final Duration minimum, final Runnable runnable) {
+        final Duration actual = time(runnable);
+
+        assertThat(actual, greaterThanOrEqualTo(minimum));
+    }
+
+    private Duration time(final Runnable runnable) {
+        final Stopwatch stopwatch = Stopwatch.createStarted();
+        runnable.run();
+        return stopwatch.stop().elapsed();
+    }
 }
diff --git a/...igure/src/test/java/org/zalando/riptide/autoconfigure/retry/XRateLimitResetRetryTest.java b/...igure/src/test/java/org/zalando/riptide/autoconfigure/retry/XRateLimitResetRetryTest.java
@@ -0,0 +1,77 @@
+package org.zalando.riptide.autoconfigure.retry;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
+import static org.springframework.http.HttpStatus.Series.SUCCESSFUL;
+import static org.springframework.test.web.client.ExpectedCount.times;
+import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo;
+import static org.springframework.test.web.client.response.MockRestResponseCreators.withRawStatus;
+import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess;
+import static org.zalando.riptide.Bindings.anySeries;
+import static org.zalando.riptide.Bindings.on;
+import static org.zalando.riptide.Navigators.series;
+import static org.zalando.riptide.Navigators.status;
+import static org.zalando.riptide.PassRoute.pass;
+import static org.zalando.riptide.failsafe.RetryRoute.retry;
+
+import com.google.common.base.Stopwatch;
+import java.time.Duration;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
+import org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpStatus;
+import org.springframework.test.context.ActiveProfiles;
+import org.springframework.test.web.client.MockRestServiceServer;
+import org.zalando.logbook.autoconfigure.LogbookAutoConfiguration;
+import org.zalando.riptide.Http;
+import org.zalando.riptide.autoconfigure.MetricsTestAutoConfiguration;
+import org.zalando.riptide.autoconfigure.OpenTracingTestAutoConfiguration;
+import org.zalando.riptide.autoconfigure.RiptideClientTest;
+
+@RiptideClientTest
+@ActiveProfiles("default")
+public class XRateLimitResetRetryTest {
+  @Configuration
+  @ImportAutoConfiguration({
+      JacksonAutoConfiguration.class,
+      LogbookAutoConfiguration.class,
+      OpenTracingTestAutoConfiguration.class,
+      MetricsTestAutoConfiguration.class,
+  })
+  static class ContextConfiguration {
+  }
+
+  @Autowired
+  @Qualifier("retry-test")
+  private Http retryClient;
+
+  @Autowired
+  private MockRestServiceServer server;
+
+  @Test
+  void shouldObeyXRateLimitHeader() {
+    server.expect(times(1), requestTo("http://retry-test"))
+        .andRespond(withRawStatus(429).headers(new HttpHeaders() {{
+          add("X-RateLimit-Limit", "1");
+          add("X-RateLimit-Remaining", "0");
+          add("X-RateLimit-Reset", "2");
+        }}));
+    server.expect(times(1), requestTo("http://retry-test")).andRespond(withSuccess());
+
+    final Stopwatch stopwatch = Stopwatch.createStarted();
+    retryClient.get()
+        .dispatch(series(),
+            on(SUCCESSFUL).call(pass()),
+            anySeries().dispatch(status(),
+                on(HttpStatus.TOO_MANY_REQUESTS).call(retry())))
+        .join();
+    Duration elapsed = stopwatch.stop().elapsed();
+
+    assertThat(elapsed, greaterThanOrEqualTo(Duration.ofSeconds(2)));
+    server.verify();
+  }
+}