From 921ade771bb2c79c565de784e746afa1620e25b2 Mon Sep 17 00:00:00 2001 From: anton Date: Fri, 11 Oct 2024 12:20:52 +0200 Subject: [PATCH] FAIRSPC-81: tuned global exception handling --- .../saturn/controller/dto/ErrorDto.java | 12 ++ .../exception/GlobalExceptionHandler.java | 30 ++-- .../services/PayloadParsingException.java | 25 ---- .../saturn/services/errors/ErrorDto.java | 20 --- .../saturn/controller/BaseControllerTest.java | 2 +- .../exception/GlobalExceptionHandlerTest.java | 135 ++++++++++++++++++ .../controller/exception/TestController.java | 23 +++ 7 files changed, 184 insertions(+), 63 deletions(-) create mode 100644 projects/saturn/src/main/java/io/fairspace/saturn/controller/dto/ErrorDto.java delete mode 100644 projects/saturn/src/main/java/io/fairspace/saturn/services/PayloadParsingException.java delete mode 100644 projects/saturn/src/main/java/io/fairspace/saturn/services/errors/ErrorDto.java create mode 100644 projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandlerTest.java create mode 100644 projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/TestController.java diff --git a/projects/saturn/src/main/java/io/fairspace/saturn/controller/dto/ErrorDto.java b/projects/saturn/src/main/java/io/fairspace/saturn/controller/dto/ErrorDto.java new file mode 100644 index 000000000..0741c4990 --- /dev/null +++ b/projects/saturn/src/main/java/io/fairspace/saturn/controller/dto/ErrorDto.java @@ -0,0 +1,12 @@ +package io.fairspace.saturn.controller.dto; + +import ioinformarics.oss.jackson.module.jsonld.annotation.JsonldProperty; +import ioinformarics.oss.jackson.module.jsonld.annotation.JsonldType; + +import io.fairspace.saturn.vocabulary.FS; + +@JsonldType(FS.ERROR_URI) +public record ErrorDto( + @JsonldProperty(FS.ERROR_STATUS_URI) int status, + @JsonldProperty(FS.ERROR_MESSAGE_URI) String message, + @JsonldProperty(FS.ERROR_DETAILS_URI) Object details) {} diff --git a/projects/saturn/src/main/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandler.java b/projects/saturn/src/main/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandler.java index 55e4deab1..d8ac294b1 100644 --- a/projects/saturn/src/main/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandler.java +++ b/projects/saturn/src/main/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandler.java @@ -1,44 +1,41 @@ package io.fairspace.saturn.controller.exception; +import java.util.stream.Collectors; + import jakarta.servlet.http.HttpServletRequest; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.security.access.AccessDeniedException; import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.bind.annotation.ResponseStatus; -import io.fairspace.saturn.services.PayloadParsingException; -import io.fairspace.saturn.services.errors.ErrorDto; +import io.fairspace.saturn.controller.dto.ErrorDto; import io.fairspace.saturn.services.metadata.validation.ValidationException; @Slf4j @ControllerAdvice public class GlobalExceptionHandler { - // // todo: add tests - // @ExceptionHandler(AccessDeniedException.class) - // public ResponseEntity handleAccessDenied(AccessDeniedException ex) { - // return new ResponseEntity<>("Access Denied: " + ex.getMessage(), HttpStatus.FORBIDDEN); - // } - - @ExceptionHandler(PayloadParsingException.class) - @ResponseStatus(HttpStatus.BAD_REQUEST) - public ResponseEntity handlePayloadParsingException(PayloadParsingException ex, HttpServletRequest req) { - log.error("Malformed request body for request {} {}", req.getMethod(), req.getRequestURI(), ex); - return buildErrorResponse(HttpStatus.BAD_REQUEST, "Malformed request body"); + @ExceptionHandler(ConstraintViolationException.class) + public ResponseEntity handleConstraintViolationException( + ConstraintViolationException ex, HttpServletRequest req) { + var violations = ex.getConstraintViolations().stream() + .map(ConstraintViolation::getMessage) + .sorted() + .collect(Collectors.joining("; ")); + return buildErrorResponse(HttpStatus.BAD_REQUEST, "Validation Error", "Violations: " + violations); } @ExceptionHandler(ValidationException.class) - @ResponseStatus(HttpStatus.BAD_REQUEST) public ResponseEntity handleValidationException(ValidationException ex, HttpServletRequest req) { log.error("Validation error for request {} {}", req.getMethod(), req.getRequestURI(), ex); return buildErrorResponse(HttpStatus.BAD_REQUEST, "Validation Error", ex.getViolations()); } @ExceptionHandler(IllegalArgumentException.class) - @ResponseStatus(HttpStatus.BAD_REQUEST) public ResponseEntity handleIllegalArgumentException( IllegalArgumentException ex, HttpServletRequest req) { log.error("Validation error for request {} {}", req.getMethod(), req.getRequestURI(), ex); @@ -46,7 +43,6 @@ public ResponseEntity handleIllegalArgumentException( } @ExceptionHandler(AccessDeniedException.class) - @ResponseStatus(HttpStatus.FORBIDDEN) public ResponseEntity handleAccessDeniedException(AccessDeniedException ex, HttpServletRequest req) { log.error("Access denied for request {} {}", req.getMethod(), req.getRequestURI(), ex); return buildErrorResponse(HttpStatus.FORBIDDEN, "Access Denied"); diff --git a/projects/saturn/src/main/java/io/fairspace/saturn/services/PayloadParsingException.java b/projects/saturn/src/main/java/io/fairspace/saturn/services/PayloadParsingException.java deleted file mode 100644 index 7fc4e4d7b..000000000 --- a/projects/saturn/src/main/java/io/fairspace/saturn/services/PayloadParsingException.java +++ /dev/null @@ -1,25 +0,0 @@ -package io.fairspace.saturn.services; - -/** - * Can represent an error that happened during parsing of HTTP request body, etc - */ -public class PayloadParsingException extends RuntimeException { - public PayloadParsingException() {} - - public PayloadParsingException(String message) { - super(message); - } - - public PayloadParsingException(String message, Throwable cause) { - super(message, cause); - } - - public PayloadParsingException(Throwable cause) { - super(cause); - } - - public PayloadParsingException( - String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { - super(message, cause, enableSuppression, writableStackTrace); - } -} diff --git a/projects/saturn/src/main/java/io/fairspace/saturn/services/errors/ErrorDto.java b/projects/saturn/src/main/java/io/fairspace/saturn/services/errors/ErrorDto.java deleted file mode 100644 index e841b7488..000000000 --- a/projects/saturn/src/main/java/io/fairspace/saturn/services/errors/ErrorDto.java +++ /dev/null @@ -1,20 +0,0 @@ -package io.fairspace.saturn.services.errors; - -import ioinformarics.oss.jackson.module.jsonld.annotation.JsonldProperty; -import ioinformarics.oss.jackson.module.jsonld.annotation.JsonldType; -import lombok.Value; - -import io.fairspace.saturn.vocabulary.FS; - -@Value -@JsonldType(FS.ERROR_URI) -public class ErrorDto { - @JsonldProperty(FS.ERROR_STATUS_URI) - private int status; - - @JsonldProperty(FS.ERROR_MESSAGE_URI) - private String message; - - @JsonldProperty(FS.ERROR_DETAILS_URI) - private Object details; -} diff --git a/projects/saturn/src/test/java/io/fairspace/saturn/controller/BaseControllerTest.java b/projects/saturn/src/test/java/io/fairspace/saturn/controller/BaseControllerTest.java index 4c843499f..afa3c256f 100644 --- a/projects/saturn/src/test/java/io/fairspace/saturn/controller/BaseControllerTest.java +++ b/projects/saturn/src/test/java/io/fairspace/saturn/controller/BaseControllerTest.java @@ -15,7 +15,7 @@ @ImportAutoConfiguration(exclude = {SecurityAutoConfiguration.class, OAuth2ResourceServerAutoConfiguration.class}) @Import(BaseControllerTest.CustomObjectMapperConfig.class) -class BaseControllerTest { +public class BaseControllerTest { @MockBean private JwtAuthConverterProperties jwtAuthConverterProperties; diff --git a/projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandlerTest.java b/projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandlerTest.java new file mode 100644 index 000000000..7b8c8b231 --- /dev/null +++ b/projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/GlobalExceptionHandlerTest.java @@ -0,0 +1,135 @@ +package io.fairspace.saturn.controller.exception; + +import java.util.Set; + +import jakarta.validation.ConstraintViolation; +import jakarta.validation.ConstraintViolationException; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.MediaType; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.test.web.servlet.MockMvc; + +import io.fairspace.saturn.controller.BaseControllerTest; +import io.fairspace.saturn.services.metadata.validation.ValidationException; +import io.fairspace.saturn.services.metadata.validation.Violation; + +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@WebMvcTest({GlobalExceptionHandler.class, TestController.class}) +public class GlobalExceptionHandlerTest extends BaseControllerTest { + + @Autowired + private MockMvc mockMvc; + + @MockBean + private TestController.TestInnerClass testInnerClass; + + @Test + public void testHandleConstraintViolationException() throws Exception { + // Mocking a ConstraintViolationException with a couple of violations + ConstraintViolation violation1 = Mockito.mock(ConstraintViolation.class); + ConstraintViolation violation2 = Mockito.mock(ConstraintViolation.class); + when(violation1.getMessage()).thenReturn("Violation 1"); + when(violation2.getMessage()).thenReturn("Violation 2"); + Set> violations = Set.of(violation1, violation2); + ConstraintViolationException exception = new ConstraintViolationException(violations); + + doThrow(exception).when(testInnerClass).method(); // Simulating the exception + + mockMvc.perform(get("/test")) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect( + content() + .json( + """ + { + "status": 400, + "message": "Validation Error", + "details": "Violations: Violation 1; Violation 2" + } + """)); + } + + @Test + public void testHandleValidationException() throws Exception { + // Mocking a ValidationException with a violation + Set violations = Set.of(new Violation("Invalid value", "subject", "predicate", "value")); + ValidationException exception = new ValidationException(violations); + + doThrow(exception).when(testInnerClass).method(); // Simulating the exception + + mockMvc.perform(get("/test")) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect( + content() + .json( + """ + { + "status": 400, + "message": "Validation Error", + "details": [ + { + "message": "Invalid value", + "subject": "subject", + "predicate": "predicate", + "value": "value" + } + ] + } + """)); + } + + @Test + public void testHandleIllegalArgumentException() throws Exception { + // Mocking an IllegalArgumentException + IllegalArgumentException exception = new IllegalArgumentException("Invalid argument"); + + doThrow(exception).when(testInnerClass).method(); // Simulating the exception + + mockMvc.perform(get("/test")) + .andExpect(status().isBadRequest()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect( + content() + .json( + """ + { + "status": 400, + "message": "Validation Error", + "details": "Invalid argument" + } + """)); + } + + @Test + public void testHandleAccessDeniedException() throws Exception { + // Mocking an AccessDeniedException + AccessDeniedException exception = new AccessDeniedException("Access denied"); + + doThrow(exception).when(testInnerClass).method(); // Simulating the exception + + mockMvc.perform(get("/test")) + .andExpect(status().isForbidden()) + .andExpect(content().contentType(MediaType.APPLICATION_JSON)) + .andExpect( + content() + .json( + """ + { + "status": 403, + "message": "Access Denied", + "details": null + } + """)); + } +} diff --git a/projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/TestController.java b/projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/TestController.java new file mode 100644 index 000000000..97151333f --- /dev/null +++ b/projects/saturn/src/test/java/io/fairspace/saturn/controller/exception/TestController.java @@ -0,0 +1,23 @@ +package io.fairspace.saturn.controller.exception; + +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Component; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RestController; + +@RestController +@RequiredArgsConstructor +public class TestController { + + private final TestInnerClass testInnerClass; + + @GetMapping("/test") + public void testMethod() { + testInnerClass.method(); + } + + @Component + public static class TestInnerClass { + public void method() {} + } +}