Policy Executor: handle errors, part 2

(fighting between IntelliJ and Checkstyle best practices)

- non-2xx responses are processed using web client exceptions
- handle unknown host exception
- upgraded spotbugs (checkstyle and related mvn plugin)
- fixed some small spotbugs due to upgrade
- added commented instructions in docker compose to enable debugging
- added some environment variables for policy executor configuration
- extract out Sleeper in stub service to achieve 100% coverage
- added cause to Policy Executor exceptions where applicable
- ignored (new) spotbug rule about catch NPE because of issue in 3pp
- ignored (new) spotbug rule about \n in string due to multiline string block

Issue-ID: CPS-2412
Change-Id: I6835a73320c436cbeea12cc7a06f15899eec7bf1
Signed-off-by: ToineSiebelink <toine.siebelink@est.tech>

5 files changed, 119 insertions, 57 deletions

diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java
index 6754965866..3c81d0f536 100644
--- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java
+++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java
@@ -39,7 +39,18 @@ public class NcmpException extends RuntimeException {
      * @param details the error details
      */
     public NcmpException(final String message, final String details) {
-        super(message);
+        this(message, details, null);
+    }
+
+    /**
+     * Constructor with cause.
+     *
+     * @param message the error message
+     * @param details the error details
+     * @param cause the cause of the exception
+     */
+    public NcmpException(final String message, final String details, final Throwable cause) {
+        super(message, cause);
         this.details = details;
     }
 
diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java
index 333c12271b..bb753b85f1 100644
--- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java
+++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java
@@ -35,8 +35,9 @@ public class PolicyExecutorException extends NcmpException {
      *
      * @param message response message
      * @param details response details
+     * @param cause   the cause of the exception
      */
-    public PolicyExecutorException(final String message, final String details) {
-        super(message, details);
+    public PolicyExecutorException(final String message, final String details, final Throwable cause) {
+        super(message, details, cause);
     }
 }
diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java
index caed28a648..af4331893d 100644
--- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java
+++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java
@@ -23,6 +23,7 @@ package org.onap.cps.ncmp.impl.data.policyexecutor;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import java.net.UnknownHostException;
 import java.time.Duration;
 import java.time.temporal.ChronoUnit;
 import java.util.Collections;
@@ -45,6 +46,7 @@ import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Service;
 import org.springframework.web.reactive.function.BodyInserters;
 import org.springframework.web.reactive.function.client.WebClient;
+import org.springframework.web.reactive.function.client.WebClientResponseException;
 
 @Slf4j
 @Service
@@ -71,6 +73,8 @@ public class PolicyExecutor {
 
     private final ObjectMapper objectMapper;
 
+    private static final Throwable NO_ERROR = null;
+
     /**
      * Use the Policy Executor to check permission for a cm write operation.
      * Wil throw an exception when the operation is not permitted (work in progress)
@@ -88,26 +92,20 @@ public class PolicyExecutor {
                                 final String changeRequestAsJson) {
         log.trace("Policy Executor Enabled: {}", enabled);
         if (enabled) {
-            ResponseEntity<JsonNode> responseEntity = null;
             try {
-                responseEntity =
-                    getPolicyExecutorResponse(yangModelCmHandle, operationType, authorization, resourceIdentifier,
-                        changeRequestAsJson);
-            } catch (final RuntimeException runtimeException) {
-                processException(runtimeException);
-            }
-            if (responseEntity == null) {
-                log.warn("No valid response from Policy Executor, ignored");
-                return;
-            }
-            if (responseEntity.getStatusCode().is2xxSuccessful()) {
-                if (responseEntity.getBody() == null) {
+                final ResponseEntity<JsonNode> responseEntity = getPolicyExecutorResponse(yangModelCmHandle,
+                                                                                          operationType,
+                                                                                          authorization,
+                                                                                          resourceIdentifier,
+                                                                                          changeRequestAsJson);
+                final JsonNode responseBody = responseEntity.getBody();
+                if (responseBody == null) {
                     log.warn("No valid response body from Policy Executor, ignored");
                     return;
                 }
-                processSuccessResponse(responseEntity.getBody());
-            } else {
-                processNon2xxResponse(responseEntity.getStatusCode().value());
+                processSuccessResponse(responseBody);
+            } catch (final RuntimeException runtimeException) {
+                processException(runtimeException);
             }
         }
     }
@@ -173,35 +171,17 @@ public class PolicyExecutor {
             .block();
     }
 
-    private void processNon2xxResponse(final int httpStatusCode) {
-        processFallbackResponse("Policy Executor returned HTTP Status code " + httpStatusCode + ".");
-    }
-
-    private void processException(final RuntimeException runtimeException) {
-        if (runtimeException.getCause() instanceof TimeoutException) {
-            processFallbackResponse("Policy Executor request timed out.");
-        } else {
-            log.warn("Request to Policy Executor failed with unexpected exception", runtimeException);
-            throw runtimeException;
-        }
-    }
-
-    private void processFallbackResponse(final String message) {
-        final String decisionId = "N/A";
-        final String decision = defaultDecision;
-        final String warning = message + " Falling back to configured default decision: " + defaultDecision;
-        log.warn(warning);
-        processDecision(decisionId, decision, warning);
-    }
-
     private static void processSuccessResponse(final JsonNode responseBody) {
         final String decisionId = responseBody.path("decisionId").asText("unknown id");
         final String decision = responseBody.path("decision").asText("unknown");
         final String messageFromPolicyExecutor = responseBody.path("message").asText();
-        processDecision(decisionId, decision, messageFromPolicyExecutor);
+        processDecision(decisionId, decision, messageFromPolicyExecutor, NO_ERROR);
     }
 
-    private static void processDecision(final String decisionId, final String decision, final String details) {
+    private static void processDecision(final String decisionId,
+                                        final String decision,
+                                        final String details,
+                                        final Throwable optionalCauseOfError) {
         log.trace("Policy Executor decision id: {} ", decisionId);
         if ("allow".equals(decision)) {
             log.trace("Operation allowed.");
@@ -209,8 +189,37 @@ public class PolicyExecutor {
             log.warn("Policy Executor decision: {}", decision);
             log.warn("Policy Executor message: {}", details);
             final String message = "Operation not allowed. Decision id " + decisionId + " : " + decision;
-            throw new PolicyExecutorException(message, details);
+            throw new PolicyExecutorException(message, details, optionalCauseOfError);
         }
     }
 
+    private void processException(final RuntimeException runtimeException) {
+        if (runtimeException instanceof WebClientResponseException) {
+            final WebClientResponseException webClientResponseException = (WebClientResponseException) runtimeException;
+            final int httpStatusCode = webClientResponseException.getStatusCode().value();
+            processFallbackResponse("Policy Executor returned HTTP Status code " + httpStatusCode + ".",
+                webClientResponseException);
+        } else {
+            final Throwable cause = runtimeException.getCause();
+            if (cause instanceof TimeoutException) {
+                processFallbackResponse("Policy Executor request timed out.", cause);
+            } else if (cause instanceof UnknownHostException) {
+                final String message
+                    = String.format("Cannot connect to Policy Executor (%s:%s).", serverAddress, serverPort);
+                processFallbackResponse(message, cause);
+            } else {
+                log.warn("Request to Policy Executor failed with unexpected exception", runtimeException);
+                throw runtimeException;
+            }
+        }
+    }
+
+    private void processFallbackResponse(final String message, final Throwable cause) {
+        final String decisionId = "N/A";
+        final String decision = defaultDecision;
+        final String warning = message + " Falling back to configured default decision: " + defaultDecision;
+        log.warn(warning);
+        processDecision(decisionId, decision, warning, cause);
+    }
+
 }
diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java
index 4bc2f10218..1d4e3c8363 100644
--- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java
+++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java
@@ -26,10 +26,10 @@ public enum LcmEventType {
 
     private final String eventName;
 
-    private final String eventTypeTemplate = "org.onap.ncmp.cmhandle-lcm-event.%s";
+    private static final String EVENT_TYPE_TEMPLATE = "org.onap.ncmp.cmhandle-lcm-event.%s";
 
     LcmEventType(final String eventName) {
-        this.eventName = String.format(eventTypeTemplate, eventName);
+        this.eventName = String.format(EVENT_TYPE_TEMPLATE, eventName);
     }
 
     public String getEventType() {
diff --git a/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy b/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy
index 46c0ddeb93..33dcf5d623 100644
--- a/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy
+++ b/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy
@@ -33,6 +33,7 @@ import org.slf4j.LoggerFactory
 import org.springframework.http.HttpStatus
 import org.springframework.http.ResponseEntity
 import org.springframework.web.reactive.function.client.WebClient
+import org.springframework.web.reactive.function.client.WebClientResponseException
 import reactor.core.publisher.Mono
 import spock.lang.Specification
 
@@ -59,6 +60,8 @@ class PolicyExecutorSpec extends Specification {
     def setup() {
         setupLogger()
         objectUnderTest.enabled = true
+        objectUnderTest.serverAddress = 'some host'
+        objectUnderTest.serverPort = 'some port'
         mockWebClient.post() >> mockRequestBodyUriSpec
         mockRequestBodyUriSpec.uri(*_) >> mockRequestBodyUriSpec
         mockRequestBodyUriSpec.header(*_) >> mockRequestBodyUriSpec
@@ -95,8 +98,8 @@ class PolicyExecutorSpec extends Specification {
     }
 
     def 'Permission check with non-2xx response and "allow" default decision.'() {
-        given: 'other http response'
-            mockResponse([], HttpStatus.I_AM_A_TEAPOT)
+        given: 'non-2xx http response'
+            mockErrorResponse()
         and: 'the configured default decision is "allow"'
             objectUnderTest.defaultDecision = 'allow'
         when: 'permission is checked for an operation'
@@ -106,8 +109,8 @@ class PolicyExecutorSpec extends Specification {
     }
 
     def 'Permission check with non-2xx response and "other" default decision.'() {
-        given: 'other http response'
-            mockResponse([], HttpStatus.I_AM_A_TEAPOT)
+        given: 'non-2xx http response'
+            def webClientException = mockErrorResponse()
         and: 'the configured default decision is NOT "allow"'
             objectUnderTest.defaultDecision = 'deny by default'
         when: 'permission is checked for an operation'
@@ -116,25 +119,23 @@ class PolicyExecutorSpec extends Specification {
             def thrownException = thrown(PolicyExecutorException)
             assert thrownException.message == 'Operation not allowed. Decision id N/A : deny by default'
             assert thrownException.details == 'Policy Executor returned HTTP Status code 418. Falling back to configured default decision: deny by default'
+        and: 'the cause is the original web client exception'
+            assert thrownException.cause == webClientException
     }
 
     def 'Permission check with invalid response from Policy Executor.'() {
         given: 'invalid response from Policy executor'
-            mockResponseSpec.toEntity(*_) >> invalidResponse
+            mockResponseSpec.toEntity(*_) >> Mono.just(new ResponseEntity<>(null, HttpStatus.OK))
         when: 'permission is checked for an operation'
             objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson)
         then: 'system logs the expected message'
-            assert getLogEntry(1) == expectedMessage
-        where: 'following invalid responses are received'
-            invalidResponse                                      || expectedMessage
-            Mono.empty()                                         || 'No valid response from Policy Executor, ignored'
-            Mono.just(new ResponseEntity<>(null, HttpStatus.OK)) || 'No valid response body from Policy Executor, ignored'
+            assert getLogEntry(1) == 'No valid response body from Policy Executor, ignored'
     }
 
     def 'Permission check with timeout exception.'() {
         given: 'a timeout during the request'
-            def cause = new TimeoutException()
-            mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(cause) }
+            def timeoutException = new TimeoutException()
+            mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(timeoutException) }
         and: 'the configured default decision is NOT "allow"'
             objectUnderTest.defaultDecision = 'deny by default'
         when: 'permission is checked for an operation'
@@ -143,6 +144,39 @@ class PolicyExecutorSpec extends Specification {
             def thrownException = thrown(PolicyExecutorException)
             assert thrownException.message == 'Operation not allowed. Decision id N/A : deny by default'
             assert thrownException.details == 'Policy Executor request timed out. Falling back to configured default decision: deny by default'
+        and: 'the cause is the original time out exception'
+            assert thrownException.cause == timeoutException
+    }
+
+    def 'Permission check with unknown host.'() {
+        given: 'a unknown host exception during the request'
+            def unknownHostException = new UnknownHostException()
+            mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(unknownHostException) }
+        and: 'the configured default decision is NOT "allow"'
+            objectUnderTest.defaultDecision = 'deny by default'
+        when: 'permission is checked for an operation'
+            objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson)
+        then: 'Policy Executor exception is thrown'
+            def thrownException = thrown(PolicyExecutorException)
+            assert thrownException.message == 'Operation not allowed. Decision id N/A : deny by default'
+            assert thrownException.details == 'Cannot connect to Policy Executor (some host:some port). Falling back to configured default decision: deny by default'
+        and: 'the cause is the original unknown host exception'
+            assert thrownException.cause == unknownHostException
+    }
+
+    def 'Permission check with #scenario exception and default decision "allow".'() {
+        given: 'a #scenario exception during the request'
+            mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(cause)}
+        and: 'the configured default decision is "allow"'
+            objectUnderTest.defaultDecision = 'allow'
+        when: 'permission is checked for an operation'
+            objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson)
+        then: 'no exception is thrown'
+            noExceptionThrown()
+        where: 'the following exceptions are thrown during the request'
+            scenario       | cause
+            'timeout'      | new TimeoutException()
+            'unknown host' | new UnknownHostException()
     }
 
     def 'Permission check with other runtime exception.'() {
@@ -180,6 +214,13 @@ class PolicyExecutorSpec extends Specification {
         mockResponseSpec.toEntity(*_) >> mono
     }
 
+    def mockErrorResponse() {
+        def webClientResponseException = Mock(WebClientResponseException)
+        webClientResponseException.getStatusCode() >> HttpStatus.I_AM_A_TEAPOT
+        mockResponseSpec.toEntity(*_) >> { throw webClientResponseException }
+        return webClientResponseException
+    }
+
     def setupLogger() {
         def logger = LoggerFactory.getLogger(PolicyExecutor)
         logger.setLevel(Level.TRACE)