From 74e96a2bb75bad470f4a4f2d5573640e75f784fc Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 10 Oct 2026 11:28:58 -0400 Subject: [PATCH] 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() {