From 24895d0ddc6154f02e711daa6dd480f40546464c Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 26 Sep 2024 20:39:13 -0700 Subject: [PATCH 1/6] Avoid triggering property methods when inspecting plugin attribute signatures --- src/pluggy/_manager.py | 14 ++++++++++++++ testing/test_pluginmanager.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index d778334b..a52044f0 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -181,6 +181,20 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None customize how hook implementation are picked up. By default, returns the options for items decorated with :class:`HookimplMarker`. """ + + # IMPORTANT: @property methods can have side effects, and are never hookimpl + # if attr is a property, skip it in advance + plugin_class = plugin if inspect.isclass(plugin) else type(plugin) + if isinstance(getattr(plugin_class, name, None), property): + return None + + # pydantic model fields are like attrs and also can never be hookimpls + plugin_is_pydantic_obj = hasattr(plugin, "__pydantic_core_schema__") + if plugin_is_pydantic_obj and name in getattr(plugin, "model_fields", {}): + # pydantic models mess with the class and attr __signature__ + # so inspect.isroutine(...) throws exceptions and cant be used + return None + method: object = getattr(plugin, name) if not inspect.isroutine(method): return None diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index c4ce08f3..67ea6789 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -4,6 +4,7 @@ import importlib.metadata from typing import Any +from typing import Dict from typing import List import pytest @@ -123,6 +124,36 @@ class A: assert pm.register(A(), "somename") +def test_register_skips_properties(he_pm: PluginManager) -> None: + class ClassWithProperties: + property_was_executed: bool = False + + @property + def some_func(self): + self.property_was_executed = True + return None + + test_plugin = ClassWithProperties() + he_pm.register(test_plugin) + assert not test_plugin.property_was_executed + + +def test_register_skips_pydantic_fields(he_pm: PluginManager) -> None: + class PydanticModelClass: + # stub to make object look like a pydantic model + model_fields: Dict[str, bool] = {"some_attr": True} + + def __pydantic_core_schema__(self): ... + + @hookimpl + def some_attr(self): ... + + test_plugin = PydanticModelClass() + he_pm.register(test_plugin) + with pytest.raises(AttributeError): + he_pm.hook.some_attr.get_hookimpls() + + def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl From aae3799a8efa9a8d5f24e74b3a69a807f16c41a3 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 26 Sep 2024 23:01:53 -0700 Subject: [PATCH 2/6] Add exception handler to deal with proxy object attrs as well --- src/pluggy/_manager.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index a52044f0..05407cfb 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -191,11 +191,16 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None # pydantic model fields are like attrs and also can never be hookimpls plugin_is_pydantic_obj = hasattr(plugin, "__pydantic_core_schema__") if plugin_is_pydantic_obj and name in getattr(plugin, "model_fields", {}): - # pydantic models mess with the class and attr __signature__ - # so inspect.isroutine(...) throws exceptions and cant be used return None - method: object = getattr(plugin, name) + method: object + try: + method = getattr(plugin, name) + except AttributeError: + # AttributeError: '__signature__' attribute of 'Plugin' is class-only + # can happen for some special objects (e.g. proxies, pydantic, etc.) + method = getattr(type(plugin), name) # use class sig instead + if not inspect.isroutine(method): return None try: From 90d9f80d74658d16cffee86c9f2a9e3651eb72db Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Tue, 1 Oct 2024 13:17:55 -0700 Subject: [PATCH 3/6] remove pydantic-specific logic and support hookspecs too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/astral-sh/ruff-pre-commit: v0.6.7 → v0.6.8](https://github.com/astral-sh/ruff-pre-commit/compare/v0.6.7...v0.6.8) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .pre-commit-config.yaml | 2 +- src/pluggy/_manager.py | 38 ++++++++++++++++++++++++----------- testing/test_pluginmanager.py | 23 +++------------------ 3 files changed, 30 insertions(+), 33 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 40f6141e..05c80cfc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: https://github.com/astral-sh/ruff-pre-commit - rev: "v0.6.7" + rev: "v0.6.8" hooks: - id: ruff args: ["--fix"] diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 05407cfb..e9905ccd 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -46,6 +46,17 @@ def _warn_for_function(warning: Warning, function: Callable[..., object]) -> Non ) +def _attr_is_property(obj: Any, name: str) -> bool: + """Check if a given attr is a @property on a module, class, or object""" + if inspect.ismodule(obj): + return False # modules can never have @property methods + + base_class = obj if inspect.isclass(obj) else type(obj) + if isinstance(getattr(base_class, name, None), property): + return True + return False + + class PluginValidationError(Exception): """Plugin failed validation. @@ -182,23 +193,16 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None options for items decorated with :class:`HookimplMarker`. """ - # IMPORTANT: @property methods can have side effects, and are never hookimpl - # if attr is a property, skip it in advance - plugin_class = plugin if inspect.isclass(plugin) else type(plugin) - if isinstance(getattr(plugin_class, name, None), property): - return None - - # pydantic model fields are like attrs and also can never be hookimpls - plugin_is_pydantic_obj = hasattr(plugin, "__pydantic_core_schema__") - if plugin_is_pydantic_obj and name in getattr(plugin, "model_fields", {}): + if _attr_is_property(plugin, name): + # @property methods can have side effects, and are never hookimpls return None method: object try: method = getattr(plugin, name) except AttributeError: - # AttributeError: '__signature__' attribute of 'Plugin' is class-only - # can happen for some special objects (e.g. proxies, pydantic, etc.) + # AttributeError: '__signature__' attribute of 'plugin' is class-only + # can happen if plugin is a proxy object wrapping a class/module method = getattr(type(plugin), name) # use class sig instead if not inspect.isroutine(method): @@ -305,7 +309,17 @@ def parse_hookspec_opts( customize how hook specifications are picked up. By default, returns the options for items decorated with :class:`HookspecMarker`. """ - method = getattr(module_or_class, name) + if _attr_is_property(module_or_class, name): + # @property methods can have side effects, and are never hookspecs + return None + + method: object + try: + method = getattr(module_or_class, name) + except AttributeError: + # AttributeError: '__signature__' attribute of is class-only + # can happen if module_or_class is a proxy obj wrapping a class/module + method = getattr(type(module_or_class), name) # use class sig instead opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) return opts diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 67ea6789..d412f7a5 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -4,7 +4,6 @@ import importlib.metadata from typing import Any -from typing import Dict from typing import List import pytest @@ -124,36 +123,20 @@ class A: assert pm.register(A(), "somename") -def test_register_skips_properties(he_pm: PluginManager) -> None: +def test_register_ignores_properties(he_pm: PluginManager) -> None: class ClassWithProperties: property_was_executed: bool = False @property def some_func(self): - self.property_was_executed = True - return None + self.property_was_executed = True # pragma: no cover + return None # pragma: no cover test_plugin = ClassWithProperties() he_pm.register(test_plugin) assert not test_plugin.property_was_executed -def test_register_skips_pydantic_fields(he_pm: PluginManager) -> None: - class PydanticModelClass: - # stub to make object look like a pydantic model - model_fields: Dict[str, bool] = {"some_attr": True} - - def __pydantic_core_schema__(self): ... - - @hookimpl - def some_attr(self): ... - - test_plugin = PydanticModelClass() - he_pm.register(test_plugin) - with pytest.raises(AttributeError): - he_pm.hook.some_attr.get_hookimpls() - - def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl From 2536c621ffb91a8499d6cb3cb8f83995aee2a86b Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Tue, 1 Oct 2024 13:41:35 -0700 Subject: [PATCH 4/6] explicitly test that properties are ignored on both classes and instances --- testing/test_pluginmanager.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index d412f7a5..8e21ccbe 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -132,6 +132,9 @@ def some_func(self): self.property_was_executed = True # pragma: no cover return None # pragma: no cover + # test registering it as a class + he_pm.register(ClassWithProperties) + # test registering it as an instance test_plugin = ClassWithProperties() he_pm.register(test_plugin) assert not test_plugin.property_was_executed From c0789a6b8e4d9f0f97dd55d0fba34c80476bd5e5 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 3 Oct 2024 10:49:43 -0400 Subject: [PATCH 5/6] swap _attr_is_property arg Any type to object type Co-authored-by: Ran Benita --- src/pluggy/_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index e9905ccd..9064b521 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -46,7 +46,7 @@ def _warn_for_function(warning: Warning, function: Callable[..., object]) -> Non ) -def _attr_is_property(obj: Any, name: str) -> bool: +def _attr_is_property(obj: object, name: str) -> bool: """Check if a given attr is a @property on a module, class, or object""" if inspect.ismodule(obj): return False # modules can never have @property methods From bfe9f915d89000f25bdc6640b3c3dd877f784c32 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 24 Oct 2024 17:44:31 -0400 Subject: [PATCH 6/6] Return None instead of falling back to getattr(type(plugin)) --- src/pluggy/_manager.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 9064b521..b6d6bdc1 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -202,8 +202,9 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None method = getattr(plugin, name) except AttributeError: # AttributeError: '__signature__' attribute of 'plugin' is class-only - # can happen if plugin is a proxy object wrapping a class/module - method = getattr(type(plugin), name) # use class sig instead + # can be raised when trying to access some descriptor/proxied fields + # https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 + return None if not inspect.isroutine(method): return None @@ -318,8 +319,9 @@ def parse_hookspec_opts( method = getattr(module_or_class, name) except AttributeError: # AttributeError: '__signature__' attribute of is class-only - # can happen if module_or_class is a proxy obj wrapping a class/module - method = getattr(type(module_or_class), name) # use class sig instead + # can be raised when trying to access some descriptor/proxied fields + # https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 + return None opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) return opts