From 73b420adb11a570725213796b4c176eb3d5bcf78 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 10:42:43 +0200 Subject: [PATCH 1/7] Refactor: Create class InternalViolationExclusions --- .../core/ValidationReportHandler.java | 14 +++----------- .../exclusions/InternalViolationExclusions.java | 17 +++++++++++++++++ .../autoconfigure/LibraryAutoConfiguration.java | 3 ++- 3 files changed, 22 insertions(+), 12 deletions(-) create mode 100644 openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java index fa85c05b..9bbfb443 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java @@ -1,13 +1,13 @@ package com.getyourguide.openapi.validation.core; import com.atlassian.oai.validator.report.ValidationReport; -import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions; import com.getyourguide.openapi.validation.api.log.LogLevel; import com.getyourguide.openapi.validation.api.log.ViolationLogger; import com.getyourguide.openapi.validation.api.metrics.MetricsReporter; import com.getyourguide.openapi.validation.api.model.Direction; import com.getyourguide.openapi.validation.api.model.OpenApiViolation; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions; import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler; import io.swagger.v3.oas.models.parameters.Parameter; import java.util.Optional; @@ -18,7 +18,7 @@ public class ValidationReportHandler { private final ValidationReportThrottler throttleHelper; private final ViolationLogger logger; private final MetricsReporter metrics; - private final ViolationExclusions violationExclusions; + private final InternalViolationExclusions violationExclusions; public void handleValidationReport( RequestMetaData request, @@ -31,7 +31,7 @@ public void handleValidationReport( .getMessages() .stream() .map(message -> buildOpenApiViolation(message, request, body, direction)) - .filter(violation -> !isViolationExcluded(violation)) + .filter(violation -> !violationExclusions.isExcluded(violation)) .forEach(violation -> throttleHelper.throttle(violation, () -> logValidationError(violation))); } } @@ -81,14 +81,6 @@ private OpenApiViolation buildOpenApiViolation( .build(); } - private boolean isViolationExcluded(OpenApiViolation openApiViolation) { - return - violationExclusions.isExcluded(openApiViolation) - // If it matches more than 1, then we don't want to log a validation error - || openApiViolation.getMessage().matches( - ".*\\[Path '[^']+'] Instance failed to match exactly one schema \\(matched [1-9][0-9]* out of \\d\\).*"); - } - private static Optional getPointersInstance(ValidationReport.Message message) { return message.getContext() .flatMap(ValidationReport.MessageContext::getPointers) diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java new file mode 100644 index 00000000..a3d930b2 --- /dev/null +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java @@ -0,0 +1,17 @@ +package com.getyourguide.openapi.validation.core.exclusions; + +import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions; +import com.getyourguide.openapi.validation.api.model.OpenApiViolation; +import lombok.AllArgsConstructor; + +@AllArgsConstructor +public class InternalViolationExclusions { + private final ViolationExclusions customViolationExclusions; + + public boolean isExcluded(OpenApiViolation violation) { + return customViolationExclusions.isExcluded(violation) + // If it matches more than 1, then we don't want to log a validation error + || violation.getMessage().matches( + ".*\\[Path '[^']+'] Instance failed to match exactly one schema \\(matched [1-9][0-9]* out of \\d\\).*"); + } +} diff --git a/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/autoconfigure/LibraryAutoConfiguration.java b/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/autoconfigure/LibraryAutoConfiguration.java index c5249b0f..3f2227b1 100644 --- a/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/autoconfigure/LibraryAutoConfiguration.java +++ b/spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/autoconfigure/LibraryAutoConfiguration.java @@ -17,6 +17,7 @@ import com.getyourguide.openapi.validation.core.OpenApiInteractionValidatorFactory; import com.getyourguide.openapi.validation.core.OpenApiRequestValidator; import com.getyourguide.openapi.validation.core.ValidationReportHandler; +import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions; import com.getyourguide.openapi.validation.core.throttle.RequestBasedValidationReportThrottler; import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler; import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottlerNone; @@ -81,7 +82,7 @@ public ValidationReportHandler validationReportHandler( validationReportThrottler, logger, metricsReporter, - violationExclusions.orElseGet(NoViolationExclusions::new) + new InternalViolationExclusions(violationExclusions.orElseGet(NoViolationExclusions::new)) ); } From f71ad6a1147e31c773c749dd5b8606d35d5a4aa3 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 10:45:22 +0200 Subject: [PATCH 2/7] Fix tests --- .../openapi/validation/core/ValidationReportHandlerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java index 9b5208da..2d757d6d 100644 --- a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java +++ b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java @@ -8,12 +8,12 @@ import static org.mockito.Mockito.when; import com.atlassian.oai.validator.report.ValidationReport; -import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions; import com.getyourguide.openapi.validation.api.log.ViolationLogger; import com.getyourguide.openapi.validation.api.metrics.MetricsReporter; import com.getyourguide.openapi.validation.api.model.Direction; import com.getyourguide.openapi.validation.api.model.OpenApiViolation; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions; import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler; import io.swagger.v3.oas.models.parameters.Parameter; import java.net.URI; @@ -35,7 +35,7 @@ public void setUp() { throttleHelper = mock(); logger = mock(); MetricsReporter metrics = mock(); - ViolationExclusions violationExclusions = mock(); + InternalViolationExclusions violationExclusions = mock(); validationReportHandler = new ValidationReportHandler(throttleHelper, logger, metrics, violationExclusions); } From ad7c7dfd37fa62b53d4a7c2c59d8f1fa877f9bf9 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 11:36:48 +0200 Subject: [PATCH 3/7] Refactor: Pass in responseMetaData when validating request if available --- .../core/OpenApiRequestValidator.java | 19 ++++++--- .../core/ValidationReportHandler.java | 6 ++- .../core/OpenApiRequestValidatorTest.java | 2 +- .../core/ValidationReportHandlerTest.java | 2 +- .../filter/OpenApiValidationHttpFilter.java | 20 +++++++--- .../filter/OpenApiValidationHttpFilter.java | 20 +++++++--- .../filter/OpenApiValidationWebFilter.java | 36 +++++++++++------ .../filter/OpenApiValidationWebFilter.java | 40 ++++++++++++------- 8 files changed, 99 insertions(+), 46 deletions(-) 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 ae902889..941b1d5a 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 @@ -14,6 +14,7 @@ import java.nio.charset.StandardCharsets; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadPoolExecutor; +import javax.annotation.Nullable; import lombok.extern.slf4j.Slf4j; import org.apache.http.client.utils.URLEncodedUtils; @@ -46,8 +47,8 @@ public boolean isReady() { return validator != null; } - public void validateRequestObjectAsync(final RequestMetaData request, String requestBody) { - executeAsync(() -> validateRequestObject(request, requestBody)); + public void validateRequestObjectAsync(final RequestMetaData request, @Nullable ResponseMetaData response, String requestBody) { + executeAsync(() -> validateRequestObject(request, response, requestBody)); } public void validateResponseObjectAsync(final RequestMetaData request, ResponseMetaData response, final String responseBody) { @@ -63,10 +64,18 @@ private void executeAsync(Runnable command) { } public ValidationResult validateRequestObject(final RequestMetaData request, String requestBody) { + return validateRequestObject(request, null, requestBody); + } + + public ValidationResult validateRequestObject( + final RequestMetaData request, + @Nullable final ResponseMetaData response, + String requestBody + ) { try { var simpleRequest = buildSimpleRequest(request, requestBody); var result = validator.validateRequest(simpleRequest); - validationReportHandler.handleValidationReport(request, Direction.REQUEST, requestBody, result); + validationReportHandler.handleValidationReport(request, response, Direction.REQUEST, requestBody, result); reportValidationHeartbeat(); return buildValidationResult(result); } catch (Exception e) { @@ -96,7 +105,7 @@ private static String nullSafeUrlDecode(String value) { public ValidationResult validateResponseObject( final RequestMetaData request, - ResponseMetaData response, + final ResponseMetaData response, final String responseBody ) { try { @@ -112,7 +121,7 @@ public ValidationResult validateResponseObject( Request.Method.valueOf(request.getMethod().toUpperCase()), responseBuilder.build() ); - validationReportHandler.handleValidationReport(request, Direction.RESPONSE, responseBody, result); + validationReportHandler.handleValidationReport(request, response, Direction.RESPONSE, responseBody, result); reportValidationHeartbeat(); return buildValidationResult(result); } catch (Exception e) { diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java index 9bbfb443..87d1523c 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java @@ -7,10 +7,12 @@ import com.getyourguide.openapi.validation.api.model.Direction; import com.getyourguide.openapi.validation.api.model.OpenApiViolation; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.api.model.ResponseMetaData; import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions; import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler; import io.swagger.v3.oas.models.parameters.Parameter; import java.util.Optional; +import javax.annotation.Nullable; import lombok.AllArgsConstructor; @AllArgsConstructor @@ -22,6 +24,7 @@ public class ValidationReportHandler { public void handleValidationReport( RequestMetaData request, + @Nullable ResponseMetaData response, Direction direction, String body, ValidationReport result @@ -30,7 +33,7 @@ public void handleValidationReport( result .getMessages() .stream() - .map(message -> buildOpenApiViolation(message, request, body, direction)) + .map(message -> buildOpenApiViolation(message, request, response, body, direction)) .filter(violation -> !violationExclusions.isExcluded(violation)) .forEach(violation -> throttleHelper.throttle(violation, () -> logValidationError(violation))); } @@ -44,6 +47,7 @@ private void logValidationError(OpenApiViolation openApiViolation) { private OpenApiViolation buildOpenApiViolation( ValidationReport.Message message, RequestMetaData request, + @Nullable ResponseMetaData response, String body, Direction direction ) { diff --git a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidatorTest.java b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidatorTest.java index 33c1cec8..95ce422e 100644 --- a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidatorTest.java +++ b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidatorTest.java @@ -44,7 +44,7 @@ public void setup() { public void testWhenThreadPoolExecutorRejectsExecutionThenItShouldNotThrow() { Mockito.doThrow(new RejectedExecutionException()).when(threadPoolExecutor).execute(any()); - openApiRequestValidator.validateRequestObjectAsync(mock(), null); + openApiRequestValidator.validateRequestObjectAsync(mock(), null, null); } @Test diff --git a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java index 2d757d6d..cdf1be14 100644 --- a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java +++ b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java @@ -46,7 +46,7 @@ public void testWhenParameterNameIsPresentThenItShouldAddItToTheMessage() { var request = mockRequestMetaData(); var validationReport = mockValidationReport("parameterName"); - validationReportHandler.handleValidationReport(request, Direction.REQUEST, null, validationReport); + validationReportHandler.handleValidationReport(request, null, Direction.REQUEST, null, validationReport); var argumentCaptor = ArgumentCaptor.forClass(OpenApiViolation.class); verify(logger).log(argumentCaptor.capture()); diff --git a/spring-boot-starter/spring-boot-starter-web-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java b/spring-boot-starter/spring-boot-starter-web-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java index 31352c41..1f6f9eb9 100644 --- a/spring-boot-starter/spring-boot-starter-web-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java +++ b/spring-boot-starter/spring-boot-starter-web-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java @@ -1,6 +1,7 @@ package com.getyourguide.openapi.validation.filter; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.api.model.ResponseMetaData; import com.getyourguide.openapi.validation.api.model.ValidationResult; import com.getyourguide.openapi.validation.api.selector.TrafficSelector; import com.getyourguide.openapi.validation.core.OpenApiRequestValidator; @@ -8,6 +9,7 @@ import com.getyourguide.openapi.validation.factory.ServletMetaDataFactory; import java.io.IOException; import java.nio.charset.StandardCharsets; +import javax.annotation.Nullable; import javax.servlet.FilterChain; import javax.servlet.ServletException; import javax.servlet.ServletRequest; @@ -59,12 +61,17 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha try { super.doFilter(requestWrapper, responseWrapper, chain); } finally { + var responseMetaData = metaDataFactory.buildResponseMetaData(responseWrapper); if (!alreadyDidRequestValidation) { - validateRequest(requestWrapper, requestMetaData, RunType.ASYNC); + validateRequest(requestWrapper, requestMetaData, responseMetaData, RunType.ASYNC); } - var validateResponseResult = - validateResponse(responseWrapper, requestMetaData, getRunTypeForResponseValidation(requestMetaData)); + var validateResponseResult = validateResponse( + responseWrapper, + requestMetaData, + responseMetaData, + getRunTypeForResponseValidation(requestMetaData) + ); throwStatusExceptionOnViolation(validateResponseResult, "Response validation failed"); responseWrapper.copyBodyToResponse(); // Needs to be done on every call, otherwise there won't be a response body @@ -87,7 +94,7 @@ private boolean validateRequestWithFailOnViolation( return false; } - var validateRequestResult = validateRequest(request, requestMetaData, RunType.SYNC); + var validateRequestResult = validateRequest(request, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); return true; } @@ -95,6 +102,7 @@ private boolean validateRequestWithFailOnViolation( private ValidationResult validateRequest( ContentCachingRequestWrapper request, RequestMetaData requestMetaData, + @Nullable ResponseMetaData responseMetaData, RunType runType ) { if (!trafficSelector.canRequestBeValidated(requestMetaData)) { @@ -106,7 +114,7 @@ private ValidationResult validateRequest( : null; if (runType == RunType.ASYNC) { - validator.validateRequestObjectAsync(requestMetaData, requestBody); + validator.validateRequestObjectAsync(requestMetaData, responseMetaData, requestBody); return ValidationResult.NOT_APPLICABLE; } else { return validator.validateRequestObject(requestMetaData, requestBody); @@ -116,9 +124,9 @@ private ValidationResult validateRequest( private ValidationResult validateResponse( ContentCachingResponseWrapper response, RequestMetaData requestMetaData, + ResponseMetaData responseMetaData, RunType runType ) { - var responseMetaData = metaDataFactory.buildResponseMetaData(response); if (!trafficSelector.canResponseBeValidated(requestMetaData, responseMetaData)) { return ValidationResult.NOT_APPLICABLE; } diff --git a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java index 656746fc..c3aaceaa 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java +++ b/spring-boot-starter/spring-boot-starter-web/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilter.java @@ -1,6 +1,7 @@ package com.getyourguide.openapi.validation.filter; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.api.model.ResponseMetaData; import com.getyourguide.openapi.validation.api.model.ValidationResult; import com.getyourguide.openapi.validation.api.selector.TrafficSelector; import com.getyourguide.openapi.validation.core.OpenApiRequestValidator; @@ -15,6 +16,7 @@ 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.core.Ordered; @@ -59,12 +61,17 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha try { super.doFilter(requestWrapper, responseWrapper, chain); } finally { + var responseMetaData = metaDataFactory.buildResponseMetaData(responseWrapper); if (!alreadyDidRequestValidation) { - validateRequest(requestWrapper, requestMetaData, RunType.ASYNC); + validateRequest(requestWrapper, requestMetaData, responseMetaData, RunType.ASYNC); } - var validateResponseResult = - validateResponse(responseWrapper, requestMetaData, getRunTypeForResponseValidation(requestMetaData)); + var validateResponseResult = validateResponse( + responseWrapper, + requestMetaData, + responseMetaData, + getRunTypeForResponseValidation(requestMetaData) + ); throwStatusExceptionOnViolation(validateResponseResult, "Response validation failed"); responseWrapper.copyBodyToResponse(); // Needs to be done on every call, otherwise there won't be a response body @@ -87,7 +94,7 @@ private boolean validateRequestWithFailOnViolation( return false; } - var validateRequestResult = validateRequest(request, requestMetaData, RunType.SYNC); + var validateRequestResult = validateRequest(request, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); return true; } @@ -95,6 +102,7 @@ private boolean validateRequestWithFailOnViolation( private ValidationResult validateRequest( ContentCachingRequestWrapper request, RequestMetaData requestMetaData, + @Nullable ResponseMetaData responseMetaData, RunType runType ) { if (!trafficSelector.canRequestBeValidated(requestMetaData)) { @@ -106,7 +114,7 @@ private ValidationResult validateRequest( : null; if (runType == RunType.ASYNC) { - validator.validateRequestObjectAsync(requestMetaData, requestBody); + validator.validateRequestObjectAsync(requestMetaData, responseMetaData, requestBody); return ValidationResult.NOT_APPLICABLE; } else { return validator.validateRequestObject(requestMetaData, requestBody); @@ -116,9 +124,9 @@ private ValidationResult validateRequest( private ValidationResult validateResponse( ContentCachingResponseWrapper response, RequestMetaData requestMetaData, + ResponseMetaData responseMetaData, RunType runType ) { - var responseMetaData = metaDataFactory.buildResponseMetaData(response); if (!trafficSelector.canResponseBeValidated(requestMetaData, responseMetaData)) { return ValidationResult.NOT_APPLICABLE; } diff --git a/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java b/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java index f6b95bf6..d8f98bbc 100644 --- a/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java +++ b/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java @@ -1,6 +1,7 @@ package com.getyourguide.openapi.validation.filter; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.api.model.ResponseMetaData; import com.getyourguide.openapi.validation.api.model.ValidationResult; import com.getyourguide.openapi.validation.api.selector.TrafficSelector; import com.getyourguide.openapi.validation.core.OpenApiRequestValidator; @@ -8,6 +9,7 @@ import com.getyourguide.openapi.validation.filter.decorator.BodyCachingServerHttpRequestDecorator; import com.getyourguide.openapi.validation.filter.decorator.BodyCachingServerHttpResponseDecorator; import com.getyourguide.openapi.validation.filter.decorator.DecoratorBuilder; +import javax.annotation.Nullable; import lombok.AllArgsConstructor; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; @@ -59,11 +61,13 @@ private Mono decorateWithValidation(ServerWebExchange exchange, WebFilterC // 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) { - validateRequest(requestDecorated, requestMetaData, RunType.ASYNC); + validateRequest(requestDecorated, requestMetaData, responseMetaData, RunType.ASYNC); } if (!alreadyDidResponseValidation) { - validateResponse(responseDecorated, requestMetaData, RunType.ASYNC); + validateResponse( + requestMetaData, responseMetaData, responseDecorated.getCachedBody(), RunType.ASYNC); } } }); @@ -71,6 +75,7 @@ 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( @@ -83,11 +88,11 @@ private boolean validateRequestWithFailOnViolation( if (requestDecorated.getHeaders().containsKey("Content-Type") && requestDecorated.getHeaders().containsKey("Content-Length")) { requestDecorated.setOnBodyCachedListener(() -> { - var validateRequestResult = validateRequest(requestDecorated, requestMetaData, RunType.SYNC); + var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); }); } else { - var validateRequestResult = validateRequest(requestDecorated, requestMetaData, RunType.SYNC); + var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); } return true; @@ -101,6 +106,7 @@ private static void throwStatusExceptionOnViolation(ValidationResult validateReq /** * Validate response and fail on violation if configured to do so. + * * @return true if validation is done as part of this method */ private boolean validateResponseWithFailOnViolation( @@ -112,7 +118,12 @@ private boolean validateResponseWithFailOnViolation( } responseDecorated.setOnBodyCachedListener(() -> { - var validateResponseResult = validateResponse(responseDecorated, requestMetaData, RunType.SYNC); + var validateResponseResult = validateResponse( + requestMetaData, + metaDataFactory.buildResponseMetaData(responseDecorated), + responseDecorated.getCachedBody(), + RunType.SYNC + ); throwStatusExceptionOnViolation(validateResponseResult, "Response validation failed"); }); return true; @@ -121,6 +132,7 @@ private boolean validateResponseWithFailOnViolation( private ValidationResult validateRequest( BodyCachingServerHttpRequestDecorator request, RequestMetaData requestMetaData, + @Nullable ResponseMetaData responseMetaData, RunType runType ) { if (!trafficSelector.canRequestBeValidated(requestMetaData)) { @@ -128,30 +140,30 @@ private ValidationResult validateRequest( } if (runType == RunType.SYNC) { - return validator.validateRequestObject(requestMetaData, request.getCachedBody()); + return validator.validateRequestObject(requestMetaData, responseMetaData, request.getCachedBody()); } else { - validator.validateRequestObjectAsync(requestMetaData, request.getCachedBody()); + validator.validateRequestObjectAsync(requestMetaData, responseMetaData, request.getCachedBody()); return ValidationResult.NOT_APPLICABLE; } } private ValidationResult validateResponse( - BodyCachingServerHttpResponseDecorator response, RequestMetaData requestMetaData, + @Nullable ResponseMetaData responseMetaData, + @Nullable String responseBody, RunType runType ) { - var responseMetaData = metaDataFactory.buildResponseMetaData(response); if (!trafficSelector.canResponseBeValidated(requestMetaData, responseMetaData)) { return ValidationResult.NOT_APPLICABLE; } if (runType == RunType.SYNC) { - return validator.validateResponseObject(requestMetaData, responseMetaData, response.getCachedBody()); + return validator.validateResponseObject(requestMetaData, responseMetaData, responseBody); } else { - validator.validateResponseObjectAsync(requestMetaData, responseMetaData, response.getCachedBody()); + validator.validateResponseObjectAsync(requestMetaData, responseMetaData, responseBody); return ValidationResult.NOT_APPLICABLE; } } - private enum RunType { SYNC, ASYNC } + private enum RunType {SYNC, ASYNC} } 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 f5218dea..d8f98bbc 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 @@ -1,6 +1,7 @@ package com.getyourguide.openapi.validation.filter; import com.getyourguide.openapi.validation.api.model.RequestMetaData; +import com.getyourguide.openapi.validation.api.model.ResponseMetaData; import com.getyourguide.openapi.validation.api.model.ValidationResult; import com.getyourguide.openapi.validation.api.selector.TrafficSelector; import com.getyourguide.openapi.validation.core.OpenApiRequestValidator; @@ -8,10 +9,11 @@ import com.getyourguide.openapi.validation.filter.decorator.BodyCachingServerHttpRequestDecorator; import com.getyourguide.openapi.validation.filter.decorator.BodyCachingServerHttpResponseDecorator; import com.getyourguide.openapi.validation.filter.decorator.DecoratorBuilder; +import javax.annotation.Nullable; import lombok.AllArgsConstructor; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; -import org.springframework.http.HttpStatusCode; +import org.springframework.http.HttpStatus; import org.springframework.lang.NonNull; import org.springframework.stereotype.Component; import org.springframework.web.server.ResponseStatusException; @@ -59,11 +61,13 @@ private Mono decorateWithValidation(ServerWebExchange exchange, WebFilterC // 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) { - validateRequest(requestDecorated, requestMetaData, RunType.ASYNC); + validateRequest(requestDecorated, requestMetaData, responseMetaData, RunType.ASYNC); } if (!alreadyDidResponseValidation) { - validateResponse(responseDecorated, requestMetaData, RunType.ASYNC); + validateResponse( + requestMetaData, responseMetaData, responseDecorated.getCachedBody(), RunType.ASYNC); } } }); @@ -71,6 +75,7 @@ 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( @@ -83,11 +88,11 @@ private boolean validateRequestWithFailOnViolation( if (requestDecorated.getHeaders().containsKey("Content-Type") && requestDecorated.getHeaders().containsKey("Content-Length")) { requestDecorated.setOnBodyCachedListener(() -> { - var validateRequestResult = validateRequest(requestDecorated, requestMetaData, RunType.SYNC); + var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); }); } else { - var validateRequestResult = validateRequest(requestDecorated, requestMetaData, RunType.SYNC); + var validateRequestResult = validateRequest(requestDecorated, requestMetaData, null, RunType.SYNC); throwStatusExceptionOnViolation(validateRequestResult, "Request validation failed"); } return true; @@ -95,12 +100,13 @@ private boolean validateRequestWithFailOnViolation( private static void throwStatusExceptionOnViolation(ValidationResult validateRequestResult, String message) { if (validateRequestResult == ValidationResult.INVALID) { - throw new ResponseStatusException(HttpStatusCode.valueOf(400), message); + throw new ResponseStatusException(HttpStatus.valueOf(400), message); } } /** * Validate response and fail on violation if configured to do so. + * * @return true if validation is done as part of this method */ private boolean validateResponseWithFailOnViolation( @@ -112,7 +118,12 @@ private boolean validateResponseWithFailOnViolation( } responseDecorated.setOnBodyCachedListener(() -> { - var validateResponseResult = validateResponse(responseDecorated, requestMetaData, RunType.SYNC); + var validateResponseResult = validateResponse( + requestMetaData, + metaDataFactory.buildResponseMetaData(responseDecorated), + responseDecorated.getCachedBody(), + RunType.SYNC + ); throwStatusExceptionOnViolation(validateResponseResult, "Response validation failed"); }); return true; @@ -121,6 +132,7 @@ private boolean validateResponseWithFailOnViolation( private ValidationResult validateRequest( BodyCachingServerHttpRequestDecorator request, RequestMetaData requestMetaData, + @Nullable ResponseMetaData responseMetaData, RunType runType ) { if (!trafficSelector.canRequestBeValidated(requestMetaData)) { @@ -128,30 +140,30 @@ private ValidationResult validateRequest( } if (runType == RunType.SYNC) { - return validator.validateRequestObject(requestMetaData, request.getCachedBody()); + return validator.validateRequestObject(requestMetaData, responseMetaData, request.getCachedBody()); } else { - validator.validateRequestObjectAsync(requestMetaData, request.getCachedBody()); + validator.validateRequestObjectAsync(requestMetaData, responseMetaData, request.getCachedBody()); return ValidationResult.NOT_APPLICABLE; } } private ValidationResult validateResponse( - BodyCachingServerHttpResponseDecorator response, RequestMetaData requestMetaData, + @Nullable ResponseMetaData responseMetaData, + @Nullable String responseBody, RunType runType ) { - var responseMetaData = metaDataFactory.buildResponseMetaData(response); if (!trafficSelector.canResponseBeValidated(requestMetaData, responseMetaData)) { return ValidationResult.NOT_APPLICABLE; } if (runType == RunType.SYNC) { - return validator.validateResponseObject(requestMetaData, responseMetaData, response.getCachedBody()); + return validator.validateResponseObject(requestMetaData, responseMetaData, responseBody); } else { - validator.validateResponseObjectAsync(requestMetaData, responseMetaData, response.getCachedBody()); + validator.validateResponseObjectAsync(requestMetaData, responseMetaData, responseBody); return ValidationResult.NOT_APPLICABLE; } } - private enum RunType { SYNC, ASYNC } + private enum RunType {SYNC, ASYNC} } From a14a4443b815adf01a026915c1a64a8bc1a85239 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 11:57:00 +0200 Subject: [PATCH 4/7] Refactor: Pass in responseMetaData when validating request if available (part 2) --- .../validation/core/ValidationReportHandler.java | 11 +++++++++-- .../filter/OpenApiValidationHttpFilterTest.java | 4 ++-- .../validation/filter/OpenApiValidationWebFilter.java | 2 +- .../validation/filter/OpenApiValidationWebFilter.java | 2 +- .../filter/OpenApiValidationWebFilterTest.java | 8 ++++---- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java index 87d1523c..3a171b6e 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java @@ -79,7 +79,7 @@ private OpenApiViolation buildOpenApiViolation( .instance(pointersInstance) .parameter(parameterName) .schema(getPointersSchema(message)) - .responseStatus(getResponseStatus(message)) + .responseStatus(getResponseStatus(response, message)) .logMessage(logMessage) .message(message.getMessage()) .build(); @@ -115,7 +115,14 @@ private static Optional getNormalizedPath(ValidationReport.Message messa .map(apiOperation -> apiOperation.getApiPath().normalised()); } - private static Optional getResponseStatus(ValidationReport.Message message) { + private static Optional getResponseStatus( + @Nullable ResponseMetaData response, + ValidationReport.Message message + ) { + if (response != null && response.getStatusCode() != null) { + return Optional.of(response.getStatusCode()); + } + return message.getContext().flatMap(ValidationReport.MessageContext::getResponseStatus); } diff --git a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilterTest.java b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilterTest.java index 9aed85ef..d1048122 100644 --- a/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilterTest.java +++ b/spring-boot-starter/spring-boot-starter-web/src/test/java/com/getyourguide/openapi/validation/filter/OpenApiValidationHttpFilterTest.java @@ -156,7 +156,7 @@ private void verifyNoValidation() { } private void verifyNoRequestValidation() { - verify(validator, never()).validateRequestObjectAsync(any(), anyString()); + verify(validator, never()).validateRequestObjectAsync(any(), any(), anyString()); verify(validator, never()).validateRequestObject(any(), anyString()); } @@ -166,7 +166,7 @@ private void verifyNoResponseValidation() { } private void verifyRequestValidatedAsync(MockSetupData mockData) { - verify(validator).validateRequestObjectAsync(eq(mockData.requestMetaData), eq(REQUEST_BODY)); + verify(validator).validateRequestObjectAsync(eq(mockData.requestMetaData), eq(mockData.responseMetaData), eq(REQUEST_BODY)); } private void verifyRequestValidatedSync(MockSetupData mockData) { diff --git a/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java b/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java index d8f98bbc..0339babe 100644 --- a/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java +++ b/spring-boot-starter/spring-boot-starter-webflux-spring2.7/src/main/java/com/getyourguide/openapi/validation/filter/OpenApiValidationWebFilter.java @@ -165,5 +165,5 @@ private ValidationResult validateResponse( } } - private enum RunType {SYNC, ASYNC} + private enum RunType { SYNC, ASYNC } } 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 d8f98bbc..0339babe 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 @@ -165,5 +165,5 @@ private ValidationResult validateResponse( } } - private enum RunType {SYNC, ASYNC} + private enum RunType { SYNC, ASYNC } } 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 bda37741..0a6a4119 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 @@ -129,7 +129,7 @@ public void testShouldFailOnReResponseViolationWithoutViolation() { @Test public void testShouldFailOnRequestViolationWithViolation() { var mockData = mockSetup(MockConfiguration.builder().shouldFailOnRequestViolation(true).build()); - when(validator.validateRequestObject(eq(mockData.requestMetaData), eq(REQUEST_BODY))) + when(validator.validateRequestObject(eq(mockData.requestMetaData), any(), eq(REQUEST_BODY))) .thenReturn(ValidationResult.INVALID); assertThrows(ResponseStatusException.class, () -> webFilter.filter(mockData.exchange, mockData.chain)); @@ -162,7 +162,7 @@ private void verifyNoValidation() { } private void verifyNoRequestValidation() { - verify(validator, never()).validateRequestObjectAsync(any(), anyString()); + verify(validator, never()).validateRequestObjectAsync(any(), any(), anyString()); verify(validator, never()).validateRequestObject(any(), anyString()); } @@ -172,11 +172,11 @@ private void verifyNoResponseValidation() { } private void verifyRequestValidatedAsync(MockSetupData mockData) { - verify(validator).validateRequestObjectAsync(eq(mockData.requestMetaData), eq(REQUEST_BODY)); + verify(validator).validateRequestObjectAsync(eq(mockData.requestMetaData), any(), eq(REQUEST_BODY)); } private void verifyRequestValidatedSync(MockSetupData mockData) { - verify(validator).validateRequestObject(eq(mockData.requestMetaData), eq(REQUEST_BODY)); + verify(validator).validateRequestObject(eq(mockData.requestMetaData), any(), eq(REQUEST_BODY)); } private void verifyResponseValidatedAsync(MockSetupData mockData) { From c84d4e157c4acb23cd3b4900e80955aa1efca49a Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 11:57:21 +0200 Subject: [PATCH 5/7] Implement 404 false positives --- .../InternalViolationExclusions.java | 12 ++- .../InternalViolationExclusionsTest.java | 85 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java index a3d930b2..bc545836 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java @@ -1,6 +1,7 @@ package com.getyourguide.openapi.validation.core.exclusions; import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions; +import com.getyourguide.openapi.validation.api.model.Direction; import com.getyourguide.openapi.validation.api.model.OpenApiViolation; import lombok.AllArgsConstructor; @@ -9,9 +10,18 @@ public class InternalViolationExclusions { private final ViolationExclusions customViolationExclusions; public boolean isExcluded(OpenApiViolation violation) { - return customViolationExclusions.isExcluded(violation) + return falsePositive404(violation) + || customViolationExclusions.isExcluded(violation) // If it matches more than 1, then we don't want to log a validation error || violation.getMessage().matches( ".*\\[Path '[^']+'] Instance failed to match exactly one schema \\(matched [1-9][0-9]* out of \\d\\).*"); } + + private boolean falsePositive404(OpenApiViolation violation) { + return "validation.request.path.missing".equals(violation.getRule()) + && ( + violation.getDirection() == Direction.REQUEST + || (violation.getDirection() == Direction.RESPONSE && violation.getResponseStatus().orElse(0) == 404) + ); + } } diff --git a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java new file mode 100644 index 00000000..5eafb5bb --- /dev/null +++ b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java @@ -0,0 +1,85 @@ +package com.getyourguide.openapi.validation.core.exclusions; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions; +import com.getyourguide.openapi.validation.api.model.Direction; +import com.getyourguide.openapi.validation.api.model.OpenApiViolation; +import java.util.Optional; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class InternalViolationExclusionsTest { + private ViolationExclusions customViolationExclusions; + private InternalViolationExclusions violationExclusions; + + @BeforeEach + public void setup() { + customViolationExclusions = mock(); + violationExclusions = new InternalViolationExclusions(customViolationExclusions); + } + + @Test + public void testWhenViolationThenReturnFalse() { + when(customViolationExclusions.isExcluded(any())).thenReturn(false); + + var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder() + .direction(Direction.REQUEST) + .rule("validation.request.something") + .responseStatus(Optional.of(404)) + .message("Some violation message") + .build()); + + assertFalse(isExcluded); + } + + @Test + public void testWhenCustomViolationExclusionThenReturnTrue() { + when(customViolationExclusions.isExcluded(any())).thenReturn(true); + + var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder().build()); + + assertTrue(isExcluded); + } + + @Test + public void testWhenInstanceFailedToMatchExactlyOneThenReturnTrue() { + when(customViolationExclusions.isExcluded(any())).thenReturn(false); + + var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder().message("[Path '/v1/endpoint'] Instance failed to match exactly one schema (matched 2 out of 4)").build()); + + assertTrue(isExcluded); + } + + @Test + public void testWhen404ResponseWithApiPathNotSpecifiedThenReturnTrue() { + when(customViolationExclusions.isExcluded(any())).thenReturn(false); + + var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder() + .direction(Direction.RESPONSE) + .rule("validation.request.path.missing") + .responseStatus(Optional.of(404)) + .message("No API path found that matches request '/nothing'") + .build()); + + assertTrue(isExcluded); + } + + @Test + public void testWhenRequestWithApiPathNotSpecifiedThenReturnTrue() { + when(customViolationExclusions.isExcluded(any())).thenReturn(false); + + var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder() + .direction(Direction.REQUEST) + .rule("validation.request.path.missing") + .responseStatus(Optional.empty()) + .message("No API path found that matches request '/nothing'") + .build()); + + assertTrue(isExcluded); + } +} From 32602c81e85ced2a9dba5b85a86deefd2c414754 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 14:11:04 +0200 Subject: [PATCH 6/7] Implement 400 false positives --- .../InternalViolationExclusions.java | 5 ++ .../InternalViolationExclusionsTest.java | 67 +++++++++++++------ 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java index bc545836..b2af2d04 100644 --- a/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java +++ b/openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java @@ -11,6 +11,7 @@ public class InternalViolationExclusions { public boolean isExcluded(OpenApiViolation violation) { return falsePositive404(violation) + || falsePositive400(violation) || customViolationExclusions.isExcluded(violation) // If it matches more than 1, then we don't want to log a validation error || violation.getMessage().matches( @@ -24,4 +25,8 @@ private boolean falsePositive404(OpenApiViolation violation) { || (violation.getDirection() == Direction.RESPONSE && violation.getResponseStatus().orElse(0) == 404) ); } + + private boolean falsePositive400(OpenApiViolation violation) { + return violation.getDirection() == Direction.REQUEST && violation.getResponseStatus().orElse(0) == 400; + } } diff --git a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java index 5eafb5bb..08554c22 100644 --- a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java +++ b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java @@ -24,61 +24,84 @@ public void setup() { } @Test - public void testWhenViolationThenReturnFalse() { + public void testWhenViolationThenViolationNotExcluded() { when(customViolationExclusions.isExcluded(any())).thenReturn(false); - var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder() - .direction(Direction.REQUEST) - .rule("validation.request.something") - .responseStatus(Optional.of(404)) - .message("Some violation message") - .build()); + checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 404)); + checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 400)); + checkViolationNotExcluded(buildSimpleViolation(Direction.REQUEST, 200)); + checkViolationNotExcluded(buildSimpleViolation(Direction.REQUEST, null)); + checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 200)); + } - assertFalse(isExcluded); + private static OpenApiViolation buildSimpleViolation(Direction direction, Integer responseStatus) { + return OpenApiViolation.builder() + .direction(direction) + .rule("validation." + (direction == Direction.REQUEST ? "request" : "response") +".something") + .responseStatus(responseStatus != null ? Optional.of(responseStatus) : Optional.empty()) + .message("Some violation message") + .build(); } + @Test - public void testWhenCustomViolationExclusionThenReturnTrue() { + public void testWhenCustomViolationExclusionThenViolationExcluded() { when(customViolationExclusions.isExcluded(any())).thenReturn(true); - var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder().build()); - - assertTrue(isExcluded); + checkViolationExcluded(OpenApiViolation.builder().build()); } @Test - public void testWhenInstanceFailedToMatchExactlyOneThenReturnTrue() { + public void testWhenInstanceFailedToMatchExactlyOneThenViolationExcluded() { when(customViolationExclusions.isExcluded(any())).thenReturn(false); - var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder().message("[Path '/v1/endpoint'] Instance failed to match exactly one schema (matched 2 out of 4)").build()); - - assertTrue(isExcluded); + checkViolationExcluded(OpenApiViolation.builder() + .message("[Path '/v1/endpoint'] Instance failed to match exactly one schema (matched 2 out of 4)").build()); } @Test - public void testWhen404ResponseWithApiPathNotSpecifiedThenReturnTrue() { + public void testWhen404ResponseWithApiPathNotSpecifiedThenViolationExcluded() { when(customViolationExclusions.isExcluded(any())).thenReturn(false); - var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder() + checkViolationExcluded(OpenApiViolation.builder() .direction(Direction.RESPONSE) .rule("validation.request.path.missing") .responseStatus(Optional.of(404)) .message("No API path found that matches request '/nothing'") .build()); - - assertTrue(isExcluded); } @Test - public void testWhenRequestWithApiPathNotSpecifiedThenReturnTrue() { + public void testWhenRequestWithApiPathNotSpecifiedThenViolationExcluded() { when(customViolationExclusions.isExcluded(any())).thenReturn(false); - var isExcluded = violationExclusions.isExcluded(OpenApiViolation.builder() + checkViolationExcluded(OpenApiViolation.builder() .direction(Direction.REQUEST) .rule("validation.request.path.missing") .responseStatus(Optional.empty()) .message("No API path found that matches request '/nothing'") .build()); + } + + @Test + public void testWhenRequestViolationsAnd400ThenViolationExcluded() { + when(customViolationExclusions.isExcluded(any())).thenReturn(false); + + checkViolationExcluded(OpenApiViolation.builder() + .direction(Direction.REQUEST) + .responseStatus(Optional.of(400)) + .message("") + .build()); + } + + private void checkViolationNotExcluded(OpenApiViolation violation) { + var isExcluded = violationExclusions.isExcluded(violation); + + assertFalse(isExcluded); + } + + private void checkViolationExcluded(OpenApiViolation violation) { + var isExcluded = violationExclusions.isExcluded(violation); assertTrue(isExcluded); } From ae7d5c6a277fe0198eb1c91803e51159321a9c03 Mon Sep 17 00:00:00 2001 From: Patrick Boos Date: Wed, 23 Aug 2023 14:21:48 +0200 Subject: [PATCH 7/7] Formatting --- .../core/exclusions/InternalViolationExclusionsTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java index 08554c22..e52bfc58 100644 --- a/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java +++ b/openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java @@ -37,13 +37,12 @@ public void testWhenViolationThenViolationNotExcluded() { private static OpenApiViolation buildSimpleViolation(Direction direction, Integer responseStatus) { return OpenApiViolation.builder() .direction(direction) - .rule("validation." + (direction == Direction.REQUEST ? "request" : "response") +".something") + .rule("validation." + (direction == Direction.REQUEST ? "request" : "response") + ".something") .responseStatus(responseStatus != null ? Optional.of(responseStatus) : Optional.empty()) .message("Some violation message") .build(); } - @Test public void testWhenCustomViolationExclusionThenViolationExcluded() { when(customViolationExclusions.isExcluded(any())).thenReturn(true);