From: Olivier Goffart Date: Tue, 9 Oct 2018 11:45:27 +0000 (+0200) Subject: New Discovery Algorithm: Ge tthe size of new folders X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~498 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=d25d87e92cc1b3dd769f831dbdb7a09cf9517111;p=nextcloud-desktop.git New Discovery Algorithm: Ge tthe size of new folders Also add a test that this works properly --- diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index a63cf35c5..a5433b9b4 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -433,17 +433,25 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( item->_modtime = serverEntry.modtime; item->_size = serverEntry.size; - auto postProcessNew = [item, this, path, serverEntry] { + auto postProcessServerNew = [item, this, path, serverEntry, localEntry, dbEntry] { if (item->isDirectory()) { - if (_discoveryData->checkSelectiveSyncNewFolder(path._server, serverEntry.remotePerm)) { - return; - } + _pendingAsyncJobs++; + _discoveryData->checkSelectiveSyncNewFolder(path._server, serverEntry.remotePerm, + [=](bool result) { + --_pendingAsyncJobs; + if (!result) { + processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer); + } + progress(); + }); + return; } // Turn new remote files into virtual files if the option is enabled. if (_discoveryData->_syncOptions._newFilesAreVirtual && item->_type == ItemTypeFile) { item->_type = ItemTypeVirtualFile; item->_file.append(_discoveryData->_syncOptions._virtualFileSuffix); } + processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer); }; if (!localEntry.isValid()) { @@ -556,26 +564,28 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( _pendingAsyncJobs++; auto job = new RequestEtagJob(_discoveryData->_account, originalPath, this); connect(job, &RequestEtagJob::finishedWithResult, this, [=](const Result &etag) mutable { + _pendingAsyncJobs--; if (etag.errorCode() != 404 || // Somehow another item claimed this original path, consider as if it existed _discoveryData->_renamedItems.contains(originalPath)) { // If the file exist or if there is another error, consider it is a new file. - postProcessNew(); - } else { - // The file do not exist, it is a rename + postProcessServerNew(); + progress(); + return; + } - // In case the deleted item was discovered in parallel - auto it = _discoveryData->_deletedItem.find(originalPath); - if (it != _discoveryData->_deletedItem.end()) { - ASSERT((*it)->_instruction == CSYNC_INSTRUCTION_REMOVE); - (*it)->_instruction = CSYNC_INSTRUCTION_NONE; - } - delete _discoveryData->_queuedDeletedDirectories.take(originalPath); + // The file do not exist, it is a rename - postProcessRename(path); + // In case the deleted item was discovered in parallel + auto it = _discoveryData->_deletedItem.find(originalPath); + if (it != _discoveryData->_deletedItem.end()) { + ASSERT((*it)->_instruction == CSYNC_INSTRUCTION_REMOVE); + (*it)->_instruction = CSYNC_INSTRUCTION_NONE; } + delete _discoveryData->_queuedDeletedDirectories.take(originalPath); + + postProcessRename(path); processFileFinalize(item, path, item->isDirectory(), item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist, _queryServer); - _pendingAsyncJobs--; progress(); }); job->start(); @@ -593,7 +603,8 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( } if (item->_instruction == CSYNC_INSTRUCTION_NEW) { - postProcessNew(); + postProcessServerNew(); + return; } } else if (serverEntry.isDirectory != (dbEntry._type == ItemTypeDirectory)) { // If the type of the entity changed, it's like NEW, but diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 0b8e8874d..f645b63a8 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -74,7 +74,8 @@ bool DiscoveryPhase::isInSelectiveSyncBlackList(const QString &path) const return false; } -bool DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path, RemotePermissions remotePerm) +void DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path, RemotePermissions remotePerm, + std::function callback) { if (_syncOptions._confirmExternalStorage && !_syncOptions._newFilesAreVirtual && remotePerm.hasPermission(RemotePermissions::IsMounted)) { @@ -86,51 +87,49 @@ bool DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path, RemotePerm // Only allow it if the white list contains exactly this path (not parents) // We want to ask confirmation for external storage even if the parents where selected if (_selectiveSyncWhiteList.contains(path + QLatin1Char('/'))) { - return false; + return callback(false); } emit newBigFolder(path, true); - return true; + return callback(true); } // If this path or the parent is in the white list, then we do not block this file if (findPathInList(_selectiveSyncWhiteList, path)) { - return false; + return callback(false); } auto limit = _syncOptions._newBigFolderSizeLimit; if (limit < 0 || _syncOptions._newFilesAreVirtual) { // no limit, everything is allowed; - return false; + return callback(false); } - // Go in the main thread to do a PROPFIND to know the size of this folder - qint64 result = -1; - - /* FIXME TOTO - { - QMutexLocker locker(&_vioMutex); - emit doGetSizeSignal(path, &result); - _vioWaitCondition.wait(&_vioMutex); - }*/ - - if (result >= limit) { - // we tell the UI there is a new folder - emit newBigFolder(path, false); - return true; - } else { - // it is not too big, put it in the white list (so we will not do more query for the children) - // and and do not block. - auto p = path; - if (!p.endsWith(QLatin1Char('/'))) { - p += QLatin1Char('/'); + // do a PROPFIND to know the size of this folder + auto propfindJob = new PropfindJob(_account, _remoteFolder + path, this); + propfindJob->setProperties(QList() << "resourcetype" + << "http://owncloud.org/ns:size"); + QObject::connect(propfindJob, &PropfindJob::finishedWithError, + this, [=] { return callback(false); }); + QObject::connect(propfindJob, &PropfindJob::result, this, [=](const QVariantMap &values) { + auto result = values.value(QLatin1String("size")).toLongLong(); + if (result >= limit) { + // we tell the UI there is a new folder + emit newBigFolder(path, false); + return callback(true); + } else { + // it is not too big, put it in the white list (so we will not do more query for the children) + // and and do not block. + auto p = path; + if (!p.endsWith(QLatin1Char('/'))) + p += QLatin1Char('/'); + _selectiveSyncWhiteList.insert( + std::upper_bound(_selectiveSyncWhiteList.begin(), _selectiveSyncWhiteList.end(), p), + p); + return callback(false); } - _selectiveSyncWhiteList.insert(std::upper_bound(_selectiveSyncWhiteList.begin(), - _selectiveSyncWhiteList.end(), p), - p); - - return false; - } + }); + propfindJob->start(); } /* Given a path on the remote, give the path as it is when the rename is done */ @@ -353,67 +352,4 @@ void DiscoverySingleDirectoryJob::lsJobFinishedWithErrorSlot(QNetworkReply *r) emit finished({httpCode, msg}); deleteLater(); } - -/* -void DiscoveryMainThread::singleDirectoryJobFirstDirectoryPermissionsSlot(RemotePermissions p) -{ - // Should be thread safe since the sync thread is blocked - if (_discoveryJob->_csync_ctx->remote.root_perms.isNull()) { - qCDebug(lcDiscovery) << "Permissions for root dir:" << p.toString(); - _discoveryJob->_csync_ctx->remote.root_perms = p; - } -} - -void DiscoveryMainThread::doGetSizeSlot(const QString &path, qint64 *result) -{ - QString fullPath = _pathPrefix; - if (!_pathPrefix.endsWith('/')) { - fullPath += '/'; - } - fullPath += path; - // remove trailing slash - while (fullPath.endsWith('/')) { - fullPath.chop(1); - } - - _currentGetSizeResult = result; - - // Schedule the DiscoverySingleDirectoryJob - auto propfindJob = new PropfindJob(_account, fullPath, this); - propfindJob->setProperties(QList() << "resourcetype" - << "http://owncloud.org/ns:size"); - QObject::connect(propfindJob, &PropfindJob::finishedWithError, - this, &DiscoveryMainThread::slotGetSizeFinishedWithError); - QObject::connect(propfindJob, &PropfindJob::result, - this, &DiscoveryMainThread::slotGetSizeResult); - propfindJob->start(); -} - -void DiscoveryMainThread::slotGetSizeFinishedWithError() -{ - if (!_currentGetSizeResult) { - return; // possibly aborted - } - - qCWarning(lcDiscovery) << "Error getting the size of the directory"; - // just let let the discovery job continue then - _currentGetSizeResult = nullptr; - QMutexLocker locker(&_discoveryJob->_vioMutex); - _discoveryJob->_vioWaitCondition.wakeAll(); -} - -void DiscoveryMainThread::slotGetSizeResult(const QVariantMap &map) -{ - if (!_currentGetSizeResult) { - return; // possibly aborted - } - - *_currentGetSizeResult = map.value(QLatin1String("size")).toLongLong(); - qCDebug(lcDiscovery) << "Size of folder:" << *_currentGetSizeResult; - _currentGetSizeResult = nullptr; - QMutexLocker locker(&_discoveryJob->_vioMutex); - _discoveryJob->_vioWaitCondition.wakeAll(); -} - -*/ } diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index 99ee57f23..8a6c00102 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -138,7 +138,11 @@ public: std::function _shouldDiscoverLocaly; bool isInSelectiveSyncBlackList(const QString &path) const; - bool checkSelectiveSyncNewFolder(const QString &path, RemotePermissions rp); + + // Check if the new folder should be deselected or not. + // May be async. "Return" via the callback, true if the item is blacklisted + void checkSelectiveSyncNewFolder(const QString &path, RemotePermissions rp, + std::function callback); QMap _deletedItem; QMap _queuedDeletedDirectories; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6741e0499..1568c9354 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -57,6 +57,7 @@ nextcloud_add_test(Blacklist "syncenginetestutils.h") nextcloud_add_test(LocalDiscovery "syncenginetestutils.h") nextcloud_add_test(RemoteDiscovery "syncenginetestutils.h") nextcloud_add_test(Permissions "syncenginetestutils.h") +nextcloud_add_test(SelectiveSync "syncenginetestutils.h") nextcloud_add_test(FolderWatcher "${FolderWatcher_SRC}") if( UNIX AND NOT APPLE ) diff --git a/test/testselectivesync.cpp b/test/testselectivesync.cpp new file mode 100644 index 000000000..c6278866b --- /dev/null +++ b/test/testselectivesync.cpp @@ -0,0 +1,92 @@ +/* + * 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 + +using namespace OCC; + + +class TestSelectiveSync : public QObject +{ + Q_OBJECT + +private slots: + + + void testSelectiveSyncBigFolders() + { + FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() }; + SyncOptions options; + options._newBigFolderSizeLimit = 20000; // 20 K + fakeFolder.syncEngine().setSyncOptions(options); + + QStringList sizeRequests; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation, const QNetworkRequest &req, QIODevice *device) + -> QNetworkReply * { + // Record what path we are querying for the size + if (req.attribute(QNetworkRequest::CustomVerbAttribute) == "PROPFIND") { + if (device->readAll().contains("extraDavProperties = "20020"; + fakeFolder.remoteModifier().find("A/newBigDir/subDir")->extraDavProperties = "20020"; + fakeFolder.remoteModifier().find("B/newSmallDir")->extraDavProperties = "10"; + fakeFolder.remoteModifier().find("B/newSmallDir/subDir")->extraDavProperties = "10"; + + QVERIFY(fakeFolder.syncOnce()); + + QCOMPARE(newBigFolder.count(), 1); + QCOMPARE(newBigFolder.first()[0].toString(), QString("A/newBigDir")); + QCOMPARE(newBigFolder.first()[1].toBool(), false); + newBigFolder.clear(); + + QCOMPARE(sizeRequests.count(), 2); // "A/newBigDir" and "B/newSmallDir"; + QCOMPARE(sizeRequests.filter("/subDir").count(), 0); // at no point we should request the size of the subdirs + sizeRequests.clear(); + + auto oldSync = fakeFolder.currentLocalState(); + // syncing again should do the same + fakeFolder.syncEngine().journal()->avoidReadFromDbOnNextSync(QString("A/newBigDir")); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), oldSync); + QCOMPARE(newBigFolder.count(), 1); // (since we don't have a real Folder, the files were not added to any list) + newBigFolder.clear(); + QCOMPARE(sizeRequests.count(), 1); // "A/newBigDir"; + sizeRequests.clear(); + + // Simulate that we accept all files by seting a wildcard white list + fakeFolder.syncEngine().journal()->setSelectiveSyncList(SyncJournalDb::SelectiveSyncWhiteList, + QStringList() << QLatin1String("/")); + fakeFolder.syncEngine().journal()->avoidReadFromDbOnNextSync(QString("A/newBigDir")); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(newBigFolder.count(), 0); + QCOMPARE(sizeRequests.count(), 0); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + } +}; + +QTEST_GUILESS_MAIN(TestSelectiveSync) +#include "testselectivesync.moc"