From 3d4d388e1a87bb7e5c6f08d6ea9e4437b5819d27 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 22 Nov 2023 16:24:46 +0100 Subject: [PATCH 01/13] add tests defining behaviour --- .../MultiReadHttpServletRequestWrapper.java | 81 +++++++++++++++++++ .../FailOnViolationIntegrationTest.java | 71 ++++++++++++++++ .../FailOnViolationIntegrationTest.java | 76 +++++++++++++++++ 3 files changed, 228 insertions(+) create mode 100644 spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java create mode 100644 spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java create mode 100644 spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java new file mode 100644 index 0000000..1a481bd --- /dev/null +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java @@ -0,0 +1,81 @@ +package com.getyourguide.openapi.validation.filter; + +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import org.apache.tomcat.util.http.fileupload.IOUtils; + +/** + * A class to provide the ability to read a {@link jakarta.servlet.ServletRequest}'s body multiple + * times. via https://stackoverflow.com/a/36619972/2257038 and https://stackoverflow.com/a/30748533/2257038 + */ +public class MultiReadHttpServletRequestWrapper extends HttpServletRequestWrapper { + + private ByteArrayOutputStream cachedBytes; + + /** + * Construct a new multi-read wrapper. + * + * @param request to wrap around + */ + public MultiReadHttpServletRequestWrapper(HttpServletRequest request) { + super(request); + } + + @Override + public ServletInputStream getInputStream() throws IOException { + if (cachedBytes == null) { + cacheInputStream(); + } + + return new CachedServletInputStream(cachedBytes.toByteArray()); + } + + @Override + public BufferedReader getReader() throws IOException { + return new BufferedReader(new InputStreamReader(getInputStream())); + } + + private void cacheInputStream() throws IOException { + /* Cache the inputstream in order to read it multiple times. For + * convenience, I use apache.commons IOUtils + */ + cachedBytes = new ByteArrayOutputStream(); + IOUtils.copy(super.getInputStream(), cachedBytes); + } + + /* An inputstream which reads the cached request body */ + private static class CachedServletInputStream extends ServletInputStream { + private final ByteArrayInputStream buffer; + + public CachedServletInputStream(byte[] contents) { + this.buffer = new ByteArrayInputStream(contents); + } + + @Override + public int read() throws IOException { + return buffer.read(); + } + + @Override + public boolean isFinished() { + return buffer.available() == 0; + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public void setReadListener(ReadListener listener) { + throw new UnsupportedOperationException("Not implemented"); + } + } +} diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java new file mode 100644 index 0000000..d4b4556 --- /dev/null +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -0,0 +1,71 @@ +package com.getyourguide.openapi.validation.integration; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.getyourguide.openapi.validation.test.TestViolationLogger; +import java.util.Optional; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +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.junit.jupiter.SpringExtension; +import org.springframework.test.web.servlet.MockMvc; + +@SpringBootTest(properties = { + "openapi.validation.should-fail-on-request-violation=true", + "openapi.validation.should-fail-on-response-violation=true", +}) +@AutoConfigureMockMvc +@ExtendWith(SpringExtension.class) +public class FailOnViolationIntegrationTest { + + @Autowired + private MockMvc mockMvc; + + @Autowired + private TestViolationLogger openApiViolationLogger; + + @BeforeEach + public void setup() { + openApiViolationLogger.clearViolations(); + } + + @Test + public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Exception { + mockMvc.perform(post("/test").content("{ \"value\": 1 }").contentType(MediaType.APPLICATION_JSON)) + .andExpectAll( + status().is4xxClientError(), + content().string(Matchers.blankOrNullString()) + ); + Thread.sleep(100); + + assertEquals(0, openApiViolationLogger.getViolations().size()); + + // TODO check that controller did not execute (inject controller here and have addition method to check & clear at setup) + // TODO check that something else gets logged? + } + + @Test + public void whenInvalidResponseThenReturn500AndViolationLogged() throws Exception { + mockMvc.perform(get("/test").queryParam("value", "invalid-response-value!")) + .andExpectAll( + status().is5xxServerError(), + content().string(Matchers.blankOrNullString()) + ); + Thread.sleep(100); + + assertEquals(1, openApiViolationLogger.getViolations().size()); + var violation = openApiViolationLogger.getViolations().get(0); + assertEquals("validation.response.body.schema.pattern", violation.getRule()); + assertEquals(Optional.of(200), violation.getResponseStatus()); + assertEquals(Optional.of("/value"), violation.getInstance()); + } +} diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java new file mode 100644 index 0000000..175c010 --- /dev/null +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -0,0 +1,76 @@ +package com.getyourguide.openapi.validation.integration; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.getyourguide.openapi.validation.test.TestViolationLogger; +import java.util.Optional; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +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.junit.jupiter.SpringExtension; +import org.springframework.test.web.reactive.server.WebTestClient; +import org.springframework.test.web.servlet.MockMvc; + +@SpringBootTest(properties = { + "openapi.validation.should-fail-on-request-violation=true", + "openapi.validation.should-fail-on-response-violation=true", +}) +@AutoConfigureMockMvc +@ExtendWith(SpringExtension.class) +public class FailOnViolationIntegrationTest { + + @Autowired + private WebTestClient webTestClient; + + @Autowired + private TestViolationLogger openApiViolationLogger; + + @BeforeEach + public void setup() { + openApiViolationLogger.clearViolations(); + } + + @Test + public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Exception { + webTestClient + .post().uri("/test") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue("{ \"value\": 1 }") + .exchange() + .expectStatus().is4xxClientError() + .expectBody().isEmpty(); + Thread.sleep(100); + + assertEquals(0, openApiViolationLogger.getViolations().size()); + + // TODO check that controller did not execute (inject controller here and have addition method to check & clear at setup) + // TODO check that something else gets logged? + } + + @Test + public void whenInvalidResponseThenReturn500AndViolationLogged() throws Exception { + webTestClient + .get().uri("/test?value=invalid-response-value!") + .accept(MediaType.APPLICATION_JSON) + .exchange() + .expectStatus().is5xxServerError() + .expectBody().isEmpty(); + Thread.sleep(100); + + assertEquals(1, openApiViolationLogger.getViolations().size()); + var violation = openApiViolationLogger.getViolations().get(0); + assertEquals("validation.response.body.schema.pattern", violation.getRule()); + assertEquals(Optional.of(200), violation.getResponseStatus()); + assertEquals(Optional.of("/value"), violation.getInstance()); + } +} From 1887e83cb2afbb3bb81220bbbcd8e6c19a6f7d4c Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 22 Nov 2023 16:25:26 +0100 Subject: [PATCH 02/13] fix status code on failing response violation (should be 500 instead of 400) --- .../validation/filter/OpenApiValidationWebFilter.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java index 6834c82..d78e4ce 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java @@ -88,18 +88,18 @@ private boolean validateRequestWithFailOnViolation( if (requestDecorated.getHeaders().containsKey("Content-Type") && requestDecorated.getHeaders().containsKey("Content-Length")) { requestDecorated.setOnBodyCachedListener(() -> { var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); - throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); + throwStatusExceptionOnViolation(validateRequestResult, 400, "Request validation failed"); }); } else { var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); - throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); + throwStatusExceptionOnViolation(validateRequestResult, 400, "Request validation failed"); } return true; } - private static void throwStatusExceptionOnViolation(ValidationResult validateRequestResult, String message) { + private static void throwStatusExceptionOnViolation(ValidationResult validateRequestResult, int statusCode, String message) { if (validateRequestResult == ValidationResult.INVALID) { - throw new ResponseStatusException(HttpStatus.valueOf(400), message); + throw new ResponseStatusException(HttpStatus.valueOf(statusCode), message); } } @@ -123,7 +123,7 @@ private boolean validateResponseWithFailOnViolation( responseDecorated.getCachedBody(), RunType.SYNC ); - throwStatusExceptionOnViolation(validateResponseResult, "Response validation failed"); + throwStatusExceptionOnViolation(validateResponseResult, 500, "Response validation failed"); }); return true; } From 5e77bdca7d73650e29a7e030b0a3ba1d2c57a4f4 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 22 Nov 2023 16:25:51 +0100 Subject: [PATCH 03/13] fix: request body can only be read once --- .../factory/ContentCachingWrapperFactory.java | 18 ++++++------ .../filter/OpenApiValidationInterceptor.java | 17 +++++++---- .../validation/filter/BaseFilterTest.java | 29 ++++++++++++------- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java index c4f705d..c793c94 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java @@ -1,19 +1,19 @@ package com.getyourguide.openapi.validation.factory; +import com.getyourguide.openapi.validation.filter.MultiReadHttpServletRequestWrapper; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import javax.annotation.Nullable; -import org.springframework.web.util.ContentCachingRequestWrapper; import org.springframework.web.util.ContentCachingResponseWrapper; import org.springframework.web.util.WebUtils; public class ContentCachingWrapperFactory { - public ContentCachingRequestWrapper buildContentCachingRequestWrapper(HttpServletRequest request) { - if (request instanceof ContentCachingRequestWrapper) { - return (ContentCachingRequestWrapper) request; + public MultiReadHttpServletRequestWrapper buildContentCachingRequestWrapper(HttpServletRequest request) { + if (request instanceof MultiReadHttpServletRequestWrapper) { + return (MultiReadHttpServletRequestWrapper) request; } - return new ContentCachingRequestWrapper(request); + return new MultiReadHttpServletRequestWrapper(request); } public ContentCachingResponseWrapper buildContentCachingResponseWrapper(HttpServletResponse response) { @@ -26,12 +26,12 @@ public ContentCachingResponseWrapper buildContentCachingResponseWrapper(HttpServ } @Nullable - public ContentCachingResponseWrapper getCachingResponse(final HttpServletResponse response) { - return WebUtils.getNativeResponse(response, ContentCachingResponseWrapper.class); + public MultiReadHttpServletRequestWrapper getCachingRequest(HttpServletRequest request) { + return request instanceof MultiReadHttpServletRequestWrapper ? (MultiReadHttpServletRequestWrapper) request : null; } @Nullable - public ContentCachingRequestWrapper getCachingRequest(HttpServletRequest request) { - return request instanceof ContentCachingRequestWrapper ? (ContentCachingRequestWrapper) request : null; + public ContentCachingResponseWrapper getCachingResponse(final HttpServletResponse response) { + return WebUtils.getNativeResponse(response, ContentCachingResponseWrapper.class); } } diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java index 58a5789..8e1bb3b 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java @@ -9,15 +9,16 @@ import com.getyourguide.openapi.validation.factory.ServletMetaDataFactory; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; import java.nio.charset.StandardCharsets; import javax.annotation.Nullable; import lombok.AllArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatusCode; +import org.springframework.util.StreamUtils; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.servlet.AsyncHandlerInterceptor; import org.springframework.web.servlet.ModelAndView; -import org.springframework.web.util.ContentCachingRequestWrapper; import org.springframework.web.util.ContentCachingResponseWrapper; @Slf4j @@ -126,7 +127,7 @@ private static RequestMetaData getRequestMetaData(HttpServletRequest request) { } private ValidationResult validateRequest( - ContentCachingRequestWrapper request, + MultiReadHttpServletRequestWrapper request, RequestMetaData requestMetaData, @Nullable ResponseMetaData responseMetaData, RunType runType @@ -137,9 +138,7 @@ private ValidationResult validateRequest( return ValidationResult.NOT_APPLICABLE; } - var requestBody = request.getContentType() != null - ? new String(request.getContentAsByteArray(), StandardCharsets.UTF_8) - : null; + var requestBody = request.getContentType() != null ? readBodyCatchingException(request) : null; if (runType == RunType.ASYNC) { validator.validateRequestObjectAsync(requestMetaData, responseMetaData, requestBody); @@ -149,6 +148,14 @@ private ValidationResult validateRequest( } } + private static String readBodyCatchingException(MultiReadHttpServletRequestWrapper request) { + try { + return StreamUtils.copyToString(request.getInputStream(), StandardCharsets.UTF_8); + } catch (IOException e) { + return null; + } + } + private ValidationResult validateResponse( HttpServletRequest request, ContentCachingResponseWrapper response, diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java index d293af6..8f496dc 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java @@ -16,11 +16,13 @@ import jakarta.servlet.ServletResponse; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import java.io.ByteArrayInputStream; +import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.HashMap; import lombok.Builder; import org.mockito.Mockito; -import org.springframework.web.util.ContentCachingRequestWrapper; +import org.springframework.mock.web.DelegatingServletInputStream; import org.springframework.web.util.ContentCachingResponseWrapper; public class BaseFilterTest { @@ -48,15 +50,12 @@ private static void mockRequestAttributes(ServletRequest request, HashMap Date: Wed, 22 Nov 2023 16:30:21 +0100 Subject: [PATCH 04/13] add test for ensuring request body can be read multiple times --- .../FailOnViolationIntegrationTest.java | 14 ++++++++++++++ .../FailOnViolationIntegrationTest.java | 15 +++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index d4b4556..d7f17ff 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -4,6 +4,7 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import com.getyourguide.openapi.validation.test.TestViolationLogger; @@ -38,6 +39,19 @@ public void setup() { openApiViolationLogger.clearViolations(); } + @Test + public void whenValidRequestThenReturnSuccessfully() throws Exception { + mockMvc.perform(post("/test") + .content("{ \"value\": \"test\", \"responseStatusCode\": 200 }").contentType(MediaType.APPLICATION_JSON)) + .andExpectAll( + status().isOk(), + jsonPath("$.value").value("test") + ); + Thread.sleep(100); + + assertEquals(0, openApiViolationLogger.getViolations().size()); + } + @Test public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Exception { mockMvc.perform(post("/test").content("{ \"value\": 1 }").contentType(MediaType.APPLICATION_JSON)) diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index 175c010..e7ad08c 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -39,6 +39,21 @@ public void setup() { openApiViolationLogger.clearViolations(); } + @Test + public void whenValidRequestThenReturnSuccessfully() throws Exception { + webTestClient + .post().uri("/test") + .accept(MediaType.APPLICATION_JSON) + .contentType(MediaType.APPLICATION_JSON) + .bodyValue("{ \"value\": \"test\", \"responseStatusCode\": 200 }") + .exchange() + .expectStatus().isOk() + .expectBody().jsonPath("$.value").isEqualTo("test"); + Thread.sleep(100); + + assertEquals(0, openApiViolationLogger.getViolations().size()); + } + @Test public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Exception { webTestClient From 63f1c208a923ec46f24a19e0dbaa3dd29b3a29bf Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 22 Nov 2023 16:35:05 +0100 Subject: [PATCH 05/13] remove unused imports --- .../integration/FailOnViolationIntegrationTest.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index e7ad08c..2516678 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -1,14 +1,9 @@ package com.getyourguide.openapi.validation.integration; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import com.getyourguide.openapi.validation.test.TestViolationLogger; import java.util.Optional; -import org.hamcrest.Matchers; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -18,7 +13,6 @@ import org.springframework.http.MediaType; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.test.web.reactive.server.WebTestClient; -import org.springframework.test.web.servlet.MockMvc; @SpringBootTest(properties = { "openapi.validation.should-fail-on-request-violation=true", From 2b8fa599560250bb2ef2ad2df66e4ef7f9d0485e Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 08:00:23 +0100 Subject: [PATCH 06/13] bugfix: allow rereading request body after it was validated before handler --- .../factory/ContentCachingWrapperFactory.java | 14 +++---- ...ultiReadContentCachingRequestWrapper.java} | 39 ++++++------------- .../filter/OpenApiValidationInterceptor.java | 4 +- .../validation/filter/BaseFilterTest.java | 6 +-- .../FailOnViolationIntegrationTest.java | 4 +- .../FailOnViolationIntegrationTest.java | 4 +- 6 files changed, 27 insertions(+), 44 deletions(-) rename spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/{MultiReadHttpServletRequestWrapper.java => MultiReadContentCachingRequestWrapper.java} (53%) diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java index c793c94..3c56450 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/factory/ContentCachingWrapperFactory.java @@ -1,6 +1,6 @@ package com.getyourguide.openapi.validation.factory; -import com.getyourguide.openapi.validation.filter.MultiReadHttpServletRequestWrapper; +import com.getyourguide.openapi.validation.filter.MultiReadContentCachingRequestWrapper; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import javax.annotation.Nullable; @@ -8,12 +8,12 @@ import org.springframework.web.util.WebUtils; public class ContentCachingWrapperFactory { - public MultiReadHttpServletRequestWrapper buildContentCachingRequestWrapper(HttpServletRequest request) { - if (request instanceof MultiReadHttpServletRequestWrapper) { - return (MultiReadHttpServletRequestWrapper) request; + public MultiReadContentCachingRequestWrapper buildContentCachingRequestWrapper(HttpServletRequest request) { + if (request instanceof MultiReadContentCachingRequestWrapper) { + return (MultiReadContentCachingRequestWrapper) request; } - return new MultiReadHttpServletRequestWrapper(request); + return new MultiReadContentCachingRequestWrapper(request); } public ContentCachingResponseWrapper buildContentCachingResponseWrapper(HttpServletResponse response) { @@ -26,8 +26,8 @@ public ContentCachingResponseWrapper buildContentCachingResponseWrapper(HttpServ } @Nullable - public MultiReadHttpServletRequestWrapper getCachingRequest(HttpServletRequest request) { - return request instanceof MultiReadHttpServletRequestWrapper ? (MultiReadHttpServletRequestWrapper) request : null; + public MultiReadContentCachingRequestWrapper getCachingRequest(HttpServletRequest request) { + return request instanceof MultiReadContentCachingRequestWrapper ? (MultiReadContentCachingRequestWrapper) request : null; } @Nullable diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadContentCachingRequestWrapper.java similarity index 53% rename from spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java rename to spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadContentCachingRequestWrapper.java index 1a481bd..b9ba01b 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadHttpServletRequestWrapper.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/MultiReadContentCachingRequestWrapper.java @@ -3,38 +3,30 @@ import jakarta.servlet.ReadListener; import jakarta.servlet.ServletInputStream; import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletRequestWrapper; import java.io.BufferedReader; import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStreamReader; -import org.apache.tomcat.util.http.fileupload.IOUtils; +import org.springframework.web.util.ContentCachingRequestWrapper; -/** - * A class to provide the ability to read a {@link jakarta.servlet.ServletRequest}'s body multiple - * times. via https://stackoverflow.com/a/36619972/2257038 and https://stackoverflow.com/a/30748533/2257038 - */ -public class MultiReadHttpServletRequestWrapper extends HttpServletRequestWrapper { +public class MultiReadContentCachingRequestWrapper extends ContentCachingRequestWrapper { - private ByteArrayOutputStream cachedBytes; - - /** - * Construct a new multi-read wrapper. - * - * @param request to wrap around - */ - public MultiReadHttpServletRequestWrapper(HttpServletRequest request) { + public MultiReadContentCachingRequestWrapper(HttpServletRequest request) { super(request); } + public MultiReadContentCachingRequestWrapper(HttpServletRequest request, int contentCacheLimit) { + super(request, contentCacheLimit); + } + @Override public ServletInputStream getInputStream() throws IOException { - if (cachedBytes == null) { - cacheInputStream(); + var inputStream = super.getInputStream(); + if (inputStream.isFinished()) { + return new CachedServletInputStream(getContentAsByteArray()); } - return new CachedServletInputStream(cachedBytes.toByteArray()); + return inputStream; } @Override @@ -42,15 +34,6 @@ public BufferedReader getReader() throws IOException { return new BufferedReader(new InputStreamReader(getInputStream())); } - private void cacheInputStream() throws IOException { - /* Cache the inputstream in order to read it multiple times. For - * convenience, I use apache.commons IOUtils - */ - cachedBytes = new ByteArrayOutputStream(); - IOUtils.copy(super.getInputStream(), cachedBytes); - } - - /* An inputstream which reads the cached request body */ private static class CachedServletInputStream extends ServletInputStream { private final ByteArrayInputStream buffer; diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java index 8e1bb3b..a43d075 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java @@ -127,7 +127,7 @@ private static RequestMetaData getRequestMetaData(HttpServletRequest request) { } private ValidationResult validateRequest( - MultiReadHttpServletRequestWrapper request, + MultiReadContentCachingRequestWrapper request, RequestMetaData requestMetaData, @Nullable ResponseMetaData responseMetaData, RunType runType @@ -148,7 +148,7 @@ private ValidationResult validateRequest( } } - private static String readBodyCatchingException(MultiReadHttpServletRequestWrapper request) { + private static String readBodyCatchingException(MultiReadContentCachingRequestWrapper request) { try { return StreamUtils.copyToString(request.getInputStream(), StandardCharsets.UTF_8); } catch (IOException e) { diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java index 8f496dc..364b47c 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/BaseFilterTest.java @@ -50,7 +50,7 @@ private static void mockRequestAttributes(ServletRequest request, HashMap Date: Thu, 23 Nov 2023 08:07:25 +0100 Subject: [PATCH 07/13] send 500 response without body on fail on request violation --- .../openapi/validation/filter/OpenApiValidationInterceptor.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java index a43d075..411d21f 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationInterceptor.java @@ -115,6 +115,8 @@ private void validateResponse( ); // Note: validateResponseResult will always be null on ASYNC if (validateResponseResult == ValidationResult.INVALID) { + response.reset(); + response.setStatus(500); throw new ResponseStatusException(HttpStatusCode.valueOf(500), "Response validation failed"); } } From b6bcb9d2ceea581aa7aeab5ea023ddc7d309c5c6 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 08:07:46 +0100 Subject: [PATCH 08/13] in tests verify controller called or not called --- .../integration/FailOnViolationIntegrationTest.java | 13 +++++++++++-- .../integration/FailOnViolationIntegrationTest.java | 12 +++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index b7134cb..f29c436 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -1,12 +1,16 @@ package com.getyourguide.openapi.validation.integration; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import com.getyourguide.openapi.validation.integration.controller.DefaultRestController; import com.getyourguide.openapi.validation.test.TestViolationLogger; import java.util.Optional; import org.hamcrest.Matchers; @@ -16,6 +20,7 @@ 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.mock.mockito.SpyBean; import org.springframework.http.MediaType; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.test.web.servlet.MockMvc; @@ -34,6 +39,9 @@ public class FailOnViolationIntegrationTest { @Autowired private TestViolationLogger openApiViolationLogger; + @SpyBean + private DefaultRestController defaultRestController; + @BeforeEach public void setup() { openApiViolationLogger.clearViolations(); @@ -50,6 +58,7 @@ public void whenValidRequestThenReturnSuccessfully() throws Exception { Thread.sleep(100); assertEquals(0, openApiViolationLogger.getViolations().size()); + verify(defaultRestController).postTest(any()); } @Test @@ -62,8 +71,7 @@ public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Excepti Thread.sleep(100); assertEquals(0, openApiViolationLogger.getViolations().size()); - - // TODO check that controller did not execute (inject controller here and have addition method to check & clear at setup) + verify(defaultRestController, never()).postTest(any()); // TODO check that something else gets logged? } @@ -81,5 +89,6 @@ public void whenInvalidResponseThenReturn500AndViolationLogged() throws Exceptio assertEquals("validation.response.body.schema.pattern", violation.getRule()); assertEquals(Optional.of(200), violation.getResponseStatus()); assertEquals(Optional.of("/value"), violation.getInstance()); + verify(defaultRestController).getTest(any(), any(), any()); } } diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index 6f00ee4..1797f14 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -1,7 +1,11 @@ package com.getyourguide.openapi.validation.integration; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import com.getyourguide.openapi.validation.integration.controller.DefaultRestController; import com.getyourguide.openapi.validation.test.TestViolationLogger; import java.util.Optional; import org.junit.jupiter.api.BeforeEach; @@ -10,6 +14,7 @@ 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.mock.mockito.SpyBean; import org.springframework.http.MediaType; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.test.web.reactive.server.WebTestClient; @@ -28,6 +33,9 @@ public class FailOnViolationIntegrationTest { @Autowired private TestViolationLogger openApiViolationLogger; + @SpyBean + private DefaultRestController defaultRestController; + @BeforeEach public void setup() { openApiViolationLogger.clearViolations(); @@ -46,6 +54,7 @@ public void whenValidRequestThenReturnSuccessfully() throws Exception { Thread.sleep(100); assertEquals(0, openApiViolationLogger.getViolations().size()); + verify(defaultRestController).postTest(any(), any()); } @Test @@ -61,8 +70,8 @@ public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Excepti Thread.sleep(100); assertEquals(0, openApiViolationLogger.getViolations().size()); + verify(defaultRestController, never()).postTest(any(), any()); - // TODO check that controller did not execute (inject controller here and have addition method to check & clear at setup) // TODO check that something else gets logged? } @@ -81,5 +90,6 @@ public void whenInvalidResponseThenReturn500AndViolationLogged() throws Exceptio assertEquals("validation.response.body.schema.pattern", violation.getRule()); assertEquals(Optional.of(200), violation.getResponseStatus()); assertEquals(Optional.of("/value"), violation.getInstance()); + verify(defaultRestController).postTest(any(), any()); } } From 885783956708dc55a5fd840b1f68e2178d5fa802 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 11:11:22 +0100 Subject: [PATCH 09/13] do not log request violation if `shouldFailOnRequestViolation` --- .../core/OpenApiRequestValidationConfiguration.java | 1 + .../openapi/validation/core/OpenApiRequestValidator.java | 9 ++++++++- .../OpenApiValidationApplicationProperties.java | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidationConfiguration.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidationConfiguration.java index 0c04f46..e37fd67 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidationConfiguration.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidationConfiguration.java @@ -8,4 +8,5 @@ public class OpenApiRequestValidationConfiguration { private double sampleRate; private int validationReportThrottleWaitSeconds; + private boolean shouldFailOnRequestViolation; } diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java index a3422ac..79f4c48 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java @@ -23,6 +23,7 @@ public class OpenApiRequestValidator { private final ThreadPoolExecutor threadPoolExecutor; private final OpenApiInteractionValidatorWrapper validator; private final ValidationReportHandler validationReportHandler; + private final OpenApiRequestValidationConfiguration configuration; public OpenApiRequestValidator( ThreadPoolExecutor threadPoolExecutor, @@ -34,6 +35,7 @@ public OpenApiRequestValidator( this.threadPoolExecutor = threadPoolExecutor; this.validator = validator; this.validationReportHandler = validationReportHandler; + this.configuration = configuration; metricsReporter.reportStartup( validator != null, @@ -74,7 +76,12 @@ public ValidationResult validateRequestObject( try { var simpleRequest = buildSimpleRequest(request, requestBody); var result = validator.validateRequest(simpleRequest); - validationReportHandler.handleValidationReport(request, response, Direction.REQUEST, requestBody, result); + // TODO this should not be done here, but currently the only way to do it -> Refactor this so that logging + // is actually done in the interceptor/filter where logging can easily be skipped then. + if (!configuration.isShouldFailOnRequestViolation()) { + validationReportHandler + .handleValidationReport(request, response, Direction.REQUEST, requestBody, result); + } return buildValidationResult(result); } catch (Exception e) { log.error("Could not validate request", e); diff --git a/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java b/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java index 6e0f891..2278298 100644 --- a/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java +++ b/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java @@ -88,6 +88,7 @@ public OpenApiRequestValidationConfiguration toOpenApiRequestValidationConfigura return OpenApiRequestValidationConfiguration.builder() .sampleRate(getSampleRate()) .validationReportThrottleWaitSeconds(getValidationReportThrottleWaitSeconds()) + .shouldFailOnRequestViolation(getShouldFailOnRequestViolation()) .build(); } } From 649a3b2cd8bf8dbdecc686e69b7982ba1d6d4ab4 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 11:11:38 +0100 Subject: [PATCH 10/13] remove already implemented TODOs --- .../integration/OpenApiValidationIntegrationTest.java | 2 -- .../integration/OpenApiValidationIntegrationTest.java | 2 -- 2 files changed, 4 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java index f4dfb54..2c6ec3b 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java @@ -123,8 +123,6 @@ public void whenTestOptionsCallThenShouldNotValidate() throws Exception { assertEquals(0, openApiViolationLogger.getViolations().size()); } - // TODO Add test that fails on request violation immediately (maybe needs separate test class & setup) should not log violation - @Nullable private OpenApiViolation getViolationByRule(List violations, String rule) { return violations.stream() diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java index 7faf80d..3a7afe4 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/OpenApiValidationIntegrationTest.java @@ -134,8 +134,6 @@ public void whenTestOptionsCallThenShouldNotValidate() throws Exception { assertEquals(0, openApiViolationLogger.getViolations().size()); } - // TODO Add test that fails on request violation immediately (maybe needs separate test class & setup) should not log violation - @Nullable private OpenApiViolation getViolationByRule(List violations, String rule) { return violations.stream() From b9a575590d0342f1fd0efc1880f111fe4e4d6f97 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 11:19:42 +0100 Subject: [PATCH 11/13] webflux: adjust test as it already is working correctly --- .../integration/FailOnViolationIntegrationTest.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index 1797f14..0cd9428 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -82,7 +82,10 @@ public void whenInvalidResponseThenReturn500AndViolationLogged() throws Exceptio .accept(MediaType.APPLICATION_JSON) .exchange() .expectStatus().is5xxServerError() - .expectBody().isEmpty(); + .expectBody() + .jsonPath("$.status").isEqualTo(500) + .jsonPath("$.path").isEqualTo("/test") + .jsonPath("$.error").isEqualTo("Internal Server Error"); Thread.sleep(100); assertEquals(1, openApiViolationLogger.getViolations().size()); @@ -90,6 +93,6 @@ public void whenInvalidResponseThenReturn500AndViolationLogged() throws Exceptio assertEquals("validation.response.body.schema.pattern", violation.getRule()); assertEquals(Optional.of(200), violation.getResponseStatus()); assertEquals(Optional.of("/value"), violation.getInstance()); - verify(defaultRestController).postTest(any(), any()); + verify(defaultRestController).getTest(any(), any(), any(), any()); } } From 58e0d9eba7d853c3a67d6945c7e45dbd5851ae1f Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 13:58:28 +0100 Subject: [PATCH 12/13] webflux: make 4xx work by making cached body re-readable --- ...penApiValidationApplicationProperties.java | 2 +- .../filter/OpenApiValidationWebFilter.java | 48 +++++++++---------- ...BodyCachingServerHttpRequestDecorator.java | 22 +++++++++ .../OpenApiValidationWebFilterTest.java | 5 +- .../FailOnViolationIntegrationTest.java | 5 +- 5 files changed, 55 insertions(+), 27 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java b/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java index 2278298..97663d3 100644 --- a/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java +++ b/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/OpenApiValidationApplicationProperties.java @@ -88,7 +88,7 @@ public OpenApiRequestValidationConfiguration toOpenApiRequestValidationConfigura return OpenApiRequestValidationConfiguration.builder() .sampleRate(getSampleRate()) .validationReportThrottleWaitSeconds(getValidationReportThrottleWaitSeconds()) - .shouldFailOnRequestViolation(getShouldFailOnRequestViolation()) + .shouldFailOnRequestViolation(getShouldFailOnRequestViolation() != null && getShouldFailOnRequestViolation()) .build(); } } diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java index d78e4ce..aa2b260 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java @@ -52,19 +52,19 @@ private Mono decorateWithValidation(ServerWebExchange exchange, WebFilterC var serverWebExchange = exchange.mutate().request(requestDecorated).response(responseDecorated).build(); - var alreadyDidRequestValidation = validateRequestWithFailOnViolation(requestMetaData, requestDecorated); - var alreadyDidResponseValidation = validateResponseWithFailOnViolation(requestMetaData, responseDecorated); - - return chain.filter(serverWebExchange) + final var alreadyDidValidation = new AlreadyDidValidation(); + alreadyDidValidation.response = validateResponseWithFailOnViolation(requestMetaData, responseDecorated); + return optionalValidateRequestWithFailOnViolation(requestMetaData, requestDecorated, alreadyDidValidation) + .flatMap(dataBuffer -> chain.filter(serverWebExchange)) .doFinally(signalType -> { // Note: Errors are not handled here. They could be handled with SignalType.ON_ERROR, but then the response body can't be accessed. // Reason seems to be that those don't use the decorated response that is set here, but use the previous response object. if (signalType == SignalType.ON_COMPLETE) { var responseMetaData = metaDataFactory.buildResponseMetaData(responseDecorated); - if (!alreadyDidRequestValidation) { + if (!alreadyDidValidation.request) { validateRequest(requestDecorated, requestMetaData, responseMetaData, RunType.ASYNC); } - if (!alreadyDidResponseValidation) { + if (!alreadyDidValidation.response) { validateResponse( requestMetaData, responseMetaData, responseDecorated.getCachedBody(), RunType.ASYNC); } @@ -72,32 +72,27 @@ private Mono decorateWithValidation(ServerWebExchange exchange, WebFilterC }); } - /** - * Validate request and fail on violation if configured to do so. - * - * @return true if validation is done as part of this method - */ - private boolean validateRequestWithFailOnViolation( + private Mono optionalValidateRequestWithFailOnViolation( RequestMetaData requestMetaData, - BodyCachingServerHttpRequestDecorator requestDecorated + BodyCachingServerHttpRequestDecorator request, + AlreadyDidValidation alreadyDidValidation ) { - if (!trafficSelector.shouldFailOnRequestViolation(requestMetaData)) { - return false; + if (!trafficSelector.shouldFailOnRequestViolation(requestMetaData) + || !request.getHeaders().containsKey("Content-Type") + || !request.getHeaders().containsKey("Content-Length")) { + return Mono.just(alreadyDidValidation); } - if (requestDecorated.getHeaders().containsKey("Content-Type") && requestDecorated.getHeaders().containsKey("Content-Length")) { - requestDecorated.setOnBodyCachedListener(() -> { - var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); + return request.consumeRequestBody() + .map((optionalRequestBody) -> { + var validateRequestResult = validateRequest(request, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, 400, "Request validation failed"); + alreadyDidValidation.request = true; + return alreadyDidValidation; }); - } else { - var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); - throwStatusExceptionOnViolation(validateRequestResult, 400, "Request validation failed"); - } - return true; } - private static void throwStatusExceptionOnViolation(ValidationResult validateRequestResult, int statusCode, String message) { + private static void throwStatusExceptionOnViolation(ValidationResult validateRequestResult, int statusCode, String message) { if (validateRequestResult == ValidationResult.INVALID) { throw new ResponseStatusException(HttpStatus.valueOf(statusCode), message); } @@ -165,4 +160,9 @@ private ValidationResult validateResponse( } private enum RunType { SYNC, ASYNC } + + private static class AlreadyDidValidation { + private boolean request = false; + private boolean response = false; + } } diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java index c883a4f..0289db9 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java @@ -3,13 +3,16 @@ import com.getyourguide.openapi.validation.api.model.RequestMetaData; import com.getyourguide.openapi.validation.api.selector.TrafficSelector; import java.nio.charset.StandardCharsets; +import java.util.stream.Collectors; import lombok.Getter; import lombok.Setter; import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpRequestDecorator; import org.springframework.lang.NonNull; import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; import reactor.core.publisher.SignalType; public class BodyCachingServerHttpRequestDecorator extends ServerHttpRequestDecorator { @@ -21,6 +24,7 @@ public class BodyCachingServerHttpRequestDecorator extends ServerHttpRequestDeco @Getter private String cachedBody; + private boolean bodyCached = false; public BodyCachingServerHttpRequestDecorator( ServerHttpRequest delegate, @@ -32,9 +36,26 @@ public BodyCachingServerHttpRequestDecorator( this.requestMetaData = requestMetaData; } + public Mono consumeRequestBody() { + return Mono.from(getBody().collectList()) + .map(dataBuffers -> + dataBuffers.stream() + .map(dataBuffer -> dataBuffer.toString(StandardCharsets.UTF_8)) + .collect(Collectors.joining("")) + ); + } + @Override @NonNull public Flux getBody() { + if (bodyCached) { + var bufferFactory = new DefaultDataBufferFactory(); + var bytes = cachedBody.getBytes(StandardCharsets.UTF_8); + var buffer = bufferFactory.allocateBuffer(bytes.length); + buffer.write(bytes); + return Flux.just(buffer); + } + if (!trafficSelector.canRequestBeValidated(requestMetaData)) { return super.getBody(); } @@ -49,6 +70,7 @@ public Flux getBody() { .doFinally(signalType -> { if (signalType == SignalType.ON_COMPLETE && onBodyCachedListener != null) { onBodyCachedListener.run(); + bodyCached = true; } }); } diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java index 0a6a411..1a3b8c4 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java @@ -132,7 +132,9 @@ public void testShouldFailOnRequestViolationWithViolation() { when(validator.validateRequestObject(eq(mockData.requestMetaData), any(), eq(REQUEST_BODY))) .thenReturn(ValidationResult.INVALID); - assertThrows(ResponseStatusException.class, () -> webFilter.filter(mockData.exchange, mockData.chain)); + StepVerifier.create(webFilter.filter(mockData.exchange, mockData.chain)) + .expectErrorMatches(throwable -> throwable instanceof ResponseStatusException) + .verify(); verifyChainNotCalled(mockData.chain); verifyRequestValidatedSync(mockData); @@ -226,6 +228,7 @@ private void mockDecoratedRequests( .thenReturn(decoratedRequest); when(decoratedRequest.getHeaders()).thenReturn(buildHeadersForBody(configuration.requestBody)); when(decoratedRequest.getCachedBody()).thenReturn(configuration.requestBody); + when(decoratedRequest.consumeRequestBody()).thenReturn(Mono.just(configuration.requestBody)); if (configuration.requestBody != null) { doAnswer(invocation -> { invocation.getArgument(0, Runnable.class).run(); diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java index 0cd9428..602236b 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/integration/FailOnViolationIntegrationTest.java @@ -66,7 +66,10 @@ public void whenInvalidRequestThenReturn400AndNoViolationLogged() throws Excepti .bodyValue("{ \"value\": 1 }") .exchange() .expectStatus().is4xxClientError() - .expectBody().isEmpty(); + .expectBody() + .jsonPath("$.status").isEqualTo(400) + .jsonPath("$.path").isEqualTo("/test") + .jsonPath("$.error").isEqualTo("Bad Request"); Thread.sleep(100); assertEquals(0, openApiViolationLogger.getViolations().size()); From bc48832bd786263516efeb4abac4421ae9742ddd Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Thu, 23 Nov 2023 14:00:45 +0100 Subject: [PATCH 13/13] remove now useless Runnable --- .../decorator/BodyCachingServerHttpRequestDecorator.java | 7 +------ .../validation/filter/OpenApiValidationWebFilterTest.java | 6 ------ 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java index 0289db9..67bf914 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/main/java/com/getyourguide/openapi/validation/filter/decorator/BodyCachingServerHttpRequestDecorator.java @@ -5,7 +5,6 @@ import java.nio.charset.StandardCharsets; import java.util.stream.Collectors; import lombok.Getter; -import lombok.Setter; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.http.server.reactive.ServerHttpRequest; @@ -19,9 +18,6 @@ public class BodyCachingServerHttpRequestDecorator extends ServerHttpRequestDeco private final TrafficSelector trafficSelector; private final RequestMetaData requestMetaData; - @Setter - private Runnable onBodyCachedListener; - @Getter private String cachedBody; private boolean bodyCached = false; @@ -68,8 +64,7 @@ public Flux getBody() { cachedBody += dataBuffer.toString(StandardCharsets.UTF_8); }) .doFinally(signalType -> { - if (signalType == SignalType.ON_COMPLETE && onBodyCachedListener != null) { - onBodyCachedListener.run(); + if (signalType == SignalType.ON_COMPLETE) { bodyCached = true; } }); diff --git a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java index 1a3b8c4..a6572d7 100644 --- a/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java +++ b/spring-boot-starter/spring-boot-starter-webflux/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilterTest.java @@ -229,12 +229,6 @@ private void mockDecoratedRequests( when(decoratedRequest.getHeaders()).thenReturn(buildHeadersForBody(configuration.requestBody)); when(decoratedRequest.getCachedBody()).thenReturn(configuration.requestBody); when(decoratedRequest.consumeRequestBody()).thenReturn(Mono.just(configuration.requestBody)); - if (configuration.requestBody != null) { - doAnswer(invocation -> { - invocation.getArgument(0, Runnable.class).run(); - return null; - }).when(decoratedRequest).setOnBodyCachedListener(any()); - } var decoratedResponse = mock(BodyCachingServerHttpResponseDecorator.class); when(decoratorBuilder.buildBodyCachingServerHttpResponseDecorator(response, requestMetaData))