Move network error handling from NetworkTask into WebTask

...and provide the response content if available.

9 files changed, 162 insertions, 119 deletions

diff --git a/src/musicbrainz/tagfetcher.cpp b/src/musicbrainz/tagfetcher.cpp
index 64f5ac4309..8da4e79ef1 100644
--- a/src/musicbrainz/tagfetcher.cpp
+++ b/src/musicbrainz/tagfetcher.cpp
@@ -189,10 +189,10 @@ void TagFetcher::slotAcoustIdTaskAborted() {
 }
 
 void TagFetcher::slotAcoustIdTaskNetworkError(
-        const QUrl& requestUrl,
         QNetworkReply::NetworkError errorCode,
-        const QString& errorString) {
-    Q_UNUSED(requestUrl);
+        const QString& errorString,
+        const mixxx::network::WebResponseWithContent& responseWithContent) {
+    Q_UNUSED(responseWithContent);
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     if (!onAcoustIdTaskTerminated()) {
         return;
@@ -231,10 +231,10 @@ void TagFetcher::slotMusicBrainzTaskAborted() {
 }
 
 void TagFetcher::slotMusicBrainzTaskNetworkError(
-        const QUrl& requestUrl,
         QNetworkReply::NetworkError errorCode,
-        const QString& errorString) {
-    Q_UNUSED(requestUrl);
+        const QString& errorString,
+        const mixxx::network::WebResponseWithContent& responseWithContent) {
+    Q_UNUSED(responseWithContent);
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     if (!onMusicBrainzTaskTerminated()) {
         return;
diff --git a/src/musicbrainz/tagfetcher.h b/src/musicbrainz/tagfetcher.h
index 1bbfddc156..160a854aed 100644
--- a/src/musicbrainz/tagfetcher.h
+++ b/src/musicbrainz/tagfetcher.h
@@ -48,9 +48,9 @@ class TagFetcher : public QObject {
             const mixxx::network::JsonWebResponse& response);
     void slotAcoustIdTaskAborted();
     void slotAcoustIdTaskNetworkError(
-            const QUrl& requestUrl,
             QNetworkReply::NetworkError errorCode,
-            const QString& errorString);
+            const QString& errorString,
+            const mixxx::network::WebResponseWithContent& responseWithContent);
 
     void slotMusicBrainzTaskSucceeded(
             const QList<mixxx::musicbrainz::TrackRelease>& guessedTrackReleases);
@@ -60,9 +60,9 @@ class TagFetcher : public QObject {
             const QString& errorMessage);
     void slotMusicBrainzTaskAborted();
     void slotMusicBrainzTaskNetworkError(
-            const QUrl& requestUrl,
             QNetworkReply::NetworkError errorCode,
-            const QString& errorString);
+            const QString& errorString,
+            const mixxx::network::WebResponseWithContent& responseWithContent);
 
   private:
     bool onAcoustIdTaskTerminated();
diff --git a/src/musicbrainz/web/musicbrainzrecordingstask.cpp b/src/musicbrainz/web/musicbrainzrecordingstask.cpp
index 5afec1225e..3384f1630c 100644
--- a/src/musicbrainz/web/musicbrainzrecordingstask.cpp
+++ b/src/musicbrainz/web/musicbrainzrecordingstask.cpp
@@ -126,6 +126,7 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
         emitFailed(
                 network::WebResponse(
                         finishedNetworkReply->url(),
+                        finishedNetworkReply->request().url(),
                         statusCode),
                 error.code,
                 std::move(error.message));
@@ -147,6 +148,7 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
         emitFailed(
                 network::WebResponse(
                         finishedNetworkReply->url(),
+                        finishedNetworkReply->request().url(),
                         statusCode),
                 -1,
                 "Failed to parse XML response");
diff --git a/src/network/jsonwebtask.cpp b/src/network/jsonwebtask.cpp
index 690d31ef36..f33a7cea0f 100644
--- a/src/network/jsonwebtask.cpp
+++ b/src/network/jsonwebtask.cpp
@@ -37,26 +37,6 @@ const QString JSON_CONTENT_TYPE = "application/json";
 
 const QMimeType JSON_MIME_TYPE = QMimeDatabase().mimeTypeForName(JSON_CONTENT_TYPE);
 
-QMimeType readContentType(
-        const QNetworkReply* reply) {
-    DEBUG_ASSERT(reply);
-    const QVariant contentTypeHeader = reply->header(QNetworkRequest::ContentTypeHeader);
-    if (!contentTypeHeader.isValid() || contentTypeHeader.isNull()) {
-        kLogger.warning()
-                << "Missing content type header";
-        return QMimeType();
-    }
-    const QString contentTypeString = contentTypeHeader.toString();
-    const QString contentTypeWithoutParams = contentTypeString.left(contentTypeString.indexOf(';'));
-    const QMimeType contentType = QMimeDatabase().mimeTypeForName(contentTypeWithoutParams);
-    if (!contentType.isValid()) {
-        kLogger.warning()
-                << "Unknown content type"
-                << contentTypeWithoutParams;
-    }
-    return contentType;
-}
-
 /// If parsing fails the functions returns std::nullopt and optionally
 /// the response content in pInvalidResponseContent for further processing.
 std::optional<QJsonDocument> readJsonContent(
@@ -64,24 +44,28 @@ std::optional<QJsonDocument> readJsonContent(
         std::pair<QMimeType, QByteArray>* pInvalidResponseContent = nullptr) {
     DEBUG_ASSERT(reply);
     DEBUG_ASSERT(JSON_MIME_TYPE.isValid());
-    QByteArray contentBytes = reply->readAll();
-    const auto contentType = readContentType(reply);
+    auto contentType = WebTask::readContentType(*reply);
+    auto optContentData = WebTask::readContentData(reply);
     if (contentType != JSON_MIME_TYPE) {
-        kLogger.warning()
-                << "Unexpected content type"
-                << contentType;
+        kLogger.debug()
+                << "Received content type"
+                << contentType
+                << "instead of"
+                << JSON_MIME_TYPE;
         if (pInvalidResponseContent) {
-            *pInvalidResponseContent = std::make_pair(contentType, contentBytes);
+            *pInvalidResponseContent = std::make_pair(
+                    std::move(contentType),
+                    optContentData.value_or(QByteArray{}));
         }
         return std::nullopt;
     }
-    if (contentBytes.isEmpty()) {
-        kLogger.info() << "Empty JSON network reply";
+    if (!optContentData || optContentData->isEmpty()) {
+        kLogger.debug() << "Empty JSON network reply";
         return QJsonDocument{};
     }
     QJsonParseError parseError;
-    const auto jsonDocument = QJsonDocument::fromJson(
-            contentBytes,
+    auto jsonDocument = QJsonDocument::fromJson(
+            *optContentData,
             &parseError);
     // QJsonDocument::fromJson() returns a non-null document
     // if parsing succeeds and otherwise null on error. The
@@ -95,7 +79,9 @@ std::optional<QJsonDocument> readJsonContent(
                 << "at offset"
                 << parseError.offset;
         if (pInvalidResponseContent) {
-            *pInvalidResponseContent = std::make_pair(contentType, contentBytes);
+            *pInvalidResponseContent = std::make_pair(
+                    std::move(contentType),
+                    std::move(*optContentData));
         }
         return std::nullopt;
     }
@@ -120,7 +106,7 @@ QNetworkRequest newRequest(
 
 QDebug operator<<(QDebug dbg, const JsonWebResponse& arg) {
     return dbg
-            << "CustomWebResponse{"
+            << "WebResponseWithContent{"
             << arg.m_response
             << arg.m_content
             << '}';
@@ -148,7 +134,7 @@ void JsonWebTask::onFinished(
 }
 
 void JsonWebTask::onFinishedCustom(
-        CustomWebResponse&& customResponse) {
+        WebResponseWithContent&& customResponse) {
     kLogger.info()
             << this
             << "Received custom response"
@@ -279,14 +265,14 @@ void JsonWebTask::doNetworkReplyFinished(
     std::optional<QJsonDocument> optJsonContent;
     auto webResponse = WebResponse{
             finishedNetworkReply->url(),
+            finishedNetworkReply->request().url(),
             statusCode};
-    if (statusCode != kHttpStatusCodeInvalid &&
-            finishedNetworkReply->bytesAvailable() > 0) {
+    if (statusCode != kHttpStatusCodeInvalid) {
         std::pair<QMimeType, QByteArray> contentTypeAndBytes;
         optJsonContent = readJsonContent(finishedNetworkReply, &contentTypeAndBytes);
         if (!optJsonContent) {
             // Failed to read JSON content
-            onFinishedCustom(CustomWebResponse{
+            onFinishedCustom(WebResponseWithContent{
                     std::move(webResponse),
                     std::move(contentTypeAndBytes.first),
                     std::move(contentTypeAndBytes.second)});
diff --git a/src/network/jsonwebtask.h b/src/network/jsonwebtask.h
index 1bd22b2d74..cd3f89f137 100644
--- a/src/network/jsonwebtask.h
+++ b/src/network/jsonwebtask.h
@@ -98,7 +98,7 @@ class JsonWebTask : public WebTask {
     virtual void onFinished(
             JsonWebResponse&& response);
     virtual void onFinishedCustom(
-            CustomWebResponse&& response);
+            WebResponseWithContent&& response);
 
     QNetworkReply* doStartNetworkRequest(
             QNetworkAccessManager* networkAccessManager,
diff --git a/src/network/networktask.cpp b/src/network/networktask.cpp
index 96ec7b9b96..4aac5191bc 100644
--- a/src/network/networktask.cpp
+++ b/src/network/networktask.cpp
@@ -65,7 +65,7 @@ void NetworkTask::abort() {
 }
 
 void NetworkTask::emitAborted(
-        QUrl&& requestUrl) {
+        const QUrl& requestUrl) {
     VERIFY_OR_DEBUG_ASSERT(
             isSignalFuncConnected(&NetworkTask::aborted)) {
         kLogger.warning()
@@ -78,27 +78,6 @@ void NetworkTask::emitAborted(
     emit aborted(requestUrl);
 }
 
-void NetworkTask::emitNetworkError(
-        QUrl&& requestUrl,
-        QNetworkReply::NetworkError errorCode,
-        QString&& errorString) {
-    VERIFY_OR_DEBUG_ASSERT(
-            isSignalFuncConnected(&NetworkTask::networkError)) {
-        kLogger.warning()
-                << this
-                << "Unhandled network error signal"
-                << requestUrl
-                << errorCode
-                << errorString;
-        deleteLater();
-        return;
-    }
-    emit networkError(
-            std::move(requestUrl),
-            errorCode,
-            std::move(errorString));
-}
-
 } // namespace network
 
 } // namespace mixxx
diff --git a/src/network/networktask.h b/src/network/networktask.h
index 70e74953fc..410b977977 100644
--- a/src/network/networktask.h
+++ b/src/network/networktask.h
@@ -51,13 +51,7 @@ class NetworkTask : public QObject {
 
     /// Send an aborted signal with the optional request URL if available.
     void emitAborted(
-            QUrl&& requestUrl = QUrl{});
-
-    /// Send a signal after an aborted/timed out/failed network response.
-    void emitNetworkError(
-            QUrl&& requestUrl,
-            QNetworkReply::NetworkError errorCode,
-            QString&& errorString);
+            const QUrl& requestUrl = QUrl{});
 
     /// All member variables must only be accessed from
     /// the event loop thread!!
@@ -95,12 +89,6 @@ class NetworkTask : public QObject {
     /// Client-side abort
     void aborted(
             const QUrl& requestUrl);
-
-    /// Network or server-side abort/timeout/failure
-    void networkError(
-            const QUrl& requestUrl,
-            QNetworkReply::NetworkError errorCode,
-            const QString& errorString);
 };
 
 } // namespace network
diff --git a/src/network/webtask.cpp b/src/network/webtask.cpp
index 6de7dcc9b1..8ddcf4b866 100644
--- a/src/network/webtask.cpp
+++ b/src/network/webtask.cpp
@@ -1,5 +1,6 @@
 #include "network/webtask.h"
 
+#include <QMimeDatabase>
 #include <QTimerEvent>
 #include <mutex> // std::once_flag
 
@@ -21,7 +22,7 @@ std::once_flag registerMetaTypesOnceFlag;
 
 void registerMetaTypesOnce() {
     WebResponse::registerMetaType();
-    CustomWebResponse::registerMetaType();
+    WebResponseWithContent::registerMetaType();
 }
 
 int readStatusCode(
@@ -48,24 +49,54 @@ int readStatusCode(
 QDebug operator<<(QDebug dbg, const WebResponse& arg) {
     return dbg
             << "WebResponse{"
+            << arg.m_requestUrl
             << arg.m_replyUrl
             << arg.m_statusCode
             << '}';
 }
 
-/*static*/ void CustomWebResponse::registerMetaType() {
-    qRegisterMetaType<CustomWebResponse>("mixxx::network::CustomWebResponse");
+/*static*/ void WebResponseWithContent::registerMetaType() {
+    qRegisterMetaType<WebResponseWithContent>("mixxx::network::WebResponseWithContent");
 }
 
-QDebug operator<<(QDebug dbg, const CustomWebResponse& arg) {
+QDebug operator<<(QDebug dbg, const WebResponseWithContent& arg) {
     return dbg
-            << "CustomWebResponse{"
+            << "WebResponseWithContent{"
             << arg.m_response
             << arg.m_contentType
-            << arg.m_contentBytes
+            << arg.m_contentData
             << '}';
 }
 
+//static
+QMimeType WebTask::readContentType(
+        const QNetworkReply& reply) {
+    const QVariant contentTypeHeader = reply.header(QNetworkRequest::ContentTypeHeader);
+    if (!contentTypeHeader.isValid() || contentTypeHeader.isNull()) {
+        kLogger.warning()
+                << "Missing content type header";
+        return QMimeType();
+    }
+    const QString contentTypeString = contentTypeHeader.toString();
+    const QString contentTypeWithoutParams = contentTypeString.left(contentTypeString.indexOf(';'));
+    const QMimeType contentType = QMimeDatabase().mimeTypeForName(contentTypeWithoutParams);
+    if (!contentType.isValid()) {
+        kLogger.warning()
+                << "Unknown content type"
+                << contentTypeWithoutParams;
+    }
+    return contentType;
+}
+
+//static
+std::optional<QByteArray> WebTask::readContentData(
+        QNetworkReply* reply) {
+    if (!reply->isReadable()) {
+        return std::nullopt;
+    }
+    return reply->readAll();
+}
+
 WebTask::WebTask(
         QNetworkAccessManager* networkAccessManager,
         QObject* parent)
@@ -76,9 +107,9 @@ WebTask::WebTask(
 }
 
 void WebTask::onNetworkError(
-        QUrl&& requestUrl,
         QNetworkReply::NetworkError errorCode,
-        QString&& errorString) {
+        const QString& errorString,
+        const WebResponseWithContent& responseWithContent) {
     DEBUG_ASSERT(m_state == State::Pending);
     DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
 
@@ -98,15 +129,36 @@ void WebTask::onNetworkError(
     DEBUG_ASSERT(hasTerminated());
 
     if (m_state == State::Aborted) {
-        emitAborted(std::move(requestUrl));
+        emitAborted(responseWithContent.replyUrl());
     } else {
         emitNetworkError(
-                std::move(requestUrl),
                 errorCode,
-                std::move(errorString));
+                errorString,
+                responseWithContent);
     }
 }
 
+void WebTask::emitNetworkError(
+        QNetworkReply::NetworkError errorCode,
+        const QString& errorString,
+        const WebResponseWithContent& responseWithContent) {
+    VERIFY_OR_DEBUG_ASSERT(
+            isSignalFuncConnected(&WebTask::networkError)) {
+        kLogger.warning()
+                << this
+                << "Unhandled network error signal"
+                << errorCode
+                << errorString
+                << responseWithContent;
+        deleteLater();
+        return;
+    }
+    emit networkError(
+            errorCode,
+            errorString,
+            responseWithContent);
+}
+
 void WebTask::slotStart(int timeoutMillis) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
     if (m_state == State::Pending) {
@@ -124,9 +176,9 @@ void WebTask::slotStart(int timeoutMillis) {
     VERIFY_OR_DEBUG_ASSERT(pNetworkAccessManager) {
         m_state = State::Pending;
         onNetworkError(
-                QUrl(),
                 QNetworkReply::NetworkSessionFailedError,
-                tr("No network access"));
+                tr("No network access"),
+                WebResponseWithContent{});
         return;
     }
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(pNetworkAccessManager);
@@ -230,7 +282,7 @@ void WebTask::slotAbort() {
     }
 
     m_state = State::Aborted;
-    emitAborted(std::move(requestUrl));
+    emitAborted(requestUrl);
 }
 
 void WebTask::timerEvent(QTimerEvent* event) {
@@ -310,17 +362,24 @@ void WebTask::slotNetworkReplyFinished() {
         m_timeoutTimerId = kInvalidTimerId;
     }
 
+    const auto statusCode = readStatusCode(pFinishedNetworkReply);
     if (pFinishedNetworkReply->error() != QNetworkReply::NetworkError::NoError) {
         onNetworkError(
-                pFinishedNetworkReply->request().url(),
                 pFinishedNetworkReply->error(),
-                pFinishedNetworkReply->errorString());
+                pFinishedNetworkReply->errorString(),
+                WebResponseWithContent{
+                        WebResponse{
+                                pFinishedNetworkReply->url(),
+                                pFinishedNetworkReply->request().url(),
+                                statusCode},
+                        readContentType(*pFinishedNetworkReply),
+                        readContentData(pFinishedNetworkReply).value_or(QByteArray{}),
+                });
         DEBUG_ASSERT(hasTerminated());
         return;
     }
 
     m_state = State::Finished;
-    const auto statusCode = readStatusCode(pFinishedNetworkReply);
     doNetworkReplyFinished(pFinishedNetworkReply, statusCode);
 }
 
diff --git a/src/network/webtask.h b/src/network/webtask.h
index 7d364c1afb..ae2e8ed654 100644
--- a/src/network/webtask.h
+++ b/src/network/webtask.h
@@ -23,9 +23,11 @@ class WebResponse final {
             : m_statusCode(kHttpStatusCodeInvalid) {
     }
     explicit WebResponse(
-            QUrl replyUrl,
+            QUrl requestUrl,
+            QUrl replyUrl = QUrl(),
             HttpStatusCode statusCode = kHttpStatusCodeInvalid)
-            : m_replyUrl(std::move(replyUrl)),
+            : m_requestUrl(std::move(requestUrl)),
+              m_replyUrl(std::move(replyUrl)),
               m_statusCode(statusCode) {
     }
     WebResponse(const WebResponse&) = default;
@@ -38,39 +40,44 @@ class WebResponse final {
         return HttpStatusCode_isSuccess(m_statusCode);
     }
 
-    HttpStatusCode statusCode() const {
-        return m_statusCode;
+    const QUrl& requestUrl() const {
+        return m_requestUrl;
     }
 
     const QUrl& replyUrl() const {
         return m_replyUrl;
     }
 
+    HttpStatusCode statusCode() const {
+        return m_statusCode;
+    }
+
     friend QDebug operator<<(QDebug dbg, const WebResponse& arg);
 
   private:
+    QUrl m_requestUrl;
     QUrl m_replyUrl;
     HttpStatusCode m_statusCode;
 };
 
-class CustomWebResponse final {
+class WebResponseWithContent final {
   public:
     static void registerMetaType();
 
-    CustomWebResponse() = default;
-    CustomWebResponse(
+    WebResponseWithContent() = default;
+    WebResponseWithContent(
             WebResponse&& response,
             QMimeType&& contentType,
-            QByteArray&& contentBytes)
+            QByteArray&& contentData)
             : m_response(std::move(response)),
               m_contentType(std::move(contentType)),
-              m_contentBytes(std::move(contentBytes)) {
+              m_contentData(std::move(contentData)) {
     }
-    CustomWebResponse(const CustomWebResponse&) = default;
-    CustomWebResponse(CustomWebResponse&&) = default;
+    WebResponseWithContent(const WebResponseWithContent&) = default;
+    WebResponseWithContent(WebResponseWithContent&&) = default;
 
-    CustomWebResponse& operator=(const CustomWebResponse&) = default;
-    CustomWebResponse& operator=(CustomWebResponse&&) = default;
+    WebResponseWithContent& operator=(const WebResponseWithContent&) = default;
+    WebResponseWithContent& operator=(WebResponseWithContent&&) = default;
 
     bool isStatusCodeSuccess() const {
         return m_response.isStatusCodeSuccess();
@@ -80,6 +87,10 @@ class CustomWebResponse final {
         return m_response.statusCode();
     }
 
+    const QUrl& requestUrl() const {
+        return m_response.requestUrl();
+    }
+
     const QUrl& replyUrl() const {
         return m_response.replyUrl();
     }
@@ -88,16 +99,16 @@ class CustomWebResponse final {
         return m_contentType;
     }
 
-    const QByteArray& contentBytes() const {
-        return m_contentBytes;
+    const QByteArray& contentData() const {
+        return m_contentData;
     }
 
-    friend QDebug operator<<(QDebug dbg, const CustomWebResponse& arg);
+    friend QDebug operator<<(QDebug dbg, const WebResponseWithContent& arg);
 
   private:
     WebResponse m_response;
     QMimeType m_contentType;
-    QByteArray m_contentBytes;
+    QByteArray m_contentData;
 };
 
 /// A transient task for performing a single HTTP network request
@@ -106,11 +117,23 @@ class WebTask : public NetworkTask {
     Q_OBJECT
 
   public:
+    static QMimeType readContentType(
+            const QNetworkReply& reply);
+    static std::optional<QByteArray> readContentData(
+            QNetworkReply* reply);
+
     explicit WebTask(
             QNetworkAccessManager* networkAccessManager,
             QObject* parent = nullptr);
     ~WebTask() override = default;
 
+  signals:
+    /// Network or server-side abort/timeout/failure
+    void networkError(
+            QNetworkReply::NetworkError errorCode,
+            const QString& errorString,
+            const mixxx::network::WebResponseWithContent& responseWithContent);
+
   public slots:
     void slotStart(
             int timeoutMillis) override;
@@ -145,6 +168,12 @@ class WebTask : public NetworkTask {
                 state() == State::Finished;
     }
 
+    /// Send a signal after a failed network response.
+    void emitNetworkError(
+            QNetworkReply::NetworkError errorCode,
+            const QString& errorString,
+            const WebResponseWithContent& responseWithContent);
+
   private:
     QUrl abortPendingNetworkReply();
 
@@ -169,9 +198,9 @@ class WebTask : public NetworkTask {
     /// gets deleted. The default implementation logs a warning
     /// and deletes the task.
     void onNetworkError(
-            QUrl&& requestUrl,
             QNetworkReply::NetworkError errorCode,
-            QString&& errorString);
+            const QString& errorString,
+            const WebResponseWithContent& responseWithContent);
 
     /// All member variables must only be accessed from
     /// the event loop thread!!
@@ -191,4 +220,4 @@ class WebTask : public NetworkTask {
 
 Q_DECLARE_METATYPE(mixxx::network::WebResponse);
 
-Q_DECLARE_METATYPE(mixxx::network::CustomWebResponse);
+Q_DECLARE_METATYPE(mixxx::network::WebResponseWithContent);