From a957d67efb3d0ad196cfdab86cb5f8e608579490 Mon Sep 17 00:00:00 2001 From: herrerad85 <35614502+herrerad85@users.noreply.github.com> Date: Fri, 19 Jun 2026 09:54:13 -0500 Subject: [PATCH] Fix deep-link playlist load: null id crash and songs never showing (#729) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a playlist by id — e.g. a tempo://asset/playlist/ deep link from Tasker — showed the playlist metadata but never its songs (perpetual spinner), then crashed after a minute or two. Root cause: PlaylistWithSongs, the type the Subsonic getPlaylist response deserializes into, annotated its id with @SerializedName("_id"), but the API sends "id". The "_id" was a workaround for Gson's "declares multiple JSON fields named 'id'" error (the property shadowed Playlist.id), but it left the id null. The page then re-fetched the songs with a null id; Retrofit dropped the null @Query("id"), the server replied "missing parameter: 'id'", and the song list spun forever behind a misleading "Playlist not found" dialog. The null id is also what produced the reporter's String.equals NPE. Fix: - PlaylistWithSongs no longer redeclares id; it inherits Playlist.id, which maps "id" correctly. The value still survives Parcelize (the base class is @Parcelize too) and a secondary constructor keeps the local "all songs" use. - Caching a deep-linked playlist's songs then hit a FOREIGN KEY constraint, because the single-playlist endpoint never inserts the playlist row (only the full-list path does, via cacheAllPlaylists). cachePlaylistSongs now inserts the parent first, through a new insertIfAbsent (IGNORE) so it can't delete-replace a row that already has child songs. Verified on the android-35 emulator against the Navidrome demo: the tempo://asset/playlist/ intent now loads metadata, cover art and the full song list with no crash, no FK violation and no false "not found" dialog; back-to-back deep links to two different playlists both succeed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tempo/database/dao/PlaylistDao.java | 7 ++++++ .../tempo/repository/PlaylistRepository.java | 12 ++++++++-- .../subsonic/models/PlaylistWithSongs.kt | 22 ++++++++++++++++--- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/cappielloantonio/tempo/database/dao/PlaylistDao.java b/app/src/main/java/com/cappielloantonio/tempo/database/dao/PlaylistDao.java index f6aea8d2a..b98892bf5 100644 --- a/app/src/main/java/com/cappielloantonio/tempo/database/dao/PlaylistDao.java +++ b/app/src/main/java/com/cappielloantonio/tempo/database/dao/PlaylistDao.java @@ -23,6 +23,13 @@ public interface PlaylistDao { @Insert(onConflict = OnConflictStrategy.REPLACE) void insert(Playlist playlist); + // Insert only if the row is absent. REPLACE would delete-then-reinsert an existing + // playlist, which fails the playlist_song foreign key when its songs already exist + // (and can race when several callers cache the same playlist at once). Used to make + // sure a deep-linked playlist's row exists before caching its songs. See issue #729. + @Insert(onConflict = OnConflictStrategy.IGNORE) + void insertIfAbsent(Playlist playlist); + @Insert(onConflict = OnConflictStrategy.REPLACE) void insertAll(List playlists); diff --git a/app/src/main/java/com/cappielloantonio/tempo/repository/PlaylistRepository.java b/app/src/main/java/com/cappielloantonio/tempo/repository/PlaylistRepository.java index 5d822dae9..4199a246e 100644 --- a/app/src/main/java/com/cappielloantonio/tempo/repository/PlaylistRepository.java +++ b/app/src/main/java/com/cappielloantonio/tempo/repository/PlaylistRepository.java @@ -177,7 +177,7 @@ public void onResponse(@NonNull Call call, @NonNull Response(); } listLivePlaylistSongs.setValue(songs); - cachePlaylistSongs(id, songs); + cachePlaylistSongs(sr.getPlaylist(), songs); } else if (sr.getError() != null && sr.getError().getCode() != null && sr.getError().getCode() == 70) { // Subsonic Standard Error Code 70: The requested data was not found. handleMissingPlaylist(id, null); @@ -199,8 +199,16 @@ public void onFailure(@NonNull Call call, @NonNull Throwable t) { return listLivePlaylistSongs; } - private void cachePlaylistSongs(String playlistId, List songs) { + private void cachePlaylistSongs(Playlist playlist, List songs) { new Thread(() -> { + String playlistId = playlist.getId(); + // playlist_song has a foreign key to playlist.id, so the playlist row must + // exist before its songs are inserted. The full-list path caches playlists via + // cacheAllPlaylists, but the single-playlist endpoint (used by deep links) does + // not — without this the songs insert crashes with a FOREIGN KEY constraint. + // Insert before the early-return dedup checks so the row is ensured even when + // the songs are already cached. See issue #729. + playlistDao.insertIfAbsent(playlist); List cached = playlistSongDao.getSongsForPlaylistSync(playlistId); if (songs == null || songs.isEmpty()) { if (cached != null && !cached.isEmpty()) { diff --git a/app/src/main/java/com/cappielloantonio/tempo/subsonic/models/PlaylistWithSongs.kt b/app/src/main/java/com/cappielloantonio/tempo/subsonic/models/PlaylistWithSongs.kt index 350dcbd4a..eae7c94b3 100644 --- a/app/src/main/java/com/cappielloantonio/tempo/subsonic/models/PlaylistWithSongs.kt +++ b/app/src/main/java/com/cappielloantonio/tempo/subsonic/models/PlaylistWithSongs.kt @@ -8,8 +8,24 @@ import kotlinx.parcelize.Parcelize @Keep @Parcelize class PlaylistWithSongs( - @SerializedName("_id") - override var id: String, @SerializedName("entry") var entries: List? = null, -) : Playlist(id), Parcelable \ No newline at end of file +) : Playlist(""), Parcelable { + + // Do NOT redeclare `id` here. Overriding the base property creates a second `id` + // backing field, and Gson then sees two JSON fields named "id" (this class's and + // Playlist's) and throws "declares multiple JSON fields named 'id'" at startup. + // The previous workaround mapped this field to "_id", which dodged the clash but + // left the id null for the Subsonic getPlaylist response (it sends "id"): opening a + // playlist by id — e.g. a tempo://asset/playlist/ deep link — then re-fetched + // its songs with a null id, the server replied "missing parameter: 'id'", the page + // showed a perpetual spinner plus a misleading "Playlist not found" dialog, and the + // null id later crashed with a String.equals NPE. Inheriting Playlist.id (which maps + // "id" correctly) fixes all of that; the value still survives Parcelize because the + // base class is @Parcelize too. See issue #729. + + // The synthetic "all songs" search result is built locally with a fixed id. + constructor(id: String, entries: List?) : this(entries) { + this.id = id + } +}