From 6c690a1b09ed2eddbb71bea6409b296979a8fa31 Mon Sep 17 00:00:00 2001 From: alex-z Date: Fri, 11 Feb 2022 19:22:40 +0200 Subject: [PATCH 1/4] Do not remove a folder that has files that were not uploaded yet during propagation. Signed-off-by: alex-z --- src/libsync/owncloudpropagator.cpp | 61 ++++++++++++++++++++++++++++++ src/libsync/owncloudpropagator.h | 2 + 2 files changed, 63 insertions(+) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index c555976462..de5fcbe89e 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -413,6 +413,64 @@ void OwncloudPropagator::resetDelayedUploadTasks() _delayedTasks.clear(); } +void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector &items) +{ + /* + process each item that is new and is a directory and make sure every parent in its tree has the instruction CSYNC_INSTRUCTION_NEW + instead of CSYNC_INSTRUCTION_REMOVE + */ + for (const auto &item : items) { + if (item->_instruction != CSYNC_INSTRUCTION_NEW || item->_direction != SyncFileItem::Up || !item->isDirectory() || item->_file == QStringLiteral("/")) { + continue; + } + + // #1 get root folder name for the current item that we need to reupload + const auto folderPathSplit = item->_file.split(QLatin1Char('/'), Qt::SkipEmptyParts); + if (folderPathSplit.isEmpty()) { + continue; + } + const auto itemRootFolderName = folderPathSplit.first(); + if (itemRootFolderName.isEmpty()) { + continue; + } + // #2 iterate backwords (for optimization) and find the root folder by name + const auto itemRootFolderReverseIt = std::find_if(std::rbegin(items), std::rend(items), [&itemRootFolderName](const auto ¤tItem) { + return currentItem->_file == itemRootFolderName; + }); + + if (itemRootFolderReverseIt == std::rend(items)) { + continue; + } + + // #3 convert reverse iterator to normal iterator + const auto itemFolderIt = (itemRootFolderReverseIt + 1).base(); + + // #4 if the root folder is set to be removed, then we will need to fix this by reuploading every folder in + // the tree, including the root + if (itemFolderIt == std::end(items)) { + continue; + } + + auto nextFolderInTreeIt = itemFolderIt; + do { + // #5 Iterate forward from the CSYNC_INSTRUCTION_NEW folder's root, and make sure every folder in it's tree is set to CSYNC_INSTRUCTION_NEW + if ((*nextFolderInTreeIt)->isDirectory() + && (*nextFolderInTreeIt)->_instruction == CSYNC_INSTRUCTION_REMOVE + && (*nextFolderInTreeIt)->_direction == SyncFileItem::Down + && item->_file.startsWith(QString((*nextFolderInTreeIt)->_file) + QLatin1Char('/'))) { + + qCWarning(lcPropagator) << "WARNING: New directory to upload " << item->_file + << "is in the removed directories tree " << (*nextFolderInTreeIt)->_file + << " This should not happen! But, we are going to reupload the entire folder structure."; + + (*nextFolderInTreeIt)->_instruction = CSYNC_INSTRUCTION_NEW; + (*nextFolderInTreeIt)->_direction = SyncFileItem::Up; + } + ++nextFolderInTreeIt; + } while (nextFolderInTreeIt != std::end(items) && (*nextFolderInTreeIt)->_file != item->_file); + } +} + qint64 OwncloudPropagator::smallFileSize() { const qint64 smallFileSize = 100 * 1024; //default to 1 MB. Not dynamic right now. @@ -448,6 +506,9 @@ void OwncloudPropagator::start(SyncFileItemVector &&items) items.end()); } + // process each item that is new and is a directory and make sure every parent in its tree has the instruction NEW instead of REMOVE + adjustDeletedFoldersWithNewChildren(items); + resetDelayedUploadTasks(); _rootJob.reset(new PropagateRootDirectory(this)); QStack> directories; diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index 28f3994b0d..79d017e828 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -665,6 +665,8 @@ private: void resetDelayedUploadTasks(); + static void adjustDeletedFoldersWithNewChildren(SyncFileItemVector &items); + AccountPtr _account; QScopedPointer _rootJob; SyncOptions _syncOptions; From 1a300d0d5699ddaa4f812f81fedb8f0c3e3892f2 Mon Sep 17 00:00:00 2001 From: alex-z Date: Thu, 17 Feb 2022 18:18:35 +0200 Subject: [PATCH 2/4] Unit tests for new files restore logic when the parent folder is removed on the server. Signed-off-by: alex-z --- test/testsyncengine.cpp | 48 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 9ad3c66a63..8c53de89a9 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -152,6 +152,54 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + void testLocalDeleteWithReuploadForNewLocalFiles() + { + FakeFolder fakeFolder{FileInfo{}}; + + // create folders hierarchy with some nested dirs and files + fakeFolder.localModifier().mkdir("A"); + fakeFolder.localModifier().insert("A/existingfile_A.txt", 100); + fakeFolder.localModifier().mkdir("A/B"); + fakeFolder.localModifier().insert("A/B/existingfile_B.data", 100); + fakeFolder.localModifier().mkdir("A/B/C"); + fakeFolder.localModifier().mkdir("A/B/C/c1"); + fakeFolder.localModifier().mkdir("A/B/C/c1/c2"); + fakeFolder.localModifier().insert("A/B/C/c1/c2/existingfile_C2.md", 100); + + QVERIFY(fakeFolder.syncOnce()); + + // make sure everything is uploaded + QVERIFY(fakeFolder.currentRemoteState().find("A/B/C/c1/c2")); + QVERIFY(fakeFolder.currentRemoteState().find("A/existingfile_A.txt")); + QVERIFY(fakeFolder.currentRemoteState().find("A/B/existingfile_B.data")); + QVERIFY(fakeFolder.currentRemoteState().find("A/B/C/c1/c2/existingfile_C2.md")); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + // remove a folder "A" on the server + fakeFolder.remoteModifier().remove("A"); + + // put new files and folders into a local folder "A" + fakeFolder.localModifier().mkdir("A/B/C/c1/c2"); + fakeFolder.localModifier().insert("A/B/C/c1/c2/newfile.txt", 100); + fakeFolder.localModifier().insert("A/B/C/c1/c2/Readme.data", 100); + fakeFolder.localModifier().mkdir("A/B/C/c1/c2/newfiles"); + fakeFolder.localModifier().insert("A/B/C/c1/c2/newfiles/newfile.txt", 100); + fakeFolder.localModifier().insert("A/B/C/c1/c2/newfiles/Readme.data", 100); + + QVERIFY(fakeFolder.syncOnce()); + // make sure new files and folders are uploaded (restored) + QVERIFY(fakeFolder.currentLocalState().find("A/B/C/c1/c2")); + QVERIFY(fakeFolder.currentLocalState().find("A/B/C/c1/c2/Readme.data")); + QVERIFY(fakeFolder.currentLocalState().find("A/B/C/c1/c2/newfiles/newfile.txt")); + QVERIFY(fakeFolder.currentLocalState().find("A/B/C/c1/c2/newfiles/Readme.data")); + // and the old files are removed + QVERIFY(!fakeFolder.currentLocalState().find("A/existingfile_A.txt")); + QVERIFY(!fakeFolder.currentLocalState().find("A/B/existingfile_B.data")); + QVERIFY(!fakeFolder.currentLocalState().find("A/B/C/c1/c2/existingfile_C2.md")); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } + void testEmlLocalChecksum() { FakeFolder fakeFolder{FileInfo{}}; fakeFolder.localModifier().insert("a1.eml", 64, 'A'); From 37168fce9f31002e2813a0d8128cdad875d86ee0 Mon Sep 17 00:00:00 2001 From: alex-z Date: Wed, 23 Feb 2022 18:47:05 +0200 Subject: [PATCH 3/4] Fix review comments. Signed-off-by: alex-z --- test/testsyncengine.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 8c53de89a9..1ec461aa12 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -179,7 +179,6 @@ private slots: fakeFolder.remoteModifier().remove("A"); // put new files and folders into a local folder "A" - fakeFolder.localModifier().mkdir("A/B/C/c1/c2"); fakeFolder.localModifier().insert("A/B/C/c1/c2/newfile.txt", 100); fakeFolder.localModifier().insert("A/B/C/c1/c2/Readme.data", 100); fakeFolder.localModifier().mkdir("A/B/C/c1/c2/newfiles"); From bf80efc7ab826470a8dc156460a4c001ef7e8408 Mon Sep 17 00:00:00 2001 From: alex-z Date: Thu, 24 Feb 2022 21:16:24 +0200 Subject: [PATCH 4/4] Fix review comments from Matthieu. Using curent reverse iterator when searching for parent. Signed-off-by: alex-z --- src/libsync/owncloudpropagator.cpp | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index de5fcbe89e..889c0dce02 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -417,15 +417,16 @@ void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector { /* process each item that is new and is a directory and make sure every parent in its tree has the instruction CSYNC_INSTRUCTION_NEW - instead of CSYNC_INSTRUCTION_REMOVE + instead of CSYNC_INSTRUCTION_REMOVE + NOTE: We are iterating backwords to take advantage of optimization later, when searching for the parent of current it */ - for (const auto &item : items) { - if (item->_instruction != CSYNC_INSTRUCTION_NEW || item->_direction != SyncFileItem::Up || !item->isDirectory() || item->_file == QStringLiteral("/")) { + for (auto it = std::crbegin(items); it != std::crend(items); ++it) { + if ((*it)->_instruction != CSYNC_INSTRUCTION_NEW || (*it)->_direction != SyncFileItem::Up || !(*it)->isDirectory() || (*it)->_file == QStringLiteral("/")) { continue; } // #1 get root folder name for the current item that we need to reupload - const auto folderPathSplit = item->_file.split(QLatin1Char('/'), Qt::SkipEmptyParts); + const auto folderPathSplit = (*it)->_file.split(QLatin1Char('/'), Qt::SkipEmptyParts); if (folderPathSplit.isEmpty()) { continue; } @@ -434,7 +435,7 @@ void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector continue; } // #2 iterate backwords (for optimization) and find the root folder by name - const auto itemRootFolderReverseIt = std::find_if(std::rbegin(items), std::rend(items), [&itemRootFolderName](const auto ¤tItem) { + const auto itemRootFolderReverseIt = std::find_if(it, std::crend(items), [&itemRootFolderName](const auto ¤tItem) { return currentItem->_file == itemRootFolderName; }); @@ -457,9 +458,9 @@ void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector if ((*nextFolderInTreeIt)->isDirectory() && (*nextFolderInTreeIt)->_instruction == CSYNC_INSTRUCTION_REMOVE && (*nextFolderInTreeIt)->_direction == SyncFileItem::Down - && item->_file.startsWith(QString((*nextFolderInTreeIt)->_file) + QLatin1Char('/'))) { + && (*it)->_file.startsWith(QString((*nextFolderInTreeIt)->_file) + QLatin1Char('/'))) { - qCWarning(lcPropagator) << "WARNING: New directory to upload " << item->_file + qCWarning(lcPropagator) << "WARNING: New directory to upload " << (*it)->_file << "is in the removed directories tree " << (*nextFolderInTreeIt)->_file << " This should not happen! But, we are going to reupload the entire folder structure."; @@ -467,7 +468,7 @@ void OwncloudPropagator::adjustDeletedFoldersWithNewChildren(SyncFileItemVector (*nextFolderInTreeIt)->_direction = SyncFileItem::Up; } ++nextFolderInTreeIt; - } while (nextFolderInTreeIt != std::end(items) && (*nextFolderInTreeIt)->_file != item->_file); + } while (nextFolderInTreeIt != std::end(items) && (*nextFolderInTreeIt)->_file != (*it)->_file); } } @@ -506,6 +507,12 @@ void OwncloudPropagator::start(SyncFileItemVector &&items) items.end()); } + QStringList files; + + for (const auto &item : items) { + files.push_back(item->_file); + } + // process each item that is new and is a directory and make sure every parent in its tree has the instruction NEW instead of REMOVE adjustDeletedFoldersWithNewChildren(items);