Fix albumsQuery artist and artist_id

[darrell-k]

See #1205

[michaelherger]

This doesn't really fix the issue, does it? It does check for the existence of $contributorID and $c->{albums.contributor} only for the name, but not for the ID. The latter would always be the ID from the query response. The problem seems to be inverted (always ID from database, rather than query instead of the opposite), but not fixed?

It would always use the ID from the database,

[darrell-k]

albums.contributor will always be consistent as it's a column from the base albums table. The problem is that contributor.name from the query is from the contributors table (via a join with contributor_album) where there is a match on the input parm.

It certainly took me a few brain cycles to get my head around this, but my testing of 3 scenarios (no input contributor, input contributor same as album contributor and input contributor different to album contributor) makes me believe I've got this right.

But feel free to prove me wrong!

[michaelherger]

My simplistic take is: whatever was passed, the artist_id should match the artist. Under what circumstances would we have a contributor.name which was not matching artist.contributor? Wouldn't that mean there's a problem further up the code, in the initial query?

Would you have an example how I can reproduce the problem?

[darrell-k]

You need to browse an artist who appears on the album but is not the main artist (as defined by albums.contributor). A trackartist, perhaps? (I've seen it with user-defined tags, and conductor, so you could just add "Conductor=Michael" to an album.)

Then use curl to submit a jsonrpc request, because I'm still not sure how to see the bug in the UI.

You make an interesting point about the initial query SQL: should it join to contributors on albums.contributor or contributor_album.contributor for contributors.name? It might do either, depending on the input parameters, including $sort.

And whatever we get in contributor.name is used in generating the favoritesUrl which seems to work correctly at present, but I'll check with various parameters to trigger both cases.

(presumably when you said artist.contributor above you meant albums.contributor?)

[darrell-k]

Actually, Favorites are NOT working when added in this case - it definitely needs the name associated with albums.contributor.

Another ancient bug referenced here:

slimserver/Slim/Control/Queries.pm

Lines 617 to 633 in a1719ae

	if ( $tags =~ /a/ ) {
	# If requesting artist data, join contributor
	if ( $sql !~ /JOIN contributors ON/ ) {
	if ( $sql =~ /JOIN contributor_album/ ) {
	# Bug 17364, if looking for an artist_id value, we need to join contributors via contributor_album
	# or No Album will not be found properly
	$sql .= 'JOIN contributors ON contributors.id = contributor_album.contributor ';
	}
	else {
	$sql .= 'JOIN contributors ON contributors.id = albums.contributor ';
	}
	}
	$c->{'contributors.name'} = 1;
	# if albums for a specific contributor are requested, then we need the album's contributor, too
	$c->{'albums.contributor'} = $contributorID;
	}

Maybe we could join to contributors twice, once on albums.contributor and again on contributor_album.contributor, to get both names and use as appropriate in the result loop processing to avoid that extra SQL call in the loop?

[darrell-k]

OK, I think that latest push is the solution.

One other point: should creating favorites_url be dependent on tag a, because it needs data for contributor.name? (This isn't a change - look at the existing code, it will try to create favorites_url without tag a already.)

[michaelherger]

Did you run any performance tests? That's creating a (potentially) second relation to the contributors table, which will come at a cost, right?

[michaelherger]

One other point: should creating favorites_url be dependent on tag a, because it needs data for contributor.name? (This isn't a change - look at the existing code, it will try to create favorites_url without tag a already.)

It shouldn't depend on that tag, no. Which probably means everything can be simplified further? We'll always need that relation?

[darrell-k]

I would think performance would be better. For tag a, we're adding a extra join in the main sql, instead of running an extra query for every album in the in the loop.

This remains true even if we remove the conditioning on tag a.

[michaelherger]

True, I should have asked the question before you added the additional query in the loop 😁.

Are you doing to look into the unconditional relation?

[darrell-k]

True, I should have asked the question before you added the additional query in the loop 😁.

Are you doing to look into the unconditional relation?

To be fair to me, the extra query in the loop was already there 😁.

Yes, I'm going to do the new join always unless tags = CC.

Also, I'm looking at

slimserver/Slim/Control/Queries.pm

Lines 460 to 473 in a1719ae

	elsif ( $sort eq 'artflow' ) {
	$order_by = "contributors.namesort $collate, albums.year, albums.titlesort $collate";
	$c->{'contributors.namesort'} = 1;
	$page_key = "SUBSTR(contributors.namesort,1,1)";
	}
	elsif ( $sort eq 'artistalbum' ) {
	$order_by = "contributors.namesort $collate, albums.titlesort $collate";
	$c->{'contributors.namesort'} = 1;
	$page_key = "SUBSTR(contributors.namesort,1,1)";
	}
	elsif ( $sort eq 'yearartistalbum' ) {
	$order_by = "albums.year, contributors.namesort $collate, albums.titlesort $collate";
	$page_key = "albums.year";
	}

and

slimserver/Slim/Control/Queries.pm

Lines 866 to 877 in a1719ae

	if ($tags =~ /s/) {
	#FIXME: see if multiple char textkey is doable for year/genre sort
	my $textKey;
	if ($sort eq 'artflow' || $sort eq 'artistalbum') {
	utf8::decode( $c->{'contributors.namesort'} ) if exists $c->{'contributors.namesort'};
	$textKey = substr $c->{'contributors.namesort'}, 0, 1;
	} elsif ( $sort eq 'album' ) {
	utf8::decode( $c->{'albums.titlesort'} ) if exists $c->{'albums.titlesort'};
	$textKey = substr $c->{'albums.titlesort'}, 0, 1;
	}
	$request->addResultLoopIfValueDefined($loopname, $chunkCount, 'textkey', $textKey);
	}

If the sort should always be using the albums.contributor name here, rather than the name of the input param contributorID if supplied, I think we can get rid of the join to contributors via contributor_album completely, because we'd only be using contributorID to restrict the result set, never needing the name.

What do you think?

[michaelherger]

To be fair to me, the extra query in the loop was already there 😁.

Oh... I thought you had introduced that one. But no, was me...

If the sort should always be using the albums.contributor name here, rather than the name of the input param contributorID if supplied, I think we can get rid of the join to contributors via contributor_album completely, because we'd only be using contributorID to restrict the result set, never needing the name.

Except if some particular role has to be dealt with. Or if we want the full list of contributors, right?

[darrell-k]

If the sort should always be using the albums.contributor name here, rather than the name of the input param contributorID if supplied, I think we can get rid of the join to contributors via contributor_album completely, because we'd only be using contributorID to restrict the result set, never needing the name.

Except if some particular role has to be dealt with. Or if we want the full list of contributors, right?

We'd still join to contributor_album if necessary for row selection, it's just that we'd never need to get name/namesort for that.

The lists of contributors/names are retrieved separately in $contributorSql.

Agreed?