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.

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

Propagator: Fix finished signal of directory being emited twice and c… #5579

Merged
merged 1 commit into from
Mar 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 8 additions & 3 deletions src/libsync/owncloudpropagator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -669,9 +669,11 @@ void PropagateDirectory::slotSubJobFinished(SyncFileItem::Status status)

if (status == SyncFileItem::FatalError ||
(wasFirstJob && status != SyncFileItem::Success && status != SyncFileItem::Restoration)) {
abort();
_state = Finished;
emit finished(status);
if (_state != Finished) {
abort();
_state = Finished;
emit finished(status);
}
return;
} else if (status == SyncFileItem::NormalError || status == SyncFileItem::SoftError) {
_hasError = status;
Expand All @@ -688,6 +690,9 @@ void PropagateDirectory::slotSubJobFinished(SyncFileItem::Status status)

void PropagateDirectory::finalize()
{
if (_state == Finished)
return;

bool ok = true;
if (!_item->isEmpty() && _hasError == SyncFileItem::NoStatus) {
if( !_item->_renameTarget.isEmpty() ) {
Expand Down
42 changes: 33 additions & 9 deletions test/syncenginetestutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ class FakeGetReply : public QNetworkReply
const FileInfo *fileInfo;
char payload;
int size;
bool aborted = false;

FakeGetReply(FileInfo &remoteRootFileInfo, QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent)
: QNetworkReply{parent} {
Expand All @@ -551,6 +552,12 @@ class FakeGetReply : public QNetworkReply
}

Q_INVOKABLE void respond() {
if (aborted) {
setError(OperationCanceledError, "Operation Canceled");
emit metaDataChanged();
emit finished();
return;
}
payload = fileInfo->contentChar;
size = fileInfo->size;
setHeader(QNetworkRequest::ContentLengthHeader, size);
Expand All @@ -564,8 +571,14 @@ class FakeGetReply : public QNetworkReply
emit finished();
}

void abort() override { }
qint64 bytesAvailable() const override { return size + QIODevice::bytesAvailable(); }
void abort() override {
aborted = true;
}
qint64 bytesAvailable() const override {
if (aborted)
return 0;
return size + QIODevice::bytesAvailable();
}

qint64 readData(char *data, qint64 maxlen) override {
qint64 len = std::min(qint64{size}, maxlen);
Expand Down Expand Up @@ -666,8 +679,9 @@ class FakeErrorReply : public QNetworkReply
{
Q_OBJECT
public:
FakeErrorReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request, QObject *parent)
: QNetworkReply{parent} {
FakeErrorReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request,
QObject *parent, int httpErrorCode)
: QNetworkReply{parent}, _httpErrorCode(httpErrorCode) {
setRequest(request);
setUrl(request.url());
setOperation(op);
Expand All @@ -676,34 +690,38 @@ class FakeErrorReply : public QNetworkReply
}

Q_INVOKABLE void respond() {
setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 500);
setAttribute(QNetworkRequest::HttpStatusCodeAttribute, _httpErrorCode);
setError(InternalServerError, "Internal Server Fake Error");
emit metaDataChanged();
emit finished();
}

void abort() override { }
qint64 readData(char *, qint64) override { return 0; }

int _httpErrorCode;
};

class FakeQNAM : public QNetworkAccessManager
{
FileInfo _remoteRootFileInfo;
FileInfo _uploadFileInfo;
QStringList _errorPaths;
// maps a path to an HTTP error
QHash<QString, int> _errorPaths;
public:
FakeQNAM(FileInfo initialRoot) : _remoteRootFileInfo{std::move(initialRoot)} { }
FileInfo &currentRemoteState() { return _remoteRootFileInfo; }
FileInfo &uploadState() { return _uploadFileInfo; }
QStringList &errorPaths() { return _errorPaths; }

QHash<QString, int> &errorPaths() { return _errorPaths; }

protected:
QNetworkReply *createRequest(Operation op, const QNetworkRequest &request,
QIODevice *outgoingData = 0) {
const QString fileName = getFilePathFromUrl(request.url());
Q_ASSERT(!fileName.isNull());
if (_errorPaths.contains(fileName))
return new FakeErrorReply{op, request, this};
return new FakeErrorReply{op, request, this, _errorPaths[fileName]};

bool isUpload = request.url().path().startsWith(sUploadUrl.path());
FileInfo &info = isUpload ? _uploadFileInfo : _remoteRootFileInfo;
Expand Down Expand Up @@ -798,7 +816,13 @@ class FakeFolder
FileInfo currentRemoteState() { return _fakeQnam->currentRemoteState(); }
FileInfo &uploadState() { return _fakeQnam->uploadState(); }

QStringList &serverErrorPaths() { return _fakeQnam->errorPaths(); }
struct ErrorList {
FakeQNAM *_qnam;
void append(const QString &path, int error = 500)
{ _qnam->errorPaths().insert(path, error); }
void clear() { _qnam->errorPaths().clear(); }
};
ErrorList serverErrorPaths() { return {_fakeQnam}; }

QString localPath() const {
// SyncEngine wants a trailing slash
Expand Down
34 changes: 34 additions & 0 deletions test/testsyncengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,40 @@ private slots:
QCOMPARE(finishedSpy.size(), 1);
QCOMPARE(finishedSpy.first().first().toBool(), false);
}

void testDirDownloadWithError() {
FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
fakeFolder.remoteModifier().mkdir("Y");
fakeFolder.remoteModifier().mkdir("Y/Z");
fakeFolder.remoteModifier().insert("Y/Z/d0");
fakeFolder.remoteModifier().insert("Y/Z/d1");
fakeFolder.remoteModifier().insert("Y/Z/d2");
fakeFolder.remoteModifier().insert("Y/Z/d3");
fakeFolder.remoteModifier().insert("Y/Z/d4");
fakeFolder.remoteModifier().insert("Y/Z/d5");
fakeFolder.remoteModifier().insert("Y/Z/d6");
fakeFolder.remoteModifier().insert("Y/Z/d7");
fakeFolder.remoteModifier().insert("Y/Z/d8");
fakeFolder.remoteModifier().insert("Y/Z/d9");
fakeFolder.serverErrorPaths().append("Y/Z/d2", 503); // 503 is a fatal error
fakeFolder.serverErrorPaths().append("Y/Z/d3", 503); // 503 is a fatal error
QVERIFY(!fakeFolder.syncOnce());
QCoreApplication::processEvents(); // should not crash

QSet<QString> seen;
for(const QList<QVariant> &args : completeSpy) {
auto item = args[0].value<SyncFileItemPtr>();
qDebug() << item->_file << item->_isDirectory << item->_status;
QVERIFY(!seen.contains(item->_file)); // signal only sent once per item
seen.insert(item->_file);
if (item->_file == "Y/Z/d2" || item->_file == "Y/Z/d3") {
QVERIFY(item->_status == SyncFileItem::FatalError);
}
QVERIFY(item->_file != "Y/Z/d9"); // we should have aborted the sync before d9 starts
}
}

};

QTEST_GUILESS_MAIN(TestSyncEngine)
Expand Down