> On Sept. 19, 2022, 1:24 a.m., Abhay Kulkarni wrote: > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > > Line 107 (original), 102 (patched) > > <https://reviews.apache.org/r/74112/diff/1/?file=2269952#file2269952line108> > > > > Current code here (and in line 100) is to ensure that the call to > > ServiceDBStore.compressDeltas() - which builds the list of deltas that is > > input to this function - is returning correct set of policy-deltas. For > > example, if compressDelta does not work correctly, then there may be more > > than one record for a given policy with change-type of either UPDATE or > > DELETE ( or there may a delta for DELETE as well as CREATE of the same > > policy). These checks ensure that the value of the argument "deltas" is > > internally consistent.
Thanks for reviewing it .I did not encounter the issue where deletedPolicies is more than one .It is very much possible that record will be more than one when compressDelta does not work correctly.In this case whatever the code is present is make sense .Let me close this review .I will do some testing .If i encounter any issue , i will create the patch for that issue > On Sept. 19, 2022, 1:24 a.m., Abhay Kulkarni wrote: > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > > Line 113 (original), 108 (patched) > > <https://reviews.apache.org/r/74112/diff/1/?file=2269952#file2269952line114> > > > > For a deleted policy, there will always be exactly one element in > > deletedPolicies array. Therefore, the error condition is if that is not > > true. > > > > Please review this change, and provide a test case where the existing > > condition is shown to be incorrect. Thanks for reviewing it .I did not encounter the issue where deletedPolicies is more than one .It is very much possible that record will be more than one when compressDelta does not work correctly.In this case whatever the code is present is make sense .Let me close this review .I will do some testing .If i encounter any issue , i will create the patch for that issue - Ramachandran ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74112/#review224677 ----------------------------------------------------------- On Sept. 13, 2022, 8:56 a.m., Ramachandran Krishnan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74112/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2022, 8:56 a.m.) > > > Review request for ranger, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, > Nikhil P, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja > Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-3903 > https://issues.apache.org/jira/browse/RANGER-3903 > > > Repository: ranger > > > Description > ------- > > while (iter.hasNext()) { > RangerPolicy policy = iter.next(); > if (policyId.equals(policy.getId()) && (changeType == > RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == > RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) { > deletedPolicies.add(policy); > iter.remove(); > } > } > switch (changeType) { > case RangerPolicyDelta.CHANGE_TYPE_POLICY_CREATE: > { > if (CollectionUtils.isNotEmpty(deletedPolicies)) { > LOG.warn("Unexpected: found existing policy for > CHANGE_TYPE_POLICY_CREATE: " + Arrays.toString(deletedPolicies.toArray())); > } > break; > } > case RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE: > { > if (CollectionUtils.isEmpty(deletedPolicies) || > deletedPolicies.size() > 1) { > LOG.warn("Unexpected: found no policy or multiple policies > for CHANGE_TYPE_POLICY_UPDATE: " + > Arrays.toString(deletedPolicies.toArray())); > } > break; > } > case RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE: > { > if (CollectionUtils.isEmpty(deletedPolicies) || > deletedPolicies.size() > 1) { > LOG.warn("Unexpected: found no policy or multiple policies > for CHANGE_TYPE_POLICY_DELETE: " + > Arrays.toString(deletedPolicies.toArray())); > } > break; > } > default: > break; > } > > > > we iterate delta policies and check if this policy exists in the existing > policy, we add that to deletePolicies list. > > If a delta change type is created, we expect that it should not be in the > existing old policy which should not have happened. So the below block code > is not needed > > > If policy Id is uniqiue and found a match, we will add the break instead of > iterating the entire list > > > > while (iter.hasNext()) { > RangerPolicy policy = iter.next(); > if (policyId.equals(policy.getId()) && (changeType == > RangerPolicyDelta.CHANGE_TYPE_POLICY_DELETE || changeType == > RangerPolicyDelta.CHANGE_TYPE_POLICY_UPDATE)) { > deletedPolicies.add(policy); > iter.remove(); > } > } > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/util/RangerPolicyDeltaUtil.java > e9223fe69 > > > Diff: https://reviews.apache.org/r/74112/diff/1/ > > > Testing > ------- > > Tested the below Rest API's to make sure everything works fine > > 1. ServiceREST Rest API :GET /plugins/policies/download/{serviceName} > > 2. ServiceREST Rest API :GET /plugins/secure/policies/download/{serviceName} > > > Thanks, > > Ramachandran Krishnan > >
