#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.

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<String> 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<String> 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<String> 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<String>(), "test", "serviceName", supplier));

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());
 

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");

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;
     }
 
     /**

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());
+  }
+}