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
--- 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 @@
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
--- 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 @@
} 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
--- 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.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 @@
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 @@
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 @@
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();
}
}