From 77ef126a7e42d533899ef87e68656a3beee4446b Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Sun, 16 Oct 2022 10:08:42 +0200 Subject: [PATCH] improved REST-API error handling for broken request body JSON --- .../config/JsonObjectMapperConfiguration.java | 2 + .../hsadminng/errors/CustomErrorResponse.java | 51 +++++++++++++++++ .../RestResponseEntityExceptionHandler.java | 57 +++++-------------- ...ceBankAccountControllerAcceptanceTest.java | 7 ++- .../TestCustomerControllerAcceptanceTest.java | 24 +++++++- 5 files changed, 94 insertions(+), 47 deletions(-) create mode 100644 src/main/java/net/hostsharing/hsadminng/errors/CustomErrorResponse.java diff --git a/src/main/java/net/hostsharing/hsadminng/config/JsonObjectMapperConfiguration.java b/src/main/java/net/hostsharing/hsadminng/config/JsonObjectMapperConfiguration.java index 8c6cb2b3..921d0c34 100644 --- a/src/main/java/net/hostsharing/hsadminng/config/JsonObjectMapperConfiguration.java +++ b/src/main/java/net/hostsharing/hsadminng/config/JsonObjectMapperConfiguration.java @@ -1,5 +1,6 @@ package net.hostsharing.hsadminng.config; +import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.SerializationFeature; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; import org.openapitools.jackson.nullable.JsonNullableModule; @@ -16,6 +17,7 @@ public class JsonObjectMapperConfiguration { public Jackson2ObjectMapperBuilder customObjectMapper() { return new Jackson2ObjectMapperBuilder() .modules(new JsonNullableModule(), new JavaTimeModule()) + .featuresToEnable(JsonParser.Feature.ALLOW_COMMENTS) .featuresToDisable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS); } } diff --git a/src/main/java/net/hostsharing/hsadminng/errors/CustomErrorResponse.java b/src/main/java/net/hostsharing/hsadminng/errors/CustomErrorResponse.java new file mode 100644 index 00000000..be8bbe65 --- /dev/null +++ b/src/main/java/net/hostsharing/hsadminng/errors/CustomErrorResponse.java @@ -0,0 +1,51 @@ +package net.hostsharing.hsadminng.errors; + +import com.fasterxml.jackson.annotation.JsonFormat; +import lombok.Getter; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.web.context.request.WebRequest; + +import java.time.LocalDateTime; + +@Getter +class CustomErrorResponse { + + static ResponseEntity errorResponse( + final WebRequest request, + final HttpStatus httpStatus, + final String message) { + return new ResponseEntity<>( + new CustomErrorResponse(request.getContextPath(), httpStatus, message), httpStatus); + } + + static String firstMessageLine(final Throwable exception) { + if (exception.getMessage() != null) { + return firstLine(exception.getMessage()); + } + return "ERROR: [500] " + exception.getClass().getName(); + } + + static String firstLine(final String message) { + return message.split("\\r|\\n|\\r\\n", 0)[0]; + } + + @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd hh:mm:ss") + private final LocalDateTime timestamp; + + private final String path; + + private final int status; + + private final String error; + + private final String message; + + CustomErrorResponse(final String path, final HttpStatus status, final String message) { + this.timestamp = LocalDateTime.now(); + this.path = path; + this.status = status.value(); + this.error = status.getReasonPhrase(); + this.message = message; + } +} diff --git a/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java b/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java index 89e9f9f5..2df06e1d 100644 --- a/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java +++ b/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java @@ -1,13 +1,12 @@ package net.hostsharing.hsadminng.errors; -import com.fasterxml.jackson.annotation.JsonFormat; -import lombok.Getter; import org.iban4j.Iban4jException; import org.springframework.core.NestedExceptionUtils; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.lang.Nullable; import org.springframework.orm.jpa.JpaObjectRetrievalFailureException; import org.springframework.orm.jpa.JpaSystemException; import org.springframework.web.bind.MethodArgumentNotValidException; @@ -17,11 +16,12 @@ import org.springframework.web.context.request.WebRequest; import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; import javax.persistence.EntityNotFoundException; -import java.time.LocalDateTime; import java.util.NoSuchElementException; import java.util.Optional; import java.util.regex.Pattern; +import static net.hostsharing.hsadminng.errors.CustomErrorResponse.*; + @ControllerAdvice public class RestResponseEntityExceptionHandler extends ResponseEntityExceptionHandler { @@ -73,6 +73,16 @@ public class RestResponseEntityExceptionHandler @Override @SuppressWarnings("unchecked,rawtypes") + protected ResponseEntity handleExceptionInternal( + Exception exc, @Nullable Object body, HttpHeaders headers, HttpStatus status, WebRequest request) { + + final var response = super.handleExceptionInternal(exc, body, headers, status, request); + return errorResponse(request, response.getStatusCode(), + Optional.ofNullable(response.getBody()).map(Object::toString).orElse(firstMessageLine(exc))); + } + + //@ExceptionHandler({ MethodArgumentNotValidException.class }) + @SuppressWarnings("unchecked,rawtypes") protected ResponseEntity handleMethodArgumentNotValid( MethodArgumentNotValidException exc, HttpHeaders headers, @@ -126,45 +136,4 @@ public class RestResponseEntityExceptionHandler return Optional.empty(); } - private static ResponseEntity errorResponse( - final WebRequest request, - final HttpStatus httpStatus, - final String message) { - return new ResponseEntity<>( - new CustomErrorResponse(request.getContextPath(), httpStatus, message), httpStatus); - } - - private String firstMessageLine(final Throwable exception) { - if (exception.getMessage() != null) { - return firstLine(exception.getMessage()); - } - return "ERROR: [500] " + exception.getClass().getName(); - } - - private String firstLine(final String message) { - return message.split("\\r|\\n|\\r\\n", 0)[0]; - } -} - -@Getter -class CustomErrorResponse { - - @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd hh:mm:ss") - private final LocalDateTime timestamp; - - private final String path; - - private final int status; - - private final String error; - - private final String message; - - public CustomErrorResponse(final String path, final HttpStatus status, final String message) { - this.timestamp = LocalDateTime.now(); - this.path = path; - this.status = status.value(); - this.error = status.getReasonPhrase(); - this.message = message; - } } diff --git a/src/test/java/net/hostsharing/hsadminng/hs/office/bankaccount/HsOfficeBankAccountControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/hs/office/bankaccount/HsOfficeBankAccountControllerAcceptanceTest.java index 91024f0e..31391fd9 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/office/bankaccount/HsOfficeBankAccountControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/office/bankaccount/HsOfficeBankAccountControllerAcceptanceTest.java @@ -136,7 +136,7 @@ class HsOfficeBankAccountControllerAcceptanceTest { .port(port) .when() .post("http://localhost/api/hs/office/bankaccounts") - .then().assertThat() + .then().log().all().assertThat() .statusCode(201) .contentType(ContentType.JSON) .body("uuid", isUuidValid()) @@ -346,7 +346,10 @@ class HsOfficeBankAccountControllerAcceptanceTest { jpaAttempt.transacted(() -> { context.define("superuser-alex@hostsharing.net", null); tempBankAccountUuids.addAll( - bankAccountRepo.findByOptionalHolderLike("some temp acc").stream().map(HsOfficeBankAccountEntity::getUuid).toList() + bankAccountRepo.findByOptionalHolderLike("some temp acc") + .stream() + .map(HsOfficeBankAccountEntity::getUuid) + .toList() ); }); tempBankAccountUuids.forEach(uuid -> { diff --git a/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerControllerAcceptanceTest.java index 401046b8..a2bd0b78 100644 --- a/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/test/cust/TestCustomerControllerAcceptanceTest.java @@ -247,6 +247,26 @@ class TestCustomerControllerAcceptanceTest { context.define("superuser-fran@hostsharing.net"); assertThat(testCustomerRepository.findCustomerByOptionalPrefixLike("uuu")).hasSize(0); } + + @Test + void invalidRequestBodyJson_raisesClientError() { + + RestAssured // @formatter:off + .given() + .header("current-user", "superuser-alex@hostsharing.net") + .contentType(ContentType.JSON) + .body("{]") // deliberately invalid JSON + .port(port) + .when() + .post("http://localhost/api/test/customers") + .then().assertThat() + .statusCode(400) + .contentType(ContentType.JSON) + .body("message", containsString("JSON parse error: Unexpected close marker ']': expected '}'")) + .body("message", containsString("line: 1, column: 1")); + // @formatter:on + } + } private UUID toCleanup(final UUID tempPartnerUuid) { @@ -262,7 +282,9 @@ class TestCustomerControllerAcceptanceTest { System.out.println("DELETING temporary partner: " + uuid); final var entity = testCustomerRepository.findByUuid(uuid); final var count = testCustomerRepository.deleteByUuid(uuid); - System.out.println("DELETED temporary partner: " + uuid + (count > 0 ? " successful" : " failed") + " (" + entity.map(TestCustomerEntity::getPrefix).orElse("???") + ")"); + System.out.println( + "DELETED temporary partner: " + uuid + (count > 0 ? " successful" : " failed") + " (" + entity.map( + TestCustomerEntity::getPrefix).orElse("???") + ")"); }).assertSuccessful(); }); }