Improve discogs cover provider

src/covermanager/discogscoverprovider.cpp

@@ -25,8 +25,6 @@
 #include <QByteArray>
 #include <QList>
 #include <QPair>
-#include <QMap>
-#include <QSet>
 #include <QVariant>
 #include <QString>
 #include <QStringBuilder>
@@ -38,7 +36,8 @@
 #include <QNetworkReply>
 #include <QJsonDocument>
 #include <QJsonObject>
-#include <QVector>
+#include <QJsonValue>
+#include <QJsonArray>
 
 #include "core/closure.h"
 #include "core/logging.h"
@@ -115,7 +114,6 @@ void DiscogsCoverProvider::SendSearchRequest(DiscogsCoverSearchContext *s_ctx) {
   }
 
   QUrlQuery url_query;
-  QUrl url(kUrlSearch);
   QStringList query_items;
 
   // Encode the arguments
@@ -125,6 +123,9 @@ void DiscogsCoverProvider::SendSearchRequest(DiscogsCoverSearchContext *s_ctx) {
     url_query.addQueryItem(encoded_arg.first, encoded_arg.second);
   }
 
+  QUrl url(kUrlSearch);
+  url.setQuery(url_query);
+
   // Sign the request
   const QByteArray data_to_sign = QString("GET\n%1\n%2\n%3").arg(url.host(), url.path(), query_items.join("&")).toUtf8();
   const QByteArray signature(Utilities::HmacSha256(QByteArray::fromBase64(kSecretKeyB64), data_to_sign));
@@ -132,10 +133,7 @@ void DiscogsCoverProvider::SendSearchRequest(DiscogsCoverSearchContext *s_ctx) {
   // Add the signature to the request
   url_query.addQueryItem("Signature", QUrl::toPercentEncoding(signature.toBase64()));
 
-  url.setQuery(url_query);
   QNetworkReply *reply = network_->get(QNetworkRequest(url));
-
-  NewClosure(reply, SIGNAL(error(QNetworkReply::NetworkError)), this, SLOT(SearchRequestError(QNetworkReply::NetworkError, QNetworkReply*, int)), reply, s_ctx->id);
   NewClosure(reply, SIGNAL(finished()), this, SLOT(HandleSearchReply(QNetworkReply*, int)), reply, s_ctx->id);
 
 }
@@ -171,61 +169,147 @@ void DiscogsCoverProvider::SendReleaseRequest(DiscogsCoverSearchContext *s_ctx,
   url_query.addQueryItem("Signature", QUrl::toPercentEncoding(signature.toBase64()));
 
   url.setQuery(url_query);
-  QNetworkReply *reply = network_->get(QNetworkRequest(url));
 
-  NewClosure(reply, SIGNAL(error(QNetworkReply::NetworkError)), this, SLOT(ReleaseRequestError(QNetworkReply::NetworkError, QNetworkReply*, int, int)), reply, s_ctx->id, r_ctx->id);
+  QNetworkReply *reply = network_->get(QNetworkRequest(url));
   NewClosure(reply, SIGNAL(finished()), this, SLOT(HandleReleaseReply(QNetworkReply*, int, int)), reply, s_ctx->id, r_ctx->id);
 
 }
 
