feat(library): improve library path validation and error handling - WIP

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

core/library.go

@@ -4,12 +4,15 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/Masterminds/squirrel"
 	"github.com/deluan/rest"
+	"github.com/navidrome/navidrome/core/storage"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/request"
@@ -158,7 +161,7 @@ func (r *libraryRepositoryWrapper) Update(id string, entity interface{}, cols ..
 func (r *libraryRepositoryWrapper) Delete(id string) error {
 	libID, err := strconv.Atoi(id)
 	if err != nil {
-		return rest.ValidationError{Errors: map[string]string{
+		return &rest.ValidationError{Errors: map[string]string{
 			"id": "invalid library ID format",
 		}}
 	}
@@ -181,41 +184,107 @@ func (r *libraryRepositoryWrapper) mapError(err error) error {
 }
 
 func (r *libraryRepositoryWrapper) validateLibrary(library *model.Library) error {
-	errors := make(map[string]string)
+	validationErrors := make(map[string]string)
 
 	if library.Name == "" {
-		errors["name"] = "library name is required"
-	}
-
-	if library.Path == "" {
-		errors["path"] = "library path is required"
+		validationErrors["name"] = "library name 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)
+		// 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 if !info.IsDir() {
-				errors["path"] = "library path must be a directory"
+			} else {
+				// For non-validation errors (like database errors), return them immediately
+				return err
 			}
 		}
 	}
 
-	if len(errors) > 0 {
-		return rest.ValidationError{Errors: errors}
+	if library.Path == "" {
+		validationErrors["path"] = "library path is required"
+	} else {
+		if err := r.validateLibraryPath(library); err != nil {
+			validationErrors["path"] = err.Error()
+		}
+	}
+
+	if len(validationErrors) > 0 {
+		return &rest.ValidationError{Errors: validationErrors}
+	}
+
+	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) {
+		return fmt.Errorf("library path must be absolute")
+	}
+
+	// Clean the path to normalize it
+	cleanPath := filepath.Clean(library.Path)
+	library.Path = cleanPath
+
+	// Check if path exists and is accessible using storage abstraction
+	fileStore, err := storage.For(library.Path)
+	if err != nil {
+		return fmt.Errorf("invalid storage scheme: %w", err)
+	}
+
+	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)
+		}
+	}
+
+	// Check if root directory exists
+	info, err := fs.Stat(fsys, ".")
+	if err != nil {
+		// Parse the error message to check for "not a directory"
+		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")
+		} else if os.IsNotExist(err) {
+			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 path: %w", err)
+		}
+	}
+
+	if !info.IsDir() {
+		return fmt.Errorf("library path must be a directory")
 	}
 
 	return nil

core/library_test.go

