From e1e5b151ca9ea5b8bd41d178e0b9f64fe4577ceb Mon Sep 17 00:00:00 2001 From: RXWatcher <14085001+RXWatcher@users.noreply.github.com> Date: Tue, 26 May 2026 18:47:22 +0200 Subject: [PATCH] feat(audiobooks): add/remove collection items POST /collections/{id}/book/{bookId} validates the item against MediaStore (404 on unknown) and is idempotent on the store side. The DELETE variant is unconditional idempotent (returns the current membership state regardless of whether the row existed). Both 404 when non-owner. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../audiobooks/abs/collections_handler.go | 97 +++++++++++++++++ .../abs/collections_handler_test.go | 101 ++++++++++++++++++ 2 files changed, 198 insertions(+) diff --git a/internal/audiobooks/abs/collections_handler.go b/internal/audiobooks/abs/collections_handler.go index 94242f2d..ac25397c 100644 --- a/internal/audiobooks/abs/collections_handler.go +++ b/internal/audiobooks/abs/collections_handler.go @@ -223,6 +223,103 @@ func (h *Handler) handleUpdateCollection(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, h.collectionFullShape(r, persisted)) } +// handleAddCollectionBook — POST /collections/{id}/book/{bookId}. +// Owner-gated. Validates the item exists via MediaStore (returns 404 +// for unknown items). Idempotent: re-adding is a silent no-op. +// Returns the parent collection's full-shape with updated books[]. +func (h *Handler) handleAddCollectionBook(w http.ResponseWriter, r *http.Request) { + a, ok := absAuthFrom(r) + if !ok || a.UserID == "" { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + if h.deps.CollectionStore == nil { + http.Error(w, "collection not found", http.StatusNotFound) + return + } + id := chiURLID(r) + bookID := chi.URLParam(r, "bookId") + if bookID == "" { + http.Error(w, "bookId required", http.StatusBadRequest) + return + } + + c, err := h.deps.CollectionStore.GetCollection(r.Context(), id) + if errors.Is(err, ErrNotFound) || (err == nil && c.UserID != a.UserID) { + http.Error(w, "collection not found", http.StatusNotFound) + return + } + if err != nil { + slog.Error("abs collection get-for-add failed", "err", err, "id", id) + http.Error(w, "collection get failed", http.StatusInternalServerError) + return + } + + // Item validation — avoid orphan refs. + item, err := h.deps.MediaStore.GetAudiobookByID(r.Context(), bookID) + if err != nil || item == nil { + http.Error(w, "item not found", http.StatusNotFound) + return + } + + if err := h.deps.CollectionStore.AddCollectionItem(r.Context(), id, bookID); err != nil { + slog.Error("abs collection add-item failed", "err", err, "id", id, "book", bookID) + http.Error(w, "collection persist failed", http.StatusInternalServerError) + return + } + + // Re-fetch to surface updated_at bump. + persisted, err := h.deps.CollectionStore.GetCollection(r.Context(), id) + if err != nil { + persisted = c + } + writeJSON(w, http.StatusOK, h.collectionFullShape(r, persisted)) +} + +// handleRemoveCollectionBook — DELETE /collections/{id}/book/{bookId}. +// Owner-gated. Idempotent: removing a non-member is a no-op. +// Returns the parent collection's full-shape with updated books[]. +func (h *Handler) handleRemoveCollectionBook(w http.ResponseWriter, r *http.Request) { + a, ok := absAuthFrom(r) + if !ok || a.UserID == "" { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + if h.deps.CollectionStore == nil { + http.Error(w, "collection not found", http.StatusNotFound) + return + } + id := chiURLID(r) + bookID := chi.URLParam(r, "bookId") + if bookID == "" { + http.Error(w, "bookId required", http.StatusBadRequest) + return + } + + c, err := h.deps.CollectionStore.GetCollection(r.Context(), id) + if errors.Is(err, ErrNotFound) || (err == nil && c.UserID != a.UserID) { + http.Error(w, "collection not found", http.StatusNotFound) + return + } + if err != nil { + slog.Error("abs collection get-for-remove failed", "err", err, "id", id) + http.Error(w, "collection get failed", http.StatusInternalServerError) + return + } + + if err := h.deps.CollectionStore.RemoveCollectionItem(r.Context(), id, bookID); err != nil { + slog.Error("abs collection remove-item failed", "err", err, "id", id, "book", bookID) + http.Error(w, "collection delete failed", http.StatusInternalServerError) + return + } + + persisted, err := h.deps.CollectionStore.GetCollection(r.Context(), id) + if err != nil { + persisted = c + } + writeJSON(w, http.StatusOK, h.collectionFullShape(r, persisted)) +} + // handleDeleteCollection — DELETE /collections/{id}. // Owner-only. Cascade drops abs_collection_items via FK CASCADE. // 204 on success; 404 for unknown or non-owned. diff --git a/internal/audiobooks/abs/collections_handler_test.go b/internal/audiobooks/abs/collections_handler_test.go index 88746470..09b4a2c6 100644 --- a/internal/audiobooks/abs/collections_handler_test.go +++ b/internal/audiobooks/abs/collections_handler_test.go @@ -460,3 +460,104 @@ func TestCollection_Delete_NonOwner_404(t *testing.T) { t.Errorf("collection wrongly deleted: %v", err) } } + +func TestCollection_AddBook_Owner_HydratesInResponse(t *testing.T) { + hb := newCollectionsHarness(t, "book-1") + id := createCollectionForUser(t, hb, "1", "", `{"name":"x"}`) + + rec := dispatchABSWithParams(http.MethodPost, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "1", "", hb.H.handleAddCollectionBook) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var got map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &got) + books, _ := got["books"].([]any) + if len(books) != 1 { + t.Fatalf("books len = %d, want 1", len(books)) + } + entry := books[0].(map[string]any) + if entry["id"] != "book-1" { + t.Errorf("book entry id = %v, want book-1", entry["id"]) + } + if _, has := entry["media"]; !has { + t.Errorf("book entry missing media hydration: %v", entry) + } +} + +func TestCollection_AddBook_Idempotent(t *testing.T) { + hb := newCollectionsHarness(t, "book-1") + id := createCollectionForUser(t, hb, "1", "", `{"name":"x"}`) + + _ = dispatchABSWithParams(http.MethodPost, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "1", "", hb.H.handleAddCollectionBook) + rec := dispatchABSWithParams(http.MethodPost, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "1", "", hb.H.handleAddCollectionBook) + if rec.Code != http.StatusOK { + t.Fatalf("second add status = %d, want 200 (idempotent); body=%s", rec.Code, rec.Body.String()) + } + var got map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &got) + books, _ := got["books"].([]any) + if len(books) != 1 { + t.Errorf("books len after double-add = %d, want 1", len(books)) + } +} + +func TestCollection_AddBook_UnknownItem_404(t *testing.T) { + hb := newCollectionsHarness(t /* no known items */) + id := createCollectionForUser(t, hb, "1", "", `{"name":"x"}`) + + rec := dispatchABSWithParams(http.MethodPost, "/api/collections/"+id+"/book/ghost", + map[string]string{"id": id, "bookId": "ghost"}, nil, "1", "", hb.H.handleAddCollectionBook) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404 (item not found); body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCollection_AddBook_NonOwner_404(t *testing.T) { + hb := newCollectionsHarness(t, "book-1") + id := createCollectionForUser(t, hb, "1", "", `{"name":"mine"}`) + + rec := dispatchABSWithParams(http.MethodPost, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "2", "", hb.H.handleAddCollectionBook) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404 (no leak); body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCollection_RemoveBook_Idempotent(t *testing.T) { + hb := newCollectionsHarness(t, "book-1") + id := createCollectionForUser(t, hb, "1", "", `{"name":"x"}`) + + // Remove book that was never added — should be 200 with empty books. + rec := dispatchABSWithParams(http.MethodDelete, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "1", "", hb.H.handleRemoveCollectionBook) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var got map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &got) + books, _ := got["books"].([]any) + if len(books) != 0 { + t.Errorf("books len = %d, want 0", len(books)) + } +} + +func TestCollection_RemoveBook_NonOwner_404(t *testing.T) { + hb := newCollectionsHarness(t, "book-1") + id := createCollectionForUser(t, hb, "1", "", `{"name":"mine"}`) + _ = dispatchABSWithParams(http.MethodPost, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "1", "", hb.H.handleAddCollectionBook) + + rec := dispatchABSWithParams(http.MethodDelete, "/api/collections/"+id+"/book/book-1", + map[string]string{"id": id, "bookId": "book-1"}, nil, "2", "", hb.H.handleRemoveCollectionBook) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404; body=%s", rec.Code, rec.Body.String()) + } + // User 1's items must be intact. + items, _ := hb.Coll.ListCollectionItems(context.Background(), id) + if len(items) != 1 { + t.Errorf("items len = %d, want 1 (non-owner remove leaked)", len(items)) + } +}