Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid triggering @property methods on plugins when looking for hookimpls during registration #536

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

pirate
Copy link

@pirate pirate commented Sep 27, 2024

Hey all, thanks for making pluggy! I'm loving using it so far. 🥳


Currently pluggy fairly aggressively inspects every plugin's attrs upon registration in order to look for potential @hookimpl-marked methods.

Based on the existing implementation in PluginManager.register() -> ... -> parse_hookimpl_opts(), it's clear that the intention is to only look for Callable methods marked with the @hookimpl decorator (it skips over any non-methods / undecorated-methods it finds in the process).

If a plugin is class-based, the current implementation using inspect.isroutine(getattr(plugin, name)) has the unintended consequence of evaluating every single @property method on the passed plugin object, which can have side effects because property methods can execute arbitrary code upon access!

This PR corrects this by pre-checking if a given plugin attr is a @property (or pydantic field), before attempting to getattr(plugin, name) to pass it to inspect.isroutine(...).

@pirate
Copy link
Author

pirate commented Sep 27, 2024

Codecov is complaining about whitespace lines and comment lines not being covered?

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, wondering if we should take static getattr from pytest for fetching declarations

src/pluggy/_manager.py Outdated Show resolved Hide resolved
src/pluggy/_manager.py Outdated Show resolved Hide resolved
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.6.7 → v0.6.8](astral-sh/ruff-pre-commit@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
@pirate
Copy link
Author

pirate commented Oct 1, 2024

Ok I've updated the PR:

  • Removed pydantic-specific logic
  • Abstracted @property check to helper function _attr_is_property(obj, name) -> bool
  • Exhaustively tested the following situations:
    • proper handling of: modules, class, objects, Pydantic models, Django models being used as namespaces
    • namespaces containing: @property, @classproperty, @classmethod, @staticmethod, ClassVars, normal variables, and normal methods

For a full demo of the exhaustive tests showing that it works in those cases ^ (including pydantic-specific tests that I did not put in the PR) see here:

https://gist.github.com/pirate/66f12beac594c99c697cd5543a1cb77b

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me, Thanks!

i'd like to get a final ok from @bluetech

src/pluggy/_manager.py Show resolved Hide resolved
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing the pydantic bit. I still have a question on the AttributeError case.

src/pluggy/_manager.py Outdated Show resolved Hide resolved
src/pluggy/_manager.py Show resolved Hide resolved
@pirate pirate requested a review from bluetech December 19, 2024 02:04
@RonnyPfannschmidt
Copy link
Member

Let's see tp rebase/merge this before Christmas

@nicoddemus
Copy link
Member

Anything else to do here @bluetech ?

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of minor comments, otherwise LGTM.

I wanted to check if it affects pytest collection performance, but I won't get to it anytime soon... Probably it doesn't though.

Comment on lines +204 to +205
# AttributeError: '__signature__' attribute of 'plugin' is class-only
# can be raised when trying to access some descriptor/proxied fields
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# AttributeError: '__signature__' attribute of 'plugin' is class-only
# can be raised when trying to access some descriptor/proxied fields
# May be raised when trying to access some descriptor/proxied fields

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean to remove the line with the exception in the suggestion? imo that's crucial to be able to grep through the codebase to find the error / understand why all this logic is here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I suggest to remove it is that the logic handles any AttributeError which may happen for other reasons. The __signature__ thing is one case. It's not very clear what it means. I think if you can make this error happen in a test it will be better.

Copy link
Author

@pirate pirate Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did here but it was removed from the PR for being too pydantic-specific: https://gist.github.com/pirate/01ca7a6b41595af9a480

Afaik this case is not triggered in any typical code unless you try to use a few specific weird objects w/ descriptors (like Django/Pydantic models) as plugin namespaces. I don't necessarily want to pull in those big libraries to test this escape hatch that just avoids crashing on them, but interaction with those libraries is really the only case where this code path would come up.

Comment on lines +321 to +322
# AttributeError: '__signature__' attribute of <m_or_c> is class-only
# can be raised when trying to access some descriptor/proxied fields
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# AttributeError: '__signature__' attribute of <m_or_c> is class-only
# can be raised when trying to access some descriptor/proxied fields
# May be raised when trying to access some descriptor/proxied fields

method: object
try:
method = getattr(plugin, name)
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're able to cook a test for this case that would be nice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment here: #536 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants