Move dehydration into updateMetadata

7 files changed, 46 insertions, 85 deletions

diff --git a/changelog/unreleased/9257 b/changelog/unreleased/9257
new file mode 100644
index 000000000..56626eee6
--- /dev/null
+++ b/changelog/unreleased/9257
@@ -0,0 +1,7 @@
+Bugfix: Fix failing dehydration causing files to be moved to trash
+
+If files where dehydrated by the user the action could fail under certain conditions
+which caused a deletion of the file.
+
+https://github.com/owncloud/client/pull/9257
+https://github.com/owncloud/client-desktop-vfs-win/pull/9
diff --git a/src/common/vfs.h b/src/common/vfs.h
index d2c768e8f..983c99910 100644
--- a/src/common/vfs.h
+++ b/src/common/vfs.h
@@ -164,13 +164,6 @@ public:
     /// Create a new dehydrated placeholder. Called from PropagateDownload.
     virtual OC_REQUIRED_RESULT 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 OC_REQUIRED_RESULT Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) = 0;
-
     /** Discovery hook: even unchanged files may need UPDATE_METADATA.
      *
      * For instance cfapi vfs wants local hydrated non-placeholder files to
@@ -291,7 +284,6 @@ public:
     bool isHydrating() const override { return false; }
 
     Result<void, QString> createPlaceholder(const SyncFileItem &) override { return {}; }
-    Result<void, QString> dehydratePlaceholder(const SyncFileItem &) override { return {}; }
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }
     bool isDehydratedPlaceholder(const QString &) override { return false; }
diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
index a7e3dc005..6f0874d8a 100644
--- a/src/libsync/owncloudpropagator.cpp
+++ b/src/libsync/owncloudpropagator.cpp
@@ -756,6 +756,18 @@ QString OwncloudPropagator::adjustRenamedPath(const QString &original) const
 
 Result<Vfs::ConvertToPlaceholderResult, QString> OwncloudPropagator::updatePlaceholder(const SyncFileItem &item, const QString &fileName, const QString &replacesFile)
 {
+    Q_ASSERT([&] {
+        if (item._type == ItemTypeVirtualFileDehydration) {
+            // when dehydrating the file must not be pinned
+            // don't use destinatio() with suffix placeholder
+            const auto pin = syncOptions()._vfs->pinState(item._file);
+            if (pin && pin.get() == PinState::AlwaysLocal) {
+                qDebug() << fileName << item.destination() << item._file;
+                return false;
+            }
+        }
+        return true;
+    }());
     return syncOptions()._vfs->updateMetadata(item, fileName, replacesFile);
 }
 
diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp
index b91a2e35a..7f491eecd 100644
--- a/src/libsync/propagatedownload.cpp
+++ b/src/libsync/propagatedownload.cpp
@@ -370,12 +370,6 @@ void PropagateDownloadFile::start()
             return;
         }
         qCDebug(lcPropagateDownload) << "dehydrating file" << _item->_file;
-        auto r = vfs->dehydratePlaceholder(*_item);
-        if (!r) {
-            done(SyncFileItem::NormalError, r.error());
-            return;
-        }
-        propagator()->_journal->deleteFileRecord(_item->_originalFile);
         updateMetadata(false);
         return;
     }
diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp
index 1641c8383..223655d14 100644
--- a/src/libsync/vfs/suffix/vfs_suffix.cpp
+++ b/src/libsync/vfs/suffix/vfs_suffix.cpp
@@ -72,7 +72,28 @@ bool VfsSuffix::isHydrating() const
 
 Result<Vfs::ConvertToPlaceholderResult, QString> VfsSuffix::updateMetadata(const SyncFileItem &item, const QString &filePath, const QString &)
 {
-    FileSystem::setModTime(filePath, item._modtime);
+    if (item._type == ItemTypeVirtualFileDehydration) {
+        SyncFileItem virtualItem(item);
+        virtualItem._file = item._renameTarget;
+        auto r = createPlaceholder(virtualItem);
+        if (!r) {
+            return r.error();
+        }
+        // Move the item's pin state
+        auto pin = _setupParams.journal->internalPinStates().rawForPath(item._file.toUtf8());
+        if (pin && *pin != PinState::Inherited) {
+            setPinState(item._renameTarget, *pin);
+        }
+        if (item._file != item._renameTarget) { // can be the same when renaming foo -> foo.owncloud to dehydrate
+            QString error;
+            if (!FileSystem::remove(_setupParams.filesystemPath + item._file, &error)) {
+                return error;
+            }
+        }
+        _setupParams.journal->deleteFileRecord(item._originalFile);
+    } else {
+        OC_ASSERT(FileSystem::setModTime(filePath, item._modtime));
+    }
     return Vfs::ConvertToPlaceholderResult::Ok;
 }
 
@@ -85,41 +106,15 @@ Result<void, QString> VfsSuffix::createPlaceholder(const SyncFileItem &item)
     QFile file(fn);
     if (file.exists() && file.size() > 1
         && !FileSystem::verifyFileUnchanged(fn, item._size, item._modtime)) {
-        return QStringLiteral("Cannot create a placeholder because a file with the placeholder name already exist");
+        return tr("Cannot create a placeholder because a file with the placeholder name already exist");
     }
 
-    if (!file.open(QFile::ReadWrite | QFile::Truncate))
+    if (!file.open(QFile::ReadWrite | QFile::Truncate)) {
         return file.errorString();
-
+    }
     file.write(" ");
     file.close();
-    FileSystem::setModTime(fn, item._modtime);
-    return {};
-}
-
-Result<void, QString> VfsSuffix::dehydratePlaceholder(const SyncFileItem &item)
-{
-    SyncFileItem virtualItem(item);
-    virtualItem._file = item._renameTarget;
-    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());
-    if (pin && *pin != PinState::Inherited) {
-        setPinState(item._renameTarget, *pin);
-        setPinState(item._file, PinState::Inherited);
-    }
-
-    // Ensure the pin state isn't contradictory
-    pin = pinState(item._renameTarget);
-    if (pin && *pin == PinState::AlwaysLocal)
-        setPinState(item._renameTarget, PinState::Unspecified);
+    OC_ASSERT(FileSystem::setModTime(fn, item._modtime));
     return {};
 }
 
diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h
index f73a964b5..4899fc82f 100644
--- a/src/libsync/vfs/suffix/vfs_suffix.h
+++ b/src/libsync/vfs/suffix/vfs_suffix.h
@@ -42,7 +42,6 @@ public:
 
 
     Result<void, QString> createPlaceholder(const SyncFileItem &item) override;
-    Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) override;
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }
     bool isDehydratedPlaceholder(const QString &filePath) override;
diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp
index a55748c9f..627af1a2a 100644
--- a/test/testsyncvirtualfiles.cpp
+++ b/test/testsyncvirtualfiles.cpp
@@ -41,6 +41,7 @@ void triggerDownload(FakeFolder &folder, const QByteArray &path)
     journal.schedulePathForRemoteDiscovery(record._path);
 }
 
+// TODO: triggering dehydration by other means than the pin state is an unsupported scenario
 void markForDehydration(FakeFolder &folder, const QByteArray &path)
 {
     auto &journal = folder.syncJournal();
@@ -1174,45 +1175,6 @@ private slots:
         QVERIFY(fakeFolder.currentLocalState().find("onlinerenamed2/file1rename" DVSUFFIX));
         QCOMPARE(*vfs->pinState("onlinerenamed2/file1rename" DVSUFFIX), PinState::OnlineOnly);
     }
-
-    void testIncompatiblePins()
-    {
-        FakeFolder fakeFolder{ FileInfo() };
-        auto vfs = setupVfs(fakeFolder);
-        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
-
-        auto setPin = [&] (const QByteArray &path, PinState state) {
-            fakeFolder.syncJournal().internalPinStates().setForPath(path, state);
-        };
-
-        fakeFolder.remoteModifier().mkdir("local");
-        fakeFolder.remoteModifier().mkdir("online");
-        QVERIFY(fakeFolder.syncOnce());
-        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
-
-        setPin("local", PinState::AlwaysLocal);
-        setPin("online", PinState::OnlineOnly);
-
-        fakeFolder.localModifier().insert("local/file1");
-        fakeFolder.localModifier().insert("online/file1");
-        QVERIFY(fakeFolder.syncOnce());
-
-        markForDehydration(fakeFolder, "local/file1");
-        triggerDownload(fakeFolder, "online/file1");
-
-        // the sync sets the changed files pin states to unspecified
-        QVERIFY(fakeFolder.syncOnce());
-
-        QVERIFY(fakeFolder.currentLocalState().find("online/file1"));
-        QVERIFY(fakeFolder.currentLocalState().find("local/file1" DVSUFFIX));
-        QCOMPARE(*vfs->pinState("online/file1"), PinState::Unspecified);
-        QCOMPARE(*vfs->pinState("local/file1" DVSUFFIX), PinState::Unspecified);
-
-        // no change on another sync
-        QVERIFY(fakeFolder.syncOnce());
-        QVERIFY(fakeFolder.currentLocalState().find("online/file1"));
-        QVERIFY(fakeFolder.currentLocalState().find("local/file1" DVSUFFIX));
-    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncVirtualFiles)