Fix Tidal album artist duplication #182
Conversation
kellnerd
left a comment
kellnerd
left a comment
There was a problem hiding this comment.
Thank you, that should address the duplication issue (and any ordering issues).
It might break with https://tidal.com/album/16533576, the missing track artist is probably also a release artist which the currently deployed version skips?
(I can't test it right now as I'm in the middle of work on a different branch.)
P.S. Please reword your commit message to follow conventional commits if you are pushing new commits anyway, otherwise I will do that during the merge.
| creditedName: "Bruno Mars", | ||
| creditedName: "Lady Gaga", | ||
| externalIds: [ | ||
| { | ||
| id: "3658521", | ||
| id: "3534754", |
There was a problem hiding this comment.
Good catch, I didn't even notice the wrong order when I added that test case. But the new order makes sense, it follows the cover art and the Tidal website (as of today at least).
P.S. Have you already tried to update the cached API testdata for this test case? I wonder whether the artist entries are now duplicated as well. I probably wouldn't commit the bloated testdata.
But maybe the Armin van Buuren release with both missing and duplicated resources would make for a good test case?
I guess we could report both API issues to Tidal, maybe they will fix them.
There was a problem hiding this comment.
No, I haven't tried updating the cached testdata. I don't actually know how to handle this.
But I think I was lucky that the test data exposed this issue, because it allowed testing the situation.
There was a problem hiding this comment.
I've updated the historic testdata locally, the new response would indeed duplicate the two artists (and add a few new but irrelevant fields as well).
Since this specific test case is also used to guarantee support for an older variant of the v2 API, I didn't commit the changes.
I haven't tried updating the cached testdata. I don't actually know how to handle this.
https://github.com/kellnerd/harmony/blob/main/CONTRIBUTING.md#testing should get you started the next time.
| }); | ||
| // Rely on the target relationship to get the correct number and order of related | ||
| // items. Consider the case that an item was not actually returned in the includes. | ||
| return targetRels.data.map((rel) => items.get(rel.id) || { id: rel.id } as T); |
There was a problem hiding this comment.
Do we currently need the fallback value at all? As far as I can see it doesn't matter whether we return the incomplete object or undefined, all callers of getRelatedItems aren't prepared to handle either case.
That's the big downside of the type assertion here, you don't notice such issues until they happen.
So far I've only seen missing resources for track artists, which has a completely separate implementation in getTrackArtists which doesn't call getRelatedItems.
In my local files I've even left a note on this function which mentions the possibility of the current issue, but I hadn't addressed it so far:
TODO: Track AC conversion is inconsistent with release AC conversion (which might be in the wrong order since it filters the existing resources instead of mapping the IDs to resources).
Store artistMap as this.artistMap?
In the long term we probably want to properly type the getRelatedItems helper or abandon it if there is no clean way to do it. I like the approach with the artistMap that we do for tracks, maybe a generic this.resourceMap and a suitable helper function is the way to go?
There was a problem hiding this comment.
Exactly because of the concerns that also release artists might be missing. I haven't seen such a case, but I think it could happen.
But yes, we could refactor this to a similar system then as used for tracks. Actually the way tracks handle this was added later, and it was done separated as the existing getRelatedItems didn't work for this use case.
The existing code returned all matching entries from included items, but Tidal API sometimes returns duplicates. Also the included items not necessarily reflect the same order as the relationships.
phw
force-pushed
the
fix-tidal-artist-duplication
branch
from
a89057f to
09fd264
Compare
phw
left a comment
phw
left a comment
There was a problem hiding this comment.
I have updated the commit message. But I won't have the time do deeply refactor the artist handling. Hence I would suggest to merge the fix and do the refactoring separately.
It might break with https://tidal.com/album/16533576, the missing track artist is probably also a release artist which the currently deployed version skips?
No, it doesn't, the release artists are all present. I don't know of a case for which it isn't. But we should expect that this can happen.
| creditedName: "Bruno Mars", | ||
| creditedName: "Lady Gaga", | ||
| externalIds: [ | ||
| { | ||
| id: "3658521", | ||
| id: "3534754", |
There was a problem hiding this comment.
No, I haven't tried updating the cached testdata. I don't actually know how to handle this.
But I think I was lucky that the test data exposed this issue, because it allowed testing the situation.
| }); | ||
| // Rely on the target relationship to get the correct number and order of related | ||
| // items. Consider the case that an item was not actually returned in the includes. | ||
| return targetRels.data.map((rel) => items.get(rel.id) || { id: rel.id } as T); |
There was a problem hiding this comment.
Exactly because of the concerns that also release artists might be missing. I haven't seen such a case, but I think it could happen.
But yes, we could refactor this to a similar system then as used for tracks. Actually the way tracks handle this was added later, and it was done separated as the existing getRelatedItems didn't work for this use case.
2 participants
This is a fix for the Tidal artist duplication reported in #181
The existing code returned all matching entries from included items, but Tidal API sometimes returns duplicates. Also the included items not necessarily reflect the same order as the relationships.
I haven't seen the latter being a problem, but purely from the code it is a potential issue that is also fixed with this change. And the existing test case seems to have at least at some point had this case, where it contained "Bruno Mars" and "Lady Gaga" as album artist as opposed this other way around (compare https://tidal.com/album/381265361).