From 7a11ca69bbfd3465ef4ce2af52966a97c51de77e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 19 Aug 2026 08:36:44 -0400 Subject: [PATCH 1/9] fix(jellyfin): honor the Filters, SortBy and MaxHeight params clients actually send (#5981) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(jellyfin): honor Filters=IsFavorite on /Artists and /Artists/AlbumArtists listArtistsByRole hand-built its itemsQuery and never set favOnly, so the favorites filter was silently dropped on both artist routes while /Items honored it. Finamp's home screen asks for favorite artists once per load and was served the entire artist list instead: 10,298 artists, 6.15 MB, 2.7s on a real library, and the wrong data on screen. Extract the favOnly parsing that parseItemsQuery already did into parseFavOnly and use it in both places. listArtists now adds the starred predicate to notMissing rather than replacing it, matching listAlbums and listSongs, so a favorite artist whose files are gone stays excluded. * fix(jellyfin): map SortBy=Runtime to duration for albums and songs sortColumnsByType had no runtime/runtimeticks key for any type, so Finamp's "Duration" sort silently misbehaved in two different ways. Albums: Finamp sends a bare SortBy=Runtime. Nothing matched, opts.Sort stayed empty, and applyOptions skips OrderBy entirely when Sort is empty — so the query ran with no ORDER BY at all and Ascending and Descending returned identical lists. Songs: Finamp sends SortBy=Runtime,AlbumArtist,Album,SortName. applySort takes the first *recognized* key, so Runtime was skipped and the list came back sorted by album artist while looking correct. Both repos already accept a duration sort (mediafile_repository maps it explicitly; album_repository falls through to the column name), so no migration is needed. Sorting 97k songs by duration costs a temp B-tree (~114ms on a prod-sized copy) — the same cost the Subsonic and UI duration sorts already pay, and correct where the previous behaviour was merely fast. * fix(jellyfin): apply the played/unplayed filters and MaxHeight image bound Filters was matched with a substring test for IsFavorite, so every other token Jellyfin defines was silently dropped and the response kept rows it should have excluded. Finamp sends Filters=IsUnplayed in normal use. Replace the bool with a parsed itemFilters carrying nullable favorite and played flags, so isFavorite=false and isPlayed=false are real filters rather than indistinguishable from an absent param. Standalone params are read first and the Filters list overrides them, the precedence real Jellyfin has. IsFavoriteOrLikes now maps to favorites deliberately instead of by substring accident; Likes, Dislikes, IsFolder, IsNotFolder and IsResumable have no Navidrome equivalent and are dropped rather than half-applied. The negative cases match NULL as well, since annotations are LEFT JOINed and an untouched item has no row. getItemImage read only maxwidth, so a client sending just MaxHeight got the full-size original: measured against a real cover, maxHeight=100 returned 82,570 bytes where maxWidth=100 returned 3,316. Use the tighter of the two bounds. * refactor(jellyfin): share the plain-param parser between /Items and /Artists listArtistsByRole hand-listed the itemsQuery fields it happened to need, which is exactly how the favorites filter went missing: the literal has been amended in four of the five commits that touched it. Extract listParams for the fields that come straight from query params so both paths read one parser, and the next supported param reaches every list path instead of only /Items. Also from the cleanup pass: collapse imageSize to a single clamped comparison and read its bounds through req.Params like the rest of the package, which drops the strconv import; build the artist and playlist filter lists with the flat append shape the album and song paths already use, instead of re-wrapping opts.Filters into a nested And per predicate; drop a nil guard in listPlaylists that no caller can reach, since both paths into queryItemsOfType build QueryOptions without Filters. applySort now logs when no SortBy key resolves at all — a miss inside a fallback list is normal, but none matching means a silently ignored sort, the failure mode that hid the Runtime bug. Its doc comment records why the remaining keys cannot simply be joined. Folds three duplicated test bodies into the tables that already parameterize them, and covers the artist-parent album branch, which reaches notMissing through filter.AlbumsByArtistID rather than the default branch. * docs(jellyfin): correct how applySort describes Jellyfin's SortBy semantics The comment claimed SortBy is a comma-separated fallback list. It is not: RequestHelpers.GetOrderBy (10.10) builds one (ItemSortBy, SortOrder) pair per key, so Jellyfin orders by every key in turn. Navidrome applies only the first recognized one, which is a real divergence — secondary keys never break ties — not the intended reading of the parameter. The assertion that the keys cannot be joined was also wrong. buildSortOrder does split its input on commas; what it maps is the whole string, so joining raw Jellyfin key names misses the mappings. Mapping each key first and joining the results would work, which makes multi-key sorting a real option rather than a blocked one. Documenting the current behaviour as a known divergence until then. * fix(jellyfin): order by every recognized SortBy key, not just the first Jellyfin orders by each SortBy key in turn, so "DatePlayed,SortName" means break ties by name. Navidrome applied only the first recognized key and dropped the rest, which is 28% of the sort traffic on a real server (23 of 82 requests in 12h carry 2-5 keys). Most were harmless because the primary key dominates, but PremiereDate,Album,ParentIndexNumber,IndexNumber,SortName came back unordered within a year. The keys cannot simply be joined: sortMapping keyed on the whole Sort string, so a joined value missed every mapping and fell through to raw column names. Make it resolve a comma list per part, but only when every part is a known key — the four existing callers that pass raw column lists (core/matcher, core/lyrics, core/maintenance, subsonic/browsing) all carry a part that is not a mapping key, several with their own direction, so they keep falling through exactly as before. Verified each one. applySort now collects every recognized key, skipping duplicates so ParentIndexNumber,IndexNumber does not repeat a column. random stays alone: the repo matches it by exact string equality, so joining it would both break that path and emit a bare 'random' column into the ORDER BY. Verified against a prod-sized copy: every multi-key combination seen in real traffic returns 200, and a secondary key now changes the order within a tied year for songs. Albums are unchanged there, because their max_year mapping already ended in ", name". * fix(persistence): resolve sort mappings exactly once Making sortMapping resolve a comma list per part broke an invariant it had been relying on: idempotence. sanitizeSort mapped the sort key up front and applyOptions then ran buildSortOrder over the result, so sortMapping was already being handed its own output. That was harmless only while a mapped value could never look like a key list. media_file's rated_at maps to "rating, rated_at", and both parts are keys, so the second pass expanded it to "rating, rating, rated_at". Found by round-tripping every mapping in all four repositories; it was the only collision, and the duplicate sort key was benign in SQL, but any future mapping of that shape would silently change meaning. sanitizeSort now validates without resolving, leaving buildSortOrder as the single mapping point. The generated SQL is unchanged — the whole suite passes apart from the two specs that asserted the old return value, which are updated and joined by a round-trip guard covering exactly the rated_at shape. Also use the paren-aware splitFunc that buildSortOrder already uses, so an expression carrying commas inside its parentheses cannot be split apart. * refactor(jellyfin,persistence): flatten the sort resolution paths Cleanup pass over the branch, no behavior change. sortMapping loses the len(parts)>1 guard, which existed only to pick between two identical toSnakeCase exits; the single-key case now falls through the same loop. lookupSortMapping hands back the snake_case form it had to derive so the fallback stops recomputing it — toSnakeCase is two regexps, and on a miss it was running twice per call. sanitizeSort now asks lookupSortMapping instead of probing the map itself, so "is this a known sort key" has one answer; the two had already drifted, since sanitizeSort tried one casing where the resolver tries three. applySort folds the nested random branch into the skip condition and the two trailing length tests into one switch. setSortMappings documents the invariant the comma-list rule depends on, where someone adding a mapping will read it. The README line describing SortBy still said only the first key applied, which the commit before last made false. Tests: the twelve near-identical sorting specs become one DescribeTable of (itemType, SortBy, want) triples, 124 lines to 36, and the applyOptions round-trip assertion collapses to the buildSortOrder call its sibling uses. * fix(jellyfin): keep annotation filters out of search, resolve sorts per part Two findings from the Codex review on #5981. The played/unplayed filters turned working requests into 500s when combined with SearchTerm. Search runs a two-phase FTS query whose first phase selects rowids with no annotation join, so a starred or play_count predicate there is "no such column", not a filter. Measured against master: MusicAlbum with SearchTerm and Filters=IsUnplayed went 200 -> 500, likewise IsPlayed and the Audio equivalents. listAlbums and listSongs now skip those predicates on the search path, matching what listArtists already did. That also clears the same 500 master already had for Filters=IsFavorite with SearchTerm. sortMapping resolved a comma list only while every part was a known key, so a list mixing a plain column with a mapped key kept neither: MusicAlbum SortBy=Runtime,SortName arrives as "duration, name", and duration is a plain album column, so name stayed raw instead of expanding to order_album_name. Albums whose name differs from its sort form — 1,366 of 6,987 on a real library — then ordered by the wrong secondary key, and PreferSortTags was ignored. Each part is now resolved on its own, which is what setSortMappings already documents for a single field. Verified every in-tree caller that passes a raw column list still produces its original ORDER BY. Codex also asked for the artist search path to apply the same filters. It would 500 for the reason above, and wrapping the library scope in a compound filter makes requestedLibraryIDs stop recognizing it, silently widening the search past the requested ParentId. * fix(jellyfin): honor the first SortOrder value for a multi-key sort applySort compared the whole SortOrder string with "Descending", so a per-key list like SortOrder=Descending,Ascending failed the match and every key, including the primary, sorted ascending — the exact opposite of the request. Take the first comma-separated value, which Jellyfin also uses for any key past the end of the SortOrder list. True per-key directions can't be expressed through the single opts.Sort string and are left out; no observed client sends a SortOrder list. --- persistence/sql_base_repository.go | 36 ++++- persistence/sql_base_repository_test.go | 59 +++++++- persistence/sql_restful.go | 12 +- server/jellyfin/README.md | 6 +- server/jellyfin/browsing.go | 19 +-- server/jellyfin/browsing_test.go | 23 +++ server/jellyfin/images.go | 15 +- server/jellyfin/images_test.go | 33 +++- server/jellyfin/items.go | 149 +++++++++++++------ server/jellyfin/items_test.go | 190 +++++++++++++++--------- 10 files changed, 394 insertions(+), 148 deletions(-) diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 428ba7a7b..c61bca1a6 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -97,6 +97,8 @@ func (r *sqlRepository) registerModel(instance any, filters map[string]filterFun } // setSortMappings sets the mappings for the sort fields. If the sort field is not in the map, it will be used as is. +// This applies per comma-separated part, so a key added here also defines that bare name wherever a +// caller uses it inside a sort list. // // If PreferSortTags is enabled, it will map the order fields to the corresponding sort expression, // which gives precedence to sort tags. @@ -147,17 +149,37 @@ func (r sqlRepository) applyOptions(sq SelectBuilder, options ...model.QueryOpti // TODO Change all sortMappings to have a consistent case func (r sqlRepository) sortMapping(sort string) string { - if mapping, ok := r.sortMappings[sort]; ok { + if mapping, _, ok := r.lookupSortMapping(sort); ok { return mapping } - if mapping, ok := r.sortMappings[toCamelCase(sort)]; ok { - return mapping + // Each part of a comma list is resolved on its own, so a mix of mapped keys and plain columns + // keeps the mappings the recognized parts have. + parts := strings.FieldsFunc(sort, splitFunc(',')) + mapped := make([]string, 0, len(parts)) + for _, part := range parts { + part = strings.TrimSpace(part) + if partMapping, _, ok := r.lookupSortMapping(part); ok { + part = partMapping + } else { + part = toSnakeCase(part) + } + mapped = append(mapped, part) } - sort = toSnakeCase(sort) - if mapping, ok := r.sortMappings[sort]; ok { - return mapping + return strings.Join(mapped, ", ") +} + +// lookupSortMapping also returns the snake_case form when it had to derive one, so a caller's +// fallback doesn't recompute it: toSnakeCase runs two regexps. +func (r sqlRepository) lookupSortMapping(sort string) (mapping, snakeCased string, ok bool) { + if mapping, ok = r.sortMappings[sort]; ok { + return mapping, sort, true } - return sort + if mapping, ok = r.sortMappings[toCamelCase(sort)]; ok { + return mapping, "", true + } + snakeCased = toSnakeCase(sort) + mapping, ok = r.sortMappings[snakeCased] + return mapping, snakeCased, ok } func (r sqlRepository) buildSortOrder(sort, order string) string { diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 9c6c6007f..0f76eb6ab 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -92,14 +92,28 @@ var _ = Describe("sqlRepository", func() { Expect(sort).To(BeEmpty()) }) - It("returns the mapped value when sort key exists", func() { + // Validation only: buildSortOrder resolves the mapping, so mapping here too would hand + // sortMapping its own output and re-map values whose parts are themselves keys. + It("accepts a known sort key without resolving it", func() { sort, _ := r.sanitizeSort("sort1", "") - Expect(sort).To(Equal("mappedSort1")) + Expect(sort).To(Equal("sort1")) }) It("is case insensitive", func() { sort, _ := r.sanitizeSort("Sort1", "") - Expect(sort).To(Equal("mappedSort1")) + Expect(sort).To(Equal("sort1")) + }) + + It("still resolves the mapping by the time the SQL is built", func() { + Expect(r.buildSortOrder("sort1", "asc")).To(Equal("mappedSort1 asc")) + }) + + // A mapping whose parts are themselves keys (media_file rated_at = "rating, rated_at") + // must survive the round trip through sanitizeSort and buildSortOrder unduplicated. + It("does not re-map a value whose parts are also keys", func() { + r.sortMappings = map[string]string{"rating": "rating", "rated_at": "rating, rated_at"} + sort, _ := r.sanitizeSort("rated_at", "") + Expect(r.buildSortOrder(sort, "asc")).To(Equal("rating asc, rated_at asc")) }) It("returns the field if it is a valid field", func() { @@ -135,6 +149,45 @@ var _ = Describe("sqlRepository", func() { }) }) + Describe("sortMapping", func() { + BeforeEach(func() { + r.sortMappings = map[string]string{ + "name": "order_album_name, order_album_artist_name", + "recently_added": "album.created_at, album.id", + } + }) + It("maps a single key", func() { + Expect(r.sortMapping("recently_added")).To(Equal("album.created_at, album.id")) + }) + It("maps every part of a comma list when all of them are known keys", func() { + Expect(r.sortMapping("recently_added, name")). + To(Equal("album.created_at, album.id, order_album_name, order_album_artist_name")) + }) + It("resolves the known parts of a mixed list and leaves the rest as columns", func() { + Expect(r.sortMapping("recently_added, play_count")). + To(Equal("album.created_at, album.id, play_count")) + }) + // Jellyfin's MusicAlbum SortBy=Runtime,SortName arrives as "duration, name"; duration is a + // plain album column while name is mapped, and the mapping must survive the mix. + It("keeps a mapping when an earlier part is a plain column", func() { + Expect(r.sortMapping("duration, name")). + To(Equal("duration, order_album_name, order_album_artist_name")) + }) + It("leaves a raw column list with directions untouched", func() { + Expect(r.sortMapping("starred desc, rating desc")).To(Equal("starred desc, rating desc")) + }) + It("does not split an expression on a comma inside its parentheses", func() { + Expect(r.sortMapping("coalesce(name, ''), title")).To(Equal("coalesce(name, ''), title")) + Expect(r.sortMapping("coalesce(nullif(a,''), b) desc, c")).To(Equal("coalesce(nullif(a,''), b) desc, c")) + }) + It("keeps a mapping whose value nests commas inside parentheses", func() { + r.sortMappings["max_year"] = "coalesce(nullif(original_date,''), cast(max_year as text)), release_date" + Expect(r.sortMapping("max_year, name")).To(Equal( + "coalesce(nullif(original_date,''), cast(max_year as text)), release_date, " + + "order_album_name, order_album_artist_name")) + }) + }) + Describe("buildSortOrder", func() { BeforeEach(func() { r.sortMappings = map[string]string{} diff --git a/persistence/sql_restful.go b/persistence/sql_restful.go index 1dcabcec6..b1cfd2379 100644 --- a/persistence/sql_restful.go +++ b/persistence/sql_restful.go @@ -69,13 +69,11 @@ func (r *sqlRepository) parseRestOptions(ctx context.Context, options ...rest.Qu func (r sqlRepository) sanitizeSort(sort, order string) (string, string) { if sort != "" { sort = toSnakeCase(sort) - if mapped, ok := r.sortMappings[sort]; ok { - sort = mapped - } else { - if !r.isFieldWhiteListed(sort) { - log.Warn(r.ctx, "Ignoring sort not whitelisted", "sort", sort, "table", r.tableName) - sort = "" - } + // Validate only: buildSortOrder resolves the mapping later, and mapping here as well would + // feed sortMapping its own output. + if _, _, known := r.lookupSortMapping(sort); !known && !r.isFieldWhiteListed(sort) { + log.Warn(r.ctx, "Ignoring sort not whitelisted", "sort", sort, "table", r.tableName) + sort = "" } } if order != "" { diff --git a/server/jellyfin/README.md b/server/jellyfin/README.md index 5759e6cc0..dc3219dfa 100644 --- a/server/jellyfin/README.md +++ b/server/jellyfin/README.md @@ -104,7 +104,11 @@ album's tracks — Feishin fetches them this way instead of `ParentId`); `GenreI genre's albums or tracks — Finamp's genre screen sends it the same way; `/Artists/AlbumArtists` and `MusicArtist` queries accept it too, matching artists credited on an album of that genre); `SearchTerm`; -favorites-only (`Filters=IsFavorite` or the standalone `isFavorite=true`); `SortBy`/`SortOrder`; +`Filters` (`IsFavorite`, `IsFavoriteOrLikes`, `IsPlayed`, `IsUnplayed`) and the standalone +`isFavorite`/`isPlayed` booleans it can also be expressed as — `Filters` wins when both are sent, as +in Jellyfin; `Likes`, `Dislikes`, `IsFolder`, `IsNotFolder` and `IsResumable` have no Navidrome +equivalent and are ignored; `SortBy`/`SortOrder` (every recognized key is applied in order, so secondary keys break ties; +unrecognized keys are skipped, and `Random` always sorts alone); `StartIndex`/`Limit`; and `Ids` (batch fetch by id). `Recursive=false` with a library `ParentId` returns direct children only (no tracks — no track is a library's direct child). diff --git a/server/jellyfin/browsing.go b/server/jellyfin/browsing.go index 9acd8d5a8..25c44e748 100644 --- a/server/jellyfin/browsing.go +++ b/server/jellyfin/browsing.go @@ -24,10 +24,6 @@ func (api *Router) getAlbumArtists(w http.ResponseWriter, r *http.Request) { // when accessible (like queryItems) or all accessible libraries otherwise. func (api *Router) listArtistsByRole(w http.ResponseWriter, r *http.Request, role model.Role) { ctx := r.Context() - p := req.Params(r) - opts := model.QueryOptions{Offset: p.IntOr("startindex", 0), Max: p.IntOr("limit", 0)} - applySort(&opts, "MusicArtist", p.StringOr("sortby", ""), p.StringOr("sortorder", "")) - scopeIDs, _, ok := parentIDScope(ctx, r) if !ok { http.Error(w, "Not Found", http.StatusNotFound) @@ -38,14 +34,13 @@ func (api *Router) listArtistsByRole(w http.ResponseWriter, r *http.Request, rol http.Error(w, "Not Found", http.StatusNotFound) return } - // Only the fields listArtists reads; /Artists has no favorites filter, so favOnly stays false. - // Finamp's artist tab sends GenreIds when a genre filter is active. - q := itemsQuery{ - scopeIDs: scopeIDs, - genreIds: genreIds, - search: searchTerm(p), - fields: dto.ParseFields(p.Strings("fields")...), - } + // This route resolves its own scope, so it shares only the plain query params with /Items. + q := listParams(req.Params(r)) + q.scopeIDs = scopeIDs + q.genreIds = genreIds + + opts := model.QueryOptions{Offset: q.offset, Max: q.limit} + applySort(&opts, "MusicArtist", q.sortBy, q.sortOrder) if q.search != "" { opts.Max = clampLimit(opts.Max, defaultSearchLimit, maxSearchLimit) } diff --git a/server/jellyfin/browsing_test.go b/server/jellyfin/browsing_test.go index 2825c03e8..657c7a42c 100644 --- a/server/jellyfin/browsing_test.go +++ b/server/jellyfin/browsing_test.go @@ -156,6 +156,29 @@ var _ = Describe("Browsing", func() { Expect(sql).NotTo(ContainSubstring("library_artist.library_id")) }) + DescribeTable("restricts to favorites", + func(url string, handler func(*Router) http.HandlerFunc) { + artistRepo := ds.Artist(context.Background()).(*tests.MockArtistRepo) + artistRepo.SetData(model.Artists{{ID: testID("ar1"), Name: "Artist"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", url, nil).WithContext(ctxUser(model.Libraries{{ID: 1}})) + invoke(handler(api), w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + sql, args, err := artistRepo.Options.Filters.ToSql() + Expect(err).NotTo(HaveOccurred()) + Expect(sql).To(ContainSubstring("starred")) + // listArtists always ANDs notMissing, favorites filter or not. + Expect(sql).To(ContainSubstring("missing")) + Expect(args).To(ContainElement(true)) + }, + Entry("Filters=IsFavorite", "/Artists?Filters=IsFavorite", + func(a *Router) http.HandlerFunc { return a.getArtists }), + Entry("isFavorite=true", "/Artists?isFavorite=true", + func(a *Router) http.HandlerFunc { return a.getArtists }), + Entry("on /Artists/AlbumArtists", "/Artists/AlbumArtists?Filters=IsFavorite", + func(a *Router) http.HandlerFunc { return a.getAlbumArtists }), + ) + It("404s a malformed ParentId instead of listing every library's artists", func() { artistRepo := ds.Artist(context.Background()).(*tests.MockArtistRepo) artistRepo.SetData(model.Artists{{ID: testID("ar1"), Name: "Artist"}}) diff --git a/server/jellyfin/images.go b/server/jellyfin/images.go index 65f02d7a0..fcf69c039 100644 --- a/server/jellyfin/images.go +++ b/server/jellyfin/images.go @@ -11,7 +11,6 @@ import ( _ "image/png" "io" "net/http" - "strconv" "github.com/dustin/go-humanize" "github.com/navidrome/navidrome/conf" @@ -20,9 +19,20 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/imghttp" + "github.com/navidrome/navidrome/utils/req" _ "golang.org/x/image/webp" ) +// imageSize picks the tighter of Jellyfin's two bounds, because Navidrome resizes on a single +// dimension: reading only MaxWidth serves the full-size original to a client that sent MaxHeight. +func imageSize(maxWidth, maxHeight int) int { + w, h := max(maxWidth, 0), max(maxHeight, 0) + if w == 0 || h == 0 { + return max(w, h) + } + return min(w, h) +} + func (api *Router) getItemImage(w http.ResponseWriter, r *http.Request) { // Public endpoint, like real Jellyfin's image routes: clients fetch cover URLs without credentials // and item ids are unguessable, so resolution runs elevated to bypass the visibility filter. @@ -31,7 +41,8 @@ func (api *Router) getItemImage(w http.ResponseWriter, r *http.Request) { if !ok { return } - size, _ := strconv.Atoi(r.URL.Query().Get("maxwidth")) + p := req.Params(r) + size := imageSize(p.IntOr("maxwidth", 0), p.IntOr("maxheight", 0)) artID := api.resolveArtworkID(ctx, itemId) img, err := api.artwork.GetOrPlaceholder(ctx, artID, size, false) diff --git a/server/jellyfin/images_test.go b/server/jellyfin/images_test.go index 0e0d6220d..2f32e6ec9 100644 --- a/server/jellyfin/images_test.go +++ b/server/jellyfin/images_test.go @@ -30,14 +30,16 @@ import ( type fakeArtwork struct { artwork.Artwork - recvId string - recvCtx context.Context - data []byte - hash string + recvId string + recvSize int + recvCtx context.Context + data []byte + hash string } func (f *fakeArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*artwork.Image, error) { f.recvId = id + f.recvSize = size f.recvCtx = ctx data := f.data if data == nil { @@ -61,6 +63,29 @@ func newImageRequest(itemId string) (*httptest.ResponseRecorder, *http.Request) } var _ = Describe("Images", func() { + // Real Jellyfin fits the image inside either bound, so a client that sends only MaxHeight must + // still get a resized image rather than the full-size original. + DescribeTable("derives the requested size from MaxWidth or MaxHeight", + func(query string, wantSize int) { + ds := &tests.MockDataStore{} + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) + fa := &fakeArtwork{} + api := &Router{ds: ds, artwork: fa} + + w, r := newImageRequest(dto.EncodeID(testID("a1"))) + r.URL.RawQuery = query + api.getItemImage(w, r) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(fa.recvSize).To(Equal(wantSize)) + }, + Entry("MaxWidth only", "maxwidth=300", 300), + Entry("MaxHeight only", "maxheight=300", 300), + Entry("both, smaller bound wins", "maxwidth=200&maxheight=300", 200), + Entry("both, smaller bound wins regardless of order", "maxwidth=300&maxheight=200", 200), + Entry("neither", "", 0), + ) + It("streams album artwork", func() { ds := &tests.MockDataStore{} ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) diff --git a/server/jellyfin/items.go b/server/jellyfin/items.go index de0da88eb..eb5cd2a1a 100644 --- a/server/jellyfin/items.go +++ b/server/jellyfin/items.go @@ -31,6 +31,51 @@ func searchTerm(p *req.Values) string { return strings.TrimSpace(p.StringOr("searchterm", "")) } +// itemFilters is the parsed Filters=... list together with the standalone isFavorite/isPlayed params +// clients may send instead. A nil field means the client asked for no filtering on that dimension. +type itemFilters struct { + favorite *bool + played *bool +} + +// parseItemFilters reads the standalone params first and lets the Filters list win, matching real +// Jellyfin. Tokens with no Navidrome equivalent (Likes, IsFolder, IsResumable) are dropped. +func parseItemFilters(p *req.Values) itemFilters { + f := itemFilters{favorite: p.BoolPtr("isfavorite"), played: p.BoolPtr("isplayed")} + for token := range strings.SplitSeq(p.StringOr("filters", ""), ",") { + switch strings.TrimSpace(token) { + case "IsFavorite", "IsFavoriteOrLikes": + f.favorite = new(true) + case "IsPlayed": + f.played = new(true) + case "IsUnplayed": + f.played = new(false) + } + } + return f +} + +// predicates renders the filters as annotation-column conditions. The negative cases have to match +// NULL as well: annotations are LEFT JOINed, so an item nobody has touched has no row at all. +func (f itemFilters) predicates() []squirrel.Sqlizer { + var out []squirrel.Sqlizer + if f.favorite != nil { + if *f.favorite { + out = append(out, squirrel.Eq{"starred": true}) + } else { + out = append(out, squirrel.Or{squirrel.Eq{"starred": nil}, squirrel.Eq{"starred": false}}) + } + } + if f.played != nil { + if *f.played { + out = append(out, squirrel.Gt{"play_count": 0}) + } else { + out = append(out, squirrel.Or{squirrel.Eq{"play_count": nil}, squirrel.Eq{"play_count": 0}}) + } + } + return out +} + func (api *Router) getItems(w http.ResponseWriter, r *http.Request) { res, err := api.queryItems(r.Context(), r) if err != nil { @@ -214,7 +259,7 @@ type itemsQuery struct { sortOrder string offset int limit int - favOnly bool + filters itemFilters // parentId scopes the query. entityParent is the same id only when it names an entity (an artist // for MusicAlbum, an album for Audio) rather than a library. parentId string @@ -231,6 +276,19 @@ type itemsQuery struct { studioIds []string } +// listParams reads the itemsQuery fields that come straight from query params. +func listParams(p *req.Values) itemsQuery { + return itemsQuery{ + fields: dto.ParseFields(p.Strings("fields")...), + search: searchTerm(p), + sortBy: p.StringOr("sortby", ""), + sortOrder: p.StringOr("sortorder", ""), + offset: p.IntOr("startindex", 0), + limit: p.IntOr("limit", 0), + filters: parseItemFilters(p), + } +} + // 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). A non-empty id param that @@ -261,24 +319,14 @@ func (api *Router) parseItemsQuery(ctx context.Context, r *http.Request) (itemsQ if !ok { return itemsQuery{}, model.ErrNotFound } - q := itemsQuery{ - fields: dto.ParseFields(p.Strings("fields")...), - ids: ids, - rawTypes: p.StringOr("includeitemtypes", ""), - search: searchTerm(p), - sortBy: p.StringOr("sortby", ""), - sortOrder: p.StringOr("sortorder", ""), - offset: p.IntOr("startindex", 0), - limit: p.IntOr("limit", 0), - // Clients express "favorites only" two ways: Filters=IsFavorite and the standalone - // isFavorite=true param (Finamp's "Favourite tracks" widget uses the latter). - favOnly: strings.Contains(p.StringOr("filters", ""), "IsFavorite") || p.BoolOr("isfavorite", false), - parentId: parentId, - genreIds: genreIds, - albumIds: albumIds, - years: parseYears(r), - studioIds: studioIds, - } + q := listParams(p) + q.ids = ids + q.rawTypes = p.StringOr("includeitemtypes", "") + q.parentId = parentId + q.genreIds = genreIds + q.albumIds = albumIds + q.years = parseYears(r) + q.studioIds = studioIds // An artist's page filters by artist, not ParentId: Finamp sends ParentId= for scoping // plus AlbumArtistIds/ArtistIds/contributingArtistIds for the artist. albumArtistScope := firstNonEmpty(p.StringOr("albumartistids", ""), p.StringOr("artistids", "")) @@ -621,8 +669,10 @@ func (api *Router) listAlbums(ctx context.Context, opts model.QueryOptions, q it if len(q.studioIds) > 0 { filters = append(filters, filter.ByStudioID(q.studioIds)) } - if q.favOnly { - filters = append(filters, filter.ByStarred().Filters) + // Not on the search path: its first FTS phase selects rowids with no annotation join, so a + // starred/play_count predicate there is "no such column" rather than a filter. + if q.search == "" { + filters = append(filters, q.filters.predicates()...) } opts.Filters = filters opts = filter.ApplyLibraryFilter(opts, q.scopeIDs) @@ -668,8 +718,10 @@ func (api *Router) listSongs(ctx context.Context, opts model.QueryOptions, q ite if len(q.studioIds) > 0 { filters = append(filters, filter.ByStudioID(q.studioIds)) } - if q.favOnly { - filters = append(filters, filter.ByStarred().Filters) + // Not on the search path: its first FTS phase selects rowids with no annotation join, so a + // starred/play_count predicate there is "no such column" rather than a filter. + if q.search == "" { + filters = append(filters, q.filters.predicates()...) } opts.Filters = filters opts = filter.ApplyLibraryFilter(opts, q.scopeIDs) @@ -720,14 +772,12 @@ func (api *Router) listArtists(ctx context.Context, opts model.QueryOptions, q i return materialized(result(slice.Map(artists, toItem), total, opts.Offset)), nil } - if q.favOnly { - opts.Filters = filter.ArtistsByStarred().Filters - } else { - opts.Filters = notMissing - } + filters := squirrel.And{notMissing} + filters = append(filters, q.filters.predicates()...) if len(q.genreIds) > 0 { - opts.Filters = squirrel.And{opts.Filters, filter.ArtistsByGenreID(q.genreIds)} + filters = append(filters, filter.ArtistsByGenreID(q.genreIds)) } + opts.Filters = filters opts = filter.ArtistsByRole(opts, role) opts = filter.ApplyArtistLibraryFilter(opts, q.scopeIDs) total, _ := repo.CountAll(model.QueryOptions{Filters: opts.Filters}) @@ -752,13 +802,8 @@ func (api *Router) listGenres(ctx context.Context, opts model.QueryOptions) (ite // listPlaylists lists playlists visible to the current user. Visibility (public or owned) is // enforced by playlistRepository, not scopeIDs. func (api *Router) listPlaylists(ctx context.Context, opts model.QueryOptions, q itemsQuery) (itemsResult, error) { - if q.favOnly { - starred := squirrel.Eq{"starred": true} - if opts.Filters == nil { - opts.Filters = starred - } else { - opts.Filters = squirrel.And{opts.Filters, starred} - } + if preds := q.filters.predicates(); len(preds) > 0 { + opts.Filters = squirrel.And(preds) } repo := api.ds.Playlist(ctx) total, err := repo.CountAll(model.QueryOptions{Filters: opts.Filters}) @@ -908,18 +953,32 @@ func result(items []dto.BaseItemDto, total, start int) dto.QueryResult { return dto.QueryResult{Items: items, TotalRecordCount: total, StartIndex: start} } -// applySort translates Jellyfin's SortBy/SortOrder into a valid model.QueryOptions sort key for the -// item type. Clients send SortBy as a comma-separated fallback list (e.g. "DateCreated,SortName"); -// this uses the first recognized key. An unrecognized SortBy is left untouched (the repo's default), -// not passed through raw where it could produce an invalid ORDER BY. +// applySort keeps every recognized SortBy key, so secondary keys break ties as Jellyfin intends. +// Unrecognized keys are skipped, not passed through raw where they could make an invalid ORDER BY. func applySort(opts *model.QueryOptions, itemType, sortBy, order string) { + var cols []string for key := range strings.SplitSeq(sortBy, ",") { - if col, ok := sortColumn(itemType, strings.TrimSpace(key)); ok { - opts.Sort = col + col, ok := sortColumn(itemType, strings.TrimSpace(key)) + // The repo matches random by exact string equality, so it can only ever sort alone. + if !ok || slices.Contains(cols, col) || (col == "random" && len(cols) > 0) { + continue + } + cols = append(cols, col) + if col == "random" { break } } - if strings.EqualFold(order, "Descending") { + switch { + case len(cols) > 0: + opts.Sort = strings.Join(cols, ", ") + case sortBy != "": + log.Debug("Jellyfin API: no usable SortBy key, falling back to the default order", + "itemType", itemType, "sortBy", sortBy) + } + // Jellyfin allows a per-key SortOrder list, which one Order can't express; honor the first value + // for every key, as Jellyfin does for keys past the end of the list. + first, _, _ := strings.Cut(order, ",") + if strings.EqualFold(first, "Descending") { opts.Order = "desc" } } @@ -941,6 +1000,8 @@ var sortColumnsByType = map[string]map[string]string{ "dateplayed": "play_date", "communityrating": "rating", "random": "random", + "runtime": "duration", + "runtimeticks": "duration", // Finamp's "Latest Releases" sorts by PremiereDate; "year" matches songs' ProductionYear. "premieredate": "year", "productionyear": "year", @@ -964,6 +1025,8 @@ var sortColumnsByType = map[string]map[string]string{ "playcount": "play_count", "dateplayed": "play_date", "communityrating": "rating", + "runtime": "duration", + "runtimeticks": "duration", "premieredate": "max_year", "productionyear": "max_year", }, "MusicGenre": { diff --git a/server/jellyfin/items_test.go b/server/jellyfin/items_test.go index b227ab719..17180c36a 100644 --- a/server/jellyfin/items_test.go +++ b/server/jellyfin/items_test.go @@ -327,17 +327,75 @@ var _ = Describe("Items", func() { Expect(albumRepo.Options.Max).To(Equal(3)) }) - It("applies a starred filter when Filters=IsFavorite", func() { - albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) - albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&Filters=IsFavorite", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - sql, _, err := albumRepo.Options.Filters.ToSql() - Expect(err).NotTo(HaveOccurred()) - Expect(sql).To(ContainSubstring("starred")) - }) + DescribeTable("translates the Filters list and its standalone equivalents", + func(query string, wantSQL, notWantSQL []string) { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&"+query, nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + sql, _, err := albumRepo.Options.Filters.ToSql() + Expect(err).NotTo(HaveOccurred()) + for _, want := range wantSQL { + Expect(sql).To(ContainSubstring(want)) + } + for _, not := range notWantSQL { + Expect(sql).NotTo(ContainSubstring(not)) + } + }, + Entry("IsFavorite", "Filters=IsFavorite", []string{"starred"}, nil), + Entry("IsFavorite,IsUnplayed combined", "Filters=IsFavorite,IsUnplayed", + []string{"starred", "play_count"}, nil), + Entry("IsUnplayed", "Filters=IsUnplayed", []string{"play_count"}, []string{"starred"}), + Entry("IsPlayed", "Filters=IsPlayed", []string{"play_count"}, []string{"starred"}), + Entry("IsFavoriteOrLikes is treated as favorites", "Filters=IsFavoriteOrLikes", []string{"starred"}, nil), + Entry("isPlayed=false", "isPlayed=false", []string{"play_count"}, nil), + Entry("isFavorite=false still filters", "isFavorite=false", []string{"starred"}, nil), + // Jellyfin builds the query from the standalone params, then applies Filters over the top. + Entry("Filters wins over the standalone param", "isFavorite=false&Filters=IsFavorite", + []string{"starred = "}, nil), + // No Navidrome equivalent: these must be dropped, not half-applied. + Entry("Likes is ignored", "Filters=Likes", nil, []string{"starred", "play_count"}), + Entry("IsResumable is ignored", "Filters=IsResumable", nil, []string{"starred", "play_count"}), + // The artist-parent branch gets notMissing from filter.AlbumsByArtistID, not the default + // branch, so favorites must not be the only predicate left on it. + Entry("keeps missing excluded under an artist parent", + "Filters=IsFavorite&ArtistIds="+dto.EncodeID(testID("ar1")), + []string{"starred", "missing"}, nil), + ) + + // Search runs a two-phase FTS query whose first phase has no annotation join, so an + // annotation predicate there is "no such column: starred" -> 500. + DescribeTable("does not push annotation filters into a search", + func(itemType, filters string) { + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", + "/Items?IncludeItemTypes="+itemType+"&SearchTerm=one&Filters="+filters, nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var opts model.QueryOptions + if itemType == "MusicAlbum" { + opts = ds.Album(context.Background()).(*tests.MockAlbumRepo).Options + } else { + opts = ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).Options + } + if opts.Filters == nil { + return + } + sql, _, err := opts.Filters.ToSql() + Expect(err).NotTo(HaveOccurred()) + Expect(sql).NotTo(ContainSubstring("starred")) + Expect(sql).NotTo(ContainSubstring("play_count")) + }, + Entry("albums, IsFavorite", "MusicAlbum", "IsFavorite"), + Entry("albums, IsUnplayed", "MusicAlbum", "IsUnplayed"), + Entry("albums, IsPlayed", "MusicAlbum", "IsPlayed"), + Entry("songs, IsFavorite", "Audio", "IsFavorite"), + Entry("songs, IsUnplayed", "Audio", "IsUnplayed"), + ) It("forwards SearchTerm to the repo's Search method", func() { albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) @@ -601,65 +659,59 @@ var _ = Describe("Items", func() { }) Describe("sorting", func() { - It("maps SortBy=PlayCount to the play_count column", func() { - albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) - albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SortBy=PlayCount", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(albumRepo.Options.Sort).To(Equal("play_count")) - }) + DescribeTable("translates SortBy into the repo's sort keys", + func(itemType, sortBy, want string) { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) + mfRepo := ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo) + mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes="+itemType+"&SortBy="+sortBy, nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + got := mfRepo.Options.Sort + if itemType == "MusicAlbum" { + got = albumRepo.Options.Sort + } + Expect(got).To(Equal(want)) + }, + Entry("PlayCount", "MusicAlbum", "PlayCount", "play_count"), + Entry("DatePlayed", "Audio", "DatePlayed", "play_date"), + Entry("Runtime on albums", "MusicAlbum", "Runtime", "duration"), + Entry("RunTimeTicks alias", "MusicAlbum", "RunTimeTicks", "duration"), + // Finamp leads its track sort with Runtime: unless that resolves, the first recognized + // key is AlbumArtist and the list looks sorted while being sorted by the wrong thing. + Entry("Finamp's Runtime-led track sort", "Audio", "Runtime,AlbumArtist,Album,SortName", + "duration, album_artist, album, title"), + Entry("every recognized key, in order", "MusicAlbum", "DateCreated,SortName", "recently_added, name"), + Entry("a key repeating a column is dropped", "Audio", + "PremiereDate,Album,ParentIndexNumber,IndexNumber,SortName", "year, album, title"), + // random is matched by exact string equality in the repo, so it can never share a sort. + Entry("Random stays alone", "MusicAlbum", "Random,SortName", "random"), + Entry("unrecognized keys are skipped", "Audio", "Runtime,Nonsense,SortName", "duration, title"), + Entry("only the last key recognized", "Audio", "Unknown1,Unknown2,SortName", "title"), + Entry("Finamp's album view is disc+track", "Audio", "ParentIndexNumber,IndexNumber,SortName", "album, title"), + Entry("nothing recognized leaves the repo default", "MusicAlbum", "SeriesSortName", ""), + ) - It("maps SortBy=DatePlayed to the play_date column", func() { - mfRepo := ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo) - mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio&SortBy=DatePlayed", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Options.Sort).To(Equal("play_date")) - }) - - It("uses the first recognized key in a comma-separated SortBy list", func() { - albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) - albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SortBy=DateCreated,SortName", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(albumRepo.Options.Sort).To(Equal("recently_added")) - }) - - It("skips unrecognized keys in a comma-separated SortBy list to find one that is", func() { - mfRepo := ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo) - mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio&SortBy=Unknown1,Unknown2,SortName", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Options.Sort).To(Equal("title")) - }) - - It("maps Finamp's album view SortBy (ParentIndexNumber,IndexNumber) to disc+track order", func() { - mfRepo := ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo) - mfRepo.SetData(model.MediaFiles{{ID: testID("s1"), Title: "Song"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio&SortBy=ParentIndexNumber,IndexNumber,SortName", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(mfRepo.Options.Sort).To(Equal("album")) - }) - - It("leaves Sort at the repo default when no SortBy key is recognized", func() { - albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) - albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SortBy=SeriesSortName", nil).WithContext(ctxUser()) - invoke(api.getItems, w, r) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(albumRepo.Options.Sort).To(Equal("")) - }) + // Jellyfin allows a per-key SortOrder list; we cannot express that through one Order, so + // we honor the first value for all keys, matching Jellyfin's fallback for extra keys. + DescribeTable("reads the first SortOrder value for the whole sort", + func(sortOrder, want string) { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: testID("a1"), Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", + "/Items?IncludeItemTypes=MusicAlbum&SortBy=Runtime,SortName&SortOrder="+sortOrder, nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(albumRepo.Options.Order).To(Equal(want)) + }, + Entry("ascending", "Ascending", ""), + Entry("descending", "Descending", "desc"), + Entry("descending leading a list", "Descending,Ascending", "desc"), + Entry("ascending leading a list", "Ascending,Descending", ""), + ) }) Describe("library scoping", func() { From 1f3034f0229777c2023e861a4d9a92aaca1a1207 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 19 Aug 2026 08:47:53 -0400 Subject: [PATCH 2/9] fix(playlist): block track edits on synced playlists across all APIs (#5984) * fix(playlist): block track edits on synced playlists across all APIs A synced playlist's tracks come from its source file, so any track edit made through the UI or an API was silently reverted on the next scan. Track mutations funnel through two service guards, checkTracksEditable (incremental edits) and Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's replace path), which each duplicated the smart-playlist check. Both now consult a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or Subsonic error 50) instead of being accepted and lost. Metadata-only edits (name, comment, public, the sync flag itself) still go through checkWritable and are unaffected. In the UI, a synced playlist's track list becomes read-only, mirroring how smart playlists already behave. * fix(playlist): return 409 Conflict for non-editable playlist track edits The previous commit rejected track edits on smart and synced playlists with ErrNotAuthorized (403). That conflates two different things: a 403 says the caller lacks permission, but a synced or smart playlist's tracks are immutable for everyone, including the owner and admins. It is a property of the resource, not the caller. Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error 50, the closest code it has (it has no read-only concept). The Native track handlers previously mapped this rejection inconsistently (400 on add, 500 on remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine authorization failures (non-owner, non-admin) still return ErrNotAuthorized. * fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic Follow-up to the track-edit lock: the read-only state was enforced but not advertised consistently, so clients still offered edits that the server rejects. - UI: the Add to Playlist picker filtered targets by isWritable only, offering synced playlists that then 409 on add. It now filters with canChangeTracks. - Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a locked playlist reported "not found" instead of 409. They now return 409 for ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks nothing). - OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned synced playlists advertised readonly=false. Readonly now also covers !TracksEditable(), matching the existing smart-playlist treatment. * fix(jellyfin): report CanEdit from playlist editability in permission probes getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so Finamp (which probes this before showing edit controls) offered track editing on synced/smart playlists whose add/remove requests now return 409. Both handlers now fetch the playlist and set CanEdit from TracksEditable(), keeping the deliberate non-owner looseness (CanEdit stays true for a normal playlist a non-owner views) and mapping any lookup error to 404 like the sibling probes. * fix(playlist): check ownership before editability when replacing tracks Create checked TracksEditable() before ownership, so a non-owner replacing another user's public smart/synced playlist (Jellyfin updatePlaylist with a non-empty Ids list) received a 409 read-only conflict instead of a 403 authorization failure. The incremental guards check ownership first via checkWritable; Create now matches that order. Subsonic is unaffected (both errors map to code 50). Owners of their own smart/synced playlists still get the read-only conflict. * fix(jellyfin): return 403 for locked playlists, matching Jellyfin Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403): PlaylistsController gates every mutation on OwnerUserId == caller or a share with CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is an ACL field, not a read-only marker, and Jellyfin core has no server-managed playlist type at all. Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps to 403 there instead of 409. The native API keeps 409 (a resource-state conflict is the accurate REST answer where we define the contract) and Subsonic keeps error 50, its closest code. * chore(playlist): trim comments added by this branch Several comments ran to three or four lines and carried rationale that belongs in the commit history rather than the code: what Jellyfin does with its own file-backed playlists, and restatements of the expressions directly below them. Each block is now one or two lines covering only the non-obvious why. --- core/playlists/playlists.go | 13 +++-- core/playlists/playlists_test.go | 46 +++++++++++++-- model/errors.go | 3 + model/playlist.go | 5 ++ model/playlist_test.go | 15 +++++ server/jellyfin/playlists.go | 49 ++++++++++++---- server/jellyfin/playlists_test.go | 63 ++++++++++++++++++++- server/nativeapi/playlists.go | 30 ++++++---- server/nativeapi/playlists_test.go | 14 +++++ server/subsonic/api.go | 3 +- server/subsonic/playlists.go | 2 +- server/subsonic/playlists_test.go | 9 +++ ui/src/common/playlistUtils.js | 2 +- ui/src/common/playlistUtils.test.js | 6 ++ ui/src/dialogs/SelectPlaylistInput.jsx | 5 +- ui/src/dialogs/SelectPlaylistInput.test.jsx | 9 +++ 16 files changed, 232 insertions(+), 42 deletions(-) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index 656bde05e..9ae8f09cb 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -130,12 +130,13 @@ func (s *playlists) Create(ctx context.Context, playlistId string, name string, if err != nil { return err } - if pls.IsSmartPlaylist() { - return model.ErrNotAuthorized - } + // Ownership first: a non-owner must get ErrNotAuthorized, not a read-only conflict. if !usr.IsAdmin && pls.OwnerID != usr.ID { return model.ErrNotAuthorized } + if !pls.TracksEditable() { + return model.ErrPlaylistNotEditable + } } else { pls = &model.Playlist{Name: name} pls.OwnerID = usr.ID @@ -230,14 +231,14 @@ func (s *playlists) checkWritable(ctx context.Context, id string) (*model.Playli return pls, nil } -// checkTracksEditable verifies the user can modify tracks (ownership + not smart playlist). +// checkTracksEditable verifies the user owns the playlist and its tracks are editable. func (s *playlists) checkTracksEditable(ctx context.Context, playlistID string) (*model.Playlist, error) { pls, err := s.checkWritable(ctx, playlistID) if err != nil { return nil, err } - if pls.IsSmartPlaylist() { - return nil, model.ErrNotAuthorized + if !pls.TracksEditable() { + return nil, model.ErrPlaylistNotEditable } return pls, nil } diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go index e8134b2ef..dd182b3f1 100644 --- a/core/playlists/playlists_test.go +++ b/core/playlists/playlists_test.go @@ -102,6 +102,8 @@ var _ = Describe("Playlists", func() { "pls-2": {ID: "pls-2", Name: "Other's", OwnerID: "other-user"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-synced": {ID: "pls-synced", Name: "Synced", OwnerID: "user-1", Sync: true}, + "pls-synced-other": {ID: "pls-synced-other", Name: "Other's Synced", OwnerID: "other-user", Sync: true, Public: true}, } ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds)) }) @@ -145,6 +147,18 @@ var _ = Describe("Playlists", func() { It("denies replacing tracks on a smart playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) _, err := ps.Create(ctx, "pls-smart", "", []string{"song-1"}) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) + }) + + It("denies replacing tracks on a synced playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + _, err := ps.Create(ctx, "pls-synced", "", []string{"song-1"}) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) + }) + + It("denies a non-owner with authorization, not a conflict, on a public synced playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + _, err := ps.Create(ctx, "pls-synced-other", "", []string{"song-1"}) Expect(err).To(MatchError(model.ErrNotAuthorized)) }) }) @@ -159,6 +173,7 @@ var _ = Describe("Playlists", func() { "pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, + "pls-synced": {ID: "pls-synced", Name: "Synced", OwnerID: "user-1", Sync: true}, } mockPlsRepo.TracksRepo = mockTracks ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds)) @@ -191,13 +206,13 @@ var _ = Describe("Playlists", func() { It("denies adding tracks to a smart playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) err := ps.Update(ctx, "pls-smart", nil, nil, nil, []string{"song-1"}, nil) - Expect(err).To(MatchError(model.ErrNotAuthorized)) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) }) It("denies removing tracks from a smart playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) err := ps.Update(ctx, "pls-smart", nil, nil, nil, nil, []int{0}) - Expect(err).To(MatchError(model.ErrNotAuthorized)) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) }) It("allows metadata updates on a smart playlist", func() { @@ -205,6 +220,18 @@ var _ = Describe("Playlists", func() { err := ps.Update(ctx, "pls-smart", new("Updated Smart"), nil, nil, nil, nil) Expect(err).ToNot(HaveOccurred()) }) + + It("denies adding tracks to a synced playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + err := ps.Update(ctx, "pls-synced", nil, nil, nil, []string{"song-1"}, nil) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) + }) + + It("allows metadata updates on a synced playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + err := ps.Update(ctx, "pls-synced", new("Renamed Synced"), nil, nil, nil, nil) + Expect(err).ToNot(HaveOccurred()) + }) }) Describe("AddTracks", func() { @@ -216,7 +243,8 @@ var _ = Describe("Playlists", func() { "pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"}, "pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1", Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}}, - "pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"}, + "pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"}, + "pls-synced": {ID: "pls-synced", Name: "Synced", OwnerID: "user-1", Sync: true}, } mockPlsRepo.TracksRepo = mockTracks ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds)) @@ -246,7 +274,13 @@ var _ = Describe("Playlists", func() { It("denies editing smart playlists", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) _, err := ps.AddTracks(ctx, "pls-smart", []string{"song-1"}) - Expect(err).To(MatchError(model.ErrNotAuthorized)) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) + }) + + It("denies editing synced playlists", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + _, err := ps.AddTracks(ctx, "pls-synced", []string{"song-1"}) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) }) It("returns error when playlist not found", func() { @@ -280,7 +314,7 @@ var _ = Describe("Playlists", func() { It("denies on smart playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) err := ps.RemoveTracks(ctx, "pls-smart", []string{"track-1"}) - Expect(err).To(MatchError(model.ErrNotAuthorized)) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) }) It("denies non-owner", func() { @@ -314,7 +348,7 @@ var _ = Describe("Playlists", func() { It("denies on smart playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) err := ps.ReorderTrack(ctx, "pls-smart", 1, 3) - Expect(err).To(MatchError(model.ErrNotAuthorized)) + Expect(err).To(MatchError(model.ErrPlaylistNotEditable)) }) }) diff --git a/model/errors.go b/model/errors.go index 41029d316..0e2543378 100644 --- a/model/errors.go +++ b/model/errors.go @@ -9,4 +9,7 @@ var ( ErrExpired = errors.New("access expired") ErrNotAvailable = errors.New("functionality not available") ErrValidation = errors.New("validation error") + + // ErrPlaylistNotEditable: tracks are server-managed, so nobody can edit them (not an ACL failure). + ErrPlaylistNotEditable = errors.New("playlist tracks are not editable") ) diff --git a/model/playlist.go b/model/playlist.go index 55b94a640..306401271 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -42,6 +42,11 @@ func (pls Playlist) IsSmartPlaylist() bool { return pls.Rules != nil && pls.Rules.Expression != nil } +// TracksEditable reports whether the track list is user-owned rather than server-managed. +func (pls Playlist) TracksEditable() bool { + return !pls.IsSmartPlaylist() && !pls.Sync +} + // RefreshDelay returns the playlist's own refresh window when set, falling // back to the global SmartPlaylistRefreshDelay. func (pls Playlist) RefreshDelay() time.Duration { diff --git a/model/playlist_test.go b/model/playlist_test.go index d936129ce..d98c85716 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -73,4 +73,19 @@ var _ = Describe("Playlist", func() { Expect(pls.RefreshDelay()).To(Equal(5 * time.Second)) }) }) + + Describe("TracksEditable", func() { + It("is true for a plain playlist", func() { + Expect(model.Playlist{}.TracksEditable()).To(BeTrue()) + }) + + It("is false for a smart playlist", func() { + pls := model.Playlist{Rules: &criteria.Criteria{Expression: criteria.Is{"loved": true}}} + Expect(pls.TracksEditable()).To(BeFalse()) + }) + + It("is false for a synced playlist", func() { + Expect(model.Playlist{Sync: true}.TracksEditable()).To(BeFalse()) + }) + }) }) diff --git a/server/jellyfin/playlists.go b/server/jellyfin/playlists.go index 805b369c8..ac790a821 100644 --- a/server/jellyfin/playlists.go +++ b/server/jellyfin/playlists.go @@ -29,11 +29,11 @@ func playlistsFolder() dto.BaseItemDto { } } -// playlistError maps core/playlists write errors to HTTP status: ownership -> 403, missing/invisible -// -> 404 (never revealing another user's private playlist), else -> 500. +// playlistError maps core/playlists write errors to HTTP status: ownership or locked -> 403, +// missing/invisible -> 404 (never revealing another user's private playlist), else -> 500. func (api *Router) playlistError(w http.ResponseWriter, r *http.Request, err error) { switch { - case errors.Is(err, model.ErrNotAuthorized): + case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, model.ErrPlaylistNotEditable): http.Error(w, "Forbidden", http.StatusForbidden) case errors.Is(err, model.ErrNotFound): http.Error(w, "Not Found", http.StatusNotFound) @@ -262,7 +262,7 @@ func (api *Router) songIDs(ctx context.Context, opts model.QueryOptions) []strin } // addToPlaylist appends items by id, expanding containers into tracks (see expandContainerIDs). -// AddTracks enforces ownership; any error maps to 404. +// AddTracks enforces ownership; a locked playlist maps to 403, any other error to 404. func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id, ok := itemIDParam(w, r, "playlistId") @@ -276,6 +276,10 @@ func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) { } ids := api.expandContainerIDs(ctx, decoded) if _, err := api.playlists.AddTracks(ctx, id, ids); err != nil { + if errors.Is(err, model.ErrPlaylistNotEditable) { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } http.Error(w, "Not Found", http.StatusNotFound) return } @@ -284,7 +288,7 @@ func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) { // removeFromPlaylist removes entries by entryIds — playlist-entry ids (PlaylistItemId), not media // file ids, since RemoveTracks deletes playlist_tracks rows by that id. RemoveTracks enforces -// ownership; any error maps to 404. +// ownership; a locked playlist maps to 403, any other error to 404. func (api *Router) removeFromPlaylist(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id, ok := itemIDParam(w, r, "playlistId") @@ -302,21 +306,44 @@ func (api *Router) removeFromPlaylist(w http.ResponseWriter, r *http.Request) { ids = append(ids, entry) } if err := api.playlists.RemoveTracks(ctx, id, ids); err != nil { + if errors.Is(err, model.ErrPlaylistNotEditable) { + http.Error(w, "Forbidden", http.StatusForbidden) + return + } http.Error(w, "Not Found", http.StatusNotFound) return } w.WriteHeader(http.StatusNoContent) } -// getPlaylistUsers and getPlaylistUser answer client probes (e.g. Finamp) made before allowing -// edits. Navidrome has no per-playlist ACL, so every user is reported CanEdit; ownership is still -// enforced by AddTracks/RemoveTracks. +// Clients probe these before offering edits. Navidrome has no per-playlist ACL, so CanEdit carries +// only editability; ownership is enforced on write, and a lookup error 404s to prevent probing. func (api *Router) getPlaylistUsers(w http.ResponseWriter, r *http.Request) { - u, _ := request.UserFrom(r.Context()) - api.ok(w, r, []dto.PlaylistUserPermissions{{UserId: dto.EncodeID(u.ID), CanEdit: true}}) + ctx := r.Context() + id, ok := itemIDParam(w, r, "playlistId") + if !ok { + return + } + pls, err := api.playlists.Get(ctx, id) + if err != nil { + http.Error(w, "Not Found", http.StatusNotFound) + return + } + u, _ := request.UserFrom(ctx) + api.ok(w, r, []dto.PlaylistUserPermissions{{UserId: dto.EncodeID(u.ID), CanEdit: pls.TracksEditable()}}) } func (api *Router) getPlaylistUser(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + id, ok := itemIDParam(w, r, "playlistId") + if !ok { + return + } + pls, err := api.playlists.Get(ctx, id) + if err != nil { + http.Error(w, "Not Found", http.StatusNotFound) + return + } userId := chi.URLParam(r, "userId") - api.ok(w, r, dto.PlaylistUserPermissions{UserId: userId, CanEdit: true}) + api.ok(w, r, dto.PlaylistUserPermissions{UserId: userId, CanEdit: pls.TracksEditable()}) } diff --git a/server/jellyfin/playlists_test.go b/server/jellyfin/playlists_test.go index 3599cc784..270f5fe08 100644 --- a/server/jellyfin/playlists_test.go +++ b/server/jellyfin/playlists_test.go @@ -381,6 +381,15 @@ var _ = Describe("Playlists", func() { Expect(w.Code).To(Equal(http.StatusNotFound)) }) + It("returns 403 when the playlist is not editable (synced/smart), like Jellyfin", func() { + fp.addErr = model.ErrPlaylistNotEditable + w := httptest.NewRecorder() + r := httptest.NewRequest("POST", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items?ids="+dto.EncodeID(testID("s1")), nil).WithContext(context.Background()) + r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1"))) + invoke(api.addToPlaylist, w, r) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + It("passes no ids (not a spurious empty string) when the ids param is absent", func() { w := httptest.NewRecorder() r := httptest.NewRequest("POST", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items", nil).WithContext(context.Background()) @@ -430,6 +439,15 @@ var _ = Describe("Playlists", func() { Expect(w.Code).To(Equal(http.StatusNotFound)) }) + It("returns 403 when the playlist is not editable (synced/smart), like Jellyfin", func() { + fp.removeErr = model.ErrPlaylistNotEditable + w := httptest.NewRecorder() + r := httptest.NewRequest("DELETE", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items?entryIds="+dto.EncodePlaylistEntryID("1"), nil).WithContext(context.Background()) + r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1"))) + invoke(api.removeFromPlaylist, w, r) + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + It("passes no ids (not a spurious empty string) when the entryIds param is absent", func() { w := httptest.NewRecorder() r := httptest.NewRequest("DELETE", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items", nil).WithContext(context.Background()) @@ -442,7 +460,8 @@ var _ = Describe("Playlists", func() { }) Describe("getPlaylistUsers", func() { - It("returns the current user with CanEdit true", func() { + It("returns the current user with CanEdit true for an editable playlist", func() { + fp.getByIDPls = &model.Playlist{ID: testID("pl1")} w := httptest.NewRecorder() ctx := request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice"}) r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users", nil).WithContext(ctx) @@ -453,21 +472,59 @@ var _ = Describe("Playlists", func() { Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) Expect(res).To(Equal([]dto.PlaylistUserPermissions{{UserId: dto.EncodeID(testID("u1")), CanEdit: true}})) }) + + It("reports CanEdit false for a synced playlist", func() { + fp.getByIDPls = &model.Playlist{ID: testID("pl1"), Sync: true} + w := httptest.NewRecorder() + ctx := request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice"}) + r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users", nil).WithContext(ctx) + r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1"))) + api.getPlaylistUsers(w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res []dto.PlaylistUserPermissions + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res[0].CanEdit).To(BeFalse()) + }) + + It("returns 404 when the playlist is not visible", func() { + fp.getByIDErr = model.ErrNotFound + w := httptest.NewRecorder() + ctx := request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice"}) + r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users", nil).WithContext(ctx) + r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1"))) + api.getPlaylistUsers(w, r) + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) }) Describe("getPlaylistUser", func() { - It("returns CanEdit true for the requested user", func() { + requestUser := func() *httptest.ResponseRecorder { w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users/"+testID("u1"), nil).WithContext(context.Background()) rctx := chi.NewRouteContext() - rctx.URLParams.Add("playlistId", testID("pl1")) + rctx.URLParams.Add("playlistId", dto.EncodeID(testID("pl1"))) rctx.URLParams.Add("userId", testID("u1")) r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx)) api.getPlaylistUser(w, r) + return w + } + + It("returns CanEdit true for an editable playlist", func() { + fp.getByIDPls = &model.Playlist{ID: testID("pl1")} + w := requestUser() Expect(w.Code).To(Equal(http.StatusOK)) var res dto.PlaylistUserPermissions Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) Expect(res).To(Equal(dto.PlaylistUserPermissions{UserId: testID("u1"), CanEdit: true})) }) + + It("reports CanEdit false for a synced playlist", func() { + fp.getByIDPls = &model.Playlist{ID: testID("pl1"), Sync: true} + w := requestUser() + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.PlaylistUserPermissions + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.CanEdit).To(BeFalse()) + }) }) }) diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index 90b2f9e94..d215eb9dd 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -20,6 +20,20 @@ import ( type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc +// writePlaylistError maps a playlist service error to an HTTP status, or defaultStatus if unknown. +func writePlaylistError(w http.ResponseWriter, err error, defaultStatus int) { + switch { + case errors.Is(err, model.ErrNotFound): + http.Error(w, err.Error(), http.StatusNotFound) + case errors.Is(err, model.ErrNotAuthorized): + http.Error(w, err.Error(), http.StatusForbidden) + case errors.Is(err, model.ErrPlaylistNotEditable): + http.Error(w, err.Error(), http.StatusConflict) + default: + http.Error(w, err.Error(), defaultStatus) + } +} + func playlistTracksHandler(pls playlists.Playlists, handler restHandler, refreshSmartPlaylist func(*http.Request) bool) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { plsId := chi.URLParam(r, "playlistId") @@ -111,7 +125,7 @@ func deleteFromPlaylist(pls playlists.Playlists) http.HandlerFunc { } if err != nil { log.Error(r.Context(), "Error deleting tracks from playlist", "playlistId", playlistId, "ids", ids, err) - http.Error(w, err.Error(), http.StatusInternalServerError) + writePlaylistError(w, err, http.StatusInternalServerError) return } writeDeleteManyResponse(w, r, ids) @@ -138,22 +152,22 @@ func addToPlaylist(pls playlists.Playlists) http.HandlerFunc { } count, c := 0, 0 if c, err = pls.AddTracks(ctx, playlistId, payload.Ids); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + writePlaylistError(w, err, http.StatusBadRequest) return } count += c if c, err = pls.AddAlbums(ctx, playlistId, payload.AlbumIds); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + writePlaylistError(w, err, http.StatusBadRequest) return } count += c if c, err = pls.AddArtists(ctx, playlistId, payload.ArtistIds); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + writePlaylistError(w, err, http.StatusBadRequest) return } count += c if c, err = pls.AddDiscs(ctx, playlistId, payload.Discs); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + writePlaylistError(w, err, http.StatusBadRequest) return } count += c @@ -192,12 +206,8 @@ func reorderItem(pls playlists.Playlists) http.HandlerFunc { return } err = pls.ReorderTrack(ctx, playlistId, id, newPos) - if errors.Is(err, model.ErrNotAuthorized) { - http.Error(w, err.Error(), http.StatusForbidden) - return - } if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + writePlaylistError(w, err, http.StatusBadRequest) return } diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 9bf502687..74ef58cab 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -183,6 +183,20 @@ var _ = Describe("Playlist Tracks Endpoint", func() { }) }) +var _ = Describe("writePlaylistError", func() { + DescribeTable("maps a service error to an HTTP status", + func(err error, expected int) { + w := httptest.NewRecorder() + writePlaylistError(w, err, http.StatusBadRequest) + Expect(w.Code).To(Equal(expected)) + }, + Entry("not found -> 404", model.ErrNotFound, http.StatusNotFound), + Entry("not authorized -> 403", model.ErrNotAuthorized, http.StatusForbidden), + Entry("not editable -> 409", model.ErrPlaylistNotEditable, http.StatusConflict), + Entry("unrecognized -> default", model.ErrValidation, http.StatusBadRequest), + ) +}) + type mockPlaylistTrackRepo struct { model.PlaylistTrackRepository tracks model.PlaylistTracks diff --git a/server/subsonic/api.go b/server/subsonic/api.go index 82e404228..029046c39 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -304,7 +304,8 @@ func mapToSubsonicError(err error) subError { err = newError(responses.ErrorGeneric, err.Error()) case errors.Is(err, model.ErrNotFound), errors.Is(err, rest.ErrNotFound): err = newError(responses.ErrorDataNotFound, "data not found") - case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, rest.ErrPermissionDenied): + case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, rest.ErrPermissionDenied), + errors.Is(err, model.ErrPlaylistNotEditable): // Subsonic has no code for "read-only resource" err = newError(responses.ErrorAuthorizationFail) case errors.Is(err, stream.ErrTooManyTranscodes): err = newError(responses.ErrorGeneric, "too many concurrent transcodes, please retry shortly") diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index bd53528d9..774a9c430 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -172,7 +172,7 @@ func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubso } } else { user, ok := request.UserFrom(ctx) - pls.Readonly = !ok || p.OwnerID != user.ID + pls.Readonly = !ok || p.OwnerID != user.ID || !p.TracksEditable() } return &pls diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index a7c9e2ec7..f18f33b47 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -111,6 +111,15 @@ var _ = Describe("buildPlaylist", func() { Expect(result.Public).To(BeTrue()) Expect(result.Readonly).To(BeFalse()) }) + + It("is read-only for a synced playlist even as owner", func() { + ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"}) + playlist.Sync = true + + result := router.buildPlaylist(ctx, playlist) + + Expect(result.Readonly).To(BeTrue()) + }) }) Context("when minimal clients list is empty", func() { diff --git a/ui/src/common/playlistUtils.js b/ui/src/common/playlistUtils.js index 74a01d47a..2d0c745ff 100644 --- a/ui/src/common/playlistUtils.js +++ b/ui/src/common/playlistUtils.js @@ -12,4 +12,4 @@ export const isReadOnly = (ownerId) => { export const isSmartPlaylist = (pls) => !!pls.rules export const canChangeTracks = (pls) => - isWritable(pls.ownerId) && !isSmartPlaylist(pls) + isWritable(pls.ownerId) && !isSmartPlaylist(pls) && !pls.sync diff --git a/ui/src/common/playlistUtils.test.js b/ui/src/common/playlistUtils.test.js index 2c671ecf5..345b6045d 100644 --- a/ui/src/common/playlistUtils.test.js +++ b/ui/src/common/playlistUtils.test.js @@ -74,5 +74,11 @@ describe('playlistUtils', () => { const playlist = { ownerId: 'user1', rules: [] } expect(canChangeTracks(playlist)).toBe(false) }) + + it('returns false if playlist is synced', () => { + localStorage.setItem('userId', 'user1') + const playlist = { ownerId: 'user1', sync: true } + expect(canChangeTracks(playlist)).toBe(false) + }) }) }) diff --git a/ui/src/dialogs/SelectPlaylistInput.jsx b/ui/src/dialogs/SelectPlaylistInput.jsx index 847107523..2f040a7f7 100644 --- a/ui/src/dialogs/SelectPlaylistInput.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.jsx @@ -16,7 +16,7 @@ import { import AddIcon from '@material-ui/icons/Add' import { useGetList, useTranslate } from 'react-admin' import PropTypes from 'prop-types' -import { isWritable } from '../common' +import { canChangeTracks } from '../common' import { makeStyles } from '@material-ui/core' const useStyles = makeStyles((theme) => ({ @@ -268,8 +268,7 @@ export const SelectPlaylistInput = ({ onChange }) => { ) const options = - ids && - ids.map((id) => data[id]).filter((option) => isWritable(option.ownerId)) + ids && ids.map((id) => data[id]).filter((option) => canChangeTracks(option)) // Filter playlists based on search text const filteredOptions = diff --git a/ui/src/dialogs/SelectPlaylistInput.test.jsx b/ui/src/dialogs/SelectPlaylistInput.test.jsx index 4ffcdf0b6..753d12c03 100644 --- a/ui/src/dialogs/SelectPlaylistInput.test.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.test.jsx @@ -16,6 +16,7 @@ const mockPlaylists = [ { id: 'playlist-2', name: 'Jazz Collection', ownerId: 'admin' }, { id: 'playlist-3', name: 'Electronic Beats', ownerId: 'admin' }, { id: 'playlist-4', name: 'Chill Vibes', ownerId: 'user2' }, // Not writable by admin + { id: 'playlist-5', name: 'Synced List', ownerId: 'admin', sync: true }, ] const mockIndexedData = { @@ -27,6 +28,12 @@ const mockIndexedData = { ownerId: 'admin', }, 'playlist-4': { id: 'playlist-4', name: 'Chill Vibes', ownerId: 'user2' }, + 'playlist-5': { + id: 'playlist-5', + name: 'Synced List', + ownerId: 'admin', + sync: true, + }, } const createTestComponent = ( @@ -89,6 +96,8 @@ describe('SelectPlaylistInput', () => { // Should not show playlists not owned by admin (not writable) expect(screen.queryByText('Chill Vibes')).not.toBeInTheDocument() + // Should not show synced playlists (their tracks are not editable) + expect(screen.queryByText('Synced List')).not.toBeInTheDocument() }) it('should filter playlists based on search input', async () => { From 2dab4b40483a6feb3be2007afa4a1ac83c2d0504 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 19 Aug 2026 08:49:15 -0400 Subject: [PATCH 3/9] chore: remove redundant comment Signed-off-by: Deluan --- model/errors.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/model/errors.go b/model/errors.go index 0e2543378..9e56f9501 100644 --- a/model/errors.go +++ b/model/errors.go @@ -3,13 +3,11 @@ package model import "errors" var ( - ErrNotFound = errors.New("data not found") - ErrInvalidAuth = errors.New("invalid authentication") - ErrNotAuthorized = errors.New("not authorized") - ErrExpired = errors.New("access expired") - ErrNotAvailable = errors.New("functionality not available") - ErrValidation = errors.New("validation error") - - // ErrPlaylistNotEditable: tracks are server-managed, so nobody can edit them (not an ACL failure). + ErrNotFound = errors.New("data not found") + ErrInvalidAuth = errors.New("invalid authentication") + ErrNotAuthorized = errors.New("not authorized") + ErrExpired = errors.New("access expired") + ErrNotAvailable = errors.New("functionality not available") + ErrValidation = errors.New("validation error") ErrPlaylistNotEditable = errors.New("playlist tracks are not editable") ) From 6d8a3e48eeacf4dda8edd6841f714f3ee064a371 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 19 Aug 2026 09:19:47 -0400 Subject: [PATCH 4/9] refactor: replace `md5` with `xxh3` for faster and more efficient hashing Signed-off-by: Deluan --- .golangci.yml | 3 +++ core/artwork/housekeeping.go | 6 ++---- core/artwork/housekeeping_test.go | 2 +- model/mediafile.go | 4 ++-- model/mediafile_test.go | 12 ++++-------- model/participants.go | 4 ++-- model/tag.go | 4 ++-- persistence/sql_base_repository.go | 6 +++--- 8 files changed, 19 insertions(+), 22 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 0804138dc..7df4f8ec3 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -27,6 +27,9 @@ linters: disable: - staticcheck settings: + errcheck: + exclude-functions: + - (*github.com/zeebo/xxh3.Hasher).Write gocritic: disable-all: true enabled-checks: diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index ae1fc0a0d..f2996d044 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -2,8 +2,6 @@ package artwork import ( "context" - "crypto/md5" - "encoding/hex" "fmt" "slices" "strconv" @@ -16,6 +14,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/slice" + "github.com/zeebo/xxh3" ) // StaleAbsentAge is how long an absent state is trusted before a recheck retries it. @@ -66,8 +65,7 @@ func FingerprintInputs() []FingerprintInput { func ConfigFingerprint() string { values := slice.Map(FingerprintInputs(), func(i FingerprintInput) string { return i.Value }) raw := fmt.Sprintf("%s|%d", strings.Join(values, "|"), artworkEpoch) - sum := md5.Sum([]byte(raw)) //nolint:gosec // fingerprint, not security-sensitive - return hex.EncodeToString(sum[:]) + return fmt.Sprintf("%016x", xxh3.Hash([]byte(raw))) } // backfill enqueues artwork resolution for every entity when the config fingerprint changed. diff --git a/core/artwork/housekeeping_test.go b/core/artwork/housekeeping_test.go index 32a7688b4..4ea15ab04 100644 --- a/core/artwork/housekeeping_test.go +++ b/core/artwork/housekeeping_test.go @@ -135,7 +135,7 @@ var _ = Describe("Housekeeping", func() { conf.Server.EnableExternalServices = true conf.Server.EnableM3UExternalAlbumArt = false - Expect(ConfigFingerprint()).To(Equal("7e537a22febc07d3d5ca40546e88da54")) + Expect(ConfigFingerprint()).To(Equal("7b538a83a870c16d")) }) It("reports the config inputs it hashes, so a change can be traced to a setting", func() { diff --git a/model/mediafile.go b/model/mediafile.go index f4e767272..99aee591e 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -2,7 +2,6 @@ package model import ( "cmp" - "crypto/md5" "encoding/json" "fmt" "iter" @@ -20,6 +19,7 @@ import ( "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/number" "github.com/navidrome/navidrome/utils/slice" + "github.com/zeebo/xxh3" ) type MediaFile struct { @@ -232,7 +232,7 @@ func (mf MediaFile) Hash() string { ZeroNil: true, } hash, _ := hashstructure.Hash(mf, opts) - sum := md5.New() + sum := xxh3.New() sum.Write(fmt.Appendf(nil, "%d", hash)) sum.Write(mf.Tags.Hash()) sum.Write(mf.Participants.Hash()) diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 3f306f1a7..097e3ca54 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -715,14 +715,10 @@ var _ = Describe("MediaFile.Movements", func() { }) var _ = Describe("MediaFile.Hash", func() { - // Guards the upgrade guarantee: converting BPM/BitDepth from int to *int must not change hashes, - // or every file would be spuriously re-imported on the next scan. - // Golden hashes were captured at 46221d516 when those fields were plain ints. - It("keeps hashes identical to the pre-pointer-conversion values", func() { - // Golden hashes computed at 46221d516, when BPM/BitDepth were plain ints — pinning - // them guarantees the pointer conversion cannot trigger a full-library re-import. - Expect(MediaFile{Title: "Song"}.Hash()).To(Equal("1d856ced42cb96db39e354a4bac9a622")) - Expect(MediaFile{Title: "Song", BPM: new(120), BitDepth: new(16)}.Hash()).To(Equal("b2b0b1d1dd7fd767093588e4af3a0689")) + // Pins the hash formula: an accidental change spuriously re-imports every file on the next scan. + It("hashes to a stable value", func() { + Expect(MediaFile{Title: "Song"}.Hash()).To(Equal("05fdf70bb0cbe090")) + Expect(MediaFile{Title: "Song", BPM: new(120), BitDepth: new(16)}.Hash()).To(Equal("b5daf6ac1009a538")) }) It("changes the hash when a pointer field has a value", func() { base := MediaFile{Title: "Song"} diff --git a/model/participants.go b/model/participants.go index afbda10de..b5390d22c 100644 --- a/model/participants.go +++ b/model/participants.go @@ -2,12 +2,12 @@ package model import ( "cmp" - "crypto/md5" "fmt" "slices" "strings" "github.com/navidrome/navidrome/utils/slice" + "github.com/zeebo/xxh3" ) var ( @@ -193,7 +193,7 @@ func (p Participants) Hash() []byte { flattened = append(flattened, role.String()+":"+strings.Join(ids, "/")) } slices.Sort(flattened) - sum := md5.New() + sum := xxh3.New() sum.Write([]byte(strings.Join(flattened, "|"))) return sum.Sum(nil) } diff --git a/model/tag.go b/model/tag.go index 1bc011495..bb4fce181 100644 --- a/model/tag.go +++ b/model/tag.go @@ -2,13 +2,13 @@ package model import ( "cmp" - "crypto/md5" "fmt" "slices" "strings" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/utils/slice" + "github.com/zeebo/xxh3" ) type Tag struct { @@ -117,7 +117,7 @@ func (t Tags) Hash() []byte { } ids := t.IDs() slices.Sort(ids) - sum := md5.New() + sum := xxh3.New() sum.Write([]byte(strings.Join(ids, "|"))) return sum.Sum(nil) } diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index c61bca1a6..d4cf9b456 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -2,7 +2,6 @@ package persistence import ( "context" - "crypto/md5" "database/sql" "errors" "fmt" @@ -23,6 +22,7 @@ import ( "github.com/navidrome/navidrome/utils/hasher" "github.com/navidrome/navidrome/utils/slice" "github.com/pocketbase/dbx" + "github.com/zeebo/xxh3" ) // sqlRepository is the base repository for all SQL repositories. It provides common functions to interact with the DB. @@ -298,8 +298,8 @@ func (r sqlRepository) visibleLibraryIDs() ([]int, error) { func (r sqlRepository) seedKey() string { // Seed keys must be all lowercase, or else SQLite3 will encode it, making it not match the seed // used in the query. Hashing the user ID and converting it to a hex string will do the trick - userIDHash := md5.Sum([]byte(loggedUser(r.ctx).ID)) - return fmt.Sprintf("%s|%x", r.tableName, userIDHash) + userIDHash := xxh3.Hash([]byte(loggedUser(r.ctx).ID)) + return fmt.Sprintf("%s|%016x", r.tableName, userIDHash) } func (r sqlRepository) resetSeededRandom(options []model.QueryOptions) { From bd6b7a66863ce4771cd8e3450e355cf512b31363 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 19 Aug 2026 10:35:15 -0400 Subject: [PATCH 5/9] test: increase timeout for cache availability checks to 10 seconds --- core/artwork/e2e/acquire_serve_test.go | 2 +- core/artwork/e2e/resolution_harness_test.go | 2 +- core/stream/media_streamer_test.go | 9 +++++---- server/subsonic/e2e/subsonic_artwork_test.go | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/core/artwork/e2e/acquire_serve_test.go b/core/artwork/e2e/acquire_serve_test.go index 0a9363b5d..34dfb2cac 100644 --- a/core/artwork/e2e/acquire_serve_test.go +++ b/core/artwork/e2e/acquire_serve_test.go @@ -106,7 +106,7 @@ var _ = Describe("Acquisition → serve loop", func() { func(context.Context, cache.Item) (io.Reader, error) { return nil, errors.New("resize not exercised in e2e") }) - Eventually(func() bool { return imgCache.Available(ctx) }).Should(BeTrue()) + Eventually(func() bool { return imgCache.Available(ctx) }, 10*time.Second).Should(BeTrue()) svc = artwork.NewArtwork(ds, imgCache, store, ffm) worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache) diff --git a/core/artwork/e2e/resolution_harness_test.go b/core/artwork/e2e/resolution_harness_test.go index 1168393df..fff62ce69 100644 --- a/core/artwork/e2e/resolution_harness_test.go +++ b/core/artwork/e2e/resolution_harness_test.go @@ -117,7 +117,7 @@ func setupResolutionHarness() { func(context.Context, cache.Item) (io.Reader, error) { return nil, fmt.Errorf("resize not exercised in e2e") }) - Eventually(func() bool { return imgCache.Available(rctx) }).Should(BeTrue()) + Eventually(func() bool { return imgCache.Available(rctx) }, 10*time.Second).Should(BeTrue()) rsvc = artwork.NewArtwork(rds, imgCache, rstore, ffm) rworker = artwork.NewWorker(rds, rstore, agents.GetAgents(rds, nil), ffm, events.NoopBroker(), imgCache) diff --git a/core/stream/media_streamer_test.go b/core/stream/media_streamer_test.go index f5ca16d3f..fb1c59a60 100644 --- a/core/stream/media_streamer_test.go +++ b/core/stream/media_streamer_test.go @@ -5,6 +5,7 @@ import ( "errors" "io" "os" + "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -33,7 +34,7 @@ var _ = Describe("MediaStreamer", func() { {ID: "123", Path: "tests/fixtures/test.mp3", Suffix: "mp3", BitRate: 128, Duration: 257.0}, }) testCache := stream.NewTranscodingCache() - Eventually(func() bool { return testCache.Available(context.TODO()) }).Should(BeTrue()) + Eventually(func() bool { return testCache.Available(context.TODO()) }, 10*time.Second).Should(BeTrue()) streamer = stream.NewMediaStreamer(ds, ffmpeg, testCache) }) AfterEach(func() { @@ -75,7 +76,7 @@ var _ = Describe("MediaStreamer", func() { conf.Server.Transcoding.MaxConcurrent = 1 conf.Server.Transcoding.MaxConcurrentPerUser = 0 tightCache := stream.NewTranscodingCache() - Eventually(func() bool { return tightCache.Available(context.TODO()) }).Should(BeTrue()) + Eventually(func() bool { return tightCache.Available(context.TODO()) }, 10*time.Second).Should(BeTrue()) tightStreamer := stream.NewMediaStreamer(ds, blockingFFmpeg, tightCache) userCtx := request.WithUsername(ctx, "alice") @@ -92,7 +93,7 @@ var _ = Describe("MediaStreamer", func() { conf.Server.Transcoding.MaxConcurrent = 1 conf.Server.Transcoding.MaxConcurrentPerUser = 0 tightCache := stream.NewTranscodingCache() - Eventually(func() bool { return tightCache.Available(context.TODO()) }).Should(BeTrue()) + Eventually(func() bool { return tightCache.Available(context.TODO()) }, 10*time.Second).Should(BeTrue()) tightStreamer := stream.NewMediaStreamer(ds, ffmpeg, tightCache) userCtx := request.WithUsername(ctx, "alice") @@ -112,7 +113,7 @@ var _ = Describe("MediaStreamer", func() { conf.Server.Transcoding.MaxConcurrent = 1 conf.Server.Transcoding.MaxConcurrentPerUser = 0 tightCache := stream.NewTranscodingCache() - Eventually(func() bool { return tightCache.Available(context.TODO()) }).Should(BeTrue()) + Eventually(func() bool { return tightCache.Available(context.TODO()) }, 10*time.Second).Should(BeTrue()) tightStreamer := stream.NewMediaStreamer(ds, ffmpeg, tightCache) userCtx := request.WithUsername(ctx, "alice") diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 619619810..9324ea9e3 100644 --- a/server/subsonic/e2e/subsonic_artwork_test.go +++ b/server/subsonic/e2e/subsonic_artwork_test.go @@ -313,7 +313,7 @@ func newDummyImageCache(ctx context.Context) cache.FileCache { func(context.Context, cache.Item) (io.Reader, error) { return nil, errors.New("resize not exercised in subsonic artwork e2e") }) - Eventually(func() bool { return c.Available(ctx) }).Should(BeTrue()) + Eventually(func() bool { return c.Available(ctx) }, 10*time.Second).Should(BeTrue()) return c } From c362519f76d1922a6c3e1345e5ec91dada6275ed Mon Sep 17 00:00:00 2001 From: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:50:32 +0200 Subject: [PATCH 6/9] test: unskip path-separator tests on Windows (#5381) (#5916) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: unskip AbsolutePath and i18n path-separator tests on Windows (#5381) Signed-off-by: junkerderprovinz * test: unskip metadata folder-PID test on Windows via path.Dir (#5381) Signed-off-by: junkerderprovinz * test(storage): make relative-folder assertion cross-platform and unskip on Windows (#5381) Signed-off-by: junkerderprovinz * fix(persistence): normalize folder-update-info paths with forward slashes on Windows (#5381) Signed-off-by: junkerderprovinz * review: drop folder-PID change, trim storage_test comment (#5381) Revert model/metadata/persistent_ids.go to master: switching the `folder` PID attribute from filepath.Dir to path.Dir would change the persistent IDs of existing Windows libraries and needs a migration path, so it is out of scope for this PR. The matching test unskip is reverted with it, leaving #TBD-path-sep-metadata open in #5381. Trim the core/storage/storage_test.go comment to two lines. Signed-off-by: junkerderprovinz --------- Signed-off-by: junkerderprovinz Co-authored-by: Deluan Quintão --- core/common_test.go | 4 ++-- core/storage/storage_test.go | 10 ++++++---- persistence/folder_repository_test.go | 5 ----- server/nativeapi/translations_test.go | 5 ++--- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/core/common_test.go b/core/common_test.go index 0d6e3a299..f57ea8d38 100644 --- a/core/common_test.go +++ b/core/common_test.go @@ -2,6 +2,7 @@ package core import ( "context" + "path/filepath" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -41,10 +42,9 @@ var _ = Describe("common.go", func() { }) It("returns the absolute path when library exists", func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-core)") ctx := context.Background() abs := AbsolutePath(ctx, ds, libId, path) - Expect(abs).To(Equal("/library/root/music/file.mp3")) + Expect(abs).To(Equal(filepath.FromSlash("/library/root/music/file.mp3"))) }) It("returns the original path if library not found", func() { diff --git a/core/storage/storage_test.go b/core/storage/storage_test.go index 336b5a7a9..03904d086 100644 --- a/core/storage/storage_test.go +++ b/core/storage/storage_test.go @@ -7,7 +7,6 @@ import ( "runtime" "testing" - "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -56,13 +55,16 @@ var _ = Describe("Storage", func() { Expect(s.(*fakeLocalStorage).u.Path).To(Equal("/tmp")) }) It("should return a file implementation for a relative folder", func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-storage)") s, err := For("tmp") Expect(err).ToNot(HaveOccurred()) cwd, _ := os.Getwd() Expect(s).To(BeAssignableToTypeOf(&fakeLocalStorage{})) - Expect(s.(*fakeLocalStorage).u.Scheme).To(Equal("file")) - Expect(s.(*fakeLocalStorage).u.Path).To(Equal(filepath.Join(cwd, "tmp"))) + u := s.(*fakeLocalStorage).u + Expect(u.Scheme).To(Equal("file")) + // On Windows the drive letter lands in u.Host, so re-join it with + // u.Path (as newLocalStorage does) to keep the assertion OS-independent. + got := filepath.Join(u.Host, filepath.FromSlash(u.Path)) + Expect(got).To(Equal(filepath.Join(cwd, "tmp"))) }) It("should return error if schema is unregistered", func() { _, err := For("webdav:///tmp") diff --git a/persistence/folder_repository_test.go b/persistence/folder_repository_test.go index b429cecb9..545d8ad49 100644 --- a/persistence/folder_repository_test.go +++ b/persistence/folder_repository_test.go @@ -8,7 +8,6 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/tests" "github.com/navidrome/navidrome/utils/slice" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -137,7 +136,6 @@ var _ = Describe("FolderRepository", func() { }) It("includes all child folders when querying parent", func() { - tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)") // Create a parent folder with multiple children parent := model.NewFolder(testLib, "TestParent/Music") child1 := model.NewFolder(testLib, "TestParent/Music/Rock/Queen") @@ -159,7 +157,6 @@ var _ = Describe("FolderRepository", func() { }) It("excludes children from other libraries", func() { - tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)") // Create parent in testLib parent := model.NewFolder(testLib, "TestIsolation/Parent") child := model.NewFolder(testLib, "TestIsolation/Parent/Child") @@ -185,7 +182,6 @@ var _ = Describe("FolderRepository", func() { }) It("excludes missing children when querying parent", func() { - tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)") // Create parent and children, mark one as missing parent := model.NewFolder(testLib, "TestMissingChild/Parent") child1 := model.NewFolder(testLib, "TestMissingChild/Parent/Child1") @@ -206,7 +202,6 @@ var _ = Describe("FolderRepository", func() { }) It("handles mix of existing and non-existing target paths", func() { - tests.SkipOnWindows("path storage (#TBD-path-sep-persistence)") // Create folders for one path but not the other existingParent := model.NewFolder(testLib, "TestMixed/Exists") existingChild := model.NewFolder(testLib, "TestMixed/Exists/Child") diff --git a/server/nativeapi/translations_test.go b/server/nativeapi/translations_test.go index 77c088f6f..1c641da77 100644 --- a/server/nativeapi/translations_test.go +++ b/server/nativeapi/translations_test.go @@ -5,12 +5,12 @@ import ( "io" "io/fs" "os" + "path" "path/filepath" "testing/fstest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/resources" - "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -18,13 +18,12 @@ import ( var _ = Describe("Translations", func() { Describe("I18n files", func() { It("contains only valid json language files", func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-nativeapi)") fsys := resources.FS() dir, _ := fsys.Open(consts.I18nFolder) files, _ := dir.(fs.ReadDirFile).ReadDir(-1) for _, f := range files { name := filepath.Base(f.Name()) - filePath := filepath.Join(consts.I18nFolder, name) + filePath := path.Join(consts.I18nFolder, name) file, _ := fsys.Open(filePath) data, _ := io.ReadAll(file) var out map[string]any From dff9e47c2ea53e040ddac2fcc36f4563e53053be Mon Sep 17 00:00:00 2001 From: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:11:00 +0200 Subject: [PATCH 7/9] fix(scanner): detect in-place playlist edits via the folder content hash (#5914) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(scanner): detect in-place playlist edits via the folder content hash Signed-off-by: junkerderprovinz * docs(scanner): clarify the playlist entries in the folder hash Shorten the comment on the playlist loop, and record why the playlist count stays in the hash header: it is redundant with the loop for change detection, but removing it changes the hashed byte stream for every folder, including folders without playlists, which would mark every folder outdated on the first scan after upgrade. Signed-off-by: junkerderprovinz * test(scanner): pin filename and size into the playlist hash assertions The playlist size test called time.Now() twice, so the modtime differed too and carried the assertion — dropping info.Size() from the hash left the suite green. It now shares one baseTime. A new rename test swaps the map key with count, size and modtime held constant, so dropping the filename from the hash fails. Both mutations were verified to fail before this change and pass after. --------- Signed-off-by: junkerderprovinz Co-authored-by: Deluan --- scanner/folder_entry.go | 22 +++++++--- scanner/folder_entry_test.go | 80 ++++++++++++++++++++++++++++++++---- scanner/phase_1_folders.go | 4 +- scanner/walk_dir_tree.go | 4 +- 4 files changed, 93 insertions(+), 17 deletions(-) diff --git a/scanner/folder_entry.go b/scanner/folder_entry.go index 0e893d6e6..e7eef223c 100644 --- a/scanner/folder_entry.go +++ b/scanner/folder_entry.go @@ -23,6 +23,7 @@ func newFolderEntry(job *scanJob, id, path string, info model.FolderUpdateInfo) path: path, audioFiles: make(map[string]fs.DirEntry), imageFiles: make(map[string]fs.DirEntry), + playlistFiles: make(map[string]fs.DirEntry), albumIDMap: make(map[string]string), updTime: info.UpdatedAt, prevHash: info.Hash, @@ -41,7 +42,7 @@ type folderEntry struct { updTime time.Time // from DB audioFiles map[string]fs.DirEntry imageFiles map[string]fs.DirEntry - numPlaylists int + playlistFiles map[string]fs.DirEntry numSubFolders int imagesUpdatedAt time.Time prevHash string // Previous hash from DB @@ -57,7 +58,7 @@ type folderEntry struct { } func (f *folderEntry) hasNoFiles() bool { - return len(f.audioFiles) == 0 && len(f.imageFiles) == 0 && f.numPlaylists == 0 + return len(f.audioFiles) == 0 && len(f.imageFiles) == 0 && len(f.playlistFiles) == 0 } func (f *folderEntry) isEmpty() bool { @@ -94,7 +95,7 @@ func (f *folderEntry) toFolder() *model.Folder { folder := model.NewFolder(f.job.lib, f.path) folder.NumAudioFiles = len(f.audioFiles) if playlists.InPath(*folder) { - folder.NumPlaylists = f.numPlaylists + folder.NumPlaylists = len(f.playlistFiles) } folder.ImageFiles = slices.Collect(maps.Keys(f.imageFiles)) folder.ImagesUpdatedAt = f.imagesUpdatedAt @@ -108,16 +109,18 @@ func (f *folderEntry) hash() string { h, "%s:%d:%d:%s", f.modTime.UTC(), - f.numPlaylists, + len(f.playlistFiles), // redundant with the loop below, but dropping it re-hashes every folder f.numSubFolders, f.imagesUpdatedAt.UTC(), ) - // Sort the keys of audio and image files to ensure consistent hashing + // Sort the keys of audio, image and playlist files to ensure consistent hashing audioKeys := slices.Collect(maps.Keys(f.audioFiles)) slices.Sort(audioKeys) imageKeys := slices.Collect(maps.Keys(f.imageFiles)) slices.Sort(imageKeys) + playlistKeys := slices.Collect(maps.Keys(f.playlistFiles)) + slices.Sort(playlistKeys) // Include audio files with their size and modtime for _, key := range audioKeys { @@ -135,5 +138,14 @@ func (f *folderEntry) hash() string { } } + // Include playlist files, so a content edit is detected even when the folder's + // mtime is preserved (rsync -a) or the playlist is not the newest file. + for _, key := range playlistKeys { + _, _ = io.WriteString(h, key) + if info, err := f.playlistFiles[key].Info(); err == nil { + _, _ = fmt.Fprintf(h, ":%d:%s", info.Size(), info.ModTime().UTC().String()) + } + } + return hex.EncodeToString(h.Sum(nil)) } diff --git a/scanner/folder_entry_test.go b/scanner/folder_entry_test.go index e8e354b38..4493f9309 100644 --- a/scanner/folder_entry_test.go +++ b/scanner/folder_entry_test.go @@ -48,6 +48,7 @@ var _ = Describe("folder_entry", func() { Expect(entry.path).To(Equal(path)) Expect(entry.audioFiles).To(BeEmpty()) Expect(entry.imageFiles).To(BeEmpty()) + Expect(entry.playlistFiles).To(BeEmpty()) Expect(entry.albumIDMap).To(BeEmpty()) Expect(entry.updTime).To(Equal(updateInfo.UpdatedAt)) Expect(entry.prevHash).To(Equal(updateInfo.Hash)) @@ -95,7 +96,7 @@ var _ = Describe("folder_entry", func() { }) It("returns false when folder has playlists", func() { - entry.numPlaylists = 1 + entry.playlistFiles["list.m3u"] = &fakeDirEntry{name: "list.m3u"} Expect(entry.hasNoFiles()).To(BeFalse()) }) @@ -107,7 +108,7 @@ var _ = Describe("folder_entry", func() { It("returns false when folder has multiple types of content", func() { entry.audioFiles["test.mp3"] = &fakeDirEntry{name: "test.mp3"} entry.imageFiles["cover.jpg"] = &fakeDirEntry{name: "cover.jpg"} - entry.numPlaylists = 2 + entry.playlistFiles["list.m3u"] = &fakeDirEntry{name: "list.m3u"} entry.numSubFolders = 3 Expect(entry.hasNoFiles()).To(BeFalse()) }) @@ -149,7 +150,11 @@ var _ = Describe("folder_entry", func() { "cover.jpg": &fakeDirEntry{name: "cover.jpg"}, "folder.png": &fakeDirEntry{name: "folder.png"}, } - entry.numPlaylists = 3 + entry.playlistFiles = map[string]fs.DirEntry{ + "list1.m3u": &fakeDirEntry{name: "list1.m3u"}, + "list2.m3u": &fakeDirEntry{name: "list2.m3u"}, + "list3.m3u": &fakeDirEntry{name: "list3.m3u"}, + } entry.imagesUpdatedAt = time.Now() }) @@ -200,7 +205,10 @@ var _ = Describe("folder_entry", func() { "z.jpg": &fakeDirEntry{name: "z.jpg"}, "x.png": &fakeDirEntry{name: "x.png"}, } - entry.numPlaylists = 2 + entry.playlistFiles = map[string]fs.DirEntry{ + "q.m3u": &fakeDirEntry{name: "q.m3u"}, + "p.m3u": &fakeDirEntry{name: "p.m3u"}, + } entry.numSubFolders = 3 hash1 := entry.hash() @@ -214,6 +222,10 @@ var _ = Describe("folder_entry", func() { "x.png": &fakeDirEntry{name: "x.png"}, "z.jpg": &fakeDirEntry{name: "z.jpg"}, } + entry.playlistFiles = map[string]fs.DirEntry{ + "p.m3u": &fakeDirEntry{name: "p.m3u"}, + "q.m3u": &fakeDirEntry{name: "q.m3u"}, + } hash2 := entry.hash() Expect(hash1).To(Equal(hash2)) @@ -252,10 +264,10 @@ var _ = Describe("folder_entry", func() { Expect(hash1).ToNot(Equal(hash2)) }) - It("produces different hash when playlist count changes", func() { + It("produces different hash when playlist files change", func() { hash1 := entry.hash() - entry.numPlaylists = 5 + entry.playlistFiles["new.m3u"] = &fakeDirEntry{name: "new.m3u"} hash2 := entry.hash() Expect(hash1).ToNot(Equal(hash2)) @@ -377,6 +389,58 @@ var _ = Describe("folder_entry", func() { Expect(hash1).ToNot(Equal(hash2)) }) + It("produces different hash when playlist file size changes", func() { + baseTime := time.Now() + entry.playlistFiles["list.m3u"] = &fakeDirEntry{ + name: "list.m3u", + fileInfo: &fakeFileInfo{name: "list.m3u", size: 1000, modTime: baseTime}, + } + hash1 := entry.hash() + + entry.playlistFiles["list.m3u"] = &fakeDirEntry{ + name: "list.m3u", + fileInfo: &fakeFileInfo{name: "list.m3u", size: 2000, modTime: baseTime}, + } + hash2 := entry.hash() + + Expect(hash1).ToNot(Equal(hash2)) + }) + + It("produces different hash when playlist file modification time changes", func() { + baseTime := time.Now() + entry.playlistFiles["list.m3u"] = &fakeDirEntry{ + name: "list.m3u", + fileInfo: &fakeFileInfo{name: "list.m3u", size: 1000, modTime: baseTime}, + } + hash1 := entry.hash() + + entry.playlistFiles["list.m3u"] = &fakeDirEntry{ + name: "list.m3u", + fileInfo: &fakeFileInfo{name: "list.m3u", size: 1000, modTime: baseTime.Add(1 * time.Hour)}, + } + hash2 := entry.hash() + + Expect(hash1).ToNot(Equal(hash2)) + }) + + It("produces different hash when a playlist is renamed", func() { + baseTime := time.Now() + entry.playlistFiles["old.m3u"] = &fakeDirEntry{ + name: "old.m3u", + fileInfo: &fakeFileInfo{name: "old.m3u", size: 1000, modTime: baseTime}, + } + hash1 := entry.hash() + + delete(entry.playlistFiles, "old.m3u") + entry.playlistFiles["new.m3u"] = &fakeDirEntry{ + name: "new.m3u", + fileInfo: &fakeFileInfo{name: "new.m3u", size: 1000, modTime: baseTime}, + } + hash2 := entry.hash() + + Expect(hash1).ToNot(Equal(hash2)) + }) + It("produces valid hex-encoded hash", func() { hash := entry.hash() Expect(hash).To(HaveLen(32)) // MD5 hash should be 32 hex characters @@ -421,7 +485,7 @@ var _ = Describe("folder_entry", func() { }) It("returns true when hash has changed", func() { - entry.numPlaylists = 10 // Change something to change the hash + entry.playlistFiles["list.m3u"] = &fakeDirEntry{name: "list.m3u"} // Change something to change the hash Expect(entry.isOutdated()).To(BeTrue()) }) @@ -445,7 +509,7 @@ var _ = Describe("folder_entry", func() { It("returns true when full scan condition is not met but hash changed", func() { entry.updTime = entry.job.lib.LastScanStartedAt.Add(1 * time.Hour) - entry.numPlaylists = 10 // Change hash + entry.playlistFiles["list.m3u"] = &fakeDirEntry{name: "list.m3u"} // Change hash Expect(entry.isOutdated()).To(BeTrue()) }) }) diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 82e91d5ad..6107b3316 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -165,7 +165,7 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { log.Trace(p.ctx, "Scanner: Checking folder state", " folder", folder.path, "_updTime", folder.updTime, "_modTime", folder.modTime, "_lastScanStartedAt", folder.job.lib.LastScanStartedAt, "numAudioFiles", len(folder.audioFiles), "numImageFiles", len(folder.imageFiles), - "numPlaylists", folder.numPlaylists, "numSubfolders", folder.numSubFolders) + "numPlaylists", len(folder.playlistFiles), "numSubfolders", folder.numSubFolders) // Check if folder is outdated if folder.isOutdated() { @@ -492,7 +492,7 @@ func (p *phaseFolders) logFolder(entry *folderEntry) (*folderEntry, error) { logCall = log.Trace } logCall(p.ctx, "Scanner: Completed processing folder", - "audioCount", len(entry.audioFiles), "imageCount", len(entry.imageFiles), "plsCount", entry.numPlaylists, + "audioCount", len(entry.audioFiles), "imageCount", len(entry.imageFiles), "plsCount", len(entry.playlistFiles), "elapsed", entry.elapsed.Elapsed(), "tracksMissing", len(entry.missingTracks), "tracksImported", len(entry.tracks), "library", entry.job.lib.Name, consts.Zwsp+"folder", entry.path) return entry, nil diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 887344b1b..20f6d4213 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -82,7 +82,7 @@ func walkFolder(ctx context.Context, job *scanJob, currentFolder string, checker dir := path.Clean(currentFolder) log.Trace(ctx, "Scanner: Found directory", " path", dir, "audioFiles", maps.Keys(folder.audioFiles), - "images", maps.Keys(folder.imageFiles), "playlists", folder.numPlaylists, "imagesUpdatedAt", folder.imagesUpdatedAt, + "images", maps.Keys(folder.imageFiles), "playlists", len(folder.playlistFiles), "imagesUpdatedAt", folder.imagesUpdatedAt, "updTime", folder.updTime, "modTime", folder.modTime, "numChildren", len(children)) folder.path = dir folder.elapsed.Start() @@ -157,7 +157,7 @@ func loadDir(ctx context.Context, job *scanJob, dirPath string, checker *IgnoreC case model.IsAudioFile(name): folder.audioFiles[entry.Name()] = entry case model.IsValidPlaylist(name): - folder.numPlaylists++ + folder.playlistFiles[entry.Name()] = entry case model.IsImageFile(name): folder.imageFiles[entry.Name()] = entry folder.imagesUpdatedAt = utils.TimeNewest(folder.imagesUpdatedAt, fileInfo.ModTime(), folder.modTime) From fd4b3256e4c3e2c6eeeb90c2e3034149b536f873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 19 Aug 2026 16:20:38 -0400 Subject: [PATCH 8/9] perf(artwork): compute the blurhash DCT separably (#5989) The cosine basis factors into cosX[i][x] * cosY[j][y], so the pixel loop does not need to visit every (i,j) pair. Each row now collapses to xComp dot products, folded over yComp once per row: w*h*xComp + h*xComp*yComp multiply-accumulates instead of w*h*xComp*yComp. Encoding is ~60% faster at every input size, and ~80% faster at the 128px size the artwork pipeline actually feeds it (263us -> 53us). Hashes are byte-identical, so the existing golden-value specs cover the rewrite. --- core/artwork/blurhash/blurhash.go | 35 +++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/core/artwork/blurhash/blurhash.go b/core/artwork/blurhash/blurhash.go index ca5bf7026..29e1ccfda 100644 --- a/core/artwork/blurhash/blurhash.go +++ b/core/artwork/blurhash/blurhash.go @@ -52,6 +52,12 @@ func Encode(img image.Image) (string, error) { lin := srgbToLinearTable() factors := make([][3]float64, xComp*yComp) + linR := make([]float64, w) + linG := make([]float64, w) + linB := make([]float64, w) + rowR := make([]float64, xComp) + rowG := make([]float64, xComp) + rowB := make([]float64, xComp) for y := range h { row := src.pix[y*src.stride:] for x := range w { @@ -60,15 +66,26 @@ func Encode(img image.Image) (string, error) { if src.straight { r, g, b = premultiply(r, g, b, row[p+3]) } - lr, lg, lb := lin[r], lin[g], lin[b] - for j := range yComp { - for i := range xComp { - basis := cosX[i][x] * cosY[j][y] - f := &factors[j*xComp+i] - f[0] += basis * lr - f[1] += basis * lg - f[2] += basis * lb - } + linR[x], linG[x], linB[x] = lin[r], lin[g], lin[b] + } + // The basis is separable, so a row costs xComp dot products plus one fold over yComp, + // rather than xComp*yComp multiply-accumulates per pixel. + for i := range xComp { + var sr, sg, sb float64 + for x, c := range cosX[i] { + sr += c * linR[x] + sg += c * linG[x] + sb += c * linB[x] + } + rowR[i], rowG[i], rowB[i] = sr, sg, sb + } + for j := range yComp { + cy := cosY[j][y] + for i := range xComp { + f := &factors[j*xComp+i] + f[0] += cy * rowR[i] + f[1] += cy * rowG[i] + f[2] += cy * rowB[i] } } } From 881073c1833e66c47839927dc3c1577e39471c25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 19 Aug 2026 16:25:12 -0400 Subject: [PATCH 9/9] feat(cli): let artwork explain/refresh accept an id without its kind (#5988) * feat(cmd): make artwork explain/refresh accept an id without its kind The kind can now come from the id itself: a full artwork id (al-) carries it in the prefix, and a bare id is resolved across tables via GetEntityByID. The explicit leader still works. * fix(cmd): keep refreshing resolvable ids when others fail to resolve resolveArtworkTargets now collects a self-describing id it cannot resolve as a failure instead of aborting, so refresh reports and skips the bad ones and still queues the rest, matching refreshItems per-item behavior. explain stays strict and rejects any unresolved input. --- cmd/artwork.go | 110 +++++++++++++++++++++++++++++++-------- cmd/artwork_test.go | 83 ++++++++++++++++++++++++++--- model/get_entity.go | 37 +++++++++---- model/get_entity_test.go | 22 ++++++++ 4 files changed, 212 insertions(+), 40 deletions(-) diff --git a/cmd/artwork.go b/cmd/artwork.go index e9c910f11..aeaec0e43 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -59,31 +59,27 @@ var artworkCmd = &cobra.Command{ } var artworkExplainCmd = &cobra.Command{ - Use: "explain ", + Use: "explain [] ", Short: "Explain why an item's artwork resolved the way it did", Long: "Explain why an item's artwork resolved the way it did.\n\n" + + "The item can be given as a bare id, a full artwork id (e.g. al-), or a pair.\n" + " is one of: " + kindPrefixes(explainKinds) + ".\n" + "A disc artwork id is the album id and the disc number, joined by a colon: :2", - Args: cobra.ExactArgs(2), + Args: cobra.RangeArgs(1, 2), Run: func(cmd *cobra.Command, args []string) { - kind, err := parseArtworkKind(args[0], explainKinds) - if err != nil { - log.Fatal(cmd.Context(), err) - } - runExplain(cmd.Context(), kind, args[1]) + runExplain(cmd.Context(), args) }, } var artworkRefreshCmd = &cobra.Command{ - Use: "refresh ...", + Use: "refresh [] ...", Short: "Clear an item's artwork state and re-resolve it", - Args: cobra.MinimumNArgs(2), + Long: "Clear an item's artwork state and re-resolve it.\n\n" + + "Each item can be given as a bare id, a full artwork id (e.g. al-), or a shared\n" + + " ... leader. is one of: " + kindPrefixes(artwork.RefreshableKinds) + ".", + Args: cobra.MinimumNArgs(1), Run: func(cmd *cobra.Command, args []string) { - kind, err := parseArtworkKind(args[0], artwork.RefreshableKinds) - if err != nil { - log.Fatal(cmd.Context(), err) - } - runRefresh(cmd.Context(), kind, args[1:]) + runRefresh(cmd.Context(), args) }, } @@ -499,19 +495,28 @@ func printReprocessPreview(out io.Writer, kinds []model.Kind, matched []int64, t } } -func runRefresh(ctx context.Context, kind model.Kind, ids []string) { +func runRefresh(ctx context.Context, args []string) { defer db.Init(ctx)() ds, ctx := getAdminContext(ctx) - if failed := refreshItems(ctx, ds, kind, ids, os.Stdout); failed > 0 { - log.Fatal(ctx, "Failed to refresh artwork", "kind", kind, "failed", failed, "total", len(ids)) + targets, failures, err := resolveArtworkTargets(ctx, ds, args, artwork.RefreshableKinds) + if err != nil { + log.Fatal(ctx, err) + } + for _, f := range failures { + log.Error(ctx, "Skipping unresolved item", f) + } + failed := refreshItems(ctx, ds, targets, os.Stdout) + len(failures) + if failed > 0 { + log.Fatal(ctx, "Failed to refresh artwork", "failed", failed, "total", len(targets)+len(failures)) } } -// refreshItems keeps going after a failure — the ids are independent — and returns how many failed. -func refreshItems(ctx context.Context, ds model.DataStore, kind model.Kind, ids []string, out io.Writer) int { +// refreshItems keeps going after a failure — the items are independent — and returns how many failed. +func refreshItems(ctx context.Context, ds model.DataStore, targets []model.ArtworkID, out io.Writer) int { var failed int - for _, id := range ids { + for _, t := range targets { + kind, id := t.Kind, t.ID // artwork.Refresh would happily queue an id that does not exist, orphaning a queue row. if _, err := artworkItemName(ctx, ds, kind, id); err != nil { log.Error(ctx, "Item not found", "kind", kind, "id", id, err) @@ -544,7 +549,56 @@ func parseArtworkKind(s string, valid []model.Kind) (model.Kind, error) { if ok && slices.Contains(valid, kind) { return kind, nil } - return kind, fmt.Errorf("invalid kind %q, expected one of: %s", s, kindPrefixes(valid)) + return kind, invalidKindErr(s, valid) +} + +func invalidKindErr(s string, valid []model.Kind) error { + return fmt.Errorf("invalid kind %q, expected one of: %s", s, kindPrefixes(valid)) +} + +// resolveArtworkTargets resolves explain/refresh positional args into artwork ids, accepting a +// shared " ..." leader or self-describing args (a bare id, or a full artwork id). A +// self-describing arg that cannot be resolved is returned as a failure rather than aborting the +// batch, so refresh can process the resolvable ids; a malformed leader is a usage error. +func resolveArtworkTargets(ctx context.Context, ds model.DataStore, args []string, valid []model.Kind) ([]model.ArtworkID, []error, error) { + if kind, ok := model.ParseKind(args[0]); ok && len(args) > 1 { + if !slices.Contains(valid, kind) { + return nil, nil, invalidKindErr(args[0], valid) + } + return slice.Map(args[1:], func(id string) model.ArtworkID { + return model.ArtworkID{Kind: kind, ID: id} + }), nil, nil + } + var targets []model.ArtworkID + var failures []error + for _, arg := range args { + target, err := artworkKindAndID(ctx, ds, arg) + if err == nil && !slices.Contains(valid, target.Kind) { + err = invalidKindErr(target.Kind.Prefix(), valid) + } + if err != nil { + failures = append(failures, err) + continue + } + targets = append(targets, target) + } + return targets, failures, nil +} + +// artworkKindAndID resolves one self-describing argument: a full artwork id (al-) takes its kind +// from the prefix, a bare id is looked up. Entity ids never start with "-", so no collision. +func artworkKindAndID(ctx context.Context, ds model.DataStore, arg string) (model.ArtworkID, error) { + if artID, err := model.ParseArtworkID(arg); err == nil && artID.ID != "" { + return model.ArtworkID{Kind: artID.Kind, ID: artID.ID}, nil + } + kind, err := model.GetEntityKindByID(ctx, ds, arg) + if errors.Is(err, model.ErrNotFound) { + return model.ArtworkID{}, fmt.Errorf("could not determine kind for %q; pass an explicit ", arg) + } + if err != nil { + return model.ArtworkID{}, err + } + return model.ArtworkID{Kind: kind, ID: arg}, nil } // explainAgents accounts for every configured agent: one the CLI cannot construct (a plugin, or a @@ -721,10 +775,22 @@ func formatTime(t time.Time) string { return t.Format(time.RFC3339) } -func runExplain(ctx context.Context, kind model.Kind, id string) { +func runExplain(ctx context.Context, args []string) { defer db.Init(ctx)() ds, ctx := getAdminContext(ctx) + targets, failures, err := resolveArtworkTargets(ctx, ds, args, explainKinds) + if err != nil { + log.Fatal(ctx, err) + } + if len(failures) > 0 { + log.Fatal(ctx, failures[0]) + } + if len(targets) != 1 { + log.Fatal(ctx, "explain takes a single item; pass one id or a pair") + } + kind, id := targets[0].Kind, targets[0].ID + name, err := artworkItemName(ctx, ds, kind, id) if err != nil { log.Fatal(ctx, "Item not found", "kind", kind, "id", id, err) diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index b28d8dac5..8b50ba775 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -54,6 +54,74 @@ var _ = Describe("parseArtworkKind", func() { }) }) +var _ = Describe("resolveArtworkTargets", func() { + var ds *tests.MockDataStore + ctx := context.Background() + + BeforeEach(func() { + artists := tests.CreateMockArtistRepo() + artists.SetData(model.Artists{{ID: "artist1"}}) + ds = &tests.MockDataStore{MockedArtist: artists} + }) + + It("accepts the explicit leader shared by every id", func() { + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"al", "x", "y"}, explainKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(failures).To(BeEmpty()) + Expect(targets).To(Equal([]model.ArtworkID{ + {Kind: model.KindAlbumArtwork, ID: "x"}, {Kind: model.KindAlbumArtwork, ID: "y"}})) + }) + + It("rejects an explicit kind the command does not accept as a usage error", func() { + _, _, err := resolveArtworkTargets(ctx, ds, []string{"dc", "x"}, artwork.RefreshableKinds) + Expect(err).To(MatchError(ContainSubstring("invalid kind"))) + }) + + It("resolves a bare id by looking it up across tables", func() { + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1"}, explainKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(failures).To(BeEmpty()) + Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindArtistArtwork, ID: "artist1"}})) + }) + + It("reads the kind from a full artwork id prefix without a database lookup", func() { + targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum"}, explainKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "realalbum"}})) + }) + + It("strips the hash suffix from a full artwork id", func() { + targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum_0123456789abcdef"}, explainKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "realalbum"}})) + }) + + It("collects a self-describing arg whose kind the command does not accept", func() { + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"dc-realalbum:2"}, artwork.RefreshableKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(BeEmpty()) + Expect(failures).To(HaveLen(1)) + Expect(failures[0]).To(MatchError(ContainSubstring("invalid kind"))) + }) + + It("collects an id that matches nothing and has no kind prefix", func() { + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"nope"}, explainKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(BeEmpty()) + Expect(failures).To(HaveLen(1)) + Expect(failures[0]).To(MatchError(ContainSubstring("could not determine kind"))) + }) + + It("resolves the valid ids and collects the unresolvable ones", func() { + targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1", "nope", "al-realalbum"}, explainKinds) + Expect(err).ToNot(HaveOccurred()) + Expect(targets).To(Equal([]model.ArtworkID{ + {Kind: model.KindArtistArtwork, ID: "artist1"}, {Kind: model.KindAlbumArtwork, ID: "realalbum"}})) + Expect(failures).To(HaveLen(1)) + Expect(failures[0]).To(MatchError(ContainSubstring("could not determine kind"))) + }) +}) + var _ = Describe("explainResult", func() { It("reports the winning source", func() { steps := []artwork.TraceStep{{Candidate: "folder", Outcome: "hit", Detail: "/music/a.jpg"}} @@ -337,10 +405,10 @@ var _ = Describe("discArtworkName", func() { }) var _ = Describe("artwork refresh command", func() { - It("requires at least a kind and one id", func() { - Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"ar"})).To(HaveOccurred()) + It("requires at least one argument", func() { + Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{})).To(HaveOccurred()) + Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"id1"})).ToNot(HaveOccurred()) Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"ar", "id1"})).ToNot(HaveOccurred()) - Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"ar", "id1", "id2"})).ToNot(HaveOccurred()) }) }) @@ -873,7 +941,8 @@ var _ = Describe("refreshItems", func() { Expect(art.PutItemArtwork(&model.ItemArtwork{ItemKind: model.KindAlbumArtwork.Prefix(), ItemID: "al-1", ImageType: model.ImageTypePrimary, Hash: "abc123"})).To(Succeed()) - Expect(refreshItems(ctx, ds, model.KindAlbumArtwork, []string{"al-1", "al-3"}, &out)).To(BeZero()) + Expect(refreshItems(ctx, ds, []model.ArtworkID{ + {Kind: model.KindAlbumArtwork, ID: "al-1"}, {Kind: model.KindAlbumArtwork, ID: "al-3"}}, &out)).To(BeZero()) _, err := art.GetItemArtwork(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary) Expect(err).To(MatchError(model.ErrNotFound)) @@ -884,7 +953,7 @@ var _ = Describe("refreshItems", func() { }) It("skips an id that does not exist instead of queuing it", func() { - Expect(refreshItems(ctx, ds, model.KindAlbumArtwork, []string{"al-2"}, &out)).To(Equal(1)) + Expect(refreshItems(ctx, ds, []model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "al-2"}}, &out)).To(Equal(1)) _, err := queue.Get(model.KindAlbumArtwork, "al-2", model.ImageTypePrimary) Expect(err).To(MatchError(model.ErrNotFound), "a typo must not leave an orphan queue row") @@ -892,8 +961,8 @@ var _ = Describe("refreshItems", func() { }) It("continues past a failing id and counts the failures", func() { - Expect(refreshItems(ctx, ds, model.KindAlbumArtwork, - []string{"al-1", "al-2", "al-3"}, &out)).To(Equal(1)) + Expect(refreshItems(ctx, ds, []model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "al-1"}, + {Kind: model.KindAlbumArtwork, ID: "al-2"}, {Kind: model.KindAlbumArtwork, ID: "al-3"}}, &out)).To(Equal(1)) Expect(out.String()).To(Equal("al/al-1: queued\nal/al-3: queued\n"), "the ids after a failure are still refreshed") diff --git a/model/get_entity.go b/model/get_entity.go index 3e1a78d1d..e5d41f1be 100644 --- a/model/get_entity.go +++ b/model/get_entity.go @@ -7,21 +7,36 @@ import ( // TODO: Should the type be encoded in the ID? func GetEntityByID(ctx context.Context, ds DataStore, id string) (any, error) { - getters := []func() (any, error){ - func() (any, error) { return ds.Artist(ctx).Get(id) }, - func() (any, error) { return ds.Album(ctx).Get(id) }, - func() (any, error) { return ds.Playlist(ctx).Get(id) }, - func() (any, error) { return ds.MediaFile(ctx).Get(id) }, - func() (any, error) { return ds.Radio(ctx).Get(id) }, + entity, _, err := getEntity(ctx, ds, id) + return entity, err +} + +// GetEntityKindByID resolves a bare entity id to its artwork Kind, searching the same tables as +// GetEntityByID. It reports ErrNotFound when no entity owns the id. +func GetEntityKindByID(ctx context.Context, ds DataStore, id string) (Kind, error) { + _, kind, err := getEntity(ctx, ds, id) + return kind, err +} + +func getEntity(ctx context.Context, ds DataStore, id string) (any, Kind, error) { + getters := []struct { + kind Kind + get func() (any, error) + }{ + {KindArtistArtwork, func() (any, error) { return ds.Artist(ctx).Get(id) }}, + {KindAlbumArtwork, func() (any, error) { return ds.Album(ctx).Get(id) }}, + {KindPlaylistArtwork, func() (any, error) { return ds.Playlist(ctx).Get(id) }}, + {KindMediaFileArtwork, func() (any, error) { return ds.MediaFile(ctx).Get(id) }}, + {KindRadioArtwork, func() (any, error) { return ds.Radio(ctx).Get(id) }}, } - for _, get := range getters { - entity, err := get() + for _, g := range getters { + entity, err := g.get() if err == nil { - return entity, nil + return entity, g.kind, nil } if !errors.Is(err, ErrNotFound) { - return nil, err + return nil, Kind{}, err } } - return nil, ErrNotFound + return nil, Kind{}, ErrNotFound } diff --git a/model/get_entity_test.go b/model/get_entity_test.go index f8a4c9e8e..4e589406a 100644 --- a/model/get_entity_test.go +++ b/model/get_entity_test.go @@ -38,3 +38,25 @@ var _ = Describe("GetEntityByID", func() { Expect(err).ToNot(MatchError(model.ErrNotFound)) }) }) + +var _ = Describe("GetEntityKindByID", func() { + var ds *tests.MockDataStore + var ctx context.Context + + BeforeEach(func() { + ds = &tests.MockDataStore{} + ctx = GinkgoT().Context() + }) + + It("returns the artwork kind for the matching id", func() { + ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1"}}) + kind, err := model.GetEntityKindByID(ctx, ds, "a1") + Expect(err).ToNot(HaveOccurred()) + Expect(kind).To(Equal(model.KindAlbumArtwork)) + }) + + It("returns ErrNotFound when no entity matches", func() { + _, err := model.GetEntityKindByID(ctx, ds, "missing") + Expect(err).To(MatchError(model.ErrNotFound)) + }) +})