fix: read playlist entries from the "item" key, honor limit by rows

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,62 +284,72 @@ 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(
                 f"⚠️ Safety limit reached: stopping at offset {current_offset}"
             )
             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

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)