refactoring to improve code quality (#67)

Author: pboos

Diff for: openapi-validation-api/src/main/java/com/getyourguide/openapi/validation/api/log/OpenApiViolationHandler.java

@@ -0,0 +1,7 @@
+package com.getyourguide.openapi.validation.api.log;
+
+import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
+
+public interface OpenApiViolationHandler {
+    void onOpenApiViolation(OpenApiViolation violation);
+}

Diff for: openapi-validation-api/src/main/java/com/getyourguide/openapi/validation/api/model/ValidationResult.java

@@ -8,5 +8,4 @@
 public class OpenApiRequestValidationConfiguration {
     private double sampleRate;
     private int validationReportThrottleWaitSeconds;
-    private boolean shouldFailOnRequestViolation;
 }

Diff for: openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidationConfiguration.java

@@ -3,15 +3,17 @@
 import com.atlassian.oai.validator.model.Request;
 import com.atlassian.oai.validator.model.SimpleRequest;
 import com.atlassian.oai.validator.model.SimpleResponse;
-import com.atlassian.oai.validator.report.ValidationReport;
+import com.getyourguide.openapi.validation.api.log.OpenApiViolationHandler;
 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.api.model.ResponseMetaData;
-import com.getyourguide.openapi.validation.api.model.ValidationResult;
+import com.getyourguide.openapi.validation.core.mapper.ValidationReportToOpenApiViolationsMapper;
 import com.getyourguide.openapi.validation.core.validator.OpenApiInteractionValidatorWrapper;
 import java.net.URLDecoder;
 import java.nio.charset.StandardCharsets;
+import java.util.List;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.ThreadPoolExecutor;
 import javax.annotation.Nullable;
@@ -22,20 +24,18 @@
 public class OpenApiRequestValidator {
     private final ThreadPoolExecutor threadPoolExecutor;
     private final OpenApiInteractionValidatorWrapper validator;
-    private final ValidationReportHandler validationReportHandler;
-    private final OpenApiRequestValidationConfiguration configuration;
+    private final ValidationReportToOpenApiViolationsMapper mapper;
 
     public OpenApiRequestValidator(
         ThreadPoolExecutor threadPoolExecutor,
-        ValidationReportHandler validationReportHandler,
         MetricsReporter metricsReporter,
         OpenApiInteractionValidatorWrapper validator,
+        ValidationReportToOpenApiViolationsMapper mapper,
         OpenApiRequestValidationConfiguration configuration
     ) {
         this.threadPoolExecutor = threadPoolExecutor;
         this.validator = validator;
-        this.validationReportHandler = validationReportHandler;
-        this.configuration = configuration;
+        this.mapper = mapper;
 
         metricsReporter.reportStartup(
             validator != null,
@@ -48,12 +48,28 @@ public boolean isReady() {
         return validator != null;
     }
 
-    public void validateRequestObjectAsync(final RequestMetaData request, @Nullable ResponseMetaData response, String requestBody) {
-        executeAsync(() -> validateRequestObject(request, response, requestBody));
+    public void validateRequestObjectAsync(
+        final RequestMetaData request,
+        @Nullable ResponseMetaData response,
+        String requestBody,
+        OpenApiViolationHandler listener
+    ) {
+        executeAsync(() -> {
+            var violations = validateRequestObject(request, response, requestBody);
+            violations.forEach(listener::onOpenApiViolation);
+        });
     }
 
-    public void validateResponseObjectAsync(final RequestMetaData request, ResponseMetaData response, final String responseBody) {
-        executeAsync(() -> validateResponseObject(request, response, responseBody));
+    public void validateResponseObjectAsync(
+        final RequestMetaData request,
+        ResponseMetaData response,
+        final String responseBody,
+        OpenApiViolationHandler listener
+    ) {
+        executeAsync(() -> {
+            var violations = validateResponseObject(request, response, responseBody);
+            violations.forEach(listener::onOpenApiViolation);
+        });
     }
 
     private void executeAsync(Runnable command) {
@@ -64,28 +80,22 @@ private void executeAsync(Runnable command) {
         }
     }
 
-    public ValidationResult validateRequestObject(final RequestMetaData request, String requestBody) {
+    public List<OpenApiViolation> validateRequestObject(final RequestMetaData request, String requestBody) {
         return validateRequestObject(request, null, requestBody);
     }
 
-    public ValidationResult validateRequestObject(
+    public List<OpenApiViolation> validateRequestObject(
         final RequestMetaData request,
         @Nullable final ResponseMetaData response,
         String requestBody
     ) {
         try {
             var simpleRequest = buildSimpleRequest(request, requestBody);
             var result = validator.validateRequest(simpleRequest);
-            // TODO this should not be done here, but currently the only way to do it -> Refactor this so that logging
-            //      is actually done in the interceptor/filter where logging can easily be skipped then.
-            if (!configuration.isShouldFailOnRequestViolation()) {
-                validationReportHandler
-                    .handleValidationReport(request, response, Direction.REQUEST, requestBody, result);
-            }
-            return buildValidationResult(result);
+            return mapper.map(result, request, response, Direction.REQUEST, requestBody);
         } catch (Exception e) {
             log.error("Could not validate request", e);
-            return ValidationResult.NOT_APPLICABLE;
+            return List.of();
         }
     }
 
@@ -108,7 +118,7 @@ private static String nullSafeUrlDecode(String value) {
         return URLDecoder.decode(value, StandardCharsets.UTF_8);
     }
 
-    public ValidationResult validateResponseObject(
+    public List<OpenApiViolation> validateResponseObject(
         final RequestMetaData request,
         final ResponseMetaData response,
         final String responseBody
@@ -126,23 +136,10 @@ public ValidationResult validateResponseObject(
                 Request.Method.valueOf(request.getMethod().toUpperCase()),
                 responseBuilder.build()
             );
-            validationReportHandler.handleValidationReport(request, response, Direction.RESPONSE, responseBody, result);
-            return buildValidationResult(result);
+            return mapper.map(result, request, response, Direction.RESPONSE, responseBody);
         } catch (Exception e) {
             log.error("Could not validate response", e);
-            return ValidationResult.NOT_APPLICABLE;
+            return List.of();
         }
     }
-
-    private ValidationResult buildValidationResult(ValidationReport validationReport) {
-        if (validationReport == null) {
-            return ValidationResult.NOT_APPLICABLE;
-        }
-
-        if (validationReport.getMessages().isEmpty()) {
-            return ValidationResult.VALID;
-        }
-
-        return ValidationResult.INVALID;
-    }
 }

Diff for: openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/OpenApiRequestValidator.java

@@ -0,0 +1,19 @@
+package com.getyourguide.openapi.validation.core.log;
+
+import com.getyourguide.openapi.validation.api.log.OpenApiViolationHandler;
+import com.getyourguide.openapi.validation.api.log.ViolationLogger;
+import com.getyourguide.openapi.validation.api.metrics.MetricsReporter;
+import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
+import lombok.AllArgsConstructor;
+
+@AllArgsConstructor
+public class DefaultOpenApiViolationHandler implements OpenApiViolationHandler {
+    private final ViolationLogger logger;
+    private final MetricsReporter metrics;
+
+    @Override
+    public void onOpenApiViolation(OpenApiViolation violation) {
+        logger.log(violation);
+        metrics.reportViolation(violation);
+    }
+}

Diff for: openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/log/DefaultOpenApiViolationHandler.java

@@ -0,0 +1,21 @@
+package com.getyourguide.openapi.validation.core.log;
+
+import com.getyourguide.openapi.validation.api.log.OpenApiViolationHandler;
+import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
+import com.getyourguide.openapi.validation.core.exclusions.InternalViolationExclusions;
+import lombok.AllArgsConstructor;
+
+@AllArgsConstructor
+public class ExclusionsOpenApiViolationHandler implements OpenApiViolationHandler {
+    private final OpenApiViolationHandler delegate;
+    private final InternalViolationExclusions violationExclusions;
+
+    @Override
+    public void onOpenApiViolation(OpenApiViolation violation) {
+        if (violationExclusions.isExcluded(violation)) {
+            return;
+        }
+
+        delegate.onOpenApiViolation(violation);
+    }
+}

Diff for: openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/log/ExclusionsOpenApiViolationHandler.java

@@ -1,5 +1,6 @@
-package com.getyourguide.openapi.validation.core.throttle;
+package com.getyourguide.openapi.validation.core.log;
 
+import com.getyourguide.openapi.validation.api.log.OpenApiViolationHandler;
 import com.getyourguide.openapi.validation.api.model.OpenApiViolation;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
@@ -8,20 +9,19 @@
 import org.joda.time.DateTime;
 
 @AllArgsConstructor
-public class RequestBasedValidationReportThrottler implements ValidationReportThrottler {
-
+public class ThrottlingOpenApiViolationHandler implements OpenApiViolationHandler {
+    private final OpenApiViolationHandler delegate;
     private final int waitSeconds;
-
     private final Map<String, DateTime> loggedMessages = new ConcurrentHashMap<>();
 
     @Override
-    public void throttle(OpenApiViolation openApiViolation, Runnable runnable) {
-        if (isThrottled(openApiViolation)) {
+    public void onOpenApiViolation(OpenApiViolation violation) {
+        if (isThrottled(violation)) {
             return;
         }
 
-        runnable.run();
-        registerLoggedMessage(openApiViolation);
+        delegate.onOpenApiViolation(violation);
+        registerLoggedMessage(violation);
     }
 
     private void registerLoggedMessage(OpenApiViolation openApiViolation) {

Diff for: openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/throttle/RequestBasedValidationReportThrottler.java renamed to openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/log/ThrottlingOpenApiViolationHandler.java

@@ -1,47 +1,29 @@
-package com.getyourguide.openapi.validation.core;
+package com.getyourguide.openapi.validation.core.mapper;
 
 import com.atlassian.oai.validator.report.ValidationReport;
 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.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.List;
 import java.util.Optional;
 import javax.annotation.Nullable;
-import lombok.AllArgsConstructor;
 
-@AllArgsConstructor
-public class ValidationReportHandler {
-    private final ValidationReportThrottler throttleHelper;
-    private final ViolationLogger logger;
-    private final MetricsReporter metrics;
-    private final InternalViolationExclusions violationExclusions;
-
-    public void handleValidationReport(
+public class ValidationReportToOpenApiViolationsMapper {
+    public List<OpenApiViolation> map(
+        ValidationReport validationReport,
         RequestMetaData request,
         @Nullable ResponseMetaData response,
         Direction direction,
-        String body,
-        ValidationReport result
+        String body
     ) {
-        if (!result.getMessages().isEmpty()) {
-            result
-                .getMessages()
-                .stream()
-                .map(message -> buildOpenApiViolation(message, request, response, body, direction))
-                .filter(violation -> !violationExclusions.isExcluded(violation))
-                .forEach(violation -> throttleHelper.throttle(violation, () -> logValidationError(violation)));
-        }
-    }
-
-    private void logValidationError(OpenApiViolation openApiViolation) {
-        logger.log(openApiViolation);
-        metrics.reportViolation(openApiViolation);
+        return validationReport
+            .getMessages()
+            .stream()
+            .map(message -> buildOpenApiViolation(message, request, response, body, direction))
+            .toList();
     }
 
     private OpenApiViolation buildOpenApiViolation(

Diff for: openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/ValidationReportHandler.java renamed to openapi-validation-core/src/main/java/com/getyourguide/openapi/validation/core/mapper/ValidationReportToOpenApiViolationsMapper.java

@@ -4,13 +4,16 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import com.atlassian.oai.validator.model.SimpleRequest;
 import com.getyourguide.openapi.validation.api.metrics.MetricsReporter;
 import com.getyourguide.openapi.validation.api.model.RequestMetaData;
+import com.getyourguide.openapi.validation.core.mapper.ValidationReportToOpenApiViolationsMapper;
 import com.getyourguide.openapi.validation.core.validator.OpenApiInteractionValidatorWrapper;
 import java.net.URI;
 import java.util.HashMap;
+import java.util.List;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.ThreadPoolExecutor;
 import org.junit.jupiter.api.BeforeEach;
@@ -29,14 +32,15 @@ public class OpenApiRequestValidatorTest {
     public void setup() {
         threadPoolExecutor = mock();
         validator = mock();
-        ValidationReportHandler validationReportHandler = mock();
         MetricsReporter metricsReporter = mock();
+        var mapper = mock(ValidationReportToOpenApiViolationsMapper.class);
+        when(mapper.map(any(), any(), any(), any(), any())).thenReturn(List.of());
 
         openApiRequestValidator = new OpenApiRequestValidator(
             threadPoolExecutor,
-            validationReportHandler,
             metricsReporter,
             validator,
+            mapper,
             mock()
         );
     }
@@ -45,7 +49,7 @@ public void setup() {
     public void testWhenThreadPoolExecutorRejectsExecutionThenItShouldNotThrow() {
         Mockito.doThrow(new RejectedExecutionException()).when(threadPoolExecutor).execute(any());
 
-        openApiRequestValidator.validateRequestObjectAsync(mock(), null, null);
+        openApiRequestValidator.validateRequestObjectAsync(mock(), null, null, mock());
     }
 
     @Test