From a584c3c5a1bb9c086f71cb864f8b1fca29d8609d Mon Sep 17 00:00:00 2001 From: Rapolas <130578328+rapolaskaseliscgi@users.noreply.github.com> Date: Thu, 16 Jan 2025 14:11:25 +0000 Subject: [PATCH] LAU-1090 replace custom retry with native openfeign retry (#408) * LAU-1090 replace custom retry with native openfeign retry --- build.gradle | 14 +++ .../gov/hmcts/reform/idam/bdd/RetrySteps.java | 6 +- .../resources/application-test.yaml | 2 + .../uk/gov/hmcts/reform/idam/Application.java | 7 ++ .../idam/service/StaleUsersService.java | 2 - .../reform/idam/service/UserRoleService.java | 2 - .../idam/service/aop/MethodRetryAspect.java | 53 ----------- .../hmcts/reform/idam/service/aop/Retry.java | 13 --- .../remote/CustomFeignErrorDecoder.java | 28 ++++++ .../service/remote/RetryableFeignConfig.java | 24 +++++ src/main/resources/application.yaml | 1 + .../service/aop/MethodRetryAspectTest.java | 93 ------------------- .../remote/CustomFeignErrorDecoderTest.java | 53 +++++++++++ 13 files changed, 130 insertions(+), 168 deletions(-) delete mode 100644 src/main/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspect.java delete mode 100644 src/main/java/uk/gov/hmcts/reform/idam/service/aop/Retry.java create mode 100644 src/main/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoder.java create mode 100644 src/main/java/uk/gov/hmcts/reform/idam/service/remote/RetryableFeignConfig.java delete mode 100644 src/test/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspectTest.java create mode 100644 src/test/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoderTest.java diff --git a/build.gradle b/build.gradle index 37b7743e..c9145ff5 100644 --- a/build.gradle +++ b/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" } } diff --git a/src/integrationTest/java/uk/gov/hmcts/reform/idam/bdd/RetrySteps.java b/src/integrationTest/java/uk/gov/hmcts/reform/idam/bdd/RetrySteps.java index 47e6bb62..6e2ad142 100644 --- a/src/integrationTest/java/uk/gov/hmcts/reform/idam/bdd/RetrySteps.java +++ b/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()); } } diff --git a/src/integrationTest/resources/application-test.yaml b/src/integrationTest/resources/application-test.yaml index 2ec357e7..2fe29905 100644 --- a/src/integrationTest/resources/application-test.yaml +++ b/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 diff --git a/src/main/java/uk/gov/hmcts/reform/idam/Application.java b/src/main/java/uk/gov/hmcts/reform/idam/Application.java index 2e0fb4c6..a21dca73 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/Application.java +++ b/src/main/java/uk/gov/hmcts/reform/idam/Application.java @@ -1,5 +1,6 @@ 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; @@ -7,6 +8,7 @@ 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); diff --git a/src/main/java/uk/gov/hmcts/reform/idam/service/StaleUsersService.java b/src/main/java/uk/gov/hmcts/reform/idam/service/StaleUsersService.java index 7baf48e2..5db23161 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/service/StaleUsersService.java +++ b/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 fetchStaleUsers() { final StaleUsersResponse staleUsersResponse; try { diff --git a/src/main/java/uk/gov/hmcts/reform/idam/service/UserRoleService.java b/src/main/java/uk/gov/hmcts/reform/idam/service/UserRoleService.java index e3baa50a..0b6b664e 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/service/UserRoleService.java +++ b/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 filterUsersWithRoles(List staleUsers) { if (staleUsers.isEmpty()) { return List.of(); diff --git a/src/main/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspect.java b/src/main/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspect.java deleted file mode 100644 index be8524d0..00000000 --- a/src/main/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspect.java +++ /dev/null @@ -1,53 +0,0 @@ -package uk.gov.hmcts.reform.idam.service.aop; - -import feign.FeignException; -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.apache.http.HttpStatus; -import org.aspectj.lang.ProceedingJoinPoint; -import org.aspectj.lang.annotation.Around; -import org.aspectj.lang.annotation.Aspect; -import org.aspectj.lang.reflect.MethodSignature; -import org.springframework.stereotype.Component; -import uk.gov.hmcts.reform.idam.util.SecurityUtil; - -import java.lang.reflect.Method; - -@Aspect -@Slf4j -@Component -@RequiredArgsConstructor -public class MethodRetryAspect { - - private final SecurityUtil securityUtil; - - @Around("@annotation(uk.gov.hmcts.reform.idam.service.aop.Retry)") - @SuppressWarnings("PMD.LawOfDemeter") - public Object retry(ProceedingJoinPoint joinPoint) throws Throwable { - Method method = ((MethodSignature) joinPoint.getSignature()).getMethod(); - - Retry retry = method.getDeclaredAnnotation(Retry.class); - int retryAttempts = retry.retryAttempts(); - Object result = null; - boolean successful = false; - do { - retryAttempts--; - try { - result = joinPoint.proceed(); - successful = true; - } catch (FeignException fe) { - if (retryAttempts >= 0 && fe.status() == HttpStatus.SC_UNAUTHORIZED) { - log.info("Method {} with {} arguments threw FeignException ", - joinPoint.getSignature(), - joinPoint.getArgs()); - securityUtil.generateTokens(); - } else { - log.error("IdamClient threw exception", fe); - throw fe; - } - } - } while (!successful); - - return result; - } -} diff --git a/src/main/java/uk/gov/hmcts/reform/idam/service/aop/Retry.java b/src/main/java/uk/gov/hmcts/reform/idam/service/aop/Retry.java deleted file mode 100644 index 57ac3b71..00000000 --- a/src/main/java/uk/gov/hmcts/reform/idam/service/aop/Retry.java +++ /dev/null @@ -1,13 +0,0 @@ -package uk.gov.hmcts.reform.idam.service.aop; - -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -@Retention(RetentionPolicy.RUNTIME) -@Target(ElementType.METHOD) -public @interface Retry { - int retryAttempts() default 3; - -} diff --git a/src/main/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoder.java b/src/main/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoder.java new file mode 100644 index 00000000..93033d65 --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/uk/gov/hmcts/reform/idam/service/remote/RetryableFeignConfig.java b/src/main/java/uk/gov/hmcts/reform/idam/service/remote/RetryableFeignConfig.java new file mode 100644 index 00000000..8ae80b9f --- /dev/null +++ b/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); + } +} diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 1f129d41..66628841 100644 --- a/src/main/resources/application.yaml +++ b/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} diff --git a/src/test/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspectTest.java b/src/test/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspectTest.java deleted file mode 100644 index 232f6e98..00000000 --- a/src/test/java/uk/gov/hmcts/reform/idam/service/aop/MethodRetryAspectTest.java +++ /dev/null @@ -1,93 +0,0 @@ -package uk.gov.hmcts.reform.idam.service.aop; - -import feign.FeignException; -import feign.Request; -import feign.RequestTemplate; -import org.aspectj.lang.ProceedingJoinPoint; -import org.aspectj.lang.reflect.MethodSignature; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; -import uk.gov.hmcts.reform.idam.util.SecurityUtil; - -import java.lang.reflect.Method; -import java.util.HashMap; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -@ExtendWith(MockitoExtension.class) -class MethodRetryAspectTest { - - @Mock - ProceedingJoinPoint proceedingJoinPoint; - - @Mock - SecurityUtil securityUtil; - - @InjectMocks - private MethodRetryAspect methodRetryAspect; - - @BeforeEach - void setUp() throws NoSuchMethodException { - methodRetryAspect = new MethodRetryAspect(securityUtil); - MethodSignature signature = mock(MethodSignature.class); - - when(proceedingJoinPoint.getSignature()).thenReturn(signature); - when(signature.getMethod()).thenReturn(methodGetter()); - - } - - @Test - void testRetryProceeds() throws Throwable { - methodRetryAspect.retry(proceedingJoinPoint); - verify(proceedingJoinPoint, times(1)).proceed(); - } - - @Test - void testRetryCatches401() throws Throwable { - Request request = Request.create(Request.HttpMethod.GET, "url", new HashMap<>(), null, new RequestTemplate()); - byte[] body = {}; - - when(proceedingJoinPoint.proceed()) - .thenThrow(new FeignException.Unauthorized("Unauthorized", request, body, null)) - .thenReturn(new Object()); - - methodRetryAspect.retry(proceedingJoinPoint); - verify(securityUtil, times(1)).generateTokens(); - verify(proceedingJoinPoint, times(2)).proceed(); - } - - @Test - void testRetryDoesNotRetryOnNon403() throws Throwable { - Request request = Request.create(Request.HttpMethod.GET, "url", new HashMap<>(), null, new RequestTemplate()); - byte[] body = {}; - - when(proceedingJoinPoint.proceed()) - .thenThrow(new FeignException.InternalServerError("Internal Server Error", request, body, null)) - .thenReturn(new Object()); - - Exception thrown = assertThrows(FeignException.InternalServerError.class, () -> { - methodRetryAspect.retry(proceedingJoinPoint); - }); - - verify(securityUtil, times(0)).generateTokens(); - verify(proceedingJoinPoint, times(1)).proceed(); - assertThat(thrown.getMessage()).contains("Internal Server Error"); - } - - public Method methodGetter() throws NoSuchMethodException { - return getClass().getDeclaredMethod("methodStub"); - } - - @Retry(retryAttempts = 2) - public void methodStub() { - } -} diff --git a/src/test/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoderTest.java b/src/test/java/uk/gov/hmcts/reform/idam/service/remote/CustomFeignErrorDecoderTest.java new file mode 100644 index 00000000..7fe6c081 --- /dev/null +++ b/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); + } +}