diff --git a/scanner/metadata/taglib/taglib.go b/scanner/metadata/taglib/taglib.go index aa9bea99..6df5aa11 100644 --- a/scanner/metadata/taglib/taglib.go +++ b/scanner/metadata/taglib/taglib.go @@ -1,6 +1,8 @@ package taglib import ( + "errors" + "os" "strconv" "github.com/navidrome/navidrome/log" @@ -13,16 +15,19 @@ type parsedTags = map[string][]string func (e *Parser) Parse(paths ...string) (map[string]parsedTags, error) { fileTags := map[string]parsedTags{} for _, path := range paths { - fileTags[path] = e.extractMetadata(path) + tags, err := e.extractMetadata(path) + if !errors.Is(err, os.ErrPermission) { + fileTags[path] = tags + } } return fileTags, nil } -func (e *Parser) extractMetadata(filePath string) parsedTags { +func (e *Parser) extractMetadata(filePath string) (parsedTags, error) { tags, err := Read(filePath) if err != nil { - log.Warn("Error reading metadata from file. Skipping", "filePath", filePath, err) - return nil + log.Warn("TagLib: Error reading metadata from file. Skipping", "filePath", filePath, err) + return nil, err } alternativeTags := map[string][]string{ @@ -46,5 +51,5 @@ func (e *Parser) extractMetadata(filePath string) parsedTags { } } } - return tags + return tags, nil } diff --git a/scanner/metadata/taglib/taglib_test.go b/scanner/metadata/taglib/taglib_test.go index 9de0e3ba..4ce228e2 100644 --- a/scanner/metadata/taglib/taglib_test.go +++ b/scanner/metadata/taglib/taglib_test.go @@ -1,20 +1,38 @@ package taglib import ( + "io/fs" + "os" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) var _ = Describe("Parser", func() { var e *Parser + // This file will have 0222 (no read) permission during these tests + var accessForbiddenFile = "tests/fixtures/test_no_read_permission.ogg" + BeforeEach(func() { e = &Parser{} + + err := os.Chmod(accessForbiddenFile, 0222) + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + err = os.Chmod(accessForbiddenFile, 0644) + Expect(err).ToNot(HaveOccurred()) + }) }) Context("Parse", func() { It("correctly parses metadata from all files in folder", func() { - mds, err := e.Parse("tests/fixtures/test.mp3", "tests/fixtures/test.ogg") + mds, err := e.Parse( + "tests/fixtures/test.mp3", + "tests/fixtures/test.ogg", + accessForbiddenFile, + ) Expect(err).NotTo(HaveOccurred()) Expect(mds).To(HaveLen(2)) + Expect(mds).ToNot(HaveKey(accessForbiddenFile)) m := mds["tests/fixtures/test.mp3"] Expect(m).To(HaveKeyWithValue("title", []string{"Song", "Song"})) @@ -47,4 +65,17 @@ var _ = Describe("Parser", func() { Expect(m["bitrate"][0]).To(BeElementOf("18", "39")) }) }) + + Context("Error Checking", func() { + It("correctly handle unreadable file due to insufficient read permission", func() { + _, err := e.extractMetadata(accessForbiddenFile) + Expect(err).To(MatchError(os.ErrPermission)) + }) + It("returns a generic ErrPath if file does not exist", func() { + testFilePath := "tests/fixtures/NON_EXISTENT.ogg" + _, err := e.extractMetadata(testFilePath) + Expect(err).To(MatchError(fs.ErrNotExist)) + }) + }) + }) diff --git a/scanner/metadata/taglib/taglib_wrapper.go b/scanner/metadata/taglib/taglib_wrapper.go index ebeb1e63..a00adca2 100644 --- a/scanner/metadata/taglib/taglib_wrapper.go +++ b/scanner/metadata/taglib/taglib_wrapper.go @@ -12,6 +12,7 @@ package taglib import "C" import ( "fmt" + "os" "runtime/debug" "strconv" "strings" @@ -37,17 +38,19 @@ func Read(filename string) (tags map[string][]string, err error) { defer deleteMap(id) res := C.taglib_read(fp, C.ulong(id)) - if log.CurrentLevel() >= log.LevelDebug { - switch res { - case C.TAGLIB_ERR_PARSE: - log.Warn("TagLib: cannot parse file", "filename", filename) - case C.TAGLIB_ERR_AUDIO_PROPS: - log.Warn("TagLib: can't get audio properties", "filename", filename) - } - } + switch res { + case C.TAGLIB_ERR_PARSE: + // Check additional case whether the file is unreadable due to permission + file, fileErr := os.OpenFile(filename, os.O_RDONLY, 0600) + defer file.Close() - if res != 0 { - return nil, fmt.Errorf("cannot process %s", filename) + if os.IsPermission(fileErr) { + return nil, fmt.Errorf("navidrome does not have permission: %w", fileErr) + } else { + return nil, fmt.Errorf("cannot parse file media file: %w", fileErr) + } + case C.TAGLIB_ERR_AUDIO_PROPS: + return nil, fmt.Errorf("can't get audio properties from file") } log.Trace("TagLib: read tags", "tags", m, "filename", filename, "id", id) return m, nil diff --git a/scanner/tag_scanner_test.go b/scanner/tag_scanner_test.go index 03cfb23e..e5bb7be5 100644 --- a/scanner/tag_scanner_test.go +++ b/scanner/tag_scanner_test.go @@ -10,9 +10,10 @@ var _ = Describe("TagScanner", func() { It("return all audio files from the folder", func() { files, err := loadAllAudioFiles("tests/fixtures") Expect(err).ToNot(HaveOccurred()) - Expect(files).To(HaveLen(3)) + Expect(files).To(HaveLen(4)) Expect(files).To(HaveKey("tests/fixtures/test.ogg")) Expect(files).To(HaveKey("tests/fixtures/test.mp3")) + Expect(files).To(HaveKey("tests/fixtures/test_no_read_permission.ogg")) Expect(files).To(HaveKey("tests/fixtures/01 Invisible (RED) Edit Version.mp3")) Expect(files).ToNot(HaveKey("tests/fixtures/._02 Invisible.mp3")) Expect(files).ToNot(HaveKey("tests/fixtures/playlist.m3u")) diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 4a226dc7..30352c32 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -36,7 +36,7 @@ var _ = Describe("walk_dir_tree", func() { Expect(collected[baseDir]).To(MatchFields(IgnoreExtras, Fields{ "HasImages": BeTrue(), "HasPlaylist": BeFalse(), - "AudioFilesCount": BeNumerically("==", 4), + "AudioFilesCount": BeNumerically("==", 5), })) Expect(collected[filepath.Join(baseDir, "playlists")].HasPlaylist).To(BeTrue()) Expect(collected).To(HaveKey(filepath.Join(baseDir, "symlink2dir"))) diff --git a/tests/fixtures/test_no_read_permission.ogg b/tests/fixtures/test_no_read_permission.ogg new file mode 100644 index 00000000..be672812 Binary files /dev/null and b/tests/fixtures/test_no_read_permission.ogg differ