From be68e40a8d598e68bc249535fac11e0b15a29e95 Mon Sep 17 00:00:00 2001 From: Andreas Pfeil Date: Tue, 28 Jan 2025 18:20:43 +0100 Subject: [PATCH 1/4] WIP --- .../edu/kit/datamanager/pit/Application.java | 6 ++ .../pit/common/RecordValidationException.java | 9 ++- .../pit/web/ExtendedErrorAttributes.java | 11 ++-- .../pit/web/ExtendedErrorAttributesTest.java | 64 +++++++++++++++++++ 4 files changed, 81 insertions(+), 9 deletions(-) create mode 100644 src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java diff --git a/src/main/java/edu/kit/datamanager/pit/Application.java b/src/main/java/edu/kit/datamanager/pit/Application.java index 5857e71a..fabf3c21 100644 --- a/src/main/java/edu/kit/datamanager/pit/Application.java +++ b/src/main/java/edu/kit/datamanager/pit/Application.java @@ -37,6 +37,7 @@ import edu.kit.datamanager.pit.pitservice.impl.TypingService; import edu.kit.datamanager.pit.typeregistry.ITypeRegistry; import edu.kit.datamanager.pit.typeregistry.impl.TypeRegistry; +import edu.kit.datamanager.pit.web.ExtendedErrorAttributes; import edu.kit.datamanager.pit.web.converter.SimplePidRecordConverter; import edu.kit.datamanager.security.filter.KeycloakJwtProperties; @@ -100,6 +101,11 @@ public Logger logger(InjectionPoint injectionPoint) { return LoggerFactory.getLogger(targetClass.getCanonicalName()); } + @Bean + public ExtendedErrorAttributes errorAttributes(ObjectMapper objectMapper) { + return new ExtendedErrorAttributes(objectMapper); + } + @Bean public ITypeRegistry typeRegistry() { return new TypeRegistry(); diff --git a/src/main/java/edu/kit/datamanager/pit/common/RecordValidationException.java b/src/main/java/edu/kit/datamanager/pit/common/RecordValidationException.java index 520cc748..6160b4f0 100644 --- a/src/main/java/edu/kit/datamanager/pit/common/RecordValidationException.java +++ b/src/main/java/edu/kit/datamanager/pit/common/RecordValidationException.java @@ -5,6 +5,8 @@ import edu.kit.datamanager.pit.domain.PIDRecord; +import java.io.Serial; + /** * Indicates that a PID was given which could not be resolved to answer the * request properly. @@ -12,11 +14,12 @@ public class RecordValidationException extends ResponseStatusException { private static final String VALIDATION_OF_RECORD = "Validation of record "; - private static final long serialVersionUID = 1L; + @Serial + private static final long serialVersionUID = -7287999233733933282L; private static final HttpStatus HTTP_STATUS = HttpStatus.BAD_REQUEST; - // For cases in which the PID record shold be appended to the error response. - private final transient PIDRecord pidRecord; + // For cases in which the PID record should be appended to the error response. + private final PIDRecord pidRecord; public RecordValidationException(PIDRecord pidRecord) { super(HTTP_STATUS, VALIDATION_OF_RECORD + pidRecord.getPid() + " failed."); diff --git a/src/main/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributes.java b/src/main/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributes.java index 17fa70b6..4c8245f7 100644 --- a/src/main/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributes.java +++ b/src/main/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributes.java @@ -2,30 +2,29 @@ import java.util.Map; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.web.error.ErrorAttributeOptions; import org.springframework.boot.web.servlet.error.DefaultErrorAttributes; -import org.springframework.stereotype.Component; import org.springframework.web.context.request.WebRequest; import com.fasterxml.jackson.databind.ObjectMapper; import edu.kit.datamanager.pit.common.RecordValidationException; -@Component public class ExtendedErrorAttributes extends DefaultErrorAttributes { - @Autowired(required = true) ObjectMapper objectMapperBean; + public ExtendedErrorAttributes(ObjectMapper objectMapperBean) { + this.objectMapperBean = objectMapperBean; + } + @Override public Map getErrorAttributes(WebRequest webRequest, ErrorAttributeOptions options) { final Map errorAttributes = super.getErrorAttributes(webRequest, options); final Throwable error = super.getError(webRequest); - if (error instanceof RecordValidationException) { - final RecordValidationException validationError = (RecordValidationException) error; + if (error instanceof RecordValidationException validationError) { try { errorAttributes.put("pid-record", objectMapperBean.writeValueAsString(validationError.getPidRecord())); } catch (Exception e) { diff --git a/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java b/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java new file mode 100644 index 00000000..66d16005 --- /dev/null +++ b/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java @@ -0,0 +1,64 @@ +package edu.kit.datamanager.pit.web; + +import java.util.Map; + +import edu.kit.datamanager.pit.common.RecordValidationException; +import edu.kit.datamanager.pit.domain.PIDRecord; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.boot.web.error.ErrorAttributeOptions; +import org.springframework.boot.web.servlet.error.ErrorAttributes; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.web.context.WebApplicationContext; +import org.springframework.web.context.request.WebRequest; +import org.springframework.web.context.request.ServletWebRequest; + +import static org.junit.jupiter.api.Assertions.*; + +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@AutoConfigureMockMvc +@ActiveProfiles("test") +public class ExtendedErrorAttributesTest { + + @Autowired + private TestRestTemplate restTemplate; + + @Autowired + private WebApplicationContext webApplicationContext; + + @Autowired + private ExtendedErrorAttributes errorAttributes; + + @Test + public void testExtendedErrorAttributes() { + // Create a mock request to pass to the error attributes + HttpServletRequest request = new MockHttpServletRequest(); + WebRequest webRequest = new ServletWebRequest(request); + + // Simulate an exception to be handled by the error attributes + request.setAttribute("jakarta.servlet.error.exception", new RecordValidationException( + new PIDRecord().withPID("asdfg"), + "Validation failed" + )); + + // Get the error attributes + Map attributes = errorAttributes.getErrorAttributes(webRequest, ErrorAttributeOptions.defaults()); + + // Check if the custom attribute is present + assertTrue(attributes.containsKey("pid-record")); + } + + @Test + public void testExtendedErrorAttributesBeanRegistration() { + // Check if the ExtendedErrorAttributes bean is registered + ExtendedErrorAttributes extendedErrorAttributes = webApplicationContext.getBean(ExtendedErrorAttributes.class); + assertNotNull(extendedErrorAttributes, "ExtendedErrorAttributes bean should be registered"); + } +} \ No newline at end of file From b00490d83659a731592cb9dca6a4bbd015e21820 Mon Sep 17 00:00:00 2001 From: Andreas Pfeil Date: Tue, 28 Jan 2025 18:41:57 +0100 Subject: [PATCH 2/4] WIP --- build.gradle | 1 - .../pit/web/ExtendedErrorAttributesTest.java | 7 ++--- .../pit/web/ITypingRestResourceTest.java | 28 +++++++++++++++++-- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/build.gradle b/build.gradle index f8bb4885..56cf4816 100644 --- a/build.gradle +++ b/build.gradle @@ -191,7 +191,6 @@ jacocoTestReport { fileTree(dir: it, exclude: [\ 'edu/kit/datamanager/pit/configuration/**', \ 'edu/kit/datamanager/pit/web/converter/**', \ - 'edu/kit/datamanager/pit/web/ExtendedErrorAttributes**', \ 'edu/kit/datamanager/pit/web/UncontrolledExceptionHandler**', \ 'edu/kit/datamanager/pit/common/**', \ 'edu/kit/datamanager/pit/Application*' diff --git a/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java b/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java index 66d16005..93d27688 100644 --- a/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java +++ b/src/test/java/edu/kit/datamanager/pit/web/ExtendedErrorAttributesTest.java @@ -11,9 +11,6 @@ import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.web.client.TestRestTemplate; import org.springframework.boot.web.error.ErrorAttributeOptions; -import org.springframework.boot.web.servlet.error.ErrorAttributes; -import org.springframework.http.HttpStatus; -import org.springframework.http.ResponseEntity; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.test.context.ActiveProfiles; import org.springframework.web.context.WebApplicationContext; @@ -37,7 +34,7 @@ public class ExtendedErrorAttributesTest { private ExtendedErrorAttributes errorAttributes; @Test - public void testExtendedErrorAttributes() { + public void testPidRecordPresence() { // Create a mock request to pass to the error attributes HttpServletRequest request = new MockHttpServletRequest(); WebRequest webRequest = new ServletWebRequest(request); @@ -56,7 +53,7 @@ public void testExtendedErrorAttributes() { } @Test - public void testExtendedErrorAttributesBeanRegistration() { + public void testBeanRegistration() { // Check if the ExtendedErrorAttributes bean is registered ExtendedErrorAttributes extendedErrorAttributes = webApplicationContext.getBean(ExtendedErrorAttributes.class); assertNotNull(extendedErrorAttributes, "ExtendedErrorAttributes bean should be registered"); diff --git a/src/test/java/edu/kit/datamanager/pit/web/ITypingRestResourceTest.java b/src/test/java/edu/kit/datamanager/pit/web/ITypingRestResourceTest.java index 4a61c79f..f877069c 100644 --- a/src/test/java/edu/kit/datamanager/pit/web/ITypingRestResourceTest.java +++ b/src/test/java/edu/kit/datamanager/pit/web/ITypingRestResourceTest.java @@ -1,15 +1,18 @@ package edu.kit.datamanager.pit.web; -import static org.junit.jupiter.api.Assertions.assertTrue; - +import com.fasterxml.jackson.databind.ObjectMapper; +import edu.kit.datamanager.pit.domain.PIDRecord; +import org.hamcrest.Matchers; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.http.MediaType; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.TestPropertySource; import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.MvcResult; import org.springframework.test.web.servlet.result.MockMvcResultHandlers; import org.springframework.test.web.servlet.result.MockMvcResultMatchers; import org.springframework.test.web.servlet.setup.MockMvcBuilders; @@ -18,7 +21,9 @@ import edu.kit.datamanager.pit.SpringTestHelper; import edu.kit.datamanager.pit.pitservice.impl.NoValidationStrategy; +import static org.junit.jupiter.api.Assertions.*; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; // Might be needed for WebApp testing according to https://www.baeldung.com/integration-testing-in-spring //@WebAppConfiguration @@ -46,6 +51,25 @@ void setup() throws Exception { .assertNoBeanInstanceOf(NoValidationStrategy.class); } + @Test + void testPidRecordInErrorJson() throws Exception { + PIDRecord r = new PIDRecord(); + r.addEntry("21.T11148/076759916209e5d62bd5", "for Testing", "21.T11148/301c6f04763a16f0f72a"); + MvcResult result = this.mockMvc + .perform( + post("/api/v1/pit/pid/") + .contentType(MediaType.APPLICATION_JSON) + .characterEncoding("utf-8") + .content(new ObjectMapper().writeValueAsString(r)) + .accept(MediaType.ALL) + ) + .andDo(MockMvcResultHandlers.print()) + .andExpect(MockMvcResultMatchers.status().isBadRequest()) + .andExpect(MockMvcResultMatchers.jsonPath("$.pid-record", Matchers.containsString("for Testing"))) + .andReturn(); + assertFalse(result.getResponse().getContentAsString().isEmpty()); + } + /** * Tests if the swagger ui and openapi definition is accessible. * From cdd6a98f62b0325f747a63cfe4e5d7329430f65a Mon Sep 17 00:00:00 2001 From: Andreas Pfeil Date: Tue, 25 Mar 2025 19:40:33 +0100 Subject: [PATCH 3/4] tests: add hurl test for error messages --- docker/test_docker.sh | 2 +- docker/tests/create_fail_error_message.hurl | 23 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 docker/tests/create_fail_error_message.hurl diff --git a/docker/test_docker.sh b/docker/test_docker.sh index 10f8f312..d20c9e69 100644 --- a/docker/test_docker.sh +++ b/docker/test_docker.sh @@ -26,7 +26,7 @@ docker run -p 8090:8090 --detach --name $container $tag ##################################### ### tests ########################### ##################################### -hurl --retry=10 --retry-interval=10000 \ +hurl --retry 100 --retry-interval 1s \ --test "$docker_dir"/tests/*.hurl failure=$? ##################################### diff --git a/docker/tests/create_fail_error_message.hurl b/docker/tests/create_fail_error_message.hurl new file mode 100644 index 00000000..f1a647ed --- /dev/null +++ b/docker/tests/create_fail_error_message.hurl @@ -0,0 +1,23 @@ +# create/register a pid record with missing required fields +POST http://localhost:8090/api/v1/pit/pid/ +Content-Type: application/vnd.datamanager.pid.simple+json +Accept: application/vnd.datamanager.pid.simple+json +{ + "record": [ + { "key": "21.T11148/076759916209e5d62bd5", "value": "21.T11148/301c6f04763a16f0f72a" } + ] +} + +# this will fail because the profile validation will fail +HTTP 400 +# in the response body, you'll get the record and the pid +[Asserts] +jsonpath "$.detail" exists +jsonpath "$.detail" contains "Missing mandatory types" +jsonpath "$.pid-record" exists + +# Lets ask if the record exists now: +HEAD http://localhost:8090/api/v1/pit/pid/{{pid}} + +# it should not: +HTTP 404 From 4153a999b1d66902259af5735ab95bfe71178a01 Mon Sep 17 00:00:00 2001 From: Andreas Pfeil Date: Tue, 25 Mar 2025 19:56:20 +0100 Subject: [PATCH 4/4] chore: fix linter complaints --- .../converter/SimplePidRecordConverter.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java b/src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java index 8132f361..02d70f51 100644 --- a/src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java +++ b/src/main/java/edu/kit/datamanager/pit/web/converter/SimplePidRecordConverter.java @@ -4,10 +4,10 @@ import java.io.IOException; import java.io.InputStreamReader; import java.nio.charset.StandardCharsets; -import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; +import jakarta.annotation.Nonnull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.http.HttpInputMessage; @@ -24,21 +24,21 @@ /** * Converts to-and-from PIDRecord format when SimplePidRecord format is actually * expected. - * + *

* Do not use explicitly. Spring will use it, as explained below. - * + *

* The idea is that all handlers in the REST API simply expect a serialized * (marshalled) version of a PID record. This is not always true, though. The * PIDRecord representation is pretty complex. To offer a simpler format, * `SimplePidRecord` was introduced. To avoid larger modifications within the * code, and to not break the API, the simple format comes into play only when * its content type is being used. - * + *

* If the client wants to send in the simple format, it needs to set the * content-type header accordingly. Spring will then use this converter to * convert it directly into a PIDRecord instance, as the handler expects. This * way only one handler must be used for multiple formats. - * + *

* For accepting formats, it is the same. With the accept header, a client may * control which format it would like to receive. If it prefers to receive the * simple format and sets the header accordingly, instead of directly @@ -54,8 +54,8 @@ private boolean isValidMediaType(MediaType arg1) { } @Override - public boolean canRead(Class arg0, MediaType arg1) { - if (arg0 == null || arg1 == null) { + public boolean canRead(@Nonnull Class arg0, MediaType arg1) { + if (arg1 == null) { return false; } LOGGER.trace("canRead: Checking applicability for class {} and mediatype {}.", arg0, arg1); @@ -63,20 +63,20 @@ public boolean canRead(Class arg0, MediaType arg1) { } @Override - public boolean canWrite(Class arg0, MediaType arg1) { + public boolean canWrite(@Nonnull Class arg0, MediaType arg1) { LOGGER.trace("canWrite: Checking applicability for class {} and mediatype {}.", arg0, arg1); return PIDRecord.class.equals(arg0) && isValidMediaType(arg1); } @Override - public List getSupportedMediaTypes() { - return Arrays.asList( + public @Nonnull List getSupportedMediaTypes() { + return List.of( MediaType.valueOf(SimplePidRecord.CONTENT_TYPE) ); } @Override - public PIDRecord read(Class arg0, HttpInputMessage arg1) + public @Nonnull PIDRecord read(@Nonnull Class arg0, @Nonnull HttpInputMessage arg1) throws IOException, HttpMessageNotReadableException { LOGGER.trace("Read simple message from client and convert to PIDRecord."); try (InputStreamReader reader = new InputStreamReader(arg1.getBody(), StandardCharsets.UTF_8)) { @@ -86,7 +86,7 @@ public PIDRecord read(Class arg0, HttpInputMessage arg1) } @Override - public void write(PIDRecord arg0, MediaType arg1, HttpOutputMessage arg2) + public void write(@Nonnull PIDRecord arg0, MediaType arg1, HttpOutputMessage arg2) throws IOException, HttpMessageNotWritableException { LOGGER.trace("Write PIDRecord to simple format for client."); SimplePidRecord sim = new SimplePidRecord(arg0);