gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

cklein · 2023-01-02T15:51:44Z

Prevent prefix "called_" for mocks in safe mode

Issue: Prevent prefix "called_" for methods on mock objects in safe mode #100690

bedevere-bot · 2023-01-02T15:51:48Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

cjw296

I'm -0 on this change, feels like we're into the diminishing realms where someone out there is going to have a legitimate use case for called_name or some such and this then prevents them using a safe mock.

Who else is hitting this?

cklein · 2023-01-02T17:04:56Z

@cjw296, the same applies to any of the forbidden prefixes. This could be solved by allowing those prefixes with a spec. Currently, even if the name appears in _mock_methods (https://github.com/python/cpython/blob/main/Lib/unittest/mock.py#L651 evaluates to true), an error will be raised. But I guess this could be a different and more fundamental discussion.

cklein · 2023-01-02T21:32:12Z

I have been thinking a bit more about the whole problem. It seems the current behavior does not match the error message:

>>> class Foo:
...     def assert_bar(self):
...         pass
...
>>> from unittest.mock import Mock
>>> m = Mock(spec=Foo)
>>> m.assert_bar()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/unittest/mock.py", line 648, in __getattr__
    raise AttributeError(
AttributeError: 'assert_bar' is not a valid assertion. Use a spec for the mock if 'assert_bar' is meant to be an attribute.

The error message says "Use a spec for the mock if 'assert_bar' is meant to be an attribute.", but this example should pass according to the error message. There's a spec and the name assert_bar should be supported.

With cklein@37aa291 applied, it behaves like described:

>>> class Foo:
...     def assert_bar(self):
...         pass
...
>>> from unittest.mock import Mock
>>> m = Mock(spec=Foo)
>>> m.assert_bar()
<Mock name='mock.assert_bar()' id='4384202080'>
>>> m.assert_frobnicated()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/cklein/github/cklein/cpython/Lib/unittest/mock.py", line 652, in __getattr__
    raise AttributeError("Mock object has no attribute %r" % name)
AttributeError: Mock object has no attribute 'assert_frobnicated'

merwok · 2023-01-03T00:13:26Z

The prefixes checked could be more specific like called_once_with, called_once, called_with to alleviate the concern about forbidding too much.

cjw296 · 2023-01-03T10:12:57Z

@cklein - I think you're right about this being a complicated issue, perhaps a PR isn't the best place to discuss it? Could you maybe close this PR an open an issue explaining your concerns and some possible ways forward?

AlexWaygood · 2023-01-03T10:48:32Z

Could you maybe close this PR an open an issue explaining your concerns and some possible ways forward?

OP has already done so:

They first opened a discussion on discuss.python.org, https://discuss.python.org/t/include-prefix-called-in-list-of-forbidden-method-prefixes-for-mock-objects-in-unsafe-mode/22249.
After discussing it there, they were advised by a core developer to open an issue: Prevent prefix "called_" for methods on mock objects in safe mode #100690
And a PR (this one).

cklein · 2023-01-03T10:49:43Z

I can also create another issue for my second branch about the spec not being properly checked, if that helps

Edit: See https://discuss.python.org/t/check-for-assert-prefixes-doesnt-respect-spec/22388

… mode Raise an AttributeError when calling e.g. `mock_obj.called_once` instead of `mock_obj.assert_called_once`.

cklein · 2023-01-03T15:33:18Z

I'm aware that the documentation is now off. I'd like to get some feedback on the change first, if that's OK

…prefix

… spec

Lib/unittest/mock.py

cjw296 · 2023-01-05T14:10:32Z

Lib/unittest/mock.py

@@ -1062,6 +1062,10 @@ def _calls_repr(self, prefix="Calls"):
        return f"\n{prefix}: {safe_repr(self.mock_calls)}."


+# gh-100690 Denylist for forbidden attribute names in safe mode
+ATTRIB_DENY_LIST = {name.removeprefix("assert_") for name in dir(NonCallableMock) if name.startswith("assert_")}


Much prefer this approach! 🎉

cjw296 · 2023-01-05T14:12:03Z

Approach is good, so once the docs are fixed up, should be good to land.
Would a backport release once that's done be helpful? (I mean, I'll probably crank the handle anyway, but thought I'd ask!)

cklein · 2023-01-05T21:01:52Z

I'm struggling a bit with documentation. What about the following:

    * `unsafe`: By default, accessing any attribute whose name starts with
      *assert*, *assret*, *asert*, *aseert*, or *assrt* raises an AttributeError.
      Additionally, an AttributeError is raised when accessing
      attributes that match the name of assertion method without the prefix
      `assert_`, e.g. accessing `called_once` instead of `assert_called_once`.
      Passing `unsafe=True` will allow access to these attributes.

Misc/NEWS.d/next/Library/2023-01-02-16-59-49.gh-issue-100690.2EgWPS.rst

cklein · 2023-01-06T17:45:00Z

Would a backport release once that's done be helpful?

Personally I'd like to see it in earlier versions of Python.
But then I'm a bit torn because on the one hand this will definitely make some tests fail, on the other hand those tests will finally be properly executed and some unwanted behavior might be uncovered.

cjw296 · 2023-01-06T18:00:42Z

@cklein - 3.10 and 3.11 are open for bug fixes so their next third point release would contain this once its landed.
...and to be clear: I consider this a bug in that there are checks for these problems but they are insufficient.

cjw296 · 2023-01-06T18:36:34Z

The Azure Pipelines failure seems unrelated.

AlexWaygood · 2023-01-06T18:38:58Z

The Azure Pipelines failure seems unrelated.

Yes, that failure happens intermittently on a lot of PRs

bedevere-bot · 2023-01-06T19:48:58Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 LTO 3.x has failed when building commit 1d4d677.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/503/builds/2845) and take a look at the build logs.
Check if the failure is related to this commit (1d4d677) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/503/builds/2845

Failed tests:

test_nntplib

Failed subtests:

tearDownClass - test.test_nntplib.NetworkedNNTPTests
test_with_statement - test.test_nntplib.NetworkedNNTPTests.test_with_statement

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

408 tests OK.

10 slowest tests:

test_concurrent_futures: 2 min 46 sec
test_multiprocessing_spawn: 1 min 45 sec
test_multiprocessing_forkserver: 1 min 20 sec
test_multiprocessing_fork: 1 min 9 sec
test_tokenize: 1 min 7 sec
test_asyncio: 1 min 2 sec
test_logging: 54.8 sec
test_signal: 48.0 sec
test_venv: 42.2 sec
test_io: 38.8 sec

1 test failed:
test_nntplib

24 tests skipped:
test_check_c_globals test_devpoll test_gdb test_idle test_ioctl
test_kqueue test_launcher test_msilib test_peg_generator
test_perf_profiler test_smtpnet test_ssl test_startfile test_tcl
test_tix test_tkinter test_ttk test_ttk_textonly test_turtle
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_nntplib

Total duration: 7 min 7 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_nntplib.py", line 252, in wrapped
    meth(self)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_nntplib.py", line 286, in test_with_statement
    server = self.NNTP_CLASS(self.NNTP_HOST, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 341, in __init__
    self._base_init(readermode)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 359, in _base_init
    self.getcapabilities()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 421, in getcapabilities
    resp, caps = self.capabilities()
                 ^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 590, in capabilities
    resp, lines = self._longcmdstring("CAPABILITIES")
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 557, in _longcmdstring
    resp, list = self._getlongresp(file)
                 ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 508, in _getlongresp
    resp = self._getresp()
           ^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 481, in _getresp
    resp = self._getline()
           ^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 469, in _getline
    if not line: raise EOFError
                 ^^^^^^^^^^^^^^
EOFError


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_nntplib.py", line 347, in tearDownClass
    cls.server.quit()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 933, in quit
    resp = self._shortcmd('QUIT')
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 543, in _shortcmd
    return self._getresp()
           ^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 481, in _getresp
    resp = self._getline()
           ^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 464, in _getline
    line = self.file.readline(_MAXLINE +1)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/socket.py", line 707, in readinto
    return self._sock.recv_into(b)
           ^^^^^^^^^^^^^^^^^^^^^^^
TimeoutError: timed out

sobolevn · 2023-01-07T09:57:35Z

Lib/unittest/mock.py

@@ -1062,6 +1062,10 @@ def _calls_repr(self, prefix="Calls"):
        return f"\n{prefix}: {safe_repr(self.mock_calls)}."


+# Denylist for forbidden attribute names in safe mode
+ATTRIB_DENY_LIST = {name.removeprefix("assert_") for name in dir(NonCallableMock) if name.startswith("assert_")}


Sorry for being late to the party, but anyways :)

I think that ATTRIB_DENY_LIST is an implementation detail, so it should be _ATTRIB_DENY_LIST. And also, it is not suited for mutation, isn't it? So, it should be a frozenset instead :)

Since this PR is already merged, I've opened #100819 with this proposal. Feel free to close if it should be public and mutable!

It is already non-public by not being included in __all__, and the mutability is not really an issue.

cklein requested a review from cjw296 as a code owner January 2, 2023 15:51

bedevere-bot added the awaiting review label Jan 2, 2023

cklein changed the title ~~Prevent prefix "called_" for attributes of safe mock objects~~ gh-100690: Prevent prefix "called_" for attributes of safe mock objects Jan 2, 2023

bedevere-bot mentioned this pull request Jan 2, 2023

Prevent prefix "called_" for methods on mock objects in safe mode #100690

Closed

cklein force-pushed the disallow_mock_method_prefix branch 2 times, most recently from 25e26d8 to 7bc8c46 Compare January 2, 2023 16:25

cjw296 reviewed Jan 2, 2023

View reviewed changes

cklein force-pushed the disallow_mock_method_prefix branch from 7bc8c46 to b84bc75 Compare January 2, 2023 16:46

Prevent calling assert functions without the prefix "assert_" in safe…

154012f

… mode Raise an AttributeError when calling e.g. `mock_obj.called_once` instead of `mock_obj.assert_called_once`.

cklein force-pushed the disallow_mock_method_prefix branch from b84bc75 to 154012f Compare January 3, 2023 11:35

merwok added the DO-NOT-MERGE label Jan 3, 2023

cklein added 2 commits January 5, 2023 07:43

Merge remote-tracking branch 'python/main' into disallow_mock_method_…

26dc793

…prefix

Added tests to show that names from denylist still can be used with a…

2bd97a1

… spec

cjw296 reviewed Jan 5, 2023

View reviewed changes

Lib/unittest/mock.py Outdated Show resolved Hide resolved

cjw296 reviewed Jan 5, 2023

View reviewed changes

merwok added type-feature A feature request or enhancement stdlib Python modules in the Lib dir labels Jan 5, 2023

Changed documentation

01946cb

merwok reviewed Jan 6, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-01-02-16-59-49.gh-issue-100690.2EgWPS.rst Outdated Show resolved Hide resolved

Reformatted NEWS

e9b8f73

cjw296 approved these changes Jan 6, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jan 6, 2023

Merge branch 'main' into disallow_mock_method_prefix

674f5bc

cjw296 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-bug An unexpected behavior, bug, or error and removed DO-NOT-MERGE type-feature A feature request or enhancement labels Jan 6, 2023

merwok added the DO-NOT-MERGE label Jan 6, 2023

cjw296 removed DO-NOT-MERGE needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jan 6, 2023

cjw296 merged commit 1d4d677 into python:main Jan 6, 2023

bedevere-bot removed the awaiting merge label Jan 6, 2023

sobolevn reviewed Jan 7, 2023

View reviewed changes

sobolevn mentioned this pull request Jan 7, 2023

gh-100690: [mock] hide ATTRIB_DENY_LIST and make it immutable #100819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

cklein commented Jan 2, 2023 •

edited by AlexWaygood

Loading

bedevere-bot commented Jan 2, 2023

cjw296 left a comment

cklein commented Jan 2, 2023

cklein commented Jan 2, 2023 •

edited by AlexWaygood

Loading

merwok commented Jan 3, 2023

cjw296 commented Jan 3, 2023

AlexWaygood commented Jan 3, 2023

cklein commented Jan 3, 2023 •

edited

Loading

cklein commented Jan 3, 2023

cjw296 Jan 5, 2023

cjw296 commented Jan 5, 2023

cklein commented Jan 5, 2023

cklein commented Jan 6, 2023

cjw296 commented Jan 6, 2023 •

edited

Loading

cjw296 commented Jan 6, 2023

AlexWaygood commented Jan 6, 2023

bedevere-bot commented Jan 6, 2023

sobolevn Jan 7, 2023

sobolevn Jan 7, 2023

merwok Jan 7, 2023

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

Conversation

cklein commented Jan 2, 2023 • edited by AlexWaygood Loading

bedevere-bot commented Jan 2, 2023

cjw296 left a comment

Choose a reason for hiding this comment

cklein commented Jan 2, 2023

cklein commented Jan 2, 2023 • edited by AlexWaygood Loading

merwok commented Jan 3, 2023

cjw296 commented Jan 3, 2023

AlexWaygood commented Jan 3, 2023

cklein commented Jan 3, 2023 • edited Loading

cklein commented Jan 3, 2023

cjw296 Jan 5, 2023

Choose a reason for hiding this comment

cjw296 commented Jan 5, 2023

cklein commented Jan 5, 2023

cklein commented Jan 6, 2023

cjw296 commented Jan 6, 2023 • edited Loading

cjw296 commented Jan 6, 2023

AlexWaygood commented Jan 6, 2023

bedevere-bot commented Jan 6, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

sobolevn Jan 7, 2023

Choose a reason for hiding this comment

sobolevn Jan 7, 2023

Choose a reason for hiding this comment

merwok Jan 7, 2023

Choose a reason for hiding this comment

cklein commented Jan 2, 2023 •

edited by AlexWaygood

Loading

cklein commented Jan 2, 2023 •

edited by AlexWaygood

Loading

cklein commented Jan 3, 2023 •

edited

Loading

cjw296 commented Jan 6, 2023 •

edited

Loading