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 {