SyncEngine: Fix renaming of folder when file are changed (#5195)

Two bugs:
 - The change filed are not considered as move, they are re-downloaded
   but the old file was not removed from the database. The change in
   owncloudpropagator.cpp takes care of removing the old entries.

 - Next sync would then remove the file in the server in the old folder
   This was not a problem until we start reusing the sync engine, and
   that the _renamedFolders map is not cleared. We were before deleting
   a non-existing file. But now we delete the actual file.

Also improve the tests to be able to do move on the server.
This include support for file id.

Issue #5192

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
index c875dcfd7bff5ca73aeec810e11bf8ce3496bacb..1ff550073d467bd5eff20da233f8b12f0f99f295 100644 (file)
--- a/src/libsync/owncloudpropagator.cpp
+++ b/src/libsync/owncloudpropagator.cpp
@@ -668,6 +668,12 @@ void PropagateDirectory::finalize()
     bool ok = true;
     if (!_item->isEmpty() && _hasError == SyncFileItem::NoStatus) {
         if( !_item->_renameTarget.isEmpty() ) {
+            if(_item->_instruction == CSYNC_INSTRUCTION_RENAME
+                    && _item->_originalFile != _item->_renameTarget) {
+                // Remove the stale entries from the database.
+                _propagator->_journal->deleteFileRecord(_item->_originalFile, true);
+            }
+
             _item->_file = _item->_renameTarget;
         }
 

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
index fa7b01725af1848aae824a6cc4e55cf848e82e49..9b9a79e51b56d427eba593455b2a6307045b06e8 100644 (file)
--- a/src/libsync/syncengine.cpp
+++ b/src/libsync/syncengine.cpp
@@ -868,6 +868,7 @@ void SyncEngine::slotDiscoveryJobFinished(int discoveryResult)
     bool walkOk = true;
     _seenFiles.clear();
     _temporarilyUnavailablePaths.clear();
+    _renamedFolders.clear();
 
     if( csync_walk_local_tree(_csync_ctx, &treewalkLocal, 0) < 0 ) {
         qDebug() << "Error in local treewalk.";

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
index 76b2855a279c4262623ee8b75a84a27fa03216ae..570952c9c0a686ba37dd591350bb4a22dc1062bf 100644 (file)
--- a/test/syncenginetestutils.h
+++ b/test/syncenginetestutils.h
@@ -14,13 +14,17 @@
 
 #include <QDir>
 #include <QNetworkReply>
+#include <QMap>
 #include <QtTest>
 
 static const QUrl sRootUrl("owncloud://somehost/owncloud/remote.php/webdav/");
 
-static QString generateEtag() {
+inline QString generateEtag() {
     return QString::number(QDateTime::currentDateTime().toMSecsSinceEpoch(), 16);
 }
+inline QByteArray generateFileId() {
+    return QByteArray::number(qrand(), 16);
+}
 
 class PathComponents : public QStringList {
 public:
@@ -44,6 +48,7 @@ public:
     virtual void setContents(const QString &relativePath, char contentChar) = 0;
     virtual void appendByte(const QString &relativePath) = 0;
     virtual void mkdir(const QString &relativePath) = 0;
+    virtual void rename(const QString &relativePath, const QString &relativeDestinationDirectory) = 0;
 };
 
 class DiskFileModifier : public FileModifier
@@ -85,6 +90,9 @@ public:
     void mkdir(const QString &relativePath) override {
         _rootDir.mkpath(relativePath);
     }
+    void rename(const QString &, const QString &) override {
+        Q_ASSERT(!"not implemented");
+    }
 };
 
 class FileInfo : public FileModifier
@@ -125,6 +133,7 @@ public:
         for (const auto &source : children) {
             auto &dest = this->children[source.name] = source;
             dest.parentPath = p;
+            dest.fixupParentPathRecursively();
         }
     }
 
@@ -156,6 +165,21 @@ public:
         createDir(relativePath);
     }
 
+    void rename(const QString &oldPath, const QString &newPath) override {
+        const PathComponents newPathComponents{newPath};
+        FileInfo *dir = findInvalidatingEtags(newPathComponents.parentDirComponents());
+        Q_ASSERT(dir);
+        Q_ASSERT(dir->isDir);
+        const PathComponents pathComponents{oldPath};
+        FileInfo *parent = findInvalidatingEtags(pathComponents.parentDirComponents());
+        Q_ASSERT(parent);
+        FileInfo fi = parent->children.take(pathComponents.fileName());
+        fi.parentPath = dir->path();
+        fi.name = newPathComponents.fileName();
+        fi.fixupParentPathRecursively();
+        dir->children.insert(newPathComponents.fileName(), std::move(fi));
+    }
+
     FileInfo *find(const PathComponents &pathComponents, const bool invalidateEtags = false) {
         if (pathComponents.isEmpty()) {
             if (invalidateEtags)
@@ -217,6 +241,7 @@ public:
     bool isShared = false;
     QDateTime lastModified = QDateTime::currentDateTime().addDays(-7);
     QString etag = generateEtag();
+    QByteArray fileId = generateFileId();
     qint64 size = 0;
     char contentChar = 'W';
 
@@ -228,6 +253,19 @@ private:
     FileInfo *findInvalidatingEtags(const PathComponents &pathComponents) {
         return find(pathComponents, true);
     }
+
+    void fixupParentPathRecursively() {
+        auto p = path();
+        for (auto it = children.begin(); it != children.end(); ++it) {
+            Q_ASSERT(it.key() == it->name);
+            it->parentPath = p;
+            it->fixupParentPathRecursively();
+        }
+    }
+
+    friend inline QDebug operator<<(QDebug dbg, const FileInfo& fi) {
+        return dbg << "{ " << fi.path() << ": " << fi.children;
+    }
 };
 
 class FakePropfindReply : public QNetworkReply
@@ -273,6 +311,7 @@ public:
             xml.writeTextElement(davUri, QStringLiteral("getcontentlength"), QString::number(fileInfo.size));
             xml.writeTextElement(davUri, QStringLiteral("getetag"), fileInfo.etag);
             xml.writeTextElement(ocUri, QStringLiteral("permissions"), fileInfo.isShared ? QStringLiteral("SRDNVCKW") : QStringLiteral("RDNVCKW"));
+            xml.writeTextElement(ocUri, QStringLiteral("id"), fileInfo.fileId);
             xml.writeEndElement(); // prop
             xml.writeTextElement(davUri, QStringLiteral("status"), "HTTP/1.1 200 OK");
             xml.writeEndElement(); // propstat
@@ -282,6 +321,7 @@ public:
         Q_ASSERT(request.url().path().startsWith(sRootUrl.path()));
         QString fileName = request.url().path().mid(sRootUrl.path().length());
         const FileInfo *fileInfo = remoteRootFileInfo.find(fileName);
+        Q_ASSERT(fileInfo);
 
         writeFileResponse(*fileInfo);
         foreach(const FileInfo &childFileInfo, fileInfo->children)
@@ -380,7 +420,7 @@ public:
     }
 
     Q_INVOKABLE void respond() {
-        // FIXME: setRawHeader("OC-FileId", fileInfo->???);
+        setRawHeader("OC-FileId", fileInfo->fileId);
         setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 201);
         emit metaDataChanged();
         emit finished();
@@ -443,6 +483,7 @@ public:
         setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 200);
         setRawHeader("OC-ETag", fileInfo->etag.toLatin1());
         setRawHeader("ETag", fileInfo->etag.toLatin1());
