Skip to content

Empty metadata support for autotagger plugins #6065

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.

Sign up for GitHub

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

Open
semohr wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Empty metadata support for autotagger plugins #6065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a27cf64
Unified search string construction between albums and items.
semohr Oct 20, 2025
e5d5ce1
Fixed issues where empty strings would break the search api of various
semohr Oct 20, 2025
a280237
Enhanced changelog.
semohr Oct 20, 2025
ddca7c4
Revert "Unified search string construction between albums and items."
semohr Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion beetsplug/beatport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,16 @@ def _get_releases(self, query: str) -> Iterator[AlbumInfo]:
# Strip medium information from query, Things like "CD1" and "disk 1"
# can also negate an otherwise positive result.
query = re.sub(r"\b(CD|disc)\s*\d+", "", query, flags=re.I)

# query may be empty strings
# We want to skip the lookup in this case.
if not query.strip():
self._log.debug(
"Empty search query after preprocessing, skipping {.data_source}.",
self,
)
return

for beatport_release in self.client.search(query, "release"):
if beatport_release is None:
continue
Expand Down Expand Up @@ -522,8 +532,18 @@ def _get_artist(self, artists):
"""
return self.get_artist(artists=artists, id_key=0, name_key=1)

def _get_tracks(self, query):
def _get_tracks(self, query: str):
"""Returns a list of TrackInfo objects for a Beatport query."""

# query may be empty strings
# We want to skip the lookup in this case.
if not query.strip():
Copy link
Member

@snejus snejus Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define a helper method in MetadataSourcePlugin to deduplicate this logic?

    ...
    def is_query_empty(query: str) -> bool:
        if not query.strip():
            self._log.debug(
                "Empty search query after preprocessing, skipping {.data_source}.",
                self,
            )
            return True

        return False

and then have

    if self.is_query_empty(query):
        return []

here.

Copy link
Contributor Author

@semohr semohr Dec 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we could add MetadataSourcePlugin.is_query_empty. And I thought about this too. The issue I see here is that we again start to pollute our currently very clean MetadataSourcePlugin interface with functions that are not used in the core and only used by some subclasses. This kinda signals wrong abstraction boundaries to me, think interface segregation principle.

We can although add a helper function to the beetsplug._utils module and I would be more than fine with that.

Copy link
Member

@snejus snejus Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern about interface segregation, but looking at the alternative, I strongly disagree that beetsplug._utils is the cleaner abstraction.

If we move this validation there, we force every metadata source plugin to import from a module that is currently a "junk drawer" of unrelated concerns. _utils contains a wide variety of utils (image manipulation, requests, vfs).

Forcing plugins to depend on that chaotic module just to perform a simple string check is a much stronger violation of separation of concerns than adding a single protected helper method to the base class they already inherit from.

MetadataSourcePlugin is the cohesive home for shared behaviour of metadata sources. is_query_empty is specific to the domain of these plugins (handling query inputs). Placing it on the base class keeps the dependencies clean and the logic where it belongs, rather than scattering it into a global utility bucket.

Copy link
Contributor Author

@semohr semohr Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move this validation there, we force every metadata source plugin to import from a module that is currently a "junk drawer" of unrelated concerns. _utils contains a wide variety of utils (image manipulation, requests, vfs).

The core issue for me is that not every metadata plugin actually needs this helper. If all metadata sources depended on it, then abstracting it into core would make sense. But that’s not the case thus I’m hesitant to add it to the main interface. Interfaces should define the minimal, common contract, not accumulate convenience helpers that only some implementations need.

MetadataSourcePlugin is the cohesive home for shared behaviour of metadata sources.

At the moment, MetadataSourcePlugin is intentionally designed as a boundary between external plugin code and the beets core. After the split from BeetsPlugin, we were quite deliberate about moving any shared functionality that isn’t required by the core layer into more specific abstractions like APIMetadataSourcePlugin.

I completely understand the motivation here, having shared helpers in a common namespace often does feel like the right direction. However, in this case, adding common functions directly to the interface risks muddying the separation between the plugin layer and the core layer. For me that separation has been a conscious design goal with the split into MetadataSourcePlugin.

self._log.debug(
"Empty search query after preprocessing, skipping {.data_source}.",
self,
)
return []

bp_tracks = self.client.search(query, release_type="track")
tracks = [self._get_track_info(x) for x in bp_tracks]
return tracks
14 changes: 13 additions & 1 deletion beetsplug/discogs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,10 @@ def get_track_from_album(
return track_info

def item_candidates(
self, item: Item, artist: str, title: str
Copy link
Member

@snejus snejus Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing seems to have changed in this line. Re your own comment: #6184 (comment)

self,
item: Item,
artist: str,
title: str,
) -> Iterable[TrackInfo]:
albums = self.candidates([item], artist, title, False)

Expand Down Expand Up @@ -291,6 +294,15 @@ def get_albums(self, query: str) -> Iterable[AlbumInfo]:
# can also negate an otherwise positive result.
query = re.sub(r"(?i)\b(CD|disc|vinyl)\s*\d+", "", query)

# query may be empty strings
# We want to skip the lookup in this case.
if not query.strip():
self._log.debug(
"Empty search query after preprocessing, skipping {.data_source}.",
self,
)
return []

try:
results = self.discogs_client.search(query, type="release")
results.per_page = self.config["search_limit"].get()
Expand Down
5 changes: 5 additions & 0 deletions beetsplug/musicbrainz.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,11 @@ def _search_api(
self._log.debug(
"Searching for MusicBrainz {}s with: {!r}", query_type, filters
)

if not filters:
self._log.debug("No valid filters provided, skipping search.")
return []

try:
method = getattr(musicbrainzngs, f"search_{query_type}s")
res = method(limit=self.config["search_limit"].get(), **filters)
Expand Down
7 changes: 7 additions & 0 deletions beetsplug/spotify.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,13 @@ def _search_api(
filters=filters, query_string=query_string
)

if not query.strip():
self._log.debug(
"Empty search query after applying filters, skipping {.data_source}.",
self,
)
return []

self._log.debug("Searching {.data_source} for '{}'", self, query)
try:
response = self._handle_response(
Expand Down
4 changes: 4 additions & 0 deletions docs/changelog.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ New features:

Bug fixes:

- :doc:`plugins/discogs`, :doc:`plugins/beatport`, :doc:`plugins/spotify`,
:doc:`plugins/musicbrainz`: Fix an issue where no metadata in a file would
crash the import process :bug:`6060`

For packagers:

Other changes:
Expand Down