Skip to content
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.

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

Spotify plugin overwrites the MB IDs even if we have the correct ones #4487

Closed
arsaboo opened this issue Sep 14, 2022 · 8 comments
Closed

Spotify plugin overwrites the MB IDs even if we have the correct ones #4487

arsaboo opened this issue Sep 14, 2022 · 8 comments
Labels
needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." stale

Comments

@arsaboo
Copy link
Contributor

arsaboo commented Sep 14, 2022

The Spotify plugin (this may be true for other plugins, but I have not checked them all) currently populates all the fields including the mb_album_id and mb_artist_id with spotify_album_id and spotify_artist_id. This makes sense during the initial import (or when we are only using Spotify as the tag source) when we don't have the MB ids and we need some IDs to be populated. However, if we initially imported the album with MusicBrainz plugin and the MB ids are correctly populated, Spotify (or any other plugin) should not overwrite the correct MB IDs.

Basically, any plugin should check if the MB ID fields are empty (i.e., initial import) and only then populate those fields. If MB IDs are already present, the plugin should only update the plugin-specific fields. Thus, if Spotify tagger is used after the album was already imported using MB, Spotify should only update the Spotify-specific fields and not incorrectly change the MB ids to Spotify IDs.

@sampsyo sampsyo added the needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." label Sep 14, 2022
@sampsyo
Copy link
Member

sampsyo commented Sep 14, 2022

Hello! I think this is roughly the same problem as #604… namely, we would like non-MB sources to not ever overwrite the MBID fields with non-MBID data. This just requires one trick to be solved: namely, fixing the way we collapse duplicate results. But given a solution there, we could just stop "polluting" this field altogether, as the original issue puts it.

@arsaboo
Copy link
Contributor Author

arsaboo commented Sep 15, 2022

While the bigger issue should be resolved, I actually had something simpler in mind. It may be ok to write the mbid fields during the initial import if non-MB tag source is being used as we use mbid as identifier in beets. However, once we have a MBid (can be checked since they have a specific format), non-mbid taggers should not overwrite them. This will require least disruption. Can we check if mbid is populated and then decide whether to overwrite it or not?

@sampsyo
Copy link
Member

sampsyo commented Sep 15, 2022

Hmm… that may be possible, but it could be surprisingly tricky. The reason is that backends supply AlbumInfo and TrackInfo objects containing the metadata to apply, and then the beets.autotag module takes care of blindly transferring that data into the actual database. There's not currently a way for a TrackInfo object to say "use this ID for disambiguation, but not actually for applying to the database." It sorta seems like we could resolve the general version of the problem just as easily as that…

@arsaboo
Copy link
Contributor Author

arsaboo commented Sep 15, 2022

It may be fine for the plugins to send all the information in the AlbumInfo or TrackInfo objects if the beets.autotag is able to pick the right information to be applied to the database. So, for example, we could add a check here before updating mb_albumid

item.mb_albumid = album_info.album_id

@sampsyo
Copy link
Member

sampsyo commented Sep 16, 2022

Yes, indeed, that would be the place to do it. But we'd need to plumb through some kind of information that says "this album_id is unreliable; please don't use it for real." At that point, I think the effort is about the same as just fixing the underlying issue—that is, separating the mb_albumid field from a different field that is used solely for disambiguation.

@arsaboo
Copy link
Contributor Author

arsaboo commented Sep 19, 2022

I am not even sure how to handle this, so I am going to leave this issue open for now. But given the number of open issues/questions related to this topic, I sincerely hope that we address this issue sooner than later.

@stale
Copy link

stale bot commented Nov 22, 2022

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 22, 2022
@JOJ0
Copy link
Member

JOJ0 commented Nov 23, 2022

I think it's still relevant. Even though there are similar issues open, in here are some useful hints on how it could possibly fixed. HTH

@stale stale bot closed this as completed Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." stale
Projects
None yet
Development

No branches or pull requests

3 participants