Improve scanner (#1054)

* Handle subdirectories without rx permission correctly
Allow ogg files w/o metadata, having taglib behave more like ffmpeg

* Fix test for walk_dir_tree, fix full reading of files in permission-
constrained directories, allow directories with leading ellipses

* Sorted directory traversal is preferred, and cleanup tests

* Small refactoring to clean-up `loadDir` function and to remove some "warnings" from IntelliJ

Co-authored-by: Deluan <deluan@navidrome.org>

scanner/metadata/taglib.go

@@ -51,7 +51,6 @@ func (e *taglibExtractor) extractMetadata(filePath string) (*taglibMetadata, err
 	md.tags, err = taglib.Read(filePath)
 	if err != nil {
 		log.Warn("Error reading metadata from file. Skipping", "filePath", filePath, err)
-		return nil, errors.New("error reading tags")
 	}
 	md.hasPicture = hasEmbeddedImage(filePath)
 	return md, nil

scanner/walk_dir_tree.go

@@ -2,9 +2,10 @@ package scanner
 
 import (
 	"context"
-	"io/ioutil"
+	"io/fs"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"time"
 
@@ -24,6 +25,26 @@ type (
 	walkResults = chan dirStats
 )
 
+func fullReadDir(name string) ([]os.DirEntry, error) {
+	f, err := os.Open(name)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+
+	var allDirs []os.DirEntry
+	for {
+		dirs, err := f.ReadDir(-1)
+		allDirs = append(allDirs, dirs...)
+		if err == nil {
+			break
+		}
+		log.Warn("Skipping DirEntry", err)
+	}
+	sort.Slice(allDirs, func(i, j int) bool { return allDirs[i].Name() < allDirs[j].Name() })
+	return allDirs, nil
+}
+
 func walkDirTree(ctx context.Context, rootFolder string, results walkResults) error {
 	err := walkFolder(ctx, rootFolder, rootFolder, results)
 	if err != nil {
@@ -49,84 +70,94 @@ func walkFolder(ctx context.Context, rootPath string, currentFolder string, resu
 	log.Trace(ctx, "Found directory", "dir", dir, "audioCount", stats.AudioFilesCount,
 		"hasImages", stats.HasImages, "hasPlaylist", stats.HasPlaylist)
 	stats.Path = dir
-	results <- stats
+	results <- *stats
 
 	return nil
 }
 
-func loadDir(ctx context.Context, dirPath string) (children []string, stats dirStats, err error) {
+func loadDir(ctx context.Context, dirPath string) ([]string, *dirStats, error) {
+	var children []string
+	stats := &dirStats{}
+
 	dirInfo, err := os.Stat(dirPath)
 	if err != nil {
 		log.Error(ctx, "Error stating dir", "path", dirPath, err)
-		return
+		return nil, nil, err
 	}
 	stats.ModTime = dirInfo.ModTime()
 
-	files, err := ioutil.ReadDir(dirPath)
+	dirEntries, err := fullReadDir(dirPath)
 	if err != nil {
-		log.Error(ctx, "Error reading dir", "path", dirPath, err)
+		log.Error(ctx, "Error in ReadDir", "path", dirPath, err)
-		return
+		return children, stats, err
 	}
-	for _, f := range files {
+	for _, entry := range dirEntries {
-		isDir, err := isDirOrSymlinkToDir(dirPath, f)
+		isDir, err := isDirOrSymlinkToDir(dirPath, entry)
 		// Skip invalid symlinks
 		if err != nil {
-			log.Error(ctx, "Invalid symlink", "dir", dirPath)
+			log.Error(ctx, "Invalid symlink", "dir", filepath.Join(dirPath, entry.Name()), err)
 			continue
 		}
-		if isDir && !isDirIgnored(dirPath, f) && isDirReadable(dirPath, f) {
+		if isDir && !isDirIgnored(dirPath, entry) && isDirReadable(dirPath, entry) {
-			children = append(children, filepath.Join(dirPath, f.Name()))
+			children = append(children, filepath.Join(dirPath, entry.Name()))
 		} else {
-			if f.ModTime().After(stats.ModTime) {
+			fileInfo, err := entry.Info()
-				stats.ModTime = f.ModTime()
+			if err != nil {
+				log.Error(ctx, "Error getting fileInfo", "name", entry.Name(), err)
+				return children, stats, err
 			}
-			if utils.IsAudioFile(f.Name()) {
+			if fileInfo.ModTime().After(stats.ModTime) {
+				stats.ModTime = fileInfo.ModTime()
+			}
+			if utils.IsAudioFile(entry.Name()) {
 				stats.AudioFilesCount++
 			} else {
-				stats.HasPlaylist = stats.HasPlaylist || utils.IsPlaylist(f.Name())
+				stats.HasPlaylist = stats.HasPlaylist || utils.IsPlaylist(entry.Name())
-				stats.HasImages = stats.HasImages || utils.IsImageFile(f.Name())
+				stats.HasImages = stats.HasImages || utils.IsImageFile(entry.Name())
 			}
 		}
 	}
-	return
+	return children, stats, nil
 }
 
-// isDirOrSymlinkToDir returns true if and only if the dirInfo represents a file
+// isDirOrSymlinkToDir returns true if and only if the dirEnt represents a file
-// system directory, or a symbolic link to a directory. Note that if the dirInfo
+// system directory, or a symbolic link to a directory. Note that if the dirEnt
 // is not a directory but is a symbolic link, this method will resolve by
 // sending a request to the operating system to follow the symbolic link.
-// Copied from github.com/karrick/godirwalk
+// originally copied from github.com/karrick/godirwalk, modified to use dirEntry for
-func isDirOrSymlinkToDir(baseDir string, dirInfo os.FileInfo) (bool, error) {
+// efficiency for go 1.16 and beyond
-	if dirInfo.IsDir() {
+func isDirOrSymlinkToDir(baseDir string, dirEnt fs.DirEntry) (bool, error) {
+	if dirEnt.IsDir() {
 		return true, nil
 	}
-	if dirInfo.Mode()&os.ModeSymlink == 0 {
+	if dirEnt.Type()&os.ModeSymlink == 0 {
 		return false, nil
 	}
 	// Does this symlink point to a directory?
-	dirInfo, err := os.Stat(filepath.Join(baseDir, dirInfo.Name()))
+	fileInfo, err := os.Stat(filepath.Join(baseDir, dirEnt.Name()))
 	if err != nil {
 		return false, err
 	}
-	return dirInfo.IsDir(), nil
+	return fileInfo.IsDir(), nil
 }
 
-// isDirIgnored returns true if the directory represented by dirInfo contains an
+// isDirIgnored returns true if the directory represented by dirEnt contains an
 // `ignore` file (named after consts.SkipScanFile)
-func isDirIgnored(baseDir string, dirInfo os.FileInfo) bool {
+func isDirIgnored(baseDir string, dirEnt fs.DirEntry) bool {
-	if strings.HasPrefix(dirInfo.Name(), ".") {
+	// allows Album folders for albums which eg start with ellipses
+	if strings.HasPrefix(dirEnt.Name(), ".") && !strings.HasPrefix(dirEnt.Name(), "..") {
 		return true
 	}
-	_, err := os.Stat(filepath.Join(baseDir, dirInfo.Name(), consts.SkipScanFile))
+	_, err := os.Stat(filepath.Join(baseDir, dirEnt.Name(), consts.SkipScanFile))
 	return err == nil
 }
 
-// isDirReadable returns true if the directory represented by dirInfo is readable
+// isDirReadable returns true if the directory represented by dirEnt is readable
-func isDirReadable(baseDir string, dirInfo os.FileInfo) bool {
+func isDirReadable(baseDir string, dirEnt fs.DirEntry) bool {
-	path := filepath.Join(baseDir, dirInfo.Name())
+	path := filepath.Join(baseDir, dirEnt.Name())
 	res, err := utils.IsDirReadable(path)
 	if !res {
-		log.Debug("Warning: Skipping unreadable directory", "path", path, err)
+		log.Warn("Skipping unreadable directory", "path", path, err)
 	}
 	return res
 }

scanner/walk_dir_tree_test.go

@@ -10,7 +10,8 @@ import (
 	. "github.com/onsi/gomega/gstruct"
 )
 
-var _ = Describe("load_tree", func() {
+var _ = Describe("walk_dir_tree", func() {
+	baseDir := filepath.Join("tests", "fixtures")
 
 	Describe("walkDirTree", func() {
 		It("reads all info correctly", func() {
@@ -18,7 +19,7 @@ var _ = Describe("load_tree", func() {
 			results := make(walkResults, 5000)
 			var err error
 			go func() {
-				err = walkDirTree(context.TODO(), "tests/fixtures", results)
+				err = walkDirTree(context.TODO(), baseDir, results)
 			}()
 
 			for {
@@ -30,49 +31,61 @@ var _ = Describe("load_tree", func() {
 			}
 
 			Expect(err).To(BeNil())
-			Expect(collected["tests/fixtures"]).To(MatchFields(IgnoreExtras, Fields{
+			Expect(collected[baseDir]).To(MatchFields(IgnoreExtras, Fields{
 				"HasImages":       BeTrue(),
 				"HasPlaylist":     BeFalse(),
 				"AudioFilesCount": BeNumerically("==", 4),
 			}))
-			Expect(collected["tests/fixtures/playlists"].HasPlaylist).To(BeTrue())
+			Expect(collected[filepath.Join(baseDir, "playlists")].HasPlaylist).To(BeTrue())
-			Expect(collected).To(HaveKey("tests/fixtures/symlink2dir"))
+			Expect(collected).To(HaveKey(filepath.Join(baseDir, "symlink2dir")))
-			Expect(collected).To(HaveKey("tests/fixtures/empty_folder"))
+			Expect(collected).To(HaveKey(filepath.Join(baseDir, "empty_folder")))
 		})
 	})
 
 	Describe("isDirOrSymlinkToDir", func() {
 		It("returns true for normal dirs", func() {
-			dir, _ := os.Stat("tests/fixtures")
+			dirEntry, _ := getDirEntry("tests", "fixtures")
-			Expect(isDirOrSymlinkToDir("tests", dir)).To(BeTrue())
+			Expect(isDirOrSymlinkToDir(baseDir, dirEntry)).To(BeTrue())
 		})
 		It("returns true for symlinks to dirs", func() {
-			dir, _ := os.Stat("tests/fixtures/symlink2dir")
+			dirEntry, _ := getDirEntry(baseDir, "symlink2dir")
-			Expect(isDirOrSymlinkToDir("tests/fixtures", dir)).To(BeTrue())
+			Expect(isDirOrSymlinkToDir(baseDir, dirEntry)).To(BeTrue())
 		})
 		It("returns false for files", func() {
-			dir, _ := os.Stat("tests/fixtures/test.mp3")
+			dirEntry, _ := getDirEntry(baseDir, "test.mp3")
-			Expect(isDirOrSymlinkToDir("tests/fixtures", dir)).To(BeFalse())
+			Expect(isDirOrSymlinkToDir(baseDir, dirEntry)).To(BeFalse())
 		})
 		It("returns false for symlinks to files", func() {
-			dir, _ := os.Stat("tests/fixtures/symlink")
+			dirEntry, _ := getDirEntry(baseDir, "symlink")
-			Expect(isDirOrSymlinkToDir("tests/fixtures", dir)).To(BeFalse())
+			Expect(isDirOrSymlinkToDir(baseDir, dirEntry)).To(BeFalse())
 		})
 	})
-
 	Describe("isDirIgnored", func() {
-		baseDir := filepath.Join("tests", "fixtures")
 		It("returns false for normal dirs", func() {
-			dir, _ := os.Stat(filepath.Join(baseDir, "empty_folder"))
+			dirEntry, _ := getDirEntry(baseDir, "empty_folder")
-			Expect(isDirIgnored(baseDir, dir)).To(BeFalse())
+			Expect(isDirIgnored(baseDir, dirEntry)).To(BeFalse())
 		})
 		It("returns true when folder contains .ndignore file", func() {
-			dir, _ := os.Stat(filepath.Join(baseDir, "ignored_folder"))
+			dirEntry, _ := getDirEntry(baseDir, "ignored_folder")
-			Expect(isDirIgnored(baseDir, dir)).To(BeTrue())
+			Expect(isDirIgnored(baseDir, dirEntry)).To(BeTrue())
 		})
 		It("returns true when folder name starts with a `.`", func() {
-			dir, _ := os.Stat(filepath.Join(baseDir, ".hidden_folder"))
+			dirEntry, _ := getDirEntry(baseDir, ".hidden_folder")
-			Expect(isDirIgnored(baseDir, dir)).To(BeTrue())
+			Expect(isDirIgnored(baseDir, dirEntry)).To(BeTrue())
+		})
+		It("returns false when folder name starts with ellipses", func() {
+			dirEntry, _ := getDirEntry(baseDir, "...unhidden_folder")
+			Expect(isDirIgnored(baseDir, dirEntry)).To(BeFalse())
 		})
 	})
 })
+
+func getDirEntry(baseDir, name string) (os.DirEntry, error) {
+	dirEntries, _ := os.ReadDir(baseDir)
+	for _, entry := range dirEntries {
+		if entry.Name() == name {
+			return entry, nil
+		}
+	}
+	return nil, os.ErrNotExist
+}