From f158190ea431176bc2dd54c7a6c85633890f6f9d Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 6 Nov 2025 20:41:54 -0500 Subject: [PATCH 01/14] fix: handle cross-library relative paths in playlists Playlists can now reference songs in other libraries using relative paths. Previously, relative paths like '../Songs/abc.mp3' would not resolve correctly when pointing to files in a different library than the playlist file. The fix resolves relative paths to absolute paths first, then checks which library they belong to using the library regex. This allows playlists to reference files across library boundaries while maintaining backward compatibility with existing single-library relative paths. Fixes #4617 --- core/playlists.go | 13 ++++++++++- core/playlists_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/core/playlists.go b/core/playlists.go index f98179f88..ca8a4add7 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -246,8 +246,19 @@ func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, fol var filePath string if folder != nil && !filepath.IsAbs(line) { - libPath = folder.LibraryPath + // For relative paths, resolve them to absolute first filePath = filepath.Join(folder.AbsolutePath(), line) + filePath = filepath.Clean(filePath) + // Try to find which library this resolved path belongs to + if libPath = libRegex.FindString(filePath); libPath == "" { + // If not found in regex, fallback to the playlist's library + libPath = folder.LibraryPath + // Verify the file is actually in a library by checking if we can get a relative path + if _, err := filepath.Rel(libPath, filePath); err != nil { + log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) + continue + } + } } else { cleanLine := filepath.Clean(line) if libPath = libRegex.FindString(cleanLine); libPath != "" { diff --git a/core/playlists_test.go b/core/playlists_test.go index fb42f9c9f..376166531 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -112,6 +112,58 @@ var _ = Describe("Playlists", func() { Expect(err.Error()).To(ContainSubstring("line 19, column 1: invalid character '\\n'")) }) }) + + Describe("Cross-library relative paths", func() { + var tmpDir, plsDir, songsDir string + + BeforeEach(func() { + // Create temp directory structure once + tmpDir = GinkgoT().TempDir() + plsDir = tmpDir + "/playlists" + songsDir = tmpDir + "/songs" + Expect(os.Mkdir(plsDir, 0755)).To(Succeed()) + Expect(os.Mkdir(songsDir, 0755)).To(Succeed()) + + // Setup two different libraries with paths matching our temp structure + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: songsDir}, + {ID: 2, Path: plsDir}, + }) + + // Create a mock media file repository that returns files for both libraries + // Note: The paths are relative to their respective library roots + ds.MockedMediaFile = &mockedMediaFileFromListRepo{ + data: []string{ + "abc.mp3", // This is songs/abc.mp3 relative to songsDir + "def.mp3", // This is playlists/def.mp3 relative to plsDir + }, + } + ps = NewPlaylists(ds) + }) + + It("handles relative paths that reference files in other libraries", func() { + // Create a temporary playlist file with relative path + plsContent := "#PLAYLIST:Cross Library Test\n../songs/abc.mp3\ndef.mp3" + plsFile := plsDir + "/test.m3u" + Expect(os.WriteFile(plsFile, []byte(plsContent), 0600)).To(Succeed()) + + // Playlist is in the Playlists library folder + // Important: Path should be relative to LibraryPath, and Name is the folder name + plsFolder := &model.Folder{ + ID: "2", + LibraryID: 2, + LibraryPath: plsDir, + Path: "", + Name: "", + } + + pls, err := ps.ImportFile(ctx, plsFolder, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Tracks).To(HaveLen(2)) + Expect(pls.Tracks[0].Path).To(Equal("abc.mp3")) + Expect(pls.Tracks[1].Path).To(Equal("def.mp3")) + }) + }) }) Describe("ImportM3U", func() { From 544f912c102966d663382bd751b238d520ec78ea Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 6 Nov 2025 21:15:38 -0500 Subject: [PATCH 02/14] fix: enhance playlist path normalization for cross-library support Signed-off-by: Deluan --- core/playlists.go | 20 ++++++++++---- core/playlists_test.go | 62 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index ca8a4add7..e26a1259a 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -233,7 +233,9 @@ func normalizePathForComparison(path string) string { return strings.ToLower(norm.NFC.String(path)) } -// TODO This won't work for multiple libraries +// normalizePaths converts playlist file paths to library-relative paths. +// For relative paths, it resolves them to absolute paths first, then determines which +// library they belong to. This allows playlists to reference files across library boundaries. func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, folder *model.Folder, lines []string) ([]string, error) { libRegex, err := s.compileLibraryPaths(ctx) if err != nil { @@ -246,15 +248,21 @@ func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, fol var filePath string if folder != nil && !filepath.IsAbs(line) { - // For relative paths, resolve them to absolute first + // Two-step resolution for relative paths: + // 1. Resolve relative path to absolute path based on playlist location + // Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" + // resolves to /music/songs/abc.mp3 filePath = filepath.Join(folder.AbsolutePath(), line) filePath = filepath.Clean(filePath) - // Try to find which library this resolved path belongs to + + // 2. Determine which library this absolute path belongs to using regex matching + // This allows playlists to reference files in different libraries if libPath = libRegex.FindString(filePath); libPath == "" { - // If not found in regex, fallback to the playlist's library + // If regex doesn't match any library, check if it's in the playlist's library libPath = folder.LibraryPath - // Verify the file is actually in a library by checking if we can get a relative path - if _, err := filepath.Rel(libPath, filePath); err != nil { + // Verify the file is actually in this library (reject paths with ..) + rel, err := filepath.Rel(libPath, filePath) + if err != nil || strings.HasPrefix(rel, "..") { log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) continue } diff --git a/core/playlists_test.go b/core/playlists_test.go index 376166531..9d16d9ccd 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -117,7 +117,7 @@ var _ = Describe("Playlists", func() { var tmpDir, plsDir, songsDir string BeforeEach(func() { - // Create temp directory structure once + // Create temp directory structure tmpDir = GinkgoT().TempDir() plsDir = tmpDir + "/playlists" songsDir = tmpDir + "/songs" @@ -160,8 +160,66 @@ var _ = Describe("Playlists", func() { pls, err := ps.ImportFile(ctx, plsFolder, "test.m3u") Expect(err).ToNot(HaveOccurred()) Expect(pls.Tracks).To(HaveLen(2)) + Expect(pls.Tracks[0].Path).To(Equal("abc.mp3")) // From songsDir library + Expect(pls.Tracks[1].Path).To(Equal("def.mp3")) // From plsDir library + }) + + It("ignores paths that point outside all libraries", func() { + // Create a temporary playlist file with path outside libraries + plsContent := "#PLAYLIST:Outside Test\n../../outside.mp3\nabc.mp3" + plsFile := plsDir + "/test.m3u" + Expect(os.WriteFile(plsFile, []byte(plsContent), 0600)).To(Succeed()) + + plsFolder := &model.Folder{ + ID: "2", + LibraryID: 2, + LibraryPath: plsDir, + Path: "", + Name: "", + } + + pls, err := ps.ImportFile(ctx, plsFolder, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + // Should only find abc.mp3, not outside.mp3 + Expect(pls.Tracks).To(HaveLen(1)) Expect(pls.Tracks[0].Path).To(Equal("abc.mp3")) - Expect(pls.Tracks[1].Path).To(Equal("def.mp3")) + }) + + It("handles relative paths with multiple '../' components", func() { + // Create a nested structure: tmpDir/playlists/subfolder/test.m3u + subFolder := plsDir + "/subfolder" + Expect(os.Mkdir(subFolder, 0755)).To(Succeed()) + + // Create the media file in the subfolder directory + // The mock will return it as "def.mp3" relative to plsDir + ds.MockedMediaFile = &mockedMediaFileFromListRepo{ + data: []string{ + "abc.mp3", // From songsDir library + "def.mp3", // From plsDir library root + }, + } + + // From subfolder, ../../songs/abc.mp3 should resolve to songs library + // ../def.mp3 should resolve to plsDir/def.mp3 + plsContent := "#PLAYLIST:Nested Test\n../../songs/abc.mp3\n../def.mp3" + plsFile := subFolder + "/test.m3u" + Expect(os.WriteFile(plsFile, []byte(plsContent), 0600)).To(Succeed()) + + // The folder: AbsolutePath = LibraryPath + Path + Name + // So for /playlists/subfolder: LibraryPath=/playlists, Path="", Name="subfolder" + plsFolder := &model.Folder{ + ID: "2", + LibraryID: 2, + LibraryPath: plsDir, + Path: "", // Empty because subfolder is directly under library root + Name: "subfolder", // The folder name + } + + pls, err := ps.ImportFile(ctx, plsFolder, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Tracks).To(HaveLen(2)) + Expect(pls.Tracks[0].Path).To(Equal("abc.mp3")) // From songsDir library + Expect(pls.Tracks[1].Path).To(Equal("def.mp3")) // From plsDir library root }) }) }) From 84449996d2ea7fda6bb810d73ddbdaa31bc5937c Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 6 Nov 2025 23:50:33 -0500 Subject: [PATCH 03/14] refactor: improve handling of relative paths in playlists for cross-library compatibility Signed-off-by: Deluan --- core/playlists.go | 120 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 83 insertions(+), 37 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index e26a1259a..0d38149e5 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -233,6 +233,30 @@ func normalizePathForComparison(path string) string { return strings.ToLower(norm.NFC.String(path)) } +// pathResolution holds the result of resolving a playlist path to a library-relative path. +type pathResolution struct { + absolutePath string + libraryPath string + valid bool +} + +// toRelativePath converts the absolute path to a library-relative path. +func (r pathResolution) toRelativePath() (string, error) { + if !r.valid { + return "", fmt.Errorf("invalid path resolution") + } + return filepath.Rel(r.libraryPath, r.absolutePath) +} + +// newValidResolution creates a valid path resolution. +func newValidResolution(absolutePath, libraryPath string) pathResolution { + return pathResolution{ + absolutePath: absolutePath, + libraryPath: libraryPath, + valid: true, + } +} + // normalizePaths converts playlist file paths to library-relative paths. // For relative paths, it resolves them to absolute paths first, then determines which // library they belong to. This allows playlists to reference files across library boundaries. @@ -244,50 +268,72 @@ func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, fol res := make([]string, 0, len(lines)) for idx, line := range lines { - var libPath string - var filePath string + resolution := s.resolvePath(line, folder, libRegex) - if folder != nil && !filepath.IsAbs(line) { - // Two-step resolution for relative paths: - // 1. Resolve relative path to absolute path based on playlist location - // Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" - // resolves to /music/songs/abc.mp3 - filePath = filepath.Join(folder.AbsolutePath(), line) - filePath = filepath.Clean(filePath) - - // 2. Determine which library this absolute path belongs to using regex matching - // This allows playlists to reference files in different libraries - if libPath = libRegex.FindString(filePath); libPath == "" { - // If regex doesn't match any library, check if it's in the playlist's library - libPath = folder.LibraryPath - // Verify the file is actually in this library (reject paths with ..) - rel, err := filepath.Rel(libPath, filePath) - if err != nil || strings.HasPrefix(rel, "..") { - log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) - continue - } - } - } else { - cleanLine := filepath.Clean(line) - if libPath = libRegex.FindString(cleanLine); libPath != "" { - filePath = cleanLine - } - } - - if libPath != "" { - if rel, err := filepath.Rel(libPath, filePath); err == nil { - res = append(res, rel) - } else { - log.Debug(ctx, "Error getting relative path", "playlist", pls.Name, "path", line, "libPath", libPath, - "filePath", filePath, err) - } - } else { + if !resolution.valid { log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) + continue } + + relativePath, err := resolution.toRelativePath() + if err != nil { + log.Debug(ctx, "Error getting relative path", "playlist", pls.Name, "path", line, + "libPath", resolution.libraryPath, "filePath", resolution.absolutePath, err) + continue + } + + res = append(res, relativePath) } return slice.Map(res, filepath.ToSlash), nil } +// resolvePath determines the absolute path and library path for a playlist entry. +func (s *playlists) resolvePath(line string, folder *model.Folder, libRegex *regexp.Regexp) pathResolution { + if folder != nil && !filepath.IsAbs(line) { + return s.resolveRelativePath(line, folder, libRegex) + } + return s.resolveAbsolutePath(line, libRegex) +} + +// resolveRelativePath handles relative paths by converting them to absolute paths +// and finding their library location. This enables cross-library playlist references. +func (s *playlists) resolveRelativePath(line string, folder *model.Folder, libRegex *regexp.Regexp) pathResolution { + // Step 1: Resolve relative path to absolute path based on playlist location + // Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" + // resolves to /music/songs/abc.mp3 + absolutePath := filepath.Clean(filepath.Join(folder.AbsolutePath(), line)) + + // Step 2: Determine which library this absolute path belongs to using regex matching + if libPath := libRegex.FindString(absolutePath); libPath != "" { + return newValidResolution(absolutePath, libPath) + } + + // Fallback: Check if it's in the playlist's own library + return s.validatePathInLibrary(absolutePath, folder.LibraryPath) +} + +// resolveAbsolutePath handles absolute paths by matching them against library paths. +func (s *playlists) resolveAbsolutePath(line string, libRegex *regexp.Regexp) pathResolution { + cleanPath := filepath.Clean(line) + libPath := libRegex.FindString(cleanPath) + + if libPath == "" { + return pathResolution{valid: false} + } + return newValidResolution(cleanPath, libPath) +} + +// validatePathInLibrary verifies that an absolute path belongs to the specified library. +// It rejects paths that escape the library using ".." segments. +func (s *playlists) validatePathInLibrary(absolutePath, libraryPath string) pathResolution { + rel, err := filepath.Rel(libraryPath, absolutePath) + if err != nil || strings.HasPrefix(rel, "..") { + return pathResolution{valid: false} + } + + return newValidResolution(absolutePath, libraryPath) +} + func (s *playlists) compileLibraryPaths(ctx context.Context) (*regexp.Regexp, error) { libs, err := s.ds.Library(ctx).GetAll() if err != nil { From eab196d7e2b5e3cd0d0199441010644994ff9d9b Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 00:08:48 -0500 Subject: [PATCH 04/14] fix: ensure longest library path matches first to resolve prefix conflicts in playlists Signed-off-by: Deluan --- core/playlists.go | 12 ++- core/playlists_internal_test.go | 186 ++++++++++++++++++++++++++++++++ core/playlists_test.go | 43 ++++++++ 3 files changed, 240 insertions(+), 1 deletion(-) create mode 100644 core/playlists_internal_test.go diff --git a/core/playlists.go b/core/playlists.go index 0d38149e5..3d1cce32f 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -1,6 +1,7 @@ package core import ( + "cmp" "context" "encoding/json" "errors" @@ -10,6 +11,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "strings" "time" @@ -340,6 +342,14 @@ func (s *playlists) compileLibraryPaths(ctx context.Context) (*regexp.Regexp, er return nil, err } + // Sort libraries by path length (descending) to ensure longest paths match first. + // This prevents shorter paths that are prefixes from incorrectly matching. + // Example: /music-classical must be checked before /music + // Otherwise, /music-classical/track.mp3 would match /music instead of /music-classical + slices.SortFunc(libs, func(i, j model.Library) int { + return cmp.Compare(len(j.Path), len(i.Path)) // Reverse order for descending + }) + // Create regex patterns for each library path patterns := make([]string, len(libs)) for i, lib := range libs { @@ -347,7 +357,7 @@ func (s *playlists) compileLibraryPaths(ctx context.Context) (*regexp.Regexp, er escapedPath := regexp.QuoteMeta(cleanPath) patterns[i] = fmt.Sprintf("^%s(?:/|$)", escapedPath) } - // Combine all patterns into a single regex + // Combine all patterns into a single regex - order matters due to alternation combinedPattern := strings.Join(patterns, "|") re, err := regexp.Compile(combinedPattern) if err != nil { diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go new file mode 100644 index 000000000..2fda603b3 --- /dev/null +++ b/core/playlists_internal_test.go @@ -0,0 +1,186 @@ +package core + +import ( + "context" + + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("compileLibraryPaths", func() { + var ds *tests.MockDataStore + var mockLibRepo *tests.MockLibraryRepo + var ps *playlists + ctx := context.Background() + + BeforeEach(func() { + mockLibRepo = &tests.MockLibraryRepo{} + ds = &tests.MockDataStore{ + MockedLibrary: mockLibRepo, + } + ps = &playlists{ds: ds} + }) + + Describe("Longest library path matching", func() { + It("matches the longest library path when multiple libraries share a prefix", func() { + // Setup libraries with prefix conflicts + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + {ID: 2, Path: "/music-classical"}, + {ID: 3, Path: "/music-classical/opera"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + + // Test that longest path matches first + // Note: The regex pattern ^path(?:/|$) will match the path plus the trailing / + testCases := []struct { + path string + expected string + }{ + {"/music-classical/opera/track.mp3", "/music-classical/opera/"}, + {"/music-classical/track.mp3", "/music-classical/"}, + {"/music/track.mp3", "/music/"}, + {"/music-classical/opera/", "/music-classical/opera/"}, // Trailing slash + {"/music-classical/opera", "/music-classical/opera"}, // Exact match (no trailing /) + } + + for _, tc := range testCases { + matched := libRegex.FindString(tc.path) + Expect(matched).To(Equal(tc.expected), "Path %s should match %s, but got %s", tc.path, tc.expected, matched) + } + }) + + It("handles libraries with similar prefixes but different structures", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/home/user/music"}, + {ID: 2, Path: "/home/user/music-backup"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + + // Test that music-backup library is matched correctly + matched := libRegex.FindString("/home/user/music-backup/track.mp3") + Expect(matched).To(Equal("/home/user/music-backup/")) + + // Test that music library is still matched correctly + matched = libRegex.FindString("/home/user/music/track.mp3") + Expect(matched).To(Equal("/home/user/music/")) + }) + + It("matches path that is exactly the library root", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + {ID: 2, Path: "/music-classical"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + + // Exact library path should match (no trailing /) + matched := libRegex.FindString("/music-classical") + Expect(matched).To(Equal("/music-classical")) + }) + + It("handles complex nested library structures", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/media"}, + {ID: 2, Path: "/media/audio"}, + {ID: 3, Path: "/media/audio/classical"}, + {ID: 4, Path: "/media/audio/classical/baroque"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + + testCases := []struct { + path string + expected string + }{ + {"/media/audio/classical/baroque/bach/track.mp3", "/media/audio/classical/baroque/"}, + {"/media/audio/classical/mozart/track.mp3", "/media/audio/classical/"}, + {"/media/audio/rock/track.mp3", "/media/audio/"}, + {"/media/video/movie.mp4", "/media/"}, + } + + for _, tc := range testCases { + matched := libRegex.FindString(tc.path) + Expect(matched).To(Equal(tc.expected), "Path %s should match %s, but got %s", tc.path, tc.expected, matched) + } + }) + }) + + Describe("Edge cases", func() { + It("handles empty library list", func() { + mockLibRepo.SetData([]model.Library{}) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(libRegex).ToNot(BeNil()) + + // Should not match anything + matched := libRegex.FindString("/music/track.mp3") + Expect(matched).To(BeEmpty()) + }) + + It("handles single library", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + + matched := libRegex.FindString("/music/track.mp3") + Expect(matched).To(Equal("/music/")) + }) + + It("handles libraries with special regex characters", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music[test]"}, + {ID: 2, Path: "/music(backup)"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(libRegex).ToNot(BeNil()) + + // Special characters should be escaped and match literally + matched := libRegex.FindString("/music[test]/track.mp3") + Expect(matched).To(Equal("/music[test]/")) + }) + }) + + Describe("Regex pattern validation", func() { + It("ensures regex alternation respects order by testing actual matching behavior", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/a"}, + {ID: 2, Path: "/ab"}, + {ID: 3, Path: "/abc"}, + }) + + libRegex, err := ps.compileLibraryPaths(ctx) + Expect(err).ToNot(HaveOccurred()) + + // Verify that longer paths match correctly (not cut off by shorter prefix) + // If ordering is wrong, /ab would match before /abc for path "/abc/file" + testCases := []struct { + path string + expected string + }{ + {"/abc/file.mp3", "/abc/"}, + {"/ab/file.mp3", "/ab/"}, + {"/a/file.mp3", "/a/"}, + } + + for _, tc := range testCases { + matched := libRegex.FindString(tc.path) + Expect(matched).To(Equal(tc.expected), "Path %s should match %s", tc.path, tc.expected) + } + }) + }) +}) diff --git a/core/playlists_test.go b/core/playlists_test.go index 9d16d9ccd..d5e33ad0f 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -221,6 +221,49 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks[0].Path).To(Equal("abc.mp3")) // From songsDir library Expect(pls.Tracks[1].Path).To(Equal("def.mp3")) // From plsDir library root }) + + It("correctly resolves libraries when one path is a prefix of another", func() { + // This tests the bug where /music would match before /music-classical + // Create temp directory structure with prefix conflict + tmpDir := GinkgoT().TempDir() + musicDir := tmpDir + "/music" + musicClassicalDir := tmpDir + "/music-classical" + Expect(os.Mkdir(musicDir, 0755)).To(Succeed()) + Expect(os.Mkdir(musicClassicalDir, 0755)).To(Succeed()) + + // Setup two libraries where one is a prefix of the other + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: musicDir}, // /tmp/xxx/music + {ID: 2, Path: musicClassicalDir}, // /tmp/xxx/music-classical + }) + + // Mock will return tracks from both libraries + ds.MockedMediaFile = &mockedMediaFileFromListRepo{ + data: []string{ + "rock.mp3", // From music library + "bach.mp3", // From music-classical library + }, + } + + // Create playlist in music library that references music-classical + plsContent := "#PLAYLIST:Cross Prefix Test\nrock.mp3\n../music-classical/bach.mp3" + plsFile := musicDir + "/test.m3u" + Expect(os.WriteFile(plsFile, []byte(plsContent), 0600)).To(Succeed()) + + plsFolder := &model.Folder{ + ID: "1", + LibraryID: 1, + LibraryPath: musicDir, + Path: "", + Name: "", + } + + pls, err := ps.ImportFile(ctx, plsFolder, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Tracks).To(HaveLen(2)) + Expect(pls.Tracks[0].Path).To(Equal("rock.mp3")) // From music library + Expect(pls.Tracks[1].Path).To(Equal("bach.mp3")) // From music-classical library (not music!) + }) }) }) From 6a2851ef9210ab551a5653c371f42199ad9d4938 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 08:46:59 -0500 Subject: [PATCH 05/14] test: refactor tests isolation Signed-off-by: Deluan --- core/playlists_internal_test.go | 26 ++++++++++++++++++ core/playlists_test.go | 48 +++++++++------------------------ 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index 2fda603b3..252192b29 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -7,6 +7,7 @@ import ( "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "golang.org/x/text/unicode/norm" ) var _ = Describe("compileLibraryPaths", func() { @@ -184,3 +185,28 @@ var _ = Describe("compileLibraryPaths", func() { }) }) }) + +var _ = Describe("normalizePathForComparison", func() { + It("normalizes Unicode characters to NFC form and converts to lowercase", func() { + // Test with NFD (decomposed) input - as would come from macOS filesystem + nfdPath := norm.NFD.String("Michèle") // Explicitly convert to NFD form + normalized := normalizePathForComparison(nfdPath) + Expect(normalized).To(Equal("michèle")) + + // Test with NFC (composed) input - as would come from Apple Music M3U + nfcPath := "Michèle" // This might be in NFC form + normalizedNfc := normalizePathForComparison(nfcPath) + + // Ensure the two paths are not equal in their original forms + Expect(nfdPath).ToNot(Equal(nfcPath)) + + // Both should normalize to the same result + Expect(normalized).To(Equal(normalizedNfc)) + }) + + It("handles paths with mixed case and Unicode characters", func() { + path := "Artist/Noël Coward/Album/Song.mp3" + normalized := normalizePathForComparison(path) + Expect(normalized).To(Equal("artist/noël coward/album/song.mp3")) + }) +}) diff --git a/core/playlists_test.go b/core/playlists_test.go index d5e33ad0f..b9886bc86 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -1,4 +1,4 @@ -package core +package core_test import ( "context" @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" @@ -20,7 +21,7 @@ import ( var _ = Describe("Playlists", func() { var ds *tests.MockDataStore - var ps Playlists + var ps core.Playlists var mockPlsRepo mockedPlaylistRepo var mockLibRepo *tests.MockLibraryRepo ctx := context.Background() @@ -40,7 +41,7 @@ var _ = Describe("Playlists", func() { Describe("ImportFile", func() { var folder *model.Folder BeforeEach(func() { - ps = NewPlaylists(ds) + ps = core.NewPlaylists(ds) ds.MockedMediaFile = &mockedMediaFileRepo{} libPath, _ := os.Getwd() folder = &model.Folder{ @@ -138,7 +139,7 @@ var _ = Describe("Playlists", func() { "def.mp3", // This is playlists/def.mp3 relative to plsDir }, } - ps = NewPlaylists(ds) + ps = core.NewPlaylists(ds) }) It("handles relative paths that reference files in other libraries", func() { @@ -272,7 +273,7 @@ var _ = Describe("Playlists", func() { BeforeEach(func() { repo = &mockedMediaFileFromListRepo{} ds.MockedMediaFile = repo - ps = NewPlaylists(ds) + ps = core.NewPlaylists(ds) mockLibRepo.SetData([]model.Library{{ID: 1, Path: "/music"}, {ID: 2, Path: "/new"}}) ctx = request.WithUser(ctx, model.User{ID: "123"}) }) @@ -383,31 +384,6 @@ var _ = Describe("Playlists", func() { }) }) - Describe("normalizePathForComparison", func() { - It("normalizes Unicode characters to NFC form and converts to lowercase", func() { - // Test with NFD (decomposed) input - as would come from macOS filesystem - nfdPath := norm.NFD.String("Michèle") // Explicitly convert to NFD form - normalized := normalizePathForComparison(nfdPath) - Expect(normalized).To(Equal("michèle")) - - // Test with NFC (composed) input - as would come from Apple Music M3U - nfcPath := "Michèle" // This might be in NFC form - normalizedNfc := normalizePathForComparison(nfcPath) - - // Ensure the two paths are not equal in their original forms - Expect(nfdPath).ToNot(Equal(nfcPath)) - - // Both should normalize to the same result - Expect(normalized).To(Equal(normalizedNfc)) - }) - - It("handles paths with mixed case and Unicode characters", func() { - path := "Artist/Noël Coward/Album/Song.mp3" - normalized := normalizePathForComparison(path) - Expect(normalized).To(Equal("artist/noël coward/album/song.mp3")) - }) - }) - Describe("InPlaylistsPath", func() { var folder model.Folder @@ -422,27 +398,27 @@ var _ = Describe("Playlists", func() { It("returns true if PlaylistsPath is empty", func() { conf.Server.PlaylistsPath = "" - Expect(InPlaylistsPath(folder)).To(BeTrue()) + Expect(core.InPlaylistsPath(folder)).To(BeTrue()) }) It("returns true if PlaylistsPath is any (**/**)", func() { conf.Server.PlaylistsPath = "**/**" - Expect(InPlaylistsPath(folder)).To(BeTrue()) + Expect(core.InPlaylistsPath(folder)).To(BeTrue()) }) It("returns true if folder is in PlaylistsPath", func() { conf.Server.PlaylistsPath = "other/**:playlists/**" - Expect(InPlaylistsPath(folder)).To(BeTrue()) + Expect(core.InPlaylistsPath(folder)).To(BeTrue()) }) It("returns false if folder is not in PlaylistsPath", func() { conf.Server.PlaylistsPath = "other" - Expect(InPlaylistsPath(folder)).To(BeFalse()) + Expect(core.InPlaylistsPath(folder)).To(BeFalse()) }) It("returns true if for a playlist in root of MusicFolder if PlaylistsPath is '.'", func() { conf.Server.PlaylistsPath = "." - Expect(InPlaylistsPath(folder)).To(BeFalse()) + Expect(core.InPlaylistsPath(folder)).To(BeFalse()) folder2 := model.Folder{ LibraryPath: "/music", @@ -450,7 +426,7 @@ var _ = Describe("Playlists", func() { Name: ".", } - Expect(InPlaylistsPath(folder2)).To(BeTrue()) + Expect(core.InPlaylistsPath(folder2)).To(BeTrue()) }) }) }) From ae042560303bba924c5f369a82c4f926bfe087f3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 13:11:19 -0500 Subject: [PATCH 06/14] fix: enhance handling of library-qualified paths and improve cross-library playlist support Signed-off-by: Deluan --- core/playlists.go | 151 ++++++++++++++++++---------- core/playlists_internal_test.go | 117 +++++++++++---------- core/playlists_test.go | 140 ++++++++++++++++++++++++-- persistence/mediafile_repository.go | 33 +++++- 4 files changed, 326 insertions(+), 115 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index 3d1cce32f..983ff6429 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -10,7 +10,6 @@ import ( "net/url" "os" "path/filepath" - "regexp" "slices" "strings" "time" @@ -206,12 +205,25 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m log.Warn(ctx, "Error reading files from DB", "playlist", pls.Name, err) continue } + // Build lookup map with library-qualified keys existing := make(map[string]int, len(found)) for idx := range found { - existing[normalizePathForComparison(found[idx].Path)] = idx + // Key format: "libraryID:path" (normalized) + key := fmt.Sprintf("%d:%s", found[idx].LibraryID, normalizePathForComparison(found[idx].Path)) + existing[key] = idx } for _, path := range paths { - idx, ok := existing[normalizePathForComparison(path)] + // Parse the library-qualified path + normalizedPath := path + if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { + // Path is already qualified: "libraryID:path" + normalizedPath = parts[0] + ":" + normalizePathForComparison(parts[1]) + } else { + // Unqualified path (backward compatibility) + normalizedPath = normalizePathForComparison(path) + } + + idx, ok := existing[normalizedPath] if ok { mfs = append(mfs, found[idx]) } else { @@ -239,131 +251,168 @@ func normalizePathForComparison(path string) string { type pathResolution struct { absolutePath string libraryPath string + libraryID int valid bool } -// toRelativePath converts the absolute path to a library-relative path. -func (r pathResolution) toRelativePath() (string, error) { +// toLibraryQualifiedPath converts the absolute path to a library-qualified path format: "libraryID:relativePath". +func (r pathResolution) toLibraryQualifiedPath() (string, error) { if !r.valid { return "", fmt.Errorf("invalid path resolution") } - return filepath.Rel(r.libraryPath, r.absolutePath) + relativePath, err := filepath.Rel(r.libraryPath, r.absolutePath) + if err != nil { + return "", err + } + return fmt.Sprintf("%d:%s", r.libraryID, relativePath), nil } // newValidResolution creates a valid path resolution. -func newValidResolution(absolutePath, libraryPath string) pathResolution { +func newValidResolution(absolutePath, libraryPath string, libraryID int) pathResolution { return pathResolution{ absolutePath: absolutePath, libraryPath: libraryPath, + libraryID: libraryID, valid: true, } } -// normalizePaths converts playlist file paths to library-relative paths. +// libraryMatcher holds sorted libraries with cleaned paths for efficient path matching. +type libraryMatcher struct { + libraries model.Libraries + cleanedPaths []string +} + +// findLibraryForPath finds which library contains the given absolute path. +// Returns library ID and path, or 0 and empty string if not found. +func (lm *libraryMatcher) findLibraryForPath(absolutePath string) (int, string) { + // Check sorted libraries (longest path first) to find the best match + for i, cleanLibPath := range lm.cleanedPaths { + // Check if absolutePath is under this library path + if strings.HasPrefix(absolutePath, cleanLibPath) { + // Ensure it's a proper path boundary (not just a prefix) + if len(absolutePath) == len(cleanLibPath) || absolutePath[len(cleanLibPath)] == filepath.Separator { + return lm.libraries[i].ID, cleanLibPath + } + } + } + return 0, "" +} + +// newLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). +// This ensures correct matching when library paths are prefixes of each other. +// Example: /music-classical must be checked before /music +// Otherwise, /music-classical/track.mp3 would match /music instead of /music-classical +func newLibraryMatcher(libs model.Libraries) *libraryMatcher { + // Sort libraries by path length (descending) to ensure longest paths match first. + slices.SortFunc(libs, func(i, j model.Library) int { + return cmp.Compare(len(j.Path), len(i.Path)) // Reverse order for descending + }) + + // Pre-clean all library paths once for efficient matching + cleanedPaths := make([]string, len(libs)) + for i, lib := range libs { + cleanedPaths[i] = filepath.Clean(lib.Path) + } + return &libraryMatcher{ + libraries: libs, + cleanedPaths: cleanedPaths, + } +} + +// normalizePaths converts playlist file paths to library-qualified paths (format: "libraryID:relativePath"). // For relative paths, it resolves them to absolute paths first, then determines which // library they belong to. This allows playlists to reference files across library boundaries. func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, folder *model.Folder, lines []string) ([]string, error) { - libRegex, err := s.compileLibraryPaths(ctx) + matcher, err := s.buildLibraryMatcher(ctx) if err != nil { return nil, err } res := make([]string, 0, len(lines)) for idx, line := range lines { - resolution := s.resolvePath(line, folder, libRegex) + resolution := s.resolvePath(line, folder, matcher) if !resolution.valid { log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) continue } - relativePath, err := resolution.toRelativePath() + qualifiedPath, err := resolution.toLibraryQualifiedPath() if err != nil { - log.Debug(ctx, "Error getting relative path", "playlist", pls.Name, "path", line, + log.Debug(ctx, "Error getting library-qualified path", "playlist", pls.Name, "path", line, "libPath", resolution.libraryPath, "filePath", resolution.absolutePath, err) continue } - res = append(res, relativePath) + res = append(res, qualifiedPath) } - return slice.Map(res, filepath.ToSlash), nil + // Convert path separators to forward slashes (only the path part, not the library ID prefix) + return slice.Map(res, func(qp string) string { + parts := strings.SplitN(qp, ":", 2) + if len(parts) == 2 { + return parts[0] + ":" + filepath.ToSlash(parts[1]) + } + return filepath.ToSlash(qp) + }), nil } // resolvePath determines the absolute path and library path for a playlist entry. -func (s *playlists) resolvePath(line string, folder *model.Folder, libRegex *regexp.Regexp) pathResolution { +func (s *playlists) resolvePath(line string, folder *model.Folder, matcher *libraryMatcher) pathResolution { if folder != nil && !filepath.IsAbs(line) { - return s.resolveRelativePath(line, folder, libRegex) + return s.resolveRelativePath(line, folder, matcher) } - return s.resolveAbsolutePath(line, libRegex) + return s.resolveAbsolutePath(line, matcher) } // resolveRelativePath handles relative paths by converting them to absolute paths // and finding their library location. This enables cross-library playlist references. -func (s *playlists) resolveRelativePath(line string, folder *model.Folder, libRegex *regexp.Regexp) pathResolution { +func (s *playlists) resolveRelativePath(line string, folder *model.Folder, matcher *libraryMatcher) pathResolution { // Step 1: Resolve relative path to absolute path based on playlist location // Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" // resolves to /music/songs/abc.mp3 absolutePath := filepath.Clean(filepath.Join(folder.AbsolutePath(), line)) - // Step 2: Determine which library this absolute path belongs to using regex matching - if libPath := libRegex.FindString(absolutePath); libPath != "" { - return newValidResolution(absolutePath, libPath) + // Step 2: Determine which library this absolute path belongs to + libID, libPath := matcher.findLibraryForPath(absolutePath) + if libID != 0 { + return newValidResolution(absolutePath, libPath, libID) } // Fallback: Check if it's in the playlist's own library - return s.validatePathInLibrary(absolutePath, folder.LibraryPath) + return s.validatePathInLibrary(absolutePath, folder.LibraryPath, folder.LibraryID) } // resolveAbsolutePath handles absolute paths by matching them against library paths. -func (s *playlists) resolveAbsolutePath(line string, libRegex *regexp.Regexp) pathResolution { +func (s *playlists) resolveAbsolutePath(line string, matcher *libraryMatcher) pathResolution { cleanPath := filepath.Clean(line) - libPath := libRegex.FindString(cleanPath) + libID, libPath := matcher.findLibraryForPath(cleanPath) - if libPath == "" { + if libID == 0 { return pathResolution{valid: false} } - return newValidResolution(cleanPath, libPath) + return newValidResolution(cleanPath, libPath, libID) } // validatePathInLibrary verifies that an absolute path belongs to the specified library. // It rejects paths that escape the library using ".." segments. -func (s *playlists) validatePathInLibrary(absolutePath, libraryPath string) pathResolution { +func (s *playlists) validatePathInLibrary(absolutePath, libraryPath string, libraryID int) pathResolution { rel, err := filepath.Rel(libraryPath, absolutePath) if err != nil || strings.HasPrefix(rel, "..") { return pathResolution{valid: false} } - return newValidResolution(absolutePath, libraryPath) + return newValidResolution(absolutePath, libraryPath, libraryID) } -func (s *playlists) compileLibraryPaths(ctx context.Context) (*regexp.Regexp, error) { +// buildLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). +func (s *playlists) buildLibraryMatcher(ctx context.Context) (*libraryMatcher, error) { libs, err := s.ds.Library(ctx).GetAll() if err != nil { return nil, err } - // Sort libraries by path length (descending) to ensure longest paths match first. - // This prevents shorter paths that are prefixes from incorrectly matching. - // Example: /music-classical must be checked before /music - // Otherwise, /music-classical/track.mp3 would match /music instead of /music-classical - slices.SortFunc(libs, func(i, j model.Library) int { - return cmp.Compare(len(j.Path), len(i.Path)) // Reverse order for descending - }) - - // Create regex patterns for each library path - patterns := make([]string, len(libs)) - for i, lib := range libs { - cleanPath := filepath.Clean(lib.Path) - escapedPath := regexp.QuoteMeta(cleanPath) - patterns[i] = fmt.Sprintf("^%s(?:/|$)", escapedPath) - } - // Combine all patterns into a single regex - order matters due to alternation - combinedPattern := strings.Join(patterns, "|") - re, err := regexp.Compile(combinedPattern) - if err != nil { - return nil, fmt.Errorf("compiling library paths `%s`: %w", combinedPattern, err) - } - return re, nil + return newLibraryMatcher(libs), nil } func (s *playlists) updatePlaylist(ctx context.Context, newPls *model.Playlist) error { diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index 252192b29..6ed57a525 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -10,7 +10,7 @@ import ( "golang.org/x/text/unicode/norm" ) -var _ = Describe("compileLibraryPaths", func() { +var _ = Describe("buildLibraryMatcher", func() { var ds *tests.MockDataStore var mockLibRepo *tests.MockLibraryRepo var ps *playlists @@ -33,25 +33,25 @@ var _ = Describe("compileLibraryPaths", func() { {ID: 3, Path: "/music-classical/opera"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) - // Test that longest path matches first - // Note: The regex pattern ^path(?:/|$) will match the path plus the trailing / + // Test that longest path matches first and returns correct library ID testCases := []struct { - path string - expected string + path string + expectedLibID int + expectedLibPath string }{ - {"/music-classical/opera/track.mp3", "/music-classical/opera/"}, - {"/music-classical/track.mp3", "/music-classical/"}, - {"/music/track.mp3", "/music/"}, - {"/music-classical/opera/", "/music-classical/opera/"}, // Trailing slash - {"/music-classical/opera", "/music-classical/opera"}, // Exact match (no trailing /) + {"/music-classical/opera/track.mp3", 3, "/music-classical/opera"}, + {"/music-classical/track.mp3", 2, "/music-classical"}, + {"/music/track.mp3", 1, "/music"}, + {"/music-classical/opera/subdir/file.mp3", 3, "/music-classical/opera"}, } for _, tc := range testCases { - matched := libRegex.FindString(tc.path) - Expect(matched).To(Equal(tc.expected), "Path %s should match %s, but got %s", tc.path, tc.expected, matched) + libID, libPath := matcher.findLibraryForPath(tc.path) + Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d, but got %d", tc.path, tc.expectedLibID, libID) + Expect(libPath).To(Equal(tc.expectedLibPath), "Path %s should match library path %s, but got %s", tc.path, tc.expectedLibPath, libPath) } }) @@ -61,16 +61,18 @@ var _ = Describe("compileLibraryPaths", func() { {ID: 2, Path: "/home/user/music-backup"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) // Test that music-backup library is matched correctly - matched := libRegex.FindString("/home/user/music-backup/track.mp3") - Expect(matched).To(Equal("/home/user/music-backup/")) + libID, libPath := matcher.findLibraryForPath("/home/user/music-backup/track.mp3") + Expect(libID).To(Equal(2)) + Expect(libPath).To(Equal("/home/user/music-backup")) // Test that music library is still matched correctly - matched = libRegex.FindString("/home/user/music/track.mp3") - Expect(matched).To(Equal("/home/user/music/")) + libID, libPath = matcher.findLibraryForPath("/home/user/music/track.mp3") + Expect(libID).To(Equal(1)) + Expect(libPath).To(Equal("/home/user/music")) }) It("matches path that is exactly the library root", func() { @@ -79,12 +81,13 @@ var _ = Describe("compileLibraryPaths", func() { {ID: 2, Path: "/music-classical"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) - // Exact library path should match (no trailing /) - matched := libRegex.FindString("/music-classical") - Expect(matched).To(Equal("/music-classical")) + // Exact library path should match + libID, libPath := matcher.findLibraryForPath("/music-classical") + Expect(libID).To(Equal(2)) + Expect(libPath).To(Equal("/music-classical")) }) It("handles complex nested library structures", func() { @@ -95,22 +98,24 @@ var _ = Describe("compileLibraryPaths", func() { {ID: 4, Path: "/media/audio/classical/baroque"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) testCases := []struct { - path string - expected string + path string + expectedLibID int + expectedLibPath string }{ - {"/media/audio/classical/baroque/bach/track.mp3", "/media/audio/classical/baroque/"}, - {"/media/audio/classical/mozart/track.mp3", "/media/audio/classical/"}, - {"/media/audio/rock/track.mp3", "/media/audio/"}, - {"/media/video/movie.mp4", "/media/"}, + {"/media/audio/classical/baroque/bach/track.mp3", 4, "/media/audio/classical/baroque"}, + {"/media/audio/classical/mozart/track.mp3", 3, "/media/audio/classical"}, + {"/media/audio/rock/track.mp3", 2, "/media/audio"}, + {"/media/video/movie.mp4", 1, "/media"}, } for _, tc := range testCases { - matched := libRegex.FindString(tc.path) - Expect(matched).To(Equal(tc.expected), "Path %s should match %s, but got %s", tc.path, tc.expected, matched) + libID, libPath := matcher.findLibraryForPath(tc.path) + Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) + Expect(libPath).To(Equal(tc.expectedLibPath), "Path %s should match library path %s", tc.path, tc.expectedLibPath) } }) }) @@ -119,13 +124,14 @@ var _ = Describe("compileLibraryPaths", func() { It("handles empty library list", func() { mockLibRepo.SetData([]model.Library{}) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(libRegex).ToNot(BeNil()) + Expect(matcher).ToNot(BeNil()) // Should not match anything - matched := libRegex.FindString("/music/track.mp3") - Expect(matched).To(BeEmpty()) + libID, libPath := matcher.findLibraryForPath("/music/track.mp3") + Expect(libID).To(Equal(0)) + Expect(libPath).To(BeEmpty()) }) It("handles single library", func() { @@ -133,54 +139,55 @@ var _ = Describe("compileLibraryPaths", func() { {ID: 1, Path: "/music"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) - matched := libRegex.FindString("/music/track.mp3") - Expect(matched).To(Equal("/music/")) + libID, libPath := matcher.findLibraryForPath("/music/track.mp3") + Expect(libID).To(Equal(1)) + Expect(libPath).To(Equal("/music")) }) - It("handles libraries with special regex characters", func() { + It("handles libraries with special characters in paths", func() { mockLibRepo.SetData([]model.Library{ {ID: 1, Path: "/music[test]"}, {ID: 2, Path: "/music(backup)"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(libRegex).ToNot(BeNil()) + Expect(matcher).ToNot(BeNil()) - // Special characters should be escaped and match literally - matched := libRegex.FindString("/music[test]/track.mp3") - Expect(matched).To(Equal("/music[test]/")) + // Special characters should match literally + libID, libPath := matcher.findLibraryForPath("/music[test]/track.mp3") + Expect(libID).To(Equal(1)) + Expect(libPath).To(Equal("/music[test]")) }) }) - Describe("Regex pattern validation", func() { - It("ensures regex alternation respects order by testing actual matching behavior", func() { + Describe("Path matching order", func() { + It("ensures longest paths match first", func() { mockLibRepo.SetData([]model.Library{ {ID: 1, Path: "/a"}, {ID: 2, Path: "/ab"}, {ID: 3, Path: "/abc"}, }) - libRegex, err := ps.compileLibraryPaths(ctx) + matcher, err := ps.buildLibraryMatcher(ctx) Expect(err).ToNot(HaveOccurred()) // Verify that longer paths match correctly (not cut off by shorter prefix) - // If ordering is wrong, /ab would match before /abc for path "/abc/file" testCases := []struct { - path string - expected string + path string + expectedLibID int }{ - {"/abc/file.mp3", "/abc/"}, - {"/ab/file.mp3", "/ab/"}, - {"/a/file.mp3", "/a/"}, + {"/abc/file.mp3", 3}, + {"/ab/file.mp3", 2}, + {"/a/file.mp3", 1}, } for _, tc := range testCases { - matched := libRegex.FindString(tc.path) - Expect(matched).To(Equal(tc.expected), "Path %s should match %s", tc.path, tc.expected) + libID, _ := matcher.findLibraryForPath(tc.path) + Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) } }) }) diff --git a/core/playlists_test.go b/core/playlists_test.go index b9886bc86..dfcd62d63 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -265,6 +265,71 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks[0].Path).To(Equal("rock.mp3")) // From music library Expect(pls.Tracks[1].Path).To(Equal("bach.mp3")) // From music-classical library (not music!) }) + + It("correctly handles identical relative paths from different libraries", func() { + // This tests the bug where two libraries have files at the same relative path + // and only one appears in the playlist + tmpDir := GinkgoT().TempDir() + musicDir := tmpDir + "/music" + classicalDir := tmpDir + "/classical" + Expect(os.Mkdir(musicDir, 0755)).To(Succeed()) + Expect(os.Mkdir(classicalDir, 0755)).To(Succeed()) + Expect(os.MkdirAll(musicDir+"/album", 0755)).To(Succeed()) + Expect(os.MkdirAll(classicalDir+"/album", 0755)).To(Succeed()) + // Create placeholder files so paths resolve correctly + Expect(os.WriteFile(musicDir+"/album/track.mp3", []byte{}, 0600)).To(Succeed()) + Expect(os.WriteFile(classicalDir+"/album/track.mp3", []byte{}, 0600)).To(Succeed()) + + // Both libraries have a file at "album/track.mp3" + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: musicDir}, + {ID: 2, Path: classicalDir}, + }) + + // Mock returns files with same relative path but different IDs and library IDs + // Keys use the library-qualified format: "libraryID:path" + ds.MockedMediaFile = &mockedMediaFileRepo{ + data: map[string]model.MediaFile{ + "1:album/track.mp3": {ID: "music-track", Path: "album/track.mp3", LibraryID: 1, Title: "Rock Song"}, + "2:album/track.mp3": {ID: "classical-track", Path: "album/track.mp3", LibraryID: 2, Title: "Classical Piece"}, + }, + } + // Recreate playlists service to pick up new mock + ps = core.NewPlaylists(ds) + + // Create playlist in music library that references both tracks + plsContent := "#PLAYLIST:Same Path Test\nalbum/track.mp3\n../classical/album/track.mp3" + plsFile := musicDir + "/test.m3u" + Expect(os.WriteFile(plsFile, []byte(plsContent), 0600)).To(Succeed()) + + plsFolder := &model.Folder{ + ID: "1", + LibraryID: 1, + LibraryPath: musicDir, + Path: "", + Name: "", + } + + pls, err := ps.ImportFile(ctx, plsFolder, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + + // Should have BOTH tracks, not just one + Expect(pls.Tracks).To(HaveLen(2), "Playlist should contain both tracks with same relative path") + + // Verify we got tracks from DIFFERENT libraries (the key fix!) + // Collect the library IDs + libIDs := make(map[int]bool) + for _, track := range pls.Tracks { + libIDs[track.LibraryID] = true + } + Expect(libIDs).To(HaveLen(2), "Tracks should come from two different libraries") + Expect(libIDs[1]).To(BeTrue(), "Should have track from library 1") + Expect(libIDs[2]).To(BeTrue(), "Should have track from library 2") + + // Both tracks should have the same relative path + Expect(pls.Tracks[0].Path).To(Equal("album/track.mp3")) + Expect(pls.Tracks[1].Path).To(Equal("album/track.mp3")) + }) }) }) @@ -431,17 +496,43 @@ var _ = Describe("Playlists", func() { }) }) -// mockedMediaFileRepo's FindByPaths method returns a list of MediaFiles with the same paths as the input +// mockedMediaFileRepo's FindByPaths method mimics the real implementation. +// If data map is provided, it looks up files using the path as the key. +// Otherwise, it creates MediaFile entries from the input paths. type mockedMediaFileRepo struct { model.MediaFileRepository + data map[string]model.MediaFile } func (r *mockedMediaFileRepo) FindByPaths(paths []string) (model.MediaFiles, error) { var mfs model.MediaFiles + + // If data map is provided, use it to look up files + if r.data != nil { + for _, path := range paths { + // Look up by the path key (test should use "path:libraryID" format for keys) + if mf, ok := r.data[path]; ok { + mfs = append(mfs, mf) + } + } + return mfs, nil + } + + // Fallback: create MediaFile entries from paths + // Parse library-qualified format: "libraryID:path" for idx, path := range paths { + libraryID := 1 // Default library ID + actualPath := path + if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { + if libID, err := strconv.Atoi(parts[0]); err == nil { + libraryID = libID + actualPath = parts[1] + } + } mfs = append(mfs, model.MediaFile{ - ID: strconv.Itoa(idx), - Path: path, + ID: strconv.Itoa(idx), + Path: actualPath, + LibraryID: libraryID, }) } return mfs, nil @@ -453,13 +544,46 @@ type mockedMediaFileFromListRepo struct { data []string } -func (r *mockedMediaFileFromListRepo) FindByPaths([]string) (model.MediaFiles, error) { +func (r *mockedMediaFileFromListRepo) FindByPaths(paths []string) (model.MediaFiles, error) { var mfs model.MediaFiles + + // Build a map of requested paths with their library IDs (handle both qualified and unqualified) + type pathRequest struct { + path string + libraryID int + qualified bool + } + requests := make([]pathRequest, 0, len(paths)) + for _, path := range paths { + req := pathRequest{path: path, libraryID: 1, qualified: false} + if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { + if libID, err := strconv.Atoi(parts[0]); err == nil { + req.libraryID = libID + req.path = parts[1] + req.qualified = true + } + } + requests = append(requests, req) + } + + // Return files that match the requests + // Need to use normalized comparison to handle Unicode normalization differences + normalizeForComparison := func(s string) string { + return strings.ToLower(norm.NFC.String(s)) + } + for idx, path := range r.data { - mfs = append(mfs, model.MediaFile{ - ID: strconv.Itoa(idx), - Path: path, - }) + normalizedPath := normalizeForComparison(path) + for _, req := range requests { + if normalizeForComparison(req.path) == normalizedPath { + mfs = append(mfs, model.MediaFile{ + ID: strconv.Itoa(idx), + Path: path, + LibraryID: req.libraryID, + }) + break + } + } } return mfs, nil } diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index e7883947a..0253c0fcd 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "slices" + "strconv" + "strings" "sync" "time" @@ -193,11 +195,40 @@ func (r *mediaFileRepository) GetCursor(options ...model.QueryOptions) (model.Me } func (r *mediaFileRepository) FindByPaths(paths []string) (model.MediaFiles, error) { - sel := r.newSelect().Columns("*").Where(Eq{"path collate nocase": paths}) + // Parse library-qualified paths and build query + // Format: "libraryID:path" or just "path" (backward compatibility) + query := Or{} + + for _, path := range paths { + parts := strings.SplitN(path, ":", 2) + if len(parts) == 2 { + // Library-qualified path: "libraryID:path" + libraryID, err := strconv.Atoi(parts[0]) + if err != nil { + // Invalid format, skip + continue + } + relativePath := parts[1] + query = append(query, And{ + Eq{"path collate nocase": relativePath}, + Eq{"library_id": libraryID}, + }) + } else { + // Unqualified path: search across all libraries + query = append(query, Eq{"path collate nocase": path}) + } + } + + if len(query) == 0 { + return model.MediaFiles{}, nil + } + + sel := r.newSelect().Columns("*").Where(query) var res dbMediaFiles if err := r.queryAll(sel, &res); err != nil { return nil, err } + return res.toModels(), nil } From 05d841e9dd6c744312fcf20898d0ab0559916f0b Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 13:47:52 -0500 Subject: [PATCH 07/14] refactor: simplify mocks Signed-off-by: Deluan --- core/playlists_test.go | 85 ++++++++++++------------------------------ 1 file changed, 23 insertions(+), 62 deletions(-) diff --git a/core/playlists_test.go b/core/playlists_test.go index dfcd62d63..fb2082e2f 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -16,7 +16,6 @@ import ( "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "golang.org/x/text/unicode/norm" ) var _ = Describe("Playlists", func() { @@ -425,28 +424,6 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks[0].Path).To(Equal("abc/tEsT1.Mp3")) }) - It("handles Unicode normalization when comparing paths", func() { - // Test case for Apple Music playlists that use NFC encoding vs macOS filesystem NFD - // The character "è" can be represented as NFC (single codepoint) or NFD (e + combining accent) - - const pathWithAccents = "artist/Michèle Desrosiers/album/Noël.m4a" - - // Simulate a database entry with NFD encoding (as stored by macOS filesystem) - nfdPath := norm.NFD.String(pathWithAccents) - repo.data = []string{nfdPath} - - // Simulate an Apple Music M3U playlist entry with NFC encoding - nfcPath := norm.NFC.String("/music/" + pathWithAccents) - m3u := strings.Join([]string{ - nfcPath, - }, "\n") - f := strings.NewReader(m3u) - - pls, err := ps.ImportM3U(ctx, f) - Expect(err).ToNot(HaveOccurred()) - Expect(pls.Tracks).To(HaveLen(1), "Should find the track despite Unicode normalization differences") - Expect(pls.Tracks[0].Path).To(Equal(nfdPath)) - }) }) Describe("InPlaylistsPath", func() { @@ -496,9 +473,8 @@ var _ = Describe("Playlists", func() { }) }) -// mockedMediaFileRepo's FindByPaths method mimics the real implementation. -// If data map is provided, it looks up files using the path as the key. -// Otherwise, it creates MediaFile entries from the input paths. +// mockedMediaFileRepo's FindByPaths method returns MediaFiles for the given paths. +// If data map is provided, looks up files by key; otherwise creates them from paths. type mockedMediaFileRepo struct { model.MediaFileRepository data map[string]model.MediaFile @@ -507,10 +483,9 @@ type mockedMediaFileRepo struct { func (r *mockedMediaFileRepo) FindByPaths(paths []string) (model.MediaFiles, error) { var mfs model.MediaFiles - // If data map is provided, use it to look up files + // If data map provided, look up files if r.data != nil { for _, path := range paths { - // Look up by the path key (test should use "path:libraryID" format for keys) if mf, ok := r.data[path]; ok { mfs = append(mfs, mf) } @@ -518,17 +493,18 @@ func (r *mockedMediaFileRepo) FindByPaths(paths []string) (model.MediaFiles, err return mfs, nil } - // Fallback: create MediaFile entries from paths - // Parse library-qualified format: "libraryID:path" + // Otherwise, create MediaFiles from paths for idx, path := range paths { - libraryID := 1 // Default library ID + // Strip library qualifier if present (format: "libraryID:path") actualPath := path + libraryID := 1 if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { - if libID, err := strconv.Atoi(parts[0]); err == nil { - libraryID = libID + if id, err := strconv.Atoi(parts[0]); err == nil { + libraryID = id actualPath = parts[1] } } + mfs = append(mfs, model.MediaFile{ ID: strconv.Itoa(idx), Path: actualPath, @@ -547,39 +523,24 @@ type mockedMediaFileFromListRepo struct { func (r *mockedMediaFileFromListRepo) FindByPaths(paths []string) (model.MediaFiles, error) { var mfs model.MediaFiles - // Build a map of requested paths with their library IDs (handle both qualified and unqualified) - type pathRequest struct { - path string - libraryID int - qualified bool - } - requests := make([]pathRequest, 0, len(paths)) - for _, path := range paths { - req := pathRequest{path: path, libraryID: 1, qualified: false} - if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { - if libID, err := strconv.Atoi(parts[0]); err == nil { - req.libraryID = libID - req.path = parts[1] - req.qualified = true + for idx, dataPath := range r.data { + for _, requestPath := range paths { + // Strip library qualifier if present (format: "libraryID:path") + actualPath := requestPath + libraryID := 1 + if parts := strings.SplitN(requestPath, ":", 2); len(parts) == 2 { + if id, err := strconv.Atoi(parts[0]); err == nil { + libraryID = id + actualPath = parts[1] + } } - } - requests = append(requests, req) - } - // Return files that match the requests - // Need to use normalized comparison to handle Unicode normalization differences - normalizeForComparison := func(s string) string { - return strings.ToLower(norm.NFC.String(s)) - } - - for idx, path := range r.data { - normalizedPath := normalizeForComparison(path) - for _, req := range requests { - if normalizeForComparison(req.path) == normalizedPath { + // Case-insensitive comparison (like SQL's "collate nocase") + if strings.EqualFold(actualPath, dataPath) { mfs = append(mfs, model.MediaFile{ ID: strconv.Itoa(idx), - Path: path, - LibraryID: req.libraryID, + Path: dataPath, + LibraryID: libraryID, }) break } From 8bd0112c79c0f7e3609890d2e68e6a9cf20ecdc5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 14:51:02 -0500 Subject: [PATCH 08/14] fix: lint Signed-off-by: Deluan --- core/playlists.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index 983ff6429..d712aed25 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -195,12 +195,12 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m } filteredLines = append(filteredLines, line) } - paths, err := s.normalizePaths(ctx, pls, folder, filteredLines) + pathsWithLibrary, err := s.normalizePaths(ctx, pls, folder, filteredLines) if err != nil { log.Warn(ctx, "Error normalizing paths in playlist", "playlist", pls.Name, err) continue } - found, err := mediaFileRepository.FindByPaths(paths) + found, err := mediaFileRepository.FindByPaths(pathsWithLibrary) if err != nil { log.Warn(ctx, "Error reading files from DB", "playlist", pls.Name, err) continue @@ -212,16 +212,11 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m key := fmt.Sprintf("%d:%s", found[idx].LibraryID, normalizePathForComparison(found[idx].Path)) existing[key] = idx } - for _, path := range paths { + for _, path := range pathsWithLibrary { // Parse the library-qualified path - normalizedPath := path - if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { - // Path is already qualified: "libraryID:path" - normalizedPath = parts[0] + ":" + normalizePathForComparison(parts[1]) - } else { - // Unqualified path (backward compatibility) - normalizedPath = normalizePathForComparison(path) - } + parts := strings.SplitN(path, ":", 2) + // Path is already qualified: "libraryID:path" + normalizedPath := parts[0] + ":" + normalizePathForComparison(parts[1]) idx, ok := existing[normalizedPath] if ok { From 9e5882729c177206407ffae84017d6b544f8f9be Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 15:37:44 -0500 Subject: [PATCH 09/14] fix: improve path resolution for cross-library playlists and enhance error handling Signed-off-by: Deluan --- core/playlists.go | 141 +++++++++-------- core/playlists_internal_test.go | 260 ++++++++++++++++++++++++++++++-- 2 files changed, 328 insertions(+), 73 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index d712aed25..4ec5d376a 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -195,12 +195,13 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m } filteredLines = append(filteredLines, line) } - pathsWithLibrary, err := s.normalizePaths(ctx, pls, folder, filteredLines) + resolvedPaths, err := s.resolvePaths(ctx, folder, filteredLines) if err != nil { - log.Warn(ctx, "Error normalizing paths in playlist", "playlist", pls.Name, err) + log.Warn(ctx, "Error resolving paths in playlist", "playlist", pls.Name, err) continue } - found, err := mediaFileRepository.FindByPaths(pathsWithLibrary) + + found, err := mediaFileRepository.FindByPaths(resolvedPaths) if err != nil { log.Warn(ctx, "Error reading files from DB", "playlist", pls.Name, err) continue @@ -212,7 +213,7 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m key := fmt.Sprintf("%d:%s", found[idx].LibraryID, normalizePathForComparison(found[idx].Path)) existing[key] = idx } - for _, path := range pathsWithLibrary { + for _, path := range resolvedPaths { // Parse the library-qualified path parts := strings.SplitN(path, ":", 2) // Path is already qualified: "libraryID:path" @@ -250,8 +251,9 @@ type pathResolution struct { valid bool } -// toLibraryQualifiedPath converts the absolute path to a library-qualified path format: "libraryID:relativePath". -func (r pathResolution) toLibraryQualifiedPath() (string, error) { +// ToQualifiedString converts the path resolution to a library-qualified string with forward slashes. +// Format: "libraryID:relativePath" with forward slashes for path separators. +func (r pathResolution) ToQualifiedString() (string, error) { if !r.valid { return "", fmt.Errorf("invalid path resolution") } @@ -259,17 +261,8 @@ func (r pathResolution) toLibraryQualifiedPath() (string, error) { if err != nil { return "", err } - return fmt.Sprintf("%d:%s", r.libraryID, relativePath), nil -} - -// newValidResolution creates a valid path resolution. -func newValidResolution(absolutePath, libraryPath string, libraryID int) pathResolution { - return pathResolution{ - absolutePath: absolutePath, - libraryPath: libraryPath, - libraryID: libraryID, - valid: true, - } + // Convert path separators to forward slashes + return fmt.Sprintf("%d:%s", r.libraryID, filepath.ToSlash(relativePath)), nil } // libraryMatcher holds sorted libraries with cleaned paths for efficient path matching. @@ -315,99 +308,121 @@ func newLibraryMatcher(libs model.Libraries) *libraryMatcher { } } -// normalizePaths converts playlist file paths to library-qualified paths (format: "libraryID:relativePath"). -// For relative paths, it resolves them to absolute paths first, then determines which -// library they belong to. This allows playlists to reference files across library boundaries. -func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, folder *model.Folder, lines []string) ([]string, error) { - matcher, err := s.buildLibraryMatcher(ctx) +// pathResolver handles path resolution logic for playlist imports. +type pathResolver struct { + matcher *libraryMatcher +} + +// newPathResolver creates a pathResolver with libraries loaded from the datastore. +func newPathResolver(ctx context.Context, ds model.DataStore) (*pathResolver, error) { + matcher, err := buildLibraryMatcher(ctx, ds) if err != nil { return nil, err } + return &pathResolver{matcher: matcher}, nil +} - res := make([]string, 0, len(lines)) - for idx, line := range lines { - resolution := s.resolvePath(line, folder, matcher) - - if !resolution.valid { - log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) - continue - } - - qualifiedPath, err := resolution.toLibraryQualifiedPath() - if err != nil { - log.Debug(ctx, "Error getting library-qualified path", "playlist", pls.Name, "path", line, - "libPath", resolution.libraryPath, "filePath", resolution.absolutePath, err) - continue - } - - res = append(res, qualifiedPath) +// buildLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). +func buildLibraryMatcher(ctx context.Context, ds model.DataStore) (*libraryMatcher, error) { + libs, err := ds.Library(ctx).GetAll() + if err != nil { + return nil, err } - // Convert path separators to forward slashes (only the path part, not the library ID prefix) - return slice.Map(res, func(qp string) string { - parts := strings.SplitN(qp, ":", 2) - if len(parts) == 2 { - return parts[0] + ":" + filepath.ToSlash(parts[1]) - } - return filepath.ToSlash(qp) - }), nil + return newLibraryMatcher(libs), nil } // resolvePath determines the absolute path and library path for a playlist entry. -func (s *playlists) resolvePath(line string, folder *model.Folder, matcher *libraryMatcher) pathResolution { +func (r *pathResolver) resolvePath(line string, folder *model.Folder) pathResolution { if folder != nil && !filepath.IsAbs(line) { - return s.resolveRelativePath(line, folder, matcher) + return r.resolveRelativePath(line, folder) } - return s.resolveAbsolutePath(line, matcher) + return r.resolveAbsolutePath(line) } // resolveRelativePath handles relative paths by converting them to absolute paths // and finding their library location. This enables cross-library playlist references. -func (s *playlists) resolveRelativePath(line string, folder *model.Folder, matcher *libraryMatcher) pathResolution { +func (r *pathResolver) resolveRelativePath(line string, folder *model.Folder) pathResolution { // Step 1: Resolve relative path to absolute path based on playlist location // Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" // resolves to /music/songs/abc.mp3 absolutePath := filepath.Clean(filepath.Join(folder.AbsolutePath(), line)) // Step 2: Determine which library this absolute path belongs to - libID, libPath := matcher.findLibraryForPath(absolutePath) + libID, libPath := r.matcher.findLibraryForPath(absolutePath) if libID != 0 { - return newValidResolution(absolutePath, libPath, libID) + return pathResolution{ + absolutePath: absolutePath, + libraryPath: libPath, + libraryID: libID, + valid: true, + } } // Fallback: Check if it's in the playlist's own library - return s.validatePathInLibrary(absolutePath, folder.LibraryPath, folder.LibraryID) + return r.validatePathInLibrary(absolutePath, folder.LibraryPath, folder.LibraryID) } // resolveAbsolutePath handles absolute paths by matching them against library paths. -func (s *playlists) resolveAbsolutePath(line string, matcher *libraryMatcher) pathResolution { +func (r *pathResolver) resolveAbsolutePath(line string) pathResolution { cleanPath := filepath.Clean(line) - libID, libPath := matcher.findLibraryForPath(cleanPath) + libID, libPath := r.matcher.findLibraryForPath(cleanPath) if libID == 0 { return pathResolution{valid: false} } - return newValidResolution(cleanPath, libPath, libID) + return pathResolution{ + absolutePath: cleanPath, + libraryPath: libPath, + libraryID: libID, + valid: true, + } } // validatePathInLibrary verifies that an absolute path belongs to the specified library. // It rejects paths that escape the library using ".." segments. -func (s *playlists) validatePathInLibrary(absolutePath, libraryPath string, libraryID int) pathResolution { +func (r *pathResolver) validatePathInLibrary(absolutePath, libraryPath string, libraryID int) pathResolution { rel, err := filepath.Rel(libraryPath, absolutePath) if err != nil || strings.HasPrefix(rel, "..") { return pathResolution{valid: false} } - return newValidResolution(absolutePath, libraryPath, libraryID) + return pathResolution{ + absolutePath: absolutePath, + libraryPath: libraryPath, + libraryID: libraryID, + valid: true, + } } -// buildLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). -func (s *playlists) buildLibraryMatcher(ctx context.Context) (*libraryMatcher, error) { - libs, err := s.ds.Library(ctx).GetAll() +// resolvePaths converts playlist file paths to library-qualified paths (format: "libraryID:relativePath"). +// For relative paths, it resolves them to absolute paths first, then determines which +// library they belong to. This allows playlists to reference files across library boundaries. +func (s *playlists) resolvePaths(ctx context.Context, folder *model.Folder, lines []string) ([]string, error) { + resolver, err := newPathResolver(ctx, s.ds) if err != nil { return nil, err } - return newLibraryMatcher(libs), nil + results := make([]string, 0, len(lines)) + for idx, line := range lines { + resolution := resolver.resolvePath(line, folder) + + if !resolution.valid { + log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) + continue + } + + qualifiedPath, err := resolution.ToQualifiedString() + if err != nil { + log.Debug(ctx, "Error getting library-qualified path", "path", line, + "libPath", resolution.libraryPath, "filePath", resolution.absolutePath, err) + continue + } + + results = append(results, qualifiedPath) + } + + return results, nil } func (s *playlists) updatePlaylist(ctx context.Context, newPls *model.Playlist) error { diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index 6ed57a525..67abd0f8b 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -13,7 +13,6 @@ import ( var _ = Describe("buildLibraryMatcher", func() { var ds *tests.MockDataStore var mockLibRepo *tests.MockLibraryRepo - var ps *playlists ctx := context.Background() BeforeEach(func() { @@ -21,7 +20,6 @@ var _ = Describe("buildLibraryMatcher", func() { ds = &tests.MockDataStore{ MockedLibrary: mockLibRepo, } - ps = &playlists{ds: ds} }) Describe("Longest library path matching", func() { @@ -33,7 +31,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 3, Path: "/music-classical/opera"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) // Test that longest path matches first and returns correct library ID @@ -61,7 +59,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 2, Path: "/home/user/music-backup"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) // Test that music-backup library is matched correctly @@ -81,7 +79,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 2, Path: "/music-classical"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) // Exact library path should match @@ -98,7 +96,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 4, Path: "/media/audio/classical/baroque"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) testCases := []struct { @@ -124,7 +122,7 @@ var _ = Describe("buildLibraryMatcher", func() { It("handles empty library list", func() { mockLibRepo.SetData([]model.Library{}) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) Expect(matcher).ToNot(BeNil()) @@ -139,7 +137,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 1, Path: "/music"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) libID, libPath := matcher.findLibraryForPath("/music/track.mp3") @@ -153,7 +151,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 2, Path: "/music(backup)"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) Expect(matcher).ToNot(BeNil()) @@ -172,7 +170,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 3, Path: "/abc"}, }) - matcher, err := ps.buildLibraryMatcher(ctx) + matcher, err := buildLibraryMatcher(ctx, ds) Expect(err).ToNot(HaveOccurred()) // Verify that longer paths match correctly (not cut off by shorter prefix) @@ -193,6 +191,248 @@ var _ = Describe("buildLibraryMatcher", func() { }) }) +var _ = Describe("pathResolver", func() { + var ds *tests.MockDataStore + var mockLibRepo *tests.MockLibraryRepo + var resolver *pathResolver + ctx := context.Background() + + BeforeEach(func() { + mockLibRepo = &tests.MockLibraryRepo{} + ds = &tests.MockDataStore{ + MockedLibrary: mockLibRepo, + } + + // Setup test libraries + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + {ID: 2, Path: "/music-classical"}, + {ID: 3, Path: "/podcasts"}, + }) + + var err error + resolver, err = newPathResolver(ctx, ds) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("resolvePath", func() { + It("resolves absolute paths", func() { + resolution := resolver.resolvePath("/music/artist/album/track.mp3", nil) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.libraryPath).To(Equal("/music")) + Expect(resolution.absolutePath).To(Equal("/music/artist/album/track.mp3")) + }) + + It("resolves relative paths when folder is provided", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + resolution := resolver.resolvePath("../artist/album/track.mp3", folder) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.absolutePath).To(Equal("/music/artist/album/track.mp3")) + }) + + It("returns invalid resolution for paths outside any library", func() { + resolution := resolver.resolvePath("/outside/library/track.mp3", nil) + + Expect(resolution.valid).To(BeFalse()) + }) + }) + + Describe("resolveAbsolutePath", func() { + It("resolves path within a library", func() { + resolution := resolver.resolveAbsolutePath("/music/track.mp3") + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.libraryPath).To(Equal("/music")) + Expect(resolution.absolutePath).To(Equal("/music/track.mp3")) + }) + + It("resolves path to the longest matching library", func() { + resolution := resolver.resolveAbsolutePath("/music-classical/track.mp3") + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(2)) + Expect(resolution.libraryPath).To(Equal("/music-classical")) + }) + + It("returns invalid resolution for path outside libraries", func() { + resolution := resolver.resolveAbsolutePath("/videos/movie.mp4") + + Expect(resolution.valid).To(BeFalse()) + }) + + It("cleans the path before matching", func() { + resolution := resolver.resolveAbsolutePath("/music//artist/../artist/track.mp3") + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.absolutePath).To(Equal("/music/artist/track.mp3")) + }) + }) + + Describe("resolveRelativePath", func() { + It("resolves relative path within same library", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + resolution := resolver.resolveRelativePath("../songs/track.mp3", folder) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.absolutePath).To(Equal("/music/songs/track.mp3")) + }) + + It("resolves relative path to different library", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + // Path goes up and into a different library + resolution := resolver.resolveRelativePath("../../podcasts/episode.mp3", folder) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(3)) + Expect(resolution.libraryPath).To(Equal("/podcasts")) + }) + + It("uses matcher to find correct library for resolved path", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + // This relative path resolves to music-classical library + resolution := resolver.resolveRelativePath("../../music-classical/track.mp3", folder) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(2)) + Expect(resolution.libraryPath).To(Equal("/music-classical")) + }) + + It("returns invalid for relative paths escaping all libraries", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + resolution := resolver.resolveRelativePath("../../../../etc/passwd", folder) + + Expect(resolution.valid).To(BeFalse()) + }) + }) + + Describe("validatePathInLibrary", func() { + It("validates path within library", func() { + resolution := resolver.validatePathInLibrary("/music/artist/track.mp3", "/music", 1) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.libraryPath).To(Equal("/music")) + }) + + It("rejects path that escapes library using ..", func() { + resolution := resolver.validatePathInLibrary("/music/../etc/passwd", "/music", 1) + + Expect(resolution.valid).To(BeFalse()) + }) + + It("rejects path outside library", func() { + resolution := resolver.validatePathInLibrary("/podcasts/track.mp3", "/music", 1) + + Expect(resolution.valid).To(BeFalse()) + }) + + It("accepts path exactly at library root", func() { + resolution := resolver.validatePathInLibrary("/music", "/music", 1) + + Expect(resolution.valid).To(BeTrue()) + }) + }) + + Describe("Cross-library resolution scenarios", func() { + It("handles playlist in library A referencing file in library B", func() { + // Playlist is in /music/playlists + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + // Relative path that goes to /podcasts library + resolution := resolver.resolvePath("../../podcasts/show/episode.mp3", folder) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(3), "Should resolve to podcasts library") + Expect(resolution.libraryPath).To(Equal("/podcasts")) + }) + + It("prefers longer library paths when resolving", func() { + // Ensure /music-classical is matched instead of /music + resolution := resolver.resolvePath("/music-classical/baroque/track.mp3", nil) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(2), "Should match /music-classical, not /music") + }) + }) +}) + +var _ = Describe("pathResolution", func() { + Describe("ToQualifiedString", func() { + It("converts valid resolution to qualified string with forward slashes", func() { + resolution := pathResolution{ + absolutePath: "/music/artist/album/track.mp3", + libraryPath: "/music", + libraryID: 1, + valid: true, + } + + qualifiedStr, err := resolution.ToQualifiedString() + + Expect(err).ToNot(HaveOccurred()) + Expect(qualifiedStr).To(Equal("1:artist/album/track.mp3")) + }) + + It("handles Windows-style paths by converting to forward slashes", func() { + resolution := pathResolution{ + absolutePath: "/music/artist/album/track.mp3", + libraryPath: "/music", + libraryID: 2, + valid: true, + } + + qualifiedStr, err := resolution.ToQualifiedString() + + Expect(err).ToNot(HaveOccurred()) + // Should always use forward slashes regardless of OS + Expect(qualifiedStr).To(ContainSubstring("2:")) + Expect(qualifiedStr).ToNot(ContainSubstring("\\")) + }) + + It("returns error for invalid resolution", func() { + resolution := pathResolution{valid: false} + + _, err := resolution.ToQualifiedString() + + Expect(err).To(HaveOccurred()) + }) + }) +}) + var _ = Describe("normalizePathForComparison", func() { It("normalizes Unicode characters to NFC form and converts to lowercase", func() { // Test with NFD (decomposed) input - as would come from macOS filesystem From 4d55c1b7093360e8634e4af4d05e0c0b5a8b119a Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 16:54:07 -0500 Subject: [PATCH 10/14] refactor Signed-off-by: Deluan --- core/playlists.go | 12 ++---------- core/playlists_internal_test.go | 33 ++++++++++++++++----------------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index 4ec5d376a..e42dd15d2 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -315,20 +315,12 @@ type pathResolver struct { // newPathResolver creates a pathResolver with libraries loaded from the datastore. func newPathResolver(ctx context.Context, ds model.DataStore) (*pathResolver, error) { - matcher, err := buildLibraryMatcher(ctx, ds) - if err != nil { - return nil, err - } - return &pathResolver{matcher: matcher}, nil -} - -// buildLibraryMatcher creates a libraryMatcher with libraries sorted by path length (longest first). -func buildLibraryMatcher(ctx context.Context, ds model.DataStore) (*libraryMatcher, error) { libs, err := ds.Library(ctx).GetAll() if err != nil { return nil, err } - return newLibraryMatcher(libs), nil + matcher := newLibraryMatcher(libs) + return &pathResolver{matcher: matcher}, nil } // resolvePath determines the absolute path and library path for a playlist entry. diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index 67abd0f8b..922187531 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -10,7 +10,7 @@ import ( "golang.org/x/text/unicode/norm" ) -var _ = Describe("buildLibraryMatcher", func() { +var _ = Describe("libraryMatcher", func() { var ds *tests.MockDataStore var mockLibRepo *tests.MockLibraryRepo ctx := context.Background() @@ -22,6 +22,13 @@ var _ = Describe("buildLibraryMatcher", func() { } }) + // Helper function to create a libraryMatcher from the mock datastore + createMatcher := func(ds model.DataStore) *libraryMatcher { + libs, err := ds.Library(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + return newLibraryMatcher(libs) + } + Describe("Longest library path matching", func() { It("matches the longest library path when multiple libraries share a prefix", func() { // Setup libraries with prefix conflicts @@ -31,8 +38,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 3, Path: "/music-classical/opera"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) // Test that longest path matches first and returns correct library ID testCases := []struct { @@ -59,8 +65,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 2, Path: "/home/user/music-backup"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) // Test that music-backup library is matched correctly libID, libPath := matcher.findLibraryForPath("/home/user/music-backup/track.mp3") @@ -79,8 +84,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 2, Path: "/music-classical"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) // Exact library path should match libID, libPath := matcher.findLibraryForPath("/music-classical") @@ -96,8 +100,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 4, Path: "/media/audio/classical/baroque"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) testCases := []struct { path string @@ -122,8 +125,7 @@ var _ = Describe("buildLibraryMatcher", func() { It("handles empty library list", func() { mockLibRepo.SetData([]model.Library{}) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) Expect(matcher).ToNot(BeNil()) // Should not match anything @@ -137,8 +139,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 1, Path: "/music"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) libID, libPath := matcher.findLibraryForPath("/music/track.mp3") Expect(libID).To(Equal(1)) @@ -151,8 +152,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 2, Path: "/music(backup)"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) Expect(matcher).ToNot(BeNil()) // Special characters should match literally @@ -170,8 +170,7 @@ var _ = Describe("buildLibraryMatcher", func() { {ID: 3, Path: "/abc"}, }) - matcher, err := buildLibraryMatcher(ctx, ds) - Expect(err).ToNot(HaveOccurred()) + matcher := createMatcher(ds) // Verify that longer paths match correctly (not cut off by shorter prefix) testCases := []struct { From dfc69d8db06a6a32450c7ae70de7925df133a80a Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 17:11:07 -0500 Subject: [PATCH 11/14] refactor: remove unnecessary path validation fallback Remove validatePathInLibrary function and its fallback logic in resolveRelativePath. The library matcher should always find the correct library, including the playlist's own library. If this fails, we now return an invalid resolution instead of attempting a fallback validation. This simplifies the code by removing redundant validation logic that was masking test setup issues. Also fixes test mock configuration to properly set up library paths that match folder LibraryPath values. --- core/playlists.go | 36 +++++++++------------------------ core/playlists_internal_test.go | 28 ------------------------- core/playlists_test.go | 4 ++-- 3 files changed, 12 insertions(+), 56 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index e42dd15d2..164221697 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -341,17 +341,17 @@ func (r *pathResolver) resolveRelativePath(line string, folder *model.Folder) pa // Step 2: Determine which library this absolute path belongs to libID, libPath := r.matcher.findLibraryForPath(absolutePath) - if libID != 0 { - return pathResolution{ - absolutePath: absolutePath, - libraryPath: libPath, - libraryID: libID, - valid: true, - } + if libID == 0 { + // Path not found in any library - this should not happen as the playlist's + // own library should have been matched above + return pathResolution{valid: false} + } + return pathResolution{ + absolutePath: absolutePath, + libraryPath: libPath, + libraryID: libID, + valid: true, } - - // Fallback: Check if it's in the playlist's own library - return r.validatePathInLibrary(absolutePath, folder.LibraryPath, folder.LibraryID) } // resolveAbsolutePath handles absolute paths by matching them against library paths. @@ -370,22 +370,6 @@ func (r *pathResolver) resolveAbsolutePath(line string) pathResolution { } } -// validatePathInLibrary verifies that an absolute path belongs to the specified library. -// It rejects paths that escape the library using ".." segments. -func (r *pathResolver) validatePathInLibrary(absolutePath, libraryPath string, libraryID int) pathResolution { - rel, err := filepath.Rel(libraryPath, absolutePath) - if err != nil || strings.HasPrefix(rel, "..") { - return pathResolution{valid: false} - } - - return pathResolution{ - absolutePath: absolutePath, - libraryPath: libraryPath, - libraryID: libraryID, - valid: true, - } -} - // resolvePaths converts playlist file paths to library-qualified paths (format: "libraryID:relativePath"). // For relative paths, it resolves them to absolute paths first, then determines which // library they belong to. This allows playlists to reference files across library boundaries. diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index 922187531..b06a004d2 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -335,34 +335,6 @@ var _ = Describe("pathResolver", func() { }) }) - Describe("validatePathInLibrary", func() { - It("validates path within library", func() { - resolution := resolver.validatePathInLibrary("/music/artist/track.mp3", "/music", 1) - - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(1)) - Expect(resolution.libraryPath).To(Equal("/music")) - }) - - It("rejects path that escapes library using ..", func() { - resolution := resolver.validatePathInLibrary("/music/../etc/passwd", "/music", 1) - - Expect(resolution.valid).To(BeFalse()) - }) - - It("rejects path outside library", func() { - resolution := resolver.validatePathInLibrary("/podcasts/track.mp3", "/music", 1) - - Expect(resolution.valid).To(BeFalse()) - }) - - It("accepts path exactly at library root", func() { - resolution := resolver.validatePathInLibrary("/music", "/music", 1) - - Expect(resolution.valid).To(BeTrue()) - }) - }) - Describe("Cross-library resolution scenarios", func() { It("handles playlist in library A referencing file in library B", func() { // Playlist is in /music/playlists diff --git a/core/playlists_test.go b/core/playlists_test.go index fb2082e2f..59b027690 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -33,8 +33,6 @@ var _ = Describe("Playlists", func() { MockedLibrary: mockLibRepo, } ctx = request.WithUser(ctx, model.User{ID: "123"}) - // Path should be libPath, but we want to match the root folder referenced in the m3u, which is `/` - mockLibRepo.SetData([]model.Library{{ID: 1, Path: "/"}}) }) Describe("ImportFile", func() { @@ -43,6 +41,8 @@ var _ = Describe("Playlists", func() { ps = core.NewPlaylists(ds) ds.MockedMediaFile = &mockedMediaFileRepo{} libPath, _ := os.Getwd() + // Set up library with the actual library path that matches the folder + mockLibRepo.SetData([]model.Library{{ID: 1, Path: libPath}}) folder = &model.Folder{ ID: "1", LibraryID: 1, From 657b0ca1223d7a5ec958b116c92938cce2d96c9c Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 17:27:25 -0500 Subject: [PATCH 12/14] refactor: consolidate path resolution logic Collapse resolveRelativePath and resolveAbsolutePath into a unified resolvePath function, extracting common library matching logic into a new findInLibraries helper method. This eliminates duplicate code (~20 lines) while maintaining clear separation of concerns: resolvePath handles path normalization (relative vs absolute), and findInLibraries handles library matching. Update tests to call resolvePath directly with appropriate parameters, maintaining full test coverage for both absolute and relative path scenarios. Signed-off-by: Deluan --- core/playlists.go | 46 ++++------ core/playlists_internal_test.go | 148 ++++++++++++++++---------------- 2 files changed, 92 insertions(+), 102 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index 164221697..3cd8f3d18 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -324,26 +324,30 @@ func newPathResolver(ctx context.Context, ds model.DataStore) (*pathResolver, er } // resolvePath determines the absolute path and library path for a playlist entry. +// For absolute paths, it uses them directly. +// For relative paths, it resolves them relative to the playlist's folder location. +// Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" +// +// resolves to /music/songs/abc.mp3 func (r *pathResolver) resolvePath(line string, folder *model.Folder) pathResolution { + var absolutePath string if folder != nil && !filepath.IsAbs(line) { - return r.resolveRelativePath(line, folder) + // Resolve relative path to absolute path based on playlist location + absolutePath = filepath.Clean(filepath.Join(folder.AbsolutePath(), line)) + } else { + // Use absolute path directly after cleaning + absolutePath = filepath.Clean(line) } - return r.resolveAbsolutePath(line) + + return r.findInLibraries(absolutePath) } -// resolveRelativePath handles relative paths by converting them to absolute paths -// and finding their library location. This enables cross-library playlist references. -func (r *pathResolver) resolveRelativePath(line string, folder *model.Folder) pathResolution { - // Step 1: Resolve relative path to absolute path based on playlist location - // Example: playlist at /music/playlists/test.m3u with line "../songs/abc.mp3" - // resolves to /music/songs/abc.mp3 - absolutePath := filepath.Clean(filepath.Join(folder.AbsolutePath(), line)) - - // Step 2: Determine which library this absolute path belongs to +// findInLibraries matches an absolute path against all known libraries and returns +// a pathResolution with the library information. Returns an invalid resolution if +// the path is not found in any library. +func (r *pathResolver) findInLibraries(absolutePath string) pathResolution { libID, libPath := r.matcher.findLibraryForPath(absolutePath) if libID == 0 { - // Path not found in any library - this should not happen as the playlist's - // own library should have been matched above return pathResolution{valid: false} } return pathResolution{ @@ -354,22 +358,6 @@ func (r *pathResolver) resolveRelativePath(line string, folder *model.Folder) pa } } -// resolveAbsolutePath handles absolute paths by matching them against library paths. -func (r *pathResolver) resolveAbsolutePath(line string) pathResolution { - cleanPath := filepath.Clean(line) - libID, libPath := r.matcher.findLibraryForPath(cleanPath) - - if libID == 0 { - return pathResolution{valid: false} - } - return pathResolution{ - absolutePath: cleanPath, - libraryPath: libPath, - libraryID: libID, - valid: true, - } -} - // resolvePaths converts playlist file paths to library-qualified paths (format: "libraryID:relativePath"). // For relative paths, it resolves them to absolute paths first, then determines which // library they belong to. This allows playlists to reference files across library boundaries. diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index b06a004d2..e5abb225d 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -245,93 +245,95 @@ var _ = Describe("pathResolver", func() { }) }) - Describe("resolveAbsolutePath", func() { - It("resolves path within a library", func() { - resolution := resolver.resolveAbsolutePath("/music/track.mp3") + Describe("resolvePath", func() { + Context("With absolute paths", func() { + It("resolves path within a library", func() { + resolution := resolver.resolvePath("/music/track.mp3", nil) - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(1)) - Expect(resolution.libraryPath).To(Equal("/music")) - Expect(resolution.absolutePath).To(Equal("/music/track.mp3")) + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.libraryPath).To(Equal("/music")) + Expect(resolution.absolutePath).To(Equal("/music/track.mp3")) + }) + + It("resolves path to the longest matching library", func() { + resolution := resolver.resolvePath("/music-classical/track.mp3", nil) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(2)) + Expect(resolution.libraryPath).To(Equal("/music-classical")) + }) + + It("returns invalid resolution for path outside libraries", func() { + resolution := resolver.resolvePath("/videos/movie.mp4", nil) + + Expect(resolution.valid).To(BeFalse()) + }) + + It("cleans the path before matching", func() { + resolution := resolver.resolvePath("/music//artist/../artist/track.mp3", nil) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.absolutePath).To(Equal("/music/artist/track.mp3")) + }) }) - It("resolves path to the longest matching library", func() { - resolution := resolver.resolveAbsolutePath("/music-classical/track.mp3") + Context("With relative paths", func() { + It("resolves relative path within same library", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(2)) - Expect(resolution.libraryPath).To(Equal("/music-classical")) - }) + resolution := resolver.resolvePath("../songs/track.mp3", folder) - It("returns invalid resolution for path outside libraries", func() { - resolution := resolver.resolveAbsolutePath("/videos/movie.mp4") + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.absolutePath).To(Equal("/music/songs/track.mp3")) + }) - Expect(resolution.valid).To(BeFalse()) - }) + It("resolves relative path to different library", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } - It("cleans the path before matching", func() { - resolution := resolver.resolveAbsolutePath("/music//artist/../artist/track.mp3") + // Path goes up and into a different library + resolution := resolver.resolvePath("../../podcasts/episode.mp3", folder) - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.absolutePath).To(Equal("/music/artist/track.mp3")) - }) - }) + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(3)) + Expect(resolution.libraryPath).To(Equal("/podcasts")) + }) - Describe("resolveRelativePath", func() { - It("resolves relative path within same library", func() { - folder := &model.Folder{ - Path: "playlists", - LibraryPath: "/music", - LibraryID: 1, - } + It("uses matcher to find correct library for resolved path", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } - resolution := resolver.resolveRelativePath("../songs/track.mp3", folder) + // This relative path resolves to music-classical library + resolution := resolver.resolvePath("../../music-classical/track.mp3", folder) - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(1)) - Expect(resolution.absolutePath).To(Equal("/music/songs/track.mp3")) - }) + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(2)) + Expect(resolution.libraryPath).To(Equal("/music-classical")) + }) - It("resolves relative path to different library", func() { - folder := &model.Folder{ - Path: "playlists", - LibraryPath: "/music", - LibraryID: 1, - } + It("returns invalid for relative paths escaping all libraries", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } - // Path goes up and into a different library - resolution := resolver.resolveRelativePath("../../podcasts/episode.mp3", folder) + resolution := resolver.resolvePath("../../../../etc/passwd", folder) - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(3)) - Expect(resolution.libraryPath).To(Equal("/podcasts")) - }) - - It("uses matcher to find correct library for resolved path", func() { - folder := &model.Folder{ - Path: "playlists", - LibraryPath: "/music", - LibraryID: 1, - } - - // This relative path resolves to music-classical library - resolution := resolver.resolveRelativePath("../../music-classical/track.mp3", folder) - - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(2)) - Expect(resolution.libraryPath).To(Equal("/music-classical")) - }) - - It("returns invalid for relative paths escaping all libraries", func() { - folder := &model.Folder{ - Path: "playlists", - LibraryPath: "/music", - LibraryID: 1, - } - - resolution := resolver.resolveRelativePath("../../../../etc/passwd", folder) - - Expect(resolution.valid).To(BeFalse()) + Expect(resolution.valid).To(BeFalse()) + }) }) }) From d327f81fe0a9587eff7b5e97df0a77e6a174750f Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 17:32:12 -0500 Subject: [PATCH 13/14] docs: add FindByPaths comment Signed-off-by: Deluan --- persistence/mediafile_repository.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 0253c0fcd..04c1b9b37 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -194,9 +194,11 @@ func (r *mediaFileRepository) GetCursor(options ...model.QueryOptions) (model.Me }, nil } +// FindByPaths finds media files by their paths. +// The paths can be library-qualified (format: "libraryID:path") or unqualified ("path"). +// Library-qualified paths search within the specified library, while unqualified paths +// search across all libraries for backward compatibility. func (r *mediaFileRepository) FindByPaths(paths []string) (model.MediaFiles, error) { - // Parse library-qualified paths and build query - // Format: "libraryID:path" or just "path" (backward compatibility) query := Or{} for _, path := range paths { From 348f700a3b409b3f623cc7ee90a2d8ee1b553e28 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 17 Nov 2025 20:15:52 -0500 Subject: [PATCH 14/14] fix: enhance Unicode normalization for path comparisons in playlists. Fixes 4663 Signed-off-by: Deluan --- core/playlists.go | 29 +++++++++++++---------------- core/playlists_internal_test.go | 26 -------------------------- core/playlists_test.go | 29 +++++++++++++++++++++++++++-- 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/core/playlists.go b/core/playlists.go index 3cd8f3d18..ed90cc23b 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -201,25 +201,29 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m continue } + // Normalize to NFD for filesystem compatibility (macOS). Database stores paths in NFD. + // See https://github.com/navidrome/navidrome/issues/4663 + resolvedPaths = slice.Map(resolvedPaths, func(path string) string { + return strings.ToLower(norm.NFD.String(path)) + }) + found, err := mediaFileRepository.FindByPaths(resolvedPaths) if err != nil { log.Warn(ctx, "Error reading files from DB", "playlist", pls.Name, err) continue } - // Build lookup map with library-qualified keys + // Build lookup map with library-qualified keys, normalized for comparison existing := make(map[string]int, len(found)) for idx := range found { - // Key format: "libraryID:path" (normalized) - key := fmt.Sprintf("%d:%s", found[idx].LibraryID, normalizePathForComparison(found[idx].Path)) + // Normalize to lowercase for case-insensitive comparison + // Key format: "libraryID:path" + key := fmt.Sprintf("%d:%s", found[idx].LibraryID, strings.ToLower(found[idx].Path)) existing[key] = idx } - for _, path := range resolvedPaths { - // Parse the library-qualified path - parts := strings.SplitN(path, ":", 2) - // Path is already qualified: "libraryID:path" - normalizedPath := parts[0] + ":" + normalizePathForComparison(parts[1]) - idx, ok := existing[normalizedPath] + // Find media files in the order of the resolved paths, to keep playlist order + for _, path := range resolvedPaths { + idx, ok := existing[path] if ok { mfs = append(mfs, found[idx]) } else { @@ -236,13 +240,6 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m return nil } -// normalizePathForComparison normalizes a file path to NFC form and converts to lowercase -// for consistent comparison. This fixes Unicode normalization issues on macOS where -// Apple Music creates playlists with NFC-encoded paths but the filesystem uses NFD. -func normalizePathForComparison(path string) string { - return strings.ToLower(norm.NFC.String(path)) -} - // pathResolution holds the result of resolving a playlist path to a library-relative path. type pathResolution struct { absolutePath string diff --git a/core/playlists_internal_test.go b/core/playlists_internal_test.go index e5abb225d..88e36cc3a 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists_internal_test.go @@ -7,7 +7,6 @@ import ( "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "golang.org/x/text/unicode/norm" ) var _ = Describe("libraryMatcher", func() { @@ -405,28 +404,3 @@ var _ = Describe("pathResolution", func() { }) }) }) - -var _ = Describe("normalizePathForComparison", func() { - It("normalizes Unicode characters to NFC form and converts to lowercase", func() { - // Test with NFD (decomposed) input - as would come from macOS filesystem - nfdPath := norm.NFD.String("Michèle") // Explicitly convert to NFD form - normalized := normalizePathForComparison(nfdPath) - Expect(normalized).To(Equal("michèle")) - - // Test with NFC (composed) input - as would come from Apple Music M3U - nfcPath := "Michèle" // This might be in NFC form - normalizedNfc := normalizePathForComparison(nfcPath) - - // Ensure the two paths are not equal in their original forms - Expect(nfdPath).ToNot(Equal(nfcPath)) - - // Both should normalize to the same result - Expect(normalized).To(Equal(normalizedNfc)) - }) - - It("handles paths with mixed case and Unicode characters", func() { - path := "Artist/Noël Coward/Album/Song.mp3" - normalized := normalizePathForComparison(path) - Expect(normalized).To(Equal("artist/noël coward/album/song.mp3")) - }) -}) diff --git a/core/playlists_test.go b/core/playlists_test.go index 59b027690..6aa8aac9a 100644 --- a/core/playlists_test.go +++ b/core/playlists_test.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "golang.org/x/text/unicode/norm" ) var _ = Describe("Playlists", func() { @@ -424,6 +425,23 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks[0].Path).To(Equal("abc/tEsT1.Mp3")) }) + It("handles Unicode normalization when comparing paths (NFD vs NFC)", func() { + // Simulate macOS filesystem: stores paths in NFD (decomposed) form + // "è" (U+00E8) in NFC becomes "e" + "◌̀" (U+0065 + U+0300) in NFD + nfdPath := "artist/Mich" + string([]rune{'e', '\u0300'}) + "le/song.mp3" // NFD: e + combining grave + repo.data = []string{nfdPath} + + // Simulate Apple Music M3U: uses NFC (composed) form + nfcPath := "/music/artist/Mich\u00E8le/song.mp3" // NFC: single è character + m3u := nfcPath + "\n" + f := strings.NewReader(m3u) + pls, err := ps.ImportM3U(ctx, f) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Tracks).To(HaveLen(1)) + // Should match despite different Unicode normalization forms + Expect(pls.Tracks[0].Path).To(Equal(nfdPath)) + }) + }) Describe("InPlaylistsPath", func() { @@ -524,6 +542,9 @@ func (r *mockedMediaFileFromListRepo) FindByPaths(paths []string) (model.MediaFi var mfs model.MediaFiles for idx, dataPath := range r.data { + // Normalize the data path to NFD (simulates macOS filesystem storage) + normalizedDataPath := norm.NFD.String(dataPath) + for _, requestPath := range paths { // Strip library qualifier if present (format: "libraryID:path") actualPath := requestPath @@ -535,11 +556,15 @@ func (r *mockedMediaFileFromListRepo) FindByPaths(paths []string) (model.MediaFi } } + // The request path should already be normalized to NFD by production code + // before calling FindByPaths (to match DB storage) + normalizedRequestPath := norm.NFD.String(actualPath) + // Case-insensitive comparison (like SQL's "collate nocase") - if strings.EqualFold(actualPath, dataPath) { + if strings.EqualFold(normalizedRequestPath, normalizedDataPath) { mfs = append(mfs, model.MediaFile{ ID: strconv.Itoa(idx), - Path: dataPath, + Path: dataPath, // Return original path from DB LibraryID: libraryID, }) break