feat(api): DELETE /api/admin/tracks/{id} for M7 #372

Admin-only handler. Calls tracks.RemoveTrack with the unmonitor flag
parsed from the query string. ErrNotFound -> 404; everything else
unexpected -> 500. Lidarr-side failures during unmonitor flow as
lidarr_unmonitor_failed: true in the success envelope (non-fatal —
the file + DB delete already completed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-05-02 22:41:51 -04:00
parent f6975cfad3
commit bb931746e3
5 changed files with 440 additions and 3 deletions
+93
View File
@@ -0,0 +1,93 @@
package api
import (
"errors"
"net/http"
"strconv"
"github.com/go-chi/chi/v5"
"git.fabledsword.com/bvandeusen/minstrel/internal/auth"
"git.fabledsword.com/bvandeusen/minstrel/internal/tracks"
)
// removeTrackResponse is the wire shape from spec §5 (M7 #372). The three
// optional fields are omitted unless the corresponding cleanup actually
// happened, so the response stays minimal in the common case.
type removeTrackResponse struct {
DeletedTrackID string `json:"deleted_track_id"`
DeletedAlbumID *string `json:"deleted_album_id,omitempty"`
DeletedArtistID *string `json:"deleted_artist_id,omitempty"`
LidarrUnmonitorFailed *bool `json:"lidarr_unmonitor_failed,omitempty"`
}
// handleRemoveTrack implements DELETE /api/admin/tracks/{id}?unmonitor=true|false.
//
// Admin-only (gated by auth.RequireAdmin on the /admin route group). Always
// deletes the file + DB row and runs the album/artist cascade tidy-up. When
// unmonitor=true and the track has an mbid, also calls Lidarr.UnmonitorTrack
// — failure there is non-fatal (the destructive part already completed) and
// surfaces as `lidarr_unmonitor_failed: true` in the success envelope.
//
// Per spec §5, Lidarr-side errors during the unmonitor step do NOT map to
// wire error codes; the only error codes this handler emits are not_found,
// server_error, plus the auth codes the middleware emits upstream.
func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) {
idStr := chi.URLParam(r, "id")
trackID, ok := parseUUID(idStr)
if !ok {
// Malformed id is functionally equivalent to "no such track" for
// the spec's error surface — collapse both to 404 not_found so
// the client doesn't have to handle a separate bad_request branch
// for a code path that's only reachable via UI bugs.
writeErr(w, http.StatusNotFound, "not_found", "track not found")
return
}
unmonitor := false
if v := r.URL.Query().Get("unmonitor"); v != "" {
if parsed, err := strconv.ParseBool(v); err == nil {
unmonitor = parsed
}
}
admin, ok := auth.UserFromContext(r.Context())
if !ok {
// Defensive: RequireUser+RequireAdmin should have run upstream.
// If we got here without a user in context the routing is broken.
writeErr(w, http.StatusUnauthorized, "unauthenticated", "no session")
return
}
deletedAlbum, deletedArtist, lidarrUnmonitorFailed, err := h.tracks.RemoveTrack(
r.Context(), trackID, admin.ID, unmonitor,
)
if err != nil {
if errors.Is(err, tracks.ErrNotFound) {
writeErr(w, http.StatusNotFound, "not_found", "track not found")
return
}
h.logger.Error("api: remove track failed", "err", err, "track_id", idStr)
writeErr(w, http.StatusInternalServerError, "server_error", "remove failed")
return
}
resp := removeTrackResponse{DeletedTrackID: idStr}
if deletedAlbum != nil {
s := uuidToString(*deletedAlbum)
resp.DeletedAlbumID = &s
}
if deletedArtist != nil {
s := uuidToString(*deletedArtist)
resp.DeletedArtistID = &s
}
// Only emit the flag when unmonitor was requested AND it failed.
// Avoids "false" cluttering the wire when the operator didn't ask
// for an unmonitor in the first place.
if unmonitor && lidarrUnmonitorFailed {
t := true
resp.LidarrUnmonitorFailed = &t
}
writeJSON(w, http.StatusOK, resp)
}
+310
View File
@@ -0,0 +1,310 @@
package api
import (
"bytes"
"context"
"encoding/json"
"errors"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"testing"
"github.com/go-chi/chi/v5"
"git.fabledsword.com/bvandeusen/minstrel/internal/auth"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
"git.fabledsword.com/bvandeusen/minstrel/internal/tracks"
)
// fakeLidarrUnmonitorer captures UnmonitorTrack calls for assertion and lets
// tests dictate the return error. Used to drive both the Lidarr-success and
// Lidarr-failure branches without spinning up a stub HTTP server.
type fakeLidarrUnmonitorer struct {
calls []fakeUnmonitorCall
err error
}
type fakeUnmonitorCall struct {
trackMbid string
albumMbid string
}
func (f *fakeLidarrUnmonitorer) UnmonitorTrack(_ context.Context, trackMbid, albumMbid string) error {
f.calls = append(f.calls, fakeUnmonitorCall{trackMbid: trackMbid, albumMbid: albumMbid})
return f.err
}
// installTracksLidarrStub replaces h.tracks with a fresh tracks.Service whose
// LidarrUnmonitorer is the supplied fake. Returns the fake so tests can
// assert call counts and inject error returns.
func installTracksLidarrStub(t *testing.T, h *handlers) *fakeLidarrUnmonitorer {
t.Helper()
fake := &fakeLidarrUnmonitorer{}
h.tracks = tracks.NewService(h.pool, h.logger, fake)
return fake
}
// newAdminTracksRouter builds a test chi router with the admin-tracks
// endpoint. RequireAdmin middleware is applied; tests inject the user into
// context manually (RequireUser is bypassed). Mirrors the pattern from
// admin_lidarr_test.go and admin_quarantine_test.go.
func newAdminTracksRouter(h *handlers) chi.Router {
r := chi.NewRouter()
r.Route("/api/admin", func(admin chi.Router) {
admin.Use(auth.RequireAdmin())
admin.Delete("/tracks/{id}", h.handleRemoveTrack)
})
return r
}
// doAdminTracksReq fires an HTTP request against the admin-tracks router
// with the given user in context.
func doAdminTracksReq(t *testing.T, h *handlers, method, path string, user dbq.User) *httptest.ResponseRecorder {
t.Helper()
req := httptest.NewRequest(method, path, bytes.NewBuffer(nil))
req = req.WithContext(context.WithValue(req.Context(), userCtxKeyForTest(), user))
w := httptest.NewRecorder()
newAdminTracksRouter(h).ServeHTTP(w, req)
return w
}
// doAdminTracksReqNoUser fires a request directly at the handler with NO
// user injected into context. Used to exercise the defensive 401 branch
// inside the handler — real traffic hits RequireUser upstream, but the
// handler defends against routing misconfigurations and we test that path
// here. We bypass the admin router entirely since RequireAdmin would
// short-circuit with 500 internal_error on missing-user-in-context (which
// is its programmer-error contract); the handler's own 401 path is what
// we want to assert.
func doAdminTracksReqNoUser(t *testing.T, h *handlers, method, path, idParam string) *httptest.ResponseRecorder {
t.Helper()
req := httptest.NewRequest(method, path, bytes.NewBuffer(nil))
rctx := chi.NewRouteContext()
rctx.URLParams.Add("id", idParam)
req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
w := httptest.NewRecorder()
h.handleRemoveTrack(w, req)
return w
}
// seedTrackForRemoveTest creates a fresh artist+album+track on disk and
// returns the track row. The mbid string is set on both track and album
// when non-empty so the unmonitor branch has the data it needs.
func seedTrackForRemoveTest(t *testing.T, h *handlers, unique, trackMbid, albumMbid string) (dbq.Track, string) {
t.Helper()
artist := seedArtist(t, h.pool, "RemoveTest Artist "+unique)
dir := t.TempDir()
path := filepath.Join(dir, "track-"+unique+".mp3")
if err := os.WriteFile(path, []byte("audio"), 0o644); err != nil {
t.Fatalf("write file: %v", err)
}
q := dbq.New(h.pool)
var albumMbidPtr *string
if albumMbid != "" {
albumMbidPtr = &albumMbid
}
album, err := q.UpsertAlbum(context.Background(), dbq.UpsertAlbumParams{
Title: "RemoveTest Album " + unique,
SortTitle: "RemoveTest Album " + unique,
ArtistID: artist.ID,
Mbid: albumMbidPtr,
})
if err != nil {
t.Fatalf("UpsertAlbum: %v", err)
}
one := int32(1)
var trackMbidPtr *string
if trackMbid != "" {
trackMbidPtr = &trackMbid
}
tr, err := q.UpsertTrack(context.Background(), dbq.UpsertTrackParams{
Title: "RemoveTest Track " + unique,
AlbumID: album.ID,
ArtistID: artist.ID,
TrackNumber: &one,
DurationMs: 30000,
FilePath: path,
FileSize: 5,
FileFormat: "mp3",
Mbid: trackMbidPtr,
})
if err != nil {
t.Fatalf("UpsertTrack: %v", err)
}
return tr, path
}
// TestHandleRemoveTrack_NonAdmin403 verifies the RequireAdmin middleware
// rejects non-admin callers. Asserts the existing middleware shape
// ({"error":"not_authorized"}) — spec §5 says `unauthorized`, but the
// shared middleware emits not_authorized today, and this handler doesn't
// own that envelope.
func TestHandleRemoveTrack_NonAdmin403(t *testing.T) {
h, pool := testHandlers(t)
truncateLibrary(t, pool)
nonAdmin := seedUser(t, pool, "regular", "pw", false)
fakeID := "00000000-0000-0000-0000-000000000001"
w := doAdminTracksReq(t, h, http.MethodDelete, "/api/admin/tracks/"+fakeID, nonAdmin)
if w.Code != http.StatusForbidden {
t.Fatalf("status = %d, want 403; body = %s", w.Code, w.Body.String())
}
var resp map[string]string
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("decode: %v; body = %s", err, w.Body.String())
}
if resp["error"] != "not_authorized" {
t.Errorf("error = %q, want not_authorized", resp["error"])
}
}
// TestHandleRemoveTrack_NoSession401 exercises the defensive no-user-in-
// context branch in the handler itself. Real traffic hits RequireUser
// upstream (which emits 401 with text "unauthenticated"), but the handler
// also defends against routing misconfigurations and we test that
// directly.
func TestHandleRemoveTrack_NoSession401(t *testing.T) {
h, pool := testHandlers(t)
truncateLibrary(t, pool)
tr, _ := seedTrackForRemoveTest(t, h, "no-session", "", "")
w := doAdminTracksReqNoUser(t, h, http.MethodDelete,
"/api/admin/tracks/"+uuidToString(tr.ID), uuidToString(tr.ID))
if w.Code != http.StatusUnauthorized {
t.Fatalf("status = %d, want 401; body = %s", w.Code, w.Body.String())
}
var env errorBody
if err := json.Unmarshal(w.Body.Bytes(), &env); err != nil {
t.Fatalf("decode: %v; body = %s", err, w.Body.String())
}
if env.Error.Code != "unauthenticated" {
t.Errorf("error.code = %q, want unauthenticated", env.Error.Code)
}
}
// TestHandleRemoveTrack_UnknownID404 verifies ErrNotFound from the service
// maps to 404 not_found in the spec envelope.
func TestHandleRemoveTrack_UnknownID404(t *testing.T) {
h, pool := testHandlers(t)
truncateLibrary(t, pool)
admin := seedUser(t, pool, "admin", "pw", true)
// Well-formed UUID with no matching row in the DB.
missing := "00000000-0000-0000-0000-0000000000aa"
w := doAdminTracksReq(t, h, http.MethodDelete, "/api/admin/tracks/"+missing, admin)
if w.Code != http.StatusNotFound {
t.Fatalf("status = %d, want 404; body = %s", w.Code, w.Body.String())
}
var env errorBody
if err := json.Unmarshal(w.Body.Bytes(), &env); err != nil {
t.Fatalf("decode: %v; body = %s", err, w.Body.String())
}
if env.Error.Code != "not_found" {
t.Errorf("error.code = %q, want not_found", env.Error.Code)
}
}
// TestHandleRemoveTrack_HappyPath_NoUnmonitor verifies the success envelope
// when the operator does NOT request unmonitor: 200 with deleted_track_id
// (and the optional cascade fields when they fired) but NO
// lidarr_unmonitor_failed key in the body.
func TestHandleRemoveTrack_HappyPath_NoUnmonitor(t *testing.T) {
h, pool := testHandlers(t)
truncateLibrary(t, pool)
admin := seedUser(t, pool, "admin", "pw", true)
// Track with no mbid; sole track on its album/artist so the cascade
// fills both deleted_album_id and deleted_artist_id.
tr, path := seedTrackForRemoveTest(t, h, "noumon-happy", "", "")
w := doAdminTracksReq(t, h, http.MethodDelete,
"/api/admin/tracks/"+uuidToString(tr.ID), admin)
if w.Code != http.StatusOK {
t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String())
}
var got removeTrackResponse
if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil {
t.Fatalf("decode: %v; body = %s", err, w.Body.String())
}
if got.DeletedTrackID != uuidToString(tr.ID) {
t.Errorf("deleted_track_id = %q, want %q", got.DeletedTrackID, uuidToString(tr.ID))
}
if got.DeletedAlbumID == nil {
t.Error("deleted_album_id missing; expected sole-track cascade")
}
if got.DeletedArtistID == nil {
t.Error("deleted_artist_id missing; expected sole-track cascade")
}
if got.LidarrUnmonitorFailed != nil {
t.Errorf("lidarr_unmonitor_failed should be omitted when unmonitor not requested; got %v",
*got.LidarrUnmonitorFailed)
}
// Verify the raw JSON has no `lidarr_unmonitor_failed` key — omitempty
// on a *bool drops it, but we double-check the wire to catch
// regressions.
var raw map[string]any
if err := json.Unmarshal(w.Body.Bytes(), &raw); err != nil {
t.Fatalf("decode raw: %v", err)
}
if _, ok := raw["lidarr_unmonitor_failed"]; ok {
t.Error("wire body contained lidarr_unmonitor_failed when unmonitor was not requested")
}
// File is gone, DB row is gone.
if _, err := os.Stat(path); !os.IsNotExist(err) {
t.Errorf("file still exists: %v", err)
}
if _, err := dbq.New(pool).GetTrackByID(context.Background(), tr.ID); err == nil {
t.Error("track row still exists")
}
}
// TestHandleRemoveTrack_UnmonitorFailureFlag verifies that when the
// operator sends ?unmonitor=true AND the Lidarr call fails, the response
// is still 200 (file + DB delete completed) but carries
// lidarr_unmonitor_failed: true.
func TestHandleRemoveTrack_UnmonitorFailureFlag(t *testing.T) {
h, pool := testHandlers(t)
truncateLibrary(t, pool)
admin := seedUser(t, pool, "admin", "pw", true)
fake := installTracksLidarrStub(t, h)
fake.err = errors.New("lidarr exploded")
tr, path := seedTrackForRemoveTest(t, h, "umon-fail", "track-mbid-x", "album-mbid-x")
w := doAdminTracksReq(t, h, http.MethodDelete,
"/api/admin/tracks/"+uuidToString(tr.ID)+"?unmonitor=true", admin)
if w.Code != http.StatusOK {
t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String())
}
var got removeTrackResponse
if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil {
t.Fatalf("decode: %v; body = %s", err, w.Body.String())
}
if got.DeletedTrackID != uuidToString(tr.ID) {
t.Errorf("deleted_track_id = %q, want %q", got.DeletedTrackID, uuidToString(tr.ID))
}
if got.LidarrUnmonitorFailed == nil || !*got.LidarrUnmonitorFailed {
t.Errorf("lidarr_unmonitor_failed = %v, want true", got.LidarrUnmonitorFailed)
}
// Lidarr was actually called with the right mbids.
if len(fake.calls) != 1 {
t.Fatalf("UnmonitorTrack call count = %d, want 1", len(fake.calls))
}
if fake.calls[0].trackMbid != "track-mbid-x" || fake.calls[0].albumMbid != "album-mbid-x" {
t.Errorf("unmonitor call = %+v, want {track-mbid-x album-mbid-x}", fake.calls[0])
}
// Destructive part still happened.
if _, err := os.Stat(path); !os.IsNotExist(err) {
t.Errorf("file still exists: %v", err)
}
if _, err := dbq.New(pool).GetTrackByID(context.Background(), tr.ID); err == nil {
t.Error("track row still exists")
}
}
+6 -1
View File
@@ -17,12 +17,13 @@ import (
"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrquarantine"
"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests"
"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
"git.fabledsword.com/bvandeusen/minstrel/internal/tracks"
)
// Mount attaches /api/* handlers to r. Public endpoints (login) are outside
// RequireUser; everything else is gated by the middleware. The events writer
// is shared with the Subsonic mount so /rest/scrobble feeds the same store.
func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, lidarrCfg *lidarrconfig.Service, lidarrReqs *lidarrrequests.Service, lidarrQuar *lidarrquarantine.Service) {
func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, lidarrCfg *lidarrconfig.Service, lidarrReqs *lidarrrequests.Service, lidarrQuar *lidarrquarantine.Service, tracksSvc *tracks.Service) {
rng := rand.New(rand.NewSource(rand.Int63()))
h := &handlers{
pool: pool, logger: logger, events: events, recCfg: recCfg,
@@ -30,6 +31,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
lidarrCfg: lidarrCfg,
lidarrRequests: lidarrReqs,
lidarrQuarantine: lidarrQuar,
tracks: tracksSvc,
}
r.Route("/api", func(api chi.Router) {
@@ -93,6 +95,8 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
admin.Post("/quarantine/{track_id}/delete-file", h.handleDeleteQuarantineFile)
admin.Post("/quarantine/{track_id}/delete-via-lidarr", h.handleDeleteQuarantineViaLidarr)
admin.Get("/quarantine/actions", h.handleListQuarantineActions)
admin.Delete("/tracks/{id}", h.handleRemoveTrack)
})
})
})
@@ -107,4 +111,5 @@ type handlers struct {
lidarrCfg *lidarrconfig.Service
lidarrRequests *lidarrrequests.Service
lidarrQuarantine *lidarrquarantine.Service
tracks *tracks.Service
}
+6 -1
View File
@@ -26,6 +26,7 @@ import (
"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrquarantine"
"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests"
"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
"git.fabledsword.com/bvandeusen/minstrel/internal/tracks"
)
// testHandlers spins up a handlers instance against MINSTREL_TEST_DATABASE_URL.
@@ -60,7 +61,11 @@ func testHandlers(t *testing.T) (*handlers, *pgxpool.Pool) {
lidarrCfg := lidarrconfig.New(pool)
lidarrReqs := lidarrrequests.NewService(pool, lidarrCfg, nil, nil)
lidarrQuar := lidarrquarantine.NewService(pool, lidarrCfg, nil)
h := &handlers{pool: pool, logger: logger, events: w, recCfg: recCfg, rng: func() float64 { return 0.5 }, lidarrCfg: lidarrCfg, lidarrRequests: lidarrReqs, lidarrQuarantine: lidarrQuar}
// tracks.Service has no Lidarr unmonitorer in tests by default; the
// admin-tracks tests below override h.tracks via installTracksLidarrStub
// when they need a stubbed Lidarr.
tracksSvc := tracks.NewService(pool, logger, nil)
h := &handlers{pool: pool, logger: logger, events: w, recCfg: recCfg, rng: func() float64 { return 0.5 }, lidarrCfg: lidarrCfg, lidarrRequests: lidarrReqs, lidarrQuarantine: lidarrQuar, tracks: tracksSvc}
return h, pool
}
+25 -1
View File
@@ -3,6 +3,7 @@ package server
import (
"context"
"encoding/json"
"errors"
"log/slog"
"net/http"
"strings"
@@ -23,9 +24,31 @@ import (
"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests"
"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
"git.fabledsword.com/bvandeusen/minstrel/internal/subsonic"
"git.fabledsword.com/bvandeusen/minstrel/internal/tracks"
"git.fabledsword.com/bvandeusen/minstrel/web"
)
// lidarrUnmonitorAdapter wires the per-call lidarrClientFn factory pattern
// (used elsewhere in this package so admin Lidarr config edits take effect
// without restart) into the tracks.LidarrUnmonitorer interface that
// internal/tracks expects. When the factory returns nil (Lidarr disabled)
// we surface a sentinel error — tracks.Service treats UnmonitorTrack
// failures as non-fatal, so this collapses to a `lidarr_unmonitor_failed`
// flag in the response and the destructive part still runs.
type lidarrUnmonitorAdapter struct {
fn func() *lidarr.Client
}
var errLidarrDisabled = errors.New("lidarr disabled")
func (a lidarrUnmonitorAdapter) UnmonitorTrack(ctx context.Context, trackMbid, albumMbid string) error {
c := a.fn()
if c == nil {
return errLidarrDisabled
}
return c.UnmonitorTrack(ctx, trackMbid, albumMbid)
}
// ScanTrigger is the subset of the scanner the HTTP handler needs. Kept as an
// interface so tests can stub it without touching the DB.
type ScanTrigger interface {
@@ -74,7 +97,8 @@ func (s *Server) Router() http.Handler {
}
lidarrReqs := lidarrrequests.NewService(s.Pool, lidarrCfg, lidarrClientFn, nil)
lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn)
api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, lidarrCfg, lidarrReqs, lidarrQuar)
tracksSvc := tracks.NewService(s.Pool, s.Logger, lidarrUnmonitorAdapter{fn: lidarrClientFn})
api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, lidarrCfg, lidarrReqs, lidarrQuar, tracksSvc)
// /api/admin/scan is the only admin route owned by the server package
// (it needs the Scanner). Register it as a single inline-middleware
// route — using r.Route("/api/admin", ...) here would create a second