From 85719a71943573b1e0131033d707ec0fbd46d6b1 Mon Sep 17 00:00:00 2001 From: Dinesh Bhor Date: Wed, 4 Jan 2017 14:22:18 +0530 Subject: [PATCH] Fix error messages are not displayed correctly Error messages are not displayed correctly for segment create and host create commands. 'response' object has the actual error message returned from service in 'text' attribute. This patch saves the exception message returned from the service to 'details' attribute so that it can be displayed properly to the user. Closes-Bug: #1656826 Change-Id: I6d728abbcc5e914b5bd025bc4e059cdcb13e2109 --- openstack/exceptions.py | 37 +++++++++ openstack/session.py | 13 +-- openstack/tests/unit/test_session.py | 114 ++++++++++++++++++++++++--- 3 files changed, 139 insertions(+), 25 deletions(-) diff --git a/openstack/exceptions.py b/openstack/exceptions.py index ff3176fd..9fa8c87f 100644 --- a/openstack/exceptions.py +++ b/openstack/exceptions.py @@ -16,6 +16,8 @@ Exception definitions. """ +import re + import six @@ -108,3 +110,38 @@ class ResourceTimeout(SDKException): class ResourceFailure(SDKException): """General resource failure.""" pass + + +def from_exception(exc): + """Return an instance of an HTTPException based on httplib response.""" + if exc.response.status_code == 404: + cls = NotFoundException + else: + cls = HttpException + + resp = exc.response + details = resp.text + resp_body = resp.content + content_type = resp.headers.get('content-type', '') + if resp_body and 'application/json' in content_type: + # Iterate over the nested objects to retrieve "message" attribute. + messages = [obj.get('message') for obj in resp.json().values()] + # Join all of the messages together nicely and filter out any objects + # that don't have a "message" attr. + details = '\n'.join(msg for msg in messages if msg) + + elif resp_body and 'text/html' in content_type: + # Split the lines, strip whitespace and inline HTML from the response. + details = [re.sub(r'<.+?>', '', i.strip()) + for i in details.splitlines()] + details = [msg for msg in details if msg] + # Remove duplicates from the list. + details_temp = [] + for detail in details: + if detail not in details_temp: + details_temp.append(detail) + # Return joined string separated by colons. + details = ': '.join(details_temp) + return cls(details=details, message=exc.message, response=exc.response, + request_id=exc.request_id, url=exc.url, method=exc.method, + http_status=exc.http_status, cause=exc) diff --git a/openstack/session.py b/openstack/session.py index 5b087fcf..3d3323cc 100644 --- a/openstack/session.py +++ b/openstack/session.py @@ -63,18 +63,7 @@ def map_exceptions(func): try: return func(*args, **kwargs) except _exceptions.HttpError as e: - if e.http_status == 404: - raise exceptions.NotFoundException( - message=e.message, details=e.details, - response=e.response, request_id=e.request_id, - url=e.url, method=e.method, - http_status=e.http_status, cause=e) - else: - raise exceptions.HttpException( - message=e.message, details=e.details, - response=e.response, request_id=e.request_id, - url=e.url, method=e.method, - http_status=e.http_status, cause=e) + raise exceptions.from_exception(e) except _exceptions.ClientException as e: raise exceptions.SDKException(message=e.message, cause=e) diff --git a/openstack/tests/unit/test_session.py b/openstack/tests/unit/test_session.py index 89e8f452..a3bdb74c 100644 --- a/openstack/tests/unit/test_session.py +++ b/openstack/tests/unit/test_session.py @@ -20,6 +20,17 @@ from openstack import profile from openstack import session from openstack import utils +HTML_MSG = """ + + 404 Entity Not Found + + +

404 Entity Not Found

+ Entity could not be found +