@@ -2,12 +2,15 @@ package core_test
 
 import (
 	"context"
+	"errors"
 	"os"
 	"path/filepath"
 
 	"github.com/deluan/rest"
+	_ "github.com/navidrome/navidrome/adapters/taglib" // Register taglib extractor
 	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/core"
+	_ "github.com/navidrome/navidrome/core/storage/local" // Register local storage
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/request"
 	"github.com/navidrome/navidrome/tests"
@@ -15,6 +18,7 @@ import (
 	. "github.com/onsi/gomega"
 )
 
+// These tests require the local storage adapter and the taglib extractor to be registered.
 var _ = Describe("Library Service", func() {
 	var service core.Library
 	var ds *tests.MockDataStore
@@ -81,6 +85,22 @@ var _ = Describe("Library Service", func() {
 				Expect(err.Error()).To(ContainSubstring("library path is 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})
+
+				// Try to create another library with the same name
+				newLibrary := &model.Library{ID: 2, Name: "Existing Library", Path: tempDir + "/other"}
+
+				_, err := repo.Save(newLibrary)
+
+				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("cleans and normalizes the path", func() {
 				unnormalizedPath := tempDir + "//../" + filepath.Base(tempDir)
 				library := &model.Library{ID: 1, Name: "Test Library", Path: unnormalizedPath}
@@ -150,9 +170,9 @@ var _ = Describe("Library Service", func() {
 					_, 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"))
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.Errors["path"]).To(Equal("library path must be absolute"))
 				})
 
 				It("fails when path does not exist", func() {
@@ -162,14 +182,14 @@ var _ = Describe("Library Service", func() {
 					_, 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"))
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.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)
+					err := os.WriteFile(testFile, []byte("test"), 0600)
 					Expect(err).NotTo(HaveOccurred())
 
 					library := &model.Library{Name: "Test", Path: testFile}
@@ -177,9 +197,9 @@ var _ = Describe("Library Service", func() {
 					_, 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"))
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.Errors["path"]).To(Equal("library path must be a directory"))
 				})
 
 				It("fails when path is not accessible due to permissions", func() {
@@ -194,12 +214,12 @@ var _ = Describe("Library Service", func() {
 					_, 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"))
+					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("library name is required"))
+					Expect(validationErr.Errors["path"]).To(Equal("library path must be absolute"))
 				})
 			})
 
@@ -216,9 +236,41 @@ var _ = Describe("Library Service", func() {
 					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"))
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					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{
+						{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("fails when updated path does not exist", func() {
@@ -228,14 +280,14 @@ var _ = Describe("Library Service", func() {
 					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"))
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.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)
+					err := os.WriteFile(testFile, []byte("test"), 0600)
 					Expect(err).NotTo(HaveOccurred())
 
 					library := &model.Library{ID: 1, Name: "Test", Path: testFile}
@@ -243,23 +295,30 @@ var _ = Describe("Library Service", func() {
 					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"))
+					var validationErr *rest.ValidationError
+					Expect(errors.As(err, &validationErr)).To(BeTrue())
+					Expect(validationErr.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"}
+					// 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"},
+					})
 
-					err := repo.Update("1", library)
+					// 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)
 
 					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"))
+					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["path"]).To(Equal("library path must be absolute"))
 				})
 			})
 		})

scanner/scanner_test.go

@@ -58,12 +58,14 @@ var _ = Describe("Scanner", Ordered, func() {
 	})
 
 	BeforeEach(func() {
+		DeferCleanup(configtest.SetupConfig())
+		conf.Server.MusicFolder = "fake:///music" // Set to match test library path
+		conf.Server.DevExternalScanner = false
+
 		db.Init(ctx)
 		DeferCleanup(func() {
 			Expect(tests.ClearDB()).To(Succeed())
 		})
-		DeferCleanup(configtest.SetupConfig())
-		conf.Server.DevExternalScanner = false
 
 		ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())}
 		mfRepo = &mockMediaFileRepo{

ui/src/library/LibraryEdit.jsx

@@ -1,4 +1,4 @@
-import React from 'react'
+import React, { useCallback } from 'react'
 import {
   Edit,
   SimpleForm,
@@ -10,6 +10,9 @@ import {
   DateField,
   FunctionField,
   useTranslate,
+  useMutation,
+  useNotify,
+  useRedirect,
 } from 'react-admin'
 import { makeStyles } from '@material-ui/core/styles'
 import { Divider, Typography } from '@material-ui/core'
@@ -83,11 +86,38 @@ const formatBytes = (bytes, decimals = 2) => {
 const LibraryEdit = (props) => {
   const classes = useStyles()
   const translate = useTranslate()
+  const [mutate] = useMutation()
+  const notify = useNotify()
+  const redirect = useRedirect()
   const isFirstLibrary = props.id === '1'
 
+  const save = useCallback(
+    async (values) => {
+      try {
+        await mutate(
+          {
+            type: 'update',
+            resource: 'library',
+            payload: { id: props.id, data: values },
+          },
+          { returnPromise: true },
+        )
+        notify('resources.library.notifications.updated', 'info', {
+          smart_count: 1,
+        })
+        redirect('/library')
+      } catch (error) {
+        if (error.body && error.body.errors) {
+          return error.body.errors
+        }
+      }
+    },
+    [mutate, notify, redirect, props.id],
+  )
+
   return (
     <Edit title={<LibraryTitle />} undoable={false} {...props}>
-      <SimpleForm variant={'outlined'} toolbar={<LibraryToolbar />}>
+      <SimpleForm variant={'outlined'} toolbar={<LibraryToolbar />} save={save}>
         <TextInput source="name" validate={[required()]} />
         <TextInput source="path" validate={[required()]} fullWidth disabled={isFirstLibrary} />
 
@@ -175,3 +205,4 @@ const LibraryEdit = (props) => {
 }
 
 export default LibraryEdit
+ 

ui/src/library/index.js

@@ -1,7 +1,7 @@
 import LibraryList from './LibraryList'
 import LibraryEdit from './LibraryEdit'
 import LibraryCreate from './LibraryCreate'
-
+ 
 export default {
   list: LibraryList,
   edit: LibraryEdit,