diff --git a/core/playlists.go b/core/playlists.go index f98179f88..ed90cc23b 100644 --- a/core/playlists.go +++ b/core/playlists.go @@ -1,6 +1,7 @@ package core import ( + "cmp" "context" "encoding/json" "errors" @@ -9,7 +10,7 @@ import ( "net/url" "os" "path/filepath" - "regexp" + "slices" "strings" "time" @@ -194,22 +195,35 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m } filteredLines = append(filteredLines, line) } - paths, 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(paths) + + // 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, normalized for comparison existing := make(map[string]int, len(found)) for idx := range found { - existing[normalizePathForComparison(found[idx].Path)] = idx + // 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 paths { - idx, ok := existing[normalizePathForComparison(path)] + + // 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 { @@ -226,69 +240,150 @@ 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 + libraryPath string + libraryID int + valid bool } -// TODO This won't work for multiple libraries -func (s *playlists) normalizePaths(ctx context.Context, pls *model.Playlist, folder *model.Folder, lines []string) ([]string, error) { - libRegex, err := s.compileLibraryPaths(ctx) +// 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") + } + relativePath, err := filepath.Rel(r.libraryPath, r.absolutePath) if err != nil { - return nil, err + return "", err } - - res := make([]string, 0, len(lines)) - for idx, line := range lines { - var libPath string - var filePath string - - if folder != nil && !filepath.IsAbs(line) { - libPath = folder.LibraryPath - filePath = filepath.Join(folder.AbsolutePath(), line) - } 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 { - log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) - } - } - return slice.Map(res, filepath.ToSlash), nil + // Convert path separators to forward slashes + return fmt.Sprintf("%d:%s", r.libraryID, filepath.ToSlash(relativePath)), nil } -func (s *playlists) compileLibraryPaths(ctx context.Context) (*regexp.Regexp, error) { - libs, err := s.ds.Library(ctx).GetAll() - if err != nil { - return nil, err - } +// libraryMatcher holds sorted libraries with cleaned paths for efficient path matching. +type libraryMatcher struct { + libraries model.Libraries + cleanedPaths []string +} - // Create regex patterns for each library path - patterns := make([]string, len(libs)) +// 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 { - cleanPath := filepath.Clean(lib.Path) - escapedPath := regexp.QuoteMeta(cleanPath) - patterns[i] = fmt.Sprintf("^%s(?:/|$)", escapedPath) + cleanedPaths[i] = filepath.Clean(lib.Path) } - // Combine all patterns into a single regex - combinedPattern := strings.Join(patterns, "|") - re, err := regexp.Compile(combinedPattern) + return &libraryMatcher{ + libraries: libs, + cleanedPaths: cleanedPaths, + } +} + +// 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) { + libs, err := ds.Library(ctx).GetAll() if err != nil { - return nil, fmt.Errorf("compiling library paths `%s`: %w", combinedPattern, err) + return nil, err } - return re, nil + matcher := newLibraryMatcher(libs) + return &pathResolver{matcher: matcher}, nil +} + +// 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) { + // 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.findInLibraries(absolutePath) +} + +// 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 { + return pathResolution{valid: false} + } + return pathResolution{ + absolutePath: absolutePath, + 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. +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 + } + + 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 new file mode 100644 index 000000000..88e36cc3a --- /dev/null +++ b/core/playlists_internal_test.go @@ -0,0 +1,406 @@ +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("libraryMatcher", func() { + var ds *tests.MockDataStore + var mockLibRepo *tests.MockLibraryRepo + ctx := context.Background() + + BeforeEach(func() { + mockLibRepo = &tests.MockLibraryRepo{} + ds = &tests.MockDataStore{ + MockedLibrary: mockLibRepo, + } + }) + + // 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 + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + {ID: 2, Path: "/music-classical"}, + {ID: 3, Path: "/music-classical/opera"}, + }) + + matcher := createMatcher(ds) + + // Test that longest path matches first and returns correct library ID + testCases := []struct { + path string + expectedLibID int + expectedLibPath string + }{ + {"/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 { + 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) + } + }) + + 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"}, + }) + + matcher := createMatcher(ds) + + // Test that music-backup library is matched correctly + 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 + 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() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + {ID: 2, Path: "/music-classical"}, + }) + + matcher := createMatcher(ds) + + // 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() { + 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"}, + }) + + matcher := createMatcher(ds) + + testCases := []struct { + path string + expectedLibID int + expectedLibPath string + }{ + {"/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 { + 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) + } + }) + }) + + Describe("Edge cases", func() { + It("handles empty library list", func() { + mockLibRepo.SetData([]model.Library{}) + + matcher := createMatcher(ds) + Expect(matcher).ToNot(BeNil()) + + // Should not match anything + libID, libPath := matcher.findLibraryForPath("/music/track.mp3") + Expect(libID).To(Equal(0)) + Expect(libPath).To(BeEmpty()) + }) + + It("handles single library", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music"}, + }) + + matcher := createMatcher(ds) + + libID, libPath := matcher.findLibraryForPath("/music/track.mp3") + Expect(libID).To(Equal(1)) + Expect(libPath).To(Equal("/music")) + }) + + It("handles libraries with special characters in paths", func() { + mockLibRepo.SetData([]model.Library{ + {ID: 1, Path: "/music[test]"}, + {ID: 2, Path: "/music(backup)"}, + }) + + matcher := createMatcher(ds) + Expect(matcher).ToNot(BeNil()) + + // 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("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"}, + }) + + matcher := createMatcher(ds) + + // Verify that longer paths match correctly (not cut off by shorter prefix) + testCases := []struct { + path string + expectedLibID int + }{ + {"/abc/file.mp3", 3}, + {"/ab/file.mp3", 2}, + {"/a/file.mp3", 1}, + } + + for _, tc := range testCases { + libID, _ := matcher.findLibraryForPath(tc.path) + Expect(libID).To(Equal(tc.expectedLibID), "Path %s should match library ID %d", tc.path, tc.expectedLibID) + } + }) + }) +}) + +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("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")) + }) + + 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")) + }) + }) + + Context("With relative paths", func() { + It("resolves relative path within same library", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + resolution := resolver.resolvePath("../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.resolvePath("../../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.resolvePath("../../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.resolvePath("../../../../etc/passwd", folder) + + Expect(resolution.valid).To(BeFalse()) + }) + }) + }) + + 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()) + }) + }) +}) diff --git a/core/playlists_test.go b/core/playlists_test.go index fb42f9c9f..6aa8aac9a 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() @@ -33,16 +34,16 @@ 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() { var folder *model.Folder BeforeEach(func() { - ps = NewPlaylists(ds) + 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, @@ -112,6 +113,224 @@ 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 + 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 = core.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")) // 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")) + }) + + 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 + }) + + 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!) + }) + + 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")) + }) + }) }) Describe("ImportM3U", func() { @@ -119,7 +338,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"}) }) @@ -206,53 +425,23 @@ 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) + 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 an Apple Music M3U playlist entry with NFC encoding - nfcPath := norm.NFC.String("/music/" + pathWithAccents) - m3u := strings.Join([]string{ - nfcPath, - }, "\n") + // 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 find the track despite Unicode normalization differences") + Expect(pls.Tracks).To(HaveLen(1)) + // Should match despite different Unicode normalization forms Expect(pls.Tracks[0].Path).To(Equal(nfdPath)) }) - }) - 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() { @@ -269,27 +458,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", @@ -297,22 +486,47 @@ var _ = Describe("Playlists", func() { Name: ".", } - Expect(InPlaylistsPath(folder2)).To(BeTrue()) + Expect(core.InPlaylistsPath(folder2)).To(BeTrue()) }) }) }) -// mockedMediaFileRepo's FindByPaths method returns a list of MediaFiles with the same paths as the input +// 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 } func (r *mockedMediaFileRepo) FindByPaths(paths []string) (model.MediaFiles, error) { var mfs model.MediaFiles + + // If data map provided, look up files + if r.data != nil { + for _, path := range paths { + if mf, ok := r.data[path]; ok { + mfs = append(mfs, mf) + } + } + return mfs, nil + } + + // Otherwise, create MediaFiles from paths for idx, path := range paths { + // Strip library qualifier if present (format: "libraryID:path") + actualPath := path + libraryID := 1 + if parts := strings.SplitN(path, ":", 2); len(parts) == 2 { + if id, err := strconv.Atoi(parts[0]); err == nil { + libraryID = id + 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 @@ -324,13 +538,38 @@ 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 - for idx, path := range r.data { - mfs = append(mfs, model.MediaFile{ - ID: strconv.Itoa(idx), - Path: path, - }) + + 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 + 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] + } + } + + // 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(normalizedRequestPath, normalizedDataPath) { + mfs = append(mfs, model.MediaFile{ + ID: strconv.Itoa(idx), + Path: dataPath, // Return original path from DB + LibraryID: libraryID, + }) + break + } + } } return mfs, nil } diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 8f32accc6..4749bb0be 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "slices" + "strconv" + "strings" "sync" "time" @@ -193,12 +195,43 @@ 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) { - sel := r.newSelect().Columns("*").Where(Eq{"path collate nocase": paths}) + 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 }