+ +""" + class TestSession(testtools.TestCase): @@ -60,27 +71,104 @@ class TestSession(testtools.TestCase): self.assertEqual({}, sot.additional_headers) - def test_map_exceptions_not_found_exception(self): - ksa_exc = _exceptions.HttpError(message="test", http_status=404) - func = mock.Mock(side_effect=ksa_exc) - + def _assert_map_exceptions(self, expected_exc, ksa_exc, func): os_exc = self.assertRaises( - exceptions.NotFoundException, session.map_exceptions(func)) - self.assertIsInstance(os_exc, exceptions.NotFoundException) + expected_exc, session.map_exceptions(func)) + self.assertIsInstance(os_exc, expected_exc) self.assertEqual(ksa_exc.message, os_exc.message) self.assertEqual(ksa_exc.http_status, os_exc.http_status) self.assertEqual(ksa_exc, os_exc.cause) + return os_exc + + def test_map_exceptions_not_found_exception(self): + response = mock.Mock() + response_body = {'NotFoundException': { + 'message': 'Resource not found'}} + response.json = mock.Mock(return_value=response_body) + response.headers = {"content-type": "application/json"} + response.status_code = 404 + ksa_exc = _exceptions.HttpError( + message="test", http_status=404, response=response) + func = mock.Mock(side_effect=ksa_exc) + os_exc = self._assert_map_exceptions( + exceptions.NotFoundException, ksa_exc, func) + self.assertEqual('Resource not found', os_exc.details) def test_map_exceptions_http_exception(self): - ksa_exc = _exceptions.HttpError(message="test", http_status=400) + response = mock.Mock() + response_body = {'HTTPBadRequest': { + 'message': 'request is invalid'}} + response.json = mock.Mock(return_value=response_body) + response.headers = {"content-type": "application/json"} + response.status_code = 400 + ksa_exc = _exceptions.HttpError( + message="test", http_status=400, response=response) func = mock.Mock(side_effect=ksa_exc) - os_exc = self.assertRaises( - exceptions.HttpException, session.map_exceptions(func)) - self.assertIsInstance(os_exc, exceptions.HttpException) - self.assertEqual(ksa_exc.message, os_exc.message) - self.assertEqual(ksa_exc.http_status, os_exc.http_status) - self.assertEqual(ksa_exc, os_exc.cause) + os_exc = self._assert_map_exceptions( + exceptions.HttpException, ksa_exc, func) + self.assertEqual('request is invalid', os_exc.details) + + def test_map_exceptions_http_exception_handle_json(self): + mock_resp = mock.Mock() + mock_resp.status_code = 413 + mock_resp.json.return_value = { + "overLimit": { + "message": "OverLimit413...", + "retryAt": "2017-01-03T13:33:06Z" + }, + "overLimitRetry": { + "message": "OverLimit Retry...", + "retryAt": "2017-01-03T13:33:06Z" + } + } + mock_resp.headers = { + "content-type": "application/json" + } + ksa_exc = _exceptions.HttpError( + message="test", http_status=413, response=mock_resp) + func = mock.Mock(side_effect=ksa_exc) + + os_exc = self._assert_map_exceptions( + exceptions.HttpException, ksa_exc, func) + # It's not sure that which 'message' will be first so exact checking is + # difficult here. It can be 'OverLimit413...\nOverLimit Retry...' or + # it can be 'OverLimit Retry...\nOverLimit413...'. + self.assertIn('OverLimit413...', os_exc.details) + self.assertIn('OverLimit Retry...', os_exc.details) + + def test_map_exceptions_notfound_exception_handle_html(self): + mock_resp = mock.Mock() + mock_resp.status_code = 404 + mock_resp.text = HTML_MSG + mock_resp.headers = { + "content-type": "text/html" + } + ksa_exc = _exceptions.HttpError( + message="test", http_status=404, response=mock_resp) + func = mock.Mock(side_effect=ksa_exc) + + os_exc = self._assert_map_exceptions( + exceptions.NotFoundException, ksa_exc, func) + self.assertEqual('404 Entity Not Found: Entity could not be found', + os_exc.details) + + def test_map_exceptions_notfound_exception_handle_other_content_type(self): + mock_resp = mock.Mock() + mock_resp.status_code = 404 + fake_text = ("{'UnknownException': {'message': " + "'UnknownException occurred...'}}") + mock_resp.text = fake_text + mock_resp.headers = { + "content-type": 'application/octet-stream' + } + ksa_exc = _exceptions.HttpError( + message="test", http_status=404, response=mock_resp) + func = mock.Mock(side_effect=ksa_exc) + + os_exc = self._assert_map_exceptions( + exceptions.NotFoundException, ksa_exc, func) + self.assertEqual(fake_text, os_exc.details) def test_map_exceptions_sdk_exception_1(self): ksa_exc = _exceptions.ClientException()