fix(scanner): respect tag split config when multiple frames map to the same tag (#5193)

* fix: split tag values from multiple sources individually

When a file has multiple tag frames mapping to the same logical tag
(e.g. both TXXX:MOOD and TMOO), TagLib merges them into one key with
multiple values. SplitTagValue had a len(values) != 1 guard that
skipped splitting entirely in this case, leaving comma-separated
values unsplit.

Change SplitTagValue to split each value individually regardless of
input count. Empty values are filtered during splitting.

Fixes #5065

* test: cover SplitTagValue with multi-frame regression cases

Add tests pinning the behavior fixed by SplitTagValue iterating over each
input value. The previous len(values) != 1 short-circuit silently skipped
splitting whenever TagLib merged multiple ID3v2 frames into the same
property (e.g. TMOO + TXXX:MOOD for mood, or duplicate TIPL entries for
composer), as reported in #5065.

Three layers of coverage:
- model/tag_mappings_test.go: direct unit tests on TagConf.SplitTagValue
  covering single/multi-value input, case-insensitive separators, missing
  SplitRx, empty input, and the empty-strings-passed-through contract that
  the downstream metadata pipeline relies on.
- model/metadata/metadata_test.go: end-to-end check that a "mood" tag
  surfaced as two raw values (the exact shape from the bug report) is
  split, trimmed, and deduplicated to the expected three moods.
- model/metadata/map_participants_test.go: parallel multi-value case for
  the COMPOSER tag, ensuring the same fix also corrects multi-frame role
  parsing.

All three new specs fail on the pre-fix code and pass on the patched
SplitTagValue.

---------

Co-authored-by: Deluan Quintão <deluan@navidrome.org>
This commit is contained in:
Tom Boucher
2026-05-23 18:20:18 -04:00
committed by GitHub
parent edffca24b1
commit 55a31f30b3
4 changed files with 112 additions and 11 deletions
+20
View File
@@ -684,6 +684,26 @@ var _ = Describe("Participants", func() {
Expect(composers[2].Name).To(Equal("The Album Artist"))
})
})
// Sibling fix to https://github.com/navidrome/navidrome/issues/5065: when
// multiple frames map to the same role tag (e.g. TIPL producer entries),
// the configured split separator must still apply to each value.
When("the tag has multiple values", func() {
It("should split each value individually", func() {
mf = toMediaFile(model.RawTags{
"COMPOSER": {"John Doe/Jane Doe", "Someone Else"},
})
participants := mf.Participants
Expect(participants).To(HaveKeyWithValue(model.RoleComposer, HaveLen(3)))
composers := participants[model.RoleComposer]
Expect(composers).To(ConsistOf(
HaveField("Name", "John Doe"),
HaveField("Name", "Jane Doe"),
HaveField("Name", "Someone Else"),
))
})
})
})
Describe("MBID tags", func() {
+15
View File
@@ -129,6 +129,21 @@ var _ = Describe("Metadata", func() {
Expect(md.Strings(model.TagGenre)).To(Equal([]string{"Rock", "Pop", "Punk"}))
})
// Regression test for https://github.com/navidrome/navidrome/issues/5065
//
// MP3s with both an ID3v2 TMOO frame and a TXXX:MOOD frame are surfaced by
// TagLib's PropertyMap as a single "mood" key with multiple values. The split
// configuration must still apply to each value individually.
It("should split values from multiple frames mapping to the same tag", func() {
props.Tags = model.RawTags{
// Same shape as the bug report: two frames, comma-separated content.
"mood": {"Love, Emotional, Ballad", "Love; Emotional; Ballad"},
}
md = metadata.New(filePath, props)
Expect(md.Strings(model.TagMood)).To(ConsistOf("Love", "Emotional", "Ballad"))
})
})
DescribeTable("Date",
+13 -11
View File
@@ -34,23 +34,25 @@ type TagConf struct {
SplitRx *regexp.Regexp `yaml:"-"`
}
// SplitTagValue splits a tag value by the split separators, but only if it has a single value.
// SplitTagValue splits tag values by the configured split separators.
// Each value in the input slice is individually split and trimmed.
func (c TagConf) SplitTagValue(values []string) []string {
// If there's not exactly one value or no separators, return early.
if len(values) != 1 || c.SplitRx == nil {
if c.SplitRx == nil || len(values) == 0 {
return values
}
tag := values[0]
// Replace all occurrences of any separator with the zero-width space.
tag = c.SplitRx.ReplaceAllString(tag, consts.Zwsp)
var result []string
for _, tag := range values {
// Replace all occurrences of any separator with the zero-width space.
tag = c.SplitRx.ReplaceAllString(tag, consts.Zwsp)
// Split by the zero-width space and trim each substring.
parts := strings.Split(tag, consts.Zwsp)
for i, part := range parts {
parts[i] = strings.TrimSpace(part)
// Split by the zero-width space and trim each substring.
parts := strings.Split(tag, consts.Zwsp)
for _, part := range parts {
result = append(result, strings.TrimSpace(part))
}
}
return parts
return result
}
type TagType string
+64
View File
@@ -0,0 +1,64 @@
package model
import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
var _ = Describe("TagConf", func() {
Describe("SplitTagValue", func() {
var conf TagConf
BeforeEach(func() {
conf = TagConf{Split: []string{";", "/", ","}}
conf.SplitRx = compileSplitRegex("test", conf.Split)
})
It("splits a single value on configured separators", func() {
Expect(conf.SplitTagValue([]string{"Rock/Pop;Punk"})).To(Equal([]string{"Rock", "Pop", "Punk"}))
})
It("trims whitespace around split values", func() {
Expect(conf.SplitTagValue([]string{"Love, Emotional, Ballad"})).To(Equal([]string{"Love", "Emotional", "Ballad"}))
})
// Regression test for https://github.com/navidrome/navidrome/issues/5065
//
// When multiple ID3v2 frames map to the same logical tag (e.g. TMOO + TXXX:MOOD),
// TagLib's PropertyMap merges them into a slice with several entries. Previously
// SplitTagValue had a `len(values) != 1` guard that skipped splitting in this case.
It("splits each value individually when given multiple inputs", func() {
input := []string{"Love, Emotional, Ballad", "Love; Emotional; Ballad"}
Expect(conf.SplitTagValue(input)).To(Equal([]string{
"Love", "Emotional", "Ballad",
"Love", "Emotional", "Ballad",
}))
})
It("matches separators case-insensitively when the split pattern allows", func() {
c := TagConf{Split: []string{" AND "}}
c.SplitRx = compileSplitRegex("test", c.Split)
Expect(c.SplitTagValue([]string{"foo and bar AND baz"})).To(Equal([]string{"foo", "bar", "baz"}))
})
It("returns values unchanged when no separators are configured", func() {
c := TagConf{}
Expect(c.SplitTagValue([]string{"Foo, Bar"})).To(Equal([]string{"Foo, Bar"}))
Expect(c.SplitTagValue([]string{"a", "b"})).To(Equal([]string{"a", "b"}))
})
It("returns an empty slice for empty input", func() {
Expect(conf.SplitTagValue([]string{})).To(BeEmpty())
})
It("handles a value with no separator as a single-element result", func() {
Expect(conf.SplitTagValue([]string{"JustOneMood"})).To(Equal([]string{"JustOneMood"}))
})
It("produces empty strings when separators are adjacent (dedup happens downstream)", func() {
// SplitTagValue itself does not filter empties; that is the job of
// filterDuplicatedOrEmptyValues in the metadata pipeline.
Expect(conf.SplitTagValue([]string{"Rock//Pop"})).To(Equal([]string{"Rock", "", "Pop"}))
})
})
})