From ee417ba2a1e822e93e733085bb75e0403cbc6160 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 16:23:38 +0000 Subject: [PATCH] images: ignore bad image uris (Fixes #364) (#365) images: ignore bad image uris (Fixes #364) 304 response cannot set a body Improve test_cache_expired_with_etag --- mopidy_spotify/images.py | 48 ++++++++++++++++++++++------------------ mopidy_spotify/web.py | 2 +- tests/test_images.py | 35 +++++++++++++++++++++++------ tests/test_web.py | 13 +++++------ tox.ini | 2 +- 5 files changed, 63 insertions(+), 37 deletions(-) diff --git a/mopidy_spotify/images.py b/mopidy_spotify/images.py index 01adf2cd..7dec2d8f 100644 --- a/mopidy_spotify/images.py +++ b/mopidy_spotify/images.py @@ -15,7 +15,8 @@ def get_images(web_client, uris): result = {} uri_type_getter = operator.itemgetter("type") - uris = sorted((_parse_uri(u) for u in uris), key=uri_type_getter) + uris = (_parse_uri(u) for u in uris) + uris = sorted((u for u in uris if u), key=uri_type_getter) for uri_type, group in itertools.groupby(uris, uri_type_getter): batch = [] for uri in group: @@ -33,25 +34,27 @@ def get_images(web_client, uris): def _parse_uri(uri): - parsed_uri = urllib.parse.urlparse(uri) - uri_type, uri_id = None, None - - if parsed_uri.scheme == "spotify": - uri_type, uri_id = parsed_uri.path.split(":")[:2] - elif parsed_uri.scheme in ("http", "https"): - if parsed_uri.netloc in ("open.spotify.com", "play.spotify.com"): - uri_type, uri_id = parsed_uri.path.split("/")[1:3] - - supported_types = ("track", "album", "artist", "playlist") - if uri_type and uri_type in supported_types and uri_id: - return { - "uri": uri, - "type": uri_type, - "id": uri_id, - "key": (uri_type, uri_id), - } - - raise ValueError(f"Could not parse {repr(uri)} as a Spotify URI") + try: + parsed_uri = urllib.parse.urlparse(uri) + uri_type, uri_id = None, None + + if parsed_uri.scheme == "spotify": + uri_type, uri_id = parsed_uri.path.split(":")[:2] + elif parsed_uri.scheme in ("http", "https"): + if parsed_uri.netloc in ("open.spotify.com", "play.spotify.com"): + uri_type, uri_id = parsed_uri.path.split("/")[1:3] + + supported_types = ("track", "album", "artist", "playlist") + if uri_type and uri_type in supported_types and uri_id: + return { + "uri": uri, + "type": uri_type, + "id": uri_id, + "key": (uri_type, uri_id), + } + raise ValueError("Unknown error") + except Exception as e: + logger.exception(f"Could not parse {repr(uri)} as a Spotify URI ({e})") def _process_uri(web_client, uri): @@ -80,7 +83,10 @@ def _process_uris(web_client, uri_type, uris): if uri["key"] not in _cache: if uri_type == "track": - album_key = _parse_uri(item["album"]["uri"])["key"] + album = _parse_uri(item["album"]["uri"]) + if not album: + continue + album_key = album["key"] if album_key not in _cache: _cache[album_key] = tuple( _translate_image(i) for i in item["album"]["images"] diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index a937d061..b5ebea53 100644 --- a/mopidy_spotify/web.py +++ b/mopidy_spotify/web.py @@ -196,7 +196,7 @@ def _request_with_retries(self, method, url, *args, **kwargs): # Decide how long to sleep in the next iteration. backoff_time = backoff_time or (2**i * self._backoff_factor) - logger.debug( + logger.error( f"Retrying {prepared_request.url} in {backoff_time:.3f} " "seconds." ) diff --git a/tests/test_images.py b/tests/test_images.py index 0ead783a..cc67e9ad 100644 --- a/tests/test_images.py +++ b/tests/test_images.py @@ -127,6 +127,27 @@ def test_get_track_images(web_client_mock, img_provider): assert image.width == 640 +def test_get_track_images_bad_album_uri(web_client_mock, img_provider): + uris = ["spotify:track:41shEpOKyyadtG6lDclooa"] + + web_client_mock.get.return_value = { + "tracks": [ + { + "id": "41shEpOKyyadtG6lDclooa", + "album": { + "uri": "spotify:bad-data", + "images": [ + {"height": 640, "url": "img://1/a", "width": 640} + ], + }, + } + ] + } + + result = img_provider.get_images(uris) + assert result == {} + + def test_get_relinked_track_images(web_client_mock, img_provider): uris = ["spotify:track:4nqN0p0FjfH39G3hxeuKad"] @@ -167,7 +188,7 @@ def test_get_relinked_track_images(web_client_mock, img_provider): def test_get_playlist_image(web_client_mock, img_provider): - uris = ["spotify:playlist:41shEpOKyyadtG6lDclooa"] + uris = ["spotify:playlist:41shEpOKyyadtG6lDclooa", "foo:bar"] web_client_mock.get.return_value = { "id": "41shEpOKyyadtG6lDclooa", @@ -181,7 +202,7 @@ def test_get_playlist_image(web_client_mock, img_provider): ) assert len(result) == 1 - assert sorted(result.keys()) == sorted(uris) + assert sorted(result.keys()) == ["spotify:playlist:41shEpOKyyadtG6lDclooa"] assert len(result[uris[0]]) == 1 image = result[uris[0]][0] @@ -231,11 +252,11 @@ def test_max_50_ids_per_request(web_client_mock, img_provider): assert request_ids_2 == "50" -def test_invalid_uri_fails(img_provider): - with pytest.raises(ValueError) as exc: - img_provider.get_images(["foo:bar"]) - - assert str(exc.value) == "Could not parse 'foo:bar' as a Spotify URI" +def test_invalid_uri(img_provider, caplog): + with caplog.at_level(5): + result = img_provider.get_images(["foo:bar"]) + assert result == {} + assert "Could not parse 'foo:bar' as a Spotify URI" in caplog.text def test_no_uris_gives_no_results(img_provider): diff --git a/tests/test_web.py b/tests/test_web.py index 90358aa9..e589f1d7 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -620,9 +620,7 @@ def test_cache_normalised_query_string( assert "tracks/abc?b=bar&f=cat" in cache -@pytest.mark.parametrize( - "status,expected", [(304, "spotify:track:abc"), (200, "spotify:track:xyz")] -) +@pytest.mark.parametrize("status,unchanged", [(304, True), (200, False)]) @responses.activate def test_cache_expired_with_etag( web_response_mock_etag, @@ -630,23 +628,24 @@ def test_cache_expired_with_etag( skip_refresh_token, oauth_client, status, - expected, + unchanged, + caplog, ): cache = {"tracks/abc": web_response_mock_etag} responses.add( responses.GET, "https://api.spotify.com/v1/tracks/abc", - json={"uri": "spotify:track:xyz"}, status=status, ) - mock_time.return_value = 1001 + mock_time.return_value = web_response_mock_etag._expires + 1 assert not cache["tracks/abc"].still_valid() result = oauth_client.get("tracks/abc", cache) assert len(responses.calls) == 1 assert responses.calls[0].request.headers["If-None-Match"] == '"1234"' - assert result["uri"] == expected assert cache["tracks/abc"] == result + assert result.status_unchanged is unchanged + assert (result.items() == web_response_mock_etag.items()) == unchanged @responses.activate diff --git a/tox.ini b/tox.ini index 209e6446..a5ad7c1a 100644 --- a/tox.ini +++ b/tox.ini @@ -5,7 +5,7 @@ envlist = py39, py310, py311, check-manifest, flake8 sitepackages = true deps = .[test] commands = - python -m pytest \ + python -m pytest -vv \ --basetemp={envtmpdir} \ --cov=mopidy_spotify --cov-report=term-missing \ {posargs}