Skip to content

#1555 - Use plural ALBUMARTISTS / ARTISTS as authoritative source of individual contributors #1557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Rouzax wants to merge 2 commits into
base: public/9.2
Choose a base branch
from
+101 −10
Draft

#1555 - Use plural ALBUMARTISTS / ARTISTS as authoritative source of individual contributors #1557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43d558d
#1555 - Stop-gap: drop MusicBrainz IDs when artist name and MBID coun…
Rouzax Apr 15, 2026
0bfb154
#1555 - Use plural ALBUMARTISTS / ARTISTS as authoritative source of …
Rouzax Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Changelog9.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ <h2><a name="v9.2.0" id="v9.2.0"></a>Version 9.2.0</h2>
<ul>
<li><a href="https://github.com/LMS-Community/slimserver/pull/1517">#1517</a> - Fix work images and artwork precaching (@darrell-k)</li>
<li><a href="https://github.com/LMS-Community/slimserver/pull/1553">#1553</a> - Allow plugins to shut down before closing the database (@SamInPgh)</li>
<li><a href="https://github.com/LMS-Community/slimserver/issues/1555">#1555</a> - Scanner: stop-gap to prevent contributor row poisoning when the MusicBrainz ID count does not match the artist name count. (@Rouzax)</li>
<li><a href="https://github.com/LMS-Community/slimserver/issues/1555">#1555</a> - Scanner: read the Picard-convention plural tags ALBUMARTISTS and ARTISTS as the authoritative list of individual contributors for the ALBUMARTIST and ARTIST roles, zipped positionally with MUSICBRAINZ_ALBUMARTIST_ID / MUSICBRAINZ_ARTIST_ID. The singular tag still provides the album/track display string. Behaviour for libraries without plural tags is unchanged. Run <code>wipecache</code> to benefit on already-scanned libraries. (@Rouzax)</li>
<li></li>
</ul>
<br />
Expand Down
92 changes: 84 additions & 8 deletions Slim/Schema.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2842,6 +2842,7 @@ sub _preCheckAttributes {
MUSICBRAINZ_ARTIST_ID MUSICBRAINZ_ALBUMARTIST_ID MUSICBRAINZ_ALBUM_ID
MUSICBRAINZ_ALBUM_TYPE MUSICBRAINZ_ALBUM_STATUS RELEASETYPE
ALBUM_EXTID ARTIST_EXTID WORK WORKSORT
ARTISTS ALBUMARTISTS TRACKARTISTS
))
{

Expand Down Expand Up @@ -3126,34 +3127,109 @@ sub _mergeAndCreateContributors {
$attributes->{'TRACKARTISTSORT'} = delete $attributes->{'ARTISTSORT'};
$attributes->{'MUSICBRAINZ_TRACKARTIST_ID'} = delete $attributes->{'MUSICBRAINZ_ARTIST_ID'} if $attributes->{'MUSICBRAINZ_ARTIST_ID'};

# Issue #1555 - carry the plural ARTISTS tag across too, so the
# per-role plural lookup below finds it under TRACKARTISTS.
$attributes->{'TRACKARTISTS'} = delete $attributes->{'ARTISTS'} if $attributes->{'ARTISTS'};

main::DEBUGLOG && $isDebug && $log->debug(sprintf("-- Contributor '%s' of role 'ARTIST' transformed to role 'TRACKARTIST'",
$attributes->{'TRACKARTIST'},
ref($attributes->{'TRACKARTIST'}) eq 'ARRAY'
? join(' / ', @{$attributes->{'TRACKARTIST'}})
: $attributes->{'TRACKARTIST'},
));
}
}

my %contributors = ();

# Issue #1555 - roles for which a plural Picard tag (ARTISTS,
# ALBUMARTISTS, TRACKARTISTS) is the authoritative individual-contributor
# list. The singular tag remains the display string; the plural tag,
# when present as a non-empty arrayref after trimming, supersedes
# splitList parsing of the singular for contributor-row creation.
my %pluralRoles = map { $_ => 1 } qw(ARTIST ALBUMARTIST TRACKARTIST);

