Skip to content

Commit

Permalink
Discovery: No fatal error on bad server response #6826
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ckamm
ckamm committed Apr 9, 2019
1 parent fcf2af3 commit 7e02463
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 32 deletions.
34 changes: 17 additions & 17 deletions src/libsync/discovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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,
Expand Down
10 changes: 9 additions & 1 deletion src/libsync/owncloudpropagator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};

Expand Down
1 change: 0 additions & 1 deletion src/libsync/syncengine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
39 changes: 26 additions & 13 deletions test/testremotediscovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,16 @@ private slots:
QTest::addColumn<int>("errorKind");
QTest::addColumn<QString>("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";
}


Expand All @@ -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() };

Expand All @@ -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) {
Expand All @@ -113,22 +116,32 @@ private slots:
// So the test that test timeout finishes fast
QScopedValueRollback<int> 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()
Expand Down

0 comments on commit 7e02463

Please sign in to comment.