From f36ac5272bb660a8ce04d950bb0fe3ec666edda6 Mon Sep 17 00:00:00 2001 From: Jonas Kvinge Date: Fri, 21 Apr 2023 02:11:23 +0200 Subject: [PATCH] Scrobbler: Simplify error handling --- src/scrobbler/listenbrainzscrobbler.cpp | 265 ++++++++------------- src/scrobbler/listenbrainzscrobbler.h | 12 +- src/scrobbler/scrobblercache.cpp | 59 ++--- src/scrobbler/scrobblercache.h | 14 +- src/scrobbler/scrobblercacheitem.cpp | 3 +- src/scrobbler/scrobblercacheitem.h | 1 + src/scrobbler/scrobblerservice.cpp | 33 +-- src/scrobbler/scrobblerservice.h | 3 +- src/scrobbler/scrobblingapi20.cpp | 299 ++++++++---------------- src/scrobbler/scrobblingapi20.h | 13 +- src/scrobbler/subsonicscrobbler.cpp | 7 - src/scrobbler/subsonicscrobbler.h | 1 - 12 files changed, 260 insertions(+), 450 deletions(-) diff --git a/src/scrobbler/listenbrainzscrobbler.cpp b/src/scrobbler/listenbrainzscrobbler.cpp index 65c5a275e..eb8a7cbef 100644 --- a/src/scrobbler/listenbrainzscrobbler.cpp +++ b/src/scrobbler/listenbrainzscrobbler.cpp @@ -227,6 +227,45 @@ void ListenBrainzScrobbler::RedirectArrived() { } +ListenBrainzScrobbler::ReplyResult ListenBrainzScrobbler::GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description) { + + ReplyResult reply_error_type = ReplyResult::ServerError; + + if (reply->error() == QNetworkReply::NoError) { + if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) { + reply_error_type = ReplyResult::Success; + } + else { + error_description = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt()); + } + } + else { + error_description = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); + } + + // See if there is Json data containing "error" and "error_description" or "code" and "error" - then use that instead. + if (reply->error() == QNetworkReply::NoError || reply->error() >= 200) { + const QByteArray data = reply->readAll(); + if (!data.isEmpty() && ExtractJsonObj(data, json_obj, error_description)) { + if (json_obj.contains("error") && json_obj.contains("error_description")) { + error_description = json_obj["error_description"].toString(); + reply_error_type = ReplyResult::APIError; + } + else if (json_obj.contains("code") && json_obj.contains("error")) { + error_description = QString("%1 (%2)").arg(json_obj["error"].toString()).arg(json_obj["code"].toInt()); + reply_error_type = ReplyResult::APIError; + } + } + if (reply->error() == QNetworkReply::ContentAccessDenied || reply->error() == QNetworkReply::ContentOperationNotPermittedError || reply->error() == QNetworkReply::AuthenticationRequiredError) { + // Session is probably expired + Logout(); + } + } + + return reply_error_type; + +} + void ListenBrainzScrobbler::RequestAccessToken(const QUrl &redirect_url, const QString &code) { refresh_login_timer_.stop(); @@ -271,50 +310,10 @@ void ListenBrainzScrobbler::AuthenticateReplyFinished(QNetworkReply *reply) { QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - QByteArray data; - - if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) { - data = reply->readAll(); - } - else { - if (reply->error() != QNetworkReply::NoError && reply->error() < 200) { - // This is a network error, there is nothing more to do. - AuthError(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error())); - } - else { - // See if there is Json data containing "error" and "error_description" - then use that instead. - data = reply->readAll(); - QString error; - QJsonParseError json_error; - QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error); - if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) { - QJsonObject json_obj = json_doc.object(); - if (json_obj.contains("error") && json_obj.contains("error_description")) { - error = json_obj["error_description"].toString(); - } - } - if (error.isEmpty()) { - if (reply->error() != QNetworkReply::NoError) { - error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); - } - else { - error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt()); - } - } - AuthError(error); - } - return; - } - - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - AuthError("Json document from server was empty."); - return; - } - - if (json_obj.contains("error") && json_obj.contains("error_description")) { - QString failure_reason = json_obj["error_description"].toString(); - AuthError(failure_reason); + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + AuthError(error_message); return; } @@ -368,56 +367,6 @@ QNetworkReply *ListenBrainzScrobbler::CreateRequest(const QUrl &url, const QJson } -QByteArray ListenBrainzScrobbler::GetReplyData(QNetworkReply *reply) { - - QByteArray data; - - if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) { - data = reply->readAll(); - } - else { - if (reply->error() < 200) { - // This is a network error, there is nothing more to do. - Error(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error())); - } - else { - // See if there is Json data containing "code" and "error" - then use that instead. - data = reply->readAll(); - QString error; - QJsonParseError json_error; - QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error); - if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) { - QJsonObject json_obj = json_doc.object(); - if (json_obj.contains("code") && json_obj.contains("error")) { - int error_code = json_obj["code"].toInt(); - QString error_message = json_obj["error"].toString(); - error = QString("%1 (%2)").arg(error_message).arg(error_code); - } - else { - error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); - } - } - if (error.isEmpty()) { - if (reply->error() != QNetworkReply::NoError) { - error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); - } - else { - error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt()); - } - } - if (reply->error() == QNetworkReply::ContentAccessDenied || reply->error() == QNetworkReply::ContentOperationNotPermittedError || reply->error() == QNetworkReply::AuthenticationRequiredError) { - // Session is probably expired - Logout(); - } - Error(error); - } - return QByteArray(); - } - - return data; - -} - QJsonObject ListenBrainzScrobbler::JsonTrackMetadata(const ScrobbleMetadata &metadata) const { QJsonObject object_track_metadata; @@ -524,30 +473,21 @@ void ListenBrainzScrobbler::UpdateNowPlayingRequestFinished(QNetworkReply *reply QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - return; - } - - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - return; - } - - if (json_obj.contains("code") && json_obj.contains("error_description")) { - QString error_desc = json_obj["error_description"].toString(); - Error(error_desc); + QJsonObject json_obj; + QString error_description; + if (GetJsonObject(reply, json_obj, error_description) != ReplyResult::Success) { + Error(error_description); return; } if (!json_obj.contains("status")) { - Error("Missing status from server.", json_obj); + Error("Now playing request is missing status from server."); return; } QString status = json_obj["status"].toString(); if (status.compare("ok", Qt::CaseInsensitive) != 0) { - Error(status); + Error(QString("Received %1 status for now playing.").arg(status)); } } @@ -600,22 +540,21 @@ void ListenBrainzScrobbler::Submit() { if (!IsEnabled() || !IsAuthenticated() || app_->scrobbler()->IsOffline()) return; QJsonArray array; - int i = 0; - QList list; - ScrobblerCacheItemPtrList cache_items = cache_->List(); - for (ScrobblerCacheItemPtr cache_item : cache_items) { + ScrobblerCacheItemPtrList cache_items_sent; + ScrobblerCacheItemPtrList all_cache_items = cache_->List(); + for (ScrobblerCacheItemPtr cache_item : all_cache_items) { if (cache_item->sent) continue; + if (cache_item->error && cache_items_sent.count() > 0) break; cache_item->sent = true; - ++i; - list << cache_item->timestamp; + cache_items_sent << cache_item; QJsonObject object_listen; object_listen.insert("listened_at", QJsonValue::fromVariant(cache_item->timestamp)); object_listen.insert("track_metadata", JsonTrackMetadata(cache_item->metadata)); array.append(QJsonValue::fromVariant(object_listen)); - if (i >= kScrobblesPerRequest) break; + if (cache_items_sent.count() >= kScrobblesPerRequest || cache_item->error) break; } - if (i <= 0) return; + if (cache_items_sent.count() <= 0) return; submitted_ = true; @@ -626,11 +565,11 @@ void ListenBrainzScrobbler::Submit() { QUrl url(QString("%1/1/submit-listens").arg(kApiUrl)); QNetworkReply *reply = CreateRequest(url, doc); - QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, list]() { ScrobbleRequestFinished(reply, list); }); + QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, cache_items_sent]() { ScrobbleRequestFinished(reply, cache_items_sent); }); } -void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, const QList &list) { +void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items) { if (!replies_.contains(reply)) return; replies_.removeAll(reply); @@ -639,38 +578,40 @@ void ListenBrainzScrobbler::ScrobbleRequestFinished(QNetworkReply *reply, const submitted_ = false; - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - cache_->ClearSent(list); + QJsonObject json_obj; + QString error_message; + const ReplyResult reply_result = GetJsonObject(reply, json_obj, error_message); + if (reply_result == ReplyResult::Success) { + if (json_obj.contains("status")) { + QString status = json_obj["status"].toString(); + qLog(Debug) << "ListenBrainz: Received scrobble status:" << status; + } + else { + qLog(Debug) << "ListenBrainz: Received scrobble reply without status."; + } + cache_->Flush(cache_items); + submit_error_ = false; + } + else { submit_error_ = true; - StartSubmit(); - return; + if (reply_result == ReplyResult::APIError) { + if (cache_items.count() == 1) { + const ScrobbleMetadata &metadata = cache_items.first()->metadata; + Error(tr("Unable to scrobble %1 - %2 because of error: %3").arg(metadata.effective_albumartist()).arg(metadata.title).arg(error_message)); + cache_->Flush(cache_items); + } + else { + Error(error_message); + cache_->SetError(cache_items); + cache_->ClearSent(cache_items); + } + } + else { + Error(error_message); + cache_->ClearSent(cache_items); + } } - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - cache_->ClearSent(list); - submit_error_ = true; - StartSubmit(); - return; - } - - if (json_obj.contains("code") && json_obj.contains("error_description")) { - QString error_desc = json_obj["error_description"].toString(); - Error(error_desc); - cache_->ClearSent(list); - submit_error_ = true; - StartSubmit(); - return; - } - - if (json_obj.contains("status")) { - QString status = json_obj["status"].toString(); - qLog(Debug) << "ListenBrainz: Received scrobble status:" << status; - } - - cache_->Flush(list); - submit_error_ = false; StartSubmit(); } @@ -682,7 +623,7 @@ void ListenBrainzScrobbler::Love() { if (!IsAuthenticated()) app_->scrobbler()->ShowConfig(); if (song_playing_.musicbrainz_recording_id().isEmpty()) { - qLog(Error) << "ListenBrainz: Missing MusicBrainz recording ID for" << song_playing_.artist() << song_playing_.album() << song_playing_.title(); + Error(tr("Missing MusicBrainz recording ID for %1 %2 %3").arg(song_playing_.artist()).arg(song_playing_.album()).arg(song_playing_.title())); return; } @@ -705,30 +646,24 @@ void ListenBrainzScrobbler::LoveRequestFinished(QNetworkReply *reply) { QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - return; - } - - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - return; - } - - if (json_obj.contains("code") && json_obj.contains("error_description")) { - Error(json_obj["error_description"].toString()); + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + Error(error_message); return; } if (json_obj.contains("status")) { - QString status = json_obj["status"].toString(); - qLog(Debug) << "ListenBrainz: Received recording-feedback status:" << status; + qLog(Debug) << "ListenBrainz: Received recording-feedback status:" << json_obj["status"].toString(); } } void ListenBrainzScrobbler::AuthError(const QString &error) { + + qLog(Error) << "ListenBrainz" << error; emit AuthenticationComplete(false, error); + } void ListenBrainzScrobbler::Error(const QString &error, const QVariant &debug) { @@ -736,6 +671,10 @@ void ListenBrainzScrobbler::Error(const QString &error, const QVariant &debug) { qLog(Error) << "ListenBrainz:" << error; if (debug.isValid()) qLog(Debug) << debug; + if (app_->scrobbler()->ShowErrorDialog()) { + emit ErrorMessage(tr("ListenBrainz error: %1").arg(error)); + } + } void ListenBrainzScrobbler::CheckScrobblePrevSong() { diff --git a/src/scrobbler/listenbrainzscrobbler.h b/src/scrobbler/listenbrainzscrobbler.h index aee617229..83b4f6c60 100644 --- a/src/scrobbler/listenbrainzscrobbler.h +++ b/src/scrobbler/listenbrainzscrobbler.h @@ -71,6 +71,12 @@ class ListenBrainzScrobbler : public ScrobblerService { void Scrobble(const Song &song) override; void Love() override; + enum class ReplyResult { + Success, + ServerError, + APIError + }; + signals: void AuthenticationComplete(const bool success, const QString &error = QString()); @@ -82,15 +88,15 @@ class ListenBrainzScrobbler : public ScrobblerService { void AuthenticateReplyFinished(QNetworkReply *reply); void RequestNewAccessToken() { RequestAccessToken(); } void UpdateNowPlayingRequestFinished(QNetworkReply *reply); - void ScrobbleRequestFinished(QNetworkReply *reply, const QList &list); + void ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items); void LoveRequestFinished(QNetworkReply *reply); private: QNetworkReply *CreateRequest(const QUrl &url, const QJsonDocument &json_doc); - QByteArray GetReplyData(QNetworkReply *reply); + ReplyResult GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description); QJsonObject JsonTrackMetadata(const ScrobbleMetadata &metadata) const; void AuthError(const QString &error); - void Error(const QString &error, const QVariant &debug = QVariant()) override; + void Error(const QString &error, const QVariant &debug = QVariant()); void RequestAccessToken(const QUrl &redirect_url = QUrl(), const QString &code = QString()); void StartSubmit(const bool initial = false) override; void CheckScrobblePrevSong(); diff --git a/src/scrobbler/scrobblercache.cpp b/src/scrobbler/scrobblercache.cpp index 79f8a8953..e164d420b 100644 --- a/src/scrobbler/scrobblercache.cpp +++ b/src/scrobbler/scrobblercache.cpp @@ -25,7 +25,6 @@ #include #include -#include #include #include #include @@ -183,9 +182,8 @@ void ScrobblerCache::ReadCache() { metadata.musicbrainz_work_id = json_obj_track["musicbrainz_work_id"].toString(); } - if (scrobbler_cache_.contains(timestamp)) continue; - std::shared_ptr cache_item = std::make_shared(metadata, timestamp); - scrobbler_cache_.insert(timestamp, cache_item); + ScrobblerCacheItemPtr cache_item = std::make_shared(metadata, timestamp); + scrobbler_cache_ << cache_item; } @@ -204,8 +202,7 @@ void ScrobblerCache::WriteCache() { } QJsonArray array; - for (QHash > ::iterator i = scrobbler_cache_.begin(); i != scrobbler_cache_.end(); ++i) { - ScrobblerCacheItemPtr cache_item = i.value(); + for (ScrobblerCacheItemPtr cache_item : scrobbler_cache_) { QJsonObject object; object.insert("timestamp", QJsonValue::fromVariant(cache_item->timestamp)); object.insert("artist", QJsonValue::fromVariant(cache_item->metadata.artist)); @@ -251,11 +248,9 @@ void ScrobblerCache::WriteCache() { ScrobblerCacheItemPtr ScrobblerCache::Add(const Song &song, const quint64 timestamp) { - if (scrobbler_cache_.contains(timestamp)) return nullptr; - ScrobblerCacheItemPtr cache_item = std::make_shared(ScrobbleMetadata(song), timestamp); - scrobbler_cache_.insert(timestamp, cache_item); + scrobbler_cache_ << cache_item; if (loaded_ && !timer_flush_->isActive()) { timer_flush_->start(); @@ -265,43 +260,35 @@ ScrobblerCacheItemPtr ScrobblerCache::Add(const Song &song, const quint64 timest } -ScrobblerCacheItemPtr ScrobblerCache::Get(const quint64 hash) { +void ScrobblerCache::Remove(ScrobblerCacheItemPtr cache_item) { - if (scrobbler_cache_.contains(hash)) { return scrobbler_cache_.value(hash); } - else return nullptr; - -} - -void ScrobblerCache::Remove(const quint64 hash) { - - if (!scrobbler_cache_.contains(hash)) { - qLog(Error) << "Tried to remove non-existing hash" << hash; - return; + if (scrobbler_cache_.contains(cache_item)) { + scrobbler_cache_.removeAll(cache_item); } - - scrobbler_cache_.remove(hash); - } -void ScrobblerCache::Remove(ScrobblerCacheItemPtr item) { - scrobbler_cache_.remove(item->timestamp); -} +void ScrobblerCache::ClearSent(ScrobblerCacheItemPtrList cache_items) { -void ScrobblerCache::ClearSent(const QList &list) { - - for (const quint64 timestamp : list) { - if (!scrobbler_cache_.contains(timestamp)) continue; - ScrobblerCacheItemPtr item = scrobbler_cache_.value(timestamp); - item->sent = false; + for (ScrobblerCacheItemPtr cache_item : cache_items) { + cache_item->sent = false; } } -void ScrobblerCache::Flush(const QList &list) { +void ScrobblerCache::SetError(ScrobblerCacheItemPtrList cache_items) { - for (const quint64 timestamp : list) { - if (!scrobbler_cache_.contains(timestamp)) continue; - scrobbler_cache_.remove(timestamp); + for (ScrobblerCacheItemPtr item : cache_items) { + item->error = true; + } + +} + +void ScrobblerCache::Flush(ScrobblerCacheItemPtrList cache_items) { + + for (ScrobblerCacheItemPtr cache_item : cache_items) { + if (scrobbler_cache_.contains(cache_item)) { + scrobbler_cache_.removeAll(cache_item); + } } if (!timer_flush_->isActive()) { diff --git a/src/scrobbler/scrobblercache.h b/src/scrobbler/scrobblercache.h index c452742e6..fa6bfc979 100644 --- a/src/scrobbler/scrobblercache.h +++ b/src/scrobbler/scrobblercache.h @@ -27,7 +27,6 @@ #include #include #include -#include #include #include "scrobblercacheitem.h" @@ -45,13 +44,12 @@ class ScrobblerCache : public QObject { void ReadCache(); ScrobblerCacheItemPtr Add(const Song &song, const quint64 timestamp); - ScrobblerCacheItemPtr Get(const quint64 hash); - void Remove(const quint64 hash); - void Remove(ScrobblerCacheItemPtr item); + void Remove(ScrobblerCacheItemPtr cache_item); int Count() const { return scrobbler_cache_.size(); }; - ScrobblerCacheItemPtrList List() const { return scrobbler_cache_.values(); } - void ClearSent(const QList &list); - void Flush(const QList &list); + ScrobblerCacheItemPtrList List() const { return scrobbler_cache_; } + void ClearSent(ScrobblerCacheItemPtrList cache_items); + void SetError(ScrobblerCacheItemPtrList cache_items); + void Flush(ScrobblerCacheItemPtrList cache_items); public slots: void WriteCache(); @@ -60,7 +58,7 @@ class ScrobblerCache : public QObject { QTimer *timer_flush_; QString filename_; bool loaded_; - QHash scrobbler_cache_; + QList scrobbler_cache_; }; diff --git a/src/scrobbler/scrobblercacheitem.cpp b/src/scrobbler/scrobblercacheitem.cpp index d73c7b06e..9f6314983 100644 --- a/src/scrobbler/scrobblercacheitem.cpp +++ b/src/scrobbler/scrobblercacheitem.cpp @@ -27,4 +27,5 @@ ScrobblerCacheItem::ScrobblerCacheItem(const ScrobbleMetadata &_metadata, const quint64 _timestamp) : metadata(_metadata), timestamp(_timestamp), - sent(false) {} + sent(false), + error(false) {} diff --git a/src/scrobbler/scrobblercacheitem.h b/src/scrobbler/scrobblercacheitem.h index 06f4a962d..86ecfa293 100644 --- a/src/scrobbler/scrobblercacheitem.h +++ b/src/scrobbler/scrobblercacheitem.h @@ -36,6 +36,7 @@ class ScrobblerCacheItem { ScrobbleMetadata metadata; quint64 timestamp; bool sent; + bool error; }; using ScrobblerCacheItemPtr = std::shared_ptr; diff --git a/src/scrobbler/scrobblerservice.cpp b/src/scrobbler/scrobblerservice.cpp index faaf5c516..87e36fcef 100644 --- a/src/scrobbler/scrobblerservice.cpp +++ b/src/scrobbler/scrobblerservice.cpp @@ -36,32 +36,21 @@ ScrobblerService::ScrobblerService(const QString &name, Application *app, QObjec } -QJsonObject ScrobblerService::ExtractJsonObj(const QByteArray &data, const bool ignore_empty) { +bool ScrobblerService::ExtractJsonObj(const QByteArray &data, QJsonObject &json_obj, QString &error_description) { - QJsonParseError error; - QJsonDocument json_doc = QJsonDocument::fromJson(data, &error); + QJsonParseError json_parse_error; + const QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_parse_error); - if (error.error != QJsonParseError::NoError) { - Error("Reply from server missing Json data.", data); - return QJsonObject(); - } - if (json_doc.isEmpty()) { - Error("Received empty Json document.", json_doc); - return QJsonObject(); - } - if (!json_doc.isObject()) { - Error("Json document is not an object.", json_doc); - return QJsonObject(); - } - QJsonObject json_obj = json_doc.object(); - if (json_obj.isEmpty()) { - if (!ignore_empty) { - Error("Received empty Json object.", json_doc); - } - return QJsonObject(); + if (json_parse_error.error != QJsonParseError::NoError) { + error_description = json_parse_error.errorString(); + return false; } - return json_obj; + if (json_doc.isObject()) { + json_obj = json_doc.object(); + } + + return true; } diff --git a/src/scrobbler/scrobblerservice.h b/src/scrobbler/scrobblerservice.h index cbfc23f1a..dc24a41b5 100644 --- a/src/scrobbler/scrobblerservice.h +++ b/src/scrobbler/scrobblerservice.h @@ -61,8 +61,7 @@ class ScrobblerService : public QObject { using ParamList = QList; using EncodedParam = QPair; - QJsonObject ExtractJsonObj(const QByteArray &data, const bool ignore_empty = false); - virtual void Error(const QString &error, const QVariant &debug = QVariant()) = 0; + bool ExtractJsonObj(const QByteArray &data, QJsonObject &json_obj, QString &error_description); QString StripAlbum(QString album) const; QString StripTitle(QString title) const; diff --git a/src/scrobbler/scrobblingapi20.cpp b/src/scrobbler/scrobblingapi20.cpp index 3991cd6c0..76faeb642 100644 --- a/src/scrobbler/scrobblingapi20.cpp +++ b/src/scrobbler/scrobblingapi20.cpp @@ -149,6 +149,51 @@ void ScrobblingAPI20::Logout() { } +ScrobblingAPI20::ReplyResult ScrobblingAPI20::GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description) { + + ReplyResult reply_error_type = ReplyResult::ServerError; + + if (reply->error() == QNetworkReply::NoError) { + if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) { + reply_error_type = ReplyResult::Success; + } + else { + error_description = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt()); + } + } + else { + error_description = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); + } + + // See if there is Json data containing "error" and "message" - then use that instead. + if (reply->error() == QNetworkReply::NoError || reply->error() >= 200) { + const QByteArray data = reply->readAll(); + int error_code = 0; + if (!data.isEmpty() && ExtractJsonObj(data, json_obj, error_description) && json_obj.contains("error") && json_obj.contains("message")) { + error_code = json_obj["error"].toInt(); + QString error_message = json_obj["message"].toString(); + error_description = QString("%1 (%2)").arg(error_message).arg(error_code); + reply_error_type = ReplyResult::APIError; + } + const ScrobbleErrorCode lastfm_error_code = static_cast(error_code); + if (reply->error() == QNetworkReply::ContentAccessDenied || + reply->error() == QNetworkReply::ContentOperationNotPermittedError || + reply->error() == QNetworkReply::AuthenticationRequiredError || + lastfm_error_code == ScrobbleErrorCode::InvalidSessionKey || + lastfm_error_code == ScrobbleErrorCode::UnauthorizedToken || + lastfm_error_code == ScrobbleErrorCode::LoginRequired || + lastfm_error_code == ScrobbleErrorCode::AuthenticationFailed || + lastfm_error_code == ScrobbleErrorCode::APIKeySuspended + ) { + // Session is probably expired + Logout(); + } + } + + return reply_error_type; + +} + void ScrobblingAPI20::Authenticate(const bool https) { if (!server_) { @@ -262,57 +307,10 @@ void ScrobblingAPI20::AuthenticateReplyFinished(QNetworkReply *reply) { QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - QByteArray data; - - if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) { - data = reply->readAll(); - } - else { - if (reply->error() < 200) { - // This is a network error, there is nothing more to do. - AuthError(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error())); - } - else { - // See if there is Json data containing "error" and "message" - then use that instead. - data = reply->readAll(); - QString error; - QJsonParseError json_error; - QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error); - if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) { - QJsonObject json_obj = json_doc.object(); - if (json_obj.contains("error") && json_obj.contains("message")) { - int code = json_obj["error"].toInt(); - QString message = json_obj["message"].toString(); - error = "Error: " + QString::number(code) + ": " + message; - } - else { - error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); - } - } - if (error.isEmpty()) { - if (reply->error() != QNetworkReply::NoError) { - error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); - } - else { - error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt()); - } - } - AuthError(error); - } - return; - } - - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - AuthError("Json document from server was empty."); - return; - } - - if (json_obj.contains("error") && json_obj.contains("message")) { - int error = json_obj["error"].toInt(); - QString message = json_obj["message"].toString(); - QString failure_reason = "Error: " + QString::number(error) + ": " + message; - AuthError(failure_reason); + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + AuthError(error_message); return; } @@ -392,63 +390,6 @@ QNetworkReply *ScrobblingAPI20::CreateRequest(const ParamList &request_params) { } -QByteArray ScrobblingAPI20::GetReplyData(QNetworkReply *reply) { - - QByteArray data; - - if (reply->error() == QNetworkReply::NoError && reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 200) { - data = reply->readAll(); - } - else { - if (reply->error() != QNetworkReply::NoError && reply->error() < 200) { - // This is a network error, there is nothing more to do. - Error(QString("%1 (%2)").arg(reply->errorString()).arg(reply->error())); - } - else { - QString error; - // See if there is Json data containing "error" and "message" - then use that instead. - data = reply->readAll(); - QJsonParseError json_error; - QJsonDocument json_doc = QJsonDocument::fromJson(data, &json_error); - int error_code = -1; - if (json_error.error == QJsonParseError::NoError && !json_doc.isEmpty() && json_doc.isObject()) { - QJsonObject json_obj = json_doc.object(); - if (json_obj.contains("error") && json_obj.contains("message")) { - error_code = json_obj["error"].toInt(); - QString error_message = json_obj["message"].toString(); - error = QString("%1 (%2)").arg(error_message).arg(error_code); - } - } - if (error.isEmpty()) { - if (reply->error() != QNetworkReply::NoError) { - error = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error()); - } - else { - error = QString("Received HTTP code %1").arg(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt()); - } - } - const ScrobbleErrorCode lastfm_error_code = static_cast(error_code); - if (reply->error() == QNetworkReply::ContentAccessDenied || - reply->error() == QNetworkReply::ContentOperationNotPermittedError || - reply->error() == QNetworkReply::AuthenticationRequiredError || - lastfm_error_code == ScrobbleErrorCode::InvalidSessionKey || - lastfm_error_code == ScrobbleErrorCode::UnauthorizedToken || - lastfm_error_code == ScrobbleErrorCode::LoginRequired || - lastfm_error_code == ScrobbleErrorCode::AuthenticationFailed || - lastfm_error_code == ScrobbleErrorCode::APIKeySuspended - ){ - // Session is probably expired - Logout(); - } - Error(error); - } - return QByteArray(); - } - - return data; - -} - void ScrobblingAPI20::UpdateNowPlaying(const Song &song) { CheckScrobblePrevSong(); @@ -484,21 +425,10 @@ void ScrobblingAPI20::UpdateNowPlayingRequestFinished(QNetworkReply *reply) { QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - return; - } - - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - return; - } - - if (json_obj.contains("error") && json_obj.contains("message")) { - int error_code = json_obj["error"].toInt(); - QString error_message = json_obj["message"].toString(); - QString error_reason = QString("%1 (%2)").arg(error_message).arg(error_code); - Error(error_reason); + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + Error(error_message); return; } @@ -566,43 +496,43 @@ void ScrobblingAPI20::Submit() { ParamList params = ParamList() << Param("method", "track.scrobble"); int i = 0; - QList list; - ScrobblerCacheItemPtrList items = cache_->List(); - for (ScrobblerCacheItemPtr item : items) { - if (item->sent) continue; - item->sent = true; + ScrobblerCacheItemPtrList all_cache_items = cache_->List(); + ScrobblerCacheItemPtrList cache_items_sent; + for (ScrobblerCacheItemPtr cache_item : all_cache_items) { + if (cache_item->sent) continue; + cache_item->sent = true; if (!batch_) { - SendSingleScrobble(item); + SendSingleScrobble(cache_item); continue; } - list << item->timestamp; - params << Param(QString("%1[%2]").arg("artist").arg(i), prefer_albumartist_ ? item->metadata.effective_albumartist() : item->metadata.artist); - params << Param(QString("%1[%2]").arg("track").arg(i), StripTitle(item->metadata.title)); - params << Param(QString("%1[%2]").arg("timestamp").arg(i), QString::number(item->timestamp)); - params << Param(QString("%1[%2]").arg("duration").arg(i), QString::number(item->metadata.length_nanosec / kNsecPerSec)); - if (!item->metadata.album.isEmpty()) { - params << Param(QString("%1[%2]").arg("album").arg(i), StripAlbum(item->metadata.album)); + cache_items_sent << cache_item; + params << Param(QString("%1[%2]").arg("artist").arg(i), prefer_albumartist_ ? cache_item->metadata.effective_albumartist() : cache_item->metadata.artist); + params << Param(QString("%1[%2]").arg("track").arg(i), StripTitle(cache_item->metadata.title)); + params << Param(QString("%1[%2]").arg("timestamp").arg(i), QString::number(cache_item->timestamp)); + params << Param(QString("%1[%2]").arg("duration").arg(i), QString::number(cache_item->metadata.length_nanosec / kNsecPerSec)); + if (!cache_item->metadata.album.isEmpty()) { + params << Param(QString("%1[%2]").arg("album").arg(i), StripAlbum(cache_item->metadata.album)); } - if (!prefer_albumartist_ && !item->metadata.albumartist.isEmpty()) { - params << Param(QString("%1[%2]").arg("albumArtist").arg(i), item->metadata.albumartist); + if (!prefer_albumartist_ && !cache_item->metadata.albumartist.isEmpty()) { + params << Param(QString("%1[%2]").arg("albumArtist").arg(i), cache_item->metadata.albumartist); } - if (item->metadata.track > 0) { - params << Param(QString("%1[%2]").arg("trackNumber").arg(i), QString::number(item->metadata.track)); + if (cache_item->metadata.track > 0) { + params << Param(QString("%1[%2]").arg("trackNumber").arg(i), QString::number(cache_item->metadata.track)); } ++i; - if (i >= kScrobblesPerRequest) break; + if (cache_items_sent.count() >= kScrobblesPerRequest) break; } - if (!batch_ || i <= 0) return; + if (!batch_ || cache_items_sent.count() <= 0) return; submitted_ = true; QNetworkReply *reply = CreateRequest(params); - QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, list]() { ScrobbleRequestFinished(reply, list); }); + QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, cache_items_sent]() { ScrobbleRequestFinished(reply, cache_items_sent); }); } -void ScrobblingAPI20::ScrobbleRequestFinished(QNetworkReply *reply, const QList &list) { +void ScrobblingAPI20::ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items) { if (!replies_.contains(reply)) return; replies_.removeAll(reply); @@ -611,34 +541,17 @@ void ScrobblingAPI20::ScrobbleRequestFinished(QNetworkReply *reply, const QList< submitted_ = false; - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - cache_->ClearSent(list); + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + Error(error_message); + cache_->ClearSent(cache_items); submit_error_ = true; StartSubmit(); return; } - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - cache_->ClearSent(list); - submit_error_ = true; - StartSubmit(); - return; - } - - if (json_obj.contains("error") && json_obj.contains("message")) { - int error_code = json_obj["error"].toInt(); - QString error_message = json_obj["message"].toString(); - QString error_reason = QString("%1 (%2)").arg(error_message).arg(error_code); - Error(error_reason); - cache_->ClearSent(list); - submit_error_ = true; - StartSubmit(); - return; - } - - cache_->Flush(list); + cache_->Flush(cache_items); submit_error_ = false; if (!json_obj.contains("scrobbles")) { @@ -799,52 +712,32 @@ void ScrobblingAPI20::SendSingleScrobble(ScrobblerCacheItemPtr item) { } QNetworkReply *reply = CreateRequest(params); - QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, item]() { SingleScrobbleRequestFinished(reply, item->timestamp); }); + QObject::connect(reply, &QNetworkReply::finished, this, [this, reply, item]() { SingleScrobbleRequestFinished(reply, item); }); } -void ScrobblingAPI20::SingleScrobbleRequestFinished(QNetworkReply *reply, const quint64 timestamp) { +void ScrobblingAPI20::SingleScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtr cache_item) { if (!replies_.contains(reply)) return; replies_.removeAll(reply); QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - ScrobblerCacheItemPtr item = cache_->Get(timestamp); - if (!item) { - Error(QString("Received reply for non-existing cache entry %1.").arg(timestamp)); - return; - } - - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - item->sent = false; - return; - } - - QJsonObject json_obj = ExtractJsonObj(data); - if (json_obj.isEmpty()) { - item->sent = false; - return; - } - - if (json_obj.contains("error") && json_obj.contains("message")) { - int error_code = json_obj["error"].toInt(); - QString error_message = json_obj["message"].toString(); - QString error_reason = QString("%1 (%2)").arg(error_message).arg(error_code); - Error(error_reason); - item->sent = false; + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + Error(error_message); + cache_item->sent = false; return; } if (!json_obj.contains("scrobbles")) { Error("Json reply from server is missing scrobbles.", json_obj); - item->sent = false; + cache_item->sent = false; return; } - cache_->Remove(timestamp); - item = nullptr; + cache_->Remove(cache_item); QJsonValue value_scrobbles = json_obj["scrobbles"]; if (!value_scrobbles.isObject()) { @@ -963,13 +856,10 @@ void ScrobblingAPI20::LoveRequestFinished(QNetworkReply *reply) { QObject::disconnect(reply, nullptr, this, nullptr); reply->deleteLater(); - QByteArray data = GetReplyData(reply); - if (data.isEmpty()) { - return; - } - - QJsonObject json_obj = ExtractJsonObj(data, true); - if (json_obj.isEmpty()) { + QJsonObject json_obj; + QString error_message; + if (GetJsonObject(reply, json_obj, error_message) != ReplyResult::Success) { + Error(error_message); return; } @@ -1008,7 +898,10 @@ void ScrobblingAPI20::LoveRequestFinished(QNetworkReply *reply) { } void ScrobblingAPI20::AuthError(const QString &error) { + + qLog(Error) << name_ << error; emit AuthenticationComplete(false, error); + } void ScrobblingAPI20::Error(const QString &error, const QVariant &debug) { diff --git a/src/scrobbler/scrobblingapi20.h b/src/scrobbler/scrobblingapi20.h index 36716fd3a..015138330 100644 --- a/src/scrobbler/scrobblingapi20.h +++ b/src/scrobbler/scrobblingapi20.h @@ -79,11 +79,16 @@ class ScrobblingAPI20 : public ScrobblerService { void RedirectArrived(); void AuthenticateReplyFinished(QNetworkReply *reply); void UpdateNowPlayingRequestFinished(QNetworkReply *reply); - void ScrobbleRequestFinished(QNetworkReply *reply, const QList &list); - void SingleScrobbleRequestFinished(QNetworkReply *reply, const quint64 timestamp); + void ScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtrList cache_items); + void SingleScrobbleRequestFinished(QNetworkReply *reply, ScrobblerCacheItemPtr cache_item); void LoveRequestFinished(QNetworkReply *reply); private: + enum class ReplyResult { + Success, + ServerError, + APIError + }; enum class ScrobbleErrorCode { NoError = 1, @@ -120,12 +125,12 @@ class ScrobblingAPI20 : public ScrobblerService { static const int kScrobblesPerRequest; QNetworkReply *CreateRequest(const ParamList &request_params); - QByteArray GetReplyData(QNetworkReply *reply); + ReplyResult GetJsonObject(QNetworkReply *reply, QJsonObject &json_obj, QString &error_description); void RequestSession(const QString &token); void AuthError(const QString &error); void SendSingleScrobble(ScrobblerCacheItemPtr item); - void Error(const QString &error, const QVariant &debug = QVariant()) override; + void Error(const QString &error, const QVariant &debug = QVariant()); static QString ErrorString(const ScrobbleErrorCode error); void StartSubmit(const bool initial = false) override; void CheckScrobblePrevSong(); diff --git a/src/scrobbler/subsonicscrobbler.cpp b/src/scrobbler/subsonicscrobbler.cpp index 04f34af25..4c51ddf7a 100644 --- a/src/scrobbler/subsonicscrobbler.cpp +++ b/src/scrobbler/subsonicscrobbler.cpp @@ -109,10 +109,3 @@ void SubsonicScrobbler::Submit() { service_->Scrobble(song_playing_.song_id(), true, time_); } - -void SubsonicScrobbler::Error(const QString &error, const QVariant &debug) { - - qLog(Error) << "SubsonicScrobbler:" << error; - if (debug.isValid()) qLog(Debug) << debug; - -} diff --git a/src/scrobbler/subsonicscrobbler.h b/src/scrobbler/subsonicscrobbler.h index 6c819393b..d732f83e9 100644 --- a/src/scrobbler/subsonicscrobbler.h +++ b/src/scrobbler/subsonicscrobbler.h @@ -52,7 +52,6 @@ class SubsonicScrobbler : public ScrobblerService { void UpdateNowPlaying(const Song &song) override; void ClearPlaying() override; void Scrobble(const Song &song) override; - void Error(const QString &error, const QVariant &debug = QVariant()) override; void StartSubmit(const bool initial = false) override { Q_UNUSED(initial) } void Submitted() override { submitted_ = true; }