for my $tag (Slim::Schema::Contributor->contributorRoles) {

my $contributor = $attributes->{$tag} || next;
my $contributor = $attributes->{$tag};
my $brainzID = $attributes->{"MUSICBRAINZ_${tag}_ID"};
my $sortBy = $attributes->{$tag . 'SORT'};
my $usedPlural = 0;

# Issue #1555 - prefer the plural Picard tag as the individual
# contributor list when it is a non-empty arrayref with at least
# one non-blank name. Trim, drop empty-name slots, and drop the
# corresponding MBID slot so positional alignment is preserved.
# Empty-string MBID slots (for contributors without an MBID,
# Picard convention) are kept; Contributor->add's `if ($mbid)`
# guard handles them. A scalar plural value is treated as "not
# plural" and falls through to the legacy singular path.
#
# Sort alignment: Picard has no standard plural sort tag, but a
# user script can overwrite the singular ALBUMARTISTSORT /
# ARTISTSORT tag with a multi-value list aligned to the plural
# names (using %_albumartists_sort% / %_artists_sort%). Accept
# that if the sort tag is an arrayref of matching length after
# empty-slot filtering. Otherwise pass undef so Contributor->add
# falls back to sort-by-name rather than polluting the first
# contributor with the joined singular sort string.
if ($pluralRoles{$tag}) {
my $plural = $attributes->{$tag . 'S'};
if (ref($plural) eq 'ARRAY' && @$plural) {
my $pluralMbid = $attributes->{"MUSICBRAINZ_${tag}_ID"};
my $pluralSort = $attributes->{$tag . 'SORT'};
my @mbids = ref($pluralMbid) eq 'ARRAY' ? @$pluralMbid : ();
my @sorts = ref($pluralSort) eq 'ARRAY' ? @$pluralSort : ();
Comment on lines +3180 to +3181
Copy link
Copy Markdown
Member

@michaelherger michaelherger Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you de-reference these two variables and not just use them directly?

Copy link
Copy Markdown
Contributor Author

@Rouzax Rouzax Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dereference is there to normalise the shape once so the rest of the loop can treat it as a plain list. Two concrete reasons:

  • $attributes->{"MUSICBRAINZ_${tag}_ID"} / ...SORT can arrive as a scalar (single MBID from a legacy singular tag), an arrayref (Picard multi-value), or undef, independently of whether the plural names tag is an arrayref. Flattening to @mbids / @sorts collapses all the "not a usable list" cases to an empty array, so $mbids[$i] at line 3189 is safe regardless of input shape.
  • The boolean checks if @mbids (3189) and @mbids ? ... : $brainzID (3194) mean "has at least one element". Using $pluralMbid directly has the wrong truthiness: an empty arrayref is truthy, so if ($pluralMbid) would wrongly enter the branch for []. I'd need ref($pluralMbid) eq 'ARRAY' && @$pluralMbid at every use site.

If you'd rather keep arrayrefs throughout for consistency with the rest of the function I'm happy to switch, but the guards at every use site would get noisier.

Copy link
Copy Markdown
Contributor Author

@Rouzax Rouzax Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: I'm signing out for the day and will be away the coming days so won't be able to respond directly

my (@names, @alignedMbids, @alignedSorts);
for (my $i = 0; $i < @$plural; $i++) {
my $name = $plural->[$i];
next unless defined $name;
$name =~ s/^\s+//; $name =~ s/\s+$//;
next if $name eq '';
push @names, $name;
push @alignedMbids, $mbids[$i] if @mbids;
push @alignedSorts, $sorts[$i] if @sorts;
}
if (@names) {
$contributor = \@names;
$brainzID = @mbids ? \@alignedMbids : $brainzID;
$sortBy = (@sorts && @alignedSorts == @names) ? \@alignedSorts : undef;
$usedPlural = 1;
main::DEBUGLOG && $isDebug && $log->debug(sprintf(
"-- Using plural %sS (%d entries) as contributor source for role '%s'%s",
$tag, scalar @names, $tag,
$sortBy ? ' with aligned sort' : '',
));
}
}
}

# Skip unless we have a source of contributor names for this role.
# Empty string, undef, or an empty plural array all skip.
next unless $usedPlural || (defined $contributor && (ref $contributor || $contributor ne ''));

# Bug 17322, strip leading/trailing spaces from name
$contributor =~ s/^ +//;
$contributor =~ s/ +$//;
# Bug 17322, strip leading/trailing spaces from name. splitTag
# already trims arrayref elements, so the regex strip is scalar
# only.
unless (ref $contributor) {
Copy link
Copy Markdown
Member

@michaelherger michaelherger Apr 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how some variables can be either a scalar or a list ref. Can't we have separate variables for them? And if they were list refs, wouldn't we have to run the cleanup on all of its items?

I see this pattern in other places, like line 3135.

Copy link
Copy Markdown
Contributor Author

@Rouzax Rouzax Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, both fair points. Taking them separately:

Scalar-or-arrayref variables. Agreed that it's awkward, but the polymorphism predates this PR: $contributor = $attributes->{$tag} has always been able to be either shape depending on the tag reader, and the ref(...) eq 'ARRAY' ? join(...) : $scalar pattern (e.g. the debug log at the old line 3135 you pointed at) is legacy. Splitting into typed variables ($contributorScalar vs $contributorList, same for $brainzID / $sortBy) is a worthwhile cleanup but it touches the whole _mergeAndCreateContributors loop and the Contributor->add call shape. I'd prefer to keep this PR scoped to #1555 and open a follow-up refactor issue for the variable split. Happy to file that and link it from here if you agree.

Cleanup on arrayref items. This one is a real gap and I'll fix it in this PR. The existing unless (ref $contributor) guard relies on the older "splitTag already trims arrayref elements" invariant (see the comment just above it). The new plural-tag path I added trims explicitly at the build step, so arrayrefs from that source are clean, but an arrayref coming through the legacy path isn't guaranteed to be. I'll replace the scalar-only strip with a shape-aware version:

if (ref $contributor eq 'ARRAY') {
    for my $n (@$contributor) {
        next unless defined $n;
        $n =~ s/^\s+//;
        $n =~ s/\s+$//;
    }
} else {
    $contributor =~ s/^ +//;
    $contributor =~ s/ +$//;
}

That way the cleanup runs regardless of which path populated $contributor.

Re: the line 3135 reference, that one is a debug-log formatter rather than a cleanup site, so I don't think it needs a behavioural change, but I'll fold it into the follow-up refactor along with the other polymorphism sites.

$contributor =~ s/^ +//;
$contributor =~ s/ +$//;
}

# Is ARTISTSORT/TSOP always right for non-artist
# contributors? I think so. ID3 doesn't have
# "BANDSORT" or similar at any rate.
push @{ $contributors{$tag} }, Slim::Schema::Contributor->add({
'artist' => $contributor,
'brainzID' => $attributes->{"MUSICBRAINZ_${tag}_ID"},
'sortBy' => $attributes->{$tag.'SORT'},
'brainzID' => $brainzID,
'sortBy' => $sortBy,
# only store EXTID for track artist, as we don't have it for other roles
'extid' => $tag eq 'ARTIST' && $attributes->{'ARTIST_EXTID'},
});

main::DEBUGLOG && $isDebug && $log->is_debug && $log->debug(sprintf("-- Track has contributor '$contributor' of role '$tag'"));
main::DEBUGLOG && $isDebug && $log->is_debug && $log->debug(sprintf(
"-- Track has contributor '%s' of role '$tag'",
ref($contributor) eq 'ARRAY' ? join(' / ', @$contributor) : $contributor,
));
}

