diff --git a/db/migrations/20260629123100_recently_added_plain_indexes.sql b/db/migrations/20260629123100_recently_added_plain_indexes.sql new file mode 100644 index 000000000..731959e62 --- /dev/null +++ b/db/migrations/20260629123100_recently_added_plain_indexes.sql @@ -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); diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 99ed10877..1d65215f7 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -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 { diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index a6270933f..f72f778db 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -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") }) }) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 740ca4956..28c11f686 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -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) { diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index d99bb65a7..ba4747cdc 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -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())) + }) + }) }) diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index a76165cc1..b986200ae 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -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 { diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index ed8f257d1..3741462a2 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -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)) + }) }) })