From 3388882c215574890a47c369c6d373704c302eb0 Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Mon, 3 Nov 2025 15:52:34 +0100 Subject: [PATCH 1/7] Added a proxy to catch and handle exceptions in metadataplugins during the autotag process. --- beets/metadata_plugins.py | 151 ++++++++++++++++++++++++++++++++-- test/test_metadata_plugins.py | 97 ++++++++++++++++++++++ 2 files changed, 239 insertions(+), 9 deletions(-) create mode 100644 test/test_metadata_plugins.py diff --git a/beets/metadata_plugins.py b/beets/metadata_plugins.py index f42e8f690..28374955c 100644 --- a/beets/metadata_plugins.py +++ b/beets/metadata_plugins.py @@ -8,14 +8,25 @@ implemented as plugins. from __future__ import annotations import abc +import inspect import re -from functools import cache, cached_property -from typing import TYPE_CHECKING, Generic, Literal, TypedDict, TypeVar +from functools import cache, cached_property, wraps +from typing import ( + TYPE_CHECKING, + Callable, + ClassVar, + Generic, + Literal, + TypedDict, + TypeVar, + overload, +) import unidecode from confuse import NotFoundError -from typing_extensions import NotRequired +from typing_extensions import NotRequired, ParamSpec +from beets import config, logging from beets.util import cached_classproperty from beets.util.id_extractors import extract_release_id @@ -26,12 +37,18 @@ if TYPE_CHECKING: from .autotag.hooks import AlbumInfo, Item, TrackInfo + P = ParamSpec("P") + R = TypeVar("R") + +# Global logger. +log = logging.getLogger("beets") + @cache def find_metadata_source_plugins() -> list[MetadataSourcePlugin]: """Return a list of all loaded metadata source plugins.""" # TODO: Make this an isinstance(MetadataSourcePlugin, ...) check in v3.0.0 - return [p for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc] + return [SafeProxy(p) for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc,arg-type] @notify_info_yielded("albuminfo_received") @@ -43,7 +60,7 @@ def candidates(*args, **kwargs) -> Iterable[AlbumInfo]: @notify_info_yielded("trackinfo_received") def item_candidates(*args, **kwargs) -> Iterable[TrackInfo]: - """Return matching track candidates fromm all metadata source plugins.""" + """Return matching track candidates from all metadata source plugins.""" for plugin in find_metadata_source_plugins(): yield from plugin.item_candidates(*args, **kwargs) @@ -54,7 +71,7 @@ def album_for_id(_id: str) -> AlbumInfo | None: A single ID can yield just a single album, so we return the first match. """ for plugin in find_metadata_source_plugins(): - if info := plugin.album_for_id(album_id=_id): + if info := plugin.album_for_id(_id): send("albuminfo_received", info=info) return info @@ -259,11 +276,11 @@ class SearchFilter(TypedDict): album: NotRequired[str] -R = TypeVar("R", bound=IDResponse) +Res = TypeVar("Res", bound=IDResponse) class SearchApiMetadataSourcePlugin( - Generic[R], MetadataSourcePlugin, metaclass=abc.ABCMeta + Generic[Res], MetadataSourcePlugin, metaclass=abc.ABCMeta ): """Helper class to implement a metadata source plugin with an API. @@ -288,7 +305,7 @@ class SearchApiMetadataSourcePlugin( query_type: Literal["album", "track"], filters: SearchFilter, query_string: str = "", - ) -> Sequence[R]: + ) -> Sequence[Res]: """Perform a search on the API. :param query_type: The type of query to perform. @@ -357,3 +374,119 @@ class SearchApiMetadataSourcePlugin( query = unidecode.unidecode(query) return query + + +# To have proper typing for the proxy class below, we need to +# trick mypy into thinking that SafeProxy is a subclass of +# MetadataSourcePlugin. +# https://stackoverflow.com/questions/71365594/how-to-make-a-proxy-object-with-typing-as-underlying-object-in-python +Proxied = TypeVar("Proxied", bound=MetadataSourcePlugin) +if TYPE_CHECKING: + base = MetadataSourcePlugin +else: + base = object + + +class SafeProxy(base): + """A proxy class that forwards all attribute access to the wrapped + MetadataSourcePlugin instance. + + We use this to catch and log exceptions from metadata source plugins + without crashing beets. E.g. on long running autotag operations. + """ + + _plugin: MetadataSourcePlugin + _SAFE_METHODS: ClassVar[set[str]] = { + "candidates", + "item_candidates", + "album_for_id", + "track_for_id", + } + + def __init__(self, plugin: MetadataSourcePlugin): + self._plugin = plugin + + def __getattribute__(self, name): + if ( + name == "_plugin" + or name == "_handle_exception" + or name == "_SAFE_METHODS" + or name == "_safe_execute" + ): + return super().__getattribute__(name) + + attr = getattr(self._plugin, name) + + if callable(attr) and name in SafeProxy._SAFE_METHODS: + return self._safe_execute(attr) + return attr + + def __setattr__(self, name, value): + if name == "_plugin": + super().__setattr__(name, value) + else: + self._plugin.__setattr__(name, value) + + @overload + def _safe_execute( + self, + func: Callable[P, Iterable[R]], + ) -> Callable[P, Iterable[R]]: ... + @overload + def _safe_execute(self, func: Callable[P, R]) -> Callable[P, R | None]: ... + def _safe_execute( + self, func: Callable[P, R] + ) -> Callable[P, R | Iterable[R] | None]: + """Wrap any function (generator or regular) and safely execute it. + + Limitation: This does not work on properties! + """ + + @wraps(func) + def wrapper( + *args: P.args, **kwargs: P.kwargs + ) -> R | Iterable[R] | None: + try: + result = func(*args, **kwargs) + except Exception as e: + self._handle_exception(func, e) + + return None + + if inspect.isgenerator(result): + try: + yield from result + except Exception as e: + self._handle_exception(func, e) + return None + else: + return result + + return wrapper + + def _handle_exception(self, func: Callable[P, R], e: Exception) -> None: + """Helper function to log exceptions from metadata source plugins.""" + if config["raise_on_error"].get(bool): + raise e + log.error( + "Error in '{}.{}': {}", + self._plugin.data_source, + func.__name__, + e, + ) + log.debug("Exception details:", exc_info=True) + + # Implement abstract methods to satisfy the ABC + # this is only needed because of the typing hack above. + + def album_for_id(self, album_id: str): + raise NotImplementedError + + def track_for_id(self, track_id: str): + raise NotImplementedError + + def candidates(self, *args, **kwargs): + raise NotImplementedError + + def item_candidates(self, *args, **kwargs): + raise NotImplementedError diff --git a/test/test_metadata_plugins.py b/test/test_metadata_plugins.py new file mode 100644 index 000000000..b8cf5fb6c --- /dev/null +++ b/test/test_metadata_plugins.py @@ -0,0 +1,97 @@ +from typing import Iterable + +import pytest + +from beets import metadata_plugins +from beets.test.helper import PluginMixin + + +class ErrorMetadataMockPlugin(metadata_plugins.MetadataSourcePlugin): + """A metadata source plugin that raises errors in all its methods.""" + + data_source = "ErrorMetadataMockPlugin" + + def candidates(self, *args, **kwargs): + raise ValueError("Mocked error") + + def item_candidates(self, *args, **kwargs): + for i in range(3): + raise ValueError("Mocked error") + yield # This is just to make this a generator + + def album_for_id(self, *args, **kwargs): + raise ValueError("Mocked error") + + def track_for_id(self, *args, **kwargs): + raise ValueError("Mocked error") + + def track_distance(self, *args, **kwargs): + raise ValueError("Mocked error") + + def album_distance(self, *args, **kwargs): + raise ValueError("Mocked error") + + +class TestMetadataPluginsException(PluginMixin): + """Check that errors during the metadata plugins do not crash beets. + They should be logged as errors instead. + """ + + @pytest.fixture(autouse=True) + def setup(self): + self.register_plugin(ErrorMetadataMockPlugin) + yield + self.unload_plugins() + + @pytest.mark.parametrize( + "method_name,args", + [ + ("candidates", ()), + ("item_candidates", ()), + ("album_for_id", ("some_id",)), + ("track_for_id", ("some_id",)), + ], + ) + def test_logging( + self, + caplog, + method_name, + args, + ): + self.config["raise_on_error"] = False + with caplog.at_level("ERROR"): + # Call the method to trigger the error + ret = getattr(metadata_plugins, method_name)(*args) + if isinstance(ret, Iterable): + list(ret) + + # Check that an error was logged + assert len(caplog.records) >= 1 + logs = [record.getMessage() for record in caplog.records] + for msg in logs: + assert ( + msg + == f"Error in 'ErrorMetadataMockPlugin.{method_name}': Mocked error" + ) + + caplog.clear() + + @pytest.mark.parametrize( + "method_name,args", + [ + ("candidates", ()), + ("item_candidates", ()), + ("album_for_id", ("some_id",)), + ("track_for_id", ("some_id",)), + ], + ) + def test_raising( + self, + method_name, + args, + ): + self.config["raise_on_error"] = True + with pytest.raises(ValueError, match="Mocked error"): + getattr(metadata_plugins, method_name)(*args) if not isinstance( + args, Iterable + ) else list(getattr(metadata_plugins, method_name)(*args)) From 4511a376991cc6ab60bd3b4cfc2f3c9169edbd9e Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Mon, 3 Nov 2025 16:22:41 +0100 Subject: [PATCH 2/7] Added default config and simplified proxy class. --- beets/config_default.yaml | 2 + beets/metadata_plugins.py | 95 ++++++++++++--------------------------- 2 files changed, 30 insertions(+), 67 deletions(-) diff --git a/beets/config_default.yaml b/beets/config_default.yaml index c0bab8056..dfc0378a1 100644 --- a/beets/config_default.yaml +++ b/beets/config_default.yaml @@ -10,6 +10,8 @@ plugins: [musicbrainz] pluginpath: [] +raise_on_error: no + # --------------- Import --------------- clutter: ["Thumbs.DB", ".DS_Store"] diff --git a/beets/metadata_plugins.py b/beets/metadata_plugins.py index 28374955c..ed492b537 100644 --- a/beets/metadata_plugins.py +++ b/beets/metadata_plugins.py @@ -8,18 +8,15 @@ implemented as plugins. from __future__ import annotations import abc -import inspect import re -from functools import cache, cached_property, wraps +from functools import cache, cached_property from typing import ( TYPE_CHECKING, Callable, - ClassVar, Generic, Literal, TypedDict, TypeVar, - overload, ) import unidecode @@ -396,30 +393,22 @@ class SafeProxy(base): """ _plugin: MetadataSourcePlugin - _SAFE_METHODS: ClassVar[set[str]] = { - "candidates", - "item_candidates", - "album_for_id", - "track_for_id", - } def __init__(self, plugin: MetadataSourcePlugin): self._plugin = plugin def __getattribute__(self, name): - if ( - name == "_plugin" - or name == "_handle_exception" - or name == "_SAFE_METHODS" - or name == "_safe_execute" - ): + if name in { + "_plugin", + "_handle_exception", + "candidates", + "item_candidates", + "album_for_id", + "track_for_id", + }: return super().__getattribute__(name) - - attr = getattr(self._plugin, name) - - if callable(attr) and name in SafeProxy._SAFE_METHODS: - return self._safe_execute(attr) - return attr + else: + return getattr(self._plugin, name) def __setattr__(self, name, value): if name == "_plugin": @@ -427,43 +416,6 @@ class SafeProxy(base): else: self._plugin.__setattr__(name, value) - @overload - def _safe_execute( - self, - func: Callable[P, Iterable[R]], - ) -> Callable[P, Iterable[R]]: ... - @overload - def _safe_execute(self, func: Callable[P, R]) -> Callable[P, R | None]: ... - def _safe_execute( - self, func: Callable[P, R] - ) -> Callable[P, R | Iterable[R] | None]: - """Wrap any function (generator or regular) and safely execute it. - - Limitation: This does not work on properties! - """ - - @wraps(func) - def wrapper( - *args: P.args, **kwargs: P.kwargs - ) -> R | Iterable[R] | None: - try: - result = func(*args, **kwargs) - except Exception as e: - self._handle_exception(func, e) - - return None - - if inspect.isgenerator(result): - try: - yield from result - except Exception as e: - self._handle_exception(func, e) - return None - else: - return result - - return wrapper - def _handle_exception(self, func: Callable[P, R], e: Exception) -> None: """Helper function to log exceptions from metadata source plugins.""" if config["raise_on_error"].get(bool): @@ -476,17 +428,26 @@ class SafeProxy(base): ) log.debug("Exception details:", exc_info=True) - # Implement abstract methods to satisfy the ABC - # this is only needed because of the typing hack above. - - def album_for_id(self, album_id: str): - raise NotImplementedError + def album_for_id(self, *args, **kwargs): + try: + return self._plugin.album_for_id(*args, **kwargs) + except Exception as e: + return self._handle_exception(self._plugin.album_for_id, e) def track_for_id(self, track_id: str): - raise NotImplementedError + try: + return self._plugin.track_for_id(track_id) + except Exception as e: + return self._handle_exception(self._plugin.track_for_id, e) def candidates(self, *args, **kwargs): - raise NotImplementedError + try: + yield from self._plugin.candidates(*args, **kwargs) + except Exception as e: + return self._handle_exception(self._plugin.candidates, e) def item_candidates(self, *args, **kwargs): - raise NotImplementedError + try: + yield from self._plugin.item_candidates(*args, **kwargs) + except Exception as e: + return self._handle_exception(self._plugin.item_candidates, e) From cfba015998d48801c0b8963e6a59ac713359fd1c Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Mon, 3 Nov 2025 16:26:31 +0100 Subject: [PATCH 3/7] Fixed cache clear issue. --- test/test_metadata_plugins.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_metadata_plugins.py b/test/test_metadata_plugins.py index b8cf5fb6c..edf66adcd 100644 --- a/test/test_metadata_plugins.py +++ b/test/test_metadata_plugins.py @@ -39,6 +39,7 @@ class TestMetadataPluginsException(PluginMixin): @pytest.fixture(autouse=True) def setup(self): + metadata_plugins.find_metadata_source_plugins.cache_clear() self.register_plugin(ErrorMetadataMockPlugin) yield self.unload_plugins() From 5cbdab40d2d3028694f7e9ca27dd7f75e4ca715a Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Mon, 3 Nov 2025 16:45:32 +0100 Subject: [PATCH 4/7] Renamed variable to use protected names. --- beets/metadata_plugins.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/beets/metadata_plugins.py b/beets/metadata_plugins.py index ed492b537..05e5c5d41 100644 --- a/beets/metadata_plugins.py +++ b/beets/metadata_plugins.py @@ -392,15 +392,15 @@ class SafeProxy(base): without crashing beets. E.g. on long running autotag operations. """ - _plugin: MetadataSourcePlugin + __plugin: MetadataSourcePlugin def __init__(self, plugin: MetadataSourcePlugin): - self._plugin = plugin + self.__plugin = plugin def __getattribute__(self, name): if name in { - "_plugin", - "_handle_exception", + "_SafeProxy__plugin", + "_SafeProxy__handle_exception", "candidates", "item_candidates", "album_for_id", @@ -408,21 +408,21 @@ class SafeProxy(base): }: return super().__getattribute__(name) else: - return getattr(self._plugin, name) + return getattr(self.__plugin, name) def __setattr__(self, name, value): - if name == "_plugin": + if name == "_SafeProxy__plugin": super().__setattr__(name, value) else: - self._plugin.__setattr__(name, value) + self.__plugin.__setattr__(name, value) - def _handle_exception(self, func: Callable[P, R], e: Exception) -> None: + def __handle_exception(self, func: Callable[P, R], e: Exception) -> None: """Helper function to log exceptions from metadata source plugins.""" if config["raise_on_error"].get(bool): raise e log.error( "Error in '{}.{}': {}", - self._plugin.data_source, + self.__plugin.data_source, func.__name__, e, ) @@ -430,24 +430,24 @@ class SafeProxy(base): def album_for_id(self, *args, **kwargs): try: - return self._plugin.album_for_id(*args, **kwargs) + return self.__plugin.album_for_id(*args, **kwargs) except Exception as e: - return self._handle_exception(self._plugin.album_for_id, e) + return self.__handle_exception(self.__plugin.album_for_id, e) def track_for_id(self, track_id: str): try: - return self._plugin.track_for_id(track_id) + return self.__plugin.track_for_id(track_id) except Exception as e: - return self._handle_exception(self._plugin.track_for_id, e) + return self.__handle_exception(self.__plugin.track_for_id, e) def candidates(self, *args, **kwargs): try: - yield from self._plugin.candidates(*args, **kwargs) + yield from self.__plugin.candidates(*args, **kwargs) except Exception as e: - return self._handle_exception(self._plugin.candidates, e) + return self.__handle_exception(self.__plugin.candidates, e) def item_candidates(self, *args, **kwargs): try: - yield from self._plugin.item_candidates(*args, **kwargs) + yield from self.__plugin.item_candidates(*args, **kwargs) except Exception as e: - return self._handle_exception(self._plugin.item_candidates, e) + return self.__handle_exception(self.__plugin.item_candidates, e) From 8e0b3f1323a1ab24d692e0727e70f912916aec8c Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Mon, 10 Nov 2025 18:33:52 +0100 Subject: [PATCH 5/7] Moved config check into find_metadata_source_plugins func. --- beets/metadata_plugins.py | 14 +++++++++----- docs/changelog.rst | 4 ++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/beets/metadata_plugins.py b/beets/metadata_plugins.py index 05e5c5d41..8c3b438b0 100644 --- a/beets/metadata_plugins.py +++ b/beets/metadata_plugins.py @@ -45,7 +45,13 @@ log = logging.getLogger("beets") def find_metadata_source_plugins() -> list[MetadataSourcePlugin]: """Return a list of all loaded metadata source plugins.""" # TODO: Make this an isinstance(MetadataSourcePlugin, ...) check in v3.0.0 - return [SafeProxy(p) for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc,arg-type] + # This should also allow us to remove the type: ignore comments below. + metadata_plugins = [p for p in find_plugins() if hasattr(p, "data_source")] + + if config["raise_on_error"].get(bool): + return metadata_plugins # type: ignore[return-value] + else: + return list(map(SafeProxy, metadata_plugins)) # type: ignore[arg-type] @notify_info_yielded("albuminfo_received") @@ -418,8 +424,6 @@ class SafeProxy(base): def __handle_exception(self, func: Callable[P, R], e: Exception) -> None: """Helper function to log exceptions from metadata source plugins.""" - if config["raise_on_error"].get(bool): - raise e log.error( "Error in '{}.{}': {}", self.__plugin.data_source, @@ -434,9 +438,9 @@ class SafeProxy(base): except Exception as e: return self.__handle_exception(self.__plugin.album_for_id, e) - def track_for_id(self, track_id: str): + def track_for_id(self, *args, **kwargs): try: - return self.__plugin.track_for_id(track_id) + return self.__plugin.track_for_id(*args, **kwargs) except Exception as e: return self.__handle_exception(self.__plugin.track_for_id, e) diff --git a/docs/changelog.rst b/docs/changelog.rst index df886700b..5529bf940 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -111,6 +111,10 @@ Bug fixes: avoiding extra lossy duplicates. - :doc:`plugins/discogs`: Fixed unexpected flex attr from the Discogs plugin. :bug:`6177` +- Errors in metadata plugins during autotage process will now be logged but + won't crash beets anymore. If you want to raise exceptions instead, set the + new configuration option ``raise_on_error`` to ``yes`` :bug:`5903`, + :bug:`4789`. For plugin developers: From cb6ad89ce65662a6daa9499aa9a8d0c6cf241836 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Fri, 30 Jan 2026 22:28:52 +0000 Subject: [PATCH 6/7] Use a decorator-based approach --- beets/metadata_plugins.py | 187 +++++++++++----------------------- beets/plugins.py | 11 +- test/test_metadata_plugins.py | 17 ++-- 3 files changed, 76 insertions(+), 139 deletions(-) diff --git a/beets/metadata_plugins.py b/beets/metadata_plugins.py index 8c3b438b0..7c08d72a3 100644 --- a/beets/metadata_plugins.py +++ b/beets/metadata_plugins.py @@ -9,33 +9,26 @@ from __future__ import annotations import abc import re -from functools import cache, cached_property -from typing import ( - TYPE_CHECKING, - Callable, - Generic, - Literal, - TypedDict, - TypeVar, -) +from contextlib import contextmanager, nullcontext +from functools import cache, cached_property, wraps +from typing import TYPE_CHECKING, Generic, Literal, TypedDict, TypeVar import unidecode from confuse import NotFoundError -from typing_extensions import NotRequired, ParamSpec +from typing_extensions import NotRequired from beets import config, logging from beets.util import cached_classproperty from beets.util.id_extractors import extract_release_id -from .plugins import BeetsPlugin, find_plugins, notify_info_yielded, send +from .plugins import BeetsPlugin, find_plugins, notify_info_yielded if TYPE_CHECKING: - from collections.abc import Iterable, Sequence + from collections.abc import Callable, Iterable, Iterator, Sequence from .autotag.hooks import AlbumInfo, Item, TrackInfo - P = ParamSpec("P") - R = TypeVar("R") + Ret = TypeVar("Ret") # Global logger. log = logging.getLogger("beets") @@ -46,52 +39,68 @@ def find_metadata_source_plugins() -> list[MetadataSourcePlugin]: """Return a list of all loaded metadata source plugins.""" # TODO: Make this an isinstance(MetadataSourcePlugin, ...) check in v3.0.0 # This should also allow us to remove the type: ignore comments below. - metadata_plugins = [p for p in find_plugins() if hasattr(p, "data_source")] + return [p for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc] - if config["raise_on_error"].get(bool): - return metadata_plugins # type: ignore[return-value] - else: - return list(map(SafeProxy, metadata_plugins)) # type: ignore[arg-type] + +@contextmanager +def handle_plugin_error(plugin: MetadataSourcePlugin, method_name: str): + """Safely call a plugin method, catching and logging exceptions.""" + try: + yield + except Exception as e: + log.error("Error in '{}.{}': {}", plugin.data_source, method_name, e) + log.debug("Exception details:", exc_info=True) + + +def _yield_from_plugins( + func: Callable[..., Iterable[Ret]], +) -> Callable[..., Iterator[Ret]]: + method_name = func.__name__ + + @wraps(func) + def wrapper(*args, **kwargs) -> Iterator[Ret]: + for plugin in find_metadata_source_plugins(): + method = getattr(plugin, method_name) + with ( + nullcontext() + if config["raise_on_error"] + else handle_plugin_error(plugin, method_name) + ): + yield from filter(None, method(*args, **kwargs)) + + return wrapper @notify_info_yielded("albuminfo_received") -def candidates(*args, **kwargs) -> Iterable[AlbumInfo]: - """Return matching album candidates from all metadata source plugins.""" - for plugin in find_metadata_source_plugins(): - yield from plugin.candidates(*args, **kwargs) +@_yield_from_plugins +def candidates(*args, **kwargs) -> Iterator[AlbumInfo]: + yield from () @notify_info_yielded("trackinfo_received") -def item_candidates(*args, **kwargs) -> Iterable[TrackInfo]: - """Return matching track candidates from all metadata source plugins.""" - for plugin in find_metadata_source_plugins(): - yield from plugin.item_candidates(*args, **kwargs) +@_yield_from_plugins +def item_candidates(*args, **kwargs) -> Iterator[TrackInfo]: + yield from () + + +@notify_info_yielded("albuminfo_received") +@_yield_from_plugins +def albums_for_ids(*args, **kwargs) -> Iterator[AlbumInfo]: + yield from () + + +@notify_info_yielded("trackinfo_received") +@_yield_from_plugins +def tracks_for_ids(*args, **kwargs) -> Iterator[TrackInfo]: + yield from () def album_for_id(_id: str) -> AlbumInfo | None: - """Get AlbumInfo object for the given ID string. - - A single ID can yield just a single album, so we return the first match. - """ - for plugin in find_metadata_source_plugins(): - if info := plugin.album_for_id(_id): - send("albuminfo_received", info=info) - return info - - return None + return next(albums_for_ids([_id]), None) def track_for_id(_id: str) -> TrackInfo | None: - """Get TrackInfo object for the given ID string. - - A single ID can yield just a single track, so we return the first match. - """ - for plugin in find_metadata_source_plugins(): - if info := plugin.track_for_id(_id): - send("trackinfo_received", info=info) - return info - - return None + return next(tracks_for_ids([_id]), None) @cache @@ -279,11 +288,11 @@ class SearchFilter(TypedDict): album: NotRequired[str] -Res = TypeVar("Res", bound=IDResponse) +R = TypeVar("R", bound=IDResponse) class SearchApiMetadataSourcePlugin( - Generic[Res], MetadataSourcePlugin, metaclass=abc.ABCMeta + Generic[R], MetadataSourcePlugin, metaclass=abc.ABCMeta ): """Helper class to implement a metadata source plugin with an API. @@ -308,7 +317,7 @@ class SearchApiMetadataSourcePlugin( query_type: Literal["album", "track"], filters: SearchFilter, query_string: str = "", - ) -> Sequence[Res]: + ) -> Sequence[R]: """Perform a search on the API. :param query_type: The type of query to perform. @@ -377,81 +386,3 @@ class SearchApiMetadataSourcePlugin( query = unidecode.unidecode(query) return query - - -# To have proper typing for the proxy class below, we need to -# trick mypy into thinking that SafeProxy is a subclass of -# MetadataSourcePlugin. -# https://stackoverflow.com/questions/71365594/how-to-make-a-proxy-object-with-typing-as-underlying-object-in-python -Proxied = TypeVar("Proxied", bound=MetadataSourcePlugin) -if TYPE_CHECKING: - base = MetadataSourcePlugin -else: - base = object - - -class SafeProxy(base): - """A proxy class that forwards all attribute access to the wrapped - MetadataSourcePlugin instance. - - We use this to catch and log exceptions from metadata source plugins - without crashing beets. E.g. on long running autotag operations. - """ - - __plugin: MetadataSourcePlugin - - def __init__(self, plugin: MetadataSourcePlugin): - self.__plugin = plugin - - def __getattribute__(self, name): - if name in { - "_SafeProxy__plugin", - "_SafeProxy__handle_exception", - "candidates", - "item_candidates", - "album_for_id", - "track_for_id", - }: - return super().__getattribute__(name) - else: - return getattr(self.__plugin, name) - - def __setattr__(self, name, value): - if name == "_SafeProxy__plugin": - super().__setattr__(name, value) - else: - self.__plugin.__setattr__(name, value) - - def __handle_exception(self, func: Callable[P, R], e: Exception) -> None: - """Helper function to log exceptions from metadata source plugins.""" - log.error( - "Error in '{}.{}': {}", - self.__plugin.data_source, - func.__name__, - e, - ) - log.debug("Exception details:", exc_info=True) - - def album_for_id(self, *args, **kwargs): - try: - return self.__plugin.album_for_id(*args, **kwargs) - except Exception as e: - return self.__handle_exception(self.__plugin.album_for_id, e) - - def track_for_id(self, *args, **kwargs): - try: - return self.__plugin.track_for_id(*args, **kwargs) - except Exception as e: - return self.__handle_exception(self.__plugin.track_for_id, e) - - def candidates(self, *args, **kwargs): - try: - yield from self.__plugin.candidates(*args, **kwargs) - except Exception as e: - return self.__handle_exception(self.__plugin.candidates, e) - - def item_candidates(self, *args, **kwargs): - try: - yield from self.__plugin.item_candidates(*args, **kwargs) - except Exception as e: - return self.__handle_exception(self.__plugin.item_candidates, e) diff --git a/beets/plugins.py b/beets/plugins.py index ec3f999c4..01d9d3327 100644 --- a/beets/plugins.py +++ b/beets/plugins.py @@ -35,7 +35,7 @@ from beets.util import unique_list from beets.util.deprecation import deprecate_for_maintainers, deprecate_for_user if TYPE_CHECKING: - from collections.abc import Callable, Iterable, Sequence + from collections.abc import Callable, Iterable, Iterator, Sequence from confuse import ConfigView @@ -58,7 +58,6 @@ if TYPE_CHECKING: P = ParamSpec("P") Ret = TypeVar("Ret", bound=Any) Listener = Callable[..., Any] - IterF = Callable[P, Iterable[Ret]] PLUGIN_NAMESPACE = "beetsplug" @@ -548,7 +547,7 @@ def named_queries(model_cls: type[AnyModel]) -> dict[str, FieldQueryType]: def notify_info_yielded( event: EventType, -) -> Callable[[IterF[P, Ret]], IterF[P, Ret]]: +) -> Callable[[Callable[P, Iterable[Ret]]], Callable[P, Iterator[Ret]]]: """Makes a generator send the event 'event' every time it yields. This decorator is supposed to decorate a generator, but any function returning an iterable should work. @@ -556,9 +555,11 @@ def notify_info_yielded( 'send'. """ - def decorator(func: IterF[P, Ret]) -> IterF[P, Ret]: + def decorator( + func: Callable[P, Iterable[Ret]], + ) -> Callable[P, Iterator[Ret]]: @wraps(func) - def wrapper(*args: P.args, **kwargs: P.kwargs) -> Iterable[Ret]: + def wrapper(*args: P.args, **kwargs: P.kwargs) -> Iterator[Ret]: for v in func(*args, **kwargs): send(event, info=v) yield v diff --git a/test/test_metadata_plugins.py b/test/test_metadata_plugins.py index edf66adcd..d34185330 100644 --- a/test/test_metadata_plugins.py +++ b/test/test_metadata_plugins.py @@ -45,18 +45,23 @@ class TestMetadataPluginsException(PluginMixin): self.unload_plugins() @pytest.mark.parametrize( - "method_name,args", + "method_name,error_method_name,args", [ - ("candidates", ()), - ("item_candidates", ()), - ("album_for_id", ("some_id",)), - ("track_for_id", ("some_id",)), + ("candidates", "candidates", ()), + ("item_candidates", "item_candidates", ()), + ("albums_for_ids", "albums_for_ids", (["some_id"],)), + ("tracks_for_ids", "tracks_for_ids", (["some_id"],)), + # Currently, singular methods call plural ones internally and log + # errors from there + ("album_for_id", "albums_for_ids", ("some_id",)), + ("track_for_id", "tracks_for_ids", ("some_id",)), ], ) def test_logging( self, caplog, method_name, + error_method_name, args, ): self.config["raise_on_error"] = False @@ -72,7 +77,7 @@ class TestMetadataPluginsException(PluginMixin): for msg in logs: assert ( msg - == f"Error in 'ErrorMetadataMockPlugin.{method_name}': Mocked error" + == f"Error in 'ErrorMetadataMockPlugin.{error_method_name}': Mocked error" # noqa: E501 ) caplog.clear() From 2196bd89de364d57b8f473fbf6e7694129c5cbac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Fri, 30 Jan 2026 22:36:54 +0000 Subject: [PATCH 7/7] Simplify tests --- test/test_metadata_plugins.py | 56 +++++++++++------------------------ 1 file changed, 18 insertions(+), 38 deletions(-) diff --git a/test/test_metadata_plugins.py b/test/test_metadata_plugins.py index d34185330..684784191 100644 --- a/test/test_metadata_plugins.py +++ b/test/test_metadata_plugins.py @@ -1,4 +1,4 @@ -from typing import Iterable +from collections.abc import Iterable import pytest @@ -9,8 +9,6 @@ from beets.test.helper import PluginMixin class ErrorMetadataMockPlugin(metadata_plugins.MetadataSourcePlugin): """A metadata source plugin that raises errors in all its methods.""" - data_source = "ErrorMetadataMockPlugin" - def candidates(self, *args, **kwargs): raise ValueError("Mocked error") @@ -25,12 +23,6 @@ class ErrorMetadataMockPlugin(metadata_plugins.MetadataSourcePlugin): def track_for_id(self, *args, **kwargs): raise ValueError("Mocked error") - def track_distance(self, *args, **kwargs): - raise ValueError("Mocked error") - - def album_distance(self, *args, **kwargs): - raise ValueError("Mocked error") - class TestMetadataPluginsException(PluginMixin): """Check that errors during the metadata plugins do not crash beets. @@ -44,6 +36,14 @@ class TestMetadataPluginsException(PluginMixin): yield self.unload_plugins() + @pytest.fixture + def call_method(self, method_name, args): + def _call(): + result = getattr(metadata_plugins, method_name)(*args) + return list(result) if isinstance(result, Iterable) else result + + return _call + @pytest.mark.parametrize( "method_name,error_method_name,args", [ @@ -57,30 +57,15 @@ class TestMetadataPluginsException(PluginMixin): ("track_for_id", "tracks_for_ids", ("some_id",)), ], ) - def test_logging( - self, - caplog, - method_name, - error_method_name, - args, - ): + def test_logging(self, caplog, call_method, error_method_name): self.config["raise_on_error"] = False - with caplog.at_level("ERROR"): - # Call the method to trigger the error - ret = getattr(metadata_plugins, method_name)(*args) - if isinstance(ret, Iterable): - list(ret) - # Check that an error was logged - assert len(caplog.records) >= 1 - logs = [record.getMessage() for record in caplog.records] - for msg in logs: - assert ( - msg - == f"Error in 'ErrorMetadataMockPlugin.{error_method_name}': Mocked error" # noqa: E501 - ) + call_method() - caplog.clear() + assert ( + f"Error in 'ErrorMetadataMock.{error_method_name}': Mocked error" + in caplog.text + ) @pytest.mark.parametrize( "method_name,args", @@ -91,13 +76,8 @@ class TestMetadataPluginsException(PluginMixin): ("track_for_id", ("some_id",)), ], ) - def test_raising( - self, - method_name, - args, - ): + def test_raising(self, call_method): self.config["raise_on_error"] = True + with pytest.raises(ValueError, match="Mocked error"): - getattr(metadata_plugins, method_name)(*args) if not isinstance( - args, Iterable - ) else list(getattr(metadata_plugins, method_name)(*args)) + call_method()