From da21cab37d51f372b7c263ac8d55dce71cb8ae8d Mon Sep 17 00:00:00 2001 From: RXWatcher <14085001+RXWatcher@users.noreply.github.com> Date: Tue, 26 May 2026 18:44:25 +0200 Subject: [PATCH] feat(audiobooks): PATCH + DELETE /collections/{id} Owner-gated mutation with partial-body PATCH semantics (only fields present in the body are updated). Non-owner attempts return 404 matching the bookmarks anti-enumeration pattern. DELETE cascades to abs_collection_items via FK. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../audiobooks/abs/collections_handler.go | 85 ++++++++++++++++++ .../abs/collections_handler_test.go | 87 +++++++++++++++++++ 2 files changed, 172 insertions(+) diff --git a/internal/audiobooks/abs/collections_handler.go b/internal/audiobooks/abs/collections_handler.go index 080c0d01..94242f2d 100644 --- a/internal/audiobooks/abs/collections_handler.go +++ b/internal/audiobooks/abs/collections_handler.go @@ -169,3 +169,88 @@ func (h *Handler) handleGetCollection(w http.ResponseWriter, r *http.Request) { } writeJSON(w, http.StatusOK, h.collectionFullShape(r, c)) } + +// handleUpdateCollection — PATCH /collections/{id}. +// Owner-only. Partial body: only fields explicitly present are +// modified. Non-owner gets 404 (no leak). +func (h *Handler) handleUpdateCollection(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) + 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-update failed", "err", err, "id", id) + http.Error(w, "collection get failed", http.StatusInternalServerError) + return + } + + var body collectionBody + if err := json.NewDecoder(io.LimitReader(r.Body, 1<<20)).Decode(&body); err != nil { + http.Error(w, "invalid body", http.StatusBadRequest) + return + } + if body.Name != nil { + c.Name = *body.Name + } + if body.Description != nil { + c.Description = *body.Description + } + if body.IsPublic != nil { + c.IsPublic = *body.IsPublic + } + if err := h.deps.CollectionStore.UpdateCollection(r.Context(), c); err != nil { + slog.Error("abs collection update failed", "err", err, "id", id) + http.Error(w, "collection persist failed", http.StatusInternalServerError) + return + } + + persisted, err := h.deps.CollectionStore.GetCollection(r.Context(), id) + if err != nil { + slog.Warn("abs collection get-after-update failed", "err", err, "id", id) + 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. +func (h *Handler) handleDeleteCollection(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) + 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-delete failed", "err", err, "id", id) + http.Error(w, "collection get failed", http.StatusInternalServerError) + return + } + if err := h.deps.CollectionStore.DeleteCollection(r.Context(), id); err != nil { + slog.Error("abs collection delete failed", "err", err, "id", id) + http.Error(w, "collection delete failed", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) +} diff --git a/internal/audiobooks/abs/collections_handler_test.go b/internal/audiobooks/abs/collections_handler_test.go index 5c115f14..88746470 100644 --- a/internal/audiobooks/abs/collections_handler_test.go +++ b/internal/audiobooks/abs/collections_handler_test.go @@ -373,3 +373,90 @@ func TestCollection_Get_Unknown_404(t *testing.T) { t.Errorf("status = %d, want 404; body=%s", rec.Code, rec.Body.String()) } } + +func TestCollection_Patch_OwnerUpdatesNameAndDescription(t *testing.T) { + hb := newCollectionsHarness(t) + id := createCollectionForUser(t, hb, "1", "", `{"name":"old","description":"d1"}`) + + body := []byte(`{"name":"new","description":"d2","isPublic":true}`) + rec := dispatchABSWithParams(http.MethodPatch, "/api/collections/"+id, map[string]string{"id": id}, body, "1", "", hb.H.handleUpdateCollection) + 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) + if got["name"] != "new" { + t.Errorf("name = %v, want 'new'", got["name"]) + } + if got["description"] != "d2" { + t.Errorf("description = %v, want 'd2'", got["description"]) + } + if got["isPublic"] != true { + t.Errorf("isPublic = %v, want true", got["isPublic"]) + } +} + +func TestCollection_Patch_PartialOnlyChangesPresentFields(t *testing.T) { + hb := newCollectionsHarness(t) + id := createCollectionForUser(t, hb, "1", "", `{"name":"keep","description":"d1"}`) + + // PATCH only name; description and isPublic must stay. + body := []byte(`{"name":"renamed"}`) + rec := dispatchABSWithParams(http.MethodPatch, "/api/collections/"+id, map[string]string{"id": id}, body, "1", "", hb.H.handleUpdateCollection) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d", rec.Code) + } + var got map[string]any + _ = json.Unmarshal(rec.Body.Bytes(), &got) + if got["name"] != "renamed" { + t.Errorf("name = %v, want 'renamed'", got["name"]) + } + if got["description"] != "d1" { + t.Errorf("description = %v, want 'd1' (unchanged)", got["description"]) + } +} + +func TestCollection_Patch_NonOwner_404(t *testing.T) { + hb := newCollectionsHarness(t) + id := createCollectionForUser(t, hb, "1", "", `{"name":"mine"}`) + + body := []byte(`{"name":"hijack"}`) + rec := dispatchABSWithParams(http.MethodPatch, "/api/collections/"+id, map[string]string{"id": id}, body, "2", "", hb.H.handleUpdateCollection) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404 (no leak); body=%s", rec.Code, rec.Body.String()) + } + // User 1's collection must be untouched. + c, _ := hb.Coll.GetCollection(context.Background(), id) + if c.Name != "mine" { + t.Errorf("collection name = %q, want 'mine'; non-owner mutation leaked", c.Name) + } +} + +func TestCollection_Delete_Owner_204(t *testing.T) { + hb := newCollectionsHarness(t) + id := createCollectionForUser(t, hb, "1", "", `{"name":"x"}`) + + rec := dispatchABSWithParams(http.MethodDelete, "/api/collections/"+id, map[string]string{"id": id}, nil, "1", "", hb.H.handleDeleteCollection) + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204; body=%s", rec.Code, rec.Body.String()) + } + // Subsequent GET must 404. + rec2 := dispatchABSWithParams(http.MethodGet, "/api/collections/"+id, map[string]string{"id": id}, nil, "1", "", hb.H.handleGetCollection) + if rec2.Code != http.StatusNotFound { + t.Errorf("post-delete GET status = %d, want 404", rec2.Code) + } +} + +func TestCollection_Delete_NonOwner_404(t *testing.T) { + hb := newCollectionsHarness(t) + id := createCollectionForUser(t, hb, "1", "", `{"name":"mine"}`) + + rec := dispatchABSWithParams(http.MethodDelete, "/api/collections/"+id, map[string]string{"id": id}, nil, "2", "", hb.H.handleDeleteCollection) + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404; body=%s", rec.Code, rec.Body.String()) + } + // User 1's collection still exists. + if _, err := hb.Coll.GetCollection(context.Background(), id); err != nil { + t.Errorf("collection wrongly deleted: %v", err) + } +}