Improve Deezer error handling #4984
Conversation
|
Since I recently work a little with Deezer as my metadata source, I cherry-picked this PR into my personal main-branch. Remind me how that goes in a couple of days/weeks, I'm sure we can merge this then! :-) Thanks for another useful submission @arsaboo 💯 |
| @@ -75,13 +75,12 @@ 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() | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
Ok...I see now what I was missing. I updated the code to handle the error key.
| @@ -75,13 +75,12 @@ 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() | |||
There was a problem hiding this comment.
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?)
|
fixed the handling of Deezer API errors, i.e., when the response includes |
beetsplug/deezer.py
Outdated
| data = response.json() | ||
| except ( | ||
| requests.ConnectionError, | ||
| requests.exceptions.RequestException, |
There was a problem hiding this comment.
stable and latest docs say both of these exceptions are in requests already: https://requests.readthedocs.io/en/stable/api/#exceptions
does your testing tell you otherwise?
not sure about older versions though, no docs for them online
There was a problem hiding this comment.
ah and now found this older comment, so actually this still applies: #4984 (comment)
There was a problem hiding this comment.
hmm @sampsyo didn't you mean the other way round in that comment? If we catch RequestException we'd also catch ConnectionError? And in fact catch each and every exception, since they all inherit from RequestException
There was a problem hiding this comment.
Indeed, it looks like that should catch all requests exception https://stackoverflow.com/questions/16511337/correct-way-to-try-except-using-python-requests-module
Will make the change tomorrow morning.
There was a problem hiding this comment.
but hey @arsaboo I wanted to add: Just my thoughts and I don't really know if catching all exceptions requests offers is the perfect idea! Maybe @sampsyo did mean it like he wrote in the first place (ConnectionError catching is enough).
The rule of thumb for catching errors often is: Don't catch everything.
Maybe there are situations where we actually want the importer to crash and tell the beets user that there is a serious problem. If we catch everything then in a normal important run without seeing logs the importer would for example simply fail silently and the user might not even see the reason why they are not getting any results from deezer!!! just think about that and look at this page again, it shows closely which errors derive from which:https://requests.readthedocs.io/en/stable/_modules/requests/exceptions/#ConnectionError
By catching RequestException we would even catch any timeout-related exceptions, including when we get rate-limited! In that case we would just move on siltently ignoring it. But maybe for this case you are planning a subsequent PR for catching timeout stuff, rigfht before the RequestException? (if that's possible to give a preference what's catched first, no expert here, need to read the docs?)
There was a problem hiding this comment.
Often these are not serious problems....the API errors out or something trivial (and often resolves in the next run). In any case, I will wait for a consensus before making changes.
Also, we are not masking anything....we are telling the user that there was an error. Instead of self._log.debug, we can make it self._log.error so that it is not missed.
There was a problem hiding this comment.
ah yeah that's true! making it error-level actually shows it always! yeah good point!
There was a problem hiding this comment.
cool...updated the PR.
Description
Fixes #4983
To Do