From 5fedd01d0a4af0d8cd4f8a6ceba5e31c0f26f84c Mon Sep 17 00:00:00 2001 From: "antonio.torre" Date: Wed, 5 Feb 2025 19:31:36 +0100 Subject: [PATCH 1/3] fix ErrorHandling and LocalDateTime json serialization --- build.gradle.kts | 3 +- gradle.lockfile | 4 + openapi/p4pa-workflow-worker.openapi.yaml | 14 ++ .../exception/WorkerExceptionHandler.java | 96 +++++++++ .../exception/WorkerExceptionHandlerTest.java | 184 ++++++++++++++++++ 5 files changed, 300 insertions(+), 1 deletion(-) create mode 100644 src/main/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandler.java create mode 100644 src/test/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandlerTest.java diff --git a/build.gradle.kts b/build.gradle.kts index f4d046f..7d261af 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -44,10 +44,10 @@ val temporalVersion = "1.27.0" val protobufJavaVersion = "3.25.5" val bouncycastleVersion = "1.79" - dependencies { implementation("org.springframework.boot:spring-boot-starter") implementation("org.springframework.boot:spring-boot-starter-web") + implementation("org.springframework.boot:spring-boot-starter-validation") implementation("org.springframework.boot:spring-boot-starter-oauth2-resource-server") implementation("org.springframework.boot:spring-boot-starter-actuator") implementation("io.micrometer:micrometer-tracing-bridge-otel:$micrometerVersion") @@ -71,6 +71,7 @@ dependencies { compileOnly("org.projectlombok:lombok") annotationProcessor("org.projectlombok:lombok") + testAnnotationProcessor("org.projectlombok:lombok") // Testing testImplementation("org.springframework.boot:spring-boot-starter-test") diff --git a/gradle.lockfile b/gradle.lockfile index 0de5a92..f9c7b9e 100644 --- a/gradle.lockfile +++ b/gradle.lockfile @@ -12,6 +12,7 @@ com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.18.2=compileClasspath com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.18.2=compileClasspath com.fasterxml.jackson.module:jackson-module-parameter-names:2.18.2=compileClasspath com.fasterxml.jackson:jackson-bom:2.18.2=compileClasspath +com.fasterxml:classmate:1.7.0=compileClasspath com.github.stephenc.jcip:jcip-annotations:1.0-1=compileClasspath com.google.api.grpc:proto-google-common-protos:2.9.0=compileClasspath com.google.code.findbugs:jsr305:3.0.2=compileClasspath @@ -84,6 +85,8 @@ org.apache.tomcat.embed:tomcat-embed-el:10.1.34=compileClasspath org.apache.tomcat.embed:tomcat-embed-websocket:10.1.34=compileClasspath org.bouncycastle:bcprov-jdk18on:1.79=compileClasspath org.checkerframework:checker-qual:3.43.0=compileClasspath +org.hibernate.validator:hibernate-validator:8.0.2.Final=compileClasspath +org.jboss.logging:jboss-logging:3.6.1.Final=compileClasspath org.jspecify:jspecify:1.0.0=compileClasspath org.openapitools:jackson-databind-nullable:0.2.6=compileClasspath org.projectlombok:lombok:1.18.36=compileClasspath @@ -101,6 +104,7 @@ org.springframework.boot:spring-boot-starter-json:3.4.1=compileClasspath org.springframework.boot:spring-boot-starter-logging:3.4.1=compileClasspath org.springframework.boot:spring-boot-starter-oauth2-resource-server:3.4.1=compileClasspath org.springframework.boot:spring-boot-starter-tomcat:3.4.1=compileClasspath +org.springframework.boot:spring-boot-starter-validation:3.4.1=compileClasspath org.springframework.boot:spring-boot-starter-web:3.4.1=compileClasspath org.springframework.boot:spring-boot-starter:3.4.1=compileClasspath org.springframework.boot:spring-boot:3.4.1=compileClasspath diff --git a/openapi/p4pa-workflow-worker.openapi.yaml b/openapi/p4pa-workflow-worker.openapi.yaml index 5da5345..fc202e6 100644 --- a/openapi/p4pa-workflow-worker.openapi.yaml +++ b/openapi/p4pa-workflow-worker.openapi.yaml @@ -32,6 +32,20 @@ paths: security: - BearerAuth: [] components: + schemas: + WorkerErrorDTO: + type: object + required: + - code + - message + properties: + code: + type: string + enum: + - WORKER_BAD_REQUEST + - WORKER_GENERIC_ERROR + message: + type: string securitySchemes: BearerAuth: type: http diff --git a/src/main/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandler.java b/src/main/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandler.java new file mode 100644 index 0000000..18be6cc --- /dev/null +++ b/src/main/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandler.java @@ -0,0 +1,96 @@ +package it.gov.pagopa.pu.worker.exception; + +import com.fasterxml.jackson.databind.JsonMappingException; +import it.gov.pagopa.pu.worker.dto.generated.WorkerErrorDTO; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.validation.ValidationException; +import lombok.extern.slf4j.Slf4j; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; +import org.springframework.http.HttpStatus; +import org.springframework.http.HttpStatusCode; +import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.validation.FieldError; +import org.springframework.web.ErrorResponse; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +import java.util.stream.Collectors; + +@RestControllerAdvice +@Slf4j +@Order(Ordered.HIGHEST_PRECEDENCE) +public class WorkerExceptionHandler { + + @ExceptionHandler({ValidationException.class, HttpMessageNotReadableException.class, MethodArgumentNotValidException.class}) + public ResponseEntity handleViolationException(Exception ex, HttpServletRequest request) { + return handleException(ex, request, HttpStatus.BAD_REQUEST, WorkerErrorDTO.CodeEnum.BAD_REQUEST); + } + + @ExceptionHandler({ServletException.class}) + public ResponseEntity handleServletException(ServletException ex, HttpServletRequest request) { + HttpStatusCode httpStatus = HttpStatus.INTERNAL_SERVER_ERROR; + WorkerErrorDTO.CodeEnum errorCode = WorkerErrorDTO.CodeEnum.GENERIC_ERROR; + if (ex instanceof ErrorResponse errorResponse) { + httpStatus = errorResponse.getStatusCode(); + if (httpStatus.is4xxClientError()) { + errorCode = WorkerErrorDTO.CodeEnum.BAD_REQUEST; + } + } + return handleException(ex, request, httpStatus, errorCode); + } + + @ExceptionHandler({RuntimeException.class}) + public ResponseEntity handleRuntimeException(RuntimeException ex, HttpServletRequest request) { + return handleException(ex, request, HttpStatus.INTERNAL_SERVER_ERROR, WorkerErrorDTO.CodeEnum.GENERIC_ERROR); + } + + static ResponseEntity handleException(Exception ex, HttpServletRequest request, HttpStatusCode httpStatus, WorkerErrorDTO.CodeEnum errorEnum) { + logException(ex, request, httpStatus); + + String message = buildReturnedMessage(ex); + + return ResponseEntity + .status(httpStatus) + .body(new WorkerErrorDTO(errorEnum, message)); + } + + private static void logException(Exception ex, HttpServletRequest request, HttpStatusCode httpStatus) { + log.info("A {} occurred handling request {}: HttpStatus {} - {}", + ex.getClass(), + getRequestDetails(request), + httpStatus.value(), + ex.getMessage()); + } + + private static String buildReturnedMessage(Exception ex) { + if (ex instanceof HttpMessageNotReadableException) { + if(ex.getCause() instanceof JsonMappingException jsonMappingException){ + return "Cannot parse body: " + + jsonMappingException.getPath().stream() + .map(JsonMappingException.Reference::getFieldName) + .collect(Collectors.joining(".")) + + ": " + jsonMappingException.getOriginalMessage(); + } + return "Required request body is missing"; + } else if (ex instanceof MethodArgumentNotValidException methodArgumentNotValidException) { + return "Invalid request content:" + + methodArgumentNotValidException.getBindingResult() + .getAllErrors().stream() + .map(e -> " " + + (e instanceof FieldError fieldError? fieldError.getField(): e.getObjectName()) + + ": " + e.getDefaultMessage()) + .sorted() + .collect(Collectors.joining(";")); + } else { + return ex.getMessage(); + } + } + + static String getRequestDetails(HttpServletRequest request) { + return "%s %s".formatted(request.getMethod(), request.getRequestURI()); + } +} diff --git a/src/test/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandlerTest.java b/src/test/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandlerTest.java new file mode 100644 index 0000000..5fbdacd --- /dev/null +++ b/src/test/java/it/gov/pagopa/pu/worker/exception/WorkerExceptionHandlerTest.java @@ -0,0 +1,184 @@ +package it.gov.pagopa.pu.worker.exception; + +import com.fasterxml.jackson.databind.ObjectMapper; +import it.gov.pagopa.payhub.activities.config.json.JsonConfig; +import jakarta.servlet.ServletException; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Pattern; +import lombok.AllArgsConstructor; +import lombok.Data; +import lombok.NoArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration; +import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; +import org.springframework.http.MediaType; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.bean.override.mockito.MockitoSpyBean; +import org.springframework.test.context.junit.jupiter.SpringExtension; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.ResultActions; +import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; +import org.springframework.test.web.servlet.result.MockMvcResultMatchers; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.server.ServerErrorException; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter; + +import java.time.LocalDateTime; +import java.util.Set; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; + +@ExtendWith({SpringExtension.class}) +@WebMvcTest(value = {WorkerExceptionHandlerTest.TestController.class}, excludeAutoConfiguration = SecurityAutoConfiguration.class) +@ContextConfiguration(classes = { + WorkerExceptionHandlerTest.TestController.class, + WorkerExceptionHandler.class, + JsonConfig.class}) +class WorkerExceptionHandlerTest { + + public static final String DATA = "data"; + public static final TestRequestBody BODY = new TestRequestBody("bodyData", null, "abc", LocalDateTime.now()); + + @Autowired + private MockMvc mockMvc; + @Autowired + private ObjectMapper objectMapper; + + @MockitoSpyBean + private TestController testControllerSpy; + @MockitoSpyBean + private RequestMappingHandlerAdapter requestMappingHandlerAdapterSpy; + + @RestController + @Slf4j + static class TestController { + @PostMapping(value = "/test", produces = MediaType.APPLICATION_JSON_VALUE) + String testEndpoint(@RequestParam(DATA) String data, @Valid @RequestBody TestRequestBody body) { + return "OK"; + } + } + + @Data + @NoArgsConstructor + @AllArgsConstructor + public static class TestRequestBody { + @NotNull + private String requiredField; + private String notRequiredField; + @Pattern(regexp = "[a-z]+") + private String lowerCaseAlphabeticField; + private LocalDateTime dateTimeField; + } + + private ResultActions performRequest(String data, MediaType accept) throws Exception { + return performRequest(data, accept, objectMapper.writeValueAsString(WorkerExceptionHandlerTest.BODY)); + } + + private ResultActions performRequest(String data, MediaType accept, String body) throws Exception { + MockHttpServletRequestBuilder requestBuilder = MockMvcRequestBuilders.post("/test") + .param(DATA, data) + .accept(accept); + + if (body != null) { + requestBuilder + .contentType(MediaType.APPLICATION_JSON) + .content(body); + } + + return mockMvc.perform(requestBuilder); + } + + @Test + void handleMissingServletRequestParameterException() throws Exception { + performRequest(null, MediaType.APPLICATION_JSON) + .andExpect(MockMvcResultMatchers.status().isBadRequest()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_BAD_REQUEST")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Required request parameter 'data' for method parameter type String is not present")); + } + + @Test + void handleRuntimeExceptionError() throws Exception { + doThrow(new RuntimeException("Error")).when(testControllerSpy).testEndpoint(DATA, BODY); + + performRequest(DATA, MediaType.APPLICATION_JSON) + .andExpect(MockMvcResultMatchers.status().isInternalServerError()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_GENERIC_ERROR")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Error")); + } + + @Test + void handleGenericServletException() throws Exception { + doThrow(new ServletException("Error")) + .when(requestMappingHandlerAdapterSpy).handle(any(), any(), any()); + + performRequest(DATA, MediaType.APPLICATION_JSON) + .andExpect(MockMvcResultMatchers.status().isInternalServerError()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_GENERIC_ERROR")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Error")); + } + + @Test + void handle4xxHttpServletException() throws Exception { + performRequest(DATA, MediaType.parseMediaType("application/hal+json")) + .andExpect(MockMvcResultMatchers.status().isNotAcceptable()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_BAD_REQUEST")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("No acceptable representation")); + } + + @Test + void handleNoBodyException() throws Exception { + performRequest(DATA, MediaType.APPLICATION_JSON, null) + .andExpect(MockMvcResultMatchers.status().isBadRequest()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_BAD_REQUEST")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Required request body is missing")); + } + + @Test + void handleInvalidBodyException() throws Exception { + performRequest(DATA, MediaType.APPLICATION_JSON, + "{\"notRequiredField\":\"notRequired\",\"lowerCaseAlphabeticField\":\"ABC\"}") + .andExpect(MockMvcResultMatchers.status().isBadRequest()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_BAD_REQUEST")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Invalid request content: lowerCaseAlphabeticField: must match \"[a-z]+\"; requiredField: must not be null")); + } + + @Test + void handleNotParsableBodyException() throws Exception { + performRequest(DATA, MediaType.APPLICATION_JSON, + "{\"notRequiredField\":\"notRequired\",\"dateTimeField\":\"2025-02-05\"}") + .andExpect(MockMvcResultMatchers.status().isBadRequest()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_BAD_REQUEST")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Cannot parse body: dateTimeField: Text '2025-02-05' could not be parsed at index 10")); + } + + @Test + void handle5xxHttpServletException() throws Exception { + doThrow(new ServerErrorException("Error", new RuntimeException("Error"))) + .when(requestMappingHandlerAdapterSpy).handle(any(), any(), any()); + + performRequest(DATA, MediaType.APPLICATION_JSON) + .andExpect(MockMvcResultMatchers.status().isInternalServerError()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_GENERIC_ERROR")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("500 INTERNAL_SERVER_ERROR \"Error\"")); + } + + @Test + void handleViolationException() throws Exception { + doThrow(new ConstraintViolationException("Error", Set.of())).when(testControllerSpy).testEndpoint(DATA, BODY); + + performRequest(DATA, MediaType.APPLICATION_JSON) + .andExpect(MockMvcResultMatchers.status().isBadRequest()) + .andExpect(MockMvcResultMatchers.jsonPath("$.code").value("WORKER_BAD_REQUEST")) + .andExpect(MockMvcResultMatchers.jsonPath("$.message").value("Error")); + } +} From ed45092cb2f145a0398266e2e1eec62f45eba4cd Mon Sep 17 00:00:00 2001 From: "antonio.torre" Date: Wed, 5 Feb 2025 20:06:24 +0100 Subject: [PATCH 2/3] fix ErrorHandling and LocalDateTime json serialization --- .github/workflows/codereview.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/codereview.yml b/.github/workflows/codereview.yml index bd4b5d8..3eb08d3 100644 --- a/.github/workflows/codereview.yml +++ b/.github/workflows/codereview.yml @@ -31,7 +31,7 @@ jobs: - name: Build with Gradle working-directory: ./ - run: ./gradlew clean build jacocoTestReport + run: ./gradlew --info clean build jacocoTestReport env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} From 824483d25b2e57ed7436e60441b26fc1e19363b4 Mon Sep 17 00:00:00 2001 From: "antonio.torre" Date: Wed, 5 Feb 2025 21:16:22 +0100 Subject: [PATCH 3/3] fix: LocalDateTime json serialization --- .github/workflows/codereview.yml | 2 +- build.gradle.kts | 2 +- gradle.lockfile | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/codereview.yml b/.github/workflows/codereview.yml index 3eb08d3..bd4b5d8 100644 --- a/.github/workflows/codereview.yml +++ b/.github/workflows/codereview.yml @@ -31,7 +31,7 @@ jobs: - name: Build with Gradle working-directory: ./ - run: ./gradlew --info clean build jacocoTestReport + run: ./gradlew clean build jacocoTestReport env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/build.gradle.kts b/build.gradle.kts index 7d261af..8f275fe 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -39,7 +39,7 @@ repositories { val springDocOpenApiVersion = "2.7.0" val openApiToolsVersion = "0.2.6" val micrometerVersion = "1.4.1" -val p4paActivitiesVersion = "1.47.2" +val p4paActivitiesVersion = "1.47.3" val temporalVersion = "1.27.0" val protobufJavaVersion = "3.25.5" val bouncycastleVersion = "1.79" diff --git a/gradle.lockfile b/gradle.lockfile index f9c7b9e..09cce34 100644 --- a/gradle.lockfile +++ b/gradle.lockfile @@ -72,7 +72,7 @@ io.temporal:temporal-spring-boot-autoconfigure:1.27.0=compileClasspath io.temporal:temporal-spring-boot-starter:1.27.0=compileClasspath io.temporal:temporal-test-server:1.27.0=compileClasspath io.temporal:temporal-testing:1.27.0=compileClasspath -it.gov.pagopa.payhub:p4pa-payhub-activities:1.47.2=compileClasspath +it.gov.pagopa.payhub:p4pa-payhub-activities:1.47.3=compileClasspath jakarta.activation:jakarta.activation-api:2.1.3=compileClasspath jakarta.annotation:jakarta.annotation-api:2.1.1=compileClasspath jakarta.validation:jakarta.validation-api:3.0.2=compileClasspath