fix: improve default error handling #1379 #1409

src/blueapi/cli/cli.py

@@ -217,13 +217,12 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T:
             raise ClickException(
                 "Failed to establish connection to blueapi server."
             ) from ce
+        except UnauthorisedAccessError as e:
+            raise ClickException(
+                "Access denied. Please check your login status and try again."
+            ) from e
         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
+            raise e
 
     return wrapper
 
@@ -352,12 +351,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,7 @@
 from blueapi.worker.task_worker import TrackableTask
 
 from .event_bus import AnyEvent, EventBusClient, OnAnyEvent
-from .rest import BlueapiRestClient, BlueskyRemoteControlError
+from .rest import BlueapiRestClient, BlueskyRemoteControlError, NotFoundError
 
 TRACER = get_tracer("client")
 
@@ -99,7 +99,7 @@ def __getitem__(self, name: str) -> "DeviceRef":
             self._cache[name] = device
             setattr(self, model.name, device)
             return device
-        except KeyError:
+        except NotFoundError:
             pass
         raise AttributeError(f"No device named '{name}' available")
 

src/blueapi/client/rest.py

@@ -33,17 +33,25 @@
 TRACER = get_tracer("rest")
 
 
-class UnauthorisedAccessError(Exception):
+class BlueskyRequestError(Exception):
+    def __init__(self, code: int | None = None, message: str = "") -> None:
+        super().__init__(code, message)
+
+
+class UnauthorisedAccessError(BlueskyRequestError):
     pass
 
 
 class BlueskyRemoteControlError(Exception):
     pass
 
 
-class BlueskyRequestError(Exception):
-    def __init__(self, code: int, message: str) -> None:
-        super().__init__(message, code)
+class NotFoundError(BlueskyRequestError):
+    pass
+
+
+class UnknownPlanError(BlueskyRequestError):
+    pass
 
 
 class NoContentError(Exception):
@@ -96,28 +104,26 @@ 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()))
+        return NotFoundError(code, response.text)
     else:
-        return BlueskyRemoteControlError(code, str(response))
+        return BlueskyRemoteControlError(response.text)
 
 
 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()

src/blueapi/service/main.py

@@ -497,11 +497,16 @@ 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:
+                raise HTTPException(
+                    status.HTTP_400_BAD_REQUEST,
+                    detail=(f"Cannot transition from {current_state} to {new_state}"),
+                ) 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,9 @@
     BlueapiRestClient,
     BlueskyRemoteControlError,
     BlueskyRequestError,
+    NotFoundError,
     ServiceUnavailableError,
+    UnauthorisedAccessError,
 )
 from blueapi.config import (
     ApplicationConfig,
@@ -217,7 +219,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 +245,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(NotFoundError, match=r"Item not found"):
         rest_client.get_plan("Not exists")
 
 
@@ -268,7 +270,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 +338,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 +365,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 "Worker already active" in exception.value.args[0]
     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 +378,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[0]
     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[0]
 
 
 def test_get_task_by_status(rest_client: BlueapiRestClient):

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

tests/unit_tests/client/test_client.py

@@ -19,7 +19,11 @@
     PlanCache,
 )
 from blueapi.client.event_bus import AnyEvent, EventBusClient
