Better error handling for decisions 49/103549/4
authorPamela Dragosh <pdragosh@research.att.com>
Wed, 11 Mar 2020 18:06:42 +0000 (14:06 -0400)
committerPamela Dragosh <pdragosh@research.att.com>
Thu, 12 Mar 2020 13:12:22 +0000 (09:12 -0400)
Throw exceptions when requests cannot be created and return
error information back. Consolidated some code to avoid sonar
duplication issues.

Companion review to https://gerrit.onap.org/r/c/policy/models/+/103548

Issue-ID: POLICY-2242
Change-Id: Ic873af933dab82e3aeef6335f55939666be20385
Signed-off-by: Pamela Dragosh <pdragosh@research.att.com>
19 files changed:
applications/common/src/main/java/org/onap/policy/pdp/xacml/application/common/ToscaPolicyTranslator.java
applications/common/src/main/java/org/onap/policy/pdp/xacml/application/common/std/StdBaseTranslator.java
applications/common/src/main/java/org/onap/policy/pdp/xacml/application/common/std/StdCombinedPolicyResultsTranslator.java
applications/common/src/main/java/org/onap/policy/pdp/xacml/application/common/std/StdMatchableTranslator.java
applications/common/src/main/java/org/onap/policy/pdp/xacml/application/common/std/StdXacmlApplicationServiceProvider.java
applications/common/src/test/java/org/onap/policy/pdp/xacml/application/common/TestUtilsCommon.java
applications/common/src/test/java/org/onap/policy/pdp/xacml/application/common/std/StdBaseTranslatorTest.java
applications/common/src/test/java/org/onap/policy/pdp/xacml/application/common/std/StdCombinedPolicyResultsTranslatorTest.java
applications/common/src/test/java/org/onap/policy/pdp/xacml/application/common/std/StdMatchableTranslatorTest.java
applications/common/src/test/java/org/onap/policy/pdp/xacml/application/common/std/StdXacmlApplicationServiceProviderTest.java
applications/guard/src/main/java/org/onap/policy/xacml/pdp/application/guard/CoordinationGuardTranslator.java
applications/guard/src/main/java/org/onap/policy/xacml/pdp/application/guard/GuardPolicyRequest.java
applications/guard/src/main/java/org/onap/policy/xacml/pdp/application/guard/GuardTranslator.java
applications/guard/src/test/java/org/onap/policy/xacml/pdp/application/guard/CoordinationTest.java
applications/guard/src/test/java/org/onap/policy/xacml/pdp/application/guard/GuardPolicyRequestTest.java
applications/monitoring/src/main/java/org/onap/policy/xacml/pdp/application/monitoring/MonitoringPdpApplication.java
applications/native/src/main/java/org/onap/policy/xacml/pdp/application/nativ/NativePdpApplicationTranslator.java
applications/optimization/src/main/java/org/onap/policy/xacml/pdp/application/optimization/OptimizationPdpApplication.java
main/src/main/java/org/onap/policy/pdpx/main/rest/provider/DecisionProvider.java

index 47ff70f..32b950b 100644 (file)
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * ONAP
  * ================================================================================
- * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -49,7 +49,7 @@ public interface ToscaPolicyTranslator {
      * @param request Incoming DecisionRequest
      * @return Xacml Request object
      */
-    Request convertRequest(DecisionRequest request);
+    Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException;
 
     /**
      * Implement this method to convert a Xacml Response
index 34936b0..3ac57b7 100644 (file)
@@ -69,7 +69,7 @@ public abstract class StdBaseTranslator implements ToscaPolicyTranslator {
     }
 
     @Override
-    public Request convertRequest(DecisionRequest request) {
+    public Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException {
         return null;
     }
 
@@ -99,9 +99,10 @@ public abstract class StdBaseTranslator implements ToscaPolicyTranslator {
                 scanAdvice(xacmlResult.getAssociatedAdvice(), decisionResponse);
             } else {
                 //
-                // TODO we have to return an ErrorResponse object instead
+                // Return error information back
                 //
-                decisionResponse.setStatus("A better error message");
+                decisionResponse.setStatus("error");
+                decisionResponse.setMessage(xacmlResult.getStatus().getStatusMessage());
             }
         }
 
index aba5e25..d966182 100644 (file)
@@ -127,17 +127,13 @@ public class StdCombinedPolicyResultsTranslator extends StdBaseTranslator {
     }
 
     @Override
-    public Request convertRequest(DecisionRequest request) {
+    public Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException {
         LOGGER.info("Converting Request {}", request);
         try {
             return RequestParser.parseRequest(StdCombinedPolicyRequest.createInstance(request));
         } catch (IllegalArgumentException | IllegalAccessException | DataTypeException e) {
-            LOGGER.error("Failed to convert DecisionRequest", e);
+            throw new ToscaPolicyConversionException("Failed to parse request", e);
         }
-        //
-        // TODO throw exception
-        //
-        return null;
     }
 
     /**
index d5e2a39..7ca995e 100644 (file)
@@ -100,17 +100,13 @@ public class StdMatchableTranslator  extends StdBaseTranslator {
     }
 
     @Override
-    public Request convertRequest(DecisionRequest request) {
+    public Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException {
         LOGGER.info("Converting Request {}", request);
         try {
             return StdMatchablePolicyRequest.createInstance(request);
         } catch (XacmlApplicationException e) {
-            LOGGER.error("Failed to convert DecisionRequest", e);
+            throw new ToscaPolicyConversionException("Failed to convert DecisionRequest", e);
         }
-        //
-        // TODO throw exception
-        //
-        return null;
     }
 
     /**
index 1127165..0fdd3a9 100644 (file)
@@ -232,7 +232,16 @@ public abstract class StdXacmlApplicationServiceProvider implements XacmlApplica
         //
         // Convert to a XacmlRequest
         //
-        Request xacmlRequest = this.getTranslator().convertRequest(request);
+        Request xacmlRequest;
+        try {
+            xacmlRequest = this.getTranslator().convertRequest(request);
+        } catch (ToscaPolicyConversionException e) {
+            LOGGER.error("Failed to convert request", e);
+            DecisionResponse response = new DecisionResponse();
+            response.setStatus("error");
+            response.setMessage(e.getLocalizedMessage());
+            return Pair.of(response, null);
+        }
         //
         // Now get a decision
         //
index ab5dde2..026bea9 100644 (file)
@@ -112,9 +112,11 @@ public class TestUtilsCommon {
      * @param policyIds Collection of IdReference objects
      * @return Response object
      */
-    public static Response createXacmlResponse(StatusCode code, Decision decision, Collection<Obligation> obligations,
+    public static Response createXacmlResponse(StatusCode code, String message, Decision decision, 
+            Collection<Obligation> obligations,
             Collection<IdReference> policyIds) {
-        StdStatus status = new StdStatus(code);
+
+        StdStatus status = new StdStatus(code, message);
 
         StdMutableResult result = new StdMutableResult(decision, status);
         result.addObligations(obligations);
index 8039a9c..8e69258 100644 (file)
@@ -120,7 +120,7 @@ public class StdBaseTranslatorTest {
     }
 
     @Test
-    public void test() throws ParseException {
+    public void testTranslatorNormalFlow() throws Exception {
         StdBaseTranslator translator = new MyStdBaseTranslator();
         assertNotNull(translator);
         assertThatThrownBy(() -> translator.convertPolicy(null)).isInstanceOf(ToscaPolicyConversionException.class);
@@ -176,7 +176,7 @@ public class StdBaseTranslatorTest {
         ids.put("onap.policies.Test", "1.0.0");
         Collection<IdReference> policyIds = TestUtilsCommon.createPolicyIdList(ids);
 
-        Response xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK,
+        Response xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK, null,
                 Decision.PERMIT, Arrays.asList(obligation), policyIds);
 
         DecisionResponse decision = translator.convertResponse(xacmlResponse);
@@ -210,7 +210,7 @@ public class StdBaseTranslatorTest {
         ids.put("onap.policies.Test", "1.0.0");
         Collection<IdReference> policyIds = TestUtilsCommon.createPolicyIdList(ids);
 
-        Response xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK,
+        Response xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK, null,
                 Decision.PERMIT, Arrays.asList(obligation), policyIds);
 
         DecisionResponse decision = translator.convertResponse(xacmlResponse);
@@ -220,28 +220,25 @@ public class StdBaseTranslatorTest {
         assertThat(decision.getPolicies()).isNotNull();
         assertThat(decision.getPolicies().size()).isEqualTo(0);
 
-        //
-        // This will need more work when I fix
-        // the convertResponse
-        //
-
         Obligation badObligation = TestUtilsCommon.createXacmlObligation(
                 ToscaDictionary.ID_OBLIGATION_REST_BODY.stringValue(),
                 Arrays.asList(assignmentBadPolicy, assignmentUnknown));
 
-        xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_MISSING_ATTRIBUTE,
+        xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_MISSING_ATTRIBUTE, null,
                 Decision.PERMIT, Arrays.asList(badObligation), policyIds);
 
         decision = translator.convertResponse(xacmlResponse);
 
         assertNotNull(decision);
 
-        xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK,
-                Decision.DENY, Arrays.asList(badObligation), policyIds);
+        xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_PROCESSING_ERROR,
+                "Bad obligation", Decision.DENY, Arrays.asList(badObligation), policyIds);
 
         decision = translator.convertResponse(xacmlResponse);
 
         assertNotNull(decision);
+        assertThat(decision.getStatus()).isEqualTo("error");
+        assertThat(decision.getMessage()).isEqualTo("Bad obligation");
     }
 
     private class MyStdBaseTranslator extends StdBaseTranslator {
index 6ff7a7f..42c13d9 100644 (file)
@@ -108,7 +108,7 @@ public class StdCombinedPolicyResultsTranslatorTest {
         ids.put("onap.policies.Test", "1.0.0");
         Collection<IdReference> policyIds = TestUtilsCommon.createPolicyIdList(ids);
 
-        Response xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK,
+        Response xacmlResponse = TestUtilsCommon.createXacmlResponse(StdStatusCode.STATUS_CODE_OK, null,
                 Decision.PERMIT, Arrays.asList(obligation), policyIds);
 
         DecisionResponse decision = translator.convertResponse(xacmlResponse);
@@ -148,7 +148,7 @@ public class StdCombinedPolicyResultsTranslatorTest {
     }
 
     @Test
-    public void testDecision() {
+    public void testDecision() throws ToscaPolicyConversionException {
         StdCombinedPolicyResultsTranslator translator = new StdCombinedPolicyResultsTranslator();
 
         DecisionRequest decision = new DecisionRequest();
index aeb4cf8..e191a08 100644 (file)
@@ -237,7 +237,7 @@ public class StdMatchableTranslatorTest {
                 Collection<IdReference> policyIds = TestUtilsCommon.createPolicyIdList(ids);
 
                 com.att.research.xacml.api.Response xacmlResponse = TestUtilsCommon.createXacmlResponse(
-                        StdStatusCode.STATUS_CODE_OK, Decision.PERMIT,
+                        StdStatusCode.STATUS_CODE_OK, null, Decision.PERMIT,
                         Arrays.asList(obligation1, obligation2, obligation3), policyIds);
                 //
                 // Test the response
index e7ea049..c95d3ca 100644 (file)
@@ -1,6 +1,6 @@
 /*-
  * ============LICENSE_START=======================================================
- * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -255,7 +255,7 @@ public class StdXacmlApplicationServiceProviderTest {
     }
 
     @Test
-    public void testMakeDecision() {
+    public void testMakeDecision() throws ToscaPolicyConversionException {
         prov.createEngine(null);
 
         DecisionRequest decreq = mock(DecisionRequest.class);
index 60fac49..f1006c6 100644 (file)
@@ -99,9 +99,8 @@ public class CoordinationGuardTranslator implements ToscaPolicyTranslator {
      * the one in LegacyGuardTranslator is used.
      */
     @Override
-    public Request convertRequest(DecisionRequest request) {
-        LOGGER.info("this convertRequest shouldn't be used");
-        return null;
+    public Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException {
+        throw new ToscaPolicyConversionException("this convertRequest shouldn't be used");
     }
 
     /**
index 51d8f0e..1e3e915 100644 (file)
@@ -34,13 +34,13 @@ import lombok.Setter;
 import lombok.ToString;
 
 import org.onap.policy.models.decisions.concepts.DecisionRequest;
+import org.onap.policy.pdp.xacml.application.common.ToscaPolicyConversionException;
 
 @Getter
 @Setter
 @ToString
 @XACMLRequest(ReturnPolicyIdList = true)
 public class GuardPolicyRequest {
-
     private static final String STR_GUARD = "guard";
 
     @XACMLSubject(includeInResults = true)
@@ -82,9 +82,11 @@ public class GuardPolicyRequest {
      *
      * @param decisionRequest Input DecisionRequest
      * @return StdMetadataPolicyRequest
+     * @throws ToscaPolicyConversionException If we cannot parse the request
      */
     @SuppressWarnings("unchecked")
-    public static GuardPolicyRequest createInstance(DecisionRequest decisionRequest) {
+    public static GuardPolicyRequest createInstance(DecisionRequest decisionRequest) 
+            throws ToscaPolicyConversionException {
         //
         // Create our return object
         //
@@ -133,14 +135,14 @@ public class GuardPolicyRequest {
             request.targetId = guard.get("target").toString();
         }
         if (guard.containsKey("vfCount")) {
-            //
-            // TODO this can potentially throw a NumberFormatException. Fix this to
-            // throw the exception when you fix the ConvertRequest to throw exceptions also.
-            //
-            request.vfCount = Integer.decode(guard.get("vfCount").toString());
+            try {
+                request.vfCount = Integer.decode(guard.get("vfCount").toString());
+            } catch (NumberFormatException e) {
+                throw new ToscaPolicyConversionException("Failed to decode vfCount", e);
+            }
         }
 
         return request;
     }
 
-}
+}
\ No newline at end of file
index 4365cad..fd46a98 100644 (file)
@@ -140,17 +140,13 @@ public class GuardTranslator implements ToscaPolicyTranslator {
     /**
      * Convert Request.
      */
-    public Request convertRequest(DecisionRequest request) {
+    public Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException {
         LOGGER.info("Converting Request {}", request);
         try {
             return RequestParser.parseRequest(GuardPolicyRequest.createInstance(request));
         } catch (IllegalArgumentException | IllegalAccessException | DataTypeException e) {
-            LOGGER.error("Failed to convert DecisionRequest", e);
+            throw new ToscaPolicyConversionException("Failed to convert DecisionRequest", e);
         }
-        //
-        // TODO throw exception
-        //
-        return null;
     }
 
     /**
index b5585a7..5b62f36 100644 (file)
@@ -23,6 +23,7 @@
 package org.onap.policy.xacml.pdp.application.guard;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
 import com.att.research.xacml.api.Response;
 import java.io.File;
@@ -191,7 +192,8 @@ public class CoordinationTest {
         // the application.
         //
         CoordinationGuardTranslator translator = new CoordinationGuardTranslator();
-        assertThat(translator.convertRequest(null)).isNull();
+        assertThatExceptionOfType(ToscaPolicyConversionException.class).isThrownBy(() ->
+            translator.convertRequest(null)).withMessage("this convertRequest shouldn't be used");
         assertThat(translator.convertResponse(null)).isNull();
         assertThat(CoordinationGuardTranslator.loadCoordinationDirectiveFromFile(
                 policyFolder.getRoot().getAbsolutePath() + "/nonexist.yaml")).isNull();
index b3ef3bd..41fd470 100644 (file)
 package org.onap.policy.xacml.pdp.application.guard;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
 import java.util.HashMap;
 import java.util.Map;
 import org.junit.Test;
 import org.onap.policy.models.decisions.concepts.DecisionRequest;
+import org.onap.policy.pdp.xacml.application.common.ToscaPolicyConversionException;
 
 public class GuardPolicyRequestTest {
 
     @Test
-    public void testAnomalies() {
+    public void testAnomalies() throws ToscaPolicyConversionException {
         DecisionRequest decisionRequest = new DecisionRequest();
         assertThat(GuardPolicyRequest.createInstance(decisionRequest)).isNotNull();
 
@@ -82,6 +84,12 @@ public class GuardPolicyRequestTest {
         resources.put("guard", guard);
         decisionRequest.setResource(resources);
         assertThat(GuardPolicyRequest.createInstance(decisionRequest)).isNotNull();
+
+        guard.put("vfCount", "I am not valid");
+        resources.put("guard", guard);
+        decisionRequest.setResource(resources);
+        assertThatExceptionOfType(ToscaPolicyConversionException.class).isThrownBy(() ->
+            GuardPolicyRequest.createInstance(decisionRequest));
     }
 
 }
index 436028f..5c41a9e 100644 (file)
@@ -22,7 +22,6 @@
 
 package org.onap.policy.xacml.pdp.application.monitoring;
 
-import com.att.research.xacml.api.Request;
 import com.att.research.xacml.api.Response;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -109,17 +108,10 @@ public class MonitoringPdpApplication extends StdXacmlApplicationServiceProvider
     public Pair<DecisionResponse, Response> makeDecision(DecisionRequest request,
             Map<String, String[]> requestQueryParams) {
         //
-        // Convert to a XacmlRequest
+        // Make the decision
         //
-        Request xacmlRequest = this.getTranslator().convertRequest(request);
-        //
-        // Now get a decision
-        //
-        Response xacmlResponse = this.xacmlDecision(xacmlRequest);
-        //
-        // Convert to a DecisionResponse
-        //
-        DecisionResponse decisionResponse = this.getTranslator().convertResponse(xacmlResponse);
+        Pair<DecisionResponse, Response> decisionPair = super.makeDecision(request, requestQueryParams);
+        DecisionResponse decisionResponse = decisionPair.getKey();
         //
         // Abbreviate results if needed
         //
@@ -137,7 +129,7 @@ public class MonitoringPdpApplication extends StdXacmlApplicationServiceProvider
                 policy.remove("version");
             }
         }
-        return Pair.of(decisionResponse, xacmlResponse);
+        return decisionPair;
     }
 
     @Override
index 98a1c65..546c29e 100644 (file)
@@ -95,11 +95,8 @@ public class NativePdpApplicationTranslator implements ToscaPolicyTranslator {
     }
 
     @Override
-    public Request convertRequest(DecisionRequest request) {
-        //
-        // We do nothing to DecisionRequest for native xacml application
-        //
-        return null;
+    public Request convertRequest(DecisionRequest request) throws ToscaPolicyConversionException {
+        throw new ToscaPolicyConversionException("Do not call native convertRequest");
     }
 
     @Override
index ee1ccdc..5a8978f 100644 (file)
@@ -25,7 +25,6 @@ package org.onap.policy.xacml.pdp.application.optimization;
 import com.att.research.xacml.api.Advice;
 import com.att.research.xacml.api.AttributeAssignment;
 import com.att.research.xacml.api.Decision;
-import com.att.research.xacml.api.Request;
 import com.att.research.xacml.api.Response;
 import com.att.research.xacml.api.Result;
 import java.nio.file.Path;
@@ -180,25 +179,16 @@ public class OptimizationPdpApplication extends StdXacmlApplicationServiceProvid
             }
         }
         //
-        // Convert to a XacmlRequest
+        // Make the decision
         //
-        Request xacmlRequest = this.getTranslator().convertRequest(request);
-        //
-        // Now get a decision
-        //
-        Response xacmlResponse = this.xacmlDecision(xacmlRequest);
-        //
-        // Convert to a DecisionResponse
-        //
-        Pair<DecisionResponse, Response> returnPair = Pair.of(this.getTranslator().convertResponse(xacmlResponse),
-                xacmlResponse);
+        Pair<DecisionResponse, Response> decisionPair = super.makeDecision(request, requestQueryParams);
         //
         // Add back in advice from subscriber
         //
         if (xacmlSubscriberResponse != null) {
-            addSubscriberAdvice(xacmlSubscriberResponse, returnPair.getLeft());
+            addSubscriberAdvice(xacmlSubscriberResponse, decisionPair.getLeft());
         }
-        return returnPair;
+        return decisionPair;
     }
 
     @Override
index 1c83b05..24d0328 100644 (file)
@@ -1,6 +1,6 @@
 /*-
  * ============LICENSE_START=======================================================
- * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -112,6 +112,10 @@ public class DecisionProvider {
     }
 
     private void calculateStatistic(Response xacmlResponse) {
+        if (xacmlResponse == null) {
+            XacmlPdpStatisticsManager.getCurrent().updateErrorCount();
+            return;
+        }
         for (Result result : xacmlResponse.getResults()) {
             switch (result.getDecision()) {
                 case PERMIT: