fix(autotag): only write mb_* fields when data source is MusicBrainz#6507
fix(autotag): only write mb_* fields when data source is MusicBrainz#6507aaronk6 wants to merge 1 commit intobeetbox:masterfrom
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull request overview
PR make sure beets only write mb_* id fields when match data come from MusicBrainz, so Spotify/Deezer/etc ids no longer get stuffed into MusicBrainz slots and break downstream clients.
Changes:
- Guard
apply_item_metadata()so it only writesmb_trackid/mb_artistid(s)whentrack_info.data_source == "MusicBrainz". - Guard
apply_metadata()so it only writes allmb_*ids whenalbum_info.data_source == "MusicBrainz".
| if track_info.data_source == "MusicBrainz": | ||
| item.mb_trackid = track_info.track_id | ||
| item.mb_releasetrackid = track_info.release_track_id | ||
| if track_info.artist_id: | ||
| item.mb_artistid = track_info.artist_id | ||
| if track_info.artists_ids: | ||
| item.mb_artistids = track_info.artists_ids |
There was a problem hiding this comment.
grug want test. new behavior: when AlbumInfo/TrackInfo data_source not MusicBrainz (Spotify, Deezer), mb_* fields should not get service id. add regression test in test/autotag/test_autotag.py for apply_metadata + apply_item_metadata to prove mb_* stay empty/cleared.
There was a problem hiding this comment.
I’ll add the test once I have maintainer feedback.
dc21308 to
757ffaf
Compare
|
This will break beets for all items that are not from MusicBrainz. As I mentioned previously, these fields act as central ID storage mechanism for any data source. For example, see data source counts in my personal library: |
Description
Plugins like Spotify and Deezer set
album_id,artist_id, andtrack_idon theirAlbumInfo/TrackInfoobjects, whichapply_metadata()andapply_item_metadata()then unconditionally write tomb_albumid,mb_artistid, etc. This means service-specific IDs end up stored in MusicBrainz fields, breaking clients such as Music Assistant.This PR adds a guard so that
mb_*fields are only written whendata_source == "MusicBrainz". Let me know if this is a good fix. If so, I’ll update the changelog and add some unit tests.Note that a broader approach (registering Spotify/Deezer IDs as their own media fields) was explored in #6349, but it turned out to be quite complex, so this PR is just fixing the obvious problem.
To Do