mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(subsonic): make "recently added" order reproducible and consistent with RecentlyAddedByModTime (#5678)
* fix(subsonic): align album `created` with RecentlyAddedByModTime sort The album `created` attribute returned by search3, getAlbumList2 and the other album endpoints was always sourced from the album's CreatedAt (oldest song birth time), while the "recently added" sort is governed by RecentlyAddedByModTime: it orders by album.updated_at when that option is enabled and album.created_at otherwise. As a result, when RecentlyAddedByModTime was enabled, clients that cache album results and sort locally by `created` (e.g. for a "Date added" view) could not reproduce the order returned by getAlbumList2?type=newest, since the exposed value did not match the column driving the sort. Make albumCreatedAt config-aware so the primary timestamp it returns mirrors recentlyAddedSort: UpdatedAt when RecentlyAddedByModTime is set, CreatedAt otherwise. The existing zero-value fallback chain is preserved so this required OpenSubsonic field is never emitted as zero on legacy rows. Note: this is a behavior change for the contractual `created` attribute. With RecentlyAddedByModTime enabled, an album's reported `created` now reflects the newest song modification time and can change when files are modified. Scope is limited to albums; the song-level `created` (BirthTime) is unchanged. * fix(subsonic): order Recently Added by full-precision timestamp with tiebreak The recently_added sort wrapped the timestamp in datetime(), truncating it to whole seconds, and had no secondary sort key. Album timestamps carry sub-second precision (aggregated from song file birth-times), so on a fresh scan many albums tie at the second; SQLite then returns ties in query-plan order, which changes when a library filter is applied. This made the web UI "Recently Added" order invert between library selections and diverge from getAlbumList2?type=newest, and clients receiving the full-precision created value could never reproduce the server order. Sort on the raw, full-precision column with an album.id / media_file.id tiebreak instead. A new migration swaps the album datetime() expression indexes (and the plain media_file indexes) for composite (col, id) indexes that cover the new sort. Timestamps were already normalized to space-format by 20260316000000_normalize_timestamps, so raw-string comparison is safe. * fix(subsonic): make song created follow RecentlyAddedByModTime The song Created field returned BirthTime (file ctime), but the recently_added sort (shared by the Subsonic and native APIs, and the web UI) orders by created_at, or updated_at when RecentlyAddedByModTime is set. A Subsonic client, which can only sort by the created value it receives, could therefore never reproduce the server's "recently added" order in either mode. Add mediaFileCreatedAt mirroring albumCreatedAt and use it for child.Created: CreatedAt by default, UpdatedAt under RecentlyAddedByModTime, with BirthTime as a legacy fallback. This aligns song created with the sort column, matching how album created already works and what the native API/web UI present.
This commit is contained in:
parent
7303c9ca47
commit
0fab1861a0
@ -0,0 +1,28 @@
|
||||
-- +goose Up
|
||||
|
||||
-- The "Recently Added" sort now uses the raw timestamp with an id tiebreak
|
||||
-- instead of datetime(), so the indexes become plain composite (col, id) to
|
||||
-- cover it. Timestamps were already normalized to space-format by
|
||||
-- 20260316000000_normalize_timestamps, so raw-string comparison is safe.
|
||||
|
||||
DROP INDEX IF EXISTS album_created_at;
|
||||
CREATE INDEX album_created_at ON album(created_at, id);
|
||||
DROP INDEX IF EXISTS album_updated_at;
|
||||
CREATE INDEX album_updated_at ON album(updated_at, id);
|
||||
|
||||
DROP INDEX IF EXISTS media_file_created_at;
|
||||
CREATE INDEX media_file_created_at ON media_file(created_at, id);
|
||||
DROP INDEX IF EXISTS media_file_updated_at;
|
||||
CREATE INDEX media_file_updated_at ON media_file(updated_at, id);
|
||||
|
||||
-- +goose Down
|
||||
|
||||
DROP INDEX IF EXISTS album_created_at;
|
||||
CREATE INDEX album_created_at ON album(datetime(created_at));
|
||||
DROP INDEX IF EXISTS album_updated_at;
|
||||
CREATE INDEX album_updated_at ON album(datetime(updated_at));
|
||||
|
||||
DROP INDEX IF EXISTS media_file_created_at;
|
||||
CREATE INDEX media_file_created_at ON media_file(created_at);
|
||||
DROP INDEX IF EXISTS media_file_updated_at;
|
||||
CREATE INDEX media_file_updated_at ON media_file(updated_at);
|
||||
@ -143,9 +143,9 @@ var albumFilters = sync.OnceValue(func() map[string]filterFunc {
|
||||
|
||||
func recentlyAddedSort() string {
|
||||
if conf.Server.RecentlyAddedByModTime {
|
||||
return "datetime(album.updated_at)"
|
||||
return "album.updated_at, album.id"
|
||||
}
|
||||
return "datetime(album.created_at)"
|
||||
return "album.created_at, album.id"
|
||||
}
|
||||
|
||||
func recentlyPlayedFilter(string, any) Sqlizer {
|
||||
|
||||
@ -112,49 +112,66 @@ var _ = Describe("AlbumRepository", func() {
|
||||
})
|
||||
|
||||
Describe("recently_added sort", func() {
|
||||
It("sorts correctly regardless of timestamp format (T-format vs space-format)", func() {
|
||||
// Both timestamps share the same date prefix "2024-01-15" so the T vs space
|
||||
// character at position 10 determines sort order in raw string comparison.
|
||||
// Without normalization, 'T' (ASCII 84) > ' ' (ASCII 32) makes the older
|
||||
// T-format timestamp sort AFTER the newer space-format one.
|
||||
AfterEach(func() {
|
||||
_, _ = albumRepo.executeSQL(squirrel.Delete("album").
|
||||
Where(squirrel.Like{"id": "ra-%"}))
|
||||
})
|
||||
|
||||
// Older album: morning of Jan 15, stored in T-format
|
||||
olderAlbum := &model.Album{LibraryID: 1, ID: "ts-older", Name: "Older Album"}
|
||||
Expect(albumRepo.Put(olderAlbum)).To(Succeed())
|
||||
// Sub-second precision must survive, and ties must break deterministically
|
||||
// so the order is independent of any filter (issue #5673).
|
||||
indexOf := func(albums model.Albums, id string) int {
|
||||
for i, a := range albums {
|
||||
if a.ID == id {
|
||||
return i
|
||||
}
|
||||
}
|
||||
return -1
|
||||
}
|
||||
|
||||
It("orders by sub-second precision, not truncated to the second", func() {
|
||||
// Same second, different nanoseconds: datetime() would tie these.
|
||||
earlier := &model.Album{LibraryID: 1, ID: "ra-earlier", Name: "Earlier"}
|
||||
later := &model.Album{LibraryID: 1, ID: "ra-later", Name: "Later"}
|
||||
Expect(albumRepo.Put(earlier)).To(Succeed())
|
||||
Expect(albumRepo.Put(later)).To(Succeed())
|
||||
_, err := albumRepo.executeSQL(squirrel.Update("album").
|
||||
Set("created_at", "2024-01-15T08:00:00Z").
|
||||
Where(squirrel.Eq{"id": "ts-older"}))
|
||||
Set("created_at", "2024-01-15 10:00:00.100000000+00:00").
|
||||
Where(squirrel.Eq{"id": "ra-earlier"}))
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
// Newer album: evening of Jan 15, stored in space-format
|
||||
newerAlbum := &model.Album{LibraryID: 1, ID: "ts-newer", Name: "Newer Album"}
|
||||
Expect(albumRepo.Put(newerAlbum)).To(Succeed())
|
||||
_, err = albumRepo.executeSQL(squirrel.Update("album").
|
||||
Set("created_at", "2024-01-15 20:00:00+00:00").
|
||||
Where(squirrel.Eq{"id": "ts-newer"}))
|
||||
Set("created_at", "2024-01-15 10:00:00.900000000+00:00").
|
||||
Where(squirrel.Eq{"id": "ra-later"}))
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
albums, err := albumRepo.GetAll(model.QueryOptions{Sort: "recently_added", Order: "desc"})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(indexOf(albums, "ra-later")).To(BeNumerically("<", indexOf(albums, "ra-earlier")),
|
||||
".900 should sort before .100 in desc order")
|
||||
})
|
||||
|
||||
// Find positions of our test albums
|
||||
olderIdx, newerIdx := -1, -1
|
||||
for i, a := range albums {
|
||||
switch a.ID {
|
||||
case "ts-older":
|
||||
olderIdx = i
|
||||
case "ts-newer":
|
||||
newerIdx = i
|
||||
}
|
||||
It("breaks ties deterministically and consistently across filters", func() {
|
||||
// All sharing one created_at: the relative order of any subset must
|
||||
// match the unfiltered order (the inversion mechanism in #5673).
|
||||
ids := []string{"ra-t1", "ra-t2", "ra-t3", "ra-t4"}
|
||||
for _, aid := range ids {
|
||||
Expect(albumRepo.Put(&model.Album{LibraryID: 1, ID: aid, Name: aid})).To(Succeed())
|
||||
}
|
||||
Expect(olderIdx).To(BeNumerically(">=", 0), "older album not found in results")
|
||||
Expect(newerIdx).To(BeNumerically(">=", 0), "newer album not found in results")
|
||||
// Newer album (evening, space-format) should come before older album (morning, T-format) in desc order
|
||||
Expect(newerIdx).To(BeNumerically("<", olderIdx),
|
||||
"Newer album (20:00 space-format) should sort before older album (08:00 T-format) in desc order")
|
||||
_, err := albumRepo.executeSQL(squirrel.Update("album").
|
||||
Set("created_at", "2024-02-20 12:00:00+00:00").
|
||||
Where(squirrel.Eq{"id": ids}))
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
// Clean up
|
||||
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": []string{"ts-older", "ts-newer"}}))
|
||||
all, err := albumRepo.GetAll(model.QueryOptions{Sort: "recently_added", Order: "desc"})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
subset, err := albumRepo.GetAll(model.QueryOptions{
|
||||
Sort: "recently_added", Order: "desc",
|
||||
Filters: squirrel.Eq{"album.id": []string{"ra-t1", "ra-t3"}}})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
Expect(indexOf(all, "ra-t1") < indexOf(all, "ra-t3")).
|
||||
To(Equal(indexOf(subset, "ra-t1") < indexOf(subset, "ra-t3")),
|
||||
"tied albums must keep the same relative order with and without a filter")
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@ -117,9 +117,9 @@ var mediaFileFilter = sync.OnceValue(func() map[string]filterFunc {
|
||||
|
||||
func mediaFileRecentlyAddedSort() string {
|
||||
if conf.Server.RecentlyAddedByModTime {
|
||||
return "media_file.updated_at"
|
||||
return "media_file.updated_at, media_file.id"
|
||||
}
|
||||
return "media_file.created_at"
|
||||
return "media_file.created_at, media_file.id"
|
||||
}
|
||||
|
||||
func (r *mediaFileRepository) CountAll(options ...model.QueryOptions) (int64, error) {
|
||||
|
||||
@ -576,6 +576,34 @@ var _ = Describe("MediaRepository", func() {
|
||||
})
|
||||
})
|
||||
|
||||
It("breaks ties deterministically when files share the same created_at", func() {
|
||||
conf.Server.RecentlyAddedByModTime = false
|
||||
ctx := log.NewContext(GinkgoT().Context())
|
||||
ctx = request.WithUser(ctx, model.User{ID: "userid"})
|
||||
repo := NewMediaFileRepository(ctx, GetDBXBuilder())
|
||||
|
||||
ids := []string{testMediaFiles[0].ID, testMediaFiles[1].ID, testMediaFiles[2].ID}
|
||||
sameTime := time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC)
|
||||
_, err := GetDBXBuilder().Update("media_file",
|
||||
dbx.Params{"created_at": sameTime},
|
||||
dbx.In("id", ids[0], ids[1], ids[2])).Execute()
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
order := func() []string {
|
||||
res, err := repo.GetAll(model.QueryOptions{
|
||||
Sort: "recently_added", Order: "desc",
|
||||
Filters: squirrel.Eq{"media_file.id": ids}})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
out := make([]string, len(res))
|
||||
for i, mf := range res {
|
||||
out[i] = mf.ID
|
||||
}
|
||||
return out
|
||||
}
|
||||
// Stable across repeated queries (no query-plan-dependent reordering).
|
||||
Expect(order()).To(Equal(order()))
|
||||
})
|
||||
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@ -217,7 +217,7 @@ func childFromMediaFile(ctx context.Context, mf model.MediaFile) responses.Child
|
||||
child.Path = fakePath(mf)
|
||||
}
|
||||
child.DiscNumber = int32(mf.DiscNumber)
|
||||
child.Created = new(mf.BirthTime)
|
||||
child.Created = new(mediaFileCreatedAt(mf))
|
||||
child.AlbumId = mf.AlbumID
|
||||
child.ArtistId = mf.ArtistID
|
||||
child.Type = "music"
|
||||
@ -326,18 +326,36 @@ func sanitizeSlashes(target string) string {
|
||||
return strings.ReplaceAll(target, "/", "_")
|
||||
}
|
||||
|
||||
// albumCreatedAt returns a best-effort timestamp for the album's `created`
|
||||
// field, which is required by the OpenSubsonic spec but may be zero on legacy
|
||||
// DB rows. Falls back to UpdatedAt → ImportedAt; can still return zero if all
|
||||
// three are unset.
|
||||
// albumCreatedAt mirrors the column used by recentlyAddedSort so clients can
|
||||
// reproduce the "recently added" order locally: UpdatedAt when
|
||||
// RecentlyAddedByModTime is set, CreatedAt otherwise. The other timestamps are
|
||||
// fallbacks for legacy rows; returns zero only when all three are unset.
|
||||
func albumCreatedAt(al model.Album) time.Time {
|
||||
if !al.CreatedAt.IsZero() {
|
||||
return al.CreatedAt
|
||||
candidates := []time.Time{al.CreatedAt, al.UpdatedAt, al.ImportedAt}
|
||||
if conf.Server.RecentlyAddedByModTime {
|
||||
candidates = []time.Time{al.UpdatedAt, al.CreatedAt, al.ImportedAt}
|
||||
}
|
||||
if !al.UpdatedAt.IsZero() {
|
||||
return al.UpdatedAt
|
||||
for _, t := range candidates {
|
||||
if !t.IsZero() {
|
||||
return t
|
||||
}
|
||||
}
|
||||
return al.ImportedAt
|
||||
return time.Time{}
|
||||
}
|
||||
|
||||
// mediaFileCreatedAt is the song counterpart of albumCreatedAt, tracking
|
||||
// mediaFileRecentlyAddedSort; BirthTime is the legacy fallback.
|
||||
func mediaFileCreatedAt(mf model.MediaFile) time.Time {
|
||||
candidates := []time.Time{mf.CreatedAt, mf.UpdatedAt, mf.BirthTime}
|
||||
if conf.Server.RecentlyAddedByModTime {
|
||||
candidates = []time.Time{mf.UpdatedAt, mf.CreatedAt, mf.BirthTime}
|
||||
}
|
||||
for _, t := range candidates {
|
||||
if !t.IsZero() {
|
||||
return t
|
||||
}
|
||||
}
|
||||
return time.Time{}
|
||||
}
|
||||
|
||||
func childFromAlbum(ctx context.Context, al model.Album) responses.Child {
|
||||
|
||||
@ -619,31 +619,122 @@ var _ = Describe("helpers", func() {
|
||||
})
|
||||
|
||||
Describe("buildAlbumID3 Created field", func() {
|
||||
It("uses CreatedAt when set", func() {
|
||||
t := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
al := model.Album{ID: "a1", Name: "A", CreatedAt: t}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(t))
|
||||
When("RecentlyAddedByModTime is false", func() {
|
||||
BeforeEach(func() {
|
||||
conf.Server.RecentlyAddedByModTime = false
|
||||
})
|
||||
|
||||
It("uses CreatedAt when set", func() {
|
||||
t := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
al := model.Album{ID: "a1", Name: "A", CreatedAt: t}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(t))
|
||||
})
|
||||
|
||||
It("falls back to UpdatedAt when CreatedAt is zero", func() {
|
||||
updated := time.Date(2019, 5, 6, 7, 8, 9, 0, time.UTC)
|
||||
al := model.Album{ID: "a2", Name: "A", UpdatedAt: updated}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(updated))
|
||||
})
|
||||
|
||||
It("falls back to ImportedAt when CreatedAt and UpdatedAt are zero", func() {
|
||||
imported := time.Date(2021, 8, 9, 10, 11, 12, 0, time.UTC)
|
||||
al := model.Album{ID: "a3", Name: "A", ImportedAt: imported}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(imported))
|
||||
})
|
||||
|
||||
It("leaves Created as zero time when all timestamps are zero", func() {
|
||||
al := model.Album{ID: "a4", Name: "A"}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created.IsZero()).To(BeTrue())
|
||||
})
|
||||
})
|
||||
|
||||
It("falls back to UpdatedAt when CreatedAt is zero", func() {
|
||||
updated := time.Date(2019, 5, 6, 7, 8, 9, 0, time.UTC)
|
||||
al := model.Album{ID: "a2", Name: "A", UpdatedAt: updated}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(updated))
|
||||
When("RecentlyAddedByModTime is true", func() {
|
||||
BeforeEach(func() {
|
||||
conf.Server.RecentlyAddedByModTime = true
|
||||
})
|
||||
|
||||
It("uses UpdatedAt even when CreatedAt is also set", func() {
|
||||
created := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
updated := time.Date(2022, 6, 7, 8, 9, 10, 0, time.UTC)
|
||||
al := model.Album{ID: "a5", Name: "A", CreatedAt: created, UpdatedAt: updated}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(updated))
|
||||
})
|
||||
|
||||
It("falls back to CreatedAt when UpdatedAt is zero", func() {
|
||||
created := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
al := model.Album{ID: "a6", Name: "A", CreatedAt: created}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(created))
|
||||
})
|
||||
|
||||
It("falls back to ImportedAt when UpdatedAt and CreatedAt are zero", func() {
|
||||
imported := time.Date(2021, 8, 9, 10, 11, 12, 0, time.UTC)
|
||||
al := model.Album{ID: "a7", Name: "A", ImportedAt: imported}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(imported))
|
||||
})
|
||||
|
||||
It("leaves Created as zero time when all timestamps are zero", func() {
|
||||
al := model.Album{ID: "a8", Name: "A"}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created.IsZero()).To(BeTrue())
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Describe("childFromMediaFile Created field", func() {
|
||||
birth := time.Date(2018, 1, 1, 0, 0, 0, 0, time.UTC)
|
||||
|
||||
When("RecentlyAddedByModTime is false", func() {
|
||||
BeforeEach(func() {
|
||||
conf.Server.RecentlyAddedByModTime = false
|
||||
})
|
||||
|
||||
It("uses CreatedAt, not BirthTime", func() {
|
||||
created := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
mf := model.MediaFile{ID: "s1", BirthTime: birth, CreatedAt: created}
|
||||
child := childFromMediaFile(ctx, mf)
|
||||
Expect(*child.Created).To(Equal(created))
|
||||
})
|
||||
|
||||
It("falls back to UpdatedAt when CreatedAt is zero", func() {
|
||||
updated := time.Date(2019, 5, 6, 7, 8, 9, 0, time.UTC)
|
||||
mf := model.MediaFile{ID: "s2", BirthTime: birth, UpdatedAt: updated}
|
||||
child := childFromMediaFile(ctx, mf)
|
||||
Expect(*child.Created).To(Equal(updated))
|
||||
})
|
||||
|
||||
It("falls back to BirthTime when CreatedAt and UpdatedAt are zero", func() {
|
||||
mf := model.MediaFile{ID: "s3", BirthTime: birth}
|
||||
child := childFromMediaFile(ctx, mf)
|
||||
Expect(*child.Created).To(Equal(birth))
|
||||
})
|
||||
})
|
||||
|
||||
It("falls back to ImportedAt when CreatedAt and UpdatedAt are zero", func() {
|
||||
imported := time.Date(2021, 8, 9, 10, 11, 12, 0, time.UTC)
|
||||
al := model.Album{ID: "a3", Name: "A", ImportedAt: imported}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created).To(Equal(imported))
|
||||
})
|
||||
When("RecentlyAddedByModTime is true", func() {
|
||||
BeforeEach(func() {
|
||||
conf.Server.RecentlyAddedByModTime = true
|
||||
})
|
||||
|
||||
It("leaves Created as zero time when all timestamps are zero", func() {
|
||||
al := model.Album{ID: "a4", Name: "A"}
|
||||
dir := buildAlbumID3(ctx, al)
|
||||
Expect(dir.Created.IsZero()).To(BeTrue())
|
||||
It("uses UpdatedAt even when CreatedAt is also set", func() {
|
||||
created := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
updated := time.Date(2022, 6, 7, 8, 9, 10, 0, time.UTC)
|
||||
mf := model.MediaFile{ID: "s4", BirthTime: birth, CreatedAt: created, UpdatedAt: updated}
|
||||
child := childFromMediaFile(ctx, mf)
|
||||
Expect(*child.Created).To(Equal(updated))
|
||||
})
|
||||
|
||||
It("falls back to CreatedAt when UpdatedAt is zero", func() {
|
||||
created := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC)
|
||||
mf := model.MediaFile{ID: "s5", BirthTime: birth, CreatedAt: created}
|
||||
child := childFromMediaFile(ctx, mf)
|
||||
Expect(*child.Created).To(Equal(created))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user