From 4cbba2ae49401d7ccda606a2dbfcbee41d787857 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Thu, 2 Jul 2026 08:29:44 -0400 Subject: [PATCH] feat(scanner): add ArtistSplitExceptions to protect artist names from splitting (#5701) * refactor(scanner): make tag value splitting position-based Replaces the ZWSP substitution trick with index-based cutting, in preparation for artist split exceptions, which need match positions. * feat(scanner): protect whitelisted names in tag value splitting Separator matches inside word-bounded exception matches no longer split. Matching is case-insensitive and longest-first; boundaries are rune-aware. * feat(scanner): add Scanner.ArtistSplitExceptions config option * feat(scanner): honor artist split exceptions for participant tags Applies Scanner.ArtistSplitExceptions to artist, albumartist and role tag splitting. Generic tags (genre, mood, ...) are unaffected. * fix(scanner): apply split exceptions when per-tag Split overrides participant tags Per-tag Tags..Split makes the generic ingestion path split the tag before participant mapping runs, bypassing the whitelist. Attach the exceptions to participant tag mappings (including sort variants) in clean(). * feat(scanner): split performer names and honor split exceptions Performer pair values were never split; multiple names in one PERFORMER value stayed a single artist. Split them with the roles separators, using the same whitelist protection as other participant tags. * test(scanner): lock MBID ordering for split performer values * refactor(scanner): consolidate split-exception wiring and drop hot-path lock ArtistSplitExceptionsRx is called per tag mapping per scanned file across concurrent goroutines; replace the mutex+joined-key cache with an atomic pointer compared via slices.Equal. Route all participant call sites through WithParticipantExceptions and a shared splitParticipantValues helper. * refactor(scanner): unexport artistSplitExceptionsRx All external callers go through WithParticipantExceptions, so the accessor does not need to be part of the model package API. --- conf/configuration.go | 24 ++-- model/metadata/map_participants.go | 42 ++++--- model/metadata/map_participants_test.go | 89 ++++++++++++++ model/metadata/metadata.go | 1 + model/tag_mappings.go | 154 +++++++++++++++++++++--- model/tag_mappings_test.go | 130 ++++++++++++++++++++ 6 files changed, 396 insertions(+), 44 deletions(-) diff --git a/conf/configuration.go b/conf/configuration.go index 665a7992f..17abfe400 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -153,17 +153,18 @@ type configOptions struct { } type scannerOptions struct { - Enabled bool - Schedule string - WatcherWait time.Duration - ScanOnStartup bool - Extractor string - ArtistJoiner string - GenreSeparators string // Deprecated: Use Tags.genre.Split instead - GroupAlbumReleases bool // Deprecated: Use PID.Album instead - FollowSymlinks bool // Whether to follow symlinks when scanning directories - IgnoreDotFolders bool // Whether to ignore folders whose name starts with a dot when scanning - PurgeMissing string // Values: "never", "always", "full" + Enabled bool + Schedule string + WatcherWait time.Duration + ScanOnStartup bool + Extractor string + ArtistJoiner string + ArtistSplitExceptions []string // Artist names never split by tag separators + GenreSeparators string // Deprecated: Use Tags.genre.Split instead + GroupAlbumReleases bool // Deprecated: Use PID.Album instead + FollowSymlinks bool // Whether to follow symlinks when scanning directories + IgnoreDotFolders bool // Whether to ignore folders whose name starts with a dot when scanning + PurgeMissing string // Values: "never", "always", "full" } type transcodingOptions struct { @@ -819,6 +820,7 @@ func setViperDefaults() { viper.SetDefault("scanner.watcherwait", consts.DefaultWatcherWait) viper.SetDefault("scanner.scanonstartup", true) viper.SetDefault("scanner.artistjoiner", consts.ArtistJoiner) + viper.SetDefault("scanner.artistsplitexceptions", []string{}) viper.SetDefault("scanner.genreseparators", "") viper.SetDefault("scanner.groupalbumreleases", false) viper.SetDefault("scanner.followsymlinks", true) diff --git a/model/metadata/map_participants.go b/model/metadata/map_participants.go index e8be6aaab..35f112a92 100644 --- a/model/metadata/map_participants.go +++ b/model/metadata/map_participants.go @@ -94,18 +94,20 @@ func (md Metadata) processPerformers(participants model.Participants, rolesMbzId roleIdx[role] = 0 } + conf := model.TagRolesConf().WithParticipantExceptions(model.TagPerformer) titleCaser := cases.Title(language.Und) for _, performer := range md.Pairs(model.TagPerformer) { - name := performer.Value() subRole := titleCaser.String(performer.Key()) - - artist := model.Artist{ - ID: md.artistID(name), - Name: name, - OrderArtistName: str.SanitizeFieldForSortingNoArticle(name), - MbzArtistID: md.getPerformerMbid(subRole, rolesMbzIdMap, roleIdx), + names := splitParticipantValues(conf, []string{performer.Value()}) + for _, name := range names { + artist := model.Artist{ + ID: md.artistID(name), + Name: name, + OrderArtistName: str.SanitizeFieldForSortingNoArticle(name), + MbzArtistID: md.getPerformerMbid(subRole, rolesMbzIdMap, roleIdx), + } + participants.AddWithSubRole(model.RolePerformer, subRole, artist) } - participants.AddWithSubRole(model.RolePerformer, subRole, artist) } } @@ -171,6 +173,16 @@ func (md Metadata) buildArtists(names, sorts, mbids []string) []model.Artist { return artists } +// splitParticipantValues splits values by the conf separators, dropping +// duplicated or empty entries. Values are returned unchanged when the conf +// has no separators. +func splitParticipantValues(conf model.TagConf, values []string) []string { + if len(conf.Split) == 0 { + return values + } + return filterDuplicatedOrEmptyValues(conf.SplitTagValue(values)) +} + // getRoleValues returns the values of a role tag, splitting them if necessary func (md Metadata) getRoleValues(role model.TagName) []string { values := md.Strings(role) @@ -181,11 +193,8 @@ func (md Metadata) getRoleValues(role model.TagName) []string { if conf.Split == nil { conf = model.TagRolesConf() } - if len(conf.Split) > 0 { - values = conf.SplitTagValue(values) - return filterDuplicatedOrEmptyValues(values) - } - return values + conf = conf.WithParticipantExceptions(role) + return splitParticipantValues(conf, values) } // getArtistValues returns the values of a single or multi artist tag, splitting them if necessary @@ -202,11 +211,8 @@ func (md Metadata) getArtistValues(single, multi model.TagName) []string { if conf.Split == nil { conf = model.TagArtistsConf() } - if len(conf.Split) > 0 { - vSingle = conf.SplitTagValue(vSingle) - return filterDuplicatedOrEmptyValues(vSingle) - } - return vSingle + conf = conf.WithParticipantExceptions(single) + return splitParticipantValues(conf, vSingle) } func (md Metadata) mapDisplayName(singularTagName, pluralTagName model.TagName) string { diff --git a/model/metadata/map_participants_test.go b/model/metadata/map_participants_test.go index 5ee802ced..ec66e12b9 100644 --- a/model/metadata/map_participants_test.go +++ b/model/metadata/map_participants_test.go @@ -4,6 +4,8 @@ import ( "os" "github.com/google/uuid" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/metadata" @@ -563,6 +565,37 @@ var _ = Describe("Participants", func() { matchPerformer("Tim Carmon", "tim carmon", "Hammond Organ"), )) }) + + It("should split multiple names in a single value", func() { + mf = toMediaFile(model.RawTags{ + "PERFORMER:GUITAR": {"Eric Clapton/B.B. King"}, + "PERFORMER:BASS": {"Nathan East"}, + }) + + participants := mf.Participants + Expect(participants).To(HaveKeyWithValue(model.RolePerformer, HaveLen(3))) + + p := participants[model.RolePerformer] + Expect(p).To(ContainElements( + matchPerformer("Eric Clapton", "eric clapton", "Guitar"), + matchPerformer("B.B. King", "b.b. king", "Guitar"), + matchPerformer("Nathan East", "nathan east", "Bass"), + )) + }) + + It("should assign MBIDs in order to names split from a single value", func() { + mf = toMediaFile(model.RawTags{ + "PERFORMER:GUITAR": {"Eric Clapton/B.B. King"}, + "MUSICBRAINZ_PERFORMERID:GUITAR": {mbid1, mbid2}, + }) + + p := mf.Participants[model.RolePerformer] + Expect(p).To(HaveLen(2)) + Expect(p[0].Name).To(Equal("Eric Clapton")) + Expect(p[0].MbzArtistID).To(Equal(mbid1)) + Expect(p[1].Name).To(Equal("B.B. King")) + Expect(p[1].MbzArtistID).To(Equal(mbid2)) + }) }) When("MUSICBRAINZ_PERFORMERID tag is set", func() { @@ -802,4 +835,60 @@ var _ = Describe("Participants", func() { } }) }) + + Describe("Artist split exceptions", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + It("does not split a whitelisted artist name on the default separators", func() { + // " feat. " is a default artists separator (mappings.yaml) + conf.Server.Scanner.ArtistSplitExceptions = []string{"Someone feat. Else"} + mf = toMediaFile(model.RawTags{ + "ARTIST": {"Artist Name feat. Someone feat. Else"}, + }) + + artists := mf.Participants[model.RoleArtist] + Expect(artists).To(HaveLen(2)) + Expect(artists[0].Name).To(Equal("Artist Name")) + Expect(artists[1].Name).To(Equal("Someone feat. Else")) + }) + + It("does not split a whitelisted name in role tags", func() { + // "/" is a default roles separator (mappings.yaml) + conf.Server.Scanner.ArtistSplitExceptions = []string{"AC/DC"} + mf = toMediaFile(model.RawTags{ + "COMPOSER": {"AC/DC/John Doe"}, + }) + + composers := mf.Participants[model.RoleComposer] + Expect(composers).To(HaveLen(2)) + Expect(composers[0].Name).To(Equal("AC/DC")) + Expect(composers[1].Name).To(Equal("John Doe")) + }) + + It("splits normally when the exception does not match", func() { + conf.Server.Scanner.ArtistSplitExceptions = []string{"Iron and Wine"} + mf = toMediaFile(model.RawTags{ + "ARTIST": {"Artist Name feat. Someone Else"}, + }) + + artists := mf.Participants[model.RoleArtist] + Expect(artists).To(HaveLen(2)) + Expect(artists[0].Name).To(Equal("Artist Name")) + Expect(artists[1].Name).To(Equal("Someone Else")) + }) + + It("does not split a whitelisted name in performer tags", func() { + conf.Server.Scanner.ArtistSplitExceptions = []string{"AC/DC"} + mf = toMediaFile(model.RawTags{ + "PERFORMER:GUITAR": {"AC/DC/Brian Johnson"}, + }) + + performers := mf.Participants[model.RolePerformer] + Expect(performers).To(HaveLen(2)) + Expect(performers[0].Name).To(Equal("AC/DC")) + Expect(performers[1].Name).To(Equal("Brian Johnson")) + }) + }) }) diff --git a/model/metadata/metadata.go b/model/metadata/metadata.go index 48928f989..729e83564 100644 --- a/model/metadata/metadata.go +++ b/model/metadata/metadata.go @@ -205,6 +205,7 @@ func clean(filePath string, tags model.RawTags) model.Tags { cleaned := make(model.Tags, len(mappings)) for name, mapping := range mappings { + mapping = mapping.WithParticipantExceptions(name) var values []string switch mapping.Type { case model.TagTypePair: diff --git a/model/tag_mappings.go b/model/tag_mappings.go index dd19a157b..ce7d2f37b 100644 --- a/model/tag_mappings.go +++ b/model/tag_mappings.go @@ -7,9 +7,11 @@ import ( "slices" "strings" "sync" + "sync/atomic" + "unicode" + "unicode/utf8" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/resources" @@ -26,12 +28,13 @@ type mappingsConf struct { type tagMappings map[TagName]TagConf type TagConf struct { - Aliases []string `yaml:"aliases"` - Type TagType `yaml:"type"` - MaxLength int `yaml:"maxLength"` - Split []string `yaml:"split"` - Album bool `yaml:"album"` - SplitRx *regexp.Regexp `yaml:"-"` + Aliases []string `yaml:"aliases"` + Type TagType `yaml:"type"` + MaxLength int `yaml:"maxLength"` + Split []string `yaml:"split"` + Album bool `yaml:"album"` + SplitRx *regexp.Regexp `yaml:"-"` + ExceptionsRx *regexp.Regexp `yaml:"-"` } // SplitTagValue splits tag values by the configured split separators. @@ -43,18 +46,139 @@ func (c TagConf) SplitTagValue(values []string) []string { 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.SplitSeq(tag, consts.Zwsp) - for part := range parts { - result = append(result, strings.TrimSpace(part)) - } + result = append(result, c.splitValue(tag)...) } return result } +func (c TagConf) splitValue(tag string) []string { + protected := protectedSpans(tag, c.ExceptionsRx) + var parts []string + start := 0 + for _, sep := range c.SplitRx.FindAllStringIndex(tag, -1) { + if overlapsAny(sep, protected) { + continue + } + parts = append(parts, strings.TrimSpace(tag[start:sep[0]])) + start = sep[1] + } + return append(parts, strings.TrimSpace(tag[start:])) +} + +// protectedSpans returns the spans of rx matches that sit on word boundaries. +// Boundaries are checked here, rune-aware, because RE2's \b is ASCII-only and +// would silently never match names starting/ending with accented letters. +func protectedSpans(tag string, rx *regexp.Regexp) [][]int { + if rx == nil { + return nil + } + var spans [][]int + for _, span := range rx.FindAllStringIndex(tag, -1) { + if isWordBounded(tag, span[0], span[1]) { + spans = append(spans, span) + } + } + return spans +} + +func isWordBounded(s string, start, end int) bool { + isWord := func(r rune) bool { return unicode.IsLetter(r) || unicode.IsDigit(r) } + before, _ := utf8.DecodeLastRuneInString(s[:start]) + after, _ := utf8.DecodeRuneInString(s[end:]) + return !isWord(before) && !isWord(after) +} + +func overlapsAny(span []int, spans [][]int) bool { + for _, s := range spans { + if span[0] < s[1] && s[0] < span[1] { + return true + } + } + return false +} + +// compileExceptionsRegex builds a case-insensitive regex matching any of the +// given literal names, or nil if there are none. +func compileExceptionsRegex(exceptions []string) *regexp.Regexp { + var names []string + for _, e := range exceptions { + if e = strings.TrimSpace(e); e != "" { + names = append(names, e) + } + } + if len(names) == 0 { + return nil + } + // Longest-first: Go regex alternation is leftmost-first, so with overlapping + // entries (e.g. "Iron and Wine Duo" vs "Iron and Wine") the longer name must + // come first to win. Ties broken lexicographically for determinism. + slices.SortFunc(names, func(a, b string) int { + if c := cmp.Compare(len(b), len(a)); c != 0 { + return c + } + return cmp.Compare(a, b) + }) + escaped := make([]string, len(names)) + for i, name := range names { + escaped[i] = regexp.QuoteMeta(name) + } + rx, err := regexp.Compile("(?i)(" + strings.Join(escaped, "|") + ")") + if err != nil { + log.Warn("Error compiling split exceptions regexp", "exceptions", exceptions, err) + return nil + } + return rx +} + +type artistSplitExceptionsCache struct { + names []string + rx *regexp.Regexp +} + +var artistSplitExceptions atomic.Pointer[artistSplitExceptionsCache] + +// artistSplitExceptionsRx returns the regex for Scanner.ArtistSplitExceptions, +// or nil if none are configured. Compiled lazily (config hooks only run once +// per process, before tests can override the option) and cached until the +// configured list changes. Lock-free on the cache-hit path, as this is called +// per tag mapping per scanned file, across concurrent scanner goroutines. +func artistSplitExceptionsRx() *regexp.Regexp { + names := conf.Server.Scanner.ArtistSplitExceptions + if c := artistSplitExceptions.Load(); c != nil && slices.Equal(c.names, names) { + return c.rx + } + c := &artistSplitExceptionsCache{names: slices.Clone(names), rx: compileExceptionsRegex(names)} + artistSplitExceptions.Store(c) + return c.rx +} + +// participantTagNames are the tags that hold artist names (or their sort +// values), where split exceptions apply. +var participantTagNames = sync.OnceValue(func() map[TagName]struct{} { + names := []TagName{ + TagTrackArtist, TagTrackArtists, TagTrackArtistSort, TagTrackArtistsSort, + TagAlbumArtist, TagAlbumArtists, TagAlbumArtistSort, TagAlbumArtistsSort, + } + set := make(map[TagName]struct{}, len(names)+2*len(AllRoles)) + for _, n := range names { + set[n] = struct{}{} + } + for role := range AllRoles { + set[TagName(role)] = struct{}{} + set[TagName(role+"sort")] = struct{}{} + } + return set +}) + +// WithParticipantExceptions returns the conf with the global artist split +// exceptions attached when name is a participant (artist/role) tag. +func (c TagConf) WithParticipantExceptions(name TagName) TagConf { + if _, ok := participantTagNames()[name]; ok { + c.ExceptionsRx = artistSplitExceptionsRx() + } + return c +} + type TagType string const ( diff --git a/model/tag_mappings_test.go b/model/tag_mappings_test.go index 1665d557b..e582c3f2f 100644 --- a/model/tag_mappings_test.go +++ b/model/tag_mappings_test.go @@ -1,6 +1,8 @@ package model import ( + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -60,5 +62,133 @@ var _ = Describe("TagConf", func() { // filterDuplicatedOrEmptyValues in the metadata pipeline. Expect(conf.SplitTagValue([]string{"Rock//Pop"})).To(Equal([]string{"Rock", "", "Pop"})) }) + + Context("with split exceptions", func() { + BeforeEach(func() { + conf = TagConf{Split: []string{" and ", ";", "/"}} + conf.SplitRx = compileSplitRegex("test", conf.Split) + conf.ExceptionsRx = compileExceptionsRegex([]string{ + "Iron and Wine", + "Iron and Wine Duo", + "Ella and Louis", + "AC/DC", + "Ólafur Arnalds and Nils Frahm", + }) + }) + + It("does not split a value that is exactly an exception", func() { + Expect(conf.SplitTagValue([]string{"Iron and Wine"})).To(Equal([]string{"Iron and Wine"})) + }) + + It("protects an exception embedded in a multi-artist value", func() { + Expect(conf.SplitTagValue([]string{"Iron and Wine and Bob"})). + To(Equal([]string{"Iron and Wine", "Bob"})) + }) + + It("protects every occurrence, not just the first", func() { + Expect(conf.SplitTagValue([]string{"Iron and Wine; Bob; Iron and Wine"})). + To(Equal([]string{"Iron and Wine", "Bob", "Iron and Wine"})) + }) + + It("matches exceptions case-insensitively and keeps the tag's casing", func() { + Expect(conf.SplitTagValue([]string{"IRON AND WINE and Bob"})). + To(Equal([]string{"IRON AND WINE", "Bob"})) + }) + + It("prefers the longest exception when entries overlap", func() { + Expect(conf.SplitTagValue([]string{"Iron and Wine Duo and Bob"})). + To(Equal([]string{"Iron and Wine Duo", "Bob"})) + }) + + It("protects exceptions containing separator characters", func() { + Expect(conf.SplitTagValue([]string{"AC/DC/Queen"})). + To(Equal([]string{"AC/DC", "Queen"})) + }) + + It("does not protect an exception embedded in a longer word", func() { + // "Ella and Louis" must not match inside "Ella and Louise" + Expect(conf.SplitTagValue([]string{"Ella and Louise"})). + To(Equal([]string{"Ella", "Louise"})) + }) + + It("handles names with non-ASCII edges", func() { + Expect(conf.SplitTagValue([]string{"Ólafur Arnalds and Nils Frahm and Bob"})). + To(Equal([]string{"Ólafur Arnalds and Nils Frahm", "Bob"})) + }) + + It("splits normally when no exception matches", func() { + Expect(conf.SplitTagValue([]string{"Foo and Bar"})).To(Equal([]string{"Foo", "Bar"})) + }) + }) + }) + + Describe("compileExceptionsRegex", func() { + It("returns nil for an empty list", func() { + Expect(compileExceptionsRegex(nil)).To(BeNil()) + Expect(compileExceptionsRegex([]string{})).To(BeNil()) + }) + + It("returns nil when all entries are blank", func() { + Expect(compileExceptionsRegex([]string{"", " "})).To(BeNil()) + }) + + It("escapes regex metacharacters in names", func() { + rx := compileExceptionsRegex([]string{"Sigur (Rós)"}) + Expect(rx.FindString("Sigur (Rós)")).To(Equal("Sigur (Rós)")) + Expect(rx.MatchString("Sigur xRósx")).To(BeFalse()) + }) + + It("matches case-insensitively", func() { + rx := compileExceptionsRegex([]string{"Iron and Wine"}) + Expect(rx.MatchString("IRON AND WINE")).To(BeTrue()) + }) + }) + + Describe("artistSplitExceptionsRx", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + }) + + It("returns nil when no exceptions are configured", func() { + conf.Server.Scanner.ArtistSplitExceptions = nil + Expect(artistSplitExceptionsRx()).To(BeNil()) + }) + + It("compiles the configured exceptions", func() { + conf.Server.Scanner.ArtistSplitExceptions = []string{"Iron and Wine"} + rx := artistSplitExceptionsRx() + Expect(rx).ToNot(BeNil()) + Expect(rx.MatchString("iron and wine")).To(BeTrue()) + }) + + It("caches the compiled regex until the configuration changes", func() { + conf.Server.Scanner.ArtistSplitExceptions = []string{"Iron and Wine"} + first := artistSplitExceptionsRx() + Expect(artistSplitExceptionsRx()).To(BeIdenticalTo(first)) + + conf.Server.Scanner.ArtistSplitExceptions = []string{"AC/DC"} + second := artistSplitExceptionsRx() + Expect(second).ToNot(BeIdenticalTo(first)) + Expect(second.MatchString("AC/DC")).To(BeTrue()) + }) + }) + + Describe("WithParticipantExceptions", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.Scanner.ArtistSplitExceptions = []string{"Iron and Wine"} + }) + + It("attaches the exceptions regex to participant tags", func() { + for _, tag := range []TagName{"artist", "albumartist", "artists", "artistsort", "composer", "lyricist", "composersort"} { + Expect(TagConf{}.WithParticipantExceptions(tag).ExceptionsRx).ToNot(BeNil(), string(tag)) + } + }) + + It("does not attach the exceptions regex to non-participant tags", func() { + for _, tag := range []TagName{"genre", "mood", "title", "releasetype"} { + Expect(TagConf{}.WithParticipantExceptions(tag).ExceptionsRx).To(BeNil(), string(tag)) + } + }) }) })