diff --git a/internal/api/admin_tracks.go b/internal/api/admin_tracks.go new file mode 100644 index 00000000..81f1b85f --- /dev/null +++ b/internal/api/admin_tracks.go @@ -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) +} diff --git a/internal/api/admin_tracks_test.go b/internal/api/admin_tracks_test.go new file mode 100644 index 00000000..dc93f7db --- /dev/null +++ b/internal/api/admin_tracks_test.go @@ -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") + } +} diff --git a/internal/api/api.go b/internal/api/api.go index bb45cceb..84887984 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -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 } diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index b38a8f72..921aa3cd 100644 --- a/internal/api/auth_test.go +++ b/internal/api/auth_test.go @@ -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 } diff --git a/internal/server/server.go b/internal/server/server.go index a54936b3..17132173 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -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