Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] Exclude violation operation.notAllowed with 405 #43

Merged
merged 9 commits into from
Oct 18, 2023
Merged

[Bugfix] Exclude violation operation.notAllowed with 405 #43

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e35102
Add failing test
pboos Oct 17, 2023
90087e2
[bugfix] 405 with `operation.notAllowed` should be excluded
pboos Oct 17, 2023
41ac31b
Fix PMD
pboos Oct 17, 2023
4f1e42b
Fix if optional is null in OpenApiViolation
pboos Oct 17, 2023
826996a
Make optional correct
pboos Oct 17, 2023
0849c96
Remove whitespace
pboos Oct 17, 2023
9bcd6ed
Extract rule constants
pboos Oct 18, 2023
487d0f7
Extract rule constants to file
pboos Oct 18, 2023
3f11f03
Separate classes by empty line
pboos Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions ...ion-api/src/main/java/com/getyourguide/openapi/validation/api/model/OpenApiViolation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,36 @@ public class OpenApiViolation {
private final RequestMetaData requestMetaData;
private final String body;
private final String rule;
private final Optional<String> operationId;
private final Optional<String> normalizedPath;
private final Optional<String> instance;
private final Optional<String> parameter;
private final Optional<String> schema;
private final Optional<Integer> responseStatus;
private final String operationId;
private final String normalizedPath;
private final String instance;
private final String parameter;
private final String schema;
private final Integer responseStatus;
private final String message;
private final String logMessage;

public Optional<String> getOperationId() {
return Optional.ofNullable(operationId);
}

public Optional<String> getNormalizedPath() {
return Optional.ofNullable(normalizedPath);
}

public Optional<String> getInstance() {
return Optional.ofNullable(instance);
}

public Optional<String> getParameter() {
return Optional.ofNullable(parameter);
}

public Optional<String> getSchema() {
return Optional.ofNullable(schema);
}

public Optional<Integer> getResponseStatus() {
return Optional.ofNullable(responseStatus);
}
}
12 changes: 6 additions & 6 deletions ...-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ private OpenApiViolation buildOpenApiViolation(
.requestMetaData(request)
.body(body)
.rule(message.getKey())
.operationId(getOperationId(message))
.normalizedPath(getNormalizedPath(message))
.instance(pointersInstance)
.parameter(parameterName)
.schema(getPointersSchema(message))
.responseStatus(getResponseStatus(response, message))
.operationId(getOperationId(message).orElse(null))
.normalizedPath(getNormalizedPath(message).orElse(null))
.instance(pointersInstance.orElse(null))
.parameter(parameterName.orElse(null))
.schema(getPointersSchema(message).orElse(null))
.responseStatus(getResponseStatus(response, message).orElse(null))
.logMessage(logMessage)
.message(message.getMessage())
.build();
Expand Down
6 changes: 6 additions & 0 deletions ...java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public class InternalViolationExclusions {
public boolean isExcluded(OpenApiViolation violation) {
return falsePositive404(violation)
|| falsePositive400(violation)
|| falsePositive405(violation)
|| customViolationExclusions.isExcluded(violation)
|| oneOfMatchesMoreThanOneSchema(violation);
}
Expand All @@ -36,4 +37,9 @@ private boolean falsePositive404(OpenApiViolation violation) {
private boolean falsePositive400(OpenApiViolation violation) {
return violation.getDirection() == Direction.REQUEST && violation.getResponseStatus().orElse(0) == 400;
}

private boolean falsePositive405(OpenApiViolation violation) {
return violation.getResponseStatus().orElse(0) == 405
&& "validation.request.operation.notAllowed".equals(violation.getRule());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the rule name here go into a constant? Are all rules defined somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not defined anywhere in swagger-request-validator. I have now extracted them here in the file at least.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will create a const file now as we also use them in other places.

}
}
42 changes: 30 additions & 12 deletions .../com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
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;

Expand All @@ -24,7 +23,7 @@ public void setup() {
}

@Test
public void testWhenViolationThenViolationNotExcluded() {
public void whenViolationThenViolationNotExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationNotExcluded(buildSimpleViolation(Direction.RESPONSE, 404));
Expand All @@ -38,20 +37,20 @@ private static OpenApiViolation buildSimpleViolation(Direction direction, Intege
return OpenApiViolation.builder()
.direction(direction)
.rule("validation." + (direction == Direction.REQUEST ? "request" : "response") + ".something")
.responseStatus(responseStatus != null ? Optional.of(responseStatus) : Optional.empty())
.responseStatus(responseStatus)
.message("Some violation message")
.build();
}

@Test
public void testWhenCustomViolationExclusionThenViolationExcluded() {
public void whenCustomViolationExclusionThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(true);

checkViolationExcluded(OpenApiViolation.builder().build());
}

@Test
public void testWhenInstanceFailedToMatchExactlyOneThenViolationExcluded() {
public void whenInstanceFailedToMatchExactlyOneThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationExcluded(OpenApiViolation.builder()
Expand All @@ -60,7 +59,7 @@ public void testWhenInstanceFailedToMatchExactlyOneThenViolationExcluded() {
}

@Test
public void testWhenInstanceFailedToMatchExactlyOneWithOneOf24ThenViolationExcluded() {
public void whenInstanceFailedToMatchExactlyOneWithOneOf24ThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationExcluded(OpenApiViolation.builder()
Expand All @@ -70,36 +69,55 @@ public void testWhenInstanceFailedToMatchExactlyOneWithOneOf24ThenViolationExclu
}

@Test
public void testWhen404ResponseWithApiPathNotSpecifiedThenViolationExcluded() {
public void when404ResponseWithApiPathNotSpecifiedThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationExcluded(OpenApiViolation.builder()
.direction(Direction.RESPONSE)
.rule("validation.request.path.missing")
.responseStatus(Optional.of(404))
.responseStatus(404)
.message("No API path found that matches request '/nothing'")
.build());
}

@Test
public void testWhenRequestWithApiPathNotSpecifiedThenViolationExcluded() {
public void whenRequestWithApiPathNotSpecifiedThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationExcluded(OpenApiViolation.builder()
.direction(Direction.REQUEST)
.rule("validation.request.path.missing")
.responseStatus(Optional.empty())
.responseStatus(null)
.message("No API path found that matches request '/nothing'")
.build());
}

@Test
public void testWhenRequestViolationsAnd400ThenViolationExcluded() {
public void whenRequestViolationsAnd400ThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationExcluded(OpenApiViolation.builder()
.direction(Direction.REQUEST)
.responseStatus(Optional.of(400))
.responseStatus(400)
.message("")
.build());
}

@Test
public void when405ResponseCodeWithOperationNotAllowedViolationThenViolationExcluded() {
when(customViolationExclusions.isExcluded(any())).thenReturn(false);

checkViolationExcluded(OpenApiViolation.builder()
.direction(Direction.REQUEST)
.rule("validation.request.operation.notAllowed")
.responseStatus(405)
.message("")
.build());

checkViolationExcluded(OpenApiViolation.builder()
.direction(Direction.RESPONSE)
.rule("validation.request.operation.notAllowed")
.responseStatus(405)
.message("")
.build());
}
Expand Down
9 changes: 4 additions & 5 deletions ...yourguide/openapi/validation/core/throttle/RequestBasedValidationReportThrottlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import com.getyourguide.openapi.validation.api.model.RequestMetaData;
import java.net.URI;
import java.util.Collections;
import java.util.Optional;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -89,10 +88,10 @@ private OpenApiViolation buildViolation(Direction direction, Request.Method meth
.requestMetaData(
new RequestMetaData(method.toString(), URI.create("https://example.com" + path), Collections.emptyMap())
)
.responseStatus(Optional.of(status))
.normalizedPath(Optional.of(path))
.instance(Optional.of(instance))
.schema(Optional.of(schema))
.responseStatus(status)
.normalizedPath(path)
.instance(instance)
.schema(schema)
.build();
}
}