+QByteArray DiscogsCoverProvider::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.
+      QString failure_reason = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
+      Error(failure_reason);
+    }
+    else {
+      // See if there is Json data containing "message" - then use that instead.
+      data = reply->readAll();
+      QJsonParseError error;
+      QJsonDocument json_doc = QJsonDocument::fromJson(data, &error);
+      QString failure_reason;
+      if (error.error == QJsonParseError::NoError && !json_doc.isNull() && !json_doc.isEmpty() && json_doc.isObject()) {
+        QJsonObject json_obj = json_doc.object();
+        if (json_obj.contains("message")) {
+          failure_reason = json_obj["message"].toString();
+        }
+        else {
+          failure_reason = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
+        }
+      }
+      else {
+        failure_reason = QString("%1 (%2)").arg(reply->errorString()).arg(reply->error());
+      }
+      Error(failure_reason);
+    }
+    return QByteArray();
+  }
+
+  return data;
+
+}
+
+QJsonObject DiscogsCoverProvider::ExtractJsonObj(const QByteArray &data) {
+
+  QJsonParseError error;
+  QJsonDocument json_doc = QJsonDocument::fromJson(data, &error);
+
+  if (error.error != QJsonParseError::NoError) {
+    Error("Reply from server missing Json data.", data);
+    return QJsonObject();
+  }
+  if (json_doc.isNull() || 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()) {
+    Error("Received empty Json object.", json_doc);
+    return QJsonObject();
+  }
+
+  return json_obj;
+
+}
+
+QJsonValue DiscogsCoverProvider::ExtractData(const QByteArray &data, const QString name, const bool silent) {
+
+  QJsonObject json_obj = ExtractJsonObj(data);
+  if (json_obj.isEmpty()) return QJsonObject();
+
+  if (json_obj.contains(name)) {
+    QJsonValue json_results = json_obj[name];
+    return json_results;
+  }
+  else if (json_obj.contains("message")) {
+    QString message = json_obj["message"].toString();
+    Error(QString("%1").arg(message));
+  }
+  else {
+    if (!silent) Error(QString("Json reply is missing \"%1\".").arg(name), json_obj);
+  }
+  return QJsonValue();
+
+}
+
 void DiscogsCoverProvider::HandleSearchReply(QNetworkReply *reply, int s_id) {
 
   reply->deleteLater();
 
   if (!requests_search_.contains(s_id)) {
-    //qLog(Error) << "Discogs: Got reply for cancelled request: " << s_id;
+    Error(QString("Got reply for cancelled request: %1").arg(s_id));
     return;
   }
   DiscogsCoverSearchContext *s_ctx = requests_search_.value(s_id);
 
-  QJsonDocument json_doc = QJsonDocument::fromJson(reply->readAll());
+  QByteArray data = GetReplyData(reply);
-  if ((json_doc.isNull()) || (!json_doc.isObject())) {
+
-    qLog(Error) << "Discogs: Failed to create JSON doc.";
+  QJsonValue json_value = ExtractData(data, "results");
+  if (!json_value.isArray()) {
     EndSearch(s_ctx);
     return;
   }
 
-  QJsonObject json_obj = json_doc.object();
+  QJsonArray json_results = json_value.toArray();
-  if (json_obj.isEmpty()) {
+  if (json_results.isEmpty()) {
-    qLog(Error) << "Discogs: JSON object is empty.";
+    Error("Json array is empty.");
     EndSearch(s_ctx);
     return;
   }
 
-  QVariantMap reply_map = json_obj.toVariantMap();
-  if (!reply_map.contains("results")) {
-    //qLog(Error) << "Discogs: Search reply from server is missing JSON results.";
-    //qLog(Error) << "Discogs: Map dump:";
-    //qLog(Error) << reply_map;
-    EndSearch(s_ctx);
-    return;
-  }
-
-  QVariantList results = reply_map["results"].toList();
   int i = 0;
-
+  for (const QJsonValue &value : json_results) {
-  for (const QVariant &result : results) {
+    if (!value.isObject()) {
-    QVariantMap result_map = result.toMap();
+      Error("Invalid Json reply, data is not an object.", value);
-    if ((result_map.contains("id")) && (result_map.contains("resource_url"))) {
+      continue;
-      int r_id = result_map["id"].toInt();
-      QString title = result_map["title"].toString();
-      QString resource_url = result_map["resource_url"].toString();
-      if (resource_url.isEmpty()) continue;
-      StartRelease(s_ctx, r_id, resource_url);
-      i++;
     }
+    QJsonObject json_obj = value.toObject();
+    if (!json_obj.contains("id") || !json_obj.contains("title") || !json_obj.contains("resource_url")) {
+      Error("Invalid Json reply, value is missing ID, title or resource_url.", json_obj);
+      continue;
+    }
+    int r_id = json_obj["id"].toInt();
+    QString title = json_obj["title"].toString();
+    QString resource_url = json_obj["resource_url"].toString();
+    if (resource_url.isEmpty()) continue;
+    StartRelease(s_ctx, r_id, resource_url);
+    i++;
+  }
+
+  if (i <= 0) {
+    Error("Ending search with no results.");
+    EndSearch(s_ctx);
   }
-  if (i <= 0) EndSearch(s_ctx);
 
 }
 
@@ -234,88 +318,58 @@ void DiscogsCoverProvider::HandleReleaseReply(QNetworkReply *reply, int s_id, in
   reply->deleteLater();
 
   if (!requests_release_.contains(r_id)) {
-    //qLog(Error) << "Discogs: Got reply for cancelled request: " << r_id;
+    Error(QString("Got reply for cancelled request: %1 %2").arg(s_id).arg(r_id));
     return;
   }
   DiscogsCoverReleaseContext *r_ctx = requests_release_.value(r_id);
 
   if (!requests_search_.contains(s_id)) {
-    //qLog(Error) << "Discogs: Got reply for cancelled request: " << s_id << " " << r_id;
+    Error(QString("Got reply for cancelled request: %1 %2").arg(s_id).arg(r_id));
     EndSearch(r_ctx);
     return;
   }
   DiscogsCoverSearchContext *s_ctx = requests_search_.value(s_id);
 
-  QJsonDocument json_doc = QJsonDocument::fromJson(reply->readAll());
+  QByteArray data = GetReplyData(reply);
-  if ((json_doc.isNull()) || (!json_doc.isObject())) {
+
-    qLog(Error) << "Discogs: Failed to create JSON doc.";
+  QJsonValue json_value = ExtractData(data, "images", true);
+  if (!json_value.isArray()) {
     EndSearch(s_ctx, r_ctx);
     return;
   }
 
-  QJsonObject json_obj = json_doc.object();
+  QJsonArray json_images = json_value.toArray();
-  if (json_obj.isEmpty()) {
+  if (json_images.isEmpty()) {
-    qLog(Error) << "Discogs: JSON object is empty.";
+    Error("Json array is empty.");
     EndSearch(s_ctx, r_ctx);
     return;
   }
 
-  QVariantMap reply_map = json_obj.toVariantMap();
+  int i = 0;
-  if (!reply_map.contains("images")) {
+  for (const QJsonValue &value : json_images) {
-    //qLog(Error) << "Discogs: Search reply from server is missing JSON images.";
+    if (!value.isObject()) {
-    //qLog(Error) << "Discogs: Map dump:";
+      Error("Invalid Json reply, value is not an object.", value);
-    //qLog(Error) << reply_map;
+      continue;
-    EndSearch(s_ctx, r_ctx);
+    }
-    return;
+    QJsonObject json_obj = value.toObject();
-  }
+    if (!json_obj.contains("type") || !json_obj.contains("resource_url")) {
-
+      Error("Invalid Json reply, value is missing ID or resource_url.", json_obj);
-  QVariantList results = reply_map["images"].toList();
+      continue;
-
+    }
-  for (const QVariant &result : results) {
-    QVariantMap result_map = result.toMap();
     CoverSearchResult cover_result;
     cover_result.description = s_ctx->title;
-
+    QString type = json_obj["type"].toString();
-    if (result_map.contains("type")) {
+    i++;
-      QString type = result_map["type"].toString();
+    if (type != "primary") {
-      if (type != "primary") continue;
+      continue;
-    }
-    if (result_map.contains("resource_url")) {
-      cover_result.image_url = QUrl(result_map["resource_url"].toString());
     }
+    cover_result.image_url = QUrl(json_obj["resource_url"].toString());
     if (cover_result.image_url.isEmpty()) continue;
     s_ctx->results.append(cover_result);
   }
-
+  if (i <= 0) {
-  EndSearch(s_ctx, r_ctx);
+    Error("Ending search with no results.");
-
-}
-
-void DiscogsCoverProvider::SearchRequestError(QNetworkReply::NetworkError error, QNetworkReply *reply, int s_id) {
-
-  if (!requests_search_.contains(s_id)) {
-    //qLog(Error) << "Discogs: got reply for cancelled request: " << s_id;
-    return;
   }
-  DiscogsCoverSearchContext *s_ctx = requests_search_.value(s_id);
-  EndSearch(s_ctx);
-
-}
-
-void DiscogsCoverProvider::ReleaseRequestError(QNetworkReply::NetworkError error, QNetworkReply *reply, int s_id, int r_id) {
-
-  if (!requests_release_.contains(r_id)) {
-    //qLog(Error) << "Discogs: got reply for cancelled request: " << s_id << r_id;
-    return;
-  }
-  DiscogsCoverReleaseContext *r_ctx = requests_release_.value(r_id);
-
-  if (!requests_search_.contains(s_id)) {
-    EndSearch(r_ctx);
-    //qLog(Error) << "Discogs: got reply for cancelled request: " << s_id << r_id;
-    return;
-  }
-  DiscogsCoverSearchContext *s_ctx = requests_search_.value(s_id);
 
   EndSearch(s_ctx, r_ctx);
 
@@ -324,19 +378,13 @@ void DiscogsCoverProvider::ReleaseRequestError(QNetworkReply::NetworkError error
 void DiscogsCoverProvider::EndSearch(DiscogsCoverSearchContext *s_ctx, DiscogsCoverReleaseContext *r_ctx) {
 
   delete requests_release_.take(r_ctx->id);
-
   s_ctx->r_count--;
-
-  //qLog(Debug) << "r_count: " << s_ctx->r_count;
-
   if (s_ctx->r_count <= 0) EndSearch(s_ctx);
 
 }
 
 void DiscogsCoverProvider::EndSearch(DiscogsCoverSearchContext *s_ctx) {
 
-  //qLog(Debug) << "Discogs: Finished." << s_ctx->id;
-
   requests_search_.remove(s_ctx->id);
   emit SearchFinished(s_ctx->id, s_ctx->results);
   delete s_ctx;
@@ -346,3 +394,8 @@ void DiscogsCoverProvider::EndSearch(DiscogsCoverSearchContext *s_ctx) {
 void DiscogsCoverProvider::EndSearch(DiscogsCoverReleaseContext* r_ctx) {
   delete requests_release_.take(r_ctx->id);
 }
+
+void DiscogsCoverProvider::Error(QString error, QVariant debug) {
+  qLog(Error) << "Discogs:" << error;
+  if (debug.isValid()) qLog(Debug) << debug;
+}

src/covermanager/discogscoverprovider.h

@@ -73,8 +73,6 @@ class DiscogsCoverProvider : public CoverProvider {
   void CancelSearch(int id);
 
  private slots:
-  void SearchRequestError(QNetworkReply::NetworkError error, QNetworkReply *reply, int s_id);
-  void ReleaseRequestError(QNetworkReply::NetworkError error, QNetworkReply *reply, int s_id, int r_id);
   void HandleSearchReply(QNetworkReply *reply, int s_id);
   void HandleReleaseReply(QNetworkReply *reply, int s_id, int r_id);
 
@@ -92,9 +90,13 @@ class DiscogsCoverProvider : public CoverProvider {
 
   void SendSearchRequest(DiscogsCoverSearchContext *s_ctx);
   void SendReleaseRequest(DiscogsCoverSearchContext *s_ctx, DiscogsCoverReleaseContext *r_ctx);
+  QByteArray GetReplyData(QNetworkReply *reply);
+  QJsonObject ExtractJsonObj(const QByteArray &data);
+  QJsonValue ExtractData(const QByteArray &data, const QString name, const bool silent = false);
   void EndSearch(DiscogsCoverSearchContext *s_ctx, DiscogsCoverReleaseContext *r_ctx);
   void EndSearch(DiscogsCoverSearchContext *s_ctx);
   void EndSearch(DiscogsCoverReleaseContext *r_ctx);
+  void Error(QString error, QVariant debug = QVariant());
 
 };