feat(library): enhance validation messages and error handling for library paths

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

core/library.go

@@ -173,6 +173,23 @@ func (r *libraryRepositoryWrapper) Delete(id string) error {
 // Helper methods
 
 func (r *libraryRepositoryWrapper) mapError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	errStr := err.Error()
+
+	// Handle database constraint violations.
+	// TODO: Being tied to react-admin translations is not ideal, but this will probably go away with the new UI/API
+	if strings.Contains(errStr, "UNIQUE constraint failed") {
+		if strings.Contains(errStr, "library.name") {
+			return &rest.ValidationError{Errors: map[string]string{"name": "ra.validation.unique"}}
+		}
+		if strings.Contains(errStr, "library.path") {
+			return &rest.ValidationError{Errors: map[string]string{"path": "ra.validation.unique"}}
+		}
+	}
+
 	switch {
 	case errors.Is(err, model.ErrNotFound):
 		return rest.ErrNotFound
@@ -187,25 +204,13 @@ func (r *libraryRepositoryWrapper) validateLibrary(library *model.Library) error
 	validationErrors := make(map[string]string)
 
 	if library.Name == "" {
-		validationErrors["name"] = "library name is required"
-	} else {
-		// Check for name uniqueness
-		if err := r.validateLibraryNameUnique(library); err != nil {
-			var validationErr *rest.ValidationError
-			if errors.As(err, &validationErr) {
-				for k, v := range validationErr.Errors {
-					validationErrors[k] = v
-				}
-			} else {
-				// For non-validation errors (like database errors), return them immediately
-				return err
-			}
-		}
+		validationErrors["name"] = "ra.validation.required"
 	}
 
 	if library.Path == "" {
-		validationErrors["path"] = "library path is required"
+		validationErrors["path"] = "ra.validation.required"
 	} else {
+		// Validate path format and accessibility
 		if err := r.validateLibraryPath(library); err != nil {
 			validationErrors["path"] = err.Error()
 		}
@@ -218,23 +223,6 @@ func (r *libraryRepositoryWrapper) validateLibrary(library *model.Library) error
 	return nil
 }
 
-func (r *libraryRepositoryWrapper) validateLibraryNameUnique(library *model.Library) error {
-	// Get all libraries to check for name conflicts
-	allLibraries, err := r.LibraryRepository.GetAll()
-	if err != nil {
-		return err
-	}
-
-	// Check if any other library has the same name
-	for _, existingLib := range allLibraries {
-		if existingLib.Name == library.Name && existingLib.ID != library.ID {
-			return &rest.ValidationError{Errors: map[string]string{"name": "ra.validation.unique"}}
-		}
-	}
-
-	return nil
-}
-
 func (r *libraryRepositoryWrapper) validateLibraryPath(library *model.Library) error {
 	// Validate path format
 	if !filepath.IsAbs(library.Path) {
@@ -253,38 +241,30 @@ func (r *libraryRepositoryWrapper) validateLibraryPath(library *model.Library) e
 
 	fsys, err := fileStore.FS()
 	if err != nil {
-		// Parse the error to provide user-friendly messages
-		errStr := err.Error()
-		if os.IsNotExist(err) ||
-			strings.Contains(errStr, "no such file or directory") ||
-			strings.Contains(errStr, "The system cannot find the path specified.") {
-			return fmt.Errorf("library path does not exist")
-		} else if os.IsPermission(err) {
-			return fmt.Errorf("library path is not accessible")
-		} else {
-			return fmt.Errorf("error accessing library storage: %w", err)
-		}
+		log.Warn(r.ctx, "Error validating library.path", "path", library.Path, err)
+		return fmt.Errorf("resources.library.validation.pathInvalid")
 	}
 
 	// Check if root directory exists
 	info, err := fs.Stat(fsys, ".")
 	if err != nil {
 		// Parse the error message to check for "not a directory"
+		log.Warn(r.ctx, "Error stating library.path", "path", library.Path, err)
 		errStr := err.Error()
 		if strings.Contains(errStr, "not a directory") ||
 			strings.Contains(errStr, "The directory name is invalid.") {
-			return fmt.Errorf("library path must be a directory")
+			return fmt.Errorf("resources.library.validation.pathNotDirectory")
 		} else if os.IsNotExist(err) {
-			return fmt.Errorf("library path does not exist")
+			return fmt.Errorf("resources.library.validation.pathNotFound")
 		} else if os.IsPermission(err) {
-			return fmt.Errorf("library path is not accessible")
+			return fmt.Errorf("resources.library.validation.pathNotAccessible")
 		} else {
-			return fmt.Errorf("error accessing library path: %w", err)
+			return fmt.Errorf("resources.library.validation.pathInvalid")
 		}
 	}
 
 	if !info.IsDir() {
-		return fmt.Errorf("library path must be a directory")
+		return fmt.Errorf("resources.library.validation.pathNotDirectory")
 	}
 
 	return nil

core/library_test.go

@@ -73,7 +73,7 @@ var _ = Describe("Library Service", func() {
 				_, err := repo.Save(library)
 
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(ContainSubstring("library name is required"))
+				Expect(err.Error()).To(ContainSubstring("ra.validation.required"))
 			})
 
 			It("fails when library path is empty", func() {
@@ -82,33 +82,71 @@ var _ = Describe("Library Service", func() {
 				_, err := repo.Save(library)
 
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(ContainSubstring("library path is required"))
+				Expect(err.Error()).To(ContainSubstring("ra.validation.required"))
 			})
 
-			It("fails when library name already exists", func() {
-				// First, create a library
-				existingLibrary := &model.Library{ID: 1, Name: "Existing Library", Path: tempDir}
-				libraryRepo.SetData(model.Libraries{*existingLibrary})
+			It("fails when library path is not absolute", func() {
+				library := &model.Library{Name: "Test", Path: "relative/path"}
 
-				// Try to create another library with the same name
-				newLibrary := &model.Library{ID: 2, Name: "Existing Library", Path: tempDir + "/other"}
-
-				_, err := repo.Save(newLibrary)
+				_, err := repo.Save(library)
 
 				Expect(err).To(HaveOccurred())
 				var validationErr *rest.ValidationError
 				Expect(errors.As(err, &validationErr)).To(BeTrue())
-				Expect(validationErr.Errors["name"]).To(Equal("ra.validation.unique"))
+				Expect(validationErr.Errors["path"]).To(Equal("library path must be absolute"))
 			})
 
-			It("cleans and normalizes the path", func() {
-				unnormalizedPath := tempDir + "//../" + filepath.Base(tempDir)
-				library := &model.Library{ID: 1, Name: "Test Library", Path: unnormalizedPath}
+			Context("Database constraint violations", func() {
+				BeforeEach(func() {
+					// Set up an existing library that will cause constraint violations
+					libraryRepo.SetData(model.Libraries{
+						{ID: 1, Name: "Existing Library", Path: tempDir},
+					})
+				})
 
-				_, err := repo.Save(library)
+				AfterEach(func() {
+					// Reset custom PutFn after each test
+					libraryRepo.PutFn = nil
+				})
 
-				Expect(err).NotTo(HaveOccurred())
-				Expect(libraryRepo.Data[1].Path).To(Equal(filepath.Clean(unnormalizedPath)))
+				It("handles name uniqueness constraint violation from database", func() {
+					// Create the directory that will be used for the test
+					otherTempDir, err := os.MkdirTemp("", "navidrome-other-")
+					Expect(err).NotTo(HaveOccurred())
+					DeferCleanup(func() { os.RemoveAll(otherTempDir) })
+
+					// Try to create another library with the same name
+					library := &model.Library{ID: 2, Name: "Existing Library", Path: otherTempDir}
+
+					// Mock the repository to return a UNIQUE constraint error
+					libraryRepo.PutFn = func(library *model.Library) error {
+						return errors.New("UNIQUE constraint failed: library.name")
+					}
+
+					_, err = repo.Save(library)
+
+					Expect(err).To(HaveOccurred())
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.Errors["name"]).To(Equal("ra.validation.unique"))
+				})
+
+				It("handles path uniqueness constraint violation from database", func() {
+					// Try to create another library with the same path
+					library := &model.Library{ID: 2, Name: "Different Library", Path: tempDir}
+
+					// Mock the repository to return a UNIQUE constraint error
+					libraryRepo.PutFn = func(library *model.Library) error {
+						return errors.New("UNIQUE constraint failed: library.path")
+					}
+
+					_, err := repo.Save(library)
+
+					Expect(err).To(HaveOccurred())
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.Errors["path"]).To(Equal("ra.validation.unique"))
+				})
 			})
 		})
 
@@ -134,9 +172,14 @@ var _ = Describe("Library Service", func() {
 			})
 
 			It("fails when library doesn't exist", func() {
-				library := &model.Library{ID: 999, Name: "Non-existent", Path: tempDir}
+				// Create a unique temporary directory to avoid path conflicts
+				uniqueTempDir, err := os.MkdirTemp("", "navidrome-nonexistent-")
+				Expect(err).NotTo(HaveOccurred())
+				DeferCleanup(func() { os.RemoveAll(uniqueTempDir) })
 
-				err := repo.Update("999", library)
+				library := &model.Library{ID: 999, Name: "Non-existent", Path: uniqueTempDir}
+
+				err = repo.Update("999", library)
 
 				Expect(err).To(HaveOccurred())
 				Expect(err).To(Equal(model.ErrNotFound))
@@ -148,7 +191,7 @@ var _ = Describe("Library Service", func() {
 				err := repo.Update("1", library)
 
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(ContainSubstring("library name is required"))
+				Expect(err.Error()).To(ContainSubstring("ra.validation.required"))
 			})
 
 			It("cleans and normalizes the path on update", func() {
@@ -160,6 +203,97 @@ var _ = Describe("Library Service", func() {
 				Expect(err).NotTo(HaveOccurred())
 				Expect(libraryRepo.Data[1].Path).To(Equal(filepath.Clean(unnormalizedPath)))
 			})
+
+			It("allows updating library with same name (no change)", func() {
+				// Set up a library
+				libraryRepo.SetData(model.Libraries{
+					{ID: 1, Name: "Test Library", Path: tempDir},
+				})
+
+				// Update the library keeping the same name (should be allowed)
+				library := &model.Library{ID: 1, Name: "Test Library", Path: tempDir}
+
+				err := repo.Update("1", library)
+
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("allows updating library with same path (no change)", func() {
+				// Set up a library
+				libraryRepo.SetData(model.Libraries{
+					{ID: 1, Name: "Test Library", Path: tempDir},
+				})
+
+				// Update the library keeping the same path (should be allowed)
+				library := &model.Library{ID: 1, Name: "Test Library", Path: tempDir}
+
+				err := repo.Update("1", library)
+
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			Context("Database constraint violations during update", func() {
+				BeforeEach(func() {
+					// Reset any custom PutFn from previous tests
+					libraryRepo.PutFn = nil
+				})
+
+				It("handles name uniqueness constraint violation during update", func() {
+					// Create additional temp directory for the test
+					otherTempDir, err := os.MkdirTemp("", "navidrome-other-")
+					Expect(err).NotTo(HaveOccurred())
+					DeferCleanup(func() { os.RemoveAll(otherTempDir) })
+
+					// Set up two libraries
+					libraryRepo.SetData(model.Libraries{
+						{ID: 1, Name: "Library One", Path: tempDir},
+						{ID: 2, Name: "Library Two", Path: otherTempDir},
+					})
+
+					// Mock database constraint violation
+					libraryRepo.PutFn = func(library *model.Library) error {
+						return errors.New("UNIQUE constraint failed: library.name")
+					}
+
+					// Try to update library 2 to have the same name as library 1
+					library := &model.Library{ID: 2, Name: "Library One", Path: otherTempDir}
+
+					err = repo.Update("2", library)
+
+					Expect(err).To(HaveOccurred())
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.Errors["name"]).To(Equal("ra.validation.unique"))
+				})
+
+				It("handles path uniqueness constraint violation during update", func() {
+					// Create additional temp directory for the test
+					otherTempDir, err := os.MkdirTemp("", "navidrome-other-")
+					Expect(err).NotTo(HaveOccurred())
+					DeferCleanup(func() { os.RemoveAll(otherTempDir) })
+
+					// Set up two libraries
+					libraryRepo.SetData(model.Libraries{
+						{ID: 1, Name: "Library One", Path: tempDir},
+						{ID: 2, Name: "Library Two", Path: otherTempDir},
+					})
+
+					// Mock database constraint violation
+					libraryRepo.PutFn = func(library *model.Library) error {
+						return errors.New("UNIQUE constraint failed: library.path")
+					}
+
+					// Try to update library 2 to have the same path as library 1
+					library := &model.Library{ID: 2, Name: "Library Two", Path: tempDir}
+
+					err = repo.Update("2", library)
+
+					Expect(err).To(HaveOccurred())
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.Errors["path"]).To(Equal("ra.validation.unique"))
+				})
+			})
 		})
 
 		Describe("Path Validation", func() {
@@ -184,7 +318,7 @@ var _ = Describe("Library Service", func() {
 					Expect(err).To(HaveOccurred())
 					var validationErr *rest.ValidationError
 					Expect(errors.As(err, &validationErr)).To(BeTrue())
-					Expect(validationErr.Errors["path"]).To(Equal("library path does not exist"))
+					Expect(validationErr.Errors["path"]).To(Equal("resources.library.validation.pathInvalid"))
 				})
 
 				It("fails when path is a file instead of directory", func() {
@@ -199,7 +333,7 @@ var _ = Describe("Library Service", func() {
 					Expect(err).To(HaveOccurred())
 					var validationErr *rest.ValidationError
 					Expect(errors.As(err, &validationErr)).To(BeTrue())
-					Expect(validationErr.Errors["path"]).To(Equal("library path must be a directory"))
+					Expect(validationErr.Errors["path"]).To(Equal("resources.library.validation.pathNotDirectory"))
 				})
 
 				It("fails when path is not accessible due to permissions", func() {
@@ -218,7 +352,7 @@ var _ = Describe("Library Service", func() {
 					Expect(errors.As(err, &validationErr)).To(BeTrue())
 					Expect(validationErr.Errors).To(HaveKey("name"))
 					Expect(validationErr.Errors).To(HaveKey("path"))
-					Expect(validationErr.Errors["name"]).To(Equal("library name is required"))
+					Expect(validationErr.Errors["name"]).To(Equal("ra.validation.required"))
 					Expect(validationErr.Errors["path"]).To(Equal("library path must be absolute"))
 				})
 			})
@@ -241,24 +375,6 @@ var _ = Describe("Library Service", func() {
 					Expect(validationErr.Errors["path"]).To(Equal("library path must be absolute"))
 				})
 
-				It("fails when updated name conflicts with existing library", func() {
-					// Set up two libraries
-					libraryRepo.SetData(model.Libraries{
-						{ID: 1, Name: "Library One", Path: tempDir},
-						{ID: 2, Name: "Library Two", Path: tempDir + "/other"},
-					})
-
-					// Try to update library 2 to have the same name as library 1
-					library := &model.Library{ID: 2, Name: "Library One", Path: tempDir + "/other"}
-
-					err := repo.Update("2", library)
-
-					Expect(err).To(HaveOccurred())
-					var validationErr *rest.ValidationError
-					Expect(errors.As(err, &validationErr)).To(BeTrue())
-					Expect(validationErr.Errors["name"]).To(Equal("ra.validation.unique"))
-				})
-
 				It("allows updating library with same name (no change)", func() {
 					// Set up a library
 					libraryRepo.SetData(model.Libraries{
@@ -282,7 +398,7 @@ var _ = Describe("Library Service", func() {
 					Expect(err).To(HaveOccurred())
 					var validationErr *rest.ValidationError
 					Expect(errors.As(err, &validationErr)).To(BeTrue())
-					Expect(validationErr.Errors["path"]).To(Equal("library path does not exist"))
+					Expect(validationErr.Errors["path"]).To(Equal("resources.library.validation.pathInvalid"))
 				})
 
 				It("fails when updated path is a file instead of directory", func() {
@@ -297,27 +413,21 @@ var _ = Describe("Library Service", func() {
 					Expect(err).To(HaveOccurred())
 					var validationErr *rest.ValidationError
 					Expect(errors.As(err, &validationErr)).To(BeTrue())
-					Expect(validationErr.Errors["path"]).To(Equal("library path must be a directory"))
+					Expect(validationErr.Errors["path"]).To(Equal("resources.library.validation.pathNotDirectory"))
 				})
 
 				It("handles multiple validation errors on update", func() {
-					// Set up two libraries to test name conflict
-					libraryRepo.SetData(model.Libraries{
-						{ID: 1, Name: "Library One", Path: tempDir},
-						{ID: 2, Name: "Library Two", Path: tempDir + "/other"},
-					})
+					// Try to update with empty name and invalid path
+					library := &model.Library{ID: 1, Name: "", Path: "relative/path"}
 
-					// Try to update with conflicting name and invalid path
-					library := &model.Library{ID: 2, Name: "Library One", Path: "relative/path"}
-
-					err := repo.Update("2", library)
+					err := repo.Update("1", library)
 
 					Expect(err).To(HaveOccurred())
 					var validationErr *rest.ValidationError
 					Expect(errors.As(err, &validationErr)).To(BeTrue())
 					Expect(validationErr.Errors).To(HaveKey("name"))
 					Expect(validationErr.Errors).To(HaveKey("path"))
-					Expect(validationErr.Errors["name"]).To(Equal("ra.validation.unique"))
+					Expect(validationErr.Errors["name"]).To(Equal("ra.validation.required"))
 					Expect(validationErr.Errors["path"]).To(Equal("library path must be absolute"))
 				})
 			})

tests/mock_library_repo.go

@@ -12,8 +12,9 @@ import (
 
 type MockLibraryRepo struct {
 	model.LibraryRepository
-	Data map[int]model.Library
-	Err  error
+	Data  map[int]model.Library
+	Err   error
+	PutFn func(*model.Library) error // Allow custom Put behavior for testing
 }
 
 func (m *MockLibraryRepo) SetData(data model.Libraries) {
@@ -88,6 +89,9 @@ func (m *MockLibraryRepo) GetPath(id int) (string, error) {
 }
 
 func (m *MockLibraryRepo) Put(library *model.Library) error {
+	if m.PutFn != nil {
+		return m.PutFn(library)
+	}
 	if m.Err != nil {
 		return m.Err
 	}

ui/src/i18n/en.json

@@ -297,6 +297,9 @@
       "validation": {
         "nameRequired": "Library name is required",
         "pathRequired": "Library path is required",
+        "pathNotDirectory": "Library path must be a directory",
+        "pathNotFound": "Library path not found",
+        "pathNotAccessible": "Library path is not accessible",
         "pathInvalid": "Invalid library path"
       },
       "messages": {

ui/src/library/LibraryCreate.jsx

@@ -37,9 +37,32 @@ const LibraryCreate = (props) => {
         })
         redirect('/library')
       } catch (error) {
-        if (error.body.errors) {
+        // Handle validation errors with proper field mapping
+        if (error.body && error.body.errors) {
           return error.body.errors
         }
+        
+        // Handle other structured errors from the server
+        if (error.body && error.body.error) {
+          const errorMsg = error.body.error
+          
+          // Handle database constraint violations
+          if (errorMsg.includes('UNIQUE constraint failed: library.name')) {
+            return { name: 'ra.validation.unique' }
+          }
+          if (errorMsg.includes('UNIQUE constraint failed: library.path')) {
+            return { path: 'ra.validation.unique' }
+          }
+          
+          // Show a general notification for other server errors
+          notify(errorMsg, 'error')
+          return
+        }
+        
+        // Fallback for unexpected error formats
+        const fallbackMessage = error.message || 
+          (typeof error === 'string' ? error : 'An unexpected error occurred')
+        notify(fallbackMessage, 'error')
       }
     },
     [mutate, notify, redirect],