From e8870334d2e89df4a898909f1a9faa362dbc0781 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 17 Aug 2020 15:47:53 -0300 Subject: [PATCH] Improve performance of DocumentsProvider#isChildDocument() The old solution iterates through all parents of the child until it finds the given parent or the storage root. This has been show to be very expensive in empirical tests. Therefore, this commit introduces a more efficient solution that simply compares the file paths of child and given parent. It also ensures that parent and child belong to the same account. Reviewers need to take extra care that this change does not introduce security issues by claiming a document is a child of a parent when it is really not. Signed-off-by: Torsten Grote --- .../operations/DownloadFileOperation.java | 2 +- .../providers/DocumentsStorageProvider.java | 28 ++++++++++++++----- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/owncloud/android/operations/DownloadFileOperation.java b/src/main/java/com/owncloud/android/operations/DownloadFileOperation.java index 1e927a92c4..393c5fc342 100644 --- a/src/main/java/com/owncloud/android/operations/DownloadFileOperation.java +++ b/src/main/java/com/owncloud/android/operations/DownloadFileOperation.java @@ -171,7 +171,7 @@ public class DownloadFileOperation extends RemoteOperation { modificationTimestamp = downloadOperation.getModificationTimestamp(); etag = downloadOperation.getEtag(); newFile = new File(getSavePath()); - if (!newFile.getParentFile().mkdirs()) { + if (!newFile.getParentFile().exists() && !newFile.getParentFile().mkdirs()) { Log_OC.e(TAG, "Unable to create parent folder " + newFile.getParentFile().getAbsolutePath()); } diff --git a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java index e5d324b33c..6359b4da05 100644 --- a/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java +++ b/src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java @@ -618,15 +618,29 @@ public class DocumentsStorageProvider extends DocumentsProvider { Log.d(TAG, "isChildDocument(), parent=" + parentDocumentId + ", id=" + documentId); try { - Document currentDocument = toDocument(documentId); + // get file for parent document Document parentDocument = toDocument(parentDocumentId); - - while (!ROOT_PATH.equals(currentDocument.getRemotePath())) { - currentDocument = currentDocument.getParent(); - if (parentDocument.getFile().equals(currentDocument.getFile())) { - return true; - } + OCFile parentFile = parentDocument.getFile(); + if (parentFile == null) { + throw new FileNotFoundException("No parent file with ID " + parentDocumentId); } + // get file for child candidate document + Document currentDocument = toDocument(documentId); + OCFile childFile = currentDocument.getFile(); + if (childFile == null) { + throw new FileNotFoundException("No child file with ID " + documentId); + } + + String parentPath = parentFile.getDecryptedRemotePath(); + String childPath = childFile.getDecryptedRemotePath(); + + // The alternative is to go up the folder hierarchy from currentDocument with getParent() + // until we arrive at parentDocument or the storage root. + // However, especially for long paths this is expensive and can take substantial time. + // The solution below uses paths and is faster by a factor of 2-10 depending on the nesting level of child. + // So far, the same document with its unique ID can never be in two places at once. + // If this assumption ever changes, this code would need to be adapted. + return parentDocument.getAccount() == currentDocument.getAccount() && childPath.startsWith(parentPath); } catch (FileNotFoundException e) { Log.e(TAG, "failed to check for child document", e);