Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Deezer error handling #4984

Merged
merged 18 commits into from
Dec 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 39 additions & 23 deletions beetsplug/deezer.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ def func(lib, opts, args):

return [deezer_update_cmd]

def fetch_data(self, url):
try:
response = requests.get(url, timeout=10)
response.raise_for_status()
data = response.json()
except requests.exceptions.RequestException as e:
self._log.error("Error fetching data from {}\n Error: {}", url, e)
return None
if "error" in data:
self._log.error("Deezer API error: {}", data["error"]["message"])
return None
return data

def album_for_id(self, album_id):
"""Fetch an album by its Deezer ID or URL and return an
AlbumInfo object or None if the album is not found.
Expand All @@ -75,13 +88,8 @@ def album_for_id(self, album_id):
deezer_id = self._get_id("album", album_id, self.id_regex)
if deezer_id is None:
return None

album_data = requests.get(self.album_url + deezer_id).json()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct me if I’m wrong, but it seems like we lost this layer of error checking. That is, it seems like we should still check for the “error” key in the JSON response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try-except block should capture it. If there is an error key, it should be caught in except block. It is possible that we may have to capture other exceptions (once we know which ones), but that is easy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm; I don't think I quite see the idea here… what exception will be raised when there is an error key? (Is there some other code that checks for the existence of this key, since it doesn't seem to happen here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...I see now what I was missing. I updated the code to handle the error key.

if "error" in album_data:
self._log.debug(
f"Error fetching album {album_id}: "
f"{album_data['error']['message']}"
)
album_data = self.fetch_data(self.album_url + deezer_id)
if album_data is None:
return None
contributors = album_data.get("contributors")
if contributors is not None:
Expand All @@ -107,9 +115,14 @@ def album_for_id(self, album_id):
"Invalid `release_date` returned "
"by {} API: '{}'".format(self.data_source, release_date)
)

tracks_obj = requests.get(self.album_url + deezer_id + "/tracks").json()
tracks_data = tracks_obj["data"]
tracks_obj = self.fetch_data(self.album_url + deezer_id + "/tracks")
if tracks_obj is None:
return None
try:
tracks_data = tracks_obj["data"]
except KeyError:
self._log.debug("Error fetching album tracks for {}", deezer_id)
tracks_data = None
if not tracks_data:
return None
while "next" in tracks_obj:
Expand Down Expand Up @@ -192,21 +205,26 @@ def track_for_id(self, track_id=None, track_data=None):
deezer_id = self._get_id("track", track_id, self.id_regex)
if deezer_id is None:
return None
track_data = requests.get(self.track_url + deezer_id).json()
if "error" in track_data:
self._log.debug(
f"Error fetching track {track_id}: "
f"{track_data['error']['message']}"
)
track_data = self.fetch_data(self.track_url + deezer_id)
if track_data is None:
return None
track = self._get_track(track_data)

# Get album's tracks to set `track.index` (position on the entire
# release) and `track.medium_total` (total number of tracks on
# the track's disc).
album_tracks_data = requests.get(
album_tracks_obj = self.fetch_data(
self.album_url + str(track_data["album"]["id"]) + "/tracks"
).json()["data"]
)
if album_tracks_obj is None:
return None
try:
album_tracks_data = album_tracks_obj["data"]
except KeyError:
self._log.debug(
"Error fetching album tracks for {}", track_data["album"]["id"]
)
return None
medium_total = 0
for i, track_data in enumerate(album_tracks_data, start=1):
if track_data["disk_number"] == track.medium:
Expand Down Expand Up @@ -283,11 +301,9 @@ def deezerupdate(self, items, write):
self._log.debug("No deezer_track_id present for: {}", item)
continue
try:
rank = (
requests.get(f"{self.track_url}{deezer_track_id}")
.json()
.get("rank")
)
rank = self.fetch_data(
f"{self.track_url}{deezer_track_id}"
).get("rank")
self._log.debug(
"Deezer track: {} has {} rank", deezer_track_id, rank
)
Expand Down
2 changes: 2 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ New features:

Bug fixes:

* :doc:`/plugins/deezer`: Improve Deezer plugin error handling and set requests timeout to 10 seconds.
:bug:`4983`
* :doc:`/plugins/spotify`: Add bad gateway (502) error handling.
* :doc:`/plugins/spotify`: Add a limit of 3 retries, instead of retrying endlessly when the API is not available.
* Fix a crash when the Spotify API timeouts or does not return a `Retry-After` interval.
Expand Down
Loading