diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 1d65215f7..31a0f5c91 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -186,8 +186,10 @@ func allRolesFilter(_ string, value any) Sqlizer { func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) { query := r.newSelect() - query = r.withAnnotation(query, "album.id") query = r.applyLibraryFilter(query) + if filtersNeedAnnotation(r.applyFilters(query, options...)) { + query = r.withAnnotation(query, "album.id") + } return r.count(query, options...) } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 56843b911..2c6b77054 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -201,7 +201,11 @@ func (r *artistRepository) selectArtist(options ...model.QueryOptions) SelectBui func (r *artistRepository) CountAll(options ...model.QueryOptions) (int64, error) { query := r.newSelect() query = r.applyLibraryFilterToArtistQuery(query) - query = r.withAnnotation(query, "artist.id") + // Only the annotation join is gated; the library_artist join above (and its count(distinct)) + // must stay, since an artist can span multiple libraries. + if filtersNeedAnnotation(r.applyFilters(query, options...)) { + query = r.withAnnotation(query, "artist.id") + } return r.count(query, options...) } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 603c5dd5e..d7b695ade 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -273,6 +273,23 @@ var _ = Describe("ArtistRepository", func() { It("returns the number of artists in the DB", func() { Expect(repo.CountAll()).To(Equal(int64(4))) }) + + It("counts starred artists when an annotation filter is present", func() { + // The Beatles (id 3) is starred for the admin user in the seed data + count, err := repo.CountAll(model.QueryOptions{ + Filters: annotationBoolFilter("starred")("starred", "true"), + }) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(1))) + }) + + It("counts with has_rating=false without a 'no such column' error (join kept)", func() { + count, err := repo.CountAll(model.QueryOptions{ + Filters: annotationBoolFilter("rating")("rating", "false"), + }) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(4))) + }) }) Describe("Exists", func() { diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 28c11f686..d7d892ed1 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -124,8 +124,11 @@ func mediaFileRecentlyAddedSort() string { func (r *mediaFileRepository) CountAll(options ...model.QueryOptions) (int64, error) { query := r.newSelect() - query = r.withAnnotation(query, "media_file.id") query = r.applyLibraryFilter(query) + // The annotation join is expensive with count(distinct) and pointless unless a filter uses it. + if filtersNeedAnnotation(r.applyFilters(query, options...)) { + query = r.withAnnotation(query, "media_file.id") + } return r.count(query, options...) } diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index ba4747cdc..80d440c41 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -45,6 +45,37 @@ var _ = Describe("MediaRepository", func() { Expect(mr.CountAll()).To(Equal(int64(13))) }) + Describe("CountAll annotation-join gating", func() { + var adminRepo model.MediaFileRepository + + BeforeEach(func() { + adminCtx := request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid", IsAdmin: true}) + adminRepo = NewMediaFileRepository(adminCtx, GetDBXBuilder()) + }) + + It("counts starred songs when an annotation filter is present", func() { + // Come Together (id 1002) is starred for the admin user in the seed data + count, err := adminRepo.CountAll(model.QueryOptions{ + Filters: annotationBoolFilter("starred")("starred", "true"), + }) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(1))) + }) + + It("counts with starred=false without a 'no such column' error (join kept)", func() { + count, err := adminRepo.CountAll(model.QueryOptions{ + Filters: annotationBoolFilter("starred")("starred", "false"), + }) + Expect(err).ToNot(HaveOccurred()) + // All songs except the one starred one + Expect(count).To(Equal(int64(12))) + }) + + It("counts unfiltered with the join dropped", func() { + Expect(adminRepo.CountAll()).To(Equal(int64(13))) + }) + }) + Describe("CountBySuffix", func() { var mp3File, flacFile1, flacFile2, flacUpperFile model.MediaFile diff --git a/persistence/sql_annotations.go b/persistence/sql_annotations.go index 07bd96975..78b7938a1 100644 --- a/persistence/sql_annotations.go +++ b/persistence/sql_annotations.go @@ -4,17 +4,61 @@ import ( "database/sql" "errors" "fmt" + "regexp" + "sort" "strings" + "sync" "time" . "github.com/Masterminds/squirrel" + "github.com/fatih/structs" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" ) const annotationTable = "annotation" +// annotationColumns are the columns withAnnotation's LEFT JOIN contributes, derived from +// model.Annotations so the set tracks schema changes. average_rating is excluded: it lives on the +// base table, not the annotation join. +var annotationColumns = sync.OnceValue(func() map[string]struct{} { + cols := map[string]struct{}{} + for name := range structs.Map(model.Annotations{}) { + if name == "average_rating" { + continue + } + cols[name] = struct{}{} + } + return cols +}) + +// annotationColumnRE matches any annotation column as a whole word. The word boundaries keep the +// base-table column average_rating from matching the annotation column rating (Go's \b treats '_' +// as a word char). It is case-insensitive because SQLite column names are, so a raw filter using +// e.g. "RATING" must still be detected. +var annotationColumnRE = sync.OnceValue(func() *regexp.Regexp { + cols := make([]string, 0, len(annotationColumns())) + for col := range annotationColumns() { + cols = append(cols, regexp.QuoteMeta(col)) + } + sort.Strings(cols) // map iteration is random; sort for a stable pattern + return regexp.MustCompile(`(?i)\b(?:` + strings.Join(cols, "|") + `)\b`) +}) + +// filtersNeedAnnotation reports whether the rendered query references an annotation column, i.e. +// whether the annotation LEFT JOIN must be kept. Scanning the rendered SQL catches every filter +// path. The placeholder column is needed because squirrel won't render a column-less SELECT; on a +// render error, keep the join to be safe. +func filtersNeedAnnotation(query SelectBuilder) bool { + sql, _, err := query.Columns("1").ToSql() + if err != nil { + return true + } + return annotationColumnRE().MatchString(sql) +} + func (r sqlRepository) withAnnotation(query SelectBuilder, idField string) SelectBuilder { userID := loggedUser(r.ctx).ID if userID == invalidUserId { diff --git a/persistence/sql_annotations_test.go b/persistence/sql_annotations_test.go index 15efc5dc7..5766f687f 100644 --- a/persistence/sql_annotations_test.go +++ b/persistence/sql_annotations_test.go @@ -150,4 +150,98 @@ var _ = Describe("Annotation Filters", func() { } Expect(found).To(BeTrue(), "Item without annotation should be included when filter is ignored") }) + + Describe("annotationColumns", func() { + It("derives the annotation join columns from model.Annotations, excluding average_rating", func() { + cols := annotationColumns() + Expect(cols).To(HaveKey("starred")) + Expect(cols).To(HaveKey("starred_at")) + Expect(cols).To(HaveKey("rating")) + Expect(cols).To(HaveKey("rated_at")) + Expect(cols).To(HaveKey("play_count")) + Expect(cols).To(HaveKey("play_date")) + Expect(cols).To(HaveLen(6), "expected exactly the 6 annotation-join columns") + Expect(cols).ToNot(HaveKey("average_rating"), "average_rating lives on the base table, not the annotation join") + }) + }) + + Describe("filtersNeedAnnotation", func() { + It("is true when the query references an annotation column", func() { + q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Eq{"starred": true}) + Expect(filtersNeedAnnotation(q)).To(BeTrue()) + }) + + It("is true for a raw expression referencing an annotation column", func() { + q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Expr("rating > 0")) + Expect(filtersNeedAnnotation(q)).To(BeTrue()) + }) + + It("is false for a query that references no annotation column", func() { + q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Eq{"missing": false}) + Expect(filtersNeedAnnotation(q)).To(BeFalse()) + }) + + It("is false for a filter on average_rating (base-table column, not the annotation rating)", func() { + // Regression: average_rating must not match the annotation column "rating". + q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Gt{"average_rating": 3}) + Expect(filtersNeedAnnotation(q)).To(BeFalse()) + }) + + It("is true when both average_rating and a real annotation column are referenced", func() { + q := squirrel.Select("count(1)").From("media_file"). + Where(squirrel.Gt{"average_rating": 3}). + Where(squirrel.Expr("COALESCE(rating, 0) > 0")) + Expect(filtersNeedAnnotation(q)).To(BeTrue()) + }) + + It("is true for uppercase/mixed-case annotation columns (SQLite is case-insensitive)", func() { + q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Expr("RATING > 0")) + Expect(filtersNeedAnnotation(q)).To(BeTrue()) + }) + + It("is false for uppercase average_rating (still excluded case-insensitively)", func() { + q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Expr("AVERAGE_RATING > 3")) + Expect(filtersNeedAnnotation(q)).To(BeFalse()) + }) + }) + + Describe("CountAll annotation-join gating", func() { + It("counts all items unfiltered (join dropped)", func() { + total, err := albumRepo.CountAll() + Expect(err).ToNot(HaveOccurred()) + Expect(total).To(BeNumerically(">=", int64(1))) + + filtered, err := albumRepo.CountAll(model.QueryOptions{ + Filters: squirrel.Eq{"album.id": albumWithoutAnnotation.ID}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(filtered).To(Equal(int64(1))) + }) + + It("counts starred items correctly (named annotation filter keeps the join)", func() { + starredAlbum := model.Album{ID: "counted-starred-album", Name: "Counted Starred", LibraryID: 1} + Expect(albumRepo.Put(&starredAlbum)).To(Succeed()) + Expect(albumRepo.SetStar(true, starredAlbum.ID)).To(Succeed()) + defer func() { + _, _ = albumRepo.executeSQL(squirrel.Delete("annotation").Where(squirrel.Eq{"item_id": starredAlbum.ID})) + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": starredAlbum.ID})) + }() + + // Exactly two albums are starred for this user: the one created above and + // albumRadioactivity (id 103) from the seed data. + count, err := albumRepo.CountAll(model.QueryOptions{ + Filters: annotationBoolFilter("starred")("starred", "true"), + }) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(2))) + }) + + It("counts via a raw annotation filter without a 'no such column' error", func() { + count, err := albumRepo.CountAll(model.QueryOptions{ + Filters: squirrel.Expr("COALESCE(rating, 0) > 0"), + }) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(BeNumerically(">=", int64(0))) + }) + }) })