From dd7ae6ce2bad7c66898e237fecb5b04c33644be4 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 16 Jul 2025 10:44:40 -0400 Subject: [PATCH] refactor: genre and tag repositories. add comprehensive tests Signed-off-by: Deluan --- persistence/genre_repository.go | 27 +-- persistence/genre_repository_test.go | 250 +++++++++++++++++++++++++++ persistence/sql_tags.go | 69 ++++++++ persistence/tag_repository.go | 38 +--- persistence/tag_repository_test.go | 239 +++++++++++++++++++++++++ 5 files changed, 569 insertions(+), 54 deletions(-) create mode 100644 persistence/genre_repository_test.go create mode 100644 persistence/tag_repository_test.go diff --git a/persistence/genre_repository.go b/persistence/genre_repository.go index e92e1491a..c3e4f1049 100644 --- a/persistence/genre_repository.go +++ b/persistence/genre_repository.go @@ -10,20 +10,14 @@ import ( ) type genreRepository struct { - sqlRepository + *baseTagRepository } func NewGenreRepository(ctx context.Context, db dbx.Builder) model.GenreRepository { - r := &genreRepository{} - r.ctx = ctx - r.db = db - r.registerModel(&model.Tag{}, map[string]filterFunc{ - "name": containsFilter("tag_value"), - }) - r.setSortMappings(map[string]string{ - "name": "tag_name", - }) - return r + genreFilter := model.TagGenre + return &genreRepository{ + baseTagRepository: newBaseTagRepository(ctx, db, &genreFilter), + } } func (r *genreRepository) selectGenre(opt ...model.QueryOptions) SelectBuilder { @@ -33,8 +27,7 @@ func (r *genreRepository) selectGenre(opt ...model.QueryOptions) SelectBuilder { "tag_value as name", "album_count", "media_file_count as song_count", - ). - Where(Eq{"tag.tag_name": model.TagGenre}) + ) } func (r *genreRepository) GetAll(opt ...model.QueryOptions) (model.Genres, error) { @@ -44,9 +37,7 @@ func (r *genreRepository) GetAll(opt ...model.QueryOptions) (model.Genres, error return res, err } -func (r *genreRepository) Count(options ...rest.QueryOptions) (int64, error) { - return r.count(r.selectGenre(), r.parseRestOptions(r.ctx, options...)) -} +// Override ResourceRepository methods to return Genre objects instead of Tag objects func (r *genreRepository) Read(id string) (interface{}, error) { sel := r.selectGenre().Columns("*").Where(Eq{"id": id}) @@ -59,10 +50,6 @@ func (r *genreRepository) ReadAll(options ...rest.QueryOptions) (interface{}, er return r.GetAll(r.parseRestOptions(r.ctx, options...)) } -func (r *genreRepository) EntityName() string { - return r.tableName -} - func (r *genreRepository) NewInstance() interface{} { return &model.Genre{} } diff --git a/persistence/genre_repository_test.go b/persistence/genre_repository_test.go new file mode 100644 index 000000000..7c0bcb130 --- /dev/null +++ b/persistence/genre_repository_test.go @@ -0,0 +1,250 @@ +package persistence + +import ( + "context" + "slices" + "strings" + + "github.com/Masterminds/squirrel" + "github.com/deluan/rest" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("GenreRepository", func() { + var repo model.GenreRepository + var restRepo model.ResourceRepository + var tagRepo model.TagRepository + var ctx context.Context + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + ctx = request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid", UserName: "johndoe"}) + genreRepo := NewGenreRepository(ctx, GetDBXBuilder()) + repo = genreRepo + restRepo = genreRepo.(model.ResourceRepository) + tagRepo = NewTagRepository(ctx, GetDBXBuilder()) + + // Clear any existing tags to ensure test isolation + db := GetDBXBuilder() + _, err := db.NewQuery("DELETE FROM tag").Execute() + Expect(err).ToNot(HaveOccurred()) + + // Add comprehensive test data that covers all test scenarios + newTag := func(name, value string) model.Tag { + return model.Tag{ID: id.NewTagID(name, value), TagName: model.TagName(name), TagValue: value} + } + + err = tagRepo.Add( + newTag("genre", "rock"), + newTag("genre", "pop"), + newTag("genre", "jazz"), + newTag("genre", "electronic"), + newTag("genre", "classical"), + newTag("genre", "ambient"), + newTag("genre", "techno"), + newTag("genre", "house"), + newTag("genre", "trance"), + newTag("genre", "Alternative Rock"), + newTag("genre", "Blues"), + newTag("genre", "Country"), + // These should not be counted as genres + newTag("mood", "happy"), + newTag("mood", "ambient"), + ) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("GetAll", func() { + It("should return all genres", func() { + genres, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(genres).To(HaveLen(12)) + + // Verify that all returned items are genres (TagName = "genre") + genreNames := make([]string, len(genres)) + for i, genre := range genres { + genreNames[i] = genre.Name + } + Expect(genreNames).To(ContainElement("rock")) + Expect(genreNames).To(ContainElement("pop")) + Expect(genreNames).To(ContainElement("jazz")) + // Should not contain mood tags + Expect(genreNames).ToNot(ContainElement("happy")) + }) + + It("should support query options", func() { + // Test with limiting results + genres, err := repo.GetAll(model.QueryOptions{Max: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(genres).To(HaveLen(1)) + }) + + It("should handle empty results gracefully", func() { + // Clear all genre tags + _, err := GetDBXBuilder().NewQuery("DELETE FROM tag WHERE tag_name = 'genre'").Execute() + Expect(err).ToNot(HaveOccurred()) + + genres, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(genres).To(BeEmpty()) + }) + Describe("filtering and sorting", func() { + It("should filter by name using like match", func() { + // Test filtering by partial name match using the "name" filter which maps to containsFilter("tag_value") + options := model.QueryOptions{ + Filters: squirrel.Like{"tag_value": "%rock%"}, // Direct field access + } + genres, err := repo.GetAll(options) + Expect(err).ToNot(HaveOccurred()) + Expect(genres).To(HaveLen(2)) // Should match "rock" and "Alternative Rock" + + // Verify all returned genres contain "rock" in their name + for _, genre := range genres { + Expect(strings.ToLower(genre.Name)).To(ContainSubstring("rock")) + } + }) + + It("should sort by name in ascending order", func() { + // Test sorting by name with the fixed mapping + options := model.QueryOptions{ + Filters: squirrel.Like{"tag_value": "%e%"}, // Should match genres containing "e" + Sort: "name", + } + genres, err := repo.GetAll(options) + Expect(err).ToNot(HaveOccurred()) + Expect(genres).To(HaveLen(7)) + + Expect(slices.IsSortedFunc(genres, func(a, b model.Genre) int { + return strings.Compare(b.Name, a.Name) // Inverted to check descending order + })) + }) + + It("should sort by name in descending order", func() { + // Test sorting by name in descending order + options := model.QueryOptions{ + Filters: squirrel.Like{"tag_value": "%e%"}, // Should match genres containing "e" + Sort: "name", + Order: "desc", + } + genres, err := repo.GetAll(options) + Expect(err).ToNot(HaveOccurred()) + Expect(genres).To(HaveLen(7)) + + Expect(slices.IsSortedFunc(genres, func(a, b model.Genre) int { + return strings.Compare(a.Name, b.Name) + })) + }) + }) + }) + + Describe("Count", func() { + It("should return correct count of genres", func() { + count, err := restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(12))) // We have 12 genre tags + }) + + It("should handle zero count", func() { + // Clear all genre tags + _, err := GetDBXBuilder().NewQuery("DELETE FROM tag WHERE tag_name = 'genre'").Execute() + Expect(err).ToNot(HaveOccurred()) + + count, err := restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(BeZero()) + }) + + It("should only count genre tags", func() { + // Add a non-genre tag + nonGenreTag := model.Tag{ + ID: id.NewTagID("mood", "energetic"), + TagName: "mood", + TagValue: "energetic", + } + err := tagRepo.Add(nonGenreTag) + Expect(err).ToNot(HaveOccurred()) + + count, err := restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + // Count should not include the mood tag + Expect(count).To(Equal(int64(12))) // Should still be 12 genre tags + }) + + It("should filter by name using like match", func() { + // Test filtering by partial name match using the "name" filter which maps to containsFilter("tag_value") + options := rest.QueryOptions{ + Filters: map[string]interface{}{"name": "%rock%"}, + } + count, err := restRepo.Count(options) + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(BeNumerically("==", 2)) + }) + }) + + Describe("Read", func() { + It("should return existing genre", func() { + // Use one of the existing genres from our consolidated dataset + genreID := id.NewTagID("genre", "rock") + result, err := restRepo.Read(genreID) + Expect(err).ToNot(HaveOccurred()) + genre := result.(*model.Genre) + Expect(genre.ID).To(Equal(genreID)) + Expect(genre.Name).To(Equal("rock")) + }) + + It("should return error for non-existent genre", func() { + _, err := restRepo.Read("non-existent-id") + Expect(err).To(HaveOccurred()) + }) + + It("should not return non-genre tags", func() { + moodID := id.NewTagID("mood", "happy") // This exists as a mood tag, not genre + _, err := restRepo.Read(moodID) + Expect(err).To(HaveOccurred()) // Should not find it as a genre + }) + }) + + Describe("ReadAll", func() { + It("should return all genres through ReadAll", func() { + result, err := restRepo.ReadAll() + Expect(err).ToNot(HaveOccurred()) + genres := result.(model.Genres) + Expect(genres).To(HaveLen(12)) // We have 12 genre tags + + genreNames := make([]string, len(genres)) + for i, genre := range genres { + genreNames[i] = genre.Name + } + // Check for some of our consolidated dataset genres + Expect(genreNames).To(ContainElement("rock")) + Expect(genreNames).To(ContainElement("pop")) + Expect(genreNames).To(ContainElement("jazz")) + }) + + It("should support rest query options", func() { + result, err := restRepo.ReadAll() + Expect(err).ToNot(HaveOccurred()) + Expect(result).ToNot(BeNil()) + }) + }) + + Describe("EntityName", func() { + It("should return correct entity name", func() { + name := restRepo.EntityName() + Expect(name).To(Equal("tag")) // Genre repository uses tag table + }) + }) + + Describe("NewInstance", func() { + It("should return new genre instance", func() { + instance := restRepo.NewInstance() + Expect(instance).To(BeAssignableToTypeOf(&model.Genre{})) + }) + }) +}) diff --git a/persistence/sql_tags.go b/persistence/sql_tags.go index d7b48f23e..3d080b392 100644 --- a/persistence/sql_tags.go +++ b/persistence/sql_tags.go @@ -1,12 +1,15 @@ package persistence import ( + "context" "encoding/json" "fmt" "strings" . "github.com/Masterminds/squirrel" + "github.com/deluan/rest" "github.com/navidrome/navidrome/model" + "github.com/pocketbase/dbx" ) // Format of a tag in the DB @@ -55,3 +58,69 @@ func tagIDFilter(name string, idValue any) Sqlizer { }, ) } + +// baseTagRepository provides common functionality for all tag-based repositories. +// It handles CRUD operations with optional filtering by tag name. +type baseTagRepository struct { + sqlRepository + tagFilter *model.TagName // nil = no filter (all tags), non-nil = filter by specific tag name +} + +// newBaseTagRepository creates a new base tag repository with optional tag filtering. +// If tagFilter is nil, the repository will work with all tags. +// If tagFilter is provided, the repository will only work with tags of that specific name. +func newBaseTagRepository(ctx context.Context, db dbx.Builder, tagFilter *model.TagName) *baseTagRepository { + r := &baseTagRepository{ + tagFilter: tagFilter, + } + r.ctx = ctx + r.db = db + r.tableName = "tag" + r.registerModel(&model.Tag{}, map[string]filterFunc{ + "name": containsFilter("tag_value"), + }) + r.setSortMappings(map[string]string{ + "name": "tag_value", + }) + return r +} + +// newSelect overrides the base implementation to conditionally apply tag name filtering. +func (r *baseTagRepository) newSelect(options ...model.QueryOptions) SelectBuilder { + sq := r.sqlRepository.newSelect(options...) + if r.tagFilter != nil { + sq = sq.Where(Eq{"tag_name": *r.tagFilter}) + } + return sq +} + +// ResourceRepository interface implementation + +func (r *baseTagRepository) Count(options ...rest.QueryOptions) (int64, error) { + return r.count(r.newSelect(), r.parseRestOptions(r.ctx, options...)) +} + +func (r *baseTagRepository) Read(id string) (interface{}, error) { + query := r.newSelect().Columns("*").Where(Eq{"id": id}) + var res model.Tag + err := r.queryOne(query, &res) + return &res, err +} + +func (r *baseTagRepository) ReadAll(options ...rest.QueryOptions) (interface{}, error) { + query := r.newSelect(r.parseRestOptions(r.ctx, options...)).Columns("*") + var res model.TagList + err := r.queryAll(query, &res) + return res, err +} + +func (r *baseTagRepository) EntityName() string { + return "tag" +} + +func (r *baseTagRepository) NewInstance() interface{} { + return model.Tag{} +} + +// Interface compliance check +var _ model.ResourceRepository = (*baseTagRepository)(nil) diff --git a/persistence/tag_repository.go b/persistence/tag_repository.go index d63584af0..f56a3a24c 100644 --- a/persistence/tag_repository.go +++ b/persistence/tag_repository.go @@ -7,23 +7,19 @@ import ( "time" . "github.com/Masterminds/squirrel" - "github.com/deluan/rest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/pocketbase/dbx" ) type tagRepository struct { - sqlRepository + *baseTagRepository } func NewTagRepository(ctx context.Context, db dbx.Builder) model.TagRepository { - r := &tagRepository{} - r.ctx = ctx - r.db = db - r.tableName = "tag" - r.registerModel(&model.Tag{}, nil) - return r + return &tagRepository{ + baseTagRepository: newBaseTagRepository(ctx, db, nil), // nil = no filter, works with all tags + } } func (r *tagRepository) Add(tags ...model.Tag) error { @@ -87,30 +83,4 @@ func (r *tagRepository) purgeUnused() error { return err } -func (r *tagRepository) Count(options ...rest.QueryOptions) (int64, error) { - return r.count(r.newSelect(), r.parseRestOptions(r.ctx, options...)) -} - -func (r *tagRepository) Read(id string) (interface{}, error) { - query := r.newSelect().Columns("*").Where(Eq{"id": id}) - var res model.Tag - err := r.queryOne(query, &res) - return &res, err -} - -func (r *tagRepository) ReadAll(options ...rest.QueryOptions) (interface{}, error) { - query := r.newSelect(r.parseRestOptions(r.ctx, options...)).Columns("*") - var res model.TagList - err := r.queryAll(query, &res) - return res, err -} - -func (r *tagRepository) EntityName() string { - return "tag" -} - -func (r *tagRepository) NewInstance() interface{} { - return model.Tag{} -} - var _ model.ResourceRepository = &tagRepository{} diff --git a/persistence/tag_repository_test.go b/persistence/tag_repository_test.go new file mode 100644 index 000000000..07b3cb1bc --- /dev/null +++ b/persistence/tag_repository_test.go @@ -0,0 +1,239 @@ +package persistence + +import ( + "context" + "slices" + "strings" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("TagRepository", func() { + var repo model.TagRepository + var restRepo model.ResourceRepository + var ctx context.Context + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + ctx = request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid", UserName: "johndoe"}) + tagRepo := NewTagRepository(ctx, GetDBXBuilder()) + repo = tagRepo + restRepo = tagRepo.(model.ResourceRepository) + + // Clean the database before each test to ensure isolation + db := GetDBXBuilder() + _, err := db.NewQuery("DELETE FROM tag").Execute() + Expect(err).ToNot(HaveOccurred()) + + // Add comprehensive test data that covers all test scenarios + newTag := func(name, value string) model.Tag { + return model.Tag{ID: id.NewTagID(name, value), TagName: model.TagName(name), TagValue: value} + } + + err = repo.Add( + // Genre tags + newTag("genre", "rock"), + newTag("genre", "pop"), + newTag("genre", "jazz"), + newTag("genre", "electronic"), + newTag("genre", "classical"), + newTag("genre", "ambient"), + newTag("genre", "techno"), + newTag("genre", "house"), + newTag("genre", "trance"), + newTag("genre", "Alternative Rock"), + newTag("genre", "Blues"), + newTag("genre", "Country"), + // Mood tags + newTag("mood", "happy"), + newTag("mood", "sad"), + newTag("mood", "energetic"), + newTag("mood", "calm"), + // Other tag types + newTag("instrument", "guitar"), + newTag("instrument", "piano"), + newTag("decade", "1980s"), + newTag("decade", "1990s"), + ) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("Add", func() { + It("should handle adding new tags", func() { + newTag := model.Tag{ + ID: id.NewTagID("genre", "experimental"), + TagName: "genre", + TagValue: "experimental", + } + + err := repo.Add(newTag) + Expect(err).ToNot(HaveOccurred()) + + // Verify tag was added + result, err := restRepo.Read(newTag.ID) + Expect(err).ToNot(HaveOccurred()) + resultTag := result.(*model.Tag) + Expect(resultTag.TagValue).To(Equal("experimental")) + + // Check count increased + count, err := restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(21))) // 20 from dataset + 1 new + }) + + It("should handle duplicate tags gracefully", func() { + // Try to add a duplicate tag + duplicateTag := model.Tag{ + ID: id.NewTagID("genre", "rock"), // This already exists + TagName: "genre", + TagValue: "rock", + } + + count, err := restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(20))) // Still 20 tags + + err = repo.Add(duplicateTag) + Expect(err).ToNot(HaveOccurred()) // Should not error + + // Count should remain the same + count, err = restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(20))) // Still 20 tags + }) + }) + + Describe("UpdateCounts", func() { + It("should update tag counts successfully", func() { + err := repo.UpdateCounts() + Expect(err).ToNot(HaveOccurred()) + }) + + It("should handle empty database gracefully", func() { + // Clear the database first + db := GetDBXBuilder() + _, err := db.NewQuery("DELETE FROM tag").Execute() + Expect(err).ToNot(HaveOccurred()) + + err = repo.UpdateCounts() + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Describe("Count", func() { + It("should return correct count of tags", func() { + count, err := restRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(20))) // From the test dataset + }) + }) + + Describe("Read", func() { + It("should return existing tag", func() { + rockID := id.NewTagID("genre", "rock") + result, err := restRepo.Read(rockID) + Expect(err).ToNot(HaveOccurred()) + resultTag := result.(*model.Tag) + Expect(resultTag.ID).To(Equal(rockID)) + Expect(resultTag.TagName).To(Equal(model.TagName("genre"))) + Expect(resultTag.TagValue).To(Equal("rock")) + }) + + It("should return error for non-existent tag", func() { + _, err := restRepo.Read("non-existent-id") + Expect(err).To(HaveOccurred()) + }) + }) + + Describe("ReadAll", func() { + It("should return all tags from dataset", func() { + result, err := restRepo.ReadAll() + Expect(err).ToNot(HaveOccurred()) + tags := result.(model.TagList) + Expect(tags).To(HaveLen(20)) + }) + + It("should filter tags by partial value correctly", func() { + options := rest.QueryOptions{ + Filters: map[string]interface{}{"name": "%rock%"}, // Tags containing 'rock' + } + result, err := restRepo.ReadAll(options) + Expect(err).ToNot(HaveOccurred()) + tags := result.(model.TagList) + Expect(tags).To(HaveLen(2)) // "rock" and "Alternative Rock" + + // Verify all returned tags contain 'rock' in their value + for _, tag := range tags { + Expect(strings.ToLower(tag.TagValue)).To(ContainSubstring("rock")) + } + }) + + It("should filter tags by partial value using LIKE", func() { + options := rest.QueryOptions{ + Filters: map[string]interface{}{"name": "%e%"}, // Tags containing 'e' + } + result, err := restRepo.ReadAll(options) + Expect(err).ToNot(HaveOccurred()) + tags := result.(model.TagList) + Expect(tags).To(HaveLen(8)) // electronic, house, trance, energetic, Blues, decade x2, Alternative Rock + + // Verify all returned tags contain 'e' in their value + for _, tag := range tags { + Expect(strings.ToLower(tag.TagValue)).To(ContainSubstring("e")) + } + }) + + It("should sort tags by value ascending", func() { + options := rest.QueryOptions{ + Filters: map[string]interface{}{"name": "%r%"}, // Tags containing 'r' + Sort: "name", + Order: "asc", + } + result, err := restRepo.ReadAll(options) + Expect(err).ToNot(HaveOccurred()) + tags := result.(model.TagList) + Expect(tags).To(HaveLen(7)) + + Expect(slices.IsSortedFunc(tags, func(a, b model.Tag) int { + return strings.Compare(strings.ToLower(a.TagValue), strings.ToLower(b.TagValue)) + })) + }) + + It("should sort tags by value descending", func() { + options := rest.QueryOptions{ + Filters: map[string]interface{}{"name": "%r%"}, // Tags containing 'r' + Sort: "name", + Order: "desc", + } + result, err := restRepo.ReadAll(options) + Expect(err).ToNot(HaveOccurred()) + tags := result.(model.TagList) + Expect(tags).To(HaveLen(7)) + + Expect(slices.IsSortedFunc(tags, func(a, b model.Tag) int { + return strings.Compare(strings.ToLower(b.TagValue), strings.ToLower(a.TagValue)) // Descending order + })) + }) + }) + + Describe("EntityName", func() { + It("should return correct entity name", func() { + name := restRepo.EntityName() + Expect(name).To(Equal("tag")) + }) + }) + + Describe("NewInstance", func() { + It("should return new tag instance", func() { + instance := restRepo.NewInstance() + Expect(instance).To(BeAssignableToTypeOf(model.Tag{})) + }) + }) +})