From 67e850f9b2d692d24569bb2f92ed0b01c5540fd4 Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Sat, 15 Oct 2022 11:29:56 +0200 Subject: [PATCH] add API validation --- build.gradle | 9 ++-- .../RestResponseEntityExceptionHandler.java | 44 +++++++++++++---- .../hsadminng/errors/package-info.java | 6 +++ .../debitor/HsOfficeDebitorController.java | 1 - .../hs-office/hs-office-debitor-schemas.yaml | 8 ++-- .../hs-office/hs-office-partner-schemas.yaml | 10 ++-- ...esponseEntityExceptionHandlerUnitTest.java | 48 +++++++++++++++++++ ...OfficeDebitorControllerAcceptanceTest.java | 21 ++++---- 8 files changed, 114 insertions(+), 33 deletions(-) create mode 100644 src/main/java/net/hostsharing/hsadminng/errors/package-info.java diff --git a/build.gradle b/build.gradle index ea4367b7..eebc55c5 100644 --- a/build.gradle +++ b/build.gradle @@ -55,6 +55,7 @@ dependencies { implementation 'org.springframework.boot:spring-boot-starter-data-rest' implementation 'org.springframework.boot:spring-boot-starter-jdbc' implementation 'org.springframework.boot:spring-boot-starter-web' + implementation 'org.springframework.boot:spring-boot-starter-validation' implementation 'com.github.gavlyukovskiy:datasource-proxy-spring-boot-starter:1.8.1' implementation 'org.springdoc:springdoc-openapi-ui:1.6.11' implementation 'org.liquibase:liquibase-core' @@ -106,7 +107,7 @@ tasks.named('test') { openapiProcessor { springRoot { processorName 'spring' - processor 'io.openapiprocessor:openapi-processor-spring:2022.4' + processor 'io.openapiprocessor:openapi-processor-spring:2022.5' apiPath "$projectDir/src/main/resources/api-definition.yaml" mapping "$projectDir/src/main/resources/api-mappings.yaml" targetDir "$projectDir/build/generated/sources/openapi" @@ -115,7 +116,7 @@ openapiProcessor { } springRbac { processorName 'spring' - processor 'io.openapiprocessor:openapi-processor-spring:2022.4' + processor 'io.openapiprocessor:openapi-processor-spring:2022.5' apiPath "$projectDir/src/main/resources/api-definition/rbac/rbac.yaml" mapping "$projectDir/src/main/resources/api-definition/rbac/api-mappings.yaml" targetDir "$projectDir/build/generated/sources/openapi" @@ -124,7 +125,7 @@ openapiProcessor { } springTest { processorName 'spring' - processor 'io.openapiprocessor:openapi-processor-spring:2022.4' + processor 'io.openapiprocessor:openapi-processor-spring:2022.5' apiPath "$projectDir/src/main/resources/api-definition/test/test.yaml" mapping "$projectDir/src/main/resources/api-definition/test/api-mappings.yaml" targetDir "$projectDir/build/generated/sources/openapi" @@ -133,7 +134,7 @@ openapiProcessor { } springHs { processorName 'spring' - processor 'io.openapiprocessor:openapi-processor-spring:2022.4' + processor 'io.openapiprocessor:openapi-processor-spring:2022.5' apiPath "$projectDir/src/main/resources/api-definition/hs-office/hs-office.yaml" mapping "$projectDir/src/main/resources/api-definition/hs-office/api-mappings.yaml" targetDir "$projectDir/build/generated/sources/openapi" diff --git a/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java b/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java index 3eb428ae..89e9f9f5 100644 --- a/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java +++ b/src/main/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandler.java @@ -5,10 +5,12 @@ 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.orm.jpa.JpaObjectRetrievalFailureException; import org.springframework.orm.jpa.JpaSystemException; +import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.context.request.WebRequest; @@ -65,21 +67,38 @@ public class RestResponseEntityExceptionHandler @ExceptionHandler(Throwable.class) protected ResponseEntity handleOtherExceptions( final Throwable exc, final WebRequest request) { - final var message = firstLine(NestedExceptionUtils.getMostSpecificCause(exc).getMessage()); + final var message = firstMessageLine(NestedExceptionUtils.getMostSpecificCause(exc)); return errorResponse(request, httpStatus(message).orElse(HttpStatus.INTERNAL_SERVER_ERROR), message); } + @Override + @SuppressWarnings("unchecked,rawtypes") + protected ResponseEntity handleMethodArgumentNotValid( + MethodArgumentNotValidException exc, + HttpHeaders headers, + HttpStatus status, + WebRequest request) { + final var errorList = exc + .getBindingResult() + .getFieldErrors() + .stream() + .map(fieldError -> fieldError.getField() + " " + fieldError.getDefaultMessage() + " but is \"" + + fieldError.getRejectedValue() + "\"") + .toList(); + return errorResponse(request, HttpStatus.BAD_REQUEST, errorList.toString()); + } + private String userReadableEntityClassName(final String exceptionMessage) { final var regex = "(net.hostsharing.hsadminng.[a-z0-9_.]*.[A-Za-z0-9_$]*Entity) "; final var pattern = Pattern.compile(regex); final var matcher = pattern.matcher(exceptionMessage); if (matcher.find()) { final var entityName = matcher.group(1); - final var entityClass = resolveClassOrNull(entityName); - if (entityClass != null) { - return (entityClass.isAnnotationPresent(DisplayName.class) - ? exceptionMessage.replace(entityName, entityClass.getAnnotation(DisplayName.class).value()) - : exceptionMessage.replace(entityName, entityClass.getSimpleName())) + final var entityClass = resolveClass(entityName); + if (entityClass.isPresent()) { + return (entityClass.get().isAnnotationPresent(DisplayName.class) + ? exceptionMessage.replace(entityName, entityClass.get().getAnnotation(DisplayName.class).value()) + : exceptionMessage.replace(entityName, entityClass.get().getSimpleName())) .replace(" with id ", " with uuid "); } @@ -87,11 +106,11 @@ public class RestResponseEntityExceptionHandler return exceptionMessage; } - private static Class resolveClassOrNull(final String entityName) { + private static Optional> resolveClass(final String entityName) { try { - return ClassLoader.getSystemClassLoader().loadClass(entityName); + return Optional.of(ClassLoader.getSystemClassLoader().loadClass(entityName)); } catch (ClassNotFoundException e) { - return null; + return Optional.empty(); } } @@ -115,6 +134,13 @@ public class RestResponseEntityExceptionHandler 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]; } diff --git a/src/main/java/net/hostsharing/hsadminng/errors/package-info.java b/src/main/java/net/hostsharing/hsadminng/errors/package-info.java new file mode 100644 index 00000000..a7813fd8 --- /dev/null +++ b/src/main/java/net/hostsharing/hsadminng/errors/package-info.java @@ -0,0 +1,6 @@ +@NonNullApi +@NonNullFields +package net.hostsharing.hsadminng.errors; + +import org.springframework.lang.NonNullApi; +import org.springframework.lang.NonNullFields; diff --git a/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java b/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java index 44b70761..50af3fab 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java @@ -19,7 +19,6 @@ import org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBui import javax.persistence.EntityManager; import java.util.List; -import java.util.NoSuchElementException; import java.util.UUID; import java.util.function.BiConsumer; diff --git a/src/main/resources/api-definition/hs-office/hs-office-debitor-schemas.yaml b/src/main/resources/api-definition/hs-office/hs-office-debitor-schemas.yaml index 56b1a941..583ae8fa 100644 --- a/src/main/resources/api-definition/hs-office/hs-office-debitor-schemas.yaml +++ b/src/main/resources/api-definition/hs-office/hs-office-debitor-schemas.yaml @@ -22,7 +22,7 @@ components: type: string vatCountryCode: type: string - pattern: '^[A_Z][A-Z]$' + pattern: '^[A-Z][A-Z]$' vatBusiness: type: boolean refundBankAccount: @@ -40,7 +40,7 @@ components: nullable: true vatCountryCode: type: string - pattern: '^[A_Z][A-Z]$' + pattern: '^[A-Z][A-Z]$' nullable: true vatBusiness: type: boolean @@ -56,9 +56,11 @@ components: partnerUuid: type: string format: uuid + nullable: false billingContactUuid: type: string format: uuid + nullable: false debitorNumber: type: integer format: int32 @@ -68,7 +70,7 @@ components: type: string vatCountryCode: type: string - pattern: '^[A_Z][A-Z]$' + pattern: '^[A-Z][A-Z]$' vatBusiness: type: boolean refundBankAccountUuid: diff --git a/src/main/resources/api-definition/hs-office/hs-office-partner-schemas.yaml b/src/main/resources/api-definition/hs-office/hs-office-partner-schemas.yaml index cd79d96f..bb68333d 100644 --- a/src/main/resources/api-definition/hs-office/hs-office-partner-schemas.yaml +++ b/src/main/resources/api-definition/hs-office/hs-office-partner-schemas.yaml @@ -15,16 +15,21 @@ components: $ref: './hs-office-contact-schemas.yaml#/components/schemas/HsOfficeContact' registrationOffice: type: string + nullable: true registrationNumber: type: string + nullable: true birthName: type: string + nullable: true birthday: type: string format: date + nullable: true dateOfDeath: type: string format: date + nullable: true HsOfficePartnerPatch: type: object @@ -84,8 +89,3 @@ components: required: - personUuid - contactUuid - - registrationOffice - - registrationNumber - - birthName - - birthday - - dateOfDeath diff --git a/src/test/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandlerUnitTest.java b/src/test/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandlerUnitTest.java index f2a1d66d..27cbfec5 100644 --- a/src/test/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandlerUnitTest.java +++ b/src/test/java/net/hostsharing/hsadminng/errors/RestResponseEntityExceptionHandlerUnitTest.java @@ -5,16 +5,24 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.core.MethodParameter; import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; import org.springframework.orm.jpa.JpaObjectRetrievalFailureException; import org.springframework.orm.jpa.JpaSystemException; +import org.springframework.validation.BindingResult; +import org.springframework.validation.FieldError; +import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.context.request.WebRequest; import javax.persistence.EntityNotFoundException; +import java.util.List; import java.util.NoSuchElementException; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) class RestResponseEntityExceptionHandlerUnitTest { @@ -167,6 +175,32 @@ class RestResponseEntityExceptionHandlerUnitTest { assertThat(errorResponse.getBody().getMessage()).isEqualTo("given error message"); } + @Test + void handleMethodArgumentNotValidException() { + // given + final var givenBindingResult = mock(BindingResult.class); + when(givenBindingResult.getFieldErrors()).thenReturn(List.of( + new FieldError("someObject", "someField", "someRejectedValue", false, null, null, "expected to be something") + )); + final var givenException = new MethodArgumentNotValidException( + mock(MethodParameter.class), + givenBindingResult + + ); + final var givenWebRequest = mock(WebRequest.class); + + // when + final var errorResponse = exceptionHandler.handleMethodArgumentNotValid(givenException, + HttpHeaders.EMPTY, HttpStatus.BAD_REQUEST, givenWebRequest); + + // then + assertThat(errorResponse.getStatusCodeValue()).isEqualTo(400); + assertThat(errorResponse.getBody()) + .isInstanceOf(CustomErrorResponse.class) + .extracting("message") + .isEqualTo("[someField expected to be something but is \"someRejectedValue\"]"); + } + @Test void handleOtherExceptionsWithoutErrorCode() { // given @@ -195,6 +229,20 @@ class RestResponseEntityExceptionHandlerUnitTest { assertThat(errorResponse.getBody().getMessage()).isEqualTo("ERROR: [418] First Line"); } + @Test + void handleOtherExceptionsWithoutMessage() { + // given + final var givenThrowable = new Error(); + final var givenWebRequest = mock(WebRequest.class); + + // when + final var errorResponse = exceptionHandler.handleOtherExceptions(givenThrowable, givenWebRequest); + + // then + assertThat(errorResponse.getStatusCodeValue()).isEqualTo(500); + assertThat(errorResponse.getBody().getMessage()).isEqualTo("ERROR: [500] java.lang.Error"); + } + public static class NoDisplayNameEntity { } diff --git a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java index e6dde210..0a149881 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java @@ -26,8 +26,7 @@ import static net.hostsharing.test.IsValidUuidMatcher.isUuidValid; import static net.hostsharing.test.JsonMatcher.lenientlyEquals; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assumptions.assumeThat; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.startsWith; +import static org.hamcrest.Matchers.*; @SpringBootTest( webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, @@ -190,36 +189,36 @@ class HsOfficeDebitorControllerAcceptanceTest { } @Test - void globalAdmin_withoutAssumedRole_canAddDebitorWithoutBankAccount() { + void globalAdmin_canAddDebitorWithoutJustRequiredData() { context.define("superuser-alex@hostsharing.net"); final var givenPartner = partnerRepo.findPartnerByOptionalNameLike("Third").get(0); final var givenContact = contactRepo.findContactByOptionalLabelLike("forth").get(0); final var location = RestAssured // @formatter:off - .given() + .given() .header("current-user", "superuser-alex@hostsharing.net") .contentType(ContentType.JSON) .body(""" { "partnerUuid": "%s", "billingContactUuid": "%s", - "debitorNumber": "%s", - "vatId": "VAT123456", - "vatCountryCode": "DE", - "vatBusiness": true + "debitorNumber": "%s" } """.formatted( givenPartner.getUuid(), givenContact.getUuid(), nextDebitorNumber++)) .port(port) - .when() + .when() .post("http://localhost/api/hs/office/debitors") - .then().log().all().assertThat() + .then().log().all().assertThat() .statusCode(201) .contentType(ContentType.JSON) .body("uuid", isUuidValid()) - .body("vatId", is("VAT123456")) .body("billingContact.label", is(givenContact.getLabel())) .body("partner.person.tradeName", is(givenPartner.getPerson().getTradeName())) + .body("vatId", equalTo(null)) + .body("vatCountryCode", equalTo(null)) + .body("vatBusiness", equalTo(false)) + .body("refundBankAccount", equalTo(null)) .header("Location", startsWith("http://localhost")) .extract().header("Location"); // @formatter:on