From 85b8ab178e21cf4dcaf792f68c15e55f6e0ee3c2 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Thu, 22 Sep 2016 09:02:47 +0200 Subject: [PATCH] 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 --- src/libsync/owncloudpropagator.cpp | 6 ++++ src/libsync/syncengine.cpp | 1 + test/syncenginetestutils.h | 45 ++++++++++++++++++++++++++++-- test/testsyncengine.cpp | 27 ++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index c875dcfd7..1ff550073 100644 --- 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 fa7b01725..9b9a79e51 100644 --- 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 76b2855a2..570952c9c 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -14,13 +14,17 @@ #include #include +#include #include 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 55d2ce614..bfb44e22b 100644 --- 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)