VFS: Do not overwrite existing files by placeholder

For issue #7557 and #7556 Note: this change the API of the VFS plugin, so the VFS plugin needs small adaptations
owncloud · Nov 1, 2019 · b2d0fa1 · b2d0fa1
1 parent 9ed55c9
commit b2d0fa1
Show file tree

Hide file tree

Showing 7 changed files with 114 additions and 24 deletions.
diff --git a/src/common/result.h b/src/common/result.h
@@ -128,6 +128,18 @@ class Result
     }
 };
 
+namespace detail {
+    struct NoResultData{};
+}
+
+template <typename Error>
+class Result<void, Error> : public Result<detail::NoResultData, Error>
+{
+public:
+    using Result<detail::NoResultData, Error>::Result;
+    Result() : Result(detail::NoResultData{}) {}
+};
+
 namespace detail {
 struct OptionalNoErrorData{};
 }

diff --git a/src/common/vfs.h b/src/common/vfs.h
@@ -151,20 +151,18 @@ class OCSYNC_EXPORT Vfs : public QObject
      *
      * If the remote metadata changes, the local placeholder's metadata should possibly
      * change as well.
-     *
-     * Returning false and setting error indicates an error.
      */
-    virtual bool updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId, QString *error) = 0;
+    virtual Result<void, QString> updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) = 0;
 
     /// Create a new dehydrated placeholder. Called from PropagateDownload.
-    virtual void createPlaceholder(const SyncFileItem &item) = 0;
+    virtual Result<void, QString> createPlaceholder(const SyncFileItem &item) = 0;
 
     /** Convert a hydrated placeholder to a dehydrated one. Called from PropagateDownlaod.
      *
      * This is different from delete+create because preserving some file metadata
      * (like pin states) may be essential for some vfs plugins.
      */
