nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-18 07:30:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(plugins): make scrobble pagination order deterministic

Browse Source 
Sorting only by submission_time left the order of equal timestamps up to the
query planner, but the cursor skips ties by offset, so an unstable order can
repeat or drop scrobbles between pages. Break ties on scrobbles.id, which the
existing scrobbles_user_time index already yields for free.

Descending is now honoured for every combination of From/To rather than only
when both or neither is set. This changes the default for a lone ToTimestamp
from newest-first to oldest-first.
This commit is contained in:
Deluan 2026-08-08 13:02:26 -04:00
parent 6982bbc285
commit 22d94877fe
3 changed files with 62 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
plugins/host/scrobble_retriever.go
View File

@ -33,9 +33,7 @@ type ScrobbleOptions struct {
// The ending unix timestamp to query for scrobbles (inclusive).
// If not specified, go up to the last scrobble
ToTimestamp *int64 `json:"toTimestamp,omitempty"`
// Descending is only relevant if both FromTimestamp and ToTimestamp are specified
// If both timestamps are specified (or omitted), the default order will be ascending submission time
// If Descending is true, instead query in descending order
// If true, return scrobbles from newest to oldest. Defaults to oldest first
Descending bool `json:"descending"`
// A cursor retrieved from a prior call to GetScrobbles
Cursor int `json:"cursor,omitempty"`
@ -79,10 +77,9 @@ type ScrobbleRetrieverService interface {
// - options.MaxItems: The maximum number of items to retrieve. The maximum value (and default) if not specified is 5000
//
// Returns:
// - Scrobbles: A list of scrobbles within the constraints given (if any). The order
// of the items depends on the options: if ToTimestamp is specified AND
// FromTimestamp is not specified, the order is in descending submission time.
// Otherwise, the scrobbles are returned in ascending submission time.
// - Scrobbles: A list of scrobbles within the constraints given (if any), ordered by
// submission time (ties broken by scrobble ID) in the direction given by
// options.Descending
// - NextTimestamp: If there are additional items to retrieve in the range, the timestamp
// of the next scrobble that would be retrieved in the order (asc or desc)
//nd:hostfunc

64
plugins/host_scrobbleretriever.go
View File

@ -10,6 +10,9 @@ import (
"github.com/navidrome/navidrome/plugins/host"
)
// maxScrobbleItems caps how many scrobbles a single GetScrobbles call can return.
const maxScrobbleItems = 5000
type scrobbleRetrieverServiceImpl struct {
ds model.DataStore
users userAccess
@ -64,8 +67,8 @@ func (s *scrobbleRetrieverServiceImpl) GetScrobbles(ctx context.Context, usernam
return nil, err
}
if options.MaxItems < 1 || options.MaxItems > 5000 {
options.MaxItems = 5000
if options.MaxItems < 1 || options.MaxItems > maxScrobbleItems {
options.MaxItems = maxScrobbleItems
}
if options.Cursor < 0 {
@ -75,34 +78,19 @@ func (s *scrobbleRetrieverServiceImpl) GetScrobbles(ctx context.Context, usernam
// Fetch one more item than requested. The last item is the next timestamp to fetch
options.MaxItems += 1
var filters squirrel.And
if options.FromTimestamp != nil {
filters = append(filters, squirrel.GtOrEq{"submission_time": *options.FromTimestamp})
}
if options.ToTimestamp != nil {
filters = append(filters, squirrel.LtOrEq{"submission_time": *options.ToTimestamp})
}
var order string
if (options.ToTimestamp != nil) == (options.FromTimestamp != nil) {
if options.Descending {
order = "DESC"
} else {
order = "ASC"
}
} else if options.ToTimestamp != nil {
order := "ASC"
if options.Descending {
order = "DESC"
} else {
order = "ASC"
}
scrobbles, err := s.ds.Scrobble(ctx).GetAll(model.QueryOptions{
Max: options.MaxItems,
Filters: filters,
Sort: "submission_time",
Order: order,
Offset: options.Cursor,
Filters: scrobbleRangeFilters(options.FromTimestamp, options.ToTimestamp),
// The id tiebreak makes the order of equal timestamps stable, which is what
// lets Cursor skip exactly the ties already returned
Sort: "scrobbles.submission_time, scrobbles.id",
Order: order,
Offset: options.Cursor,
})
if err != nil {
@ -163,24 +151,20 @@ func (s *scrobbleRetrieverServiceImpl) GetScrobbleCount(ctx context.Context, use
return 0, err
}
var filters squirrel.And
if options.FromTimestamp != nil {
filters = append(filters, squirrel.GtOrEq{"submission_time": *options.FromTimestamp})
}
if options.ToTimestamp != nil {
filters = append(filters, squirrel.LtOrEq{"submission_time": *options.ToTimestamp})
}
count, err := s.ds.Scrobble(ctx).CountAll(model.QueryOptions{
Filters: filters,
return s.ds.Scrobble(ctx).CountAll(model.QueryOptions{
Filters: scrobbleRangeFilters(options.FromTimestamp, options.ToTimestamp),
})
}
if err != nil {
return 0, err
func scrobbleRangeFilters(from, to *int64) squirrel.And {
var filters squirrel.And
if from != nil {
filters = append(filters, squirrel.GtOrEq{"scrobbles.submission_time": *from})
}
return count, nil
if to != nil {
filters = append(filters, squirrel.LtOrEq{"scrobbles.submission_time": *to})
}
return filters
}
var _ host.ScrobbleRetrieverService = (*scrobbleRetrieverServiceImpl)(nil)

45
plugins/host_scrobbleretriever_test.go
View File

@ -259,13 +259,14 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() {
Entry("calls scrobbles in ascending order", `{"username":"adminuser"}`, scrobbles, nil, 0),
Entry("calls scrobbles in descending order by request", `{"username":"adminuser","descending":true}`, scrobblesReversed, nil, 0),
Entry("calls scrobbles in ascending order, beyond range", `{"username":"adminuser","fromTimestamp":-1, "toTimestamp": 1000}`, scrobbles, nil, 0),
Entry("defaults to ascending even when only toTimestamp is given", `{"username":"adminuser","toTimestamp":2}`, scrobbles, nil, 0),
Entry("calls subset of scrobbles in ascending order, next timestamp", `{"username":"adminuser","maxItems":2}`, scrobbles[:2], p(2), 0),
Entry("calls subset of scrobbles in ascending order, with offset next timestamp", `{"username":"adminuser","maxItems":2,"fromTimestamp":1}`, scrobbles[1:3], p(2), 1),
Entry("calls subset of scrobbles in ascending order, from and to timestamp", `{"username":"adminuser","toTimestamp":2,"fromTimestamp":1}`, scrobbles[1:], nil, 0),
Entry("calls subset of scrobbles in descending order, from and to timestamp", `{"username":"adminuser","toTimestamp":2,"fromTimestamp":1,"descending":true}`, scrobblesReversed[:3], nil, 0),
Entry("calls in reverse order, full", `{"username":"adminuser","toTimestamp":2}`, scrobblesReversed, nil, 0),
Entry("calls in reverse order, with count", `{"username":"adminuser","toTimestamp":2, "maxItems": 3}`, scrobblesReversed[:3], p(0), 0),
Entry("calls in reverse order, with count of 1", `{"username":"adminuser","toTimestamp":2, "maxItems": 1}`, scrobblesReversed[:1], p(2), 1),
Entry("calls in reverse order, full", `{"username":"adminuser","toTimestamp":2,"descending":true}`, scrobblesReversed, nil, 0),
Entry("calls in reverse order, with count", `{"username":"adminuser","toTimestamp":2,"descending":true, "maxItems": 3}`, scrobblesReversed[:3], p(0), 0),
Entry("calls in reverse order, with count of 1", `{"username":"adminuser","toTimestamp":2,"descending":true, "maxItems": 1}`, scrobblesReversed[:1], p(2), 1),
)
DescribeTable("GetScrobblesCount", func(params string, count int) {
@ -313,17 +314,16 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() {
MaxItems int `json:"maxItems"`
}
DescribeTable("Edge cases; duplicate scrobbles", func(cursor, count int, descending bool, scrobbles []host.ScrobbleRef, timestamp *int64, expectedCursor int) {
op := TestScrobbleOptions{
getPage := func(cursor, count int, descending bool) host.ScrobbleList {
GinkgoHelper()
payload, err := json.Marshal(TestScrobbleOptions{
Username: "adminuser",
FromTimestamp: p(100),
ToTimestamp: p(100),
Descending: descending,
Cursor: cursor,
MaxItems: count,
}
payload, err := json.Marshal(op)
})
Expect(err).ToNot(HaveOccurred())
exit, output, err := instance.Call("call_get_scrobbles", payload)
@ -332,7 +332,11 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() {
var scrobbleList host.ScrobbleList
Expect(json.Unmarshal(output, &scrobbleList)).To(Succeed())
Expect(scrobbleList).To(Equal(host.ScrobbleList{
return scrobbleList
}
DescribeTable("Edge cases; duplicate scrobbles", func(cursor, count int, descending bool, scrobbles []host.ScrobbleRef, timestamp *int64, expectedCursor int) {
Expect(getPage(cursor, count, descending)).To(Equal(host.ScrobbleList{
Scrobbles: scrobbles,
NextTimestamp: timestamp,
Cursor: expectedCursor,
@ -341,13 +345,32 @@ var _ = Describe("Scrobble Retriever Host Function", Ordered, func() {
Entry("All tracks, in order", 0, 100, false, duplicates, nil, 0),
Entry("All tracks, in reverse order", 0, 100, true, duplicatesReversed, nil, 0),
Entry("Ascending order, from the start", 0, 1, false, duplicates[:1], p(100), 1),
Entry("Ascending order, from the start", 1, 2, false, duplicates[1:3], p(100), 3),
Entry("Ascending order, middle page", 1, 2, false, duplicates[1:3], p(100), 3),
Entry("Ascending order, to the end", 3, 100, false, duplicates[3:], nil, 0),
Entry("Ascending order, from the start, continuing cursor", 3, 1, false, duplicates[3:4], p(100), 4),
Entry("start descending", 0, 2, true, duplicatesReversed[:2], p(100), 2),
Entry("start descending, next step", 2, 2, true, duplicatesReversed[2:4], p(100), 4),
Entry("start descending, end", 4, 1, true, duplicatesReversed[4:], nil, 0),
)
It("walks every page exactly once when all timestamps collide", func() {
for _, descending := range []bool{false, true} {
var seen []host.ScrobbleRef
page := host.ScrobbleList{}
for range 10 {
page = getPage(page.Cursor, 2, descending)
seen = append(seen, page.Scrobbles...)
if page.NextTimestamp == nil {
break
}
}
Expect(page.NextTimestamp).To(BeNil(), "pagination did not terminate")
if descending {
Expect(seen).To(Equal(duplicatesReversed))
} else {
Expect(seen).To(Equal(duplicates))
}
}
})
})
})