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().

8 files changed, 226 insertions, 94 deletions

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 <QtCore/QJsonObject>
 #include <QtCore/QRegularExpression>
 #include <QtCore/QTimer>
 #include <QtCore/QStringBuilder>
+#include <QtCore/QMetaEnum>
 #include <QtNetwork/QNetworkAccessManager>
 #include <QtNetwork/QNetworkReply>
 #include <QtNetwork/QNetworkRequest>
@@ -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<QNetworkReply, NetworkReplyDeleter> 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 <QtCore/QJsonDocument>
 #include <QtCore/QObject>
-#include <QtCore/QUrlQuery>
-#include <QtCore/QMetaEnum>
 
 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 <typename T, typename StrT> // Waiting for QStringViews...
+    T loadFromJson(const StrT& keyName, T&& defaultValue = {}) const
+    {
+        const auto& jv = jsonData().value(keyName);
+        return jv.isUndefined() ? std::forward<T>(defaultValue)
+                                : fromJson<T>(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 <typename T>
+    T takeFromJson(const QString& key, T&& defaultValue = {})
+    {
+        if (const auto& jv = takeValueFromJson(key); !jv.isUndefined())
+            return fromJson<T>(jv);
+
+        return std::forward<T>(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;