From ea1e2b95a70074a8e4e98074736f13f0b5ea72a5 Mon Sep 17 00:00:00 2001 From: hotorcelexo Date: Sun, 16 Aug 2026 21:14:41 +0300 Subject: [PATCH 1/2] fix(db): keep album created_at in the driver's timestamp format when copying (#5867) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(db): keep album created_at in the driver's timestamp format when copying Signed-off-by: IgorPolyakov * fix(db): move created_at renormalize migration after merged migrations The migration was versioned 20260813140000, which is older than 20260815015320 (already merged). goose.UpContext runs without WithAllowMissing, so any database that already applied the newer migration would fail with "found 1 missing migrations" and db.Init would log.Fatal on startup. --------- Signed-off-by: IgorPolyakov Co-authored-by: Deluan Quintão --- ...816180040_renormalize_album_created_at.sql | 11 ++++++++ persistence/album_repository.go | 5 +++- persistence/album_repository_test.go | 26 +++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 db/migrations/20260816180040_renormalize_album_created_at.sql diff --git a/db/migrations/20260816180040_renormalize_album_created_at.sql b/db/migrations/20260816180040_renormalize_album_created_at.sql new file mode 100644 index 000000000..9af7b2e5a --- /dev/null +++ b/db/migrations/20260816180040_renormalize_album_created_at.sql @@ -0,0 +1,11 @@ +-- +goose Up + +-- Repairs album.created_at values stored in RFC3339 T-format by CopyAttributes. +-- These values sort incorrectly in "Recently Added", which compares timestamps as raw strings. + +UPDATE album SET created_at = replace(replace(created_at, 'T', ' '), 'Z', '+00:00') +WHERE created_at LIKE '%T%'; + +-- +goose Down + +SELECT 1; diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 2f0621b73..5d7aad22e 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -329,8 +329,11 @@ func (r *albumRepository) GetYears(libraryIDs ...int) ([]int, error) { } func (r *albumRepository) CopyAttributes(fromID, toID string, columns ...string) error { + // Cast values to text so go-sqlite3 does not decode datetime columns as time.Time + // and reformat them as RFC3339 when written back. + sel := slice.Map(columns, func(c string) string { return fmt.Sprintf("cast(%[1]s as text) as %[1]s", c) }) var from dbx.NullStringMap - err := r.queryOne(Select(columns...).From(r.tableName).Where(Eq{"id": fromID}), &from) + err := r.queryOne(Select(sel...).From(r.tableName).Where(Eq{"id": fromID}), &from) if err != nil { return fmt.Errorf("getting album to copy fields from: %w", err) } diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 526642aa6..f6768768d 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -19,6 +19,16 @@ import ( . "github.com/onsi/gomega" ) +// rawColumn returns a column exactly as stored, bypassing go-sqlite3's decoding of +// `datetime` columns into time.Time. +func rawColumn(r sqlRepository, id, column string) string { + var res struct{ Value string } + sel := squirrel.Select("cast(" + column + " as text) as value"). + From(r.tableName).Where(squirrel.Eq{"id": id}) + ExpectWithOffset(1, r.queryOne(sel, &res)).To(Succeed()) + return res.Value +} + var _ = Describe("AlbumRepository", func() { var albumRepo *albumRepository var ctx context.Context @@ -69,6 +79,22 @@ var _ = Describe("AlbumRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect(got.CreatedAt).To(BeTemporally("~", dstTime, time.Second)) }) + It("returns not found and leaves destination untouched when source does not exist", func() { + err := albumRepo.CopyAttributes("copy-missing", "copy-dst", "created_at") + Expect(errors.Is(err, model.ErrNotFound)).To(BeTrue()) + got, getErr := albumRepo.Get("copy-dst") + Expect(getErr).ToNot(HaveOccurred()) + Expect(got.CreatedAt).To(BeTemporally("~", dstTime, time.Second)) + }) + It("keeps the copied created_at in the driver's space-separated format", func() { + // Copying through a Go string would rewrite it as RFC3339 ("2020-01-02T03:04:05Z"), + // which string-sorts above every space-format timestamp and pins the album to the + // top of "Recently Added". + Expect(albumRepo.CopyAttributes("copy-src", "copy-dst", "created_at")).To(Succeed()) + Expect(rawColumn(albumRepo.sqlRepository, "copy-dst", "created_at")). + To(Equal(rawColumn(albumRepo.sqlRepository, "copy-src", "created_at"))) + Expect(rawColumn(albumRepo.sqlRepository, "copy-dst", "created_at")).ToNot(ContainSubstring("T")) + }) }) Describe("GetCursor", func() { From 65751d7665e3f1a85c96f661a10227a77bb655d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Mon, 17 Aug 2026 15:47:41 -0400 Subject: [PATCH 2/2] fix(playlists): chunk track deletes to stay under the SQLite variable limit (#5977) PlaylistTrackRepository.Delete built a single IN clause with one bind variable per track, so removing more tracks than SQLITE_MAX_VARIABLE_NUMBER (32766) failed with "too many SQL variables". Clients that sync a large playlist by adding the desired tracks and then removing the stale ones would get the add committed and the removal rejected, leaving the playlist with both sets of tracks and growing it on every sync. Delete now works in chunks of 200, the same size addTracks already uses, and renumbers once after the last chunk. Both callers already run inside a transaction, so the delete stays atomic. --- persistence/playlist_track_repository.go | 11 ++-- persistence/playlist_track_repository_test.go | 50 +++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index c1f6fcf69..a5e1975fd 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -2,6 +2,7 @@ package persistence import ( "database/sql" + "slices" . "github.com/Masterminds/squirrel" "github.com/deluan/rest" @@ -224,10 +225,14 @@ func (r *playlistTrackRepository) AddDiscs(discs []model.DiscID) (int, error) { return r.addMediaFileIds(clauses) } +// deleteChunkSize keeps each DELETE under SQLITE_MAX_VARIABLE_NUMBER, matching addTracks. +const deleteChunkSize = 200 + func (r *playlistTrackRepository) Delete(ids ...string) error { - err := r.delete(And{Eq{"playlist_id": r.playlistId}, Eq{"id": ids}}) - if err != nil { - return err + for chunk := range slices.Chunk(ids, deleteChunkSize) { + if err := r.delete(And{Eq{"playlist_id": r.playlistId}, Eq{"id": chunk}}); err != nil { + return err + } } return r.playlistRepo.renumber(r.playlistId) diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index a5c67b92c..88ddbff48 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -1,6 +1,8 @@ package persistence import ( + "strconv" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -8,6 +10,9 @@ import ( . "github.com/onsi/gomega" ) +// sqliteMaxVariables is SQLITE_MAX_VARIABLE_NUMBER as compiled into the driver +const sqliteMaxVariables = 32766 + var _ = Describe("PlaylistTrackRepository", func() { var repo model.PlaylistTrackRepository @@ -72,4 +77,49 @@ var _ = Describe("PlaylistTrackRepository", func() { To(Equal([]string{songRadioactivity.ID})) }) }) + + Describe("Delete", func() { + var tracks model.PlaylistTrackRepository + const numTracks = deleteChunkSize*2 + 1 + + positionsUpTo := func(n int) []string { + positions := make([]string, 0, n) + for i := 1; i <= n; i++ { + positions = append(positions, strconv.Itoa(i)) + } + return positions + } + + BeforeEach(func() { + ctx := log.NewContext(GinkgoT().Context()) + ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "userid", IsAdmin: true}) + plsRepo := NewPlaylistRepository(ctx, GetDBXBuilder()) + + pls := model.Playlist{Name: "Chunked Delete", OwnerID: "userid", OwnerName: "userid"} + Expect(plsRepo.Put(&pls)).To(Succeed()) + DeferCleanup(func() { Expect(plsRepo.Delete(pls.ID)).To(Succeed()) }) + + tracks = plsRepo.Tracks(pls.ID, false) + songIds := make([]string, numTracks) + for i := range songIds { + songIds[i] = songDayInALife.ID + } + Expect(tracks.Add(songIds)).To(Equal(numTracks)) + }) + + It("removes positions spanning several chunks, and renumbers what is left", func() { + Expect(tracks.Delete(positionsUpTo(numTracks - 1)...)).To(Succeed()) + + Expect(tracks.CountAll()).To(Equal(int64(1))) + remaining, err := tracks.GetAll(model.QueryOptions{Sort: "id"}) + Expect(err).ToNot(HaveOccurred()) + Expect(remaining[0].ID).To(Equal("1"), "the surviving track must be renumbered to position 1") + }) + + It("accepts more ids than SQLite allows as bind variables", func() { + Expect(tracks.Delete(positionsUpTo(sqliteMaxVariables + 100)...)).To(Succeed()) + + Expect(tracks.CountAll()).To(BeZero()) + }) + }) })