Skip to content

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

Merged
cjw296 merged 3 commits into
python:mainfrom
rbtcollins:rbt/bug-119600
Jun 11, 2024
Merged

gh-119600: mock: do not access attributes of original when … …new_callable is set#119601
cjw296 merged 3 commits into
python:mainfrom
rbtcollins:rbt/bug-119600

Conversation

@rbtcollins

@rbtcollins rbtcollins commented May 27, 2024

Copy link
Copy Markdown
Member

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
@ghost

ghost commented May 27, 2024

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@rbtcollins

Copy link
Copy Markdown
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

cjw296 commented May 27, 2024

Copy link
Copy Markdown
Contributor

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

@rbtcollins

Copy link
Copy Markdown
Member Author

@cjw296 added!

@rbtcollins

Copy link
Copy Markdown
Member Author

@cjw296 ping?

@cjw296 cjw296 left a comment

Copy link
Copy Markdown
Contributor

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.

@tirkarthi tirkarthi left a comment

Copy link
Copy Markdown
Member

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 only 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
@miss-islington-app

Copy link
Copy Markdown

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 <robert.collins@cognite.com>
@bedevere-app

bedevere-app Bot commented Jun 11, 2024

Copy link
Copy Markdown

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 <robert.collins@cognite.com>
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 11, 2024
@bedevere-app

bedevere-app Bot commented Jun 11, 2024

Copy link
Copy Markdown

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 only 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 <robert.collins@cognite.com>
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 <robert.collins@cognite.com>
@rbtcollins

Copy link
Copy Markdown
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