feat(library): enhance library validation and management with path checks and normalization - WIP

Signed-off-by: Deluan <deluan@navidrome.org>

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

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},
 				})
 			})
 

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

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))
 	})
 })

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() {

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() {