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

gh-119600: mock: do not access attributes of original when … …new_callable is set #119601

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

rbtcollins
Copy link
Member

@rbtcollins rbtcollins commented May 27, 2024

In order to patch flask.g e.g. as in #84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.

@rbtcollins rbtcollins requested a review from cjw296 as a code owner May 27, 2024 12:05
Copy link

cpython-cla-bot bot commented May 27, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@rbtcollins
Copy link
Member Author

(I haven't contributed to CPython for some years, so please forgive any irregularities and let me know what I need to do to make the PR acceptable !)

I can of course write a test; its likely worth it, but that will need to be a few days away

@rbtcollins rbtcollins force-pushed the rbt/bug-119600 branch 2 times, most recently from 4e69397 to 224f2b5 Compare May 27, 2024 13:14
@rbtcollins rbtcollins changed the title mock: do not access attributes of original when new_callable is set Issue #119600: mock: do not access attributes of original when … …new_callable is set May 27, 2024
@cjw296
Copy link
Contributor

cjw296 commented May 27, 2024

Yes, please add a reproducer test that fails before the change and succeeds after.

@rbtcollins
Copy link
Member Author

@cjw296 added!

@rbtcollins
Copy link
Member Author

@cjw296 ping?

Copy link
Contributor

@cjw296 cjw296 left a comment

Choose a reason for hiding this comment

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

LGTM, need to add blurb to make the news check happy.
Also curious about why it's not finding the issue number?

@rbtcollins rbtcollins changed the title Issue #119600: mock: do not access attributes of original when … …new_callable is set gh-119600: mock: do not access attributes of original when … …new_callable is set Jun 10, 2024
…new_callable is set

In order to patch flask.g e.g. as in python#84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
Copy link
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch.

@cjw296 cjw296 added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jun 11, 2024
@cjw296 cjw296 merged commit 422c4fc into python:main Jun 11, 2024
35 checks passed
@miss-islington-app
Copy link

Thanks @rbtcollins for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 11, 2024
…callable is set (pythonGH-119601)

In order to patch flask.g e.g. as in pythonGH-84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
(cherry picked from commit 422c4fc)

Co-authored-by: Robert Collins <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jun 11, 2024

GH-120334 is a backport of this pull request to the 3.13 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 11, 2024
…callable is set (pythonGH-119601)

In order to patch flask.g e.g. as in pythonGH-84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
(cherry picked from commit 422c4fc)

Co-authored-by: Robert Collins <[email protected]>
@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 11, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jun 11, 2024

GH-120335 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jun 11, 2024
cjw296 pushed a commit that referenced this pull request Jun 11, 2024
…_callable is set (GH-119601) (#120335)

gh-119600: mock: do not access attributes of original when new_callable is set (GH-119601)

In order to patch flask.g e.g. as in GH-84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
(cherry picked from commit 422c4fc)

Co-authored-by: Robert Collins <[email protected]>
cjw296 pushed a commit that referenced this pull request Jun 11, 2024
…_callable is set (GH-119601) (#120334)

gh-119600: mock: do not access attributes of original when new_callable is set (GH-119601)

In order to patch flask.g e.g. as in GH-84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
(cherry picked from commit 422c4fc)

Co-authored-by: Robert Collins <[email protected]>
@rbtcollins
Copy link
Member Author

Also curious about why it's not finding the issue number?
It seems to want gh-xxxx not Issue #xxxx - probably because of the coexistence of the old tracker ...

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
…callable is set (python#119601)

In order to patch flask.g e.g. as in python#84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…callable is set (python#119601)

In order to patch flask.g e.g. as in python#84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…callable is set (python#119601)

In order to patch flask.g e.g. as in python#84982, that
proxies getattr must not be invoked. For that,
mock must not try to read from the original
object. In some cases that is unavoidable, e.g.
when doing autospec. However, patch("flask.g",
new_callable=MagicMock) should be entirely safe.
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.

3 participants