Vfs: Lots of tests and corrections for suffix edge cases

Avoid or deal with problems that happen when suffixed files exist on the
server or suffix and non-suffixed files exist locally.

See #7350, #7261.

src/libsync/discovery.cpp

@@ -67,12 +67,14 @@ void ProcessDirectoryJob::process()
 
     QString localDir;
 
-    //
     // Build lookup tables for local, remote and db entries.
-    // For suffix-virtual files, the key will always be the base file name
+    // For suffix-virtual files, the key will normally be the base file name
     // without the suffix.
-    //
+    // However, if foo and foo.owncloud exists locally, there'll be "foo"
+    // with local, db, server entries and "foo.owncloud" with only a local
+    // entry.
     struct Entries {
+        QString nameOverride;
         SyncJournalFileRecord dbEntry;
         RemoteInfo serverEntry;
         LocalInfo localEntry;
@@ -98,17 +100,36 @@ void ProcessDirectoryJob::process()
     }
 
     for (auto &e : _localNormalQueryEntries) {
-        // Normally for vfs-suffix files the local entries need the suffix removed.
-        // However, don't do it if "foo.owncloud" exists on the server or in the db
-        // (as a non-virtual file): we don't want to create two entries.
-        auto name = e.name;
-        if (e.isVirtualFile && isVfsWithSuffix() && entries.find(name) == entries.end()) {
-            chopVirtualFileSuffix(name);
-            // If there is both a virtual file and a real file, we must keep the real file
-            if (entries[name].localEntry.isValid())
+        entries[e.name].localEntry = e;
+    }
+    if (isVfsWithSuffix()) {
+        // For vfs-suffix the local data for suffixed files should usually be associated
+        // with the non-suffixed name. Unless both names exist locally or there's
+        // other data about the suffixed file.
+        // This is done in a second path in order to not depend on the order of
+        // _localNormalQueryEntries.
+        for (auto &e : _localNormalQueryEntries) {
+            if (!e.isVirtualFile)
                 continue;
+            auto &suffixedEntry = entries[e.name];
+            bool hasOtherData = suffixedEntry.serverEntry.isValid() || suffixedEntry.dbEntry.isValid();
+
+            auto nonvirtualName = e.name;
+            chopVirtualFileSuffix(nonvirtualName);
+            auto &nonvirtualEntry = entries[nonvirtualName];
+            // If the non-suffixed entry has no data, move it
+            if (!nonvirtualEntry.localEntry.isValid()) {
+                std::swap(nonvirtualEntry.localEntry, suffixedEntry.localEntry);
+                if (!hasOtherData)
+                    entries.erase(e.name);
+            } else if (!hasOtherData) {
+                // Normally a lone local suffixed file would be processed under the
+                // unsuffixed name. In this special case it's under the suffixed name.
+                // To avoid lots of special casing, make sure PathTuple::addName()
+                // will be called with the unsuffixed name anyway.
+                suffixedEntry.nameOverride = nonvirtualName;
+            }
         }
-        entries[name].localEntry = std::move(e);
     }
     _localNormalQueryEntries.clear();
 
@@ -119,23 +140,23 @@ void ProcessDirectoryJob::process()
         const auto &e = f.second;
 
         PathTuple path;
-        path = _currentFolder.addName(f.first);
+        path = _currentFolder.addName(e.nameOverride.isEmpty() ? f.first : e.nameOverride);
 
         if (isVfsWithSuffix()) {
-            // If the file is virtual in the db, adjust path._original
-            if (e.dbEntry.isVirtualFile()) {
-                ASSERT(hasVirtualFileSuffix(e.dbEntry._path));
-                addVirtualFileSuffix(path._original);
-            } else if (e.localEntry.isVirtualFile && !e.dbEntry.isValid()) {
+            // Without suffix vfs the paths would be good. But since the dbEntry and localEntry
+            // can have different names from f.first when suffix vfs is on, make sure the
+            // corresponding _original and _local paths are right.
+
+            if (e.dbEntry.isValid()) {
+                path._original = e.dbEntry._path;
+            } else if (e.localEntry.isVirtualFile) {
                 // We don't have a db entry - but it should be at this path
-                addVirtualFileSuffix(path._original);
+                path._original = PathTuple::pathAppend(_currentFolder._original,  e.localEntry.name);
             }
-
-            // If the file is virtual locally, adjust path._local
-            if (e.localEntry.isVirtualFile) {
-                ASSERT(hasVirtualFileSuffix(e.localEntry.name));
-                addVirtualFileSuffix(path._local);
-            } else if (e.dbEntry.isVirtualFile() && _queryLocal == ParentNotChanged) {
+            if (e.localEntry.isValid()) {
+                path._local = PathTuple::pathAppend(_currentFolder._local, e.localEntry.name);
+            } else if (e.dbEntry.isVirtualFile()) {
+                // We don't have a local entry - but it should be at this path
                 addVirtualFileSuffix(path._local);
             }
         }

src/libsync/discovery.h

@@ -111,13 +111,17 @@ private:
         QString _target; // Path that will be the result after the sync (and will be in the DB)
         QString _server; // Path on the server (before the sync)
         QString _local; // Path locally (before the sync)
+        static QString pathAppend(const QString &base, const QString &name)
+        {
+            return base.isEmpty() ? name : base + QLatin1Char('/') + name;
+        }
         PathTuple addName(const QString &name) const
         {
             PathTuple result;
-            result._original = _original.isEmpty() ? name : _original + QLatin1Char('/') + name;
+            result._original = pathAppend(_original, name);
             auto buildString = [&](const QString &other) {
                 // Optimize by trying to keep all string implicitly shared if they are the same (common case)
-                return other == _original ? result._original : other.isEmpty() ? name : other + QLatin1Char('/') + name;
+                return other == _original ? result._original : pathAppend(other, name);
             };
             result._target = buildString(_target);
             result._server = buildString(_server);

src/libsync/vfs/suffix/vfs_suffix.cpp

@@ -41,6 +41,20 @@ QString VfsSuffix::fileSuffix() const
     return QStringLiteral(APPLICATION_DOTVIRTUALFILE_SUFFIX);
 }
 
+void VfsSuffix::startImpl(const VfsSetupParams &params)
+{
+    // It is unsafe for the database to contain any ".owncloud" file entries
+    // that are not marked as a virtual file. These could be real .owncloud
+    // files that were synced before vfs was enabled.
+    QByteArrayList toWipe;
+    params.journal->getFilesBelowPath("", [&toWipe](const SyncJournalFileRecord &rec) {
+        if (!rec.isVirtualFile() && rec._path.endsWith(APPLICATION_DOTVIRTUALFILE_SUFFIX))
+            toWipe.append(rec._path);
+    });
+    for (const auto &path : toWipe)
+        params.journal->deleteFileRecord(path);
+}
+
 void VfsSuffix::stop()
 {
 }

src/libsync/vfs/suffix/vfs_suffix.h

@@ -58,7 +58,7 @@ public slots:
     void fileStatusChanged(const QString &, SyncFileStatus) override {}
 
 protected:
-    void startImpl(const VfsSetupParams &) override {}
+    void startImpl(const VfsSetupParams &params) override;
 };
 
 class SuffixVfsPluginFactory : public QObject, public DefaultPluginFactory<VfsSuffix>

test/testsyncvirtualfiles.cpp

@@ -383,7 +383,7 @@ private slots:
         QVERIFY(itemInstruction(completeSpy, "A/a4m", CSYNC_INSTRUCTION_NEW));
         QVERIFY(itemInstruction(completeSpy, "A/a4" DVSUFFIX, CSYNC_INSTRUCTION_REMOVE));
         QVERIFY(itemInstruction(completeSpy, "A/a5", CSYNC_INSTRUCTION_CONFLICT));
-        QVERIFY(itemInstruction(completeSpy, "A/a5" DVSUFFIX, CSYNC_INSTRUCTION_NONE));
+        QVERIFY(itemInstruction(completeSpy, "A/a5" DVSUFFIX, CSYNC_INSTRUCTION_REMOVE));
         QVERIFY(itemInstruction(completeSpy, "A/a6", CSYNC_INSTRUCTION_CONFLICT));
         QVERIFY(itemInstruction(completeSpy, "A/a7", CSYNC_INSTRUCTION_SYNC));
         QVERIFY(itemInstruction(completeSpy, "A/b1", CSYNC_INSTRUCTION_SYNC));
@@ -977,8 +977,9 @@ private slots:
         QVERIFY(fakeFolder.currentLocalState().find("unspec/file1" DVSUFFIX));
     }
 
-    // Check what happens if vfs-suffixed files exist on the server or in the db
-    void testSuffixOnServerOrDb()
+    // Check what happens if vfs-suffixed files exist on the server or locally
+    // while the file is hydrated
+    void testSuffixFilesWhileLocalHydrated()
     {
         FakeFolder fakeFolder{ FileInfo() };
 
@@ -988,9 +989,22 @@ private slots:
         };
         cleanup();
 
-        // file1.nextcloud is happily synced with Vfs::Off
+        // suffixed files are happily synced with Vfs::Off
         fakeFolder.remoteModifier().mkdir("A");
-        fakeFolder.remoteModifier().insert("A/file1" DVSUFFIX);
+        fakeFolder.remoteModifier().insert("A/test1" DVSUFFIX, 10, 'A');
+        fakeFolder.remoteModifier().insert("A/test2" DVSUFFIX, 20, 'A');
+        fakeFolder.remoteModifier().insert("A/file1" DVSUFFIX, 30, 'A');
+        fakeFolder.remoteModifier().insert("A/file2", 40, 'A');
+        fakeFolder.remoteModifier().insert("A/file2" DVSUFFIX, 50, 'A');
+        fakeFolder.remoteModifier().insert("A/file3", 60, 'A');
+        fakeFolder.remoteModifier().insert("A/file3" DVSUFFIX, 70, 'A');
+        fakeFolder.remoteModifier().insert("A/file3" DVSUFFIX DVSUFFIX, 80, 'A');
+        fakeFolder.remoteModifier().insert("A/remote1" DVSUFFIX, 30, 'A');
+        fakeFolder.remoteModifier().insert("A/remote2", 40, 'A');
+        fakeFolder.remoteModifier().insert("A/remote2" DVSUFFIX, 50, 'A');
+        fakeFolder.remoteModifier().insert("A/remote3", 60, 'A');
+        fakeFolder.remoteModifier().insert("A/remote3" DVSUFFIX, 70, 'A');
+        fakeFolder.remoteModifier().insert("A/remote3" DVSUFFIX DVSUFFIX, 80, 'A');
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
         cleanup();
@@ -998,26 +1012,148 @@ private slots:
         // Enable suffix vfs
         setupVfs(fakeFolder);
 
+        // A simple sync removes the files that are now ignored (?)
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/file1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file2" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        cleanup();
+
+        // Add a real file where the suffixed file exists
+        fakeFolder.localModifier().insert("A/test1", 11, 'A');
+        fakeFolder.remoteModifier().insert("A/test2", 21, 'A');
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/test1", CSYNC_INSTRUCTION_NEW));
+        // this isn't fully good since some code requires size == 1 for placeholders
+        // (when renaming placeholder to real file). But the alternative would mean
+        // special casing this to allow CONFLICT at virtual file creation level. Ew.
+        QVERIFY(itemInstruction(completeSpy, "A/test2" DVSUFFIX, CSYNC_INSTRUCTION_UPDATE_METADATA));
+        cleanup();
+
         // Local changes of suffixed file do nothing
-        fakeFolder.localModifier().appendByte("A/file1" DVSUFFIX);
+        fakeFolder.localModifier().setContents("A/file1" DVSUFFIX, 'B');
+        fakeFolder.localModifier().setContents("A/file2" DVSUFFIX, 'B');
+        fakeFolder.localModifier().setContents("A/file3" DVSUFFIX, 'B');
+        fakeFolder.localModifier().setContents("A/file3" DVSUFFIX DVSUFFIX, 'B');
         QVERIFY(fakeFolder.syncOnce());
         QVERIFY(itemInstruction(completeSpy, "A/file1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file2" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
         cleanup();
 
-        // Remote don't do anything either
-        fakeFolder.remoteModifier().appendByte("A/file1" DVSUFFIX);
+        // Remote changes don't do anything either
+        fakeFolder.remoteModifier().setContents("A/file1" DVSUFFIX, 'C');
+        fakeFolder.remoteModifier().setContents("A/file2" DVSUFFIX, 'C');
+        fakeFolder.remoteModifier().setContents("A/file3" DVSUFFIX, 'C');
+        fakeFolder.remoteModifier().setContents("A/file3" DVSUFFIX DVSUFFIX, 'C');
         QVERIFY(fakeFolder.syncOnce());
         QVERIFY(itemInstruction(completeSpy, "A/file1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file2" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
         cleanup();
 
-        // New files with a suffix aren't propagated downwards in the first place
-        fakeFolder.remoteModifier().insert("A/file2" DVSUFFIX);
+        // Local removal: when not querying server
+        fakeFolder.localModifier().remove("A/file1" DVSUFFIX);
+        fakeFolder.localModifier().remove("A/file2" DVSUFFIX);
+        fakeFolder.localModifier().remove("A/file3" DVSUFFIX);
+        fakeFolder.localModifier().remove("A/file3" DVSUFFIX DVSUFFIX);
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(findItem(completeSpy, "A/file1" DVSUFFIX)->isEmpty());
+        QVERIFY(findItem(completeSpy, "A/file2" DVSUFFIX)->isEmpty());
+        QVERIFY(findItem(completeSpy, "A/file3" DVSUFFIX)->isEmpty());
+        QVERIFY(findItem(completeSpy, "A/file3" DVSUFFIX DVSUFFIX)->isEmpty());
+        cleanup();
+
+        // Local removal: when querying server
+        fakeFolder.remoteModifier().setContents("A/file1" DVSUFFIX, 'D');
         QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/file1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
         QVERIFY(itemInstruction(completeSpy, "A/file2" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
-        QVERIFY(fakeFolder.currentRemoteState().find("A/file2" DVSUFFIX));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        cleanup();
+
+        // Remote removal
+        fakeFolder.remoteModifier().remove("A/remote1" DVSUFFIX);
+        fakeFolder.remoteModifier().remove("A/remote2" DVSUFFIX);
+        fakeFolder.remoteModifier().remove("A/remote3" DVSUFFIX);
+        fakeFolder.remoteModifier().remove("A/remote3" DVSUFFIX DVSUFFIX);
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/remote1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/remote2" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/remote3" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/remote3" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        cleanup();
+
+        // New files with a suffix aren't propagated downwards in the first place
+        fakeFolder.remoteModifier().insert("A/new1" DVSUFFIX);
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/new1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(fakeFolder.currentRemoteState().find("A/new1" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find("A/new1"));
+        QVERIFY(!fakeFolder.currentLocalState().find("A/new1" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find("A/new1" DVSUFFIX DVSUFFIX));
+        cleanup();
+    }
+
+    // Check what happens if vfs-suffixed files exist on the server or in the db
+    void testExtraFilesLocalDehydrated()
+    {
+        FakeFolder fakeFolder{ FileInfo() };
+        setupVfs(fakeFolder);
+
+        QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
+        auto cleanup = [&]() {
+            completeSpy.clear();
+        };
+        cleanup();
+
+        // create a bunch of local virtual files, in some instances
+        // ignore remote files
+        fakeFolder.remoteModifier().mkdir("A");
+        fakeFolder.remoteModifier().insert("A/file1", 30, 'A');
+        fakeFolder.remoteModifier().insert("A/file2", 40, 'A');
+        fakeFolder.remoteModifier().insert("A/file3", 60, 'A');
+        fakeFolder.remoteModifier().insert("A/file3" DVSUFFIX, 70, 'A');
+        fakeFolder.remoteModifier().insert("A/file4", 80, 'A');
+        fakeFolder.remoteModifier().insert("A/file4" DVSUFFIX, 90, 'A');
+        fakeFolder.remoteModifier().insert("A/file4" DVSUFFIX DVSUFFIX, 100, 'A');
+        fakeFolder.remoteModifier().insert("A/file5", 110, 'A');
+        fakeFolder.remoteModifier().insert("A/file6", 120, 'A');
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(!fakeFolder.currentLocalState().find("A/file1"));
+        QVERIFY(fakeFolder.currentLocalState().find("A/file1" DVSUFFIX));
         QVERIFY(!fakeFolder.currentLocalState().find("A/file2"));
-        QVERIFY(!fakeFolder.currentLocalState().find("A/file2" DVSUFFIX));
-        QVERIFY(!fakeFolder.currentLocalState().find("A/file2" DVSUFFIX DVSUFFIX));
+        QVERIFY(fakeFolder.currentLocalState().find("A/file2" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find("A/file3"));
+        QVERIFY(fakeFolder.currentLocalState().find("A/file3" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find("A/file4"));
+        QVERIFY(fakeFolder.currentLocalState().find("A/file4" DVSUFFIX));
+        QVERIFY(!fakeFolder.currentLocalState().find("A/file4" DVSUFFIX DVSUFFIX));
+        QVERIFY(itemInstruction(completeSpy, "A/file1" DVSUFFIX, CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "A/file2" DVSUFFIX, CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "A/file3" DVSUFFIX, CSYNC_INSTRUCTION_NEW));
+        QVERIFY(itemInstruction(completeSpy, "A/file4" DVSUFFIX, CSYNC_INSTRUCTION_NEW));
+        // technically file3.owncloud and file4.owncloud are also ignored
+        QVERIFY(itemInstruction(completeSpy, "A/file4" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        cleanup();
+
+        // Create odd extra files locally and remotely
+        fakeFolder.localModifier().insert("A/file1", 10, 'A');
+        fakeFolder.localModifier().insert("A/file2" DVSUFFIX DVSUFFIX, 10, 'A');
+        fakeFolder.remoteModifier().insert("A/file5" DVSUFFIX, 10, 'A');
+        fakeFolder.localModifier().insert("A/file6", 10, 'A');
+        fakeFolder.remoteModifier().insert("A/file6" DVSUFFIX, 10, 'A');
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(itemInstruction(completeSpy, "A/file1", CSYNC_INSTRUCTION_CONFLICT));
+        QVERIFY(itemInstruction(completeSpy, "A/file1" DVSUFFIX, CSYNC_INSTRUCTION_REMOVE)); // it's now a pointless real virtual file
+        QVERIFY(itemInstruction(completeSpy, "A/file2" DVSUFFIX DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file5" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
+        QVERIFY(itemInstruction(completeSpy, "A/file6", CSYNC_INSTRUCTION_CONFLICT));
+        QVERIFY(itemInstruction(completeSpy, "A/file6" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE));
         cleanup();
     }