Issue 316100043: Fix bug#26742691: Support for same name files in a folder - Part 2 (Closed)
DescriptionFix bug#26742691: Support for same name files in a folder - Part 2
Changes to send all the DocId urls with objectId appended.
Modified tests to use hexadecimal objectId. These objectIds are
genreated with object type, name and zero and padding to form
hexadecimal objectId.
Patch Set 1 #
Total comments: 33
Patch Set 2 : Changes as per review. #
Total comments: 15
Patch Set 3 : Changes as per review. #
Total comments: 11
Patch Set 4 : Changes as per review. #
Total comments: 2
Patch Set 5 : LGTM Changes. #
MessagesTotal messages: 10
Please review.
Sign in to reply to this message.
Please add descriptions of the actual changes in each part. You missed part 1, but please add one for parts 2 and 3. John L https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:306: private static String docIdToName(String id) { If you convince me to keep this, it should take a DocId and use a local variable called "name", or just if { return id.substring } else { return id }. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:307: if (id.matches(".*:\\p{XDigit}{16}")) { If we're going to have multiples of this, we should use a constant, or perhaps even a compiled pattern, maybe OBJECT_ID_SUFFIX. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:680: cabinets.add(cabinet + ":" + objectId); I do not like hiding the string concatenation here like this. At the least, add a helper method next to docIdFromPath, but try to think about how to get more parallelism here by altering the downstream calls to the single argument docIdFromPath instead. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1437: logger.log(Level.WARNING, "Path doesn't contain objectid: {0}", path); This is at best a part 3 change. It's not invalid to crawl a docid without an object ID. We just no longer produce them. Also, s/WARNING/INFO or FINE/. There is nothing actionable about this. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1540: htmlWriter.addLink(docid, docIdToName(docid.getUniqueId())); This is not as we discussed. What's your argument for truncating the start paths? https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:220: Preconditions.checkArgument(name.length() <= 14, else { return super.pad(name); } https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:226: private static final HashMap<String, String> reservedCabinets = ImmutableMap.Builder might be cleaner, or you're using an anonymous inner class with an instance initializer down on lines 348ff. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:230: reservedCabinets.put("dm_bof_registry", CABINET.pad("1")); This is worth a comment. CABINET.pad is going to use the partially constructed reservedCabinets, though it always gets a cache miss, and would do so even if it were fully constructed. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:351: put("/Folder1/path1", "0b01081f80078d2a"); What's going on with the difference in naming conventions here? https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:992: if(id.toString().startsWith("09") || id.toString().startsWith("0b")) { Add space after "if". https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1731: insertCabinets("System", "Cab1", "Cab2"); aaa, FFF, Cab? Maybe add a comment somewhere near or even at the top about this naming convention. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1864: new MockRequest(DocumentumAdaptor.docIdFromPath(START_PATH, "aaa", These are terrible line breaks. Maybe statically import docIdFromPath? https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1884: private MockResponse getDocContent(ObjectIdFactory obj, String path) s/obj/type/g https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1888: path.substring(0,path.lastIndexOf("/")), name, obj.pad(name))); Add space after "0,". https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1997: String name = path.substring(path.lastIndexOf("/") + 1); This is too much scattered string manipulation. You need a helper method for these.
Sign in to reply to this message.
Please review. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:306: private static String docIdToName(String id) { On 2017/01/05 23:36:06, JohnL wrote: > If you convince me to keep this, it should take a DocId and use a local variable > called "name", or just if { return id.substring } else { return id }. Removed it. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:307: if (id.matches(".*:\\p{XDigit}{16}")) { On 2017/01/05 23:36:06, JohnL wrote: > If we're going to have multiples of this, we should use a constant, or perhaps > even a compiled pattern, maybe OBJECT_ID_SUFFIX. Since this method s removed, not adding OBJECT_ID_SUFFIX https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:680: cabinets.add(cabinet + ":" + objectId); On 2017/01/05 23:36:06, JohnL wrote: > I do not like hiding the string concatenation here like this. At the least, add > a helper method next to docIdFromPath, but try to think about how to get more > parallelism here by altering the downstream calls to the single argument > docIdFromPath instead. Restructured this around a bit. Using DocId for the List instead of String. Caller anyway converts it back to DocId so better to return List<DocId>. Also using object_name for cabinet. This results in using docIdFromPath() without need for concatenation. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1437: logger.log(Level.WARNING, "Path doesn't contain objectid: {0}", path); On 2017/01/05 23:36:06, JohnL wrote: > This is at best a part 3 change. It's not invalid to crawl a docid without an > object ID. We just no longer produce them. Also, s/WARNING/INFO or FINE/. There > is nothing actionable about this. Done. https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1540: htmlWriter.addLink(docid, docIdToName(docid.getUniqueId())); On 2017/01/05 23:36:06, JohnL wrote: > This is not as we discussed. What's your argument for truncating the start > paths? docIdToName removed the objectId as we don't want objectId in the label. Anyway, removed docIdToName method. Using docIdToPath() instead. So there will be leading "/" for the cabinet label, which should be ok. Should this be using docid.getUniqueId() for the label? If we don't want leading "/" and don't want objectId, need to add docIdToName() method back. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:220: Preconditions.checkArgument(name.length() <= 14, On 2017/01/05 23:36:07, JohnL wrote: > else { return super.pad(name); } Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:226: private static final HashMap<String, String> reservedCabinets = On 2017/01/05 23:36:07, JohnL wrote: > ImmutableMap.Builder might be cleaner, or you're using an anonymous inner class > with an instance initializer down on lines 348ff. Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:230: reservedCabinets.put("dm_bof_registry", CABINET.pad("1")); On 2017/01/05 23:36:07, JohnL wrote: > This is worth a comment. CABINET.pad is going to use the partially constructed > reservedCabinets, though it always gets a cache miss, and would do so even if it > were fully constructed. Yes, it gets a cache miss for reserved cabinets and creates one and adds to the cache. Now with Immutable Map.Builder(), CABINET.pad can't be used. Using hardcoded 16 digit id instead. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:351: put("/Folder1/path1", "0b01081f80078d2a"); On 2017/01/05 23:36:07, JohnL wrote: > What's going on with the difference in naming conventions here? The start path"/Folder1/FFF0" needs an id, as expected by few tests, hence added to this map. Other earlier folder paths didn't change. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:992: if(id.toString().startsWith("09") || id.toString().startsWith("0b")) { On 2017/01/05 23:36:06, JohnL wrote: > Add space after "if". Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1731: insertCabinets("System", "Cab1", "Cab2"); On 2017/01/05 23:36:07, JohnL wrote: > aaa, FFF, Cab? Maybe add a comment somewhere near or even at the top about this > naming convention. Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1864: new MockRequest(DocumentumAdaptor.docIdFromPath(START_PATH, "aaa", On 2017/01/05 23:36:07, JohnL wrote: > These are terrible line breaks. Maybe statically import docIdFromPath? Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1884: private MockResponse getDocContent(ObjectIdFactory obj, String path) On 2017/01/05 23:36:07, JohnL wrote: > s/obj/type/g Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1888: path.substring(0,path.lastIndexOf("/")), name, obj.pad(name))); On 2017/01/05 23:36:06, JohnL wrote: > Add space after "0,". Done. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1997: String name = path.substring(path.lastIndexOf("/") + 1); On 2017/01/05 23:36:07, JohnL wrote: > This is too much scattered string manipulation. You need a helper method for > these. Done.
Sign in to reply to this message.
The description can be more specific. Is this CL about anything but adding the object IDs to the docids for the start paths? John L https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1540: htmlWriter.addLink(docid, docIdToName(docid.getUniqueId())); On 2017/01/06 05:39:34, Srinivas wrote: > On 2017/01/05 23:36:06, JohnL wrote: > > This is not as we discussed. What's your argument for truncating the start > > paths? > > docIdToName removed the objectId as we don't want objectId in the label. Anyway, > removed docIdToName method. Using docIdToPath() instead. So there will be > leading "/" for the cabinet label, which should be ok. > > Should this be using docid.getUniqueId() for the label? If we don't want leading > "/" and don't want objectId, need to add docIdToName() method back. I misunderstood the context here. I thought it was start paths, but it's the cabinets for /. So the path and the object name differ only in a slash. I don't strongly care, this isn't production code, nor does the label matter much anyway. docIdToPath(docid) differs from the ideal value only in the /, which isn't worth additional code to patch up. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:230: reservedCabinets.put("dm_bof_registry", CABINET.pad("1")); On 2017/01/06 05:39:34, Srinivas wrote: > On 2017/01/05 23:36:07, JohnL wrote: > > This is worth a comment. CABINET.pad is going to use the partially constructed > > reservedCabinets, though it always gets a cache miss, and would do so even if > it > > were fully constructed. > > Yes, it gets a cache miss for reserved cabinets and creates one and adds to the > cache. > Now with Immutable Map.Builder(), CABINET.pad can't be used. Using hardcoded 16 > digit id instead. There are at least two better solutions. One is to make reservedCabinets a field and use super.pad, I like that one for multiple reasons. Another option is to use a separate, temporary new ObjectIdFactory("0c") to do the padding. Both variations avoid the premature reference to reservedCabinets. https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (left): https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1441: //TODO (Srinivas): remove this code You need to restore this, too, for now. https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:651: private List<DocId>listRootCabinets(IDfSession session) throws DfException { Add space after > https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:668: for (int j = 0; j < result.getValueCount("r_folder_path"); j++) { Since a cabinet can only have one path, we can drop this loop and the selection of r_folder_path. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:123: * Naming convention used: Add something like "All names are valid hexadecimal strings." https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2011: getDocContent(ImmutableMap.of("documentum.displayUrlPattern", The line breaks in this instance were better before. We don't observe the rectangle rule in details, but: something(else(argument, morearguments), another); is really hard to read, with "another" dangling but belonging to "something" rather than to "else". https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2047: new MockRequest(docIdFromPath(path, name, type.pad(name)))); This isn't correct. It doesn't strip the name from path, so it now appears twice in the docid, which no longer uses the path so nothing breaks. There is still too many copies of this string manipulation (4 or 5, I think), and errors like this are exactly why I didn't want the repetition. I know you removed some of it, but fiddle with it some more, please. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2155: Request request = new MockRequest(docIdFromPath(START_PATH, name, Break after = is a higher-level break. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2316: Request request = new MockRequest(docIdFromPath(START_PATH, name, Same here.
Sign in to reply to this message.
Please review. https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:230: reservedCabinets.put("dm_bof_registry", CABINET.pad("1")); On 2017/01/07 00:30:43, JohnL wrote: > On 2017/01/06 05:39:34, Srinivas wrote: > > On 2017/01/05 23:36:07, JohnL wrote: > > > This is worth a comment. CABINET.pad is going to use the partially > constructed > > > reservedCabinets, though it always gets a cache miss, and would do so even > if > > it > > > were fully constructed. > > > > Yes, it gets a cache miss for reserved cabinets and creates one and adds to > the > > cache. > > Now with Immutable Map.Builder(), CABINET.pad can't be used. Using hardcoded > 16 > > digit id instead. > > There are at least two better solutions. One is to make reservedCabinets a field > and use super.pad, I like that one for multiple reasons. Another option is to > use a separate, temporary new ObjectIdFactory("0c") to do the padding. Both > variations avoid the premature reference to reservedCabinets. Done. https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:651: private List<DocId>listRootCabinets(IDfSession session) throws DfException { On 2017/01/07 00:30:43, JohnL wrote: > Add space after > Done. https://codereview.appspot.com/316100043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:668: for (int j = 0; j < result.getValueCount("r_folder_path"); j++) { On 2017/01/07 00:30:43, JohnL wrote: > Since a cabinet can only have one path, we can drop this loop and the selection > of r_folder_path. As discussed, changed back to use r_folder_path. instead of object_name. Changed to use docIdFromPath(path, id) instead of docIdFromPath(path, name, id) https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:123: * Naming convention used: On 2017/01/07 00:30:43, JohnL wrote: > Add something like "All names are valid hexadecimal strings." Done. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2011: getDocContent(ImmutableMap.of("documentum.displayUrlPattern", On 2017/01/07 00:30:44, JohnL wrote: > The line breaks in this instance were better before. We don't observe the > rectangle rule in details, but: > > something(else(argument, > morearguments), another); > > is really hard to read, with "another" dangling but belonging to "something" > rather than to "else". Done. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2047: new MockRequest(docIdFromPath(path, name, type.pad(name)))); On 2017/01/07 00:30:44, JohnL wrote: > This isn't correct. It doesn't strip the name from path, so it now appears twice > in the docid, which no longer uses the path so nothing breaks. There is still > too many copies of this string manipulation (4 or 5, I think), and errors like > this are exactly why I didn't want the repetition. I know you removed some of > it, but fiddle with it some more, please. Yes, changed it as discussed. Added and used docIdFromPath(path, id) method here and elsewhere. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2155: Request request = new MockRequest(docIdFromPath(START_PATH, name, On 2017/01/07 00:30:43, JohnL wrote: > Break after = is a higher-level break. Done. https://codereview.appspot.com/316100043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2316: Request request = new MockRequest(docIdFromPath(START_PATH, name, On 2017/01/07 00:30:43, JohnL wrote: > Same here. Done.
Sign in to reply to this message.
https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:665: "SELECT r_object_id, object_name, r_folder_path FROM dm_cabinet" Remove object_name. https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1409: String path = docIdToPath(id); This seems broken. This strips the object ID, so the check on line 1426 never matches. https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1549: resp.addAnchor(docIdEncoder.encodeDocId(docid), docid.getUniqueId()); Use docIdToPath(docid) in place of getUniqueId here, too. https://codereview.appspot.com/316100043/diff/100001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:205: private final DfException NO_EXCEPTION = null; Why did you remove static here? https://codereview.appspot.com/316100043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2008: MockResponse response = Restore original line breaks.
Sign in to reply to this message.
Please review https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:665: "SELECT r_object_id, object_name, r_folder_path FROM dm_cabinet" On 2017/01/10 21:33:29, JohnL wrote: > Remove object_name. Done. https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1409: String path = docIdToPath(id); On 2017/01/10 21:33:29, JohnL wrote: > This seems broken. This strips the object ID, so the check on line 1426 never > matches. Yes, thx. Fixed it. https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1409: String path = docIdToPath(id); On 2017/01/10 21:33:29, JohnL wrote: > This seems broken. This strips the object ID, so the check on line 1426 never > matches. Done. https://codereview.appspot.com/316100043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1549: resp.addAnchor(docIdEncoder.encodeDocId(docid), docid.getUniqueId()); On 2017/01/10 21:33:29, JohnL wrote: > Use docIdToPath(docid) in place of getUniqueId here, too. Done. https://codereview.appspot.com/316100043/diff/100001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:205: private final DfException NO_EXCEPTION = null; On 2017/01/10 21:33:29, JohnL wrote: > Why did you remove static here? Done. https://codereview.appspot.com/316100043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2008: MockResponse response = On 2017/01/10 21:33:29, JohnL wrote: > Restore original line breaks. Done.
Sign in to reply to this message.
LGTM++ John L https://codereview.appspot.com/316100043/diff/120001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1719: URI anchor = response.anchors.get(DocumentumAdaptor.docIdToPath( Bad line break, non-sensical argument to the DocId constructor, explicitly constructing an old-style docid, and introducing an inconsistency from the same string 3 lines above. There are a number of ways to resolve this, but I think using "/" + cabinet consistently here is the best.
Sign in to reply to this message.
Final LGTM changes https://codereview.appspot.com/316100043/diff/120001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/316100043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1719: URI anchor = response.anchors.get(DocumentumAdaptor.docIdToPath( On 2017/01/11 03:11:00, JohnL wrote: > Bad line break, non-sensical argument to the DocId constructor, explicitly > constructing an old-style docid, and introducing an inconsistency from the same > string 3 lines above. > > There are a number of ways to resolve this, but I think using "/" + cabinet > consistently here is the best. Done.
Sign in to reply to this message.
Committed changes to Documentum repository. https://github.com/googlegsa/documentum/commit/f474cffe27de13fa4611353dc199f5...
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
