From ea8970a8cbda89298e8da406b03f2a419984e579 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 7 Jun 2024 11:31:15 -0700 Subject: [PATCH 1/2] Support pickling of SlackException Exception classes that call the `BaseException.__init__` method (possibly via `Exception`) with a different number of arguments than the `__init__` method of the derived exception class takes cannot be pickled and unpickled. Since arq uses pickle to record the exception of failed jobs, and we may want such exceptions to be derived from `SlackException` so that we can easily report them to Slack, this is an annoying limitation. The restriction is due to the properties of the default `__reduce__` method on `BaseException` and probably cannot be changed in Python. Work around this by using the pattern recommended at the end of the discussion in https://github.com/python/cpython/issues/44791, namely avoid calling `BaseException.__init__` entirely. This means we can no longer rely on the default `__str__` behavior and must implement `__str__`. Add a test that `SlackException` and classes derived from it with different numbers of constructor arguments can be pickled and unpickled without loss of the information that goes into `to_slack`. --- changelog.d/20240607_112935_rra_DM_44720.md | 3 +++ pyproject.toml | 1 + src/safir/slack/blockkit.py | 29 ++++++++++++++++++--- tests/slack/blockkit_test.py | 26 ++++++++++++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 changelog.d/20240607_112935_rra_DM_44720.md diff --git a/changelog.d/20240607_112935_rra_DM_44720.md b/changelog.d/20240607_112935_rra_DM_44720.md new file mode 100644 index 00000000..acd8312c --- /dev/null +++ b/changelog.d/20240607_112935_rra_DM_44720.md @@ -0,0 +1,3 @@ +### New features + +- Support pickling of `SlackException` so that subclasses of it can be thrown by arq workers and unpickled correctly when retrieving results. diff --git a/pyproject.toml b/pyproject.toml index 7f94ad67..3716509c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -263,6 +263,7 @@ select = ["ALL"] "PT012", # way too aggressive about limiting pytest.raises blocks "S101", # tests should use assert "S106", # tests are allowed to hard-code dummy passwords + "S301", # one test verifies that a class can be pickled "SLF001", # tests are allowed to access private members ] diff --git a/src/safir/slack/blockkit.py b/src/safir/slack/blockkit.py index 610e5755..b9099e18 100644 --- a/src/safir/slack/blockkit.py +++ b/src/safir/slack/blockkit.py @@ -234,9 +234,19 @@ def to_slack(self) -> dict[str, Any]: class SlackException(Exception): """Parent class of exceptions that can be reported to Slack. - Intended to be subclassed. Subclasses may wish to override the + Intended to be subclassed. Subclasses may wish to override the ``to_slack`` method. + Attributes + ---------- + message + Error message represented by this exception. + user + Username associated with the exception. + failed_at + When the failure occurred. Defaults to the time the exception was + created. + Parameters ---------- message @@ -254,12 +264,24 @@ def __init__( *, failed_at: datetime | None = None, ) -> None: + # Do not call the parent Exception constructor here, because calling + # it with a different number of arguments than the constructor + # argument of derived exceptions breaks pickling. See the end of + # https://github.com/python/cpython/issues/44791. This requires + # implementing __str__ rather than relying on the default behavior. + # + # Arguably, this is a bug in the __reduce__ method of BaseException + # and its interaction with constructors, but it appears to be hard to + # fix. See https://github.com/python/cpython/issues/76877. + self.message = message self.user = user if failed_at: self.failed_at = failed_at else: self.failed_at = current_datetime(microseconds=True) - super().__init__(message) + + def __str__(self) -> str: + return self.message def to_slack(self) -> SlackMessage: """Format the exception as a Slack message. @@ -366,12 +388,11 @@ def __init__( status: int | None = None, body: str | None = None, ) -> None: - self.message = message + super().__init__(message, user, failed_at=failed_at) self.method = method self.url = url self.status = status self.body = body - super().__init__(message, user, failed_at=failed_at) def __str__(self) -> str: result = self.message diff --git a/tests/slack/blockkit_test.py b/tests/slack/blockkit_test.py index cd895db5..f5d3ebb6 100644 --- a/tests/slack/blockkit_test.py +++ b/tests/slack/blockkit_test.py @@ -2,6 +2,7 @@ from __future__ import annotations +import pickle from unittest.mock import ANY import pytest @@ -294,6 +295,31 @@ class SomeError(SlackException): ] +class SlackSubclassException(SlackException): + """Subclsas for testing pickling.""" + + def __init__(self) -> None: + super().__init__("Some error", "username") + + +def test_exception_pickling() -> None: + """Test that a `SlackException` can be pickled and unpickled. + + Errors that may be raised by backend workers in an arq queue must support + pickling so that they can be passed correctly to other workers or the + frontend. + """ + exc = SlackException("some message", "username") + pickled_exc = pickle.loads(pickle.dumps(exc)) + assert exc.to_slack() == pickled_exc.to_slack() + + # Try the same with a derived class with a different number of constructor + # arguments. + subexc = SlackSubclassException() + pickled_subexc = pickle.loads(pickle.dumps(subexc)) + assert subexc.to_slack() == pickled_subexc.to_slack() + + @pytest.mark.asyncio async def test_web_exception( respx_mock: respx.Router, mock_slack: MockSlackWebhook From 6e7e3e3b0754e1c55ee30bc35b50954d6d405578 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 7 Jun 2024 11:47:41 -0700 Subject: [PATCH 2/2] Fix parameters in testing/gcs_test.py Some of the parameters in the test for the Google Storage mock were misspelled. This previously was not caught because that Python library wasn't typed, but the latest version has types and mypy started rejecting the misspelled arguments. Fix the spelling. --- tests/testing/gcs_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testing/gcs_test.py b/tests/testing/gcs_test.py index 0a7d639c..e8105fc3 100644 --- a/tests/testing/gcs_test.py +++ b/tests/testing/gcs_test.py @@ -24,7 +24,7 @@ def test_mock(mock_gcs: MockStorageClient) -> None: credentials = google.auth.default() signing_credentials = impersonated_credentials.Credentials( source_credentials=credentials, - target_principle="some-service-account", + target_principal="some-service-account", target_scopes="https://www.googleapis.com/auth/devstorage.read_only", lifetime=2, ) @@ -62,7 +62,7 @@ def test_mock_files(mock_gcs_file: MockStorageClient) -> None: credentials = google.auth.default() signing_credentials = impersonated_credentials.Credentials( source_credentials=credentials, - target_principle="some-service-account", + target_principal="some-service-account", target_scopes="https://www.googleapis.com/auth/devstorage.read_only", lifetime=2, )