-    virtual void dehydratePlaceholder(const SyncFileItem &item) = 0;
+    virtual Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) = 0;
 
     /** Discovery hook: even unchanged files may need UPDATE_METADATA.
      *
@@ -289,9 +287,9 @@ class OCSYNC_EXPORT VfsOff : public Vfs
     bool socketApiPinStateActionsShown() const override { return false; }
     bool isHydrating() const override { return false; }
 
-    bool updateMetadata(const QString &, time_t, qint64, const QByteArray &, QString *) override { return true; }
-    void createPlaceholder(const SyncFileItem &) override {}
-    void dehydratePlaceholder(const SyncFileItem &) override {}
+    Result<void, QString> updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; }
+    Result<void, QString> createPlaceholder(const SyncFileItem &) override { return {}; }
+    Result<void, QString> dehydratePlaceholder(const SyncFileItem &) override { return {}; }
     void convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override {}
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }

diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp
@@ -365,7 +365,11 @@ void PropagateDownloadFile::start()
         }
 
         qCDebug(lcPropagateDownload) << "dehydrating file" << _item->_file;
-        vfs->dehydratePlaceholder(*_item);
+        auto r = vfs->dehydratePlaceholder(*_item);
+        if (!r) {
+            done(SyncFileItem::NormalError, r.error());
+            return;
+        }
         propagator()->_journal->deleteFileRecord(_item->_originalFile);
         updateMetadata(false);
         return;
@@ -376,7 +380,11 @@ void PropagateDownloadFile::start()
     }
     if (_item->_type == ItemTypeVirtualFile) {
         qCDebug(lcPropagateDownload) << "creating virtual file" << _item->_file;
-        vfs->createPlaceholder(*_item);
+        auto r = vfs->createPlaceholder(*_item);
+        if (!r) {
+            done(SyncFileItem::NormalError, r.error());
+            return;
+        }
         updateMetadata(false);
         return;
     }

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
@@ -345,10 +345,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item)
 
             // Update on-disk virtual file metadata
             if (item->_type == ItemTypeVirtualFile) {
-                QString error;
-                if (!_syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId, &error)) {
+                auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId);
+                if (!r) {
                     item->_instruction = CSYNC_INSTRUCTION_ERROR;
-                    item->_errorString = tr("Could not update virtual file metadata: %1").arg(error);
+                    item->_errorString = tr("Could not update virtual file metadata: %1").arg(r.error());
                     return;
                 }
             }

diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp
@@ -68,34 +68,47 @@ bool VfsSuffix::isHydrating() const
     return false;
 }
 
-bool VfsSuffix::updateMetadata(const QString &filePath, time_t modtime, qint64, const QByteArray &, QString *)
+Result<void, QString> VfsSuffix::updateMetadata(const QString &filePath, time_t modtime, qint64, const QByteArray &)
 {
     FileSystem::setModTime(filePath, modtime);
-    return true;
+    return {};
 }
 
-void VfsSuffix::createPlaceholder(const SyncFileItem &item)
+Result<void, QString> VfsSuffix::createPlaceholder(const SyncFileItem &item)
 {
     // The concrete shape of the placeholder is also used in isDehydratedPlaceholder() below
     QString fn = _setupParams.filesystemPath + item._file;
     if (!fn.endsWith(fileSuffix())) {
         ASSERT(false, "vfs file isn't ending with suffix");
-        return;
+        return QString("vfs file isn't ending with suffix");
     }
 
     QFile file(fn);
-    file.open(QFile::ReadWrite | QFile::Truncate);
+    if (file.exists() && file.size() > 1
+        && !FileSystem::verifyFileUnchanged(fn, item._size, item._modtime)) {
+        return QString("Cannot create a placeholder because a file with the placeholder name already exist");
+    }
+
+    if (!file.open(QFile::ReadWrite | QFile::Truncate))
+        return file.errorString();
+
     file.write(" ");
     file.close();
     FileSystem::setModTime(fn, item._modtime);
+    return {};
 }
 
-void VfsSuffix::dehydratePlaceholder(const SyncFileItem &item)
+Result<void, QString> VfsSuffix::dehydratePlaceholder(const SyncFileItem &item)
 {
-    QFile::remove(_setupParams.filesystemPath + item._file);
     SyncFileItem virtualItem(item);
     virtualItem._file = item._renameTarget;
-    createPlaceholder(virtualItem);
+    auto r = createPlaceholder(virtualItem);
+    if (!r)
+        return r;
+
+    if (item._file != item._renameTarget) { // can be the same when renaming foo -> foo.owncloud to dehydrate
+        QFile::remove(_setupParams.filesystemPath + item._file);
+    }
 
     // Move the item's pin state
     auto pin = _setupParams.journal->internalPinStates().rawForPath(item._file.toUtf8());
@@ -108,6 +121,7 @@ void VfsSuffix::dehydratePlaceholder(const SyncFileItem &item)
     pin = pinState(item._renameTarget);
     if (pin && *pin == PinState::AlwaysLocal)
         setPinState(item._renameTarget, PinState::Unspecified);
+    return {};
 }
 
 void VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &)

diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h
@@ -38,10 +38,10 @@ class VfsSuffix : public Vfs
     bool socketApiPinStateActionsShown() const override { return true; }
     bool isHydrating() const override;
 
-    bool updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId, QString *error) override;
+    Result<void, QString> updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) override;
 
-    void createPlaceholder(const SyncFileItem &item) override;
-    void dehydratePlaceholder(const SyncFileItem &item) override;
+    Result<void, QString> createPlaceholder(const SyncFileItem &item) override;
+    Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) override;
     void convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override;
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }

diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp
@@ -603,6 +603,7 @@ private slots:
 
         QVERIFY(!fakeFolder.currentLocalState().find("A/a1"));
         QVERIFY(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX));
+        QVERIFY(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)->size <= 1);
         QVERIFY(fakeFolder.currentRemoteState().find("A/a1"));
         QVERIFY(itemInstruction(completeSpy, "A/a1" DVSUFFIX, CSYNC_INSTRUCTION_SYNC));
         QCOMPARE(dbRecord(fakeFolder, "A/a1" DVSUFFIX)._type, ItemTypeVirtualFile);
@@ -1342,6 +1343,63 @@ private slots:
         QVERIFY(fakeFolder.currentLocalState().find("online/file1"));
         QVERIFY(fakeFolder.currentLocalState().find("local/file1" DVSUFFIX));
     }
+
+    void testPlaceHolderExist() {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+        fakeFolder.remoteModifier().insert("A/a1" DVSUFFIX, 111);
+        fakeFolder.remoteModifier().insert("A/hello" DVSUFFIX, 222);
+        QVERIFY(fakeFolder.syncOnce());
+        auto vfs = setupVfs(fakeFolder);
+
+        ItemCompletedSpy completeSpy(fakeFolder);
+        auto cleanup = [&]() { completeSpy.clear(); };
+        cleanup();
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QVERIFY(itemInstruction(completeSpy, "A/a1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/hello" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+
+        fakeFolder.remoteModifier().insert("A/a2" DVSUFFIX);
+        fakeFolder.remoteModifier().insert("A/hello", 12);
+        fakeFolder.localModifier().insert("A/igno" DVSUFFIX, 123);
+        cleanup();
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/a1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/igno" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+
+        // verify that the files are still present
+        QCOMPARE(fakeFolder.currentLocalState().find("A/hello" DVSUFFIX)->size, 222);
+        QCOMPARE(*fakeFolder.currentLocalState().find("A/hello" DVSUFFIX),
+                 *fakeFolder.currentRemoteState().find("A/hello" DVSUFFIX));
+        QCOMPARE(fakeFolder.currentLocalState().find("A/igno" DVSUFFIX)->size, 123);
+
+        cleanup();
+        // Dehydrate
+        vfs->setPinState(QString(), PinState::OnlineOnly);
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QVERIFY(itemInstruction(completeSpy, "A/igno" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        // verify that the files are still present
+        QCOMPARE(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)->size, 111);
+        QCOMPARE(fakeFolder.currentLocalState().find("A/hello" DVSUFFIX)->size, 222);
+        QCOMPARE(*fakeFolder.currentLocalState().find("A/hello" DVSUFFIX),
+                 *fakeFolder.currentRemoteState().find("A/hello" DVSUFFIX));
+        QCOMPARE(*fakeFolder.currentLocalState().find("A/a1"),
+                 *fakeFolder.currentRemoteState().find("A/a1"));
+        QCOMPARE(fakeFolder.currentLocalState().find("A/igno" DVSUFFIX)->size, 123);
+
+        // Now disable vfs and check that all files are still there
+        cleanup();
+        SyncEngine::wipeVirtualFiles(fakeFolder.localPath(), fakeFolder.syncJournal(), *vfs);
+        fakeFolder.switchToVfs(QSharedPointer<Vfs>(new VfsOff));
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        QCOMPARE(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)->size, 111);
+        QCOMPARE(fakeFolder.currentLocalState().find("A/hello")->size, 12);
+        QCOMPARE(fakeFolder.currentLocalState().find("A/hello" DVSUFFIX)->size, 222);
+        QCOMPARE(fakeFolder.currentLocalState().find("A/igno" DVSUFFIX)->size, 123);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncVirtualFiles)