diff --git a/internal/api/admin_users.go b/internal/api/admin_users.go index 032db765..e27f0811 100644 --- a/internal/api/admin_users.go +++ b/internal/api/admin_users.go @@ -212,20 +212,11 @@ func (h *handlers) handleAdminDeleteUser(w http.ResponseWriter, r *http.Request) writeErr(w, apierror.Unauthorized("auth_required", "")) return } - targetID, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("invalid_id", "")) - return - } q := dbq.New(h.pool) - target, err := q.GetUserByID(r.Context(), targetID) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: ""}) - return - } - writeErrWithLog(w, h.logger, "admin delete user: lookup failed", apierror.Internal(err)) + target, apiErr := resolveByID(r, "id", q.GetUserByID, "user") + if apiErr != nil { + writeErr(w, apiErr) return } @@ -243,12 +234,12 @@ func (h *handlers) handleAdminDeleteUser(w http.ResponseWriter, r *http.Request) } } - if err := q.DeleteUser(r.Context(), targetID); err != nil { + if err := q.DeleteUser(r.Context(), target.ID); err != nil { writeErrWithLog(w, h.logger, "admin delete user: delete failed", apierror.Internal(err)) return } - if err := audit.Write(r.Context(), h.pool, caller.ID, targetID, audit.ActionDeleteUser, + if err := audit.Write(r.Context(), h.pool, caller.ID, target.ID, audit.ActionDeleteUser, map[string]any{"username": target.Username, "was_admin": target.IsAdmin}); err != nil { h.logger.Warn("admin delete user: audit failed", "err", err) } diff --git a/internal/api/library.go b/internal/api/library.go index 9e0b66f0..a6e0f219 100644 --- a/internal/api/library.go +++ b/internal/api/library.go @@ -16,20 +16,10 @@ import ( // artist names so the response is self-contained — clients should not need // a follow-up request to render a track outside an album view. func (h *handlers) handleGetTrack(w http.ResponseWriter, r *http.Request) { - id, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("bad_request", "invalid track id")) - return - } q := dbq.New(h.pool) - track, err := q.GetTrackByID(r.Context(), id) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, apierror.NotFound("track")) - return - } - h.logger.Error("api: get track failed", "err", err) - writeErr(w, apierror.InternalMsg("lookup failed", err)) + track, apiErr := resolveByID(r, "id", q.GetTrackByID, "track") + if apiErr != nil { + writeErr(w, apiErr) return } album, err := q.GetAlbumByID(r.Context(), track.AlbumID) @@ -51,20 +41,10 @@ func (h *handlers) handleGetTrack(w http.ResponseWriter, r *http.Request) { // tracks (ordered by disc/track number via the underlying query) with // duration summed from the track list — keeps one source of truth. func (h *handlers) handleGetAlbum(w http.ResponseWriter, r *http.Request) { - id, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("bad_request", "invalid album id")) - return - } q := dbq.New(h.pool) - album, err := q.GetAlbumByID(r.Context(), id) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, apierror.NotFound("album")) - return - } - h.logger.Error("api: get album failed", "err", err) - writeErr(w, apierror.InternalMsg("lookup failed", err)) + album, apiErr := resolveByID(r, "id", q.GetAlbumByID, "album") + if apiErr != nil { + writeErr(w, apiErr) return } artist, err := q.GetArtistByID(r.Context(), album.ArtistID) @@ -76,10 +56,10 @@ func (h *handlers) handleGetAlbum(w http.ResponseWriter, r *http.Request) { var tracks []dbq.Track if user, ok := auth.UserFromContext(r.Context()); ok { tracks, err = q.ListTracksByAlbumForUser(r.Context(), dbq.ListTracksByAlbumForUserParams{ - AlbumID: id, UserID: user.ID, + AlbumID: album.ID, UserID: user.ID, }) } else { - tracks, err = q.ListTracksByAlbum(r.Context(), id) + tracks, err = q.ListTracksByAlbum(r.Context(), album.ID) } if err != nil { h.logger.Error("api: list tracks failed", "err", err) @@ -104,23 +84,13 @@ func (h *handlers) handleGetAlbum(w http.ResponseWriter, r *http.Request) { // each album carries its own track_count (one count query per album, same // pattern as subsonic). At M1 sizes (albums-per-artist << 100) this is fine. func (h *handlers) handleGetArtist(w http.ResponseWriter, r *http.Request) { - id, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("bad_request", "invalid artist id")) - return - } q := dbq.New(h.pool) - artist, err := q.GetArtistByID(r.Context(), id) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, apierror.NotFound("artist")) - return - } - h.logger.Error("api: get artist failed", "err", err) - writeErr(w, apierror.InternalMsg("lookup failed", err)) + artist, apiErr := resolveByID(r, "id", q.GetArtistByID, "artist") + if apiErr != nil { + writeErr(w, apiErr) return } - albums, err := q.ListAlbumsByArtist(r.Context(), id) + albums, err := q.ListAlbumsByArtist(r.Context(), artist.ID) if err != nil { h.logger.Error("api: list albums by artist failed", "err", err) writeErr(w, apierror.InternalMsg("lookup failed", err)) diff --git a/internal/api/media.go b/internal/api/media.go index bd53abbf..b5583e10 100644 --- a/internal/api/media.go +++ b/internal/api/media.go @@ -8,15 +8,11 @@ package api import ( "context" - "errors" "net/http" "os" "path/filepath" "strings" - "github.com/go-chi/chi/v5" - "github.com/jackc/pgx/v5" - "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" @@ -88,20 +84,10 @@ func imageContentType(path string) string { // byte-serving to http.ServeContent so clients get Range / If-Modified-Since // / ETag for free. func (h *handlers) handleGetCover(w http.ResponseWriter, r *http.Request) { - id, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("bad_request", "invalid album id")) - return - } q := dbq.New(h.pool) - album, err := q.GetAlbumByID(r.Context(), id) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, &apierror.Error{Status: 404, Code: "not_found", Message: "album not found"}) - return - } - h.logger.Error("api: get album for cover failed", "err", err) - writeErr(w, apierror.InternalMsg("server error", err)) + album, apiErr := resolveByID(r, "id", q.GetAlbumByID, "album") + if apiErr != nil { + writeErr(w, apiErr) return } path := resolveAlbumCoverPath(r.Context(), q, album) @@ -133,20 +119,9 @@ func (h *handlers) handleGetCover(w http.ResponseWriter, r *http.Request) { // disk and delegates byte-serving to http.ServeContent, which handles Range, // If-Modified-Since, and ETag based on the file's mod time. func (h *handlers) handleGetStream(w http.ResponseWriter, r *http.Request) { - id, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("bad_request", "invalid track id")) - return - } - q := dbq.New(h.pool) - track, err := q.GetTrackByID(r.Context(), id) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, &apierror.Error{Status: 404, Code: "not_found", Message: "track not found"}) - return - } - h.logger.Error("api: get track for stream failed", "err", err) - writeErr(w, apierror.InternalMsg("server error", err)) + track, apiErr := resolveByID(r, "id", dbq.New(h.pool).GetTrackByID, "track") + if apiErr != nil { + writeErr(w, apiErr) return } f, err := os.Open(track.FilePath) diff --git a/internal/api/requests.go b/internal/api/requests.go index 906ceb2a..cdc76a3d 100644 --- a/internal/api/requests.go +++ b/internal/api/requests.go @@ -7,7 +7,6 @@ import ( "net/http" "github.com/go-chi/chi/v5" - "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" @@ -218,20 +217,9 @@ func (h *handlers) handleGetRequest(w http.ResponseWriter, r *http.Request) { return } - id, ok := parseUUID(chi.URLParam(r, "id")) - if !ok { - writeErr(w, apierror.BadRequest("bad_request", "invalid request id")) - return - } - - row, err := dbq.New(h.pool).GetLidarrRequestByID(r.Context(), id) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, apierror.NotFound("request")) - return - } - h.logger.Error("api: get request", "err", err) - writeErr(w, apierror.InternalMsg("get failed", err)) + row, apiErr := resolveByID(r, "id", dbq.New(h.pool).GetLidarrRequestByID, "request") + if apiErr != nil { + writeErr(w, apiErr) return } diff --git a/internal/api/resolve.go b/internal/api/resolve.go new file mode 100644 index 00000000..58e85f57 --- /dev/null +++ b/internal/api/resolve.go @@ -0,0 +1,41 @@ +package api + +import ( + "context" + "errors" + "net/http" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" +) + +// resolveByID parses paramKey as a UUID, fetches the row via fetch, and +// returns the row. On invalid UUID, returns BadRequest. On pgx.ErrNoRows, +// returns NotFound with code "_not_found". On other fetch +// errors, returns Internal (caller is responsible for logging if needed). +// +// Generic over T because dbq getters return concrete dbq row types. +func resolveByID[T any]( + r *http.Request, + paramKey string, + fetch func(context.Context, pgtype.UUID) (T, error), + notFoundCode string, +) (T, *apierror.Error) { + var zero T + raw := chi.URLParam(r, paramKey) + id, ok := parseUUID(raw) + if !ok { + return zero, apierror.BadRequest("bad_request", "invalid "+paramKey+" id") + } + row, err := fetch(r.Context(), id) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return zero, apierror.NotFound(notFoundCode) + } + return zero, apierror.Internal(err) + } + return row, nil +} diff --git a/internal/api/resolve_test.go b/internal/api/resolve_test.go new file mode 100644 index 00000000..475afe56 --- /dev/null +++ b/internal/api/resolve_test.go @@ -0,0 +1,101 @@ +package api + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" +) + +type fakeRow struct { + ID pgtype.UUID + Name string +} + +func reqWithParam(_ *testing.T, key, value string) *http.Request { + r := httptest.NewRequest("GET", "/", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add(key, value) + return r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx)) +} + +func TestResolveByID_InvalidUUID(t *testing.T) { + called := 0 + fetch := func(ctx context.Context, id pgtype.UUID) (fakeRow, error) { + called++ + return fakeRow{}, nil + } + r := reqWithParam(t, "id", "not-a-uuid") + row, apiErr := resolveByID(r, "id", fetch, "track") + if apiErr == nil { + t.Fatalf("expected apiErr, got nil (row=%+v)", row) + } + if apiErr.Status != 400 { + t.Errorf("Status: got %d, want 400", apiErr.Status) + } + if apiErr.Code != "bad_request" { + t.Errorf("Code: got %q, want %q", apiErr.Code, "bad_request") + } + if called != 0 { + t.Errorf("fetch was called %d times; expected 0", called) + } +} + +func TestResolveByID_NotFound(t *testing.T) { + fetch := func(ctx context.Context, id pgtype.UUID) (fakeRow, error) { + return fakeRow{}, pgx.ErrNoRows + } + r := reqWithParam(t, "id", "11111111-1111-1111-1111-111111111111") + _, apiErr := resolveByID(r, "id", fetch, "track") + if apiErr == nil { + t.Fatal("expected apiErr, got nil") + } + if apiErr.Status != 404 { + t.Errorf("Status: got %d, want 404", apiErr.Status) + } + if apiErr.Code != "track_not_found" { + t.Errorf("Code: got %q, want %q", apiErr.Code, "track_not_found") + } +} + +func TestResolveByID_Internal(t *testing.T) { + cause := errors.New("boom") + fetch := func(ctx context.Context, id pgtype.UUID) (fakeRow, error) { + return fakeRow{}, cause + } + r := reqWithParam(t, "id", "11111111-1111-1111-1111-111111111111") + _, apiErr := resolveByID(r, "id", fetch, "track") + if apiErr == nil { + t.Fatal("expected apiErr, got nil") + } + if apiErr.Status != 500 { + t.Errorf("Status: got %d, want 500", apiErr.Status) + } + if !errors.Is(apiErr, cause) { + t.Errorf("expected cause %v wrapped in apiErr; got %v", cause, apiErr) + } +} + +func TestResolveByID_Success(t *testing.T) { + want := fakeRow{Name: "alpha"} + fetch := func(ctx context.Context, id pgtype.UUID) (fakeRow, error) { + want.ID = id + return want, nil + } + r := reqWithParam(t, "id", "11111111-1111-1111-1111-111111111111") + got, apiErr := resolveByID(r, "id", fetch, "track") + if apiErr != nil { + t.Fatalf("unexpected apiErr: %v", apiErr) + } + if got.Name != want.Name { + t.Errorf("Name: got %q, want %q", got.Name, want.Name) + } + if got.ID != want.ID { + t.Errorf("ID: got %+v, want %+v", got.ID, want.ID) + } +}