chore: Improve default error handling (#1491)

Fixes #1379
Fixes #1409

Both issues related to error handling in the REST client.

1379 - Catches 'UnauthorisedAccessErrorexplicitly instead of
string-matching onBlueskyRemoteControlError`.

1409 - Expands the exception hierarchy so each
HTTP status code raises a distinct typed exception rather than bundling
everything into BlueskyRemoteControlError.
Tests

    Update tests to match new exception signatures
    Add missing coverage for 422 non-list detail response
    Add missing coverage for streaming error in run command

---------

Co-authored-by: Peter Holloway <peter.holloway@diamond.ac.uk>
Co-authored-by: Shreelakshmi Iyengar <shreelakshmi.iyengar@diamond.ac.uk>
Co-authored-by: Zoheb Shaikh <zoheb.shaikh@diamond.ac.uk>
Co-authored-by: Daniel Fernandes <65790536+dan-fernandes@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Zoheb Shaikh <26975142+ZohebShaikh@users.noreply.github.com>
Co-authored-by: Abigail Emery <abigail.emery@diamond.ac.uk>
Co-authored-by: oliwenmandiamond <136330507+oliwenmandiamond@users.noreply.github.com>
Co-authored-by: NeilSmithDLS <118457264+NeilSmithDLS@users.noreply.github.com>

Author: 9 people

src/blueapi/cli/cli.py

@@ -254,13 +254,10 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T:
             raise ClickException(
                 "Failed to establish connection to blueapi server."
             ) from ce
-        except BlueskyRemoteControlError as e:
-            if str(e) == "<Response [401]>":
-                raise ClickException(
-                    "Access denied. Please check your login status and try again."
-                ) from e
-            else:
-                raise e
+        except UnauthorisedAccessError as e:
+            raise ClickException(
+                "Access denied. Please check your login status and try again."
+            ) from e
 
     return wrapper
 
@@ -394,12 +391,12 @@ def on_event(event: AnyEvent) -> None:
         raise ClickException(*mse.args) from mse
     except UnknownPlanError as up:
         raise ClickException(f"Plan '{name}' was not recognised") from up
-    except UnauthorisedAccessError as ua:
-        raise ClickException("Unauthorised request") from ua
     except InvalidParametersError as ip:
         raise ClickException(ip.message()) from ip
-    except (BlueskyRemoteControlError, BlueskyStreamingError) as e:
-        raise ClickException(f"server error with this message: {e}") from e
+    except BlueskyStreamingError as se:
+        raise ClickException(f"Streaming error: {se}") from se
+    except BlueskyRemoteControlError as e:
+        raise ClickException(f"Remote control error: {e.args[0]}") from e
     except ValueError as ve:
         raise ClickException(f"task could not run: {ve}") from ve
 

src/blueapi/client/client.py

@@ -42,7 +42,13 @@
 from blueapi.worker.task_worker import TrackableTask
 
 from .event_bus import AnyEvent, EventBusClient, OnAnyEvent
-from .rest import BlueapiRestClient, BlueskyRemoteControlError
+from .rest import (
+    BlueapiRestClient,
+    BlueskyRemoteControlError,
+    BlueskyRequestError,
+    NotFoundError,
+    ServiceUnavailableError,
+)
 
 TRACER = get_tracer("client")
 
@@ -99,9 +105,8 @@ def __getitem__(self, name: str) -> "DeviceRef":
             self._cache[name] = device
             setattr(self, model.name, device)
             return device
-        except KeyError:
-            pass
-        raise AttributeError(f"No device named '{name}' available")
+        except NotFoundError as e:
+            raise AttributeError(f"No device named '{name}' available") from e
 
     def __getattr__(self, name: str) -> "DeviceRef":
         if name.startswith("_"):
@@ -668,9 +673,13 @@ def reload_environment(
             EnvironmentResponse: Details of the new worker
             environment.
         """
-
         try:
             status = self._rest.delete_environment()
+        except (
+            BlueskyRequestError,
+            ServiceUnavailableError,
+        ):
+            raise
         except Exception as e:
             raise BlueskyRemoteControlError(
                 "Failed to tear down the environment"

src/blueapi/client/rest.py

@@ -38,21 +38,41 @@
 LOGGER = logging.getLogger(__name__)
 
 
-class UnauthorisedAccessError(Exception):
+class BlueskyRequestError(Exception):
+    """An error response from the blueapi server."""
+
+    def __init__(self, code: int | None = None, message: str = "") -> None:
+        super().__init__(code, message)
+
+
+class UnauthorisedAccessError(BlueskyRequestError):
+    """Request was rejected due to missing or invalid credentials (401/403)."""
+
     pass
 
 
-class BlueskyRemoteControlError(Exception):
+class NotFoundError(BlueskyRequestError):
+    """Requested something that couldn't be found (404)."""
+
     pass
 
 
-class NonJsonResponseError(Exception):
+class UnknownPlanError(BlueskyRequestError):
+    """Plan '{name}' was not recognised"""
+
     pass
 
 
-class BlueskyRequestError(Exception):
-    def __init__(self, code: int, message: str) -> None:
-        super().__init__(message, code)
+class BlueskyRemoteControlError(Exception):
+    """Unexpected or failed response from the blueapi server."""
+
+    pass
+
+
+class NonJsonResponseError(Exception):
+    """Server returned a response that could not be parsed as JSON."""
+
+    pass
 
 
 class NoContentError(Exception):
@@ -105,28 +125,35 @@ def from_validation_error(cls, ve: ValidationError):
         )
 
 
-class UnknownPlanError(Exception):
-    pass
-
-
 def _exception(response: requests.Response) -> Exception | None:
     code = response.status_code
     if code < 400:
         return None
+    elif code in (401, 403):
+        return UnauthorisedAccessError(code, response.text)
     elif code == 404:
-        return KeyError(str(_response_json(response)))
+        return NotFoundError(code, response.text)
     else:
-        return BlueskyRemoteControlError(code, str(response))
+        try:
+            body = _response_json(response)
+            message = (
+                body.get("detail", response.text)
+                if isinstance(body, dict)
+                else response.text
+            )
+        except NonJsonResponseError:
+            message = response.text
+        return BlueskyRemoteControlError(code, message)
 
 
 def _create_task_exceptions(response: requests.Response) -> Exception | None:
     code = response.status_code
     if code < 400:
         return None
     elif code == 401 or code == 403:
-        return UnauthorisedAccessError()
+        return UnauthorisedAccessError(code, response.text)
     elif code == 404:
-        return UnknownPlanError()
+        return UnknownPlanError(code, response.text)
     elif code == 422:
         try:
             content = _response_json(response)
@@ -173,7 +200,10 @@ def get_plans(self) -> PlanResponse:
         return self._request_and_deserialize("/plans", PlanResponse)
 
     def get_plan(self, name: str) -> PlanModel:
-        return self._request_and_deserialize(f"/plans/{name}", PlanModel)
+        try:
+            return self._request_and_deserialize(f"/plans/{name}", PlanModel)
+        except NotFoundError as e:
+            raise UnknownPlanError(404, f"Plan '{name}' not found") from e
 
     def get_devices(self) -> DeviceResponse:
         return self._request_and_deserialize("/devices", DeviceResponse)

src/blueapi/service/main.py

@@ -534,11 +534,20 @@ def set_state(
                     state_change_request.new_state is WorkerState.ABORTING,
                     state_change_request.reason,
                 )
-            except TransitionError:
-                response.status_code = status.HTTP_400_BAD_REQUEST
+            except TransitionError as e:
+                suffix = f" - {e}" if str(e) else ""
+                raise HTTPException(
+                    status.HTTP_400_BAD_REQUEST,
+                    detail=(
+                        f"Error while transitioning from {current_state} "
+                        f"to {new_state}{suffix}"
+                    ),
+                ) from e
     else:
-        response.status_code = status.HTTP_400_BAD_REQUEST
-
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail=(f"Cannot transition from {current_state} to {new_state}"),
+        )
     return runner.run(interface.get_worker_state)
 
 

tests/system_tests/test_blueapi_system.py

@@ -15,7 +15,10 @@
     BlueapiRestClient,
     BlueskyRemoteControlError,
     BlueskyRequestError,
+    NotFoundError,
     ServiceUnavailableError,
+    UnauthorisedAccessError,
+    UnknownPlanError,
 )
 from blueapi.config import (
     ApplicationConfig,
@@ -217,7 +220,7 @@ def test_cannot_access_endpoints(
         "get_oidc_config"
     )  # get_oidc_config can be accessed without auth
     for get_method in blueapi_rest_client_get_methods:
-        with pytest.raises(BlueskyRemoteControlError, match=r"<Response \[401\]>"):
+        with pytest.raises(UnauthorisedAccessError, match=r"Not authenticated"):
             getattr(client_without_auth._rest, get_method)()
 
 
@@ -243,7 +246,7 @@ def test_get_plans_by_name(client: BlueapiClient, expected_plans: PlanResponse):
 
 
 def test_get_non_existent_plan(rest_client: BlueapiRestClient):
-    with pytest.raises(KeyError, match="{'detail': 'Item not found'}"):
+    with pytest.raises(UnknownPlanError, match=r"Plan 'Not exists' not found"):
         rest_client.get_plan("Not exists")
 
 
@@ -268,7 +271,7 @@ def test_get_device_by_name(
 
 
 def test_get_non_existent_device(rest_client: BlueapiRestClient):
-    with pytest.raises(KeyError, match="{'detail': 'Item not found'}"):
+    with pytest.raises(NotFoundError, match=r"Item not found"):
         rest_client.get_device("Not exists")
 
 
@@ -336,12 +339,12 @@ def test_get_task_by_id(rest_client: BlueapiRestClient):
 
 
 def test_get_non_existent_task(rest_client: BlueapiRestClient):
-    with pytest.raises(KeyError, match="{'detail': 'Item not found'}"):
+    with pytest.raises(NotFoundError, match=r"Item not found"):
         rest_client.get_task("Not-exists")
 
 
 def test_delete_non_existent_task(rest_client: BlueapiRestClient):
-    with pytest.raises(KeyError, match="{'detail': 'Item not found'}"):
+    with pytest.raises(NotFoundError, match=r"Item not found"):
         rest_client.clear_task("Not-exists")
 
 
@@ -363,7 +366,7 @@ def test_put_worker_task_fails_if_not_idle(rest_client: BlueapiRestClient):
 
     with pytest.raises(BlueskyRemoteControlError) as exception:
         rest_client.update_worker_task(WorkerTask(task_id=small_task.task_id))
-    assert "<Response [409]>" in str(exception)
+    assert exception.value.args[0] == 409
     rest_client.cancel_current_task(WorkerState.ABORTING)
     rest_client.clear_task(small_task.task_id)
     rest_client.clear_task(long_task.task_id)
@@ -376,10 +379,10 @@ def test_get_worker_state(client: BlueapiClient):
 def test_set_state_transition_error(client: BlueapiClient):
     with pytest.raises(BlueskyRemoteControlError) as exception:
         client.resume()
-    assert "<Response [400]>" in str(exception)
+    assert "Cannot transition from IDLE to RUNNING" in exception.value.args[1]
     with pytest.raises(BlueskyRemoteControlError) as exception:
         client.pause()
-    assert "<Response [400]>" in str(exception)
+    assert "Cannot transition from IDLE to PAUSED" in exception.value.args[1]
 
 
 def test_get_task_by_status(rest_client: BlueapiRestClient):
@@ -621,7 +624,7 @@ def on_event(event: AnyEvent) -> None:
 
 # Regression test for #1480
 def test_task_submission_after_invalid_task(client_with_stomp: BlueapiClient):
-    with pytest.raises(KeyError):
+    with pytest.raises(NotFoundError):
         # This task hasn't been submitted so should return an error...
         client_with_stomp._rest.update_worker_task(WorkerTask(task_id="missing"))
 

tests/unit_tests/cli/test_cli.py

@@ -118,9 +118,8 @@ def test_connection_error_caught_by_wrapper_func(
 def test_authentication_error_caught_by_wrapper_func(
     mock_requests: Mock, runner: CliRunner
 ):
-    mock_requests.side_effect = BlueskyRemoteControlError("<Response [401]>")
+    mock_requests.side_effect = UnauthorisedAccessError(message="<Response [401]>")
     result = runner.invoke(main, ["controller", "plans"])
-
     assert (
         result.output
         == "Error: Access denied. Please check your login status and try again.\n"
@@ -130,7 +129,6 @@ def test_authentication_error_caught_by_wrapper_func(
 @patch("blueapi.client.rest.requests.Session.request")
 def test_remote_error_raised_by_wrapper_func(mock_requests: Mock, runner: CliRunner):
     mock_requests.side_effect = BlueskyRemoteControlError("Response [450]")
-
     result = runner.invoke(main, ["controller", "plans"])
     assert (
         isinstance(result.exception, BlueskyRemoteControlError)
@@ -701,7 +699,10 @@ def test_env_reload_server_side_error(runner: CliRunner):
     "exception, error_message",
     [
         (UnknownPlanError(), "Error: Plan 'sleep' was not recognised\n"),
-        (UnauthorisedAccessError(), "Error: Unauthorised request\n"),
+        (
+            UnauthorisedAccessError(),
+            "Error: Access denied. Please check your login status and try again.\n",
+        ),
         (
             InvalidParametersError(
                 errors=[
@@ -717,19 +718,24 @@ def test_env_reload_server_side_error(runner: CliRunner):
         ),
         (
             BlueskyRemoteControlError("Server error"),
-            "Error: server error with this message: Server error\n",
+            "Error: Remote control error: Server error\n",
         ),
         (
             ValueError("Error parsing parameters"),
             "Error: task could not run: Error parsing parameters\n",
         ),
+        (
+            BlueskyStreamingError("streaming failed"),
+            "Error: Streaming error: streaming failed\n",
+        ),
     ],
     ids=[
         "unknown_plan",
         "unauthorised_access",
         "invalid_parameters",
         "remote_control",
         "value_error",
+        "streaming_error",
     ],
 )
 def test_error_handling(exception, error_message, runner: CliRunner):
@@ -1163,7 +1169,7 @@ def test_invalid_json(
         responses.GET,
         "http://localhost:8000/config/oidc",
         body="blah blah",
-        status=404,
+        status=200,
     )
 
     result = runner.invoke(main, ["-c", config_with_auth, "login"])