From 5925e8dbd8b1f423a3e3e6114f489459d4e6c67a Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 7 Nov 2025 00:08:48 -0500 Subject: [PATCH] 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!) + }) }) })