From 1cc7eb6cad71b6915cae928853ee5ecdd549b9e7 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 7 May 2026 19:48:25 -0400 Subject: [PATCH] refactor(server/api): writeErr accepts error; migrate admin handlers (1/3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite writeErr(w, err) to wrap *apierror.Error via apierror.From, preserving the existing {"error": {"code", "message"}} wire envelope. Add writeErrWithLog helper for 500-class errors that need an operator log line. Migrate all 13 admin_*.go handler files (~76 call sites) to the new signature; T3 will sweep the remaining api package. The old 4-arg writeErr is removed, so non-admin call sites in internal/api will not compile until T3 lands. This is by design — T2 and T3 are paired. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/admin_cover_sources.go | 9 +- internal/api/admin_coverage.go | 4 +- internal/api/admin_coverart_research.go | 9 +- internal/api/admin_covers.go | 11 +-- internal/api/admin_invites.go | 24 ++--- internal/api/admin_scan.go | 7 +- internal/api/admin_scan_schedule.go | 16 ++- internal/api/admin_smtp.go | 28 +++--- internal/api/admin_tracks.go | 9 +- internal/api/admin_users.go | 91 ++++++++--------- internal/api/errors.go | 35 +++++-- internal/api/errors_test.go | 125 ++++++++++++++++++++---- 12 files changed, 229 insertions(+), 139 deletions(-) diff --git a/internal/api/admin_cover_sources.go b/internal/api/admin_cover_sources.go index 80a7d988..28d6bafd 100644 --- a/internal/api/admin_cover_sources.go +++ b/internal/api/admin_cover_sources.go @@ -8,6 +8,7 @@ import ( "github.com/go-chi/chi/v5" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" ) @@ -75,7 +76,7 @@ func (h *handlers) handleUpdateCoverSource(w http.ResponseWriter, r *http.Reques var body updateCoverSourceReq if err := json.NewDecoder(r.Body).Decode(&body); err != nil { - writeErr(w, http.StatusBadRequest, "bad_body", "invalid JSON") + writeErr(w, apierror.BadRequest("bad_body", "invalid JSON")) return } @@ -85,11 +86,11 @@ func (h *handlers) handleUpdateCoverSource(w http.ResponseWriter, r *http.Reques }) if err != nil { if errors.Is(err, coverart.ErrProviderNotFound) { - writeErr(w, http.StatusNotFound, "not_found", "no such provider") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "no such provider"}) return } h.logger.Error("admin: update cover source", "id", id, "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "update failed") + writeErr(w, apierror.Internal(err)) return } @@ -141,7 +142,7 @@ func (h *handlers) handleTestCoverSource(w http.ResponseWriter, r *http.Request) } if errors.Is(err, coverart.ErrProviderNotFound) { - writeErr(w, http.StatusNotFound, "not_found", "no such provider") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "no such provider"}) return } diff --git a/internal/api/admin_coverage.go b/internal/api/admin_coverage.go index eb43a2da..cf762ae1 100644 --- a/internal/api/admin_coverage.go +++ b/internal/api/admin_coverage.go @@ -3,6 +3,7 @@ package api import ( "net/http" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) @@ -24,8 +25,7 @@ func (h *handlers) handleGetLibraryCoverage(w http.ResponseWriter, r *http.Reque q := dbq.New(h.pool) row, err := q.GetAlbumCoverageRollup(r.Context()) if err != nil { - h.logger.Error("admin: get coverage rollup", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "lookup failed") + writeErrWithLog(w, h.logger, "admin: get coverage rollup", apierror.Internal(err)) return } writeJSON(w, http.StatusOK, coverageRollupResp{ diff --git a/internal/api/admin_coverart_research.go b/internal/api/admin_coverart_research.go index 98b835ce..bcf97193 100644 --- a/internal/api/admin_coverart_research.go +++ b/internal/api/admin_coverart_research.go @@ -1,6 +1,10 @@ package api -import "net/http" +import ( + "net/http" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" +) // handleResearchMissingArt bumps the cover-art version unconditionally // so every 'none' row becomes eligible at the next enrichment pass. @@ -14,8 +18,7 @@ import "net/http" func (h *handlers) handleResearchMissingArt(w http.ResponseWriter, r *http.Request) { newVer, err := h.coverSettings.BumpVersion(r.Context()) if err != nil { - h.logger.Error("admin: research missing art bump failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "bump failed") + writeErrWithLog(w, h.logger, "admin: research missing art bump failed", apierror.Internal(err)) return } writeJSON(w, http.StatusOK, map[string]any{"version": newVer}) diff --git a/internal/api/admin_covers.go b/internal/api/admin_covers.go index 056565d0..84dfa979 100644 --- a/internal/api/admin_covers.go +++ b/internal/api/admin_covers.go @@ -8,6 +8,7 @@ import ( "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" ) @@ -23,22 +24,20 @@ type adminCoverRefetchResp struct { func (h *handlers) handleAdminAlbumRefetchCover(w http.ResponseWriter, r *http.Request) { albumID, ok := parseUUID(chi.URLParam(r, "id")) if !ok { - writeErr(w, http.StatusBadRequest, "bad_request", "invalid album id") + writeErr(w, apierror.BadRequest("bad_request", "invalid album id")) return } if err := h.coverart.RetryAlbum(r.Context(), albumID); err != nil { if errors.Is(err, coverart.ErrNotFound) || errors.Is(err, pgx.ErrNoRows) { - writeErr(w, http.StatusNotFound, "not_found", "album not found") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "album not found"}) return } - h.logger.Error("admin: cover refetch failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "refetch failed") + writeErrWithLog(w, h.logger, "admin: cover refetch failed", apierror.Internal(err)) return } row, err := dbq.New(h.pool).GetAlbumWithFirstTrackPath(r.Context(), albumID) if err != nil { - h.logger.Error("admin: post-refetch read failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "read failed") + writeErrWithLog(w, h.logger, "admin: post-refetch read failed", apierror.Internal(err)) return } writeJSON(w, http.StatusOK, adminCoverRefetchResp{ diff --git a/internal/api/admin_invites.go b/internal/api/admin_invites.go index 6117f594..44cd28d0 100644 --- a/internal/api/admin_invites.go +++ b/internal/api/admin_invites.go @@ -12,6 +12,7 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" @@ -41,8 +42,7 @@ func (h *handlers) handleListInvites(w http.ResponseWriter, r *http.Request) { q := dbq.New(h.pool) rows, err := q.ListInvitesActive(r.Context()) if err != nil { - h.logger.Error("admin: list invites failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: list invites failed", apierror.Internal(err)) return } out := make([]inviteResp, 0, len(rows)) @@ -59,7 +59,7 @@ type createInviteReq struct { func (h *handlers) handleCreateInvite(w http.ResponseWriter, r *http.Request) { user, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } var req createInviteReq @@ -70,8 +70,7 @@ func (h *handlers) handleCreateInvite(w http.ResponseWriter, r *http.Request) { tokenBytes := make([]byte, 16) if _, err := rand.Read(tokenBytes); err != nil { - h.logger.Error("admin: invite token gen failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: invite token gen failed", apierror.Internal(err)) return } token := hex.EncodeToString(tokenBytes) @@ -90,8 +89,7 @@ func (h *handlers) handleCreateInvite(w http.ResponseWriter, r *http.Request) { ExpiresAt: pgtype.Timestamptz{Time: expiresAt, Valid: true}, }) if err != nil { - h.logger.Error("admin: create invite failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: create invite failed", apierror.Internal(err)) return } @@ -106,12 +104,12 @@ func (h *handlers) handleCreateInvite(w http.ResponseWriter, r *http.Request) { func (h *handlers) handleDeleteInvite(w http.ResponseWriter, r *http.Request) { user, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } token := chi.URLParam(r, "token") if token == "" { - writeErr(w, http.StatusBadRequest, "invalid_token", "") + writeErr(w, apierror.BadRequest("invalid_token", "")) return } q := dbq.New(h.pool) @@ -119,16 +117,14 @@ func (h *handlers) handleDeleteInvite(w http.ResponseWriter, r *http.Request) { // return a useful 404 vs 200. if _, err := q.GetInviteByToken(r.Context(), token); err != nil { if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, http.StatusNotFound, "not_found", "") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: ""}) return } - h.logger.Error("admin: get invite failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: get invite failed", apierror.Internal(err)) return } if err := q.DeleteInvite(r.Context(), token); err != nil { - h.logger.Error("admin: delete invite failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: delete invite failed", apierror.Internal(err)) return } if err := audit.Write(r.Context(), h.pool, user.ID, pgtype.UUID{}, audit.ActionInviteRevoke, diff --git a/internal/api/admin_scan.go b/internal/api/admin_scan.go index 70ba6bdf..f9859f50 100644 --- a/internal/api/admin_scan.go +++ b/internal/api/admin_scan.go @@ -8,6 +8,7 @@ import ( "github.com/jackc/pgx/v5" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" ) @@ -39,8 +40,7 @@ func (h *handlers) handleGetScanStatus(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, scanStatusResp{InFlight: false}) return } - h.logger.Error("admin: get scan status", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "lookup failed") + writeErrWithLog(w, h.logger, "admin: get scan status", apierror.Internal(err)) return } @@ -85,8 +85,7 @@ func (h *handlers) handleTriggerScan(w http.ResponseWriter, _ *http.Request) { h.logger.With("source", "manual"), h.scanCfg, ) if err != nil { - h.logger.Error("admin: trigger scan failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "trigger failed") + writeErrWithLog(w, h.logger, "admin: trigger scan failed", apierror.Internal(err)) return } if !started { diff --git a/internal/api/admin_scan_schedule.go b/internal/api/admin_scan_schedule.go index aa3816f8..4727fe36 100644 --- a/internal/api/admin_scan_schedule.go +++ b/internal/api/admin_scan_schedule.go @@ -9,6 +9,7 @@ import ( "github.com/jackc/pgx/v5" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/library" ) @@ -34,11 +35,10 @@ func (h *handlers) handleGetScanSchedule(w http.ResponseWriter, r *http.Request) if err != nil { if errors.Is(err, pgx.ErrNoRows) { h.logger.Error("admin: scan_schedule singleton row missing") - writeErr(w, http.StatusInternalServerError, "server_error", "schedule row missing") + writeErr(w, apierror.Internal(errors.New("schedule row missing"))) return } - h.logger.Error("admin: get scan schedule", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "lookup failed") + writeErrWithLog(w, h.logger, "admin: get scan schedule", apierror.Internal(err)) return } @@ -48,12 +48,12 @@ func (h *handlers) handleGetScanSchedule(w http.ResponseWriter, r *http.Request) func (h *handlers) handlePatchScanSchedule(w http.ResponseWriter, r *http.Request) { var req scanScheduleReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeErr(w, http.StatusBadRequest, "bad_body", "invalid JSON") + writeErr(w, apierror.BadRequest("bad_body", "invalid JSON")) return } if err := validateScheduleReq(&req); err != nil { - writeErr(w, http.StatusBadRequest, "validation", err.Error()) + writeErr(w, apierror.BadRequest("validation", err.Error())) return } @@ -64,8 +64,7 @@ func (h *handlers) handlePatchScanSchedule(w http.ResponseWriter, r *http.Reques TimeOfDay: req.TimeOfDay, WeeklyDay: pgInt32Ptr(req.WeeklyDay), }); err != nil { - h.logger.Error("admin: update scan schedule", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "update failed") + writeErrWithLog(w, h.logger, "admin: update scan schedule", apierror.Internal(err)) return } @@ -73,8 +72,7 @@ func (h *handlers) handlePatchScanSchedule(w http.ResponseWriter, r *http.Reques row, err := q.GetScanSchedule(r.Context()) if err != nil { - h.logger.Error("admin: re-read scan schedule", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "re-read failed") + writeErrWithLog(w, h.logger, "admin: re-read scan schedule", apierror.Internal(err)) return } cfg := library.ScheduleConfig{Mode: row.Mode} diff --git a/internal/api/admin_smtp.go b/internal/api/admin_smtp.go index aa246ca0..83ad0e13 100644 --- a/internal/api/admin_smtp.go +++ b/internal/api/admin_smtp.go @@ -7,6 +7,7 @@ import ( "net/http" "time" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/mailer" @@ -38,8 +39,7 @@ func (h *handlers) handleGetSMTPConfig(w http.ResponseWriter, r *http.Request) { q := dbq.New(h.pool) cfg, err := q.GetSMTPConfig(r.Context()) if err != nil { - h.logger.Error("admin smtp: get failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin smtp: get failed", apierror.Internal(err)) return } masked := "" @@ -61,21 +61,20 @@ func (h *handlers) handleGetSMTPConfig(w http.ResponseWriter, r *http.Request) { func (h *handlers) handleUpdateSMTPConfig(w http.ResponseWriter, r *http.Request) { var req updateSMTPConfigReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeErr(w, http.StatusBadRequest, "invalid_body", "") + writeErr(w, apierror.BadRequest("invalid_body", "")) return } if req.Enabled { if req.Host == "" || req.FromAddress == "" { - writeErr(w, http.StatusBadRequest, "missing_fields", - "host and from_address are required when enabled") + writeErr(w, apierror.BadRequest("missing_fields", + "host and from_address are required when enabled")) return } } q := dbq.New(h.pool) current, err := q.GetSMTPConfig(r.Context()) if err != nil { - h.logger.Error("admin smtp: get failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin smtp: get failed", apierror.Internal(err)) return } // Preserve existing password when the request sends "" (so the @@ -94,8 +93,7 @@ func (h *handlers) handleUpdateSMTPConfig(w http.ResponseWriter, r *http.Request FromName: req.FromName, UseTls: req.UseTLS, }); err != nil { - h.logger.Error("admin smtp: update failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin smtp: update failed", apierror.Internal(err)) return } w.WriteHeader(http.StatusNoContent) @@ -104,12 +102,12 @@ func (h *handlers) handleUpdateSMTPConfig(w http.ResponseWriter, r *http.Request func (h *handlers) handleTestSMTPConfig(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } if caller.Email == nil || *caller.Email == "" { - writeErr(w, http.StatusBadRequest, "no_email_on_file", - "set your email in /settings before testing SMTP") + writeErr(w, apierror.BadRequest("no_email_on_file", + "set your email in /settings before testing SMTP")) return } @@ -123,12 +121,12 @@ func (h *handlers) handleTestSMTPConfig(w http.ResponseWriter, r *http.Request) htmlBody := "

This is a test message from your Minstrel server.

If you got this, your SMTP config is working.

— Minstrel

" if err := sender.Send(ctx, *caller.Email, "Minstrel SMTP test", textBody, htmlBody); err != nil { if errors.Is(err, mailer.ErrNotConfigured) { - writeErr(w, http.StatusBadRequest, "not_configured", - "SMTP is not enabled or required fields are empty") + writeErr(w, apierror.BadRequest("not_configured", + "SMTP is not enabled or required fields are empty")) return } h.logger.Error("admin smtp: test send failed", "err", err) - writeErr(w, http.StatusInternalServerError, "send_failed", err.Error()) + writeErr(w, &apierror.Error{Status: http.StatusInternalServerError, Code: "send_failed", Message: err.Error()}) return } w.WriteHeader(http.StatusNoContent) diff --git a/internal/api/admin_tracks.go b/internal/api/admin_tracks.go index 81f1b85f..8dca2780 100644 --- a/internal/api/admin_tracks.go +++ b/internal/api/admin_tracks.go @@ -7,6 +7,7 @@ import ( "github.com/go-chi/chi/v5" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/tracks" ) @@ -40,7 +41,7 @@ func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) { // 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") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "track not found"}) return } @@ -55,7 +56,7 @@ func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) { 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") + writeErr(w, apierror.Unauthorized("unauthenticated", "no session")) return } @@ -64,11 +65,11 @@ func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) { ) if err != nil { if errors.Is(err, tracks.ErrNotFound) { - writeErr(w, http.StatusNotFound, "not_found", "track not found") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: "track not found"}) return } h.logger.Error("api: remove track failed", "err", err, "track_id", idStr) - writeErr(w, http.StatusInternalServerError, "server_error", "remove failed") + writeErr(w, apierror.Internal(err)) return } diff --git a/internal/api/admin_users.go b/internal/api/admin_users.go index 491f08cb..032db765 100644 --- a/internal/api/admin_users.go +++ b/internal/api/admin_users.go @@ -12,6 +12,7 @@ import ( "github.com/jackc/pgx/v5/pgconn" "golang.org/x/crypto/bcrypt" + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" @@ -34,8 +35,7 @@ func (h *handlers) handleAdminListUsers(w http.ResponseWriter, r *http.Request) q := dbq.New(h.pool) rows, err := q.ListUsers(r.Context()) if err != nil { - h.logger.Error("admin: list users failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: list users failed", apierror.Internal(err)) return } out := make([]adminUserView, 0, len(rows)) @@ -60,17 +60,17 @@ type updateUserAdminReq struct { func (h *handlers) handleUpdateUserAdmin(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } targetID, ok := parseUUID(chi.URLParam(r, "id")) if !ok { - writeErr(w, http.StatusBadRequest, "invalid_id", "") + writeErr(w, apierror.BadRequest("invalid_id", "")) return } var req updateUserAdminReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeErr(w, http.StatusBadRequest, "invalid_body", "") + writeErr(w, apierror.BadRequest("invalid_body", "")) return } @@ -86,8 +86,7 @@ func (h *handlers) handleUpdateUserAdmin(w http.ResponseWriter, r *http.Request) if !req.IsAdmin { count, err := q.CountAdmins(r.Context()) if err != nil { - h.logger.Error("admin: count admins failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: count admins failed", apierror.Internal(err)) return } // We need to know whether the target is currently an admin. @@ -95,13 +94,12 @@ func (h *handlers) handleUpdateUserAdmin(w http.ResponseWriter, r *http.Request) // allow it). If they ARE admin and they're the last one, refuse. current, err := q.GetUserByID(r.Context(), targetID) if err != nil { - h.logger.Error("admin: get user failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: get user failed", apierror.Internal(err)) return } if current.IsAdmin && count <= 1 { - writeErr(w, http.StatusConflict, "last_admin", - "can't remove the last admin — promote someone else first") + writeErr(w, apierror.Conflict("last_admin", + "can't remove the last admin — promote someone else first")) return } } @@ -111,8 +109,7 @@ func (h *handlers) handleUpdateUserAdmin(w http.ResponseWriter, r *http.Request) IsAdmin: req.IsAdmin, }) if err != nil { - h.logger.Error("admin: update user admin failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin: update user admin failed", apierror.Internal(err)) return } @@ -146,33 +143,31 @@ type adminCreateUserReq struct { func (h *handlers) handleAdminCreateUser(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } var req adminCreateUserReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeErr(w, http.StatusBadRequest, "invalid_body", "") + writeErr(w, apierror.BadRequest("invalid_body", "")) return } if !usernameRe.MatchString(req.Username) { - writeErr(w, http.StatusBadRequest, "username_invalid", "username must be 3-32 chars, alphanumeric + underscore + hyphen") + writeErr(w, apierror.BadRequest("username_invalid", "username must be 3-32 chars, alphanumeric + underscore + hyphen")) return } if len(req.Password) < minPasswordLength { - writeErr(w, http.StatusBadRequest, "password_too_short", "password must be at least 8 chars") + writeErr(w, apierror.BadRequest("password_too_short", "password must be at least 8 chars")) return } hash, err := bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost) if err != nil { - h.logger.Error("admin create user: hash failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin create user: hash failed", apierror.Internal(err)) return } apiToken, err := auth.MintSessionToken() if err != nil { - h.logger.Error("admin create user: mint token failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin create user: mint token failed", apierror.Internal(err)) return } @@ -187,11 +182,10 @@ func (h *handlers) handleAdminCreateUser(w http.ResponseWriter, r *http.Request) if err != nil { var pgErr *pgconn.PgError if errors.As(err, &pgErr) && pgErr.Code == pgerrcode.UniqueViolation { - writeErr(w, http.StatusConflict, "username_taken", "") + writeErr(w, apierror.Conflict("username_taken", "")) return } - h.logger.Error("admin create user: insert failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin create user: insert failed", apierror.Internal(err)) return } @@ -215,12 +209,12 @@ func (h *handlers) handleAdminCreateUser(w http.ResponseWriter, r *http.Request) func (h *handlers) handleAdminDeleteUser(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } targetID, ok := parseUUID(chi.URLParam(r, "id")) if !ok { - writeErr(w, http.StatusBadRequest, "invalid_id", "") + writeErr(w, apierror.BadRequest("invalid_id", "")) return } @@ -228,11 +222,10 @@ func (h *handlers) handleAdminDeleteUser(w http.ResponseWriter, r *http.Request) target, err := q.GetUserByID(r.Context(), targetID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, http.StatusNotFound, "not_found", "") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: ""}) return } - h.logger.Error("admin delete user: lookup failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin delete user: lookup failed", apierror.Internal(err)) return } @@ -240,20 +233,18 @@ func (h *handlers) handleAdminDeleteUser(w http.ResponseWriter, r *http.Request) if target.IsAdmin { count, cerr := q.CountAdmins(r.Context()) if cerr != nil { - h.logger.Error("admin delete user: count admins failed", "err", cerr) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin delete user: count admins failed", apierror.Internal(cerr)) return } if count <= 1 { - writeErr(w, http.StatusConflict, "last_admin", - "can't delete the last admin — promote someone else first") + writeErr(w, apierror.Conflict("last_admin", + "can't delete the last admin — promote someone else first")) return } } if err := q.DeleteUser(r.Context(), targetID); err != nil { - h.logger.Error("admin delete user: delete failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin delete user: delete failed", apierror.Internal(err)) return } @@ -274,21 +265,21 @@ type adminResetPasswordReq struct { func (h *handlers) handleAdminResetPassword(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } targetID, ok := parseUUID(chi.URLParam(r, "id")) if !ok { - writeErr(w, http.StatusBadRequest, "invalid_id", "") + writeErr(w, apierror.BadRequest("invalid_id", "")) return } var req adminResetPasswordReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeErr(w, http.StatusBadRequest, "invalid_body", "") + writeErr(w, apierror.BadRequest("invalid_body", "")) return } if len(req.Password) < minPasswordLength { - writeErr(w, http.StatusBadRequest, "password_too_short", "password must be at least 8 chars") + writeErr(w, apierror.BadRequest("password_too_short", "password must be at least 8 chars")) return } @@ -296,26 +287,23 @@ func (h *handlers) handleAdminResetPassword(w http.ResponseWriter, r *http.Reque // Verify target exists for a useful 404. if _, err := q.GetUserByID(r.Context(), targetID); err != nil { if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, http.StatusNotFound, "not_found", "") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: ""}) return } - h.logger.Error("admin reset password: lookup failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin reset password: lookup failed", apierror.Internal(err)) return } hash, err := bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost) if err != nil { - h.logger.Error("admin reset password: hash failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin reset password: hash failed", apierror.Internal(err)) return } if err := q.ResetUserPassword(r.Context(), dbq.ResetUserPasswordParams{ ID: targetID, PasswordHash: string(hash), }); err != nil { - h.logger.Error("admin reset password: update failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin reset password: update failed", apierror.Internal(err)) return } @@ -335,17 +323,17 @@ type adminAutoApproveReq struct { func (h *handlers) handleAdminAutoApproveToggle(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { - writeErr(w, http.StatusUnauthorized, "auth_required", "") + writeErr(w, apierror.Unauthorized("auth_required", "")) return } targetID, ok := parseUUID(chi.URLParam(r, "id")) if !ok { - writeErr(w, http.StatusBadRequest, "invalid_id", "") + writeErr(w, apierror.BadRequest("invalid_id", "")) return } var req adminAutoApproveReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - writeErr(w, http.StatusBadRequest, "invalid_body", "") + writeErr(w, apierror.BadRequest("invalid_body", "")) return } @@ -356,11 +344,10 @@ func (h *handlers) handleAdminAutoApproveToggle(w http.ResponseWriter, r *http.R }) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - writeErr(w, http.StatusNotFound, "not_found", "") + writeErr(w, &apierror.Error{Status: http.StatusNotFound, Code: "not_found", Message: ""}) return } - h.logger.Error("admin auto-approve: update failed", "err", err) - writeErr(w, http.StatusInternalServerError, "server_error", "") + writeErrWithLog(w, h.logger, "admin auto-approve: update failed", apierror.Internal(err)) return } diff --git a/internal/api/errors.go b/internal/api/errors.go index 3380f851..58410641 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -2,11 +2,18 @@ package api import ( "encoding/json" + "log/slog" "net/http" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" ) -// errorBody is the JSON envelope for failures on /api/*. Kept separate from -// the Subsonic envelope so native clients don't have to parse two shapes. +// errorBody / errorPayload preserve the existing wire envelope: +// +// {"error": {"code": "...", "message": "..."}} +// +// Kept separate from the Subsonic envelope so native clients don't have +// to parse two shapes. type errorBody struct { Error errorPayload `json:"error"` } @@ -16,12 +23,26 @@ type errorPayload struct { Message string `json:"message"` } -// writeErr is the canonical way to emit a failure. Code is a stable -// short-string clients can switch on; message is human-readable. -func writeErr(w http.ResponseWriter, status int, code, message string) { +// writeErr emits the failure envelope for err. If err is or wraps +// *apierror.Error, its Status/Code/Message drive the response; +// otherwise it's mapped to a 500 server_error. +func writeErr(w http.ResponseWriter, err error) { + apiErr := apierror.From(err) w.Header().Set("Content-Type", "application/json") - w.WriteHeader(status) - _ = json.NewEncoder(w).Encode(errorBody{Error: errorPayload{Code: code, Message: message}}) + w.WriteHeader(apiErr.Status) + _ = json.NewEncoder(w).Encode(errorBody{Error: errorPayload{ + Code: apiErr.Code, + Message: apiErr.Message, + }}) +} + +// writeErrWithLog logs the error at Error level and writes the response. +// Use for 500-class errors where the operator needs the cause in logs. +func writeErrWithLog(w http.ResponseWriter, logger *slog.Logger, msg string, err error) { + if logger != nil { + logger.Error(msg, "err", err) + } + writeErr(w, err) } // writeJSON emits a 2xx response with the given body JSON-encoded. Mirrors diff --git a/internal/api/errors_test.go b/internal/api/errors_test.go index 8c7a4de1..8cc627c5 100644 --- a/internal/api/errors_test.go +++ b/internal/api/errors_test.go @@ -2,31 +2,118 @@ package api import ( "encoding/json" - "net/http" + "errors" + "fmt" "net/http/httptest" "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" ) -func TestWriteErr_EmitsJSONEnvelope(t *testing.T) { - w := httptest.NewRecorder() - writeErr(w, http.StatusBadRequest, "bad_request", "field x required") +func decodeEnvelope(t *testing.T, body []byte) errorBody { + t.Helper() + var env errorBody + if err := json.Unmarshal(body, &env); err != nil { + t.Fatalf("decode envelope: %v (body=%s)", err, string(body)) + } + return env +} - if w.Code != http.StatusBadRequest { - t.Errorf("status = %d, want 400", w.Code) +func TestWriteErr_APIError(t *testing.T) { + rec := httptest.NewRecorder() + writeErr(rec, apierror.NotFound("track")) + + if rec.Code != 404 { + t.Fatalf("status: want 404, got %d", rec.Code) } - if got := w.Header().Get("Content-Type"); got != "application/json" { - t.Errorf("content-type = %q, want application/json", got) + env := decodeEnvelope(t, rec.Body.Bytes()) + if env.Error.Code != "track_not_found" { + t.Errorf("code: want track_not_found, got %q", env.Error.Code) } - var body struct { - Error struct { - Code string `json:"code"` - Message string `json:"message"` - } `json:"error"` - } - if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { - t.Fatalf("decode: %v\nbody=%s", err, w.Body.String()) - } - if body.Error.Code != "bad_request" || body.Error.Message != "field x required" { - t.Errorf("body = %+v", body) + if env.Error.Message != "track not found" { + t.Errorf("message: want %q, got %q", "track not found", env.Error.Message) + } +} + +func TestWriteErr_Sentinel(t *testing.T) { + rec := httptest.NewRecorder() + writeErr(rec, apierror.ErrUnauthorized) + + if rec.Code != 401 { + t.Fatalf("status: want 401, got %d", rec.Code) + } + env := decodeEnvelope(t, rec.Body.Bytes()) + if env.Error.Code != "unauthorized" { + t.Errorf("code: want unauthorized, got %q", env.Error.Code) + } + if env.Error.Message != "authentication required" { + t.Errorf("message: want %q, got %q", "authentication required", env.Error.Message) + } +} + +func TestWriteErr_PlainError(t *testing.T) { + rec := httptest.NewRecorder() + writeErr(rec, errors.New("boom")) + + if rec.Code != 500 { + t.Fatalf("status: want 500, got %d", rec.Code) + } + env := decodeEnvelope(t, rec.Body.Bytes()) + if env.Error.Code != "server_error" { + t.Errorf("code: want server_error, got %q", env.Error.Code) + } + if env.Error.Message != "internal server error" { + t.Errorf("message: want %q, got %q", "internal server error", env.Error.Message) + } +} + +func TestWriteErr_WrappedAPIError(t *testing.T) { + rec := httptest.NewRecorder() + writeErr(rec, fmt.Errorf("ctx: %w", apierror.BadRequest("bad", "no good"))) + + if rec.Code != 400 { + t.Fatalf("status: want 400, got %d", rec.Code) + } + env := decodeEnvelope(t, rec.Body.Bytes()) + if env.Error.Code != "bad" { + t.Errorf("code: want bad, got %q", env.Error.Code) + } + if env.Error.Message != "no good" { + t.Errorf("message: want %q, got %q", "no good", env.Error.Message) + } +} + +func TestWriteErr_EnvelopeShape(t *testing.T) { + rec := httptest.NewRecorder() + writeErr(rec, apierror.BadRequest("x", "y")) + + var raw map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &raw); err != nil { + t.Fatalf("decode raw: %v", err) + } + if len(raw) != 1 { + t.Fatalf("top-level keys: want 1, got %d (%v)", len(raw), raw) + } + inner, ok := raw["error"].(map[string]any) + if !ok { + t.Fatalf("error: want object, got %T", raw["error"]) + } + if len(inner) != 2 { + t.Fatalf("inner keys: want 2, got %d (%v)", len(inner), inner) + } + if _, ok := inner["code"]; !ok { + t.Errorf("inner.code missing") + } + if _, ok := inner["message"]; !ok { + t.Errorf("inner.message missing") + } +} + +func TestWriteErr_ContentType(t *testing.T) { + rec := httptest.NewRecorder() + writeErr(rec, apierror.BadRequest("x", "y")) + + if got := rec.Header().Get("Content-Type"); got != "application/json" { + t.Errorf("Content-Type: want application/json, got %q", got) } }