images: ignore bad image uris (Fixes mopidy#364) (mopidy#365)

images: ignore bad image uris (Fixes mopidy#364)

304 response cannot set a body
Improve test_cache_expired_with_etag

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"]

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."
             )

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):

tests/test_web.py

@@ -620,33 +620,32 @@ 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,
     mock_time,
     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

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}