From bdd1e72ddae03c7ed96dbc0f5b693422c195f599 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 13 Jul 2018 17:47:06 +0200 Subject: [PATCH] New discovery algoritmh: more on Renames --- src/libsync/abstractnetworkjob.h | 58 +++++++++++++ src/libsync/discovery.cpp | 138 +++++++++++++++++++------------ src/libsync/discovery.h | 45 ---------- src/libsync/networkjobs.cpp | 6 +- src/libsync/networkjobs.h | 1 + test/testsyncmove.cpp | 5 ++ 6 files changed, 156 insertions(+), 97 deletions(-) diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index a05711916..8a98af665 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -24,6 +24,7 @@ #include #include #include "accountfwd.h" +#include "common/asserts.h" class QUrl; @@ -31,6 +32,63 @@ namespace OCC { class AbstractSslErrorHandler; + +/** + * A Result of type T, or an Error that contains a code and a string + **/ +template +class Result +{ + struct Error + { + QString string; + int code; + }; + union { + T _result; + Error _error; + }; + bool _isError; + +public: + Result(T value) + : _result(std::move(value)) + , _isError(false){}; + Result(int code, QString str) + : _error({ std::move(str), code }) + , _isError(true) + { + } + ~Result() + { + if (_isError) + _error.~Error(); + else + _result.~T(); + } + explicit operator bool() const { return !_isError; } + const T &operator*() const & + { + ASSERT(!_isError); + return _result; + } + T operator*() && + { + ASSERT(!_isError); + return std::move(_result); + } + QString errorMessage() const + { + ASSERT(_isError); + return _error.string; + } + int errorCode() const + { + return _isError ? _error.code : 0; + } +}; + + /** * @brief The AbstractNetworkJob class * @ingroup libsync diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 86741882a..b87e2e47c 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -55,8 +55,8 @@ DiscoverServerJob::DiscoverServerJob(const AccountPtr &account, const QString &p }); connect(this, &DiscoverySingleDirectoryJob::finishedWithError, this, - [this](int, const QString &msg) { - emit this->finished({ Error, msg }); + [this](int code, const QString &msg) { + emit this->finished({ code, msg }); }); } @@ -352,7 +352,7 @@ void ProcessDirectoryJob::processFile(PathTuple path, item->_etag = serverEntry.etag; item->_previousSize = localEntry.size; item->_previousModtime = localEntry.modtime; - if (!dbEntry.isValid()) { // New file? + if (!dbEntry.isValid()) { // New file on the server item->_instruction = CSYNC_INSTRUCTION_NEW; item->_direction = SyncFileItem::Down; item->_modtime = serverEntry.modtime; @@ -422,13 +422,13 @@ void ProcessDirectoryJob::processFile(PathTuple path, /* A remote rename can also mean Encryption Mangled Name. * if we find one of those in the database, we ignore it. */ - else if (!base._e2eMangledName.isEmpty()) { + if (!base._e2eMangledName.isEmpty()) { qCWarning(lcDisco, "Encrypted file can not rename"); done = true; return; } - else if (item->_type == ItemTypeFile) { + if (item->_type == ItemTypeFile) { csync_file_stat_t buf; if (csync_vio_local_stat((_discoveryData->_localDir + originalPath).toUtf8(), &buf)) { qCInfo(lcDisco) << "Local file does not exist anymore." << originalPath; @@ -438,6 +438,11 @@ void ProcessDirectoryJob::processFile(PathTuple path, qCInfo(lcDisco) << "File has changed locally, not a rename." << originalPath; return; } + } else { + if (!QFile::exists(_discoveryData->_localDir + originalPath)) { + qCInfo(lcDisco) << "Local directory does not exist anymore." << originalPath; + return; + } } bool wasDeletedOnServer = false; @@ -473,51 +478,33 @@ void ProcessDirectoryJob::processFile(PathTuple path, } else { // we need to make a request to the server to know that the original file is deleted on the server _pendingAsyncJobs++; - auto job = new PropfindJob(_discoveryData->_account, originalPath, this); - auto considerNew = [=] { - // The original file still exist, consider it is new. - postProcessNew(); - qCInfo(lcDisco) << "Discovered" << item->_file << item->_instruction << item->_direction << item->isDirectory(); - if (item->isDirectory()) { - auto job = new ProcessDirectoryJob(item, recurseQueryServer, ParentDontExist, _discoveryData, this); - job->_currentFolder = path; - connect(job, &ProcessDirectoryJob::itemDiscovered, this, &ProcessDirectoryJob::itemDiscovered); - connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished); - _queuedJobs.push_back(job); + auto job = new RequestEtagJob(_discoveryData->_account, originalPath, this); + connect(job, &RequestEtagJob::finishedWithResult, this, [=](const Result &etag) mutable { + 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 { - emit itemDiscovered(item); - } + // The file do not exist, it is a rename - _pendingAsyncJobs--; - progress(); - }; - connect(job, &PropfindJob::result, this, considerNew); - connect(job, &PropfindJob::finishedWithError, this, [=](QNetworkReply *reply) mutable { - if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() != 404) { - qDebug() << reply->request().url() << reply->error() << reply->errorString(); - qFatal("TODO: Handle error"); - } - if (_discoveryData->_renamedItems.contains(originalPath)) { - // Somehow another item claimed this original path, consider as if it existed - considerNew(); - 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); - // 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; + postProcessRename(path); } - delete _discoveryData->_queuedDeletedDirectories.take(originalPath); - - // Normal use case: this is a rename. - postProcessRename(path); qCInfo(lcDisco) << "Discovered" << item->_file << item->_instruction << item->_direction << item->isDirectory(); if (item->isDirectory()) { - auto job = new ProcessDirectoryJob(item, recurseQueryServer, NormalQuery, _discoveryData, this); + auto job = new ProcessDirectoryJob(item, recurseQueryServer, + item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist, + _discoveryData, this); job->_currentFolder = path; connect(job, &ProcessDirectoryJob::itemDiscovered, this, &ProcessDirectoryJob::itemDiscovered); connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished); @@ -551,6 +538,9 @@ void ProcessDirectoryJob::processFile(PathTuple path, item->_size = serverEntry.size; if (serverEntry.isDirectory && dbEntry._type == ItemTypeDirectory) { item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA; + } else if (!localEntry.isValid()) { + // Deleted locally, changed on server + item->_instruction = CSYNC_INSTRUCTION_NEW; } else { item->_instruction = CSYNC_INSTRUCTION_SYNC; } @@ -659,25 +649,28 @@ void ProcessDirectoryJob::processFile(PathTuple path, isRename = item->_checksumHeader == base._checksumHeader; } } + auto originalPath = QString::fromUtf8(base._path); + if (isRename && _discoveryData->_renamedItems.contains(originalPath)) + isRename = false; if (isRename) { - auto originalPath = QString::fromUtf8(base._path); - auto it = _discoveryData->_deletedItem.find(originalPath); QByteArray oldEtag; + auto it = _discoveryData->_deletedItem.find(originalPath); if (it != _discoveryData->_deletedItem.end()) { if ((*it)->_instruction != CSYNC_INSTRUCTION_REMOVE) isRename = false; else (*it)->_instruction = CSYNC_INSTRUCTION_NONE; oldEtag = (*it)->_etag; - if (!item->isDirectory() && oldEtag != base._etag) + if (!item->isDirectory() && oldEtag != base._etag) { isRename = false; - } else { - // FIXME! We should do a server query to find out if the original path still exist and has the same etag + } } - if (_discoveryData->_renamedItems.contains(originalPath)) - isRename = false; - if (isRename) { + if (auto deleteJob = static_cast(_discoveryData->_queuedDeletedDirectories.value(originalPath).data())) { + oldEtag = deleteJob->_dirItem->_etag; delete _discoveryData->_queuedDeletedDirectories.take(originalPath); + } + + auto processRename = [item, originalPath, base, this](PathTuple &path) { _discoveryData->_renamedItems.insert(originalPath); item->_modtime = base._modtime; @@ -692,12 +685,55 @@ void ProcessDirectoryJob::processFile(PathTuple path, item->_etag = base._etag; path._original = originalPath; path._server = originalPath; - recurseQueryServer = oldEtag == base._etag ? ParentNotChanged : NormalQuery; qCInfo(lcDisco) << "Rename detected (up) " << item->_file << " -> " << item->_renameTarget; + }; + if (isRename && !oldEtag.isEmpty()) { + recurseQueryServer = oldEtag == base._etag ? ParentNotChanged : NormalQuery; + processRename(path); + } else if (isRename) { + // We must query the server to know if the etag has not changed + _pendingAsyncJobs++; + auto job = new RequestEtagJob(_discoveryData->_account, originalPath, this); + connect(job, &RequestEtagJob::finishedWithResult, this, [=](const Result &etag) mutable { + if (!etag || (*etag != base._etag && !item->isDirectory()) || _discoveryData->_renamedItems.contains(originalPath)) { + qCInfo(lcDisco) << "Can't rename because the etag has changed or the directory is gone" << originalPath; + // Can't be a rename, leave it as a new. + } else { + // 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); + + processRename(path); + recurseQueryServer = *etag == base._etag ? ParentNotChanged : NormalQuery; + } + + qCInfo(lcDisco) << "Discovered" << item->_file << item->_instruction << item->_direction << item->isDirectory(); + if (item->isDirectory()) { + auto job = new ProcessDirectoryJob(item, recurseQueryServer, NormalQuery, _discoveryData, this); + job->_currentFolder = path; + connect(job, &ProcessDirectoryJob::itemDiscovered, this, &ProcessDirectoryJob::itemDiscovered); + connect(job, &ProcessDirectoryJob::finished, this, &ProcessDirectoryJob::subJobFinished); + _queuedJobs.push_back(job); + } else { + emit itemDiscovered(item); + } + _pendingAsyncJobs--; + progress(); + }); + job->start(); + return; } } } else { item->_instruction = CSYNC_INSTRUCTION_SYNC; + if (_queryServer != ParentNotChanged && !serverEntry.isValid()) { + // Special case! deleted on server, modified on client, the instruction is then NEW + item->_instruction = CSYNC_INSTRUCTION_NEW; + } item->_direction = SyncFileItem::Up; item->_checksumHeader.clear(); item->_size = localEntry.size; diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index 10647690f..727c44b23 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -24,51 +24,6 @@ class ExcludedFiles; namespace OCC { class SyncJournalDb; -enum ErrorTag { Error }; - -template -class Result -{ - union { - T _result; - QString _errorString; - }; - bool _isError; - -public: - Result(T value) - : _result(std::move(value)) - , _isError(false){}; - Result(ErrorTag, QString str) - : _errorString(std::move(str)) - , _isError(true) - { - } - ~Result() - { - if (_isError) - _errorString.~QString(); - else - _result.~T(); - } - explicit operator bool() const { return !_isError; } - const T &operator*() const & - { - ASSERT(!_isError); - return _result; - } - T operator*() && - { - ASSERT(!_isError); - return std::move(_result); - } - QString errorMessage() const - { - ASSERT(_isError); - return _errorString; - } -}; - struct RemoteInfo { QString name; diff --git a/src/libsync/networkjobs.cpp b/src/libsync/networkjobs.cpp index fc8464558..a32286b41 100644 --- a/src/libsync/networkjobs.cpp +++ b/src/libsync/networkjobs.cpp @@ -96,7 +96,8 @@ bool RequestEtagJob::finished() qCInfo(lcEtagJob) << "Request Etag of" << reply()->request().url() << "FINISHED WITH STATUS" << replyStatusString(); - if (reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute) == 207) { + auto httpCode = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + if (httpCode == 207) { // Parse DAV response QXmlStreamReader reader(reply()); reader.addExtraNamespaceDeclaration(QXmlStreamNamespaceDeclaration("d", "DAV:")); @@ -111,6 +112,9 @@ bool RequestEtagJob::finished() } } emit etagRetrieved(etag); + emit finishedWithResult(etag); + } else { + emit finishedWithResult({ httpCode, errorString() }); } return true; } diff --git a/src/libsync/networkjobs.h b/src/libsync/networkjobs.h index 510a2a6bf..743b87ac5 100644 --- a/src/libsync/networkjobs.h +++ b/src/libsync/networkjobs.h @@ -335,6 +335,7 @@ public: signals: void etagRetrieved(const QString &etag); + void finishedWithResult(const Result &etag); private slots: bool finished() override; diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 33aa7414f..c5d43afe3 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -470,6 +470,11 @@ private slots: { resetCounters(); local.setContents("AM/a2m", 'C'); + // We must change the modtime for it is likely that it did not change between sync. + // (Previous version of the client (<=2.5) would not need this because it was always doing + // checksum comparison for all renames. But newer version no longer does it if the file is + // renamed because the parent folder is renamed) + local.setModTime("AM/a2m", QDateTime::currentDateTimeUtc().addDays(3)); local.rename("AM", "A2"); remote.setContents("BM/b2m", 'C'); remote.rename("BM", "B2"); -- 2.30.2