LAU-1090 replace custom retry with native openfeign retry (#408)

* LAU-1090 replace custom retry with native openfeign retry

build.gradle

@@ -3,6 +3,7 @@ plugins {
   id 'checkstyle'
   id 'pmd'
   id 'jacoco'
+  id 'idea'
   id 'io.spring.dependency-management' version '1.1.7'
   id 'org.springframework.boot' version '3.4.1'
   id 'org.owasp.dependencycheck' version '11.1.1'
@@ -77,6 +78,17 @@ sourceSets {
   }
 }
 
+idea {
+  module {
+    testSources.from(project.sourceSets.integrationTest.java.srcDirs)
+    testSources.from(project.sourceSets.functionalTest.java.srcDirs)
+    testSources.from(project.sourceSets.smokeTest.java.srcDirs)
+    testResources.from(project.sourceSets.integrationTest.resources.srcDirs)
+    testResources.from(project.sourceSets.functionalTest.resources.srcDirs)
+    testResources.from(project.sourceSets.smokeTest.resources.srcDirs)
+  }
+}
+
 configurations {
   cucumberRuntime {
     extendsFrom testImplementation
@@ -174,6 +186,8 @@ sonar {
     property "sonar.coverage.exclusions", "**/idam/Application.java"
     property "sonar.pitest.mode", "reuseReport"
     property "sonar.pitest.reportsDirectory", "build/reports/pitest"
+    property "sonar.sources", "src/main/java"
+    property "sonar.tests", "src/test/java,src/smokeTest/java,src/functionalTest/java,src/integrationTest/java"
   }
 }
 

src/integrationTest/java/uk/gov/hmcts/reform/idam/bdd/RetrySteps.java

@@ -32,11 +32,6 @@ public void idamApiRespondsWith500() {
     public void itShouldRetryMakingIdamCall() {
         service.run();
         wiremock.verify(2, WireMock.getRequestedFor(WireMock.urlPathEqualTo("/api/v1/staleUsers")));
-
-        wiremock.verify(WireMock.postRequestedFor(
-            WireMock.urlPathEqualTo("/o/token")
-        ));
-        wiremock.verify(WireMock.postRequestedFor(WireMock.urlPathEqualTo("/lease")));
     }
 
     @Then("it should throw exception")
@@ -45,6 +40,7 @@ public void itShouldthrowException() {
             service.run();
             fail("This line should not be reached");
         } catch (Exception e) {
+            wiremock.verify(3, WireMock.getRequestedFor(WireMock.urlPathEqualTo("/api/v1/staleUsers")));
             assertNotNull(e.getMessage());
         }
     }

src/integrationTest/resources/application-test.yaml

@@ -36,3 +36,5 @@ stale-users:
   citizen-roles: claimant,defendant,divorce-private-beta,cmc-private-beta,probate-private-beta
   citizen-letter-role-pattern: letter-
 
+service:
+  feign-retry-min-wait: 1

src/main/java/uk/gov/hmcts/reform/idam/Application.java

@@ -1,12 +1,14 @@
 package uk.gov.hmcts.reform.idam;
 
+import feign.codec.ErrorDecoder;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.boot.SpringApplication;
 import org.springframework.boot.autoconfigure.SpringBootApplication;
 import org.springframework.cloud.openfeign.EnableFeignClients;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.EnableAspectJAutoProxy;
+import uk.gov.hmcts.reform.idam.service.remote.CustomFeignErrorDecoder;
 
 import java.time.Clock;
 
@@ -22,6 +24,11 @@ public Clock clock() {
         return Clock.systemUTC();
     }
 
+    @Bean
+    public ErrorDecoder customFeignErrorDecoder() {
+        return new CustomFeignErrorDecoder();
+    }
+
     public static void main(final String[] args) {
         final ApplicationContext context = SpringApplication.run(Application.class);
         SpringApplication.exit(context);

src/main/java/uk/gov/hmcts/reform/idam/service/StaleUsersService.java

@@ -6,7 +6,6 @@
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Service;
 import uk.gov.hmcts.reform.idam.parameter.ParameterResolver;
-import uk.gov.hmcts.reform.idam.service.aop.Retry;
 import uk.gov.hmcts.reform.idam.service.remote.client.IdamClient;
 import uk.gov.hmcts.reform.idam.service.remote.responses.StaleUsersResponse;
 import uk.gov.hmcts.reform.idam.service.remote.responses.UserContent;
@@ -37,7 +36,6 @@ public class StaleUsersService {
     private final IdamTokenGenerator idamTokenGenerator;
     private final ParameterResolver parameterResolver;
 
-    @Retry(retryAttempts = 2)
     public List<String> fetchStaleUsers() {
         final StaleUsersResponse staleUsersResponse;
         try {

src/main/java/uk/gov/hmcts/reform/idam/service/UserRoleService.java

@@ -4,7 +4,6 @@
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.stereotype.Service;
 import uk.gov.hmcts.reform.idam.parameter.ParameterResolver;
-import uk.gov.hmcts.reform.idam.service.aop.Retry;
 import uk.gov.hmcts.reform.idam.service.remote.client.RoleAssignmentClient;
 import uk.gov.hmcts.reform.idam.service.remote.requests.RoleAssignmentsQueryRequest;
 import uk.gov.hmcts.reform.idam.service.remote.responses.RoleAssignment;
@@ -27,7 +26,6 @@ public class UserRoleService {
     private final SecurityUtil securityUtil;
     private final ParameterResolver parameterResolver;
 
-    @Retry(retryAttempts = 2)
     public List<String> filterUsersWithRoles(List<String> staleUsers) {
         if (staleUsers.isEmpty()) {
             return List.of();

src/main/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoder.java

@@ -0,0 +1,28 @@
+package uk.gov.hmcts.reform.idam.service.remote;
+
+import feign.FeignException;
+import feign.Response;
+import feign.RetryableException;
+import feign.codec.ErrorDecoder;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class CustomFeignErrorDecoder implements ErrorDecoder {
+
+    @Override
+    public Exception decode(String methodKey, Response response) {
+        int status = response.status();
+        FeignException exception = FeignException.errorStatus(methodKey, response);
+        log.info("Feign response status: {}, message - {}", status, exception.getMessage());
+        if (status >= 400) {
+            return new RetryableException(
+                response.status(),
+                exception.getMessage(),
+                response.request().httpMethod(),
+                (Long) null,  // unix timestamp, if given retries after that point in time
+                response.request()
+            );
+        }
+        return exception;
+    }
+}

src/main/java/uk/gov/hmcts/reform/idam/service/remote/RetryableFeignConfig.java

@@ -0,0 +1,24 @@
+package uk.gov.hmcts.reform.idam.service.remote;
+
+import feign.Retryer;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+@Configuration
+public class RetryableFeignConfig {
+
+    @Value("${service.feign-retry-min-wait:60}")
+    private int periodToWait;
+
+    @Bean
+    public Retryer retryer() {
+        long period = SECONDS.toMillis(periodToWait);
+        long maxPeriod = SECONDS.toMillis(300);
+        // default backoff calculation formula:
+        // Math.min(period * Math.pow(1.5, attempt - 1), maxPeriod)
+        return new Retryer.Default(period, maxPeriod, 3);
+    }
+}

src/main/resources/application.yaml

@@ -28,6 +28,7 @@ ccd:
 
 service:
   enabled: ${DISPOSER_IDAM_USER_ENABLED:false}
+  feign-retry-min-wait: ${DISPOSER_IDAM_USER_FEIGN_RETRY_MIN_WAIT:60} # in seconds
 
 stale-users:
   batch.size: ${DISPOSER_IDAM_USER_BATCH_SIZE:100}

src/test/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoderTest.java

@@ -0,0 +1,53 @@
+package uk.gov.hmcts.reform.idam.service.remote;
+
+import feign.Request;
+import feign.RequestTemplate;
+import feign.Response;
+import feign.RetryableException;
+import feign.codec.ErrorDecoder;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+import java.util.Map;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class CustomFeignErrorDecoderTest {
+
+    private final ErrorDecoder errorDecoder = new CustomFeignErrorDecoder();
+
+    @ParameterizedTest
+    @CsvSource({
+        "400, Bad Request",
+        "401, Unauthorized",
+        "403, Forbidden",
+        "500, Internal Server Error",
+        "502, Bad Gateway",
+        "503, Service Unavailable",
+        "504, Gateway Timeout"
+    })
+    void testDecodeErrorReturnsRetryable(int status, String reason) {
+
+        Response response = Response.builder()
+            .status(status)
+            .reason(reason)
+            .request(Request.create(Request.HttpMethod.GET, "/", Map.of(), null, new RequestTemplate()))
+            .build();
+        Exception exc = errorDecoder.decode("methodKey", response);
+        assertThat(exc).isInstanceOf(RetryableException.class);
+    }
+
+    @Test
+    void testDecodeNonRetryableException() {
+        Response response = Response.builder()
+            .status(302)
+            .reason("Found")
+            .request(Request.create(Request.HttpMethod.GET, "/", Map.of(), null, new RequestTemplate()))
+            .build();
+        Exception exc = errorDecoder.decode("methodKey", response);
+        assertThat(exc)
+            .isInstanceOf(feign.FeignException.class)
+            .isNotInstanceOf(RetryableException.class);
+    }
+}