From a398fbe62df648526dfeb038692470bf2a233e59 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= <snejus@protonmail.com>
Date: Tue, 27 Aug 2024 18:14:38 +0100
Subject: [PATCH] LRCLib: Improve exception handling

---
 beetsplug/lyrics.py         | 41 ++++++++++++++++++++++---------------
 test/plugins/test_lyrics.py | 26 +++++++++++++++++++++--
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/beetsplug/lyrics.py b/beetsplug/lyrics.py
index 736b71ea5..23cf9c68c 100644
--- a/beetsplug/lyrics.py
+++ b/beetsplug/lyrics.py
@@ -284,6 +284,19 @@ class LRCLibItem(TypedDict):
 class LRCLib(Backend):
     base_url = "https://lrclib.net/api/search"
 
+    def warn(self, message: str, *args) -> None:
+        """Log a warning message with the class name."""
+        self._log.warning(f"{self.__class__.__name__}: {message}", *args)
+
+    def fetch_json(self, *args, **kwargs):
+        """Wrap the request method to raise an exception on HTTP errors."""
+        kwargs.setdefault("timeout", 10)
+        kwargs.setdefault("headers", {"User-Agent": USER_AGENT})
+        r = requests.get(*args, **kwargs)
+        r.raise_for_status()
+
+        return r.json()
+
     @staticmethod
     def get_rank(
         target_duration: float, item: LRCLibItem
@@ -327,25 +340,21 @@ class LRCLib(Backend):
             params["duration"] = length
 
         try:
-            response = requests.get(
-                self.base_url,
-                params=params,
-                timeout=10,
-            )
-            data: list[LRCLibItem] = response.json()
-        except (requests.RequestException, json.decoder.JSONDecodeError) as exc:
-            self._log.debug("LRCLib API request failed: {0}", exc)
-            return None
+            data = self.fetch_json(self.base_url, params=params)
+        except requests.JSONDecodeError:
+            self.warn("Could not decode response JSON data")
+        except requests.RequestException as exc:
+            self.warn("Request error: {}", exc)
+        else:
+            if data:
+                item = self.pick_lyrics(length, data)
 
-        if not data:
-            return None
+                if self.config["synced"] and (synced := item["syncedLyrics"]):
+                    return synced
 
-        item = self.pick_lyrics(length, data)
+                return item["plainLyrics"]
 
-        if self.config["synced"] and (synced_lyrics := item["syncedLyrics"]):
-            return synced_lyrics
-
-        return item["plainLyrics"]
+        return None
 
 
 class DirectBackend(Backend):
diff --git a/test/plugins/test_lyrics.py b/test/plugins/test_lyrics.py
index dcffeede3..dceec79b5 100644
--- a/test/plugins/test_lyrics.py
+++ b/test/plugins/test_lyrics.py
@@ -14,7 +14,9 @@
 
 """Tests for the 'lyrics' plugin."""
 
+import re
 from functools import partial
+from http import HTTPStatus
 
 import pytest
 
@@ -360,8 +362,12 @@ class TestLRCLibLyrics(LyricsBackendTest):
         return "lrclib"
 
     @pytest.fixture
-    def fetch_lyrics(self, backend, requests_mock, response_data):
-        requests_mock.get(backend.base_url, json=response_data)
+    def request_kwargs(self, response_data):
+        return {"json": response_data}
+
+    @pytest.fixture
+    def fetch_lyrics(self, backend, requests_mock, request_kwargs):
+        requests_mock.get(backend.base_url, **request_kwargs)
 
         return partial(backend.fetch, "la", "la", "la", self.ITEM_DURATION)
 
@@ -407,3 +413,19 @@ class TestLRCLibLyrics(LyricsBackendTest):
     @pytest.mark.parametrize("plugin_config", [{"synced": True}])
     def test_fetch_lyrics(self, fetch_lyrics, expected_lyrics):
         assert fetch_lyrics() == expected_lyrics
+
+    @pytest.mark.parametrize(
+        "request_kwargs, expected_log_match",
+        [
+            (
+                {"status_code": HTTPStatus.BAD_GATEWAY},
+                r"LRCLib: Request error: 502",
+            ),
+            ({"text": "invalid"}, r"LRCLib: Could not decode.*JSON"),
+        ],
+    )
+    def test_error(self, caplog, fetch_lyrics, expected_log_match):
+        assert fetch_lyrics() is None
+        assert caplog.messages
+        assert (last_log := caplog.messages[-1])
+        assert re.search(expected_log_match, last_log, re.I)