diff --git a/plugins/host/scrobble_retriever.go b/plugins/host/scrobble_retriever.go index 02c6005a4..f953c2baf 100644 --- a/plugins/host/scrobble_retriever.go +++ b/plugins/host/scrobble_retriever.go @@ -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 diff --git a/plugins/host_scrobbleretriever.go b/plugins/host_scrobbleretriever.go index c7ccb607e..b405db4ae 100644 --- a/plugins/host_scrobbleretriever.go +++ b/plugins/host_scrobbleretriever.go @@ -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) diff --git a/plugins/host_scrobbleretriever_test.go b/plugins/host_scrobbleretriever_test.go index e43d529c4..6a51ff1ad 100644 --- a/plugins/host_scrobbleretriever_test.go +++ b/plugins/host_scrobbleretriever_test.go @@ -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)) + } + } + }) }) })