Issue 310970043: Remove chronicle ID from getUpdateAcls (Closed)
DescriptionRemove chronicle ID from getUpdateAcls. Chronicle id is same as audited object id for ACL modification in ACL audit trail.
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changes as per review. #
Total comments: 3
Patch Set 3 : Changes as per review #Patch Set 4 : Rebased with master #
MessagesTotal messages: 9
Please review. https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2858: "insert into dm_audittrail_acl(r_object_id, chronicle_id, " Could have removed chronicle_id from the audittrail here. Kept it to make it to close to Documentum. Tests don't use it, can remove it if you prefer. https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3043: + "attribute_list, audited_obj_id, chronicle_id) " same here.
Sign in to reply to this message.
https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:97: .append("SELECT r_object_id, chronicle_id, audited_obj_id, ") Remove unused chronicle_id. https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:205: "Skipping already procesed ACL: {0}", modifyObjectId); Why make this change to the message? https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2858: "insert into dm_audittrail_acl(r_object_id, chronicle_id, " On 2016/09/24 01:24:43, Srinivas wrote: > Could have removed chronicle_id from the audittrail here. Kept it to make it to > close to Documentum. Tests don't use it, can remove it if you prefer. Please remove it. We don't include most fields in the tests, and certainly not ones neither the connector nor the tests need. Plus, you have an error in this insert, adding the r_object_id as the chronicle_id instead of the audited_obj_id (though you got it right further down). https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2962: public void testUpdateAclsWithMultipleChangesOnAcl() throws Exception { I don't know if it matters, but the original had all the changes on the same ACL, and this does not. Any particular reason?
Sign in to reply to this message.
Please review. https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:97: .append("SELECT r_object_id, chronicle_id, audited_obj_id, ") On 2016/09/26 20:43:31, JohnL wrote: > Remove unused chronicle_id. Done. https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:205: "Skipping already procesed ACL: {0}", modifyObjectId); On 2016/09/26 20:43:31, JohnL wrote: > Why make this change to the message? Think mentioning this is ACL is clear. Anyway, reverted back. I am ok eitherway. https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:205: "Skipping already procesed ACL: {0}", modifyObjectId); On 2016/09/26 20:43:31, JohnL wrote: > Why make this change to the message? Done. https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2858: "insert into dm_audittrail_acl(r_object_id, chronicle_id, " On 2016/09/26 20:43:31, JohnL wrote: > On 2016/09/24 01:24:43, Srinivas wrote: > > Could have removed chronicle_id from the audittrail here. Kept it to make it > to > > close to Documentum. Tests don't use it, can remove it if you prefer. > > Please remove it. We don't include most fields in the tests, and certainly not > ones neither the connector nor the tests need. Plus, you have an error in this > insert, adding the r_object_id as the chronicle_id instead of the audited_obj_id > (though you got it right further down). Done. https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2962: public void testUpdateAclsWithMultipleChangesOnAcl() throws Exception { On 2016/09/26 20:43:32, JohnL wrote: > I don't know if it matters, but the original had all the changes on the same > ACL, and this does not. Any particular reason? Earlier the test was using 4501081f80000100, 101& 102 ACL ids. Now modified to two ids, 100 & 101. One ACL has modify and delete events. Without these changes the test is same as testUpdateAclsPrincipals. Hence changed the test name as well. https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3043: + "attribute_list, audited_obj_id, chronicle_id) " On 2016/09/24 01:24:43, Srinivas wrote: > same here. Removed chronicle_id.
Sign in to reply to this message.
https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:205: "Skipping already procesed ACL: {0}", modifyObjectId); On 2016/09/26 22:54:52, Srinivas wrote: > On 2016/09/26 20:43:31, JohnL wrote: > > Why make this change to the message? > > Think mentioning this is ACL is clear. Anyway, reverted back. I am ok eitherway. Since you didn't include my message changes, I'll still put up a CL for those, so this will get replaced anyway. https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2962: public void testUpdateAclsWithMultipleChangesOnAcl() throws Exception { On 2016/09/26 22:54:52, Srinivas wrote: > On 2016/09/26 20:43:32, JohnL wrote: > > I don't know if it matters, but the original had all the changes on the same > > ACL, and this does not. Any particular reason? > > Earlier the test was using 4501081f80000100, 101& 102 ACL ids. Now modified to > two ids, 100 & 101. One ACL has modify and delete events. > > Without these changes the test is same as testUpdateAclsPrincipals. Hence > changed the test name as well. The change I would have expected here is all three events on 100. That isn't the same as testUpdateAcls (or testUpdateAclsPrincipals, which is anyway a different kind of test). Instead, you have two modified ACLs instead of one. That's what I was asking about the reasoning for. Also, be careful with the naming. I think the comparison with testMultiUpdateAcls should be more clear. Maybe just testUpdateAclsWithSameObjectId (i.e., s/Chronicle/Object/)? https://codereview.appspot.com/310970043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (left): https://codereview.appspot.com/310970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3132: public void testUpdatedPermissions_SameChronicleId() throws Exception { This test and it's related machinery above shouldn't be removed. We are still (correctly) filtering by chronicle ID in the permissions updates.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/310970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:205: "Skipping already procesed ACL: {0}", modifyObjectId); On 2016/09/26 23:21:20, JohnL wrote: > On 2016/09/26 22:54:52, Srinivas wrote: > > On 2016/09/26 20:43:31, JohnL wrote: > > > Why make this change to the message? > > > > Think mentioning this is ACL is clear. Anyway, reverted back. I am ok > eitherway. > > Since you didn't include my message changes, I'll still put up a CL for those, > so this will get replaced anyway. As discussed, changed the log messages, https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/310970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2962: public void testUpdateAclsWithMultipleChangesOnAcl() throws Exception { On 2016/09/26 23:21:20, JohnL wrote: > On 2016/09/26 22:54:52, Srinivas wrote: > > On 2016/09/26 20:43:32, JohnL wrote: > > > I don't know if it matters, but the original had all the changes on the same > > > ACL, and this does not. Any particular reason? > > > > Earlier the test was using 4501081f80000100, 101& 102 ACL ids. Now modified to > > two ids, 100 & 101. One ACL has modify and delete events. > > > > Without these changes the test is same as testUpdateAclsPrincipals. Hence > > changed the test name as well. > > The change I would have expected here is all three events on 100. That isn't the > same as testUpdateAcls (or testUpdateAclsPrincipals, which is anyway a different > kind of test). Instead, you have two modified ACLs instead of one. That's what I > was asking about the reasoning for. > > Also, be careful with the naming. I think the comparison with > testMultiUpdateAcls should be more clear. Maybe just > testUpdateAclsWithSameObjectId (i.e., s/Chronicle/Object/)? Done. https://codereview.appspot.com/310970043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (left): https://codereview.appspot.com/310970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3132: public void testUpdatedPermissions_SameChronicleId() throws Exception { On 2016/09/26 23:21:20, JohnL wrote: > This test and it's related machinery above shouldn't be removed. We are still > (correctly) filtering by chronicle ID in the permissions updates. Thanks. this is not audittrail_acl. wanted to make changes only to dm_audittrail_acl. Restored the changes. https://codereview.appspot.com/310970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3132: public void testUpdatedPermissions_SameChronicleId() throws Exception { On 2016/09/26 23:21:20, JohnL wrote: > This test and it's related machinery above shouldn't be removed. We are still > (correctly) filtering by chronicle ID in the permissions updates. Done.
Sign in to reply to this message.
Please rebase and put up another patch set. I moved some of the code you've changed. John L
Sign in to reply to this message.
Please review.
Sign in to reply to this message.
LGTM John L
Sign in to reply to this message.
Committed changes to Documentum repository https://github.com/googlegsa/documentum/commit/947a6740a3f9b8895e07e7a65f0688...
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
