From: Christian Kamm Date: Tue, 17 Apr 2018 09:23:37 +0000 (+0200) Subject: LocalDiscoveryTracker: Separate from Folder and move to libsync X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~599 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=765c12dae1bc84a3eb1fc666b7b5e6e18d246b8d;p=nextcloud-desktop.git LocalDiscoveryTracker: Separate from Folder and move to libsync To allow relevant code to be closer together and for testing in unittests without having to get a gui Folder. See #6120 --- diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 9fdc4cfaa..1042d2f48 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -30,6 +30,7 @@ #include "socketapi.h" #include "theme.h" #include "filesystem.h" +#include "localdiscoverytracker.h" #include "creds/abstractcredentials.h" @@ -110,6 +111,12 @@ Folder::Folder(const FolderDefinition &definition, connect(ProgressDispatcher::instance(), &ProgressDispatcher::folderConflicts, this, &Folder::slotFolderConflicts); + + _localDiscoveryTracker.reset(new LocalDiscoveryTracker); + connect(_engine.data(), &SyncEngine::finished, + _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotSyncFinished); + connect(_engine.data(), &SyncEngine::itemCompleted, + _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotItemCompleted); } Folder::~Folder() @@ -478,8 +485,7 @@ void Folder::slotWatchedPathChanged(const QString &path) // We do this before checking for our own sync-related changes to make // extra sure to not miss relevant changes. auto relativePathBytes = relativePath.toUtf8(); - _localDiscoveryPaths.insert(relativePathBytes); - qCDebug(lcFolder) << "local discovery: inserted" << relativePath << "due to file watcher"; + _localDiscoveryTracker->addTouchedPath(relativePathBytes); // The folder watcher fires a lot of bogus notifications during // a sync operation, both for actual user files and the database @@ -681,22 +687,15 @@ void Folder::startSync(const QStringList &pathList) && hasDoneFullLocalDiscovery && !periodicFullLocalDiscoveryNow) { qCInfo(lcFolder) << "Allowing local discovery to read from the database"; - _engine->setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, _localDiscoveryPaths); - - if (lcFolder().isDebugEnabled()) { - QByteArrayList paths; - for (auto &path : _localDiscoveryPaths) - paths.append(path); - qCDebug(lcFolder) << "local discovery paths: " << paths; - } - - _previousLocalDiscoveryPaths = std::move(_localDiscoveryPaths); + _engine->setLocalDiscoveryOptions( + LocalDiscoveryStyle::DatabaseAndFilesystem, + _localDiscoveryTracker->localDiscoveryPaths()); + _localDiscoveryTracker->startSyncPartialDiscovery(); } else { qCInfo(lcFolder) << "Forbidding local discovery to read from the database"; _engine->setLocalDiscoveryOptions(LocalDiscoveryStyle::FilesystemOnly); - _previousLocalDiscoveryPaths.clear(); + _localDiscoveryTracker->startSyncFullDiscovery(); } - _localDiscoveryPaths.clear(); _engine->setIgnoreHiddenFiles(_definition.ignoreHiddenFiles); @@ -839,23 +838,14 @@ void Folder::slotSyncFinished(bool success) journalDb()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncWhiteList, QStringList()); } - // bug: This function uses many different criteria for "sync was successful" - investigate! if ((_syncResult.status() == SyncResult::Success || _syncResult.status() == SyncResult::Problem) && success) { if (_engine->lastLocalDiscoveryStyle() == LocalDiscoveryStyle::FilesystemOnly) { _timeSinceLastFullLocalDiscovery.start(); } - qCDebug(lcFolder) << "Sync success, forgetting last sync's local discovery path list"; - } else { - // On overall-failure we can't forget about last sync's local discovery - // paths yet, reuse them for the next sync again. - // C++17: Could use std::set::merge(). - _localDiscoveryPaths.insert( - _previousLocalDiscoveryPaths.begin(), _previousLocalDiscoveryPaths.end()); - qCDebug(lcFolder) << "Sync failed, keeping last sync's local discovery path list"; } - _previousLocalDiscoveryPaths.clear(); + emit syncStateChange(); diff --git a/src/gui/folder.h b/src/gui/folder.h index 3759f16da..c52f666af 100644 --- a/src/gui/folder.h +++ b/src/gui/folder.h @@ -39,6 +39,7 @@ class SyncEngine; class AccountState; class SyncRunFileLog; class FolderWatcher; +class LocalDiscoveryTracker; /** * @brief The FolderDefinition class @@ -397,20 +398,9 @@ private: QScopedPointer _folderWatcher; /** - * The paths that should be checked by the next local discovery. - * - * Mostly a collection of files the filewatchers have reported as touched. - * Also includes files that have had errors in the last sync run. - */ - std::set _localDiscoveryPaths; - - /** - * The paths that the current sync run used for local discovery. - * - * For failing syncs, this list will be merged into _localDiscoveryPaths - * again when the sync is done to make sure everything is retried. + * Keeps track of locally dirty files so we can skip local discovery sometimes. */ - std::set _previousLocalDiscoveryPaths; + QScopedPointer _localDiscoveryTracker; }; } diff --git a/src/libsync/CMakeLists.txt b/src/libsync/CMakeLists.txt index 0bcbe40ec..648214da1 100644 --- a/src/libsync/CMakeLists.txt +++ b/src/libsync/CMakeLists.txt @@ -48,6 +48,7 @@ set(libsync_SRCS syncfileitem.cpp syncfilestatus.cpp syncfilestatustracker.cpp + localdiscoverytracker.cpp syncresult.cpp theme.cpp clientsideencryption.cpp diff --git a/src/libsync/localdiscoverytracker.cpp b/src/libsync/localdiscoverytracker.cpp new file mode 100644 index 000000000..0e69c69ad --- /dev/null +++ b/src/libsync/localdiscoverytracker.cpp @@ -0,0 +1,95 @@ +/* + * Copyright (C) by Christian Kamm + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "localdiscoverytracker.h" + +#include "syncfileitem.h" + +#include + +using namespace OCC; + +Q_LOGGING_CATEGORY(lcLocalDiscoveryTracker, "sync.localdiscoverytracker", QtInfoMsg) + +LocalDiscoveryTracker::LocalDiscoveryTracker() +{ +} + +void LocalDiscoveryTracker::addTouchedPath(const QByteArray &relativePath) +{ + qCDebug(lcLocalDiscoveryTracker) << "inserted touched" << relativePath; + _localDiscoveryPaths.insert(relativePath); +} + +void LocalDiscoveryTracker::startSyncFullDiscovery() +{ + _localDiscoveryPaths.clear(); + _previousLocalDiscoveryPaths.clear(); + qCDebug(lcLocalDiscoveryTracker) << "full discovery"; +} + +void LocalDiscoveryTracker::startSyncPartialDiscovery() +{ + if (lcLocalDiscoveryTracker().isDebugEnabled()) { + QByteArrayList paths; + for (auto &path : _localDiscoveryPaths) + paths.append(path); + qCDebug(lcLocalDiscoveryTracker) << "partial discovery with paths: " << paths; + } + + _previousLocalDiscoveryPaths = std::move(_localDiscoveryPaths); + _localDiscoveryPaths.clear(); +} + +const std::set &LocalDiscoveryTracker::localDiscoveryPaths() const +{ + return _localDiscoveryPaths; +} + +void LocalDiscoveryTracker::slotItemCompleted(const SyncFileItemPtr &item) +{ + // For successes, we want to wipe the file from the list to ensure we don't + // rediscover it even if this overall sync fails. + // + // For failures, we want to add the file to the list so the next sync + // will be able to retry it. + if (item->_status == SyncFileItem::Success + || item->_status == SyncFileItem::FileIgnored + || item->_status == SyncFileItem::Restoration + || item->_status == SyncFileItem::Conflict + || (item->_status == SyncFileItem::NoStatus + && (item->_instruction == CSYNC_INSTRUCTION_NONE + || item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA))) { + if (_previousLocalDiscoveryPaths.erase(item->_file.toUtf8())) + qCDebug(lcLocalDiscoveryTracker) << "wiped successful item" << item->_file; + } else { + _localDiscoveryPaths.insert(item->_file.toUtf8()); + qCDebug(lcLocalDiscoveryTracker) << "inserted error item" << item->_file; + } +} + +void LocalDiscoveryTracker::slotSyncFinished(bool success) +{ + if (success) { + qCDebug(lcLocalDiscoveryTracker) << "sync success, forgetting last sync's local discovery path list"; + } else { + // On overall-failure we can't forget about last sync's local discovery + // paths yet, reuse them for the next sync again. + // C++17: Could use std::set::merge(). + _localDiscoveryPaths.insert( + _previousLocalDiscoveryPaths.begin(), _previousLocalDiscoveryPaths.end()); + qCDebug(lcLocalDiscoveryTracker) << "sync failed, keeping last sync's local discovery path list"; + } + _previousLocalDiscoveryPaths.clear(); +} diff --git a/src/libsync/localdiscoverytracker.h b/src/libsync/localdiscoverytracker.h new file mode 100644 index 000000000..913365913 --- /dev/null +++ b/src/libsync/localdiscoverytracker.h @@ -0,0 +1,103 @@ +/* + * Copyright (C) by Christian Kamm + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#ifndef LOCALDISCOVERYTRACKER_H +#define LOCALDISCOVERYTRACKER_H + +#include "owncloudlib.h" +#include +#include +#include +#include + +namespace OCC { + +class SyncFileItem; +typedef QSharedPointer SyncFileItemPtr; + +/** + * @brief Tracks files that must be rediscovered locally + * + * It does this by being notified about + * - modified files (addTouchedPath(), from file watcher) + * - starting syncs (startSync*()) + * - finished items (slotItemCompleted(), by SyncEngine signal) + * - finished syncs (slotSyncFinished(), by SyncEngine signal) + * + * Then localDiscoveryPaths() can be used to determine paths to rediscover + * and send to SyncEngine::setLocalDiscoveryOptions(). + * + * This class is primarily used from Folder and separate primarily for + * readability and testing purposes. + * + * All paths used in this class are expected to be utf8 encoded byte arrays, + * relative to the folder that is being synced, without a starting slash. + * + * @ingroup libsync + */ +class OWNCLOUDSYNC_EXPORT LocalDiscoveryTracker : public QObject +{ + Q_OBJECT +public: + LocalDiscoveryTracker(); + + /** Adds a path that must be locally rediscovered later. + * + * This should be a full relative file path, example: + * foo/bar/file.txt + */ + void addTouchedPath(const QByteArray &relativePath); + + /** Call when a sync run starts that rediscovers all local files */ + void startSyncFullDiscovery(); + + /** Call when a sync using localDiscoveryPaths() starts */ + void startSyncPartialDiscovery(); + + /** Access list of files that shall be locally rediscovered. */ + const std::set &localDiscoveryPaths() const; + +public slots: + /** + * Success and failure of sync items adjust what the next sync is + * supposed to do. + */ + void slotItemCompleted(const SyncFileItemPtr &item); + + /** + * When a sync finishes, the lists must be updated + */ + void slotSyncFinished(bool success); + +private: + /** + * The paths that should be checked by the next local discovery. + * + * Mostly a collection of files the filewatchers have reported as touched. + * Also includes files that have had errors in the last sync run. + */ + std::set _localDiscoveryPaths; + + /** + * The paths that the current sync run used for local discovery. + * + * For failing syncs, this list will be merged into _localDiscoveryPaths + * again when the sync is done to make sure everything is retried. + */ + std::set _previousLocalDiscoveryPaths; +}; + +} // namespace OCC + +#endif diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 429a47541..a3ff8e214 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -55,6 +55,7 @@ nextcloud_add_test(ChunkingNg "syncenginetestutils.h") nextcloud_add_test(UploadReset "syncenginetestutils.h") nextcloud_add_test(AllFilesDeleted "syncenginetestutils.h") nextcloud_add_test(Blacklist "syncenginetestutils.h") +nextcloud_add_test(LocalDiscovery "syncenginetestutils.h") nextcloud_add_test(FolderWatcher "${FolderWatcher_SRC}") if( UNIX AND NOT APPLE ) diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp new file mode 100644 index 000000000..34e1e2f47 --- /dev/null +++ b/test/testlocaldiscovery.cpp @@ -0,0 +1,103 @@ +/* + * This software is in the public domain, furnished "as is", without technical + * support, and with no warranty, express or implied, as to its usefulness for + * any purpose. + * + */ + +#include +#include "syncenginetestutils.h" +#include +#include + +using namespace OCC; + +class TestLocalDiscovery : public QObject +{ + Q_OBJECT + +private slots: + // Check correct behavior when local discovery is partially drawn from the db + void testLocalDiscoveryStyle() + { + FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; + + LocalDiscoveryTracker tracker; + connect(&fakeFolder.syncEngine(), &SyncEngine::itemCompleted, &tracker, &LocalDiscoveryTracker::slotItemCompleted); + connect(&fakeFolder.syncEngine(), &SyncEngine::finished, &tracker, &LocalDiscoveryTracker::slotSyncFinished); + + // More subdirectories are useful for testing + fakeFolder.localModifier().mkdir("A/X"); + fakeFolder.localModifier().mkdir("A/Y"); + fakeFolder.localModifier().insert("A/X/x1"); + fakeFolder.localModifier().insert("A/Y/y1"); + tracker.addTouchedPath("A/X"); + + tracker.startSyncFullDiscovery(); + QVERIFY(fakeFolder.syncOnce()); + + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(tracker.localDiscoveryPaths().empty()); + + // Test begins + fakeFolder.localModifier().insert("A/a3"); + fakeFolder.localModifier().insert("A/X/x2"); + fakeFolder.localModifier().insert("A/Y/y2"); + fakeFolder.localModifier().insert("B/b3"); + fakeFolder.remoteModifier().insert("C/c3"); + + tracker.addTouchedPath("A/X"); + fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, tracker.localDiscoveryPaths()); + + tracker.startSyncPartialDiscovery(); + QVERIFY(fakeFolder.syncOnce()); + + QVERIFY(fakeFolder.currentRemoteState().find("A/a3")); + QVERIFY(fakeFolder.currentRemoteState().find("A/X/x2")); + QVERIFY(!fakeFolder.currentRemoteState().find("A/Y/y2")); + QVERIFY(!fakeFolder.currentRemoteState().find("B/b3")); + QVERIFY(fakeFolder.currentLocalState().find("C/c3")); + QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::DatabaseAndFilesystem); + QVERIFY(tracker.localDiscoveryPaths().empty()); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::FilesystemOnly); + QVERIFY(tracker.localDiscoveryPaths().empty()); + } + + void testLocalDiscoveryDecision() + { + FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; + auto &engine = fakeFolder.syncEngine(); + + QVERIFY(engine.shouldDiscoverLocally("")); + QVERIFY(engine.shouldDiscoverLocally("A")); + QVERIFY(engine.shouldDiscoverLocally("A/X")); + + fakeFolder.syncEngine().setLocalDiscoveryOptions( + LocalDiscoveryStyle::DatabaseAndFilesystem, + { "A/X", "foo bar space/touch", "foo/", "zzz" }); + + QVERIFY(engine.shouldDiscoverLocally("")); + QVERIFY(engine.shouldDiscoverLocally("A")); + QVERIFY(engine.shouldDiscoverLocally("A/X")); + QVERIFY(!engine.shouldDiscoverLocally("B")); + QVERIFY(!engine.shouldDiscoverLocally("A B")); + QVERIFY(!engine.shouldDiscoverLocally("B/X")); + QVERIFY(!engine.shouldDiscoverLocally("A/X/Y")); + QVERIFY(engine.shouldDiscoverLocally("foo bar space")); + QVERIFY(engine.shouldDiscoverLocally("foo")); + QVERIFY(!engine.shouldDiscoverLocally("foo bar")); + QVERIFY(!engine.shouldDiscoverLocally("foo bar/touch")); + + fakeFolder.syncEngine().setLocalDiscoveryOptions( + LocalDiscoveryStyle::DatabaseAndFilesystem, + {}); + + QVERIFY(!engine.shouldDiscoverLocally("")); + } +}; + +QTEST_GUILESS_MAIN(TestLocalDiscovery) +#include "testlocaldiscovery.moc" diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 6ab28af42..7071eef87 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -572,72 +572,6 @@ private slots: QVERIFY(!fakeFolder.currentRemoteState().find("C/myfile.txt")); } - // Check correct behavior when local discovery is partially drawn from the db - void testLocalDiscoveryStyle() - { - FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; - - // More subdirectories are useful for testing - fakeFolder.localModifier().mkdir("A/X"); - fakeFolder.localModifier().mkdir("A/Y"); - fakeFolder.localModifier().insert("A/X/x1"); - fakeFolder.localModifier().insert("A/Y/y1"); - QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); - - // Test begins - fakeFolder.localModifier().insert("A/a3"); - fakeFolder.localModifier().insert("A/X/x2"); - fakeFolder.localModifier().insert("A/Y/y2"); - fakeFolder.localModifier().insert("B/b3"); - fakeFolder.remoteModifier().insert("C/c3"); - - fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, { "A/X" }); - QVERIFY(fakeFolder.syncOnce()); - QVERIFY(fakeFolder.currentRemoteState().find("A/a3")); - QVERIFY(fakeFolder.currentRemoteState().find("A/X/x2")); - QVERIFY(!fakeFolder.currentRemoteState().find("A/Y/y2")); - QVERIFY(!fakeFolder.currentRemoteState().find("B/b3")); - QVERIFY(fakeFolder.currentLocalState().find("C/c3")); - QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::DatabaseAndFilesystem); - - QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); - QCOMPARE(fakeFolder.syncEngine().lastLocalDiscoveryStyle(), LocalDiscoveryStyle::FilesystemOnly); - } - - void testLocalDiscoveryDecision() - { - FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; - auto &engine = fakeFolder.syncEngine(); - - QVERIFY(engine.shouldDiscoverLocally("")); - QVERIFY(engine.shouldDiscoverLocally("A")); - QVERIFY(engine.shouldDiscoverLocally("A/X")); - - fakeFolder.syncEngine().setLocalDiscoveryOptions( - LocalDiscoveryStyle::DatabaseAndFilesystem, - { "A/X", "foo bar space/touch", "foo/", "zzz" }); - - QVERIFY(engine.shouldDiscoverLocally("")); - QVERIFY(engine.shouldDiscoverLocally("A")); - QVERIFY(engine.shouldDiscoverLocally("A/X")); - QVERIFY(!engine.shouldDiscoverLocally("B")); - QVERIFY(!engine.shouldDiscoverLocally("A B")); - QVERIFY(!engine.shouldDiscoverLocally("B/X")); - QVERIFY(!engine.shouldDiscoverLocally("A/X/Y")); - QVERIFY(engine.shouldDiscoverLocally("foo bar space")); - QVERIFY(engine.shouldDiscoverLocally("foo")); - QVERIFY(!engine.shouldDiscoverLocally("foo bar")); - QVERIFY(!engine.shouldDiscoverLocally("foo bar/touch")); - - fakeFolder.syncEngine().setLocalDiscoveryOptions( - LocalDiscoveryStyle::DatabaseAndFilesystem, - {}); - - QVERIFY(!engine.shouldDiscoverLocally("")); - } - void testDiscoveryHiddenFile() { FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };