On folder move execute only one UPDATE query for all nested items.

src/common/preparedsqlquerymanager.h

@@ -18,7 +18,7 @@
 
 #pragma once
 
 #include "ocsynclib.h"
 #include "ownsql.h"
 #include "common/asserts.h"
 
@@ -108,7 +108,7 @@
         GetE2EeLockedFoldersQuery,
         DeleteE2EeLockedFolderQuery,
         ListAllTopLevelE2eeFoldersStatusLessThanQuery,
-
+        RelocateFolderToNewPathRecursivelyQuery,
         PreparedQueryCount
     };
     PreparedSqlQueryManager() = default;

src/common/syncjournaldb.cpp

@@ -1,4 +1,4 @@
 /*
  * Copyright (C) by Klaas Freitag <[email protected]>
  *
  * This library is free software; you can redistribute it and/or
@@ -16,7 +16,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 #include <QCryptographicHash>
 #include <QFile>
 #include <QLoggingCategory>
 #include <QStringList>
@@ -394,6 +394,18 @@
                                                                         end - text, 0));
                                 }, nullptr, nullptr);
 
+    sqlite3_create_function(_db.sqliteDb(), "path_hash", 1, SQLITE_UTF8 | SQLITE_DETERMINISTIC, nullptr,
+                                [] (sqlite3_context *ctx,int, sqlite3_value **argv) {
+                                    const auto text = reinterpret_cast<const char*>(sqlite3_value_text(argv[0]));
+                                    sqlite3_result_int64(ctx, c_jhash64(reinterpret_cast<const uint8_t*>(text), strlen(text), 0));
+                                }, nullptr, nullptr);
+
+    sqlite3_create_function(_db.sqliteDb(), "path_length", 1, SQLITE_UTF8 | SQLITE_DETERMINISTIC, nullptr,
+                                [] (sqlite3_context *ctx,int, sqlite3_value **argv) {
+                                    const auto text = reinterpret_cast<const char*>(sqlite3_value_text(argv[0]));
+                                    sqlite3_result_int64(ctx, strlen(text));
+                                }, nullptr, nullptr);
+
     /* Because insert is so slow, we do everything in a transaction, and only need one call to commit */
     startTransaction();
 
@@ -1075,9 +1087,10 @@
 
     if (!checkConnect())
         return false;
-    const auto query = _queryManager.get(PreparedSqlQueryManager::ListAllTopLevelE2eeFoldersStatusLessThanQuery,
-                                         QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE type == 2 AND isE2eEncrypted >= ?1 AND isE2eEncrypted < ?2 ORDER BY path||'/' ASC"),
-                                         _db);
+    const auto query =
+        _queryManager.get(PreparedSqlQueryManager::ListAllTopLevelE2eeFoldersStatusLessThanQuery,
+                          QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE type == 2 AND isE2eEncrypted >= ?1 AND isE2eEncrypted < ?2 ORDER BY path||'/' ASC"),
+                          _db);
     if (!query) {
         return false;
     }
@@ -1132,6 +1145,29 @@
     return true;
 }
 
+bool SyncJournalDb::relocateFolderToNewPathRecursively(const QByteArray &oldParentPath, const QByteArray &newParentPath)
+{
+    qCInfo(lcDb) << "Relocating folder recursively from path" << oldParentPath << "to path" << newParentPath;
+
+    if (!checkConnect()) {
+        qCWarning(lcDb) << "Failed to connect database.";
+        return false;
+    }
+
+    const auto query = _queryManager.get(
+        PreparedSqlQueryManager::RelocateFolderToNewPathRecursivelyQuery,
+        QByteArrayLiteral("UPDATE metadata"
+                          " SET path = REPLACE(path, ?1, ?2), phash = path_hash(REPLACE(path, ?1, ?2)), pathlen = path_length(REPLACE(path, ?1, ?2))"
+                          "  WHERE " IS_PREFIX_PATH_OF("?1", "path")),
+        _db);
+    if (!query) {
+        return false;
+    }
+    query->bindValue(1, oldParentPath);
+    query->bindValue(2, newParentPath);
+    return query->exec();
+}
+
 void SyncJournalDb::keyValueStoreSet(const QString &key, QVariant value)
 {
     QMutexLocker locker(&_mutex);

src/common/syncjournaldb.h

@@ -1,4 +1,4 @@
 /*
  * Copyright (C) by Klaas Freitag <[email protected]>
  *
  * This library is free software; you can redistribute it and/or
@@ -19,7 +19,7 @@
 #ifndef SYNCJOURNALDB_H
 #define SYNCJOURNALDB_H
 
 #include <QObject>
 #include <QDateTime>
 #include <QHash>
 #include <QMutex>
@@ -70,10 +70,13 @@
     [[nodiscard]] bool getFilesBelowPath(const QByteArray &path, const std::function<void(const SyncJournalFileRecord&)> &rowCallback);
     [[nodiscard]] bool listFilesInPath(const QByteArray &path, const std::function<void(const SyncJournalFileRecord&)> &rowCallback);
     [[nodiscard]] Result<void, QString> setFileRecord(const SyncJournalFileRecord &record);
+
     [[nodiscard]] bool getRootE2eFolderRecord(const QString &remoteFolderPath, SyncJournalFileRecord *rec);
     [[nodiscard]] bool listAllE2eeFoldersWithEncryptionStatusLessThan(const int status, const std::function<void(const SyncJournalFileRecord &)> &rowCallback);
     [[nodiscard]] bool findEncryptedAncestorForRecord(const QString &filename, SyncJournalFileRecord *rec);
-
+    // use this after moving a folder and all its contents under new parent (e.g. "folderA" move to "parentFolder", such that "folderA" -> "parentFolder/folderA"
+    // all nested items will have their paths updated accordingly wiht a single UPDATE query
+    [[nodiscard]] bool relocateFolderToNewPathRecursively(const QByteArray &oldParentPath, const QByteArray &newParentPath); 
     void keyValueStoreSet(const QString &key, QVariant value);
     [[nodiscard]] qint64 keyValueStoreGetInt(const QString &key, qint64 defaultValue);
     void keyValueStoreDelete(const QString &key);

src/libsync/owncloudpropagator.cpp

@@ -1,4 +1,4 @@
 /*
  * Copyright (C) by Olivier Goffart <[email protected]>
  * Copyright (C) by Klaas Freitag <[email protected]>
  *
@@ -490,6 +490,29 @@
         } while (nextFolderInTreeIt != std::end(items) && (*nextFolderInTreeIt)->_file != (*it)->_file);
     }
 }
+void OwncloudPropagator::cleanupLocallyMovedFoldersFromNestedItems(SyncFileItemVector &items)
+{
+    // TODO: this method is not the fastest we could do, but, for now it works, maybe add a flag "_isAnyMultipleRenameUploads" in discovery
+    // so this could be skipped if no moves discovered?
+    QMap<QString, QString> renamedDirectories;
+    for (const auto &item : items) {
+        // TODO: for now, let's only process uploads (for downloads, we need to also adjust PropagateLocalRename such that correct DB records and pin states are set)
+        if (item->isDirectory() && item->_instruction == CSYNC_INSTRUCTION_RENAME && item->_direction == SyncFileItem::Up) {
+            renamedDirectories.insert(item->_file, item->_renameTarget);
+        }
+    }
+
+    if (renamedDirectories.isEmpty()) {
+        return;
+    }
+
+    // get rid of nested items that are inside already moved folders such that we only run one move job (for parent, and just update children in DB during sync)
+    const auto eraseBeginIt = std::remove_if(std::begin(items), std::end(items), [&renamedDirectories](const SyncFileItemPtr &item) {
+        const auto origin = staticAdjustRenamedPath(renamedDirectories, item->_file);
+        return origin == item->_renameTarget;
+    });
+    items.erase(eraseBeginIt, std::end(items));
+}
 
 qint64 OwncloudPropagator::smallFileSize()
 {
@@ -508,6 +531,12 @@
      * In order to do that we loop over the items. (which are sorted by destination)
      * When we enter a directory, we can create the directory job and push it on the stack. */
 
+    qCInfo(lcPropagator) << "BEGIN ITEMS SYNC";
+    for (const auto &item : items) {
+        qCInfo(lcPropagator) << "item->_originalFile:" << item->_originalFile << "item->_renameTarget" << item->_renameTarget << "item->_direction"
+                             << item->_direction << "item->_instruction:" << item->_instruction;
+    }
+
     const auto regex = syncOptions().fileRegex();
     if (regex.isValid()) {
         QSet<QStringView> names;
@@ -528,14 +557,36 @@
             items.end());
     }
 
-    QStringList files;
-
+    // 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);
+    qCInfo(lcPropagator) << "BEGIN ITEMS CLEANUP";
     for (const auto &item : items) {
-        files.push_back(item->_file);
+        qCInfo(lcPropagator) << "item->_originalFile:"
+                             << item->_originalFile
+                             << "item->_renameTarget"
+                             << item->_renameTarget
+                             << "item->_direction"
+                             << item->_direction
+                             << "item->_instruction:"
+                             << item->_instruction;
     }
 
