From 72a69306be72a164e6c41f57c985a967a9c960cc Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 19 Jun 2026 15:52:01 -0400 Subject: [PATCH] refactor(lyrics): unify parser dispatch and centralize empty-list invariant Apply thermo-nuclear review findings (behavior-preserving): - Replace the suffix switch + three single-use closure adapters (parseTTMLKnown/parseSRTKnown + inline YAML closure) with a bySuffix map of a single lyricParser(lang, contents) signature. Normalize parseTTMLWithDefaultLang/parseSRTWithLanguage to that (lang, contents) order so no adapter glue is needed. - Collapse the parallel sniffLyrics engine into one parseFirstMatch primitive shared by both the suffix and content-sniff paths (sniffOrder candidate list). TTML stays gated via parseTTMLIfDocument in sniff mode to avoid running the XML decoder on plain/LRC text. - Add LyricList.MarshalJSON so empty/nil always serializes to [] (the lyrics column invariant), in one canonical place. Delete the migration's nil-guard, which the marshaler now subsumes. Behavior verified unchanged: full suite + race + e2e green. --- .../20231209211223_alter_lyric_column.go | 4 - model/lyrics.go | 11 ++ model/lyrics_parse.go | 125 +++++++++--------- model/lyrics_srt.go | 4 +- model/lyrics_ttml.go | 4 +- 5 files changed, 77 insertions(+), 71 deletions(-) diff --git a/db/migrations/20231209211223_alter_lyric_column.go b/db/migrations/20231209211223_alter_lyric_column.go index 9afc3f2c9..259a37745 100644 --- a/db/migrations/20231209211223_alter_lyric_column.go +++ b/db/migrations/20231209211223_alter_lyric_column.go @@ -50,10 +50,6 @@ func upAlterLyricColumn(ctx context.Context, tx *sql.Tx) error { if err != nil { return err } - if parsed == nil { - // Preserve the column invariant: empty lyrics serialize to [], never null - parsed = model.LyricList{} - } text, err := json.Marshal(parsed) if err != nil { diff --git a/model/lyrics.go b/model/lyrics.go index ba63daa14..9712ca30a 100644 --- a/model/lyrics.go +++ b/model/lyrics.go @@ -2,6 +2,7 @@ package model import ( "cmp" + "encoding/json" "regexp" "slices" "strconv" @@ -413,6 +414,16 @@ func parseTime(line string, match []int) (int64, error) { type LyricList []Lyrics +// MarshalJSON owns the lyrics column invariant: an empty or nil list serializes +// to [], never null. This keeps every writer consistent without each one having +// to pre-initialize or nil-guard the slice. +func (ll LyricList) MarshalJSON() ([]byte, error) { + if len(ll) == 0 { + return []byte("[]"), nil + } + return json.Marshal([]Lyrics(ll)) +} + // Main returns the main-kind lyric, falling back to the first entry so untyped // lyrics still resolve. The bool is false only when the list is empty. It is // used to surface a single lyric through the plain-text legacy getLyrics diff --git a/model/lyrics_parse.go b/model/lyrics_parse.go index 4539e3641..e415365ca 100644 --- a/model/lyrics_parse.go +++ b/model/lyrics_parse.go @@ -8,49 +8,73 @@ import ( "github.com/navidrome/navidrome/log" ) -// ParseLyrics is the single entry point for parsing lyrics. When suffix names a -// known format (.ttml/.srt/.yaml/.yml/.lrc) it routes to that parser, falling -// back to plain text on failure. When suffix is empty or "auto" it content-sniffs -// (TTML → SRT → YAML/Lyricsfile → LRC/plain) — used for tag-embedded lyrics and -// plugin responses. -func ParseLyrics(suffix, lang string, contents []byte) (LyricList, error) { - switch { - case suffix == "" || strings.EqualFold(suffix, "auto"): - return sniffLyrics(lang, contents) - case strings.EqualFold(suffix, ".ttml"): - return parseWithPlainFallback(lang, contents, parseTTMLKnown(lang)) - case strings.EqualFold(suffix, ".srt"): - return parseWithPlainFallback(lang, contents, parseSRTKnown(lang)) - case strings.EqualFold(suffix, ".yaml"), strings.EqualFold(suffix, ".yml"): - return parseWithPlainFallback(lang, contents, func(c []byte) (LyricList, error) { - return parseLyricsfile(string(c)) - }) - default: // .lrc and any unknown suffix: LRC/plain is the floor - return plainLRC(lang, contents) - } +// lyricParser parses a single lyrics format. It returns an empty list (not an +// error) when the input is well-formed but not its format, so candidates can be +// tried in order. The lang argument is the default language for formats that do +// not carry their own. +type lyricParser func(lang string, contents []byte) (LyricList, error) + +func parseLyricsfileBytes(_ string, contents []byte) (LyricList, error) { + return parseLyricsfile(string(contents)) } -// parseWithPlainFallback runs a known-format parser; on error or empty result it -// falls back to the LRC/plain floor (never another structured format). -func parseWithPlainFallback(lang string, contents []byte, parse func([]byte) (LyricList, error)) (LyricList, error) { - list, err := parse(contents) - if err == nil && len(list) > 0 { - return list, nil +// parseTTMLIfDocument parses TTML only when the content actually looks like a +// TTML document, so content-sniffing plain text or LRC does not run the XML +// decoder (and never logs a spurious parse warning for non-TTML input). +func parseTTMLIfDocument(lang string, contents []byte) (LyricList, error) { + if !isTTMLDocument(string(contents)) { + return nil, nil } - if err != nil { - log.Warn("Error parsing lyrics by suffix, falling back to plain text", "error", err) + return parseTTMLWithDefaultLang(lang, contents) +} + +// bySuffix maps a sidecar file extension to its parser. LRC and unknown +// extensions are absent: they fall straight to the plain-text floor. The suffix +// already declares the format, so TTML parses unconditionally here (a malformed +// .ttml surfaces its error and then the plain-text fallback). +var bySuffix = map[string]lyricParser{ + ".ttml": parseTTMLWithDefaultLang, + ".srt": parseSRTWithLanguage, + ".yaml": parseLyricsfileBytes, + ".yml": parseLyricsfileBytes, +} + +// sniffOrder is the content-sniff candidate order for tag-embedded lyrics and +// plugin responses (no suffix to dispatch on): TTML → SRT → YAML → LRC/plain. +// TTML is gated on looking like a document so plain/LRC text is not run through +// the XML decoder. +var sniffOrder = []lyricParser{parseTTMLIfDocument, parseSRTWithLanguage, parseLyricsfileBytes} + +// ParseLyrics is the single entry point for parsing lyrics. A known suffix +// (.ttml/.srt/.yaml/.yml/.lrc) routes to that format's parser; an empty or +// "auto" suffix content-sniffs. In both modes a structured parser that does not +// match falls back to the LRC/plain-text floor — never to another structured +// format. +func ParseLyrics(suffix, lang string, contents []byte) (LyricList, error) { + if suffix == "" || strings.EqualFold(suffix, "auto") { + return parseFirstMatch(lang, stripBOM(contents), sniffOrder...) + } + if parse, ok := bySuffix[strings.ToLower(suffix)]; ok { + return parseFirstMatch(lang, contents, parse) + } + return plainLRC(lang, contents) // .lrc and any unknown suffix +} + +// parseFirstMatch tries each candidate in order, returning the first non-empty +// result. When every candidate misses, it falls to the LRC/plain-text floor. +func parseFirstMatch(lang string, contents []byte, candidates ...lyricParser) (LyricList, error) { + for _, parse := range candidates { + list, err := parse(lang, contents) + if err == nil && len(list) > 0 { + return list, nil + } + if err != nil { + log.Warn("Error parsing lyrics, falling back to plain text", "error", err) + } } return plainLRC(lang, contents) } -func parseTTMLKnown(lang string) func([]byte) (LyricList, error) { - return func(c []byte) (LyricList, error) { return parseTTMLWithDefaultLang(c, lang) } -} - -func parseSRTKnown(lang string) func([]byte) (LyricList, error) { - return func(c []byte) (LyricList, error) { return parseSRTWithLanguage(c, lang) } -} - func plainLRC(lang string, contents []byte) (LyricList, error) { lyric, err := parseLRC(lang, string(contents)) if err != nil { @@ -62,33 +86,8 @@ func plainLRC(lang string, contents []byte) (LyricList, error) { return LyricList{*lyric}, nil } -// sniffLyrics detects the format from content. Order: TTML → SRT → YAML → LRC/plain. -func sniffLyrics(lang string, contents []byte) (LyricList, error) { - text := strings.TrimPrefix(string(contents), "\ufeff") - - if isTTMLDocument(text) { - list, err := parseTTMLWithDefaultLang([]byte(text), lang) - if err == nil && len(list) > 0 { - return list, nil - } - if err != nil { - log.Warn("Error parsing embedded TTML lyrics, falling back to plain lyrics", "error", err) - } - } - - list, err := parseSRTWithLanguage([]byte(text), lang) - if err == nil && len(list) > 0 { - return list, nil - } - if err != nil && strings.Contains(text, "-->") { - log.Warn("Error parsing embedded SRT lyrics, falling back to plain lyrics", "error", err) - } - - if yamlList, yErr := parseLyricsfile(text); yErr == nil && len(yamlList) > 0 { - return yamlList, nil - } - - return plainLRC(lang, []byte(text)) +func stripBOM(contents []byte) []byte { + return []byte(strings.TrimPrefix(string(contents), "\ufeff")) } func isTTMLDocument(text string) bool { diff --git a/model/lyrics_srt.go b/model/lyrics_srt.go index 4cfa9fd00..342e2548f 100644 --- a/model/lyrics_srt.go +++ b/model/lyrics_srt.go @@ -15,10 +15,10 @@ var ( ) func parseSRT(contents []byte) (LyricList, error) { - return parseSRTWithLanguage(contents, "xxx") + return parseSRTWithLanguage("xxx", contents) } -func parseSRTWithLanguage(contents []byte, language string) (LyricList, error) { +func parseSRTWithLanguage(language string, contents []byte) (LyricList, error) { raw := strings.ReplaceAll(string(contents), "\r\n", "\n") raw = strings.ReplaceAll(raw, "\r", "\n") diff --git a/model/lyrics_ttml.go b/model/lyrics_ttml.go index e70ed66b0..4f49c8818 100644 --- a/model/lyrics_ttml.go +++ b/model/lyrics_ttml.go @@ -103,10 +103,10 @@ type ttmlParser struct { } func parseTTML(contents []byte) (LyricList, error) { - return parseTTMLWithDefaultLang(contents, "xxx") + return parseTTMLWithDefaultLang("xxx", contents) } -func parseTTMLWithDefaultLang(contents []byte, defaultLang string) (LyricList, error) { +func parseTTMLWithDefaultLang(defaultLang string, contents []byte) (LyricList, error) { contents = xmlEncodingRegex.ReplaceAll(contents, []byte(``)) p := ttmlParser{