From 7e024638b5dde075ba313919823c2bae2c5e7f58 Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Tue, 9 Apr 2019 15:18:51 +0200 Subject: [PATCH] Discovery: No fatal error on bad server response #6826 Previously receiving unexpected replies from the server during discovery would terminate the sync run with a fatal error. Now it only causes an error on the affected item. --- src/libsync/discovery.cpp | 34 ++++++++++++++-------------- src/libsync/owncloudpropagator.h | 10 +++++++- src/libsync/syncengine.cpp | 1 - test/testremotediscovery.cpp | 39 +++++++++++++++++++++----------- 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 13de3edb359..e36135e6aa0 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -383,7 +383,6 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( missingData.append(tr("file id")); if (!missingData.isEmpty()) { item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_status = SyncFileItem::NormalError; _childIgnored = true; item->_errorString = tr("server reported no %1").arg(missingData.join(QLatin1String(", "))); emit _discoveryData->itemDiscovered(item); @@ -1137,13 +1136,11 @@ bool ProcessDirectoryJob::checkPermissions(const OCC::SyncFileItemPtr &item) } else if (item->isDirectory() && !perms.hasPermission(RemotePermissions::CanAddSubDirectories)) { qCWarning(lcDisco) << "checkForPermission: ERROR" << item->_file; item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_status = SyncFileItem::NormalError; item->_errorString = tr("Not allowed because you don't have permission to add subfolders to that folder"); return false; } else if (!item->isDirectory() && !perms.hasPermission(RemotePermissions::CanAddFile)) { qCWarning(lcDisco) << "checkForPermission: ERROR" << item->_file; item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_status = SyncFileItem::NormalError; item->_errorString = tr("Not allowed because you don't have permission to add files in that folder"); return false; } @@ -1337,31 +1334,34 @@ DiscoverySingleDirectoryJob *ProcessDirectoryJob::startAsyncServerQuery() if (_localQueryDone) this->process(); } else { - if (results.error().code == 403) { - // 403 Forbidden can be sent by the server if the file firewall is active. - // A file or directory should be ignored and sync must continue. See #3490 - qCWarning(lcDisco, "Directory access Forbidden (File Firewall?)"); + auto dirErrorOrFatal = [&](csync_instructions_e instr) { if (_dirItem) { - _dirItem->_instruction = CSYNC_INSTRUCTION_IGNORE; + _dirItem->_instruction = instr; _dirItem->_errorString = results.error().message; emit this->finished(); - return; + } else { + // Fatal for the root job since it has no SyncFileItem + emit _discoveryData->fatalError(tr("Server replied with an error while reading directory '%1' : %2") + .arg(_currentFolder._server, results.error().message)); } + }; + + if (results.error().code == 403) { + // 403 Forbidden can be sent by the server if the file firewall is active. + // A file or directory should be ignored and sync must continue. See #3490 + qCWarning(lcDisco, "Directory access Forbidden (File Firewall?)"); + dirErrorOrFatal(CSYNC_INSTRUCTION_IGNORE); } else if (results.error().code == 503) { // The server usually replies with the custom "503 Storage not available" // if some path is temporarily unavailable. But in some cases a standard 503 // is returned too. Thus we can't distinguish the two and will treat any // 503 as request to ignore the folder. See #3113 #2884. qCWarning(lcDisco(), "Storage was not available!"); - if (_dirItem) { - _dirItem->_instruction = CSYNC_INSTRUCTION_IGNORE; - _dirItem->_errorString = results.error().message; - emit this->finished(); - return; - } + dirErrorOrFatal(CSYNC_INSTRUCTION_IGNORE); + } else { + qCWarning(lcDisco) << "Server error in directory" << _currentFolder._server << results.error().message; + dirErrorOrFatal(CSYNC_INSTRUCTION_ERROR); } - emit _discoveryData->fatalError(tr("Server replied with an error while reading directory '%1' : %2") - .arg(_currentFolder._server, results.error().message)); } }); connect(serverJob, &DiscoverySingleDirectoryJob::firstDirectoryPermissions, this, diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index 3005a847b97..6abe7bd38e6 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -353,7 +353,15 @@ class PropagateIgnoreJob : public PropagateItemJob void start() Q_DECL_OVERRIDE { SyncFileItem::Status status = _item->_status; - done(status == SyncFileItem::NoStatus ? SyncFileItem::FileIgnored : status, _item->_errorString); + if (status == SyncFileItem::NoStatus) { + if (_item->_instruction == CSYNC_INSTRUCTION_ERROR) { + status = SyncFileItem::NormalError; + } else { + status = SyncFileItem::FileIgnored; + ASSERT(_item->_instruction == CSYNC_INSTRUCTION_IGNORE); + } + } + done(status, _item->_errorString); } }; diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 551106a4603..fb1c888a326 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -347,7 +347,6 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) QString error; if (!_syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId, &error)) { item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_status = SyncFileItem::NormalError; item->_errorString = tr("Could not update virtual file metadata: %1").arg(error); return; } diff --git a/test/testremotediscovery.cpp b/test/testremotediscovery.cpp index 9eecb094b3a..f4752708c7e 100644 --- a/test/testremotediscovery.cpp +++ b/test/testremotediscovery.cpp @@ -64,15 +64,16 @@ private slots: QTest::addColumn("errorKind"); QTest::addColumn("expectedErrorString"); - QString httpErrorMessage = "Server replied with an error while reading directory 'B' : Internal Server Fake Error"; + QString httpErrorMessage = "Internal Server Fake Error"; + QTest::newRow("403") << 403 << httpErrorMessage; QTest::newRow("404") << 404 << httpErrorMessage; QTest::newRow("500") << 500 << httpErrorMessage; QTest::newRow("503") << 503 << httpErrorMessage; // 200 should be an error since propfind should return 207 QTest::newRow("200") << 200 << httpErrorMessage; - QTest::newRow("InvalidXML") << +InvalidXML << "error while reading directory 'B' : Unknown error"; - QTest::newRow("Timeout") << +Timeout << "error while reading directory 'B' : Operation canceled"; + QTest::newRow("InvalidXML") << +InvalidXML << "Unknown error"; + QTest::newRow("Timeout") << +Timeout << "Operation canceled"; } @@ -81,7 +82,8 @@ private slots: { QFETCH(int, errorKind); QFETCH(QString, expectedErrorString); - bool syncSucceeds = errorKind == 503; // 503 just ignore the temporarily unavailable directory + // 403/503 just ignore the temporarily unavailable directory + bool syncSucceeds = errorKind == 503 || errorKind == 403; FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; @@ -96,9 +98,10 @@ private slots: auto oldLocalState = fakeFolder.currentLocalState(); auto oldRemoteState = fakeFolder.currentRemoteState(); + QString errorFolder = "webdav/B"; fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) -> QNetworkReply *{ - if (req.attribute(QNetworkRequest::CustomVerbAttribute) == "PROPFIND" && req.url().path().endsWith("/B")) { + if (req.attribute(QNetworkRequest::CustomVerbAttribute) == "PROPFIND" && req.url().path().endsWith(errorFolder)) { if (errorKind == InvalidXML) { return new FakeBrokenXmlPropfindReply(fakeFolder.remoteModifier(), op, req, this); } else if (errorKind == Timeout) { @@ -113,22 +116,32 @@ private slots: // So the test that test timeout finishes fast QScopedValueRollback setHttpTimeout(AbstractNetworkJob::httpTimeout, errorKind == Timeout ? 1 : 10000); - QSignalSpy errorSpy(&fakeFolder.syncEngine(), &SyncEngine::syncError); + QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &))); QCOMPARE(fakeFolder.syncOnce(), syncSucceeds); - qDebug() << "errorSpy=" << errorSpy; // The folder B should not have been sync'ed (and in particular not removed) QCOMPARE(oldLocalState.children["B"], fakeFolder.currentLocalState().children["B"]); QCOMPARE(oldRemoteState.children["B"], fakeFolder.currentRemoteState().children["B"]); if (!syncSucceeds) { - // Check we got the right error - QCOMPARE(errorSpy.count(), 1); - QVERIFY(errorSpy[0][0].toString().contains(expectedErrorString)); + QCOMPARE(findItem(completeSpy, "B")->_instruction, CSYNC_INSTRUCTION_ERROR); } else { - // The other folder should have been sync'ed as the sync just ignored the faulty dir - QCOMPARE(fakeFolder.currentRemoteState().children["A"], fakeFolder.currentLocalState().children["A"]); - QCOMPARE(fakeFolder.currentRemoteState().children["C"], fakeFolder.currentLocalState().children["C"]); + QCOMPARE(findItem(completeSpy, "B")->_instruction, CSYNC_INSTRUCTION_IGNORE); } + QVERIFY(findItem(completeSpy, "B")->_errorString.contains(expectedErrorString)); + + // The other folder should have been sync'ed as the sync just ignored the faulty dir + QCOMPARE(fakeFolder.currentRemoteState().children["A"], fakeFolder.currentLocalState().children["A"]); + QCOMPARE(fakeFolder.currentRemoteState().children["C"], fakeFolder.currentLocalState().children["C"]); + QCOMPARE(findItem(completeSpy, "A/z1")->_instruction, CSYNC_INSTRUCTION_NEW); + + // + // Check the same discovery error on the sync root + // + errorFolder = "webdav/"; + QSignalSpy errorSpy(&fakeFolder.syncEngine(), &SyncEngine::syncError); + QVERIFY(!fakeFolder.syncOnce()); + QVERIFY(errorSpy.size() == 1); + QVERIFY(errorSpy[0][0].toString().contains(expectedErrorString)); } void testMissingData()