Avoid Unidecoding Query Strings in Deezer Metadata Plugin #5882

semohr · 2025-07-17T15:14:34Z

Moved the _construct_search_query method that was duplicated in the spotify and deezer plugins into the (new) shared helper baseclass ( SearchApiMetadataSourcePlugin).

By default deezer now also does not ascii encode a query when it is send but the legacy behaviour can be restored using the deezer.search_query_ascii option.

closes #5860

Changelog
Documentation

Copilot

Pull Request Overview

This PR moves duplicated search query construction logic from the Spotify and Deezer plugins into a shared base class and changes the default behavior to avoid ASCII encoding of search queries. The key motivation is to improve search results for non-Latin characters while maintaining backward compatibility.

Refactored _construct_search_query method into shared SearchApiMetadataSourcePlugin base class
Changed default behavior to preserve Unicode characters in search queries instead of ASCII encoding
Added configuration option search_query_ascii to restore legacy ASCII encoding behavior

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
beets/metadata_plugins.py	Added shared `_construct_search_query` method and `search_query_ascii` config option to base class
beetsplug/spotify.py	Removed duplicated `_construct_search_query` method and config option
beetsplug/deezer.py	Removed duplicated `_construct_search_query` method and unidecode import
docs/plugins/deezer.rst	Added documentation for new `search_query_ascii` configuration option
docs/changelog.rst	Added changelog entry describing the fix and new configuration option

docs/plugins/deezer.rst

beets/metadata_plugins.py

deezer and spotify functionalities.

codecov · 2025-07-17T15:37:27Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.77%. Comparing base (bde5de4) to head (d2cca4a).

Files with missing lines	Patch %	Lines
beets/metadata_plugins.py	90.90%	1 Missing ⚠️
beetsplug/deezer.py	0.00%	1 Missing ⚠️

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

snejus · 2025-07-18T20:41:34Z

beets/metadata_plugins.py

+        if self.config["search_query_ascii"].get():
+            query = unidecode.unidecode(query)


I'd say we should just bomb unidecode. I just checked that deezer returns crap if I unidecode a japanese album name, and it returns the correct album if I don't.

requests these days are more than capable of dealing with any sort of strings, so I think we can safely unidecode for good.

I thought so too before I looked into why this was implemented in the first place. I don't think we should just remove it. The query never was the issue. The main issue is the terminal or badly pasted queries if I remember correctly.

Please have a look at #5705

beets/metadata_plugins.py

snejus · 2025-07-18T20:59:28Z

beets/metadata_plugins.py

+
+        query_components = [
+            keywords,
+            " ".join(f'{k}:"{v}"' for k, v in filters.items()),


What happens if there is a double quote character in the query?

No idea. Depends on spotify or deezer, I guess. I check that both support quoted queries and they do but have not checked double quoted.

Deezer even needs quoted strings otherwise the api just returns rubbish. Before this PR spotify did not quote string whereas deezer did. Was the only difference in the functions.

snejus · 2025-07-18T21:12:34Z

beets/metadata_plugins.py

+        """Construct a query string with the specified filters and keywords to
+        be provided to the Spotify (or similar) Search API.
+
+        At the moment, this is used to construct a query string for:
+        - Spotify (https://developer.spotify.com/documentation/web-api/reference/search).
+        - Deezer (https://developers.deezer.com/api/search).
+
+        :param filters: Field filters to apply.
+        :param keywords: Query keywords to use.
+        :return: Query string to be provided to the Search API.
+        """


The interface sets up this method, but it shouldn't know the specifics about what implements it.

Since the parameters' names are clear, I don't think we need to repeat this information in the docstring.

Suggested change

"""Construct a query string with the specified filters and keywords to

be provided to the Spotify (or similar) Search API.

At the moment, this is used to construct a query string for:

- Spotify (https://developer.spotify.com/documentation/web-api/reference/search).

- Deezer (https://developers.deezer.com/api/search).

:param filters: Field filters to apply.

:param keywords: Query keywords to use.

:return: Query string to be provided to the Search API.

"""

"""Construct a query with the specified filters and query string."""

I agree, the issue is tho that we lose the references to the docs if we just remove it, as these methods aren't present in the plugins file anymore. There is no repeated information.

Also this function kinda is bound to the implementation specific as it only makes sense in the context of these two plugins and for the deezer and spotify api. Again kinda an ambiguity with our deeper structure if you ask me:

plugin = beetsplug(beets) # vs plugin = beets(beetsplug(beets))

This docstring sort of violates the Open/Closed SOLID principle: if we add another data source that inherits SearchApiMetadataSourcePlugin and uses this method, we will need to update this docstring, which is bad practice. Practically speaking, this docstring would not get updated and end up being misleading.

This method (including the docstring) should be implemented/documented in a generic manner and closed for modification in order to prevent this issue.

You can move the URLs under respective _search_api implementations instead.

Yeah, I can move the comment, but I still think it fits better alongside the query function. The root issue here is that this setup already violates the dependency inversion principle, so moving the comment doesn’t really solve anything, it just reinforces the design flaw.

This docstring sort of violates the Open/Closed SOLID principle: if we add another data source that inherits SearchApiMetadataSourcePlugin and uses this method, we will need to update this docstring, which is bad practice. Practically speaking, this docstring would not get updated and end up being misleading.

I get the concern about the docstring violating the Open/Closed Principle, and that’s valid, but we’re already operating in a structure that makes adhering to these principles tricky. Rather than selectively applying them, I think we should either address the underlying design or be explicit about the compromises we're making.

I mainly think it is important for someone looking at the function to know why the query string looks that way. And this is only clear in the context of the spotify api documentation.

I don't think I'm following you regarding these two points:

The root issue here is that this setup already violates the dependency inversion principle

And this is only clear in the context of the spotify api documentation

I've been trying to point out for quite some time that there's a problem with how plugins and core logic are not properly separated (see, for example, Avoid Unidecoding Query Strings in Deezer Metadata Plugin #5882 (comment) or the get_artists discussion). Our ongoing debates stem from this fundamental issue. Since you value design principles, this essentially comes down to a violation of the dependency inversion principle.

Consider encountering the create_query function somewhere without any context. A comment explaining why the query is constructed that way, such as linking to spotifys or deezers specifications, would be very helpful. Having to dive into the spotify or deezer plugin just to understand the rationale makes maintenance harder, even if adding such comments slightly bends some design principles.

removed legacy isinstance check

semohr requested review from Copilot and snejus and removed request for Copilot July 17, 2025 15:14

Copilot AI reviewed Jul 17, 2025

View reviewed changes

docs/plugins/deezer.rst Outdated Show resolved Hide resolved

beets/metadata_plugins.py Outdated Show resolved Hide resolved

Moved construct search into SearchApiMetadataSource to dedupe some

e81856e

deezer and spotify functionalities.

semohr force-pushed the #5860 branch from c70003d to e81856e Compare July 17, 2025 15:26

Fixed tests as query formatting changed slightly.

0b60fdc

snejus requested changes Jul 18, 2025

View reviewed changes

renamed keywords to query_string, shortened query construct expression,

d2cca4a

removed legacy isinstance check

semohr force-pushed the #5860 branch from 8a63b70 to d2cca4a Compare July 18, 2025 22:05

		if self.config["search_query_ascii"].get():
		query = unidecode.unidecode(query)

Avoid Unidecoding Query Strings in Deezer Metadata Plugin #5882

Are you sure you want to change the base?

Avoid Unidecoding Query Strings in Deezer Metadata Plugin #5882

Uh oh!

Conversation

semohr commented Jul 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semohr Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

semohr Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jul 17, 2025 •

edited

Loading

semohr Jul 18, 2025 •

edited

Loading

semohr Jul 18, 2025 •

edited

Loading