diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 7bafa831f..138fb8049 100644 --- a/src/blueapi/cli/cli.py +++ b/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) == "": - raise ClickException( - "Access denied. Please check your login status and try again." - ) from e - else: - raise e + raise e return wrapper @@ -356,8 +355,10 @@ def on_event(event: AnyEvent) -> None: 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 diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index c5b41ff45..a968547a7 100644 --- a/src/blueapi/client/client.py +++ b/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") diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 52150d36f..342d0f6e9 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -33,7 +33,12 @@ 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 @@ -41,9 +46,12 @@ 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,18 +104,16 @@ 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: @@ -115,9 +121,9 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: 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() diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 3e075a776..084fe13d7 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/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""): + 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 "" 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 "" in str(exception) + assert exception.value.args[0] with pytest.raises(BlueskyRemoteControlError) as exception: client.pause() - assert "" in str(exception) + assert exception.value.args[0] def test_get_task_by_status(rest_client: BlueapiRestClient): diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 93a01d0c9..ed7fa4c27 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/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("") + mock_requests.side_effect = UnauthorisedAccessError(message="") 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) @@ -717,12 +715,16 @@ 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", @@ -730,6 +732,7 @@ def test_env_reload_server_side_error(runner: CliRunner): "invalid_parameters", "remote_control", "value_error", + "streaming_error", ], ) def test_error_handling(exception, error_message, runner: CliRunner): diff --git a/tests/unit_tests/client/test_client.py b/tests/unit_tests/client/test_client.py index a96f428e8..b036ffe4c 100644 --- a/tests/unit_tests/client/test_client.py +++ b/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, "") + 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 diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 2ddcdd380..6301feb67 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/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(