diff --git a/persistence/playqueue_repository.go b/persistence/playqueue_repository.go index 9dc520c44..4373bb987 100644 --- a/persistence/playqueue_repository.go +++ b/persistence/playqueue_repository.go @@ -80,10 +80,47 @@ func (r *playQueueRepository) RetrieveWithMediaFiles(ctx context.Context, userId var res playQueue err := r.queryOne(ctx, sel, &res) q := r.toModel(&res) - q.Items = r.loadTracks(ctx, q.Items) + stored := q.Items + var loadErr error + q.Items, loadErr = r.loadTracks(ctx, stored) + if loadErr == nil && len(q.Items) < len(stored) { + q.Current, q.Position = remapCurrent(stored, q.Items, q.Current, q.Position) + go r.saveCleanedQueue(context.WithoutCancel(ctx), res, r.fromModel(&q)) + } return &q, err } +// remapCurrent relies on loaded being stored in the same order, minus the missing tracks. If the current track +// is gone, it moves to the next remaining one (or the last) and restarts it. +func remapCurrent(stored, loaded model.MediaFiles, current int, position int64) (int, int64) { + kept := 0 + for i, t := range stored { + if kept >= len(loaded) || loaded[kept].ID != t.ID { + continue + } + if i >= current { + if i > current { + position = 0 + } + return kept, position + } + kept++ + } + return max(len(loaded)-1, 0), 0 +} + +// saveCleanedQueue only writes if the queue was not changed since it was read, so it never undoes a newer save. +func (r *playQueueRepository) saveCleanedQueue(ctx context.Context, read, cleaned playQueue) { + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + upd := Update(r.tableName). + Set("items", cleaned.Items).Set("current", cleaned.Current).Set("position", cleaned.Position). + Where(Eq{"id": read.ID, "items": read.Items, "current": read.Current, "position": read.Position}) + if _, err := r.executeSQL(ctx, upd); err != nil { + log.Warn(ctx, "Could not remove missing tracks from playqueue", "user", read.UserID, err) + } +} + func (r *playQueueRepository) Retrieve(ctx context.Context, userId string) (*model.PlayQueue, error) { sel := r.newSelect(ctx).Columns("*").Where(Eq{"user_id": userId}) var res playQueue @@ -130,13 +167,14 @@ func (r *playQueueRepository) toModel(pq *playQueue) model.PlayQueue { // loadTracks loads the tracks from the database. It receives a list of track IDs and returns a list of MediaFiles // in the same order as the input list. -func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.MediaFiles) model.MediaFiles { +func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.MediaFiles) (model.MediaFiles, error) { if len(tracks) == 0 { - return nil + return nil, nil } mfRepo := NewMediaFileRepository(r.db) trackMap := map[string]model.MediaFile{} + var loadErr error // Create an iterator to collect all track IDs ids := slice.SeqFunc(tracks, func(t model.MediaFile) string { return t.ID }) @@ -148,6 +186,7 @@ func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.Media if err != nil { u := loggedUser(ctx) log.Error(ctx, "Could not load playqueue/bookmark's tracks", "user", u.UserName, err) + loadErr = err } for _, t := range tracks { trackMap[t.ID] = t @@ -162,7 +201,7 @@ func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.Media newTracks = append(newTracks, track) } } - return newTracks + return newTracks, loadErr } func (r *playQueueRepository) clearPlayQueue(ctx context.Context, userId string) error { diff --git a/persistence/playqueue_repository_test.go b/persistence/playqueue_repository_test.go index 877faddfc..5d18cdd86 100644 --- a/persistence/playqueue_repository_test.go +++ b/persistence/playqueue_repository_test.go @@ -20,7 +20,7 @@ var _ = Describe("PlayQueueRepository", func() { BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) - ctx = log.NewContext(context.TODO()) + ctx = log.NewContext(GinkgoT().Context()) ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "userid", IsAdmin: true}) repo = NewPlayQueueRepository(GetDBXBuilder()) }) @@ -311,36 +311,68 @@ var _ = Describe("PlayQueueRepository", func() { AssertPlayQueue(expected, actual) }) - It("does not return tracks if they don't exist in the DB", func() { - // Add a new song to the DB - newSong := songRadioactivity - newSong.ID = "temp-track" - newSong.Path = "/new-path" - mfRepo := NewMediaFileRepository(GetDBXBuilder()) + Context("when tracks are no longer in the library", func() { + var mfRepo model.MediaFileRepository + gone := songRadioactivity + gone.ID = "gone-track" + gone.Path = "/gone-track" - Expect(mfRepo.Put(ctx, &newSong)).To(Succeed()) + BeforeEach(func() { + mfRepo = NewMediaFileRepository(GetDBXBuilder()) + Expect(mfRepo.Put(ctx, &gone)).To(Succeed()) + DeferCleanup(func() { _ = mfRepo.Delete(ctx, gone.ID) }) + }) - // Create a playqueue with the new song - pq := aPlayQueue("userid", 0, 0, newSong, songAntenna) - Expect(repo.Store(ctx, pq)).To(Succeed()) + retrieveAfterRemoval := func(current int, position int64, items ...model.MediaFile) *model.PlayQueue { + Expect(repo.Store(ctx, aPlayQueue("userid", current, position, items...))).To(Succeed()) + Expect(mfRepo.Delete(ctx, gone.ID)).To(Succeed()) + actual, err := repo.RetrieveWithMediaFiles(ctx, "userid") + Expect(err).ToNot(HaveOccurred()) + return actual + } - // Retrieve the playqueue - actual, err := repo.RetrieveWithMediaFiles(ctx, "userid") - Expect(err).ToNot(HaveOccurred()) + DescribeTable("points to the right current track", + func(current int, items []model.MediaFile, expectedID string, expectedPosition int64) { + actual := retrieveAfterRemoval(current, 45, items...) - // The playqueue should contain both tracks - AssertPlayQueue(pq, actual) + Expect(actual.Items).To(HaveLen(len(items) - 1)) + Expect(actual.Items[actual.Current].ID).To(Equal(expectedID)) + Expect(actual.Position).To(Equal(expectedPosition)) + }, + Entry("when an earlier track is removed", 1, []model.MediaFile{gone, songComeTogether, songDayInALife}, songComeTogether.ID, int64(45)), + Entry("when a later track is removed", 0, []model.MediaFile{songComeTogether, gone}, songComeTogether.ID, int64(45)), + Entry("moving to the next track from the start when the current one is removed", 1, []model.MediaFile{songComeTogether, gone, songDayInALife}, songDayInALife.ID, int64(0)), + Entry("moving to the last track when the removed current one was the last", 1, []model.MediaFile{songComeTogether, gone}, songComeTogether.ID, int64(0)), + ) - // Delete the new song - Expect(mfRepo.Delete(ctx, "temp-track")).To(Succeed()) + It("saves the cleaned queue", func() { + retrieveAfterRemoval(1, 45, gone, songComeTogether, songDayInALife) - // Retrieve the playqueue - actual, err = repo.RetrieveWithMediaFiles(ctx, "userid") - Expect(err).ToNot(HaveOccurred()) + Eventually(func(g Gomega) { + stored, err := repo.Retrieve(ctx, "userid") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(stored.Items).To(HaveLen(2)) + g.Expect(stored.Items[0].ID).To(Equal(songComeTogether.ID)) + g.Expect(stored.Current).To(BeZero()) + g.Expect(stored.Position).To(Equal(int64(45))) + }).Should(Succeed()) + }) - // The playqueue should not contain the deleted track - Expect(actual.Items).To(HaveLen(1)) - Expect(actual.Items[0].ID).To(Equal(songAntenna.ID)) + It("does not overwrite a queue that changed after it was read", func() { + r := repo.(*playQueueRepository) + read := aPlayQueue("userid", 1, 45, gone, songComeTogether) + Expect(repo.Store(ctx, read)).To(Succeed()) + + By("Saving a new queue from a client before the cleanup runs") + Expect(repo.Store(ctx, aPlayQueue("userid", 0, 10, songAntenna))).To(Succeed()) + r.saveCleanedQueue(ctx, r.fromModel(read), playQueue{Items: songComeTogether.ID, Position: 45}) + + stored, err := repo.Retrieve(ctx, "userid") + Expect(err).ToNot(HaveOccurred()) + Expect(stored.Items).To(HaveLen(1)) + Expect(stored.Items[0].ID).To(Equal(songAntenna.ID)) + Expect(stored.Position).To(Equal(int64(10))) + }) }) })