diff --git a/core/share.go b/core/share.go index add88322d..202c27d89 100644 --- a/core/share.go +++ b/core/share.go @@ -149,7 +149,7 @@ func (r *shareRepositoryWrapper) contentsLabelFromArtist(shareID string, ids str func (r *shareRepositoryWrapper) contentsLabelFromAlbums(shareID string, ids string) string { idList := strings.Split(ids, ",") - all, err := r.ds.Album(r.ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"id": idList}}) + all, err := r.ds.Album(r.ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"album.id": idList}}) if err != nil { log.Error(r.ctx, "Error retrieving album names for share", "share", shareID, err) return "" diff --git a/model/tag.go b/model/tag.go index 8f9c60f37..674f688ca 100644 --- a/model/tag.go +++ b/model/tag.go @@ -12,11 +12,11 @@ import ( ) type Tag struct { - ID string `json:"id,omitempty"` - TagName TagName `json:"tagName,omitempty"` - TagValue string `json:"tagValue,omitempty"` - AlbumCount int `json:"albumCount,omitempty"` - MediaFileCount int `json:"songCount,omitempty"` + ID string `json:"id,omitempty"` + TagName TagName `json:"tagName,omitempty"` + TagValue string `json:"tagValue,omitempty"` + AlbumCount int `json:"albumCount,omitempty"` + SongCount int `json:"songCount,omitempty"` } type TagList []Tag diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index af95e0670..da46d8a11 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -156,7 +156,7 @@ func NewArtistRepository(ctx context.Context, db dbx.Builder) model.ArtistReposi func roleFilter(_ string, role any) Sqlizer { if role, ok := role.(string); ok { if _, ok := model.AllRoles[role]; ok { - return Expr("EXISTS (SELECT 1 FROM library_artist WHERE library_artist.artist_id = artist.id AND JSON_EXTRACT(library_artist.stats, '$." + role + ".m') IS NOT NULL)") + return Expr("JSON_EXTRACT(library_artist.stats, '$." + role + ".m') IS NOT NULL") } } return Eq{"1": 2} @@ -170,14 +170,16 @@ func artistLibraryIdFilter(_ string, value interface{}) Sqlizer { // applyLibraryFilterToArtistQuery applies library filtering to artist queries through the library_artist junction table func (r *artistRepository) applyLibraryFilterToArtistQuery(query SelectBuilder) SelectBuilder { user := loggedUser(r.ctx) - if user.ID == invalidUserId { - // No user context - return empty result set - return query.Where(Eq{"1": "0"}) - } + // Join with library_artist first to ensure only artists with content in libraries are included + // Exclude artists with empty stats (no actual content in the library) + query = query.Join("library_artist on library_artist.artist_id = artist.id") + //query = query.Join("library_artist on library_artist.artist_id = artist.id AND library_artist.stats != '{}'") - // Apply library filtering by joining only with accessible libraries - query = query.LeftJoin("library_artist on library_artist.artist_id = artist.id"). - Join("user_library on user_library.library_id = library_artist.library_id AND user_library.user_id = ?", user.ID) + // Admin users see all artists from all libraries, no additional filtering needed + if user.ID != invalidUserId && !user.IsAdmin { + // Apply library filtering only for non-admin users by joining with their accessible libraries + query = query.Join("user_library on user_library.library_id = library_artist.library_id AND user_library.user_id = ?", user.ID) + } return query } @@ -503,6 +505,15 @@ func (r *artistRepository) RefreshStats(allArtists bool) (int64, error) { totalRowsAffected += rowsAffected } + // // Remove library_artist entries for artists that no longer have any content in any library + cleanupSQL := Delete("library_artist").Where("stats = '{}'") + cleanupRows, err := r.executeSQL(cleanupSQL) + if err != nil { + log.Warn(r.ctx, "Failed to cleanup empty library_artist entries", "error", err) + } else if cleanupRows > 0 { + log.Debug(r.ctx, "Cleaned up empty library_artist entries", "rowsDeleted", cleanupRows) + } + log.Debug(r.ctx, "RefreshStats: Successfully updated stats.", "totalArtistsProcessed", len(allTouchedArtistIDs), "totalDBRowsAffected", totalRowsAffected) return totalRowsAffected, nil } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 2e19892a1..2259012ac 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -3,12 +3,10 @@ package persistence import ( "context" "encoding/json" - "strings" "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils" @@ -16,6 +14,34 @@ import ( . "github.com/onsi/gomega" ) +// Test helper functions to reduce duplication +func createTestArtistWithMBID(id, name, mbid string) model.Artist { + return model.Artist{ + ID: id, + Name: name, + MbzArtistID: mbid, + } +} + +func createUserWithLibraries(userID string, libraryIDs []int) model.User { + user := model.User{ + ID: userID, + UserName: userID, + Name: userID, + Email: userID + "@test.com", + IsAdmin: false, + } + + if len(libraryIDs) > 0 { + user.Libraries = make(model.Libraries, len(libraryIDs)) + for i, libID := range libraryIDs { + user.Libraries[i] = model.Library{ID: libID, Name: "Test Library", Path: "/test"} + } + } + + return user +} + var _ = Describe("ArtistRepository", func() { Context("Core Functionality", func() { @@ -43,7 +69,7 @@ var _ = Describe("ArtistRepository", func() { func(role string, shouldBeValid bool) { result := roleFilter("", role) if shouldBeValid { - expectedExpr := squirrel.Expr("EXISTS (SELECT 1 FROM library_artist WHERE library_artist.artist_id = artist.id AND JSON_EXTRACT(library_artist.stats, '$." + role + ".m') IS NOT NULL)") + expectedExpr := squirrel.Expr("JSON_EXTRACT(library_artist.stats, '$." + role + ".m') IS NOT NULL") Expect(result).To(Equal(expectedExpr)) } else { expectedInvalid := squirrel.Eq{"1": 2} @@ -158,8 +184,7 @@ var _ = Describe("ArtistRepository", func() { var repo model.ArtistRepository BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - ctx := log.NewContext(context.TODO()) + ctx := GinkgoT().Context() ctx = request.WithUser(ctx, adminUser) repo = NewArtistRepository(ctx, GetDBXBuilder()) }) @@ -361,55 +386,119 @@ var _ = Describe("ArtistRepository", func() { }) }) - Describe("MBID Search", func() { - var artistWithMBID model.Artist - var raw *artistRepository + Describe("MBID and Text Search", func() { + var lib2 model.Library + var lr model.LibraryRepository + var restrictedUser model.User + var restrictedRepo model.ArtistRepository + var headlessRepo model.ArtistRepository BeforeEach(func() { - raw = repo.(*artistRepository) - // Create a test artist with MBID - artistWithMBID = model.Artist{ - ID: "test-mbid-artist", - Name: "Test MBID Artist", - MbzArtistID: "550e8400-e29b-41d4-a716-446655440010", // Valid UUID v4 - } + // Set up headless repo (no user context) + headlessRepo = NewArtistRepository(context.Background(), GetDBXBuilder()) - // Insert the test artist into the database with proper library association - err := createArtistWithLibrary(repo, &artistWithMBID, 1) + // Create library for testing access restrictions + lib2 = model.Library{ID: 0, Name: "Artist Test Library", Path: "/artist/test/lib"} + lr = NewLibraryRepository(request.WithUser(GinkgoT().Context(), adminUser), GetDBXBuilder()) + err := lr.Put(&lib2) + Expect(err).ToNot(HaveOccurred()) + + // Create a user with access to only library 1 + restrictedUser = createUserWithLibraries("search_user", []int{1}) + + // Create repository context for the restricted user + ctx := request.WithUser(GinkgoT().Context(), restrictedUser) + restrictedRepo = NewArtistRepository(ctx, GetDBXBuilder()) + + // Ensure both test artists are associated with library 1 + err = lr.AddArtist(1, artistBeatles.ID) + Expect(err).ToNot(HaveOccurred()) + err = lr.AddArtist(1, artistKraftwerk.ID) + Expect(err).ToNot(HaveOccurred()) + + // Create the restricted user in the database + ur := NewUserRepository(request.WithUser(GinkgoT().Context(), adminUser), GetDBXBuilder()) + err = ur.Put(&restrictedUser) + Expect(err).ToNot(HaveOccurred()) + err = ur.SetUserLibraries(restrictedUser.ID, []int{1}) Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { - // Clean up test data using direct SQL - _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": artistWithMBID.ID})) + // Clean up library 2 + lr := NewLibraryRepository(request.WithUser(GinkgoT().Context(), adminUser), GetDBXBuilder()) + _ = lr.(*libraryRepository).delete(squirrel.Eq{"id": lib2.ID}) }) - It("finds artist by mbz_artist_id", func() { - results, err := repo.Search("550e8400-e29b-41d4-a716-446655440010", 0, 10, false) + DescribeTable("MBID search behavior across different user types", + func(testRepo *model.ArtistRepository, shouldFind bool, testDesc string) { + // Create test artist with MBID + artistWithMBID := createTestArtistWithMBID("test-mbid-artist", "Test MBID Artist", "550e8400-e29b-41d4-a716-446655440010") + + err := createArtistWithLibrary(*testRepo, &artistWithMBID, 1) + Expect(err).ToNot(HaveOccurred()) + + // Test the search + results, err := (*testRepo).Search("550e8400-e29b-41d4-a716-446655440010", 0, 10, false) + Expect(err).ToNot(HaveOccurred()) + + if shouldFind { + Expect(results).To(HaveLen(1), testDesc) + Expect(results[0].ID).To(Equal("test-mbid-artist")) + } else { + Expect(results).To(BeEmpty(), testDesc) + } + + // Clean up + if raw, ok := (*testRepo).(*artistRepository); ok { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": artistWithMBID.ID})) + } + }, + Entry("Admin user can find artist by MBID", &repo, true, "Admin should find MBID artist"), + Entry("Restricted user can find artist by MBID in accessible library", &restrictedRepo, true, "Restricted user should find MBID artist in accessible library"), + Entry("Headless process can find artist by MBID", &headlessRepo, true, "Headless process should find MBID artist"), + ) + + It("prevents restricted user from finding artist by MBID when not in accessible library", func() { + // Create an artist in library 2 (not accessible to restricted user) + inaccessibleArtist := createTestArtistWithMBID("inaccessible-mbid-artist", "Inaccessible MBID Artist", "a74b1b7f-71a5-4011-9441-d0b5e4122711") + err := repo.Put(&inaccessibleArtist) Expect(err).ToNot(HaveOccurred()) - Expect(results).To(HaveLen(1)) - Expect(results[0].ID).To(Equal("test-mbid-artist")) - Expect(results[0].Name).To(Equal("Test MBID Artist")) - }) - It("returns empty result when MBID is not found", func() { - results, err := repo.Search("550e8400-e29b-41d4-a716-446655440099", 0, 10, false) + // Add to library 2 (not accessible to restricted user) + err = lr.AddArtist(lib2.ID, inaccessibleArtist.ID) + Expect(err).ToNot(HaveOccurred()) + + // Restricted user should not find this artist + results, err := restrictedRepo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10, false) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) + + // But admin should find it + results, err = repo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10, false) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(1)) + + // Clean up + if raw, ok := repo.(*artistRepository); ok { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": inaccessibleArtist.ID})) + } }) It("handles includeMissing parameter for MBID search", func() { // Create a missing artist with MBID - missingArtist := model.Artist{ - ID: "test-missing-mbid-artist", - Name: "Test Missing MBID Artist", - MbzArtistID: "550e8400-e29b-41d4-a716-446655440012", - Missing: true, - } + missingArtist := createTestArtistWithMBID("test-missing-mbid-artist", "Test Missing MBID Artist", "550e8400-e29b-41d4-a716-446655440012") + missingArtist.Missing = true err := createArtistWithLibrary(repo, &missingArtist, 1) Expect(err).ToNot(HaveOccurred()) + // Mark as missing + if raw, ok := repo.(*artistRepository); ok { + _, err = raw.executeSQL(squirrel.Update(raw.tableName).Set("missing", true).Where(squirrel.Eq{"id": missingArtist.ID})) + Expect(err).ToNot(HaveOccurred()) + } + // Should not find missing artist when includeMissing is false results, err := repo.Search("550e8400-e29b-41d4-a716-446655440012", 0, 10, false) Expect(err).ToNot(HaveOccurred()) @@ -422,7 +511,95 @@ var _ = Describe("ArtistRepository", func() { Expect(results[0].ID).To(Equal("test-missing-mbid-artist")) // Clean up - _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingArtist.ID})) + if raw, ok := repo.(*artistRepository); ok { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingArtist.ID})) + } + }) + + Context("Text Search", func() { + It("allows admin to find artists by name regardless of library", func() { + results, err := repo.Search("Beatles", 0, 10, false) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(1)) + Expect(results[0].Name).To(Equal("The Beatles")) + }) + + It("correctly prevents restricted user from finding artists by name when not in accessible library", func() { + // Create an artist in library 2 (not accessible to restricted user) + inaccessibleArtist := model.Artist{ + ID: "inaccessible-text-artist", + Name: "Unique Search Name Artist", + } + err := repo.Put(&inaccessibleArtist) + Expect(err).ToNot(HaveOccurred()) + + // Add to library 2 (not accessible to restricted user) + err = lr.AddArtist(lib2.ID, inaccessibleArtist.ID) + Expect(err).ToNot(HaveOccurred()) + + // Restricted user should not find this artist + results, err := restrictedRepo.Search("Unique Search Name", 0, 10, false) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(BeEmpty(), "Text search should respect library filtering") + + // Clean up + if raw, ok := repo.(*artistRepository); ok { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": inaccessibleArtist.ID})) + } + }) + }) + + Context("Headless Processes (No User Context)", func() { + It("should see all artists from all libraries when no user is in context", func() { + // Add artists to different libraries + err := lr.AddArtist(lib2.ID, artistBeatles.ID) + Expect(err).ToNot(HaveOccurred()) + + // Headless processes should see all artists regardless of library + artists, err := headlessRepo.GetAll() + Expect(err).ToNot(HaveOccurred()) + + // Should see all artists from all libraries + found := false + for _, artist := range artists { + if artist.ID == artistBeatles.ID { + found = true + break + } + } + Expect(found).To(BeTrue(), "Headless process should see artists from all libraries") + }) + + It("should allow headless processes to apply explicit library_id filters", func() { + // Add artists to different libraries + err := lr.AddArtist(lib2.ID, artistBeatles.ID) + Expect(err).ToNot(HaveOccurred()) + + // Filter by specific library + artists, err := headlessRepo.GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"library_id": lib2.ID}, + }) + Expect(err).ToNot(HaveOccurred()) + + // Should see only artists from the specified library + for _, artist := range artists { + if artist.ID == artistBeatles.ID { + return // Found the expected artist + } + } + Expect(false).To(BeTrue(), "Should find artist from specified library") + }) + + It("should get individual artists when no user is in context", func() { + // Add artist to a library + err := lr.AddArtist(lib2.ID, artistBeatles.ID) + Expect(err).ToNot(HaveOccurred()) + + // Headless process should be able to get the artist + artist, err := headlessRepo.Get(artistBeatles.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(artist.ID).To(Equal(artistBeatles.ID)) + }) }) }) @@ -441,6 +618,45 @@ var _ = Describe("ArtistRepository", func() { Expect(exists).To(BeTrue()) }) }) + + Describe("Missing Artist Handling", func() { + var missingArtist model.Artist + var raw *artistRepository + + BeforeEach(func() { + raw = repo.(*artistRepository) + missingArtist = model.Artist{ID: "missing_test", Name: "Missing Artist", OrderArtistName: "missing artist"} + + // Create and mark as missing + err := createArtistWithLibrary(repo, &missingArtist, 1) + Expect(err).ToNot(HaveOccurred()) + + _, err = raw.executeSQL(squirrel.Update(raw.tableName).Set("missing", true).Where(squirrel.Eq{"id": missingArtist.ID})) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingArtist.ID})) + }) + + It("admin can see missing artists when explicitly included", func() { + // Should see missing artist in GetAll by default for admin users + artists, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(artists).To(HaveLen(3)) // Including the missing artist + + // Should see missing artist when searching with includeMissing=true + results, err := repo.Search("Missing Artist", 0, 10, true) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(1)) + Expect(results[0].ID).To(Equal("missing_test")) + + // Should not see missing artist when searching with includeMissing=false + results, err = repo.Search("Missing Artist", 0, 10, false) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(BeEmpty()) + }) + }) }) Context("Regular User Operations", func() { @@ -448,12 +664,11 @@ var _ = Describe("ArtistRepository", func() { var unauthorizedUser model.User BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) // Create a user without access to any libraries unauthorizedUser = model.User{ID: "restricted_user", UserName: "restricted", Name: "Restricted User", Email: "restricted@test.com", IsAdmin: false} // Create repository context for the unauthorized user - ctx := log.NewContext(context.TODO()) + ctx := GinkgoT().Context() ctx = request.WithUser(ctx, unauthorizedUser) restrictedRepo = NewArtistRepository(ctx, GetDBXBuilder()) }) @@ -509,8 +724,9 @@ var _ = Describe("ArtistRepository", func() { Context("when user gains library access", func() { BeforeEach(func() { + ctx := GinkgoT().Context() // Give the user access to library 1 - ur := NewUserRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) + ur := NewUserRepository(request.WithUser(ctx, adminUser), GetDBXBuilder()) // First create the user if not exists err := ur.Put(&unauthorizedUser) @@ -526,14 +742,13 @@ var _ = Describe("ArtistRepository", func() { unauthorizedUser.Libraries = libraries // Recreate repository context with updated user - ctx := log.NewContext(context.TODO()) ctx = request.WithUser(ctx, unauthorizedUser) restrictedRepo = NewArtistRepository(ctx, GetDBXBuilder()) }) AfterEach(func() { // Clean up: remove the user's library access - ur := NewUserRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) + ur := NewUserRepository(request.WithUser(GinkgoT().Context(), adminUser), GetDBXBuilder()) _ = ur.SetUserLibraries(unauthorizedUser.ID, []int{}) }) @@ -578,292 +793,6 @@ var _ = Describe("ArtistRepository", func() { }) }) }) - - Context("Permission-Based Behavior Comparison", func() { - Describe("Missing Artist Visibility", func() { - var repo model.ArtistRepository - var raw *artistRepository - var missing model.Artist - - insertMissing := func() { - missing = model.Artist{ID: "m1", Name: "Missing", OrderArtistName: "missing"} - Expect(repo.Put(&missing)).To(Succeed()) - raw = repo.(*artistRepository) - _, err := raw.executeSQL(squirrel.Update(raw.tableName).Set("missing", true).Where(squirrel.Eq{"id": missing.ID})) - Expect(err).ToNot(HaveOccurred()) - - // Add missing artist to library 1 so it can be found by library filtering - lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - err = lr.AddArtist(1, missing.ID) - Expect(err).ToNot(HaveOccurred()) - - // Ensure the test user exists and has library access - ur := NewUserRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - currentUser, ok := request.UserFrom(repo.(*artistRepository).ctx) - if ok { - // Create the user if it doesn't exist with default values if missing - testUser := model.User{ - ID: currentUser.ID, - UserName: currentUser.UserName, - Name: currentUser.Name, - Email: currentUser.Email, - IsAdmin: currentUser.IsAdmin, - } - // Provide defaults for missing fields - if testUser.UserName == "" { - testUser.UserName = testUser.ID - } - if testUser.Name == "" { - testUser.Name = testUser.ID - } - if testUser.Email == "" { - testUser.Email = testUser.ID + "@test.com" - } - - // Try to put the user (will fail silently if already exists) - _ = ur.Put(&testUser) - - // Add library association using SetUserLibraries - err = ur.SetUserLibraries(currentUser.ID, []int{1}) - // Ignore error if user already has these libraries or other conflicts - if err != nil && !strings.Contains(err.Error(), "UNIQUE constraint failed") && !strings.Contains(err.Error(), "duplicate key") { - Expect(err).ToNot(HaveOccurred()) - } - } - } - - removeMissing := func() { - if raw != nil { - _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missing.ID})) - } - } - - Context("regular user", func() { - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - // Create user with library access (simulating middleware behavior) - regularUserWithLibs := model.User{ - ID: "u1", - IsAdmin: false, - Libraries: model.Libraries{ - {ID: 1, Name: "Test Library", Path: "/test"}, - }, - } - ctx := log.NewContext(context.TODO()) - ctx = request.WithUser(ctx, regularUserWithLibs) - repo = NewArtistRepository(ctx, GetDBXBuilder()) - insertMissing() - }) - - AfterEach(func() { removeMissing() }) - - It("does not return missing artist in GetAll", func() { - artists, err := repo.GetAll(model.QueryOptions{Filters: squirrel.Eq{"artist.missing": false}}) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).To(HaveLen(2)) - }) - - It("does not return missing artist in Search", func() { - res, err := repo.Search("missing", 0, 10, false) - Expect(err).ToNot(HaveOccurred()) - Expect(res).To(BeEmpty()) - }) - - It("does not return missing artist in GetIndex", func() { - idx, err := repo.GetIndex(false, []int{1}) - Expect(err).ToNot(HaveOccurred()) - // Only 2 artists should be present - total := 0 - for _, ix := range idx { - total += len(ix.Artists) - } - Expect(total).To(Equal(2)) - }) - }) - - Context("admin user", func() { - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - ctx := log.NewContext(context.TODO()) - ctx = request.WithUser(ctx, model.User{ID: "admin", IsAdmin: true}) - repo = NewArtistRepository(ctx, GetDBXBuilder()) - insertMissing() - }) - - AfterEach(func() { removeMissing() }) - - It("returns missing artist in GetAll", func() { - artists, err := repo.GetAll() - Expect(err).ToNot(HaveOccurred()) - Expect(artists).To(HaveLen(3)) - }) - - It("returns missing artist in Search", func() { - res, err := repo.Search("missing", 0, 10, true) - Expect(err).ToNot(HaveOccurred()) - Expect(res).To(HaveLen(1)) - }) - - It("returns missing artist in GetIndex when included", func() { - idx, err := repo.GetIndex(true, []int{1}) - Expect(err).ToNot(HaveOccurred()) - total := 0 - for _, ix := range idx { - total += len(ix.Artists) - } - Expect(total).To(Equal(3)) - }) - }) - }) - - Describe("Library Filtering", func() { - var restrictedUser model.User - var restrictedRepo model.ArtistRepository - var adminRepo model.ArtistRepository - var lib2 model.Library - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - - // Set up admin repo - ctx := log.NewContext(context.TODO()) - ctx = request.WithUser(ctx, adminUser) - adminRepo = NewArtistRepository(ctx, GetDBXBuilder()) - - // Create library for testing access restrictions - lib2 = model.Library{ID: 0, Name: "Artist Test Library", Path: "/artist/test/lib"} - lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - err := lr.Put(&lib2) - Expect(err).ToNot(HaveOccurred()) - - // Create a user with access to only library 1 - restrictedUser = model.User{ - ID: "search_user", - IsAdmin: false, - Libraries: model.Libraries{ - {ID: 1, Name: "Library 1", Path: "/lib1"}, - }, - } - - // Create repository context for the restricted user - ctx = log.NewContext(context.TODO()) - ctx = request.WithUser(ctx, restrictedUser) - restrictedRepo = NewArtistRepository(ctx, GetDBXBuilder()) - - // Ensure both test artists are associated with library 1 - err = lr.AddArtist(1, artistBeatles.ID) - Expect(err).ToNot(HaveOccurred()) - err = lr.AddArtist(1, artistKraftwerk.ID) - Expect(err).ToNot(HaveOccurred()) - - // Create the restricted user in the database - ur := NewUserRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - err = ur.Put(&restrictedUser) - Expect(err).ToNot(HaveOccurred()) - err = ur.SetUserLibraries(restrictedUser.ID, []int{1}) - Expect(err).ToNot(HaveOccurred()) - }) - - AfterEach(func() { - // Clean up library 2 - lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - _ = lr.(*libraryRepository).delete(squirrel.Eq{"id": lib2.ID}) - }) - - Context("MBID Search", func() { - var artistWithMBID model.Artist - - BeforeEach(func() { - artistWithMBID = model.Artist{ - ID: "search-mbid-artist", - Name: "Search MBID Artist", - MbzArtistID: "f4fdbb4c-e4b7-47a0-b83b-d91bbfcfa387", - } - err := createArtistWithLibrary(adminRepo, &artistWithMBID, 1) - Expect(err).ToNot(HaveOccurred()) - }) - - AfterEach(func() { - raw := adminRepo.(*artistRepository) - _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": artistWithMBID.ID})) - }) - - It("allows admin to find artist by MBID regardless of library", func() { - results, err := adminRepo.Search("f4fdbb4c-e4b7-47a0-b83b-d91bbfcfa387", 0, 10, false) - Expect(err).ToNot(HaveOccurred()) - Expect(results).To(HaveLen(1)) - Expect(results[0].ID).To(Equal("search-mbid-artist")) - }) - - It("allows restricted user to find artist by MBID when in accessible library", func() { - results, err := restrictedRepo.Search("f4fdbb4c-e4b7-47a0-b83b-d91bbfcfa387", 0, 10, false) - Expect(err).ToNot(HaveOccurred()) - Expect(results).To(HaveLen(1)) - Expect(results[0].ID).To(Equal("search-mbid-artist")) - }) - - It("prevents restricted user from finding artist by MBID when not in accessible library", func() { - // Create an artist in library 2 (not accessible to restricted user) - inaccessibleArtist := model.Artist{ - ID: "inaccessible-mbid-artist", - Name: "Inaccessible MBID Artist", - MbzArtistID: "a74b1b7f-71a5-4011-9441-d0b5e4122711", - } - err := adminRepo.Put(&inaccessibleArtist) - Expect(err).ToNot(HaveOccurred()) - - // Add to library 2 (not accessible to restricted user) - lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - err = lr.AddArtist(lib2.ID, inaccessibleArtist.ID) - Expect(err).ToNot(HaveOccurred()) - - // Restricted user should not find this artist - results, err := restrictedRepo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10, false) - Expect(err).ToNot(HaveOccurred()) - Expect(results).To(BeEmpty()) - - // Clean up - raw := adminRepo.(*artistRepository) - _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": inaccessibleArtist.ID})) - }) - }) - - Context("Text Search", func() { - It("allows admin to find artists by name regardless of library", func() { - results, err := adminRepo.Search("Beatles", 0, 10, false) - Expect(err).ToNot(HaveOccurred()) - Expect(results).To(HaveLen(1)) - Expect(results[0].Name).To(Equal("The Beatles")) - }) - - It("correctly prevents restricted user from finding artists by name when not in accessible library", func() { - // Create an artist in library 2 (not accessible to restricted user) - inaccessibleArtist := model.Artist{ - ID: "inaccessible-text-artist", - Name: "Unique Search Name Artist", - } - err := adminRepo.Put(&inaccessibleArtist) - Expect(err).ToNot(HaveOccurred()) - - // Add to library 2 (not accessible to restricted user) - lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) - err = lr.AddArtist(lib2.ID, inaccessibleArtist.ID) - Expect(err).ToNot(HaveOccurred()) - - // Restricted user should not find this artist - results, err := restrictedRepo.Search("Unique Search Name", 0, 10, false) - Expect(err).ToNot(HaveOccurred()) - - // Text search correctly respects library filtering - Expect(results).To(BeEmpty(), "Text search should respect library filtering") - - // Clean up - raw := adminRepo.(*artistRepository) - _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": inaccessibleArtist.ID})) - }) - }) - }) - }) }) // Helper function to create an artist with proper library association. @@ -875,6 +804,6 @@ func createArtistWithLibrary(repo model.ArtistRepository, artist *model.Artist, } // Add the artist to the specified library - lr := NewLibraryRepository(request.WithUser(log.NewContext(context.TODO()), adminUser), GetDBXBuilder()) + lr := NewLibraryRepository(request.WithUser(GinkgoT().Context(), adminUser), GetDBXBuilder()) return lr.AddArtist(libraryID, artist.ID) } diff --git a/persistence/genre_repository.go b/persistence/genre_repository.go index 311eb0a68..5857350a6 100644 --- a/persistence/genre_repository.go +++ b/persistence/genre_repository.go @@ -21,7 +21,7 @@ func NewGenreRepository(ctx context.Context, db dbx.Builder) model.GenreReposito } func (r *genreRepository) selectGenre(opt ...model.QueryOptions) SelectBuilder { - return r.newSelect(opt...) + return r.newSelect(opt...).Columns("tag.tag_value as name") } func (r *genreRepository) GetAll(opt ...model.QueryOptions) (model.Genres, error) { diff --git a/persistence/genre_repository_test.go b/persistence/genre_repository_test.go index e7b43689c..67e84ce51 100644 --- a/persistence/genre_repository_test.go +++ b/persistence/genre_repository_test.go @@ -7,8 +7,6 @@ import ( "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" @@ -23,8 +21,7 @@ var _ = Describe("GenreRepository", func() { var ctx context.Context BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - ctx = request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid", UserName: "johndoe", IsAdmin: true}) + ctx = request.WithUser(GinkgoT().Context(), model.User{ID: "userid", UserName: "johndoe", IsAdmin: true}) genreRepo := NewGenreRepository(ctx, GetDBXBuilder()) repo = genreRepo restRepo = genreRepo.(model.ResourceRepository) @@ -240,6 +237,82 @@ var _ = Describe("GenreRepository", func() { }) }) + Describe("Library Filtering", func() { + Context("Headless Processes (No User Context)", func() { + var headlessRepo model.GenreRepository + var headlessRestRepo model.ResourceRepository + + BeforeEach(func() { + // Create a repository with no user context (headless) + headlessGenreRepo := NewGenreRepository(context.Background(), GetDBXBuilder()) + headlessRepo = headlessGenreRepo + headlessRestRepo = headlessGenreRepo.(model.ResourceRepository) + + // Add genres to different libraries + db := GetDBXBuilder() + _, err := db.NewQuery("INSERT OR IGNORE INTO library (id, name, path) VALUES (2, 'Test Library 2', '/test2')").Execute() + Expect(err).ToNot(HaveOccurred()) + + // Add tags to different libraries + newTag := func(name, value string) model.Tag { + return model.Tag{ID: id.NewTagID(name, value), TagName: model.TagName(name), TagValue: value} + } + + err = tagRepo.Add(2, newTag("genre", "jazz")) + Expect(err).ToNot(HaveOccurred()) + }) + + It("should see all genres from all libraries when no user is in context", func() { + // Headless processes should see all genres regardless of library + genres, err := headlessRepo.GetAll() + Expect(err).ToNot(HaveOccurred()) + + // Should see genres from all libraries + var genreNames []string + for _, genre := range genres { + genreNames = append(genreNames, genre.Name) + } + + // Should include both rock (library 1) and jazz (library 2) + Expect(genreNames).To(ContainElement("rock")) + Expect(genreNames).To(ContainElement("jazz")) + }) + + It("should count all genres from all libraries when no user is in context", func() { + count, err := headlessRestRepo.Count() + Expect(err).ToNot(HaveOccurred()) + + // Should count all genres from all libraries + Expect(count).To(BeNumerically(">=", 2)) + }) + + It("should allow headless processes to apply explicit library_id filters", func() { + // Filter by specific library + genres, err := headlessRestRepo.ReadAll(rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": 2}, + }) + Expect(err).ToNot(HaveOccurred()) + + genreList := genres.(model.Genres) + // Should see only genres from library 2 + Expect(genreList).To(HaveLen(1)) + Expect(genreList[0].Name).To(Equal("jazz")) + }) + + It("should get individual genres when no user is in context", func() { + // Get all genres first to find an ID + genres, err := headlessRepo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(genres).ToNot(BeEmpty()) + + // Headless process should be able to get the genre + genre, err := headlessRestRepo.Read(genres[0].ID) + Expect(err).ToNot(HaveOccurred()) + Expect(genre).ToNot(BeNil()) + }) + }) + }) + Describe("EntityName", func() { It("should return correct entity name", func() { name := restRepo.EntityName() diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index a3d5ebc74..1007d84fe 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -218,7 +218,13 @@ var _ = BeforeSuite(func() { if err := arr.SetStar(true, artistBeatles.ID); err != nil { panic(err) } - ar, _ := arr.Get(artistBeatles.ID) + ar, err := arr.Get(artistBeatles.ID) + if err != nil { + panic(err) + } + if ar == nil { + panic("artist not found after SetStar") + } artistBeatles.Starred = true artistBeatles.StarredAt = ar.StarredAt testArtists[1] = artistBeatles @@ -230,6 +236,9 @@ var _ = BeforeSuite(func() { if err != nil { panic(err) } + if al == nil { + panic("album not found after SetStar") + } albumRadioactivity.Starred = true albumRadioactivity.StarredAt = al.StarredAt testAlbums[2] = albumRadioactivity diff --git a/persistence/scrobble_buffer_repository.go b/persistence/scrobble_buffer_repository.go index ac0d8adeb..d0f88903e 100644 --- a/persistence/scrobble_buffer_repository.go +++ b/persistence/scrobble_buffer_repository.go @@ -8,7 +8,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" - "github.com/navidrome/navidrome/model/request" "github.com/pocketbase/dbx" ) @@ -83,20 +82,7 @@ func (r *scrobbleBufferRepository) Next(service string, userId string) (*model.S if err != nil { return nil, err } - - // Create context with user information for getParticipants call - // This is needed because the artist repository requires user context for multi-library support - userRepo := NewUserRepository(r.ctx, r.db) - user, err := userRepo.Get(res.ScrobbleEntry.UserID) - if err != nil { - return nil, err - } - // Temporarily use user context for getParticipants - originalCtx := r.ctx - r.ctx = request.WithUser(r.ctx, *user) res.ScrobbleEntry.Participants, err = r.getParticipants(&res.ScrobbleEntry.MediaFile) - r.ctx = originalCtx // Restore original context - if err != nil { return nil, err } diff --git a/persistence/share_repository.go b/persistence/share_repository.go index abe1ea6e6..d943943e0 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -95,7 +95,7 @@ func (r *shareRepository) loadMedia(share *model.Share) error { return err case "album": albumRepo := NewAlbumRepository(r.ctx, r.db) - share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"id": ids})}) + share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album.id": ids})}) if err != nil { return err } diff --git a/persistence/share_repository_test.go b/persistence/share_repository_test.go new file mode 100644 index 000000000..252115175 --- /dev/null +++ b/persistence/share_repository_test.go @@ -0,0 +1,133 @@ +package persistence + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ShareRepository", func() { + var repo model.ShareRepository + var ctx context.Context + var adminUser = model.User{ID: "admin", UserName: "admin", IsAdmin: true} + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + ctx = request.WithUser(log.NewContext(context.TODO()), adminUser) + repo = NewShareRepository(ctx, GetDBXBuilder()) + + // Insert the admin user into the database (required for foreign key constraint) + ur := NewUserRepository(ctx, GetDBXBuilder()) + err := ur.Put(&adminUser) + Expect(err).ToNot(HaveOccurred()) + + // Clean up shares + db := GetDBXBuilder() + _, err = db.NewQuery("DELETE FROM share").Execute() + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("Headless Access", func() { + Context("Repository creation and basic operations", func() { + It("should create repository successfully with no user context", func() { + // Create repository with no user context (headless) + headlessRepo := NewShareRepository(context.Background(), GetDBXBuilder()) + Expect(headlessRepo).ToNot(BeNil()) + }) + + It("should handle GetAll for headless processes", func() { + // Create a simple share directly in database + shareID := "headless-test-share" + _, err := GetDBXBuilder().NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]interface{}{ + "id": shareID, + "user": adminUser.ID, + "desc": "Headless Test Share", + "type": "song", + "ids": "song-1", + "created": time.Now(), + "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + + // Headless process should see all shares + headlessRepo := NewShareRepository(context.Background(), GetDBXBuilder()) + shares, err := headlessRepo.GetAll() + Expect(err).ToNot(HaveOccurred()) + + found := false + for _, s := range shares { + if s.ID == shareID { + found = true + break + } + } + Expect(found).To(BeTrue(), "Headless process should see all shares") + }) + + It("should handle individual share retrieval for headless processes", func() { + // Create a simple share + shareID := "headless-get-share" + _, err := GetDBXBuilder().NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]interface{}{ + "id": shareID, + "user": adminUser.ID, + "desc": "Headless Get Share", + "type": "song", + "ids": "song-2", + "created": time.Now(), + "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + + // Headless process should be able to get the share + headlessRepo := NewShareRepository(context.Background(), GetDBXBuilder()) + share, err := headlessRepo.Get(shareID) + Expect(err).ToNot(HaveOccurred()) + Expect(share.ID).To(Equal(shareID)) + Expect(share.Description).To(Equal("Headless Get Share")) + }) + }) + }) + + Describe("SQL ambiguity fix verification", func() { + It("should handle share operations without SQL ambiguity errors", func() { + // This test verifies that the loadMedia function doesn't cause SQL ambiguity + // The key fix was using "album.id" instead of "id" in the album query filters + + // Create a share that would trigger the loadMedia function + shareID := "sql-test-share" + _, err := GetDBXBuilder().NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]interface{}{ + "id": shareID, + "user": adminUser.ID, + "desc": "SQL Test Share", + "type": "album", + "ids": "non-existent-album", // Won't find albums, but shouldn't cause SQL errors + "created": time.Now(), + "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + + // The Get operation should work without SQL ambiguity errors + // even if no albums are found + share, err := repo.Get(shareID) + Expect(err).ToNot(HaveOccurred()) + Expect(share.ID).To(Equal(shareID)) + // Albums array should be empty since we used non-existent album ID + Expect(share.Albums).To(BeEmpty()) + }) + }) +}) diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 9e7a58713..ce026a3c3 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -205,27 +205,20 @@ func libraryIdFilter(_ string, value interface{}) Sqlizer { func (r sqlRepository) applyLibraryFilter(sq SelectBuilder, tableName ...string) SelectBuilder { user := loggedUser(r.ctx) - // Admin users see all content - if user.IsAdmin { + // If the user is an admin, or the user ID is invalid (e.g., when no user is logged in), skip the library filter + if user.IsAdmin || user.ID == invalidUserId { return sq } - // Get user's accessible library IDs - userID := loggedUser(r.ctx).ID - if userID == invalidUserId { - // No user context - return empty result set - return sq.Where(Eq{"1": "0"}) - } - table := r.tableName if len(tableName) > 0 { table = tableName[0] } + // Get user's accessible library IDs // Use subquery to filter by user's library access - // This approach doesn't require DataStore in context return sq.Where(Expr(table+".library_id IN ("+ - "SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)", userID)) + "SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)", user.ID)) } func (r sqlRepository) seedKey() string { diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 7ba2a0021..b46e2066b 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -223,4 +223,62 @@ var _ = Describe("sqlRepository", func() { Expect(hasher.CurrentSeed(id)).To(Equal("seed")) }) }) + + Describe("applyLibraryFilter", func() { + var sq squirrel.SelectBuilder + + BeforeEach(func() { + sq = squirrel.Select("*").From("test_table") + }) + + Context("Admin User", func() { + BeforeEach(func() { + r.ctx = request.WithUser(context.Background(), model.User{ID: "admin", IsAdmin: true}) + }) + + It("should not apply library filter for admin users", func() { + result := r.applyLibraryFilter(sq) + sql, _, _ := result.ToSql() + Expect(sql).To(Equal("SELECT * FROM test_table")) + }) + }) + + Context("Regular User", func() { + BeforeEach(func() { + r.ctx = request.WithUser(context.Background(), model.User{ID: "user123", IsAdmin: false}) + }) + + It("should apply library filter for regular users", func() { + result := r.applyLibraryFilter(sq) + sql, args, _ := result.ToSql() + Expect(sql).To(ContainSubstring("IN (SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)")) + Expect(args).To(ContainElement("user123")) + }) + + It("should use custom table name when provided", func() { + result := r.applyLibraryFilter(sq, "custom_table") + sql, args, _ := result.ToSql() + Expect(sql).To(ContainSubstring("custom_table.library_id IN")) + Expect(args).To(ContainElement("user123")) + }) + }) + + Context("Headless Process (No User Context)", func() { + BeforeEach(func() { + r.ctx = context.Background() // No user context + }) + + It("should not apply library filter for headless processes", func() { + result := r.applyLibraryFilter(sq) + sql, _, _ := result.ToSql() + Expect(sql).To(Equal("SELECT * FROM test_table")) + }) + + It("should not apply library filter even with custom table name", func() { + result := r.applyLibraryFilter(sq, "custom_table") + sql, _, _ := result.ToSql() + Expect(sql).To(Equal("SELECT * FROM test_table")) + }) + }) + }) }) diff --git a/persistence/sql_participations.go b/persistence/sql_participations.go index 006b7063b..4345184f1 100644 --- a/persistence/sql_participations.go +++ b/persistence/sql_participations.go @@ -68,7 +68,7 @@ func (r sqlRepository) updateParticipants(itemID string, participants model.Part func (r *sqlRepository) getParticipants(m *model.MediaFile) (model.Participants, error) { ar := NewArtistRepository(r.ctx, r.db) ids := m.Participants.AllIDs() - artists, err := ar.GetAll(model.QueryOptions{Filters: Eq{"id": ids}}) + artists, err := ar.GetAll(model.QueryOptions{Filters: Eq{"artist.id": ids}}) if err != nil { return nil, fmt.Errorf("getting participants: %w", err) } diff --git a/persistence/sql_tags.go b/persistence/sql_tags.go index b92e18e60..8c3c1e89d 100644 --- a/persistence/sql_tags.go +++ b/persistence/sql_tags.go @@ -91,61 +91,66 @@ func newBaseTagRepository(ctx context.Context, db dbx.Builder, tagFilter *model. return r } +// applyLibraryFiltering adds the appropriate library joins based on user context +func (r *baseTagRepository) applyLibraryFiltering(sq SelectBuilder) SelectBuilder { + // Add library_tag join + sq = sq.LeftJoin("library_tag on library_tag.tag_id = tag.id") + + // For authenticated users, also join with user_library to filter by accessible libraries + user := loggedUser(r.ctx) + if user.ID != invalidUserId { + sq = sq.Join("user_library on user_library.library_id = library_tag.library_id AND user_library.user_id = ?", user.ID) + } + + return sq +} + // newSelect overrides the base implementation to apply tag name filtering and library filtering. func (r *baseTagRepository) newSelect(options ...model.QueryOptions) SelectBuilder { - user := loggedUser(r.ctx) - if user.ID == invalidUserId { - // No user context - return empty result set - return SelectBuilder{}.Where(Eq{"1": "0"}) - } sq := r.sqlRepository.newSelect(options...) + + // Apply tag name filtering if specified if r.tagFilter != nil { sq = sq.Where(Eq{"tag.tag_name": *r.tagFilter}) } - sq = sq.Columns( + + // Apply library filtering and set up aggregation columns + sq = r.applyLibraryFiltering(sq).Columns( "tag.id", - "tag.tag_value as name", + "tag.tag_name", + "tag.tag_value", "COALESCE(SUM(library_tag.album_count), 0) as album_count", "COALESCE(SUM(library_tag.media_file_count), 0) as song_count", - ). - LeftJoin("library_tag on library_tag.tag_id = tag.id"). - // Apply library filtering by joining only with accessible libraries - Join("user_library on user_library.library_id = library_tag.library_id AND user_library.user_id = ?", user.ID). - GroupBy("tag.id", "tag.tag_value") + ).GroupBy("tag.id", "tag.tag_name", "tag.tag_value") + return sq } // ResourceRepository interface implementation func (r *baseTagRepository) Count(options ...rest.QueryOptions) (int64, error) { - // Create a query that counts distinct tags without GROUP BY - user := loggedUser(r.ctx) - if user.ID == invalidUserId { - return 0, nil - } + sq := Select("COUNT(DISTINCT tag.id)").From("tag") - // Build the same base query as newSelect but for counting - sq := Select() + // Apply tag name filtering if specified if r.tagFilter != nil { sq = sq.Where(Eq{"tag.tag_name": *r.tagFilter}) } - // Apply the same joins as newSelect - sq = sq.LeftJoin("library_tag on library_tag.tag_id = tag.id"). - Join("user_library on user_library.library_id = library_tag.library_id AND user_library.user_id = ?", user.ID) + // Apply library filtering + sq = r.applyLibraryFiltering(sq) return r.count(sq, r.parseRestOptions(r.ctx, options...)) } func (r *baseTagRepository) Read(id string) (interface{}, error) { - query := r.newSelect().Columns("*").Where(Eq{"id": id}) + query := r.newSelect().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("*") + query := r.newSelect(r.parseRestOptions(r.ctx, options...)) var res model.TagList err := r.queryAll(query, &res) return res, err diff --git a/persistence/tag_library_filtering_test.go b/persistence/tag_library_filtering_test.go index 8017528fe..ab0d57d52 100644 --- a/persistence/tag_library_filtering_test.go +++ b/persistence/tag_library_filtering_test.go @@ -14,214 +14,245 @@ import ( "github.com/pocketbase/dbx" ) +const ( + adminUserID = "userid" + regularUserID = "2222" + libraryID1 = 1 + libraryID2 = 2 + libraryID3 = 3 + + tagNameGenre = "genre" + tagValueRock = "rock" + tagValuePop = "pop" + tagValueJazz = "jazz" +) + var _ = Describe("Tag Library Filtering", func() { + var ( + tagRockID = id.NewTagID(tagNameGenre, tagValueRock) + tagPopID = id.NewTagID(tagNameGenre, tagValuePop) + tagJazzID = id.NewTagID(tagNameGenre, tagValueJazz) + ) + + expectTagValues := func(tagList model.TagList, expected []string) { + tagValues := make([]string, len(tagList)) + for i, tag := range tagList { + tagValues[i] = tag.TagValue + } + Expect(tagValues).To(ContainElements(expected)) + } BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) - // Clean up all relevant tables + // Clean up database db := GetDBXBuilder() _, err := db.NewQuery("DELETE FROM library_tag").Execute() Expect(err).ToNot(HaveOccurred()) _, err = db.NewQuery("DELETE FROM tag").Execute() Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("DELETE FROM user_library WHERE user_id != 'userid' AND user_id != '2222'").Execute() + _, err = db.NewQuery("DELETE FROM user_library WHERE user_id != {:admin} AND user_id != {:regular}"). + Bind(dbx.Params{"admin": adminUserID, "regular": regularUserID}).Execute() Expect(err).ToNot(HaveOccurred()) _, err = db.NewQuery("DELETE FROM library WHERE id > 1").Execute() Expect(err).ToNot(HaveOccurred()) // Create test libraries - _, err = db.NewQuery("INSERT INTO library (id, name, path) VALUES (2, 'Library 2', '/music/lib2')").Execute() + _, err = db.NewQuery("INSERT INTO library (id, name, path) VALUES ({:id}, {:name}, {:path})"). + Bind(dbx.Params{"id": libraryID2, "name": "Library 2", "path": "/music/lib2"}).Execute() Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("INSERT INTO library (id, name, path) VALUES (3, 'Library 3', '/music/lib3')").Execute() + _, err = db.NewQuery("INSERT INTO library (id, name, path) VALUES ({:id}, {:name}, {:path})"). + Bind(dbx.Params{"id": libraryID3, "name": "Library 3", "path": "/music/lib3"}).Execute() Expect(err).ToNot(HaveOccurred()) - // Ensure admin user has access to all libraries (since admin users should have access to all libraries) - _, err = db.NewQuery("INSERT OR IGNORE INTO user_library (user_id, library_id) VALUES ('userid', 1)").Execute() - Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("INSERT OR IGNORE INTO user_library (user_id, library_id) VALUES ('userid', 2)").Execute() - Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("INSERT OR IGNORE INTO user_library (user_id, library_id) VALUES ('userid', 3)").Execute() - Expect(err).ToNot(HaveOccurred()) - - // Set up test tags - newTag := func(name, value string) model.Tag { - return model.Tag{ID: id.NewTagID(name, value), TagName: model.TagName(name), TagValue: value} + // Give admin access to all libraries + for _, libID := range []int{libraryID1, libraryID2, libraryID3} { + _, err = db.NewQuery("INSERT OR IGNORE INTO user_library (user_id, library_id) VALUES ({:user}, {:lib})"). + Bind(dbx.Params{"user": adminUserID, "lib": libID}).Execute() + Expect(err).ToNot(HaveOccurred()) } - // Create tags in admin context + // Create test tags adminCtx := request.WithUser(log.NewContext(context.TODO()), adminUser) tagRepo := NewTagRepository(adminCtx, GetDBXBuilder()) - // Add tags to different libraries - err = tagRepo.Add(1, newTag("genre", "rock")) - Expect(err).ToNot(HaveOccurred()) - err = tagRepo.Add(2, newTag("genre", "pop")) - Expect(err).ToNot(HaveOccurred()) - err = tagRepo.Add(3, newTag("genre", "jazz")) - Expect(err).ToNot(HaveOccurred()) - err = tagRepo.Add(2, newTag("genre", "rock")) - Expect(err).ToNot(HaveOccurred()) + createTag := func(libraryID int, name, value string) { + tag := model.Tag{ID: id.NewTagID(name, value), TagName: model.TagName(name), TagValue: value} + err := tagRepo.Add(libraryID, tag) + Expect(err).ToNot(HaveOccurred()) + } - // Update counts manually for testing - _, err = db.NewQuery("UPDATE library_tag SET album_count = 5, media_file_count = 20 WHERE tag_id = {:tagId} AND library_id = 1").Bind(dbx.Params{"tagId": id.NewTagID("genre", "rock")}).Execute() - Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("UPDATE library_tag SET album_count = 3, media_file_count = 10 WHERE tag_id = {:tagId} AND library_id = 2").Bind(dbx.Params{"tagId": id.NewTagID("genre", "pop")}).Execute() - Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("UPDATE library_tag SET album_count = 2, media_file_count = 8 WHERE tag_id = {:tagId} AND library_id = 3").Bind(dbx.Params{"tagId": id.NewTagID("genre", "jazz")}).Execute() - Expect(err).ToNot(HaveOccurred()) - _, err = db.NewQuery("UPDATE library_tag SET album_count = 1, media_file_count = 4 WHERE tag_id = {:tagId} AND library_id = 2").Bind(dbx.Params{"tagId": id.NewTagID("genre", "rock")}).Execute() - Expect(err).ToNot(HaveOccurred()) + createTag(libraryID1, tagNameGenre, tagValueRock) + createTag(libraryID2, tagNameGenre, tagValuePop) + createTag(libraryID3, tagNameGenre, tagValueJazz) + createTag(libraryID2, tagNameGenre, tagValueRock) // Rock appears in both lib1 and lib2 - // Set up user library access - Regular user has access to libraries 1 and 2 only - _, err = db.NewQuery("INSERT INTO user_library (user_id, library_id) VALUES ('2222', 2)").Execute() + // Set tag counts (manually for testing) + setCounts := func(tagID string, libID, albums, songs int) { + _, err := db.NewQuery("UPDATE library_tag SET album_count = {:albums}, media_file_count = {:songs} WHERE tag_id = {:tag} AND library_id = {:lib}"). + Bind(dbx.Params{"albums": albums, "songs": songs, "tag": tagID, "lib": libID}).Execute() + Expect(err).ToNot(HaveOccurred()) + } + + setCounts(tagRockID, libraryID1, 5, 20) + setCounts(tagPopID, libraryID2, 3, 10) + setCounts(tagJazzID, libraryID3, 2, 8) + setCounts(tagRockID, libraryID2, 1, 4) + + // Give regular user access to library 2 only + _, err = db.NewQuery("INSERT INTO user_library (user_id, library_id) VALUES ({:user}, {:lib})"). + Bind(dbx.Params{"user": regularUserID, "lib": libraryID2}).Execute() Expect(err).ToNot(HaveOccurred()) }) Describe("TagRepository Library Filtering", func() { + // Helper to create repository and read all tags + readAllTags := func(user *model.User, filters ...rest.QueryOptions) model.TagList { + var ctx context.Context + if user != nil { + ctx = request.WithUser(log.NewContext(context.TODO()), *user) + } else { + ctx = context.Background() // Headless context + } + + tagRepo := NewTagRepository(ctx, GetDBXBuilder()) + repo := tagRepo.(model.ResourceRepository) + + var opts rest.QueryOptions + if len(filters) > 0 { + opts = filters[0] + } + + tags, err := repo.ReadAll(opts) + Expect(err).ToNot(HaveOccurred()) + return tags.(model.TagList) + } + + // Helper to count tags + countTags := func(user *model.User) int64 { + var ctx context.Context + if user != nil { + ctx = request.WithUser(log.NewContext(context.TODO()), *user) + } else { + ctx = context.Background() + } + + tagRepo := NewTagRepository(ctx, GetDBXBuilder()) + repo := tagRepo.(model.ResourceRepository) + + count, err := repo.Count() + Expect(err).ToNot(HaveOccurred()) + return count + } + Context("Admin User", func() { It("should see all tags regardless of library", func() { - ctx := request.WithUser(log.NewContext(context.TODO()), adminUser) - tagRepo := NewTagRepository(ctx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - tags, err := repo.ReadAll() - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - Expect(tagList).To(HaveLen(3)) + tags := readAllTags(&adminUser) + Expect(tags).To(HaveLen(3)) }) }) Context("Regular User with Limited Library Access", func() { It("should only see tags from accessible libraries", func() { - ctx := request.WithUser(log.NewContext(context.TODO()), regularUser) - tagRepo := NewTagRepository(ctx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - tags, err := repo.ReadAll() - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - + tags := readAllTags(®ularUser) // Should see rock (libraries 1,2) and pop (library 2), but not jazz (library 3) - Expect(tagList).To(HaveLen(2)) + Expect(tags).To(HaveLen(2)) }) It("should respect explicit library_id filters within accessible libraries", func() { - ctx := request.WithUser(log.NewContext(context.TODO()), regularUser) - tagRepo := NewTagRepository(ctx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - // Filter by library 2 (user has access to libraries 1 and 2) - tags, err := repo.ReadAll(rest.QueryOptions{ - Filters: map[string]interface{}{ - "library_id": 2, - }, + tags := readAllTags(®ularUser, rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": libraryID2}, }) - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - // Should see only tags from library 2: pop and rock(lib2) - Expect(tagList).To(HaveLen(2)) - - // Verify the tags are correct - tagValues := make([]string, len(tagList)) - for i, tag := range tagList { - tagValues[i] = tag.TagValue - } - Expect(tagValues).To(ContainElements("pop", "rock")) + Expect(tags).To(HaveLen(2)) + expectTagValues(tags, []string{tagValuePop, tagValueRock}) }) It("should not return tags when filtering by inaccessible library", func() { - ctx := request.WithUser(log.NewContext(context.TODO()), regularUser) - tagRepo := NewTagRepository(ctx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - // Try to filter by library 3 (user doesn't have access) - tags, err := repo.ReadAll(rest.QueryOptions{ - Filters: map[string]interface{}{ - "library_id": 3, - }, + tags := readAllTags(®ularUser, rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": libraryID3}, }) - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - // Should return no tags since user can't access library 3 - Expect(tagList).To(HaveLen(0)) + Expect(tags).To(HaveLen(0)) }) It("should filter by library 1 correctly", func() { - ctx := request.WithUser(log.NewContext(context.TODO()), regularUser) - tagRepo := NewTagRepository(ctx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - // Filter by library 1 (user has access) - tags, err := repo.ReadAll(rest.QueryOptions{ - Filters: map[string]interface{}{ - "library_id": 1, - }, + tags := readAllTags(®ularUser, rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": libraryID1}, }) - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - // Should see only rock from library 1 - Expect(tagList).To(HaveLen(1)) - Expect(tagList[0].TagValue).To(Equal("rock")) + Expect(tags).To(HaveLen(1)) + Expect(tags[0].TagValue).To(Equal(tagValueRock)) + }) + }) + + Context("Headless Processes (No User Context)", func() { + It("should see all tags from all libraries when no user is in context", func() { + tags := readAllTags(nil) // nil = headless context + // Should see all tags from all libraries (no filtering applied) + Expect(tags).To(HaveLen(3)) + expectTagValues(tags, []string{tagValueRock, tagValuePop, tagValueJazz}) + }) + + It("should count all tags from all libraries when no user is in context", func() { + count := countTags(nil) + // Should count all tags from all libraries + Expect(count).To(Equal(int64(3))) + }) + + It("should calculate proper statistics from all libraries for headless processes", func() { + tags := readAllTags(nil) + + // Find the rock tag (appears in libraries 1 and 2) + var rockTag *model.Tag + for _, tag := range tags { + if tag.TagValue == tagValueRock { + rockTag = &tag + break + } + } + Expect(rockTag).ToNot(BeNil()) + + // Should have stats from all libraries where rock appears + // Library 1: 5 albums, 20 songs + // Library 2: 1 album, 4 songs + // Total: 6 albums, 24 songs + Expect(rockTag.AlbumCount).To(Equal(6)) + Expect(rockTag.SongCount).To(Equal(24)) + }) + + It("should allow headless processes to apply explicit library_id filters", func() { + tags := readAllTags(nil, rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": libraryID3}, + }) + // Should see only jazz from library 3 + Expect(tags).To(HaveLen(1)) + Expect(tags[0].TagValue).To(Equal(tagValueJazz)) }) }) Context("Admin User with Explicit Library Filtering", func() { It("should see all tags when no filter is applied", func() { - adminCtx := request.WithUser(log.NewContext(context.TODO()), adminUser) - tagRepo := NewTagRepository(adminCtx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - tags, err := repo.ReadAll() - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - Expect(tagList).To(HaveLen(3)) + tags := readAllTags(&adminUser) + Expect(tags).To(HaveLen(3)) }) It("should respect explicit library_id filters", func() { - adminCtx := request.WithUser(log.NewContext(context.TODO()), adminUser) - tagRepo := NewTagRepository(adminCtx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - // Filter by library 3 - tags, err := repo.ReadAll(rest.QueryOptions{ - Filters: map[string]interface{}{ - "library_id": 3, - }, + tags := readAllTags(&adminUser, rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": libraryID3}, }) - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - // Should see only jazz from library 3 - Expect(tagList).To(HaveLen(1)) - Expect(tagList[0].TagValue).To(Equal("jazz")) + Expect(tags).To(HaveLen(1)) + Expect(tags[0].TagValue).To(Equal(tagValueJazz)) }) It("should filter by library 2 correctly", func() { - adminCtx := request.WithUser(log.NewContext(context.TODO()), adminUser) - tagRepo := NewTagRepository(adminCtx, GetDBXBuilder()) - repo := tagRepo.(model.ResourceRepository) - - // Filter by library 2 - tags, err := repo.ReadAll(rest.QueryOptions{ - Filters: map[string]interface{}{ - "library_id": 2, - }, + tags := readAllTags(&adminUser, rest.QueryOptions{ + Filters: map[string]interface{}{"library_id": libraryID2}, }) - Expect(err).ToNot(HaveOccurred()) - tagList := tags.(model.TagList) - // Should see pop and rock from library 2 - Expect(tagList).To(HaveLen(2)) - - tagValues := make([]string, len(tagList)) - for i, tag := range tagList { - tagValues[i] = tag.TagValue - } - Expect(tagValues).To(ContainElements("pop", "rock")) + Expect(tags).To(HaveLen(2)) + expectTagValues(tags, []string{tagValuePop, tagValueRock}) }) }) }) diff --git a/ui/src/artist/ArtistList.jsx b/ui/src/artist/ArtistList.jsx index 7a14e9efe..e175763e3 100644 --- a/ui/src/artist/ArtistList.jsx +++ b/ui/src/artist/ArtistList.jsx @@ -132,8 +132,10 @@ const ArtistListView = ({ hasShow, hasEdit, hasList, width, ...rest }) => { useResourceRefresh('artist') const role = filterValues?.role - const getCounter = (record, counter) => - role ? record?.stats[role]?.[counter] : record?.[counter] + const getCounter = (record, counter) => { + if (!record) return undefined + return role ? record?.stats?.[role]?.[counter] : record?.[counter] + } const getAlbumCount = (record) => getCounter(record, 'albumCount') const getSongCount = (record) => getCounter(record, 'songCount') const getSize = (record) => {