# Bug 15553, Primary contributor can only be Album Artist or Artist,
Expand Down
17 changes: 15 additions & 2 deletions Slim/Schema/Contributor.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ my @allAlbumLinkRoles;
my @inArtistsRoles;

my $prefs = preferences('server');
my $log = logger('scan.scanner');

initializeRoles();

Expand Down Expand Up @@ -248,6 +249,20 @@ sub add {
my @brainzIDList;
if ($brainzID) {
@brainzIDList = Slim::Music::Info::splitTag($brainzID);

# Issue #1555 - if the MBID count differs from the name count, the
# positional pairing below would bind an MBID to a joined-display name
# (e.g. "Artist A & Artist B" with 2 MBIDs, or "Artist A ft. Artist B"
# with 1 name after splitTag and 2 MBIDs), poisoning the contributor
# row. Drop the ambiguous MBIDs rather than mis-bind them.
if (@brainzIDList && scalar(@brainzIDList) != scalar(@artistList)) {
main::INFOLOG && $log->is_info && $log->info(
"Ignoring MBID list for '$artist': name count ("
. scalar(@artistList) . ") differs from MBID count ("
. scalar(@brainzIDList) . ")"
);
@brainzIDList = ();
}
}

# Using native DBI here to improve performance during scanning
Expand Down Expand Up @@ -356,8 +371,6 @@ sub isInLibrary {
sub rescan {
my ( $class, $ids, $albumId ) = @_;

my $log = logger('scan.scanner');

my $dbh = Slim::Schema->dbh;

my $contributorSth = $dbh->prepare_cached( qq{
Expand Down