fix(db): Include items with no annotation for starred=false, handle has_rating=false (#4921)

* fix(db): Include items with no annotation for starred=false, handle has_rating=false * hardcode starred instead * test: ensure albums and artists without annotations are included in starred and has_rating filters Signed-off-by: Deluan <deluan@navidrome.org> * refactor: replace starred and has_rating filters with annotationBoolFilter for consistency Signed-off-by: Deluan <deluan@navidrome.org> * fix: update annotationBoolFilter to handle boolean values correctly in SQL expressions Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: Deluan <deluan@navidrome.org>
2026-01-21 10:45:17 -08:00
parent 6fce30c133
commit b1b488be77
8 changed files with 342 additions and 8 deletions
@@ -119,8 +119,8 @@ var albumFilters = sync.OnceValue(func() map[string]filterFunc {
 		"artist_id":       artistFilter,
 		"year":            yearFilter,
 		"recently_played": recentlyPlayedFilter,
-		"starred":         booleanFilter,
-		"has_rating":      hasRatingFilter,
+		"starred":         annotationBoolFilter("starred"),
+		"has_rating":      annotationBoolFilter("rating"),
 		"missing":         booleanFilter,
 		"genre_id":        tagIDFilter,
 		"role_total_id":   allRolesFilter,
@@ -149,10 +149,6 @@ func recentlyPlayedFilter(string, interface{}) Sqlizer {
 	return Gt{"play_count": 0}
 }

-func hasRatingFilter(string, interface{}) Sqlizer {
-	return Gt{"rating": 0}
-}
-
 func yearFilter(_ string, value interface{}) Sqlizer {
 	return Or{
 		And{
@@ -5,6 +5,7 @@ import (
 	"time"

 	"github.com/Masterminds/squirrel"
+	"github.com/deluan/rest"
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/consts"
 	"github.com/navidrome/navidrome/model"
@@ -77,6 +78,82 @@ var _ = Describe("AlbumRepository", func() {
 		})
 	})

+	Context("Filters", func() {
+		var albumWithoutAnnotation model.Album
+
+		BeforeEach(func() {
+			// Create album without any annotation (no star, no rating)
+			albumWithoutAnnotation = model.Album{ID: "no-annotation-album", Name: "No Annotation", LibraryID: 1}
+			Expect(albumRepo.Put(&albumWithoutAnnotation)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": albumWithoutAnnotation.ID}))
+		})
+
+		Describe("starred", func() {
+			It("false includes items without annotations", func() {
+				res, err := albumRepo.ReadAll(rest.QueryOptions{
+					Filters: map[string]any{"starred": "false"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				albums := res.(model.Albums)
+
+				var found bool
+				for _, a := range albums {
+					if a.ID == albumWithoutAnnotation.ID {
+						found = true
+						break
+					}
+				}
+				Expect(found).To(BeTrue(), "Album without annotation should be included in starred=false filter")
+			})
+
+			It("true excludes items without annotations", func() {
+				res, err := albumRepo.ReadAll(rest.QueryOptions{
+					Filters: map[string]any{"starred": "true"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				albums := res.(model.Albums)
+
+				for _, a := range albums {
+					Expect(a.ID).ToNot(Equal(albumWithoutAnnotation.ID))
+				}
+			})
+		})
+
+		Describe("has_rating", func() {
+			It("false includes items without annotations", func() {
+				res, err := albumRepo.ReadAll(rest.QueryOptions{
+					Filters: map[string]any{"has_rating": "false"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				albums := res.(model.Albums)
+
+				var found bool
+				for _, a := range albums {
+					if a.ID == albumWithoutAnnotation.ID {
+						found = true
+						break
+					}
+				}
+				Expect(found).To(BeTrue(), "Album without annotation should be included in has_rating=false filter")
+			})
+
+			It("true excludes items without annotations", func() {
+				res, err := albumRepo.ReadAll(rest.QueryOptions{
+					Filters: map[string]any{"has_rating": "true"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				albums := res.(model.Albums)
+
+				for _, a := range albums {
+					Expect(a.ID).ToNot(Equal(albumWithoutAnnotation.ID))
+				}
+			})
+		})
+	})
+
 	Describe("Album.PlayCount", func() {
 		// Implementation is in withAnnotation() method
 		DescribeTable("normalizes play count when AlbumPlayCountMode is absolute",
@@ -133,7 +133,7 @@ func NewArtistRepository(ctx context.Context, db dbx.Builder) model.ArtistReposi
 	r.registerModel(&model.Artist{}, map[string]filterFunc{
 		"id":         idFilter(r.tableName),
 		"name":       fullTextFilter(r.tableName, "mbz_artist_id"),
-		"starred":    booleanFilter,
+		"starred":    annotationBoolFilter("starred"),
 		"role":       roleFilter,
 		"missing":    booleanFilter,
 		"library_id": artistLibraryIdFilter,
@@ -5,6 +5,7 @@ import (
 	"encoding/json"

 	"github.com/Masterminds/squirrel"
+	"github.com/deluan/rest"
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/model"
@@ -386,6 +387,54 @@ var _ = Describe("ArtistRepository", func() {
 			})
 		})

+		Describe("Filters", func() {
+			var artistWithoutAnnotation model.Artist
+
+			BeforeEach(func() {
+				// Create artist without any annotation
+				artistWithoutAnnotation = model.Artist{ID: "no-annotation-artist", Name: "No Annotation Artist"}
+				err := createArtistWithLibrary(repo, &artistWithoutAnnotation, 1)
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			AfterEach(func() {
+				if raw, ok := repo.(*artistRepository); ok {
+					_, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": artistWithoutAnnotation.ID}))
+				}
+			})
+
+			Describe("starred", func() {
+				It("false includes items without annotations", func() {
+					res, err := repo.(model.ResourceRepository).ReadAll(rest.QueryOptions{
+						Filters: map[string]any{"starred": "false"},
+					})
+					Expect(err).ToNot(HaveOccurred())
+					artists := res.(model.Artists)
+
+					var found bool
+					for _, a := range artists {
+						if a.ID == artistWithoutAnnotation.ID {
+							found = true
+							break
+						}
+					}
+					Expect(found).To(BeTrue(), "Artist without annotation should be included in starred=false filter")
+				})
+
+				It("true excludes items without annotations", func() {
+					res, err := repo.(model.ResourceRepository).ReadAll(rest.QueryOptions{
+						Filters: map[string]any{"starred": "true"},
+					})
+					Expect(err).ToNot(HaveOccurred())
+					artists := res.(model.Artists)
+
+					for _, a := range artists {
+						Expect(a.ID).ToNot(Equal(artistWithoutAnnotation.ID))
+					}
+				})
+			})
+		})
+
 		Describe("MBID and Text Search", func() {
 			var lib2 model.Library
 			var lr model.LibraryRepository
@@ -95,7 +95,7 @@ var mediaFileFilter = sync.OnceValue(func() map[string]filterFunc {
 	filters := map[string]filterFunc{
 		"id":         idFilter("media_file"),
 		"title":      fullTextFilter("media_file", "mbz_recording_id", "mbz_release_track_id"),
-		"starred":    booleanFilter,
+		"starred":    annotationBoolFilter("starred"),
 		"genre_id":   tagIDFilter,
 		"missing":    booleanFilter,
 		"artists_id": artistFilter,
@@ -5,6 +5,7 @@ import (
 	"time"

 	"github.com/Masterminds/squirrel"
+	"github.com/deluan/rest"
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/log"
@@ -417,6 +418,50 @@ var _ = Describe("MediaRepository", func() {
 		})
 	})

+	Context("Filters", func() {
+		var mfWithoutAnnotation model.MediaFile
+
+		BeforeEach(func() {
+			mfWithoutAnnotation = model.MediaFile{ID: "no-annotation-file", LibraryID: 1, Path: "/test/no-annotation.mp3", Title: "No Annotation"}
+			Expect(mr.Put(&mfWithoutAnnotation)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			_ = mr.Delete(mfWithoutAnnotation.ID)
+		})
+
+		Describe("starred", func() {
+			It("false includes items without annotations", func() {
+				res, err := mr.(model.ResourceRepository).ReadAll(rest.QueryOptions{
+					Filters: map[string]any{"starred": "false"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				files := res.(model.MediaFiles)
+
+				var found bool
+				for _, f := range files {
+					if f.ID == mfWithoutAnnotation.ID {
+						found = true
+						break
+					}
+				}
+				Expect(found).To(BeTrue(), "MediaFile without annotation should be included in starred=false filter")
+			})
+
+			It("true excludes items without annotations", func() {
+				res, err := mr.(model.ResourceRepository).ReadAll(rest.QueryOptions{
+					Filters: map[string]any{"starred": "true"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				files := res.(model.MediaFiles)
+
+				for _, f := range files {
+					Expect(f.ID).ToNot(Equal(mfWithoutAnnotation.ID))
+				}
+			})
+		})
+	})
+
 	Describe("Search", func() {
 		Context("text search", func() {
 			It("finds media files by title", func() {
@@ -4,6 +4,7 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"strings"
 	"time"

 	. "github.com/Masterminds/squirrel"
@@ -43,6 +44,19 @@ func (r sqlRepository) withAnnotation(query SelectBuilder, idField string) Selec
 	return query
 }

+func annotationBoolFilter(field string) func(string, any) Sqlizer {
+	return func(_ string, value any) Sqlizer {
+		v, ok := value.(string)
+		if !ok {
+			return nil
+		}
+		if strings.ToLower(v) == "true" {
+			return Expr(fmt.Sprintf("COALESCE(%s, 0) > 0", field))
+		}
+		return Expr(fmt.Sprintf("COALESCE(%s, 0) = 0", field))
+	}
+}
+
 func (r sqlRepository) annId(itemID ...string) And {
 	userID := loggedUser(r.ctx).ID
 	return And{
@@ -0,0 +1,153 @@
+package persistence
+
+import (
+	"context"
+
+	"github.com/Masterminds/squirrel"
+	"github.com/deluan/rest"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/model/request"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Annotation Filters", func() {
+	var (
+		albumRepo              *albumRepository
+		albumWithoutAnnotation model.Album
+	)
+
+	BeforeEach(func() {
+		ctx := request.WithUser(context.Background(), model.User{ID: "userid", UserName: "johndoe"})
+		albumRepo = NewAlbumRepository(ctx, GetDBXBuilder()).(*albumRepository)
+
+		// Create album without any annotation (no star, no rating)
+		albumWithoutAnnotation = model.Album{ID: "no-annotation-album", Name: "No Annotation", LibraryID: 1}
+		Expect(albumRepo.Put(&albumWithoutAnnotation)).To(Succeed())
+	})
+
+	AfterEach(func() {
+		_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": albumWithoutAnnotation.ID}))
+	})
+
+	Describe("annotationBoolFilter", func() {
+		DescribeTable("creates correct SQL expressions",
+			func(field, value string, expectedSQL string, expectedArgs []interface{}) {
+				sqlizer := annotationBoolFilter(field)(field, value)
+				sql, args, err := sqlizer.ToSql()
+				Expect(err).ToNot(HaveOccurred())
+				Expect(sql).To(Equal(expectedSQL))
+				Expect(args).To(Equal(expectedArgs))
+			},
+			Entry("starred=true", "starred", "true", "COALESCE(starred, 0) > 0", []interface{}(nil)),
+			Entry("starred=false", "starred", "false", "COALESCE(starred, 0) = 0", []interface{}(nil)),
+			Entry("starred=True (case insensitive)", "starred", "True", "COALESCE(starred, 0) > 0", []interface{}(nil)),
+			Entry("rating=true", "rating", "true", "COALESCE(rating, 0) > 0", []interface{}(nil)),
+		)
+
+		It("returns nil if value is not a string", func() {
+			sqlizer := annotationBoolFilter("starred")("starred", 123)
+			Expect(sqlizer).To(BeNil())
+		})
+	})
+
+	Describe("starredFilter", func() {
+		It("false includes items without annotations", func() {
+			albums, err := albumRepo.GetAll(model.QueryOptions{
+				Filters: annotationBoolFilter("starred")("starred", "false"),
+			})
+			Expect(err).ToNot(HaveOccurred())
+
+			var found bool
+			for _, a := range albums {
+				if a.ID == albumWithoutAnnotation.ID {
+					found = true
+					break
+				}
+			}
+			Expect(found).To(BeTrue(), "Item without annotation should be included in starred=false filter")
+		})
+
+		It("true excludes items without annotations", func() {
+			albums, err := albumRepo.GetAll(model.QueryOptions{
+				Filters: annotationBoolFilter("starred")("starred", "true"),
+			})
+			Expect(err).ToNot(HaveOccurred())
+
+			for _, a := range albums {
+				Expect(a.ID).ToNot(Equal(albumWithoutAnnotation.ID))
+			}
+		})
+	})
+
+	Describe("hasRatingFilter", func() {
+		It("false includes items without annotations", func() {
+			albums, err := albumRepo.GetAll(model.QueryOptions{
+				Filters: annotationBoolFilter("rating")("rating", "false"),
+			})
+			Expect(err).ToNot(HaveOccurred())
+
+			var found bool
+			for _, a := range albums {
+				if a.ID == albumWithoutAnnotation.ID {
+					found = true
+					break
+				}
+			}
+			Expect(found).To(BeTrue(), "Item without annotation should be included in has_rating=false filter")
+		})
+
+		It("true excludes items without annotations", func() {
+			albums, err := albumRepo.GetAll(model.QueryOptions{
+				Filters: annotationBoolFilter("rating")("rating", "true"),
+			})
+			Expect(err).ToNot(HaveOccurred())
+
+			for _, a := range albums {
+				Expect(a.ID).ToNot(Equal(albumWithoutAnnotation.ID))
+			}
+		})
+
+		It("true includes items with rating > 0", func() {
+			// Create album with rating 1
+			ratedAlbum := model.Album{ID: "rated-album", Name: "Rated Album", LibraryID: 1}
+			Expect(albumRepo.Put(&ratedAlbum)).To(Succeed())
+			Expect(albumRepo.SetRating(1, ratedAlbum.ID)).To(Succeed())
+			defer func() {
+				_, _ = albumRepo.executeSQL(squirrel.Delete("annotation").Where(squirrel.Eq{"item_id": ratedAlbum.ID}))
+				_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": ratedAlbum.ID}))
+			}()
+
+			albums, err := albumRepo.GetAll(model.QueryOptions{
+				Filters: annotationBoolFilter("rating")("rating", "true"),
+			})
+			Expect(err).ToNot(HaveOccurred())
+
+			var found bool
+			for _, a := range albums {
+				if a.ID == ratedAlbum.ID {
+					found = true
+					break
+				}
+			}
+			Expect(found).To(BeTrue(), "Album with rating 5 should be included in has_rating=true filter")
+		})
+	})
+
+	It("ignores invalid filter values (not strings)", func() {
+		res, err := albumRepo.ReadAll(rest.QueryOptions{
+			Filters: map[string]any{"starred": 123},
+		})
+		Expect(err).ToNot(HaveOccurred())
+		albums := res.(model.Albums)
+
+		var found bool
+		for _, a := range albums {
+			if a.ID == albumWithoutAnnotation.ID {
+				found = true
+				break
+			}
+		}
+		Expect(found).To(BeTrue(), "Item without annotation should be included when filter is ignored")
+	})
+})