From 337e5f454809e00c535aae22153bd2481da566a3 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Sat, 7 Oct 2023 18:19:58 -0400 Subject: [PATCH 01/14] gracefully handle Spotify timeout error --- beetsplug/spotify.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 0ada97e8b..55e0fe2ed 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -171,11 +171,16 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): :return: JSON data for the class:`Response ` object. :rtype: dict """ - response = request_type( - url, - headers={'Authorization': f'Bearer {self.access_token}'}, - params=params, - ) + try: + response = request_type( + url, + headers={'Authorization': f'Bearer {self.access_token}'}, + params=params, + ) + response.raise_for_status() + except requests.exceptions.ReadTimeout: + self._log.debug('ReadTimeout. Retrying.') + return self._handle_response(request_type, url, params=params) if response.status_code != 200: if 'token expired' in response.text: self._log.debug( From 36a92381e79b60ac7abad57aa87b252c4f0ad6b9 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Sat, 7 Oct 2023 18:22:39 -0400 Subject: [PATCH 02/14] add a default timeout of 30 seconds --- beetsplug/spotify.py | 1 + 1 file changed, 1 insertion(+) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 55e0fe2ed..dd44ddd1b 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -176,6 +176,7 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): url, headers={'Authorization': f'Bearer {self.access_token}'}, params=params, + timeout=30, ) response.raise_for_status() except requests.exceptions.ReadTimeout: From d09af885ed8ef0b04c721586756c4baab4a33c1d Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Sat, 7 Oct 2023 18:23:16 -0400 Subject: [PATCH 03/14] Change the log to error --- beetsplug/spotify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index dd44ddd1b..9cd210e5a 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -180,7 +180,7 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): ) response.raise_for_status() except requests.exceptions.ReadTimeout: - self._log.debug('ReadTimeout. Retrying.') + self._log.error('ReadTimeout. Retrying.') return self._handle_response(request_type, url, params=params) if response.status_code != 200: if 'token expired' in response.text: From 97ad4baee4c25686c8cc04a5ba52f739c72c2a20 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Sat, 7 Oct 2023 18:30:03 -0400 Subject: [PATCH 04/14] Update spotify.py --- beetsplug/spotify.py | 1 - 1 file changed, 1 deletion(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 9cd210e5a..1131dbc28 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -178,7 +178,6 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): params=params, timeout=30, ) - response.raise_for_status() except requests.exceptions.ReadTimeout: self._log.error('ReadTimeout. Retrying.') return self._handle_response(request_type, url, params=params) From e68369bc8a93d8a2390c51c548be9631dd8daab6 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Sat, 7 Oct 2023 18:59:02 -0400 Subject: [PATCH 05/14] Change retry logic --- beetsplug/spotify.py | 77 +++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 34 deletions(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 1131dbc28..9ad87a13e 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -171,42 +171,51 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): :return: JSON data for the class:`Response ` object. :rtype: dict """ - try: - response = request_type( - url, - headers={'Authorization': f'Bearer {self.access_token}'}, - params=params, - timeout=30, - ) - except requests.exceptions.ReadTimeout: - self._log.error('ReadTimeout. Retrying.') - return self._handle_response(request_type, url, params=params) - if response.status_code != 200: - if 'token expired' in response.text: - self._log.debug( - '{} access token has expired. Reauthenticating.', - self.data_source, + retries = 0 + while retries < 5: + try: + response = request_type( + url, + headers={'Authorization': f'Bearer {self.access_token}'}, + params=params, + timeout=10, ) - self._authenticate() - return self._handle_response(request_type, url, params=params) - elif response.status_code == 429: - seconds = response.headers.get('Retry-After', - DEFAULT_WAITING_TIME) - self._log.debug('Too many API requests. Retrying after {} \ - seconds.', seconds) - time.sleep(int(seconds) + 1) - return self._handle_response(request_type, url, params=params) - elif response.status_code == 404: - raise SpotifyAPIError("API Error: {}\nURL: {}\nparams: {}". - format(response.status_code, url, - params)) - else: - raise ui.UserError( - '{} API error:\n{}\nURL:\n{}\nparams:\n{}'.format( - self.data_source, response.text, url, params + response.raise_for_status() + return response.json() + except requests.exceptions.ReadTimeout: + self._log.error('ReadTimeout. Retrying.') + retries += 1 + time.sleep(1) + except requests.exceptions.RequestException as e: + if 'token expired' in str(e): + self._log.debug( + f'{self.data_source} access token has expired.' + f' Reauthenticating.' ) - ) - return response.json() + self._authenticate() + elif isinstance(e, requests.exceptions.HTTPError): + if e.response.status_code == 429: + seconds = e.response.headers.get('Retry-After', + DEFAULT_WAITING_TIME) + self._log.debug(f'Too many API requests. ' + f'Retrying after {seconds} seconds.') + time.sleep(int(seconds) + 1) + retries += 1 + elif e.response.status_code == 404: + raise SpotifyAPIError(f'API Error: ' + f'{e.response.status_code}\n' + f'URL: {url}\nparams: {params}') + else: + raise ui.UserError( + f'{self.data_source} API error:\n' + f'{e.response.text}\nURL:\n{url}\n' + f'params:\n{params}' + ) + else: + self._log.error(f'Request failed. Retrying. Error: {e}') + retries += 1 + time.sleep(1) + raise ui.UserError('Maximum retries exceeded.') def album_for_id(self, album_id): """Fetch an album by its Spotify ID or URL and return an From d87e3e6628c78193fa7719882bbfaa0755c24fee Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Mon, 9 Oct 2023 08:59:15 -0400 Subject: [PATCH 06/14] Update error code --- beetsplug/spotify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 9ad87a13e..b5a46714d 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -187,7 +187,7 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): retries += 1 time.sleep(1) except requests.exceptions.RequestException as e: - if 'token expired' in str(e): + if e.response.status_code == 401: self._log.debug( f'{self.data_source} access token has expired.' f' Reauthenticating.' From 391b95b912af158e5807606970d26a0e33015732 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Mon, 9 Oct 2023 09:29:21 -0400 Subject: [PATCH 07/14] Update changelog.rst --- docs/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index afb6cc3de..9f99984e7 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -128,6 +128,8 @@ New features: Bug fixes: +* :doc:`/plugins/spotify`: Fix a crash when the Spotify API timeouts or does not return a `Retry-After` interval. + :bug:`4942` * :doc:`/plugins/scrub`: Fixed the import behavior where scrubbed database tags were restored to newly imported tracks with config settings ``scrub.auto: yes`` and ``import.write: no``. From dc6def34a7d58678a4fe7287c231ecde9894a6ea Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Mon, 9 Oct 2023 09:30:08 -0400 Subject: [PATCH 08/14] Update changelog.rst --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 9f99984e7..31274d8e9 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -128,7 +128,7 @@ New features: Bug fixes: -* :doc:`/plugins/spotify`: Fix a crash when the Spotify API timeouts or does not return a `Retry-After` interval. +* Fix a crash when the Spotify API timeouts or does not return a `Retry-After` interval. :bug:`4942` * :doc:`/plugins/scrub`: Fixed the import behavior where scrubbed database tags were restored to newly imported tracks with config settings ``scrub.auto: yes`` From 989ee5a69c2be11bd273d086955912e56ad779ec Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Tue, 10 Oct 2023 14:49:50 -0400 Subject: [PATCH 09/14] Remove retry logic --- beetsplug/spotify.py | 73 ++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 44 deletions(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index b5a46714d..bb1aaff4c 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -171,51 +171,36 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): :return: JSON data for the class:`Response ` object. :rtype: dict """ - retries = 0 - while retries < 5: - try: - response = request_type( - url, - headers={'Authorization': f'Bearer {self.access_token}'}, - params=params, - timeout=10, + try: + response = request_type( + url, + headers={'Authorization': f'Bearer {self.access_token}'}, + params=params, + timeout=10, + ) + response.raise_for_status() + return response.json() + except requests.exceptions.ReadTimeout: + self._log.error('ReadTimeout.') + raise SpotifyAPIError('Request timed out.') + except requests.exceptions.RequestException as e: + if e.response.status_code == 401: + self._log.debug( + f'{self.data_source} access token has expired. ' + f'Reauthenticating.' ) - response.raise_for_status() - return response.json() - except requests.exceptions.ReadTimeout: - self._log.error('ReadTimeout. Retrying.') - retries += 1 - time.sleep(1) - except requests.exceptions.RequestException as e: - if e.response.status_code == 401: - self._log.debug( - f'{self.data_source} access token has expired.' - f' Reauthenticating.' - ) - self._authenticate() - elif isinstance(e, requests.exceptions.HTTPError): - if e.response.status_code == 429: - seconds = e.response.headers.get('Retry-After', - DEFAULT_WAITING_TIME) - self._log.debug(f'Too many API requests. ' - f'Retrying after {seconds} seconds.') - time.sleep(int(seconds) + 1) - retries += 1 - elif e.response.status_code == 404: - raise SpotifyAPIError(f'API Error: ' - f'{e.response.status_code}\n' - f'URL: {url}\nparams: {params}') - else: - raise ui.UserError( - f'{self.data_source} API error:\n' - f'{e.response.text}\nURL:\n{url}\n' - f'params:\n{params}' - ) - else: - self._log.error(f'Request failed. Retrying. Error: {e}') - retries += 1 - time.sleep(1) - raise ui.UserError('Maximum retries exceeded.') + self._authenticate() + elif e.response.status_code == 404: + raise SpotifyAPIError(f'API Error: {e.response.status_code}\n' + f'URL: {url}\nparams: {params}') + elif e.response is not None: + raise SpotifyAPIError( + f'{self.data_source} API error:\n{e.response.text}\n' + f'URL:\n{url}\nparams:\n{params}' + ) + else: + self._log.error(f'Request failed. Error: {e}') + raise SpotifyAPIError('Request failed.') def album_for_id(self, album_id): """Fetch an album by its Spotify ID or URL and return an From b69de85d4935a45e22ec285a70e6521dd3feeef2 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Tue, 10 Oct 2023 15:20:29 -0400 Subject: [PATCH 10/14] Add 429 and 503 error handling --- beetsplug/spotify.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index bb1aaff4c..be6e3ada4 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -193,6 +193,16 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): elif e.response.status_code == 404: raise SpotifyAPIError(f'API Error: {e.response.status_code}\n' f'URL: {url}\nparams: {params}') + elif e.response.status_code == 429: + seconds = response.headers.get('Retry-After', + DEFAULT_WAITING_TIME) + self._log.debug('Too many API requests. Retrying after {} \ + seconds.', seconds) + time.sleep(int(seconds) + 1) + return self._handle_response(request_type, url, params=params) + elif e.response.status_code == 503: + self._log.error('Service Unavailable.') + raise SpotifyAPIError('Service Unavailable.') elif e.response is not None: raise SpotifyAPIError( f'{self.data_source} API error:\n{e.response.text}\n' @@ -655,8 +665,8 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): track_data = self._handle_response( requests.get, self.track_url + track_id ) - self._log.debug('track_data: {}', track_data['popularity']) - return track_data['popularity'] + self._log.debug('track_data: {}', track_data.get('popularity')) + return track_data.get('popularity') def track_audio_features(self, track_id=None): """Fetch track audio features by its Spotify ID.""" From 4b955152c5bc4e534a8492d0538c80ee2c7f5a25 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Tue, 10 Oct 2023 15:22:10 -0400 Subject: [PATCH 11/14] change to f-string --- beetsplug/spotify.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index be6e3ada4..c2bf38310 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -196,8 +196,8 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): elif e.response.status_code == 429: seconds = response.headers.get('Retry-After', DEFAULT_WAITING_TIME) - self._log.debug('Too many API requests. Retrying after {} \ - seconds.', seconds) + self._log.debug(f'Too many API requests. Retrying after' + f'{seconds} seconds.') time.sleep(int(seconds) + 1) return self._handle_response(request_type, url, params=params) elif e.response.status_code == 503: From d9a99ff5f78284ac3047d0571378eb77347bc000 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Tue, 10 Oct 2023 15:42:38 -0400 Subject: [PATCH 12/14] add missing space in the log message --- beetsplug/spotify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index c2bf38310..325949626 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -196,7 +196,7 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): elif e.response.status_code == 429: seconds = response.headers.get('Retry-After', DEFAULT_WAITING_TIME) - self._log.debug(f'Too many API requests. Retrying after' + self._log.debug(f'Too many API requests. Retrying after ' f'{seconds} seconds.') time.sleep(int(seconds) + 1) return self._handle_response(request_type, url, params=params) From e064492bd8343fb91b8db6305f5fc132b777165b Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Wed, 11 Oct 2023 20:37:12 -0400 Subject: [PATCH 13/14] additional error handling --- beetsplug/spotify.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 325949626..160e7300e 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -228,6 +228,8 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): album_data = self._handle_response( requests.get, self.album_url + spotify_id ) + if album_data is None: + return None if album_data['name'] == "": self._log.debug("Album removed from Spotify: {}", album_id) return None From 7fd470a793f3ebed7465df3a4629257943c1bd90 Mon Sep 17 00:00:00 2001 From: Alok Saboo Date: Wed, 11 Oct 2023 20:39:53 -0400 Subject: [PATCH 14/14] resume after reauthentication --- beetsplug/spotify.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/beetsplug/spotify.py b/beetsplug/spotify.py index 160e7300e..239e143e2 100644 --- a/beetsplug/spotify.py +++ b/beetsplug/spotify.py @@ -190,6 +190,7 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): f'Reauthenticating.' ) self._authenticate() + return self._handle_response(request_type, url, params=params) elif e.response.status_code == 404: raise SpotifyAPIError(f'API Error: {e.response.status_code}\n' f'URL: {url}\nparams: {params}') @@ -228,8 +229,6 @@ class SpotifyPlugin(MetadataSourcePlugin, BeetsPlugin): album_data = self._handle_response( requests.get, self.album_url + spotify_id ) - if album_data is None: - return None if album_data['name'] == "": self._log.debug("Album removed from Spotify: {}", album_id) return None