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 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, )