diff --git a/homeassistant/components/spotify/coordinator.py b/homeassistant/components/spotify/coordinator.py index d983ef909fbb..94a716656384 100644 --- a/homeassistant/components/spotify/coordinator.py +++ b/homeassistant/components/spotify/coordinator.py @@ -5,6 +5,7 @@ from datetime import datetime, timedelta import logging from typing import override +from mashumaro.exceptions import InvalidFieldValue, MissingField from spotifyaio import ( ContextType, Device, @@ -143,8 +144,8 @@ class SpotifyCoordinator(DataUpdateCoordinator[SpotifyCoordinatorData]): self._checked_playlist_id = context.uri self._playlist = None if context.context_type == ContextType.PLAYLIST: - # Make sure any playlist lookups don't break the current - # playback state update + # Optional playlist metadata must not block setup or playback + # updates when fetching or deserialization fails. try: self._playlist = await self.client.get_playlist(context.uri) except SpotifyNotFoundError: @@ -154,7 +155,7 @@ class SpotifyCoordinator(DataUpdateCoordinator[SpotifyCoordinatorData]): context.uri, ) self._playlist = None - except SpotifyConnectionError: + except SpotifyConnectionError, InvalidFieldValue, MissingField: _LOGGER.debug( "Unable to load spotify playlist '%s'. " "Continuing without playlist data", diff --git a/tests/components/spotify/test_media_browser.py b/tests/components/spotify/test_media_browser.py index 93feb9ff7539..60a3c3ed6163 100644 --- a/tests/components/spotify/test_media_browser.py +++ b/tests/components/spotify/test_media_browser.py @@ -1,8 +1,10 @@ """Test the media browser interface.""" +import json from unittest.mock import MagicMock import pytest +from spotifyaio import Playlist, Track from syrupy.assertion import SnapshotAssertion from homeassistant.components.media_player import BrowseError @@ -14,7 +16,7 @@ from homeassistant.core import HomeAssistant from . import setup_integration from .conftest import SCOPES -from tests.common import MockConfigEntry +from tests.common import MockConfigEntry, async_load_json_object_fixture @pytest.mark.usefixtures("setup_credentials") @@ -137,6 +139,49 @@ async def test_browsing( assert response.as_dict() == snapshot +@pytest.mark.usefixtures("setup_credentials") +@pytest.mark.parametrize( + "name", + [ + pytest.param("Don't Stop", id="apostrophe"), + pytest.param('The "Blue" Album', id="double-quotes"), + pytest.param('Don\'t Stop "Now"', id="mixed-quotes"), + pytest.param('Don\'t Stop \\ "Now"', id="backslash"), + pytest.param("L\u2019été 音楽", id="unicode"), + ], +) +async def test_playlist_names_preserve_special_characters( + hass: HomeAssistant, + mock_spotify: MagicMock, + mock_config_entry: MockConfigEntry, + name: str, +) -> None: + """Test JSON parsing, setup and browsing preserve playlist, track and album names.""" + playlist_data = await async_load_json_object_fixture(hass, "playlist.json", DOMAIN) + playlist_data["name"] = name + track_data = playlist_data["tracks"]["items"][0]["track"] + track_data["name"] = name + track_data["album"]["name"] = name + playlist = Playlist.from_json(json.dumps(playlist_data)) + mock_spotify.return_value.get_playlist.return_value = playlist + + await setup_integration(hass, mock_config_entry) + assert (state := hass.states.get("media_player.spotify_spotify_1")) + assert state.attributes["media_playlist"] == name + + response = await async_browse_media( + hass, + "spotify://playlist", + f"spotify://{mock_config_entry.entry_id}/{playlist.uri}", + ) + assert response.title == name + assert response.children + assert response.children[0].title == name + track = playlist.items.items[0].track + assert isinstance(track, Track) + assert track.album.name == name + + @pytest.mark.parametrize("media_content_id", ["artist", None]) @pytest.mark.usefixtures("setup_credentials") async def test_invalid_spotify_url( diff --git a/tests/components/spotify/test_media_player.py b/tests/components/spotify/test_media_player.py index 8b328ff9fab8..e5c2ab4f9526 100644 --- a/tests/components/spotify/test_media_player.py +++ b/tests/components/spotify/test_media_player.py @@ -2,12 +2,15 @@ from dataclasses import replace from datetime import timedelta +import json from unittest.mock import MagicMock, patch from freezegun.api import FrozenDateTimeFactory +from mashumaro.exceptions import MissingField import pytest from spotifyaio import ( PlaybackState, + Playlist, RepeatMode as SpotifyRepeatMode, SpotifyConnectionError, SpotifyNotFoundError, @@ -35,6 +38,7 @@ from homeassistant.components.media_player import ( RepeatMode, ) from homeassistant.components.spotify import DOMAIN +from homeassistant.config_entries import ConfigEntryState from homeassistant.const import ( ATTR_ENTITY_ID, ATTR_ENTITY_PICTURE, @@ -58,6 +62,7 @@ from tests.common import ( MockConfigEntry, async_fire_time_changed, async_load_fixture, + async_load_json_object_fixture, snapshot_platform, ) @@ -197,14 +202,22 @@ async def test_normal_playlist( @pytest.mark.usefixtures("setup_credentials") +@pytest.mark.parametrize( + "error", + [ + pytest.param(SpotifyConnectionError(), id="connection"), + pytest.param(MissingField("name", str, Playlist), id="missing-playlist-name"), + ], +) async def test_fetching_playlist_does_not_fail( hass: HomeAssistant, mock_spotify: MagicMock, mock_config_entry: MockConfigEntry, freezer: FrozenDateTimeFactory, + error: SpotifyConnectionError | MissingField, ) -> None: """Test failing fetching playlist does not fail update.""" - mock_spotify.return_value.get_playlist.side_effect = SpotifyConnectionError + mock_spotify.return_value.get_playlist.side_effect = error await setup_integration(hass, mock_config_entry) state = hass.states.get("media_player.spotify_spotify_1") assert state @@ -219,6 +232,61 @@ async def test_fetching_playlist_does_not_fail( assert mock_spotify.return_value.get_playlist.call_count == 2 +@pytest.mark.usefixtures("setup_credentials") +@pytest.mark.parametrize("items_key", ["tracks", "items"]) +async def test_playlist_item_schema_error_does_not_block_setup( + hass: HomeAssistant, + mock_spotify: MagicMock, + mock_config_entry: MockConfigEntry, + freezer: FrozenDateTimeFactory, + caplog: pytest.LogCaptureFixture, + items_key: str, +) -> None: + """Test playlist deserialization failures do not block setup or playback.""" + playlist_data = await async_load_json_object_fixture(hass, "playlist.json", DOMAIN) + playlist_items = playlist_data.pop("tracks") + for playlist_item in playlist_items["items"]: + playlist_item["item"] = playlist_item.pop("track") + playlist_data[items_key] = playlist_items + playlist_json = json.dumps(playlist_data) + + def parse_playlist(_playlist_id: str) -> Playlist: + """Deserialize the response during the coordinator's playlist lookup.""" + return Playlist.from_json(playlist_json) + + client = mock_spotify.return_value + client.get_playlist.side_effect = parse_playlist + await setup_integration(hass, mock_config_entry) + + assert mock_config_entry.state is ConfigEntryState.LOADED + assert (state := hass.states.get("media_player.spotify_spotify_1")) + assert state.state == MediaPlayerState.PLAYING + assert state.attributes["media_title"] == client.get_playback.return_value.item.name + assert "media_playlist" not in state.attributes + client.get_playlist.assert_called_once() + + freezer.tick(timedelta(seconds=30)) + async_fire_time_changed(hass) + await hass.async_block_till_done() + + assert (state := hass.states.get("media_player.spotify_spotify_1")) + assert state.state == MediaPlayerState.PLAYING + assert "media_playlist" not in state.attributes + assert client.get_playlist.call_count == 2 + # Deserialization exceptions contain the entire playlist and must not be logged. + assert "has invalid value" not in caplog.text + + client.get_playlist.side_effect = None + freezer.tick(timedelta(seconds=30)) + async_fire_time_changed(hass) + await hass.async_block_till_done() + + assert (state := hass.states.get("media_player.spotify_spotify_1")) + assert state.state == MediaPlayerState.PLAYING + assert state.attributes["media_playlist"] == "Spotify Web API Testing playlist" + assert client.get_playlist.call_count == 3 + + @pytest.mark.usefixtures("setup_credentials") async def test_fetching_playlist_once( hass: HomeAssistant,