-    // 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);
+    // when a folder is moved on the local device we only need to perform one MOVE on the server and then just update database, so we only keep unique moves (topmost moved folder items)
+    cleanupLocallyMovedFoldersFromNestedItems(items);
+
+    //TODO: After cleanup, in case items size changes, we need to update the progress info (maybe in: void SyncEngine::slotDiscoveryFinished or move this code to slotDiscoveryFinished)
+
+    qCInfo(lcPropagator) << "END ITEMS CLEANUP";
+    for (const auto &item : items) {
+        qCInfo(lcPropagator) << "item->_originalFile:"
+                             << item->_originalFile
+                             << "item->_renameTarget"
+                             << item->_renameTarget
+                             << "item->_direction"
+                             << item->_direction
+                             << "item->_instruction:"
+                             << item->_instruction;
+    }
 
     resetDelayedUploadTasks();
     _rootJob.reset(new PropagateRootDirectory(this));
@@ -1050,7 +1101,7 @@
 
 QString OwncloudPropagator::adjustRenamedPath(const QString &original) const
 {
-    return OCC::adjustRenamedPath(_renamedDirectories, original);
+    return staticAdjustRenamedPath(_renamedDirectories, original);
 }
 
 Result<Vfs::ConvertToPlaceholderResult, QString> OwncloudPropagator::updateMetadata(const SyncFileItem &item, Vfs::UpdateMetadataTypes updateType)
@@ -1080,6 +1131,11 @@
     return Vfs::ConvertToPlaceholderResult::Ok;
 }
 
