From 7d92a0869a9c61f77e17e526099e0c209e412f5a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 29 Mar 2022 14:18:00 +0100 Subject: [PATCH] Remove per-client SMS exception classes In response to: [^1]. The stacktrace conveys the same and more information. We don't do anything different for each exception class, so there's no value in having three of them over one exception. I did think about DRYing-up the duplicate exception behaviour into the base class one. This isn't ideal because the base class would be making assumptions about how inheriting classes make requests, which might change with future providers. Although it might be nice to have more info in the top-level message, we'll still get it in the stacktrace e.g. ValueError: Expected 'code' to be '0' During handling of the above exception, another exception occurred: app.clients.sms.SmsClientResponseException: SMS client error (Invalid response JSON) requests.exceptions.ReadTimeout During handling of the above exception, another exception occurred: app.clients.sms.SmsClientResponseException: SMS client error (Request failed) [^1]: https://github.com/alphagov/notifications-api/pull/3493#discussion_r837363717 --- app/clients/sms/__init__.py | 2 +- app/clients/sms/firetext.py | 22 +++++----------------- app/clients/sms/mmg.py | 8 ++++---- app/clients/sms/reach.py | 21 ++++----------------- tests/app/clients/test_firetext.py | 24 +++++++----------------- tests/app/clients/test_mmg.py | 23 +++++++---------------- tests/app/clients/test_reach.py | 21 ++++++--------------- 7 files changed, 34 insertions(+), 87 deletions(-) diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 3a2ba1367..0aca5b58c 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -12,7 +12,7 @@ class SmsClientResponseException(ClientException): self.message = message def __str__(self): - return "Message {}".format(self.message) + return f"SMS client error ({self.message})" class SmsClient(Client): diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index f2336f2aa..0d8f5a685 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -44,18 +44,6 @@ def get_message_status_and_reason_from_firetext_code(detailed_status_code): return firetext_codes[detailed_status_code]['status'], firetext_codes[detailed_status_code]['reason'] -class FiretextClientResponseException(SmsClientResponseException): - def __init__(self, response, exception): - status_code = response.status_code if response is not None else 504 - text = response.text if response is not None else "Gateway Time-out" - self.status_code = status_code - self.text = text - self.exception = exception - - def __str__(self): - return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception)) - - class FiretextClient(SmsClient): ''' FireText sms client. @@ -91,10 +79,10 @@ class FiretextClient(SmsClient): try: json.loads(response.text) if response.json()['code'] != 0: - raise ValueError() - except (ValueError, AttributeError) as e: - raise FiretextClientResponseException(response=response, exception=e) - except RequestException as e: - raise FiretextClientResponseException(response=e.response, exception=e) + raise ValueError("Expected 'code' to be '0'") + except (ValueError, AttributeError): + raise SmsClientResponseException("Invalid response JSON") + except RequestException: + raise SmsClientResponseException("Request failed") return response diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index a1408a16e..b05c275ea 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -102,9 +102,9 @@ class MMGClient(SmsClient): response.raise_for_status() try: json.loads(response.text) - except (ValueError, AttributeError) as e: - raise MMGClientResponseException(response=response, exception=e) - except RequestException as e: - raise MMGClientResponseException(response=e.response, exception=e) + except (ValueError, AttributeError): + raise SmsClientResponseException("Invalid response JSON") + except RequestException: + raise SmsClientResponseException("Request failed") return response diff --git a/app/clients/sms/reach.py b/app/clients/sms/reach.py index ad4eee17f..3541f8b5c 100644 --- a/app/clients/sms/reach.py +++ b/app/clients/sms/reach.py @@ -16,19 +16,6 @@ def get_reach_responses(status, detailed_status_code=None): raise KeyError -class ReachClientResponseException(SmsClientResponseException): - def __init__(self, response, exception): - status_code = response.status_code if response is not None else 504 - text = response.text if response is not None else "Gateway Time-out" - - self.status_code = status_code - self.text = text - self.exception = exception - - def __str__(self): - return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception)) - - class ReachClient(SmsClient): def init_app(self, *args, **kwargs): super().init_app(*args, **kwargs) @@ -57,9 +44,9 @@ class ReachClient(SmsClient): response.raise_for_status() try: json.loads(response.text) - except (ValueError, AttributeError) as e: - raise ReachClientResponseException(response=response, exception=e) - except RequestException as e: - raise ReachClientResponseException(response=e.response, exception=e) + except (ValueError, AttributeError): + raise SmsClientResponseException("Invalid response JSON") + except RequestException: + raise SmsClientResponseException("Request failed") return response diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 089a61fc0..75febf605 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -2,11 +2,9 @@ from urllib.parse import parse_qs import pytest import requests_mock -from requests import HTTPError from requests.exceptions import ConnectTimeout, ReadTimeout from app.clients.sms.firetext import ( - FiretextClientResponseException, SmsClientResponseException, get_firetext_responses, ) @@ -116,9 +114,7 @@ def test_try_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): request_mock.post('https://example.com/firetext', json=response_dict, status_code=200) mock_firetext_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 200 - assert '"description": "Some kind of error"' in exc.value.text - assert '"code": 1' in exc.value.text + assert "Invalid response JSON" in str(exc.value) def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mock_firetext_client): @@ -129,9 +125,7 @@ def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mo request_mock.post('https://example.com/firetext', json=response_dict, status_code=400) mock_firetext_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 400 - assert exc.value.text == '{"something": "gone bad"}' - assert type(exc.value.exception) == HTTPError + assert "Request failed" in str(exc.value) def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_firetext_client): @@ -142,28 +136,24 @@ def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_f request_mock.post('https://example.com/firetext', text=response_dict, status_code=200) mock_firetext_client.try_send_sms(to, content, reference, False, 'sender') - assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value) # noqa - assert exc.value.status_code == 200 - assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' + assert "Invalid response JSON" in str(exc.value) def test_try_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client): to = content = reference = 'foo' - with pytest.raises(FiretextClientResponseException) as exc: + with pytest.raises(SmsClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/firetext', exc=ConnectTimeout) mock_firetext_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) def test_try_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firetext_client): to = content = reference = 'foo' - with pytest.raises(FiretextClientResponseException) as exc: + with pytest.raises(SmsClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/firetext', exc=ReadTimeout) mock_firetext_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 0164af452..767701ed9 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -1,11 +1,9 @@ import pytest import requests_mock -from requests import HTTPError from requests.exceptions import ConnectTimeout, ReadTimeout from app import mmg_client -from app.clients.sms import SmsClientResponseException -from app.clients.sms.mmg import MMGClientResponseException, get_mmg_responses +from app.clients.sms.mmg import SmsClientResponseException, get_mmg_responses @pytest.mark.parametrize('detailed_status_code, result', [ @@ -85,10 +83,7 @@ def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker): request_mock.post('https://example.com/mmg', json=response_dict, status_code=400) mmg_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 400 - assert '"Error": 206' in exc.value.text - assert '"Description": "Some kind of error"' in exc.value.text - assert type(exc.value.exception) == HTTPError + assert "Request failed" in str(exc.value) def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): @@ -99,28 +94,24 @@ def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): request_mock.post('https://example.com/mmg', text=response_dict, status_code=200) mmg_client.try_send_sms(to, content, reference, False, 'sender') - assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value) # noqa - assert exc.value.status_code == 200 - assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' + assert "Invalid response JSON" in str(exc.value) def test_try_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock): to = content = reference = 'foo' - with pytest.raises(MMGClientResponseException) as exc: + with pytest.raises(SmsClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/mmg', exc=ConnectTimeout) mmg_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) def test_try_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock): to = content = reference = 'foo' - with pytest.raises(MMGClientResponseException) as exc: + with pytest.raises(SmsClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/mmg', exc=ReadTimeout) mmg_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) diff --git a/tests/app/clients/test_reach.py b/tests/app/clients/test_reach.py index 7cc8b2bc3..b62a5fd0b 100644 --- a/tests/app/clients/test_reach.py +++ b/tests/app/clients/test_reach.py @@ -1,11 +1,9 @@ import pytest import requests_mock -from requests import HTTPError from requests.exceptions import ConnectTimeout, ReadTimeout from app import reach_client from app.clients.sms import SmsClientResponseException -from app.clients.sms.reach import ReachClientResponseException # TODO: tests for get_reach_responses @@ -52,10 +50,7 @@ def test_try_send_sms_raises_if_reach_rejects(notify_api, mocker): request_mock.post('https://example.com/reach', json=response_dict, status_code=400) reach_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 400 - assert '"Error": 206' in exc.value.text - assert '"Description": "Some kind of error"' in exc.value.text - assert type(exc.value.exception) == HTTPError + assert "Request failed" in str(exc) def test_try_send_sms_raises_if_reach_fails_to_return_json(notify_api, mocker): @@ -66,28 +61,24 @@ def test_try_send_sms_raises_if_reach_fails_to_return_json(notify_api, mocker): request_mock.post('https://example.com/reach', text=response_dict, status_code=200) reach_client.try_send_sms(to, content, reference, False, 'sender') - assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value) # noqa - assert exc.value.status_code == 200 - assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' + assert 'Invalid response JSON' in str(exc.value) def test_try_send_sms_raises_if_reach_rejects_with_connect_timeout(rmock): to = content = reference = 'foo' - with pytest.raises(ReachClientResponseException) as exc: + with pytest.raises(SmsClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/reach', exc=ConnectTimeout) reach_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert 'Request failed' in str(exc.value) def test_try_send_sms_raises_if_reach_rejects_with_read_timeout(rmock): to = content = reference = 'foo' - with pytest.raises(ReachClientResponseException) as exc: + with pytest.raises(SmsClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/reach', exc=ReadTimeout) reach_client.try_send_sms(to, content, reference, False, 'sender') - assert exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert 'Request failed' in str(exc.value)