From 67eab48f10798cab68c3fa5e72d30954fab941e9 Mon Sep 17 00:00:00 2001 From: Thibault Pensec <39826516+symphony-thibault@users.noreply.github.com> Date: Fri, 30 Apr 2021 11:26:27 +0200 Subject: [PATCH] #504 Status 500 is now really considered as a server error (#509) We recently noticed that the DatafeedLoop was crashing with `500` errors returned from Agent. This is something we initially stated that we did not wanted to retry on this specific status. Now we do, as it has actually been raised by some customers. --- .../Resilience4jRetryWithRecoveryTest.java | 14 ++++---- .../datafeed/impl/DatafeedLoopV1Test.java | 2 +- .../datafeed/impl/DatafeedLoopV2Test.java | 2 +- .../symphony/bdk/http/api/ApiException.java | 2 +- .../bdk/http/api/ApiExceptionTest.java | 32 +++++++++++++++++++ 5 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 symphony-bdk-http/symphony-bdk-http-api/src/test/java/com/symphony/bdk/http/api/ApiExceptionTest.java diff --git a/symphony-bdk-core/src/test/java/com/symphony/bdk/core/retry/resilience4j/Resilience4jRetryWithRecoveryTest.java b/symphony-bdk-core/src/test/java/com/symphony/bdk/core/retry/resilience4j/Resilience4jRetryWithRecoveryTest.java index ff0d00844..7386639da 100644 --- a/symphony-bdk-core/src/test/java/com/symphony/bdk/core/retry/resilience4j/Resilience4jRetryWithRecoveryTest.java +++ b/symphony-bdk-core/src/test/java/com/symphony/bdk/core/retry/resilience4j/Resilience4jRetryWithRecoveryTest.java @@ -93,7 +93,7 @@ void testSupplierWithExceptionAndNoRetryShouldFailWithException() throws Throwab ofMinimalInterval(), supplier, (t) -> false, Collections.emptyList()); - assertThrows(ApiException.class, () -> r.execute()); + assertThrows(ApiException.class, r::execute); verify(supplier, times(1)).get(); } @@ -108,7 +108,7 @@ void testMaxAttemptsReachedShouldFailWithException() throws ApiException { retryConfig, supplier, (t) -> true, Collections.emptyList()); - assertThrows(ApiException.class, () -> r.execute()); + assertThrows(ApiException.class, r::execute); verify(supplier, times(retryConfig.getMaxAttempts())).get(); } @@ -122,7 +122,7 @@ void testExceptionNotMatchingRetryPredicateShouldBeForwarded() throws ApiExcepti (t) -> t instanceof ApiException && ((ApiException) t).isServerError(), Collections.emptyList()); - assertThrows(ApiException.class, () -> r.execute()); + assertThrows(ApiException.class, r::execute); verify(supplier, times(1)).get(); } @@ -166,13 +166,13 @@ void testNonMatchingExceptionShouldNotTriggerRecoveryAndRetry() throws Throwable final String value = "string"; SupplierWithApiException supplier = mock(ConcreteSupplier.class); - when(supplier.get()).thenThrow(new ApiException(500, "error")).thenReturn(value); + when(supplier.get()).thenThrow(new ApiException(400, "error")).thenReturn(value); ConcreteConsumer consumer = mock(ConcreteConsumer.class); Resilience4jRetryWithRecovery r = new Resilience4jRetryWithRecovery<>("name", "localhost.symphony.com", ofMinimalInterval(), supplier, (t) -> true, - Collections.singletonList(new RecoveryStrategy(ApiException.class, e -> e.isClientError(), consumer))); + Collections.singletonList(new RecoveryStrategy(ApiException.class, ApiException::isServerError, consumer))); assertEquals(value, r.execute()); verify(supplier, times(2)).get(); @@ -195,7 +195,7 @@ void testThrowableInRecoveryAndNotMatchingRetryPredicateShouldBeForwarded() thro (t) -> t instanceof ApiException, Collections.singletonList(new RecoveryStrategy(ApiException.class, ApiException::isClientError, consumer))); - assertThrows(IndexOutOfBoundsException.class, () -> r.execute()); + assertThrows(IndexOutOfBoundsException.class, r::execute); InOrder inOrder = inOrder(supplier, consumer); inOrder.verify(supplier).get(); @@ -241,7 +241,7 @@ void testExecuteAndRetrySucceeds() throws Throwable { @Test void testExecuteAndRetryShouldConvertApiExceptionIntoApiRuntimeException() throws Throwable { SupplierWithApiException supplier = mock(ConcreteSupplier.class); - when(supplier.get()).thenThrow(new ApiException(500, "")); + when(supplier.get()).thenThrow(new ApiException(400, "")); assertThrows(ApiRuntimeException.class, () -> Resilience4jRetryWithRecovery.executeAndRetry(new RetryWithRecoveryBuilder(), "test", "serviceName", supplier)); diff --git a/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV1Test.java b/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV1Test.java index 15651b4ef..1bbb0685d 100644 --- a/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV1Test.java +++ b/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV1Test.java @@ -259,7 +259,7 @@ void startTestRecreateDatafeedError() throws ApiException { when(datafeedApi.v4DatafeedIdReadGet("persisted-id", "1234", "1234", null)) .thenThrow(new ApiException(400, "expired DF id")); when(datafeedApi.v4DatafeedCreatePost("1234", "1234")) - .thenThrow(new ApiException(500, "unhandled exception")); + .thenThrow(new ApiException(404, "unhandled exception")); assertThrows(NestedRetryException.class, () -> this.datafeedService.start()); diff --git a/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV2Test.java b/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV2Test.java index 3b24305a7..920b3d0c9 100644 --- a/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV2Test.java +++ b/symphony-bdk-core/src/test/java/com/symphony/bdk/core/service/datafeed/impl/DatafeedLoopV2Test.java @@ -531,7 +531,7 @@ void testStartInternalServerErrorReadDatafeedShouldNotBeRetried() throws ApiExce AckId ackId = datafeedService.getAckId(); when(datafeedApi.listDatafeed("1234", "1234", "tibot")).thenReturn( Collections.singletonList(new V5Datafeed().id("test-id"))); - when(datafeedApi.readDatafeed("test-id", "1234", "1234", ackId)).thenThrow(new ApiException(500, "client-error")); + when(datafeedApi.readDatafeed("test-id", "1234", "1234", ackId)).thenThrow(new ApiException(404, "client-error")); assertThrows(ApiException.class, this.datafeedService::start); verify(datafeedApi, times(1)).listDatafeed("1234", "1234", "tibot"); diff --git a/symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/ApiException.java b/symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/ApiException.java index e5b27f00d..59520c55b 100644 --- a/symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/ApiException.java +++ b/symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/ApiException.java @@ -81,7 +81,7 @@ public boolean isClientError() { * @return true if response status strictly greater than 500, false otherwise */ public boolean isServerError() { - return this.code > HttpURLConnection.HTTP_INTERNAL_ERROR; + return this.code >= HttpURLConnection.HTTP_INTERNAL_ERROR; } /** diff --git a/symphony-bdk-http/symphony-bdk-http-api/src/test/java/com/symphony/bdk/http/api/ApiExceptionTest.java b/symphony-bdk-http/symphony-bdk-http-api/src/test/java/com/symphony/bdk/http/api/ApiExceptionTest.java new file mode 100644 index 000000000..a7e91151a --- /dev/null +++ b/symphony-bdk-http/symphony-bdk-http-api/src/test/java/com/symphony/bdk/http/api/ApiExceptionTest.java @@ -0,0 +1,32 @@ +package com.symphony.bdk.http.api; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class ApiExceptionTest { + + @Test + void isServerError() { + assertFalse(new ApiException(499, "An error").isServerError()); + assertTrue(new ApiException(500, "Internal Server Error").isServerError()); + assertTrue(new ApiException(502, "Bad Gateway").isServerError()); + assertTrue(new ApiException(503, "Service Unavailable").isServerError()); + } + + @Test + void isUnauthorized() { + assertTrue(new ApiException(401, "Unauthorized").isUnauthorized()); + } + + @Test + void isClientError() { + assertTrue(new ApiException(400, "Bad Request").isClientError()); + } + + @Test + void isTooManyRequestsError() { + assertTrue(new ApiException(429, "Too Many Requests").isTooManyRequestsError()); + } +}