diff --git a/cmd/root.go b/cmd/root.go index 08773176a..38c617bfd 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -175,7 +175,13 @@ func pidHashChanged(ds model.DataStore) (bool, error) { if err != nil { return false, err } - return !strings.EqualFold(pidAlbum, conf.Server.PID.Album) || !strings.EqualFold(pidTrack, conf.Server.PID.Track), nil + pidArtist, err := ds.Property(context.Background()).DefaultGet(consts.PIDArtistKey, "") + if err != nil { + return false, err + } + return !strings.EqualFold(pidAlbum, conf.Server.PID.Album) || + !strings.EqualFold(pidTrack, conf.Server.PID.Track) || + !strings.EqualFold(pidArtist, conf.Server.PID.Artist), nil } // runInitialScan runs an initial scan of the music library if needed. diff --git a/conf/configuration.go b/conf/configuration.go index 665a7992f..195bd6e24 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -243,8 +243,9 @@ type backupOptions struct { } type pidOptions struct { - Track string - Album string + Track string + Album string + Artist string } type inspectOptions struct { @@ -440,6 +441,7 @@ func Load(noConfigDump bool) { // Make sure we don't have empty PIDs Server.PID.Album = cmp.Or(Server.PID.Album, consts.DefaultAlbumPID) Server.PID.Track = cmp.Or(Server.PID.Track, consts.DefaultTrackPID) + Server.PID.Artist = cmp.Or(Server.PID.Artist, consts.DefaultArtistPID) // Parse LastFM.Language into Languages slice (comma-separated, with fallback to DefaultInfoLanguage) Server.LastFM.Languages = parseLanguages(Server.LastFM.Language) @@ -853,6 +855,7 @@ func setViperDefaults() { viper.SetDefault("backup.count", 0) viper.SetDefault("pid.track", consts.DefaultTrackPID) viper.SetDefault("pid.album", consts.DefaultAlbumPID) + viper.SetDefault("pid.artist", consts.DefaultArtistPID) viper.SetDefault("inspect.enabled", true) viper.SetDefault("inspect.maxrequests", 1) viper.SetDefault("inspect.backloglimit", consts.RequestThrottleBacklogLimit) diff --git a/consts/consts.go b/consts/consts.go index 3795b590a..27be488ce 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -125,10 +125,12 @@ const ( const ( //DefaultAlbumPID = "album_legacy" - DefaultAlbumPID = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" - DefaultTrackPID = "musicbrainz_trackid|albumid,discnumber,tracknumber,title" - PIDAlbumKey = "PIDAlbum" - PIDTrackKey = "PIDTrack" + DefaultAlbumPID = "musicbrainz_albumid|albumartistid,album,albumversion,releasedate" + DefaultTrackPID = "musicbrainz_trackid|albumid,discnumber,tracknumber,title" + DefaultArtistPID = "name" + PIDAlbumKey = "PIDAlbum" + PIDTrackKey = "PIDTrack" + PIDArtistKey = "PIDArtist" ) const ( diff --git a/db/migrations/20260525000912_set_default_pid_artist.sql b/db/migrations/20260525000912_set_default_pid_artist.sql new file mode 100644 index 000000000..cb12244fe --- /dev/null +++ b/db/migrations/20260525000912_set_default_pid_artist.sql @@ -0,0 +1,11 @@ +-- +goose Up +-- Backfill PIDArtist property to reflect the historical artist-ID computation. +-- Existing artist IDs were produced by the legacy hardcoded artistID() function, +-- which is byte-identical to computeArtistPID(p, "name", ...). Recording "name" +-- here ensures that on the next scan, prevArtistPIDConf is never empty — closing +-- the upgrade-time window where a user who pre-configured a non-default PID.Artist +-- would have artist IDs silently regenerated without annotation migration. +insert into property (id, value) values ('PIDArtist', 'name') on conflict do nothing; + +-- +goose Down +delete from property where id = 'PIDArtist'; diff --git a/model/artist.go b/model/artist.go index 2085f0051..54671c3ef 100644 --- a/model/artist.go +++ b/model/artist.go @@ -89,6 +89,7 @@ type ArtistRepository interface { GetIndex(includeMissing bool, libraryIds []int, roles ...Role) (ArtistIndexes, error) // The following methods are used exclusively by the scanner: + CopyAttributes(fromID, toID string, columns ...string) error RefreshPlayCounts() (int64, error) RefreshStats(allArtists bool) (int64, error) diff --git a/model/metadata/map_participants.go b/model/metadata/map_participants.go index e8be6aaab..ac4370a9f 100644 --- a/model/metadata/map_participants.go +++ b/model/metadata/map_participants.go @@ -7,64 +7,82 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/utils/str" "golang.org/x/text/cases" "golang.org/x/text/language" ) type roleTags struct { - name model.TagName - sort model.TagName - mbid model.TagName + name model.TagName + sort model.TagName + mbid model.TagName + credit model.TagName } var roleMappings = map[model.Role]roleTags{ - model.RoleComposer: {name: model.TagComposer, sort: model.TagComposerSort, mbid: model.TagMusicBrainzComposerID}, - model.RoleLyricist: {name: model.TagLyricist, sort: model.TagLyricistSort, mbid: model.TagMusicBrainzLyricistID}, - model.RoleConductor: {name: model.TagConductor, mbid: model.TagMusicBrainzConductorID}, - model.RoleArranger: {name: model.TagArranger, mbid: model.TagMusicBrainzArrangerID}, - model.RoleDirector: {name: model.TagDirector, mbid: model.TagMusicBrainzDirectorID}, - model.RoleProducer: {name: model.TagProducer, mbid: model.TagMusicBrainzProducerID}, - model.RoleEngineer: {name: model.TagEngineer, mbid: model.TagMusicBrainzEngineerID}, - model.RoleMixer: {name: model.TagMixer, mbid: model.TagMusicBrainzMixerID}, - model.RoleRemixer: {name: model.TagRemixer, mbid: model.TagMusicBrainzRemixerID}, - model.RoleDJMixer: {name: model.TagDJMixer, mbid: model.TagMusicBrainzDJMixerID}, + model.RoleComposer: {name: model.TagComposer, sort: model.TagComposerSort, mbid: model.TagMusicBrainzComposerID, credit: model.TagComposerCredit}, + model.RoleLyricist: {name: model.TagLyricist, sort: model.TagLyricistSort, mbid: model.TagMusicBrainzLyricistID, credit: model.TagLyricistCredit}, + model.RoleConductor: {name: model.TagConductor, mbid: model.TagMusicBrainzConductorID, credit: model.TagConductorCredit}, + model.RoleArranger: {name: model.TagArranger, mbid: model.TagMusicBrainzArrangerID, credit: model.TagArrangerCredit}, + model.RoleDirector: {name: model.TagDirector, mbid: model.TagMusicBrainzDirectorID, credit: model.TagDirectorCredit}, + model.RoleProducer: {name: model.TagProducer, mbid: model.TagMusicBrainzProducerID, credit: model.TagProducerCredit}, + model.RoleEngineer: {name: model.TagEngineer, mbid: model.TagMusicBrainzEngineerID, credit: model.TagEngineerCredit}, + model.RoleMixer: {name: model.TagMixer, mbid: model.TagMusicBrainzMixerID, credit: model.TagMixerCredit}, + model.RoleRemixer: {name: model.TagRemixer, mbid: model.TagMusicBrainzRemixerID, credit: model.TagRemixerCredit}, + model.RoleDJMixer: {name: model.TagDJMixer, mbid: model.TagMusicBrainzDJMixerID, credit: model.TagDJMixerCredit}, } func (md Metadata) mapParticipants() model.Participants { participants := make(model.Participants) - // Parse track artists - artists := md.parseArtists( - model.TagTrackArtist, model.TagTrackArtists, - model.TagTrackArtistSort, model.TagTrackArtistsSort, - model.TagMusicBrainzArtistID, - ) - participants.Add(model.RoleArtist, artists...) + // Parse track artists. MBIDs go through getRoleValues to split on the same + // separators as the names, keeping positional alignment. + trackNames := md.getArtistValues(model.TagTrackArtist, model.TagTrackArtists) + if len(trackNames) == 0 { + trackNames = []string{consts.UnknownArtist} + } + trackSorts := md.getArtistValues(model.TagTrackArtistSort, model.TagTrackArtistsSort) + trackMbids := md.getRoleValues(model.TagMusicBrainzArtistID) + trackCredits := md.getArtistValues(model.TagTrackArtistCredit, model.TagTrackArtistsCredit) + trackArtistParticipants := md.buildParticipants(trackNames, trackSorts, trackMbids, trackCredits) + participants.AddParticipants(model.RoleArtist, trackArtistParticipants...) // Parse album artists - albumArtists := md.parseArtists( - model.TagAlbumArtist, model.TagAlbumArtists, - model.TagAlbumArtistSort, model.TagAlbumArtistsSort, - model.TagMusicBrainzAlbumArtistID, - ) - if len(albumArtists) == 1 && albumArtists[0].Name == consts.UnknownArtist { - if md.Bool(model.TagCompilation) { - albumArtists = md.buildArtists([]string{consts.VariousArtists}, nil, []string{consts.VariousArtistsMbzId}) - } else { - albumArtists = artists - } - } - participants.Add(model.RoleAlbumArtist, albumArtists...) + albumNames := md.getArtistValues(model.TagAlbumArtist, model.TagAlbumArtists) + albumSorts := md.getArtistValues(model.TagAlbumArtistSort, model.TagAlbumArtistsSort) + albumMbids := md.getRoleValues(model.TagMusicBrainzAlbumArtistID) + albumCredits := md.getArtistValues(model.TagAlbumArtistCredit, model.TagAlbumArtistsCredit) - // Parse all other roles + // Treat "no albumartist tag" and "albumartist == UnknownArtist placeholder" + // as missing, matching the prior parseArtists path and defending against + // the placeholder round-tripping back into a tag. + albumArtistMissing := len(albumNames) == 0 || + (len(albumNames) == 1 && albumNames[0] == consts.UnknownArtist) + + var albumArtistParticipants []model.Participant + if albumArtistMissing { + if md.Bool(model.TagCompilation) { + albumArtistParticipants = md.buildParticipants( + []string{consts.VariousArtists}, nil, + []string{consts.VariousArtistsMbzId}, nil) + } else { + albumArtistParticipants = trackArtistParticipants + } + } else { + albumArtistParticipants = md.buildParticipants(albumNames, albumSorts, albumMbids, albumCredits) + } + participants.AddParticipants(model.RoleAlbumArtist, albumArtistParticipants...) + + // All parallel lists go through getRoleValues so they split consistently + // with names (e.g. COMPOSER="A;B" + COMPOSER_CREDIT="AA;BB" → 2 each). for role, info := range roleMappings { names := md.getRoleValues(info.name) if len(names) > 0 { - sorts := md.Strings(info.sort) - mbids := md.Strings(info.mbid) - artists := md.buildArtists(names, sorts, mbids) - participants.Add(role, artists...) + sorts := md.getRoleValues(info.sort) + mbids := md.getRoleValues(info.mbid) + credits := md.getRoleValues(info.credit) + participants.AddParticipants(role, md.buildParticipants(names, sorts, mbids, credits)...) } } @@ -100,12 +118,20 @@ func (md Metadata) processPerformers(participants model.Participants, rolesMbzId subRole := titleCaser.String(performer.Key()) 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) + artist.ID = computeArtistPID( + model.Participant{Artist: artist}, + conf.Server.PID.Artist, + id.NewHash, + ) + participants.AddParticipants(model.RolePerformer, model.Participant{ + Artist: artist, + SubRole: subRole, + CreditedAs: name, + }) } } @@ -138,25 +164,31 @@ func (md Metadata) syncMissingMbzIDs(participants model.Participants) { } } -func (md Metadata) parseArtists( - name model.TagName, names model.TagName, sort model.TagName, - sorts model.TagName, mbid model.TagName, -) []model.Artist { - nameValues := md.getArtistValues(name, names) - sortValues := md.getArtistValues(sort, sorts) - mbids := md.Strings(mbid) - if len(nameValues) == 0 { - nameValues = []string{consts.UnknownArtist} +// buildParticipants wraps each Artist into a Participant with CreditedAs +// populated. credits pairs positionally with names; a length mismatch falls +// back to the canonical name for every entry. +func (md Metadata) buildParticipants(names, sorts, mbids, credits []string) []model.Participant { + if len(credits) != 0 && len(credits) != len(names) { + credits = nil } - return md.buildArtists(nameValues, sortValues, mbids) + artists := md.buildArtists(names, sorts, mbids) + out := make([]model.Participant, len(artists)) + for i, a := range artists { + p := model.Participant{Artist: a} + if i < len(credits) && credits[i] != "" { + p.CreditedAs = credits[i] + } else { + p.CreditedAs = a.Name + } + out[i] = p + } + return out } func (md Metadata) buildArtists(names, sorts, mbids []string) []model.Artist { var artists []model.Artist for i, name := range names { - id := md.artistID(name) artist := model.Artist{ - ID: id, Name: name, OrderArtistName: str.SanitizeFieldForSortingNoArticle(name), } @@ -166,6 +198,12 @@ func (md Metadata) buildArtists(names, sorts, mbids []string) []model.Artist { if i < len(mbids) { artist.MbzArtistID = mbids[i] } + // Assign ID after Sort/MBID are set so MBID/sort-based PID specs resolve. + artist.ID = computeArtistPID( + model.Participant{Artist: artist}, + conf.Server.PID.Artist, + id.NewHash, + ) artists = append(artists, artist) } return artists diff --git a/model/metadata/map_participants_test.go b/model/metadata/map_participants_test.go index 5ee802ced..95984ff59 100644 --- a/model/metadata/map_participants_test.go +++ b/model/metadata/map_participants_test.go @@ -460,6 +460,26 @@ var _ = Describe("Participants", func() { }) }) + When("the COMPILATION tag is true and ALBUMARTIST is the UnknownArtist placeholder", func() { + BeforeEach(func() { + // Some rippers emit the literal '[Unknown Artist]' string + // when no album artist is set. The fallback must still + // route to Various Artists for compilations. + mf = toMediaFile(model.RawTags{ + "COMPILATION": {"1"}, + "ALBUMARTIST": {consts.UnknownArtist}, + }) + }) + + It("should substitute Various Artists as the album artist", func() { + participants := mf.Participants + Expect(participants).To(HaveKeyWithValue(model.RoleAlbumArtist, HaveLen(1))) + albumArtist := participants[model.RoleAlbumArtist][0] + Expect(albumArtist.Name).To(Equal("Various Artists")) + Expect(albumArtist.MbzArtistID).To(Equal(consts.VariousArtistsMbzId)) + }) + }) + When("the COMPILATION tag is true and there are ALBUMARTIST tags", func() { BeforeEach(func() { mf = toMediaFile(model.RawTags{ @@ -802,4 +822,105 @@ var _ = Describe("Participants", func() { } }) }) + + Describe("CreditedAs population", func() { + It("uses the canonical name as CreditedAs when no credit tag is present", func() { + mf = toMediaFile(model.RawTags{ + "ARTISTS": {"Some Artist"}, + }) + artists := mf.Participants[model.RoleArtist] + Expect(artists).To(HaveLen(1)) + Expect(artists[0].Name).To(Equal("Some Artist")) + Expect(artists[0].CreditedAs).To(Equal("Some Artist")) + }) + + It("uses the credit tag value when present, paired positionally", func() { + mf = toMediaFile(model.RawTags{ + "ARTISTS": {"Planetary Assault Systems", "Other"}, + "ARTISTSCREDIT": {"PAS", "Other"}, + }) + artists := mf.Participants[model.RoleArtist] + Expect(artists).To(HaveLen(2)) + Expect(artists[0].Name).To(Equal("Planetary Assault Systems")) + Expect(artists[0].CreditedAs).To(Equal("PAS")) + Expect(artists[1].Name).To(Equal("Other")) + Expect(artists[1].CreditedAs).To(Equal("Other")) + }) + + It("falls back to canonical name when credit list length differs from name list", func() { + mf = toMediaFile(model.RawTags{ + "ARTISTS": {"A", "B", "C"}, + "ARTISTSCREDIT": {"only one"}, // mismatch + }) + artists := mf.Participants[model.RoleArtist] + Expect(artists).To(HaveLen(3)) + for _, a := range artists { + Expect(a.CreditedAs).To(Equal(a.Name)) + } + }) + + It("populates CreditedAs for non-artist roles (composer)", func() { + mf = toMediaFile(model.RawTags{ + "COMPOSER": {"Real Composer"}, + "COMPOSERCREDIT": {"R. Composer"}, + }) + composers := mf.Participants[model.RoleComposer] + Expect(composers).To(HaveLen(1)) + Expect(composers[0].CreditedAs).To(Equal("R. Composer")) + }) + + It("always populates CreditedAs (never empty)", func() { + mf = toMediaFile(model.RawTags{ + "ARTIST": {"Track Artist"}, + "ALBUMARTIST": {"Album Artist"}, + "COMPOSER": {"A Composer"}, + "PERFORMER:GUITAR": {"A Guitarist"}, + }) + for role, list := range mf.Participants { + for _, p := range list { + Expect(p.CreditedAs).NotTo(BeEmpty(), "role: %s, participant: %+v", role, p) + } + } + }) + + It("aligns composer credits with names when both use a role separator", func() { + // Names and credits both split on ';' under the roles.split config. + // Prior to the alignment fix, credits were read raw and would have + // stayed as one value while names became three, producing a length + // mismatch that silently dropped all credits. + mf = toMediaFile(model.RawTags{ + "COMPOSER": {"Comp A;Comp B;Comp C"}, + "COMPOSERCREDIT": {"CA;CB;CC"}, + }) + composers := mf.Participants[model.RoleComposer] + Expect(composers).To(HaveLen(3)) + Expect(composers[0].Name).To(Equal("Comp A")) + Expect(composers[0].CreditedAs).To(Equal("CA")) + Expect(composers[1].CreditedAs).To(Equal("CB")) + Expect(composers[2].CreditedAs).To(Equal("CC")) + }) + + It("splits composer MBIDs to align with split composer names", func() { + // Both names and MBIDs use the role separator ';'. The fix routes + // MBIDs through getRoleValues so they split alongside names rather + // than staying as a single value (which would have produced 2 names + // and 1 MBID — broken positional alignment). + // + // We test via a single delimited canonical name string AND a single + // delimited MBID-pair string. Note: the tag-reader UUID validation + // happens at metadata parse time on individual values, so we hand + // multi-valued tag entries already split into separate slice + // entries (matching how a multi-value tag would surface). + mf = toMediaFile(model.RawTags{ + "COMPOSER": {"Comp A;Comp B"}, + "MUSICBRAINZ_COMPOSERID": {"11111111-1111-1111-1111-111111111111", "22222222-2222-2222-2222-222222222222"}, + }) + composers := mf.Participants[model.RoleComposer] + Expect(composers).To(HaveLen(2)) + Expect(composers[0].Name).To(Equal("Comp A")) + Expect(composers[0].MbzArtistID).To(Equal("11111111-1111-1111-1111-111111111111")) + Expect(composers[1].Name).To(Equal("Comp B")) + Expect(composers[1].MbzArtistID).To(Equal("22222222-2222-2222-2222-222222222222")) + }) + }) }) diff --git a/model/metadata/persistent_ids.go b/model/metadata/persistent_ids.go index db315dc6b..168e1081c 100644 --- a/model/metadata/persistent_ids.go +++ b/model/metadata/persistent_ids.go @@ -87,10 +87,62 @@ func (md Metadata) albumID(mf model.MediaFile, pidConf string) string { return computePID(mf, md, pidConf, true, id.NewHash) } -// BFR Must be configurable? -func (md Metadata) artistID(name string) string { - mf := model.MediaFile{AlbumArtist: name} - return computePID(mf, md, "albumartistid", false, id.NewHash) +// computeArtistPID computes a persistent ID for a single participant using the +// given spec. The spec grammar is the same pipe/comma form used by Album/Track +// PIDs. Attributes recognised: +// - name → hash(clear(lower(participant.Name))) — the inner +// hash matches the historical 'albumartistid' path +// so the default spec "name" produces byte-identical +// IDs to the previous hardcoded artistID(). +// - musicbrainz_artistid → participant.MbzArtistID (opaque identifier — no +// normalization). +// - sort_name → participant.SortArtistName (raw — sort forms are +// already canonicalized by taggers, and folding case +// could collapse legitimately distinct sort variants). +// +// Unlike album/track PIDs, no library prefix is applied: artists are shared +// across libraries by design. +func computeArtistPID(p model.Participant, spec string, hash hashFunc) string { + pid := "" + fields := strings.SplitSeq(spec, "|") + for field := range fields { + attributes := strings.Split(field, ",") + values := make([]string, len(attributes)) + hasValue := false + for i, attr := range attributes { + v := getArtistPIDAttr(p, attr, hash) + if v != "" { + hasValue = true + } + values[i] = v + } + if hasValue { + pid += strings.Join(values, "\\") + break + } + } + return hash(pid) +} + +// ComputeArtistPID is the exported entry point for callers outside this package +// (e.g. the scanner) that need to compute an artist PID under a specific spec. +func ComputeArtistPID(p model.Participant, spec string) string { + return computeArtistPID(p, spec, id.NewHash) +} + +func getArtistPIDAttr(p model.Participant, attr string, hash hashFunc) string { + switch strings.TrimSpace(strings.ToLower(attr)) { + case "name": + if p.Name == "" { + return "" + } + return hash(str.Clear(strings.ToLower(p.Name))) + case "musicbrainz_artistid": + return p.MbzArtistID + case "sort_name": + return p.SortArtistName + } + return "" } func (md Metadata) mapTrackTitle() string { diff --git a/model/metadata/persistent_ids_test.go b/model/metadata/persistent_ids_test.go index eb66d11d1..47cb66115 100644 --- a/model/metadata/persistent_ids_test.go +++ b/model/metadata/persistent_ids_test.go @@ -5,7 +5,9 @@ import ( "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/id" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -291,3 +293,107 @@ var _ = Describe("getPID", func() { }) }) }) + +var _ = Describe("computeArtistPID", func() { + var p model.Participant + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + p = model.Participant{Artist: model.Artist{Name: "The Beatles"}} + }) + + Context("default spec 'name'", func() { + BeforeEach(func() { conf.Server.PID.Artist = consts.DefaultArtistPID }) + + It("produces the same ID as the legacy artistID(name) hardcoded function", func() { + // Legacy formula: hash(hash(clear(lower(name)))) + // It came from artistID(name) routing through computePID(spec="albumartistid"), + // which inside getPIDAttr returns hash(clear(lower(mf.AlbumArtist))), then + // computePID hashes that again. + mfWithAlbumArtist := model.MediaFile{AlbumArtist: p.Name} + legacyMD := Metadata{} + legacyID := computePID(mfWithAlbumArtist, legacyMD, "albumartistid", false, id.NewHash) + + newID := computeArtistPID(p, conf.Server.PID.Artist, id.NewHash) + Expect(newID).To(Equal(legacyID)) + }) + + It("normalizes name (clear(lower(...))) before hashing", func() { + // Different casing / punctuation must collapse to the same ID. + p2 := model.Participant{Artist: model.Artist{Name: "the beatles"}} + Expect(computeArtistPID(p, conf.Server.PID.Artist, id.NewHash)). + To(Equal(computeArtistPID(p2, conf.Server.PID.Artist, id.NewHash))) + }) + }) + + Context("spec 'musicbrainz_artistid|name'", func() { + BeforeEach(func() { conf.Server.PID.Artist = "musicbrainz_artistid|name" }) + + It("uses MBID when present", func() { + p.MbzArtistID = "mbid-1" + withMBID := computeArtistPID(p, conf.Server.PID.Artist, id.NewHash) + + p2 := model.Participant{Artist: model.Artist{Name: "Different", MbzArtistID: "mbid-1"}} + withMBIDOtherName := computeArtistPID(p2, conf.Server.PID.Artist, id.NewHash) + + Expect(withMBID).To(Equal(withMBIDOtherName)) + }) + + It("falls back to normalized name when MBID is missing", func() { + withoutMBID := computeArtistPID(p, conf.Server.PID.Artist, id.NewHash) + byName := computeArtistPID(p, "name", id.NewHash) + Expect(withoutMBID).To(Equal(byName)) + }) + }) + + Context("composite spec 'musicbrainz_artistid|sort_name,name'", func() { + BeforeEach(func() { conf.Server.PID.Artist = "musicbrainz_artistid|sort_name,name" }) + + It("hashes sort+name composite when MBID is absent", func() { + p.SortArtistName = "Beatles, The" + withSort := computeArtistPID(p, conf.Server.PID.Artist, id.NewHash) + + p2 := p + p2.SortArtistName = "Different Sort" + withDifferentSort := computeArtistPID(p2, conf.Server.PID.Artist, id.NewHash) + + Expect(withSort).NotTo(Equal(withDifferentSort)) + }) + }) + + Context("spec 'sort_name|name' with empty sort_name", func() { + BeforeEach(func() { conf.Server.PID.Artist = "sort_name|name" }) + + It("falls through to name when SortArtistName is empty", func() { + // sort_name is empty → first pipe alternative produces no value → + // engine falls through to name. + fallback := computeArtistPID(p, conf.Server.PID.Artist, id.NewHash) + byName := computeArtistPID(p, "name", id.NewHash) + Expect(fallback).To(Equal(byName)) + }) + }) + + Context("empty inputs", func() { + BeforeEach(func() { conf.Server.PID.Artist = "musicbrainz_artistid|name" }) + + It("does not panic on empty participant", func() { + empty := model.Participant{Artist: model.Artist{Name: ""}} + Expect(func() { computeArtistPID(empty, conf.Server.PID.Artist, id.NewHash) }). + NotTo(Panic()) + }) + }) + + It("buildArtists produces the same Artist.ID as the legacy artistID() under default PID.Artist", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.PID.Artist = consts.DefaultArtistPID + + name := "Some Artist" + md := Metadata{} + artists := md.buildArtists([]string{name}, nil, nil) + + // Legacy result computed independently via the old "albumartistid" path: + legacyMF := model.MediaFile{AlbumArtist: name} + expected := computePID(legacyMF, Metadata{}, "albumartistid", false, id.NewHash) + Expect(artists).To(HaveLen(1)) + Expect(artists[0].ID).To(Equal(expected)) + }) +}) diff --git a/model/participants.go b/model/participants.go index afbda10de..8554afbb7 100644 --- a/model/participants.go +++ b/model/participants.go @@ -77,7 +77,12 @@ func RoleFromString(role string) Role { type Participant struct { Artist - SubRole string `json:"subRole,omitempty"` + SubRole string `json:"subRole,omitempty"` + CreditedAs string `json:"creditedAs,omitempty"` +} + +func (p Participant) DisplayName() string { + return cmp.Or(p.CreditedAs, p.Name) } type ParticipantList []Participant @@ -91,6 +96,18 @@ func (p ParticipantList) Join(sep string) string { }), sep) } +// JoinCredited joins the credited names of the participants with sep. +// Falls back to Name when CreditedAs is empty. +func (p ParticipantList) JoinCredited(sep string) string { + return strings.Join(slice.Map(p, func(part Participant) string { + n := part.DisplayName() + if part.SubRole != "" { + return n + " (" + part.SubRole + ")" + } + return n + }), sep) +} + type Participants map[Role]ParticipantList // Add adds the artists to the role, ignoring duplicates. @@ -109,6 +126,12 @@ func (p Participants) AddWithSubRole(role Role, subRole string, artists ...Artis p.add(role, participants...) } +// AddParticipants adds Participants directly (preserving CreditedAs and SubRole), +// ignoring duplicates. +func (p Participants) AddParticipants(role Role, participants ...Participant) { + p.add(role, participants...) +} + func (p Participants) Sort() { for _, artists := range p { slices.SortFunc(artists, func(a1, a2 Participant) int { @@ -133,16 +156,28 @@ func (p Participants) Merge(other Participants) { } func (p Participants) add(role Role, participants ...Participant) { - seen := make(map[string]struct{}, len(p[role])) - for _, artist := range p[role] { - seen[artist.ID+artist.SubRole] = struct{}{} + // Use a separator that can't appear in either field so e.g. + // (ID="12", SubRole="3") doesn't collide with (ID="1", SubRole="23"). + const sep = "\x00" + seen := make(map[string]int, len(p[role])) + for i, artist := range p[role] { + seen[artist.ID+sep+artist.SubRole] = i } for _, participant := range participants { - key := participant.ID + participant.SubRole - if _, ok := seen[key]; !ok { - seen[key] = struct{}{} - p[role] = append(p[role], participant) + key := participant.ID + sep + participant.SubRole + if idx, ok := seen[key]; ok { + // Same artist/sub-role seen before. The merge (e.g. building + // album.Participants from per-track participants) is inherently + // lossy when tracks differ on CreditedAs, but silently dropping + // the later value can mean the wrong credit ends up on the album. + // Prefer the most recently observed non-empty CreditedAs. + if participant.CreditedAs != "" { + p[role][idx].CreditedAs = participant.CreditedAs + } + continue } + seen[key] = len(p[role]) + p[role] = append(p[role], participant) } } diff --git a/model/participants_test.go b/model/participants_test.go index dad84b6dd..6ae99f603 100644 --- a/model/participants_test.go +++ b/model/participants_test.go @@ -116,6 +116,40 @@ var _ = Describe("Participants", func() { RoleAlbumArtist: []Participant{_p("3", "AlbumArtist1"), _p("4", "AlbumArtist2"), _p("7", "AlbumArtist3"), _p("8", "AlbumArtist4")}, })) }) + + It("upgrades CreditedAs from a later occurrence when the dedup key matches", func() { + // Same artist (same ID+SubRole) credited differently on two tracks. + // The merge is inherently lossy; the later non-empty credit wins so + // it isn't silently dropped just because of arrival order. + p1 := Participants{ + RoleArtist: []Participant{ + {Artist: Artist{ID: "a1", Name: "Canonical"}, CreditedAs: "Credit One"}, + }, + } + p2 := Participants{ + RoleArtist: []Participant{ + {Artist: Artist{ID: "a1", Name: "Canonical"}, CreditedAs: "Credit Two"}, + }, + } + p1.Merge(p2) + Expect(p1[RoleArtist]).To(HaveLen(1)) + Expect(p1[RoleArtist][0].CreditedAs).To(Equal("Credit Two")) + }) + + It("does not overwrite an existing CreditedAs with an empty one", func() { + p1 := Participants{ + RoleArtist: []Participant{ + {Artist: Artist{ID: "a1", Name: "Canonical"}, CreditedAs: "Credit One"}, + }, + } + p2 := Participants{ + RoleArtist: []Participant{ + {Artist: Artist{ID: "a1", Name: "Canonical"}}, // empty CreditedAs + }, + } + p1.Merge(p2) + Expect(p1[RoleArtist][0].CreditedAs).To(Equal("Credit One")) + }) }) Describe("Hash", func() { @@ -182,6 +216,36 @@ var _ = Describe("Participants", func() { }) }) +var _ = Describe("Participant.CreditedAs", func() { + It("round-trips through JSON when set", func() { + p := Participant{ + Artist: Artist{ID: "abc", Name: "Planetary Assault Systems"}, + CreditedAs: "PAS", + } + data, err := json.Marshal(p) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(ContainSubstring(`"creditedAs":"PAS"`)) + + var back Participant + Expect(json.Unmarshal(data, &back)).To(Succeed()) + Expect(back.CreditedAs).To(Equal("PAS")) + }) + + It("omits CreditedAs from JSON when empty", func() { + p := Participant{Artist: Artist{ID: "abc", Name: "Foo"}} + data, err := json.Marshal(p) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).NotTo(ContainSubstring("creditedAs")) + }) + + It("deserializes legacy JSON without the creditedAs key", func() { + legacy := `{"id":"abc","name":"Foo","subRole":""}` + var p Participant + Expect(json.Unmarshal([]byte(legacy), &p)).To(Succeed()) + Expect(p.CreditedAs).To(Equal("")) + }) +}) + var _ = Describe("ParticipantList", func() { Describe("Join", func() { It("joins the participants with the given separator", func() { diff --git a/model/tag.go b/model/tag.go index 02ccac05d..86fe0bb4b 100644 --- a/model/tag.go +++ b/model/tag.go @@ -205,27 +205,44 @@ const ( // Artists and roles - TagAlbumArtist TagName = "albumartist" - TagAlbumArtists TagName = "albumartists" - TagAlbumArtistSort TagName = "albumartistsort" - TagAlbumArtistsSort TagName = "albumartistssort" - TagTrackArtist TagName = "artist" - TagTrackArtists TagName = "artists" - TagTrackArtistSort TagName = "artistsort" - TagTrackArtistsSort TagName = "artistssort" - TagComposer TagName = "composer" - TagComposerSort TagName = "composersort" - TagLyricist TagName = "lyricist" - TagLyricistSort TagName = "lyricistsort" - TagDirector TagName = "director" - TagProducer TagName = "producer" - TagEngineer TagName = "engineer" - TagMixer TagName = "mixer" - TagRemixer TagName = "remixer" - TagDJMixer TagName = "djmixer" - TagConductor TagName = "conductor" - TagArranger TagName = "arranger" - TagPerformer TagName = "performer" + TagAlbumArtist TagName = "albumartist" + TagAlbumArtists TagName = "albumartists" + TagAlbumArtistSort TagName = "albumartistsort" + TagAlbumArtistsSort TagName = "albumartistssort" + TagAlbumArtistCredit TagName = "albumartistcredit" + TagAlbumArtistsCredit TagName = "albumartistscredit" + TagTrackArtist TagName = "artist" + TagTrackArtists TagName = "artists" + TagTrackArtistSort TagName = "artistsort" + TagTrackArtistsSort TagName = "artistssort" + TagTrackArtistCredit TagName = "artistcredit" + TagTrackArtistsCredit TagName = "artistscredit" + TagComposer TagName = "composer" + TagComposerSort TagName = "composersort" + TagComposerCredit TagName = "composercredit" + TagLyricist TagName = "lyricist" + TagLyricistSort TagName = "lyricistsort" + TagLyricistCredit TagName = "lyricistcredit" + TagDirector TagName = "director" + TagDirectorCredit TagName = "directorcredit" + TagProducer TagName = "producer" + TagProducerCredit TagName = "producercredit" + TagEngineer TagName = "engineer" + TagEngineerCredit TagName = "engineercredit" + TagMixer TagName = "mixer" + TagMixerCredit TagName = "mixercredit" + TagRemixer TagName = "remixer" + TagRemixerCredit TagName = "remixercredit" + TagDJMixer TagName = "djmixer" + TagDJMixerCredit TagName = "djmixercredit" + TagConductor TagName = "conductor" + TagConductorCredit TagName = "conductorcredit" + TagArranger TagName = "arranger" + TagArrangerCredit TagName = "arrangercredit" + TagPerformer TagName = "performer" + // Performer credits are not currently surfaced: performer tags are + // `type: pair` ('PERFORMER:instrument') and the matching credit + // representation hasn't been designed yet. Tracked separately. // ReplayGain diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 56843b911..a56abc396 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -224,6 +224,34 @@ func (r *artistRepository) Put(a *model.Artist, colsToUpdate ...string) error { return err } +// CopyAttributes copies the named columns from the row identified by fromID +// to the row identified by toID. Used by the scanner after a PID change to +// preserve attributes (notably created_at) across artist-ID migrations. +func (r *artistRepository) CopyAttributes(fromID, toID string, columns ...string) error { + var from dbx.NullStringMap + err := r.queryOne(Select(columns...).From(r.tableName).Where(Eq{"id": fromID}), &from) + if err != nil { + return fmt.Errorf("getting artist to copy fields from: %w", err) + } + to := make(map[string]any) + for _, col := range columns { + v := from[col] + // created_at on the previous row may be zero/poisoned (e.g. legacy + // rows or rows where it was never populated). Skip in that case so we + // don't propagate a bad value forward on every metadata-driven ID + // change. Matches the behavior in album_repository.CopyAttributes. + if col == "created_at" && (!v.Valid || v.String == "" || strings.HasPrefix(v.String, "0001-")) { + continue + } + to[col] = v + } + if len(to) == 0 { + return nil + } + _, err = r.executeSQL(Update(r.tableName).SetMap(to).Where(Eq{"id": toID})) + return err +} + func (r *artistRepository) UpdateExternalInfo(a *model.Artist) error { dba := &dbArtist{Artist: a} _, err := r.put(a.ID, dba, diff --git a/persistence/sql_participations.go b/persistence/sql_participations.go index 38b0203fa..96a9704a6 100644 --- a/persistence/sql_participations.go +++ b/persistence/sql_participations.go @@ -10,9 +10,10 @@ import ( ) type participant struct { - ID string `json:"id"` - Name string `json:"name"` - SubRole string `json:"subRole,omitempty"` + ID string `json:"id"` + Name string `json:"name"` + SubRole string `json:"subRole,omitempty"` + CreditedAs string `json:"creditedAs,omitempty"` } // flatParticipant represents a flattened participant structure for SQL processing @@ -26,7 +27,12 @@ func marshalParticipants(participants model.Participants) string { dbParticipants := make(map[model.Role][]participant) for role, artists := range participants { for _, artist := range artists { - dbParticipants[role] = append(dbParticipants[role], participant{ID: artist.ID, SubRole: artist.SubRole, Name: artist.Name}) + dbParticipants[role] = append(dbParticipants[role], participant{ + ID: artist.ID, + Name: artist.Name, + SubRole: artist.SubRole, + CreditedAs: artist.CreditedAs, + }) } } res, _ := json.Marshal(dbParticipants) @@ -43,7 +49,11 @@ func unmarshalParticipants(data string) (model.Participants, error) { participants := make(model.Participants, len(dbParticipants)) for role, participantList := range dbParticipants { artists := slice.Map(participantList, func(p participant) model.Participant { - return model.Participant{Artist: model.Artist{ID: p.ID, Name: p.Name}, SubRole: p.SubRole} + return model.Participant{ + Artist: model.Artist{ID: p.ID, Name: p.Name}, + SubRole: p.SubRole, + CreditedAs: p.CreditedAs, + } }) participants[role] = artists } diff --git a/persistence/sql_participations_test.go b/persistence/sql_participations_test.go new file mode 100644 index 000000000..685ab4ce6 --- /dev/null +++ b/persistence/sql_participations_test.go @@ -0,0 +1,51 @@ +package persistence + +import ( + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("sql_participations", func() { + Describe("marshalParticipants/unmarshalParticipants", func() { + It("preserves CreditedAs through marshal/unmarshal round-trip", func() { + original := model.Participants{ + model.RoleArtist: model.ParticipantList{ + {Artist: model.Artist{ID: "canon-1", Name: "Planetary Assault Systems"}, CreditedAs: "PAS"}, + }, + model.RoleComposer: model.ParticipantList{ + {Artist: model.Artist{ID: "canon-2", Name: "John Lennon"}, CreditedAs: "J. Lennon"}, + }, + } + serialized := marshalParticipants(original) + restored, err := unmarshalParticipants(serialized) + Expect(err).ToNot(HaveOccurred()) + Expect(restored[model.RoleArtist]).To(HaveLen(1)) + Expect(restored[model.RoleArtist][0].CreditedAs).To(Equal("PAS")) + Expect(restored[model.RoleComposer][0].CreditedAs).To(Equal("J. Lennon")) + }) + + It("preserves SubRole through marshal/unmarshal round-trip", func() { + original := model.Participants{ + model.RolePerformer: model.ParticipantList{ + {Artist: model.Artist{ID: "p-1", Name: "Performer"}, SubRole: "Guitar"}, + }, + } + serialized := marshalParticipants(original) + restored, err := unmarshalParticipants(serialized) + Expect(err).ToNot(HaveOccurred()) + Expect(restored[model.RolePerformer]).To(HaveLen(1)) + Expect(restored[model.RolePerformer][0].SubRole).To(Equal("Guitar")) + }) + + It("does not include CreditedAs in serialized JSON when empty (omitempty)", func() { + p := model.Participants{ + model.RoleArtist: model.ParticipantList{ + {Artist: model.Artist{ID: "canon-1", Name: "Foo"}}, + }, + } + serialized := marshalParticipants(p) + Expect(serialized).NotTo(ContainSubstring("creditedAs")) + }) + }) +}) diff --git a/resources/mappings.yaml b/resources/mappings.yaml index 294654b6a..95d8f8343 100644 --- a/resources/mappings.yaml +++ b/resources/mappings.yaml @@ -38,8 +38,14 @@ main: aliases: [ txxx:artists, artists, ----:com.apple.itunes:artists, wm/artists ] artistssort: aliases: [ artistssort ] + artistcredit: + aliases: [ artist_credit, artistcredit ] + artistscredit: + aliases: [ artists_credit, artistscredit ] arranger: aliases: [ tipl:arranger, ipls:arranger, arranger ] + arrangercredit: + aliases: [ arranger_credit, arrangercredit ] composer: aliases: [ tcom, composer, ©wrt, wm/composer, imus, writer, txxx:writer, iwri, @@ -50,24 +56,42 @@ main: # aliases: [ WRITER, TXXX:Writer, IWRI ] composersort: aliases: [ tsoc, txxx:composersort, composersort, soco, wm/composersortorder ] + composercredit: + aliases: [ composer_credit, composercredit ] lyricist: aliases: [ text, lyricist, ----:com.apple.itunes:lyricist, wm/writer ] lyricistsort: aliases: [ lyricistsort ] + lyricistcredit: + aliases: [ lyricist_credit, lyricistcredit ] conductor: aliases: [ tpe3, conductor, ----:com.apple.itunes:conductor, wm/conductor ] + conductorcredit: + aliases: [ conductor_credit, conductorcredit ] director: aliases: [ txxx:director, director, ©dir, wm/director ] + directorcredit: + aliases: [ director_credit, directorcredit ] djmixer: aliases: [ tipl:dj-mix, ipls:dj-mix, djmixer, ----:com.apple.itunes:djmixer, wm/djmixer ] + djmixercredit: + aliases: [ djmixer_credit, djmixercredit ] mixer: aliases: [ tipl:mix, ipls:mix, mixer, ----:com.apple.itunes:mixer, wm/mixer ] + mixercredit: + aliases: [ mixer_credit, mixercredit ] engineer: aliases: [ tipl:engineer, ipls:engineer, engineer, ----:com.apple.itunes:engineer, wm/engineer, ieng ] + engineercredit: + aliases: [ engineer_credit, engineercredit ] producer: aliases: [ tipl:producer, ipls:producer, producer, ----:com.apple.itunes:producer, wm/producer, ipro ] + producercredit: + aliases: [ producer_credit, producercredit ] remixer: aliases: [ tpe4, remixer, mixartist, ----:com.apple.itunes:remixer, wm/modifiedby ] + remixercredit: + aliases: [ remixer_credit, remixercredit ] albumartist: aliases: [ tpe2, albumartist, album artist, album_artist, aart, wm/albumartist ] albumartistsort: @@ -76,6 +100,10 @@ main: aliases: [ txxx:album artists, albumartists ] albumartistssort: aliases: [ albumartistssort ] + albumartistcredit: + aliases: [ albumartist_credit, albumartistcredit ] + albumartistscredit: + aliases: [ albumartists_credit, albumartistscredit ] album: aliases: [ talb, album, ©alb, wm/albumtitle, iprd ] albumsort: diff --git a/scanner/folder_entry.go b/scanner/folder_entry.go index c7cc88ee1..6a26a44bb 100644 --- a/scanner/folder_entry.go +++ b/scanner/folder_entry.go @@ -17,14 +17,15 @@ import ( func newFolderEntry(job *scanJob, id, path string, updTime time.Time, hash string) *folderEntry { f := &folderEntry{ - id: id, - job: job, - path: path, - audioFiles: make(map[string]fs.DirEntry), - imageFiles: make(map[string]fs.DirEntry), - albumIDMap: make(map[string]string), - updTime: updTime, - prevHash: hash, + id: id, + job: job, + path: path, + audioFiles: make(map[string]fs.DirEntry), + imageFiles: make(map[string]fs.DirEntry), + albumIDMap: make(map[string]string), + artistIDMap: make(map[string]string), + updTime: updTime, + prevHash: hash, } return f } @@ -46,6 +47,7 @@ type folderEntry struct { albums model.Albums albumIDMap map[string]string artists model.Artists + artistIDMap map[string]string tags model.TagList missingTracks []*model.MediaFile } diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 38967832c..77409344c 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -123,11 +123,12 @@ func (j *scanJob) createFolderEntry(path string) *folderEntry { // The phaseFolders struct implements the phase interface, providing methods to produce // folder entries, process folders, persist changes to the database, and log the results. type phaseFolders struct { - jobs []*scanJob - ds model.DataStore - ctx context.Context - state *scanState - prevAlbumPIDConf string + jobs []*scanJob + ds model.DataStore + ctx context.Context + state *scanState + prevAlbumPIDConf string + prevArtistPIDConf string } func (p *phaseFolders) description() string { @@ -141,6 +142,10 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { if err != nil { return fmt.Errorf("getting album PID conf: %w", err) } + p.prevArtistPIDConf, err = p.ds.Property(p.ctx).DefaultGet(consts.PIDArtistKey, "") + if err != nil { + return fmt.Errorf("getting artist PID conf: %w", err) + } // TODO Parallelize multiple job when we have multiple libraries var total int64 @@ -298,6 +303,30 @@ func (p *phaseFolders) loadTagsFromFiles(entry *folderEntry, toImport map[string if prevAlbumID != track.AlbumID && !ok { entry.albumIDMap[track.AlbumID] = prevAlbumID } + + // Build artistIDMap: for each participant on this track, if the previous + // spec produces a different ID than the current one, record the mapping. + // + // The map is keyed by the new artist ID so it stores ONE previous ID per + // new ID, even when many old IDs collapse to the same new one (e.g. + // switching from "name" to "musicbrainz_artistid|name" where multiple + // name-keyed IDs share an MBID). This is intentional: ReassignAnnotation + // is idempotent and the other prior IDs orphan their annotations into + // the unique-constraint collision case documented in the spec — a + // known limitation tracked for a separate follow-up that fixes + // ReassignAnnotation merge semantics at the SQL level. + if p.prevArtistPIDConf != "" && p.prevArtistPIDConf != conf.Server.PID.Artist { + for _, list := range track.Participants { + for _, part := range list { + prevID := metadata.ComputeArtistPID(part, p.prevArtistPIDConf) + if prevID != part.ID { + if _, already := entry.artistIDMap[part.ID]; !already { + entry.artistIDMap[part.ID] = prevID + } + } + } + } + } } } entry.tracks = tracks @@ -359,9 +388,7 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) // Save all new/modified artists to DB. Their information will be incomplete, but they will be refreshed later for i := range entry.artists { - err = artistRepo.Put(&entry.artists[i], "name", - "mbz_artist_id", "sort_artist_name", "order_artist_name", "full_text", "updated_at") - if err != nil { + if err = p.persistArtist(artistRepo, &entry.artists[i], entry.artistIDMap); err != nil { log.Error(p.ctx, "Scanner: Error persisting artist to DB", "folder", entry.path, "artist", entry.artists[i].Name, err) return err } @@ -462,6 +489,36 @@ func (p *phaseFolders) persistAlbum(repo model.AlbumRepository, a *model.Album, return nil } +// persistArtist persists the given artist and reassigns annotations from the +// previous artist ID (if recorded in idMap). Mirrors persistAlbum. +func (p *phaseFolders) persistArtist(repo model.ArtistRepository, a *model.Artist, idMap map[string]string) error { + if err := repo.Put(a, "name", "mbz_artist_id", "sort_artist_name", + "order_artist_name", "full_text", "updated_at"); err != nil { + return fmt.Errorf("persisting artist %s: %w", a.ID, err) + } + prevID := idMap[a.ID] + if prevID == "" { + return nil + } + log.Trace(p.ctx, "Reassigning artist annotations", "from", prevID, "to", a.ID, "artist", a.Name) + if err := repo.ReassignAnnotation(prevID, a.ID); err != nil { + log.Warn(p.ctx, "Scanner: Could not reassign artist annotations", "from", prevID, "to", a.ID, "artist", a.Name, err) + p.state.sendWarning(fmt.Sprintf("Could not reassign artist annotations from %s to %s ('%s'): %v", prevID, a.ID, a.Name, err)) + } + + // Keep created_at field from previous instance of the artist. Without this, + // the just-inserted row carries the scan timestamp, breaking "recently + // added" semantics on every PID config change. + if err := repo.CopyAttributes(prevID, a.ID, "created_at"); err != nil { + if !errors.Is(err, model.ErrNotFound) { + log.Warn(p.ctx, "Scanner: Could not copy artist fields", "from", prevID, "to", a.ID, "artist", a.Name, err) + p.state.sendWarning(fmt.Sprintf("Could not copy artist fields from %s to %s ('%s'): %v", prevID, a.ID, a.Name, err)) + } + } + delete(idMap, a.ID) + return nil +} + func (p *phaseFolders) logFolder(entry *folderEntry) (*folderEntry, error) { logCall := log.Info if entry.isEmpty() { diff --git a/scanner/phase_1_folders_test.go b/scanner/phase_1_folders_test.go new file mode 100644 index 000000000..f3861a430 --- /dev/null +++ b/scanner/phase_1_folders_test.go @@ -0,0 +1,67 @@ +package scanner + +import ( + "context" + + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("phaseFolders", func() { + Describe("Artist re-PID migration", func() { + var ( + artRepo *tests.MockArtistRepo + p *phaseFolders + ) + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + artRepo = tests.CreateMockArtistRepo() + if artRepo.ReassignAnnotationCalls == nil { + artRepo.ReassignAnnotationCalls = map[string]string{} + } + p = &phaseFolders{ctx: context.Background(), state: &scanState{}} + }) + + It("calls ReassignAnnotation when persisting an artist whose ID changed", func() { + oldID := "old-id" + newID := "new-id" + idMap := map[string]string{newID: oldID} + artist := model.Artist{ID: newID, Name: "Foo"} + + Expect(p.persistArtist(artRepo, &artist, idMap)).To(Succeed()) + Expect(artRepo.ReassignAnnotationCalls).To(HaveKeyWithValue(oldID, newID)) + // Mapping should be removed after successful reassignment + Expect(idMap).ToNot(HaveKey(newID)) + }) + + It("does not call ReassignAnnotation when no mapping exists", func() { + idMap := map[string]string{} + artist := model.Artist{ID: "some-id", Name: "Foo"} + + Expect(p.persistArtist(artRepo, &artist, idMap)).To(Succeed()) + Expect(artRepo.ReassignAnnotationCalls).To(BeEmpty()) + }) + + It("calls CopyAttributes(created_at) when persisting an artist whose ID changed", func() { + oldID := "old-id" + newID := "new-id" + idMap := map[string]string{newID: oldID} + artist := model.Artist{ID: newID, Name: "Foo"} + + Expect(p.persistArtist(artRepo, &artist, idMap)).To(Succeed()) + Expect(artRepo.CopyAttributesCalls).To(HaveKeyWithValue(oldID, newID)) + }) + + It("does not call CopyAttributes when no mapping exists", func() { + idMap := map[string]string{} + artist := model.Artist{ID: "some-id", Name: "Foo"} + + Expect(p.persistArtist(artRepo, &artist, idMap)).To(Succeed()) + Expect(artRepo.CopyAttributesCalls).To(BeEmpty()) + }) + }) +}) diff --git a/scanner/scanner.go b/scanner/scanner.go index 871b0c696..054e404be 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -309,6 +309,11 @@ func (s *scannerImpl) runUpdateLibraries(ctx context.Context, state *scanState) log.Error(ctx, "Scanner: Error updating album PID conf", err) return fmt.Errorf("updating album PID conf: %w", err) } + err = tx.Property(ctx).Put(consts.PIDArtistKey, conf.Server.PID.Artist) + if err != nil { + log.Error(ctx, "Scanner: Error updating artist PID conf", err) + return fmt.Errorf("updating artist PID conf: %w", err) + } if state.changesDetected.Load() { log.Debug(ctx, "Scanner: Refreshing library stats", "lib", lib.Name) if err := tx.Library(ctx).RefreshStats(lib.ID); err != nil { diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index a76165cc1..7777910d1 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -272,7 +272,7 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op child.DisplayAlbumArtist = mf.AlbumArtist child.AlbumArtists = artistRefs(mf.Participants[model.RoleAlbumArtist]) var contributors []responses.Contributor - child.DisplayComposer = mf.Participants[model.RoleComposer].Join(consts.ArtistJoiner) + child.DisplayComposer = mf.Participants[model.RoleComposer].JoinCredited(consts.ArtistJoiner) for role, participants := range mf.Participants { if role == model.RoleArtist || role == model.RoleAlbumArtist { continue @@ -283,7 +283,7 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op SubRole: participant.SubRole, Artist: responses.ArtistID3Ref{ Id: participant.ID, - Name: participant.Name, + Name: participant.DisplayName(), }, }) } @@ -303,7 +303,7 @@ func artistRefs(participants model.ParticipantList) []responses.ArtistID3Ref { return slice.Map(participants, func(p model.Participant) responses.ArtistID3Ref { return responses.ArtistID3Ref{ Id: p.ID, - Name: p.Name, + Name: p.DisplayName(), } }) } diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index ed8f257d1..af1fa4c7a 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -318,6 +318,45 @@ var _ = Describe("helpers", func() { Expect(child.Title).To(Equal("")) }) }) + + It("returns CreditedAs as the contributor name when present, with canonical artist ID", func() { + mf := model.MediaFile{ + ID: "song-1", + Participants: model.Participants{ + model.RoleArtist: model.ParticipantList{ + {Artist: model.Artist{ID: "canon-1", Name: "Planetary Assault Systems"}, CreditedAs: "PAS"}, + }, + model.RoleComposer: model.ParticipantList{ + {Artist: model.Artist{ID: "canon-2", Name: "Real Composer"}, CreditedAs: "R. Composer"}, + }, + }, + } + child := childFromMediaFile(context.Background(), mf) + Expect(child.OpenSubsonicChild).NotTo(BeNil()) + Expect(child.OpenSubsonicChild.Artists).To(HaveLen(1)) + Expect(child.OpenSubsonicChild.Artists[0].Id).To(Equal("canon-1")) + Expect(child.OpenSubsonicChild.Artists[0].Name).To(Equal("PAS")) + + Expect(child.OpenSubsonicChild.Contributors).To(HaveLen(1)) + Expect(child.OpenSubsonicChild.Contributors[0].Artist.Id).To(Equal("canon-2")) + Expect(child.OpenSubsonicChild.Contributors[0].Artist.Name).To(Equal("R. Composer")) + + Expect(child.OpenSubsonicChild.DisplayComposer).To(Equal("R. Composer")) + }) + + It("falls back to canonical Name when CreditedAs is empty (legacy participant rows)", func() { + mf := model.MediaFile{ + ID: "song-2", + Participants: model.Participants{ + model.RoleArtist: model.ParticipantList{ + {Artist: model.Artist{ID: "canon-3", Name: "Some Artist"}}, // no CreditedAs + }, + }, + } + child := childFromMediaFile(context.Background(), mf) + Expect(child.OpenSubsonicChild).NotTo(BeNil()) + Expect(child.OpenSubsonicChild.Artists[0].Name).To(Equal("Some Artist")) + }) }) Describe("osChildFromMediaFile", func() { diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index b7a6fb811..198564bd0 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -16,9 +16,11 @@ func CreateMockArtistRepo() *MockArtistRepo { type MockArtistRepo struct { model.ArtistRepository - Data map[string]*model.Artist - Err bool - Options model.QueryOptions + Data map[string]*model.Artist + Err bool + Options model.QueryOptions + ReassignAnnotationCalls map[string]string // prevID -> newID + CopyAttributesCalls map[string]string // fromID -> toID } func (m *MockArtistRepo) SetError(err bool) { @@ -157,4 +159,30 @@ func (m *MockArtistRepo) Search(q string, options ...model.QueryOptions) (model. return allArtists, err } +// ReassignAnnotation reassigns annotations from one artist to another +func (m *MockArtistRepo) ReassignAnnotation(prevID string, newID string) error { + if m.Err { + return errors.New("unexpected error") + } + // Mock implementation - track the reassignment calls + if m.ReassignAnnotationCalls == nil { + m.ReassignAnnotationCalls = make(map[string]string) + } + m.ReassignAnnotationCalls[prevID] = newID + return nil +} + +// CopyAttributes is a no-op in the mock; tests that need to verify call +// observation can extend this. +func (m *MockArtistRepo) CopyAttributes(fromID, toID string, columns ...string) error { + if m.Err { + return errors.New("unexpected error") + } + if m.CopyAttributesCalls == nil { + m.CopyAttributesCalls = make(map[string]string) + } + m.CopyAttributesCalls[fromID] = toID + return nil +} + var _ model.ArtistRepository = (*MockArtistRepo)(nil) diff --git a/ui/src/common/ArtistLinkField.jsx b/ui/src/common/ArtistLinkField.jsx index d41b47b06..055ad18f0 100644 --- a/ui/src/common/ArtistLinkField.jsx +++ b/ui/src/common/ArtistLinkField.jsx @@ -12,6 +12,10 @@ const ALink = withWidth()((props) => { const artistLink = useGetHandleArtistClick(width) const dispatch = useDispatch() + const displayName = artist.creditedAs || artist.name + const showCanonicalTooltip = + artist.creditedAs && artist.creditedAs !== artist.name + return ( { e.stopPropagation() dispatch(closeExtendedInfoDialog()) }} + title={showCanonicalTooltip ? artist.name : undefined} {...rest} > - {artist.name} + {displayName} {artist.subroles?.length > 0 ? ` (${artist.subroles.join(', ')})` : ''} ) @@ -37,7 +42,12 @@ const parseAndReplaceArtists = ( let lastIndex = 0 albumArtists?.forEach((artist) => { - const index = displayAlbumArtist.indexOf(artist.name, lastIndex) + // Match on the canonical name — that's what appears in displayAlbumArtist + // (sourced from mf.Artist / mf.AlbumArtist, which are set from the canonical + // ARTIST tag at scan time). The ALink itself renders creditedAs || name, + // so the displayed link text still reflects the credit. + const matchName = artist.name + const index = displayAlbumArtist.indexOf(matchName, lastIndex) if (index !== -1) { // Add text before the artist name if (index > lastIndex) { @@ -47,7 +57,7 @@ const parseAndReplaceArtists = ( result.push( , ) - lastIndex = index + artist.name.length + lastIndex = index + matchName.length } }) diff --git a/ui/src/common/ArtistLinkField.test.jsx b/ui/src/common/ArtistLinkField.test.jsx index 09fdf64a4..a9c850a2c 100644 --- a/ui/src/common/ArtistLinkField.test.jsx +++ b/ui/src/common/ArtistLinkField.test.jsx @@ -213,6 +213,113 @@ describe('ArtistLinkField', () => { }) }) + describe('creditedAs', () => { + it('renders creditedAs as the link text when present', () => { + const record = { + artist: 'PAS', + participants: { + artist: [ + { + id: 'canon-1', + name: 'Planetary Assault Systems', + creditedAs: 'PAS', + }, + ], + }, + } + + render() + + expect(screen.getByText('PAS')).toBeInTheDocument() + expect( + screen.queryByText('Planetary Assault Systems'), + ).not.toBeInTheDocument() + }) + + it('sets a title tooltip with the canonical name when creditedAs differs', () => { + const record = { + artist: 'PAS', + participants: { + artist: [ + { + id: 'canon-1', + name: 'Planetary Assault Systems', + creditedAs: 'PAS', + }, + ], + }, + } + + render() + + const link = screen.getByRole('link') + expect(link).toHaveAttribute('title', 'Planetary Assault Systems') + }) + + it('falls back to name when creditedAs is missing', () => { + const record = { + artist: 'Some Artist', + participants: { + artist: [{ id: 'canon-2', name: 'Some Artist' }], + }, + } + + render() + + expect(screen.getByText('Some Artist')).toBeInTheDocument() + const link = screen.getByRole('link') + expect(link).not.toHaveAttribute('title') + }) + + it('does not set a tooltip when creditedAs equals name', () => { + const record = { + artist: 'Same Name', + participants: { + artist: [ + { id: 'canon-3', name: 'Same Name', creditedAs: 'Same Name' }, + ], + }, + } + + render() + + const link = screen.getByRole('link') + expect(link).not.toHaveAttribute('title') + }) + + it('inline-linkifies when displayArtist holds the canonical name (Picard-default tagging)', () => { + // Picard "use standardized artist names" mode: ARTIST tag carries the + // canonical name, ARTIST_CREDIT carries the credit. The display string + // is the canonical name; parseAndReplaceArtists must match on `name` + // (not `creditedAs`) to embed the link inline. The link text itself + // still renders the credit via ALink. + const record = { + artist: 'Planetary Assault Systems', + participants: { + artist: [ + { + id: 'canon-1', + name: 'Planetary Assault Systems', + creditedAs: 'PAS', + }, + ], + }, + } + + render() + + const link = screen.getByRole('link') + // Link text is the credit, tooltip carries canonical + expect(link).toHaveTextContent('PAS') + expect(link).toHaveAttribute('title', 'Planetary Assault Systems') + // The original canonical string should not appear as raw plain text + // (it was replaced by the link) + expect( + screen.queryByText(/^Planetary Assault Systems$/), + ).not.toBeInTheDocument() + }) + }) + describe('when limiting displayed artists', () => { it('limits the number of artists displayed', () => { const record = {