From d08f20d90e3cc4beb4ff46d790bfde6249f5bf28 Mon Sep 17 00:00:00 2001 From: Pahansith Date: Mon, 24 Aug 2020 21:39:09 +0530 Subject: [PATCH] Fix code review discussions --- .../public/js/policy-create.js | 1 - .../app/units/cdmf.unit.policy.view/view.js | 2 - .../mgt/core/dao/impl/PolicyDAOImpl.java | 32 ++-- .../mgt/core/mgt/impl/PolicyManagerImpl.java | 138 +++++++----------- .../mgt/core/util/PolicyManagerUtil.java | 14 +- .../src/test/resources/sql/CreateH2TestDB.sql | 8 +- .../resources/dbscripts/cdm/postgresql.sql | 8 +- 7 files changed, 83 insertions(+), 120 deletions(-) diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.create/public/js/policy-create.js b/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.create/public/js/policy-create.js index f3cc76c3c7..8c5bf96dd9 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.create/public/js/policy-create.js +++ b/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.create/public/js/policy-create.js @@ -594,7 +594,6 @@ $(document).ready(function () { } }); - //todo $('input[type=radio][name=policy-type-radio-btn]').change(function() { if ($(this).val() === "CORRECTIVE") { $("#select-general-policy-type").addClass("hidden"); diff --git a/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.view/view.js b/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.view/view.js index 239f9c7ae8..0b25f3cdcb 100644 --- a/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.view/view.js +++ b/components/device-mgt/org.wso2.carbon.device.mgt.ui/src/main/resources/jaggeryapps/devicemgt/app/units/cdmf.unit.policy.view/view.js @@ -44,8 +44,6 @@ function onRequest(context) { var devicemgtProps = require("/app/modules/conf-reader/main.js")["conf"]; page["isCloud"] = devicemgtProps.isCloud; - - /*page["correctivePolicies"] = JSON.stringify(policyModule.getAllPoliciesByType("CORRECTIVE")["content"]);*/ return page; } \ No newline at end of file diff --git a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/dao/impl/PolicyDAOImpl.java b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/dao/impl/PolicyDAOImpl.java index 32c5464d93..e99cd3da9d 100644 --- a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/dao/impl/PolicyDAOImpl.java +++ b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/dao/impl/PolicyDAOImpl.java @@ -306,16 +306,12 @@ public class PolicyDAOImpl implements PolicyDAO { public List getCorrectiveActionsOfPolicy(int policyId) throws PolicyManagerDAOException { try { Connection conn = this.getConnection(); - String query = "SELECT * " + + String query = "SELECT ACTION_TYPE, CORRECTIVE_POLICY_ID, FEATURE_ID, POLICY_ID " + "FROM DM_POLICY_CORRECTIVE_ACTION " + "WHERE POLICY_ID = ?"; try (PreparedStatement selectStmt = conn.prepareStatement(query)) { - List correctiveActions = new ArrayList<>(); selectStmt.setInt(1, policyId); - try (ResultSet rs = selectStmt.executeQuery()) { - extractCorrectivePolicies(selectStmt, correctiveActions); - } - return correctiveActions; + return extractCorrectivePolicies(selectStmt); } } catch (SQLException e) { String msg = "Error occurred while retrieving corrective actions of policy ID " + policyId; @@ -328,12 +324,11 @@ public class PolicyDAOImpl implements PolicyDAO { public List getAllCorrectiveActions() throws PolicyManagerDAOException { try { Connection conn = this.getConnection(); - String query = "SELECT * " + + String query = "SELECT ACTION_TYPE, CORRECTIVE_POLICY_ID, FEATURE_ID, POLICY_ID " + "FROM DM_POLICY_CORRECTIVE_ACTION "; try (PreparedStatement stmt = conn.prepareStatement(query)) { List correctiveActions = new ArrayList<>(); - extractCorrectivePolicies(stmt, correctiveActions); - return correctiveActions; + return extractCorrectivePolicies(stmt); } } catch (SQLException e) { String msg = "Error occurred while retrieving all corrective actions"; @@ -342,8 +337,14 @@ public class PolicyDAOImpl implements PolicyDAO { } } - private void extractCorrectivePolicies(PreparedStatement stmt, List - correctiveActions) throws SQLException { + /** + * Extract corrective policies from DB query result + * @param stmt DB Query statement + * @return List of corrective actions queries + * @throws SQLException when a DB related issue occurs + */ + private List extractCorrectivePolicies(PreparedStatement stmt) throws SQLException { + List correctiveActions = new ArrayList<>(); try (ResultSet rs = stmt.executeQuery()) { CorrectiveAction correctiveAction; while (rs.next()) { @@ -355,6 +356,7 @@ public class PolicyDAOImpl implements PolicyDAO { correctiveActions.add(correctiveAction); } } + return correctiveActions; } @Override @@ -362,14 +364,12 @@ public class PolicyDAOImpl implements PolicyDAO { int policyId, int featureId) throws PolicyManagerDAOException { try { - boolean isFeatureIdContains = false; Connection conn = this.getConnection(); String query = "UPDATE DM_POLICY_CORRECTIVE_ACTION " + "SET CORRECTIVE_POLICY_ID = ? " + "WHERE ACTION_TYPE = ? " + "AND POLICY_ID = ? "; if (featureId != -1) { - isFeatureIdContains = true; query = query.concat("AND FEATURE_ID = ?"); } try (PreparedStatement updateStmt = conn.prepareStatement(query)) { @@ -377,7 +377,7 @@ public class PolicyDAOImpl implements PolicyDAO { updateStmt.setInt(1, correctiveAction.getPolicyId()); updateStmt.setString(2, correctiveAction.getActionType()); updateStmt.setInt(3, policyId); - if (isFeatureIdContains) { + if (featureId != -1) { updateStmt.setInt(4, featureId); } updateStmt.addBatch(); @@ -400,16 +400,14 @@ public class PolicyDAOImpl implements PolicyDAO { String query = "DELETE FROM DM_POLICY_CORRECTIVE_ACTION " + "WHERE ACTION_TYPE = ? " + "AND POLICY_ID = ? "; - boolean isFeatueIdContains = false; if (featureId != -1) { - isFeatueIdContains = true; query = query.concat("AND FEATURE_ID = ?"); } try (PreparedStatement deleteStmt = conn.prepareStatement(query)) { for (CorrectiveAction correctiveAction : correctiveActions) { deleteStmt.setString(1, correctiveAction.getActionType()); deleteStmt.setInt(2, policyId); - if (isFeatueIdContains) { + if (featureId != -1) { deleteStmt.setInt(3, featureId); } deleteStmt.addBatch(); diff --git a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/mgt/impl/PolicyManagerImpl.java b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/mgt/impl/PolicyManagerImpl.java index e0d8df80d3..7097391496 100644 --- a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/mgt/impl/PolicyManagerImpl.java +++ b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/mgt/impl/PolicyManagerImpl.java @@ -170,14 +170,6 @@ public class PolicyManagerImpl implements PolicyManager { policyDAO.addPolicyCriteriaProperties(policy.getPolicyCriterias()); } - /*if (policy.getCorrectiveActions() != null && !policy.getCorrectiveActions().isEmpty()) { - if (log.isDebugEnabled()) { - log.debug("Adding corrective actions for policy " + policy.getPolicyName() + - " having policy id " + policy.getId()); - } - policyDAO.addCorrectiveActionsOfPolicy(policy.getCorrectiveActions(), policy.getId()); - }*/ - if (policy.isActive()) { policyDAO.activatePolicy(policy.getId()); } @@ -358,6 +350,13 @@ public class PolicyManagerImpl implements PolicyManager { return policy; } + /** + * Using for update old type of corrective policies which has single corrective policy + * per single general policy + * @param policy updating new corrective policy + * @param previousPolicy previous corrective policy + * @throws PolicyManagerDAOException for errors occur while updating corrective actions + */ private void updateSingleCorrectiveActionList(Policy policy, Policy previousPolicy) throws PolicyManagerDAOException { List updatedCorrectiveActions = policy.getCorrectiveActions(); @@ -400,42 +399,30 @@ public class PolicyManagerImpl implements PolicyManager { } if (!correctiveActionsToUpdate.isEmpty()) { - try { - policyDAO.updateCorrectiveActionsOfPolicy(correctiveActionsToUpdate, - previousPolicy.getId(), -1); - } catch (PolicyManagerDAOException e) { - String msg = "Error occurred while updating corrective policies of the policy" + - " "+ previousPolicy.getId(); - log.error(msg, e); - throw new PolicyManagerDAOException(msg, e); - } + policyDAO.updateCorrectiveActionsOfPolicy(correctiveActionsToUpdate, + previousPolicy.getId(), -1); } if (!correctiveActionsToAdd.isEmpty()) { - try { - policyDAO.addCorrectiveActionsOfPolicy(correctiveActionsToAdd, - previousPolicy.getId(), -1); - } catch (PolicyManagerDAOException e) { - String msg = "Error occurred while adding new corrective policies to the " + - "policy the policy " + previousPolicy.getId(); - log.error(msg, e); - throw new PolicyManagerDAOException(msg, e); - } + policyDAO.addCorrectiveActionsOfPolicy(correctiveActionsToAdd, + previousPolicy.getId(), -1); } if (!correctiveActionsToDelete.isEmpty()) { - try { - policyDAO.deleteCorrectiveActionsOfPolicy(correctiveActionsToDelete, - previousPolicy.getId(), -1); - } catch (PolicyManagerDAOException e) { - String msg = "Error occurred while deleting corrective actions from the " + - "policy " + previousPolicy.getId(); - log.error(msg, e); - throw new PolicyManagerDAOException(msg, e); - } + policyDAO.deleteCorrectiveActionsOfPolicy(correctiveActionsToDelete, + previousPolicy.getId(), -1); } } + /** + * Using for update new type of corrective policies which has multiple corrective policies + * per single general policy + * @param updatedCorrectiveActionsMap updated corrective actions + * @param existingCorrectiveActionsMap existing corrective actions + * @param policy updating policy + * @param previousPolicy for errors occur while updating corrective actions + * @throws PolicyManagerDAOException + */ private void updateMultipleCorrectiveActions( Map> updatedCorrectiveActionsMap, Map> existingCorrectiveActionsMap, @@ -496,7 +483,6 @@ public class PolicyManagerImpl implements PolicyManager { } } - for (Integer featureId : existingCorrectiveActionsMap.keySet()) { List existingCorrectiveActions = existingCorrectiveActionsMap .get(featureId); @@ -521,51 +507,29 @@ public class PolicyManagerImpl implements PolicyManager { } if (!correctiveActionsToUpdate.isEmpty()) { - try { - for (Integer featureId : correctiveActionsToUpdate.keySet()) { - List correctiveActions = correctiveActionsToUpdate - .get(featureId); - policyDAO.updateCorrectiveActionsOfPolicy(correctiveActions, - previousPolicy.getId(), featureId); - } - } catch (PolicyManagerDAOException e) { - String msg = "Error occurred while updating corrective policies of the policy" + - " "+ previousPolicy.getId(); - log.error(msg, e); - throw new PolicyManagerDAOException(msg, e); + for (Integer featureId : correctiveActionsToUpdate.keySet()) { + List correctiveActions = correctiveActionsToUpdate + .get(featureId); + policyDAO.updateCorrectiveActionsOfPolicy(correctiveActions, + previousPolicy.getId(), featureId); } } if (!correctiveActionsToAdd.isEmpty()) { - try { - for (Integer featureId : correctiveActionsToAdd.keySet()) { - List correctiveActions = correctiveActionsToAdd - .get(featureId); - policyDAO.addCorrectiveActionsOfPolicy(correctiveActions, - previousPolicy.getId(), featureId); - } - - } catch (PolicyManagerDAOException e) { - String msg = "Error occurred while adding new corrective policies to the " + - "policy the policy " + previousPolicy.getId(); - log.error(msg, e); - throw new PolicyManagerDAOException(msg, e); + for (Integer featureId : correctiveActionsToAdd.keySet()) { + List correctiveActions = correctiveActionsToAdd + .get(featureId); + policyDAO.addCorrectiveActionsOfPolicy(correctiveActions, + previousPolicy.getId(), featureId); } } if (!correctiveActionsToDelete.isEmpty()) { - try { - for (Integer featureId : correctiveActionsToDelete.keySet()) { - List correctiveActions = correctiveActionsToDelete - .get(featureId); - policyDAO.deleteCorrectiveActionsOfPolicy(correctiveActions, - previousPolicy.getId(), featureId); - } - } catch (PolicyManagerDAOException e) { - String msg = "Error occurred while deleting corrective actions from the " + - "policy " + previousPolicy.getId(); - log.error(msg, e); - throw new PolicyManagerDAOException(msg, e); + for (Integer featureId : correctiveActionsToDelete.keySet()) { + List correctiveActions = correctiveActionsToDelete + .get(featureId); + policyDAO.deleteCorrectiveActionsOfPolicy(correctiveActions, + previousPolicy.getId(), featureId); } } } @@ -908,6 +872,12 @@ public class PolicyManagerImpl implements PolicyManager { try { Profile profile = profileManager.getProfile(policy.getProfileId()); policy.setProfile(profile); + } catch (ProfileManagementException e) { + throw new PolicyManagementException("Error occurred while getting the profile related to policy ID (" + + policyId + ")", e); + } + + try { PolicyManagementDAOFactory.openConnection(); List correctiveActionsOfPolicy = policyDAO .getCorrectiveActionsOfPolicy(policyId); @@ -922,13 +892,6 @@ public class PolicyManagerImpl implements PolicyManager { policy.setCorrectiveActions(getSingleCorrectiveAction (correctiveActionsOfPolicy, policyId)); } - } catch (ProfileManagementException e) { - throw new PolicyManagementException("Error occurred while getting the profile related to policy ID (" + - policyId + ")", e); -// } catch (SQLException e) { -// throw new PolicyManagementException("Error occurred while opening a connection to the data source", e); -// } finally { -// PolicyManagementDAOFactory.closeConnection(); } catch (PolicyManagerDAOException e) { String msg = "Error occurred while getting the corrective actions related to policy " + "ID (" + policyId + ")"; @@ -941,8 +904,6 @@ public class PolicyManagerImpl implements PolicyManager { } finally { PolicyManagementDAOFactory.closeConnection(); } - - return policy; } @@ -1525,8 +1486,15 @@ public class PolicyManagerImpl implements PolicyManager { return policyList; } + /** + * Build the list of policies which are included new and old types of corrective actions + * @param policyList queried policy list + * @param profileList queried profile list + * @throws PolicyManagerDAOException when failed to read policies from DB + * @throws GroupManagementException when failed to read policy groups from DB + */ private void buildPolicyList(List policyList, List profileList) - throws PolicyManagerDAOException, GroupManagementException, PolicyManagementException { + throws PolicyManagerDAOException, GroupManagementException { List allCorrectiveActions = policyDAO.getAllCorrectiveActions(); for (Policy policy : policyList) { String policyPayloadVersion = policy.getPolicyPayloadVersion(); @@ -1594,6 +1562,10 @@ public class PolicyManagerImpl implements PolicyManager { } } + /** + * Clear corrective action metadata values to avoid sending in payload + * @param correctiveAction list of corrective actions + */ private void clearMetaDataValues(CorrectiveAction correctiveAction) { correctiveAction.setAssociatedGeneralPolicyId(null); //avoiding send in payload correctiveAction.setFeatureId(null); //avoiding send in payload diff --git a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/util/PolicyManagerUtil.java b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/util/PolicyManagerUtil.java index 9d45e8e7b8..a431fd8744 100644 --- a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/util/PolicyManagerUtil.java +++ b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/main/java/org/wso2/carbon/policy/mgt/core/util/PolicyManagerUtil.java @@ -150,7 +150,7 @@ public class PolicyManagerUtil { PolicyManagementConstants.GENERAL_POLICY_TYPE.equals(policy.getPolicyType())) { String policyPayloadVersion = policy.getPolicyPayloadVersion(); float payloadVersion = 0f; - if (policyPayloadVersion != null && !StringUtils.isEmpty(policyPayloadVersion)) { + if (!StringUtils.isEmpty(policyPayloadVersion)) { payloadVersion = Float.parseFloat(policyPayloadVersion); } if (payloadVersion >= 2.0f) { @@ -229,8 +229,9 @@ public class PolicyManagerUtil { correctiveProfileOperation.setCode(PolicyManagementConstants.POLICY_ACTIONS); Set correctivePolicyIdSet = new HashSet<>(); try { + List correctiveActions; for (ProfileFeature feature : features) { - List correctiveActions = feature.getCorrectiveActions(); + correctiveActions = feature.getCorrectiveActions(); for (CorrectiveAction correctiveAction : correctiveActions) { if (PolicyManagementConstants.POLICY_CORRECTIVE_ACTION_TYPE .equals(correctiveAction.getActionType())) { @@ -242,13 +243,12 @@ public class PolicyManagerUtil { PolicyAdministratorPoint pap = new PolicyAdministratorPointImpl(); List allCorrectivePolicies = pap .getPolicies(PolicyManagementConstants.CORRECTIVE_POLICY_TYPE); - idLoop: for (Integer policyId : correctivePolicyIdSet) { for (Policy correctivePolicy : allCorrectivePolicies) { if (policyId == correctivePolicy.getId()) { createCorrectiveProfileOperations(correctivePolicy, correctiveProfileOperation); policyOperation.getProfileOperations().add(correctiveProfileOperation); - continue idLoop; + break; } } } @@ -287,7 +287,11 @@ public class PolicyManagerUtil { correctiveOperationList.setPayLoad(payLoad); } - + /** + * Create list of profile operations + * @param effectiveFeatures effective features of the policy + * @return List of ProfileOperation + */ public static List createProfileOperations(List effectiveFeatures) { List profileOperations = new ArrayList<>(); for (ProfileFeature feature : effectiveFeatures) { diff --git a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/test/resources/sql/CreateH2TestDB.sql b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/test/resources/sql/CreateH2TestDB.sql index 102cf838a8..1d9dda31be 100644 --- a/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/test/resources/sql/CreateH2TestDB.sql +++ b/components/policy-mgt/org.wso2.carbon.policy.mgt.core/src/test/resources/sql/CreateH2TestDB.sql @@ -206,20 +206,16 @@ CREATE TABLE IF NOT EXISTS DM_POLICY_CORRECTIVE_ACTION ( ACTION_TYPE VARCHAR(45) NOT NULL, CORRECTIVE_POLICY_ID INT(11) DEFAULT NULL, POLICY_ID INT(11) NOT NULL, - FEATURE_ID INT(11) NOT NULL, + FEATURE_ID INT(11) DEFAULT NULL, PRIMARY KEY (ID), CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION FOREIGN KEY (POLICY_ID) REFERENCES DM_POLICY (ID) ON DELETE NO ACTION - ON UPDATE NO ACTION, - CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION - FOREIGN KEY (FEATURE_ID) - REFERENCES DM_PROFILE_FEATURES (ID) - ON DELETE NO ACTION ON UPDATE NO ACTION ); + DROP TABLE IF EXISTS DM_DEVICE_POLICY; CREATE TABLE IF NOT EXISTS DM_DEVICE_POLICY ( ID INT(11) NOT NULL AUTO_INCREMENT , diff --git a/features/device-mgt/org.wso2.carbon.device.mgt.basics.feature/src/main/resources/dbscripts/cdm/postgresql.sql b/features/device-mgt/org.wso2.carbon.device.mgt.basics.feature/src/main/resources/dbscripts/cdm/postgresql.sql index 452eb34793..4b24b080d1 100644 --- a/features/device-mgt/org.wso2.carbon.device.mgt.basics.feature/src/main/resources/dbscripts/cdm/postgresql.sql +++ b/features/device-mgt/org.wso2.carbon.device.mgt.basics.feature/src/main/resources/dbscripts/cdm/postgresql.sql @@ -243,19 +243,15 @@ CREATE TABLE IF NOT EXISTS DM_POLICY_CORRECTIVE_ACTION ( ACTION_TYPE VARCHAR(45) NOT NULL, CORRECTIVE_POLICY_ID INTEGER DEFAULT NULL, POLICY_ID INTEGER NOT NULL, - FEATURE_ID INTEGER NOT NULL, + FEATURE_ID INTEGER DEFAULT NULL, CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION FOREIGN KEY (POLICY_ID) REFERENCES DM_POLICY (ID) ON DELETE NO ACTION - ON UPDATE NO ACTION, - CONSTRAINT FK_DM_POLICY_DM_POLICY_CORRECTIVE_ACTION - FOREIGN KEY (FEATURE_ID) - REFERENCES DM_PROFILE_FEATURES (ID) - ON DELETE NO ACTION ON UPDATE NO ACTION ); + CREATE TABLE IF NOT EXISTS DM_DEVICE_POLICY ( ID INTEGER NOT NULL DEFAULT NEXTVAL ('DM_DEVICE_POLICY_seq') , DEVICE_ID INTEGER NOT NULL ,