From 9aa7b80d0d78aee437f5385fb0909615caf4a25b Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Nov 2023 12:13:36 -0500 Subject: [PATCH 1/4] Generalize BreakUp/RangByChunks functions --- persistence/playlist_repository.go | 4 ++-- persistence/playqueue_repository.go | 4 ++-- persistence/sql_genres.go | 10 +++++----- scanner/refresher.go | 3 +-- scanner/tag_scanner.go | 4 ++-- utils/slice/slice.go | 25 +++++++++++++++++++++++++ utils/slice/slice_test.go | 21 +++++++++++++++++++++ utils/strings.go | 25 ------------------------- utils/strings_test.go | 21 --------------------- 9 files changed, 58 insertions(+), 59 deletions(-) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index fe1ecd5e7..f2addc150 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -13,7 +13,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" - "github.com/navidrome/navidrome/utils" + "github.com/navidrome/navidrome/utils/slice" ) type playlistRepository struct { @@ -297,7 +297,7 @@ func (r *playlistRepository) updatePlaylist(playlistId string, mediaFileIds []st func (r *playlistRepository) addTracks(playlistId string, startingPos int, mediaFileIds []string) error { // Break the track list in chunks to avoid hitting SQLITE_MAX_FUNCTION_ARG limit - chunks := utils.BreakUpStringSlice(mediaFileIds, 200) + chunks := slice.BreakUp(mediaFileIds, 200) // Add new tracks, chunk by chunk pos := startingPos diff --git a/persistence/playqueue_repository.go b/persistence/playqueue_repository.go index 14bd1b9bd..beda2a832 100644 --- a/persistence/playqueue_repository.go +++ b/persistence/playqueue_repository.go @@ -9,7 +9,7 @@ import ( "github.com/beego/beego/v2/client/orm" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils" + "github.com/navidrome/navidrome/utils/slice" ) type playQueueRepository struct { @@ -113,7 +113,7 @@ func (r *playQueueRepository) loadTracks(tracks model.MediaFiles) model.MediaFil } // Break the list in chunks, up to 50 items, to avoid hitting SQLITE_MAX_FUNCTION_ARG limit - chunks := utils.BreakUpStringSlice(ids, 50) + chunks := slice.BreakUp(ids, 50) // Query each chunk of media_file ids and store results in a map mfRepo := NewMediaFileRepository(r.ctx, r.ormer) diff --git a/persistence/sql_genres.go b/persistence/sql_genres.go index 397e43368..be245721a 100644 --- a/persistence/sql_genres.go +++ b/persistence/sql_genres.go @@ -3,7 +3,7 @@ package persistence import ( . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils" + "github.com/navidrome/navidrome/utils/slice" ) func (r sqlRepository) withGenres(sql SelectBuilder) SelectBuilder { @@ -25,7 +25,7 @@ func (r *sqlRepository) updateGenres(id string, tableName string, genres model.G for _, g := range genres { genreIds = append(genreIds, g.ID) } - err = utils.RangeByChunks(genreIds, 100, func(ids []string) error { + err = slice.RangeByChunks(genreIds, 100, func(ids []string) error { ins := Insert(tableName+"_genres").Columns("genre_id", tableName+"_id") for _, gid := range ids { ins = ins.Values(gid, id) @@ -45,7 +45,7 @@ func (r *sqlRepository) loadMediaFileGenres(mfs *model.MediaFiles) error { m[mf.ID] = mf } - return utils.RangeByChunks(ids, 900, func(ids []string) error { + return slice.RangeByChunks(ids, 900, func(ids []string) error { sql := Select("g.*", "mg.media_file_id").From("genre g").Join("media_file_genres mg on mg.genre_id = g.id"). Where(Eq{"mg.media_file_id": ids}).OrderBy("mg.media_file_id", "mg.rowid") var genres []struct { @@ -74,7 +74,7 @@ func (r *sqlRepository) loadAlbumGenres(mfs *model.Albums) error { m[mf.ID] = mf } - return utils.RangeByChunks(ids, 900, func(ids []string) error { + return slice.RangeByChunks(ids, 900, func(ids []string) error { sql := Select("g.*", "ag.album_id").From("genre g").Join("album_genres ag on ag.genre_id = g.id"). Where(Eq{"ag.album_id": ids}).OrderBy("ag.album_id", "ag.rowid") var genres []struct { @@ -103,7 +103,7 @@ func (r *sqlRepository) loadArtistGenres(mfs *model.Artists) error { m[mf.ID] = mf } - return utils.RangeByChunks(ids, 900, func(ids []string) error { + return slice.RangeByChunks(ids, 900, func(ids []string) error { sql := Select("g.*", "ag.artist_id").From("genre g").Join("artist_genres ag on ag.genre_id = g.id"). Where(Eq{"ag.artist_id": ids}).OrderBy("ag.artist_id", "ag.rowid") var genres []struct { diff --git a/scanner/refresher.go b/scanner/refresher.go index ec17fc1e6..3a35e8fea 100644 --- a/scanner/refresher.go +++ b/scanner/refresher.go @@ -12,7 +12,6 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/slice" "golang.org/x/exp/maps" ) @@ -71,7 +70,7 @@ func (r *refresher) flushMap(ctx context.Context, m map[string]struct{}, entity } ids := maps.Keys(m) - chunks := utils.BreakUpStringSlice(ids, 100) + chunks := slice.BreakUp(ids, 100) for _, chunk := range chunks { err := refresh(ctx, chunk...) if err != nil { diff --git a/scanner/tag_scanner.go b/scanner/tag_scanner.go index 1317581bc..5a719acd4 100644 --- a/scanner/tag_scanner.go +++ b/scanner/tag_scanner.go @@ -19,7 +19,7 @@ import ( "github.com/navidrome/navidrome/scanner/metadata" _ "github.com/navidrome/navidrome/scanner/metadata/ffmpeg" _ "github.com/navidrome/navidrome/scanner/metadata/taglib" - "github.com/navidrome/navidrome/utils" + "github.com/navidrome/navidrome/utils/slice" ) type TagScanner struct { @@ -351,7 +351,7 @@ func (s *TagScanner) addOrUpdateTracksInDB( log.Trace(ctx, "Updating mediaFiles in DB", "dir", dir, "numFiles", len(filesToUpdate)) // Break the file list in chunks to avoid calling ffmpeg with too many parameters - chunks := utils.BreakUpStringSlice(filesToUpdate, filesBatchSize) + chunks := slice.BreakUp(filesToUpdate, filesBatchSize) for _, chunk := range chunks { // Load tracks Metadata from the folder newTracks, err := s.loadTracks(chunk) diff --git a/utils/slice/slice.go b/utils/slice/slice.go index cd012330b..78169685f 100644 --- a/utils/slice/slice.go +++ b/utils/slice/slice.go @@ -54,3 +54,28 @@ func Move[T any](slice []T, srcIndex int, dstIndex int) []T { value := slice[srcIndex] return Insert(Remove(slice, srcIndex), value, dstIndex) } + +func BreakUp[T any](items []T, chunkSize int) [][]T { + numTracks := len(items) + var chunks [][]T + for i := 0; i < numTracks; i += chunkSize { + end := i + chunkSize + if end > numTracks { + end = numTracks + } + + chunks = append(chunks, items[i:end]) + } + return chunks +} + +func RangeByChunks[T any](items []T, chunkSize int, cb func([]T) error) error { + chunks := BreakUp(items, chunkSize) + for _, chunk := range chunks { + err := cb(chunk) + if err != nil { + return err + } + } + return nil +} diff --git a/utils/slice/slice_test.go b/utils/slice/slice_test.go index a27416f3d..7436a34ee 100644 --- a/utils/slice/slice_test.go +++ b/utils/slice/slice_test.go @@ -69,4 +69,25 @@ var _ = Describe("Slice Utils", func() { Expect(slice.Move([]string{"1", "2", "3"}, 1, 1)).To(ConsistOf("1", "2", "3")) }) }) + + Describe("BreakUp", func() { + It("returns no chunks if slice is empty", func() { + var s []string + chunks := slice.BreakUp(s, 10) + Expect(chunks).To(HaveLen(0)) + }) + It("returns the slice in one chunk if len < chunkSize", func() { + s := []string{"a", "b", "c"} + chunks := slice.BreakUp(s, 10) + Expect(chunks).To(HaveLen(1)) + Expect(chunks[0]).To(ConsistOf("a", "b", "c")) + }) + It("breaks up the slice if len > chunkSize", func() { + s := []string{"a", "b", "c", "d", "e"} + chunks := slice.BreakUp(s, 3) + Expect(chunks).To(HaveLen(2)) + Expect(chunks[0]).To(ConsistOf("a", "b", "c")) + Expect(chunks[1]).To(ConsistOf("d", "e")) + }) + }) }) diff --git a/utils/strings.go b/utils/strings.go index 9fbe0d448..1bd26b10a 100644 --- a/utils/strings.go +++ b/utils/strings.go @@ -17,31 +17,6 @@ func NoArticle(name string) string { return name } -func BreakUpStringSlice(items []string, chunkSize int) [][]string { - numTracks := len(items) - var chunks [][]string - for i := 0; i < numTracks; i += chunkSize { - end := i + chunkSize - if end > numTracks { - end = numTracks - } - - chunks = append(chunks, items[i:end]) - } - return chunks -} - -func RangeByChunks(items []string, chunkSize int, cb func([]string) error) error { - chunks := BreakUpStringSlice(items, chunkSize) - for _, chunk := range chunks { - err := cb(chunk) - if err != nil { - return err - } - } - return nil -} - func LongestCommonPrefix(list []string) string { if len(list) == 0 { return "" diff --git a/utils/strings_test.go b/utils/strings_test.go index 0983faa29..4b24dab0c 100644 --- a/utils/strings_test.go +++ b/utils/strings_test.go @@ -35,27 +35,6 @@ var _ = Describe("Strings", func() { }) }) - Describe("BreakUpStringSlice", func() { - It("returns no chunks if slice is empty", func() { - var slice []string - chunks := BreakUpStringSlice(slice, 10) - Expect(chunks).To(HaveLen(0)) - }) - It("returns the slice in one chunk if len < chunkSize", func() { - slice := []string{"a", "b", "c"} - chunks := BreakUpStringSlice(slice, 10) - Expect(chunks).To(HaveLen(1)) - Expect(chunks[0]).To(ConsistOf("a", "b", "c")) - }) - It("breaks up the slice if len > chunkSize", func() { - slice := []string{"a", "b", "c", "d", "e"} - chunks := BreakUpStringSlice(slice, 3) - Expect(chunks).To(HaveLen(2)) - Expect(chunks[0]).To(ConsistOf("a", "b", "c")) - Expect(chunks[1]).To(ConsistOf("d", "e")) - }) - }) - Describe("LongestCommonPrefix", func() { It("finds the longest common prefix", func() { Expect(LongestCommonPrefix(testPaths)).To(Equal("/Music/iTunes 1/iTunes Media/Music/")) From b964018cd70597e8b7799797d33bef24d0f20dd5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Nov 2023 13:54:38 -0500 Subject: [PATCH 2/4] Show SQL errors in queryAll --- persistence/sql_base_repository.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index d1ecf2d18..162bc4385 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -168,7 +168,7 @@ func (r sqlRepository) queryAll(sq Sqlizer, response interface{}) error { r.logSQL(query, args, nil, c, start) return model.ErrNotFound } - r.logSQL(query, args, nil, c, start) + r.logSQL(query, args, err, c, start) return err } From 8c8e1ea701456870fd4591cf619d74a1dc58d8e4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Nov 2023 22:29:05 -0500 Subject: [PATCH 3/4] Replace `COUNT(DISTINCT primary_key)` with `COUNT(*)` --- persistence/album_repository.go | 1 - persistence/artist_repository.go | 1 - persistence/mediafile_repository.go | 1 - persistence/sql_base_repository.go | 3 ++- 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 8f0fffc4a..ca05bd283 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -76,7 +76,6 @@ func artistFilter(field string, value interface{}) Sqlizer { func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) { sql := r.newSelectWithAnnotation("album.id") - sql = r.withGenres(sql) return r.count(sql, options...) } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index e4a0a0f76..6097ae576 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -52,7 +52,6 @@ func (r *artistRepository) selectArtist(options ...model.QueryOptions) SelectBui func (r *artistRepository) CountAll(options ...model.QueryOptions) (int64, error) { sql := r.newSelectWithAnnotation("artist.id") - sql = r.withGenres(sql) return r.count(sql, options...) } diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index d242a2813..783beb7f7 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -40,7 +40,6 @@ func NewMediaFileRepository(ctx context.Context, o orm.QueryExecutor) *mediaFile func (r *mediaFileRepository) CountAll(options ...model.QueryOptions) (int64, error) { sql := r.newSelectWithAnnotation("media_file.id") - sql = r.withGenres(sql) return r.count(sql, options...) } diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 162bc4385..2774089ae 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -180,7 +180,8 @@ func (r sqlRepository) exists(existsQuery SelectBuilder) (bool, error) { } func (r sqlRepository) count(countQuery SelectBuilder, options ...model.QueryOptions) (int64, error) { - countQuery = countQuery.Columns("count(distinct " + r.tableName + ".id) as count").From(r.tableName) + countQuery = countQuery. + RemoveColumns().Columns("count(*) as count").From(r.tableName) countQuery = r.applyFilters(countQuery, options...) var res struct{ Count int64 } err := r.queryOne(countQuery, &res) From 28dc98dec46a83d6163c5943d2522015b76969c4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Nov 2023 23:08:20 -0500 Subject: [PATCH 4/4] Revert "Replace `COUNT(DISTINCT primary_key)` with `COUNT(*)`" Genres are required as part of the count queries, so filter by genres work --- persistence/album_repository.go | 1 + persistence/artist_repository.go | 1 + persistence/mediafile_repository.go | 1 + persistence/sql_base_repository.go | 3 ++- 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index ca05bd283..e2eae06d4 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -76,6 +76,7 @@ func artistFilter(field string, value interface{}) Sqlizer { func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) { sql := r.newSelectWithAnnotation("album.id") + sql = r.withGenres(sql) // Required for filtering by genre return r.count(sql, options...) } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 6097ae576..9b0d72316 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -52,6 +52,7 @@ func (r *artistRepository) selectArtist(options ...model.QueryOptions) SelectBui func (r *artistRepository) CountAll(options ...model.QueryOptions) (int64, error) { sql := r.newSelectWithAnnotation("artist.id") + sql = r.withGenres(sql) // Required for filtering by genre return r.count(sql, options...) } diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 783beb7f7..9ebbf68b3 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -40,6 +40,7 @@ func NewMediaFileRepository(ctx context.Context, o orm.QueryExecutor) *mediaFile func (r *mediaFileRepository) CountAll(options ...model.QueryOptions) (int64, error) { sql := r.newSelectWithAnnotation("media_file.id") + sql = r.withGenres(sql) // Required for filtering by genre return r.count(sql, options...) } diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 2774089ae..d0ecd37fb 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -181,7 +181,8 @@ func (r sqlRepository) exists(existsQuery SelectBuilder) (bool, error) { func (r sqlRepository) count(countQuery SelectBuilder, options ...model.QueryOptions) (int64, error) { countQuery = countQuery. - RemoveColumns().Columns("count(*) as count").From(r.tableName) + RemoveColumns().Columns("count(distinct " + r.tableName + ".id) as count"). + From(r.tableName) countQuery = r.applyFilters(countQuery, options...) var res struct{ Count int64 } err := r.queryOne(countQuery, &res)