Fix policycreation test

This test was only performing code coverage, it should have revealed
that IllegalArgument exceptions were incorrect.

Fixed bug in savePolicy when an exception being thrown has a null
message.

Added warning and error messages for debugging in the future.

Issue-ID: POLICY-1590
Change-Id: Ie32a73adbaf944f96e411a2c612cd8293382911c
Signed-off-by: Pamela Dragosh <pdragosh@research.att.com>

diff --git a/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/components/PolicyDbDaoTransactionInstance.java b/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/components/PolicyDbDaoTransactionInstance.java
index 67214ac..e694f7e 100644 (file)
--- a/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/components/PolicyDbDaoTransactionInstance.java
+++ b/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/components/PolicyDbDaoTransactionInstance.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * ONAP-PAP-REST
  * ================================================================================
- * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved.
  * Modifications Copyright (C) 2019 Nordix Foundation.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -549,6 +549,9 @@ public class PolicyDbDaoTransactionInstance implements PolicyDbDaoTransaction {
 
             IOUtils.closeQuietly(policyXmlStream);
             if (PolicyDbDao.isJunit()) {
+                if (policyDataString != null) {
+                    logger.warn("isJUnit will overwrite policyDataString");
+                }
                 // Using parentPath object to set policy data.
                 policyDataString = policy.policyAdapter.getParentPath();
             }

diff --git a/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreation.java b/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreation.java
index 29b2440..f2f2e8d 100644 (file)
--- a/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreation.java
+++ b/ONAP-PAP-REST/src/main/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreation.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * ONAP-PAP-REST
  * ================================================================================
- * Copyright (C) 2017-2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2017-2020 AT&T Intellectual Property. All rights reserved.
  * Modifications Copyright (C) 2019 Nordix Foundation.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -618,7 +618,13 @@ public class PolicyCreation extends AbstractPolicyCreation {
         } catch (Exception e) {
             LOGGER.error("Exception Occured : " + e.getMessage(), e);
             body = ERROR;
-            response.addHeader(ERROR, e.getMessage());
+            //
+            // Because we are catching any old exception instead of a dedicated exception,
+            // its possible the e.getMessage() returns a null value. You cannot add a header
+            // to the response with a null value, it will throw an exception. This is something
+            // this is undesirable.
+            //
+            response.addHeader(ERROR, (e.getMessage() == null ? "missing exception message" : e.getMessage()));
             response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
         }
         return new ResponseEntity<>(body, status);

diff --git a/ONAP-PAP-REST/src/test/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreationTest.java b/ONAP-PAP-REST/src/test/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreationTest.java
index c182b9b..953fff3 100644 (file)
--- a/ONAP-PAP-REST/src/test/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreationTest.java
+++ b/ONAP-PAP-REST/src/test/java/org/onap/policy/pap/xacml/rest/policycontroller/PolicyCreationTest.java
@@ -20,20 +20,16 @@
 
 package org.onap.policy.pap.xacml.rest.policycontroller;
 
-import static org.assertj.core.api.Assertions.assertThatCode;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
-
-import com.mockrunner.mock.web.MockHttpServletRequest;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-
 import org.hibernate.Criteria;
 import org.hibernate.Session;
 import org.hibernate.SessionFactory;
@@ -47,8 +43,10 @@ import org.onap.policy.rest.jpa.PolicyDbDaoEntity;
 import org.onap.policy.rest.jpa.PolicyVersion;
 import org.powermock.reflect.Whitebox;
 import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
 import org.springframework.http.converter.HttpMessageNotReadableException;
 import org.springframework.mock.web.MockHttpServletResponse;
+import com.mockrunner.mock.web.MockHttpServletRequest;
 
 public class PolicyCreationTest {
     @Test
@@ -66,7 +64,7 @@ public class PolicyCreationTest {
         Exception cause = new Exception("oops");
         HttpMessageNotReadableException exception = new HttpMessageNotReadableException("oops", cause);
         assertEquals(HttpStatus.BAD_REQUEST,
-            creation.messageNotReadableExceptionHandler(req, exception).getStatusCode());
+                creation.messageNotReadableExceptionHandler(req, exception).getStatusCode());
 
         HttpServletResponse response = new MockHttpServletResponse();
         PolicyRestAdapter policyData = new PolicyRestAdapter();
@@ -74,31 +72,39 @@ public class PolicyCreationTest {
         policyData.setConfigPolicyType("ClosedLoop_Fault");
         policyData.setDomainDir("domain");
         policyData.setPolicyName("policyname");
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        ResponseEntity<String> responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         version.setHigherVersion(1);
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         policyData.setEditPolicy(true);
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         policyData.setEditPolicy(false);
         version.setHigherVersion(0);
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         policyData.setEditPolicy(true);
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         version.setHigherVersion(1);
         policyData.setConfigPolicyType("Firewall Config");
-        assertThatThrownBy(() -> creation.savePolicy(policyData, response))
-            .isInstanceOf(IllegalArgumentException.class);
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         policyData.setConfigPolicyType("BRMS_Raw");
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
+
         policyData.setConfigPolicyType("BRMS_Param");
-        assertThatThrownBy(() -> creation.savePolicy(policyData, response))
-            .isInstanceOf(IllegalArgumentException.class);
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
+
         policyData.setConfigPolicyType("Base");
         Mockito.when(policyData.getRuleData()).thenReturn(new LinkedHashMap<>());
 
@@ -116,26 +122,31 @@ public class PolicyCreationTest {
         PolicyDbDaoTransaction mockTransaction = Mockito.mock(PolicyDbDaoTransaction.class);
         Mockito.when(mockPolicyDbDao.getNewTransaction()).thenReturn(mockTransaction);
 
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
+
         policyData.setConfigPolicyType("ClosedLoop_PM");
-        assertThatThrownBy(() -> creation.savePolicy(policyData, response))
-            .isInstanceOf(IllegalArgumentException.class);
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
+
         policyData.setConfigPolicyType("Micro Service");
-        assertThatThrownBy(() -> creation.savePolicy(policyData, response))
-            .isInstanceOf(IllegalArgumentException.class);
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
+
         policyData.setConfigPolicyType("Optimization");
-        assertThatThrownBy(() -> creation.savePolicy(policyData, response))
-            .isInstanceOf(IllegalArgumentException.class);
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         policyData.setPolicyType("Action");
-        List<Object> choices = new ArrayList<Object>();
+        List<Object> choices = new ArrayList<>();
         policyData.setRuleAlgorithmschoices(choices);
-        assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException();
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
 
         policyData.setPolicyType("Decision");
-        List<Object> settings = new ArrayList<Object>();
+        List<Object> settings = new ArrayList<>();
         policyData.setSettings(settings);
-        assertThatThrownBy(() -> creation.savePolicy(policyData, response))
-            .isInstanceOf(IllegalArgumentException.class);
+        responseEntity = creation.savePolicy(policyData, response);
+        assertThat(responseEntity).isNotNull();
     }
 }