Issue 314480043: DCTM: Changes to use chronicle id instead of object id in the DocId urls (Closed)
DescriptionChanges to use chronicle id instead of object id in the DocId urls
Patch Set 1 #
Total comments: 18
Patch Set 2 : Changes as per review. #
Total comments: 7
Patch Set 3 : Changes as per review #
MessagesTotal messages: 8
Please review the changes.
Sign in to reply to this message.
Two real issues, then mostly readability issues and my Freudian id vs ID pet peeve. John L https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1275: * @param objectId the ID of a Documentum object Change this tag and the parameter name? https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1435: logger.log(Level.FINER, "Chronicle Id: {0}", objId); s/Id/ID/ since you're changing this line. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1438: } catch (DfIdNotFoundException e) { This needs to be reworked. getObjectByQualification returns null instead of throwing DfIdNotFoundException like getObject does. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1443: logger.log(Level.FINE, "Path does not contain chronicle id: {0}", path); s/id/ID/ https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1507: if (sysObj == null) { This isn't doing anything, it's just return sysObj. Maybe just return dmSession.getObjectByQualification(...); https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1711: DocId childDocId = docIdFromPath(docIdToPath(id), objName, objId); What about switching to chronicle ID here? https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1743: logger.log(Level.FINER, "Chronicle Id: {0}; Name: {1}", s/Id/ID/ https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: String id = query.substring(query.toLowerCase(). Tie dot to lastIndexOf. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1086: lastIndexOf("i_chronicle_id = '") + 18); s/18/X.length()/, where X is either the same literal, or just declare the string as a variable (idPrefix or something). https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1088: // Documentum returns the CURRENT version. To emulate the Move this comment to before the call to rs.last() above. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1093: return getFolderBySpecification(id.substring(0, Break before "id.substring". https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1094: id.length() - 1)); Maybe put this chopping of the trailing quote into the original query.substring? https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1923: private void testDocContent(Date lastCrawled, Date lastModified, Keep this before assertDocContent to minimize diffs and make the change more obvious. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2053: public void testGetDocContent_chronicleId() throws Exception { Is this the missing test from the other CL, with multiple versions tacked on, or just a multiple versions test?
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1275: * @param objectId the ID of a Documentum object On 2017/02/15 01:36:08, JohnL wrote: > Change this tag and the parameter name? Done. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1435: logger.log(Level.FINER, "Chronicle Id: {0}", objId); On 2017/02/15 01:36:08, JohnL wrote: > s/Id/ID/ since you're changing this line. Done. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1438: } catch (DfIdNotFoundException e) { On 2017/02/15 01:36:08, JohnL wrote: > This needs to be reworked. getObjectByQualification returns null instead of > throwing DfIdNotFoundException like getObject does. Removed the try, catch block. In the other CL we are setting dmPersObj to null in catch block anyway. getObjectByQualification returns null for invalid id. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1438: } catch (DfIdNotFoundException e) { On 2017/02/15 01:36:08, JohnL wrote: > This needs to be reworked. getObjectByQualification returns null instead of > throwing DfIdNotFoundException like getObject does. Done. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1443: logger.log(Level.FINE, "Path does not contain chronicle id: {0}", path); On 2017/02/15 01:36:08, JohnL wrote: > s/id/ID/ Done. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1507: if (sysObj == null) { On 2017/02/15 01:36:08, JohnL wrote: > This isn't doing anything, it's just return sysObj. Maybe just return > dmSession.getObjectByQualification(...); Removed this method and called getObjectByQualification() directly. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1711: DocId childDocId = docIdFromPath(docIdToPath(id), objName, objId); On 2017/02/15 01:36:08, JohnL wrote: > What about switching to chronicle ID here? Yes, thanks. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1711: DocId childDocId = docIdFromPath(docIdToPath(id), objName, objId); On 2017/02/15 01:36:08, JohnL wrote: > What about switching to chronicle ID here? Done. https://codereview.appspot.com/314480043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1743: logger.log(Level.FINER, "Chronicle Id: {0}; Name: {1}", On 2017/02/15 01:36:08, JohnL wrote: > s/Id/ID/ Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: String id = query.substring(query.toLowerCase(). On 2017/02/15 01:36:08, JohnL wrote: > Tie dot to lastIndexOf. Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1086: lastIndexOf("i_chronicle_id = '") + 18); On 2017/02/15 01:36:08, JohnL wrote: > s/18/X.length()/, where X is either the same literal, or just declare the string > as a variable (idPrefix or something). Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1088: // Documentum returns the CURRENT version. To emulate the On 2017/02/15 01:36:08, JohnL wrote: > Move this comment to before the call to rs.last() above. Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1093: return getFolderBySpecification(id.substring(0, On 2017/02/15 01:36:08, JohnL wrote: > Break before "id.substring". Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1094: id.length() - 1)); On 2017/02/15 01:36:08, JohnL wrote: > Maybe put this chopping of the trailing quote into the original query.substring? Yes, thanks :) https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1094: id.length() - 1)); On 2017/02/15 01:36:08, JohnL wrote: > Maybe put this chopping of the trailing quote into the original query.substring? Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1923: private void testDocContent(Date lastCrawled, Date lastModified, On 2017/02/15 01:36:08, JohnL wrote: > Keep this before assertDocContent to minimize diffs and make the change more > obvious. Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2053: public void testGetDocContent_chronicleId() throws Exception { On 2017/02/15 01:36:08, JohnL wrote: > Is this the missing test from the other CL, with multiple versions tacked on, or > just a multiple versions test? This is in addition to the two tests in other CL. This tests by use of chronicle Id, getDocContent() is returning the latest version (and different content) of the document. If it was not a chronicle Id, the same content would be returned both times.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: String id = query.substring(query.toLowerCase(). On 2017/02/15 01:36:08, JohnL wrote: > Tie dot to lastIndexOf. Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1088: // Documentum returns the CURRENT version. To emulate the On 2017/02/15 01:36:08, JohnL wrote: > Move this comment to before the call to rs.last() above. Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1093: return getFolderBySpecification(id.substring(0, On 2017/02/15 01:36:08, JohnL wrote: > Break before "id.substring". Done. https://codereview.appspot.com/314480043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1094: id.length() - 1)); On 2017/02/15 01:36:08, JohnL wrote: > Maybe put this chopping of the trailing quote into the original query.substring? Done.
Sign in to reply to this message.
A few nits in the tests. John L https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: if (query.toLowerCase().lastIndexOf(idPrefix) == -1) { toLowerCase should always use a locale, usually Locale.ENGLISH, statically imported is good for brevity. https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: if (query.toLowerCase().lastIndexOf(idPrefix) == -1) { What is lastIndexOf getting you here that indexOf does not? https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1088: String id = query.substring(query.toLowerCase().lastIndexOf( This line-break is hard to read. I was thinking about using regular expressions with a group instead to make this extraction more readable. Another simpler change would be cache the index from lastIndexOf into a variable before the if, which makes this ... = query.substring(index + idPrefix.length(), query.length() - 1);
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: if (query.toLowerCase().lastIndexOf(idPrefix) == -1) { On 2017/02/17 01:54:19, JohnL wrote: > toLowerCase should always use a locale, usually Locale.ENGLISH, statically > imported is good for brevity. Done. https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: if (query.toLowerCase().lastIndexOf(idPrefix) == -1) { On 2017/02/17 01:54:19, JohnL wrote: > What is lastIndexOf getting you here that indexOf does not? Yes, indexOf() would work https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1085: if (query.toLowerCase().lastIndexOf(idPrefix) == -1) { On 2017/02/17 01:54:19, JohnL wrote: > What is lastIndexOf getting you here that indexOf does not? Done. https://codereview.appspot.com/314480043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1088: String id = query.substring(query.toLowerCase().lastIndexOf( On 2017/02/17 01:54:19, JohnL wrote: > This line-break is hard to read. I was thinking about using regular expressions > with a group instead to make this extraction more readable. Another simpler > change would be cache the index from lastIndexOf into a variable before the if, > which makes this > > ... = query.substring(index + idPrefix.length(), query.length() - 1); Done.
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/268db1102dae69e2d70ae2cd0c1800...
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
