From 09cacc4cd438dd3d16ed9924b2bfe171d37f4e5a Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Wed, 4 Apr 2018 16:27:08 +0200 Subject: [PATCH] Blacklist: remember the X-Request-ID Issue #6420 Store the X-Request-ID in the SyncFileItem and also in the blacklist. Note that for consistency reason, the X-Request-ID is also in the SyncFileItem if the request succeeds. Currently there is no UI to access it, but it can be queried with sql commands --- src/common/syncjournaldb.cpp | 20 +++++++++++++++----- src/common/syncjournalfilerecord.h | 3 +++ src/libsync/abstractnetworkjob.cpp | 5 +++++ src/libsync/abstractnetworkjob.h | 2 ++ src/libsync/owncloudpropagator.cpp | 1 + src/libsync/propagatedownload.cpp | 6 ++++-- src/libsync/propagateremotedelete.cpp | 4 ++-- src/libsync/propagateremotemkdir.cpp | 3 ++- src/libsync/propagateremotemove.cpp | 4 ++-- src/libsync/propagateupload.cpp | 1 + src/libsync/propagateuploadng.cpp | 7 ++++++- src/libsync/propagateuploadv1.cpp | 4 ++-- src/libsync/syncfileitem.h | 1 + test/testblacklist.cpp | 8 +++++++- 14 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp index 7a6bee7e9..41cf3e0bf 100644 --- a/src/common/syncjournaldb.cpp +++ b/src/common/syncjournaldb.cpp @@ -568,7 +568,7 @@ bool SyncJournalDb::checkConnect() return sqlFail("prepare _deleteUploadInfoQuery", _deleteUploadInfoQuery); } - QByteArray sql("SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory " + QByteArray sql("SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory, requestId " "FROM blacklist WHERE path=?1"); if (Utility::fsCasePreserving()) { // if the file system is case preserving we have to check the blacklist @@ -798,6 +798,16 @@ bool SyncJournalDb::updateErrorBlacklistTableStructure() commitInternal("update database structure: add errorCategory col"); } + if (columns.indexOf("requestId") == -1) { + SqlQuery query(_db); + query.prepare("ALTER TABLE blacklist ADD COLUMN requestId VARCHAR(36);"); + if (!query.exec()) { + sqlFail("updateBlacklistTableStructure: Add requestId", query); + re = false; + } + commitInternal("update database structure: add errorCategory col"); + } + SqlQuery query(_db); query.prepare("CREATE INDEX IF NOT EXISTS blacklist_index ON blacklist(path collate nocase);"); if (!query.exec()) { @@ -1538,8 +1548,6 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry(const QString if (file.isEmpty()) return entry; - // SELECT lastTryEtag, lastTryModtime, retrycount, errorstring - if (checkConnect()) { _getErrorBlacklistQuery.reset_and_clear_bindings(); _getErrorBlacklistQuery.bindValue(1, file); @@ -1554,6 +1562,7 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry(const QString entry._renameTarget = _getErrorBlacklistQuery.stringValue(6); entry._errorCategory = static_cast( _getErrorBlacklistQuery.intValue(7)); + entry._requestId = _getErrorBlacklistQuery.baValue(8); entry._file = file; } } @@ -1673,8 +1682,8 @@ void SyncJournalDb::setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord if (!_setErrorBlacklistQuery.initOrReset(QByteArrayLiteral( "INSERT OR REPLACE INTO blacklist " - "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory) " - "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)"), _db)) { + "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory, requestId) " + "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)"), _db)) { return; } @@ -1687,6 +1696,7 @@ void SyncJournalDb::setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord _setErrorBlacklistQuery.bindValue(7, item._ignoreDuration); _setErrorBlacklistQuery.bindValue(8, item._renameTarget); _setErrorBlacklistQuery.bindValue(9, item._errorCategory); + _setErrorBlacklistQuery.bindValue(10, item._requestId); _setErrorBlacklistQuery.exec(); } diff --git a/src/common/syncjournalfilerecord.h b/src/common/syncjournalfilerecord.h index 2bff92022..91e7fb870 100644 --- a/src/common/syncjournalfilerecord.h +++ b/src/common/syncjournalfilerecord.h @@ -102,6 +102,9 @@ public: QString _file; QString _renameTarget; + /// The last X-Request-ID of the request that failled + QByteArray _requestId; + bool isValid() const; }; diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index 0168c9053..f5695fec8 100644 --- a/src/libsync/abstractnetworkjob.cpp +++ b/src/libsync/abstractnetworkjob.cpp @@ -275,6 +275,11 @@ QByteArray AbstractNetworkJob::responseTimestamp() return _responseTimestamp; } +QByteArray AbstractNetworkJob::requestId() +{ + return _reply ? _reply->request().rawHeader("X-Request-ID") : QByteArray(); +} + QString AbstractNetworkJob::errorString() const { if (_timedout) { diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index bf35a0792..a05711916 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -69,6 +69,8 @@ public: bool followRedirects() const { return _followRedirects; } QByteArray responseTimestamp(); + /* Content of the X-Request-ID header. (Only set after the request is sent) */ + QByteArray requestId(); qint64 timeoutMsec() const { return _timer.interval(); } bool timedOut() const { return _timedout; } diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 5584c8652..14f3c03f4 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -139,6 +139,7 @@ static SyncJournalErrorBlacklistRecord createBlacklistEntry( entry._lastTryTime = Utility::qDateTimeToTime_t(QDateTime::currentDateTimeUtc()); entry._renameTarget = item._renameTarget; entry._retryCount = old._retryCount + 1; + entry._requestId = item._requestId; static qint64 minBlacklistTime(getMinBlacklistTime()); static qint64 maxBlacklistTime(qMax(getMaxBlacklistTime(), minBlacklistTime)); diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 32f5840ac..69ba22bd7 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -590,9 +590,12 @@ void PropagateDownloadFile::slotGetFinished() GETFileJob *job = _job; ASSERT(job); + _item->_httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_responseTimeStamp = job->responseTimestamp(); + _item->_requestId = job->requestId(); + QNetworkReply::NetworkError err = job->reply()->error(); if (err != QNetworkReply::NoError) { - _item->_httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); // If we sent a 'Range' header and get 416 back, we want to retry // without the header. @@ -671,7 +674,6 @@ void PropagateDownloadFile::slotGetFinished() // so make sure we have the up-to-date time _item->_modtime = job->lastModified(); } - _item->_responseTimeStamp = job->responseTimestamp(); _tmpFile.close(); _tmpFile.flush(); diff --git a/src/libsync/propagateremotedelete.cpp b/src/libsync/propagateremotedelete.cpp index 931bbf00f..848d94328 100644 --- a/src/libsync/propagateremotedelete.cpp +++ b/src/libsync/propagateremotedelete.cpp @@ -130,6 +130,8 @@ void PropagateRemoteDelete::slotDeleteJobFinished() QNetworkReply::NetworkError err = _job->reply()->error(); const int httpStatus = _job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); _item->_httpErrorCode = httpStatus; + _item->_responseTimeStamp = _job->responseTimestamp(); + _item->_requestId = _job->requestId(); if (err != QNetworkReply::NoError && err != QNetworkReply::ContentNotFoundError) { SyncFileItem::Status status = classifyError(err, _item->_httpErrorCode, @@ -138,8 +140,6 @@ void PropagateRemoteDelete::slotDeleteJobFinished() return; } - _item->_responseTimeStamp = _job->responseTimestamp(); - // A 404 reply is also considered a success here: We want to make sure // a file is gone from the server. It not being there in the first place // is ok. This will happen for files that are in the DB but not on diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index d19bcf481..d2fd73700 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -193,6 +193,8 @@ void PropagateRemoteMkdir::slotMkcolJobFinished() QNetworkReply::NetworkError err = _job->reply()->error(); _item->_httpErrorCode = _job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_responseTimeStamp = _job->responseTimestamp(); + _item->_requestId = _job->requestId(); if (_item->_httpErrorCode == 405) { // This happens when the directory already exists. Nothing to do. @@ -212,7 +214,6 @@ void PropagateRemoteMkdir::slotMkcolJobFinished() return; } - _item->_responseTimeStamp = _job->responseTimestamp(); _item->_fileId = _job->reply()->rawHeader("OC-FileId"); if (_item->_fileId.isEmpty()) { diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index 39fb1a032..45fa23e15 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -116,6 +116,8 @@ void PropagateRemoteMove::slotMoveJobFinished() QNetworkReply::NetworkError err = _job->reply()->error(); _item->_httpErrorCode = _job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_responseTimeStamp = _job->responseTimestamp(); + _item->_requestId = _job->requestId(); if (err != QNetworkReply::NoError) { SyncFileItem::Status status = classifyError(err, _item->_httpErrorCode, @@ -124,8 +126,6 @@ void PropagateRemoteMove::slotMoveJobFinished() return; } - _item->_responseTimeStamp = _job->responseTimestamp(); - if (_item->_httpErrorCode != 201) { // Normally we expect "201 Created" // If it is not the case, it might be because of a proxy or gateway intercepting the request, so we must diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 19067957e..60bcef43c 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -113,6 +113,7 @@ bool PollJob::finished() QNetworkReply::NetworkError err = reply()->error(); if (err != QNetworkReply::NoError) { _item->_httpErrorCode = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_requestId = requestId(); _item->_status = classifyError(err, _item->_httpErrorCode); _item->_errorString = errorString(); diff --git a/src/libsync/propagateuploadng.cpp b/src/libsync/propagateuploadng.cpp index 1ab60b323..7732af3a6 100644 --- a/src/libsync/propagateuploadng.cpp +++ b/src/libsync/propagateuploadng.cpp @@ -185,6 +185,7 @@ void PropagateUploadFileNG::slotPropfindFinishedWithError() auto httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); auto status = classifyError(err, httpErrorCode, &propagator()->_anotherSyncNeeded); if (status == SyncFileItem::FatalError) { + _item->_requestId = job->requestId(); propagator()->_activeJobList.removeOne(this); abortWithError(status, job->errorStringParsingBody()); return; @@ -203,6 +204,7 @@ void PropagateUploadFileNG::slotDeleteJobFinished() const int httpStatus = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); SyncFileItem::Status status = classifyError(err, httpStatus); if (status == SyncFileItem::FatalError) { + _item->_requestId = job->requestId(); abortWithError(status, job->errorString()); return; } else { @@ -262,6 +264,7 @@ void PropagateUploadFileNG::slotMkColFinished(QNetworkReply::NetworkError) _item->_httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); if (err != QNetworkReply::NoError || _item->_httpErrorCode != 201) { + _item->_requestId = job->requestId(); SyncFileItem::Status status = classifyError(err, _item->_httpErrorCode, &propagator()->_anotherSyncNeeded); abortWithError(status, job->errorStringParsingBody()); @@ -368,6 +371,7 @@ void PropagateUploadFileNG::slotPutFinished() if (err != QNetworkReply::NoError) { _item->_httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_requestId = job->requestId(); commonErrorHandling(job); return; } @@ -448,6 +452,8 @@ void PropagateUploadFileNG::slotMoveJobFinished() slotJobDestroyed(job); // remove it from the _jobs list QNetworkReply::NetworkError err = job->reply()->error(); _item->_httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_responseTimeStamp = job->responseTimestamp(); + _item->_requestId = job->requestId(); if (err != QNetworkReply::NoError) { commonErrorHandling(job); @@ -478,7 +484,6 @@ void PropagateUploadFileNG::slotMoveJobFinished() abortWithError(SyncFileItem::NormalError, tr("Missing ETag from server")); return; } - _item->_responseTimeStamp = job->responseTimestamp(); finalize(); } diff --git a/src/libsync/propagateuploadv1.cpp b/src/libsync/propagateuploadv1.cpp index 2214d459c..e28a7be62 100644 --- a/src/libsync/propagateuploadv1.cpp +++ b/src/libsync/propagateuploadv1.cpp @@ -201,6 +201,8 @@ void PropagateUploadFileV1::slotPutFinished() } _item->_httpErrorCode = job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); + _item->_responseTimeStamp = job->responseTimestamp(); + _item->_requestId = job->requestId(); QNetworkReply::NetworkError err = job->reply()->error(); if (err != QNetworkReply::NoError) { commonErrorHandling(job); @@ -306,8 +308,6 @@ void PropagateUploadFileV1::slotPutFinished() _item->_etag = etag; - _item->_responseTimeStamp = job->responseTimestamp(); - if (job->reply()->rawHeader("X-OC-MTime") != "accepted") { // X-OC-MTime is supported since owncloud 5.0. But not when chunking. // Normally Owncloud 6 always puts X-OC-MTime diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h index adcd4ae4d..f98ea8c0a 100644 --- a/src/libsync/syncfileitem.h +++ b/src/libsync/syncfileitem.h @@ -221,6 +221,7 @@ public: RemotePermissions _remotePerm; QString _errorString; // Contains a string only in case of error QByteArray _responseTimeStamp; + QByteArray _requestId; // X-Request-Id of the failed request quint32 _affectedItems = 1; // the number of affected items by the operation on this item. // usually this value is 1, but for removes on dirs, it might be much higher. diff --git a/test/testblacklist.cpp b/test/testblacklist.cpp index f571affa4..cad9f35f2 100644 --- a/test/testblacklist.cpp +++ b/test/testblacklist.cpp @@ -51,7 +51,9 @@ private slots: auto &modifier = remote ? fakeFolder.remoteModifier() : fakeFolder.localModifier(); int counter = 0; - fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &, QIODevice *) -> QNetworkReply * { + QByteArray reqId; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) -> QNetworkReply * { + reqId = req.rawHeader("X-Request-ID"); if (!remote && op == QNetworkAccessManager::PutOperation) ++counter; if (remote && op == QNetworkAccessManager::GetOperation) @@ -82,6 +84,7 @@ private slots: QCOMPARE(entry._retryCount, 1); QCOMPARE(counter, 1); QVERIFY(entry._ignoreDuration > 0); + QCOMPARE(entry._requestId, reqId); if (remote) QCOMPARE(journalRecord(fakeFolder, "A")._etag, initialEtag); @@ -102,6 +105,7 @@ private slots: QCOMPARE(entry._retryCount, 1); QCOMPARE(counter, 1); QVERIFY(entry._ignoreDuration > 0); + QCOMPARE(entry._requestId, reqId); if (remote) QCOMPARE(journalRecord(fakeFolder, "A")._etag, initialEtag); @@ -128,6 +132,7 @@ private slots: QCOMPARE(entry._retryCount, 2); QCOMPARE(counter, 2); QVERIFY(entry._ignoreDuration > 0); + QCOMPARE(entry._requestId, reqId); if (remote) QCOMPARE(journalRecord(fakeFolder, "A")._etag, initialEtag); @@ -149,6 +154,7 @@ private slots: QCOMPARE(entry._retryCount, 3); QCOMPARE(counter, 3); QVERIFY(entry._ignoreDuration > 0); + QCOMPARE(entry._requestId, reqId); if (remote) QCOMPARE(journalRecord(fakeFolder, "A")._etag, initialEtag); -- 2.30.2