+        setRawHeader("OC-FileId", fileInfo->fileId);
         emit metaDataChanged();
         if (bytesAvailable())
             emit readyRead();

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
index 55d2ce614064a6a67a149b01e11a7cad09d76b9a..bfb44e22bcfd598a4fbc80d46561d9e0e7c64501 100644 (file)
--- a/test/testsyncengine.cpp
+++ b/test/testsyncengine.cpp
@@ -119,6 +119,33 @@ private slots:
         QVERIFY(itemDidCompleteSuccessfully(completeSpy, "a3.eml"));
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
+
+    void testRemoteChangeInMovedFolder() {
+        // issue #5192
+        FakeFolder fakeFolder{FileInfo{ QString(), {
+            FileInfo { QStringLiteral("folder"), {
+                FileInfo{ QStringLiteral("folderA"), { { QStringLiteral("file.txt"), 400 } } },
+                QStringLiteral("folderB")
+            }
+        }}}};
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        // Edit a file in a moved directory.
+        fakeFolder.remoteModifier().setContents("folder/folderA/file.txt", 'a');
+        fakeFolder.remoteModifier().rename("folder/folderA", "folder/folderB/folderA");
+        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")));
+
+        // This sync should not remove the file
+        fakeFolder.syncOnce();
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QCOMPARE(fakeFolder.currentLocalState(), oldState);
+
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncEngine)