SyncEngine: Fix renaming a folder should keep the selective sync state

Issue #5224

Two problems:

 - In the discovery phase, we need to check the selective sync entries of
   the source path in case of renames.

 - When the rename is done, we need to actually update the black list in the
   database.

diff --git a/csync/src/csync_rename.cc b/csync/src/csync_rename.cc
index f7c77a4ea27dc1af901ad1480a9df1c175116c41..5cc43ce22f59113261f0b2aebfac63f7b5a36d02 100644 (file)
--- a/csync/src/csync_rename.cc
+++ b/csync/src/csync_rename.cc
@@ -20,6 +20,7 @@
 
 extern "C" {
 #include "csync_private.h"
+#include "csync_rename.h"
 }
 
 #include <map>
@@ -93,5 +94,9 @@ char* csync_rename_adjust_path_source(CSYNC* ctx, const char* path)
     return c_strdup(path);
 }
 
+bool csync_rename_count(CSYNC *ctx) {
+    csync_rename_s* d = csync_rename_s::get(ctx);
+    return d->folder_renamed_from.size();
+}
 
 }

diff --git a/csync/src/csync_rename.h b/csync/src/csync_rename.h
index 8aa2e60c7d811e9327ac2e85fe0dfb809742e3a9..53968324c95bcb2d77aa912f16ba9900126d51cd 100644 (file)
--- a/csync/src/csync_rename.h
+++ b/csync/src/csync_rename.h
@@ -32,6 +32,8 @@ char OCSYNC_EXPORT *csync_rename_adjust_path(CSYNC *ctx, const char *path);
 char OCSYNC_EXPORT *csync_rename_adjust_path_source(CSYNC *ctx, const char *path);
 void OCSYNC_EXPORT csync_rename_destroy(CSYNC *ctx);
 void OCSYNC_EXPORT csync_rename_record(CSYNC *ctx, const char *from, const char *to);
+/*  Return the amount of renamed item recorded */
+bool OCSYNC_EXPORT csync_rename_count(CSYNC *ctx);
 
 #ifdef __cplusplus
 }

diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp
index b55ecbf62e9f19c7950b13b3646bb5ff595e5112..bb35f1f97f8a58ddb30a202be4ce6931f5787dba 100644 (file)
--- a/src/libsync/discoveryphase.cpp
+++ b/src/libsync/discoveryphase.cpp
@@ -14,6 +14,7 @@
 
 #include "discoveryphase.h"
 #include <csync_private.h>
+#include <csync_rename.h>
 #include <qdebug.h>
 
 #include <QUrl>
@@ -51,7 +52,7 @@ static bool findPathInList(const QStringList &list, const QString &path)
     return pathSlash.startsWith(*it);
 }
 
-bool DiscoveryJob::isInSelectiveSyncBlackList(const QString& path) const
+bool DiscoveryJob::isInSelectiveSyncBlackList(const char *path) const
 {
     if (_selectiveSyncBlackList.isEmpty()) {
         // If there is no black list, everything is allowed
@@ -59,13 +60,25 @@ bool DiscoveryJob::isInSelectiveSyncBlackList(const QString& path) const
     }
 
     // Block if it is in the black list
-    return findPathInList(_selectiveSyncBlackList, path);
+    if (findPathInList(_selectiveSyncBlackList, QString::fromUtf8(path))) {
+        return true;
+    }
+
+    // Also try to adjust the path if there was renames
+    if (csync_rename_count(_csync_ctx)) {
+        QScopedPointer<char, QScopedPointerPodDeleter> adjusted(
+            csync_rename_adjust_path_source(_csync_ctx, path));
+        if (strcmp(adjusted.data(), path) != 0) {
+            return findPathInList(_selectiveSyncBlackList, QString::fromUtf8(adjusted.data()));
+        }
+    }
 
+    return false;
 }
 
 int DiscoveryJob::isInSelectiveSyncBlackListCallback(void *data, const char *path)
 {
-    return static_cast<DiscoveryJob*>(data)->isInSelectiveSyncBlackList(QString::fromUtf8(path));
+    return static_cast<DiscoveryJob*>(data)->isInSelectiveSyncBlackList(path);
 }
 
 bool DiscoveryJob::checkSelectiveSyncNewFolder(const QString& path)

diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h
index b442d9a2031bd1b210a9162b160481442e0e3fb2..c030adb3339177014698e87e860148f767d1950d 100644 (file)
--- a/src/libsync/discoveryphase.h
+++ b/src/libsync/discoveryphase.h
@@ -174,7 +174,7 @@ class DiscoveryJob : public QObject {
      * return true if the given path should be ignored,
      * false if the path should be synced
      */
-    bool isInSelectiveSyncBlackList(const QString &path) const;
+    bool isInSelectiveSyncBlackList(const char* path) const;
     static int isInSelectiveSyncBlackListCallback(void *, const char *);
     bool checkSelectiveSyncNewFolder(const QString &path);
     static int checkSelectiveSyncNewFolderCallback(void*, const char*);

diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp
index 5a75e761e96576b30b8d588be3b45c133e467307..bc0b4efa16eaaf5a4d4408f2bc60a652269fdbb1 100644 (file)
--- a/src/libsync/propagateremotemove.cpp
+++ b/src/libsync/propagateremotemove.cpp
@@ -13,6 +13,7 @@
  */
 
 #include "propagateremotemove.h"
+#include "propagatorjobs.h"
 #include "owncloudpropagator_p.h"
 #include "account.h"
 #include "syncjournalfilerecord.h"
@@ -175,10 +176,45 @@ void PropagateRemoteMove::finalize()
         done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
         return;
     }
+
+    if (_item->_isDirectory) {
+        if (!adjustSelectiveSync(_propagator->_journal, _item->_file, _item->_renameTarget)) {
+            done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
+            return;
+        }
+    }
+
     _propagator->_journal->commit("Remote Rename");
     done(SyncFileItem::Success);
 }
 
+bool PropagateRemoteMove::adjustSelectiveSync(SyncJournalDb *journal, const QString &from_, const QString &to_)
+{
+    bool ok;
+    // We only care about preserving the blacklist.   The white list should anyway be empty.
+    // And the undecided list will be repopulated on the next sync, if there is anything too big.
+    QStringList list = journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok);
+    if (!ok)
+        return false;
+
+    bool changed = false;
+    Q_ASSERT(!from_.endsWith(QLatin1String("/")));
+    Q_ASSERT(!to_.endsWith(QLatin1String("/")));
+    QString from = from_ + QLatin1String("/");
+    QString to = to_ + QLatin1String("/");
+
+    for (auto it = list.begin(); it != list.end(); ++it) {
+        if (it->startsWith(from)) {
+            *it = it->replace(0, from.size(), to);
+            changed = true;
+        }
+    }
+
+    if (changed) {
+        journal->setSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, list);
+    }
+    return true;
+}
 
 }
 

diff --git a/src/libsync/propagateremotemove.h b/src/libsync/propagateremotemove.h
index 3ce54f4a809843b6fb5a6217c450769383ae1dd3..829e7a1fb35ef9d23df824980dfbd5016cc488d3 100644 (file)
--- a/src/libsync/propagateremotemove.h
+++ b/src/libsync/propagateremotemove.h
@@ -51,6 +51,12 @@ public:
     void start() Q_DECL_OVERRIDE;
     void abort() Q_DECL_OVERRIDE;
     JobParallelism parallelism() Q_DECL_OVERRIDE { return OCC::PropagatorJob::WaitForFinishedInParentDirectory; }
+
+    /**
+     * Rename the directory in the selective sync list
+     */
+    static bool adjustSelectiveSync(SyncJournalDb *journal, const QString &from, const QString &to);
+
 private slots:
     void slotMoveJobFinished();
     void finalize();

diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
index 58c599dc1b77e297e6b85a496777094c195b91ea..272a0b879ce43568408479715a213332f135b7d5 100644 (file)
--- a/src/libsync/propagatorjobs.cpp
+++ b/src/libsync/propagatorjobs.cpp
@@ -15,7 +15,7 @@
 
 #include "propagatorjobs.h"
 #include "owncloudpropagator_p.h"
-
+#include "propagateremotemove.h"
 #include "utility.h"
 #include "syncjournaldb.h"
 #include "syncjournalfilerecord.h"
@@ -231,6 +231,7 @@ void PropagateLocalRename::start()
     _propagator->_journal->deleteFileRecord(_item->_originalFile);
 
     // store the rename file name in the item.
+    const auto oldFile = _item->_file;
     _item->_file = _item->_renameTarget;
 
     SyncJournalFileRecord record(*_item, targetFile);
@@ -245,9 +246,14 @@ void PropagateLocalRename::start()
             done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
             return;
         }
+    } else {
+        if (!PropagateRemoteMove::adjustSelectiveSync(_propagator->_journal, oldFile, _item->_renameTarget)) {
+            done(SyncFileItem::FatalError, tr("Error writing metadata to the database"));
+            return;
+        }
     }
-    _propagator->_journal->commit("localRename");
 
+    _propagator->_journal->commit("localRename");
 
     done(SyncFileItem::Success);
 }

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
index 570952c9c0a686ba37dd591350bb4a22dc1062bf..883837c6d9948e8d3013c728e9df9fab4677ef4a 100644 (file)
--- a/test/syncenginetestutils.h
+++ b/test/syncenginetestutils.h
@@ -28,6 +28,7 @@ inline QByteArray generateFileId() {
 
 class PathComponents : public QStringList {
 public:
+    PathComponents(const char *path) : PathComponents{QString::fromUtf8(path)} {}
     PathComponents(const QString &path) : QStringList{path.split('/', QString::SkipEmptyParts)} { }
     PathComponents(const QStringList &pathComponents) : QStringList{pathComponents} { }
 
@@ -90,8 +91,9 @@ public:
     void mkdir(const QString &relativePath) override {
         _rootDir.mkpath(relativePath);
     }
-    void rename(const QString &, const QString &) override {
-        Q_ASSERT(!"not implemented");
+    void rename(const QString &from, const QString &to) override {
+        QVERIFY(_rootDir.exists(from));
+        QVERIFY(_rootDir.rename(from, to));
     }
 };
 
@@ -457,6 +459,36 @@ public:
     qint64 readData(char *, qint64) override { return 0; }
 };
 
+class FakeMoveReply : public QNetworkReply
+{
+    Q_OBJECT
+public:
+    FakeMoveReply(FileInfo &remoteRootFileInfo, QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent)
+    : QNetworkReply{parent} {
+        setRequest(request);
+        setUrl(request.url());
+        setOperation(op);
+        open(QIODevice::ReadOnly);
+
+        Q_ASSERT(request.url().path().startsWith(sRootUrl.path()));
+        QString fileName = request.url().path().mid(sRootUrl.path().length());
+        QString destPath = request.rawHeader("Destination");
+        Q_ASSERT(destPath.startsWith(sRootUrl.path()));
+        QString dest = destPath.mid(sRootUrl.path().length());
+        remoteRootFileInfo.rename(fileName, dest);
+        QMetaObject::invokeMethod(this, "respond", Qt::QueuedConnection);
+    }
+
+    Q_INVOKABLE void respond() {
+        setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 201);
+        emit metaDataChanged();
+        emit finished();
+    }
+
+    void abort() override { }
+    qint64 readData(char *, qint64) override { return 0; }
+};
+
 class FakeGetReply : public QNetworkReply
 {
     Q_OBJECT
@@ -552,6 +584,8 @@ protected:
             return new FakeMkcolReply{_remoteRootFileInfo, op, request, this};
         else if (verb == QLatin1String("DELETE"))
             return new FakeDeleteReply{_remoteRootFileInfo, op, request, this};
+        else if (verb == QLatin1String("MOVE"))
+            return new FakeMoveReply{_remoteRootFileInfo, op, request, this};
         else {
             qDebug() << verb << outgoingData;
             Q_UNREACHABLE();

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
index bfb44e22bcfd598a4fbc80d46561d9e0e7c64501..f54ff1110e1ad119a354aa72fdb400eaf2d8f94e 100644 (file)
--- a/test/testsyncengine.cpp
+++ b/test/testsyncengine.cpp
@@ -7,6 +7,7 @@
 
 #include <QtTest>
 #include "syncenginetestutils.h"
+#include <syncengine.h>
 
 using namespace OCC;
 
@@ -137,8 +138,8 @@ private slots:
         fakeFolder.syncOnce();
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         auto oldState = fakeFolder.currentLocalState();
-        QVERIFY(oldState.find(PathComponents("folder/folderB/folderA/file.txt")));
-        QVERIFY(!oldState.find(PathComponents("folder/folderA/file.txt")));
+        QVERIFY(oldState.find("folder/folderB/folderA/file.txt"));
+        QVERIFY(!oldState.find("folder/folderA/file.txt"));
 
         // This sync should not remove the file
         fakeFolder.syncOnce();
@@ -146,6 +147,72 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), oldState);
 
     }
+
+    void testSelectiveSyncModevFolder() {
+        // issue #5224
+        FakeFolder fakeFolder{FileInfo{ QString(), {
+            FileInfo { QStringLiteral("parentFolder"), {
+                FileInfo{ QStringLiteral("subFolderA"), { { QStringLiteral("fileA.txt"), 400 } } },
+                FileInfo{ QStringLiteral("subFolderB"), { { QStringLiteral("fileB.txt"), 400 } } }
+            }
+        }}}};
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        auto expectedServerState = fakeFolder.currentRemoteState();
+
+        // Remove subFolderA with selectiveSync:
+        fakeFolder.syncEngine().journal()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList,
+                                                                {"parentFolder/subFolderA/"});
+        fakeFolder.syncEngine().journal()->avoidReadFromDbOnNextSync("parentFolder/subFolderA/");
+
+        fakeFolder.syncOnce();
+
+        {
+            // Nothing changed on the server
+            QCOMPARE(fakeFolder.currentRemoteState(), expectedServerState);
+            // The local state should not have subFolderA
+            auto remoteState = fakeFolder.currentRemoteState();
+            remoteState.remove("parentFolder/subFolderA");
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+        }
+
+        // Rename parentFolder on the server
+        fakeFolder.remoteModifier().rename("parentFolder", "parentFolderRenamed");
+        expectedServerState = fakeFolder.currentRemoteState();
+        fakeFolder.syncOnce();
+
+        {
+            QCOMPARE(fakeFolder.currentRemoteState(), expectedServerState);
+            auto remoteState = fakeFolder.currentRemoteState();
+            // The subFolderA should still be there on the server.
+            QVERIFY(remoteState.find("parentFolderRenamed/subFolderA/fileA.txt"));
+            // But not on the client because of the selective sync
+            remoteState.remove("parentFolderRenamed/subFolderA");
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+        }
+
+        // Rename it again, locally this time.
+        fakeFolder.localModifier().rename("parentFolderRenamed", "parentThirdName");
+        fakeFolder.syncOnce();
+
+        {
+            auto remoteState = fakeFolder.currentRemoteState();
+            // The subFolderA should still be there on the server.
+            QVERIFY(remoteState.find("parentThirdName/subFolderA/fileA.txt"));
+            // But not on the client because of the selective sync
+            remoteState.remove("parentThirdName/subFolderA");
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+
+            expectedServerState = fakeFolder.currentRemoteState();
+            QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItem &, const PropagatorJob &)));
+            fakeFolder.syncOnce(); // This sync should do nothing
+            QCOMPARE(completeSpy.count(), 0);
+
+            QCOMPARE(fakeFolder.currentRemoteState(), expectedServerState);
+            QCOMPARE(fakeFolder.currentLocalState(), remoteState);
+        }
+    }
+
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)