diff --git a/persistence/album_repository.go b/persistence/album_repository.go index dab25578..adca058c 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -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{ diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 612e459f..2705653a 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -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", diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index c9e38a1e..5c34ace5 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -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, diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index dfaf499a..18883378 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -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 diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 7f65540c..9c682369 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -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, diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index e3363972..9f62a6a7 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -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() { diff --git a/persistence/sql_annotations.go b/persistence/sql_annotations.go index fac51982..cf95d39a 100644 --- a/persistence/sql_annotations.go +++ b/persistence/sql_annotations.go @@ -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{ diff --git a/persistence/sql_annotations_test.go b/persistence/sql_annotations_test.go new file mode 100644 index 00000000..1848bbc8 --- /dev/null +++ b/persistence/sql_annotations_test.go @@ -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") + }) +})