Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Discovery: No fatal error on bad server response #7134

Closed
wants to merge 1 commit into from
Closed

Discovery: No fatal error on bad server response #7134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions src/libsync/discovery.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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