From 4ab37346fc716700fe19224dae1765cb3571a39a Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 23 Jun 2025 16:44:55 -0400 Subject: [PATCH] feat(library): enhance library validation and management with path checks and normalization - WIP Signed-off-by: Deluan --- core/library.go | 39 +++++- core/library_test.go | 177 +++++++++++++++++++++++-- persistence/library_repository.go | 14 +- persistence/library_repository_test.go | 95 ++++++++++++- persistence/user_repository_test.go | 22 +-- scanner/controller_test.go | 2 - 6 files changed, 317 insertions(+), 32 deletions(-) diff --git a/core/library.go b/core/library.go index 5d6e132a2..0ef31e720 100644 --- a/core/library.go +++ b/core/library.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" "strconv" "github.com/Masterminds/squirrel" @@ -179,16 +181,41 @@ func (r *libraryRepositoryWrapper) mapError(err error) error { } func (r *libraryRepositoryWrapper) validateLibrary(library *model.Library) error { + errors := make(map[string]string) + if library.Name == "" { - return rest.ValidationError{Errors: map[string]string{ - "name": "library name is required", - }} + errors["name"] = "library name is required" } if library.Path == "" { - return rest.ValidationError{Errors: map[string]string{ - "path": "library path is required", - }} + errors["path"] = "library path is required" + } else { + // Validate path format + if !filepath.IsAbs(library.Path) { + errors["path"] = "library path must be absolute" + } else { + // Clean the path to normalize it + cleanPath := filepath.Clean(library.Path) + library.Path = cleanPath + + // Check if path exists and is accessible + info, err := os.Stat(library.Path) + if err != nil { + if os.IsNotExist(err) { + errors["path"] = "library path does not exist" + } else if os.IsPermission(err) { + errors["path"] = "library path is not accessible" + } else { + errors["path"] = fmt.Sprintf("error accessing library path: %v", err) + } + } else if !info.IsDir() { + errors["path"] = "library path must be a directory" + } + } + } + + if len(errors) > 0 { + return rest.ValidationError{Errors: errors} } return nil diff --git a/core/library_test.go b/core/library_test.go index ceee81219..d956047c9 100644 --- a/core/library_test.go +++ b/core/library_test.go @@ -2,6 +2,8 @@ package core_test import ( "context" + "os" + "path/filepath" "github.com/deluan/rest" "github.com/navidrome/navidrome/conf/configtest" @@ -19,6 +21,7 @@ var _ = Describe("Library Service", func() { var libraryRepo *tests.MockLibraryRepo var userRepo *tests.MockedUserRepo var ctx context.Context + var tempDir string BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) @@ -31,6 +34,14 @@ var _ = Describe("Library Service", func() { service = core.NewLibrary(ds) ctx = context.Background() + + // Create a temporary directory for testing valid paths + var err error + tempDir, err = os.MkdirTemp("", "navidrome-library-test-") + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { + os.RemoveAll(tempDir) + }) }) Describe("Library CRUD Operations", func() { @@ -43,17 +54,17 @@ var _ = Describe("Library Service", func() { Describe("Create", func() { It("creates a new library successfully", func() { - library := &model.Library{ID: 1, Name: "New Library", Path: "/new/music"} + library := &model.Library{ID: 1, Name: "New Library", Path: tempDir} _, err := repo.Save(library) Expect(err).NotTo(HaveOccurred()) Expect(libraryRepo.Data[1].Name).To(Equal("New Library")) - Expect(libraryRepo.Data[1].Path).To(Equal("/new/music")) + Expect(libraryRepo.Data[1].Path).To(Equal(tempDir)) }) It("fails when library name is empty", func() { - library := &model.Library{Path: "/music"} + library := &model.Library{Path: tempDir} _, err := repo.Save(library) @@ -69,27 +80,41 @@ var _ = Describe("Library Service", func() { Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("library path is required")) }) + + It("cleans and normalizes the path", func() { + unnormalizedPath := tempDir + "//../" + filepath.Base(tempDir) + library := &model.Library{ID: 1, Name: "Test Library", Path: unnormalizedPath} + + _, err := repo.Save(library) + + Expect(err).NotTo(HaveOccurred()) + Expect(libraryRepo.Data[1].Path).To(Equal(filepath.Clean(unnormalizedPath))) + }) }) Describe("Update", func() { BeforeEach(func() { libraryRepo.SetData(model.Libraries{ - {ID: 1, Name: "Original Library", Path: "/original"}, + {ID: 1, Name: "Original Library", Path: tempDir}, }) }) It("updates an existing library successfully", func() { - library := &model.Library{ID: 1, Name: "Updated Library", Path: "/updated"} + newTempDir, err := os.MkdirTemp("", "navidrome-library-update-") + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { os.RemoveAll(newTempDir) }) - err := repo.Update("1", library) + library := &model.Library{ID: 1, Name: "Updated Library", Path: newTempDir} + + err = repo.Update("1", library) Expect(err).NotTo(HaveOccurred()) Expect(libraryRepo.Data[1].Name).To(Equal("Updated Library")) - Expect(libraryRepo.Data[1].Path).To(Equal("/updated")) + Expect(libraryRepo.Data[1].Path).To(Equal(newTempDir)) }) It("fails when library doesn't exist", func() { - library := &model.Library{ID: 999, Name: "Non-existent", Path: "/path"} + library := &model.Library{ID: 999, Name: "Non-existent", Path: tempDir} err := repo.Update("999", library) @@ -98,19 +123,151 @@ var _ = Describe("Library Service", func() { }) It("fails when library name is empty", func() { - library := &model.Library{ID: 1, Path: "/music"} + library := &model.Library{ID: 1, Path: tempDir} err := repo.Update("1", library) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("library name is required")) }) + + It("cleans and normalizes the path on update", func() { + unnormalizedPath := tempDir + "//../" + filepath.Base(tempDir) + library := &model.Library{ID: 1, Name: "Updated Library", Path: unnormalizedPath} + + err := repo.Update("1", library) + + Expect(err).NotTo(HaveOccurred()) + Expect(libraryRepo.Data[1].Path).To(Equal(filepath.Clean(unnormalizedPath))) + }) + }) + + Describe("Path Validation", func() { + Context("Create operation", func() { + It("fails when path is not absolute", func() { + library := &model.Library{Name: "Test", Path: "relative/path"} + + _, err := repo.Save(library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path must be absolute")) + }) + + It("fails when path does not exist", func() { + nonExistentPath := filepath.Join(tempDir, "nonexistent") + library := &model.Library{Name: "Test", Path: nonExistentPath} + + _, err := repo.Save(library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path does not exist")) + }) + + It("fails when path is a file instead of directory", func() { + testFile := filepath.Join(tempDir, "testfile.txt") + err := os.WriteFile(testFile, []byte("test"), 0644) + Expect(err).NotTo(HaveOccurred()) + + library := &model.Library{Name: "Test", Path: testFile} + + _, err = repo.Save(library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path must be a directory")) + }) + + It("fails when path is not accessible due to permissions", func() { + Skip("Permission tests are environment-dependent and may fail in CI") + // This test is skipped because creating a directory with no read permissions + // is complex and may not work consistently across different environments + }) + + It("handles multiple validation errors", func() { + library := &model.Library{Name: "", Path: "relative/path"} + + _, err := repo.Save(library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors).To(HaveKey("name")) + Expect(err.(rest.ValidationError).Errors).To(HaveKey("path")) + Expect(err.(rest.ValidationError).Errors["name"]).To(Equal("library name is required")) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path must be absolute")) + }) + }) + + Context("Update operation", func() { + BeforeEach(func() { + libraryRepo.SetData(model.Libraries{ + {ID: 1, Name: "Test Library", Path: tempDir}, + }) + }) + + It("fails when updated path is not absolute", func() { + library := &model.Library{ID: 1, Name: "Test", Path: "relative/path"} + + err := repo.Update("1", library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path must be absolute")) + }) + + It("fails when updated path does not exist", func() { + nonExistentPath := filepath.Join(tempDir, "nonexistent") + library := &model.Library{ID: 1, Name: "Test", Path: nonExistentPath} + + err := repo.Update("1", library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path does not exist")) + }) + + It("fails when updated path is a file instead of directory", func() { + testFile := filepath.Join(tempDir, "updatefile.txt") + err := os.WriteFile(testFile, []byte("test"), 0644) + Expect(err).NotTo(HaveOccurred()) + + library := &model.Library{ID: 1, Name: "Test", Path: testFile} + + err = repo.Update("1", library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path must be a directory")) + }) + + It("handles multiple validation errors on update", func() { + library := &model.Library{ID: 1, Name: "", Path: "relative/path"} + + err := repo.Update("1", library) + + Expect(err).To(HaveOccurred()) + var validationErr rest.ValidationError + Expect(err).To(BeAssignableToTypeOf(validationErr)) + Expect(err.(rest.ValidationError).Errors).To(HaveKey("name")) + Expect(err.(rest.ValidationError).Errors).To(HaveKey("path")) + Expect(err.(rest.ValidationError).Errors["name"]).To(Equal("library name is required")) + Expect(err.(rest.ValidationError).Errors["path"]).To(Equal("library path must be absolute")) + }) + }) }) Describe("Delete", func() { BeforeEach(func() { libraryRepo.SetData(model.Libraries{ - {ID: 1, Name: "Library to Delete", Path: "/delete"}, + {ID: 1, Name: "Library to Delete", Path: tempDir}, }) }) diff --git a/persistence/library_repository.go b/persistence/library_repository.go index 11332805e..bfe3fd75d 100644 --- a/persistence/library_repository.go +++ b/persistence/library_repository.go @@ -83,10 +83,12 @@ func (r *libraryRepository) Put(l *model.Library) error { var err error if l.ID == 0 { + // Insert with autoassigned ID l.CreatedAt = time.Now() l.UpdatedAt = time.Now() err = r.db.Model(l).Insert() } else { + // Try to update first cols := map[string]any{ "name": l.Name, "path": l.Path, @@ -94,7 +96,17 @@ func (r *libraryRepository) Put(l *model.Library) error { "updated_at": time.Now(), } sq := Update(r.tableName).SetMap(cols).Where(Eq{"id": l.ID}) - _, err = r.executeSQL(sq) + rowsAffected, updateErr := r.executeSQL(sq) + if updateErr != nil { + return updateErr + } + + // If no rows were affected, the record doesn't exist, so insert it + if rowsAffected == 0 { + l.CreatedAt = time.Now() + l.UpdatedAt = time.Now() + err = r.db.Model(l).Insert() + } } if err != nil { return err diff --git a/persistence/library_repository_test.go b/persistence/library_repository_test.go index e49716ce3..6f4df1beb 100644 --- a/persistence/library_repository_test.go +++ b/persistence/library_repository_test.go @@ -22,6 +22,96 @@ var _ = Describe("LibraryRepository", func() { repo = NewLibraryRepository(ctx, conn) }) + AfterEach(func() { + // Clean up test libraries (keep ID 1 which is the default library) + _, _ = conn.NewQuery("DELETE FROM library WHERE id > 1").Execute() + }) + + Describe("Put", func() { + Context("when ID is 0", func() { + It("inserts a new library with autoassigned ID", func() { + lib := &model.Library{ + ID: 0, + Name: "Test Library", + Path: "/music/test", + } + + err := repo.Put(lib) + Expect(err).ToNot(HaveOccurred()) + Expect(lib.ID).To(BeNumerically(">", 0)) + Expect(lib.CreatedAt).ToNot(BeZero()) + Expect(lib.UpdatedAt).ToNot(BeZero()) + + // Verify it was inserted + savedLib, err := repo.Get(lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(savedLib.Name).To(Equal("Test Library")) + Expect(savedLib.Path).To(Equal("/music/test")) + }) + }) + + Context("when ID is non-zero and record exists", func() { + It("updates the existing record", func() { + // First create a library + lib := &model.Library{ + ID: 0, + Name: "Original Library", + Path: "/music/original", + } + err := repo.Put(lib) + Expect(err).ToNot(HaveOccurred()) + + originalID := lib.ID + originalCreatedAt := lib.CreatedAt + + // Now update it + lib.Name = "Updated Library" + lib.Path = "/music/updated" + err = repo.Put(lib) + Expect(err).ToNot(HaveOccurred()) + + // Verify it was updated, not inserted + Expect(lib.ID).To(Equal(originalID)) + Expect(lib.CreatedAt).To(Equal(originalCreatedAt)) + Expect(lib.UpdatedAt).To(BeTemporally(">", originalCreatedAt)) + + // Verify the changes were saved + savedLib, err := repo.Get(lib.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(savedLib.Name).To(Equal("Updated Library")) + Expect(savedLib.Path).To(Equal("/music/updated")) + }) + }) + + Context("when ID is non-zero but record doesn't exist", func() { + It("inserts a new record with the specified ID", func() { + lib := &model.Library{ + ID: 999, + Name: "New Library with ID", + Path: "/music/new", + } + + // Ensure the record doesn't exist + _, err := repo.Get(999) + Expect(err).To(HaveOccurred()) + + // Put should insert it + err = repo.Put(lib) + Expect(err).ToNot(HaveOccurred()) + Expect(lib.ID).To(Equal(999)) + Expect(lib.CreatedAt).ToNot(BeZero()) + Expect(lib.UpdatedAt).ToNot(BeZero()) + + // Verify it was inserted with the correct ID + savedLib, err := repo.Get(999) + Expect(err).ToNot(HaveOccurred()) + Expect(savedLib.ID).To(Equal(999)) + Expect(savedLib.Name).To(Equal("New Library with ID")) + Expect(savedLib.Path).To(Equal("/music/new")) + }) + }) + }) + It("refreshes stats", func() { libBefore, err := repo.Get(1) Expect(err).ToNot(HaveOccurred()) @@ -31,7 +121,8 @@ var _ = Describe("LibraryRepository", func() { Expect(libAfter.UpdatedAt).To(BeTemporally(">", libBefore.UpdatedAt)) var songsRes, albumsRes, artistsRes, foldersRes, filesRes, missingRes struct{ Count int64 } - var sizeRes, durationRes struct{ Sum int64 } + var sizeRes struct{ Sum int64 } + var durationRes struct{ Sum float64 } Expect(conn.NewQuery("select count(*) as count from media_file where library_id = {:id} and missing = 0").Bind(dbx.Params{"id": 1}).One(&songsRes)).To(Succeed()) Expect(conn.NewQuery("select count(*) as count from album where library_id = {:id} and missing = 0").Bind(dbx.Params{"id": 1}).One(&albumsRes)).To(Succeed()) @@ -49,6 +140,6 @@ var _ = Describe("LibraryRepository", func() { Expect(libAfter.TotalFiles).To(Equal(int(filesRes.Count))) Expect(libAfter.TotalMissingFiles).To(Equal(int(missingRes.Count))) Expect(libAfter.TotalSize).To(Equal(sizeRes.Sum)) - Expect(libAfter.TotalDuration).To(Equal(int(durationRes.Sum))) + Expect(libAfter.TotalDuration).To(Equal(durationRes.Sum)) }) }) diff --git a/persistence/user_repository_test.go b/persistence/user_repository_test.go index b768930e5..3b453b8f2 100644 --- a/persistence/user_repository_test.go +++ b/persistence/user_repository_test.go @@ -254,8 +254,8 @@ var _ = Describe("UserRepository", func() { Expect(repo.Put(&testUser)).To(BeNil()) userID = testUser.ID - library1 = model.Library{ID: 500, Name: "Library 500", Path: "/path/500"} - library2 = model.Library{ID: 501, Name: "Library 501", Path: "/path/501"} + library1 = model.Library{ID: 0, Name: "Library 500", Path: "/path/500"} + library2 = model.Library{ID: 0, Name: "Library 501", Path: "/path/501"} // Create test libraries libRepo := NewLibraryRepository(log.NewContext(context.TODO()), GetDBXBuilder()) @@ -269,7 +269,7 @@ var _ = Describe("UserRepository", func() { // Clean up test libraries to ensure isolation between test groups libRepo := NewLibraryRepository(log.NewContext(context.TODO()), GetDBXBuilder()) - _ = libRepo.(*libraryRepository).delete(squirrel.Eq{"id": []int{500, 501}}) + _ = libRepo.(*libraryRepository).delete(squirrel.Eq{"id": []int{library1.ID, library2.ID}}) }) Describe("AddUserLibrary", func() { @@ -396,8 +396,8 @@ var _ = Describe("UserRepository", func() { BeforeEach(func() { libRepo = NewLibraryRepository(log.NewContext(context.TODO()), GetDBXBuilder()) - library1 = model.Library{ID: 201, Name: "Admin Test Library 1", Path: "/admin/test/path1"} - library2 = model.Library{ID: 202, Name: "Admin Test Library 2", Path: "/admin/test/path2"} + library1 = model.Library{ID: 0, Name: "Admin Test Library 1", Path: "/admin/test/path1"} + library2 = model.Library{ID: 0, Name: "Admin Test Library 2", Path: "/admin/test/path2"} // Create test libraries Expect(libRepo.Put(&library1)).To(BeNil()) @@ -406,10 +406,10 @@ var _ = Describe("UserRepository", func() { AfterEach(func() { // Clean up test libraries and their associations - _ = libRepo.(*libraryRepository).delete(squirrel.Eq{"id": []int{201, 202}}) + _ = libRepo.(*libraryRepository).delete(squirrel.Eq{"id": []int{library1.ID, library2.ID}}) // Clean up user-library associations for these test libraries - _, _ = repo.(*userRepository).executeSQL(squirrel.Delete("user_library").Where(squirrel.Eq{"library_id": []int{201, 202}})) + _, _ = repo.(*userRepository).executeSQL(squirrel.Delete("user_library").Where(squirrel.Eq{"library_id": []int{library1.ID, library2.ID}})) }) It("automatically assigns all libraries to admin users when created", func() { @@ -503,8 +503,8 @@ var _ = Describe("UserRepository", func() { BeforeEach(func() { libRepo = NewLibraryRepository(log.NewContext(context.TODO()), GetDBXBuilder()) - library1 = model.Library{ID: 301, Name: "Field Test Library 1", Path: "/field/test/path1"} - library2 = model.Library{ID: 302, Name: "Field Test Library 2", Path: "/field/test/path2"} + library1 = model.Library{ID: 0, Name: "Field Test Library 1", Path: "/field/test/path1"} + library2 = model.Library{ID: 0, Name: "Field Test Library 2", Path: "/field/test/path2"} // Create test libraries Expect(libRepo.Put(&library1)).To(BeNil()) @@ -527,11 +527,11 @@ var _ = Describe("UserRepository", func() { AfterEach(func() { // Clean up test libraries and their associations - _ = libRepo.(*libraryRepository).delete(squirrel.Eq{"id": []int{301, 302}}) + _ = libRepo.(*libraryRepository).delete(squirrel.Eq{"id": []int{library1.ID, library2.ID}}) _ = repo.(*userRepository).delete(squirrel.Eq{"id": testUser.ID}) // Clean up user-library associations for these test libraries - _, _ = repo.(*userRepository).executeSQL(squirrel.Delete("user_library").Where(squirrel.Eq{"library_id": []int{301, 302}})) + _, _ = repo.(*userRepository).executeSQL(squirrel.Delete("user_library").Where(squirrel.Eq{"library_id": []int{library1.ID, library2.ID}})) }) It("populates Libraries field when getting a single user", func() { diff --git a/scanner/controller_test.go b/scanner/controller_test.go index 4f6576a39..e551e15b1 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -9,7 +9,6 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/db" - "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/persistence" "github.com/navidrome/navidrome/scanner" "github.com/navidrome/navidrome/server/events" @@ -32,7 +31,6 @@ var _ = Describe("Controller", func() { ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())} ds.MockedProperty = &tests.MockedPropertyRepo{} ctrl = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), core.NewPlaylists(ds), metrics.NewNoopInstance()) - Expect(ds.Library(ctx).Put(&model.Library{ID: 1, Name: "lib", Path: "/tmp"})).To(Succeed()) }) It("includes last scan error", func() {