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

getyourguide · Oct 18, 2023 · 6176fd2 · 6176fd2
1 parent 2f21230
commit 6176fd2
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 32 deletions.
diff --git a/openapi-validation-api/src/main/java/com/getyourguide/openapi/validation/api/Rules.java b/openapi-validation-api/src/main/java/com/getyourguide/openapi/validation/api/Rules.java
@@ -0,0 +1,13 @@
+package com.getyourguide.openapi.validation.api;
+
+public class Rules {
+    public static class Request {
+        public static final String PATH_MISSING = "validation.request.path.missing";
+        public static final String OPERATION_NOT_ALLOWED = "validation.request.operation.notAllowed";
+        public static final String BODY_SCHEMA_ONE_OF = "validation.request.body.schema.oneOf";
+    }
+
+    public static class Response {
+        public static final String BODY_SCHEMA_ONE_OF = "validation.response.body.schema.oneOf";
+    }
+}
diff --git a/...ion-api/src/main/java/com/getyourguide/openapi/validation/api/model/OpenApiViolation.java b/...ion-api/src/main/java/com/getyourguide/openapi/validation/api/model/OpenApiViolation.java
@@ -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);
+    }
 }
diff --git a/...-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java b/...-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java
@@ -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();

diff --git a/...java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java b/...java/com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusions.java
@@ -1,32 +1,35 @@
 package com.getyourguide.openapi.validation.core.exclusions;
 
+import com.getyourguide.openapi.validation.api.Rules;
 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;
 
 @AllArgsConstructor
 public class InternalViolationExclusions {
+
     private final ViolationExclusions customViolationExclusions;
 
     public boolean isExcluded(OpenApiViolation violation) {
         return falsePositive404(violation)
             || falsePositive400(violation)
+            || falsePositive405(violation)
             || customViolationExclusions.isExcluded(violation)
             || oneOfMatchesMoreThanOneSchema(violation);
     }
 
     private static boolean oneOfMatchesMoreThanOneSchema(OpenApiViolation violation) {
         return (
-            "validation.response.body.schema.oneOf".equals(violation.getRule())
-                || "validation.request.body.schema.oneOf".equals(violation.getRule())
+            Rules.Response.BODY_SCHEMA_ONE_OF.equals(violation.getRule())
+                || Rules.Request.BODY_SCHEMA_ONE_OF.equals(violation.getRule())
             )
             && violation.getMessage()
             .matches(".*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())
+        return Rules.Request.PATH_MISSING.equals(violation.getRule())
             && (
                 violation.getDirection() == Direction.REQUEST
                 || (violation.getDirection() == Direction.RESPONSE && violation.getResponseStatus().orElse(0) == 404)
@@ -36,4 +39,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
+            && Rules.Request.OPERATION_NOT_ALLOWED.equals(violation.getRule());
+    }
 }
diff --git a/.../com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java b/.../com/getyourguide/openapi/validation/core/exclusions/InternalViolationExclusionsTest.java
@@ -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;
 
@@ -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));
@@ -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()
@@ -60,7 +59,7 @@ public void testWhenInstanceFailedToMatchExactlyOneThenViolationExcluded() {
     }
 
     @Test
-    public void testWhenInstanceFailedToMatchExactlyOneWithOneOf24ThenViolationExcluded() {
+    public void whenInstanceFailedToMatchExactlyOneWithOneOf24ThenViolationExcluded() {
         when(customViolationExclusions.isExcluded(any())).thenReturn(false);
 
         checkViolationExcluded(OpenApiViolation.builder()
@@ -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());
     }

diff --git a/...yourguide/openapi/validation/core/throttle/RequestBasedValidationReportThrottlerTest.java b/...yourguide/openapi/validation/core/throttle/RequestBasedValidationReportThrottlerTest.java
@@ -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;
 
@@ -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();
     }
 }