diff --git a/core/storage/local/local_test.go b/core/storage/local/local_test.go index aef89cdd5..d65d8214a 100644 --- a/core/storage/local/local_test.go +++ b/core/storage/local/local_test.go @@ -13,7 +13,6 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/model/metadata" - "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -45,16 +44,13 @@ var _ = Describe("LocalStorage", func() { }) Describe("newLocalStorage", func() { - BeforeEach(func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-storage-local)") - }) Context("with valid path", func() { It("should create a localStorage instance with correct path", func() { - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) localStorage := storage.(*localStorage) Expect(localStorage.u.Scheme).To(Equal("file")) @@ -94,10 +90,10 @@ var _ = Describe("LocalStorage", func() { err = os.Symlink(realDir, linkDir) Expect(err).ToNot(HaveOccurred()) - u, err := url.Parse("file://" + linkDir) + u, err := storage.LocalPathToURL(linkDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) localStorage, ok := storage.(*localStorage) Expect(ok).To(BeTrue()) @@ -110,10 +106,10 @@ var _ = Describe("LocalStorage", func() { // Use a non-existent path to trigger symlink resolution failure nonExistentPath := filepath.Join(tempDir, "non-existent") - u, err := url.Parse("file://" + nonExistentPath) + u, err := storage.LocalPathToURL(nonExistentPath) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) localStorage, ok := storage.(*localStorage) Expect(ok).To(BeTrue()) @@ -137,7 +133,9 @@ var _ = Describe("LocalStorage", func() { localStorage, ok := storage.(*localStorage) Expect(ok).To(BeTrue()) - Expect(localStorage.u.Path).To(Equal("C:/music")) + // newLocalStorage re-joins the drive letter (u.Host) with u.Path via + // filepath.Join, which yields an OS-native (backslash) path on Windows. + Expect(localStorage.u.Path).To(Equal(filepath.Join("C:", "/music"))) }) }) @@ -159,10 +157,10 @@ var _ = Describe("LocalStorage", func() { It("falls back to the default extractor instead of crashing", func() { conf.Server.Scanner.Extractor = "nonexistent-extractor" - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) ls, ok := storage.(*localStorage) Expect(ok).To(BeTrue()) Expect(ls.extractor).To(BeIdenticalTo(defaultExtractor)) @@ -171,16 +169,13 @@ var _ = Describe("LocalStorage", func() { }) Describe("localStorage.FS", func() { - BeforeEach(func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-storage-local)") - }) Context("with existing directory", func() { It("should return a localFS instance", func() { - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) musicFS, err := storage.FS() Expect(err).ToNot(HaveOccurred()) Expect(musicFS).ToNot(BeNil()) @@ -193,10 +188,10 @@ var _ = Describe("LocalStorage", func() { Context("with non-existent directory", func() { It("should return an error", func() { nonExistentPath := filepath.Join(tempDir, "non-existent") - u, err := url.Parse("file://" + nonExistentPath) + u, err := storage.LocalPathToURL(nonExistentPath) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) _, err = storage.FS() Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring(nonExistentPath)) @@ -208,7 +203,6 @@ var _ = Describe("LocalStorage", func() { var testFile string BeforeEach(func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-storage-local)") // Create a test file testFile = filepath.Join(tempDir, "test.mp3") err := os.WriteFile(testFile, []byte("test data"), 0600) @@ -235,9 +229,9 @@ var _ = Describe("LocalStorage", func() { testExtractor.results["test.mp3"] = expectedInfo - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) musicFS, err := storage.FS() Expect(err).ToNot(HaveOccurred()) @@ -259,9 +253,9 @@ var _ = Describe("LocalStorage", func() { testExtractor.results["test.mp3"] = incompleteInfo - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) musicFS, err := storage.FS() Expect(err).ToNot(HaveOccurred()) @@ -288,9 +282,9 @@ var _ = Describe("LocalStorage", func() { testExtractor.results["non-existent.mp3"] = incompleteInfo - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) musicFS, err := storage.FS() Expect(err).ToNot(HaveOccurred()) @@ -303,9 +297,9 @@ var _ = Describe("LocalStorage", func() { It("should return the extractor error", func() { testExtractor.err = &extractorError{message: "extractor failed"} - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) musicFS, err := storage.FS() Expect(err).ToNot(HaveOccurred()) @@ -334,9 +328,9 @@ var _ = Describe("LocalStorage", func() { testExtractor.results["test.mp3"] = info1 testExtractor.results["test2.mp3"] = info2 - u, err := url.Parse("file://" + tempDir) + u, err := storage.LocalPathToURL(tempDir) Expect(err).ToNot(HaveOccurred()) - storage := newLocalStorage(*u) + storage := newLocalStorage(u) musicFS, err := storage.FS() Expect(err).ToNot(HaveOccurred()) @@ -390,9 +384,8 @@ var _ = Describe("LocalStorage", func() { Describe("Storage registration", func() { It("should register localStorage for file scheme", func() { - tests.SkipOnWindows("path separator bug (#TBD-path-sep-storage-local)") // This tests the init() function indirectly - storage, err := storage.For("file://" + tempDir) + storage, err := storage.For(tempDir) Expect(err).ToNot(HaveOccurred()) Expect(storage).To(BeAssignableToTypeOf(&localStorage{})) }) diff --git a/core/storage/storage.go b/core/storage/storage.go index b9fceb1fd..242965c99 100644 --- a/core/storage/storage.go +++ b/core/storage/storage.go @@ -25,6 +25,27 @@ func Register(schema string, c constructor) { registry[schema] = c } +// LocalPathToURL converts a bare OS filesystem path into an absolute file:// URL, +// applying the same slash-normalisation and per-component escaping the scanner +// relies on. It is the single source of truth for how a local path becomes a +// storage URL, shared by For and by storage tests. +func LocalPathToURL(osPath string) (url.URL, error) { + abs, _ := filepath.Abs(osPath) + abs = filepath.ToSlash(abs) + + // Properly escape each path component using URL standards + pathParts := strings.Split(abs, "/") + escapedParts := slice.Map(pathParts, func(s string) string { + return url.PathEscape(s) + }) + + u, err := url.Parse(LocalSchemaID + "://" + strings.Join(escapedParts, "/")) + if err != nil { + return url.URL{}, err + } + return *u, nil +} + // For returns a Storage implementation for the given URI. // It uses the schema part of the URI to find the correct registered // Storage constructor. @@ -34,24 +55,22 @@ func For(uri string) (Storage, error) { defer lock.RUnlock() parts := strings.Split(uri, "://") + var u *url.URL // Paths without schema are treated as file:// and use the default LocalStorage implementation if len(parts) < 2 { - uri, _ = filepath.Abs(uri) - uri = filepath.ToSlash(uri) - - // Properly escape each path component using URL standards - pathParts := strings.Split(uri, "/") - escapedParts := slice.Map(pathParts, func(s string) string { - return url.PathEscape(s) - }) - - uri = LocalSchemaID + "://" + strings.Join(escapedParts, "/") + parsed, err := LocalPathToURL(uri) + if err != nil { + return nil, err + } + u = &parsed + } else { + parsed, err := url.Parse(uri) + if err != nil { + return nil, err + } + u = parsed } - u, err := url.Parse(uri) - if err != nil { - return nil, err - } c, ok := registry[u.Scheme] if !ok { return nil, errors.New("schema '" + u.Scheme + "' not registered") diff --git a/core/storage/storage_test.go b/core/storage/storage_test.go index 32fbac413..336b5a7a9 100644 --- a/core/storage/storage_test.go +++ b/core/storage/storage_test.go @@ -4,6 +4,7 @@ import ( "net/url" "os" "path/filepath" + "runtime" "testing" "github.com/navidrome/navidrome/tests" @@ -83,6 +84,43 @@ var _ = Describe("Storage", func() { Entry("multiple special chars", "/tmp/Song #1 & More?.mp3"), ) }) + + Describe("LocalPathToURL", func() { + It("builds a file:// URL from an absolute path", func() { + u, err := LocalPathToURL("/tmp/music") + Expect(err).ToNot(HaveOccurred()) + Expect(u.Scheme).To(Equal("file")) + Expect(u.Path).To(Equal("/tmp/music")) + }) + + It("escapes special characters and decodes them back in Path", func() { + u, err := LocalPathToURL("/tmp/Song #1 & More?.mp3") + Expect(err).ToNot(HaveOccurred()) + Expect(u.Path).To(Equal("/tmp/Song #1 & More?.mp3")) + }) + + It("produces the same url.URL that For uses for a bare path", func() { + registry = map[string]constructor{} + Register("file", func(url url.URL) Storage { return &fakeLocalStorage{u: url} }) + s, err := For("/tmp/music") + Expect(err).ToNot(HaveOccurred()) + direct, err := LocalPathToURL("/tmp/music") + Expect(err).ToNot(HaveOccurred()) + Expect(s.(*fakeLocalStorage).u).To(Equal(direct)) + }) + + // On Windows, library paths are drive-lettered (e.g. C:\Music). This + // exercises the real conversion to confirm a drive-letter path yields a + // usable file:// URL on the actual platform (no-op on Unix). + It("handles a Windows drive-letter path", func() { + if runtime.GOOS != "windows" { + Skip("Windows-specific path handling") + } + u, err := LocalPathToURL(`C:\Music`) + Expect(err).ToNot(HaveOccurred()) + Expect(u.Scheme).To(Equal("file")) + }) + }) }) type fakeLocalStorage struct {