From c1c7cfa49f3f755249ea0425565d12b387a05831 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 6 Jun 2025 16:16:39 -0400 Subject: [PATCH] refactor(library): implement library management service and update API routes Signed-off-by: Deluan --- cmd/wire_gen.go | 3 +- core/library.go | 185 ++++++++++++++ core/library_test.go | 297 +++++++++++++++++++++++ core/wire_providers.go | 1 + model/errors.go | 1 + server/nativeapi/config_test.go | 2 +- server/nativeapi/library.go | 174 ++++--------- server/nativeapi/library_test.go | 101 +++++++- server/nativeapi/native_api.go | 5 +- server/nativeapi/native_api_song_test.go | 36 +-- 10 files changed, 637 insertions(+), 168 deletions(-) create mode 100644 core/library.go create mode 100644 core/library_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index dc558c393..d6584f235 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -58,7 +58,8 @@ func CreateNativeAPIRouter() *nativeapi.Router { share := core.NewShare(dataStore) playlists := core.NewPlaylists(dataStore) insights := metrics.GetInstance(dataStore) - router := nativeapi.New(dataStore, share, playlists, insights) + library := core.NewLibrary(dataStore) + router := nativeapi.New(dataStore, share, playlists, insights, library) return router } diff --git a/core/library.go b/core/library.go new file mode 100644 index 000000000..a0a9baed0 --- /dev/null +++ b/core/library.go @@ -0,0 +1,185 @@ +package core + +import ( + "context" + "fmt" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" +) + +// Library provides business logic for library management and user-library associations +type Library interface { + // Library CRUD operations + GetAll(ctx context.Context) (model.Libraries, error) + Get(ctx context.Context, id int) (*model.Library, error) + Create(ctx context.Context, library *model.Library) error + Update(ctx context.Context, library *model.Library) error + Delete(ctx context.Context, id int) error + + // User-library association operations + GetUserLibraries(ctx context.Context, userID string) (model.Libraries, error) + SetUserLibraries(ctx context.Context, userID string, libraryIDs []int) error + ValidateLibraryAccess(ctx context.Context, userID string, libraryID int) error +} + +type libraryService struct { + ds model.DataStore +} + +// NewLibrary creates a new Library service +func NewLibrary(ds model.DataStore) Library { + return &libraryService{ds: ds} +} + +// Library CRUD operations + +func (s *libraryService) GetAll(ctx context.Context) (model.Libraries, error) { + return s.ds.Library(ctx).GetAll() +} + +func (s *libraryService) Get(ctx context.Context, id int) (*model.Library, error) { + return s.ds.Library(ctx).Get(id) +} + +func (s *libraryService) Create(ctx context.Context, library *model.Library) error { + if err := s.validateLibrary(library); err != nil { + return err + } + + return s.ds.Library(ctx).Put(library) +} + +func (s *libraryService) Update(ctx context.Context, library *model.Library) error { + if err := s.validateLibrary(library); err != nil { + return err + } + + // Verify library exists + if _, err := s.ds.Library(ctx).Get(library.ID); err != nil { + return err + } + + return s.ds.Library(ctx).Put(library) +} + +func (s *libraryService) Delete(ctx context.Context, id int) error { + // Verify library exists + if _, err := s.ds.Library(ctx).Get(id); err != nil { + return err + } + + return s.ds.Library(ctx).Delete(id) +} + +// User-library association operations + +func (s *libraryService) GetUserLibraries(ctx context.Context, userID string) (model.Libraries, error) { + // Verify user exists + if _, err := s.ds.User(ctx).Get(userID); err != nil { + return nil, err + } + + return s.ds.User(ctx).GetUserLibraries(userID) +} + +func (s *libraryService) SetUserLibraries(ctx context.Context, userID string, libraryIDs []int) error { + // Verify user exists + user, err := s.ds.User(ctx).Get(userID) + if err != nil { + return err + } + + // Admin users get all libraries automatically - don't allow manual assignment + if user.IsAdmin { + return fmt.Errorf("%w: cannot manually assign libraries to admin users", model.ErrValidation) + } + + // Regular users must have at least one library + if len(libraryIDs) == 0 { + return fmt.Errorf("%w: at least one library must be assigned to non-admin users", model.ErrValidation) + } + + // Validate all library IDs exist + if len(libraryIDs) > 0 { + if err := s.validateLibraryIDs(ctx, libraryIDs); err != nil { + return err + } + } + + return s.ds.User(ctx).SetUserLibraries(userID, libraryIDs) +} + +func (s *libraryService) ValidateLibraryAccess(ctx context.Context, userID string, libraryID int) error { + user, ok := request.UserFrom(ctx) + if !ok { + return fmt.Errorf("user not found in context") + } + + // Admin users have access to all libraries + if user.IsAdmin { + return nil + } + + // Check if user has explicit access to this library + libraries, err := s.ds.User(ctx).GetUserLibraries(userID) + if err != nil { + log.Error(ctx, "Error checking library access", "userID", userID, "libraryID", libraryID, err) + return fmt.Errorf("error checking library access: %w", err) + } + + for _, lib := range libraries { + if lib.ID == libraryID { + return nil + } + } + + return fmt.Errorf("%w: user does not have access to library %d", model.ErrNotAuthorized, libraryID) +} + +// Helper methods + +func (s *libraryService) validateLibrary(library *model.Library) error { + if library.Name == "" { + return fmt.Errorf("%w: library name is required", model.ErrValidation) + } + if library.Path == "" { + return fmt.Errorf("%w: library path is required", model.ErrValidation) + } + return nil +} + +func (s *libraryService) validateLibraryIDs(ctx context.Context, libraryIDs []int) error { + // Use CountAll with IN filter to efficiently check if all library IDs exist + count, err := s.ds.Library(ctx).CountAll(model.QueryOptions{ + Filters: squirrel.Eq{"id": libraryIDs}, + }) + if err != nil { + return fmt.Errorf("error validating library IDs: %w", err) + } + + if int(count) != len(libraryIDs) { + // Find which library IDs don't exist for better error message + existingLibraries, err := s.ds.Library(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"id": libraryIDs}, + }) + if err != nil { + return fmt.Errorf("error getting libraries: %w", err) + } + + libraryMap := make(map[int]bool) + for _, lib := range existingLibraries { + libraryMap[lib.ID] = true + } + + for _, libID := range libraryIDs { + if !libraryMap[libID] { + return fmt.Errorf("%w: library ID %d does not exist", model.ErrNotFound, libID) + } + } + } + + return nil +} diff --git a/core/library_test.go b/core/library_test.go new file mode 100644 index 000000000..b2e1edead --- /dev/null +++ b/core/library_test.go @@ -0,0 +1,297 @@ +package core + +import ( + "context" + + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Library Service", func() { + var service Library + var ds *tests.MockDataStore + var libraryRepo *tests.MockLibraryRepo + var userRepo *tests.MockedUserRepo + var ctx context.Context + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + + ds = &tests.MockDataStore{} + libraryRepo = &tests.MockLibraryRepo{} + userRepo = tests.CreateMockUserRepo() + ds.MockedLibrary = libraryRepo + ds.MockedUser = userRepo + + service = NewLibrary(ds) + ctx = context.Background() + }) + + Describe("Library CRUD Operations", func() { + Describe("GetAll", func() { + It("returns all libraries", func() { + libraries := model.Libraries{ + {ID: 1, Name: "Library 1", Path: "/music1"}, + {ID: 2, Name: "Library 2", Path: "/music2"}, + } + libraryRepo.SetData(libraries) + + result, err := service.GetAll(ctx) + + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(HaveLen(2)) + Expect(result[0].Name).To(Equal("Library 1")) + Expect(result[1].Name).To(Equal("Library 2")) + }) + + It("returns empty list when no libraries exist", func() { + result, err := service.GetAll(ctx) + + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(HaveLen(0)) + }) + }) + + Describe("Get", func() { + It("returns a specific library", func() { + library := &model.Library{ID: 1, Name: "Test Library", Path: "/music"} + libraryRepo.SetData(model.Libraries{*library}) + + result, err := service.Get(ctx, 1) + + Expect(err).NotTo(HaveOccurred()) + Expect(result.Name).To(Equal("Test Library")) + Expect(result.Path).To(Equal("/music")) + }) + + It("returns error when library not found", func() { + _, err := service.Get(ctx, 999) + + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(model.ErrNotFound)) + }) + }) + + Describe("Create", func() { + It("creates a new library successfully", func() { + library := &model.Library{ID: 1, Name: "New Library", Path: "/new/music"} + + err := service.Create(ctx, library) + + Expect(err).NotTo(HaveOccurred()) + Expect(libraryRepo.Data[1].Name).To(Equal("New Library")) + Expect(libraryRepo.Data[1].Path).To(Equal("/new/music")) + }) + + It("fails when library name is empty", func() { + library := &model.Library{Path: "/music"} + + err := service.Create(ctx, library) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("library name is required")) + }) + + It("fails when library path is empty", func() { + library := &model.Library{Name: "Test"} + + err := service.Create(ctx, library) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("library path is required")) + }) + }) + + Describe("Update", func() { + BeforeEach(func() { + libraryRepo.SetData(model.Libraries{ + {ID: 1, Name: "Original Library", Path: "/original"}, + }) + }) + + It("updates an existing library successfully", func() { + library := &model.Library{ID: 1, Name: "Updated Library", Path: "/updated"} + + err := service.Update(ctx, library) + + Expect(err).NotTo(HaveOccurred()) + Expect(libraryRepo.Data[1].Name).To(Equal("Updated Library")) + Expect(libraryRepo.Data[1].Path).To(Equal("/updated")) + }) + + It("fails when library doesn't exist", func() { + library := &model.Library{ID: 999, Name: "Non-existent", Path: "/path"} + + err := service.Update(ctx, library) + + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(model.ErrNotFound)) + }) + + It("fails when library name is empty", func() { + library := &model.Library{ID: 1, Path: "/music"} + + err := service.Update(ctx, library) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("library name is required")) + }) + }) + + Describe("Delete", func() { + BeforeEach(func() { + libraryRepo.SetData(model.Libraries{ + {ID: 1, Name: "Library to Delete", Path: "/delete"}, + }) + }) + + It("deletes an existing library successfully", func() { + err := service.Delete(ctx, 1) + + Expect(err).NotTo(HaveOccurred()) + Expect(libraryRepo.Data).To(HaveLen(0)) + }) + + It("fails when library doesn't exist", func() { + err := service.Delete(ctx, 999) + + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(model.ErrNotFound)) + }) + }) + }) + + Describe("User-Library Association Operations", func() { + var regularUser, adminUser *model.User + + BeforeEach(func() { + regularUser = &model.User{ID: "user1", UserName: "regular", IsAdmin: false} + adminUser = &model.User{ID: "admin1", UserName: "admin", IsAdmin: true} + + userRepo.Data = map[string]*model.User{ + "regular": regularUser, + "admin": adminUser, + } + libraryRepo.SetData(model.Libraries{ + {ID: 1, Name: "Library 1", Path: "/music1"}, + {ID: 2, Name: "Library 2", Path: "/music2"}, + {ID: 3, Name: "Library 3", Path: "/music3"}, + }) + }) + + Describe("GetUserLibraries", func() { + It("returns user's libraries", func() { + userRepo.UserLibraries = map[string][]int{ + "user1": {1}, + } + + result, err := service.GetUserLibraries(ctx, "user1") + + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].ID).To(Equal(1)) + }) + + It("fails when user doesn't exist", func() { + _, err := service.GetUserLibraries(ctx, "nonexistent") + + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(model.ErrNotFound)) + }) + }) + + Describe("SetUserLibraries", func() { + It("sets libraries for regular user successfully", func() { + err := service.SetUserLibraries(ctx, "user1", []int{1, 2}) + + Expect(err).NotTo(HaveOccurred()) + libraries := userRepo.UserLibraries["user1"] + Expect(libraries).To(HaveLen(2)) + }) + + It("fails when user doesn't exist", func() { + err := service.SetUserLibraries(ctx, "nonexistent", []int{1}) + + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(model.ErrNotFound)) + }) + + It("fails when trying to set libraries for admin user", func() { + err := service.SetUserLibraries(ctx, "admin1", []int{1}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot manually assign libraries to admin users")) + }) + + It("fails when no libraries provided for regular user", func() { + err := service.SetUserLibraries(ctx, "user1", []int{}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("at least one library must be assigned to non-admin users")) + }) + + It("fails when library doesn't exist", func() { + err := service.SetUserLibraries(ctx, "user1", []int{999}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("library ID 999 does not exist")) + }) + + It("fails when some libraries don't exist", func() { + err := service.SetUserLibraries(ctx, "user1", []int{1, 999, 2}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("library ID 999 does not exist")) + }) + }) + + Describe("ValidateLibraryAccess", func() { + Context("admin user", func() { + BeforeEach(func() { + ctx = request.WithUser(ctx, *adminUser) + }) + + It("allows access to any library", func() { + err := service.ValidateLibraryAccess(ctx, "admin1", 1) + + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Context("regular user", func() { + BeforeEach(func() { + ctx = request.WithUser(ctx, *regularUser) + userRepo.UserLibraries = map[string][]int{ + "user1": {1}, + } + }) + + It("allows access to user's libraries", func() { + err := service.ValidateLibraryAccess(ctx, "user1", 1) + + Expect(err).NotTo(HaveOccurred()) + }) + + It("denies access to libraries user doesn't have", func() { + err := service.ValidateLibraryAccess(ctx, "user1", 2) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("user does not have access to library 2")) + }) + }) + + Context("no user in context", func() { + It("fails with user not found error", func() { + err := service.ValidateLibraryAccess(ctx, "user1", 1) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("user not found in context")) + }) + }) + }) + }) +}) diff --git a/core/wire_providers.go b/core/wire_providers.go index 482cfbefe..ae365156a 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -17,6 +17,7 @@ var Set = wire.NewSet( NewPlayers, NewShare, NewPlaylists, + NewLibrary, agents.GetAgents, external.NewProvider, wire.Bind(new(external.Agents), new(*agents.Agents)), diff --git a/model/errors.go b/model/errors.go index ff4be5723..41029d316 100644 --- a/model/errors.go +++ b/model/errors.go @@ -8,4 +8,5 @@ var ( ErrNotAuthorized = errors.New("not authorized") ErrExpired = errors.New("access expired") ErrNotAvailable = errors.New("functionality not available") + ErrValidation = errors.New("validation error") ) diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index 206c17327..2e89e3440 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -28,7 +28,7 @@ var _ = Describe("Config API", func() { conf.Server.DevUIShowConfig = true // Enable config endpoint for tests ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/library.go b/server/nativeapi/library.go index 071ddddfa..e0ea6d189 100644 --- a/server/nativeapi/library.go +++ b/server/nativeapi/library.go @@ -3,12 +3,12 @@ package nativeapi import ( "context" "encoding/json" - "fmt" + "errors" "net/http" "strconv" - "github.com/Masterminds/squirrel" "github.com/go-chi/chi/v5" + "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" ) @@ -16,13 +16,13 @@ import ( // Library management endpoints (admin only) func (n *Router) addLibraryRoute(r chi.Router) { r.Route("/library", func(r chi.Router) { - r.Get("/", getLibraries(n.ds)) - r.Post("/", createLibrary(n.ds)) + r.Get("/", getLibraries(n.libs)) + r.Post("/", createLibrary(n.libs)) r.Route("/{id}", func(r chi.Router) { r.Use(parseIDMiddleware) - r.Get("/", getLibrary(n.ds)) - r.Put("/", updateLibrary(n.ds)) - r.Delete("/", deleteLibrary(n.ds)) + r.Get("/", getLibrary(n.libs)) + r.Put("/", updateLibrary(n.libs)) + r.Delete("/", deleteLibrary(n.libs)) }) }) } @@ -31,8 +31,8 @@ func (n *Router) addLibraryRoute(r chi.Router) { func (n *Router) addUserLibraryRoute(r chi.Router) { r.Route("/user/{id}/library", func(r chi.Router) { r.Use(parseUserIDMiddleware) - r.Get("/", getUserLibraries(n.ds)) - r.Put("/", setUserLibraries(n.ds)) + r.Get("/", getUserLibraries(n.libs)) + r.Put("/", setUserLibraries(n.libs)) }) } @@ -65,9 +65,9 @@ func parseUserIDMiddleware(next http.Handler) http.Handler { // Library CRUD handlers -func getLibraries(ds model.DataStore) http.HandlerFunc { +func getLibraries(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - libraries, err := ds.Library(r.Context()).GetAll() + libraries, err := service.GetAll(r.Context()) if err != nil { log.Error(r.Context(), "Error getting libraries", err) http.Error(w, "Internal server error", http.StatusInternalServerError) @@ -81,13 +81,13 @@ func getLibraries(ds model.DataStore) http.HandlerFunc { } } -func getLibrary(ds model.DataStore) http.HandlerFunc { +func getLibrary(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { libraryID := r.Context().Value("libraryID").(int) - library, err := ds.Library(r.Context()).Get(libraryID) + lib, err := service.Get(r.Context(), libraryID) if err != nil { - if err == model.ErrNotFound { + if errors.Is(err, model.ErrNotFound) { http.Error(w, "Library not found", http.StatusNotFound) return } @@ -97,92 +97,80 @@ func getLibrary(ds model.DataStore) http.HandlerFunc { } w.Header().Set("Content-Type", "application/json") - if err := json.NewEncoder(w).Encode(library); err != nil { + if err := json.NewEncoder(w).Encode(lib); err != nil { log.Error(r.Context(), "Error encoding library response", err) } } } -func createLibrary(ds model.DataStore) http.HandlerFunc { +func createLibrary(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - var library model.Library - if err := json.NewDecoder(r.Body).Decode(&library); err != nil { + var lib model.Library + if err := json.NewDecoder(r.Body).Decode(&lib); err != nil { log.Error(r.Context(), "Error decoding library request", err) http.Error(w, "Invalid request body", http.StatusBadRequest) return } - // Validate required fields - if library.Name == "" { - http.Error(w, "Library name is required", http.StatusBadRequest) - return - } - if library.Path == "" { - http.Error(w, "Library path is required", http.StatusBadRequest) - return - } - - if err := ds.Library(r.Context()).Put(&library); err != nil { + if err := service.Create(r.Context(), &lib); err != nil { log.Error(r.Context(), "Error creating library", err) + if errors.Is(err, model.ErrValidation) { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } http.Error(w, "Failed to create library", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated) - if err := json.NewEncoder(w).Encode(library); err != nil { + if err := json.NewEncoder(w).Encode(lib); err != nil { log.Error(r.Context(), "Error encoding library response", err) } } } -func updateLibrary(ds model.DataStore) http.HandlerFunc { +func updateLibrary(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { libraryID := r.Context().Value("libraryID").(int) - var library model.Library - if err := json.NewDecoder(r.Body).Decode(&library); err != nil { + var lib model.Library + if err := json.NewDecoder(r.Body).Decode(&lib); err != nil { log.Error(r.Context(), "Error decoding library request", err) http.Error(w, "Invalid request body", http.StatusBadRequest) return } // Ensure the ID matches the URL parameter - library.ID = libraryID + lib.ID = libraryID - // Validate required fields - if library.Name == "" { - http.Error(w, "Library name is required", http.StatusBadRequest) - return - } - if library.Path == "" { - http.Error(w, "Library path is required", http.StatusBadRequest) - return - } - - if err := ds.Library(r.Context()).Put(&library); err != nil { - if err == model.ErrNotFound { + if err := service.Update(r.Context(), &lib); err != nil { + if errors.Is(err, model.ErrNotFound) { http.Error(w, "Library not found", http.StatusNotFound) return } + if errors.Is(err, model.ErrValidation) { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } log.Error(r.Context(), "Error updating library", "libraryID", libraryID, err) http.Error(w, "Failed to update library", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "application/json") - if err := json.NewEncoder(w).Encode(library); err != nil { + if err := json.NewEncoder(w).Encode(lib); err != nil { log.Error(r.Context(), "Error encoding library response", err) } } } -func deleteLibrary(ds model.DataStore) http.HandlerFunc { +func deleteLibrary(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { libraryID := r.Context().Value("libraryID").(int) - if err := ds.Library(r.Context()).Delete(libraryID); err != nil { - if err == model.ErrNotFound { + if err := service.Delete(r.Context(), libraryID); err != nil { + if errors.Is(err, model.ErrNotFound) { http.Error(w, "Library not found", http.StatusNotFound) return } @@ -197,23 +185,16 @@ func deleteLibrary(ds model.DataStore) http.HandlerFunc { // User-library association handlers -func getUserLibraries(ds model.DataStore) http.HandlerFunc { +func getUserLibraries(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { userID := r.Context().Value("userID").(string) - // Verify user exists - if _, err := ds.User(r.Context()).Get(userID); err != nil { - if err == model.ErrNotFound { + libraries, err := service.GetUserLibraries(r.Context(), userID) + if err != nil { + if errors.Is(err, model.ErrNotFound) { http.Error(w, "User not found", http.StatusNotFound) return } - log.Error(r.Context(), "Error getting user", "userID", userID, err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } - - libraries, err := ds.User(r.Context()).GetUserLibraries(userID) - if err != nil { log.Error(r.Context(), "Error getting user libraries", "userID", userID, err) http.Error(w, "Internal server error", http.StatusInternalServerError) return @@ -226,22 +207,10 @@ func getUserLibraries(ds model.DataStore) http.HandlerFunc { } } -func setUserLibraries(ds model.DataStore) http.HandlerFunc { +func setUserLibraries(service core.Library) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { userID := r.Context().Value("userID").(string) - // Verify user exists - user, err := ds.User(r.Context()).Get(userID) - if err != nil { - if err == model.ErrNotFound { - http.Error(w, "User not found", http.StatusNotFound) - return - } - log.Error(r.Context(), "Error getting user", "userID", userID, err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } - var request struct { LibraryIDs []int `json:"libraryIds"` } @@ -251,63 +220,22 @@ func setUserLibraries(ds model.DataStore) http.HandlerFunc { return } - // Admin users get all libraries automatically - don't allow manual assignment - if user.IsAdmin { - http.Error(w, "Cannot manually assign libraries to admin users", http.StatusBadRequest) - return - } - - // Validate library IDs exist - if len(request.LibraryIDs) > 0 { - // Use CountAll with IN filter to efficiently check if all library IDs exist - count, err := ds.Library(r.Context()).CountAll(model.QueryOptions{ - Filters: squirrel.Eq{"id": request.LibraryIDs}, - }) - if err != nil { - log.Error(r.Context(), "Error counting libraries", err) - http.Error(w, "Internal server error", http.StatusInternalServerError) + if err := service.SetUserLibraries(r.Context(), userID, request.LibraryIDs); err != nil { + log.Error(r.Context(), "Error setting user libraries", "userID", userID, err) + if errors.Is(err, model.ErrNotFound) { + http.Error(w, "User not found", http.StatusNotFound) return } - - if int(count) != len(request.LibraryIDs) { - // Some library IDs don't exist - find which ones for better error message - existingLibraries, err := ds.Library(r.Context()).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"id": request.LibraryIDs}, - }) - if err != nil { - log.Error(r.Context(), "Error getting all libraries", err) - http.Error(w, "Internal server error", http.StatusInternalServerError) - return - } - - libraryMap := make(map[int]bool) - for _, lib := range existingLibraries { - libraryMap[lib.ID] = true - } - - for _, libID := range request.LibraryIDs { - if !libraryMap[libID] { - http.Error(w, fmt.Sprintf("Library ID %d does not exist", libID), http.StatusBadRequest) - return - } - } + if errors.Is(err, model.ErrValidation) { + http.Error(w, err.Error(), http.StatusBadRequest) + return } - } - - // Regular users must have at least one library - if len(request.LibraryIDs) == 0 { - http.Error(w, "At least one library must be assigned to non-admin users", http.StatusBadRequest) - return - } - - if err := ds.User(r.Context()).SetUserLibraries(userID, request.LibraryIDs); err != nil { - log.Error(r.Context(), "Error setting user libraries", "userID", userID, err) http.Error(w, "Failed to set user libraries", http.StatusInternalServerError) return } // Return updated user libraries - libraries, err := ds.User(r.Context()).GetUserLibraries(userID) + libraries, err := service.GetUserLibraries(r.Context(), userID) if err != nil { log.Error(r.Context(), "Error getting updated user libraries", "userID", userID, err) http.Error(w, "Internal server error", http.StatusInternalServerError) diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index 06f7cfc4a..66a2022dd 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -10,6 +10,7 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server" @@ -28,7 +29,7 @@ var _ = Describe("Library API", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, &mockLibraryService{}) router = server.JWTVerifier(nativeRouter) // Create test users @@ -162,7 +163,7 @@ var _ = Describe("Library API", func() { router.ServeHTTP(w, req) Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(w.Body.String()).To(ContainSubstring("Library name is required")) + Expect(w.Body.String()).To(ContainSubstring("library name is required")) }) It("validates path field", func() { @@ -177,7 +178,7 @@ var _ = Describe("Library API", func() { router.ServeHTTP(w, req) Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(w.Body.String()).To(ContainSubstring("Library path is required")) + Expect(w.Body.String()).To(ContainSubstring("library path is required")) }) }) @@ -352,7 +353,7 @@ var _ = Describe("Library API", func() { router.ServeHTTP(w, req) Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(w.Body.String()).To(ContainSubstring("Library ID 999 does not exist")) + Expect(w.Body.String()).To(ContainSubstring("library ID 999 does not exist")) }) It("requires at least one library for regular users", func() { @@ -366,7 +367,7 @@ var _ = Describe("Library API", func() { router.ServeHTTP(w, req) Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(w.Body.String()).To(ContainSubstring("At least one library must be assigned")) + Expect(w.Body.String()).To(ContainSubstring("at least one library must be assigned")) }) It("prevents manual assignment to admin users", func() { @@ -380,7 +381,7 @@ var _ = Describe("Library API", func() { router.ServeHTTP(w, req) Expect(w.Code).To(Equal(http.StatusBadRequest)) - Expect(w.Body.String()).To(ContainSubstring("Cannot manually assign libraries to admin users")) + Expect(w.Body.String()).To(ContainSubstring("cannot manually assign libraries to admin users")) }) }) }) @@ -440,3 +441,91 @@ func Split(s, sep string) []string { result = append(result, s[start:]) return result } + +// Mock library service for testing +type mockLibraryService struct { + core.Library +} + +func (m *mockLibraryService) GetAll(ctx context.Context) (model.Libraries, error) { + return model.Libraries{ + {ID: 1, Name: "Test Library 1", Path: "/music/library1"}, + {ID: 2, Name: "Test Library 2", Path: "/music/library2"}, + }, nil +} + +func (m *mockLibraryService) Get(ctx context.Context, id int) (*model.Library, error) { + if id == 999 { + return nil, model.ErrNotFound + } + return &model.Library{ID: 1, Name: "Test Library 1", Path: "/music/library1"}, nil +} + +func (m *mockLibraryService) Create(ctx context.Context, library *model.Library) error { + if library.Name == "" { + return fmt.Errorf("%w: library name is required", model.ErrValidation) + } + if library.Path == "" { + return fmt.Errorf("%w: library path is required", model.ErrValidation) + } + return nil +} + +func (m *mockLibraryService) Update(ctx context.Context, library *model.Library) error { + if library.ID == 999 { + return model.ErrNotFound + } + if library.Name == "" { + return fmt.Errorf("%w: library name is required", model.ErrValidation) + } + if library.Path == "" { + return fmt.Errorf("%w: library path is required", model.ErrValidation) + } + return nil +} + +func (m *mockLibraryService) Delete(ctx context.Context, id int) error { + if id == 999 { + return model.ErrNotFound + } + return nil +} + +func (m *mockLibraryService) GetUserLibraries(ctx context.Context, userID string) (model.Libraries, error) { + if userID == "non-existent" { + return nil, model.ErrNotFound + } + // If setting multiple libraries, return all requested ones + if userID == "user-1" { + return model.Libraries{ + {ID: 1, Name: "Test Library 1", Path: "/music/library1"}, + {ID: 2, Name: "Test Library 2", Path: "/music/library2"}, + }, nil + } + return model.Libraries{ + {ID: 1, Name: "Test Library 1", Path: "/music/library1"}, + }, nil +} + +func (m *mockLibraryService) SetUserLibraries(ctx context.Context, userID string, libraryIDs []int) error { + if userID == "non-existent" { + return model.ErrNotFound + } + if userID == "admin-1" { + return fmt.Errorf("%w: cannot manually assign libraries to admin users", model.ErrValidation) + } + if len(libraryIDs) == 0 { + return fmt.Errorf("%w: at least one library must be assigned to non-admin users", model.ErrValidation) + } + for _, id := range libraryIDs { + if id == 999 { + return fmt.Errorf("%w: library ID 999 does not exist", model.ErrValidation) + } + } + return nil +} + +func (m *mockLibraryService) ValidateLibraryAccess(ctx context.Context, userID string, libraryID int) error { + // For testing purposes, allow admin access and check basic user access + return nil +} diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index be5e38030..b08be153e 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -26,10 +26,11 @@ type Router struct { share core.Share playlists core.Playlists insights metrics.Insights + libs core.Library } -func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights} +func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService} r.Handler = r.routes() return r } diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go index 0b183c1d9..64257f1f5 100644 --- a/server/nativeapi/native_api_song_test.go +++ b/server/nativeapi/native_api_song_test.go @@ -2,20 +2,16 @@ package nativeapi import ( "bytes" - "context" "encoding/json" "net/http" "net/http/httptest" "net/url" "time" - "github.com/deluan/rest" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/auth" - "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server" "github.com/navidrome/navidrome/tests" @@ -23,31 +19,6 @@ import ( . "github.com/onsi/gomega" ) -// Simple mock implementations for missing types -type mockShare struct { - core.Share -} - -func (m *mockShare) NewRepository(ctx context.Context) rest.Repository { - return &tests.MockShareRepo{} -} - -type mockPlaylists struct { - core.Playlists -} - -func (m *mockPlaylists) ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) { - return &model.Playlist{}, nil -} - -type mockInsights struct { - metrics.Insights -} - -func (m *mockInsights) LastRun(ctx context.Context) (time.Time, bool) { - return time.Now(), true -} - var _ = Describe("Song Endpoints", func() { var ( router http.Handler @@ -122,13 +93,8 @@ var _ = Describe("Song Endpoints", func() { } mfRepo.SetData(testSongs) - // Setup router with mocked dependencies - mockShareImpl := &mockShare{} - mockPlaylistsImpl := &mockPlaylists{} - mockInsightsImpl := &mockInsights{} - // Create the native API router and wrap it with the JWTVerifier middleware - nativeRouter := New(ds, mockShareImpl, mockPlaylistsImpl, mockInsightsImpl) + nativeRouter := New(ds, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() })