mirror of
https://github.com/navidrome/navidrome.git
synced 2026-11-05 09:48:37 +01:00
refactor(lyrics): read sidecar files via library storage FS
Routes fromExternalFile reads through storage.For(mf.LibraryPath) instead of os.Open on AbsolutePath, fixing sidecar reads for non-local backends. UTF-16 LE/BE and BOM handling preserved via ioutils.UTF8Reader.
This commit is contained in:
Deluan
parent
3a14faa033
commit
60c2a78396
3 changed files with 73 additions and 40 deletions
|
|
@ -1,9 +1,13 @@
|
|||
package lyrics_test
|
||||
|
||||
import (
|
||||
"io/fs"
|
||||
"testing"
|
||||
|
||||
"github.com/navidrome/navidrome/consts"
|
||||
"github.com/navidrome/navidrome/core/storage/local"
|
||||
"github.com/navidrome/navidrome/log"
|
||||
"github.com/navidrome/navidrome/model/metadata"
|
||||
"github.com/navidrome/navidrome/tests"
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
. "github.com/onsi/gomega"
|
||||
|
|
@ -15,3 +19,16 @@ func TestLyrics(t *testing.T) {
|
|||
RegisterFailHandler(Fail)
|
||||
RunSpecs(t, "Lyrics Suite")
|
||||
}
|
||||
|
||||
// Register a no-op extractor so that storage.For("file://...") works in tests.
|
||||
// The lyrics package only needs FS() for sidecar reads; ReadTags is never called.
|
||||
var _ = BeforeSuite(func() {
|
||||
local.RegisterExtractor(consts.DefaultScannerExtractor, func(fs.FS, string) local.Extractor {
|
||||
return &noopExtractor{}
|
||||
})
|
||||
})
|
||||
|
||||
type noopExtractor struct{}
|
||||
|
||||
func (e *noopExtractor) Parse(_ ...string) (map[string]metadata.Info, error) { return nil, nil }
|
||||
func (e *noopExtractor) Version() string { return "noop" }
|
||||
|
|
|
|||
|
|
@ -3,9 +3,13 @@ package lyrics
|
|||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"os"
|
||||
"fmt"
|
||||
"io"
|
||||
"io/fs"
|
||||
"path"
|
||||
|
||||
"github.com/navidrome/navidrome/core/storage"
|
||||
_ "github.com/navidrome/navidrome/core/storage/local"
|
||||
"github.com/navidrome/navidrome/log"
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/utils/ioutils"
|
||||
|
|
@ -23,31 +27,44 @@ func fromEmbedded(ctx context.Context, mf *model.MediaFile) (model.LyricList, er
|
|||
}
|
||||
|
||||
func fromExternalFile(ctx context.Context, mf *model.MediaFile, suffix string) (model.LyricList, error) {
|
||||
basePath := mf.AbsolutePath()
|
||||
ext := path.Ext(basePath)
|
||||
ext := path.Ext(mf.Path)
|
||||
sidecarRelPath := mf.Path[0:len(mf.Path)-len(ext)] + suffix
|
||||
|
||||
externalLyric := basePath[0:len(basePath)-len(ext)] + suffix
|
||||
store, err := storage.For(mf.LibraryPath)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("getting storage for library: %w", err)
|
||||
}
|
||||
fsys, err := store.FS()
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("opening library filesystem: %w", err)
|
||||
}
|
||||
|
||||
contents, err := ioutils.UTF8ReadFile(externalLyric)
|
||||
if errors.Is(err, os.ErrNotExist) {
|
||||
log.Trace(ctx, "no lyrics found at path", "path", externalLyric)
|
||||
f, err := fsys.Open(sidecarRelPath)
|
||||
if errors.Is(err, fs.ErrNotExist) {
|
||||
log.Trace(ctx, "no lyrics found at path", "path", sidecarRelPath)
|
||||
return nil, nil
|
||||
} else if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer f.Close()
|
||||
|
||||
contents, err := io.ReadAll(ioutils.UTF8Reader(f))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
list, err := model.ParseLyricsFile(suffix, contents)
|
||||
if err != nil {
|
||||
log.Error(ctx, "error parsing external lyric file", "path", externalLyric, err)
|
||||
log.Error(ctx, "error parsing external lyric file", "path", sidecarRelPath, err)
|
||||
return nil, err
|
||||
}
|
||||
|
||||
if len(list) == 0 {
|
||||
log.Trace(ctx, "empty lyrics from external file", "path", externalLyric)
|
||||
log.Trace(ctx, "empty lyrics from external file", "path", sidecarRelPath)
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
log.Trace(ctx, "retrieved lyrics from external file", "path", externalLyric)
|
||||
log.Trace(ctx, "retrieved lyrics from external file", "path", sidecarRelPath)
|
||||
return list, nil
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ package lyrics
|
|||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"path/filepath"
|
||||
|
||||
"github.com/navidrome/navidrome/model"
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
|
|
@ -53,46 +54,51 @@ var _ = Describe("sources", func() {
|
|||
})
|
||||
|
||||
Describe("fromExternalFile", func() {
|
||||
var fixturesDir string
|
||||
|
||||
BeforeEach(func() {
|
||||
// tests.Init sets CWD to the repo root, so "tests/fixtures" resolves correctly.
|
||||
abs, err := filepath.Abs("tests/fixtures")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
fixturesDir = abs
|
||||
})
|
||||
|
||||
mf := func(name string) *model.MediaFile {
|
||||
return &model.MediaFile{LibraryPath: fixturesDir, Path: name}
|
||||
}
|
||||
|
||||
It("should return nil for lyrics that don't exist", func() {
|
||||
mf := model.MediaFile{Path: "tests/fixtures/01 Invisible (RED) Edit Version.mp3"}
|
||||
lyrics, err := fromExternalFile(ctx, &mf, ".lrc")
|
||||
lyrics, err := fromExternalFile(ctx, mf("01 Invisible (RED) Edit Version.mp3"), ".lrc")
|
||||
|
||||
Expect(err).To(BeNil())
|
||||
Expect(lyrics).To(HaveLen(0))
|
||||
})
|
||||
|
||||
// fromExternalFile delegates format parsing to model.ParseLyricsFile; the
|
||||
// fromExternalFile delegates format parsing to model.ParseLyrics; the
|
||||
// per-format parser output is covered exhaustively in the model package.
|
||||
// Here we only verify each suffix is read from disk and routed to a parser.
|
||||
// Here we only verify each suffix is read from the library FS and routed.
|
||||
DescribeTable("should read the sidecar file and route its suffix to a parser",
|
||||
func(path, suffix string, expectSynced bool) {
|
||||
mf := model.MediaFile{Path: path}
|
||||
lyrics, err := fromExternalFile(ctx, &mf, suffix)
|
||||
func(name, suffix string, expectSynced bool) {
|
||||
lyrics, err := fromExternalFile(ctx, mf(name), suffix)
|
||||
|
||||
Expect(err).To(BeNil())
|
||||
Expect(lyrics).ToNot(BeEmpty())
|
||||
Expect(lyrics[0].Line).ToNot(BeEmpty())
|
||||
Expect(lyrics[0].Synced).To(Equal(expectSynced))
|
||||
},
|
||||
Entry(".lrc synced", "tests/fixtures/test.mp3", ".lrc", true),
|
||||
Entry(".elrc enhanced", "tests/fixtures/test.mp3", ".elrc", true),
|
||||
Entry(".txt plain", "tests/fixtures/test.mp3", ".txt", false),
|
||||
Entry(".srt subtitles", "tests/fixtures/test.mp3", ".srt", true),
|
||||
Entry(".ttml multilingual", "tests/fixtures/test.mp3", ".ttml", true),
|
||||
Entry(".yaml lyricsfile", "tests/fixtures/test.mp3", ".yaml", true),
|
||||
Entry(".lrc synced", "test.mp3", ".lrc", true),
|
||||
Entry(".elrc enhanced", "test.mp3", ".elrc", true),
|
||||
Entry(".txt plain", "test.mp3", ".txt", false),
|
||||
Entry(".srt subtitles", "test.mp3", ".srt", true),
|
||||
Entry(".ttml multilingual", "test.mp3", ".ttml", true),
|
||||
Entry(".yaml lyricsfile", "test.mp3", ".yaml", true),
|
||||
)
|
||||
|
||||
It("should handle LRC files with UTF-8 BOM marker (issue #4631)", func() {
|
||||
// The function looks for <basePath-without-ext><suffix>, so we need to pass
|
||||
// a MediaFile with .mp3 path and look for .lrc suffix
|
||||
mf := model.MediaFile{Path: "tests/fixtures/bom-test.mp3"}
|
||||
lyrics, err := fromExternalFile(ctx, &mf, ".lrc")
|
||||
lyrics, err := fromExternalFile(ctx, mf("bom-test.mp3"), ".lrc")
|
||||
|
||||
Expect(err).To(BeNil())
|
||||
Expect(lyrics).ToNot(BeNil())
|
||||
Expect(lyrics).To(HaveLen(1))
|
||||
|
||||
// The critical assertion: even with BOM, synced should be true
|
||||
Expect(lyrics[0].Synced).To(BeTrue(), "Lyrics with BOM marker should be recognized as synced")
|
||||
Expect(lyrics[0].Line).To(HaveLen(1))
|
||||
Expect(lyrics[0].Line[0].Start).To(Equal(new(int64(0))))
|
||||
|
|
@ -100,14 +106,10 @@ var _ = Describe("sources", func() {
|
|||
})
|
||||
|
||||
It("should handle UTF-16 LE encoded LRC files", func() {
|
||||
mf := model.MediaFile{Path: "tests/fixtures/bom-utf16-test.mp3"}
|
||||
lyrics, err := fromExternalFile(ctx, &mf, ".lrc")
|
||||
lyrics, err := fromExternalFile(ctx, mf("bom-utf16-test.mp3"), ".lrc")
|
||||
|
||||
Expect(err).To(BeNil())
|
||||
Expect(lyrics).ToNot(BeNil())
|
||||
Expect(lyrics).To(HaveLen(1))
|
||||
|
||||
// UTF-16 should be properly converted to UTF-8
|
||||
Expect(lyrics[0].Synced).To(BeTrue(), "UTF-16 encoded lyrics should be recognized as synced")
|
||||
Expect(lyrics[0].Line).To(HaveLen(2))
|
||||
Expect(lyrics[0].Line[0].Start).To(Equal(new(int64(18800))))
|
||||
|
|
@ -117,8 +119,7 @@ var _ = Describe("sources", func() {
|
|||
})
|
||||
|
||||
It("should handle TTML files with UTF-8 BOM marker", func() {
|
||||
mf := model.MediaFile{Path: "tests/fixtures/bom-test.mp3"}
|
||||
lyrics, err := fromExternalFile(ctx, &mf, ".ttml")
|
||||
lyrics, err := fromExternalFile(ctx, mf("bom-test.mp3"), ".ttml")
|
||||
|
||||
Expect(err).To(BeNil())
|
||||
Expect(lyrics).To(HaveLen(1))
|
||||
|
|
@ -130,8 +131,7 @@ var _ = Describe("sources", func() {
|
|||
})
|
||||
|
||||
It("should handle UTF-16 BE encoded TTML files", func() {
|
||||
mf := model.MediaFile{Path: "tests/fixtures/bom-utf16-test.mp3"}
|
||||
lyrics, err := fromExternalFile(ctx, &mf, ".ttml")
|
||||
lyrics, err := fromExternalFile(ctx, mf("bom-utf16-test.mp3"), ".ttml")
|
||||
|
||||
Expect(err).To(BeNil())
|
||||
Expect(lyrics).To(HaveLen(1))
|
||||
|
|
@ -143,6 +143,5 @@ var _ = Describe("sources", func() {
|
|||
Expect(lyrics[0].Line[1].Start).To(Equal(new(int64(22801))))
|
||||
Expect(lyrics[0].Line[1].Value).To(Equal("UTF16 line two"))
|
||||
})
|
||||
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue