From 01caf4e734fb012f0b5b19c24a8ffeaef31a06be Mon Sep 17 00:00:00 2001 From: d34dh0r53 Date: Mon, 19 Jan 2015 18:02:08 -0600 Subject: [PATCH] Strip json and html from error messages Error messages were being passed with either JSON or HTML formatting leading to an unpleasant user experience. This strips the JSON or HTML tags and presents the clean error message to the user. Rewrote the lispy function to be more pythonic, added test cases and cleaned up some pep8 violations. Removed commented out cruft from a previous version of this patch. Changed the HTML details from a set to a list in order to ensure the ordering of the exception details. Added code to eliminate the duplicate detail messages from the list and reordered the assertion string to match actual output. Refactored duplicate elimination code to be more readable and reliable. Some reworking of the duplication elimination loop to make it more readable. Removed unneeded conditional to filter out empty elements. Change-Id: I79985b3e305cb30328a3c16b025315a8e969243d Closes-Bug: 1398838 --- glanceclient/exc.py | 27 +++++++++++++++++++++++++-- glanceclient/shell.py | 26 ++++++++++++++------------ tests/test_exc.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 14 deletions(-) diff --git a/glanceclient/exc.py b/glanceclient/exc.py index 3eeaffa1..95eb575f 100644 --- a/glanceclient/exc.py +++ b/glanceclient/exc.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import re import sys @@ -141,7 +142,7 @@ class HTTPServiceUnavailable(ServiceUnavailable): pass -#NOTE(bcwaldon): Build a mapping of HTTP codes to corresponding exception +# NOTE(bcwaldon): Build a mapping of HTTP codes to corresponding exception # classes _code_map = {} for obj_name in dir(sys.modules[__name__]): @@ -153,7 +154,29 @@ for obj_name in dir(sys.modules[__name__]): def from_response(response, body=None): """Return an instance of an HTTPException based on httplib response.""" cls = _code_map.get(response.status_code, HTTPException) - if body: + if body and 'json' in response.headers['content-type']: + # Iterate over the nested objects and retreive the "message" attribute. + messages = [obj.get('message') for obj in response.json().values()] + # Join all of the messages together nicely and filter out any objects + # that don't have a "message" attr. + details = '\n'.join(i for i in messages if i is not None) + return cls(details=details) + elif body and 'html' in response.headers['content-type']: + # Split the lines, strip whitespace and inline HTML from the response. + details = [re.sub(r'<.+?>', '', i.strip()) + for i in response.text.splitlines()] + details = [i for i in details if i] + # Remove duplicates from the list. + details_seen = set() + details_temp = [] + for i in details: + if i not in details_seen: + details_temp.append(i) + details_seen.add(i) + # Return joined string separated by colons. + details = ': '.join(details_temp) + return cls(details=details) + elif body: details = body.replace('\n\n', '\n') return cls(details=details) diff --git a/glanceclient/shell.py b/glanceclient/shell.py index 4eb174eb..4b5b73d2 100644 --- a/glanceclient/shell.py +++ b/glanceclient/shell.py @@ -477,14 +477,16 @@ class OpenStackImagesShell(object): "or prompted response")) # Validate password flow auth - project_info = (args.os_tenant_name or - args.os_tenant_id or - (args.os_project_name and - (args.os_project_domain_name or - args.os_project_domain_id)) or - args.os_project_id) + project_info = ( + args.os_tenant_name or args.os_tenant_id or ( + args.os_project_name and ( + args.os_project_domain_name or + args.os_project_domain_id + ) + ) or args.os_project_id + ) - if (not project_info): + if not project_info: # tenant is deprecated in Keystone v3. Use the latest # terminology instead. raise exc.CommandError( @@ -571,14 +573,14 @@ class OpenStackImagesShell(object): with open(schema_file_path, 'w') as f: f.write(json.dumps(schema.raw())) except Exception: - #NOTE(esheffield) do nothing here, we'll get a message - #later if the schema is missing + # NOTE(esheffield) do nothing here, we'll get a message + # later if the schema is missing pass def main(self, argv): # Parse args once to find version - #NOTE(flepied) Under Python3, parsed arguments are removed + # NOTE(flepied) Under Python3, parsed arguments are removed # from the list so make a copy for the first parsing base_argv = copy.deepcopy(argv) parser = self.get_base_parser() @@ -642,8 +644,8 @@ class OpenStackImagesShell(object): except exc.Unauthorized: raise exc.CommandError("Invalid OpenStack Identity credentials.") except Exception: - #NOTE(kragniz) Print any exceptions raised to stderr if the --debug - # flag is set + # NOTE(kragniz) Print any exceptions raised to stderr if the + # --debug flag is set if args.debug: traceback.print_exc() raise diff --git a/tests/test_exc.py b/tests/test_exc.py index c8ad2dfc..575c62b5 100644 --- a/tests/test_exc.py +++ b/tests/test_exc.py @@ -17,6 +17,17 @@ import testtools from glanceclient import exc +HTML_MSG = """ + + 404 Entity Not Found + + +

404 Entity Not Found

+ Entity could not be found +

+ +""" + class TestHTTPExceptions(testtools.TestCase): def test_from_response(self): @@ -25,3 +36,35 @@ class TestHTTPExceptions(testtools.TestCase): mock_resp.status_code = 400 out = exc.from_response(mock_resp) self.assertIsInstance(out, exc.HTTPBadRequest) + + def test_handles_json(self): + """exc.from_response should not print JSON.""" + mock_resp = mock.Mock() + mock_resp.status_code = 413 + mock_resp.json.return_value = { + "overLimit": { + "code": 413, + "message": "OverLimit Retry...", + "details": "Error Details...", + "retryAt": "2014-12-03T13:33:06Z" + } + } + mock_resp.headers = { + "content-type": "application/json" + } + err = exc.from_response(mock_resp, "Non-empty body") + self.assertIsInstance(err, exc.HTTPOverLimit) + self.assertEqual("OverLimit Retry...", err.details) + + def test_handles_html(self): + """exc.from_response should not print HTML.""" + mock_resp = mock.Mock() + mock_resp.status_code = 404 + mock_resp.text = HTML_MSG + mock_resp.headers = { + "content-type": "text/html" + } + err = exc.from_response(mock_resp, HTML_MSG) + self.assertIsInstance(err, exc.HTTPNotFound) + self.assertEqual("404 Entity Not Found: Entity could not be found", + err.details)