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() {