From 3710a41879717441884c75262015c536819a34c2 Mon Sep 17 00:00:00 2001 From: Pamela Dragosh Date: Fri, 21 Feb 2020 12:48:49 -0500 Subject: [PATCH] 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 --- .../components/PolicyDbDaoTransactionInstance.java | 5 +- .../rest/policycontroller/PolicyCreation.java | 10 +++- .../rest/policycontroller/PolicyCreationTest.java | 67 +++++++++++++--------- 3 files changed, 51 insertions(+), 31 deletions(-) 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 67214acf2..e694f7e0b 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 @@ 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 29b244046..f2f2e8dbc 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 @@ 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 c182b9bb5..953fff35c 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.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 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 choices = new ArrayList(); + List choices = new ArrayList<>(); policyData.setRuleAlgorithmschoices(choices); - assertThatCode(() -> creation.savePolicy(policyData, response)).doesNotThrowAnyException(); + responseEntity = creation.savePolicy(policyData, response); + assertThat(responseEntity).isNotNull(); policyData.setPolicyType("Decision"); - List settings = new ArrayList(); + List settings = new ArrayList<>(); policyData.setSettings(settings); - assertThatThrownBy(() -> creation.savePolicy(policyData, response)) - .isInstanceOf(IllegalArgumentException.class); + responseEntity = creation.savePolicy(policyData, response); + assertThat(responseEntity).isNotNull(); } } -- 2.16.6