From 410e457e5a9d0cb6f639eb18989fc0bd3249b5a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 11 Jun 2025 12:02:31 -0400 Subject: [PATCH] feat(server): add update and clear play queue endpoints to native API (#4215) * Refactor queue payload handling * Refine queue update validation * refactor(queue): avoid loading tracks for validation * refactor/rename repository methods Signed-off-by: Deluan * more tests Signed-off-by: Deluan * refactor Signed-off-by: Deluan --------- Signed-off-by: Deluan --- model/playqueue.go | 7 +- persistence/playqueue_repository.go | 41 +++- persistence/playqueue_repository_test.go | 281 ++++++++++++++++++++++- server/nativeapi/native_api.go | 2 + server/nativeapi/queue.go | 172 ++++++++++++-- server/nativeapi/queue_test.go | 128 ++++++++++- server/subsonic/bookmarks.go | 2 +- tests/mock_playqueue_repo.go | 34 ++- 8 files changed, 616 insertions(+), 51 deletions(-) diff --git a/model/playqueue.go b/model/playqueue.go index 6b666b188..03b562253 100644 --- a/model/playqueue.go +++ b/model/playqueue.go @@ -18,6 +18,11 @@ type PlayQueue struct { type PlayQueues []PlayQueue type PlayQueueRepository interface { - Store(queue *PlayQueue) error + Store(queue *PlayQueue, colNames ...string) error + // Retrieve returns the playqueue without loading the full MediaFiles + // (Items only contain IDs) Retrieve(userId string) (*PlayQueue, error) + // RetrieveWithMediaFiles returns the playqueue with full MediaFiles loaded + RetrieveWithMediaFiles(userId string) (*PlayQueue, error) + Clear(userId string) error } diff --git a/persistence/playqueue_repository.go b/persistence/playqueue_repository.go index a74c31e38..9948253b0 100644 --- a/persistence/playqueue_repository.go +++ b/persistence/playqueue_repository.go @@ -35,22 +35,27 @@ type playQueue struct { UpdatedAt time.Time `structs:"updated_at"` } -func (r *playQueueRepository) Store(q *model.PlayQueue) error { +func (r *playQueueRepository) Store(q *model.PlayQueue, colNames ...string) error { u := loggedUser(r.ctx) - err := r.clearPlayQueue(q.UserID) - if err != nil { - log.Error(r.ctx, "Error deleting previous playqueue", "user", u.UserName, err) - return err - } - if len(q.Items) == 0 { - return nil + + // When no specific columns are provided, we replace the whole queue + if len(colNames) == 0 { + err := r.clearPlayQueue(q.UserID) + if err != nil { + log.Error(r.ctx, "Error deleting previous playqueue", "user", u.UserName, err) + return err + } + if len(q.Items) == 0 { + return nil + } } + pq := r.fromModel(q) if pq.ID == "" { pq.CreatedAt = time.Now() } pq.UpdatedAt = time.Now() - _, err = r.put(pq.ID, pq) + _, err := r.put(pq.ID, pq, colNames...) if err != nil { log.Error(r.ctx, "Error saving playqueue", "user", u.UserName, err) return err @@ -58,12 +63,21 @@ func (r *playQueueRepository) Store(q *model.PlayQueue) error { return nil } +func (r *playQueueRepository) RetrieveWithMediaFiles(userId string) (*model.PlayQueue, error) { + sel := r.newSelect().Columns("*").Where(Eq{"user_id": userId}) + var res playQueue + err := r.queryOne(sel, &res) + q := r.toModel(&res) + q.Items = r.loadTracks(q.Items) + return &q, err +} + func (r *playQueueRepository) Retrieve(userId string) (*model.PlayQueue, error) { sel := r.newSelect().Columns("*").Where(Eq{"user_id": userId}) var res playQueue err := r.queryOne(sel, &res) - pls := r.toModel(&res) - return &pls, err + q := r.toModel(&res) + return &q, err } func (r *playQueueRepository) fromModel(q *model.PlayQueue) playQueue { @@ -100,7 +114,6 @@ func (r *playQueueRepository) toModel(pq *playQueue) model.PlayQueue { q.Items = append(q.Items, model.MediaFile{ID: t}) } } - q.Items = r.loadTracks(q.Items) return q } @@ -145,4 +158,8 @@ func (r *playQueueRepository) clearPlayQueue(userId string) error { return r.delete(Eq{"user_id": userId}) } +func (r *playQueueRepository) Clear(userId string) error { + return r.clearPlayQueue(userId) +} + var _ model.PlayQueueRepository = (*playQueueRepository)(nil) diff --git a/persistence/playqueue_repository_test.go b/persistence/playqueue_repository_test.go index dcf2c99ca..f0422450e 100644 --- a/persistence/playqueue_repository_test.go +++ b/persistence/playqueue_repository_test.go @@ -5,6 +5,7 @@ import ( "time" "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" @@ -18,18 +19,165 @@ var _ = Describe("PlayQueueRepository", func() { var ctx context.Context BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) ctx = log.NewContext(context.TODO()) ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "userid", IsAdmin: true}) repo = NewPlayQueueRepository(ctx, GetDBXBuilder()) }) - Describe("PlayQueues", func() { + Describe("Store", func() { + It("stores a complete playqueue", func() { + expected := aPlayQueue("userid", 1, 123, songComeTogether, songDayInALife) + Expect(repo.Store(expected)).To(Succeed()) + + actual, err := repo.RetrieveWithMediaFiles("userid") + Expect(err).ToNot(HaveOccurred()) + AssertPlayQueue(expected, actual) + Expect(countPlayQueues(repo, "userid")).To(Equal(1)) + }) + + It("replaces existing playqueue when storing without column names", func() { + By("Storing initial playqueue") + initial := aPlayQueue("userid", 0, 100, songComeTogether) + Expect(repo.Store(initial)).To(Succeed()) + + By("Storing replacement playqueue") + replacement := aPlayQueue("userid", 1, 200, songDayInALife, songAntenna) + Expect(repo.Store(replacement)).To(Succeed()) + + actual, err := repo.RetrieveWithMediaFiles("userid") + Expect(err).ToNot(HaveOccurred()) + AssertPlayQueue(replacement, actual) + Expect(countPlayQueues(repo, "userid")).To(Equal(1)) + }) + + It("clears playqueue when storing empty items", func() { + By("Storing initial playqueue") + initial := aPlayQueue("userid", 0, 100, songComeTogether) + Expect(repo.Store(initial)).To(Succeed()) + + By("Storing empty playqueue") + empty := aPlayQueue("userid", 0, 0) + Expect(repo.Store(empty)).To(Succeed()) + + By("Verifying playqueue is cleared") + _, err := repo.Retrieve("userid") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("updates only current field when specified", func() { + By("Storing initial playqueue") + initial := aPlayQueue("userid", 0, 100, songComeTogether, songDayInALife) + Expect(repo.Store(initial)).To(Succeed()) + + By("Getting the existing playqueue to obtain its ID") + existing, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + + By("Updating only current field") + update := &model.PlayQueue{ + ID: existing.ID, // Use existing ID for partial update + UserID: "userid", + Current: 1, + ChangedBy: "test-update", + } + Expect(repo.Store(update, "current")).To(Succeed()) + + By("Verifying only current was updated") + actual, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Current).To(Equal(1)) + Expect(actual.Position).To(Equal(int64(100))) // Should remain unchanged + Expect(actual.Items).To(HaveLen(2)) // Should remain unchanged + }) + + It("updates only position field when specified", func() { + By("Storing initial playqueue") + initial := aPlayQueue("userid", 1, 100, songComeTogether, songDayInALife) + Expect(repo.Store(initial)).To(Succeed()) + + By("Getting the existing playqueue to obtain its ID") + existing, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + + By("Updating only position field") + update := &model.PlayQueue{ + ID: existing.ID, // Use existing ID for partial update + UserID: "userid", + Position: 500, + ChangedBy: "test-update", + } + Expect(repo.Store(update, "position")).To(Succeed()) + + By("Verifying only position was updated") + actual, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Position).To(Equal(int64(500))) + Expect(actual.Current).To(Equal(1)) // Should remain unchanged + Expect(actual.Items).To(HaveLen(2)) // Should remain unchanged + }) + + It("updates multiple specified fields", func() { + By("Storing initial playqueue") + initial := aPlayQueue("userid", 0, 100, songComeTogether) + Expect(repo.Store(initial)).To(Succeed()) + + By("Getting the existing playqueue to obtain its ID") + existing, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + + By("Updating current and position fields") + update := &model.PlayQueue{ + ID: existing.ID, // Use existing ID for partial update + UserID: "userid", + Current: 1, + Position: 300, + ChangedBy: "test-update", + } + Expect(repo.Store(update, "current", "position")).To(Succeed()) + + By("Verifying both fields were updated") + actual, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Current).To(Equal(1)) + Expect(actual.Position).To(Equal(int64(300))) + Expect(actual.Items).To(HaveLen(1)) // Should remain unchanged + }) + + It("preserves existing data when updating with empty items list and column names", func() { + By("Storing initial playqueue") + initial := aPlayQueue("userid", 0, 100, songComeTogether, songDayInALife) + Expect(repo.Store(initial)).To(Succeed()) + + By("Getting the existing playqueue to obtain its ID") + existing, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + + By("Updating only position with empty items") + update := &model.PlayQueue{ + ID: existing.ID, // Use existing ID for partial update + UserID: "userid", + Position: 200, + ChangedBy: "test-update", + Items: []model.MediaFile{}, // Empty items + } + Expect(repo.Store(update, "position")).To(Succeed()) + + By("Verifying items are preserved") + actual, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + Expect(actual.Position).To(Equal(int64(200))) + Expect(actual.Items).To(HaveLen(2)) // Should remain unchanged + }) + }) + + Describe("Retrieve", func() { It("returns notfound error if there's no playqueue for the user", func() { _, err := repo.Retrieve("user999") Expect(err).To(MatchError(model.ErrNotFound)) }) - It("stores and retrieves the playqueue for the user", func() { + It("retrieves the playqueue with only track IDs (no full MediaFile data)", func() { By("Storing a playqueue for the user") expected := aPlayQueue("userid", 1, 123, songComeTogether, songDayInALife) @@ -38,18 +186,76 @@ var _ = Describe("PlayQueueRepository", func() { actual, err := repo.Retrieve("userid") Expect(err).ToNot(HaveOccurred()) - AssertPlayQueue(expected, actual) + // Basic playqueue properties should match + Expect(actual.ID).To(Equal(expected.ID)) + Expect(actual.UserID).To(Equal(expected.UserID)) + Expect(actual.Current).To(Equal(expected.Current)) + Expect(actual.Position).To(Equal(expected.Position)) + Expect(actual.ChangedBy).To(Equal(expected.ChangedBy)) + Expect(actual.Items).To(HaveLen(len(expected.Items))) - By("Storing a new playqueue for the same user") + // Items should only contain IDs, not full MediaFile data + for i, item := range actual.Items { + Expect(item.ID).To(Equal(expected.Items[i].ID)) + // These fields should be empty since we're not loading full MediaFiles + Expect(item.Title).To(BeEmpty()) + Expect(item.Path).To(BeEmpty()) + Expect(item.Album).To(BeEmpty()) + Expect(item.Artist).To(BeEmpty()) + } + }) - another := aPlayQueue("userid", 1, 321, songAntenna, songRadioactivity) - Expect(repo.Store(another)).To(Succeed()) + It("returns items with IDs even when some tracks 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(ctx, GetDBXBuilder()) - actual, err = repo.Retrieve("userid") + Expect(mfRepo.Put(&newSong)).To(Succeed()) + + // Create a playqueue with the new song + pq := aPlayQueue("userid", 0, 0, newSong, songAntenna) + Expect(repo.Store(pq)).To(Succeed()) + + // Delete the new song from the database + Expect(mfRepo.Delete("temp-track")).To(Succeed()) + + // Retrieve the playqueue with Retrieve method + actual, err := repo.Retrieve("userid") Expect(err).ToNot(HaveOccurred()) - AssertPlayQueue(another, actual) - Expect(countPlayQueues(repo, "userid")).To(Equal(1)) + // The playqueue should still contain both track IDs (including the deleted one) + Expect(actual.Items).To(HaveLen(2)) + Expect(actual.Items[0].ID).To(Equal("temp-track")) + Expect(actual.Items[1].ID).To(Equal(songAntenna.ID)) + + // Items should only contain IDs, no other data + for _, item := range actual.Items { + Expect(item.Title).To(BeEmpty()) + Expect(item.Path).To(BeEmpty()) + Expect(item.Album).To(BeEmpty()) + Expect(item.Artist).To(BeEmpty()) + } + }) + }) + + Describe("RetrieveWithMediaFiles", func() { + It("returns notfound error if there's no playqueue for the user", func() { + _, err := repo.RetrieveWithMediaFiles("user999") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("retrieves the playqueue with full MediaFile data", func() { + By("Storing a playqueue for the user") + + expected := aPlayQueue("userid", 1, 123, songComeTogether, songDayInALife) + Expect(repo.Store(expected)).To(Succeed()) + + actual, err := repo.RetrieveWithMediaFiles("userid") + Expect(err).ToNot(HaveOccurred()) + + AssertPlayQueue(expected, actual) }) It("does not return tracks if they don't exist in the DB", func() { @@ -66,7 +272,7 @@ var _ = Describe("PlayQueueRepository", func() { Expect(repo.Store(pq)).To(Succeed()) // Retrieve the playqueue - actual, err := repo.Retrieve("userid") + actual, err := repo.RetrieveWithMediaFiles("userid") Expect(err).ToNot(HaveOccurred()) // The playqueue should contain both tracks @@ -76,7 +282,7 @@ var _ = Describe("PlayQueueRepository", func() { Expect(mfRepo.Delete("temp-track")).To(Succeed()) // Retrieve the playqueue - actual, err = repo.Retrieve("userid") + actual, err = repo.RetrieveWithMediaFiles("userid") Expect(err).ToNot(HaveOccurred()) // The playqueue should not contain the deleted track @@ -84,6 +290,59 @@ var _ = Describe("PlayQueueRepository", func() { Expect(actual.Items[0].ID).To(Equal(songAntenna.ID)) }) }) + + Describe("Clear", func() { + It("clears an existing playqueue", func() { + By("Storing a playqueue") + expected := aPlayQueue("userid", 1, 123, songComeTogether, songDayInALife) + Expect(repo.Store(expected)).To(Succeed()) + + By("Verifying playqueue exists") + _, err := repo.Retrieve("userid") + Expect(err).ToNot(HaveOccurred()) + + By("Clearing the playqueue") + Expect(repo.Clear("userid")).To(Succeed()) + + By("Verifying playqueue is cleared") + _, err = repo.Retrieve("userid") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("does not error when clearing non-existent playqueue", func() { + // Clear should not error even if no playqueue exists + Expect(repo.Clear("nonexistent-user")).To(Succeed()) + }) + + It("only clears the specified user's playqueue", func() { + By("Creating users in the database to avoid foreign key constraints") + userRepo := NewUserRepository(ctx, GetDBXBuilder()) + user1 := &model.User{ID: "user1", UserName: "user1", Name: "User 1", Email: "user1@test.com"} + user2 := &model.User{ID: "user2", UserName: "user2", Name: "User 2", Email: "user2@test.com"} + Expect(userRepo.Put(user1)).To(Succeed()) + Expect(userRepo.Put(user2)).To(Succeed()) + + By("Storing playqueues for two users") + user1Queue := aPlayQueue("user1", 0, 100, songComeTogether) + user2Queue := aPlayQueue("user2", 1, 200, songDayInALife) + Expect(repo.Store(user1Queue)).To(Succeed()) + Expect(repo.Store(user2Queue)).To(Succeed()) + + By("Clearing only user1's playqueue") + Expect(repo.Clear("user1")).To(Succeed()) + + By("Verifying user1's playqueue is cleared") + _, err := repo.Retrieve("user1") + Expect(err).To(MatchError(model.ErrNotFound)) + + By("Verifying user2's playqueue still exists") + actual, err := repo.Retrieve("user2") + Expect(err).ToNot(HaveOccurred()) + Expect(actual.UserID).To(Equal("user2")) + Expect(actual.Current).To(Equal(1)) + Expect(actual.Position).To(Equal(int64(200))) + }) + }) }) func countPlayQueues(repo model.PlayQueueRepository, userId string) int { diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 5f9013d6d..aed24e963 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -157,6 +157,8 @@ func (n *Router) addQueueRoute(r chi.Router) { r.Route("/queue", func(r chi.Router) { r.Get("/", getQueue(n.ds)) r.Post("/", saveQueue(n.ds)) + r.Put("/", updateQueue(n.ds)) + r.Delete("/", clearQueue(n.ds)) }) } diff --git a/server/nativeapi/queue.go b/server/nativeapi/queue.go index e9a5c6e51..0a3136660 100644 --- a/server/nativeapi/queue.go +++ b/server/nativeapi/queue.go @@ -1,6 +1,7 @@ package nativeapi import ( + "context" "encoding/json" "errors" "net/http" @@ -8,13 +9,61 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + . "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/slice" ) -type queuePayload struct { - Ids []string `json:"ids"` - Current int `json:"current"` - Position int64 `json:"position"` +type updateQueuePayload struct { + Ids *[]string `json:"ids,omitempty"` + Current *int `json:"current,omitempty"` + Position *int64 `json:"position,omitempty"` +} + +// validateCurrentIndex validates that the current index is within bounds of the items array. +// Returns false if validation fails (and sends error response), true if validation passes. +func validateCurrentIndex(w http.ResponseWriter, current int, itemsLength int) bool { + if current < 0 || current >= itemsLength { + http.Error(w, "current index out of bounds", http.StatusBadRequest) + return false + } + return true +} + +// retrieveExistingQueue retrieves an existing play queue for a user with proper error handling. +// Returns the queue (nil if not found) and false if an error occurred and response was sent. +func retrieveExistingQueue(ctx context.Context, w http.ResponseWriter, ds model.DataStore, userID string) (*model.PlayQueue, bool) { + existing, err := ds.PlayQueue(ctx).Retrieve(userID) + if err != nil && !errors.Is(err, model.ErrNotFound) { + log.Error(ctx, "Error retrieving queue", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return nil, false + } + return existing, true +} + +// decodeUpdatePayload decodes the JSON payload from the request body. +// Returns false if decoding fails (and sends error response), true if successful. +func decodeUpdatePayload(w http.ResponseWriter, r *http.Request) (*updateQueuePayload, bool) { + var payload updateQueuePayload + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return nil, false + } + return &payload, true +} + +// createMediaFileItems converts a slice of IDs to MediaFile items. +func createMediaFileItems(ids []string) []model.MediaFile { + return slice.Map(ids, func(id string) model.MediaFile { + return model.MediaFile{ID: id} + }) +} + +// extractUserAndClient extracts user and client from the request context. +func extractUserAndClient(ctx context.Context) (model.User, string) { + user, _ := request.UserFrom(ctx) + client, _ := request.ClientFrom(ctx) + return user, client } func getQueue(ds model.DataStore) http.HandlerFunc { @@ -22,7 +71,7 @@ func getQueue(ds model.DataStore) http.HandlerFunc { ctx := r.Context() user, _ := request.UserFrom(ctx) repo := ds.PlayQueue(ctx) - pq, err := repo.Retrieve(user.ID) + pq, err := repo.RetrieveWithMediaFiles(user.ID) if err != nil && !errors.Is(err, model.ErrNotFound) { log.Error(ctx, "Error retrieving queue", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -45,24 +94,21 @@ func getQueue(ds model.DataStore) http.HandlerFunc { func saveQueue(ds model.DataStore) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - var payload queuePayload - if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + payload, ok := decodeUpdatePayload(w, r) + if !ok { return } - user, _ := request.UserFrom(ctx) - client, _ := request.ClientFrom(ctx) - items := slice.Map(payload.Ids, func(id string) model.MediaFile { - return model.MediaFile{ID: id} - }) - if len(payload.Ids) > 0 && (payload.Current < 0 || payload.Current >= len(payload.Ids)) { - http.Error(w, "current index out of bounds", http.StatusBadRequest) + user, client := extractUserAndClient(ctx) + ids := V(payload.Ids) + items := createMediaFileItems(ids) + current := V(payload.Current) + if len(ids) > 0 && !validateCurrentIndex(w, current, len(ids)) { return } pq := &model.PlayQueue{ UserID: user.ID, - Current: payload.Current, - Position: max(payload.Position, 0), + Current: current, + Position: max(V(payload.Position), 0), ChangedBy: client, Items: items, } @@ -74,3 +120,95 @@ func saveQueue(ds model.DataStore) http.HandlerFunc { w.WriteHeader(http.StatusNoContent) } } + +func updateQueue(ds model.DataStore) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + + // Decode and validate the JSON payload + payload, ok := decodeUpdatePayload(w, r) + if !ok { + return + } + + // Extract user and client information from request context + user, client := extractUserAndClient(ctx) + + // Initialize play queue with user ID and client info + pq := &model.PlayQueue{UserID: user.ID, ChangedBy: client} + var cols []string // Track which columns to update in the database + + // Handle queue items update + if payload.Ids != nil { + pq.Items = createMediaFileItems(*payload.Ids) + cols = append(cols, "items") + + // If current index is not being updated, validate existing current index + // against the new items list to ensure it remains valid + if payload.Current == nil { + existing, ok := retrieveExistingQueue(ctx, w, ds, user.ID) + if !ok { + return + } + if existing != nil && !validateCurrentIndex(w, existing.Current, len(*payload.Ids)) { + return + } + } + } + + // Handle current track index update + if payload.Current != nil { + pq.Current = *payload.Current + cols = append(cols, "current") + + if payload.Ids != nil { + // If items are also being updated, validate current index against new items + if !validateCurrentIndex(w, *payload.Current, len(*payload.Ids)) { + return + } + } else { + // If only current index is being updated, validate against existing items + existing, ok := retrieveExistingQueue(ctx, w, ds, user.ID) + if !ok { + return + } + if existing != nil && !validateCurrentIndex(w, *payload.Current, len(existing.Items)) { + return + } + } + } + + // Handle playback position update + if payload.Position != nil { + pq.Position = max(*payload.Position, 0) // Ensure position is non-negative + cols = append(cols, "position") + } + + // If no fields were specified for update, return success without doing anything + if len(cols) == 0 { + w.WriteHeader(http.StatusNoContent) + return + } + + // Perform partial update of the specified columns only + if err := ds.PlayQueue(ctx).Store(pq, cols...); err != nil { + log.Error(ctx, "Error updating queue", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) + } +} + +func clearQueue(ds model.DataStore) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + user, _ := request.UserFrom(ctx) + if err := ds.PlayQueue(ctx).Clear(user.ID); err != nil { + log.Error(ctx, "Error clearing queue", err) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) + } +} diff --git a/server/nativeapi/queue_test.go b/server/nativeapi/queue_test.go index 64f2e066b..ef971ee68 100644 --- a/server/nativeapi/queue_test.go +++ b/server/nativeapi/queue_test.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" + "github.com/navidrome/navidrome/utils/gg" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -31,7 +32,7 @@ var _ = Describe("Queue Endpoints", func() { Describe("POST /queue", func() { It("saves the queue", func() { - payload := queuePayload{Ids: []string{"s1", "s2"}, Current: 1, Position: 10} + payload := updateQueuePayload{Ids: gg.P([]string{"s1", "s2"}), Current: gg.P(1), Position: gg.P(int64(10))} body, _ := json.Marshal(payload) req := httptest.NewRequest("POST", "/queue", bytes.NewReader(body)) ctx := request.WithUser(req.Context(), user) @@ -49,7 +50,7 @@ var _ = Describe("Queue Endpoints", func() { }) It("saves an empty queue", func() { - payload := queuePayload{Ids: []string{}, Current: 0, Position: 0} + payload := updateQueuePayload{Ids: gg.P([]string{}), Current: gg.P(0), Position: gg.P(int64(0))} body, _ := json.Marshal(payload) req := httptest.NewRequest("POST", "/queue", bytes.NewReader(body)) req = req.WithContext(request.WithUser(req.Context(), user)) @@ -62,7 +63,7 @@ var _ = Describe("Queue Endpoints", func() { }) It("returns bad request for invalid current index (negative)", func() { - payload := queuePayload{Ids: []string{"s1", "s2"}, Current: -1, Position: 10} + payload := updateQueuePayload{Ids: gg.P([]string{"s1", "s2"}), Current: gg.P(-1), Position: gg.P(int64(10))} body, _ := json.Marshal(payload) req := httptest.NewRequest("POST", "/queue", bytes.NewReader(body)) req = req.WithContext(request.WithUser(req.Context(), user)) @@ -74,7 +75,7 @@ var _ = Describe("Queue Endpoints", func() { }) It("returns bad request for invalid current index (too large)", func() { - payload := queuePayload{Ids: []string{"s1", "s2"}, Current: 2, Position: 10} + payload := updateQueuePayload{Ids: gg.P([]string{"s1", "s2"}), Current: gg.P(2), Position: gg.P(int64(10))} body, _ := json.Marshal(payload) req := httptest.NewRequest("POST", "/queue", bytes.NewReader(body)) req = req.WithContext(request.WithUser(req.Context(), user)) @@ -96,7 +97,7 @@ var _ = Describe("Queue Endpoints", func() { It("returns internal server error when store fails", func() { repo.Err = true - payload := queuePayload{Ids: []string{"s1"}, Current: 0, Position: 10} + payload := updateQueuePayload{Ids: gg.P([]string{"s1"}), Current: gg.P(0), Position: gg.P(int64(10))} body, _ := json.Marshal(payload) req := httptest.NewRequest("POST", "/queue", bytes.NewReader(body)) req = req.WithContext(request.WithUser(req.Context(), user)) @@ -161,4 +162,121 @@ var _ = Describe("Queue Endpoints", func() { Expect(w.Code).To(Equal(http.StatusInternalServerError)) }) }) + + Describe("PUT /queue", func() { + It("updates the queue fields", func() { + repo.Queue = &model.PlayQueue{UserID: user.ID, Items: model.MediaFiles{{ID: "s1"}, {ID: "s2"}, {ID: "s3"}}} + payload := updateQueuePayload{Current: gg.P(2), Position: gg.P(int64(20))} + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader(body)) + ctx := request.WithUser(req.Context(), user) + ctx = request.WithClient(ctx, "TestClient") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(repo.Queue).ToNot(BeNil()) + Expect(repo.Queue.Current).To(Equal(2)) + Expect(repo.Queue.Position).To(Equal(int64(20))) + Expect(repo.Queue.ChangedBy).To(Equal("TestClient")) + }) + + It("updates only ids", func() { + repo.Queue = &model.PlayQueue{UserID: user.ID, Current: 1} + payload := updateQueuePayload{Ids: gg.P([]string{"s1", "s2"})} + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader(body)) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(repo.Queue.Items).To(HaveLen(2)) + Expect(repo.LastCols).To(ConsistOf("items")) + }) + + It("updates ids and current", func() { + repo.Queue = &model.PlayQueue{UserID: user.ID} + payload := updateQueuePayload{Ids: gg.P([]string{"s1", "s2"}), Current: gg.P(1)} + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader(body)) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(repo.Queue.Items).To(HaveLen(2)) + Expect(repo.Queue.Current).To(Equal(1)) + Expect(repo.LastCols).To(ConsistOf("items", "current")) + }) + + It("returns bad request when new ids invalidate current", func() { + repo.Queue = &model.PlayQueue{UserID: user.ID, Current: 2} + payload := updateQueuePayload{Ids: gg.P([]string{"s1", "s2"})} + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader(body)) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("returns bad request when current out of bounds", func() { + repo.Queue = &model.PlayQueue{UserID: user.ID, Items: model.MediaFiles{{ID: "s1"}}} + payload := updateQueuePayload{Current: gg.P(3)} + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader(body)) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("returns bad request for malformed JSON", func() { + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader([]byte("{"))) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("returns internal server error when store fails", func() { + repo.Err = true + payload := updateQueuePayload{Position: gg.P(int64(10))} + body, _ := json.Marshal(payload) + req := httptest.NewRequest("PUT", "/queue", bytes.NewReader(body)) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + updateQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) + }) + + Describe("DELETE /queue", func() { + It("clears the queue", func() { + repo.Queue = &model.PlayQueue{UserID: user.ID, Items: model.MediaFiles{{ID: "s1"}}} + req := httptest.NewRequest("DELETE", "/queue", nil) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + clearQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(repo.Queue).To(BeNil()) + }) + + It("returns internal server error when clear fails", func() { + repo.Err = true + req := httptest.NewRequest("DELETE", "/queue", nil) + req = req.WithContext(request.WithUser(req.Context(), user)) + w := httptest.NewRecorder() + + clearQueue(ds)(w, req) + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) + }) }) diff --git a/server/subsonic/bookmarks.go b/server/subsonic/bookmarks.go index 2316e474a..d7286c20c 100644 --- a/server/subsonic/bookmarks.go +++ b/server/subsonic/bookmarks.go @@ -73,7 +73,7 @@ func (api *Router) GetPlayQueue(r *http.Request) (*responses.Subsonic, error) { user, _ := request.UserFrom(r.Context()) repo := api.ds.PlayQueue(r.Context()) - pq, err := repo.Retrieve(user.ID) + pq, err := repo.RetrieveWithMediaFiles(user.ID) if err != nil && !errors.Is(err, model.ErrNotFound) { return nil, err } diff --git a/tests/mock_playqueue_repo.go b/tests/mock_playqueue_repo.go index 4812e0667..19976db57 100644 --- a/tests/mock_playqueue_repo.go +++ b/tests/mock_playqueue_repo.go @@ -8,11 +8,12 @@ import ( type MockPlayQueueRepo struct { model.PlayQueueRepository - Queue *model.PlayQueue - Err bool + Queue *model.PlayQueue + Err bool + LastCols []string } -func (m *MockPlayQueueRepo) Store(q *model.PlayQueue) error { +func (m *MockPlayQueueRepo) Store(q *model.PlayQueue, cols ...string) error { if m.Err { return errors.New("error") } @@ -21,10 +22,11 @@ func (m *MockPlayQueueRepo) Store(q *model.PlayQueue) error { qCopy := *q qCopy.Items = copyItems m.Queue = &qCopy + m.LastCols = cols return nil } -func (m *MockPlayQueueRepo) Retrieve(userId string) (*model.PlayQueue, error) { +func (m *MockPlayQueueRepo) RetrieveWithMediaFiles(userId string) (*model.PlayQueue, error) { if m.Err { return nil, errors.New("error") } @@ -37,3 +39,27 @@ func (m *MockPlayQueueRepo) Retrieve(userId string) (*model.PlayQueue, error) { qCopy.Items = copyItems return &qCopy, nil } + +func (m *MockPlayQueueRepo) Retrieve(userId string) (*model.PlayQueue, error) { + if m.Err { + return nil, errors.New("error") + } + if m.Queue == nil || m.Queue.UserID != userId { + return nil, model.ErrNotFound + } + copyItems := make(model.MediaFiles, len(m.Queue.Items)) + for i, t := range m.Queue.Items { + copyItems[i] = model.MediaFile{ID: t.ID} + } + qCopy := *m.Queue + qCopy.Items = copyItems + return &qCopy, nil +} + +func (m *MockPlayQueueRepo) Clear(userId string) error { + if m.Err { + return errors.New("error") + } + m.Queue = nil + return nil +}