mirror of
https://github.com/navidrome/navidrome.git
synced 2026-05-03 06:51:16 +00:00
fix: ensure longest library path matches first to resolve prefix conflicts in playlists
Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
parent
84449996d2
commit
eab196d7e2
@ -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 {
|
||||
|
||||
186
core/playlists_internal_test.go
Normal file
186
core/playlists_internal_test.go
Normal file
@ -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)
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
@ -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!)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user