mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
fix(jellyfin): return a mixed, globally-limited list for multi-type /Items requests (#5935)
* feat(jellyfin): allow random sort for artist/genre/playlist item types * feat(jellyfin): add round-robin interleave helper for merged item types * fix(jellyfin): mix and globally limit multi-type Items requests Run per-type queries in parallel and round-robin interleave the results so a request for multiple IncludeItemTypes (e.g. Finamp's random favorite) returns a mixed, globally-limited page instead of one type's rows followed by the next. * fix(jellyfin): dedupe repeated IncludeItemTypes to avoid duplicate items and redundant queries * refactor(jellyfin): dedupe via slice.Unique and extract queryTypeWindow helper * perf(jellyfin): serve random multi-type pages from offset 0 A random merge reshuffles every request, so paginating it is meaningless — page N is just another fresh draw (as in real Jellyfin). Serving from offset 0 caps the per-type fetch at limit instead of offset+limit, avoiding deep-offset blow-up for the random case (Finamp's random-favorite quick action). * refactor(jellyfin): resolve random-merge via applySort; simplify merge signatures Detect the random-page shortcut by resolving each type's sort through applySort (matching how the sort is actually chosen) instead of string-matching SortBy, and only when every type is random. Drop the always-zero window param from mergeTypesStreaming and derive the window inside mergeTypesPaged.
This commit is contained in:
parent
7736bbb545
commit
d080707060
@ -18,6 +18,7 @@ import (
|
||||
"github.com/navidrome/navidrome/server/jellyfin/dto"
|
||||
"github.com/navidrome/navidrome/utils/req"
|
||||
"github.com/navidrome/navidrome/utils/slice"
|
||||
"golang.org/x/sync/errgroup"
|
||||
)
|
||||
|
||||
// notMissing excludes items whose backing files are all gone ("missing" is a real column on
|
||||
@ -330,52 +331,97 @@ func (api *Router) playlistTracksRepo(ctx context.Context, q itemsQuery) (model.
|
||||
}
|
||||
|
||||
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
|
||||
if q.limit == 0 {
|
||||
return api.mergeTypesStreaming(ctx, q)
|
||||
}
|
||||
// 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)
|
||||
// A random page doesn't stack on the previous one (the order reshuffles each request), so serving
|
||||
// from 0 is an equivalent fresh draw and avoids materializing offset+limit rows per type.
|
||||
offset := q.offset
|
||||
if randomlySorted(q) {
|
||||
offset = 0
|
||||
}
|
||||
return api.mergeTypesPaged(ctx, q, offset)
|
||||
}
|
||||
|
||||
// randomlySorted reports whether every merged type resolves to a random sort — the case where a page
|
||||
// is an independent draw, so the offset can be collapsed to 0. Resolving via applySort (rather than
|
||||
// matching the raw SortBy) keeps this in step with how each type's sort is actually chosen.
|
||||
func randomlySorted(q itemsQuery) bool {
|
||||
for _, itemType := range q.types {
|
||||
var opts model.QueryOptions
|
||||
applySort(&opts, itemType, q.sortBy, q.sortOrder)
|
||||
if opts.Sort != "random" {
|
||||
return false
|
||||
}
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
// mergeTypesStreaming keeps the unbounded path lazy: chaining the per-type cursors yields their rows
|
||||
// in order minus the first offset, without pulling every row into memory.
|
||||
func (api *Router) mergeTypesStreaming(ctx context.Context, q itemsQuery) (itemsResult, error) {
|
||||
var results []itemsResult
|
||||
total := 0
|
||||
for _, itemType := range q.types {
|
||||
var opts model.QueryOptions
|
||||
opts.Max = window
|
||||
applySort(&opts, itemType, q.sortBy, q.sortOrder)
|
||||
res, err := api.queryItemsOfType(ctx, itemType, opts, q)
|
||||
res, err := api.queryTypeWindow(ctx, itemType, 0, q)
|
||||
if err != nil {
|
||||
return itemsResult{}, err
|
||||
}
|
||||
results = append(results, res)
|
||||
total += res.total
|
||||
}
|
||||
if q.limit == 0 {
|
||||
// No cap above, so merging in memory would pull every row of every type. The merged page is
|
||||
// just their rows in order minus the first offset — what chaining the cursors yields.
|
||||
return chained(results, total, q.offset), nil
|
||||
return chained(results, total, q.offset), nil
|
||||
}
|
||||
|
||||
// queryTypeWindow queries one type for the merge paths, capping it to window rows with the sort applied.
|
||||
func (api *Router) queryTypeWindow(ctx context.Context, itemType string, window int, q itemsQuery) (itemsResult, error) {
|
||||
var opts model.QueryOptions
|
||||
opts.Max = window
|
||||
applySort(&opts, itemType, q.sortBy, q.sortOrder)
|
||||
return api.queryItemsOfType(ctx, itemType, opts, q)
|
||||
}
|
||||
|
||||
// mergeTypesPaged runs each type's query concurrently, then round-robins the per-type rows so the limited page
|
||||
// is a mix rather than one type's rows followed by the next.
|
||||
func (api *Router) mergeTypesPaged(ctx context.Context, q itemsQuery, offset int) (itemsResult, error) {
|
||||
// Each per-type query needs at most offset+limit rows (worst case: one type fills the whole window).
|
||||
window := offset + q.limit
|
||||
if q.search != "" {
|
||||
window = min(window, maxSearchLimit)
|
||||
}
|
||||
var items []dto.BaseItemDto
|
||||
for _, res := range results {
|
||||
typeItems, err := res.collect()
|
||||
if err != nil {
|
||||
return itemsResult{}, err
|
||||
}
|
||||
items = append(items, typeItems...)
|
||||
lists := make([][]dto.BaseItemDto, len(q.types))
|
||||
totals := make([]int, len(q.types))
|
||||
g, ctx := errgroup.WithContext(ctx)
|
||||
for i, itemType := range q.types {
|
||||
g.Go(func() error {
|
||||
res, err := api.queryTypeWindow(ctx, itemType, window, q)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
items, err := res.collect()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
lists[i] = items
|
||||
totals[i] = res.total
|
||||
return nil
|
||||
})
|
||||
}
|
||||
if err := g.Wait(); err != nil {
|
||||
return itemsResult{}, err
|
||||
}
|
||||
total := 0
|
||||
for _, t := range totals {
|
||||
total += t
|
||||
}
|
||||
items := interleave(lists)
|
||||
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.
|
||||
// type's rows. The total is what's pageable overall, so a client paging on it won't stop early.
|
||||
items = items[:min(window, len(items))]
|
||||
total = min(total, maxSearchLimit)
|
||||
}
|
||||
return materialized(result(paginate(items, q.offset, q.limit), total, q.offset)), nil
|
||||
return materialized(result(paginate(items, offset, q.limit), total, q.offset)), nil
|
||||
}
|
||||
|
||||
func (api *Router) queryItemsOfType(ctx context.Context, itemType string, opts model.QueryOptions, q itemsQuery) (itemsResult, error) {
|
||||
@ -440,6 +486,8 @@ func parseTypes(types string) []string {
|
||||
recognized = append(recognized, t)
|
||||
}
|
||||
}
|
||||
// Dedupe: a repeated type would duplicate items in the merge and spawn a redundant query.
|
||||
recognized = slice.Unique(recognized)
|
||||
if len(recognized) == 0 {
|
||||
return []string{"MusicAlbum"}
|
||||
}
|
||||
@ -459,6 +507,25 @@ func paginate(items []dto.BaseItemDto, offset, limit int) []dto.BaseItemDto {
|
||||
return items
|
||||
}
|
||||
|
||||
// interleave merges per-type item lists round-robin: one item from each list in turn, preserving
|
||||
// each list's own order, so no single type dominates the head of a mixed-type result.
|
||||
func interleave(lists [][]dto.BaseItemDto) []dto.BaseItemDto {
|
||||
total, maxLen := 0, 0
|
||||
for _, l := range lists {
|
||||
total += len(l)
|
||||
maxLen = max(maxLen, len(l))
|
||||
}
|
||||
out := make([]dto.BaseItemDto, 0, total)
|
||||
for i := 0; i < maxLen; i++ {
|
||||
for _, l := range lists {
|
||||
if i < len(l) {
|
||||
out = append(out, l[i])
|
||||
}
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// 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 (
|
||||
@ -818,9 +885,8 @@ func applySort(opts *model.QueryOptions, itemType, sortBy, order string) {
|
||||
}
|
||||
}
|
||||
|
||||
// sortColumnsByType maps lowercased-SortBy -> repo-sort-key per item type. Each repository maps
|
||||
// logical fields to different real columns (e.g. media_file has "title" not "name"; artist has no
|
||||
// "random").
|
||||
// sortColumnsByType maps lowercased-SortBy -> repo-sort-key per item type (repos map logical fields
|
||||
// to different real columns, e.g. media_file has "title" not "name").
|
||||
var sortColumnsByType = map[string]map[string]string{
|
||||
"Audio": {
|
||||
"sortname": "title", "name": "title",
|
||||
@ -848,6 +914,7 @@ var sortColumnsByType = map[string]map[string]string{
|
||||
"playcount": "play_count",
|
||||
"dateplayed": "play_date",
|
||||
"communityrating": "rating",
|
||||
"random": "random",
|
||||
},
|
||||
"MusicAlbum": {
|
||||
"sortname": "name", "name": "name", "album": "name",
|
||||
@ -862,10 +929,12 @@ var sortColumnsByType = map[string]map[string]string{
|
||||
},
|
||||
"MusicGenre": {
|
||||
"sortname": "name", "name": "name",
|
||||
"random": "random",
|
||||
},
|
||||
"Playlist": {
|
||||
"sortname": "name", "name": "name",
|
||||
"datecreated": "created_at",
|
||||
"random": "random",
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
@ -462,9 +462,9 @@ var _ = Describe("Items", func() {
|
||||
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.
|
||||
// Clipped to the window; the interleaved album takes one slot, shifting this song in by one.
|
||||
Expect(res.Items).To(HaveLen(1))
|
||||
Expect(res.Items[0].Id).To(Equal(dto.EncodeID(songs[maxSearchLimit-1].ID)))
|
||||
Expect(res.Items[0].Id).To(Equal(dto.EncodeID(songs[maxSearchLimit-2].ID)))
|
||||
})
|
||||
|
||||
It("bounds an unbounded multi-type search to the default in total, not per type", func() {
|
||||
@ -500,7 +500,8 @@ var _ = Describe("Items", func() {
|
||||
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)))
|
||||
// The interleaved album takes one slot ahead of it, shifting this song in by one.
|
||||
Expect(res.Items[0].Id).To(Equal(dto.EncodeID(songs[defaultSearchLimit+49].ID)))
|
||||
})
|
||||
|
||||
It("reports a search total beyond the fetched page instead of the page length", func() {
|
||||
@ -748,6 +749,68 @@ var _ = Describe("Items", func() {
|
||||
Expect(sql).NotTo(ContainSubstring("library_id"))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("mixed IncludeItemTypes merge", func() {
|
||||
BeforeEach(func() {
|
||||
ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}, {ID: "a2", Name: "Two"}})
|
||||
ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{{ID: "s1", Title: "S1"}, {ID: "s2", Title: "S2"}})
|
||||
})
|
||||
|
||||
It("returns a mix of both types, not all of one", func() {
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&Recursive=true&Limit=4", nil).WithContext(ctxUser())
|
||||
invoke(api.getItems, w, r)
|
||||
var res dto.QueryResult
|
||||
Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed())
|
||||
Expect(res.Items).To(HaveLen(4))
|
||||
Expect(res.TotalRecordCount).To(Equal(4))
|
||||
types := map[string]int{}
|
||||
for _, it := range res.Items {
|
||||
types[it.Type]++
|
||||
}
|
||||
Expect(types["Audio"]).To(Equal(2))
|
||||
Expect(types["MusicAlbum"]).To(Equal(2))
|
||||
})
|
||||
|
||||
It("interleaves types round-robin (Audio first, per IncludeItemTypes order)", func() {
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&Recursive=true&Limit=4", nil).WithContext(ctxUser())
|
||||
invoke(api.getItems, w, r)
|
||||
var res dto.QueryResult
|
||||
Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed())
|
||||
got := []string{res.Items[0].Type, res.Items[1].Type, res.Items[2].Type, res.Items[3].Type}
|
||||
Expect(got).To(Equal([]string{"Audio", "MusicAlbum", "Audio", "MusicAlbum"}))
|
||||
})
|
||||
|
||||
It("honors Limit across the merged set", func() {
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&Recursive=true&Limit=1", nil).WithContext(ctxUser())
|
||||
invoke(api.getItems, w, r)
|
||||
var res dto.QueryResult
|
||||
Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed())
|
||||
Expect(res.Items).To(HaveLen(1))
|
||||
Expect(res.TotalRecordCount).To(Equal(4))
|
||||
})
|
||||
|
||||
It("serves a full random page from offset 0 regardless of StartIndex", func() {
|
||||
// A deep StartIndex on a random merge must not materialize offset+limit rows; since random
|
||||
// reshuffles per request, offset 0 is an equivalent fresh draw. Old behavior returned empty.
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&SortBy=Random&Recursive=true&StartIndex=1000&Limit=4", nil).WithContext(ctxUser())
|
||||
invoke(api.getItems, w, r)
|
||||
var res dto.QueryResult
|
||||
Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed())
|
||||
Expect(res.Items).To(HaveLen(4))
|
||||
})
|
||||
|
||||
It("propagates a per-type query error", func() {
|
||||
ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo).SetError(true)
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Items?IncludeItemTypes=Audio,MusicAlbum&Recursive=true&Limit=4", nil).WithContext(ctxUser())
|
||||
invoke(api.getItems, w, r)
|
||||
Expect(w.Code).To(Equal(http.StatusInternalServerError))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Describe("getItem", func() {
|
||||
@ -896,4 +959,44 @@ var _ = Describe("Items", func() {
|
||||
Expect(args).To(ContainElements(1, 2))
|
||||
})
|
||||
})
|
||||
|
||||
Describe("applySort random for all merge types", func() {
|
||||
DescribeTable("maps Random -> random",
|
||||
func(itemType string) {
|
||||
var opts model.QueryOptions
|
||||
applySort(&opts, itemType, "Random", "")
|
||||
Expect(opts.Sort).To(Equal("random"))
|
||||
},
|
||||
Entry("Audio", "Audio"),
|
||||
Entry("MusicAlbum", "MusicAlbum"),
|
||||
Entry("MusicArtist", "MusicArtist"),
|
||||
Entry("MusicGenre", "MusicGenre"),
|
||||
Entry("Playlist", "Playlist"),
|
||||
)
|
||||
})
|
||||
|
||||
Describe("interleave", func() {
|
||||
It("round-robins one item per list in turn", func() {
|
||||
lists := [][]dto.BaseItemDto{
|
||||
{{Id: "a0"}, {Id: "a1"}, {Id: "a2"}},
|
||||
{{Id: "b0"}, {Id: "b1"}},
|
||||
}
|
||||
got := interleave(lists)
|
||||
ids := make([]string, len(got))
|
||||
for i, it := range got {
|
||||
ids[i] = it.Id
|
||||
}
|
||||
Expect(ids).To(Equal([]string{"a0", "b0", "a1", "b1", "a2"}))
|
||||
})
|
||||
|
||||
It("returns empty for no lists", func() {
|
||||
Expect(interleave(nil)).To(BeEmpty())
|
||||
})
|
||||
})
|
||||
|
||||
Describe("parseTypes", func() {
|
||||
It("dedupes repeated types, preserving first-seen order", func() {
|
||||
Expect(parseTypes("Audio,MusicAlbum,Audio")).To(Equal([]string{"Audio", "MusicAlbum"}))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user