From 3be4a89aeeaf965c4f9ced05cf062ba12f125078 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Tue, 15 Jul 2025 11:58:25 +0100 Subject: [PATCH] refactor: convert _types from class attributes to cached properties Convert static _types dictionaries to dynamic cached class properties to enable proper plugin type inheritance and avoid mutating shared state. Key changes: - Replace static _types dicts with @cached_classproperty decorators - Update cached_classproperty to support proper caching with class names - Remove manual _types mutation in plugin loading/unloading - Add pluginload event and cache clearing for proper plugin integration - Fix test to trigger type checking during item creation This ensures plugin types are properly inherited through the class hierarchy and eliminates issues with shared mutable state between test runs. --- beets/dbcore/db.py | 7 ++--- beets/library/models.py | 19 +++++++------ beets/plugins.py | 2 +- beets/test/helper.py | 12 +++------ beets/ui/__init__.py | 9 +------ beets/util/__init__.py | 45 ++++++++++++++++++++++++------- beetsplug/advancedrewrite.py | 2 ++ test/plugins/test_types_plugin.py | 2 +- 8 files changed, 60 insertions(+), 38 deletions(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index 16ca54995..b1c9e18d4 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -289,9 +289,10 @@ 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 diff --git a/beets/library/models.py b/beets/library/models.py index 68c80b934..8de1c2982 100644 --- a/beets/library/models.py +++ b/beets/library/models.py @@ -41,6 +41,14 @@ 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 writable_media_fields(cls) -> set[str]: return set(MediaFile.fields()) & cls._fields.keys() @@ -265,10 +273,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 +722,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 diff --git a/beets/plugins.py b/beets/plugins.py index 821a96152..9893633fb 100644 --- a/beets/plugins.py +++ b/beets/plugins.py @@ -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(): diff --git a/beets/test/helper.py b/beets/test/helper.py index 4f26e8448..a1d741b16 100644 --- a/beets/test/helper.py +++ b/beets/test/helper.py @@ -58,6 +58,7 @@ from beets.ui.commands import TerminalImportSession from beets.util import ( MoveOperation, bytestring_path, + cached_classproperty, clean_module_tempdir, syspath, ) @@ -471,8 +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) @@ -494,13 +493,12 @@ 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)) + # Take a backup of the original _queries to restore when unloading. Item._queries.update(beets.plugins.named_queries(Item)) Album._queries.update(beets.plugins.named_queries(Album)) @@ -512,8 +510,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 diff --git a/beets/ui/__init__.py b/beets/ui/__init__.py index 74dee550c..85fdda254 100644 --- a/beets/ui/__init__.py +++ b/beets/ui/__init__.py @@ -1609,14 +1609,7 @@ 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 - + # Add queries defined by plugins. library.Item._queries.update(plugins.named_queries(library.Item)) library.Album._queries.update(plugins.named_queries(library.Album)) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index a4b6ef3d6..58b08c844 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -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]): diff --git a/beetsplug/advancedrewrite.py b/beetsplug/advancedrewrite.py index 9a5feaaff..8bc63c0cb 100644 --- a/beetsplug/advancedrewrite.py +++ b/beetsplug/advancedrewrite.py @@ -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( [ diff --git a/test/plugins/test_types_plugin.py b/test/plugins/test_types_plugin.py index b41e9bb18..41807b80d 100644 --- a/test/plugins/test_types_plugin.py +++ b/test/plugins/test_types_plugin.py @@ -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