Skip to content

Commit d21c4f4

Browse files
pboospboos
authored
Exclude false positive violations (#24)
1 parent 85cd957 commit d21c4f4

File tree

13 files changed

+258
-66
lines changed

13 files changed

+258
-66
lines changed

‎openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import java.nio.charset.StandardCharsets;
1515
import java.util.concurrent.RejectedExecutionException;
1616
import java.util.concurrent.ThreadPoolExecutor;
17+
import javax.annotation.Nullable;
1718
import lombok.extern.slf4j.Slf4j;
1819
import org.apache.http.client.utils.URLEncodedUtils;
1920

@@ -46,8 +47,8 @@ public boolean isReady() {
4647
return validator != null;
4748
}
4849

49-
public void validateRequestObjectAsync(final RequestMetaData request, String requestBody) {
50-
executeAsync(() -> validateRequestObject(request, requestBody));
50+
public void validateRequestObjectAsync(final RequestMetaData request, @Nullable ResponseMetaData response, String requestBody) {
51+
executeAsync(() -> validateRequestObject(request, response, requestBody));
5152
}
5253

5354
public void validateResponseObjectAsync(final RequestMetaData request, ResponseMetaData response, final String responseBody) {
@@ -63,10 +64,18 @@ private void executeAsync(Runnable command) {
6364
}
6465

6566
public ValidationResult validateRequestObject(final RequestMetaData request, String requestBody) {
67+
return validateRequestObject(request, null, requestBody);
68+
}
69+
70+
public ValidationResult validateRequestObject(
71+
final RequestMetaData request,
72+
@Nullable final ResponseMetaData response,
73+
String requestBody
74+
) {
6675
try {
6776
var simpleRequest = buildSimpleRequest(request, requestBody);
6877
var result = validator.validateRequest(simpleRequest);
69-
validationReportHandler.handleValidationReport(request, Direction.REQUEST, requestBody, result);
78+
validationReportHandler.handleValidationReport(request, response, Direction.REQUEST, requestBody, result);
7079
reportValidationHeartbeat();
7180
return buildValidationResult(result);
7281
} catch (Exception e) {
@@ -96,7 +105,7 @@ private static String nullSafeUrlDecode(String value) {
96105

97106
public ValidationResult validateResponseObject(
98107
final RequestMetaData request,
99-
ResponseMetaData response,
108+
final ResponseMetaData response,
100109
final String responseBody
101110
) {
102111
try {
@@ -112,7 +121,7 @@ public ValidationResult validateResponseObject(
112121
Request.Method.valueOf(request.getMethod().toUpperCase()),
113122
responseBuilder.build()
114123
);
115-
validationReportHandler.handleValidationReport(request, Direction.RESPONSE, responseBody, result);
124+
validationReportHandler.handleValidationReport(request, response, Direction.RESPONSE, responseBody, result);
116125
reportValidationHeartbeat();
117126
return buildValidationResult(result);
118127
} catch (Exception e) {

‎openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,30 @@
11
package com.getyourguide.openapi.validation.core;
22

33
import com.atlassian.oai.validator.report.ValidationReport;
4-
import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions;
54
import com.getyourguide.openapi.validation.api.log.LogLevel;
65
import com.getyourguide.openapi.validation.api.log.ViolationLogger;
76
import com.getyourguide.openapi.validation.api.metrics.MetricsReporter;
87
import com.getyourguide.openapi.validation.api.model.Direction;
98
import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
109
import com.getyourguide.openapi.validation.api.model.RequestMetaData;
10+
import com.getyourguide.openapi.validation.api.model.ResponseMetaData;
11+
import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions;
1112
import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler;
1213
import io.swagger.v3.oas.models.parameters.Parameter;
1314
import java.util.Optional;
15+
import javax.annotation.Nullable;
1416
import lombok.AllArgsConstructor;
1517

1618
@AllArgsConstructor
1719
public class ValidationReportHandler {
1820
private final ValidationReportThrottler throttleHelper;
1921
private final ViolationLogger logger;
2022
private final MetricsReporter metrics;
21-
private final ViolationExclusions violationExclusions;
23+
private final InternalViolationExclusions violationExclusions;
2224

2325
public void handleValidationReport(
2426
RequestMetaData request,
27+
@Nullable ResponseMetaData response,
2528
Direction direction,
2629
String body,
2730
ValidationReport result
@@ -30,8 +33,8 @@ public void handleValidationReport(
3033
result
3134
.getMessages()
3235
.stream()
33-
.map(message -> buildOpenApiViolation(message, request, body, direction))
34-
.filter(violation -> !isViolationExcluded(violation))
36+
.map(message -> buildOpenApiViolation(message, request, response, body, direction))
37+
.filter(violation -> !violationExclusions.isExcluded(violation))
3538
.forEach(violation -> throttleHelper.throttle(violation, () -> logValidationError(violation)));
3639
}
3740
}
@@ -44,6 +47,7 @@ private void logValidationError(OpenApiViolation openApiViolation) {
4447
private OpenApiViolation buildOpenApiViolation(
4548
ValidationReport.Message message,
4649
RequestMetaData request,
50+
@Nullable ResponseMetaData response,
4751
String body,
4852
Direction direction
4953
) {
@@ -75,20 +79,12 @@ private OpenApiViolation buildOpenApiViolation(
7579
.instance(pointersInstance)
7680
.parameter(parameterName)
7781
.schema(getPointersSchema(message))
78-
.responseStatus(getResponseStatus(message))
82+
.responseStatus(getResponseStatus(response, message))
7983
.logMessage(logMessage)
8084
.message(message.getMessage())
8185
.build();
8286
}
8387

84-
private boolean isViolationExcluded(OpenApiViolation openApiViolation) {
85-
return
86-
violationExclusions.isExcluded(openApiViolation)
87-
// If it matches more than 1, then we don't want to log a validation error
88-
|| openApiViolation.getMessage().matches(
89-
".*\\[Path '[^']+'] Instance failed to match exactly one schema \\(matched [1-9][0-9]* out of \\d\\).*");
90-
}
91-
9288
private static Optional<String> getPointersInstance(ValidationReport.Message message) {
9389
return message.getContext()
9490
.flatMap(ValidationReport.MessageContext::getPointers)
@@ -119,7 +115,14 @@ private static Optional<String> getNormalizedPath(ValidationReport.Message messa
119115
.map(apiOperation -> apiOperation.getApiPath().normalised());
120116
}
121117

122-
private static Optional<Integer> getResponseStatus(ValidationReport.Message message) {
118+
private static Optional<Integer> getResponseStatus(
119+
@Nullable ResponseMetaData response,
120+
ValidationReport.Message message
121+
) {
122+
if (response != null && response.getStatusCode() != null) {
123+
return Optional.of(response.getStatusCode());
124+
}
125+
123126
return message.getContext().flatMap(ValidationReport.MessageContext::getResponseStatus);
124127
}
125128

‎openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package com.getyourguide.openapi.validation.core.exclusions;
2+
3+
import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions;
4+
import com.getyourguide.openapi.validation.api.model.Direction;
5+
import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
6+
import lombok.AllArgsConstructor;
7+
8+
@AllArgsConstructor
9+
public class InternalViolationExclusions {
10+
private final ViolationExclusions customViolationExclusions;
11+
12+
public boolean isExcluded(OpenApiViolation violation) {
13+
return falsePositive404(violation)
14+
|| falsePositive400(violation)
15+
|| customViolationExclusions.isExcluded(violation)
16+
// If it matches more than 1, then we don't want to log a validation error
17+
|| violation.getMessage().matches(
18+
".*\\[Path '[^']+'] Instance failed to match exactly one schema \\(matched [1-9][0-9]* out of \\d\\).*");
19+
}
20+
21+
private boolean falsePositive404(OpenApiViolation violation) {
22+
return "validation.request.path.missing".equals(violation.getRule())
23+
&& (
24+
violation.getDirection() == Direction.REQUEST
25+
|| (violation.getDirection() == Direction.RESPONSE && violation.getResponseStatus().orElse(0) == 404)
26+
);
27+
}
28+
29+
private boolean falsePositive400(OpenApiViolation violation) {
30+
return violation.getDirection() == Direction.REQUEST && violation.getResponseStatus().orElse(0) == 400;
31+
}
32+
}

‎openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidatorTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public void setup() {
4444
public void testWhenThreadPoolExecutorRejectsExecutionThenItShouldNotThrow() {
4545
Mockito.doThrow(new RejectedExecutionException()).when(threadPoolExecutor).execute(any());
4646

47-
openApiRequestValidator.validateRequestObjectAsync(mock(), null);
47+
openApiRequestValidator.validateRequestObjectAsync(mock(), null, null);
4848
}
4949

5050
@Test

‎openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/ValidationReportHandlerTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@
88
import static org.mockito.Mockito.when;
99

1010
import com.atlassian.oai.validator.report.ValidationReport;
11-
import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions;
1211
import com.getyourguide.openapi.validation.api.log.ViolationLogger;
1312
import com.getyourguide.openapi.validation.api.metrics.MetricsReporter;
1413
import com.getyourguide.openapi.validation.api.model.Direction;
1514
import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
1615
import com.getyourguide.openapi.validation.api.model.RequestMetaData;
16+
import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions;
1717
import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler;
1818
import io.swagger.v3.oas.models.parameters.Parameter;
1919
import java.net.URI;
@@ -35,7 +35,7 @@ public void setUp() {
3535
throttleHelper = mock();
3636
logger = mock();
3737
MetricsReporter metrics = mock();
38-
ViolationExclusions violationExclusions = mock();
38+
InternalViolationExclusions violationExclusions = mock();
3939

4040
validationReportHandler = new ValidationReportHandler(throttleHelper, logger, metrics, violationExclusions);
4141
}
@@ -46,7 +46,7 @@ public void testWhenParameterNameIsPresentThenItShouldAddItToTheMessage() {
4646
var request = mockRequestMetaData();
4747
var validationReport = mockValidationReport("parameterName");
4848

49-
validationReportHandler.handleValidationReport(request, Direction.REQUEST, null, validationReport);
49+
validationReportHandler.handleValidationReport(request, null, Direction.REQUEST, null, validationReport);
5050

5151
var argumentCaptor = ArgumentCaptor.forClass(OpenApiViolation.class);
5252
verify(logger).log(argumentCaptor.capture());

‎openapi-validation-core/src/test/java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
package com.getyourguide.openapi.validation.core.exclusions;
2+
3+
import static org.junit.jupiter.api.Assertions.assertFalse;
4+
import static org.junit.jupiter.api.Assertions.assertTrue;
5+
import static org.mockito.ArgumentMatchers.any;
6+
import static org.mockito.Mockito.mock;
7+
import static org.mockito.Mockito.when;
8+
9+
import com.getyourguide.openapi.validation.api.exclusions.ViolationExclusions;
10+
import com.getyourguide.openapi.validation.api.model.Direction;
11+
import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
12+
import java.util.Optional;
13+
import org.junit.jupiter.api.BeforeEach;
14+
import org.junit.jupiter.api.Test;
15+
16+
public class InternalViolationExclusionsTest {
17+
private ViolationExclusions customViolationExclusions;
18+
private InternalViolationExclusions violationExclusions;
19+
20+
@BeforeEach
21+
public void setup() {
22+
customViolationExclusions = mock();
23+
violationExclusions = new InternalViolationExclusions(customViolationExclusions);
24+
}
25+
26+
@Test
27+
public void testWhenViolationThenViolationNotExcluded() {
28+
when(customViolationExclusions.isExcluded(any())).thenReturn(false);
29+
30+
checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 404));
31+
checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 400));
32+
checkViolationNotExcluded(buildSimpleViolation(Direction.REQUEST, 200));
33+
checkViolationNotExcluded(buildSimpleViolation(Direction.REQUEST, null));
34+
checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 200));
35+
}
36+
37+
private static OpenApiViolation buildSimpleViolation(Direction direction, Integer responseStatus) {
38+
return OpenApiViolation.builder()
39+
.direction(direction)
40+
.rule("validation." + (direction == Direction.REQUEST ? "request" : "response") + ".something")
41+
.responseStatus(responseStatus != null ? Optional.of(responseStatus) : Optional.empty())
42+
.message("Some violation message")
43+
.build();
44+
}
45+
46+
@Test
47+
public void testWhenCustomViolationExclusionThenViolationExcluded() {
48+
when(customViolationExclusions.isExcluded(any())).thenReturn(true);
49+
50+
checkViolationExcluded(OpenApiViolation.builder().build());
51+
}
52+
53+
@Test
54+
public void testWhenInstanceFailedToMatchExactlyOneThenViolationExcluded() {
55+
when(customViolationExclusions.isExcluded(any())).thenReturn(false);
56+
57+
checkViolationExcluded(OpenApiViolation.builder()
58+
.message("[Path '/v1/endpoint'] Instance failed to match exactly one schema (matched 2 out of 4)").build());
59+
}
60+
61+
@Test
62+
public void testWhen404ResponseWithApiPathNotSpecifiedThenViolationExcluded() {
63+
when(customViolationExclusions.isExcluded(any())).thenReturn(false);
64+
65+
checkViolationExcluded(OpenApiViolation.builder()
66+
.direction(Direction.RESPONSE)
67+
.rule("validation.request.path.missing")
68+
.responseStatus(Optional.of(404))
69+
.message("No API path found that matches request '/nothing'")
70+
.build());
71+
}
72+
73+
@Test
74+
public void testWhenRequestWithApiPathNotSpecifiedThenViolationExcluded() {
75+
when(customViolationExclusions.isExcluded(any())).thenReturn(false);
76+
77+
checkViolationExcluded(OpenApiViolation.builder()
78+
.direction(Direction.REQUEST)
79+
.rule("validation.request.path.missing")
80+
.responseStatus(Optional.empty())
81+
.message("No API path found that matches request '/nothing'")
82+
.build());
83+
}
84+
85+
@Test
86+
public void testWhenRequestViolationsAnd400ThenViolationExcluded() {
87+
when(customViolationExclusions.isExcluded(any())).thenReturn(false);
88+
89+
checkViolationExcluded(OpenApiViolation.builder()
90+
.direction(Direction.REQUEST)
91+
.responseStatus(Optional.of(400))
92+
.message("")
93+
.build());
94+
}
95+
96+
private void checkViolationNotExcluded(OpenApiViolation violation) {
97+
var isExcluded = violationExclusions.isExcluded(violation);
98+
99+
assertFalse(isExcluded);
100+
}
101+
102+
private void checkViolationExcluded(OpenApiViolation violation) {
103+
var isExcluded = violationExclusions.isExcluded(violation);
104+
105+
assertTrue(isExcluded);
106+
}
107+
}

‎spring-boot-starter/spring-boot-starter-core/src/main/java/com/getyourguide/openapi/validation/autoconfigure/LibraryAutoConfiguration.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import com.getyourguide.openapi.validation.core.OpenApiInteractionValidatorFactory;
1818
import com.getyourguide.openapi.validation.core.OpenApiRequestValidator;
1919
import com.getyourguide.openapi.validation.core.ValidationReportHandler;
20+
import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions;
2021
import com.getyourguide.openapi.validation.core.throttle.RequestBasedValidationReportThrottler;
2122
import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottler;
2223
import com.getyourguide.openapi.validation.core.throttle.ValidationReportThrottlerNone;
@@ -81,7 +82,7 @@ public ValidationReportHandler validationReportHandler(
8182
validationReportThrottler,
8283
logger,
8384
metricsReporter,
84-
violationExclusions.orElseGet(NoViolationExclusions::new)
85+
new InternalViolationExclusions(violationExclusions.orElseGet(NoViolationExclusions::new))
8586
);
8687
}
8788

0 commit comments

Comments
 (0)