DM-44720: Support pickling of SlackException

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.

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
 ]
 

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

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

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