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 f42e8f690..7c08d72a3 100644 --- a/beets/metadata_plugins.py +++ b/beets/metadata_plugins.py @@ -9,69 +9,98 @@ from __future__ import annotations import abc import re -from functools import cache, cached_property +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 +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 + Ret = TypeVar("Ret") + +# 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 + # This should also allow us to remove the type: ignore comments below. return [p for p in find_plugins() if hasattr(p, "data_source")] # type: ignore[misc] +@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 fromm 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(album_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 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/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: diff --git a/test/test_metadata_plugins.py b/test/test_metadata_plugins.py new file mode 100644 index 000000000..684784191 --- /dev/null +++ b/test/test_metadata_plugins.py @@ -0,0 +1,83 @@ +from collections.abc 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.""" + + 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") + + +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): + metadata_plugins.find_metadata_source_plugins.cache_clear() + self.register_plugin(ErrorMetadataMockPlugin) + 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", + [ + ("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, call_method, error_method_name): + self.config["raise_on_error"] = False + + call_method() + + assert ( + f"Error in 'ErrorMetadataMock.{error_method_name}': Mocked error" + in caplog.text + ) + + @pytest.mark.parametrize( + "method_name,args", + [ + ("candidates", ()), + ("item_candidates", ()), + ("album_for_id", ("some_id",)), + ("track_for_id", ("some_id",)), + ], + ) + def test_raising(self, call_method): + self.config["raise_on_error"] = True + + with pytest.raises(ValueError, match="Mocked error"): + call_method()