From aacc4bcb4a487871daae6717f77605aaba444341 Mon Sep 17 00:00:00 2001 From: Marc Deop Date: Sat, 2 Mar 2019 12:26:57 +0100 Subject: style: apply .clang-format to all .cpp and .h files --- lib/jobs/basejob.cpp | 436 ++++++++++++++---------------- lib/jobs/basejob.h | 584 +++++++++++++++++++++-------------------- lib/jobs/downloadfilejob.cpp | 61 ++--- lib/jobs/downloadfilejob.h | 28 +- lib/jobs/mediathumbnailjob.cpp | 37 +-- lib/jobs/mediathumbnailjob.h | 29 +- lib/jobs/postreadmarkersjob.h | 20 +- lib/jobs/requestdata.cpp | 19 +- lib/jobs/requestdata.h | 37 ++- lib/jobs/syncjob.cpp | 18 +- lib/jobs/syncjob.h | 26 +- 11 files changed, 624 insertions(+), 671 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 8c3381ae..a023d4f7 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -13,7 +13,7 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #include "basejob.h" @@ -21,19 +21,18 @@ #include "connectiondata.h" #include "util.h" +#include +#include +#include #include -#include #include -#include -#include -#include +#include #include using namespace QMatrixClient; -struct NetworkReplyDeleter : public QScopedPointerDeleteLater -{ +struct NetworkReplyDeleter : public QScopedPointerDeleteLater { static inline void cleanup(QNetworkReply* reply) { if (reply && reply->isRunning()) @@ -45,51 +44,58 @@ struct NetworkReplyDeleter : public QScopedPointerDeleteLater class BaseJob::Private { public: - // Using an idiom from clang-tidy: - // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html - Private(HttpVerb v, QString endpoint, const QUrlQuery& q, - Data&& data, bool nt) - : verb(v), apiEndpoint(std::move(endpoint)), requestQuery(q) - , requestData(std::move(data)), needsToken(nt) - { } - - void sendRequest(bool inBackground); - const JobTimeoutConfig& getCurrentTimeoutConfig() const; - - const ConnectionData* connection = nullptr; - - // Contents for the network request - HttpVerb verb; - QString apiEndpoint; - QHash requestHeaders; - QUrlQuery requestQuery; - Data requestData; - bool needsToken; - - // There's no use of QMimeType here because we don't want to match - // content types against the known MIME type hierarchy; and at the same - // type QMimeType is of little help with MIME type globs (`text/*` etc.) - QByteArrayList expectedContentTypes; - - QScopedPointer reply; - Status status = Pending; - QByteArray rawResponse; - QUrl errorUrl; //< May contain a URL to help with some errors - - QTimer timer; - QTimer retryTimer; - - QVector errorStrategy = - { { 90, 5 }, { 90, 10 }, { 120, 30 } }; - int maxRetries = errorStrategy.size(); - int retriesTaken = 0; - - LoggingCategory logCat = JOBS; + // Using an idiom from clang-tidy: + // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html + Private(HttpVerb v, QString endpoint, const QUrlQuery& q, Data&& data, + bool nt) + : verb(v), + apiEndpoint(std::move(endpoint)), + requestQuery(q), + requestData(std::move(data)), + needsToken(nt) + { + } + + void sendRequest(bool inBackground); + const JobTimeoutConfig& getCurrentTimeoutConfig() const; + + const ConnectionData* connection = nullptr; + + // Contents for the network request + HttpVerb verb; + QString apiEndpoint; + QHash requestHeaders; + QUrlQuery requestQuery; + Data requestData; + bool needsToken; + + // There's no use of QMimeType here because we don't want to match + // content types against the known MIME type hierarchy; and at the same + // type QMimeType is of little help with MIME type globs (`text/*` etc.) + QByteArrayList expectedContentTypes; + + QScopedPointer reply; + Status status = Pending; + QByteArray rawResponse; + QUrl errorUrl; //< May contain a URL to help with some errors + + QTimer timer; + QTimer retryTimer; + + QVector errorStrategy = { { 90, 5 }, + { 90, 10 }, + { 120, 30 } }; + int maxRetries = errorStrategy.size(); + int retriesTaken = 0; + + LoggingCategory logCat = JOBS; }; -BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, bool needsToken) - : BaseJob(verb, name, endpoint, Query { }, Data { }, needsToken) -{ } +BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, + bool needsToken) + : BaseJob(verb, name, endpoint, Query {}, Data {}, needsToken) +{ +} BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, const Query& query, Data&& data, bool needsToken) @@ -98,7 +104,7 @@ BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, setObjectName(name); setExpectedContentTypes({ "application/json" }); d->timer.setSingleShot(true); - connect (&d->timer, &QTimer::timeout, this, &BaseJob::timeout); + connect(&d->timer, &QTimer::timeout, this, &BaseJob::timeout); } BaseJob::~BaseJob() @@ -114,21 +120,20 @@ QUrl BaseJob::requestUrl() const bool BaseJob::isBackground() const { - return d->reply && d->reply->request().attribute( - QNetworkRequest::BackgroundRequestAttribute).toBool(); + return d->reply + && d->reply->request() + .attribute(QNetworkRequest::BackgroundRequestAttribute) + .toBool(); } -const QString& BaseJob::apiEndpoint() const -{ - return d->apiEndpoint; -} +const QString& BaseJob::apiEndpoint() const { return d->apiEndpoint; } void BaseJob::setApiEndpoint(const QString& apiEndpoint) { d->apiEndpoint = apiEndpoint; } -const BaseJob::headers_t&BaseJob::requestHeaders() const +const BaseJob::headers_t& BaseJob::requestHeaders() const { return d->requestHeaders; } @@ -144,25 +149,16 @@ void BaseJob::setRequestHeaders(const BaseJob::headers_t& headers) d->requestHeaders = headers; } -const QUrlQuery& BaseJob::query() const -{ - return d->requestQuery; -} +const QUrlQuery& BaseJob::query() const { return d->requestQuery; } void BaseJob::setRequestQuery(const QUrlQuery& query) { d->requestQuery = query; } -const BaseJob::Data& BaseJob::requestData() const -{ - return d->requestData; -} +const BaseJob::Data& BaseJob::requestData() const { return d->requestData; } -void BaseJob::setRequestData(Data&& data) -{ - std::swap(d->requestData, data); -} +void BaseJob::setRequestData(Data&& data) { std::swap(d->requestData, data); } const QByteArrayList& BaseJob::expectedContentTypes() const { @@ -179,22 +175,22 @@ void BaseJob::setExpectedContentTypes(const QByteArrayList& contentTypes) d->expectedContentTypes = contentTypes; } -QUrl BaseJob::makeRequestUrl(QUrl baseUrl, - const QString& path, const QUrlQuery& query) +QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QString& path, + const QUrlQuery& query) { auto pathBase = baseUrl.path(); if (!pathBase.endsWith('/') && !path.startsWith('/')) pathBase.push_back('/'); - baseUrl.setPath( pathBase + path ); + baseUrl.setPath(pathBase + path); baseUrl.setQuery(query); return baseUrl; } void BaseJob::Private::sendRequest(bool inBackground) { - QNetworkRequest req - { makeRequestUrl(connection->baseUrl(), apiEndpoint, requestQuery) }; + QNetworkRequest req { makeRequestUrl(connection->baseUrl(), apiEndpoint, + requestQuery) }; if (!requestHeaders.contains("Content-Type")) req.setHeader(QNetworkRequest::ContentTypeHeader, "application/json"); req.setRawHeader("Authorization", @@ -211,38 +207,34 @@ void BaseJob::Private::sendRequest(bool inBackground) #endif for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) req.setRawHeader(it.key(), it.value()); - switch( verb ) - { - case HttpVerb::Get: - reply.reset( connection->nam()->get(req) ); - break; - case HttpVerb::Post: - reply.reset( connection->nam()->post(req, requestData.source()) ); - break; - case HttpVerb::Put: - reply.reset( connection->nam()->put(req, requestData.source()) ); - break; - case HttpVerb::Delete: - reply.reset( connection->nam()->deleteResource(req) ); - break; + switch (verb) { + case HttpVerb::Get: + reply.reset(connection->nam()->get(req)); + break; + case HttpVerb::Post: + reply.reset(connection->nam()->post(req, requestData.source())); + break; + case HttpVerb::Put: + reply.reset(connection->nam()->put(req, requestData.source())); + break; + case HttpVerb::Delete: + reply.reset(connection->nam()->deleteResource(req)); + break; } } -void BaseJob::beforeStart(const ConnectionData*) -{ } +void BaseJob::beforeStart(const ConnectionData*) {} -void BaseJob::afterStart(const ConnectionData*, QNetworkReply*) -{ } +void BaseJob::afterStart(const ConnectionData*, QNetworkReply*) {} -void BaseJob::beforeAbandon(QNetworkReply*) -{ } +void BaseJob::beforeAbandon(QNetworkReply*) {} void BaseJob::start(const ConnectionData* connData, bool inBackground) { d->connection = connData; d->retryTimer.setSingleShot(true); - connect (&d->retryTimer, &QTimer::timeout, - this, [this,inBackground] { sendRequest(inBackground); }); + connect(&d->retryTimer, &QTimer::timeout, this, + [this, inBackground] { sendRequest(inBackground); }); beforeStart(connData); if (status().good()) @@ -261,27 +253,23 @@ void BaseJob::sendRequest(bool inBackground) if (!d->requestQuery.isEmpty()) qCDebug(d->logCat) << " query:" << d->requestQuery.toString(); d->sendRequest(inBackground); - connect( d->reply.data(), &QNetworkReply::finished, this, &BaseJob::gotReply ); - if (d->reply->isRunning()) - { - connect( d->reply.data(), &QNetworkReply::metaDataChanged, - this, &BaseJob::checkReply); - connect( d->reply.data(), &QNetworkReply::uploadProgress, - this, &BaseJob::uploadProgress); - connect( d->reply.data(), &QNetworkReply::downloadProgress, - this, &BaseJob::downloadProgress); + connect(d->reply.data(), &QNetworkReply::finished, this, + &BaseJob::gotReply); + if (d->reply->isRunning()) { + connect(d->reply.data(), &QNetworkReply::metaDataChanged, this, + &BaseJob::checkReply); + connect(d->reply.data(), &QNetworkReply::uploadProgress, this, + &BaseJob::uploadProgress); + connect(d->reply.data(), &QNetworkReply::downloadProgress, this, + &BaseJob::downloadProgress); d->timer.start(getCurrentTimeout()); qCDebug(d->logCat) << this << "request has been sent"; emit started(); - } - else + } else qCWarning(d->logCat) << this << "request could not start"; } -void BaseJob::checkReply() -{ - setStatus(doCheckReply(d->reply.data())); -} +void BaseJob::checkReply() { setStatus(doCheckReply(d->reply.data())); } void BaseJob::gotReply() { @@ -293,20 +281,18 @@ void BaseJob::gotReply() d->rawResponse = d->reply->readAll(); const auto jsonBody = d->reply->rawHeader("Content-Type") == "application/json"; - qCDebug(d->logCat).noquote() - << "Error body (truncated if long):" << d->rawResponse.left(500); - if (jsonBody) - { + qCDebug(d->logCat).noquote() << "Error body (truncated if long):" + << d->rawResponse.left(500); + if (jsonBody) { auto json = QJsonDocument::fromJson(d->rawResponse).object(); const auto errCode = json.value("errcode"_ls).toString(); - if (error() == TooManyRequestsError || - errCode == "M_LIMIT_EXCEEDED") - { + if (error() == TooManyRequestsError + || errCode == "M_LIMIT_EXCEEDED") { QString msg = tr("Too many requests"); auto retryInterval = json.value("retry_after_ms"_ls).toInt(-1); if (retryInterval != -1) msg += tr(", next retry advised after %1 ms") - .arg(retryInterval); + .arg(retryInterval); else // We still have to figure some reasonable interval retryInterval = getNextRetryInterval(); @@ -320,19 +306,16 @@ void BaseJob::gotReply() emit retryScheduled(d->retriesTaken, retryInterval); return; } - if (errCode == "M_CONSENT_NOT_GIVEN") - { + if (errCode == "M_CONSENT_NOT_GIVEN") { d->status.code = UserConsentRequiredError; d->errorUrl = json.value("consent_uri"_ls).toString(); - } - else if (errCode == "M_UNSUPPORTED_ROOM_VERSION" || - errCode == "M_INCOMPATIBLE_ROOM_VERSION") - { + } else if (errCode == "M_UNSUPPORTED_ROOM_VERSION" + || errCode == "M_INCOMPATIBLE_ROOM_VERSION") { d->status.code = UnsupportedRoomVersionError; if (json.contains("room_version")) d->status.message = - tr("Requested room version: %1") - .arg(json.value("room_version").toString()); + tr("Requested room version: %1") + .arg(json.value("room_version").toString()); } else if (!json.isEmpty()) // Not localisable on the client side setStatus(IncorrectRequestError, json.value("error"_ls).toString()); @@ -350,18 +333,18 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) // ignore possible appendixes of the content type const auto ctype = type.split(';').front(); - for (const auto& pattern: patterns) - { + for (const auto& pattern : patterns) { if (pattern.startsWith('*') || ctype == pattern) // Fast lane return true; auto patternParts = pattern.split('/'); Q_ASSERT_X(patternParts.size() <= 2, __FUNCTION__, - "BaseJob: Expected content type should have up to two" - " /-separated parts; violating pattern: " + pattern); + "BaseJob: Expected content type should have up to two" + " /-separated parts; violating pattern: " + + pattern); - if (ctype.split('/').front() == patternParts.front() && - patternParts.back() == "*") + if (ctype.split('/').front() == patternParts.front() + && patternParts.back() == "*") return true; // Exact match already went on fast lane } @@ -376,24 +359,26 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes const auto httpCodeHeader = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); - if (!httpCodeHeader.isValid()) - { + if (!httpCodeHeader.isValid()) { qCWarning(d->logCat) << this << "didn't get valid HTTP headers"; return { NetworkError, reply->errorString() }; } - const QString replyState = reply->isRunning() ? - QStringLiteral("(tentative)") : QStringLiteral("(final)"); + const QString replyState = reply->isRunning() + ? QStringLiteral("(tentative)") + : QStringLiteral("(final)"); const auto urlString = '|' + d->reply->url().toDisplayString(); const auto httpCode = httpCodeHeader.toInt(); const auto reason = - reply->attribute(QNetworkRequest::HttpReasonPhraseAttribute).toString(); + reply->attribute(QNetworkRequest::HttpReasonPhraseAttribute) + .toString(); if (httpCode / 100 == 2) // 2xx { qCDebug(d->logCat).noquote().nospace() << this << urlString; - qCDebug(d->logCat).noquote() << " " << httpCode << reason << replyState; + qCDebug(d->logCat).noquote() + << " " << httpCode << reason << replyState; if (!checkContentType(reply->rawHeader("Content-Type"), - d->expectedContentTypes)) + d->expectedContentTypes)) return { UnexpectedResponseTypeWarning, "Unexpected content type of the response" }; return NoError; @@ -402,29 +387,37 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const qCWarning(d->logCat).noquote().nospace() << this << urlString; qCWarning(d->logCat).noquote() << " " << httpCode << reason << replyState; return { [httpCode]() -> StatusCode { - if (httpCode / 10 == 41) - return httpCode == 410 ? IncorrectRequestError : NotFoundError; - switch (httpCode) - { - case 401: case 403: case 407: + if (httpCode / 10 == 41) + return httpCode == 410 ? IncorrectRequestError + : NotFoundError; + switch (httpCode) { + case 401: + case 403: + case 407: return ContentAccessError; case 404: return NotFoundError; - case 400: case 405: case 406: case 426: case 428: + case 400: + case 405: + case 406: + case 426: + case 428: case 505: case 494: // Unofficial nginx "Request header too large" case 497: // Unofficial nginx "HTTP request sent to HTTPS port" return IncorrectRequestError; case 429: return TooManyRequestsError; - case 501: case 510: + case 501: + case 510: return RequestNotImplementedError; case 511: return NetworkAuthRequiredError; default: return NetworkError; - } - }(), reply->errorString() }; + } + }(), + reply->errorString() }; } BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) @@ -432,30 +425,25 @@ BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) d->rawResponse = reply->readAll(); QJsonParseError error; const auto& json = QJsonDocument::fromJson(d->rawResponse, &error); - if( error.error == QJsonParseError::NoError ) + if (error.error == QJsonParseError::NoError) return parseJson(json); return { IncorrectResponseError, error.errorString() }; } -BaseJob::Status BaseJob::parseJson(const QJsonDocument&) -{ - return Success; -} +BaseJob::Status BaseJob::parseJson(const QJsonDocument&) { return Success; } void BaseJob::stop() { d->timer.stop(); - if (d->reply) - { + if (d->reply) { d->reply->disconnect(this); // Ignore whatever comes from the reply - if (d->reply->isRunning()) - { - qCWarning(d->logCat) << this << "stopped without ready network reply"; + if (d->reply->isRunning()) { + qCWarning(d->logCat) + << this << "stopped without ready network reply"; d->reply->abort(); } - } - else + } else qCWarning(d->logCat) << this << "stopped with empty network reply"; } @@ -463,16 +451,16 @@ void BaseJob::finishJob() { stop(); if ((error() == NetworkError || error() == TimeoutError) - && d->retriesTaken < d->maxRetries) - { + && d->retriesTaken < d->maxRetries) { // TODO: The whole retrying thing should be put to ConnectionManager // otherwise independently retrying jobs make a bit of notification // storm towards the UI. const auto retryInterval = error() == TimeoutError ? 0 : getNextRetryInterval(); ++d->retriesTaken; - qCWarning(d->logCat).nospace() << this << ": retry #" << d->retriesTaken - << " in " << retryInterval/1000 << " s"; + qCWarning(d->logCat).nospace() + << this << ": retry #" << d->retriesTaken << " in " + << retryInterval / 1000 << " s"; d->retryTimer.start(retryInterval); emit retryScheduled(d->retriesTaken, retryInterval); return; @@ -510,93 +498,78 @@ BaseJob::duration_t BaseJob::millisToRetry() const return d->retryTimer.isActive() ? d->retryTimer.remainingTime() : 0; } -int BaseJob::maxRetries() const -{ - return d->maxRetries; -} +int BaseJob::maxRetries() const { return d->maxRetries; } void BaseJob::setMaxRetries(int newMaxRetries) { d->maxRetries = newMaxRetries; } -BaseJob::Status BaseJob::status() const -{ - return d->status; -} +BaseJob::Status BaseJob::status() const { return d->status; } QByteArray BaseJob::rawData(int bytesAtMost) const { return bytesAtMost > 0 && d->rawResponse.size() > bytesAtMost - ? d->rawResponse.left(bytesAtMost) : d->rawResponse; + ? d->rawResponse.left(bytesAtMost) + : d->rawResponse; } QString BaseJob::rawDataSample(int bytesAtMost) const { auto data = rawData(bytesAtMost); Q_ASSERT(data.size() <= d->rawResponse.size()); - return data.size() == d->rawResponse.size() - ? data : data + tr("...(truncated, %Ln bytes in total)", - "Comes after trimmed raw network response", - d->rawResponse.size()); - + return data.size() == d->rawResponse.size() ? data + : data + + tr("...(truncated, %Ln bytes in total)", + "Comes after trimmed raw network response", + d->rawResponse.size()); } QString BaseJob::statusCaption() const { - switch (d->status.code) - { - case Success: - return tr("Success"); - case Pending: - return tr("Request still pending response"); - case UnexpectedResponseTypeWarning: - return tr("Warning: Unexpected response type"); - case Abandoned: - return tr("Request was abandoned"); - case NetworkError: - return tr("Network problems"); - case JsonParseError: - return tr("Response could not be parsed"); - case TimeoutError: - return tr("Request timed out"); - case ContentAccessError: - return tr("Access error"); - case NotFoundError: - return tr("Not found"); - case IncorrectRequestError: - return tr("Invalid request"); - case IncorrectResponseError: - return tr("Response could not be parsed"); - case TooManyRequestsError: - return tr("Too many requests"); - case RequestNotImplementedError: - return tr("Function not implemented by the server"); - case NetworkAuthRequiredError: - return tr("Network authentication required"); - case UserConsentRequiredError: - return tr("User consent required"); - case UnsupportedRoomVersionError: - return tr("The server does not support the needed room version"); - default: - return tr("Request failed"); + switch (d->status.code) { + case Success: + return tr("Success"); + case Pending: + return tr("Request still pending response"); + case UnexpectedResponseTypeWarning: + return tr("Warning: Unexpected response type"); + case Abandoned: + return tr("Request was abandoned"); + case NetworkError: + return tr("Network problems"); + case JsonParseError: + return tr("Response could not be parsed"); + case TimeoutError: + return tr("Request timed out"); + case ContentAccessError: + return tr("Access error"); + case NotFoundError: + return tr("Not found"); + case IncorrectRequestError: + return tr("Invalid request"); + case IncorrectResponseError: + return tr("Response could not be parsed"); + case TooManyRequestsError: + return tr("Too many requests"); + case RequestNotImplementedError: + return tr("Function not implemented by the server"); + case NetworkAuthRequiredError: + return tr("Network authentication required"); + case UserConsentRequiredError: + return tr("User consent required"); + case UnsupportedRoomVersionError: + return tr("The server does not support the needed room version"); + default: + return tr("Request failed"); } } -int BaseJob::error() const -{ - return d->status.code; -} +int BaseJob::error() const { return d->status.code; } -QString BaseJob::errorString() const -{ - return d->status.message; -} +QString BaseJob::errorString() const { return d->status.message; } -QUrl BaseJob::errorUrl() const -{ - return d->errorUrl; -} +QUrl BaseJob::errorUrl() const { return d->errorUrl; } void BaseJob::setStatus(Status s) { @@ -629,11 +602,8 @@ void BaseJob::abandon() void BaseJob::timeout() { - setStatus( TimeoutError, "The job has timed out" ); + setStatus(TimeoutError, "The job has timed out"); finishJob(); } -void BaseJob::setLoggingCategory(LoggingCategory lcf) -{ - d->logCat = lcf; -} +void BaseJob::setLoggingCategory(LoggingCategory lcf) { d->logCat = lcf; } diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 4c1c7706..8ff25d42 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -13,7 +13,7 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #pragma once @@ -21,332 +21,336 @@ #include "../logging.h" #include "requestdata.h" +#include #include #include -#include class QNetworkReply; class QSslError; -namespace QMatrixClient -{ +namespace QMatrixClient { class ConnectionData; enum class HttpVerb { Get, Put, Post, Delete }; - struct JobTimeoutConfig - { + struct JobTimeoutConfig { int jobTimeout; int nextRetryInterval; }; - class BaseJob: public QObject + class BaseJob : public QObject { - Q_OBJECT - Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) - Q_PROPERTY(int maxRetries READ maxRetries WRITE setMaxRetries) + Q_OBJECT + Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) + Q_PROPERTY(int maxRetries READ maxRetries WRITE setMaxRetries) public: - /* Just in case, the values are compatible with KJob - * (which BaseJob used to inherit from). */ - enum StatusCode { NoError = 0 // To be compatible with Qt conventions - , Success = 0 - , Pending = 1 - , WarningLevel = 20 - , UnexpectedResponseTypeWarning = 21 - , Abandoned = 50 //< A very brief period between abandoning and object deletion - , ErrorLevel = 100 //< Errors have codes starting from this - , NetworkError = 100 - , JsonParseError // TODO: Merge into IncorrectResponseError - , TimeoutError - , ContentAccessError - , NotFoundError - , IncorrectRequestError - , IncorrectResponseError - , TooManyRequestsError - , RequestNotImplementedError - , UnsupportedRoomVersionError - , NetworkAuthRequiredError - , UserConsentRequiredError - , UserDefinedError = 200 - }; - - /** - * A simple wrapper around QUrlQuery that allows its creation from - * a list of string pairs - */ - class Query : public QUrlQuery + /* Just in case, the values are compatible with KJob + * (which BaseJob used to inherit from). */ + enum StatusCode { + NoError = 0 // To be compatible with Qt conventions + , + Success = 0, + Pending = 1, + WarningLevel = 20, + UnexpectedResponseTypeWarning = 21, + Abandoned = 50 //< A very brief period between abandoning and object + //deletion + , + ErrorLevel = 100 //< Errors have codes starting from this + , + NetworkError = 100, + JsonParseError // TODO: Merge into IncorrectResponseError + , + TimeoutError, + ContentAccessError, + NotFoundError, + IncorrectRequestError, + IncorrectResponseError, + TooManyRequestsError, + RequestNotImplementedError, + UnsupportedRoomVersionError, + NetworkAuthRequiredError, + UserConsentRequiredError, + UserDefinedError = 200 + }; + + /** + * A simple wrapper around QUrlQuery that allows its creation from + * a list of string pairs + */ + class Query : public QUrlQuery + { + public: + using QUrlQuery::QUrlQuery; + Query() = default; + Query(const std::initializer_list>& l) { - public: - using QUrlQuery::QUrlQuery; - Query() = default; - Query(const std::initializer_list< QPair >& l) - { - setQueryItems(l); - } - }; - - using Data = RequestData; - - /** - * This structure stores the status of a server call job. The status consists - * of a code, that is described (but not delimited) by the respective enum, - * and a freeform message. - * - * To extend the list of error codes, define an (anonymous) enum - * along the lines of StatusCode, with additional values - * starting at UserDefinedError - */ - class Status + setQueryItems(l); + } + }; + + using Data = RequestData; + + /** + * This structure stores the status of a server call job. The status + * consists of a code, that is described (but not delimited) by the + * respective enum, and a freeform message. + * + * To extend the list of error codes, define an (anonymous) enum + * along the lines of StatusCode, with additional values + * starting at UserDefinedError + */ + class Status + { + public: + Status(StatusCode c) : code(c) {} + Status(int c, QString m) : code(c), message(std::move(m)) {} + + bool good() const { return code < ErrorLevel; } + friend QDebug operator<<(QDebug dbg, const Status& s) { - public: - Status(StatusCode c) : code(c) { } - Status(int c, QString m) : code(c), message(std::move(m)) { } - - bool good() const { return code < ErrorLevel; } - friend QDebug operator<<(QDebug dbg, const Status& s) - { - QDebugStateSaver _s(dbg); - return dbg.noquote().nospace() - << s.code << ": " << s.message; - } - - bool operator==(const Status& other) const - { - return code == other.code && message == other.message; - } - bool operator!=(const Status& other) const - { - return !operator==(other); - } - - int code; - QString message; - }; - - using duration_t = int; // milliseconds + QDebugStateSaver _s(dbg); + return dbg.noquote().nospace() << s.code << ": " << s.message; + } - public: - BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, - bool needsToken = true); - BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, - const Query& query, Data&& data = {}, - bool needsToken = true); - - QUrl requestUrl() const; - bool isBackground() const; - - /** Current status of the job */ - Status status() const; - /** Short human-friendly message on the job status */ - QString statusCaption() const; - /** Get raw response body as received from the server - * \param bytesAtMost return this number of leftmost bytes, or -1 - * to return the entire response - */ - QByteArray rawData(int bytesAtMost = -1) const; - /** Get UI-friendly sample of raw data - * - * This is almost the same as rawData but appends the "truncated" - * suffix if not all data fit in bytesAtMost. This call is - * recommended to present a sample of raw data as "details" next to - * error messages. Note that the default \p bytesAtMost value is - * also tailored to UI cases. - */ - QString rawDataSample(int bytesAtMost = 65535) const; - - /** Error (more generally, status) code - * Equivalent to status().code - * \sa status - */ - int error() const; - /** Error-specific message, as returned by the server */ - virtual QString errorString() const; - /** A URL to help/clarify the error, if provided by the server */ - QUrl errorUrl() const; - - int maxRetries() const; - void setMaxRetries(int newMaxRetries); - - Q_INVOKABLE duration_t getCurrentTimeout() const; - Q_INVOKABLE duration_t getNextRetryInterval() const; - Q_INVOKABLE duration_t millisToRetry() const; - - friend QDebug operator<<(QDebug dbg, const BaseJob* j) + bool operator==(const Status& other) const { - return dbg << j->objectName(); + return code == other.code && message == other.message; } + bool operator!=(const Status& other) const + { + return !operator==(other); + } + + int code; + QString message; + }; + + using duration_t = int; // milliseconds + + public: + BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, + bool needsToken = true); + BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, + const Query& query, Data&& data = {}, bool needsToken = true); + + QUrl requestUrl() const; + bool isBackground() const; + + /** Current status of the job */ + Status status() const; + /** Short human-friendly message on the job status */ + QString statusCaption() const; + /** Get raw response body as received from the server + * \param bytesAtMost return this number of leftmost bytes, or -1 + * to return the entire response + */ + QByteArray rawData(int bytesAtMost = -1) const; + /** Get UI-friendly sample of raw data + * + * This is almost the same as rawData but appends the "truncated" + * suffix if not all data fit in bytesAtMost. This call is + * recommended to present a sample of raw data as "details" next to + * error messages. Note that the default \p bytesAtMost value is + * also tailored to UI cases. + */ + QString rawDataSample(int bytesAtMost = 65535) const; + + /** Error (more generally, status) code + * Equivalent to status().code + * \sa status + */ + int error() const; + /** Error-specific message, as returned by the server */ + virtual QString errorString() const; + /** A URL to help/clarify the error, if provided by the server */ + QUrl errorUrl() const; + + int maxRetries() const; + void setMaxRetries(int newMaxRetries); + + Q_INVOKABLE duration_t getCurrentTimeout() const; + Q_INVOKABLE duration_t getNextRetryInterval() const; + Q_INVOKABLE duration_t millisToRetry() const; + + friend QDebug operator<<(QDebug dbg, const BaseJob* j) + { + return dbg << j->objectName(); + } public slots: - void start(const ConnectionData* connData, - bool inBackground = false); - - /** - * Abandons the result of this job, arrived or unarrived. - * - * This aborts waiting for a reply from the server (if there was - * any pending) and deletes the job object. No result signals - * (result, success, failure) are emitted. - */ - void abandon(); + void start(const ConnectionData* connData, bool inBackground = false); + + /** + * Abandons the result of this job, arrived or unarrived. + * + * This aborts waiting for a reply from the server (if there was + * any pending) and deletes the job object. No result signals + * (result, success, failure) are emitted. + */ + void abandon(); signals: - /** The job is about to send a network request */ - void aboutToStart(); - - /** The job has sent a network request */ - void started(); - - /** The job has changed its status */ - void statusChanged(Status newStatus); - - /** - * The previous network request has failed; the next attempt will - * be done in the specified time - * @param nextAttempt the 1-based number of attempt (will always be more than 1) - * @param inMilliseconds the interval after which the next attempt will be taken - */ - void retryScheduled(int nextAttempt, int inMilliseconds); - - /** - * Emitted when the job is finished, in any case. It is used to notify - * observers that the job is terminated and that progress can be hidden. - * - * This should not be emitted directly by subclasses; - * use finishJob() instead. - * - * In general, to be notified of a job's completion, client code - * should connect to result(), success(), or failure() - * rather than finished(). However if you need to track the job's - * lifecycle you should connect to this instead of result(); - * in particular, only this signal will be emitted on abandoning. - * - * @param job the job that emitted this signal - * - * @see result, success, failure - */ - void finished(BaseJob* job); - - /** - * Emitted when the job is finished (except when abandoned). - * - * Use error() to know if the job was finished with error. - * - * @param job the job that emitted this signal - * - * @see success, failure - */ - void result(BaseJob* job); - - /** - * Emitted together with result() in case there's no error. - * - * @see result, failure - */ - void success(BaseJob*); - - /** - * Emitted together with result() if there's an error. - * Similar to result(), this won't be emitted in case of abandon(). - * - * @see result, success - */ - void failure(BaseJob*); - - void downloadProgress(qint64 bytesReceived, qint64 bytesTotal); - void uploadProgress(qint64 bytesSent, qint64 bytesTotal); + /** The job is about to send a network request */ + void aboutToStart(); + + /** The job has sent a network request */ + void started(); + + /** The job has changed its status */ + void statusChanged(Status newStatus); + + /** + * The previous network request has failed; the next attempt will + * be done in the specified time + * @param nextAttempt the 1-based number of attempt (will always be more + * than 1) + * @param inMilliseconds the interval after which the next attempt will + * be taken + */ + void retryScheduled(int nextAttempt, int inMilliseconds); + + /** + * Emitted when the job is finished, in any case. It is used to notify + * observers that the job is terminated and that progress can be hidden. + * + * This should not be emitted directly by subclasses; + * use finishJob() instead. + * + * In general, to be notified of a job's completion, client code + * should connect to result(), success(), or failure() + * rather than finished(). However if you need to track the job's + * lifecycle you should connect to this instead of result(); + * in particular, only this signal will be emitted on abandoning. + * + * @param job the job that emitted this signal + * + * @see result, success, failure + */ + void finished(BaseJob* job); + + /** + * Emitted when the job is finished (except when abandoned). + * + * Use error() to know if the job was finished with error. + * + * @param job the job that emitted this signal + * + * @see success, failure + */ + void result(BaseJob* job); + + /** + * Emitted together with result() in case there's no error. + * + * @see result, failure + */ + void success(BaseJob*); + + /** + * Emitted together with result() if there's an error. + * Similar to result(), this won't be emitted in case of abandon(). + * + * @see result, success + */ + void failure(BaseJob*); + + void downloadProgress(qint64 bytesReceived, qint64 bytesTotal); + void uploadProgress(qint64 bytesSent, qint64 bytesTotal); protected: - using headers_t = QHash; - - const QString& apiEndpoint() const; - void setApiEndpoint(const QString& apiEndpoint); - const headers_t& requestHeaders() const; - void setRequestHeader(const headers_t::key_type& headerName, - const headers_t::mapped_type& headerValue); - void setRequestHeaders(const headers_t& headers); - const QUrlQuery& query() const; - void setRequestQuery(const QUrlQuery& query); - const Data& requestData() const; - void setRequestData(Data&& data); - const QByteArrayList& expectedContentTypes() const; - void addExpectedContentType(const QByteArray& contentType); - void setExpectedContentTypes(const QByteArrayList& contentTypes); - - /** Construct a URL out of baseUrl, path and query - * The function automatically adds '/' between baseUrl's path and - * \p path if necessary. The query component of \p baseUrl - * is ignored. - */ - static QUrl makeRequestUrl(QUrl baseUrl, const QString& path, - const QUrlQuery& query = {}); - - virtual void beforeStart(const ConnectionData* connData); - virtual void afterStart(const ConnectionData* connData, - QNetworkReply* reply); - virtual void beforeAbandon(QNetworkReply*); - - /** - * Used by gotReply() to check the received reply for general - * issues such as network errors or access denial. - * Returning anything except NoError/Success prevents - * further parseReply()/parseJson() invocation. - * - * @param reply the reply received from the server - * @return the result of checking the reply - * - * @see gotReply - */ - virtual Status doCheckReply(QNetworkReply* reply) const; - - /** - * Processes the reply. By default, parses the reply into - * a QJsonDocument and calls parseJson() if it's a valid JSON. - * - * @param reply raw contents of a HTTP reply from the server (without headers) - * - * @see gotReply, parseJson - */ - virtual Status parseReply(QNetworkReply* reply); - - /** - * Processes the JSON document received from the Matrix server. - * By default returns succesful status without analysing the JSON. - * - * @param json valid JSON document received from the server - * - * @see parseReply - */ - virtual Status parseJson(const QJsonDocument&); - - void setStatus(Status s); - void setStatus(int code, QString message); - - // Q_DECLARE_LOGGING_CATEGORY return different function types - // in different versions - using LoggingCategory = decltype(JOBS)*; - void setLoggingCategory(LoggingCategory lcf); - - // Job objects should only be deleted via QObject::deleteLater - ~BaseJob() override; + using headers_t = QHash; + + const QString& apiEndpoint() const; + void setApiEndpoint(const QString& apiEndpoint); + const headers_t& requestHeaders() const; + void setRequestHeader(const headers_t::key_type& headerName, + const headers_t::mapped_type& headerValue); + void setRequestHeaders(const headers_t& headers); + const QUrlQuery& query() const; + void setRequestQuery(const QUrlQuery& query); + const Data& requestData() const; + void setRequestData(Data&& data); + const QByteArrayList& expectedContentTypes() const; + void addExpectedContentType(const QByteArray& contentType); + void setExpectedContentTypes(const QByteArrayList& contentTypes); + + /** Construct a URL out of baseUrl, path and query + * The function automatically adds '/' between baseUrl's path and + * \p path if necessary. The query component of \p baseUrl + * is ignored. + */ + static QUrl makeRequestUrl(QUrl baseUrl, const QString& path, + const QUrlQuery& query = {}); + + virtual void beforeStart(const ConnectionData* connData); + virtual void afterStart(const ConnectionData* connData, + QNetworkReply* reply); + virtual void beforeAbandon(QNetworkReply*); + + /** + * Used by gotReply() to check the received reply for general + * issues such as network errors or access denial. + * Returning anything except NoError/Success prevents + * further parseReply()/parseJson() invocation. + * + * @param reply the reply received from the server + * @return the result of checking the reply + * + * @see gotReply + */ + virtual Status doCheckReply(QNetworkReply* reply) const; + + /** + * Processes the reply. By default, parses the reply into + * a QJsonDocument and calls parseJson() if it's a valid JSON. + * + * @param reply raw contents of a HTTP reply from the server (without + * headers) + * + * @see gotReply, parseJson + */ + virtual Status parseReply(QNetworkReply* reply); + + /** + * Processes the JSON document received from the Matrix server. + * By default returns succesful status without analysing the JSON. + * + * @param json valid JSON document received from the server + * + * @see parseReply + */ + virtual Status parseJson(const QJsonDocument&); + + void setStatus(Status s); + void setStatus(int code, QString message); + + // Q_DECLARE_LOGGING_CATEGORY return different function types + // in different versions + using LoggingCategory = decltype(JOBS)*; + void setLoggingCategory(LoggingCategory lcf); + + // Job objects should only be deleted via QObject::deleteLater + ~BaseJob() override; protected slots: - void timeout(); + void timeout(); private slots: - void sendRequest(bool inBackground); - void checkReply(); - void gotReply(); + void sendRequest(bool inBackground); + void checkReply(); + void gotReply(); private: - void stop(); - void finishJob(); + void stop(); + void finishJob(); - class Private; - QScopedPointer d; + class Private; + QScopedPointer d; }; inline bool isJobRunning(BaseJob* job) { return job && job->error() == BaseJob::Pending; } -} // namespace QMatrixClient +} // namespace QMatrixClient diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 2bf9dd8f..12aacb8b 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -1,23 +1,24 @@ #include "downloadfilejob.h" -#include #include #include +#include using namespace QMatrixClient; class DownloadFileJob::Private { public: - Private() : tempFile(new QTemporaryFile()) { } + Private() : tempFile(new QTemporaryFile()) {} - explicit Private(const QString& localFilename) - : targetFile(new QFile(localFilename)) - , tempFile(new QFile(targetFile->fileName() + ".qmcdownload")) - { } + explicit Private(const QString& localFilename) + : targetFile(new QFile(localFilename)), + tempFile(new QFile(targetFile->fileName() + ".qmcdownload")) + { + } - QScopedPointer targetFile; - QScopedPointer tempFile; + QScopedPointer targetFile; + QScopedPointer tempFile; }; QUrl DownloadFileJob::makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri) @@ -28,8 +29,8 @@ QUrl DownloadFileJob::makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri) DownloadFileJob::DownloadFileJob(const QString& serverName, const QString& mediaId, const QString& localFilename) - : GetContentJob(serverName, mediaId) - , d(localFilename.isEmpty() ? new Private : new Private(localFilename)) + : GetContentJob(serverName, mediaId), + d(localFilename.isEmpty() ? new Private : new Private(localFilename)) { setObjectName("DownloadFileJob"); } @@ -41,16 +42,15 @@ QString DownloadFileJob::targetFileName() const void DownloadFileJob::beforeStart(const ConnectionData*) { - if (d->targetFile && !d->targetFile->isReadable() && - !d->targetFile->open(QIODevice::WriteOnly)) - { - qCWarning(JOBS) << "Couldn't open the file" - << d->targetFile->fileName() << "for writing"; + if (d->targetFile && !d->targetFile->isReadable() + && !d->targetFile->open(QIODevice::WriteOnly)) { + qCWarning(JOBS) << "Couldn't open the file" << d->targetFile->fileName() + << "for writing"; setStatus(FileError, "Could not open the target file for writing"); return; } - if (!d->tempFile->isReadable() && !d->tempFile->open(QIODevice::WriteOnly)) - { + if (!d->tempFile->isReadable() + && !d->tempFile->open(QIODevice::WriteOnly)) { qCWarning(JOBS) << "Couldn't open the temporary file" << d->tempFile->fileName() << "for writing"; setStatus(FileError, "Could not open the temporary download file"); @@ -61,16 +61,14 @@ void DownloadFileJob::beforeStart(const ConnectionData*) void DownloadFileJob::afterStart(const ConnectionData*, QNetworkReply* reply) { - connect(reply, &QNetworkReply::metaDataChanged, this, [this,reply] { + connect(reply, &QNetworkReply::metaDataChanged, this, [this, reply] { if (!status().good()) return; auto sizeHeader = reply->header(QNetworkRequest::ContentLengthHeader); - if (sizeHeader.isValid()) - { + if (sizeHeader.isValid()) { auto targetSize = sizeHeader.value(); if (targetSize != -1) - if (!d->tempFile->resize(targetSize)) - { + if (!d->tempFile->resize(targetSize)) { qCWarning(JOBS) << "Failed to allocate" << targetSize << "bytes for" << d->tempFile->fileName(); setStatus(FileError, @@ -78,16 +76,15 @@ void DownloadFileJob::afterStart(const ConnectionData*, QNetworkReply* reply) } } }); - connect(reply, &QIODevice::readyRead, this, [this,reply] { + connect(reply, &QIODevice::readyRead, this, [this, reply] { if (!status().good()) return; auto bytes = reply->read(reply->bytesAvailable()); if (!bytes.isEmpty()) d->tempFile->write(bytes); else - qCWarning(JOBS) - << "Unexpected empty chunk when downloading from" - << reply->url() << "to" << d->tempFile->fileName(); + qCWarning(JOBS) << "Unexpected empty chunk when downloading from" + << reply->url() << "to" << d->tempFile->fileName(); }); } @@ -100,22 +97,18 @@ void DownloadFileJob::beforeAbandon(QNetworkReply*) BaseJob::Status DownloadFileJob::parseReply(QNetworkReply*) { - if (d->targetFile) - { + if (d->targetFile) { d->targetFile->close(); - if (!d->targetFile->remove()) - { + if (!d->targetFile->remove()) { qCWarning(JOBS) << "Failed to remove the target file placeholder"; return { FileError, "Couldn't finalise the download" }; } - if (!d->tempFile->rename(d->targetFile->fileName())) - { + if (!d->tempFile->rename(d->targetFile->fileName())) { qCWarning(JOBS) << "Failed to rename" << d->tempFile->fileName() << "to" << d->targetFile->fileName(); return { FileError, "Couldn't finalise the download" }; } - } - else + } else d->tempFile->close(); qCDebug(JOBS) << "Saved a file as" << targetFileName(); return Success; diff --git a/lib/jobs/downloadfilejob.h b/lib/jobs/downloadfilejob.h index ce47ab1c..fd34ba5a 100644 --- a/lib/jobs/downloadfilejob.h +++ b/lib/jobs/downloadfilejob.h @@ -2,29 +2,27 @@ #include "csapi/content-repo.h" -namespace QMatrixClient -{ +namespace QMatrixClient { class DownloadFileJob : public GetContentJob { public: - enum { FileError = BaseJob::UserDefinedError + 1 }; + enum { FileError = BaseJob::UserDefinedError + 1 }; - using GetContentJob::makeRequestUrl; - static QUrl makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri); + using GetContentJob::makeRequestUrl; + static QUrl makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri); - DownloadFileJob(const QString& serverName, const QString& mediaId, - const QString& localFilename = {}); + DownloadFileJob(const QString& serverName, const QString& mediaId, + const QString& localFilename = {}); - QString targetFileName() const; + QString targetFileName() const; private: - class Private; - QScopedPointer d; + class Private; + QScopedPointer d; - void beforeStart(const ConnectionData*) override; - void afterStart(const ConnectionData*, - QNetworkReply* reply) override; - void beforeAbandon(QNetworkReply*) override; - Status parseReply(QNetworkReply*) override; + void beforeStart(const ConnectionData*) override; + void afterStart(const ConnectionData*, QNetworkReply* reply) override; + void beforeAbandon(QNetworkReply*) override; + Status parseReply(QNetworkReply*) override; }; } diff --git a/lib/jobs/mediathumbnailjob.cpp b/lib/jobs/mediathumbnailjob.cpp index aeb49839..d3370f1f 100644 --- a/lib/jobs/mediathumbnailjob.cpp +++ b/lib/jobs/mediathumbnailjob.cpp @@ -13,41 +13,42 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #include "mediathumbnailjob.h" using namespace QMatrixClient; -QUrl MediaThumbnailJob::makeRequestUrl(QUrl baseUrl, - const QUrl& mxcUri, QSize requestedSize) +QUrl MediaThumbnailJob::makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri, + QSize requestedSize) { - return makeRequestUrl(std::move(baseUrl), - mxcUri.authority(), mxcUri.path().mid(1), - requestedSize.width(), requestedSize.height()); + return makeRequestUrl(std::move(baseUrl), mxcUri.authority(), + mxcUri.path().mid(1), requestedSize.width(), + requestedSize.height()); } MediaThumbnailJob::MediaThumbnailJob(const QString& serverName, - const QString& mediaId, QSize requestedSize) - : GetContentThumbnailJob(serverName, mediaId, - requestedSize.width(), requestedSize.height()) -{ } + const QString& mediaId, + QSize requestedSize) + : GetContentThumbnailJob(serverName, mediaId, requestedSize.width(), + requestedSize.height()) +{ +} MediaThumbnailJob::MediaThumbnailJob(const QUrl& mxcUri, QSize requestedSize) - : MediaThumbnailJob(mxcUri.authority(), mxcUri.path().mid(1), // sans leading '/' + : MediaThumbnailJob(mxcUri.authority(), + mxcUri.path().mid(1), // sans leading '/' requestedSize) -{ } - -QImage MediaThumbnailJob::thumbnail() const { - return _thumbnail; } +QImage MediaThumbnailJob::thumbnail() const { return _thumbnail; } + QImage MediaThumbnailJob::scaledThumbnail(QSize toSize) const { - return _thumbnail.scaled(toSize, - Qt::KeepAspectRatio, Qt::SmoothTransformation); + return _thumbnail.scaled(toSize, Qt::KeepAspectRatio, + Qt::SmoothTransformation); } BaseJob::Status MediaThumbnailJob::parseReply(QNetworkReply* reply) @@ -56,7 +57,7 @@ BaseJob::Status MediaThumbnailJob::parseReply(QNetworkReply* reply) if (!result.good()) return result; - if( _thumbnail.loadFromData(data()->readAll()) ) + if (_thumbnail.loadFromData(data()->readAll())) return Success; return { IncorrectResponseError, "Could not read image data" }; diff --git a/lib/jobs/mediathumbnailjob.h b/lib/jobs/mediathumbnailjob.h index 7963796e..1dcf8ccb 100644 --- a/lib/jobs/mediathumbnailjob.h +++ b/lib/jobs/mediathumbnailjob.h @@ -13,7 +13,7 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #pragma once @@ -22,26 +22,25 @@ #include -namespace QMatrixClient -{ - class MediaThumbnailJob: public GetContentThumbnailJob +namespace QMatrixClient { + class MediaThumbnailJob : public GetContentThumbnailJob { public: - using GetContentThumbnailJob::makeRequestUrl; - static QUrl makeRequestUrl(QUrl baseUrl, - const QUrl& mxcUri, QSize requestedSize); + using GetContentThumbnailJob::makeRequestUrl; + static QUrl makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri, + QSize requestedSize); - MediaThumbnailJob(const QString& serverName, const QString& mediaId, - QSize requestedSize); - MediaThumbnailJob(const QUrl& mxcUri, QSize requestedSize); + MediaThumbnailJob(const QString& serverName, const QString& mediaId, + QSize requestedSize); + MediaThumbnailJob(const QUrl& mxcUri, QSize requestedSize); - QImage thumbnail() const; - QImage scaledThumbnail(QSize toSize) const; + QImage thumbnail() const; + QImage scaledThumbnail(QSize toSize) const; protected: - Status parseReply(QNetworkReply* reply) override; + Status parseReply(QNetworkReply* reply) override; private: - QImage _thumbnail; + QImage _thumbnail; }; -} // namespace QMatrixClient +} // namespace QMatrixClient diff --git a/lib/jobs/postreadmarkersjob.h b/lib/jobs/postreadmarkersjob.h index 63a8e1d0..3c5cac89 100644 --- a/lib/jobs/postreadmarkersjob.h +++ b/lib/jobs/postreadmarkersjob.h @@ -13,7 +13,7 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #pragma once @@ -27,13 +27,13 @@ using namespace QMatrixClient; class PostReadMarkersJob : public BaseJob { public: - explicit PostReadMarkersJob(const QString& roomId, - const QString& readUpToEventId) - : BaseJob(HttpVerb::Post, "PostReadMarkersJob", - QStringLiteral("_matrix/client/r0/rooms/%1/read_markers") - .arg(roomId)) - { - setRequestData(QJsonObject {{ - QStringLiteral("m.fully_read"), readUpToEventId }}); - } + explicit PostReadMarkersJob(const QString& roomId, + const QString& readUpToEventId) + : BaseJob(HttpVerb::Post, "PostReadMarkersJob", + QStringLiteral("_matrix/client/r0/rooms/%1/read_markers") + .arg(roomId)) + { + setRequestData(QJsonObject { + { QStringLiteral("m.fully_read"), readUpToEventId } }); + } }; diff --git a/lib/jobs/requestdata.cpp b/lib/jobs/requestdata.cpp index 5cb62221..477f49e7 100644 --- a/lib/jobs/requestdata.cpp +++ b/lib/jobs/requestdata.cpp @@ -1,10 +1,10 @@ #include "requestdata.h" +#include #include -#include #include #include -#include +#include using namespace QMatrixClient; @@ -17,22 +17,15 @@ auto fromData(const QByteArray& data) return source; } -template -inline auto fromJson(const JsonDataT& jdata) +template inline auto fromJson(const JsonDataT& jdata) { return fromData(QJsonDocument(jdata).toJson(QJsonDocument::Compact)); } -RequestData::RequestData(const QByteArray& a) - : _source(fromData(a)) -{ } +RequestData::RequestData(const QByteArray& a) : _source(fromData(a)) {} -RequestData::RequestData(const QJsonObject& jo) - : _source(fromJson(jo)) -{ } +RequestData::RequestData(const QJsonObject& jo) : _source(fromJson(jo)) {} -RequestData::RequestData(const QJsonArray& ja) - : _source(fromJson(ja)) -{ } +RequestData::RequestData(const QJsonArray& ja) : _source(fromJson(ja)) {} RequestData::~RequestData() = default; diff --git a/lib/jobs/requestdata.h b/lib/jobs/requestdata.h index db011b61..207ff731 100644 --- a/lib/jobs/requestdata.h +++ b/lib/jobs/requestdata.h @@ -13,7 +13,7 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #pragma once @@ -26,8 +26,7 @@ class QJsonArray; class QJsonDocument; class QIODevice; -namespace QMatrixClient -{ +namespace QMatrixClient { /** * A simple wrapper that represents the request body. * Provides a unified interface to dump an unstructured byte stream @@ -37,25 +36,23 @@ namespace QMatrixClient class RequestData { public: - RequestData() = default; - RequestData(const QByteArray& a); - RequestData(const QJsonObject& jo); - RequestData(const QJsonArray& ja); - RequestData(QIODevice* source) - : _source(std::unique_ptr(source)) - { } - RequestData(const RequestData&) = delete; - RequestData& operator=(const RequestData&) = delete; - RequestData(RequestData&&) = default; - RequestData& operator=(RequestData&&) = default; - ~RequestData(); + RequestData() = default; + RequestData(const QByteArray& a); + RequestData(const QJsonObject& jo); + RequestData(const QJsonArray& ja); + RequestData(QIODevice* source) + : _source(std::unique_ptr(source)) + { + } + RequestData(const RequestData&) = delete; + RequestData& operator=(const RequestData&) = delete; + RequestData(RequestData&&) = default; + RequestData& operator=(RequestData&&) = default; + ~RequestData(); - QIODevice* source() const - { - return _source.get(); - } + QIODevice* source() const { return _source.get(); } private: - std::unique_ptr _source; + std::unique_ptr _source; }; } // namespace QMatrixClient diff --git a/lib/jobs/syncjob.cpp b/lib/jobs/syncjob.cpp index 84385b55..db11005a 100644 --- a/lib/jobs/syncjob.cpp +++ b/lib/jobs/syncjob.cpp @@ -13,7 +13,7 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #include "syncjob.h" @@ -29,25 +29,26 @@ SyncJob::SyncJob(const QString& since, const QString& filter, int timeout, { setLoggingCategory(SYNCJOB); QUrlQuery query; - if( !filter.isEmpty() ) + if (!filter.isEmpty()) query.addQueryItem(QStringLiteral("filter"), filter); - if( !presence.isEmpty() ) + if (!presence.isEmpty()) query.addQueryItem(QStringLiteral("set_presence"), presence); - if( timeout >= 0 ) + if (timeout >= 0) query.addQueryItem(QStringLiteral("timeout"), QString::number(timeout)); - if( !since.isEmpty() ) + if (!since.isEmpty()) query.addQueryItem(QStringLiteral("since"), since); setRequestQuery(query); setMaxRetries(std::numeric_limits::max()); } -SyncJob::SyncJob(const QString& since, const Filter& filter, - int timeout, const QString& presence) +SyncJob::SyncJob(const QString& since, const Filter& filter, int timeout, + const QString& presence) : SyncJob(since, QJsonDocument(toJson(filter)).toJson(QJsonDocument::Compact), timeout, presence) -{ } +{ +} BaseJob::Status SyncJob::parseJson(const QJsonDocument& data) { @@ -59,4 +60,3 @@ BaseJob::Status SyncJob::parseJson(const QJsonDocument& data) << d.unresolvedRooms().join(','); return BaseJob::IncorrectResponseError; } - diff --git a/lib/jobs/syncjob.h b/lib/jobs/syncjob.h index 036b25d0..2afaf0f7 100644 --- a/lib/jobs/syncjob.h +++ b/lib/jobs/syncjob.h @@ -13,33 +13,31 @@ * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ #pragma once #include "basejob.h" -#include "../syncdata.h" #include "../csapi/definitions/sync_filter.h" +#include "../syncdata.h" -namespace QMatrixClient -{ - class SyncJob: public BaseJob +namespace QMatrixClient { + class SyncJob : public BaseJob { public: - explicit SyncJob(const QString& since = {}, - const QString& filter = {}, - int timeout = -1, const QString& presence = {}); - explicit SyncJob(const QString& since, const Filter& filter, - int timeout = -1, const QString& presence = {}); + explicit SyncJob(const QString& since = {}, const QString& filter = {}, + int timeout = -1, const QString& presence = {}); + explicit SyncJob(const QString& since, const Filter& filter, + int timeout = -1, const QString& presence = {}); - SyncData &&takeData() { return std::move(d); } + SyncData&& takeData() { return std::move(d); } protected: - Status parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: - SyncData d; + SyncData d; }; -} // namespace QMatrixClient +} // namespace QMatrixClient -- cgit v1.2.3 From d6cddf3e016a4fe3e906e2ab815a1e8b775c284c Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 14 Mar 2019 07:42:24 +0900 Subject: Fix read receipts and redactions on v3 rooms Previously slashes in eventIds (that come plenty in v3 due to base64 encoding) were not properly encoded - they are now. --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 8c3381ae..f738ce7a 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -186,7 +186,7 @@ QUrl BaseJob::makeRequestUrl(QUrl baseUrl, if (!pathBase.endsWith('/') && !path.startsWith('/')) pathBase.push_back('/'); - baseUrl.setPath( pathBase + path ); + baseUrl.setPath(pathBase + path, QUrl::TolerantMode); baseUrl.setQuery(query); return baseUrl; } -- cgit v1.2.3 From 432e7fd7107d8260e0016a1adcd8d94263dc1044 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 4 Apr 2019 21:27:38 +0900 Subject: Clean up on clang-tidy/clazy analysis --- lib/jobs/basejob.cpp | 2 +- lib/jobs/downloadfilejob.cpp | 5 +++-- lib/jobs/mediathumbnailjob.cpp | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index f738ce7a..f521cc4b 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -430,7 +430,7 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) { d->rawResponse = reply->readAll(); - QJsonParseError error; + QJsonParseError error { 0, QJsonParseError::MissingObject }; const auto& json = QJsonDocument::fromJson(d->rawResponse, &error); if( error.error == QJsonParseError::NoError ) return parseJson(json); diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 2bf9dd8f..672a7b2d 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -22,7 +22,8 @@ class DownloadFileJob::Private QUrl DownloadFileJob::makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri) { - return makeRequestUrl(baseUrl, mxcUri.authority(), mxcUri.path().mid(1)); + return makeRequestUrl( + std::move(baseUrl), mxcUri.authority(), mxcUri.path().mid(1)); } DownloadFileJob::DownloadFileJob(const QString& serverName, @@ -31,7 +32,7 @@ DownloadFileJob::DownloadFileJob(const QString& serverName, : GetContentJob(serverName, mediaId) , d(localFilename.isEmpty() ? new Private : new Private(localFilename)) { - setObjectName("DownloadFileJob"); + setObjectName(QStringLiteral("DownloadFileJob")); } QString DownloadFileJob::targetFileName() const diff --git a/lib/jobs/mediathumbnailjob.cpp b/lib/jobs/mediathumbnailjob.cpp index aeb49839..edb9b156 100644 --- a/lib/jobs/mediathumbnailjob.cpp +++ b/lib/jobs/mediathumbnailjob.cpp @@ -59,5 +59,5 @@ BaseJob::Status MediaThumbnailJob::parseReply(QNetworkReply* reply) if( _thumbnail.loadFromData(data()->readAll()) ) return Success; - return { IncorrectResponseError, "Could not read image data" }; + return { IncorrectResponseError, QStringLiteral("Could not read image data") }; } -- cgit v1.2.3 From a6df4183d6da324f2c2329f21d732071b3337cb8 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 15 Apr 2019 20:29:00 +0900 Subject: BaseJob: fix a possible crash upon logout See https://github.com/QMatrixClient/Quaternion/issues/566 for details. --- lib/jobs/basejob.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index f521cc4b..a79d0e03 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -600,10 +600,25 @@ QUrl BaseJob::errorUrl() const void BaseJob::setStatus(Status s) { + // The crash that led to this code has been reported in + // https://github.com/QMatrixClient/Quaternion/issues/566 - basically, + // when cleaning up childrent of a deleted Connection, there's a chance + // of pending jobs being abandoned, calling setStatus(Abandoned). + // There's nothing wrong with this; however, the safety check for + // cleartext access tokens below uses d->connection - which is a dangling + // pointer. + // To alleviate that, a stricter condition is applied, that for Abandoned + // and to-be-Abandoned jobs the status message will be disregarded entirely. + // For 0.6 we might rectify the situation by making d->connection + // a QPointer<> (and derive ConnectionData from QObject, respectively). + if (d->status.code == Abandoned || s.code == Abandoned) + s.message.clear(); + if (d->status == s) return; - if (d->connection && !d->connection->accessToken().isEmpty()) + if (!s.message.isEmpty() + && d->connection && !d->connection->accessToken().isEmpty()) s.message.replace(d->connection->accessToken(), "(REDACTED)"); if (!s.good()) qCWarning(d->logCat) << this << "status" << s; -- cgit v1.2.3 From 8a3bb2d5b27824890482fa7185bbc1fcda9dd49c Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 16 Apr 2019 16:21:53 +0900 Subject: BaseJob: preserve the calculated error code if JSON error code is unknown Resetting the code to IncorrectRequestError has been a part of the cause for the incorrect Quaternion behaviour on expired tokens. --- lib/jobs/basejob.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index a79d0e03..0d9b9f10 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -334,8 +334,7 @@ void BaseJob::gotReply() tr("Requested room version: %1") .arg(json.value("room_version").toString()); } else if (!json.isEmpty()) // Not localisable on the client side - setStatus(IncorrectRequestError, - json.value("error"_ls).toString()); + setStatus(d->status.code, json.value("error"_ls).toString()); } } -- cgit v1.2.3 From 26c51455901a6c0112531d86d61d65d31a70dfaa Mon Sep 17 00:00:00 2001 From: Ville Ranki Date: Thu, 4 Apr 2019 11:45:41 +0300 Subject: Ignore some errors on leaving rooms, add new error enum. Fixes #307 --- lib/jobs/basejob.cpp | 3 +++ lib/jobs/basejob.h | 1 + 2 files changed, 4 insertions(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 0d9b9f10..97b5a904 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -325,6 +325,9 @@ void BaseJob::gotReply() d->status.code = UserConsentRequiredError; d->errorUrl = json.value("consent_uri"_ls).toString(); } + else if (errCode == "M_UNKNOWN") { + d->status.code = UnknownObjectError; + } else if (errCode == "M_UNSUPPORTED_ROOM_VERSION" || errCode == "M_INCOMPATIBLE_ROOM_VERSION") { diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 4c1c7706..c1747cca 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -67,6 +67,7 @@ namespace QMatrixClient , UnsupportedRoomVersionError , NetworkAuthRequiredError , UserConsentRequiredError + , UnknownObjectError // Unknown room or other item (M_UNKNOWN) , UserDefinedError = 200 }; -- cgit v1.2.3 From e8718d8b1d61d6f70e61da0436f793a461e83bd7 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 7 Jun 2019 19:23:55 +0900 Subject: BaseJob::StatusCode: add Error-less synonyms; officially deprecate JsonParseError --- lib/jobs/basejob.cpp | 2 -- lib/jobs/basejob.h | 34 +++++++++++++++++++++------------- 2 files changed, 21 insertions(+), 15 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 0d9b9f10..34fc0f57 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -555,8 +555,6 @@ QString BaseJob::statusCaption() const return tr("Request was abandoned"); case NetworkError: return tr("Network problems"); - case JsonParseError: - return tr("Response could not be parsed"); case TimeoutError: return tr("Request timed out"); case ContentAccessError: diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 4c1c7706..4ee63adb 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -46,28 +46,36 @@ namespace QMatrixClient Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) Q_PROPERTY(int maxRetries READ maxRetries WRITE setMaxRetries) public: - /* Just in case, the values are compatible with KJob - * (which BaseJob used to inherit from). */ enum StatusCode { NoError = 0 // To be compatible with Qt conventions , Success = 0 , Pending = 1 , WarningLevel = 20 - , UnexpectedResponseTypeWarning = 21 + , UnexpectedResponseType = 21 + , UnexpectedResponseTypeWarning = UnexpectedResponseType , Abandoned = 50 //< A very brief period between abandoning and object deletion , ErrorLevel = 100 //< Errors have codes starting from this , NetworkError = 100 - , JsonParseError // TODO: Merge into IncorrectResponseError - , TimeoutError + , Timeout + , TimeoutError = Timeout , ContentAccessError , NotFoundError - , IncorrectRequestError - , IncorrectResponseError - , TooManyRequestsError - , RequestNotImplementedError - , UnsupportedRoomVersionError - , NetworkAuthRequiredError - , UserConsentRequiredError - , UserDefinedError = 200 + , IncorrectRequest + , IncorrectRequestError = IncorrectRequest + , IncorrectResponse + , IncorrectResponseError = IncorrectResponse + , JsonParseError //< deprecated; Use IncorrectResponse instead + = IncorrectResponse + , TooManyRequests + , TooManyRequestsError = TooManyRequests + , RequestNotImplemented + , RequestNotImplementedError = RequestNotImplemented + , UnsupportedRoomVersion + , UnsupportedRoomVersionError = UnsupportedRoomVersion + , NetworkAuthRequired + , NetworkAuthRequiredError = NetworkAuthRequired + , UserConsentRequired + , UserConsentRequiredError = UserConsentRequired + , UserDefinedError = 256 }; /** -- cgit v1.2.3 From 5722ceaf4bd10c29f1091e3dc5a87f5650ea8c71 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 26 Jun 2019 07:51:08 +0900 Subject: BaseJob::Status: fromHttpCode --- lib/jobs/basejob.cpp | 61 +++++++++++++++++++++++++--------------------------- lib/jobs/basejob.h | 4 ++-- 2 files changed, 31 insertions(+), 34 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index a0a3dc29..9c0b431c 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -348,6 +348,34 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) return false; } +BaseJob::Status BaseJob::Status::fromHttpCode(int httpCode, QString msg) +{ + // clang-format off + return { [httpCode]() -> StatusCode { + if (httpCode / 10 == 41) // 41x errors + return httpCode == 410 ? IncorrectRequestError : NotFoundError; + switch (httpCode) { + case 401: case 403: case 407: + return ContentAccessError; + case 404: + return NotFoundError; + case 400: case 405: case 406: case 426: case 428: case 505: + case 494: // Unofficial nginx "Request header too large" + case 497: // Unofficial nginx "HTTP request sent to HTTPS port" + return IncorrectRequestError; + case 429: + return TooManyRequestsError; + case 501: case 510: + return RequestNotImplementedError; + case 511: + return NetworkAuthRequiredError; + default: + return NetworkError; + } + }(), msg }; + // clang-format on +} + BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const { // QNetworkReply error codes seem to be flawed when it comes to HTTP; @@ -381,38 +409,7 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const qCWarning(d->logCat).noquote().nospace() << this << urlString; qCWarning(d->logCat).noquote() << " " << httpCode << reason << replyState; - return { [httpCode]() -> StatusCode { - if (httpCode / 10 == 41) - return httpCode == 410 ? IncorrectRequestError - : NotFoundError; - switch (httpCode) { - case 401: - case 403: - case 407: - return ContentAccessError; - case 404: - return NotFoundError; - case 400: - case 405: - case 406: - case 426: - case 428: - case 505: - case 494: // Unofficial nginx "Request header too large" - case 497: // Unofficial nginx "HTTP request sent to HTTPS port" - return IncorrectRequestError; - case 429: - return TooManyRequestsError; - case 501: - case 510: - return RequestNotImplementedError; - case 511: - return NetworkAuthRequiredError; - default: - return NetworkError; - } - }(), - reply->errorString() }; + return Status::fromHttpCode(httpCode, reply->errorString()); } BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 04d79c66..4d379f26 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -116,9 +116,8 @@ public: * along the lines of StatusCode, with additional values * starting at UserDefinedError */ - class Status + struct Status { - public: Status(StatusCode c) : code(c) {} @@ -126,6 +125,7 @@ public: : code(c) , message(std::move(m)) {} + static Status fromHttpCode(int httpCode, QString msg = {}); bool good() const { return code < ErrorLevel; } friend QDebug operator<<(QDebug dbg, const Status& s) -- cgit v1.2.3 From 12478cf7330727083103d22f76de92c4aa476f5b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 1 Jul 2019 16:10:35 +0900 Subject: Handle M_UNKNOWN as The Spec says; factor out BaseJob::parseError() --- lib/jobs/basejob.cpp | 96 +++++++++++++++++++++++++++------------------------- lib/jobs/basejob.h | 14 ++++++-- 2 files changed, 60 insertions(+), 50 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 97b5a904..dbf197ec 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -289,59 +289,23 @@ void BaseJob::gotReply() if (status().good()) setStatus(parseReply(d->reply.data())); else { - // FIXME: Factor out to smth like BaseJob::handleError() d->rawResponse = d->reply->readAll(); const auto jsonBody = - d->reply->rawHeader("Content-Type") == "application/json"; + d->reply->rawHeader("Content-Type") == "application/json"; qCDebug(d->logCat).noquote() - << "Error body (truncated if long):" << d->rawResponse.left(500); + << "Error body (truncated if long):" << d->rawResponse.left(500); if (jsonBody) - { - auto json = QJsonDocument::fromJson(d->rawResponse).object(); - const auto errCode = json.value("errcode"_ls).toString(); - if (error() == TooManyRequestsError || - errCode == "M_LIMIT_EXCEEDED") - { - QString msg = tr("Too many requests"); - auto retryInterval = json.value("retry_after_ms"_ls).toInt(-1); - if (retryInterval != -1) - msg += tr(", next retry advised after %1 ms") - .arg(retryInterval); - else // We still have to figure some reasonable interval - retryInterval = getNextRetryInterval(); - - setStatus(TooManyRequestsError, msg); - - // Shortcut to retry instead of executing finishJob() - stop(); - qCWarning(d->logCat) - << this << "will retry in" << retryInterval << "ms"; - d->retryTimer.start(retryInterval); - emit retryScheduled(d->retriesTaken, retryInterval); - return; - } - if (errCode == "M_CONSENT_NOT_GIVEN") - { - d->status.code = UserConsentRequiredError; - d->errorUrl = json.value("consent_uri"_ls).toString(); - } - else if (errCode == "M_UNKNOWN") { - d->status.code = UnknownObjectError; - } - else if (errCode == "M_UNSUPPORTED_ROOM_VERSION" || - errCode == "M_INCOMPATIBLE_ROOM_VERSION") - { - d->status.code = UnsupportedRoomVersionError; - if (json.contains("room_version")) - d->status.message = - tr("Requested room version: %1") - .arg(json.value("room_version").toString()); - } else if (!json.isEmpty()) // Not localisable on the client side - setStatus(d->status.code, json.value("error"_ls).toString()); - } + setStatus( + parseError(d->reply.data(), + QJsonDocument::fromJson(d->rawResponse).object())); } - finishJob(); + if (error() != TooManyRequestsError) + finishJob(); + else { + stop(); + emit retryScheduled(d->retriesTaken, d->retryTimer.interval()); + } } bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) @@ -445,8 +409,46 @@ BaseJob::Status BaseJob::parseJson(const QJsonDocument&) return Success; } +BaseJob::Status BaseJob::parseError(QNetworkReply* reply, + const QJsonObject& errorJson) +{ + const auto errCode = errorJson.value("errcode"_ls).toString(); + if (error() == TooManyRequestsError || errCode == "M_LIMIT_EXCEEDED") { + QString msg = tr("Too many requests"); + auto retryInterval = errorJson.value("retry_after_ms"_ls).toInt(-1); + if (retryInterval != -1) + msg += tr(", next retry advised after %1 ms").arg(retryInterval); + else // We still have to figure some reasonable interval + retryInterval = getNextRetryInterval(); + + qCWarning(d->logCat) << this << "will retry in" << retryInterval << "ms"; + d->retryTimer.start(retryInterval); + + return { TooManyRequestsError, msg }; + } + if (errCode == "M_CONSENT_NOT_GIVEN") { + d->errorUrl = errorJson.value("consent_uri"_ls).toString(); + return { UserConsentRequiredError }; + } + if (errCode == "M_UNSUPPORTED_ROOM_VERSION" + || errCode == "M_INCOMPATIBLE_ROOM_VERSION") + return { UnsupportedRoomVersionError, + errorJson.contains("room_version"_ls) + ? tr("Requested room version: %1") + .arg(errorJson.value("room_version"_ls).toString()) + : errorJson.value("error"_ls).toString() }; + + // Not localisable on the client side + if (errorJson.contains("error"_ls)) + d->status.message = errorJson.value("error"_ls).toString(); + + return d->status; +} + void BaseJob::stop() { + // This method is used to semi-finalise the job before retrying; so + // stop the timeout timer but keep the retry timer running. d->timer.stop(); if (d->reply) { diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index c1747cca..30ecceb3 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -67,7 +67,6 @@ namespace QMatrixClient , UnsupportedRoomVersionError , NetworkAuthRequiredError , UserConsentRequiredError - , UnknownObjectError // Unknown room or other item (M_UNKNOWN) , UserDefinedError = 200 }; @@ -303,7 +302,7 @@ namespace QMatrixClient * Processes the reply. By default, parses the reply into * a QJsonDocument and calls parseJson() if it's a valid JSON. * - * @param reply raw contents of a HTTP reply from the server (without headers) + * @param reply raw contents of a HTTP reply from the server * * @see gotReply, parseJson */ @@ -311,7 +310,7 @@ namespace QMatrixClient /** * Processes the JSON document received from the Matrix server. - * By default returns succesful status without analysing the JSON. + * By default returns successful status without analysing the JSON. * * @param json valid JSON document received from the server * @@ -319,6 +318,15 @@ namespace QMatrixClient */ virtual Status parseJson(const QJsonDocument&); + /** + * Processes the reply in case of unsuccessful HTTP code. + * The body is already loaded from the reply object to errorJson. + * @param reply the HTTP reply from the server + * @param errorJson the JSON payload describing the error + */ + virtual Status parseError(QNetworkReply* reply, + const QJsonObject& errorJson); + void setStatus(Status s); void setStatus(int code, QString message); -- cgit v1.2.3 From 74caea2669b8f76ca76507bc40321fdcd23dc522 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 12 Jul 2019 18:02:27 +0900 Subject: Minor polish --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 6ad8d971..b3cfc527 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -300,7 +300,7 @@ void BaseJob::gotReply() QJsonDocument::fromJson(d->rawResponse).object())); } - if (error() != TooManyRequestsError) + if (status().code != TooManyRequestsError) finishJob(); else { stop(); -- cgit v1.2.3 From c05ade838f0fce81f2bbe80a3295618a8a26ff52 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 2 Aug 2019 19:59:40 +0900 Subject: Apply the new brace wrapping to source files --- lib/jobs/basejob.cpp | 6 ++---- lib/jobs/basejob.h | 35 +++++++++-------------------------- lib/jobs/downloadfilejob.cpp | 7 ++----- lib/jobs/downloadfilejob.h | 11 +++-------- lib/jobs/mediathumbnailjob.h | 6 ++---- lib/jobs/postreadmarkersjob.h | 3 +-- lib/jobs/requestdata.cpp | 12 +++--------- lib/jobs/requestdata.h | 9 +++------ lib/jobs/syncjob.h | 6 ++---- 9 files changed, 27 insertions(+), 68 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index e4a74954..a6471ece 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -32,8 +32,7 @@ using namespace QMatrixClient; -struct NetworkReplyDeleter : public QScopedPointerDeleteLater -{ +struct NetworkReplyDeleter : public QScopedPointerDeleteLater { static inline void cleanup(QNetworkReply* reply) { if (reply && reply->isRunning()) @@ -42,8 +41,7 @@ struct NetworkReplyDeleter : public QScopedPointerDeleteLater } }; -class BaseJob::Private -{ +class BaseJob::Private { public: // Using an idiom from clang-tidy: // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index f445d087..c85d2d90 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -28,32 +28,22 @@ class QNetworkReply; class QSslError; -namespace QMatrixClient -{ +namespace QMatrixClient { class ConnectionData; -enum class HttpVerb -{ - Get, - Put, - Post, - Delete -}; +enum class HttpVerb { Get, Put, Post, Delete }; -struct JobTimeoutConfig -{ +struct JobTimeoutConfig { int jobTimeout; int nextRetryInterval; }; -class BaseJob : public QObject -{ +class BaseJob : public QObject { Q_OBJECT Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) Q_PROPERTY(int maxRetries READ maxRetries WRITE setMaxRetries) public: - enum StatusCode - { + enum StatusCode { NoError = 0 // To be compatible with Qt conventions , Success = 0, @@ -93,8 +83,7 @@ public: * A simple wrapper around QUrlQuery that allows its creation from * a list of string pairs */ - class Query : public QUrlQuery - { + class Query : public QUrlQuery { public: using QUrlQuery::QUrlQuery; Query() = default; @@ -115,16 +104,10 @@ public: * along the lines of StatusCode, with additional values * starting at UserDefinedError */ - class Status - { + class Status { public: - Status(StatusCode c) - : code(c) - {} - Status(int c, QString m) - : code(c) - , message(std::move(m)) - {} + Status(StatusCode c) : code(c) {} + Status(int c, QString m) : code(c), message(std::move(m)) {} bool good() const { return code < ErrorLevel; } friend QDebug operator<<(QDebug dbg, const Status& s) diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 3dff5a68..9722186c 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -6,12 +6,9 @@ using namespace QMatrixClient; -class DownloadFileJob::Private -{ +class DownloadFileJob::Private { public: - Private() - : tempFile(new QTemporaryFile()) - {} + Private() : tempFile(new QTemporaryFile()) {} explicit Private(const QString& localFilename) : targetFile(new QFile(localFilename)) diff --git a/lib/jobs/downloadfilejob.h b/lib/jobs/downloadfilejob.h index 58858448..ebfe5a0d 100644 --- a/lib/jobs/downloadfilejob.h +++ b/lib/jobs/downloadfilejob.h @@ -2,15 +2,10 @@ #include "csapi/content-repo.h" -namespace QMatrixClient -{ -class DownloadFileJob : public GetContentJob -{ +namespace QMatrixClient { +class DownloadFileJob : public GetContentJob { public: - enum - { - FileError = BaseJob::UserDefinedError + 1 - }; + enum { FileError = BaseJob::UserDefinedError + 1 }; using GetContentJob::makeRequestUrl; static QUrl makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri); diff --git a/lib/jobs/mediathumbnailjob.h b/lib/jobs/mediathumbnailjob.h index eeabe7a9..df0a7f31 100644 --- a/lib/jobs/mediathumbnailjob.h +++ b/lib/jobs/mediathumbnailjob.h @@ -22,10 +22,8 @@ #include -namespace QMatrixClient -{ -class MediaThumbnailJob : public GetContentThumbnailJob -{ +namespace QMatrixClient { +class MediaThumbnailJob : public GetContentThumbnailJob { public: using GetContentThumbnailJob::makeRequestUrl; static QUrl makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri, diff --git a/lib/jobs/postreadmarkersjob.h b/lib/jobs/postreadmarkersjob.h index d53ae66c..cf482a9a 100644 --- a/lib/jobs/postreadmarkersjob.h +++ b/lib/jobs/postreadmarkersjob.h @@ -24,8 +24,7 @@ using namespace QMatrixClient; -class PostReadMarkersJob : public BaseJob -{ +class PostReadMarkersJob : public BaseJob { public: explicit PostReadMarkersJob(const QString& roomId, const QString& readUpToEventId) diff --git a/lib/jobs/requestdata.cpp b/lib/jobs/requestdata.cpp index 8248d6b1..6ad7c007 100644 --- a/lib/jobs/requestdata.cpp +++ b/lib/jobs/requestdata.cpp @@ -23,16 +23,10 @@ inline auto fromJson(const JsonDataT& jdata) return fromData(QJsonDocument(jdata).toJson(QJsonDocument::Compact)); } -RequestData::RequestData(const QByteArray& a) - : _source(fromData(a)) -{} +RequestData::RequestData(const QByteArray& a) : _source(fromData(a)) {} -RequestData::RequestData(const QJsonObject& jo) - : _source(fromJson(jo)) -{} +RequestData::RequestData(const QJsonObject& jo) : _source(fromJson(jo)) {} -RequestData::RequestData(const QJsonArray& ja) - : _source(fromJson(ja)) -{} +RequestData::RequestData(const QJsonArray& ja) : _source(fromJson(ja)) {} RequestData::~RequestData() = default; diff --git a/lib/jobs/requestdata.h b/lib/jobs/requestdata.h index 974a9ddf..55987a3b 100644 --- a/lib/jobs/requestdata.h +++ b/lib/jobs/requestdata.h @@ -26,23 +26,20 @@ class QJsonArray; class QJsonDocument; class QIODevice; -namespace QMatrixClient -{ +namespace QMatrixClient { /** * A simple wrapper that represents the request body. * Provides a unified interface to dump an unstructured byte stream * as well as JSON (and possibly other structures in the future) to * a QByteArray consumed by QNetworkAccessManager request methods. */ -class RequestData -{ +class RequestData { public: RequestData() = default; RequestData(const QByteArray& a); RequestData(const QJsonObject& jo); RequestData(const QJsonArray& ja); - RequestData(QIODevice* source) - : _source(std::unique_ptr(source)) + RequestData(QIODevice* source) : _source(std::unique_ptr(source)) {} RequestData(const RequestData&) = delete; RequestData& operator=(const RequestData&) = delete; diff --git a/lib/jobs/syncjob.h b/lib/jobs/syncjob.h index e2cec8f7..8f925414 100644 --- a/lib/jobs/syncjob.h +++ b/lib/jobs/syncjob.h @@ -22,10 +22,8 @@ #include "../syncdata.h" #include "basejob.h" -namespace QMatrixClient -{ -class SyncJob : public BaseJob -{ +namespace QMatrixClient { +class SyncJob : public BaseJob { public: explicit SyncJob(const QString& since = {}, const QString& filter = {}, int timeout = -1, const QString& presence = {}); -- cgit v1.2.3 From 7a5b359b8823646ce97cbaf05c251cb04c291466 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 7 Jul 2019 18:16:30 +0900 Subject: Rename zero-impact strings --- lib/jobs/basejob.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index a6471ece..5615736e 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -321,7 +321,7 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const { // QNetworkReply error codes seem to be flawed when it comes to HTTP; - // see, e.g., https://github.com/QMatrixClient/libqmatrixclient/issues/200 + // see, e.g., https://github.com/quotient-im/libQuotient/issues/200 // so check genuine HTTP codes. The below processing is based on // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes const auto httpCodeHeader = @@ -576,7 +576,7 @@ QUrl BaseJob::errorUrl() const { return d->errorUrl; } void BaseJob::setStatus(Status s) { // The crash that led to this code has been reported in - // https://github.com/QMatrixClient/Quaternion/issues/566 - basically, + // https://github.com/quotient-im/Quaternion/issues/566 - basically, // when cleaning up childrent of a deleted Connection, there's a chance // of pending jobs being abandoned, calling setStatus(Abandoned). // There's nothing wrong with this; however, the safety check for -- cgit v1.2.3 From 27ca32a1e5a56e09b9cc1d94224d2831004dcf3d Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 7 Jul 2019 19:32:34 +0900 Subject: Namespace: QMatrixClient -> Quotient (with back comp alias) --- lib/jobs/basejob.cpp | 2 +- lib/jobs/basejob.h | 4 ++-- lib/jobs/downloadfilejob.cpp | 2 +- lib/jobs/downloadfilejob.h | 4 ++-- lib/jobs/mediathumbnailjob.cpp | 2 +- lib/jobs/mediathumbnailjob.h | 4 ++-- lib/jobs/postreadmarkersjob.h | 2 +- lib/jobs/requestdata.cpp | 2 +- lib/jobs/requestdata.h | 6 ++++-- lib/jobs/syncjob.cpp | 2 +- lib/jobs/syncjob.h | 4 ++-- 11 files changed, 18 insertions(+), 16 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 5615736e..ec6b8375 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -30,7 +30,7 @@ #include -using namespace QMatrixClient; +using namespace Quotient; struct NetworkReplyDeleter : public QScopedPointerDeleteLater { static inline void cleanup(QNetworkReply* reply) diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index c85d2d90..90c20c37 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -28,7 +28,7 @@ class QNetworkReply; class QSslError; -namespace QMatrixClient { +namespace Quotient { class ConnectionData; enum class HttpVerb { Get, Put, Post, Delete }; @@ -364,4 +364,4 @@ inline bool isJobRunning(BaseJob* job) { return job && job->error() == BaseJob::Pending; } -} // namespace QMatrixClient +} // namespace Quotient diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 9722186c..3a03efde 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -4,7 +4,7 @@ #include #include -using namespace QMatrixClient; +using namespace Quotient; class DownloadFileJob::Private { public: diff --git a/lib/jobs/downloadfilejob.h b/lib/jobs/downloadfilejob.h index ebfe5a0d..fa697219 100644 --- a/lib/jobs/downloadfilejob.h +++ b/lib/jobs/downloadfilejob.h @@ -2,7 +2,7 @@ #include "csapi/content-repo.h" -namespace QMatrixClient { +namespace Quotient { class DownloadFileJob : public GetContentJob { public: enum { FileError = BaseJob::UserDefinedError + 1 }; @@ -24,4 +24,4 @@ private: void beforeAbandon(QNetworkReply*) override; Status parseReply(QNetworkReply*) override; }; -} // namespace QMatrixClient +} // namespace Quotient diff --git a/lib/jobs/mediathumbnailjob.cpp b/lib/jobs/mediathumbnailjob.cpp index db2bbc13..0a346392 100644 --- a/lib/jobs/mediathumbnailjob.cpp +++ b/lib/jobs/mediathumbnailjob.cpp @@ -18,7 +18,7 @@ #include "mediathumbnailjob.h" -using namespace QMatrixClient; +using namespace Quotient; QUrl MediaThumbnailJob::makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri, QSize requestedSize) diff --git a/lib/jobs/mediathumbnailjob.h b/lib/jobs/mediathumbnailjob.h index df0a7f31..75e2e55a 100644 --- a/lib/jobs/mediathumbnailjob.h +++ b/lib/jobs/mediathumbnailjob.h @@ -22,7 +22,7 @@ #include -namespace QMatrixClient { +namespace Quotient { class MediaThumbnailJob : public GetContentThumbnailJob { public: using GetContentThumbnailJob::makeRequestUrl; @@ -42,4 +42,4 @@ protected: private: QImage _thumbnail; }; -} // namespace QMatrixClient +} // namespace Quotient diff --git a/lib/jobs/postreadmarkersjob.h b/lib/jobs/postreadmarkersjob.h index cf482a9a..5a4d942c 100644 --- a/lib/jobs/postreadmarkersjob.h +++ b/lib/jobs/postreadmarkersjob.h @@ -22,7 +22,7 @@ #include -using namespace QMatrixClient; +using namespace Quotient; class PostReadMarkersJob : public BaseJob { public: diff --git a/lib/jobs/requestdata.cpp b/lib/jobs/requestdata.cpp index 6ad7c007..0c70f085 100644 --- a/lib/jobs/requestdata.cpp +++ b/lib/jobs/requestdata.cpp @@ -6,7 +6,7 @@ #include #include -using namespace QMatrixClient; +using namespace Quotient; auto fromData(const QByteArray& data) { diff --git a/lib/jobs/requestdata.h b/lib/jobs/requestdata.h index 55987a3b..020d5ef2 100644 --- a/lib/jobs/requestdata.h +++ b/lib/jobs/requestdata.h @@ -26,7 +26,7 @@ class QJsonArray; class QJsonDocument; class QIODevice; -namespace QMatrixClient { +namespace Quotient { /** * A simple wrapper that represents the request body. * Provides a unified interface to dump an unstructured byte stream @@ -52,4 +52,6 @@ public: private: std::unique_ptr _source; }; -} // namespace QMatrixClient +} // namespace Quotient +/// \deprecated Use namespace Quotient instead +namespace QMatrixClient = Quotient; diff --git a/lib/jobs/syncjob.cpp b/lib/jobs/syncjob.cpp index f660e1b6..cd7709e1 100644 --- a/lib/jobs/syncjob.cpp +++ b/lib/jobs/syncjob.cpp @@ -18,7 +18,7 @@ #include "syncjob.h" -using namespace QMatrixClient; +using namespace Quotient; static size_t jobId = 0; diff --git a/lib/jobs/syncjob.h b/lib/jobs/syncjob.h index 8f925414..df419ba8 100644 --- a/lib/jobs/syncjob.h +++ b/lib/jobs/syncjob.h @@ -22,7 +22,7 @@ #include "../syncdata.h" #include "basejob.h" -namespace QMatrixClient { +namespace Quotient { class SyncJob : public BaseJob { public: explicit SyncJob(const QString& since = {}, const QString& filter = {}, @@ -38,4 +38,4 @@ protected: private: SyncData d; }; -} // namespace QMatrixClient +} // namespace Quotient -- cgit v1.2.3 From fadce11be92abe76cecfe6356b3b38f25dd93e8d Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 13 Aug 2019 20:28:56 +0900 Subject: Support for server notices rooms (MSC1452) Closes #326. --- lib/jobs/basejob.cpp | 3 +++ lib/jobs/basejob.h | 1 + 2 files changed, 4 insertions(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index ec6b8375..5930e8b8 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -426,6 +426,9 @@ BaseJob::Status BaseJob::parseError(QNetworkReply* reply, ? tr("Requested room version: %1") .arg(errorJson.value("room_version"_ls).toString()) : errorJson.value("error"_ls).toString() }; + if (errCode == "M_CANNOT_LEAVE_SERVER_NOTICE_ROOM") + return { CannotLeaveRoom, + tr("It's not allowed to leave a server notices room") }; // Not localisable on the client side if (errorJson.contains("error"_ls)) diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 90c20c37..68467d48 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -76,6 +76,7 @@ public: NetworkAuthRequiredError = NetworkAuthRequired, UserConsentRequired, UserConsentRequiredError = UserConsentRequired, + CannotLeaveRoom, UserDefinedError = 256 }; -- cgit v1.2.3 From 8663c2e78407a0c0df872eaf9bb6b41de2fbdc9e Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 16 Aug 2019 13:40:09 +0900 Subject: BaseJob: support M_USER_DEACTIVATED error code Closes #344. --- lib/jobs/basejob.cpp | 2 ++ lib/jobs/basejob.h | 1 + 2 files changed, 3 insertions(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 5930e8b8..f3ba00b5 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -429,6 +429,8 @@ BaseJob::Status BaseJob::parseError(QNetworkReply* reply, if (errCode == "M_CANNOT_LEAVE_SERVER_NOTICE_ROOM") return { CannotLeaveRoom, tr("It's not allowed to leave a server notices room") }; + if (errCode == "M_USER_DEACTIVATED") + return { UserDeactivated }; // Not localisable on the client side if (errorJson.contains("error"_ls)) diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 68467d48..fd7beca0 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -77,6 +77,7 @@ public: UserConsentRequired, UserConsentRequiredError = UserConsentRequired, CannotLeaveRoom, + UserDeactivated, UserDefinedError = 256 }; -- cgit v1.2.3 From ad159b5206de615762e22f95e97ae61400f11761 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 20 Aug 2019 19:56:54 +0900 Subject: BaseJob: Status/StatusCode tweaks, cleanup, mo' comments Notably, recovered Status::fromHttpCode() that was introduced in 5722ceaf4bd10c29f1091e3dc5a87f5650ea8c71 but fell victim of a careless merge (so much for introducing non-topical changes in feature branches). --- lib/jobs/basejob.cpp | 135 ++++++++++++++++++++++++++++----------------------- lib/jobs/basejob.h | 37 ++++++++------ 2 files changed, 97 insertions(+), 75 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index f3ba00b5..621762be 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -77,6 +77,8 @@ public: QByteArray rawResponse; QUrl errorUrl; //< May contain a URL to help with some errors + LoggingCategory logCat = JOBS; + QTimer timer; QTimer retryTimer; @@ -86,7 +88,12 @@ public: int maxRetries = errorStrategy.size(); int retriesTaken = 0; - LoggingCategory logCat = JOBS; + QString urlForLog() const + { + return reply + ? reply->url().toString(QUrl::RemoveQuery) + : makeRequestUrl(connection->baseUrl(), apiEndpoint).toString(); + } }; BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, @@ -112,7 +119,7 @@ BaseJob::~BaseJob() QUrl BaseJob::requestUrl() const { - return d->reply ? d->reply->request().url() : QUrl(); + return d->reply ? d->reply->url() : QUrl(); } bool BaseJob::isBackground() const @@ -193,17 +200,13 @@ void BaseJob::Private::sendRequest(bool inBackground) req.setRawHeader("Authorization", QByteArray("Bearer ") + connection->accessToken()); req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); -#if (QT_VERSION >= QT_VERSION_CHECK(5, 6, 0)) req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); -#endif req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); -#if QT_VERSION >= QT_VERSION_CHECK(5, 9, 0) - // some sources claim that there are issues with QT 5.8 req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true); -#endif for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) req.setRawHeader(it.key(), it.value()); + switch (verb) { case HttpVerb::Get: reply.reset(connection->nam()->get(req)); @@ -318,6 +321,47 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) return false; } +BaseJob::Status BaseJob::Status::fromHttpCode(int httpCode, QString msg) +{ + // clang-format off + return { [httpCode]() -> StatusCode { + if (httpCode / 10 == 41) // 41x errors + return httpCode == 410 ? IncorrectRequestError : NotFoundError; + switch (httpCode) { + case 401: case 403: case 407: + return ContentAccessError; + case 404: + return NotFoundError; + case 400: case 405: case 406: case 426: case 428: case 505: + case 494: // Unofficial nginx "Request header too large" + case 497: // Unofficial nginx "HTTP request sent to HTTPS port" + return IncorrectRequestError; + case 429: + return TooManyRequestsError; + case 501: case 510: + return RequestNotImplementedError; + case 511: + return NetworkAuthRequiredError; + default: + return NetworkError; + } + }(), std::move(msg) }; + // clang-format on +} + +QDebug BaseJob::Status::dumpToLog(QDebug dbg) const +{ + QDebugStateSaver _s(dbg); + dbg.noquote().nospace(); + if (auto* const k = QMetaEnum::fromType().valueToKey(code)) { + const QByteArray b = k; + dbg << b.mid(b.lastIndexOf(':')); + } else + dbg << code; + return dbg << ": " << message; + +} + BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const { // QNetworkReply error codes seem to be flawed when it comes to HTTP; @@ -327,62 +371,30 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const const auto httpCodeHeader = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); if (!httpCodeHeader.isValid()) { - qCWarning(d->logCat) << this << "didn't get valid HTTP headers"; + qCWarning(d->logCat) << "No valid HTTP headers from" << d->urlForLog(); return { NetworkError, reply->errorString() }; } - const QString replyState = reply->isRunning() - ? QStringLiteral("(tentative)") - : QStringLiteral("(final)"); - const auto urlString = '|' + d->reply->url().toDisplayString(); const auto httpCode = httpCodeHeader.toInt(); - const auto reason = - reply->attribute(QNetworkRequest::HttpReasonPhraseAttribute).toString(); if (httpCode / 100 == 2) // 2xx { - qCDebug(d->logCat).noquote().nospace() << this << urlString; - qCDebug(d->logCat).noquote() << " " << httpCode << reason << replyState; + if (reply->isFinished()) + qCInfo(d->logCat) << httpCode << "<-" << d->urlForLog(); if (!checkContentType(reply->rawHeader("Content-Type"), d->expectedContentTypes)) return { UnexpectedResponseTypeWarning, "Unexpected content type of the response" }; return NoError; } + if (reply->isFinished()) + qCWarning(d->logCat) << httpCode << "<-" << d->urlForLog(); - qCWarning(d->logCat).noquote().nospace() << this << urlString; - qCWarning(d->logCat).noquote() << " " << httpCode << reason << replyState; - return { [httpCode]() -> StatusCode { - if (httpCode / 10 == 41) - return httpCode == 410 ? IncorrectRequestError - : NotFoundError; - switch (httpCode) { - case 401: - case 403: - case 407: - return ContentAccessError; - case 404: - return NotFoundError; - case 400: - case 405: - case 406: - case 426: - case 428: - case 505: - case 494: // Unofficial nginx "Request header too large" - case 497: // Unofficial nginx "HTTP request sent to HTTPS port" - return IncorrectRequestError; - case 429: - return TooManyRequestsError; - case 501: - case 510: - return RequestNotImplementedError; - case 511: - return NetworkAuthRequiredError; - default: - return NetworkError; - } - }(), - reply->errorString() }; + auto message = reply->errorString(); + if (message.isEmpty()) + message = reply->attribute(QNetworkRequest::HttpReasonPhraseAttribute) + .toString(); + + return Status::fromHttpCode(httpCode, message); } BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) @@ -398,7 +410,7 @@ BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) BaseJob::Status BaseJob::parseJson(const QJsonDocument&) { return Success; } -BaseJob::Status BaseJob::parseError(QNetworkReply* reply, +BaseJob::Status BaseJob::parseError(QNetworkReply* /*reply*/, const QJsonObject& errorJson) { const auto errCode = errorJson.value("errcode"_ls).toString(); @@ -441,7 +453,7 @@ BaseJob::Status BaseJob::parseError(QNetworkReply* reply, void BaseJob::stop() { - // This method is used to semi-finalise the job before retrying; so + // This method is (also) used to semi-finalise the job before retrying; so // stop the timeout timer but keep the retry timer running. d->timer.stop(); if (d->reply) { @@ -449,7 +461,7 @@ void BaseJob::stop() if (d->reply->isRunning()) { qCWarning(d->logCat) << this << "stopped without ready network reply"; - d->reply->abort(); + d->reply->abort(); // Keep the reply object in case clients need it } } else qCWarning(d->logCat) << this << "stopped with empty network reply"; @@ -474,10 +486,10 @@ void BaseJob::finishJob() return; } - // Notify those interested in any completion of the job (including killing) + // Notify those interested in any completion of the job including abandon() emit finished(this); - emit result(this); + emit result(this); // abandon() doesn't emit this if (error()) emit failure(this); else @@ -582,21 +594,22 @@ void BaseJob::setStatus(Status s) { // The crash that led to this code has been reported in // https://github.com/quotient-im/Quaternion/issues/566 - basically, - // when cleaning up childrent of a deleted Connection, there's a chance + // when cleaning up children of a deleted Connection, there's a chance // of pending jobs being abandoned, calling setStatus(Abandoned). // There's nothing wrong with this; however, the safety check for // cleartext access tokens below uses d->connection - which is a dangling // pointer. // To alleviate that, a stricter condition is applied, that for Abandoned // and to-be-Abandoned jobs the status message will be disregarded entirely. - // For 0.6 we might rectify the situation by making d->connection - // a QPointer<> (and derive ConnectionData from QObject, respectively). - if (d->status.code == Abandoned || s.code == Abandoned) - s.message.clear(); - + // We could rectify the situation by making d->connection a QPointer<> + // (and deriving ConnectionData from QObject, respectively) but it's + // a too edge case for the hassle. if (d->status == s) return; + if (d->status.code == Abandoned || s.code == Abandoned) + s.message.clear(); + if (!s.message.isEmpty() && d->connection && !d->connection->accessToken().isEmpty()) s.message.replace(d->connection->accessToken(), "(REDACTED)"); diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index fd7beca0..9de7b49d 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -24,6 +24,7 @@ #include #include #include +#include class QNetworkReply; class QSslError; @@ -43,18 +44,23 @@ class BaseJob : public QObject { Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) Q_PROPERTY(int maxRetries READ maxRetries WRITE setMaxRetries) public: + /*! The status code of a job + * + * Every job is created in Unprepared status; upon calling prepare() + * from Connection (if things are fine) it go to Pending status. After + * that, the next transition comes after the reply arrives and its contents + * are analysed. At any point in time the job can be abandon()ed, causing + * it to switch to status Abandoned for a brief period before deletion. + */ enum StatusCode { - NoError = 0 // To be compatible with Qt conventions - , Success = 0, + NoError = Success, // To be compatible with Qt conventions Pending = 1, - WarningLevel = 20, + WarningLevel = 20, //< Warnings have codes starting from this UnexpectedResponseType = 21, UnexpectedResponseTypeWarning = UnexpectedResponseType, - Abandoned = 50 //< A tiny period between abandoning and object deletion - , - ErrorLevel = 100 //< Errors have codes starting from this - , + Abandoned = 50, //< A tiny period between abandoning and object deletion + ErrorLevel = 100, //< Errors have codes starting from this NetworkError = 100, Timeout, TimeoutError = Timeout, @@ -64,10 +70,12 @@ public: IncorrectRequestError = IncorrectRequest, IncorrectResponse, IncorrectResponseError = IncorrectResponse, - JsonParseError //< deprecated; Use IncorrectResponse instead + JsonParseError + Q_DECL_ENUMERATOR_DEPRECATED_X("Use IncorrectResponse instead") = IncorrectResponse, TooManyRequests, TooManyRequestsError = TooManyRequests, + RateLimited = TooManyRequests, RequestNotImplemented, RequestNotImplementedError = RequestNotImplemented, UnsupportedRoomVersion, @@ -80,6 +88,7 @@ public: UserDeactivated, UserDefinedError = 256 }; + Q_ENUM(StatusCode) /** * A simple wrapper around QUrlQuery that allows its creation from @@ -97,7 +106,7 @@ public: using Data = RequestData; - /** + /*! * This structure stores the status of a server call job. The status * consists of a code, that is described (but not delimited) by the * respective enum, and a freeform message. @@ -106,16 +115,16 @@ public: * along the lines of StatusCode, with additional values * starting at UserDefinedError */ - class Status { - public: + struct Status { Status(StatusCode c) : code(c) {} Status(int c, QString m) : code(c), message(std::move(m)) {} + static Status fromHttpCode(int httpCode, QString msg = {}); bool good() const { return code < ErrorLevel; } - friend QDebug operator<<(QDebug dbg, const Status& s) + QDebug dumpToLog(QDebug dbg) const; + friend QDebug operator<<(const QDebug& dbg, const Status& s) { - QDebugStateSaver _s(dbg); - return dbg.noquote().nospace() << s.code << ": " << s.message; + return s.dumpToLog(dbg); } bool operator==(const Status& other) const -- cgit v1.2.3 From 59c4996a602e9eeae4e3bfc0210ff15f957df38f Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 20 Aug 2019 20:09:09 +0900 Subject: BaseJob/ConnectionData: connection-wide rate-limiting As before, completely transparent for clients, driven by 529 errors from the server (but cases of rate limiting are signalled by BaseJob::rateLimited). That brings changes to BaseJob API: timeouts now use int64_t and also can be handled in std::chrono terms; aboutToStart() -> aboutToSendRequest(); started() -> sentRequest(). Closes #292. --- lib/jobs/basejob.cpp | 159 ++++++++++++++++++++++++------------------- lib/jobs/basejob.h | 63 +++++++++++------ lib/jobs/downloadfilejob.cpp | 4 +- lib/jobs/downloadfilejob.h | 4 +- 4 files changed, 137 insertions(+), 93 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 621762be..0a17431c 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -31,6 +31,8 @@ #include using namespace Quotient; +using std::chrono::seconds, std::chrono::milliseconds; +using std::chrono_literals::operator""s; struct NetworkReplyDeleter : public QScopedPointerDeleteLater { static inline void cleanup(QNetworkReply* reply) @@ -43,6 +45,11 @@ struct NetworkReplyDeleter : public QScopedPointerDeleteLater { class BaseJob::Private { public: + struct JobTimeoutConfig { + seconds jobTimeout; + seconds nextRetryInterval; + }; + // Using an idiom from clang-tidy: // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html Private(HttpVerb v, QString endpoint, const QUrlQuery& q, Data&& data, @@ -52,12 +59,14 @@ public: , requestQuery(q) , requestData(std::move(data)) , needsToken(nt) - {} + { + timer.setSingleShot(true); + retryTimer.setSingleShot(true); + } - void sendRequest(bool inBackground); - const JobTimeoutConfig& getCurrentTimeoutConfig() const; + void sendRequest(); - const ConnectionData* connection = nullptr; + ConnectionData* connection = nullptr; // Contents for the network request HttpVerb verb; @@ -67,13 +76,15 @@ public: Data requestData; bool needsToken; + bool inBackground = false; + // There's no use of QMimeType here because we don't want to match // content types against the known MIME type hierarchy; and at the same // type QMimeType is of little help with MIME type globs (`text/*` etc.) - QByteArrayList expectedContentTypes; + QByteArrayList expectedContentTypes { "application/json" }; QScopedPointer reply; - Status status = Pending; + Status status = Unprepared; QByteArray rawResponse; QUrl errorUrl; //< May contain a URL to help with some errors @@ -82,12 +93,18 @@ public: QTimer timer; QTimer retryTimer; - QVector errorStrategy = { { 90, 5 }, - { 90, 10 }, - { 120, 30 } }; - int maxRetries = errorStrategy.size(); + static constexpr std::array errorStrategy { + { { 90s, 5s }, { 90s, 10s }, { 120s, 30s } } + }; + int maxRetries = int(errorStrategy.size()); int retriesTaken = 0; + const JobTimeoutConfig& getCurrentTimeoutConfig() const + { + return errorStrategy[std::min(size_t(retriesTaken), + errorStrategy.size() - 1)]; + } + QString urlForLog() const { return reply @@ -106,9 +123,11 @@ BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, : d(new Private(verb, endpoint, query, std::move(data), needsToken)) { setObjectName(name); - setExpectedContentTypes({ "application/json" }); - d->timer.setSingleShot(true); connect(&d->timer, &QTimer::timeout, this, &BaseJob::timeout); + connect(&d->retryTimer, &QTimer::timeout, this, [this] { + setStatus(Pending); + sendRequest(); + }); } BaseJob::~BaseJob() @@ -124,10 +143,7 @@ QUrl BaseJob::requestUrl() const bool BaseJob::isBackground() const { - return d->reply - && d->reply->request() - .attribute(QNetworkRequest::BackgroundRequestAttribute) - .toBool(); + return d->inBackground; } const QString& BaseJob::apiEndpoint() const { return d->apiEndpoint; } @@ -191,7 +207,7 @@ QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QString& path, return baseUrl; } -void BaseJob::Private::sendRequest(bool inBackground) +void BaseJob::Private::sendRequest() { QNetworkRequest req { makeRequestUrl(connection->baseUrl(), apiEndpoint, requestQuery) }; @@ -223,36 +239,31 @@ void BaseJob::Private::sendRequest(bool inBackground) } } -void BaseJob::beforeStart(const ConnectionData*) {} +void BaseJob::doPrepare() {} -void BaseJob::afterStart(const ConnectionData*, QNetworkReply*) {} +void BaseJob::onSentRequest(QNetworkReply*) {} void BaseJob::beforeAbandon(QNetworkReply*) {} -void BaseJob::start(const ConnectionData* connData, bool inBackground) +void BaseJob::prepare(ConnectionData* connData, bool inBackground) { + d->inBackground = inBackground; d->connection = connData; - d->retryTimer.setSingleShot(true); - connect(&d->retryTimer, &QTimer::timeout, this, - [this, inBackground] { sendRequest(inBackground); }); - - beforeStart(connData); - if (status().good()) - sendRequest(inBackground); - if (status().good()) - afterStart(connData, d->reply.data()); - if (!status().good()) + doPrepare(); + if (status().code != Unprepared && status().code != Pending) QTimer::singleShot(0, this, &BaseJob::finishJob); + setStatus(Pending); } -void BaseJob::sendRequest(bool inBackground) +void BaseJob::sendRequest() { - emit aboutToStart(); - d->retryTimer.stop(); // In case we were counting down at the moment - qCDebug(d->logCat) << this << "sending request to" << d->apiEndpoint; - if (!d->requestQuery.isEmpty()) - qCDebug(d->logCat) << " query:" << d->requestQuery.toString(); - d->sendRequest(inBackground); + if (status().code == Abandoned) + return; + Q_ASSERT(d->connection && status().code == Pending); + qCDebug(d->logCat) << "Making request to" << d->urlForLog(); + emit aboutToSendRequest(); + d->sendRequest(); + Q_ASSERT(d->reply); connect(d->reply.data(), &QNetworkReply::finished, this, &BaseJob::gotReply); if (d->reply->isRunning()) { connect(d->reply.data(), &QNetworkReply::metaDataChanged, this, @@ -262,10 +273,12 @@ void BaseJob::sendRequest(bool inBackground) connect(d->reply.data(), &QNetworkReply::downloadProgress, this, &BaseJob::downloadProgress); d->timer.start(getCurrentTimeout()); - qCDebug(d->logCat) << this << "request has been sent"; - emit started(); + qCInfo(d->logCat).noquote() << "Request sent to" << d->urlForLog(); + onSentRequest(d->reply.data()); + emit sentRequest(); } else - qCWarning(d->logCat) << this << "request could not start"; + qCWarning(d->logCat).noquote() + << "Request could not start:" << d->urlForLog(); } void BaseJob::checkReply() { setStatus(doCheckReply(d->reply.data())); } @@ -286,13 +299,7 @@ void BaseJob::gotReply() parseError(d->reply.data(), QJsonDocument::fromJson(d->rawResponse).object())); } - - if (status().code != TooManyRequestsError) - finishJob(); - else { - stop(); - emit retryScheduled(d->retriesTaken, d->retryTimer.interval()); - } + finishJob(); } bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) @@ -416,14 +423,13 @@ BaseJob::Status BaseJob::parseError(QNetworkReply* /*reply*/, const auto errCode = errorJson.value("errcode"_ls).toString(); if (error() == TooManyRequestsError || errCode == "M_LIMIT_EXCEEDED") { QString msg = tr("Too many requests"); - auto retryInterval = errorJson.value("retry_after_ms"_ls).toInt(-1); - if (retryInterval != -1) - msg += tr(", next retry advised after %1 ms").arg(retryInterval); + int64_t retryAfterMs = errorJson.value("retry_after_ms"_ls).toInt(-1); + if (retryAfterMs >= 0) + msg += tr(", next retry advised after %1 ms").arg(retryAfterMs); else // We still have to figure some reasonable interval - retryInterval = getNextRetryInterval(); + retryAfterMs = getNextRetryMs(); - qCWarning(d->logCat) << this << "will retry in" << retryInterval << "ms"; - d->retryTimer.start(retryInterval); + d->connection->limitRate(milliseconds(retryAfterMs)); return { TooManyRequestsError, msg }; } @@ -470,19 +476,23 @@ void BaseJob::stop() void BaseJob::finishJob() { stop(); - if ((error() == NetworkError || error() == TimeoutError) + if (error() == TooManyRequests) { + emit rateLimited(); + setStatus(Pending); + d->connection->submit(this); + return; + } + if ((error() == NetworkError || error() == Timeout) && d->retriesTaken < d->maxRetries) { - // TODO: The whole retrying thing should be put to ConnectionManager + // TODO: The whole retrying thing should be put to Connection(Manager) // otherwise independently retrying jobs make a bit of notification // storm towards the UI. - const auto retryInterval = error() == TimeoutError - ? 0 - : getNextRetryInterval(); + const seconds retryIn = error() == Timeout ? 0s : getNextRetryInterval(); ++d->retriesTaken; qCWarning(d->logCat).nospace() << this << ": retry #" << d->retriesTaken - << " in " << retryInterval / 1000 << " s"; - d->retryTimer.start(retryInterval); - emit retryScheduled(d->retriesTaken, retryInterval); + << " in " << retryIn.count() << " s"; + d->retryTimer.start(retryIn); + emit retryScheduled(d->retriesTaken, milliseconds(retryIn).count()); return; } @@ -498,24 +508,35 @@ void BaseJob::finishJob() deleteLater(); } -const JobTimeoutConfig& BaseJob::Private::getCurrentTimeoutConfig() const +seconds BaseJob::getCurrentTimeout() const +{ + return d->getCurrentTimeoutConfig().jobTimeout; +} + +BaseJob::duration_ms_t BaseJob::getCurrentTimeoutMs() const +{ + return milliseconds(getCurrentTimeout()).count(); +} + +seconds BaseJob::getNextRetryInterval() const { - return errorStrategy[std::min(retriesTaken, errorStrategy.size() - 1)]; + return d->getCurrentTimeoutConfig().nextRetryInterval; } -BaseJob::duration_t BaseJob::getCurrentTimeout() const +BaseJob::duration_ms_t BaseJob::getNextRetryMs() const { - return d->getCurrentTimeoutConfig().jobTimeout * 1000; + return milliseconds(getNextRetryInterval()).count(); } -BaseJob::duration_t BaseJob::getNextRetryInterval() const +milliseconds BaseJob::timeToRetry() const { - return d->getCurrentTimeoutConfig().nextRetryInterval * 1000; + return d->retryTimer.isActive() ? d->retryTimer.remainingTimeAsDuration() + : 0s; } -BaseJob::duration_t BaseJob::millisToRetry() const +BaseJob::duration_ms_t BaseJob::millisToRetry() const { - return d->retryTimer.isActive() ? d->retryTimer.remainingTime() : 0; + return timeToRetry().count(); } int BaseJob::maxRetries() const { return d->maxRetries; } diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 9de7b49d..4dc287f8 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -34,11 +34,6 @@ class ConnectionData; enum class HttpVerb { Get, Put, Post, Delete }; -struct JobTimeoutConfig { - int jobTimeout; - int nextRetryInterval; -}; - class BaseJob : public QObject { Q_OBJECT Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) @@ -59,6 +54,7 @@ public: WarningLevel = 20, //< Warnings have codes starting from this UnexpectedResponseType = 21, UnexpectedResponseTypeWarning = UnexpectedResponseType, + Unprepared = 25, //< Initial job state is incomplete, hence warning level Abandoned = 50, //< A tiny period between abandoning and object deletion ErrorLevel = 100, //< Errors have codes starting from this NetworkError = 100, @@ -140,8 +136,6 @@ public: QString message; }; - using duration_t = int; // milliseconds - public: BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, bool needsToken = true); @@ -153,13 +147,16 @@ public: /** Current status of the job */ Status status() const; + /** Short human-friendly message on the job status */ QString statusCaption() const; + /** Get raw response body as received from the server * \param bytesAtMost return this number of leftmost bytes, or -1 * to return the entire response */ QByteArray rawData(int bytesAtMost = -1) const; + /** Get UI-friendly sample of raw data * * This is almost the same as rawData but appends the "truncated" @@ -175,17 +172,24 @@ public: * \sa status */ int error() const; + /** Error-specific message, as returned by the server */ virtual QString errorString() const; + /** A URL to help/clarify the error, if provided by the server */ QUrl errorUrl() const; int maxRetries() const; void setMaxRetries(int newMaxRetries); - Q_INVOKABLE duration_t getCurrentTimeout() const; - Q_INVOKABLE duration_t getNextRetryInterval() const; - Q_INVOKABLE duration_t millisToRetry() const; + using duration_ms_t = std::chrono::milliseconds::rep; // normally int64_t + + std::chrono::seconds getCurrentTimeout() const; + Q_INVOKABLE duration_ms_t getCurrentTimeoutMs() const; + std::chrono::seconds getNextRetryInterval() const; + Q_INVOKABLE duration_ms_t getNextRetryMs() const; + std::chrono::milliseconds timeToRetry() const; + Q_INVOKABLE duration_ms_t millisToRetry() const; friend QDebug operator<<(QDebug dbg, const BaseJob* j) { @@ -193,7 +197,7 @@ public: } public slots: - void start(const ConnectionData* connData, bool inBackground = false); + void prepare(ConnectionData* connData, bool inBackground); /** * Abandons the result of this job, arrived or unarrived. @@ -206,10 +210,10 @@ public slots: signals: /** The job is about to send a network request */ - void aboutToStart(); + void aboutToSendRequest(); /** The job has sent a network request */ - void started(); + void sentRequest(); /** The job has changed its status */ void statusChanged(Status newStatus); @@ -222,7 +226,14 @@ signals: * @param inMilliseconds the interval after which the next attempt will be * taken */ - void retryScheduled(int nextAttempt, int inMilliseconds); + void retryScheduled(int nextAttempt, duration_ms_t inMilliseconds); + + /** + * The previous network request has been rate-limited; the next attempt + * will be queued and run sometime later. Since other jobs may already + * wait in the queue, it's not possible to predict the wait time. + */ + void rateLimited(); /** * Emitted when the job is finished, in any case. It is used to notify @@ -297,9 +308,20 @@ protected: static QUrl makeRequestUrl(QUrl baseUrl, const QString& path, const QUrlQuery& query = {}); - virtual void beforeStart(const ConnectionData* connData); - virtual void afterStart(const ConnectionData* connData, - QNetworkReply* reply); + /*! Prepares the job for execution + * + * This method is called no more than once per job lifecycle, + * when it's first scheduled for execution; in particular, it is not called + * on retries. + */ + virtual void doPrepare(); + /*! Postprocessing after the network request has been sent + * + * This method is called every time the job receives a running + * QNetworkReply object from NetworkAccessManager - basically, after + * successfully sending a network request (including retries). + */ + virtual void onSentRequest(QNetworkReply*); virtual void beforeAbandon(QNetworkReply*); /** @@ -341,8 +363,7 @@ protected: * @param reply the HTTP reply from the server * @param errorJson the JSON payload describing the error */ - virtual Status parseError(QNetworkReply* reply, - const QJsonObject& errorJson); + virtual Status parseError(QNetworkReply*, const QJsonObject& errorJson); void setStatus(Status s); void setStatus(int code, QString message); @@ -359,10 +380,12 @@ protected slots: void timeout(); private slots: - void sendRequest(bool inBackground); + void sendRequest(); void checkReply(); void gotReply(); + friend class ConnectionData; // to provide access to sendRequest() + private: void stop(); void finishJob(); diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 3a03efde..4e997326 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -39,7 +39,7 @@ QString DownloadFileJob::targetFileName() const return (d->targetFile ? d->targetFile : d->tempFile)->fileName(); } -void DownloadFileJob::beforeStart(const ConnectionData*) +void DownloadFileJob::doPrepare() { if (d->targetFile && !d->targetFile->isReadable() && !d->targetFile->open(QIODevice::WriteOnly)) { @@ -57,7 +57,7 @@ void DownloadFileJob::beforeStart(const ConnectionData*) qCDebug(JOBS) << "Downloading to" << d->tempFile->fileName(); } -void DownloadFileJob::afterStart(const ConnectionData*, QNetworkReply* reply) +void DownloadFileJob::onSentRequest(QNetworkReply* reply) { connect(reply, &QNetworkReply::metaDataChanged, this, [this, reply] { if (!status().good()) diff --git a/lib/jobs/downloadfilejob.h b/lib/jobs/downloadfilejob.h index fa697219..b7d2d75b 100644 --- a/lib/jobs/downloadfilejob.h +++ b/lib/jobs/downloadfilejob.h @@ -19,8 +19,8 @@ private: class Private; QScopedPointer d; - void beforeStart(const ConnectionData*) override; - void afterStart(const ConnectionData*, QNetworkReply* reply) override; + void doPrepare() override; + void onSentRequest(QNetworkReply* reply) override; void beforeAbandon(QNetworkReply*) override; Status parseReply(QNetworkReply*) override; }; -- cgit v1.2.3 From c4a20a6d9a5f6bfbd4315450ccaff8195a3f521a Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 21 Aug 2019 08:17:09 +0900 Subject: Don't use enumerator attributes Anything after enumerators is a problem for moc before Qt 5.12; so we can't use enumerator attributes before then. --- lib/jobs/basejob.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 4dc287f8..6c1b802c 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -66,8 +66,7 @@ public: IncorrectRequestError = IncorrectRequest, IncorrectResponse, IncorrectResponseError = IncorrectResponse, - JsonParseError - Q_DECL_ENUMERATOR_DEPRECATED_X("Use IncorrectResponse instead") + JsonParseError //< \deprecated Use IncorrectResponse instead = IncorrectResponse, TooManyRequests, TooManyRequestsError = TooManyRequests, -- cgit v1.2.3 From 97cac4b919735eb59493dd1cd528992ba90e611b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 27 Aug 2019 17:22:22 +0900 Subject: More compliant 'using' for chrono_literals Compilers warn on using 'using ...::operator""s' because they think we're redefining the reserved suffix. --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 0a17431c..54931c83 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -32,7 +32,7 @@ using namespace Quotient; using std::chrono::seconds, std::chrono::milliseconds; -using std::chrono_literals::operator""s; +using namespace std::chrono_literals; struct NetworkReplyDeleter : public QScopedPointerDeleteLater { static inline void cleanup(QNetworkReply* reply) -- cgit v1.2.3 From f9c0e04259be9fd3be70486bc1eb76bf8f2612fe Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 29 Sep 2019 18:05:17 +0900 Subject: Rename pieces with qmc/qmatrixclient --- lib/jobs/downloadfilejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 4e997326..3e037680 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -12,7 +12,7 @@ public: explicit Private(const QString& localFilename) : targetFile(new QFile(localFilename)) - , tempFile(new QFile(targetFile->fileName() + ".qmcdownload")) + , tempFile(new QFile(targetFile->fileName() + ".qtntdownload")) {} QScopedPointer targetFile; -- cgit v1.2.3 From 7802bcb4dd44c7523cbc687b52dd2e65b900c636 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 29 Sep 2019 18:11:35 +0900 Subject: BaseJob, urlForLog() -> dumpRequest(): include HTTP verb into log lines --- lib/jobs/basejob.cpp | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 54931c83..6a70bc40 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -105,11 +106,17 @@ public: errorStrategy.size() - 1)]; } - QString urlForLog() const + QString dumpRequest() const { - return reply - ? reply->url().toString(QUrl::RemoveQuery) - : makeRequestUrl(connection->baseUrl(), apiEndpoint).toString(); + // Thanks to C++17, std::array's type and bounds are deduced + static const auto verbs = + std::array { QStringLiteral("GET"), QStringLiteral("PUT"), + QStringLiteral("POST"), QStringLiteral("DELETE") }; + const auto verbWord = verbs.at(size_t(verb)); + return verbWord % ' ' + % (reply ? reply->url().toString(QUrl::RemoveQuery) + : makeRequestUrl(connection->baseUrl(), apiEndpoint) + .toString()); } }; @@ -260,7 +267,7 @@ void BaseJob::sendRequest() if (status().code == Abandoned) return; Q_ASSERT(d->connection && status().code == Pending); - qCDebug(d->logCat) << "Making request to" << d->urlForLog(); + qCDebug(d->logCat).noquote() << "Making" << d->dumpRequest(); emit aboutToSendRequest(); d->sendRequest(); Q_ASSERT(d->reply); @@ -273,12 +280,12 @@ void BaseJob::sendRequest() connect(d->reply.data(), &QNetworkReply::downloadProgress, this, &BaseJob::downloadProgress); d->timer.start(getCurrentTimeout()); - qCInfo(d->logCat).noquote() << "Request sent to" << d->urlForLog(); + qCInfo(d->logCat).noquote() << "Sent" << d->dumpRequest(); onSentRequest(d->reply.data()); emit sentRequest(); } else qCWarning(d->logCat).noquote() - << "Request could not start:" << d->urlForLog(); + << "Request could not start:" << d->dumpRequest(); } void BaseJob::checkReply() { setStatus(doCheckReply(d->reply.data())); } @@ -378,7 +385,8 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const const auto httpCodeHeader = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); if (!httpCodeHeader.isValid()) { - qCWarning(d->logCat) << "No valid HTTP headers from" << d->urlForLog(); + qCWarning(d->logCat).noquote() + << "No valid HTTP headers from" << d->dumpRequest(); return { NetworkError, reply->errorString() }; } @@ -386,7 +394,7 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const if (httpCode / 100 == 2) // 2xx { if (reply->isFinished()) - qCInfo(d->logCat) << httpCode << "<-" << d->urlForLog(); + qCInfo(d->logCat).noquote() << httpCode << "<-" << d->dumpRequest(); if (!checkContentType(reply->rawHeader("Content-Type"), d->expectedContentTypes)) return { UnexpectedResponseTypeWarning, @@ -394,7 +402,7 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const return NoError; } if (reply->isFinished()) - qCWarning(d->logCat) << httpCode << "<-" << d->urlForLog(); + qCWarning(d->logCat).noquote() << httpCode << "<-" << d->dumpRequest(); auto message = reply->errorString(); if (message.isEmpty()) -- cgit v1.2.3 From a2badac44d94294d9404224a55b3193c87cc201d Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 2 Oct 2019 11:36:54 +0900 Subject: More fixes for Apple not having deduction guides in stdlib --- lib/jobs/basejob.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 6a70bc40..7336fc47 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -44,6 +44,12 @@ struct NetworkReplyDeleter : public QScopedPointerDeleteLater { } }; +template +constexpr auto make_array(Ts&&... items) +{ + return std::array, sizeof...(Ts)>({items...}); +} + class BaseJob::Private { public: struct JobTimeoutConfig { @@ -108,10 +114,10 @@ public: QString dumpRequest() const { - // Thanks to C++17, std::array's type and bounds are deduced + // FIXME: use std::array {} when Apple stdlib gets deduction guides for it static const auto verbs = - std::array { QStringLiteral("GET"), QStringLiteral("PUT"), - QStringLiteral("POST"), QStringLiteral("DELETE") }; + make_array(QStringLiteral("GET"), QStringLiteral("PUT"), + QStringLiteral("POST"), QStringLiteral("DELETE")); const auto verbWord = verbs.at(size_t(verb)); return verbWord % ' ' % (reply ? reply->url().toString(QUrl::RemoveQuery) -- cgit v1.2.3 From 4623632e14ee9367e5728daf745d5323eb6a51d5 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 2 Oct 2019 12:04:32 +0900 Subject: BaseJob::Private: experiment with [[nodiscard]] --- lib/jobs/basejob.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 7336fc47..13e65188 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -106,13 +106,13 @@ public: int maxRetries = int(errorStrategy.size()); int retriesTaken = 0; - const JobTimeoutConfig& getCurrentTimeoutConfig() const + [[nodiscard]] const JobTimeoutConfig& getCurrentTimeoutConfig() const { return errorStrategy[std::min(size_t(retriesTaken), errorStrategy.size() - 1)]; } - QString dumpRequest() const + [[nodiscard]] QString dumpRequest() const { // FIXME: use std::array {} when Apple stdlib gets deduction guides for it static const auto verbs = -- cgit v1.2.3 From 8b9207d5a04386957d8eab8dd251421eaaa7c0d2 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 20 Oct 2019 19:13:56 +0900 Subject: Qualify types in signals and Q_INVOKABLEs Because https://doc.qt.io/qt-5/moc.html#limitations . For direct connections that doesn't matter but it very much does for queued ones. Along with this DirectChatsMap and IgnoredUsersList have been moved from Connection:: to Quotient::. --- lib/jobs/basejob.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 6c1b802c..c4da40f5 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -184,11 +184,11 @@ public: using duration_ms_t = std::chrono::milliseconds::rep; // normally int64_t std::chrono::seconds getCurrentTimeout() const; - Q_INVOKABLE duration_ms_t getCurrentTimeoutMs() const; + Q_INVOKABLE Quotient::BaseJob::duration_ms_t getCurrentTimeoutMs() const; std::chrono::seconds getNextRetryInterval() const; - Q_INVOKABLE duration_ms_t getNextRetryMs() const; + Q_INVOKABLE Quotient::BaseJob::duration_ms_t getNextRetryMs() const; std::chrono::milliseconds timeToRetry() const; - Q_INVOKABLE duration_ms_t millisToRetry() const; + Q_INVOKABLE Quotient::BaseJob::duration_ms_t millisToRetry() const; friend QDebug operator<<(QDebug dbg, const BaseJob* j) { @@ -215,7 +215,7 @@ signals: void sentRequest(); /** The job has changed its status */ - void statusChanged(Status newStatus); + void statusChanged(Quotient::BaseJob::Status newStatus); /** * The previous network request has failed; the next attempt will @@ -225,7 +225,8 @@ signals: * @param inMilliseconds the interval after which the next attempt will be * taken */ - void retryScheduled(int nextAttempt, duration_ms_t inMilliseconds); + void retryScheduled(int nextAttempt, + Quotient::BaseJob::duration_ms_t inMilliseconds); /** * The previous network request has been rate-limited; the next attempt @@ -251,7 +252,7 @@ signals: * * @see result, success, failure */ - void finished(BaseJob* job); + void finished(Quotient::BaseJob* job); /** * Emitted when the job is finished (except when abandoned). @@ -262,14 +263,14 @@ signals: * * @see success, failure */ - void result(BaseJob* job); + void result(Quotient::BaseJob* job); /** * Emitted together with result() in case there's no error. * * @see result, failure */ - void success(BaseJob*); + void success(Quotient::BaseJob*); /** * Emitted together with result() if there's an error. @@ -277,7 +278,7 @@ signals: * * @see result, success */ - void failure(BaseJob*); + void failure(Quotient::BaseJob*); void downloadProgress(qint64 bytesReceived, qint64 bytesTotal); void uploadProgress(qint64 bytesSent, qint64 bytesTotal); -- cgit v1.2.3 From 5937127b73a82fc86f6546397373ce9dbaf4e560 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 19 Nov 2019 17:57:41 +0900 Subject: BaseJob: Don't send accessToken if not needed; send again on 401 The first part closes #358; the second part is a workaround for non-standard cases when endpoints without security by the spec turn out to be secured (in particular, the case of authenticating media servers). --- lib/jobs/basejob.cpp | 66 +++++++++++++++++++++++++++++++--------------------- lib/jobs/basejob.h | 8 ++++++- 2 files changed, 47 insertions(+), 27 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 13e65188..7f558685 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -226,8 +226,9 @@ void BaseJob::Private::sendRequest() requestQuery) }; if (!requestHeaders.contains("Content-Type")) req.setHeader(QNetworkRequest::ContentTypeHeader, "application/json"); - req.setRawHeader("Authorization", - QByteArray("Bearer ") + connection->accessToken()); + if (needsToken) + req.setRawHeader("Authorization", + QByteArray("Bearer ") + connection->accessToken()); req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); @@ -274,6 +275,7 @@ void BaseJob::sendRequest() return; Q_ASSERT(d->connection && status().code == Pending); qCDebug(d->logCat).noquote() << "Making" << d->dumpRequest(); + d->needsToken |= d->connection->needsToken(objectName()); emit aboutToSendRequest(); d->sendRequest(); Q_ASSERT(d->reply); @@ -341,32 +343,32 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) return false; } -BaseJob::Status BaseJob::Status::fromHttpCode(int httpCode, QString msg) +BaseJob::StatusCode BaseJob::Status::fromHttpCode(int httpCode) { + if (httpCode / 10 == 41) // 41x errors + return httpCode == 410 ? IncorrectRequestError : NotFoundError; + switch (httpCode) { + case 401: + return Unauthorised; // clang-format off - return { [httpCode]() -> StatusCode { - if (httpCode / 10 == 41) // 41x errors - return httpCode == 410 ? IncorrectRequestError : NotFoundError; - switch (httpCode) { - case 401: case 403: case 407: - return ContentAccessError; - case 404: - return NotFoundError; - case 400: case 405: case 406: case 426: case 428: case 505: - case 494: // Unofficial nginx "Request header too large" - case 497: // Unofficial nginx "HTTP request sent to HTTPS port" - return IncorrectRequestError; - case 429: - return TooManyRequestsError; - case 501: case 510: - return RequestNotImplementedError; - case 511: - return NetworkAuthRequiredError; - default: - return NetworkError; - } - }(), std::move(msg) }; - // clang-format on + case 403: case 407: // clang-format on + return ContentAccessError; + case 404: + return NotFoundError; + // clang-format off + case 400: case 405: case 406: case 426: case 428: case 505: // clang-format on + case 494: // Unofficial nginx "Request header too large" + case 497: // Unofficial nginx "HTTP request sent to HTTPS port" + return IncorrectRequestError; + case 429: + return TooManyRequestsError; + case 501: case 510: + return RequestNotImplementedError; + case 511: + return NetworkAuthRequiredError; + default: + return NetworkError; + } } QDebug BaseJob::Status::dumpToLog(QDebug dbg) const @@ -496,6 +498,16 @@ void BaseJob::finishJob() d->connection->submit(this); return; } + if (error() == Unauthorised && !d->needsToken + && !d->connection->accessToken().isEmpty()) { + // Rerun with access token (extension of the spec while + // https://github.com/matrix-org/matrix-doc/issues/701 is pending) + d->connection->setNeedsToken(objectName()); + qCWarning(d->logCat) << this << "re-running with authentication"; + emit retryScheduled(d->retriesTaken, 0); + setStatus(Pending); + sendRequest(); + } if ((error() == NetworkError || error() == Timeout) && d->retriesTaken < d->maxRetries) { // TODO: The whole retrying thing should be put to Connection(Manager) @@ -596,6 +608,8 @@ QString BaseJob::statusCaption() const return tr("Network problems"); case TimeoutError: return tr("Request timed out"); + case Unauthorised: + return tr("Unauthorised request"); case ContentAccessError: return tr("Access error"); case NotFoundError: diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index c4da40f5..ecbec74a 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -60,6 +60,7 @@ public: NetworkError = 100, Timeout, TimeoutError = Timeout, + Unauthorised, ContentAccessError, NotFoundError, IncorrectRequest, @@ -113,7 +114,12 @@ public: struct Status { Status(StatusCode c) : code(c) {} Status(int c, QString m) : code(c), message(std::move(m)) {} - static Status fromHttpCode(int httpCode, QString msg = {}); + + static StatusCode fromHttpCode(int httpCode); + static Status fromHttpCode(int httpCode, QString msg) + { + return { fromHttpCode(httpCode), std::move(msg) }; + } bool good() const { return code < ErrorLevel; } QDebug dumpToLog(QDebug dbg) const; -- cgit v1.2.3 From 7b5f65ba64ae4df54919336f4beec19493d1b5ee Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 11 Dec 2019 20:26:39 +0300 Subject: BaseJob::StatusCode: offset first error from ErrorLevel Q_ENUM seems to resolve int to the first enum identifier with that value so NetworkError == ErrorLevel looks confusing in logs. --- lib/jobs/basejob.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index ecbec74a..b4c4c136 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -57,7 +57,7 @@ public: Unprepared = 25, //< Initial job state is incomplete, hence warning level Abandoned = 50, //< A tiny period between abandoning and object deletion ErrorLevel = 100, //< Errors have codes starting from this - NetworkError = 100, + NetworkError = 101, Timeout, TimeoutError = Timeout, Unauthorised, -- cgit v1.2.3 From 1ffa6b96adeba8c1a00a0a32112c8b0aaeb2350e Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 12 Dec 2019 19:11:16 +0300 Subject: RequestData: optimise fromData() and preopen the buffer It was just a coincidence that QBuffer allowed reading from it without being isReadable() at the moment of starting a job. --- lib/jobs/requestdata.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/requestdata.cpp b/lib/jobs/requestdata.cpp index 0c70f085..cec15954 100644 --- a/lib/jobs/requestdata.cpp +++ b/lib/jobs/requestdata.cpp @@ -11,9 +11,8 @@ using namespace Quotient; auto fromData(const QByteArray& data) { auto source = std::make_unique(); - source->open(QIODevice::WriteOnly); - source->write(data); - source->close(); + source->setData(data); + source->open(QIODevice::ReadOnly); return source; } -- cgit v1.2.3 From ec3ddd6930991b04d13cdb12720f141482f9a6eb Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 12 Dec 2019 19:37:09 +0300 Subject: Move FileError definition from DownloadFileJob to BaseJob Will use it in BaseJob in a later commit. --- lib/jobs/basejob.h | 1 + lib/jobs/downloadfilejob.h | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index b4c4c136..e708ba8d 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -82,6 +82,7 @@ public: UserConsentRequiredError = UserConsentRequired, CannotLeaveRoom, UserDeactivated, + FileError, UserDefinedError = 256 }; Q_ENUM(StatusCode) diff --git a/lib/jobs/downloadfilejob.h b/lib/jobs/downloadfilejob.h index b7d2d75b..06dc145c 100644 --- a/lib/jobs/downloadfilejob.h +++ b/lib/jobs/downloadfilejob.h @@ -5,8 +5,6 @@ namespace Quotient { class DownloadFileJob : public GetContentJob { public: - enum { FileError = BaseJob::UserDefinedError + 1 }; - using GetContentJob::makeRequestUrl; static QUrl makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri); -- cgit v1.2.3 From f6505a541fc0a2e02f9c79496488121a3e46fb01 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 12 Dec 2019 19:47:07 +0300 Subject: BaseJob: prepare() -> initiate() + refactoring around it * BaseJob::initiate() now calls ConnectionData::submit() without relying on Connection to do that * ConnectionData::submit() is now the only site where a job enters Pending state * No more shortcuts to BaseJob::sendRequest(), even retries are sent through the ConnectionData submission queue * Additional validation in BaseJob::initiate() that the request data device is actually open (because QtNetwork API officially requires that, even if you can get away passing a closed QBuffer to it) --- lib/jobs/basejob.cpp | 28 +++++++++++++++++++--------- lib/jobs/basejob.h | 2 +- 2 files changed, 20 insertions(+), 10 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 7f558685..4653e58a 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -138,8 +138,7 @@ BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, setObjectName(name); connect(&d->timer, &QTimer::timeout, this, &BaseJob::timeout); connect(&d->retryTimer, &QTimer::timeout, this, [this] { - setStatus(Pending); - sendRequest(); + d->connection->submit(this); }); } @@ -259,14 +258,27 @@ void BaseJob::onSentRequest(QNetworkReply*) {} void BaseJob::beforeAbandon(QNetworkReply*) {} -void BaseJob::prepare(ConnectionData* connData, bool inBackground) +void BaseJob::initiate(ConnectionData* connData, bool inBackground) { + Q_ASSERT(connData != nullptr); + d->inBackground = inBackground; d->connection = connData; doPrepare(); - if (status().code != Unprepared && status().code != Pending) + + if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) + && !d->requestData.source()->isReadable()) { + setStatus(FileError, "Request data not ready"); + } + Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this + if (status().code == Unprepared) { + d->connection->submit(this); + } else { + qDebug(d->logCat).noquote() + << "Request failed preparation and won't be sent:" + << d->dumpRequest(); QTimer::singleShot(0, this, &BaseJob::finishJob); - setStatus(Pending); + } } void BaseJob::sendRequest() @@ -292,7 +304,7 @@ void BaseJob::sendRequest() onSentRequest(d->reply.data()); emit sentRequest(); } else - qCWarning(d->logCat).noquote() + qCCritical(d->logCat).noquote() << "Request could not start:" << d->dumpRequest(); } @@ -494,7 +506,6 @@ void BaseJob::finishJob() stop(); if (error() == TooManyRequests) { emit rateLimited(); - setStatus(Pending); d->connection->submit(this); return; } @@ -505,8 +516,7 @@ void BaseJob::finishJob() d->connection->setNeedsToken(objectName()); qCWarning(d->logCat) << this << "re-running with authentication"; emit retryScheduled(d->retriesTaken, 0); - setStatus(Pending); - sendRequest(); + d->connection->submit(this); } if ((error() == NetworkError || error() == Timeout) && d->retriesTaken < d->maxRetries) { diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index e708ba8d..c8046e9e 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -203,7 +203,7 @@ public: } public slots: - void prepare(ConnectionData* connData, bool inBackground); + void initiate(ConnectionData* connData, bool inBackground); /** * Abandons the result of this job, arrived or unarrived. -- cgit v1.2.3 From 38f45066a2304935550e2c5b6be21a9744f66bf1 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 12 Dec 2019 20:57:41 +0300 Subject: BaseJob::initiate(): fix nullptr dereferencing --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 4653e58a..68adeaf6 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -267,7 +267,7 @@ void BaseJob::initiate(ConnectionData* connData, bool inBackground) doPrepare(); if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) - && !d->requestData.source()->isReadable()) { + && d->requestData.source() && !d->requestData.source()->isReadable()) { setStatus(FileError, "Request data not ready"); } Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this -- cgit v1.2.3 From ecc31fc7a615faa516a03c07ebd96d0d2494c315 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 30 Mar 2020 19:12:06 +0200 Subject: BaseJob: shutdown timers on abandoning and destruction A part of the fix for #398. --- lib/jobs/basejob.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 68adeaf6..8b63f7e8 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -145,6 +145,7 @@ BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, BaseJob::~BaseJob() { stop(); + d->retryTimer.stop(); // See #398 qCDebug(d->logCat) << this << "destroyed"; } @@ -686,6 +687,8 @@ void BaseJob::setStatus(int code, QString message) void BaseJob::abandon() { beforeAbandon(d->reply ? d->reply.data() : nullptr); + d->timer.stop(); + d->retryTimer.stop(); // In case abandon() was called between retries setStatus(Abandoned); if (d->reply) d->reply->disconnect(this); -- cgit v1.2.3 From d38516e4e123474876e4fc7f850fe321ec5ccb54 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 30 Mar 2020 19:12:40 +0200 Subject: BaseJob: check the connection even harder --- lib/jobs/basejob.cpp | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 8b63f7e8..65668521 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -234,6 +234,7 @@ void BaseJob::Private::sendRequest() req.setMaximumRedirectsAllowed(10); req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true); + Q_ASSERT(req.url().isValid()); for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) req.setRawHeader(it.key(), it.value()); @@ -261,25 +262,31 @@ void BaseJob::beforeAbandon(QNetworkReply*) {} void BaseJob::initiate(ConnectionData* connData, bool inBackground) { - Q_ASSERT(connData != nullptr); + if (connData && connData->baseUrl().isValid()) { + d->inBackground = inBackground; + d->connection = connData; + doPrepare(); - d->inBackground = inBackground; - d->connection = connData; - doPrepare(); - - if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) - && d->requestData.source() && !d->requestData.source()->isReadable()) { - setStatus(FileError, "Request data not ready"); - } - Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this - if (status().code == Unprepared) { - d->connection->submit(this); - } else { - qDebug(d->logCat).noquote() + if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) + && d->requestData.source() && !d->requestData.source()->isReadable()) { + setStatus(FileError, "Request data not ready"); + } + Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this + if (status().code == Unprepared) { + d->connection->submit(this); + return; + } + qCWarning(d->logCat).noquote() << "Request failed preparation and won't be sent:" << d->dumpRequest(); - QTimer::singleShot(0, this, &BaseJob::finishJob); + } else { + qCCritical(d->logCat) + << "Developers, ensure the Connection is valid before using it"; + Q_ASSERT(false); + setStatus(IncorrectRequestError, tr("Invalid server connection")); } + // The status is no good, finalise + QTimer::singleShot(0, this, &BaseJob::finishJob); } void BaseJob::sendRequest() @@ -338,7 +345,7 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) // ignore possible appendixes of the content type const auto ctype = type.split(';').front(); - for (const auto& pattern : patterns) { + for (const auto& pattern: patterns) { if (pattern.startsWith('*') || ctype == pattern) // Fast lane return true; -- cgit v1.2.3 From 3d04593f513916ee6e4fcb1ec2d18afa7b76590a Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 5 Apr 2020 15:48:31 +0200 Subject: RequestData: create empty QByteArray by default May fix #380. Also: remove explicit copying disablers, the unique_ptr<> member disables copying just fine. --- lib/jobs/requestdata.h | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/requestdata.h b/lib/jobs/requestdata.h index 020d5ef2..9cb5ecaf 100644 --- a/lib/jobs/requestdata.h +++ b/lib/jobs/requestdata.h @@ -18,9 +18,10 @@ #pragma once +#include + #include -class QByteArray; class QJsonObject; class QJsonArray; class QJsonDocument; @@ -35,14 +36,11 @@ namespace Quotient { */ class RequestData { public: - RequestData() = default; - RequestData(const QByteArray& a); + RequestData(const QByteArray& a = {}); RequestData(const QJsonObject& jo); RequestData(const QJsonArray& ja); RequestData(QIODevice* source) : _source(std::unique_ptr(source)) {} - RequestData(const RequestData&) = delete; - RequestData& operator=(const RequestData&) = delete; RequestData(RequestData&&) = default; RequestData& operator=(RequestData&&) = default; ~RequestData(); -- cgit v1.2.3 From ab9996fc62767d61da779d869e68bfc966ffce8f Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 6 Apr 2020 20:53:26 +0200 Subject: CMakeLists: install header files to a subdirectory This is a Quotient part of #328 fix; QtOlm part is pending. --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 65668521..4ede073f 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -232,7 +232,7 @@ void BaseJob::Private::sendRequest() req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); - req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); +// req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true); Q_ASSERT(req.url().isValid()); for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) -- cgit v1.2.3 From a8572345e7a30c96b8ede47e95af65ff2cdef86c Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 6 Apr 2020 21:20:32 +0200 Subject: Revert changes accidentally sneaked in with the previous commit --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 4ede073f..65668521 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -232,7 +232,7 @@ void BaseJob::Private::sendRequest() req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); -// req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); + req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true); Q_ASSERT(req.url().isValid()); for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) -- cgit v1.2.3 From fc2614d88ccc3c938ea69fd6a8f4978bc9663e16 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 6 Apr 2020 21:58:26 +0200 Subject: BaseJob: disable pipelining Longer running on RHEL/CentOS 8 leads to crashes that no more occur with disabled pipelining. --- lib/jobs/basejob.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 65668521..dfc3d3dd 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -232,7 +232,9 @@ void BaseJob::Private::sendRequest() req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); - req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); + // Pipelining doesn't fly quite well with SSL, occasionally crashing at + // what seems like an attempt to write to a closed channel. +// req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true); Q_ASSERT(req.url().isValid()); for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) -- cgit v1.2.3 From 160b11bdfe32c9983fe5f34eafa783148b2decbe Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 7 Apr 2020 16:14:21 +0200 Subject: BaseJob: don't finish and re-try at the same time Due to a missing return statement, a retry with auth case led to the job being finished and pending at the same time, with no good consequences. --- lib/jobs/basejob.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index dfc3d3dd..7cddc343 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -527,6 +527,7 @@ void BaseJob::finishJob() qCWarning(d->logCat) << this << "re-running with authentication"; emit retryScheduled(d->retriesTaken, 0); d->connection->submit(this); + return; } if ((error() == NetworkError || error() == Timeout) && d->retriesTaken < d->maxRetries) { @@ -542,6 +543,8 @@ void BaseJob::finishJob() return; } + Q_ASSERT(status().code != Pending); + // Notify those interested in any completion of the job including abandon() emit finished(this); -- cgit v1.2.3 From 4088ab4572c5b7cde603aeb1a89bc4515833beaf Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 14 Apr 2020 14:21:26 +0200 Subject: BaseJob::makeRequestUrl(): even more tolerance to slash separators The code is really defensive now, making sure there's exactly one slash between the base path and the endpoint. It's still very conservative about the path composition otherwise (no normalisation etc.). --- lib/jobs/basejob.cpp | 7 +++++-- lib/jobs/basejob.h | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 7cddc343..8e1a63aa 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -212,8 +212,11 @@ QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QString& path, const QUrlQuery& query) { auto pathBase = baseUrl.path(); - if (!pathBase.endsWith('/') && !path.startsWith('/')) - pathBase.push_back('/'); + // QUrl::adjusted(QUrl::StripTrailingSlashes) doesn't help with root '/' + while (pathBase.endsWith('/')) + pathBase.chop(1); + if (!path.startsWith('/')) // Normally API files do start with '/' + pathBase.push_back('/'); // so this shouldn't be needed these days baseUrl.setPath(pathBase + path, QUrl::TolerantMode); baseUrl.setQuery(query); diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index c8046e9e..2049f59c 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -308,9 +308,9 @@ protected: void setExpectedContentTypes(const QByteArrayList& contentTypes); /** Construct a URL out of baseUrl, path and query - * The function automatically adds '/' between baseUrl's path and - * \p path if necessary. The query component of \p baseUrl - * is ignored. + * + * The function ensures exactly one '/' between the path component of + * \p baseUrl and \p path. The query component of \p baseUrl is ignored. */ static QUrl makeRequestUrl(QUrl baseUrl, const QString& path, const QUrlQuery& query = {}); -- cgit v1.2.3 From 3c2f98375c4b82e7f1ee8a0f5d477e55c8685075 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 12 May 2020 21:23:56 +0200 Subject: basejob.cpp: nitpicks from clang-format --- lib/jobs/basejob.cpp | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 8e1a63aa..470250bf 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -149,15 +149,9 @@ BaseJob::~BaseJob() qCDebug(d->logCat) << this << "destroyed"; } -QUrl BaseJob::requestUrl() const -{ - return d->reply ? d->reply->url() : QUrl(); -} +QUrl BaseJob::requestUrl() const { return d->reply ? d->reply->url() : QUrl(); } -bool BaseJob::isBackground() const -{ - return d->inBackground; -} +bool BaseJob::isBackground() const { return d->inBackground; } const QString& BaseJob::apiEndpoint() const { return d->apiEndpoint; } @@ -215,7 +209,7 @@ QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QString& path, // QUrl::adjusted(QUrl::StripTrailingSlashes) doesn't help with root '/' while (pathBase.endsWith('/')) pathBase.chop(1); - if (!path.startsWith('/')) // Normally API files do start with '/' + if (!path.startsWith('/')) // Normally API files do start with '/' pathBase.push_back('/'); // so this shouldn't be needed these days baseUrl.setPath(pathBase + path, QUrl::TolerantMode); @@ -259,11 +253,11 @@ void BaseJob::Private::sendRequest() } } -void BaseJob::doPrepare() {} +void BaseJob::doPrepare() { } -void BaseJob::onSentRequest(QNetworkReply*) {} +void BaseJob::onSentRequest(QNetworkReply*) { } -void BaseJob::beforeAbandon(QNetworkReply*) {} +void BaseJob::beforeAbandon(QNetworkReply*) { } void BaseJob::initiate(ConnectionData* connData, bool inBackground) { @@ -273,7 +267,8 @@ void BaseJob::initiate(ConnectionData* connData, bool inBackground) doPrepare(); if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) - && d->requestData.source() && !d->requestData.source()->isReadable()) { + && d->requestData.source() + && !d->requestData.source()->isReadable()) { setStatus(FileError, "Request data not ready"); } Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this -- cgit v1.2.3 From cf4d3d7ed82e62b57a11fbc7f491535a761dc75c Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 27 May 2020 19:03:52 +0200 Subject: Move around and format code No functional changes here. --- lib/jobs/basejob.cpp | 94 +++++++++++++++++++++++++++++----------------------- lib/jobs/basejob.h | 17 ++++++---- 2 files changed, 63 insertions(+), 48 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 470250bf..100c05f2 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -35,6 +35,47 @@ using namespace Quotient; using std::chrono::seconds, std::chrono::milliseconds; using namespace std::chrono_literals; +BaseJob::StatusCode BaseJob::Status::fromHttpCode(int httpCode) +{ + if (httpCode / 10 == 41) // 41x errors + return httpCode == 410 ? IncorrectRequestError : NotFoundError; + switch (httpCode) { + case 401: + return Unauthorised; + // clang-format off + case 403: case 407: // clang-format on + return ContentAccessError; + case 404: + return NotFoundError; + // clang-format off + case 400: case 405: case 406: case 426: case 428: case 505: // clang-format on + case 494: // Unofficial nginx "Request header too large" + case 497: // Unofficial nginx "HTTP request sent to HTTPS port" + return IncorrectRequestError; + case 429: + return TooManyRequestsError; + case 501: + case 510: + return RequestNotImplementedError; + case 511: + return NetworkAuthRequiredError; + default: + return NetworkError; + } +} + +QDebug BaseJob::Status::dumpToLog(QDebug dbg) const +{ + QDebugStateSaver _s(dbg); + dbg.noquote().nospace(); + if (auto* const k = QMetaEnum::fromType().valueToKey(code)) { + const QByteArray b = k; + dbg << b.mid(b.lastIndexOf(':')); + } else + dbg << code; + return dbg << ": " << message; +} + struct NetworkReplyDeleter : public QScopedPointerDeleteLater { static inline void cleanup(QNetworkReply* reply) { @@ -316,7 +357,15 @@ void BaseJob::sendRequest() << "Request could not start:" << d->dumpRequest(); } -void BaseJob::checkReply() { setStatus(doCheckReply(d->reply.data())); } +BaseJob::Status BaseJob::Private::parseJsonDocument() +{ + QJsonParseError error { 0, QJsonParseError::MissingObject }; + jsonResponse = QJsonDocument::fromJson(d->rawResponse, &error); + return { error.error == QJsonParseError::NoError + ? BaseJob::NoError + : BaseJob::IncorrectResponse, + error.errorString() }; +} void BaseJob::gotReply() { @@ -363,47 +412,6 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) return false; } -BaseJob::StatusCode BaseJob::Status::fromHttpCode(int httpCode) -{ - if (httpCode / 10 == 41) // 41x errors - return httpCode == 410 ? IncorrectRequestError : NotFoundError; - switch (httpCode) { - case 401: - return Unauthorised; - // clang-format off - case 403: case 407: // clang-format on - return ContentAccessError; - case 404: - return NotFoundError; - // clang-format off - case 400: case 405: case 406: case 426: case 428: case 505: // clang-format on - case 494: // Unofficial nginx "Request header too large" - case 497: // Unofficial nginx "HTTP request sent to HTTPS port" - return IncorrectRequestError; - case 429: - return TooManyRequestsError; - case 501: case 510: - return RequestNotImplementedError; - case 511: - return NetworkAuthRequiredError; - default: - return NetworkError; - } -} - -QDebug BaseJob::Status::dumpToLog(QDebug dbg) const -{ - QDebugStateSaver _s(dbg); - dbg.noquote().nospace(); - if (auto* const k = QMetaEnum::fromType().valueToKey(code)) { - const QByteArray b = k; - dbg << b.mid(b.lastIndexOf(':')); - } else - dbg << code; - return dbg << ": " << message; - -} - BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const { // QNetworkReply error codes seem to be flawed when it comes to HTTP; @@ -440,6 +448,8 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const return Status::fromHttpCode(httpCode, message); } +void BaseJob::checkReply() { setStatus(doCheckReply(d->reply.data())); } + BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) { d->rawResponse = reply->readAll(); diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 2049f59c..954b0777 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -170,6 +170,8 @@ public: * recommended to present a sample of raw data as "details" next to * error messages. Note that the default \p bytesAtMost value is * also tailored to UI cases. + * + * \sa rawData */ QString rawDataSample(int bytesAtMost = 65535) const; @@ -322,6 +324,7 @@ protected: * on retries. */ virtual void doPrepare(); + /*! Postprocessing after the network request has been sent * * This method is called every time the job receives a running @@ -331,13 +334,15 @@ protected: virtual void onSentRequest(QNetworkReply*); virtual void beforeAbandon(QNetworkReply*); - /** - * Used by gotReply() to check the received reply for general - * issues such as network errors or access denial. - * Returning anything except NoError/Success prevents - * further parseReply()/parseJson() invocation. + /*! \brief Check the pending or received reply for upfront issues + * + * This is invoked when headers are first received and also once + * the complete reply is obtained; the base implementation checks the HTTP + * headers to detect general issues such as network errors or access denial. + * It cannot read the response body (use parseReply/parseError to check + * for problems in the body). Returning anything except NoError/Success + * prevents further processing of the reply. * - * @param reply the reply received from the server * @return the result of checking the reply * * @see gotReply -- cgit v1.2.3 From a9edfbd19a624aa4e882df2f5e577ba1831914a3 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 28 May 2020 08:42:04 +0200 Subject: BaseJob::rawData: overload for (even) quicker access No functional changes either. --- lib/jobs/basejob.cpp | 2 ++ lib/jobs/basejob.h | 13 +++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 100c05f2..e649ea87 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -612,6 +612,8 @@ QByteArray BaseJob::rawData(int bytesAtMost) const : d->rawResponse; } +const QByteArray& BaseJob::rawData() const { return d->rawResponse; } + QString BaseJob::rawDataSample(int bytesAtMost) const { auto data = rawData(bytesAtMost); diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 954b0777..010aca78 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -157,11 +157,16 @@ public: /** Short human-friendly message on the job status */ QString statusCaption() const; - /** Get raw response body as received from the server - * \param bytesAtMost return this number of leftmost bytes, or -1 - * to return the entire response + /*! Get first bytes of the raw response body as received from the server + * + * \param bytesAtMost the number of leftmost bytes to return + * + * \sa rawDataSample */ - QByteArray rawData(int bytesAtMost = -1) const; + QByteArray rawData(int bytesAtMost) const; + + /*! Access the whole response body as received from the server */ + const QByteArray& rawData() const; /** Get UI-friendly sample of raw data * -- cgit v1.2.3 From 51ef6011d6dfc55ea0c8b0cdd1d9e7315087aad9 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 28 May 2020 13:24:16 +0200 Subject: BaseJob: use non-deprecated API for Qt 5.15 Qt 5.15 renamed HTTP2AllowedAttribute to Http2AllowedAttribute, deprecating the old spelling. --- lib/jobs/basejob.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index e649ea87..26782edc 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -273,7 +273,13 @@ void BaseJob::Private::sendRequest() // Pipelining doesn't fly quite well with SSL, occasionally crashing at // what seems like an attempt to write to a closed channel. // req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); - req.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, true); + req.setAttribute( +#if (QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)) + QNetworkRequest::Http2AllowedAttribute +#else + QNetworkRequest::HTTP2AllowedAttribute +#endif + , true); Q_ASSERT(req.url().isValid()); for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) req.setRawHeader(it.key(), it.value()); -- cgit v1.2.3 From 7ac93db4553aa624326a8e28b74151c784b937a7 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 1 Jun 2020 14:47:15 +0200 Subject: Remove the piece of code introduced too early on parseJsonDocument() will come later. --- lib/jobs/basejob.cpp | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 26782edc..2519713e 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -363,16 +363,6 @@ void BaseJob::sendRequest() << "Request could not start:" << d->dumpRequest(); } -BaseJob::Status BaseJob::Private::parseJsonDocument() -{ - QJsonParseError error { 0, QJsonParseError::MissingObject }; - jsonResponse = QJsonDocument::fromJson(d->rawResponse, &error); - return { error.error == QJsonParseError::NoError - ? BaseJob::NoError - : BaseJob::IncorrectResponse, - error.errorString() }; -} - void BaseJob::gotReply() { checkReply(); -- cgit v1.2.3 From 04956392c4b1102ac9ae627c324493f035887472 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 5 Jun 2020 18:09:52 +0200 Subject: BaseJob: expose statusCode as Q_PROPERTY --- lib/jobs/basejob.h | 1 + 1 file changed, 1 insertion(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 010aca78..6920ebc1 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -38,6 +38,7 @@ class BaseJob : public QObject { Q_OBJECT Q_PROPERTY(QUrl requestUrl READ requestUrl CONSTANT) Q_PROPERTY(int maxRetries READ maxRetries WRITE setMaxRetries) + Q_PROPERTY(int statusCode READ error NOTIFY statusChanged) public: /*! The status code of a job * -- cgit v1.2.3 From eccab6df7003bb9c20c48dfe0af57502383ba42e Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 5 Jun 2020 18:11:02 +0200 Subject: MediaThumbnailJob: be specific about the transform What's needed for a thumbnail is normally "scale", not "crop" (as these are defined in The Spec). --- lib/jobs/mediathumbnailjob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/mediathumbnailjob.cpp b/lib/jobs/mediathumbnailjob.cpp index 0a346392..df3763b2 100644 --- a/lib/jobs/mediathumbnailjob.cpp +++ b/lib/jobs/mediathumbnailjob.cpp @@ -31,7 +31,7 @@ QUrl MediaThumbnailJob::makeRequestUrl(QUrl baseUrl, const QUrl& mxcUri, MediaThumbnailJob::MediaThumbnailJob(const QString& serverName, const QString& mediaId, QSize requestedSize) : GetContentThumbnailJob(serverName, mediaId, requestedSize.width(), - requestedSize.height()) + requestedSize.height(), "scale") {} MediaThumbnailJob::MediaThumbnailJob(const QUrl& mxcUri, QSize requestedSize) -- cgit v1.2.3 From 21c04d5b035cec0b6378e60acc93523f52c1c973 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 5 Jun 2020 18:09:12 +0200 Subject: BaseJob: jsonData() and prepareResult/Error() * JSON response is stored internally in BaseJob, rather than passed around virtual response handlers. This allow to lazily deserialise parts of the JSON response when the client calls for them instead of deserialising upon arrival and storing POD pieces. This is incompatible with the current generated code, so temporarily FTBFS. * BaseJob::loadFromJson() and BaseJob::takeFromJson() have been added to facilitate picking parts of the result as described above in derived job classes. * BaseJob::jsonData(), BaseJob::jsonItems() and (protected) BaseJob::reply() for direct access to the response in its various forms. * To further eliminate boilerplate code in generated job classes, a group of *ExpectedKeys() methods has been added - this allows to reflect the API definition of required response keys in a more "declarative" way, delegating validation to BaseJob. * parseReply() and parseJson() pair turns to singular prepareResult(). Thanks to all the changes above, in most cases it will not need overriding, unlike before. * BaseJob::Private::parseJson() is introduced, to wrap QJsonDocument::parseJson() into something less verbose. This serves a completely different purpose to the former BaseJob::parseJson(). * BaseJob::doCheckReply() takes the place, and the name, of checkReply(). --- lib/jobs/basejob.cpp | 164 ++++++++++++++++++++++++++++++----------- lib/jobs/basejob.h | 134 +++++++++++++++++++++++---------- lib/jobs/downloadfilejob.cpp | 4 +- lib/jobs/downloadfilejob.h | 4 +- lib/jobs/mediathumbnailjob.cpp | 6 +- lib/jobs/mediathumbnailjob.h | 2 +- lib/jobs/syncjob.cpp | 4 +- lib/jobs/syncjob.h | 2 +- 8 files changed, 226 insertions(+), 94 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 2519713e..3978dbcb 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -19,12 +19,11 @@ #include "basejob.h" #include "connectiondata.h" -#include "util.h" -#include #include #include #include +#include #include #include #include @@ -37,6 +36,7 @@ using namespace std::chrono_literals; BaseJob::StatusCode BaseJob::Status::fromHttpCode(int httpCode) { + // Based on https://en.wikipedia.org/wiki/List_of_HTTP_status_codes if (httpCode / 10 == 41) // 41x errors return httpCode == 410 ? IncorrectRequestError : NotFoundError; switch (httpCode) { @@ -113,6 +113,13 @@ public: } void sendRequest(); + /*! \brief Parse the response byte array into JSON + * + * This calls QJsonDocument::fromJson() on rawResponse, converts + * the QJsonParseError result to BaseJob::Status and stores the resulting + * JSON in jsonResponse. + */ + Status parseJson(); ConnectionData* connection = nullptr; @@ -131,9 +138,17 @@ public: // type QMimeType is of little help with MIME type globs (`text/*` etc.) QByteArrayList expectedContentTypes { "application/json" }; + QByteArrayList expectedKeys; + QScopedPointer reply; Status status = Unprepared; QByteArray rawResponse; + /// Contains a null document in case of non-JSON body (for a successful + /// or unsuccessful response); a document with QJsonObject or QJsonArray + /// in case of a successful response with JSON payload, as per the API + /// definition (including an empty JSON object - QJsonObject{}); + /// and QJsonObject in case of an API error. + QJsonDocument jsonResponse; QUrl errorUrl; //< May contain a URL to help with some errors LoggingCategory logCat = JOBS; @@ -243,6 +258,19 @@ void BaseJob::setExpectedContentTypes(const QByteArrayList& contentTypes) d->expectedContentTypes = contentTypes; } +const QByteArrayList BaseJob::expectedKeys() const { return d->expectedKeys; } + +void BaseJob::addExpectedKey(const QByteArray& key) { d->expectedKeys << key; } + +void BaseJob::setExpectedKeys(const QByteArrayList& keys) +{ + d->expectedKeys = keys; +} + +const QNetworkReply* BaseJob::reply() const { return d->reply.data(); } + +QNetworkReply* BaseJob::reply() { return d->reply.data(); } + QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QString& path, const QUrlQuery& query) { @@ -304,7 +332,7 @@ void BaseJob::doPrepare() { } void BaseJob::onSentRequest(QNetworkReply*) { } -void BaseJob::beforeAbandon(QNetworkReply*) { } +void BaseJob::beforeAbandon() { } void BaseJob::initiate(ConnectionData* connData, bool inBackground) { @@ -346,40 +374,74 @@ void BaseJob::sendRequest() emit aboutToSendRequest(); d->sendRequest(); Q_ASSERT(d->reply); - connect(d->reply.data(), &QNetworkReply::finished, this, &BaseJob::gotReply); + connect(reply(), &QNetworkReply::finished, this, [this] { + gotReply(); + finishJob(); + }); if (d->reply->isRunning()) { - connect(d->reply.data(), &QNetworkReply::metaDataChanged, this, - &BaseJob::checkReply); - connect(d->reply.data(), &QNetworkReply::uploadProgress, this, + connect(reply(), &QNetworkReply::metaDataChanged, this, + [this] { checkReply(reply()); }); + connect(reply(), &QNetworkReply::uploadProgress, this, &BaseJob::uploadProgress); - connect(d->reply.data(), &QNetworkReply::downloadProgress, this, + connect(reply(), &QNetworkReply::downloadProgress, this, &BaseJob::downloadProgress); d->timer.start(getCurrentTimeout()); qCInfo(d->logCat).noquote() << "Sent" << d->dumpRequest(); - onSentRequest(d->reply.data()); + onSentRequest(reply()); emit sentRequest(); } else qCCritical(d->logCat).noquote() << "Request could not start:" << d->dumpRequest(); } +BaseJob::Status BaseJob::Private::parseJson() +{ + QJsonParseError error { 0, QJsonParseError::MissingObject }; + jsonResponse = QJsonDocument::fromJson(rawResponse, &error); + return { error.error == QJsonParseError::NoError + ? BaseJob::NoError + : BaseJob::IncorrectResponse, + error.errorString() }; +} + void BaseJob::gotReply() { - checkReply(); + setStatus(checkReply(reply())); + + if (status().good() + && d->expectedContentTypes == QByteArrayList { "application/json" }) { + d->rawResponse = reply()->readAll(); + setStatus(d->parseJson()); + if (status().good() && !expectedKeys().empty()) { + const auto& responseObject = jsonData(); + QByteArrayList missingKeys; + for (const auto& k: expectedKeys()) + if (!responseObject.contains(k)) + missingKeys.push_back(k); + if (!missingKeys.empty()) + setStatus(IncorrectResponse, tr("Required JSON keys missing: ") + + missingKeys.join()); + } + if (!status().good()) // Bad JSON in a "good" reply: bail out + return; + } // else { + // If the endpoint expects anything else than just (API-related) JSON + // reply()->readAll() is not performed and the whole reply processing + // is left to derived job classes: they may read it piecemeal or customise + // per content type in prepareResult(), or even have read it already + // (see, e.g., DownloadFileJob). + // } + if (status().good()) - setStatus(parseReply(d->reply.data())); + setStatus(prepareResult()); else { - d->rawResponse = d->reply->readAll(); - const auto jsonBody = d->reply->rawHeader("Content-Type") - == "application/json"; + d->rawResponse = reply()->readAll(); qCDebug(d->logCat).noquote() - << "Error body (truncated if long):" << d->rawResponse.left(500); - if (jsonBody) - setStatus( - parseError(d->reply.data(), - QJsonDocument::fromJson(d->rawResponse).object())); + << "Error body (truncated if long):" << rawDataSample(500); + // Parse the error payload and update the status if needed + if (const auto newStatus = prepareError(); !newStatus.good()) + setStatus(newStatus); } - finishJob(); } bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) @@ -408,12 +470,10 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) return false; } -BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const +BaseJob::Status BaseJob::checkReply(const QNetworkReply* reply) const { - // QNetworkReply error codes seem to be flawed when it comes to HTTP; - // see, e.g., https://github.com/quotient-im/libQuotient/issues/200 - // so check genuine HTTP codes. The below processing is based on - // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes + // QNetworkReply error codes are insufficient for our purposes (e.g. they + // don't allow to discern HTTP code 429) so check the original code instead const auto httpCodeHeader = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); if (!httpCodeHeader.isValid()) { @@ -444,24 +504,23 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const return Status::fromHttpCode(httpCode, message); } -void BaseJob::checkReply() { setStatus(doCheckReply(d->reply.data())); } +BaseJob::Status BaseJob::prepareResult() { return Success; } -BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) +BaseJob::Status BaseJob::prepareError() { - d->rawResponse = reply->readAll(); - QJsonParseError error { 0, QJsonParseError::MissingObject }; - const auto& json = QJsonDocument::fromJson(d->rawResponse, &error); - if (error.error == QJsonParseError::NoError) - return parseJson(json); + // Since it's an error, the expected content type is of no help; + // check the actually advertised content type instead + if (reply()->rawHeader("Content-Type") != "application/json") + return NoError; // Retain the status if the error payload is not JSON - return { IncorrectResponseError, error.errorString() }; -} + if (const auto status = d->parseJson(); !status.good()) + return status; -BaseJob::Status BaseJob::parseJson(const QJsonDocument&) { return Success; } + if (d->jsonResponse.isArray()) + return { IncorrectResponse, + tr("Malformed error JSON: an array instead of an object") }; -BaseJob::Status BaseJob::parseError(QNetworkReply* /*reply*/, - const QJsonObject& errorJson) -{ + const auto& errorJson = jsonData(); const auto errCode = errorJson.value("errcode"_ls).toString(); if (error() == TooManyRequestsError || errCode == "M_LIMIT_EXCEEDED") { QString msg = tr("Too many requests"); @@ -499,6 +558,16 @@ BaseJob::Status BaseJob::parseError(QNetworkReply* /*reply*/, return d->status; } +QJsonValue BaseJob::takeValueFromJson(const QString& key) +{ + if (!d->jsonResponse.isObject()) + return QJsonValue::Undefined; + auto o = d->jsonResponse.object(); + auto v = o.take(key); + d->jsonResponse.setObject(o); + return v; +} + void BaseJob::stop() { // This method is (also) used to semi-finalise the job before retrying; so @@ -616,10 +685,19 @@ QString BaseJob::rawDataSample(int bytesAtMost) const Q_ASSERT(data.size() <= d->rawResponse.size()); return data.size() == d->rawResponse.size() ? data - : data - + tr("...(truncated, %Ln bytes in total)", - "Comes after trimmed raw network response", - d->rawResponse.size()); + : data + tr("...(truncated, %Ln bytes in total)", + "Comes after trimmed raw network response", + d->rawResponse.size()); +} + +QJsonObject BaseJob::jsonData() const +{ + return d->jsonResponse.object(); +} + +QJsonArray BaseJob::jsonItems() const +{ + return d->jsonResponse.array(); } QString BaseJob::statusCaption() const @@ -704,7 +782,7 @@ void BaseJob::setStatus(int code, QString message) void BaseJob::abandon() { - beforeAbandon(d->reply ? d->reply.data() : nullptr); + beforeAbandon(); d->timer.stop(); d->retryTimer.stop(); // In case abandon() was called between retries setStatus(Abandoned); diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index 6920ebc1..be2926be 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -18,13 +18,11 @@ #pragma once -#include "../logging.h" #include "requestdata.h" +#include "../logging.h" +#include "../converters.h" -#include #include -#include -#include class QNetworkReply; class QSslError; @@ -181,6 +179,49 @@ public: */ QString rawDataSample(int bytesAtMost = 65535) const; + /** Get the response body as a JSON object + * + * If the job's returned content type is not `application/json` + * or if the top-level JSON entity is not an object, an empty object + * is returned. + */ + QJsonObject jsonData() const; + + /** Get the response body as a JSON array + * + * If the job's returned content type is not `application/json` + * or if the top-level JSON entity is not an array, an empty array + * is returned. + */ + QJsonArray jsonItems() const; + + /** Load the property from the JSON response assuming a given C++ type + * + * If there's no top-level JSON object in the response or if there's + * no node with the key \p keyName, \p defaultValue is returned. + */ + template // Waiting for QStringViews... + T loadFromJson(const StrT& keyName, T&& defaultValue = {}) const + { + const auto& jv = jsonData().value(keyName); + return jv.isUndefined() ? std::forward(defaultValue) + : fromJson(jv); + } + + /** Load the property from the JSON response and delete it from JSON + * + * If there's no top-level JSON object in the response or if there's + * no node with the key \p keyName, \p defaultValue is returned. + */ + template + T takeFromJson(const QString& key, T&& defaultValue = {}) + { + if (const auto& jv = takeValueFromJson(key); !jv.isUndefined()) + return fromJson(jv); + + return std::forward(defaultValue); + } + /** Error (more generally, status) code * Equivalent to status().code * \sa status @@ -314,6 +355,12 @@ protected: const QByteArrayList& expectedContentTypes() const; void addExpectedContentType(const QByteArray& contentType); void setExpectedContentTypes(const QByteArrayList& contentTypes); + const QByteArrayList expectedKeys() const; + void addExpectedKey(const QByteArray &key); + void setExpectedKeys(const QByteArrayList &keys); + + const QNetworkReply* reply() const; + QNetworkReply* reply(); /** Construct a URL out of baseUrl, path and query * @@ -338,50 +385,42 @@ protected: * successfully sending a network request (including retries). */ virtual void onSentRequest(QNetworkReply*); - virtual void beforeAbandon(QNetworkReply*); + virtual void beforeAbandon(); - /*! \brief Check the pending or received reply for upfront issues + /*! \brief An extension point for additional reply processing. * - * This is invoked when headers are first received and also once - * the complete reply is obtained; the base implementation checks the HTTP - * headers to detect general issues such as network errors or access denial. - * It cannot read the response body (use parseReply/parseError to check - * for problems in the body). Returning anything except NoError/Success - * prevents further processing of the reply. - * - * @return the result of checking the reply + * The base implementation does nothing and returns Success. * - * @see gotReply + * \sa gotReply */ - virtual Status doCheckReply(QNetworkReply* reply) const; + virtual Status prepareResult(); - /** - * Processes the reply. By default, parses the reply into - * a QJsonDocument and calls parseJson() if it's a valid JSON. - * - * @param reply raw contents of a HTTP reply from the server + /*! \brief Process details of the error * - * @see gotReply, parseJson + * The function processes the reply in case when status from checkReply() + * was not good (usually because of an unsuccessful HTTP code). + * The base implementation assumes Matrix JSON error object in the body; + * overrides are strongly recommended to call it for all stock Matrix + * responses as early as possible but in addition can process custom errors, + * with JSON or non-JSON payload. */ - virtual Status parseReply(QNetworkReply* reply); + virtual Status prepareError(); - /** - * Processes the JSON document received from the Matrix server. - * By default returns successful status without analysing the JSON. + /*! \brief Get direct access to the JSON response object in the job * - * @param json valid JSON document received from the server + * This allows to implement deserialisation with "move" semantics for parts + * of the response. Assuming that the response body is a valid JSON object, + * the function calls QJsonObject::take(key) on it and returns the result. * - * @see parseReply - */ - virtual Status parseJson(const QJsonDocument&); - - /** - * Processes the reply in case of unsuccessful HTTP code. - * The body is already loaded from the reply object to errorJson. - * @param reply the HTTP reply from the server - * @param errorJson the JSON payload describing the error + * \return QJsonValue::Null, if the response content type is not + * advertised as `application/json`; + * QJsonValue::Undefined, if the response is a JSON object but + * doesn't have \p key; + * the value for \p key otherwise. + * + * \sa takeFromJson */ - virtual Status parseError(QNetworkReply*, const QJsonObject& errorJson); + QJsonValue takeValueFromJson(const QString& key); void setStatus(Status s); void setStatus(int code, QString message); @@ -397,9 +436,28 @@ protected: protected slots: void timeout(); + /*! \brief Check the pending or received reply for upfront issues + * + * This is invoked when headers are first received and also once + * the complete reply is obtained; the base implementation checks the HTTP + * headers to detect general issues such as network errors or access denial + * and it's strongly recommended to call it from overrides, + * as early as possible. + * This slot is const and cannot read the response body. If you need to read + * the body on the fly, override onSentRequest() and connect in it + * to reply->readyRead(); and if you only need to validate the body after + * it fully arrived, use prepareResult() for that). Returning anything + * except NoError/Success switches further processing from prepareResult() + * to prepareError(). + * + * @return the result of checking the reply + * + * @see gotReply + */ + virtual Status checkReply(const QNetworkReply *reply) const; + private slots: void sendRequest(); - void checkReply(); void gotReply(); friend class ConnectionData; // to provide access to sendRequest() diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 3e037680..7b4cf690 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -86,14 +86,14 @@ void DownloadFileJob::onSentRequest(QNetworkReply* reply) }); } -void DownloadFileJob::beforeAbandon(QNetworkReply*) +void DownloadFileJob::beforeAbandon() { if (d->targetFile) d->targetFile->remove(); d->tempFile->remove(); } -BaseJob::Status DownloadFileJob::parseReply(QNetworkReply*) +BaseJob::Status DownloadFileJob::prepareResult() { if (d->targetFile) { d->targetFile->close(); diff --git a/lib/jobs/downloadfilejob.h b/lib/jobs/downloadfilejob.h index 06dc145c..e00fd9e4 100644 --- a/lib/jobs/downloadfilejob.h +++ b/lib/jobs/downloadfilejob.h @@ -19,7 +19,7 @@ private: void doPrepare() override; void onSentRequest(QNetworkReply* reply) override; - void beforeAbandon(QNetworkReply*) override; - Status parseReply(QNetworkReply*) override; + void beforeAbandon() override; + Status prepareResult() override; }; } // namespace Quotient diff --git a/lib/jobs/mediathumbnailjob.cpp b/lib/jobs/mediathumbnailjob.cpp index df3763b2..33f4236c 100644 --- a/lib/jobs/mediathumbnailjob.cpp +++ b/lib/jobs/mediathumbnailjob.cpp @@ -48,12 +48,8 @@ QImage MediaThumbnailJob::scaledThumbnail(QSize toSize) const Qt::SmoothTransformation); } -BaseJob::Status MediaThumbnailJob::parseReply(QNetworkReply* reply) +BaseJob::Status MediaThumbnailJob::prepareResult() { - auto result = GetContentThumbnailJob::parseReply(reply); - if (!result.good()) - return result; - if (_thumbnail.loadFromData(data()->readAll())) return Success; diff --git a/lib/jobs/mediathumbnailjob.h b/lib/jobs/mediathumbnailjob.h index 75e2e55a..e6d39085 100644 --- a/lib/jobs/mediathumbnailjob.h +++ b/lib/jobs/mediathumbnailjob.h @@ -37,7 +37,7 @@ public: QImage scaledThumbnail(QSize toSize) const; protected: - Status parseReply(QNetworkReply* reply) override; + Status prepareResult() override; private: QImage _thumbnail; diff --git a/lib/jobs/syncjob.cpp b/lib/jobs/syncjob.cpp index cd7709e1..6b8cfe4b 100644 --- a/lib/jobs/syncjob.cpp +++ b/lib/jobs/syncjob.cpp @@ -49,9 +49,9 @@ SyncJob::SyncJob(const QString& since, const Filter& filter, int timeout, timeout, presence) {} -BaseJob::Status SyncJob::parseJson(const QJsonDocument& data) +BaseJob::Status SyncJob::prepareResult() { - d.parseJson(data.object()); + d.parseJson(jsonData()); if (d.unresolvedRooms().isEmpty()) return BaseJob::Success; diff --git a/lib/jobs/syncjob.h b/lib/jobs/syncjob.h index df419ba8..bf139a7b 100644 --- a/lib/jobs/syncjob.h +++ b/lib/jobs/syncjob.h @@ -33,7 +33,7 @@ public: SyncData&& takeData() { return std::move(d); } protected: - Status parseJson(const QJsonDocument& data) override; + Status prepareResult() override; private: SyncData d; -- cgit v1.2.3 From 0db2db1d247a0a4e6e6545ec0d72879416e6226b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 11 Jun 2020 19:24:39 +0200 Subject: BaseJob::prepareError(): be more tolerant to empty error payloads TooManyRequests can come without a payload, apparently. --- lib/jobs/basejob.cpp | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 3978dbcb..d7daa170 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -508,17 +508,13 @@ BaseJob::Status BaseJob::prepareResult() { return Success; } BaseJob::Status BaseJob::prepareError() { - // Since it's an error, the expected content type is of no help; - // check the actually advertised content type instead - if (reply()->rawHeader("Content-Type") != "application/json") - return NoError; // Retain the status if the error payload is not JSON - - if (const auto status = d->parseJson(); !status.good()) - return status; - - if (d->jsonResponse.isArray()) - return { IncorrectResponse, - tr("Malformed error JSON: an array instead of an object") }; + if (!d->rawResponse.isEmpty()) { + if (const auto status = d->parseJson(); !status.good()) + return status; // If there's anything there, it should be JSON + if (d->jsonResponse.isArray()) // ...specifically, a JSON object + return { IncorrectResponse, + tr("Malformed error JSON: an array instead of an object") }; + } const auto& errorJson = jsonData(); const auto errCode = errorJson.value("errcode"_ls).toString(); @@ -534,6 +530,7 @@ BaseJob::Status BaseJob::prepareError() return { TooManyRequestsError, msg }; } + if (errCode == "M_CONSENT_NOT_GIVEN") { d->errorUrl = errorJson.value("consent_uri"_ls).toString(); return { UserConsentRequiredError }; @@ -552,10 +549,10 @@ BaseJob::Status BaseJob::prepareError() return { UserDeactivated }; // Not localisable on the client side - if (errorJson.contains("error"_ls)) - d->status.message = errorJson.value("error"_ls).toString(); + if (errorJson.contains("error"_ls)) // Keep the code, update the message + return { d->status.code, errorJson.value("error"_ls).toString() }; - return d->status; + return NoError; // Retain the status if the error payload is not recognised } QJsonValue BaseJob::takeValueFromJson(const QString& key) -- cgit v1.2.3 From ebdb2ba9d15e6cdfb1458e7895032afd641aafe3 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 12 Jun 2020 20:20:10 +0200 Subject: BaseJob: fail early if the job needs token and there's none --- lib/jobs/basejob.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index d7daa170..7774a6e2 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -341,7 +341,9 @@ void BaseJob::initiate(ConnectionData* connData, bool inBackground) d->connection = connData; doPrepare(); - if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) + if (d->needsToken && d->connection->accessToken().isEmpty()) + setStatus(Unauthorised); + else if ((d->verb == HttpVerb::Post || d->verb == HttpVerb::Put) && d->requestData.source() && !d->requestData.source()->isReadable()) { setStatus(FileError, "Request data not ready"); -- cgit v1.2.3 From b1e84568dbb70dc4fa24a915c6d15438be958378 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 26 Jun 2020 19:05:36 +0200 Subject: Disable HTTP2; enable pipelining Long run tests over 2+ days kept crashing before this commit but stopped crashing with pipelining on and HTTP2 off. --- lib/jobs/basejob.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 7774a6e2..465287c6 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -298,16 +298,17 @@ void BaseJob::Private::sendRequest() req.setAttribute(QNetworkRequest::BackgroundRequestAttribute, inBackground); req.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); req.setMaximumRedirectsAllowed(10); - // Pipelining doesn't fly quite well with SSL, occasionally crashing at - // what seems like an attempt to write to a closed channel. -// req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); + req.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true); req.setAttribute( #if (QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)) QNetworkRequest::Http2AllowedAttribute #else QNetworkRequest::HTTP2AllowedAttribute #endif - , true); + // Qt doesn't combine HTTP2 with SSL quite right, occasionally crashing at + // what seems like an attempt to write to a closed channel. If/when that + // changes, false should be turned to true below. + , false); Q_ASSERT(req.url().isValid()); for (auto it = requestHeaders.cbegin(); it != requestHeaders.cend(); ++it) req.setRawHeader(it.key(), it.value()); -- cgit v1.2.3 From 3648c6a40c585e718bcfc0068cfa431e46f24a52 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 4 Aug 2020 13:28:09 +0200 Subject: Cleanup around [BaseJob::]IncorrectResponse[Error] --- lib/jobs/basejob.cpp | 5 ++--- lib/jobs/mediathumbnailjob.cpp | 3 +-- lib/jobs/syncjob.cpp | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 465287c6..d6d48d2f 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -401,9 +401,8 @@ BaseJob::Status BaseJob::Private::parseJson() { QJsonParseError error { 0, QJsonParseError::MissingObject }; jsonResponse = QJsonDocument::fromJson(rawResponse, &error); - return { error.error == QJsonParseError::NoError - ? BaseJob::NoError - : BaseJob::IncorrectResponse, + return { error.error == QJsonParseError::NoError ? NoError + : IncorrectResponse, error.errorString() }; } diff --git a/lib/jobs/mediathumbnailjob.cpp b/lib/jobs/mediathumbnailjob.cpp index 33f4236c..a69f00e9 100644 --- a/lib/jobs/mediathumbnailjob.cpp +++ b/lib/jobs/mediathumbnailjob.cpp @@ -53,6 +53,5 @@ BaseJob::Status MediaThumbnailJob::prepareResult() if (_thumbnail.loadFromData(data()->readAll())) return Success; - return { IncorrectResponseError, - QStringLiteral("Could not read image data") }; + return { IncorrectResponse, QStringLiteral("Could not read image data") }; } diff --git a/lib/jobs/syncjob.cpp b/lib/jobs/syncjob.cpp index 6b8cfe4b..9087fe50 100644 --- a/lib/jobs/syncjob.cpp +++ b/lib/jobs/syncjob.cpp @@ -53,9 +53,9 @@ BaseJob::Status SyncJob::prepareResult() { d.parseJson(jsonData()); if (d.unresolvedRooms().isEmpty()) - return BaseJob::Success; + return Success; qCCritical(MAIN).noquote() << "Incomplete sync response, missing rooms:" << d.unresolvedRooms().join(','); - return BaseJob::IncorrectResponseError; + return IncorrectResponse; } -- cgit v1.2.3 From d88663890261c2c276cd76576655b137a8758670 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 4 Aug 2020 13:32:57 +0200 Subject: BaseJob: go for a retry on IncorrectResponse The most frequent occurence of IncorrectResponse so far is a proxy/CDN failure. This is not a grave error; there's a chance that the retry will succeed. In the worst case the job will fail after 3 identical errors (except SyncJob that will try to get through forever - but SyncJob failures should still be indicated in the client's UI in some non-intrusive way). --- lib/jobs/basejob.cpp | 55 +++++++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 24 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index d6d48d2f..1f0e84ba 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -586,33 +586,40 @@ void BaseJob::stop() void BaseJob::finishJob() { stop(); - if (error() == TooManyRequests) { + switch(error()) { + case TooManyRequests: emit rateLimited(); d->connection->submit(this); return; - } - if (error() == Unauthorised && !d->needsToken - && !d->connection->accessToken().isEmpty()) { - // Rerun with access token (extension of the spec while - // https://github.com/matrix-org/matrix-doc/issues/701 is pending) - d->connection->setNeedsToken(objectName()); - qCWarning(d->logCat) << this << "re-running with authentication"; - emit retryScheduled(d->retriesTaken, 0); - d->connection->submit(this); - return; - } - if ((error() == NetworkError || error() == Timeout) - && d->retriesTaken < d->maxRetries) { - // TODO: The whole retrying thing should be put to Connection(Manager) - // otherwise independently retrying jobs make a bit of notification - // storm towards the UI. - const seconds retryIn = error() == Timeout ? 0s : getNextRetryInterval(); - ++d->retriesTaken; - qCWarning(d->logCat).nospace() << this << ": retry #" << d->retriesTaken - << " in " << retryIn.count() << " s"; - d->retryTimer.start(retryIn); - emit retryScheduled(d->retriesTaken, milliseconds(retryIn).count()); - return; + case Unauthorised: + if (!d->needsToken && !d->connection->accessToken().isEmpty()) { + // Rerun with access token (extension of the spec while + // https://github.com/matrix-org/matrix-doc/issues/701 is pending) + d->connection->setNeedsToken(objectName()); + qCWarning(d->logCat) << this << "re-running with authentication"; + emit retryScheduled(d->retriesTaken, 0); + d->connection->submit(this); + return; + } + break; + case NetworkError: + case IncorrectResponse: + case Timeout: + if (d->retriesTaken < d->maxRetries) { + // TODO: The whole retrying thing should be put to + // Connection(Manager) otherwise independently retrying jobs make a + // bit of notification storm towards the UI. + const seconds retryIn = error() == Timeout ? 0s + : getNextRetryInterval(); + ++d->retriesTaken; + qCWarning(d->logCat).nospace() + << this << ": retry #" << d->retriesTaken << " in " + << retryIn.count() << " s"; + d->retryTimer.start(retryIn); + emit retryScheduled(d->retriesTaken, milliseconds(retryIn).count()); + return; + } + default:; } Q_ASSERT(status().code != Pending); -- cgit v1.2.3 From ff2cb9f8a3a3ba01c9881ee1b196c9397c91ee6d Mon Sep 17 00:00:00 2001 From: Tobias Fella Date: Thu, 10 Dec 2020 01:53:04 +0100 Subject: Fix DELETE jobs with json data DeleteDeviceJob requires authentication, but the JSON data is not added for DELETE requests. Since QNetworkAccessManager::deleteResource does not support body data, we need to send a custom request. --- lib/jobs/basejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 1f0e84ba..b757854a 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -324,7 +324,7 @@ void BaseJob::Private::sendRequest() reply.reset(connection->nam()->put(req, requestData.source())); break; case HttpVerb::Delete: - reply.reset(connection->nam()->deleteResource(req)); + reply.reset(connection->nam()->sendCustomRequest(req, "DELETE", requestData.source())); break; } } -- cgit v1.2.3 From 9ef83e044ed4f8409156b19d529dfc7e45f565c1 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 23 Dec 2020 17:21:01 +0100 Subject: BaseJob: tolerate unexpected error payloads Proxy servers may return arbitrary HTML, for one example; so don't expect to find a valid JSON object in whatever non-empty payload next to a non-2xx HTTP code. Fixes #421. --- lib/jobs/basejob.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index b757854a..e16ba4ef 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -510,14 +510,15 @@ BaseJob::Status BaseJob::prepareResult() { return Success; } BaseJob::Status BaseJob::prepareError() { - if (!d->rawResponse.isEmpty()) { - if (const auto status = d->parseJson(); !status.good()) - return status; // If there's anything there, it should be JSON - if (d->jsonResponse.isArray()) // ...specifically, a JSON object - return { IncorrectResponse, - tr("Malformed error JSON: an array instead of an object") }; - } - + // Try to make sense of the error payload but be prepared for all kinds + // of unexpected stuff (raw HTML, plain text, foreign JSON among those) + if (!d->rawResponse.isEmpty() + && reply()->rawHeader("Content-Type") == "application/json") + d->parseJson(); + + // By now, if d->parseJson() above succeeded then jsonData() will return + // a valid JSON object - or an empty object otherwise (in which case most + // of if's below will fall through to `return NoError` at the end const auto& errorJson = jsonData(); const auto errCode = errorJson.value("errcode"_ls).toString(); if (error() == TooManyRequestsError || errCode == "M_LIMIT_EXCEEDED") { -- cgit v1.2.3 From 1a832ae9b6a0d679b551fd644136e4bc17e7db29 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 23 Dec 2020 17:21:44 +0100 Subject: BaseJob: add [[fallthrough]] as clang-tidy says --- lib/jobs/basejob.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index e16ba4ef..3fa1cd94 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -620,6 +620,7 @@ void BaseJob::finishJob() emit retryScheduled(d->retriesTaken, milliseconds(retryIn).count()); return; } + [[fallthrough]]; default:; } -- cgit v1.2.3 From 0a2acd750a4155969092be674ed3dd9a71b2354f Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 24 Dec 2020 18:20:04 +0100 Subject: Fix clang-tidy/clazy warnings --- lib/jobs/downloadfilejob.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/downloadfilejob.cpp b/lib/jobs/downloadfilejob.cpp index 7b4cf690..0011a97c 100644 --- a/lib/jobs/downloadfilejob.cpp +++ b/lib/jobs/downloadfilejob.cpp @@ -64,7 +64,7 @@ void DownloadFileJob::onSentRequest(QNetworkReply* reply) return; auto sizeHeader = reply->header(QNetworkRequest::ContentLengthHeader); if (sizeHeader.isValid()) { - auto targetSize = sizeHeader.value(); + auto targetSize = sizeHeader.toLongLong(); if (targetSize != -1) if (!d->tempFile->resize(targetSize)) { qCWarning(JOBS) << "Failed to allocate" << targetSize -- cgit v1.2.3 From f286ef4c5b3c71510d6ef15e8cc12cada84f3682 Mon Sep 17 00:00:00 2001 From: Nicolas Fella Date: Sun, 27 Dec 2020 21:24:06 +0100 Subject: Fix use-after-free of QNetworkReply in BaseJob Usually QNetworkAccessManager expects the user to delete the replies, but when the QNetworkAccessManager itself is deleted it deletes all pending replies (https://code.woboq.org/qt5/qtbase/src/network/access/qnetworkaccessmanager.cpp.html#529). This can lead to use-after-free crashes when d->reply is accessed. By putting the reply into a QPointer the exiting if(d->reply) checks can work properly. (cherry picked from commit 9d854e778d8d6ef8e03e1ea74fe958675b24fd45) --- lib/jobs/basejob.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 3fa1cd94..2ac942f5 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -76,15 +77,6 @@ QDebug BaseJob::Status::dumpToLog(QDebug dbg) const return dbg << ": " << message; } -struct NetworkReplyDeleter : public QScopedPointerDeleteLater { - static inline void cleanup(QNetworkReply* reply) - { - if (reply && reply->isRunning()) - reply->abort(); - QScopedPointerDeleteLater::cleanup(reply); - } -}; - template constexpr auto make_array(Ts&&... items) { @@ -112,6 +104,16 @@ public: retryTimer.setSingleShot(true); } + ~Private() + { + if (reply) { + if (reply->isRunning()) { + reply->abort(); + } + delete reply; + } + } + void sendRequest(); /*! \brief Parse the response byte array into JSON * @@ -140,7 +142,10 @@ public: QByteArrayList expectedKeys; - QScopedPointer reply; + // When the QNetworkAccessManager is destroyed it destroys all pending replies. + // Using QPointer allows us to know when that happend. + QPointer reply; + Status status = Unprepared; QByteArray rawResponse; /// Contains a null document in case of non-JSON body (for a successful @@ -315,16 +320,16 @@ void BaseJob::Private::sendRequest() switch (verb) { case HttpVerb::Get: - reply.reset(connection->nam()->get(req)); + reply = connection->nam()->get(req); break; case HttpVerb::Post: - reply.reset(connection->nam()->post(req, requestData.source())); + reply = connection->nam()->post(req, requestData.source()); break; case HttpVerb::Put: - reply.reset(connection->nam()->put(req, requestData.source())); + reply = connection->nam()->put(req, requestData.source()); break; case HttpVerb::Delete: - reply.reset(connection->nam()->sendCustomRequest(req, "DELETE", requestData.source())); + reply = connection->nam()->sendCustomRequest(req, "DELETE", requestData.source()); break; } } -- cgit v1.2.3 From 12c37cf3ce2aa397050fff6ce846d557524a3fe7 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 27 Dec 2020 23:32:18 +0100 Subject: BaseJob::initiate: add Q_LIKELY ...to show the sunny-day case. (cherry picked from commit 5d15e3b23649a54abdb3812c10f4a7d2ce07d7dd) --- lib/jobs/basejob.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 2ac942f5..135274d6 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -342,7 +342,7 @@ void BaseJob::beforeAbandon() { } void BaseJob::initiate(ConnectionData* connData, bool inBackground) { - if (connData && connData->baseUrl().isValid()) { + if (Q_LIKELY(connData && connData->baseUrl().isValid())) { d->inBackground = inBackground; d->connection = connData; doPrepare(); @@ -355,7 +355,7 @@ void BaseJob::initiate(ConnectionData* connData, bool inBackground) setStatus(FileError, "Request data not ready"); } Q_ASSERT(status().code != Pending); // doPrepare() must NOT set this - if (status().code == Unprepared) { + if (Q_LIKELY(status().code == Unprepared)) { d->connection->submit(this); return; } -- cgit v1.2.3 From 4f06d46d6d6062d6d17f69eeaddb7810edac5bbf Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 7 Jan 2021 21:15:28 +0100 Subject: BaseJob: more logging --- lib/jobs/basejob.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 135274d6..da66a9f2 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -199,6 +199,7 @@ BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint, setObjectName(name); connect(&d->timer, &QTimer::timeout, this, &BaseJob::timeout); connect(&d->retryTimer, &QTimer::timeout, this, [this] { + qCDebug(d->logCat) << "Retrying" << this; d->connection->submit(this); }); } @@ -374,8 +375,11 @@ void BaseJob::initiate(ConnectionData* connData, bool inBackground) void BaseJob::sendRequest() { - if (status().code == Abandoned) + if (status().code == Abandoned) { + qCDebug(d->logCat) << "Won't proceed with the abandoned request:" + << d->dumpRequest(); return; + } Q_ASSERT(d->connection && status().code == Pending); qCDebug(d->logCat).noquote() << "Making" << d->dumpRequest(); d->needsToken |= d->connection->needsToken(objectName()); -- cgit v1.2.3 From 12e00b234e5c5f4ed57b5c400d06f780e71014f4 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 7 Jan 2021 21:18:01 +0100 Subject: BaseJob: setStatus(Pending) on scheduling a retry Fixes #437. --- lib/jobs/basejob.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'lib/jobs') diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index da66a9f2..5960203d 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -625,6 +625,7 @@ void BaseJob::finishJob() qCWarning(d->logCat).nospace() << this << ": retry #" << d->retriesTaken << " in " << retryIn.count() << " s"; + setStatus(Pending, "Pending retry"); d->retryTimer.start(retryIn); emit retryScheduled(d->retriesTaken, milliseconds(retryIn).count()); return; -- cgit v1.2.3