mirror of
https://github.com/navidrome/navidrome.git
synced 2026-07-09 17:18:45 +00:00
aa5aa731dc
* 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.
* refactor(lyrics): address review feedback on sidecar FS read
- Move blank local-storage import from sources.go into lyrics_suite_test.go
(the test suite already imports the local package for RegisterExtractor,
so local's init() runs; production binaries get the scheme via normal wiring)
- Fix misleading comment: model.ParseLyrics → model.ParseLyricsFile
- Replace what-comment with why-comment in BeforeSuite explaining the
log.Fatal guard that requires the no-op extractor registration
* test(lyrics): add subsonic e2e baseline for getLyrics endpoints
Establishes a behavioral baseline for getLyricsBySongId (v2 structured)
and getLyrics (legacy) before the lyrics parser refactor. Covers embedded
formats (LRC synced, plain text, TTML) and sidecar formats (LRC, SRT,
YAML), all isolated under a Lyrics/ fixture folder so the new fixtures
do not perturb existing test behavior beyond fixture counts.
Sidecar files are injected as raw &fstest.MapFile{Data: []byte(...)}
entries; the scanner skips non-audio extensions (.lrc, .srt, .yaml) so
they are invisible to scanning but reachable via the fake FS at request
time through fromExternalFile/storage.For.
Update album/artist/song counts in the album-list, multi-library, and
search3 empty-query tests to reflect the six new tracks (1 new artist,
1 new album, 6 new songs).
* test(lyrics): strengthen e2e lyrics baseline (lang assertions, rename helper)
Rename the local helper `main` to `firstLyric` to avoid collision with the
reserved-feeling built-in name. Add `Lang` assertions to both embedded and
sidecar DescribeTable entries, locking the current observed values: "xxx"
(ISO 639-2 "no language specified") for all embedded and LRC/SRT sidecars,
and "eng" for the YAML sidecar (which explicitly sets `language: eng`).
* feat(lyrics): detect Lyricsfile YAML in content-sniffing
* feat(plugins): content-sniff plugin lyrics for all formats
Replace model.ToLyrics (LRC/plain only) with model.ParseEmbedded so plugin
responses are content-sniffed for TTML, SRT, YAML, LRC, and plain text.
ParseEmbedded returns a LyricList, so the loop now flattens multiple tracks
per response entry.
The test-lyrics WASM plugin gains a "ttml" format mode (configured via
pdk.GetConfig) that returns a minimal TTML document; rebuilt with the
standard Go wasip1 toolchain (GOOS=wasip1 GOARCH=wasm). A new Ginkgo test
asserts Synced==true and the exact cue value, which the old plain-text path
could not produce.
GetLyrics doc comment updated to reflect content-sniffing; a later task will
retarget it to ParseLyrics once that function is introduced.
* test(plugins): validate plugin lyrics auto-detect across all formats
The test-lyrics WASM plugin now supports per-format modes via the
"format" config key: ttml, srt, yaml, lrc, and plain, in addition to
the existing default plain-text response. The plugin is rebuilt with the
standard Go wasip1 compiler.
lyrics_adapter_test.go gains a DescribeTable covering all five formats,
asserting both Synced (the discriminator that proves correct format
detection) and the exact line value. This validates the full
auto-detect chain (TTML → SRT → YAML/Lyricsfile → LRC → plain) end-to-end
through the real plugin → adapter → parser flow.
* refactor(lyrics): consolidate parsers into model.ParseLyrics
* refactor(lyrics): retarget legacy callers to model.ParseLyrics
Pin suffix to ".lrc" to preserve byte-identical output for stored
plain/LRC text that was previously handled by the now-removed ToLyrics.
* test(lyrics): fix lyrics tests after parser consolidation
- Rewrite the YAML-fallback test to assert the correct design: a
non-Lyricsfile .yaml sidecar returns as plain text and shadows
lower-priority sources (rather than falling through to .lrc).
- Add LibraryPath + relative Path split to the three subsonic tests
that read sidecar files via storage.For(), so they resolve against
the correct fixtures directory.
- Register a no-op extractor in api_suite_test.go BeforeSuite so
newLocalStorage does not fatal when storage.For is called during
sidecar-lyrics tests.
* test(lyrics): add per-format ParseLyrics benchmarks
Baseline measurements (count=2 runs) on M2:
BenchmarkParseLyrics_LRC-8 5725 178796 ns/op 49.78 MB/s 427877 B/op 523 allocs/op
BenchmarkParseLyrics_Plain-8 5425 230854 ns/op 32.44 MB/s 102508 B/op 16 allocs/op
BenchmarkParseLyrics_EnhancedLRC-8 1942 605893 ns/op 17.66 MB/s 860678 B/op 4256 allocs/op
BenchmarkParseLyrics_SRT-8 3249 373991 ns/op 25.91 MB/s 1113575 B/op 4407 allocs/op
BenchmarkParseLyrics_TTML-8 1483 813027 ns/op 13.86 MB/s 2198052 B/op 8665 allocs/op
BenchmarkParseLyrics_YAML-8 1700 678250 ns/op 13.01 MB/s 1235096 B/op 8288 allocs/op
BenchmarkParseLyrics_SniffTTML-8 1525 776482 ns/op 14.51 MB/s 2225448 B/op 8681 allocs/op
BenchmarkParseLyrics_SniffSRT-8 2528 451210 ns/op 21.48 MB/s 1157000 B/op 4422 allocs/op
BenchmarkParseLyrics_SniffYAML-8 1333 827152 ns/op 10.67 MB/s 1337195 B/op 8718 allocs/op
BenchmarkParseLyrics_SniffLRC-8 2820 413038 ns/op 21.55 MB/s 588934 B/op 1812 allocs/op
BenchmarkParseLyrics_SniffPlain-8 2968 409091 ns/op 18.31 MB/s 254470 B/op 1491 allocs/op
Content-sniff path overhead: 1.5–15% depending on format.
* test(lyrics): use real public-domain fixtures for parser benchmarks
Replace synthetic benchmark payloads with 'Auld Lang Syne' (Robert Burns,
1788, public domain) rendered into every supported format (LRC, plain,
enhanced LRC, SRT, TTML, Lyricsfile YAML) so the numbers reflect realistic
content. Same song across formats makes per-format cost comparable.
Baseline (Apple M-series, -benchmem, real fixtures):
LRC ~28 us/op 42 KB 147 allocs
Plain ~23 us/op 18 KB 22 allocs
EnhancedLRC ~37 us/op 51 KB 374 allocs
SRT ~52 us/op 139 KB 581 allocs
TTML ~119 us/op 276 KB 1227 allocs
YAML ~142 us/op 193 KB 1732 allocs
Sniff(LRC) ~47 us/op 57 KB 237 allocs
Sniff(TTML) ~122 us/op 282 KB 1250 allocs
Sniff(YAML) ~186 us/op 218 KB 1847 allocs
Fixtures in tests/fixtures/lyrics/.
* fix(lyrics): preserve [] (not null) for empty lyrics in backfill migration
ParseLyrics returns nil for zero-line input (whitespace-only stored
lyrics). json.Marshal(nil LyricList) produces null, violating the DB
invariant that media_file.lyrics uses [] for empty lyrics, never null.
Initialize to model.LyricList{} when ParseLyrics returns nil so the
marshalled result is always [].
* 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.
* refactor(lyrics): single registry drives both suffix dispatch and sniff order
Collapse the bySuffix map and sniffOrder slice into one ordered registry:
slice order is the content-sniff probe order, each row's suffixes drive
sidecar dispatch, and per-row bySuffix/byContent parsers preserve the
gated-TTML-when-sniffing distinction. One source of truth, no duplicated
parser references.
* refactor(lyrics): self-skipping parsers collapse the format table to one column
Move the TTML <tt>-document gate into parseTTMLWithDefaultLang itself (after
the encoding fixup, so UTF-16-declared docs are still recognized): non-TTML
content returns (nil, nil) to skip; a malformed <tt> document still errors.
SRT and Lyricsfile YAML already self-skip. With every structured parser
self-skipping, the format table drops to one {suffixes, parse} column named
lyricFormats — no bySuffix/byContent split, no separate sniff-only TTML gate.
Both the suffix and content-sniff paths share the same parser per format.
* refactor(lyrics): strip BOM once at ParseLyrics entry for all paths
Previously only the content-sniff path stripped the BOM; the suffix path
relied on its callers (fromExternalFile via UTF8Reader) having already
stripped it. That implicit contract was fragile — a caller passing raw
BOM-prefixed bytes with a suffix would reach the parsers with the BOM intact
(SanitizeText does not strip it). Strip once at entry so every path and
parser sees clean bytes regardless of caller. No-op for already-stripped
input.
* refactor(lyrics): trim verbose comments to essential why
* refactor(lyrics): move LRC parser to its own lyrics_lrc.go
Extract parseLRC, the enhanced-LRC helpers (parseEnhancedLine, adjustGroup,
stripEnhancedMarkers, shiftELRCCues), parseTime, and the LRC regexes from
lyrics.go into lyrics_lrc.go, with the parseLRC tests in lyrics_lrc_test.go.
This makes the layout symmetric — one file per format (lrc/srt/ttml/yaml) —
and leaves lyrics.go holding only shared types and cue normalization. All
moved symbols were already LRC-private; no behavior change.
* refactor(lyrics): collapse ParseLyrics suffix/sniff branches into one loop
Both modes differ only in which formats to try, so select candidates in a
single loop (all formats when sniffing, the suffix's own otherwise) and run
them through parseFirstMatch once. Drops the projected-slice make+index and
the ContainsFunc closure; unmatched suffixes yield no candidates and fall to
the plain-text floor, as before.
* refactor(lyrics): apply simplify-review cleanups
- stripBOM: bytes.TrimPrefix instead of []byte<->string round-trip (no alloc)
- ParseLyrics: pre-size the candidates slice
- move isTTMLDocument to lyrics_ttml.go beside its only caller (the dispatch
layer should hold no per-format knowledge)
* refactor(lyrics): simplify test descriptions for structured lyrics
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(lyrics): fold parseLyricsfile into lyricParser signature and rename file
- parseLyricsfile now matches the lyricParser signature directly (reads via
bytes.NewReader), removing the parseLyricsfileBytes adapter and the
string(contents) copy; the lyricFormats table references it directly.
- StructuredLyrics drops the vestigial LyricList{} init (json.Unmarshal
overwrites; MarshalJSON owns the empty->[] invariant).
- Rename lyricsfile.go -> lyrics_lyricsfile.go (and its test) to match the
lyrics_<format>.go convention used by lrc/srt/ttml.
* refactor(lyrics): move test-only parseTTML/parseSRT wrappers to test files
These zero-arg wrappers (defaulting lang to "xxx") had no production callers
after the consolidation — only the format tests used them. Move each beside
its tests so the production files carry no test-only code.
* build: exclude generated *_gen.go files from linting
The plugin host *_gen.go files (ndpgen output) were tripping the whitespace
linter despite carrying a generated marker. Exclude them by path so make lint
and the pre-push hook pass on untouched generated code.
* perf(lyrics): drop []byte/string round-trips in parsers
Apply code-review feedback to remove avoidable allocations in the lyrics
parsers. isTTMLDocument now takes []byte directly, so parseTTMLWithDefaultLang
no longer copies its buffer into a string before the TTML probe. parseSRTBlock
splits its block with strings.Split instead of converting to []byte and back
per line. ParseLyrics hoists strings.ToLower(suffix) out of the format loop.
No behavior change; the dropped len(scanner)==0 SRT guard was dead (strings.Split
never returns an empty slice, and the existing len(lines)==0 check still covers
empty input).
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(lyrics): colocate and unexport cue-normalization helpers
Move the cue-normalization machinery out of lyrics.go into a dedicated
lyrics_normalize.go (with lyrics_normalize_test.go), leaving lyrics.go to hold
just the shared lyric types and their methods. lyrics.go was mixing the domain
type/contract definitions with format-agnostic post-processing.
Unexport normalizeLyrics, normalizeCueLines, and normalizeLineTiming: they have
no callers outside the model package, so they should not be part of its public
API. NormalizeCueEnds stays exported because the Subsonic enhanced-lyrics
serializer (server/subsonic/lyrics.go) resolves cue ends per agent group while
building the response; that is the only legitimate cross-package caller.
Also includes a small no-op robustness tweak in parseLRC: len(times) == 0
instead of times == nil (equivalent here, more idiomatic).
No behavior change.
* test(lyrics): add direct coverage for NormalizeCueEnds
NormalizeCueEnds is exported and carries the most intricate logic in the
normalization cluster (fill-from-next, fill-from-fallback, both clamps, and the
all-or-none clear), but was only exercised transitively. Add a focused spec
covering each branch plus the empty-input and no-mutation guarantees, bringing
the function to 100% coverage.
* test(lyrics): cover legacy getLyrics across formats and sources
Expand the legacy getLyrics e2e coverage from a single embedded-plain case to a
table over all six fixtures: embedded LRC/plain/TTML and sidecar LRC/SRT/YAML.
Each case asserts the v1 plain-text fallback contract — the structured lyric is
flattened to LRC-style plain text with no timing markup leaking through (no LRC
brackets, SRT arrows, or XML tags), regardless of the source format or whether
it is embedded or a sidecar file. This pins the behavior that synced TTML/SRT/
YAML formats degrade gracefully to plain text on the legacy endpoint.
* test(lyrics): cover songLyrics v1 vs v2 with word-level fixtures
Correct and expand the e2e lyrics coverage to match the OpenSubsonic songLyrics
extension contract:
- v1 (getLyricsBySongId, no enhanced): line-level lyrics with no cueLine, kind,
or agents — even for word-level formats (ELRC, Lyricsfile YAML).
- v2 (getLyricsBySongId?enhanced=true): word-level cueLine surfaces for ELRC and
YAML sources; kind="main" is set; a line-level source (SRT) still yields no
cueLine even when enhanced.
- legacy getLyrics (artist/title): the original Subsonic endpoint, flattening any
format to plain text. A prior commit mislabeled this as the "v1 contract";
getLyrics predates OpenSubsonic and is unrelated to the extension versions.
Drive these with the public-domain tests/fixtures/lyrics files (the same set the
parser benchmarks use) so the e2e content stays in sync and actually carries the
word-level timing needed to distinguish v1 from v2. The embedded "synced LRC"
fixture is upgraded to ELRC (word-level); track counts are unchanged, so the
rest of the suite is unaffected.
* test(lyrics): parameterize v2 enhanced coverage across all formats
Convert the v2 (enhanced) e2e block from three ad-hoc cases into a DescribeTable
covering all six formats, matching the v1 and legacy tables. Each entry declares
whether the source carries word-level timing: ELRC, TTML, and Lyricsfile YAML
surface a cueLine; LRC, SRT, and plain text do not. All six get kind="main".
Add word-level <span> timing to the first line of the auld-lang-syne.ttml
fixture so TTML exercises the word-level cueLine path (the parser already
supports <span begin/end>, but the fixture was line-level only). The first line
now yields the same five word cues as the ELRC and YAML fixtures, keeping the
table assertions uniform across formats.
* fix(lyrics): honor caller language when Lyricsfile YAML omits it
parseLyricsfile discarded the caller's language argument, so a Lyricsfile YAML
parsed from an embedded tag or plugin response with no metadata.language was
labeled "xxx" even when ParseLyrics was given a language. The SRT and TTML
parsers already use the caller language as their default; fall back to it here
too, preferring the document's own metadata.language when present.
Also reword a misleading TTML comment: isTTMLDocument still runs an XML decode
(it stops at the first element), so the skip avoids the full TTML parse, not the
XML decoder entirely.
* refactor(lyrics): consolidate lyrics parsing functions names
Signed-off-by: Deluan <deluan@navidrome.org>
* test(lyrics): drop test-only parse wrappers after parser rename
Commit 48c0173e8 renamed the production parsers to parseTTML/parseSRT, which
collided with the same-named test-only wrappers and broke the model test build
(parseTTML/parseSRT redeclared). Remove the wrappers and call the production
parsers directly with the placeholder language at each test site.
* test(lyrics): complete the truncated enhanced-LRC fixture
The auld-lang-syne.elrc fixture stopped after the first two stanzas (8 lyric
lines) while every other format fixture carries the full 24-line song. Extend it
to all 24 lines with per-word timing so it is a faithful enhanced-LRC sample and
the EnhancedLRC parser benchmark runs on a workload comparable to the others.
The first line's word timings are unchanged, so the e2e cueLine assertions still
hold.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
79 lines
1.5 KiB
YAML
79 lines
1.5 KiB
YAML
version: "2"
|
|
run:
|
|
build-tags:
|
|
- netgo
|
|
- sqlite_fts5
|
|
linters:
|
|
enable:
|
|
- asasalint
|
|
- asciicheck
|
|
- bidichk
|
|
- bodyclose
|
|
- copyloopvar
|
|
- dogsled
|
|
- durationcheck
|
|
- errorlint
|
|
- forbidigo
|
|
- gocritic
|
|
- gocyclo
|
|
- goprintffuncname
|
|
- gosec
|
|
- misspell
|
|
- nakedret
|
|
- nilerr
|
|
- rowserrcheck
|
|
- unconvert
|
|
- whitespace
|
|
disable:
|
|
- staticcheck
|
|
settings:
|
|
gocritic:
|
|
disable-all: true
|
|
enabled-checks:
|
|
- deprecatedComment
|
|
gosec:
|
|
excludes:
|
|
- G501
|
|
- G401
|
|
- G505
|
|
- G115
|
|
forbidigo:
|
|
forbid:
|
|
- pattern: 'tx\.Exec$'
|
|
msg: "use tx.ExecContext(ctx, ...) in migrations to propagate context"
|
|
- pattern: 'tx\.Query$'
|
|
msg: "use tx.QueryContext(ctx, ...) in migrations to propagate context"
|
|
- pattern: 'tx\.QueryRow$'
|
|
msg: "use tx.QueryRowContext(ctx, ...) in migrations to propagate context"
|
|
govet:
|
|
enable:
|
|
- nilness
|
|
exclusions:
|
|
rules:
|
|
- linters:
|
|
- gosec
|
|
path: _test\.go
|
|
text: "G703"
|
|
- path-except: 'db/migrations/'
|
|
linters:
|
|
- forbidigo
|
|
generated: lax
|
|
presets:
|
|
- comments
|
|
- common-false-positives
|
|
- legacy
|
|
- std-error-handling
|
|
paths:
|
|
- third_party$
|
|
- builtin$
|
|
- examples$
|
|
- node_modules
|
|
- _gen\.go$
|
|
formatters:
|
|
exclusions:
|
|
generated: lax
|
|
paths:
|
|
- third_party$
|
|
- builtin$
|
|
- examples$
|
|
- node_modules
|