+QString OwncloudPropagator::staticAdjustRenamedPath(const QMap<QString, QString> &renamedDirectories, const QString &original)
+{
+    return OCC::adjustRenamedPath(renamedDirectories, original);
+}
+
 bool OwncloudPropagator::isDelayedUploadItem(const SyncFileItemPtr &item) const
 {
     const auto checkFileShouldBeEncrypted = [this] (const SyncFileItemPtr &item) -> bool {

src/libsync/owncloudpropagator.h

@@ -15,7 +15,7 @@
 #ifndef OWNCLOUDPROPAGATOR_H
 #define OWNCLOUDPROPAGATOR_H
 
 #include <QHash>
 #include <QObject>
 #include <QMap>
 #include <QElapsedTimer>
@@ -629,6 +629,8 @@
                                                                                  SyncJournalDb * const journal,
                                                                                  Vfs::UpdateMetadataTypes updateType);
 
+    static QString staticAdjustRenamedPath(const QMap<QString, QString> &renamedDirectories, const QString &original);
+
     Q_REQUIRED_RESULT bool isDelayedUploadItem(const SyncFileItemPtr &item) const;
 
     Q_REQUIRED_RESULT const std::deque<SyncFileItemPtr>& delayedTasks() const
@@ -695,6 +697,7 @@
     void resetDelayedUploadTasks();
 
     static void adjustDeletedFoldersWithNewChildren(SyncFileItemVector &items);
+    static void cleanupLocallyMovedFoldersFromNestedItems(SyncFileItemVector &items);
 
     AccountPtr _account;
     QScopedPointer<PropagateRootDirectory> _rootJob;

src/libsync/propagateremotemove.cpp

@@ -1,4 +1,4 @@
 /*
  * Copyright (C) by Olivier Goffart <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -243,26 +243,29 @@
     // reopens the db successfully.
     // The db is only queried to transfer the content checksum from the old
     // to the new record. It is not a problem to skip it here.
+    QString origin = propagator()->adjustRenamedPath(_item->_originalFile);
     SyncJournalFileRecord oldRecord;
-    if (!propagator()->_journal->getFileRecord(_item->_originalFile, &oldRecord)) {
-        qCWarning(lcPropagateRemoteMove) << "could not get file from local DB" << _item->_originalFile;
-        done(SyncFileItem::NormalError, tr("could not get file %1 from local DB").arg(_item->_originalFile), ErrorCategory::GenericError);
+    if (!propagator()->_journal->getFileRecord(origin, &oldRecord)) {
+        qCWarning(lcPropagateRemoteMove) << "could not get file from local DB" << origin;
+        done(SyncFileItem::NormalError, tr("could not get file %1 from local DB").arg(origin), ErrorCategory::GenericError);
         return;
     }
     auto &vfs = propagator()->syncOptions()._vfs;
-    auto pinState = vfs->pinState(_item->_originalFile);
+    // TODO: vfs->pinState(origin); does not make sense as item is already gone from original location, do we need this?
+    auto pinState = vfs->pinState(origin);
 
     const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget);
 
     if (FileSystem::fileExists(targetFile)) {
         // Delete old db data.
-        if (!propagator()->_journal->deleteFileRecord(_item->_originalFile)) {
-            qCWarning(lcPropagateRemoteMove) << "could not delete file from local DB" << _item->_originalFile;
-            done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(_item->_originalFile), ErrorCategory::GenericError);
+        if (!propagator()->_journal->deleteFileRecord(origin)) {
+            qCWarning(lcPropagateRemoteMove) << "could not delete file from local DB" << origin;
+            done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(origin), ErrorCategory::GenericError);
             return;
         }
-        if (!vfs->setPinState(_item->_originalFile, PinState::Inherited)) {
-            qCWarning(lcPropagateRemoteMove) << "Could not set pin state of" << _item->_originalFile << "to inherited";
+        // TODO: vfs->setPinState(origin, PinState::Inherited) will always fail as item is already gone from original location, do we need this?
+        if (!vfs->setPinState(origin, PinState::Inherited)) {
+            qCWarning(lcPropagateRemoteMove) << "Could not set pin state of" << origin << "to inherited";
         }
     }
 
@@ -306,6 +309,70 @@
         }
     }
 
+    if (_item->isDirectory()) {
+        if (!propagator()->_journal->relocateFolderToNewPathRecursively(origin.toUtf8(), _item->_renameTarget.toUtf8())) {
+            done(SyncFileItem::FatalError, tr("Failed to move folder: %1").arg(_item->_file), ErrorCategory::GenericError);
+            return;
+        }
+
+        if (vfs->mode() != Vfs::Off && vfs->mode() != Vfs::WindowsCfApi) {
+            // the following slow code is only useful for VFS with suffix which is used for TestSyncVirtualFiles::testPinStateLocals test case
+            // TODO: Get rid of the TestSyncVirtualFiles::testPinStateLocals or change it, native Virtual Files (e.g. CfAPI do not need this code as pinstate is moved with corresponding placeholder)
+            if (!propagator()->_journal->getFilesBelowPath(_item->_renameTarget.toUtf8(), [&](const SyncJournalFileRecord &rec) {
+                    // TODO: not sure if this is needed, inode seems to never change for move/rename
+                    auto newItem = SyncFileItem::fromSyncJournalFileRecord(rec);
+                    newItem->_originalFile = QString(newItem->_file).replace(_item->_renameTarget, origin);
+                    newItem->_renameTarget = newItem->_file;
+                    newItem->_instruction = CSYNC_INSTRUCTION_RENAME;
+                    newItem->_direction = SyncFileItem::Up;
+                    const auto fsPath = propagator()->fullLocalPath(newItem->_renameTarget);
+                    quint64 inode = rec._inode;
+                    if (!FileSystem::getInode(fsPath, &inode)) {
+                        qCWarning(lcPropagateRemoteMove) << "Could not get inode for moved file" << fsPath;
+                        return;
+                    }
+                    if (inode != rec._inode) {
+                        auto newRec = rec;
+                        newRec._inode = inode;
+                        if (!propagator()->_journal->setFileRecord(newRec)) {
+                            qCWarning(lcPropagateRemoteMove) << "Could not update inode for moved file" << newRec.path();
+                            return;
+                        }
+                    }
+                    auto &vfs = propagator()->syncOptions()._vfs;
+                    // TODO: vfs->pinState(origin); does not make sense as item is already gone from original location, do we need this?
+                    auto pinState = vfs->pinState(newItem->_originalFile);
+                    const auto targetFile = propagator()->fullLocalPath(newItem->_renameTarget);
+
+                    if (QFileInfo::exists(targetFile)) {
+                        // Delete old db data.
+                        if (!propagator()->_journal->deleteFileRecord(newItem->_originalFile)) {
+                            qCWarning(lcPropagateRemoteMove) << "could not delete file from local DB" << newItem->_originalFile;
+                        }
+                        // TODO: vfs->setPinState(origin, PinState::Inherited) will always fail as item is already gone from original location, do we
+                        // need this?
+                        if (!vfs->setPinState(newItem->_originalFile, PinState::Inherited)) {
+                            qCWarning(lcPropagateRemoteMove) << "Could not set pin state of" << newItem->_originalFile << "to inherited";
+                        }
+                    }
+                    const auto result = propagator()->updateMetadata(*newItem);
+                    if (!result) {
+                        done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error()), ErrorCategory::GenericError);
+                        return;
+                    } else if (*result == Vfs::ConvertToPlaceholderResult::Locked) {
+                        done(SyncFileItem::SoftError, tr("The file %1 is currently in use").arg(newItem->_file), ErrorCategory::GenericError);
+                        return;
+                    }
+                    if (pinState && *pinState != PinState::Inherited && !vfs->setPinState(newItem->_renameTarget, *pinState)) {
+                        done(SyncFileItem::NormalError, tr("Error setting pin state"), ErrorCategory::GenericError);
+                        return;
+                    }
+                })) {
+                qCWarning(lcPropagateRemoteMove) << "Could not update inode for moved files in" << _item->_renameTarget;
+            }
+        }
+    }
+
     propagator()->_journal->commit("Remote Rename");
     done(SyncFileItem::Success, {}, ErrorCategory::NoError);
 }