mirror of
https://github.com/beetbox/beets.git
synced 2026-01-09 01:15:38 +01:00
Register types and queries from plugins on models dynamically (#5872)
## Fix dynamic plugin type and query registration This PR refactors the plugin system to properly handle dynamic type and query registration by converting static class attributes to cached class properties. **Problem**: Plugin types and queries were stored as mutable class attributes that were manually updated during plugin loading/unloading. This caused issues where: - Plugin types weren't properly registered in test environments - Shared mutable state between test runs caused inconsistent behavior - Manual cleanup was error-prone and incomplete See https://github.com/beetbox/beets/pull/5833 and specifically https://github.com/beetbox/beets/pull/5833#issuecomment-3016635209 for the context. **Solution**: - Convert `_types` and `_queries` from static dictionaries to `@cached_classproperty` decorators - Types and queries are now dynamically computed from loaded plugins when accessed - Eliminates manual mutation of class attributes during plugin loading - Properly clears cached properties when plugins are loaded/unloaded - Ensures plugin types are available immediately after registration **Key Changes**: - `Model._types` and `LibModel._queries` now use `@cached_classproperty` - Removed manual `_types.update()` calls in plugin loading code - Added proper cache clearing in test infrastructure - Plugin types are now inherited through the class hierarchy correctly This fixes the developer's issue where `item_types` weren't being registered properly in tests - the new dynamic property approach ensures plugin types are available as soon as plugins are loaded.
This commit is contained in:
commit
bde5de42b3
8 changed files with 78 additions and 61 deletions
|
|
@ -289,19 +289,22 @@ class Model(ABC, Generic[D]):
|
|||
terms.
|
||||
"""
|
||||
|
||||
_types: dict[str, types.Type] = {}
|
||||
"""Optional Types for non-fixed (i.e., flexible and computed) fields.
|
||||
"""
|
||||
@cached_classproperty
|
||||
def _types(cls) -> dict[str, types.Type]:
|
||||
"""Optional types for non-fixed (flexible and computed) fields."""
|
||||
return {}
|
||||
|
||||
_sorts: dict[str, type[FieldSort]] = {}
|
||||
"""Optional named sort criteria. The keys are strings and the values
|
||||
are subclasses of `Sort`.
|
||||
"""
|
||||
|
||||
_queries: dict[str, FieldQueryType] = {}
|
||||
"""Named queries that use a field-like `name:value` syntax but which
|
||||
do not relate to any specific field.
|
||||
"""
|
||||
@cached_classproperty
|
||||
def _queries(cls) -> dict[str, FieldQueryType]:
|
||||
"""Named queries that use a field-like `name:value` syntax but which
|
||||
do not relate to any specific field.
|
||||
"""
|
||||
return {}
|
||||
|
||||
_always_dirty = False
|
||||
"""By default, fields only become "dirty" when their value actually
|
||||
|
|
|
|||
|
|
@ -41,6 +41,18 @@ class LibModel(dbcore.Model["Library"]):
|
|||
_format_config_key: str
|
||||
path: bytes
|
||||
|
||||
@cached_classproperty
|
||||
def _types(cls) -> dict[str, types.Type]:
|
||||
"""Return the types of the fields in this model."""
|
||||
return {
|
||||
**plugins.types(cls), # type: ignore[arg-type]
|
||||
"data_source": types.STRING,
|
||||
}
|
||||
|
||||
@cached_classproperty
|
||||
def _queries(cls) -> dict[str, FieldQueryType]:
|
||||
return plugins.named_queries(cls) # type: ignore[arg-type]
|
||||
|
||||
@cached_classproperty
|
||||
def writable_media_fields(cls) -> set[str]:
|
||||
return set(MediaFile.fields()) & cls._fields.keys()
|
||||
|
|
@ -265,10 +277,9 @@ class Album(LibModel):
|
|||
|
||||
_search_fields = ("album", "albumartist", "genre")
|
||||
|
||||
_types = {
|
||||
"path": types.PathType(),
|
||||
"data_source": types.STRING,
|
||||
}
|
||||
@cached_classproperty
|
||||
def _types(cls) -> dict[str, types.Type]:
|
||||
return {**super()._types, "path": types.PathType()}
|
||||
|
||||
_sorts = {
|
||||
"albumartist": dbcore.query.SmartArtistSort,
|
||||
|
|
@ -715,10 +726,6 @@ class Item(LibModel):
|
|||
"genre",
|
||||
)
|
||||
|
||||
_types = {
|
||||
"data_source": types.STRING,
|
||||
}
|
||||
|
||||
# Set of item fields that are backed by `MediaFile` fields.
|
||||
# Any kind of field (fixed, flexible, and computed) may be a media
|
||||
# field. Only these fields are read from disk in `read` and written in
|
||||
|
|
@ -737,7 +744,9 @@ class Item(LibModel):
|
|||
|
||||
_sorts = {"artist": dbcore.query.SmartArtistSort}
|
||||
|
||||
_queries = {"singleton": dbcore.query.SingletonQuery}
|
||||
@cached_classproperty
|
||||
def _queries(cls) -> dict[str, FieldQueryType]:
|
||||
return {**super()._queries, "singleton": dbcore.query.SingletonQuery}
|
||||
|
||||
_format_config_key = "format_item"
|
||||
|
||||
|
|
|
|||
|
|
@ -362,7 +362,7 @@ def queries() -> dict[str, type[Query]]:
|
|||
|
||||
|
||||
def types(model_cls: type[AnyModel]) -> dict[str, Type]:
|
||||
# Gives us `item_types` and `album_types`
|
||||
"""Return mapping between flex field names and types for the given model."""
|
||||
attr_name = f"{model_cls.__name__.lower()}_types"
|
||||
types: dict[str, Type] = {}
|
||||
for plugin in find_plugins():
|
||||
|
|
@ -379,13 +379,13 @@ def types(model_cls: type[AnyModel]) -> dict[str, Type]:
|
|||
|
||||
|
||||
def named_queries(model_cls: type[AnyModel]) -> dict[str, FieldQueryType]:
|
||||
# Gather `item_queries` and `album_queries` from the plugins.
|
||||
"""Return mapping between field names and queries for the given model."""
|
||||
attr_name = f"{model_cls.__name__.lower()}_queries"
|
||||
queries: dict[str, FieldQueryType] = {}
|
||||
for plugin in find_plugins():
|
||||
plugin_queries = getattr(plugin, attr_name, {})
|
||||
queries.update(plugin_queries)
|
||||
return queries
|
||||
return {
|
||||
field: query
|
||||
for plugin in find_plugins()
|
||||
for field, query in getattr(plugin, attr_name, {}).items()
|
||||
}
|
||||
|
||||
|
||||
def notify_info_yielded(event: str) -> Callable[[IterF[P, Ret]], IterF[P, Ret]]:
|
||||
|
|
|
|||
|
|
@ -52,12 +52,13 @@ import beets.plugins
|
|||
from beets import importer, logging, util
|
||||
from beets.autotag.hooks import AlbumInfo, TrackInfo
|
||||
from beets.importer import ImportSession
|
||||
from beets.library import Album, Item, Library
|
||||
from beets.library import Item, Library
|
||||
from beets.test import _common
|
||||
from beets.ui.commands import TerminalImportSession
|
||||
from beets.util import (
|
||||
MoveOperation,
|
||||
bytestring_path,
|
||||
cached_classproperty,
|
||||
clean_module_tempdir,
|
||||
syspath,
|
||||
)
|
||||
|
|
@ -471,11 +472,6 @@ class PluginMixin(ConfigMixin):
|
|||
plugin: ClassVar[str]
|
||||
preload_plugin: ClassVar[bool] = True
|
||||
|
||||
original_item_types = dict(Item._types)
|
||||
original_album_types = dict(Album._types)
|
||||
original_item_queries = dict(Item._queries)
|
||||
original_album_queries = dict(Album._queries)
|
||||
|
||||
def setup_beets(self):
|
||||
super().setup_beets()
|
||||
if self.preload_plugin:
|
||||
|
|
@ -494,16 +490,11 @@ class PluginMixin(ConfigMixin):
|
|||
# FIXME this should eventually be handled by a plugin manager
|
||||
plugins = (self.plugin,) if hasattr(self, "plugin") else plugins
|
||||
self.config["plugins"] = plugins
|
||||
cached_classproperty.cache.clear()
|
||||
beets.plugins.load_plugins(plugins)
|
||||
beets.plugins.send("pluginload")
|
||||
beets.plugins.find_plugins()
|
||||
|
||||
# Take a backup of the original _types and _queries to restore
|
||||
# when unloading.
|
||||
Item._types.update(beets.plugins.types(Item))
|
||||
Album._types.update(beets.plugins.types(Album))
|
||||
Item._queries.update(beets.plugins.named_queries(Item))
|
||||
Album._queries.update(beets.plugins.named_queries(Album))
|
||||
|
||||
def unload_plugins(self) -> None:
|
||||
"""Unload all plugins and remove them from the configuration."""
|
||||
# FIXME this should eventually be handled by a plugin manager
|
||||
|
|
@ -512,10 +503,6 @@ class PluginMixin(ConfigMixin):
|
|||
self.config["plugins"] = []
|
||||
beets.plugins._classes = set()
|
||||
beets.plugins._instances = {}
|
||||
Item._types = self.original_item_types
|
||||
Album._types = self.original_album_types
|
||||
Item._queries = self.original_item_queries
|
||||
Album._queries = self.original_album_queries
|
||||
|
||||
@contextmanager
|
||||
def configure_plugin(self, config: Any):
|
||||
|
|
|
|||
|
|
@ -1609,17 +1609,6 @@ def _setup(options, lib=None):
|
|||
|
||||
plugins = _load_plugins(options, config)
|
||||
|
||||
# Add types and queries defined by plugins.
|
||||
plugin_types_album = plugins.types(library.Album)
|
||||
library.Album._types.update(plugin_types_album)
|
||||
item_types = plugin_types_album.copy()
|
||||
item_types.update(library.Item._types)
|
||||
item_types.update(plugins.types(library.Item))
|
||||
library.Item._types = item_types
|
||||
|
||||
library.Item._queries.update(plugins.named_queries(library.Item))
|
||||
library.Album._queries.update(plugins.named_queries(library.Album))
|
||||
|
||||
plugins.send("pluginload")
|
||||
|
||||
# Get the default subcommands.
|
||||
|
|
|
|||
|
|
@ -41,6 +41,7 @@ from typing import (
|
|||
Any,
|
||||
AnyStr,
|
||||
Callable,
|
||||
ClassVar,
|
||||
Generic,
|
||||
NamedTuple,
|
||||
TypeVar,
|
||||
|
|
@ -1052,20 +1053,46 @@ def par_map(transform: Callable[[T], Any], items: Sequence[T]) -> None:
|
|||
|
||||
|
||||
class cached_classproperty:
|
||||
"""A decorator implementing a read-only property that is *lazy* in
|
||||
the sense that the getter is only invoked once. Subsequent accesses
|
||||
through *any* instance use the cached result.
|
||||
"""Descriptor implementing cached class properties.
|
||||
|
||||
Provides class-level dynamic property behavior where the getter function is
|
||||
called once per class and the result is cached for subsequent access. Unlike
|
||||
instance properties, this operates on the class rather than instances.
|
||||
"""
|
||||
|
||||
def __init__(self, getter):
|
||||
cache: ClassVar[dict[tuple[Any, str], Any]] = {}
|
||||
|
||||
name: str
|
||||
|
||||
# Ideally, we would like to use `Callable[[type[T]], Any]` here,
|
||||
# however, `mypy` is unable to see this as a **class** property, and thinks
|
||||
# that this callable receives an **instance** of the object, failing the
|
||||
# type check, for example:
|
||||
# >>> class Album:
|
||||
# >>> @cached_classproperty
|
||||
# >>> def foo(cls):
|
||||
# >>> reveal_type(cls) # mypy: revealed type is "Album"
|
||||
# >>> return cls.bar
|
||||
#
|
||||
# Argument 1 to "cached_classproperty" has incompatible type
|
||||
# "Callable[[Album], ...]"; expected "Callable[[type[Album]], ...]"
|
||||
#
|
||||
# Therefore, we just use `Any` here, which is not ideal, but works.
|
||||
def __init__(self, getter: Callable[[Any], Any]) -> None:
|
||||
"""Initialize the descriptor with the property getter function."""
|
||||
self.getter = getter
|
||||
self.cache = {}
|
||||
|
||||
def __get__(self, instance, owner):
|
||||
if owner not in self.cache:
|
||||
self.cache[owner] = self.getter(owner)
|
||||
def __set_name__(self, owner: Any, name: str) -> None:
|
||||
"""Capture the attribute name this descriptor is assigned to."""
|
||||
self.name = name
|
||||
|
||||
return self.cache[owner]
|
||||
def __get__(self, instance: Any, owner: type[Any]) -> Any:
|
||||
"""Compute and cache if needed, and return the property value."""
|
||||
key = owner, self.name
|
||||
if key not in self.cache:
|
||||
self.cache[key] = self.getter(owner)
|
||||
|
||||
return self.cache[key]
|
||||
|
||||
|
||||
class LazySharedInstance(Generic[T]):
|
||||
|
|
|
|||
|
|
@ -58,7 +58,9 @@ class AdvancedRewritePlugin(BeetsPlugin):
|
|||
def __init__(self):
|
||||
"""Parse configuration and register template fields for rewriting."""
|
||||
super().__init__()
|
||||
self.register_listener("pluginload", self.loaded)
|
||||
|
||||
def loaded(self):
|
||||
template = confuse.Sequence(
|
||||
confuse.OneOf(
|
||||
[
|
||||
|
|
|
|||
|
|
@ -134,7 +134,7 @@ class TypesPluginTest(PluginTestCase):
|
|||
def test_unknown_type_error(self):
|
||||
self.config["types"] = {"flex": "unkown type"}
|
||||
with pytest.raises(ConfigValueError):
|
||||
self.run_command("ls")
|
||||
self.add_item(flex="test")
|
||||
|
||||
def test_template_if_def(self):
|
||||
# Tests for a subtle bug when using %ifdef in templates along with
|
||||
|
|
|
|||
Loading…
Reference in a new issue