From 3d438b08efc705928a187b51042ca0778feeb900 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 15 Jul 2026 18:09:59 -0400 Subject: [PATCH] fix(jellyfin): close the unbounded playlist and search paths left by #5783 (#5784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(jellyfin): stream playlist tracks instead of loading every one PR #5783 left the playlist paths materializing: all three loaded every track of a playlist, whatever the client asked for. A playlist can be the whole library — a smart playlist matching everything — so this is the same OOM class that PR fixed for the other collections. Measured on a 96k-track smart playlist, /Items?ParentId=&Limit=10 peaked at 1.3GB to return ten tracks. Playlist tracks now stream from a cursor, like every other collection: - PlaylistTrackRepository gains GetCursor and CountAll, sharing the select builder with loadTracks so cursor rows hydrate identically, plus GetMediaFileIDs for callers that need every id but no track data. - playlists.Tracks(ctx, id) exposes that repo to the HTTP layer with visibility enforced, returning ErrNotFound rather than the repo's nil-and-log-a-warning (which /Items would hit on every album browse, since ParentId is usually not a playlist). - /Playlists/{id}/Items now honors StartIndex/Limit, which it silently ignored before — it always returned the whole playlist. Real Jellyfin pages it. - getPlaylist runs an id-only query: PlaylistInfo carries every track id so it can't be paged, but it no longer hydrates rows it discards. - The route joins the throttled group, as it's now cursor-backed. Measured against a copy of a 96k-track production DB, peak RSS over idle, with byte-for-byte identical responses on every endpoint: /Items?ParentId=&Limit=10 1275MB -> 1MB 8.8s -> 3.6s (0.27s warm) /Items?ParentId= unbounded 1353MB -> 8MB 8.7s -> 3.4s /Playlists//Items 1217MB -> 7MB 8.8s -> 3.4s /Playlists/ 1178MB -> 33MB 9.1s -> 3.8s TotalRecordCount now costs a count query where the old path got it from len(tracks): 6ms on the largest real playlist in that library (2638 tracks), 288ms on the synthetic all-96k one. The old path paid 1.3GB and 4s+ instead. * fix(jellyfin): bound unbounded /Items searches The other collections stream, so an unbounded one costs about one item of memory. Search can't: the repositories' Search returns a slice, so it materializes every match. PR #5783 left two ways to reach that. A whitespace-only SearchTerm was the first. " " != "", so it took the search path, where doSearch trims it back to empty and hits its "empty query, return everything in natural order" branch — with no LIMIT, since executeTwoPhase only applies one when Max > 0. The whole library, materialized. Trimming at the two parse sites makes the `search != ""` checks mean what they look like they mean: a blank term is not a search, so it takes the unfiltered streaming path, which still returns everything, exactly as real Jellyfin does for an empty term. A real search with no Limit was the second, and needs an actual bound. Search gets a default of 100 when the client sends no Limit — matching the DefaultSearchLimit in Jellyfin's unreleased SqlSearchProvider — plus a ceiling, without which Limit=999999 would still materialize the library. The default alone wouldn't have closed the hole. An explicit Limit under the ceiling is honored unclamped, as upstream does; truncating a search is safe in a way truncating /Items is not, since nothing syncs a library through searchTerm. Note this is upstream's own bug: v10.11's /Search/Hints returns the entire library for a whitespace-only term, which master fixed by switching to ThrowIfNullOrWhiteSpace. * fix(jellyfin): cap the search Limit the client asked for, not the merge window searchPage sees two different things in opts.Max: the client's Limit for a single-type query, and mergeTypes' internal offset+limit window for a multi-type one. Clamping there hit both, so a multi-type search paging past the ceiling fetched only `ceiling` rows of the first type and the merged page skipped into the next one — IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=2000 &Limit=1 returned an album instead of the 2001st song. The ceiling now applies where the client's Limit is read: queryItems (after the playlist branch, so a playlist parent's page isn't capped by a stray SearchTerm) and getArtists, which reads its own. A limit of 0 stays 0, keeping searchPage's default. What a deep page materializes is then bounded by the client's StartIndex, as it already was for any multi-type query, search or not. Also from review: the playlist-track mock reused the previously stored Options when called without any, so a later no-args call inherited stale paging. * fix(jellyfin): apply the search default to the client's Limit, not per type The default lived in searchPage, which runs per type and after mergeTypes has already picked its branch. So an unbounded multi-type search left q.limit at 0, mergeTypes took its chained branch, and StartIndex was applied to a list each type had already truncated to the default — dropping matches rather than paging them. With 200 matching songs, IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song &StartIndex=150 returned nothing at all, and without StartIndex it returned the default per type instead of in total. clampSearchLimit now applies the default and the ceiling together, to the client's Limit, at the two places it's read (queryItems and getArtists). A search therefore always gives mergeTypes a real window, so it pages the merged result and cuts it once, at the end. * fix(jellyfin): bound the multi-type search window against StartIndex mergeTypes asks each type for offset+limit rows before paginating the merged list, so a search still materialized whatever StartIndex asked for: IncludeItemTypes=Audio,MusicAlbum&SearchTerm=x&StartIndex=500000&Limit=1 pulled ~500001 matches per type. Bounding the window alone isn't enough — below it the merged rows are the client's page, but at it a truncated type is followed by the next one's rows, which is what made a clamped window serve an album where the 2001st song belonged. So the window is capped at maxSearchLimit and the page is clipped to it: pages below the ceiling are served in full and unchanged, a page straddling it is cut at it, and past it the result is empty rather than another type's rows. The total reports what can actually be paged to, so a client stops instead of asking for pages that no longer exist. This is the merge's own limit, not the client's: a single-type search still pages as deep as it likes, since its offset goes to SQL. Non-search multi-type queries keep the unbounded offset+limit window, which predates this and wants the same treatment via CountAll (exact per-type totals let whole types be skipped) rather than a cap. * fix(jellyfin): advertise the pageable search total, not the page window Clipping the multi-type search total to `window` clipped it to StartIndex+Limit on an ordinary page, so a first page of Limit=10 reported TotalRecordCount 10 however many matches there were, and a client paging on the total stopped after one page. The cap belongs at the ceiling — what can be paged to overall — not at the current page. The tests missed it because the only one asserting a total used StartIndex=maxSearchLimit, where the window happens to equal the ceiling. * refactor(jellyfin): fold the search clamp into clampLimit and simplify mergeTypes Cleanup pass over the branch, no behaviour change: - clampSearchLimit was clampLimit (similar.go) with different constants, so the latter takes the default and ceiling as arguments and both call it. Similar's default moves out of three IntOr calls into defaultSimilarLimit. - mergeTypes derived a second `limit` and needed an early return for the empty page, only because paginate reads 0 as "unbounded". Clipping the merged slice to the window instead lets q.limit be passed straight through: window is min(offset+limit, ceiling), so clipping there is the same cut. - playlistTracks returned (result, handled, error) where handled=false always meant error=nil. Splitting the lookup out gives playlistTracksRepo returning (repo, ok), and the nilerr suppression goes with it. - The comment on playlists.Tracks blamed the log warning for its extra Get; the reason is that PlaylistRepository.Tracks discards the error behind a nil. Also drops a stale reference to a renamed variable. --- core/playlists/playlists.go | 16 ++ core/playlists/playlists_test.go | 22 ++ model/playlist.go | 5 + persistence/playlist_repository.go | 12 +- persistence/playlist_track_repository.go | 32 +++ persistence/playlist_track_repository_test.go | 61 +++++ server/jellyfin/api.go | 2 +- server/jellyfin/browsing.go | 5 +- server/jellyfin/browsing_test.go | 17 ++ server/jellyfin/items.go | 84 +++++-- server/jellyfin/items_test.go | 208 ++++++++++++++++++ server/jellyfin/playlists.go | 49 ++++- server/jellyfin/playlists_test.go | 46 +++- server/jellyfin/similar.go | 20 +- tests/mock_album_repo.go | 2 + tests/mock_playlist_track_repo.go | 64 +++++- 16 files changed, 591 insertions(+), 54 deletions(-) create mode 100644 persistence/playlist_track_repository_test.go diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index 3da24706c..1ef083bbb 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -22,6 +22,7 @@ type Playlists interface { GetAll(ctx context.Context, options ...model.QueryOptions) (model.Playlists, error) Get(ctx context.Context, id string) (*model.Playlist, error) GetWithTracks(ctx context.Context, id string) (*model.Playlist, error) + Tracks(ctx context.Context, id string) (model.PlaylistTrackRepository, error) GetPlaylists(ctx context.Context, mediaFileId string) (model.Playlists, error) // Mutations @@ -98,6 +99,21 @@ func (s *playlists) GetPlaylists(ctx context.Context, mediaFileId string) (model return s.ds.Playlist(ctx).GetPlaylists(mediaFileId) } +// Tracks scopes a repository to one playlist's tracks, for callers that page or stream them rather +// than loading every one like GetWithTracks. Gets first because PlaylistRepository.Tracks discards +// its error behind a nil (and warns), and this is probed with ids that are usually not playlists. +func (s *playlists) Tracks(ctx context.Context, id string) (model.PlaylistTrackRepository, error) { + repo := s.ds.Playlist(ctx) + if _, err := repo.Get(id); err != nil { + return nil, err + } + tracks := repo.Tracks(id, true) + if tracks == nil { + return nil, model.ErrNotFound + } + return tracks, nil +} + // --- Mutation operations --- // Create creates a new playlist (when name is provided) or replaces tracks on an existing diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go index f849a0a21..0c9674bed 100644 --- a/core/playlists/playlists_test.go +++ b/core/playlists/playlists_test.go @@ -73,6 +73,28 @@ var _ = Describe("Playlists", func() { }) }) + Describe("Tracks", func() { + var mockTracks *tests.MockPlaylistTrackRepo + + BeforeEach(func() { + mockTracks = &tests.MockPlaylistTrackRepo{} + mockPlsRepo.Data = map[string]*model.Playlist{ + "pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"}, + } + mockPlsRepo.TracksRepo = mockTracks + ps = playlists.NewPlaylists(ds, core.NewImageUploadService()) + }) + + It("returns the playlist's track repository", func() { + Expect(ps.Tracks(ctx, "pls-1")).To(BeIdenticalTo(mockTracks)) + }) + + It("returns ErrNotFound for an unknown or invisible playlist", func() { + _, err := ps.Tracks(ctx, "nonexistent") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) + Describe("Create", func() { BeforeEach(func() { mockPlsRepo.Data = map[string]*model.Playlist{ diff --git a/model/playlist.go b/model/playlist.go index f2586f52d..40adb8d0a 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -157,10 +157,15 @@ func (plt PlaylistTracks) MediaFiles() MediaFiles { return mfs } +type PlaylistTrackCursor iter.Seq2[PlaylistTrack, error] + type PlaylistTrackRepository interface { ResourceRepository + CountAll(options ...QueryOptions) (int64, error) GetAll(options ...QueryOptions) (PlaylistTracks, error) + GetCursor(options ...QueryOptions) (PlaylistTrackCursor, error) GetAlbumIDs(options ...QueryOptions) ([]string, error) + GetMediaFileIDs(options ...QueryOptions) ([]string, error) Add(mediaFileIds []string) (int, error) AddAlbums(albumIds []string) (int, error) AddArtists(artistIds []string) (int, error) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 9626aad6a..e39f0bbd3 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -298,10 +298,11 @@ func (r *playlistRepository) refreshCounters(pls *model.Playlist) error { return nil } -func (r *playlistRepository) loadTracks(sel SelectBuilder, id string) (model.PlaylistTracks, error) { - sel = r.applyLibraryFilter(sel, "f") +// tracksQuery is shared by loadTracks and GetCursor, so both hydrate rows identically. +func (r *playlistRepository) tracksQuery(query SelectBuilder, id string) SelectBuilder { + query = r.applyLibraryFilter(query, "f") userID := loggedUser(r.ctx).ID - tracksQuery := sel. + return query. Columns( "coalesce(starred, 0) as starred", "starred_at", @@ -321,8 +322,11 @@ func (r *playlistRepository) loadTracks(sel SelectBuilder, id string) (model.Pla Join("media_file f on f.id = media_file_id"). Join("library on f.library_id = library.id"). Where(Eq{"playlist_id": id}) +} + +func (r *playlistRepository) loadTracks(query SelectBuilder, id string) (model.PlaylistTracks, error) { tracks := dbPlaylistTracks{} - err := r.queryAll(tracksQuery, &tracks) + err := r.queryAll(r.tracksQuery(query, id), &tracks) if err != nil { return nil, err } diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 1a7062cc2..e51ff8ea6 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -77,6 +77,14 @@ func (r *playlistRepository) Tracks(playlistId string, refreshSmartPlaylist bool return p } +func (r *playlistTrackRepository) CountAll(options ...model.QueryOptions) (int64, error) { + query := Select(). + Join("media_file f on f.id = media_file_id"). + Where(Eq{"playlist_id": r.playlistId}) + query = r.applyLibraryFilter(query, "f") + return r.count(query, options...) +} + func (r *playlistTrackRepository) Count(options ...rest.QueryOptions) (int64, error) { query := Select(). LeftJoin("media_file f on f.id = media_file_id"). @@ -116,6 +124,30 @@ func (r *playlistTrackRepository) GetAll(options ...model.QueryOptions) (model.P return tracks, err } +func (r *playlistTrackRepository) GetCursor(options ...model.QueryOptions) (model.PlaylistTrackCursor, error) { + sel := r.playlistRepo.tracksQuery(r.newSelect(options...), r.playlistId) + cursor, err := queryWithStableResults[dbPlaylistTrack](r.sqlRepository, sel) + if err != nil { + return nil, err + } + return model.PlaylistTrackCursor(wrapCursor(cursor, func(t dbPlaylistTrack) *model.PlaylistTrack { + return t.PlaylistTrack + })), nil +} + +// GetMediaFileIDs returns the tracks' song ids, for callers that need every id but no track data. +func (r *playlistTrackRepository) GetMediaFileIDs(options ...model.QueryOptions) ([]string, error) { + query := r.newSelect(options...).Columns("media_file_id"). + Join("media_file f on f.id = media_file_id"). + Where(Eq{"playlist_id": r.playlistId}) + query = r.applyLibraryFilter(query, "f") + var ids []string + if err := r.queryAllSlice(query, &ids); err != nil { + return nil, err + } + return ids, nil +} + func (r *playlistTrackRepository) GetAlbumIDs(options ...model.QueryOptions) ([]string, error) { query := r.newSelect(options...).Columns("distinct mf.album_id"). Join("media_file mf on mf.id = media_file_id"). diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go new file mode 100644 index 000000000..36f9ae4a9 --- /dev/null +++ b/persistence/playlist_track_repository_test.go @@ -0,0 +1,61 @@ +package persistence + +import ( + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("PlaylistTrackRepository", func() { + var repo model.PlaylistTrackRepository + + BeforeEach(func() { + ctx := log.NewContext(GinkgoT().Context()) + ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "userid", IsAdmin: true}) + repo = NewPlaylistRepository(ctx, GetDBXBuilder()).Tracks(plsBest.ID, true) + }) + + Describe("GetCursor", func() { + It("yields the same tracks as GetAll", func() { + opts := model.QueryOptions{Sort: "id"} + want, err := repo.GetAll(opts) + Expect(err).ToNot(HaveOccurred()) + Expect(want).To(HaveLen(2)) + + Expect(collectCursor(repo.GetCursor(opts))).To(Equal([]model.PlaylistTrack(want))) + }) + + It("honors Max and Offset", func() { + opts := model.QueryOptions{Sort: "id", Max: 1, Offset: 1} + want, err := repo.GetAll(opts) + Expect(err).ToNot(HaveOccurred()) + Expect(want).To(HaveLen(1)) + + Expect(collectCursor(repo.GetCursor(opts))).To(Equal([]model.PlaylistTrack(want))) + }) + }) + + Describe("CountAll", func() { + It("returns the number of tracks in the playlist", func() { + Expect(repo.CountAll()).To(Equal(int64(2))) + }) + + It("ignores Max and Offset", func() { + Expect(repo.CountAll(model.QueryOptions{Max: 1, Offset: 1})).To(Equal(int64(2))) + }) + }) + + Describe("GetMediaFileIDs", func() { + It("returns the song ids in playlist order", func() { + Expect(repo.GetMediaFileIDs(model.QueryOptions{Sort: "id"})). + To(Equal([]string{songDayInALife.ID, songRadioactivity.ID})) + }) + + It("honors Max and Offset", func() { + Expect(repo.GetMediaFileIDs(model.QueryOptions{Sort: "id", Max: 1, Offset: 1})). + To(Equal([]string{songRadioactivity.ID})) + }) + }) +}) diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index d169a4c83..e94d64e85 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -102,6 +102,7 @@ func (api *Router) routes() http.Handler { r.Get("/Users/{userId}/Items/Latest", api.getLatest) r.Get("/Artists", api.getArtists) r.Get("/Artists/AlbumArtists", api.getAlbumArtists) + r.Get("/Playlists/{playlistId}/Items", api.getPlaylistItems) }) r.Get("/Items/{itemId}", api.getItem) @@ -131,7 +132,6 @@ func (api *Router) routes() http.Handler { r.Post("/Playlists", api.createPlaylist) r.Get("/Playlists/{playlistId}", api.getPlaylist) r.Post("/Playlists/{playlistId}", api.updatePlaylist) - r.Get("/Playlists/{playlistId}/Items", api.getPlaylistItems) r.Post("/Playlists/{playlistId}/Items", api.addToPlaylist) r.Delete("/Playlists/{playlistId}/Items", api.removeFromPlaylist) r.Get("/Playlists/{playlistId}/Users", api.getPlaylistUsers) diff --git a/server/jellyfin/browsing.go b/server/jellyfin/browsing.go index d5a00e492..fae21fc0b 100644 --- a/server/jellyfin/browsing.go +++ b/server/jellyfin/browsing.go @@ -33,7 +33,10 @@ func (api *Router) listArtistsByRole(w http.ResponseWriter, r *http.Request, rol q := itemsQuery{ scopeIDs: scopeIDs, genreIds: decodedQueryIDs(r, "genreids"), - search: p.StringOr("searchterm", ""), + search: searchTerm(p), + } + if q.search != "" { + opts.Max = clampLimit(opts.Max, defaultSearchLimit, maxSearchLimit) } res, err := api.listArtists(ctx, opts, q, role) diff --git a/server/jellyfin/browsing_test.go b/server/jellyfin/browsing_test.go index 7f50355e1..660e5d293 100644 --- a/server/jellyfin/browsing_test.go +++ b/server/jellyfin/browsing_test.go @@ -111,6 +111,23 @@ var _ = Describe("Browsing", func() { Expect(res.Items).To(HaveLen(1)) }) + It("bounds a search the client left unbounded, and clamps an oversized one", func() { + artistRepo := ds.Artist(context.Background()).(*tests.MockArtistRepo) + artistRepo.SetData(model.Artists{{ID: "ar1", Name: "Artist"}}) + + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Artists?SearchTerm=art", nil).WithContext(ctxUser(model.Libraries{{ID: 1}})) + invoke(api.getArtists, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(artistRepo.Options.Max).To(Equal(defaultSearchLimit + 1)) + + w = httptest.NewRecorder() + r = httptest.NewRequest("GET", "/Artists?SearchTerm=art&Limit=999999", nil).WithContext(ctxUser(model.Libraries{{ID: 1}})) + invoke(api.getArtists, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(artistRepo.Options.Max).To(Equal(maxSearchLimit + 1)) + }) + It("forwards StartIndex/Limit as Offset/Max", func() { artistRepo := ds.Artist(context.Background()).(*tests.MockArtistRepo) artistRepo.SetData(model.Artists{{ID: "ar1", Name: "Artist"}}) diff --git a/server/jellyfin/items.go b/server/jellyfin/items.go index 0dca48938..ff38b6491 100644 --- a/server/jellyfin/items.go +++ b/server/jellyfin/items.go @@ -24,6 +24,12 @@ import ( // album, artist and media_file). var notMissing = squirrel.Eq{"missing": false} +// searchTerm trims, so a whitespace-only term is not a search: doSearch would read it as "match +// everything" and materialize the library, where the unfiltered path streams. +func searchTerm(p *req.Values) string { + return strings.TrimSpace(p.StringOr("searchterm", "")) +} + func (api *Router) getItems(w http.ResponseWriter, r *http.Request) { res, err := api.queryItems(r.Context(), r) if err != nil { @@ -226,7 +232,7 @@ func (api *Router) parseItemsQuery(ctx context.Context, r *http.Request) itemsQu fields: dto.ParseFields(p.StringOr("fields", "")), ids: decodedQueryIDs(r, "ids"), rawTypes: p.StringOr("includeitemtypes", ""), - search: p.StringOr("searchterm", ""), + search: searchTerm(p), sortBy: p.StringOr("sortby", ""), sortOrder: p.StringOr("sortorder", ""), offset: p.IntOr("startindex", 0), @@ -281,8 +287,11 @@ func (api *Router) queryItems(ctx context.Context, r *http.Request) (itemsResult case strings.Contains(q.rawTypes, "ManualPlaylistsFolder"): return materialized(result([]dto.BaseItemDto{playlistsFolder()}, 1, 0)), nil } - if res, ok := api.playlistTracks(ctx, q); ok { - return res, nil + if repo, ok := api.playlistTracksRepo(ctx, q); ok { + return api.playlistTrackPage(repo, q.fields, q.offset, q.limit) + } + if q.search != "" { + q.limit = clampLimit(q.limit, defaultSearchLimit, maxSearchLimit) } if len(q.types) == 1 { opts := model.QueryOptions{Offset: q.offset, Max: q.limit} @@ -292,32 +301,39 @@ func (api *Router) queryItems(ctx context.Context, r *http.Request) (itemsResult return api.mergeTypes(ctx, q) } -// playlistTracks resolves a playlist parent to its tracks, whatever IncludeItemTypes says: Jellify -// opens a playlist with ParentId=&IncludeItemTypes=Audio, and routing that through -// listSongs would treat the playlist id as an album id and return nothing. -func (api *Router) playlistTracks(ctx context.Context, q itemsQuery) (itemsResult, bool) { +// playlistTracksRepo resolves a playlist parent, whatever IncludeItemTypes says: Jellify opens a +// playlist with ParentId=&IncludeItemTypes=Audio, and routing that through listSongs would +// treat the playlist id as an album id and return nothing. +// +// ok is false when ParentId isn't a visible playlist, so the caller falls through to the type +// dispatch: ParentId is usually an album or artist. +func (api *Router) playlistTracksRepo(ctx context.Context, q itemsQuery) (model.PlaylistTrackRepository, bool) { if q.parentId == "" || q.isLibraryParent || q.parentId == playlistsFolderID { - return itemsResult{}, false + return nil, false } - pls, err := api.playlists.GetWithTracks(ctx, q.parentId) - if err != nil { - return itemsResult{}, false - } - // GetWithTracks enforces visibility (public or owned by the current user). - items := slice.Map(pls.Tracks, func(t model.PlaylistTrack) dto.BaseItemDto { return trackToBaseItem(t, q.fields) }) - return materialized(result(paginate(items, q.offset, q.limit), len(items), q.offset)), true + // Tracks enforces visibility. + repo, err := api.playlists.Tracks(ctx, q.parentId) + return repo, err == nil } func (api *Router) mergeTypes(ctx context.Context, q itemsQuery) (itemsResult, error) { // Each per-type query needs at most offset+limit rows (the worst case where one type fills the // whole [offset, offset+limit) window). Totals are unaffected — they come from CountAll. + window := 0 + if q.limit > 0 { + window = q.offset + q.limit + } + // A search can't stream, so the window is what each type materializes and StartIndex would drive + // it without bound. Only below the window are the merged rows the true order, hence the clip + // below too. Non-search stays unbounded in StartIndex: a known gap, fixable with per-type counts. + if q.search != "" { + window = min(window, maxSearchLimit) + } var results []itemsResult total := 0 for _, itemType := range q.types { var opts model.QueryOptions - if q.limit > 0 { - opts.Max = q.offset + q.limit - } + opts.Max = window applySort(&opts, itemType, q.sortBy, q.sortOrder) res, err := api.queryItemsOfType(ctx, itemType, opts, q) if err != nil { @@ -339,6 +355,13 @@ func (api *Router) mergeTypes(ctx context.Context, q itemsQuery) (itemsResult, e } items = append(items, typeItems...) } + if q.search != "" { + // Past the window the merged order isn't the true one, so drop it rather than serve another + // type's rows. The total is what's pageable overall, not this page, or a client paging on it + // would stop after the first page. + items = items[:min(window, len(items))] + total = min(total, maxSearchLimit) + } return materialized(result(paginate(items, q.offset, q.limit), total, q.offset)), nil } @@ -412,20 +435,37 @@ func paginate(items []dto.BaseItemDto, offset, limit int) []dto.BaseItemDto { return items } +// Search can't stream (Search returns a slice), so it needs both a default and a ceiling: without +// the ceiling, Limit=999999 still materializes every match. +const ( + defaultSearchLimit = 100 + maxSearchLimit = 2000 +) + +// clampLimit bounds a client-supplied limit, 0 or less meaning it sent none, so it can't drive an +// oversized allocation or provider fetch (flagged by CodeQL as a user-controlled allocation size). +// +// Searches clamp their Limit here rather than in searchPage, which also sees mergeTypes' larger +// offset+limit window: bounding that would truncate each type before the merged page is cut. +func clampLimit(limit, def, ceiling int) int { + if limit <= 0 { + return def + } + return min(limit, ceiling) +} + // searchPage runs a repository Search fetching one extra row to derive TotalRecordCount, since the // Search API returns no match count and CountAll can't see the search term. offset+len(rows) is // exact once matches end (and a growing lower bound before), so paging terminates at the last match. func searchPage[S ~[]E, E any](opts model.QueryOptions, search func(model.QueryOptions) (S, error)) (S, int, error) { fetch := opts - if fetch.Max > 0 { - fetch.Max++ - } + fetch.Max++ rows, err := search(fetch) if err != nil { return nil, 0, err } total := opts.Offset + len(rows) - if opts.Max > 0 && len(rows) > opts.Max { + if len(rows) > opts.Max { rows = rows[:opts.Max] } return rows, total, nil diff --git a/server/jellyfin/items_test.go b/server/jellyfin/items_test.go index 801db9e3a..049151651 100644 --- a/server/jellyfin/items_test.go +++ b/server/jellyfin/items_test.go @@ -3,6 +3,7 @@ package jellyfin import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" @@ -71,6 +72,59 @@ var _ = Describe("Items", func() { Expect(res.Items[0].Id).To(Equal(dto.EncodeID("s1"))) }) + It("lists a playlist's tracks when ParentId is a playlist, whatever the type", func() { + fp.getPls = &model.Playlist{ID: "pl1", Tracks: model.PlaylistTracks{ + {ID: "1", MediaFileID: "s1", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s1"}}, + {ID: "2", MediaFileID: "s2", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s2"}}, + }} + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?ParentId="+dto.EncodeID("pl1")+"&IncludeItemTypes=Audio", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).To(HaveLen(2)) + Expect(res.Items[0].Id).To(Equal(dto.EncodeID("s1"))) + Expect(res.Items[0].PlaylistItemId).To(Equal(dto.EncodeID("1"))) + Expect(res.TotalRecordCount).To(Equal(2)) + }) + + It("pages a playlist parent's tracks in the query, not in memory", func() { + fp.getPls = &model.Playlist{ID: "pl1", Tracks: model.PlaylistTracks{ + {ID: "1", MediaFileID: "s1", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s1"}}, + {ID: "2", MediaFileID: "s2", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s2"}}, + {ID: "3", MediaFileID: "s3", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s3"}}, + }} + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?ParentId="+dto.EncodeID("pl1")+"&StartIndex=1&Limit=1", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.TotalRecordCount).To(Equal(3)) + Expect(res.Items).To(HaveLen(1)) + Expect(res.Items[0].Id).To(Equal(dto.EncodeID("s2"))) + Expect(fp.tracksRepo.Options.Offset).To(Equal(1)) + Expect(fp.tracksRepo.Options.Max).To(Equal(1)) + }) + + It("falls through to the type dispatch when ParentId is not a playlist", func() { + fp.getErr = model.ErrNotFound + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{{ID: "s1", AlbumID: "a1"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?ParentId="+dto.EncodeID("a1")+"&IncludeItemTypes=Audio", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).To(HaveLen(1)) + Expect(res.Items[0].Id).To(Equal(dto.EncodeID("s1"))) + }) + It("returns 500 when the song cursor fails to open, instead of a truncated 200", func() { ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetError(true) w := httptest.NewRecorder() @@ -220,6 +274,160 @@ var _ = Describe("Items", func() { Expect(res.Items).To(HaveLen(1)) }) + It("caps a search the client left unbounded", func() { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SearchTerm=one", nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(albumRepo.Options.Max).To(Equal(defaultSearchLimit + 1)) + }) + + It("honors an explicit search Limit up to the ceiling", func() { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SearchTerm=one&Limit=500", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(albumRepo.Options.Max).To(Equal(501)) + }) + + It("clamps a search Limit that would materialize the library", func() { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SearchTerm=one&Limit=999999", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(albumRepo.Options.Max).To(Equal(maxSearchLimit + 1)) + }) + + It("treats an all-whitespace SearchTerm as no search, streaming the unfiltered list", func() { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: "a1", Name: "One"}, {ID: "a2", Name: "Two"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=MusicAlbum&SearchTerm=%20%20", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).To(HaveLen(2)) + Expect(albumRepo.SearchQuery).To(BeEmpty()) + }) + + It("reports a multi-type search total past the page, so clients keep paging", func() { + songs := make(model.MediaFiles, defaultSearchLimit*2) + for i := range songs { + songs[i] = model.MediaFile{ID: fmt.Sprintf("s%05d", i), Title: "Song"} + } + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(songs) + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&Limit=10", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).To(HaveLen(10)) + Expect(res.TotalRecordCount).To(BeNumerically(">", 10)) + }) + + It("bounds the multi-type search window however large StartIndex is", func() { + albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo) + albumRepo.SetData(model.Albums{{ID: "a1", Name: "One"}}) + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{{ID: "s1", Title: "Song"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=500000&Limit=1", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + // Without the bound this asks each type for ~500001 rows. + Expect(albumRepo.Options.Max).To(Equal(maxSearchLimit + 1)) + }) + + It("stops a multi-type search at the ceiling rather than serving another type's rows", func() { + // Bounding the per-type window is what keeps StartIndex from driving it without limit, and + // past that window the merged order is no longer the true one. + songs := make(model.MediaFiles, maxSearchLimit+1) + for i := range songs { + songs[i] = model.MediaFile{ID: fmt.Sprintf("s%05d", i), Title: "Song"} + } + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(songs) + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", + fmt.Sprintf("/Items?IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=%d&Limit=1", maxSearchLimit), + nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).To(BeEmpty()) + Expect(res.TotalRecordCount).To(Equal(maxSearchLimit)) + }) + + It("serves the last page below the ceiling in full", func() { + songs := make(model.MediaFiles, maxSearchLimit+1) + for i := range songs { + songs[i] = model.MediaFile{ID: fmt.Sprintf("s%05d", i), Title: "Song"} + } + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(songs) + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", + fmt.Sprintf("/Items?IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=%d&Limit=10", maxSearchLimit-1), + nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + // Clipped to the window, and still the real row at that index — not the album behind it. + Expect(res.Items).To(HaveLen(1)) + Expect(res.Items[0].Id).To(Equal(dto.EncodeID(songs[maxSearchLimit-1].ID))) + }) + + It("bounds an unbounded multi-type search to the default in total, not per type", func() { + songs := make(model.MediaFiles, defaultSearchLimit*2) + for i := range songs { + songs[i] = model.MediaFile{ID: fmt.Sprintf("s%05d", i), Title: "Song"} + } + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(songs) + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song", nil). + WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).To(HaveLen(defaultSearchLimit)) + }) + + It("pages an unbounded multi-type search past the default without dropping matches", func() { + songs := make(model.MediaFiles, defaultSearchLimit*2) + for i := range songs { + songs[i] = model.MediaFile{ID: fmt.Sprintf("s%05d", i), Title: "Song"} + } + ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(songs) + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", + fmt.Sprintf("/Items?IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=%d", defaultSearchLimit+50), + nil).WithContext(ctxUser()) + invoke(api.getItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.Items).ToNot(BeEmpty()) + Expect(res.Items[0].Id).To(Equal(dto.EncodeID(songs[defaultSearchLimit+50].ID))) + }) + It("reports a search total beyond the fetched page instead of the page length", func() { ds.Artist(context.Background()).(*tests.MockArtistRepo).SetData(model.Artists{ {ID: "r1", Name: "Alpha"}, {ID: "r2", Name: "Beta"}, {ID: "r3", Name: "Gamma"}, diff --git a/server/jellyfin/playlists.go b/server/jellyfin/playlists.go index ffc2c6543..a157aae2e 100644 --- a/server/jellyfin/playlists.go +++ b/server/jellyfin/playlists.go @@ -125,6 +125,21 @@ func (api *Router) clearPlaylist(ctx context.Context, id string) error { return api.playlists.RemoveTracks(ctx, id, entryIDs) } +// playlistTrackPage streams one page of a playlist's tracks. Streams because a playlist can be the +// whole library (a smart playlist matching everything) and clients may omit Limit. Excludes missing +// tracks, and counts the same set, like GetWithTracks. +func (api *Router) playlistTrackPage(repo model.PlaylistTrackRepository, fields dto.Fields, offset, limit int) (itemsResult, error) { + total, err := repo.CountAll(model.QueryOptions{Filters: notMissing}) + if err != nil { + return itemsResult{}, err + } + opts := model.QueryOptions{Sort: "id", Offset: offset, Max: limit, Filters: notMissing} + open := streamCursor(func() (func(func(model.PlaylistTrack, error) bool), error) { + return repo.GetCursor(opts) + }, func(t model.PlaylistTrack) dto.BaseItemDto { return trackToBaseItem(t, fields) }) + return streamed(open, int(total), offset), nil +} + // trackToBaseItem maps a playlist entry to a BaseItemDto, tagging it with PlaylistItemId (the // entry's id, model.PlaylistTrack.ID, not the song id). Clients echo it back via // DELETE .../Items?EntryIds= to remove a specific occurrence, so duplicates of the same song remain @@ -136,17 +151,28 @@ func trackToBaseItem(t model.PlaylistTrack, fields dto.Fields) dto.BaseItemDto { } // getPlaylist returns a playlist's visibility flag and item ids (Finamp reads OpenAccess before the -// edit screen). GetWithTracks enforces visibility; any error maps to 404 so private playlists can't +// edit screen). Get and Tracks enforce visibility; any error maps to 404 so private playlists can't // be probed. func (api *Router) getPlaylist(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := dto.DecodeID(chi.URLParam(r, "playlistId")) - pls, err := api.playlists.GetWithTracks(ctx, id) + pls, err := api.playlists.Get(ctx, id) if err != nil { http.Error(w, "Not Found", http.StatusNotFound) return } - itemIds := slice.Map(pls.Tracks, func(t model.PlaylistTrack) string { return dto.EncodeID(t.MediaFileID) }) + repo, err := api.playlists.Tracks(ctx, id) + if err != nil { + http.Error(w, "Not Found", http.StatusNotFound) + return + } + // PlaylistInfo carries every track id, so this can't be paged — but it needs no track data. + trackIDs, err := repo.GetMediaFileIDs(model.QueryOptions{Sort: "id", Filters: notMissing}) + if err != nil { + api.internalError(w, r, err) + return + } + itemIds := slice.Map(trackIDs, dto.EncodeID) api.ok(w, r, dto.PlaylistInfo{ OpenAccess: pls.Public, Shares: []dto.PlaylistUserPermissions{}, @@ -154,19 +180,24 @@ func (api *Router) getPlaylist(w http.ResponseWriter, r *http.Request) { }) } -// getPlaylistItems relies on GetWithTracks to enforce visibility; any error maps to a generic 404 so -// a playlist id can't probe for private playlists. +// getPlaylistItems relies on Tracks to enforce visibility; any error maps to a generic 404 so a +// playlist id can't probe for private playlists. func (api *Router) getPlaylistItems(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := dto.DecodeID(chi.URLParam(r, "playlistId")) - pls, err := api.playlists.GetWithTracks(ctx, id) + repo, err := api.playlists.Tracks(ctx, id) if err != nil { http.Error(w, "Not Found", http.StatusNotFound) return } - fields := dto.ParseFields(req.Params(r).StringOr("fields", "")) - items := slice.Map(pls.Tracks, func(t model.PlaylistTrack) dto.BaseItemDto { return trackToBaseItem(t, fields) }) - api.ok(w, r, dto.QueryResult{Items: items, TotalRecordCount: len(items)}) + p := req.Params(r) + fields := dto.ParseFields(p.StringOr("fields", "")) + res, err := api.playlistTrackPage(repo, fields, p.IntOr("startindex", 0), p.IntOr("limit", 0)) + if err != nil { + api.internalError(w, r, err) + return + } + api.ok(w, r, res) } // queryIDs reads an id-list query param that clients spell two ways: comma-separated in a single diff --git a/server/jellyfin/playlists_test.go b/server/jellyfin/playlists_test.go index 804123003..27264b280 100644 --- a/server/jellyfin/playlists_test.go +++ b/server/jellyfin/playlists_test.go @@ -29,8 +29,9 @@ type fakePlaylists struct { createdIds []string createErr error - getPls *model.Playlist - getErr error + getPls *model.Playlist + getErr error + tracksRepo *tests.MockPlaylistTrackRepo getByIDPls *model.Playlist getByIDErr error @@ -92,6 +93,20 @@ func (f *fakePlaylists) GetWithTracks(_ context.Context, _ string) (*model.Playl return f.getPls, nil } +// Tracks serves the same getPls fixture as GetWithTracks. tracksRepo is kept so tests can assert +// what was pushed down to the query. +func (f *fakePlaylists) Tracks(_ context.Context, _ string) (model.PlaylistTrackRepository, error) { + if f.getErr != nil { + return nil, f.getErr + } + if f.getPls == nil { + return nil, model.ErrNotFound + } + f.tracksRepo = &tests.MockPlaylistTrackRepo{} + f.tracksRepo.SetData(f.getPls.Tracks) + return f.tracksRepo, nil +} + func (f *fakePlaylists) AddTracks(_ context.Context, playlistID string, ids []string) (int, error) { f.addPlaylistID = playlistID f.addIds = ids @@ -184,6 +199,30 @@ var _ = Describe("Playlists", func() { Expect(res.Items[1].PlaylistItemId).To(Equal(dto.EncodeID("2"))) }) + It("pages with StartIndex/Limit, pushing them down to the query", func() { + fp.getPls = &model.Playlist{ + ID: "pl1", + Tracks: model.PlaylistTracks{ + {ID: "1", MediaFileID: "s1", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s1"}}, + {ID: "2", MediaFileID: "s2", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s2"}}, + {ID: "3", MediaFileID: "s3", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s3"}}, + }, + } + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Playlists/pl1/Items?StartIndex=1&Limit=1", nil). + WithContext(context.Background()) + r = withChiURLParam(r, "playlistId", "pl1") + invoke(api.getPlaylistItems, w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.QueryResult + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res.TotalRecordCount).To(Equal(3)) + Expect(res.Items).To(HaveLen(1)) + Expect(res.Items[0].Id).To(Equal(dto.EncodeID("s2"))) + Expect(fp.tracksRepo.Options.Offset).To(Equal(1)) + Expect(fp.tracksRepo.Options.Max).To(Equal(1)) + }) + It("returns 404 for a non-owned or absent playlist", func() { fp.getErr = model.ErrNotFound w := httptest.NewRecorder() @@ -247,7 +286,7 @@ var _ = Describe("Playlists", func() { Describe("getPlaylist", func() { It("returns OpenAccess from Public and item ids (encoded media file ids, not entry ids)", func() { - fp.getPls = &model.Playlist{ + pls := &model.Playlist{ ID: "pl1", Public: true, Tracks: model.PlaylistTracks{ @@ -255,6 +294,7 @@ var _ = Describe("Playlists", func() { {ID: "2", MediaFileID: "s2", PlaylistID: "pl1", MediaFile: model.MediaFile{ID: "s2"}}, }, } + fp.getPls, fp.getByIDPls = pls, pls w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/Playlists/pl1", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") diff --git a/server/jellyfin/similar.go b/server/jellyfin/similar.go index db3671fe4..33cdc4c88 100644 --- a/server/jellyfin/similar.go +++ b/server/jellyfin/similar.go @@ -20,7 +20,10 @@ import ( // tests can shorten it. var similarWait = 10 * time.Second -const maxSimilarLimit = 100 +const ( + defaultSimilarLimit = 20 + maxSimilarLimit = 100 +) // similarFetchTimeout bounds the detached background fetch so a hung provider can't hold a goroutine // indefinitely. @@ -51,7 +54,7 @@ func (api *Router) awaitSimilar(ctx context.Context, id string, limit int, fetch // returned. Any provider error degrades to an empty result, not a 404 the client would keep retrying. func (api *Router) getSimilarArtists(w http.ResponseWriter, r *http.Request) { id := api.resolveItemID(r.Context(), dto.DecodeID(chi.URLParam(r, "itemId"))) - limit := clampLimit(req.Params(r).IntOr("limit", 20)) + limit := clampLimit(req.Params(r).IntOr("limit", 0), defaultSimilarLimit, maxSimilarLimit) api.ok(w, r, api.awaitSimilar(r.Context(), id, limit, func(ctx context.Context) dto.QueryResult { return api.similarArtists(ctx, id, limit) })) @@ -63,7 +66,7 @@ func (api *Router) getSimilarArtists(w http.ResponseWriter, r *http.Request) { func (api *Router) getSimilarItems(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := api.resolveItemID(ctx, dto.DecodeID(chi.URLParam(r, "itemId"))) - limit := clampLimit(req.Params(r).IntOr("limit", 20)) + limit := clampLimit(req.Params(r).IntOr("limit", 0), defaultSimilarLimit, maxSimilarLimit) entity, err := model.GetEntityByID(ctx, api.ds, id) if err != nil { @@ -88,7 +91,7 @@ func (api *Router) getSimilarItems(w http.ResponseWriter, r *http.Request) { func (api *Router) getInstantMix(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := api.resolveItemID(ctx, dto.DecodeID(chi.URLParam(r, "itemId"))) - limit := clampLimit(req.Params(r).IntOr("limit", 20)) + limit := clampLimit(req.Params(r).IntOr("limit", 0), defaultSimilarLimit, maxSimilarLimit) entity, err := model.GetEntityByID(ctx, api.ds, id) if err != nil { @@ -136,15 +139,6 @@ func (api *Router) similarArtists(ctx context.Context, id string, limit int) dto return result(items, len(items), 0) } -// clampLimit bounds a client-supplied limit so it can't drive an oversized allocation or provider -// fetch (flagged by CodeQL as a user-controlled allocation size). -func clampLimit(limit int) int { - if limit <= 0 { - return 20 - } - return min(limit, maxSimilarLimit) -} - func (api *Router) similarSongs(ctx context.Context, id string, limit int) dto.QueryResult { songs, err := api.provider.SimilarSongs(ctx, id, limit) if err != nil { diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 6635881b7..03dfed879 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -20,6 +20,7 @@ type MockAlbumRepo struct { All model.Albums Err bool Options model.QueryOptions + SearchQuery string // last query passed to Search ReassignAnnotationCalls map[string]string // prevID -> newID CopyAttributesCalls map[string]string // fromID -> toID } @@ -134,6 +135,7 @@ func (m *MockAlbumRepo) UpdateExternalInfo(album *model.Album) error { } func (m *MockAlbumRepo) Search(q string, options ...model.QueryOptions) (model.Albums, error) { + m.SearchQuery = q if len(options) > 0 { m.Options = options[0] } diff --git a/tests/mock_playlist_track_repo.go b/tests/mock_playlist_track_repo.go index c11b077d2..2835baadd 100644 --- a/tests/mock_playlist_track_repo.go +++ b/tests/mock_playlist_track_repo.go @@ -1,9 +1,14 @@ package tests -import "github.com/navidrome/navidrome/model" +import ( + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" +) type MockPlaylistTrackRepo struct { model.PlaylistTrackRepository + Data model.PlaylistTracks + Options model.QueryOptions AddedIds []string DeletedIds []string Reordered bool @@ -11,6 +16,63 @@ type MockPlaylistTrackRepo struct { Err error } +func (m *MockPlaylistTrackRepo) SetData(tracks model.PlaylistTracks) { + m.Data = tracks +} + +// page applies Max/Offset as the real repository's SQL would. +func (m *MockPlaylistTrackRepo) page(options ...model.QueryOptions) model.PlaylistTracks { + var opts model.QueryOptions + if len(options) > 0 { + opts = options[0] + m.Options = opts + } + tracks := m.Data + if opts.Offset >= len(tracks) { + return nil + } + tracks = tracks[opts.Offset:] + if opts.Max > 0 && opts.Max < len(tracks) { + tracks = tracks[:opts.Max] + } + return tracks +} + +func (m *MockPlaylistTrackRepo) CountAll(_ ...model.QueryOptions) (int64, error) { + if m.Err != nil { + return 0, m.Err + } + return int64(len(m.Data)), nil +} + +func (m *MockPlaylistTrackRepo) GetAll(options ...model.QueryOptions) (model.PlaylistTracks, error) { + if m.Err != nil { + return nil, m.Err + } + return m.page(options...), nil +} + +func (m *MockPlaylistTrackRepo) GetCursor(options ...model.QueryOptions) (model.PlaylistTrackCursor, error) { + if m.Err != nil { + return nil, m.Err + } + tracks := m.page(options...) + return func(yield func(model.PlaylistTrack, error) bool) { + for _, t := range tracks { + if !yield(t, nil) { + return + } + } + }, nil +} + +func (m *MockPlaylistTrackRepo) GetMediaFileIDs(options ...model.QueryOptions) ([]string, error) { + if m.Err != nil { + return nil, m.Err + } + return slice.Map(m.page(options...), func(t model.PlaylistTrack) string { return t.MediaFileID }), nil +} + func (m *MockPlaylistTrackRepo) Add(ids []string) (int, error) { m.AddedIds = append(m.AddedIds, ids...) if m.Err != nil {