From 3158451b8d56cabf63e6b43b818bab99534d0396 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 18 Jul 2026 19:30:04 -0400 Subject: [PATCH] refactor(server): drop redundant error return from req.Strings parsing (#5812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(req): drop redundant error return from Strings The error from Strings carried no information beyond emptiness — it fired exactly when the param was absent — and nearly every caller discarded it with a blank identifier. Strings now just returns the values (empty when absent), making the common optional-list reads one clean expression. The few required-param callers (scrobble, createShare) check for emptiness and return the same Subsonic error code 10 as before; their e2e tests now pin that code. Ints and Times keep their contracts by synthesizing ErrMissingParam themselves, so selectedMusicFolderIds is untouched. The jellyfin parseFields helper is inlined away, since ParseFields(p.Strings("fields")...) now compiles directly. * docs(req): clarify Strings returns nil when param is absent --- server/jellyfin/items.go | 11 ++-------- server/jellyfin/playlists.go | 2 +- server/nativeapi/missing.go | 2 +- server/nativeapi/playlists.go | 2 +- server/subsonic/bookmarks.go | 4 ++-- .../e2e/subsonic_media_annotation_test.go | 1 + server/subsonic/e2e/subsonic_sharing_test.go | 1 + server/subsonic/jukebox.go | 4 ++-- server/subsonic/library_scanning.go | 3 ++- server/subsonic/media_annotation.go | 18 +++++++-------- server/subsonic/playlists.go | 4 ++-- server/subsonic/sharing.go | 6 ++--- utils/req/req.go | 22 +++++++++---------- utils/req/req_test.go | 4 +--- 14 files changed, 38 insertions(+), 46 deletions(-) diff --git a/server/jellyfin/items.go b/server/jellyfin/items.go index 6ed24b056..c3c217cfc 100644 --- a/server/jellyfin/items.go +++ b/server/jellyfin/items.go @@ -224,20 +224,13 @@ type itemsQuery struct { albumIds []string } -// parseFields reads the Fields param, accepting both repeated params (Fields=a&Fields=b) and a -// comma-separated value (Fields=a,b), matching real Jellyfin. StringOr would keep only one value. -func parseFields(p *req.Values) dto.Fields { - values, _ := p.Strings("fields") - return dto.ParseFields(values...) -} - // parseItemsQuery also resolves the entity types (inferring them from the parent when // IncludeItemTypes is absent) and the library scope. Query keys are read lowercase because // normalizeQueryKeys folded them (Jellyfin binds case-insensitively). func (api *Router) parseItemsQuery(ctx context.Context, r *http.Request) itemsQuery { p := req.Params(r) q := itemsQuery{ - fields: parseFields(p), + fields: dto.ParseFields(p.Strings("fields")...), ids: decodedQueryIDs(r, "ids"), rawTypes: p.StringOr("includeitemtypes", ""), search: searchTerm(p), @@ -737,7 +730,7 @@ func (api *Router) itemsByIDs(ctx context.Context, ids []string, fields dto.Fiel func (api *Router) getItem(w http.ResponseWriter, r *http.Request) { id := api.resolveItemID(r.Context(), dto.DecodeID(chi.URLParam(r, "itemId"))) - fields := parseFields(req.Params(r)) + fields := dto.ParseFields(req.Params(r).Strings("fields")...) if item, ok := api.resolveItemByID(r.Context(), id, fields); ok { api.ok(w, r, item) return diff --git a/server/jellyfin/playlists.go b/server/jellyfin/playlists.go index 4d3959a79..9afee5ac3 100644 --- a/server/jellyfin/playlists.go +++ b/server/jellyfin/playlists.go @@ -191,7 +191,7 @@ func (api *Router) getPlaylistItems(w http.ResponseWriter, r *http.Request) { return } p := req.Params(r) - fields := parseFields(p) + fields := dto.ParseFields(p.Strings("fields")...) res, err := api.playlistTrackPage(repo, fields, p.IntOr("startindex", 0), p.IntOr("limit", 0)) if err != nil { api.internalError(w, r, err) diff --git a/server/nativeapi/missing.go b/server/nativeapi/missing.go index 2b455e622..0ad9bb0cc 100644 --- a/server/nativeapi/missing.go +++ b/server/nativeapi/missing.go @@ -68,7 +68,7 @@ func deleteMissingFiles(maintenance core.Maintenance) http.HandlerFunc { ctx := r.Context() p := req.Params(r) - ids, _ := p.Strings("id") + ids := p.Strings("id") var err error if len(ids) == 0 { diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index ea1cf579b..90b2f9e94 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -102,7 +102,7 @@ func deleteFromPlaylist(pls playlists.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { p := req.Params(r) playlistId, _ := p.String(":playlistId") - ids, _ := p.Strings("id") + ids := p.Strings("id") err := pls.RemoveTracks(r.Context(), playlistId, ids) if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { log.Warn(r.Context(), "Track not found in playlist", "playlistId", playlistId, "id", ids[0]) diff --git a/server/subsonic/bookmarks.go b/server/subsonic/bookmarks.go index 337712750..4a7ebaa6c 100644 --- a/server/subsonic/bookmarks.go +++ b/server/subsonic/bookmarks.go @@ -103,7 +103,7 @@ func (api *Router) GetPlayQueue(r *http.Request) (*responses.Subsonic, error) { func (api *Router) SavePlayQueue(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - ids, _ := p.Strings("id") + ids := p.Strings("id") currentID, _ := p.String("current") position := p.Int64Or("position", 0) @@ -176,7 +176,7 @@ func (api *Router) GetPlayQueueByIndex(r *http.Request) (*responses.Subsonic, er func (api *Router) SavePlayQueueByIndex(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - ids, _ := p.Strings("id") + ids := p.Strings("id") position := p.Int64Or("position", 0) diff --git a/server/subsonic/e2e/subsonic_media_annotation_test.go b/server/subsonic/e2e/subsonic_media_annotation_test.go index ec9b070de..74b5238f2 100644 --- a/server/subsonic/e2e/subsonic_media_annotation_test.go +++ b/server/subsonic/e2e/subsonic_media_annotation_test.go @@ -155,6 +155,7 @@ var _ = Describe("Media Annotation Endpoints", Ordered, func() { Expect(resp.Status).To(Equal(responses.StatusFailed)) Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(responses.ErrorMissingParameter)) }) }) diff --git a/server/subsonic/e2e/subsonic_sharing_test.go b/server/subsonic/e2e/subsonic_sharing_test.go index 03bf1f80f..0421d96ea 100644 --- a/server/subsonic/e2e/subsonic_sharing_test.go +++ b/server/subsonic/e2e/subsonic_sharing_test.go @@ -109,6 +109,7 @@ var _ = Describe("Sharing Endpoints", Ordered, func() { Expect(resp.Status).To(Equal(responses.StatusFailed)) Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(responses.ErrorMissingParameter)) }) It("updateShare returns error when id parameter is missing", func() { diff --git a/server/subsonic/jukebox.go b/server/subsonic/jukebox.go index c4bc643ab..d8bf53360 100644 --- a/server/subsonic/jukebox.go +++ b/server/subsonic/jukebox.go @@ -68,7 +68,7 @@ func (api *Router) JukeboxControl(r *http.Request) (*responses.Subsonic, error) case ActionStatus: return createResponse(pb.Status(ctx)) case ActionSet: - ids, _ := p.Strings("id") + ids := p.Strings("id") return createResponse(pb.Set(ctx, ids)) case ActionStart: return createResponse(pb.Start(ctx)) @@ -82,7 +82,7 @@ func (api *Router) JukeboxControl(r *http.Request) (*responses.Subsonic, error) offset := p.IntOr("offset", 0) return createResponse(pb.Skip(ctx, index, offset)) case ActionAdd: - ids, _ := p.Strings("id") + ids := p.Strings("id") return createResponse(pb.Add(ctx, ids)) case ActionClear: return createResponse(pb.Clear(ctx)) diff --git a/server/subsonic/library_scanning.go b/server/subsonic/library_scanning.go index e6f64456d..9630425d2 100644 --- a/server/subsonic/library_scanning.go +++ b/server/subsonic/library_scanning.go @@ -45,7 +45,8 @@ func (api *Router) StartScan(r *http.Request) (*responses.Subsonic, error) { // Parse optional target parameters for selective scanning var targets []model.ScanTarget - if targetParams, err := p.Strings("target"); err == nil && len(targetParams) > 0 { + if targetParams := p.Strings("target"); len(targetParams) > 0 { + var err error targets, err = model.ParseTargets(targetParams) if err != nil { return nil, newError(responses.ErrorGeneric, fmt.Sprintf("Invalid target parameter: %v", err)) diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index 27170c11b..cfbff3ecb 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -71,9 +71,9 @@ func (api *Router) setRating(ctx context.Context, id string, rating int) error { func (api *Router) Star(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - ids, _ := p.Strings("id") - albumIds, _ := p.Strings("albumId") - artistIds, _ := p.Strings("artistId") + ids := p.Strings("id") + albumIds := p.Strings("albumId") + artistIds := p.Strings("artistId") if len(ids)+len(albumIds)+len(artistIds) == 0 { return nil, newError(responses.ErrorMissingParameter, "Required id parameter is missing") } @@ -90,9 +90,9 @@ func (api *Router) Star(r *http.Request) (*responses.Subsonic, error) { func (api *Router) Unstar(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - ids, _ := p.Strings("id") - albumIds, _ := p.Strings("albumId") - artistIds, _ := p.Strings("artistId") + ids := p.Strings("id") + albumIds := p.Strings("albumId") + artistIds := p.Strings("artistId") if len(ids)+len(albumIds)+len(artistIds) == 0 { return nil, newError(responses.ErrorMissingParameter, "Required id parameter is missing") } @@ -163,9 +163,9 @@ func (api *Router) setStar(ctx context.Context, star bool, ids ...string) error func (api *Router) Scrobble(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - ids, err := p.Strings("id") - if err != nil { - return nil, err + ids := p.Strings("id") + if len(ids) == 0 { + return nil, newError(responses.ErrorMissingParameter, "missing parameter: 'id'") } times, _ := p.Times("time") if len(times) > 0 && len(times) != len(ids) { diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index c58fb9ab9..17ba1b2c9 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -62,7 +62,7 @@ func (api *Router) getPlaylist(ctx context.Context, id string) (*responses.Subso func (api *Router) CreatePlaylist(r *http.Request) (*responses.Subsonic, error) { ctx := r.Context() p := req.Params(r) - songIds, _ := p.Strings("songId") + songIds := p.Strings("songId") playlistId, _ := p.String("playlistId") name, _ := p.String("name") if playlistId == "" && name == "" { @@ -99,7 +99,7 @@ func (api *Router) UpdatePlaylist(r *http.Request) (*responses.Subsonic, error) if err != nil { return nil, err } - songsToAdd, _ := p.Strings("songIdToAdd") + songsToAdd := p.Strings("songIdToAdd") songIndexesToRemove, _ := p.Ints("songIndexToRemove") var plsName *string if s, err := p.String("name"); err == nil { diff --git a/server/subsonic/sharing.go b/server/subsonic/sharing.go index a9ccfdca4..540ae79d7 100644 --- a/server/subsonic/sharing.go +++ b/server/subsonic/sharing.go @@ -52,9 +52,9 @@ func (api *Router) buildShare(r *http.Request, share model.Share) responses.Shar func (api *Router) CreateShare(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) - ids, err := p.Strings("id") - if err != nil { - return nil, err + ids := p.Strings("id") + if len(ids) == 0 { + return nil, newError(responses.ErrorMissingParameter, "missing parameter: 'id'") } description, _ := p.String("description") diff --git a/utils/req/req.go b/utils/req/req.go index 2757fc3f5..861cca9f7 100644 --- a/utils/req/req.go +++ b/utils/req/req.go @@ -60,12 +60,10 @@ func (r *Values) StringOr(param, def string) string { return v } -func (r *Values) Strings(param string) ([]string, error) { - values := r.URL.Query()[param] - if len(values) == 0 { - return nil, newError(ErrMissingParam, param) - } - return values, nil +// Strings returns all occurrences of the param, or a nil (empty) slice when absent. Callers that +// require the param should check for emptiness themselves. +func (r *Values) Strings(param string) []string { + return r.URL.Query()[param] } func (r *Values) TimeOr(param string, def time.Time) time.Time { @@ -85,9 +83,9 @@ func (r *Values) TimeOr(param string, def time.Time) time.Time { } func (r *Values) Times(param string) ([]time.Time, error) { - pStr, err := r.Strings(param) - if err != nil { - return nil, err + pStr := r.Strings(param) + if len(pStr) == 0 { + return nil, newError(ErrMissingParam, param) } times := make([]time.Time, len(pStr)) for i, t := range pStr { @@ -139,9 +137,9 @@ func (r *Values) Int64Or(param string, def int64) int64 { } func (r *Values) Ints(param string) ([]int, error) { - pStr, err := r.Strings(param) - if err != nil { - return nil, err + pStr := r.Strings(param) + if len(pStr) == 0 { + return nil, newError(ErrMissingParam, param) } ints := make([]int, 0, len(pStr)) for _, s := range pStr { diff --git a/utils/req/req_test.go b/utils/req/req_test.go index d76b3b934..5f9de8483 100644 --- a/utils/req/req_test.go +++ b/utils/req/req_test.go @@ -60,9 +60,7 @@ var _ = Describe("Request Helpers", func() { }) It("returns empty array if param does not exist", func() { - v, err := r.Strings("xx") - Expect(err).To(MatchError(req.ErrMissingParam)) - Expect(v).To(BeEmpty()) + Expect(r.Strings("xx")).To(BeEmpty()) }) })