From: Christian Kamm Date: Fri, 2 Aug 2019 12:25:40 +0000 (+0200) Subject: Vfs: Lots of tests and corrections for suffix edge cases X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~164 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=4c4cbf0d9792dc56e6e27fc4576c7a5075c0bb36;p=nextcloud-desktop.git 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. --- diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index c43dd5c95..5bd2ed006 100644 --- a/src/libsync/discovery.cpp +++ b/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); } } diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index 6b165421e..a45050827 100644 --- a/src/libsync/discovery.h +++ b/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); diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp index e9d3ee9a1..6827126a9 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.cpp +++ b/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 ¶ms) +{ + // 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() { } diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h index 8fd4cb8bc..3426c39fe 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.h +++ b/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 ¶ms) override; }; class SuffixVfsPluginFactory : public QObject, public DefaultPluginFactory diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp index 56ecfcf8d..c203ba88d 100644 --- a/test/testsyncvirtualfiles.cpp +++ b/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(); }