diff --git a/src/spotify_mcp/fastmcp_server.py b/src/spotify_mcp/fastmcp_server.py index 3a099e0..25ac9ba 100644 --- a/src/spotify_mcp/fastmcp_server.py +++ b/src/spotify_mcp/fastmcp_server.py @@ -64,7 +64,7 @@ class Track(BaseModel): """A Spotify track with metadata.""" name: str - id: str + id: str | None = None artist: str artists: list[str] | None = None album: str | None = None @@ -74,6 +74,7 @@ class Track(BaseModel): popularity: int | None = None external_urls: dict[str, str] | None = None added_at: str | None = None + is_local: bool = False class PlaybackState(BaseModel): @@ -213,12 +214,16 @@ class RemovalConfirmation(BaseModel): def parse_track(item: TrackObject) -> Track: - """Parse Spotify track data into Track model.""" + """Parse Spotify track data into Track model. + + Tolerates entries with no ID (local files, unavailable/removed tracks): + they are returned with `id=None` rather than dropped or raising. + """ album_data = item.get("album", {}) artists = item.get("artists", []) return Track( - name=item["name"], - id=item["id"], + name=item.get("name") or "Unknown", + id=item.get("id"), artist=artists[0]["name"] if artists else "Unknown", artists=[a["name"] for a in artists], album=album_data.get("name"), @@ -227,6 +232,37 @@ def parse_track(item: TrackObject) -> Track: duration_ms=item.get("duration_ms"), popularity=item.get("popularity"), external_urls=cast("dict[str, str]", item.get("external_urls")), + is_local=bool(item.get("is_local", False)), + ) + + +def extract_playlist_entry(row: object) -> TrackObject | None: + """Pull the track object out of one playlist-items row. + + The Web API returns the entry under "item"; historically it was "track", + which spotipy fixtures and older responses still use. Accept either, and + ignore anything that is not an object (the nested "track" flag inside an + entry is a bool, not a track). + """ + if not isinstance(row, dict): + return None + for key in ("track", "item"): + entry = row.get(key) + if isinstance(entry, dict): + return cast("TrackObject", entry) + return None + + +def placeholder_track(row: dict[str, object]) -> Track: + """Stand-in for a row whose entry is null (removed/region-locked track). + + Kept in the list so positions stay aligned with the playlist itself. + """ + return Track( + name="Unavailable", + id=None, + artist="Unknown", + is_local=bool(row.get("is_local", False)), ) @@ -248,55 +284,60 @@ async def get_playlist_tracks_paginated( Returns: List of Track objects """ - tracks = [] + tracks: list[Track] = [] current_offset = offset - batch_size = min(limit, 100) if limit else 100 # Spotify API max is 100 per request - remaining = limit + remaining = limit # None means "every track" + unavailable = 0 logger.info( f"📄 Starting paginated fetch for playlist {playlist_id} (limit={limit}, offset={offset})" ) while True: - # Determine how many to fetch in this batch - batch_limit = min(batch_size, remaining) if remaining else batch_size + # Spotify API max is 100 per request + batch_limit = 100 if remaining is None else min(100, remaining) + if batch_limit <= 0: + break logger.info(f"📄 Fetching batch: offset={current_offset}, limit={batch_limit}") - # Get playlist tracks with pagination tracks_result = spotify_client.playlist_tracks( playlist_id, limit=batch_limit, offset=current_offset ) - if not tracks_result or not tracks_result.get("items"): + rows = (tracks_result or {}).get("items") or [] + if not rows: break - # Parse and add tracks - batch_tracks = [] - for item in tracks_result["items"]: - if item and item.get("track"): - batch_tracks.append(parse_track(item["track"])) + for row in rows: + entry = extract_playlist_entry(row) + if entry is None: + unavailable += 1 + tracks.append(placeholder_track(row if isinstance(row, dict) else {})) + continue + tracks.append(parse_track(entry)) - tracks.extend(batch_tracks) logger.info( - f"📄 Batch complete: retrieved {len(batch_tracks)} tracks (total so far: {len(tracks)})" + f"📄 Batch complete: read {len(rows)} rows (total so far: {len(tracks)})" ) if ctx is not None: await ctx.report_progress(progress=len(tracks), total=total) await ctx.info(f"Fetched {len(tracks)} tracks so far") - # Update remaining count if we have a limit - if remaining: - remaining -= len(batch_tracks) + # `limit`/`offset` count playlist positions, so consume the rows the API + # returned - never the subset we managed to parse. Counting parsed + # tracks lets an unparseable page hold `remaining` at its starting value + # and page through the entire playlist regardless of `limit`. + current_offset += len(rows) + if remaining is not None: + remaining -= len(rows) if remaining <= 0: break # Check if we've reached the end - if len(tracks_result["items"]) < batch_limit or not tracks_result.get("next"): + if len(rows) < batch_limit or not (tracks_result or {}).get("next"): break - current_offset += len(tracks_result["items"]) - # Safety check to prevent infinite loops if current_offset > 10000: logger.warning( @@ -304,6 +345,11 @@ async def get_playlist_tracks_paginated( ) break + if unavailable: + logger.warning( + f"⚠️ {unavailable} playlist entries had no track object and were " + f"returned as placeholders" + ) logger.info(f"📄 Pagination complete: total {len(tracks)} tracks retrieved") return tracks @@ -837,8 +883,10 @@ async def get_playlist_tracks( ) # Fetch total up front so progress notifications have a denominator - playlist_info = spotify_client.playlist(playlist_id, fields="tracks.total") - total_tracks = (playlist_info.get("tracks") or {}).get("total") + head = spotify_client.playlist_items( + playlist_id, limit=1, offset=0, fields="total" + ) + total_tracks = (head or {}).get("total") tracks = await get_playlist_tracks_paginated( playlist_id, limit, offset, ctx=ctx, total=total_tracks diff --git a/tests/test_fastmcp_tools.py b/tests/test_fastmcp_tools.py index 7258041..38ad54b 100644 --- a/tests/test_fastmcp_tools.py +++ b/tests/test_fastmcp_tools.py @@ -590,7 +590,7 @@ async def test_basic(self, mock_spotify_api, sample_track_data): "total": 2, "next": None, } - mock_spotify_api.playlist.return_value = {"tracks": {"total": 2}} + mock_spotify_api.playlist_items.return_value = {"total": 2} result = await get_playlist_tracks("pl1", limit=50) @@ -599,17 +599,94 @@ async def test_basic(self, mock_spotify_api, sample_track_data): assert result.returned == 2 mock_spotify_api.playlist_tracks.assert_called_with("pl1", limit=50, offset=0) - async def test_skips_null_track_items(self, mock_spotify_api, sample_track_data): + async def test_reads_entries_under_item_key( + self, mock_spotify_api, sample_track_data + ): + # The Web API returns playlist entries under "item", not "track" mock_spotify_api.playlist_tracks.return_value = { - "items": [{"track": sample_track_data}, {"track": None}], + "items": [{"item": sample_track_data}, {"item": sample_track_data}], "total": 2, "next": None, } - mock_spotify_api.playlist.return_value = {"tracks": {"total": 2}} + mock_spotify_api.playlist_items.return_value = {"total": 2} result = await get_playlist_tracks("pl1", limit=50) - assert result.returned == 1 + assert result.returned == 2 + assert result.items[0].id == sample_track_data["id"] + + async def test_null_entries_returned_as_placeholders( + self, mock_spotify_api, sample_track_data + ): + # A null entry keeps its slot so positions stay aligned with the playlist + mock_spotify_api.playlist_tracks.return_value = { + "items": [{"item": sample_track_data}, {"item": None, "is_local": True}], + "total": 2, + "next": None, + } + mock_spotify_api.playlist_items.return_value = {"total": 2} + + result = await get_playlist_tracks("pl1", limit=50) + + assert result.returned == 2 + assert result.items[1].id is None + assert result.items[1].name == "Unavailable" + assert result.items[1].is_local is True + + async def test_tracks_without_id_are_marked_not_dropped( + self, mock_spotify_api, sample_track_data + ): + # Local files and unavailable/removed tracks come back with "id": None + local_file = {**sample_track_data, "id": None, "is_local": True} + mock_spotify_api.playlist_tracks.return_value = { + "items": [{"item": sample_track_data}, {"item": local_file}], + "total": 2, + "next": None, + } + mock_spotify_api.playlist_items.return_value = {"total": 2} + + result = await get_playlist_tracks("pl1", limit=50) + + assert result.returned == 2 + assert result.items[0].id == sample_track_data["id"] + assert result.items[1].id is None + assert result.items[1].is_local is True + assert result.items[1].name == sample_track_data["name"] + + async def test_limit_respected_when_no_entry_is_parseable( + self, mock_spotify_api, sample_track_data + ): + # Regression: unparseable rows used to leave `remaining` untouched, so a + # small limit paged through the whole playlist until the client timed out + mock_spotify_api.playlist_tracks.return_value = { + "items": [{"unexpected_key": sample_track_data}] * 5, + "total": 1263, + "next": "https://api.spotify.com/next", + } + mock_spotify_api.playlist_items.return_value = {"total": 1263} + + result = await get_playlist_tracks("pl1", limit=5) + + assert mock_spotify_api.playlist_tracks.call_count == 1 + assert result.returned == 5 + assert all(t.id is None for t in result.items) + + async def test_total_uses_playlist_items_head_request( + self, mock_spotify_api, sample_track_data + ): + mock_spotify_api.playlist_tracks.return_value = { + "items": [{"track": sample_track_data}], + "next": None, + } + mock_spotify_api.playlist_items.return_value = {"total": 42} + + result = await get_playlist_tracks("pl1", limit=50) + + assert result.total == 42 + mock_spotify_api.playlist_items.assert_called_once_with( + "pl1", limit=1, offset=0, fields="total" + ) + mock_spotify_api.playlist.assert_not_called() async def test_spotify_error(self, mock_spotify_api): mock_spotify_api.playlist_tracks.side_effect = SPOTIFY_ERROR @@ -625,7 +702,7 @@ async def test_reports_progress_with_context( "total": 1, "next": None, } - mock_spotify_api.playlist.return_value = {"tracks": {"total": 1}} + mock_spotify_api.playlist_items.return_value = {"total": 1} await get_playlist_tracks("pl1", limit=50, ctx=mock_context) @@ -646,7 +723,7 @@ async def test_paginates_across_multiple_batches( "next": None, } mock_spotify_api.playlist_tracks.side_effect = [batch1, batch2] - mock_spotify_api.playlist.return_value = {"tracks": {"total": 150}} + mock_spotify_api.playlist_items.return_value = {"total": 150} result = await get_playlist_tracks("pl1", limit=150) @@ -666,7 +743,7 @@ async def test_stops_when_batch_shorter_than_requested( "total": 500, "next": "https://api.spotify.com/next", } - mock_spotify_api.playlist.return_value = {"tracks": {"total": 500}} + mock_spotify_api.playlist_items.return_value = {"total": 500} result = await get_playlist_tracks("pl1", limit=100) @@ -675,7 +752,7 @@ async def test_stops_when_batch_shorter_than_requested( async def test_empty_playlist_returns_no_tracks(self, mock_spotify_api): mock_spotify_api.playlist_tracks.return_value = {"items": []} - mock_spotify_api.playlist.return_value = {"tracks": {"total": 0}} + mock_spotify_api.playlist_items.return_value = {"total": 0} result = await get_playlist_tracks("pl1") @@ -685,12 +762,12 @@ async def test_empty_playlist_returns_no_tracks(self, mock_spotify_api): async def test_total_falls_back_to_returned_count( self, mock_spotify_api, sample_track_data ): - # playlist() omits tracks.total -> total should fall back to len(tracks) + # playlist_items() omits total -> total should fall back to len(tracks) mock_spotify_api.playlist_tracks.return_value = { "items": [{"track": sample_track_data}], "next": None, } - mock_spotify_api.playlist.return_value = {} + mock_spotify_api.playlist_items.return_value = {} result = await get_playlist_tracks("pl1", limit=50)