From 74e96a2bb75bad470f4a4f2d5573640e75f784fc Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 10 Oct 2026 11:28:58 -0400 Subject: [PATCH 1/2] fix(playlists): drop unknown track ids when adding to a playlist Adding tracks to a playlist skipped the media_file lookup for admins and for users who can see every library, so ids matching no track were stored as orphan playlist_tracks rows and reported as added. The native API returned an inflated "added" count, and the hidden rows left gaps in the positions, so Subsonic songIndexToRemove could delete a hidden row instead of the song at that index. keepAccessible now always checks the ids against media_file. applyLibraryFilter is already a no-op for those users, so the check only drops unknown ids. Rows stored before this fix are still removed by the existing GC removeOrphans step on the next full scan, or on any scan that finds changes. Fixes #6310 --- persistence/persistence_suite_test.go | 9 +++++ persistence/playlist_repository.go | 7 ++-- persistence/playlist_repository_test.go | 26 ++++++++++++++ persistence/playlist_track_repository_test.go | 34 +++++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index ee2794454..84fdffa4d 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -352,6 +352,15 @@ func GetDBXBuilder() *dbx.DB { return dbx.NewFromDB(db.Db(), db.Dialect) } +// storedRows reads playlist_tracks without joining media_file, so it also shows rows pointing to no track. +func storedRows(playlistID string) []string { + GinkgoHelper() + var rows []string + Expect(GetDBXBuilder().NewQuery("SELECT id || ':' || media_file_id FROM playlist_tracks WHERE playlist_id = {:id} ORDER BY id"). + Bind(dbx.Params{"id": playlistID}).Column(&rows)).To(Succeed()) + return rows +} + // collectCursor takes the cursor's underlying func type so the named cursor types // (model.AlbumCursor, ...) infer T. func collectCursor[T any](cursor func(func(T, error) bool), err error) []T { diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 41b75266c..c7559de18 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -305,12 +305,9 @@ func (r *playlistRepository) addTracks(ctx context.Context, playlistId string, s return len(mediaFileIds), r.refreshCounters(ctx, &model.Playlist{ID: playlistId}) } -// keepAccessible drops ids the caller cannot read, preserving order and duplicates. Chunked -// because callers pass unbounded id lists (M3U import), well past SQLITE_MAX_VARIABLE_NUMBER. +// keepAccessible drops ids the caller cannot read or that match no track, preserving order and duplicates. +// Chunked because callers pass unbounded id lists (M3U import), well past SQLITE_MAX_VARIABLE_NUMBER. func (r *playlistRepository) keepAccessible(ctx context.Context, mediaFileIds []string) ([]string, error) { - if visible, err := r.visibleLibraryIDs(ctx); err == nil && r.userSeesAllLibraries(ctx, visible) { - return mediaFileIds, nil - } accessible := make(map[string]struct{}, len(mediaFileIds)) for chunk := range slices.Chunk(slice.Unique(mediaFileIds), 200) { sq := r.applyLibraryFilter(ctx, Select("id").From("media_file").Where(Eq{"id": chunk}), "media_file") diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 93d37c928..d0f1f7f44 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -469,6 +469,32 @@ var _ = Describe("PlaylistRepository", func() { }) }) + Describe("removeOrphans", func() { + It("deletes rows pointing to no track and renumbers the rest", func() { + pls := model.Playlist{Name: "Orphans", OwnerID: "userid"} + pls.AddMediaFilesByID([]string{"1001", "1002"}) + Expect(repo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { Expect(repo.Delete(ctx, pls.ID)).To(Succeed()) }) + // Rows an older version stored for unknown ids, interleaved with real tracks + for _, q := range []string{"-(id * 2)", "-id"} { + _, err := GetDBXBuilder().NewQuery(`UPDATE playlist_tracks SET id = ` + q + ` WHERE playlist_id = {:id}`). + Bind(dbx.Params{"id": pls.ID}).Execute() + Expect(err).ToNot(HaveOccurred()) + } + _, err := GetDBXBuilder().NewQuery(`INSERT INTO playlist_tracks (playlist_id, media_file_id, id) + VALUES ({:id}, 'no-such-track', 1), ({:id}, '!!not-an-id!!', 3)`). + Bind(dbx.Params{"id": pls.ID}).Execute() + Expect(err).ToNot(HaveOccurred()) + + Expect(repo.(*playlistRepository).removeOrphans(ctx)).To(Succeed()) + + Expect(storedRows(pls.ID)).To(Equal([]string{"1:1001", "2:1002"})) + got, err := repo.Get(ctx, pls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(got.SongCount).To(Equal(2)) + }) + }) + // Exists is ctx-sensitive through userFilter, so callers that only want "does it still exist" // -- the public image route serving a share -- must elevate, or a private playlist looks gone. Describe("Exists visibility", func() { diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index 119c57116..aba4fdd95 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -123,6 +123,40 @@ var _ = Describe("PlaylistTrackRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect([]string{all[0].ID, all[1].ID, all[2].ID}).To(Equal([]string{"1", "2", "3"})) }) + + It("drops ids that match no track, keeping positions contiguous", func() { + Expect(tracks.Insert(ctx, []string{"no-such-track", songComeTogether.ID}, 1)).To(Equal(1)) + + Expect(storedRows(tracks.(*playlistTrackRepository).playlistId)).To(Equal([]string{ + "1:" + songComeTogether.ID, "2:" + songDayInALife.ID, "3:" + songRadioactivity.ID, + })) + }) + }) + + Describe("Add", func() { + var tracks model.PlaylistTrackRepository + var plsID string + + BeforeEach(func() { + plsRepo := NewPlaylistRepository(GetDBXBuilder()) + pls := model.Playlist{Name: "Add", OwnerID: "userid", OwnerName: "userid"} + Expect(plsRepo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { Expect(plsRepo.Delete(ctx, pls.ID)).To(Succeed()) }) + plsID = pls.ID + tracks = plsRepo.Tracks(ctx, pls.ID, false) + }) + + It("drops ids that match no track and counts only the tracks added", func() { + Expect(tracks.Add(ctx, []string{songDayInALife.ID, "no-such-track", songComeTogether.ID, "!!not-an-id!!"})).To(Equal(2)) + + Expect(storedRows(plsID)).To(Equal([]string{"1:" + songDayInALife.ID, "2:" + songComeTogether.ID})) + }) + + It("adds nothing when no id matches a track", func() { + Expect(tracks.Add(ctx, []string{"no-such-track", "!!not-an-id!!"})).To(BeZero()) + + Expect(storedRows(plsID)).To(BeEmpty()) + }) }) Describe("Reorder", func() { From 803b2da8ee37c80f36c2c93ba42ef0496ffaef08 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 10 Oct 2026 11:35:43 -0400 Subject: [PATCH 2/2] refactor(playlists): simplify #6310 tests and update check comments Builds the removeOrphans fixture with a single INSERT instead of shifting positions in two UPDATE passes, and has the Insert test keep the playlist id instead of reading a private repository field. Two comments now say that addTracks owns the existence check as well as the library check, and that Insert closes holes left by any id keepAccessible drops. No behavior change. --- persistence/playlist_repository.go | 2 +- persistence/playlist_repository_test.go | 10 ++-------- persistence/playlist_track_repository.go | 2 +- persistence/playlist_track_repository_test.go | 4 +++- 4 files changed, 7 insertions(+), 11 deletions(-) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index c7559de18..1a1b05a5c 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -281,7 +281,7 @@ func (r *playlistRepository) updatePlaylist(ctx context.Context, playlistId stri } // addTracks is the only path that writes playlist_tracks rows (smart playlists aside), so it owns -// the library check: every caller, including a full replace through Put, goes through it. +// the library and existence check: every caller, including a full replace through Put, goes through it. func (r *playlistRepository) addTracks(ctx context.Context, playlistId string, startingPos int, mediaFileIds []string) (int, error) { mediaFileIds, err := r.keepAccessible(ctx, mediaFileIds) if err != nil { diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index d0f1f7f44..b7be0588b 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -472,17 +472,11 @@ var _ = Describe("PlaylistRepository", func() { Describe("removeOrphans", func() { It("deletes rows pointing to no track and renumbers the rest", func() { pls := model.Playlist{Name: "Orphans", OwnerID: "userid"} - pls.AddMediaFilesByID([]string{"1001", "1002"}) Expect(repo.Put(ctx, &pls)).To(Succeed()) DeferCleanup(func() { Expect(repo.Delete(ctx, pls.ID)).To(Succeed()) }) - // Rows an older version stored for unknown ids, interleaved with real tracks - for _, q := range []string{"-(id * 2)", "-id"} { - _, err := GetDBXBuilder().NewQuery(`UPDATE playlist_tracks SET id = ` + q + ` WHERE playlist_id = {:id}`). - Bind(dbx.Params{"id": pls.ID}).Execute() - Expect(err).ToNot(HaveOccurred()) - } + // Rows as an older version stored them, with an unknown id between real tracks _, err := GetDBXBuilder().NewQuery(`INSERT INTO playlist_tracks (playlist_id, media_file_id, id) - VALUES ({:id}, 'no-such-track', 1), ({:id}, '!!not-an-id!!', 3)`). + VALUES ({:id}, '1001', 1), ({:id}, 'no-such-track', 2), ({:id}, '1002', 3)`). Bind(dbx.Params{"id": pls.ID}).Execute() Expect(err).ToNot(HaveOccurred()) diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 392446cef..587410305 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -213,7 +213,7 @@ func (r *playlistTrackRepository) Insert(ctx context.Context, mediaFileIds []str if err != nil || inserted == n { return inserted, err } - // The shift above reserved a slot per requested id, so ids dropped by the library filter + // The shift above reserved a slot per requested id, so ids dropped by keepAccessible // leave a hole. Close it. return inserted, r.playlistRepo.renumber(ctx, r.playlistId) } diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index aba4fdd95..29911ebd4 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -83,12 +83,14 @@ var _ = Describe("PlaylistTrackRepository", func() { Describe("Insert", func() { var tracks model.PlaylistTrackRepository + var plsID string BeforeEach(func() { plsRepo := NewPlaylistRepository(GetDBXBuilder()) pls := model.Playlist{Name: "Insert", OwnerID: "userid", OwnerName: "userid"} Expect(plsRepo.Put(ctx, &pls)).To(Succeed()) DeferCleanup(func() { Expect(plsRepo.Delete(ctx, pls.ID)).To(Succeed()) }) + plsID = pls.ID tracks = plsRepo.Tracks(ctx, pls.ID, false) Expect(tracks.Add(ctx, []string{songDayInALife.ID, songRadioactivity.ID})).To(Equal(2)) @@ -127,7 +129,7 @@ var _ = Describe("PlaylistTrackRepository", func() { It("drops ids that match no track, keeping positions contiguous", func() { Expect(tracks.Insert(ctx, []string{"no-such-track", songComeTogether.ID}, 1)).To(Equal(1)) - Expect(storedRows(tracks.(*playlistTrackRepository).playlistId)).To(Equal([]string{ + Expect(storedRows(plsID)).To(Equal([]string{ "1:" + songComeTogether.ID, "2:" + songDayInALife.ID, "3:" + songRadioactivity.ID, })) })