Issue 317160043: Fix bug#26742691: Support for same name files in a folder - Part 3 (Closed)
Description Following changes are made:
- Send respondNotFound for requests with DocId urls containing no
object id.
- If the above DocId url corresponds to a valid object, push
new DocId with object id asynchronously.
- Added mock implementation of AsyncDocIdPusher for tests.
Patch Set 1 #
Total comments: 20
Patch Set 2 : Changes as per review #
Total comments: 12
Patch Set 3 : Changes as per review #
Total comments: 40
Patch Set 4 : Changes as per review. #
Total comments: 29
Patch Set 5 : Changes as per review #
Total comments: 15
Patch Set 6 : Changes per review. #
Total comments: 11
Patch Set 7 : Rebase changes #
Total comments: 1
MessagesTotal messages: 20
Please review the changes.
Sign in to reply to this message.
https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1451: if (dmPersObj != null) { You need to do this for the catch block also. Brett suggested just leaving dmPersObj null in both cases and having a check on line 1460 for dmPersObj == null, then call getObjectByPath followed by this block. https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1453: logger.log(Level.FINE, "New location: {0}", newId.toString()); toString is redundant. https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1454: URI newUri = docIdEncoder.encodeDocId(newId); Why are you sending an URI here, since you're no longer using a 301 response? https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1455: context.getAsyncDocIdPusher().pushDocId(new DocId(newUri.toString())); Brett says this returns fall if the queuing failed, in which case we likely don't want to send a 404. Maybe throw an IllegalStateException (the related Queue exception) or an IOException (if we want to throw a checked exception, but I think the result is the same). https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2029: public void testGetDocContent_404() throws Exception { Maybe s/404/missingChronicleId/. Do we have an actual 404 test, and a test for an object name that looks like it ends with an object ID? https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2045: context.getDocIdEncoder().encodeDocId( This is unnecessary, like it is in production. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:110: /** Fix indentation. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:132: this.namedResource.put(docId, acl); "this" isn't needed here. More generally, is this code copied from somewhere? Why is it so complete (and yet needlessly complex; using Map and HashMap, when the map can't even be read at all)?
Sign in to reply to this message.
Please review. https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1451: if (dmPersObj != null) { On 2017/02/01 22:52:58, JohnL wrote: > You need to do this for the catch block also. Brett suggested just leaving > dmPersObj null in both cases and having a check on line 1460 for dmPersObj == > null, then call getObjectByPath followed by this block. Done. https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1453: logger.log(Level.FINE, "New location: {0}", newId.toString()); On 2017/02/01 22:52:59, JohnL wrote: > toString is redundant. Done. https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1454: URI newUri = docIdEncoder.encodeDocId(newId); On 2017/02/01 22:52:58, JohnL wrote: > Why are you sending an URI here, since you're no longer using a 301 response? Done. https://codereview.appspot.com/317160043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1455: context.getAsyncDocIdPusher().pushDocId(new DocId(newUri.toString())); On 2017/02/01 22:52:58, JohnL wrote: > Brett says this returns fall if the queuing failed, in which case we likely > don't want to send a 404. Maybe throw an IllegalStateException (the related > Queue exception) or an IOException (if we want to throw a checked exception, but > I think the result is the same). Done. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2029: public void testGetDocContent_404() throws Exception { On 2017/02/01 22:52:59, JohnL wrote: > Maybe s/404/missingChronicleId/. > > Do we have an actual 404 test, and a test for an object name that looks like it > ends with an object ID? Done. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2045: context.getDocIdEncoder().encodeDocId( On 2017/02/01 22:52:59, JohnL wrote: > This is unnecessary, like it is in production. Done. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:110: /** On 2017/02/01 22:52:59, JohnL wrote: > Fix indentation. Done. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:132: this.namedResource.put(docId, acl); On 2017/02/01 22:52:59, JohnL wrote: > "this" isn't needed here. More generally, is this code copied from somewhere? > Why is it so complete (and yet needlessly complex; using Map and HashMap, when > the map can't even be read at all)? Initially had two lines just assigning to this.docId and this.acl (as changed now). Also had getters, getRecord() and getNamedResource(). Removed the extra getters as they are not used. Changed to map to just to be consistent with pushNamedResources() in AccumulatingDocIdPusher. Don't mind leaving out the assignments in pushRecord() and pushNamedResource() and just return true. These two are not used in tests.
Sign in to reply to this message.
https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2029: public void testGetDocContent_404() throws Exception { On 2017/02/04 02:27:54, Srinivas wrote: > On 2017/02/01 22:52:59, JohnL wrote: > > Maybe s/404/missingChronicleId/. > > > > Do we have an actual 404 test, and a test for an object name that looks like > it > > ends with an object ID? > > Done. But my question wasn't answered. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:132: this.namedResource.put(docId, acl); On 2017/02/04 02:27:54, Srinivas wrote: > On 2017/02/01 22:52:59, JohnL wrote: > > "this" isn't needed here. More generally, is this code copied from somewhere? > > Why is it so complete (and yet needlessly complex; using Map and HashMap, when > > the map can't even be read at all)? > > Initially had two lines just assigning to this.docId and this.acl (as changed > now). Also had getters, getRecord() and getNamedResource(). Removed the extra > getters as they are not used. Changed to map to just to be consistent with > pushNamedResources() in AccumulatingDocIdPusher. > > Don't mind leaving out the assignments in pushRecord() and pushNamedResource() > and just return true. These two are not used in tests. I would prefer making that clear by throwing an UnsupportedOperationException. There's also no point to the record and acl fields, which also cannot be read. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1453: String docPath = docIdToPath(id); This issue is more subtle than I was thinking about. We have a docid that looks like it has an object ID on the end, but the lookup failed. Previously, such docids were rare, so we checked for a false positive by looking up the path before returning a 404. Now such docids are expected, so maybe the quick 404 is OK. With a false positive, we would now return a 404, and then eventually rediscover the object and give it a new docid (with what look like two object IDs on the end). Calling docIdToPath here seems wrong, because we've shown the docid to not have a valid object ID on the end, so we should be checking the path as-is, with the hex string on the end (if we continue checking it at all). One further specter this raises is whether we should be validating the path after finding the object ID. I think the answer is yes, otherwise we may miss deleted links where, given path:objectid, objectid is valid, but path is no longer one of the object's paths. We shouldn't use getObjectByPath to do the validation, since we know that fails in at least two cases, but we should check the path against the constructed paths for the object (I think). https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1458: if (context.getAsyncDocIdPusher().pushDocId(newId) == false) { if (!...) is more idiomatic. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1460: MessageFormat.format("Failed to queue DocId: {0}", newId); Too technical; maybe "Failed to feed new DocId: {0}" https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1462: throw new IllegalStateException(message); Log-and-throw anti-pattern. Just throw, the caller will log it.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2029: public void testGetDocContent_404() throws Exception { On 2017/02/07 00:59:35, JohnL wrote: > On 2017/02/04 02:27:54, Srinivas wrote: > > On 2017/02/01 22:52:59, JohnL wrote: > > > Maybe s/404/missingChronicleId/. > > > > > > Do we have an actual 404 test, and a test for an object name that looks like > > it > > > ends with an object ID? > > > > Done. > > But my question wasn't answered. We discussed that it would be added in another CL (objectId to chronicleId changes CL). Anyway, added the test here as the changes to validate path is in this CL. https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java (right): https://codereview.appspot.com/317160043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:132: this.namedResource.put(docId, acl); On 2017/02/07 00:59:35, JohnL wrote: > On 2017/02/04 02:27:54, Srinivas wrote: > > On 2017/02/01 22:52:59, JohnL wrote: > > > "this" isn't needed here. More generally, is this code copied from > somewhere? > > > Why is it so complete (and yet needlessly complex; using Map and HashMap, > when > > > the map can't even be read at all)? > > > > Initially had two lines just assigning to this.docId and this.acl (as changed > > now). Also had getters, getRecord() and getNamedResource(). Removed the extra > > getters as they are not used. Changed to map to just to be consistent with > > pushNamedResources() in AccumulatingDocIdPusher. > > > > Don't mind leaving out the assignments in pushRecord() and pushNamedResource() > > and just return true. These two are not used in tests. > > I would prefer making that clear by throwing an UnsupportedOperationException. > There's also no point to the record and acl fields, which also cannot be read. Done. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1453: String docPath = docIdToPath(id); On 2017/02/07 00:59:35, JohnL wrote: > This issue is more subtle than I was thinking about. We have a docid that looks > like it has an object ID on the end, but the lookup failed. Previously, such > docids were rare, so we checked for a false positive by looking up the path > before returning a 404. > > Now such docids are expected, so maybe the quick 404 is OK. With a false > positive, we would now return a 404, and then eventually rediscover the object > and give it a new docid (with what look like two object IDs on the end). > > Calling docIdToPath here seems wrong, because we've shown the docid to not have > a valid object ID on the end, so we should be checking the path as-is, with the > hex string on the end (if we continue checking it at all). > > One further specter this raises is whether we should be validating the path > after finding the object ID. I think the answer is yes, otherwise we may miss > deleted links where, given path:objectid, objectid is valid, but path is no > longer one of the object's paths. We shouldn't use getObjectByPath to do the > validation, since we know that fails in at least two cases, but we should check > the path against the constructed paths for the object (I think). Made changes as discussed, added check to verify the at least one of the object paths match docId path (without the objectId). For the false positives, set the object to null. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1453: String docPath = docIdToPath(id); On 2017/02/07 00:59:35, JohnL wrote: > This issue is more subtle than I was thinking about. We have a docid that looks > like it has an object ID on the end, but the lookup failed. Previously, such > docids were rare, so we checked for a false positive by looking up the path > before returning a 404. > > Now such docids are expected, so maybe the quick 404 is OK. With a false > positive, we would now return a 404, and then eventually rediscover the object > and give it a new docid (with what look like two object IDs on the end). > > Calling docIdToPath here seems wrong, because we've shown the docid to not have > a valid object ID on the end, so we should be checking the path as-is, with the > hex string on the end (if we continue checking it at all). > > One further specter this raises is whether we should be validating the path > after finding the object ID. I think the answer is yes, otherwise we may miss > deleted links where, given path:objectid, objectid is valid, but path is no > longer one of the object's paths. We shouldn't use getObjectByPath to do the > validation, since we know that fails in at least two cases, but we should check > the path against the constructed paths for the object (I think). Done. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1458: if (context.getAsyncDocIdPusher().pushDocId(newId) == false) { On 2017/02/07 00:59:35, JohnL wrote: > if (!...) is more idiomatic. Done. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1460: MessageFormat.format("Failed to queue DocId: {0}", newId); On 2017/02/07 00:59:35, JohnL wrote: > Too technical; maybe "Failed to feed new DocId: {0}" Done. https://codereview.appspot.com/317160043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1462: throw new IllegalStateException(message); On 2017/02/07 00:59:35, JohnL wrote: > Log-and-throw anti-pattern. Just throw, the caller will log it. Done.
Sign in to reply to this message.
Partial review. Haven't looked at the tests yet in detail. John L https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1447: logger.log(Level.FINER, "Object paths do not match to DocId: {0}", Delete "to". https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1450: return; I'm not sure that I like this flow overall, with 3 returns and the explicit assignment to null, with a throw mixed in. But let's get this change committed and then revisit that. I don't think it's logically wrong, just complicated, and I want to make sure it's not convoluted. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1527: IDfPersistentObject dmPersObj) throws DfException { Cast this before the call, as we do elsewhere. IDfPersistentObject is only used briefly in getDocContent. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1533: String docIdPath = docIdToPath(id); This and the next two lines don't need to happen in the loop. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1536: if (index != -1 && docIdPath.substring(0, index). Dot shouldn't be separated from the name that follows it. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1537: equalsIgnoreCase(objectPath.getFullPath())) { Why ignore case? Are object names case-insensitive? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1810: private void insertDocument(String path) throws Exception { Tests can say "throws Exception", but helpers should be more specific.
Sign in to reply to this message.
Finished reviewing the tests. John L https://codereview.appspot.com/317160043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2524: // First try a DocId with an unencoded slash in the path. Why did you remove this block? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1861: setParentFolderIdFromPaths(lastModified, id, name, paths); This seems twisted. We are monkeying with the database structure and adding a separate sysobject for each path, but then you're turning around and setting i_folder_id to a CSV in each replica. More fodder for my worries below. Maybe we need to clean up the database structure (separately). https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1872: private void setParentFolderIdFromPaths(String lastModified, String id, This is hard to read. What are id and name the ID and name of, and what are paths the paths for? Maybe better names and/or javadoc tags for these params. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1874: List<String> folderIds = new ArrayList<String>(); ... = new ArrayList<>(); https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1877: continue; These continue statements feel like bugs or half-features. What triggers them? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1884: if (!Strings.isNullOrEmpty(folderPath) How could folderPath or folderName be null? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1886: insertFolder(getTimePlusMinutes(lastModified, -5), What's the -5 doing? Is it just keeping these folders out of the currently expected results? Maybe a comment to whatever effect it is causing? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1894: "UPDATE dm_sysobject SET i_folder_id = '%s' WHERE r_object_id = " We're already inconsistent about whether i_folder_id is a CSV string or a single value. This is making me nervous about the integrity of the tests. Not sure there's anything to do, this doesn't seem to trigger lots of badness, but that doesn't mean nothing is broken. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2078: public void testGetDocContent_docWithChronicleId() throws Exception { How and why did "missingChronicleId" get renamed to "docWithChronicleId"? Shouldn't the names of the new and existing tests here be parallel in some way?
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/317160043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2524: // First try a DocId with an unencoded slash in the path. On 2017/02/15 00:42:00, JohnL wrote: > Why did you remove this block? We are encoding "/" with "%2F" in name. Hence removed these lines that test with unencoded name. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1447: logger.log(Level.FINER, "Object paths do not match to DocId: {0}", On 2017/02/14 02:30:47, JohnL wrote: > Delete "to". Done. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1450: return; On 2017/02/14 02:30:48, JohnL wrote: > I'm not sure that I like this flow overall, with 3 returns and the explicit > assignment to null, with a throw mixed in. But let's get this change committed > and then revisit that. I don't think it's logically wrong, just complicated, and > I want to make sure it's not convoluted. Acknowledged. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1527: IDfPersistentObject dmPersObj) throws DfException { On 2017/02/14 02:30:47, JohnL wrote: > Cast this before the call, as we do elsewhere. IDfPersistentObject is only used > briefly in getDocContent. Done. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1533: String docIdPath = docIdToPath(id); On 2017/02/14 02:30:47, JohnL wrote: > This and the next two lines don't need to happen in the loop. Yes, thx, moved https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1533: String docIdPath = docIdToPath(id); On 2017/02/14 02:30:47, JohnL wrote: > This and the next two lines don't need to happen in the loop. Done. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1536: if (index != -1 && docIdPath.substring(0, index). On 2017/02/14 02:30:48, JohnL wrote: > Dot shouldn't be separated from the name that follows it. Done. https://codereview.appspot.com/317160043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1537: equalsIgnoreCase(objectPath.getFullPath())) { On 2017/02/14 02:30:47, JohnL wrote: > Why ignore case? Are object names case-insensitive? Done. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1810: private void insertDocument(String path) throws Exception { On 2017/02/14 02:30:48, JohnL wrote: > Tests can say "throws Exception", but helpers should be more specific. Done. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1861: setParentFolderIdFromPaths(lastModified, id, name, paths); On 2017/02/15 00:42:00, JohnL wrote: > This seems twisted. We are monkeying with the database structure and adding a > separate sysobject for each path, but then you're turning around and setting > i_folder_id to a CSV in each replica. More fodder for my worries below. Maybe we > need to clean up the database structure (separately). Yes, we are using i_folder_id as CSV value for our mocks. Documentum has entries for each document/folder in dm_sysobject/dm_folder and then has an entry of parent folder id in i_folder_id as well. Same is done here. Varargs paths is not seem to be used in tests. Scope for clean up. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1872: private void setParentFolderIdFromPaths(String lastModified, String id, On 2017/02/15 00:42:00, JohnL wrote: > This is hard to read. What are id and name the ID and name of, and what are > paths the paths for? Maybe better names and/or javadoc tags for these params. Done. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1872: private void setParentFolderIdFromPaths(String lastModified, String id, On 2017/02/15 00:42:00, JohnL wrote: > This is hard to read. What are id and name the ID and name of, and what are > paths the paths for? Maybe better names and/or javadoc tags for these params. Refactored names https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1874: List<String> folderIds = new ArrayList<String>(); On 2017/02/15 00:42:01, JohnL wrote: > ... = new ArrayList<>(); Done. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1877: continue; On 2017/02/15 00:42:00, JohnL wrote: > These continue statements feel like bugs or half-features. What triggers them? Folder path can be empty. As this method and insertFolder recursively adds paths all the way to cabinet. The last parent folder path for cabinet would be empty. Folder name can't be empty. Removed the continue statements. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1884: if (!Strings.isNullOrEmpty(folderPath) On 2017/02/15 00:42:01, JohnL wrote: > How could folderPath or folderName be null? Folder path can be empty, and folder name can't be empty as mentioned above. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1886: insertFolder(getTimePlusMinutes(lastModified, -5), On 2017/02/15 00:42:00, JohnL wrote: > What's the -5 doing? Is it just keeping these folders out of the currently > expected results? Maybe a comment to whatever effect it is causing? Yes, create parent folders 5 min before, to keep the parent folders out of result set. Added comments. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1894: "UPDATE dm_sysobject SET i_folder_id = '%s' WHERE r_object_id = " On 2017/02/15 00:42:00, JohnL wrote: > We're already inconsistent about whether i_folder_id is a CSV string or a single > value. This is making me nervous about the integrity of the tests. Not sure > there's anything to do, this doesn't seem to trigger lots of badness, but that > doesn't mean nothing is broken. We are emulating the repeating attribute of i_folder_id as CSV in mocks. Don't see anything to do. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2078: public void testGetDocContent_docWithChronicleId() throws Exception { On 2017/02/15 00:42:00, JohnL wrote: > How and why did "missingChronicleId" get renamed to "docWithChronicleId"? > Shouldn't the names of the new and existing tests here be parallel in some way? Mixup in renaming with other CL, Fixed it and changed the name of test "testGetDocContent_idInName" to "testGetDocContent_chronicleIdPartOfName" https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2078: public void testGetDocContent_docWithChronicleId() throws Exception { On 2017/02/15 00:42:00, JohnL wrote: > How and why did "missingChronicleId" get renamed to "docWithChronicleId"? > Shouldn't the names of the new and existing tests here be parallel in some way? Done.
Sign in to reply to this message.
Still looking at this. John L https://codereview.appspot.com/317160043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2524: // First try a DocId with an unencoded slash in the path. On 2017/02/16 05:46:11, Srinivas wrote: > On 2017/02/15 00:42:00, JohnL wrote: > > Why did you remove this block? > > We are encoding "/" with "%2F" in name. Hence removed these lines that test with > unencoded name. That was part of what Brett was testing here, that it didn't matter if the path was unencoded, because we are using the object ID (now chronicle ID). You could argue it's unnecessary, but I think it's fine, and it's unrelated to this CL in any case. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1861: setParentFolderIdFromPaths(lastModified, id, name, paths); On 2017/02/16 05:46:11, Srinivas wrote: > On 2017/02/15 00:42:00, JohnL wrote: > > This seems twisted. We are monkeying with the database structure and adding a > > separate sysobject for each path, but then you're turning around and setting > > i_folder_id to a CSV in each replica. More fodder for my worries below. Maybe > we > > need to clean up the database structure (separately). > > Yes, we are using i_folder_id as CSV value for our mocks. > Documentum has entries for each document/folder in dm_sysobject/dm_folder and > then has an entry of parent folder id in i_folder_id as well. Same is done here. > > Varargs paths is not seem to be used in tests. Scope for clean up. I think you're adding an entry to dm_sysobject for each path, and then adding the full CSV of paths to each object as well. I'll look at this closer. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1877: continue; On 2017/02/16 05:46:12, Srinivas wrote: > On 2017/02/15 00:42:00, JohnL wrote: > > These continue statements feel like bugs or half-features. What triggers them? > > Folder path can be empty. As this method and insertFolder recursively adds paths > all the way to cabinet. The last parent folder path for cabinet would be empty. > Folder name can't be empty. Removed the continue statements. I'm not following the argument for why you removed the continue statements, since you're saying they have a trigger at the end of the recursion. Can you give an example? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1884: if (!Strings.isNullOrEmpty(folderPath) On 2017/02/16 05:46:12, Srinivas wrote: > On 2017/02/15 00:42:01, JohnL wrote: > > How could folderPath or folderName be null? > > Folder path can be empty, and folder name can't be empty as mentioned above. But it can't be null, so why check isNullOrEmpty? https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1894: "UPDATE dm_sysobject SET i_folder_id = '%s' WHERE r_object_id = " On 2017/02/16 05:46:12, Srinivas wrote: > On 2017/02/15 00:42:00, JohnL wrote: > > We're already inconsistent about whether i_folder_id is a CSV string or a > single > > value. This is making me nervous about the integrity of the tests. Not sure > > there's anything to do, this doesn't seem to trigger lots of badness, but that > > doesn't mean nothing is broken. > > We are emulating the repeating attribute of i_folder_id as CSV in mocks. Don't > see anything to do. Both VdocRootNodeMock and FolderMock are assuming that i_folder_id is a single value. That's what I mean by we're not consistent. https://codereview.appspot.com/317160043/diff/60001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/60001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1535: int index = docIdPath.lastIndexOf("/" + name.replace("/", "%2F")); This still doesn't need to be in the loop, and the index != -1 can be moved into the loop condition (or another if, as you prefer). https://codereview.appspot.com/317160043/diff/60001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1536: if (index != -1 && docIdPath.substring(0, index).equals( Break before &&. Before .equals was also better, but && is the highest level available. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1888: // show up in result set. Can we say "expected results" instead of "result set"? Is that accurate? It seems to give more reason for why we don't want these in the result set.
Sign in to reply to this message.
More partial review. I'd still like to look closer at setParentFolderIdFromPaths. John L https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2063: public void testGetDocContent_chronicleIdPartOfName() throws Exception { This doesn't seem like a new test or a test of what it claims. It seems like a replica of the getDocContent tests above. To be a test with a object ID in the name, it seems like there are 2 variations, one of which is covered by the other CL. First, where the object ID in the name is not valid, so we should just get a plain 404 with no use of AsyncDocIdPusher. This is essentially a modern 404 test since a deleted document would present the same as an invalid ID in the name. Second, with a different but valid ID in the name, the path check should fail, with the same results. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2082: public void testGetDocContent_missingChronicleId() throws Exception { This seems like a duplicate of testGetDocContentNotFound. It could be repurposed for the new 404 test, as mentioned above.
Sign in to reply to this message.
https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1657: return getObjectUnderTest(ProxyAdaptorContext.getInstance(), Why change this overload at all?
Sign in to reply to this message.
The test gear is getting a little fragile. I've put together a followup CL to start fixing some of the issues, with more to follow. John L https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1875: * Sets the parent folder ids to i_folder_id and inserts the parent folders. The first part of this sentence is confusing. Maybe you meant the other way around: "Sets the i_folder_id to the parent folder IDs" (not ids)? https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1885: String folderName = folderPath.substring(folderPath.lastIndexOf("/") + 1); If folderPath is empty this still works, but feels awkward (lastIndexOf fails, but we add 1 and all is well). Maybe move this inside the if, since we don't need it otherwise anyway? https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1898: + "'%s'", Joiner.on(",").join(folderIds), objectId)); It looks like '%s' fits as part of the string on the previous line. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3677: throws ParseException { I'm going to delete this method in a followup CL, so let's catch ParseException and wrap it in a RuntimeException so we don't have to change all those method signatures and mess up the more interesting git history.
Sign in to reply to this message.
One last comment, perhaps. John L https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1820: setParentFolderIdFromPaths(dateFormat.format(lastModified), id, name, path); This call is redundant, because the call to insertDocument on the line above already did this.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1877: continue; On 2017/02/17 02:06:58, JohnL wrote: > On 2017/02/16 05:46:12, Srinivas wrote: > > On 2017/02/15 00:42:00, JohnL wrote: > > > These continue statements feel like bugs or half-features. What triggers > them? > > > > Folder path can be empty. As this method and insertFolder recursively adds > paths > > all the way to cabinet. The last parent folder path for cabinet would be > empty. > > Folder name can't be empty. Removed the continue statements. > > I'm not following the argument for why you removed the continue statements, > since you're saying they have a trigger at the end of the recursion. Can you > give an example? "/Cabinet/Folder1/Folder2/test.doc" would insert Folder2, Folder1 and Cabinet folders. So at the end, parent of Cabinet would be empty (but not null). And in all cases, folder names are not null nor empty. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1884: if (!Strings.isNullOrEmpty(folderPath) On 2017/02/17 02:06:59, JohnL wrote: > On 2017/02/16 05:46:12, Srinivas wrote: > > On 2017/02/15 00:42:01, JohnL wrote: > > > How could folderPath or folderName be null? > > > > Folder path can be empty, and folder name can't be empty as mentioned above. > > But it can't be null, so why check isNullOrEmpty? Done. https://codereview.appspot.com/317160043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1894: "UPDATE dm_sysobject SET i_folder_id = '%s' WHERE r_object_id = " On 2017/02/17 02:06:57, JohnL wrote: > On 2017/02/16 05:46:12, Srinivas wrote: > > On 2017/02/15 00:42:00, JohnL wrote: > > > We're already inconsistent about whether i_folder_id is a CSV string or a > > single > > > value. This is making me nervous about the integrity of the tests. Not sure > > > there's anything to do, this doesn't seem to trigger lots of badness, but > that > > > doesn't mean nothing is broken. > > > > We are emulating the repeating attribute of i_folder_id as CSV in mocks. Don't > > see anything to do. > > Both VdocRootNodeMock and FolderMock are assuming that i_folder_id is a single > value. That's what I mean by we're not consistent. Yes, we are not consistent. Only SysObjectMock uses multiple values for i_folder_id, hence I think we can remove/clean up varargs for folder and vdoc https://codereview.appspot.com/317160043/diff/60001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/60001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1535: int index = docIdPath.lastIndexOf("/" + name.replace("/", "%2F")); On 2017/02/17 02:07:00, JohnL wrote: > This still doesn't need to be in the loop, and the index != -1 can be moved into > the loop condition (or another if, as you prefer). Done. https://codereview.appspot.com/317160043/diff/60001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1536: if (index != -1 && docIdPath.substring(0, index).equals( On 2017/02/17 02:07:00, JohnL wrote: > Break before &&. Before .equals was also better, but && is the highest level > available. Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1657: return getObjectUnderTest(ProxyAdaptorContext.getInstance(), On 2017/02/18 01:02:22, JohnL wrote: > Why change this overload at all? Thx, this doesn't need to change. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1657: return getObjectUnderTest(ProxyAdaptorContext.getInstance(), On 2017/02/18 01:02:22, JohnL wrote: > Why change this overload at all? Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1820: setParentFolderIdFromPaths(dateFormat.format(lastModified), id, name, path); On 2017/02/19 08:48:11, JohnL wrote: > This call is redundant, because the call to insertDocument on the line above > already did this. Yes, thx. Added "setParentFolderIdFromPaths" to insertDocument later and didn't remove the call from here. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1875: * Sets the parent folder ids to i_folder_id and inserts the parent folders. On 2017/02/18 23:54:57, JohnL wrote: > The first part of this sentence is confusing. Maybe you meant the other way > around: "Sets the i_folder_id to the parent folder IDs" (not ids)? Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1885: String folderName = folderPath.substring(folderPath.lastIndexOf("/") + 1); On 2017/02/18 23:54:57, JohnL wrote: > If folderPath is empty this still works, but feels awkward (lastIndexOf fails, > but we add 1 and all is well). Maybe move this inside the if, since we don't > need it otherwise anyway? Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1888: // show up in result set. On 2017/02/17 02:07:01, JohnL wrote: > Can we say "expected results" instead of "result set"? Is that accurate? It > seems to give more reason for why we don't want these in the result set. Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1898: + "'%s'", Joiner.on(",").join(folderIds), objectId)); On 2017/02/18 23:54:57, JohnL wrote: > It looks like '%s' fits as part of the string on the previous line. Yes, thx :) https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1898: + "'%s'", Joiner.on(",").join(folderIds), objectId)); On 2017/02/18 23:54:57, JohnL wrote: > It looks like '%s' fits as part of the string on the previous line. Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2063: public void testGetDocContent_chronicleIdPartOfName() throws Exception { On 2017/02/17 02:20:09, JohnL wrote: > This doesn't seem like a new test or a test of what it claims. It seems like a > replica of the getDocContent tests above. To be a test with a object ID in the > name, it seems like there are 2 variations, one of which is covered by the other > CL. First, where the object ID in the name is not valid, so we should just get a > plain 404 with no use of AsyncDocIdPusher. This is essentially a modern 404 test > since a deleted document would present the same as an invalid ID in the name. > Second, with a different but valid ID in the name, the path check should fail, > with the same results. Don't we need a test where the name contains 16 digit id? https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2082: public void testGetDocContent_missingChronicleId() throws Exception { On 2017/02/17 02:20:09, JohnL wrote: > This seems like a duplicate of testGetDocContentNotFound. It could be repurposed > for the new 404 test, as mentioned above. Done. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3677: throws ParseException { On 2017/02/18 23:54:57, JohnL wrote: > I'm going to delete this method in a followup CL, so let's catch ParseException > and wrap it in a RuntimeException so we don't have to change all those method > signatures and mess up the more interesting git history. Don't you need this method?
Sign in to reply to this message.
https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2063: public void testGetDocContent_chronicleIdPartOfName() throws Exception { On 2017/02/21 01:03:01, Srinivas wrote: > On 2017/02/17 02:20:09, JohnL wrote: > > This doesn't seem like a new test or a test of what it claims. It seems like a > > replica of the getDocContent tests above. To be a test with a object ID in the > > name, it seems like there are 2 variations, one of which is covered by the > other > > CL. First, where the object ID in the name is not valid, so we should just get > a > > plain 404 with no use of AsyncDocIdPusher. This is essentially a modern 404 > test > > since a deleted document would present the same as an invalid ID in the name. > > Second, with a different but valid ID in the name, the path check should fail, > > with the same results. > > Don't we need a test where the name contains 16 digit id? As discussed previously, the production code doesn't distinguish between a name with a 16 digit ID at the end and one with a proper object ID at the end, which may or may not exist, and may or may not match the path. Those two cases are the only ones the production code checks for. We definitely need tests for an invalid ID (which you have now morphed this test to, so we're good there), and one that doesn't match the path in the docid (which I think we still need). https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2082: public void testGetDocContent_missingChronicleId() throws Exception { On 2017/02/21 01:03:02, Srinivas wrote: > On 2017/02/17 02:20:09, JohnL wrote: > > This seems like a duplicate of testGetDocContentNotFound. It could be > repurposed > > for the new 404 test, as mentioned above. > > Done. Sorry, I was just mistaken here. We need this one, which has a path that does exist, and is the essence of testing part 3 (namely, the feed of the new docid). https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3677: throws ParseException { On 2017/02/21 01:03:02, Srinivas wrote: > On 2017/02/18 23:54:57, JohnL wrote: > > I'm going to delete this method in a followup CL, so let's catch > ParseException > > and wrap it in a RuntimeException so we don't have to change all those method > > signatures and mess up the more interesting git history. > > Don't you need this method? No. I'm deleting the call to insertFolder inside setParentFolderIdFromPaths. We don't need the recursive call, but it's part of other changes, and I didn't want to continue holding up this CL for test gear cleanup. https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1444: if (dmPersObj != null) { Why? getObject doesn't return null. https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1538: if (index != -1 I was a little bit misleading by commenting on moving this condition and changing the line break here. Why bother with the loop if index == -1? https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1826: throws SQLException { Restore to previous line. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1920: throws DfException, IOException, SQLException { Restore to previous line. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2062: public void testGetDocContent_missingChronicleId() throws Exception { s/missing/invalid/; the chronicle ID itself is not missing. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2071: assertTrue(response.notFound); I think we should also verify that the AsyncDocIdPusher was not used here. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2551: // Now try a DocId with encoded slash in the name. Still need to restore this. It's a notFound now, due to the nature of the path checking, which I think is correct. A docid like this is invalid and would not be produced.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2063: public void testGetDocContent_chronicleIdPartOfName() throws Exception { On 2017/02/21 03:18:40, JohnL wrote: > On 2017/02/21 01:03:01, Srinivas wrote: > > On 2017/02/17 02:20:09, JohnL wrote: > > > This doesn't seem like a new test or a test of what it claims. It seems like > a > > > replica of the getDocContent tests above. To be a test with a object ID in > the > > > name, it seems like there are 2 variations, one of which is covered by the > > other > > > CL. First, where the object ID in the name is not valid, so we should just > get > > a > > > plain 404 with no use of AsyncDocIdPusher. This is essentially a modern 404 > > test > > > since a deleted document would present the same as an invalid ID in the > name. > > > Second, with a different but valid ID in the name, the path check should > fail, > > > with the same results. > > > > Don't we need a test where the name contains 16 digit id? > > As discussed previously, the production code doesn't distinguish between a name > with a 16 digit ID at the end and one with a proper object ID at the end, which > may or may not exist, and may or may not match the path. Those two cases are the > only ones the production code checks for. We definitely need tests for an > invalid ID (which you have now morphed this test to, so we're good there), and > one that doesn't match the path in the docid (which I think we still need). Added test "testGetDocContent_pathMismatch()" https://codereview.appspot.com/317160043/diff/60001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2082: public void testGetDocContent_missingChronicleId() throws Exception { On 2017/02/21 03:18:40, JohnL wrote: > On 2017/02/21 01:03:02, Srinivas wrote: > > On 2017/02/17 02:20:09, JohnL wrote: > > > This seems like a duplicate of testGetDocContentNotFound. It could be > > repurposed > > > for the new 404 test, as mentioned above. > > > > Done. > > Sorry, I was just mistaken here. We need this one, which has a path that does > exist, and is the essence of testing part 3 (namely, the feed of the new docid). Restored. https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1444: if (dmPersObj != null) { On 2017/02/21 03:18:41, JohnL wrote: > Why? getObject doesn't return null. Yes, however, our mock returned null. Fixed the mock https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1538: if (index != -1 On 2017/02/21 03:18:41, JohnL wrote: > I was a little bit misleading by commenting on moving this condition and > changing the line break here. Why bother with the loop if index == -1? Modified the code around a bit. https://codereview.appspot.com/317160043/diff/100001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1538: if (index != -1 On 2017/02/21 03:18:41, JohnL wrote: > I was a little bit misleading by commenting on moving this condition and > changing the line break here. Why bother with the loop if index == -1? Done. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1826: throws SQLException { On 2017/02/21 03:18:41, JohnL wrote: > Restore to previous line. Done. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1920: throws DfException, IOException, SQLException { On 2017/02/21 03:18:41, JohnL wrote: > Restore to previous line. Done. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2062: public void testGetDocContent_missingChronicleId() throws Exception { On 2017/02/21 03:18:41, JohnL wrote: > s/missing/invalid/; the chronicle ID itself is not missing. Done. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2071: assertTrue(response.notFound); On 2017/02/21 03:18:41, JohnL wrote: > I think we should also verify that the AsyncDocIdPusher was not used here. Done. https://codereview.appspot.com/317160043/diff/100001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2551: // Now try a DocId with encoded slash in the name. On 2017/02/21 03:18:41, JohnL wrote: > Still need to restore this. It's a notFound now, due to the nature of the path > checking, which I think is correct. A docid like this is invalid and would not > be produced. Done.
Sign in to reply to this message.
Just nits. I would, however, like to see another patch set after the rebasing, since there are conflicts in the meat of this CL. John L https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:42: import com.documentum.fc.client.DfIdNotFoundException; This will be unused after the rebasing. https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1527: IDfEnumeration enumPaths = We don't need this (or pathMatches) until after the if. https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1534: return false; In such a short method, the mix of early exit and single return seems odd. Maybe ditch pathMatches, and just return true and return false below? https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1021: throw new DfIdNotFoundException(id, id.toString() + " not found."); throw new DfIdNotFoundException(id) as in getAclObject below. toString() is redundant, and the second argument isn't a message at all, but a type description. The DfIdNotFoundException already constructs a suitable message around the given ID, IIRC. We won't need this at all after the rebasing, because you changed the call from getObject to getObjectByQualification, which returns null, in the chronicle ID CL. ;-) I think I would keep the machinery for now. https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java (right): https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:29: import java.util.HashMap; These 2 imports are unused.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:42: import com.documentum.fc.client.DfIdNotFoundException; On 2017/02/22 02:38:27, JohnL wrote: > This will be unused after the rebasing. Done. https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1527: IDfEnumeration enumPaths = On 2017/02/22 02:38:27, JohnL wrote: > We don't need this (or pathMatches) until after the if. Done. https://codereview.appspot.com/317160043/diff/120001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1534: return false; On 2017/02/22 02:38:27, JohnL wrote: > In such a short method, the mix of early exit and single return seems odd. Maybe > ditch pathMatches, and just return true and return false below? Done. https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1021: throw new DfIdNotFoundException(id, id.toString() + " not found."); On 2017/02/22 02:38:27, JohnL wrote: > throw new DfIdNotFoundException(id) as in getAclObject below. > > toString() is redundant, and the second argument isn't a message at all, but a > type description. The DfIdNotFoundException already constructs a suitable > message around the given ID, IIRC. > > We won't need this at all after the rebasing, because you changed the call from > getObject to getObjectByQualification, which returns null, in the chronicle ID > CL. ;-) I think I would keep the machinery for now. Yes, I know we removed it. https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:1021: throw new DfIdNotFoundException(id, id.toString() + " not found."); On 2017/02/22 02:38:27, JohnL wrote: > throw new DfIdNotFoundException(id) as in getAclObject below. > > toString() is redundant, and the second argument isn't a message at all, but a > type description. The DfIdNotFoundException already constructs a suitable > message around the given ID, IIRC. > > We won't need this at all after the rebasing, because you changed the call from > getObject to getObjectByQualification, which returns null, in the chronicle ID > CL. ;-) I think I would keep the machinery for now. Done. https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... File test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java (right): https://codereview.appspot.com/317160043/diff/120001/test/com/google/enterpri... test/com/google/enterprise/adaptor/documentum/ProxyAdaptorContext.java:29: import java.util.HashMap; On 2017/02/22 02:38:27, JohnL wrote: > These 2 imports are unused. Done. https://codereview.appspot.com/317160043/diff/160001/src/com/google/enterpris... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/317160043/diff/160001/src/com/google/enterpris... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:1444: if (dmPersObj != null) { Back to adding the check for null, because getObjectByQualification can return null.
Sign in to reply to this message.
LGTM Thanks, John L
Sign in to reply to this message.
Committed changes to Documentum repository. https://github.com/googlegsa/documentum/commit/989b2e4b7b1663e742347a0bfadb34...
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
