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