-from blueapi.client.rest import BlueapiRestClient, BlueskyRemoteControlError
+from blueapi.client.rest import (
+    BlueapiRestClient,
+    BlueskyRemoteControlError,
+    NotFoundError,
+)
 from blueapi.config import MissingStompConfigurationError
 from blueapi.core import DataEvent
 from blueapi.service.model import (
@@ -98,7 +102,14 @@ def mock_rest() -> BlueapiRestClient:
     mock.get_plans.return_value = PLANS
     mock.get_plan.side_effect = lambda n: {p.name: p for p in PLANS.plans}[n]
     mock.get_devices.return_value = DEVICES
-    mock.get_device.side_effect = lambda n: {d.name: d for d in DEVICES.devices}[n]
+    device_map = {d.name: d for d in DEVICES.devices}
+
+    def get_device(n: str):
+        if n not in device_map:
+            raise NotFoundError(404, "<Response [404]>")
+        return device_map[n]
+
+    mock.get_device.side_effect = get_device
     mock.get_state.return_value = WorkerState.IDLE
     mock.get_task.return_value = TASK
     mock.get_all_tasks.return_value = TASKS

tests/unit_tests/client/test_rest.py

@@ -11,6 +11,7 @@
     BlueskyRemoteControlError,
     BlueskyRequestError,
     InvalidParametersError,
+    NotFoundError,
     ParameterError,
     UnauthorisedAccessError,
     UnknownPlanError,
@@ -39,8 +40,9 @@ def rest_with_auth(oidc_config: OIDCConfig, tmp_path) -> BlueapiRestClient:
 @pytest.mark.parametrize(
     "code,expected_exception",
     [
-        (404, KeyError),
-        (401, BlueskyRemoteControlError),
+        (404, NotFoundError),
+        (401, UnauthorisedAccessError),
+        (403, UnauthorisedAccessError),
         (450, BlueskyRemoteControlError),
         (500, BlueskyRemoteControlError),
     ],
@@ -63,9 +65,17 @@ def test_rest_error_code(
     "code,content,expected_exception",
     [
         (200, None, None),
-        (401, None, UnauthorisedAccessError()),
-        (403, None, UnauthorisedAccessError()),
-        (404, None, UnknownPlanError()),
+        (
+            401,
+            "unauthorised access",
+            UnauthorisedAccessError(401, "unauthorised access"),
+        ),
+        (403, "Forbidden", UnauthorisedAccessError(403, "Forbidden")),
+        (
+            404,
+            "Plan '{name}' was not recognised",
+            UnknownPlanError(404, "Plan '{name}' was not recognised"),
+        ),
         (
             422,
             """{
@@ -87,6 +97,11 @@ def test_rest_error_code(
                 ]
             ),
         ),
+        (
+            422,
+            '{"detail": "not a list"}',
+            BlueskyRequestError(422, ""),
+        ),
         (450, "non-standard", BlueskyRequestError(450, "non-standard")),
         (500, "internal_error", BlueskyRequestError(500, "internal_error")),
     ],
@@ -102,8 +117,13 @@ def test_create_task_exceptions(
     response.json.side_effect = lambda: json.loads(content) if content else None
     err = _create_task_exceptions(response)
     assert isinstance(err, type(expected_exception))
-    if expected_exception is not None:
-        assert err.args == expected_exception.args
+    if isinstance(expected_exception, InvalidParametersError):
+        assert isinstance(err, InvalidParametersError)
+        assert err.errors == expected_exception.errors
+    elif expected_exception is not None:
+        assert err.args[0] == code
+        if content is not None:
+            assert err.args[1] == content
 
 
 def test_auth_request_functionality(

tests/unit_tests/service/test_rest_api.py

@@ -635,15 +635,17 @@ def test_set_state_transition_error(mock_runner: Mock, client: TestClient):
     current_state = WorkerState.RUNNING
     final_state = WorkerState.STOPPING
 
-    mock_runner.run.side_effect = [current_state, TransitionError(), final_state]
+    mock_runner.run.side_effect = [current_state, TransitionError()]
 
     response = client.put(
         "/worker/state",
         json=StateChangeRequest(new_state=final_state).model_dump(),
     )
 
     assert response.status_code == status.HTTP_400_BAD_REQUEST
-    assert response.json() == final_state
+    assert response.json() == {
+        "detail": f"Cannot transition from {current_state} to {final_state}"
+    }
 
 
 def test_set_state_invalid_transition(mock_runner: Mock, client: TestClient):
@@ -659,7 +661,9 @@ def test_set_state_invalid_transition(mock_runner: Mock, client: TestClient):
     )
 
     assert response.status_code == status.HTTP_400_BAD_REQUEST
-    assert response.json() == final_state
+    assert response.json() == {
+        "detail": f"Cannot transition from {current_state} to {requested_state}"
+    }
 
 
 def test_get_environment_idle(mock_runner: Mock, client: TestClient) -> None: