Merge pull request #6313 from navidrome/investigate-6310-20261010

fix(playlists): drop unknown track ids when adding to a playlist

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 {

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 {
@@ -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")

persistence/playlist_repository_test.go

@@ -469,6 +469,26 @@ 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"}
+			Expect(repo.Put(ctx, &pls)).To(Succeed())
+			DeferCleanup(func() { Expect(repo.Delete(ctx, pls.ID)).To(Succeed()) })
+			// 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}, '1001', 1), ({:id}, 'no-such-track', 2), ({:id}, '1002', 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() {

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)
 }

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))
@@ -123,6 +125,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(plsID)).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() {