From 15a13a77c2b4ce356ae2267e07e7ae3fd88d0f28 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Fri, 20 Oct 2023 00:44:57 +0100 Subject: [PATCH 1/9] images: ignore bad image uris (Fixes #364) --- mopidy_spotify/images.py | 48 ++++++++++++++++++++++------------------ tests/test_images.py | 35 +++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 28 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/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): From 4f80d52cfea437823cbb55a0054180f2bdda25b4 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 00:30:41 +0000 Subject: [PATCH 2/9] verbosity --- mopidy_spotify/web.py | 2 +- tests/test_web.py | 1 + tox.ini | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) 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_web.py b/tests/test_web.py index 90358aa9..4b960d12 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -631,6 +631,7 @@ def test_cache_expired_with_etag( oauth_client, status, expected, + caplog, ): cache = {"tracks/abc": web_response_mock_etag} responses.add( 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} From 4e0f5eb89e6ea1861b4d01f820493377a53b3827 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 11:33:08 +0000 Subject: [PATCH 3/9] is it because my 304 response has a body make the body longer.... --- tests/test_web.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_web.py b/tests/test_web.py index 4b960d12..fd624902 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -637,7 +637,7 @@ def test_cache_expired_with_etag( responses.add( responses.GET, "https://api.spotify.com/v1/tracks/abc", - json={"uri": "spotify:track:xyz"}, + json={"uri": "spotify:track:xyz hello"}, status=status, ) mock_time.return_value = 1001 From 661b40c010a14f9c30558294fbf388087c2e4f64 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 11:47:44 +0000 Subject: [PATCH 4/9] Fixed 304 mock response 304 response cannot set a body --- tests/test_web.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_web.py b/tests/test_web.py index fd624902..9e5f6e23 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -621,7 +621,7 @@ def test_cache_normalised_query_string( @pytest.mark.parametrize( - "status,expected", [(304, "spotify:track:abc"), (200, "spotify:track:xyz")] + "status,json,expected", [(304, None, "spotify:track:abc"), (200, {"uri": "spotify:track:xyz"}, "spotify:track:xyz")] ) @responses.activate def test_cache_expired_with_etag( @@ -637,7 +637,7 @@ def test_cache_expired_with_etag( responses.add( responses.GET, "https://api.spotify.com/v1/tracks/abc", - json={"uri": "spotify:track:xyz hello"}, + json=json, status=status, ) mock_time.return_value = 1001 From 6e9c259039199958b1a92ab624520bce7f792f67 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 13:24:17 +0000 Subject: [PATCH 5/9] fix test_cache_expired_with_etag --- tests/test_web.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_web.py b/tests/test_web.py index 9e5f6e23..43692463 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -621,7 +621,7 @@ def test_cache_normalised_query_string( @pytest.mark.parametrize( - "status,json,expected", [(304, None, "spotify:track:abc"), (200, {"uri": "spotify:track:xyz"}, "spotify:track:xyz")] + "status,expected", [(304, "spotify:track:abc"), (200, None)] ) @responses.activate def test_cache_expired_with_etag( @@ -637,16 +637,15 @@ def test_cache_expired_with_etag( responses.add( responses.GET, "https://api.spotify.com/v1/tracks/abc", - json=json, 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 result.get("uri") == expected assert cache["tracks/abc"] == result From 723f3c372c44ab6fa08df9761bdaa250ea299241 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 13:37:09 +0000 Subject: [PATCH 6/9] Improve test_cache_expired_with_etag --- tests/test_web.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_web.py b/tests/test_web.py index 43692463..d15cc493 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -621,7 +621,7 @@ def test_cache_normalised_query_string( @pytest.mark.parametrize( - "status,expected", [(304, "spotify:track:abc"), (200, None)] + "status,expected_unchanged", [(304, True), (200, False)] ) @responses.activate def test_cache_expired_with_etag( @@ -645,8 +645,9 @@ def test_cache_expired_with_etag( result = oauth_client.get("tracks/abc", cache) assert len(responses.calls) == 1 assert responses.calls[0].request.headers["If-None-Match"] == '"1234"' - assert result.get("uri") == expected assert cache["tracks/abc"] == result + assert result.status_unchanged is expected_unchanged + assert (result.items() == web_response_mock_etag.items()) == expected_unchanged @responses.activate From 0b819bd9194336e030a8c18079c81f01c44a04f9 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 13:43:42 +0000 Subject: [PATCH 7/9] try again --- tests/test_web.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_web.py b/tests/test_web.py index d15cc493..8b85a012 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -630,7 +630,7 @@ def test_cache_expired_with_etag( skip_refresh_token, oauth_client, status, - expected, + expected_unchanged, caplog, ): cache = {"tracks/abc": web_response_mock_etag} From be8e50254e8a18bcd35de7eea32d1baad3c6aac1 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 13:50:06 +0000 Subject: [PATCH 8/9] Update test_web.py --- tests/test_web.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_web.py b/tests/test_web.py index 8b85a012..05f9160a 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -621,7 +621,7 @@ def test_cache_normalised_query_string( @pytest.mark.parametrize( - "status,expected_unchanged", [(304, True), (200, False)] + "status,unchanged", [(304, True), (200, False)] ) @responses.activate def test_cache_expired_with_etag( @@ -630,7 +630,7 @@ def test_cache_expired_with_etag( skip_refresh_token, oauth_client, status, - expected_unchanged, + unchanged, caplog, ): cache = {"tracks/abc": web_response_mock_etag} @@ -646,8 +646,8 @@ def test_cache_expired_with_etag( assert len(responses.calls) == 1 assert responses.calls[0].request.headers["If-None-Match"] == '"1234"' assert cache["tracks/abc"] == result - assert result.status_unchanged is expected_unchanged - assert (result.items() == web_response_mock_etag.items()) == expected_unchanged + assert result.status_unchanged is unchanged + assert (result.items() == web_response_mock_etag.items()) == unchanged @responses.activate From edbc053937d6a24d476fd6d78733e83b7379ed1d Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 1 Nov 2023 14:12:37 +0000 Subject: [PATCH 9/9] sigh --- tests/test_web.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_web.py b/tests/test_web.py index 05f9160a..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,unchanged", [(304, True), (200, False)] -) +@pytest.mark.parametrize("status,unchanged", [(304, True), (200, False)]) @responses.activate def test_cache_expired_with_etag( web_response_mock_etag,