From 302a8a08c59fd986ea4e460f35694cd05d145a01 Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Thu, 9 Apr 2026 08:43:49 +0100 Subject: [PATCH 01/10] improve error handling when request receives non-JSON response --- src/blueapi/client/rest.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 095a23afc1..08dab61362 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -105,6 +105,10 @@ class UnknownPlanError(Exception): def _exception(response: requests.Response) -> Exception | None: + try: + response.json() + except: + return BlueskyRemoteControlError("Response does not contain a valid JSON object") code = response.status_code if code < 400: return None From e201d2534eac058d8d5f5cc08d6ed871dedfc234 Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Thu, 9 Apr 2026 13:30:59 +0100 Subject: [PATCH 02/10] fixing tests and ruff issues --- src/blueapi/cli/cli.py | 17 ++++++++++------- src/blueapi/client/rest.py | 6 ++++-- tests/unit_tests/cli/test_cli.py | 3 ++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 86a2cd5615..d638b46801 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -513,13 +513,16 @@ def login(obj: dict) -> None: print("Logged in") except Exception: client = BlueapiClient.from_config(config) - if oidc := client.oidc_config: - auth = SessionManager( - oidc, cache_manager=SessionCacheManager(config.auth_token_path) - ) - auth.start_device_flow() - else: - print("Server is not configured to use authentication!") + try: + if oidc := client.oidc_config: + auth = SessionManager( + oidc, cache_manager=SessionCacheManager(config.auth_token_path) + ) + auth.start_device_flow() + else: + print("Server is not configured to use authentication!") + except BlueskyRemoteControlError: + print ("Error thrown whilst checking authentication") @main.command(name="logout") diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 08dab61362..d6b2f3cfad 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -2,6 +2,7 @@ from collections.abc import Callable, Mapping from typing import Any, Literal, TypeVar +import json import requests from fastapi import status from observability_utils.tracing import ( @@ -107,8 +108,9 @@ class UnknownPlanError(Exception): def _exception(response: requests.Response) -> Exception | None: try: response.json() - except: - return BlueskyRemoteControlError("Response does not contain a valid JSON object") + except json.decoder.JSONDecodeError as je: + return BlueskyRemoteControlError( + "Response does not contain a valid JSON object") code = response.status_code if code < 400: return None diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 35bc830ae6..88b11aa95d 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1134,7 +1134,8 @@ def test_login_with_unauthenticated_server( mock_unauthenticated_server: responses.RequestsMock, ): result = runner.invoke(main, ["-c", config_with_auth, "login"]) - assert "Server is not configured to use authentication!\n" == result.output + print (f"result.output={result.output}") + assert "Error thrown whilst checking authentication\n" == result.output assert result.exit_code == 0 From 547cceb908c629f01abc0d129c091749a3597b56 Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Thu, 9 Apr 2026 14:00:39 +0100 Subject: [PATCH 03/10] fixing ruff (again) --- tests/unit_tests/cli/test_cli.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 88b11aa95d..67d22ad6a8 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1134,7 +1134,6 @@ def test_login_with_unauthenticated_server( mock_unauthenticated_server: responses.RequestsMock, ): result = runner.invoke(main, ["-c", config_with_auth, "login"]) - print (f"result.output={result.output}") assert "Error thrown whilst checking authentication\n" == result.output assert result.exit_code == 0 From cf50791532272695f11965dd2024dee5aab666de Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Thu, 9 Apr 2026 14:07:05 +0100 Subject: [PATCH 04/10] ruff (again, again) --- src/blueapi/cli/cli.py | 2 +- src/blueapi/client/rest.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index d638b46801..28ef04d527 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -522,7 +522,7 @@ def login(obj: dict) -> None: else: print("Server is not configured to use authentication!") except BlueskyRemoteControlError: - print ("Error thrown whilst checking authentication") + print("Error thrown whilst checking authentication") @main.command(name="logout") diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index d6b2f3cfad..a16951b378 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -1,8 +1,8 @@ +import json import logging from collections.abc import Callable, Mapping from typing import Any, Literal, TypeVar -import json import requests from fastapi import status from observability_utils.tracing import ( @@ -108,9 +108,10 @@ class UnknownPlanError(Exception): def _exception(response: requests.Response) -> Exception | None: try: response.json() - except json.decoder.JSONDecodeError as je: + except json.decoder.JSONDecodeError: return BlueskyRemoteControlError( - "Response does not contain a valid JSON object") + "Response does not contain a valid JSON object" + ) code = response.status_code if code < 400: return None From 2d64ab735d71af776d930e0c98875910d8814a89 Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Thu, 9 Apr 2026 14:23:34 +0100 Subject: [PATCH 05/10] rework after guidance from Peter --- src/blueapi/cli/cli.py | 17 +++++++---------- src/blueapi/client/rest.py | 13 +++++++------ tests/unit_tests/cli/test_cli.py | 2 +- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 28ef04d527..86a2cd5615 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -513,16 +513,13 @@ def login(obj: dict) -> None: print("Logged in") except Exception: client = BlueapiClient.from_config(config) - try: - if oidc := client.oidc_config: - auth = SessionManager( - oidc, cache_manager=SessionCacheManager(config.auth_token_path) - ) - auth.start_device_flow() - else: - print("Server is not configured to use authentication!") - except BlueskyRemoteControlError: - print("Error thrown whilst checking authentication") + if oidc := client.oidc_config: + auth = SessionManager( + oidc, cache_manager=SessionCacheManager(config.auth_token_path) + ) + auth.start_device_flow() + else: + print("Server is not configured to use authentication!") @main.command(name="logout") diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index a16951b378..e88e4e966e 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -106,13 +106,14 @@ class UnknownPlanError(Exception): def _exception(response: requests.Response) -> Exception | None: - try: - response.json() - except json.decoder.JSONDecodeError: - return BlueskyRemoteControlError( - "Response does not contain a valid JSON object" - ) code = response.status_code + if response.content: + try: + response.json() + except json.decoder.JSONDecodeError: + return BlueskyRemoteControlError( + "Response does not contain a valid JSON object" + ) if code < 400: return None elif code == 404: diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 67d22ad6a8..35bc830ae6 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1134,7 +1134,7 @@ def test_login_with_unauthenticated_server( mock_unauthenticated_server: responses.RequestsMock, ): result = runner.invoke(main, ["-c", config_with_auth, "login"]) - assert "Error thrown whilst checking authentication\n" == result.output + assert "Server is not configured to use authentication!\n" == result.output assert result.exit_code == 0 From 4d9f07d4a05e756360cfe18915e51715d69a64a6 Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Fri, 10 Apr 2026 11:56:21 +0100 Subject: [PATCH 06/10] adding test --- src/blueapi/cli/cli.py | 17 ++++++++++------- tests/unit_tests/cli/test_cli.py | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 86a2cd5615..3651193a61 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -513,13 +513,16 @@ def login(obj: dict) -> None: print("Logged in") except Exception: client = BlueapiClient.from_config(config) - if oidc := client.oidc_config: - auth = SessionManager( - oidc, cache_manager=SessionCacheManager(config.auth_token_path) - ) - auth.start_device_flow() - else: - print("Server is not configured to use authentication!") + try: + if oidc := client.oidc_config: + auth = SessionManager( + oidc, cache_manager=SessionCacheManager(config.auth_token_path) + ) + auth.start_device_flow() + else: + print("Server is not configured to use authentication!") + except BlueskyRemoteControlError as e: + print(str(e)) @main.command(name="logout") diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 35bc830ae6..d418754ed8 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1138,6 +1138,26 @@ def test_login_with_unauthenticated_server( assert result.exit_code == 0 +@responses.activate +def test_invalid_json( + runner: CliRunner, + config_with_auth: str, + mock_authn_server: responses.RequestsMock, +): + response = responses.add( + responses.GET, + "http://localhost:8000/config/oidc", + body="blah blah", + status=200, + ) + + result = runner.invoke(main, ["-c", config_with_auth, "login"]) + + assert response.call_count == 1 + assert "Response does not contain a valid JSON object\n" == result.output + assert result.exit_code == 0 + + def test_logout_success( runner: CliRunner, config_with_auth: str, From 55f732f6217613c1c71e3ea899b62e1975be183c Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Mon, 13 Apr 2026 09:38:38 +0100 Subject: [PATCH 07/10] updates after code review --- src/blueapi/client/rest.py | 24 ++++++++++++++---------- tests/unit_tests/cli/test_cli.py | 20 -------------------- 2 files changed, 14 insertions(+), 30 deletions(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index e88e4e966e..27a6e36e12 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -107,17 +107,10 @@ class UnknownPlanError(Exception): def _exception(response: requests.Response) -> Exception | None: code = response.status_code - if response.content: - try: - response.json() - except json.decoder.JSONDecodeError: - return BlueskyRemoteControlError( - "Response does not contain a valid JSON object" - ) if code < 400: return None elif code == 404: - return KeyError(str(response.json())) + return KeyError(str(_response_json(response))) else: return BlueskyRemoteControlError(code, str(response)) @@ -132,7 +125,7 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: return UnknownPlanError() elif code == 422: try: - content = response.json() + content = _response_json(response) return InvalidParametersError( TypeAdapter(list[ParameterError]).validate_python( content.get("detail", []) @@ -146,6 +139,15 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: return BlueskyRequestError(code, response.text) +def _response_json(response) -> Any: + try: + return response.json() + except json.decoder.JSONDecodeError: + return BlueskyRemoteControlError( + "Response does not contain a valid JSON object" + ) + + class BlueapiRestClient: _config: RestConfig _session_manager: SessionManager | None @@ -294,7 +296,9 @@ def _request_and_deserialize( f"but client version is {client_version}. " f"Some features may not work as expected." ) - deserialized = TypeAdapter(target_type).validate_python(response.json()) + deserialized = TypeAdapter(target_type).validate_python( + _response_json(response) + ) return deserialized diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index d418754ed8..35bc830ae6 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1138,26 +1138,6 @@ def test_login_with_unauthenticated_server( assert result.exit_code == 0 -@responses.activate -def test_invalid_json( - runner: CliRunner, - config_with_auth: str, - mock_authn_server: responses.RequestsMock, -): - response = responses.add( - responses.GET, - "http://localhost:8000/config/oidc", - body="blah blah", - status=200, - ) - - result = runner.invoke(main, ["-c", config_with_auth, "login"]) - - assert response.call_count == 1 - assert "Response does not contain a valid JSON object\n" == result.output - assert result.exit_code == 0 - - def test_logout_success( runner: CliRunner, config_with_auth: str, From 889eafb615f14f874d53be057d2a9d0966312cf5 Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Mon, 13 Apr 2026 10:53:30 +0100 Subject: [PATCH 08/10] updating tests --- src/blueapi/client/rest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 27a6e36e12..7e4806f045 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -142,10 +142,10 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: def _response_json(response) -> Any: try: return response.json() - except json.decoder.JSONDecodeError: - return BlueskyRemoteControlError( + except json.decoder.JSONDecodeError as exc: + raise BlueskyRemoteControlError( "Response does not contain a valid JSON object" - ) + ) from exc class BlueapiRestClient: From 51f99f3646da993786e04119d9d3cc524e133cff Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Mon, 13 Apr 2026 11:10:42 +0100 Subject: [PATCH 09/10] re-adding test that went missing --- tests/unit_tests/cli/test_cli.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 35bc830ae6..cbde517414 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1138,6 +1138,26 @@ def test_login_with_unauthenticated_server( assert result.exit_code == 0 +@responses.activate +def test_invalid_json( + runner: CliRunner, + config_with_auth: str, + mock_authn_server: responses.RequestsMock, +): + response = responses.add( + responses.GET, + "http://localhost:8000/config/oidc", + body="blah blah", + status=404, + ) + + result = runner.invoke(main, ["-c", config_with_auth, "login"]) + + assert response.call_count == 1 + assert "Response does not contain a valid JSON object\n" == result.output + assert result.exit_code == 0 + + def test_logout_success( runner: CliRunner, config_with_auth: str, From a6ad5175ff4a9856835367988e2ebacecb129bde Mon Sep 17 00:00:00 2001 From: NeilSmithDLS Date: Mon, 13 Apr 2026 14:49:21 +0100 Subject: [PATCH 10/10] updates after second review --- src/blueapi/cli/cli.py | 3 ++- src/blueapi/client/rest.py | 11 +++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 3651193a61..bd2154f0a1 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -26,6 +26,7 @@ from blueapi.client.rest import ( BlueskyRemoteControlError, InvalidParametersError, + NonJsonResponseError, ServiceUnavailableError, UnauthorisedAccessError, UnknownPlanError, @@ -521,7 +522,7 @@ def login(obj: dict) -> None: auth.start_device_flow() else: print("Server is not configured to use authentication!") - except BlueskyRemoteControlError as e: + except NonJsonResponseError as e: print(str(e)) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 7e4806f045..6caa0add20 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -46,6 +46,10 @@ class BlueskyRemoteControlError(Exception): pass +class NonJsonResponseError(Exception): + pass + + class BlueskyRequestError(Exception): def __init__(self, code: int, message: str) -> None: super().__init__(message, code) @@ -139,11 +143,14 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: return BlueskyRequestError(code, response.text) -def _response_json(response) -> Any: +def _response_json(response: requests.Response) -> Any: try: return response.json() except json.decoder.JSONDecodeError as exc: - raise BlueskyRemoteControlError( + LOGGER.debug( + f"Invalid json response from <{response.request.url}>: <{response.content}>" + ) + raise NonJsonResponseError( "Response does not contain a valid JSON object" ) from exc