Repository: boto/botocore
Pull request #799 (2016-02-18 22:03:24)

Handle 504 error from cloudsearch
This pulls in https://github.com/boto/botocore/pull/731 with a few changes:

* Move up common functionality into base class.  The logic for parsing a JSON body, handling encoding, and defaulting to an empty dict when no body is present is now consolidated to a single base class method.
* Group all error tests into a single class.  The eventual goal is to pull these into the shared protocol tests,.  Grouping them into a single test class makes this easier.

It's hard to see the new test because I've grouped all the error tests together, but the test case for this bug fix is here: https://github.com/boto/botocore/pull/799/files#diff-16d168f91b157ec88dcddaf854150629R503

Closes #731.

cc @kyleknap @rayluo @JordonPhillips 


Patch:
diff --git a/botocore/parsers.py b/botocore/parsers.py
index 67efe6001..9b9e1f197 100644
--- a/botocore/parsers.py
+++ b/botocore/parsers.py
@@ -507,18 +507,18 @@ class BaseJSONParser(ResponseParser):
         return self._timestamp_parser(value)
 
     def _do_error_parse(self, response, shape):
-        body = json.loads(response['body'].decode(self.DEFAULT_ENCODING))
-        error = {"Error": {}, "ResponseMetadata": {}}
+        body = self._parse_body_as_json(response['body'])
+        error = {"Error": {"Message": '', "Code": ''}, "ResponseMetadata": {}}
         # Error responses can have slightly different structures for json.
         # The basic structure is:
         #
         # {"__type":"ConnectClientException",
         #  "message":"The error message."}
-        # Code, Type
 
         # The error message can either come in the 'message' or 'Message' key
         # so we need to check for both.
-        error['Error']['Message'] = body.get('message', body.get('Message'))
+        error['Error']['Message'] = body.get('message',
+                                             body.get('Message', ''))
         code = body.get('__type')
         if code is not None:
             # code has a couple forms as well:
@@ -535,6 +535,13 @@ class BaseJSONParser(ResponseParser):
             parsed.setdefault('ResponseMetadata', {})['RequestId'] = (
                 headers['x-amzn-requestid'])
 
+    def _parse_body_as_json(self, body_contents):
+        if not body_contents:
+            return {}
+        body = body_contents.decode(self.DEFAULT_ENCODING)
+        original_parsed = json.loads(body)
+        return original_parsed
+
 
 class JSONParser(BaseJSONParser):
     """Response parse for the "json" protocol."""
@@ -544,8 +551,7 @@ class JSONParser(BaseJSONParser):
         # to richer types (blobs, timestamps, etc.
         parsed = {}
         if shape is not None:
-            body = response['body'].decode(self.DEFAULT_ENCODING)
-            original_parsed = json.loads(body)
+            original_parsed = self._parse_body_as_json(response['body'])
             parsed = self._parse_shape(shape, original_parsed)
         self._inject_response_metadata(parsed, response['headers'])
         return parsed
@@ -643,17 +649,17 @@ class BaseRestParser(ResponseParser):
 class RestJSONParser(BaseRestParser, BaseJSONParser):
 
     def _initial_body_parse(self, body_contents):
-        if not body_contents:
-            return {}
-        body = body_contents.decode(self.DEFAULT_ENCODING)
-        original_parsed = json.loads(body)
-        return original_parsed
+        return self._parse_body_as_json(body_contents)
 
     def _do_error_parse(self, response, shape):
-        body = self._initial_body_parse(response['body'])
         error = super(RestJSONParser, self)._do_error_parse(response, shape)
-        error['Error']['Message'] = body.get('message',
-                                             body.get('Message', ''))
+        self._inject_error_code(error, response)
+        return error
+
+    def _inject_error_code(self, error, response):
+        # The "Code" value can come from either a response
+        # header or a value in the JSON body.
+        body = self._initial_body_parse(response['body'])
         if 'x-amzn-errortype' in response['headers']:
             code = response['headers']['x-amzn-errortype']
             # Could be:
@@ -663,7 +669,6 @@ class RestJSONParser(BaseRestParser, BaseJSONParser):
         elif 'code' in body or 'Code' in body:
             error['Error']['Code'] = body.get(
                 'code', body.get('Code', ''))
-        return error
 
 
 class RestXMLParser(BaseRestParser, BaseXMLResponseParser):


Test patch:
diff --git a/tests/unit/test_parsers.py b/tests/unit/test_parsers.py
index a75c1dc08..6e67fe2f2 100644
--- a/tests/unit/test_parsers.py
+++ b/tests/unit/test_parsers.py
@@ -93,47 +93,6 @@ class TestResponseMetadataParsed(unittest.TestCase):
                      'ResponseMetadata': {'RequestId': 'request-id',
                                           'HTTPStatusCode': 200}})
 
-    def test_response_metadata_errors_for_json(self):
-        parser = parsers.JSONParser()
-        response = {
-            "body": b"""
-                {"__type":"amazon.foo.validate#ValidationException",
-                 "message":"this is a message"}
-                """,
-            "status_code": 400,
-            "headers": {
-                "x-amzn-requestid": "request-id"
-            }
-        }
-        parsed = parser.parse(response, None)
-        # Even (especially) on an error condition, the
-        # ResponseMetadata should be populated.
-        self.assertIn('ResponseMetadata', parsed)
-        self.assertEqual(parsed['ResponseMetadata']['RequestId'], 'request-id')
-
-        self.assertIn('Error', parsed)
-        self.assertEqual(parsed['Error']['Message'], 'this is a message')
-        self.assertEqual(parsed['Error']['Code'], 'ValidationException')
-
-    def test_response_metadata_errors_alternate_form(self):
-        # Sometimes there is no '#' in the __type.  We need to be
-        # able to parse this error message as well.
-        parser = parsers.JSONParser()
-        response = {
-            "body": b"""
-                {"__type":"ValidationException",
-                 "message":"this is a message"}
-                """,
-            "status_code": 400,
-            "headers": {
-                "x-amzn-requestid": "request-id"
-            }
-        }
-        parsed = parser.parse(response, None)
-        self.assertIn('Error', parsed)
-        self.assertEqual(parsed['Error']['Message'], 'this is a message')
-        self.assertEqual(parsed['Error']['Code'], 'ValidationException')
-
     def test_response_metadata_on_normal_request(self):
         parser = parsers.JSONParser()
         response = b'{"Str": "mystring"}'
@@ -202,150 +161,6 @@ class TestResponseMetadataParsed(unittest.TestCase):
                                   'HostId': 'second-id',
                                   'HTTPStatusCode': 200}})
 
-    def test_s3_error_response(self):
-        body = (
-            '<Error>'
-            '  <Code>NoSuchBucket</Code>'
-            '  <Message>error message</Message>'
-            '  <BucketName>asdf</BucketName>'
-            '  <RequestId>EF1EF43A74415102</RequestId>'
-            '  <HostId>hostid</HostId>'
-            '</Error>'
-        ).encode('utf-8')
-        headers = {
-            'x-amz-id-2': 'second-id',
-            'x-amz-request-id': 'request-id'
-        }
-        parser = parsers.RestXMLParser()
-        parsed = parser.parse(
-            {'body': body, 'headers': headers, 'status_code': 400}, None)
-        self.assertIn('Error', parsed)
-        self.assertEqual(parsed['Error'], {
-            'Code': 'NoSuchBucket',
-            'Message': 'error message',
-            'BucketName': 'asdf',
-            # We don't want the RequestId/HostId because they're already
-            # present in the ResponseMetadata key.
-        })
-        self.assertEqual(parsed['ResponseMetadata'], {
-            'RequestId': 'request-id',
-            'HostId': 'second-id',
-            'HTTPStatusCode': 400,
-        })
-
-    def test_s3_error_response_with_no_body(self):
-        # If you try to HeadObject a key that does not exist,
-        # you will get an empty body.  When this happens
-        # we expect that we will use Code/Message from the
-        # HTTP status code.
-        body = ''
-        headers = {
-            'x-amz-id-2': 'second-id',
-            'x-amz-request-id': 'request-id'
-        }
-        parser = parsers.RestXMLParser()
-        parsed = parser.parse(
-            {'body': body, 'headers': headers, 'status_code': 404}, None)
-        self.assertIn('Error', parsed)
-        self.assertEqual(parsed['Error'], {
-            'Code': '404',
-            'Message': 'Not Found',
-        })
-        self.assertEqual(parsed['ResponseMetadata'], {
-            'RequestId': 'request-id',
-            'HostId': 'second-id',
-            'HTTPStatusCode': 404,
-        })
-
-    def test_can_parse_sdb_error_response(self):
-        body = (
-            '<OperationNameResponse>'
-            '    <Errors>'
-            '        <Error>'
-            '            <Code>1</Code>'
-            '            <Message>msg</Message>'
-            '        </Error>'
-            '    </Errors>'
-            '    <RequestId>abc-123</RequestId>'
-            '</OperationNameResponse>'
-        ).encode('utf-8')
-        parser = parsers.QueryParser()
-        parsed = parser.parse({
-            'body': body, 'headers': {}, 'status_code': 500}, None)
-        self.assertIn('Error', parsed)
-        self.assertEqual(parsed['Error'], {
-            'Code': '1',
-            'Message': 'msg'
-        })
-        self.assertEqual(parsed['ResponseMetadata'], {
-            'RequestId': 'abc-123',
-            'HTTPStatusCode': 500
-        })
-
-    def test_can_parse_glacier_error_response(self):
-        body = (b'{"code":"AccessDeniedException","type":"Client","message":'
-                b'"Access denied"}')
-        headers = {
-             'x-amzn-requestid': 'request-id'
-        }
-        parser = parsers.RestJSONParser()
-        parsed = parser.parse(
-            {'body': body, 'headers': headers, 'status_code': 400}, None)
-        self.assertEqual(parsed['Error'], {'Message': 'Access denied',
-                                           'Code': 'AccessDeniedException'})
-
-    def test_can_parse_restjson_error_code(self):
-        body = b'''{
-            "status": "error",
-            "errors": [{"message": "[*Deprecated*: blah"}],
-            "adds": 0,
-            "__type": "#WasUnableToParseThis",
-            "message": "blah",
-            "deletes": 0}'''
-        headers = {
-             'x-amzn-requestid': 'request-id'
-        }
-        parser = parsers.RestJSONParser()
-        parsed = parser.parse(
-            {'body': body, 'headers': headers, 'status_code': 400}, None)
-        self.assertEqual(parsed['Error'], {'Message': 'blah',
-                                           'Code': 'WasUnableToParseThis'})
-
-    def test_can_parse_with_case_insensitive_keys(self):
-        body = (b'{"Code":"AccessDeniedException","type":"Client","Message":'
-                b'"Access denied"}')
-        headers = {
-             'x-amzn-requestid': 'request-id'
-        }
-        parser = parsers.RestJSONParser()
-        parsed = parser.parse(
-            {'body': body, 'headers': headers, 'status_code': 400}, None)
-        self.assertEqual(parsed['Error'], {'Message': 'Access denied',
-                                           'Code': 'AccessDeniedException'})
-
-    def test_can_parse_route53_with_missing_message(self):
-        # The message isn't always in the XML response (or even the headers).
-        # We should be able to handle this gracefully and still at least
-        # populate a "Message" key so that consumers don't have to
-        # conditionally check for this.
-        body =  (
-            '<ErrorResponse>'
-            '  <Error>'
-            '    <Type>Sender</Type>'
-            '    <Code>InvalidInput</Code>'
-            '  </Error>'
-            '  <RequestId>id</RequestId>'
-            '</ErrorResponse>'
-        ).encode('utf-8')
-        parser = parsers.RestXMLParser()
-        parsed = parser.parse({
-            'body': body, 'headers': {}, 'status_code': 400}, None)
-        error = parsed['Error']
-        self.assertEqual(error['Code'], 'InvalidInput')
-        # Even though there's no <Message /> we should
-        # still populate an empty string.
-        self.assertEqual(error['Message'], '')
-
 
 class TestResponseParsingDatetimes(unittest.TestCase):
     def test_can_parse_float_timestamps(self):
@@ -531,3 +346,292 @@ class TestRESTXMLResponses(unittest.TestCase):
             output_shape)
         # Ensure the first element is used out of the list.
         self.assertEqual(parsed['Foo'], {'Bar': 'first_value'})
+
+
+class TestParseErrorResponses(unittest.TestCase):
+    # This class consolidates all the error parsing tests
+    # across all the protocols.  We may potentially pull
+    # this into the shared protocol tests in the future,
+    # so consolidating them into a single class will make
+    # this easier.
+    def test_response_metadata_errors_for_json_protocol(self):
+        parser = parsers.JSONParser()
+        response = {
+            "body": b"""
+                {"__type":"amazon.foo.validate#ValidationException",
+                 "message":"this is a message"}
+                """,
+            "status_code": 400,
+            "headers": {
+                "x-amzn-requestid": "request-id"
+            }
+        }
+        parsed = parser.parse(response, None)
+        # Even (especially) on an error condition, the
+        # ResponseMetadata should be populated.
+        self.assertIn('ResponseMetadata', parsed)
+        self.assertEqual(parsed['ResponseMetadata']['RequestId'], 'request-id')
+
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error']['Message'], 'this is a message')
+        self.assertEqual(parsed['Error']['Code'], 'ValidationException')
+
+    def test_response_metadata_errors_alternate_form_json_protocol(self):
+        # Sometimes there is no '#' in the __type.  We need to be
+        # able to parse this error message as well.
+        parser = parsers.JSONParser()
+        response = {
+            "body": b"""
+                {"__type":"ValidationException",
+                 "message":"this is a message"}
+                """,
+            "status_code": 400,
+            "headers": {
+                "x-amzn-requestid": "request-id"
+            }
+        }
+        parsed = parser.parse(response, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error']['Message'], 'this is a message')
+        self.assertEqual(parsed['Error']['Code'], 'ValidationException')
+
+    def test_parse_error_response_for_query_protocol(self):
+        body = (
+            '<ErrorResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/">'
+            '  <Error>'
+            '    <Type>Sender</Type>'
+            '    <Code>InvalidInput</Code>'
+            '    <Message>ARN asdf is not valid.</Message>'
+            '  </Error>'
+            '  <RequestId>request-id</RequestId>'
+            '</ErrorResponse>'
+        ).encode('utf-8')
+        parser = parsers.QueryParser()
+        parsed = parser.parse({
+            'body': body, 'headers': {}, 'status_code': 400}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': 'InvalidInput',
+            'Message': 'ARN asdf is not valid.',
+            'Type': 'Sender',
+        })
+
+    def test_can_parse_sdb_error_response_query_protocol(self):
+        body = (
+            '<OperationNameResponse>'
+            '    <Errors>'
+            '        <Error>'
+            '            <Code>1</Code>'
+            '            <Message>msg</Message>'
+            '        </Error>'
+            '    </Errors>'
+            '    <RequestId>abc-123</RequestId>'
+            '</OperationNameResponse>'
+        ).encode('utf-8')
+        parser = parsers.QueryParser()
+        parsed = parser.parse({
+            'body': body, 'headers': {}, 'status_code': 500}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': '1',
+            'Message': 'msg'
+        })
+        self.assertEqual(parsed['ResponseMetadata'], {
+            'RequestId': 'abc-123',
+            'HTTPStatusCode': 500
+        })
+
+    def test_can_parser_ec2_errors(self):
+        body = (
+            '<Response>'
+            '  <Errors>'
+            '    <Error>'
+            '      <Code>InvalidInstanceID.NotFound</Code>'
+            '      <Message>The instance ID i-12345 does not exist</Message>'
+            '    </Error>'
+            '  </Errors>'
+            '  <RequestID>06f382b0-d521-4bb6-988c-ca49d5ae6070</RequestID>'
+            '</Response>'
+        ).encode('utf-8')
+        parser = parsers.EC2QueryParser()
+        parsed = parser.parse({
+            'body': body, 'headers': {}, 'status_code': 400}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': 'InvalidInstanceID.NotFound',
+            'Message': 'The instance ID i-12345 does not exist',
+        })
+
+    def test_can_parse_rest_xml_errors(self):
+        body = (
+            '<ErrorResponse xmlns="https://route53.amazonaws.com/doc/2013-04-01/">'
+            '  <Error>'
+            '    <Type>Sender</Type>'
+            '    <Code>NoSuchHostedZone</Code>'
+            '    <Message>No hosted zone found with ID: foobar</Message>'
+            '  </Error>'
+            '  <RequestId>bc269cf3-d44f-11e5-8779-2d21c30eb3f1</RequestId>'
+            '</ErrorResponse>'
+        ).encode('utf-8')
+        parser = parsers.RestXMLParser()
+        parsed = parser.parse({
+            'body': body, 'headers': {}, 'status_code': 400}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': 'NoSuchHostedZone',
+            'Message': 'No hosted zone found with ID: foobar',
+            'Type': 'Sender',
+        })
+
+    def test_can_parse_rest_json_errors(self):
+        body = (
+            '{"Message":"Function not found: foo","Type":"User"}'
+        ).encode('utf-8')
+        headers = {
+            'x-amzn-requestid': 'request-id',
+            'x-amzn-errortype': 'ResourceNotFoundException:http://url/',
+        }
+        parser = parsers.RestJSONParser()
+        parsed = parser.parse({
+            'body': body, 'headers': headers, 'status_code': 400}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': 'ResourceNotFoundException',
+            'Message': 'Function not found: foo',
+        })
+
+    def test_error_response_with_no_body_rest_json(self):
+        parser = parsers.RestJSONParser()
+        response = b''
+        headers = {'content-length': '0', 'connection': 'keep-alive'}
+        output_shape = None
+        parsed = parser.parse({'body': response, 'headers': headers,
+                               'status_code': 504}, output_shape)
+
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': '',
+            'Message': ''
+        })
+        self.assertEqual(parsed['ResponseMetadata'], {
+            'HTTPStatusCode': 504,
+        })
+
+    def test_s3_error_response(self):
+        body = (
+            '<Error>'
+            '  <Code>NoSuchBucket</Code>'
+            '  <Message>error message</Message>'
+            '  <BucketName>asdf</BucketName>'
+            '  <RequestId>EF1EF43A74415102</RequestId>'
+            '  <HostId>hostid</HostId>'
+            '</Error>'
+        ).encode('utf-8')
+        headers = {
+            'x-amz-id-2': 'second-id',
+            'x-amz-request-id': 'request-id'
+        }
+        parser = parsers.RestXMLParser()
+        parsed = parser.parse(
+            {'body': body, 'headers': headers, 'status_code': 400}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': 'NoSuchBucket',
+            'Message': 'error message',
+            'BucketName': 'asdf',
+            # We don't want the RequestId/HostId because they're already
+            # present in the ResponseMetadata key.
+        })
+        self.assertEqual(parsed['ResponseMetadata'], {
+            'RequestId': 'request-id',
+            'HostId': 'second-id',
+            'HTTPStatusCode': 400,
+        })
+
+    def test_s3_error_response_with_no_body(self):
+        # If you try to HeadObject a key that does not exist,
+        # you will get an empty body.  When this happens
+        # we expect that we will use Code/Message from the
+        # HTTP status code.
+        body = ''
+        headers = {
+            'x-amz-id-2': 'second-id',
+            'x-amz-request-id': 'request-id'
+        }
+        parser = parsers.RestXMLParser()
+        parsed = parser.parse(
+            {'body': body, 'headers': headers, 'status_code': 404}, None)
+        self.assertIn('Error', parsed)
+        self.assertEqual(parsed['Error'], {
+            'Code': '404',
+            'Message': 'Not Found',
+        })
+        self.assertEqual(parsed['ResponseMetadata'], {
+            'RequestId': 'request-id',
+            'HostId': 'second-id',
+            'HTTPStatusCode': 404,
+        })
+
+    def test_can_parse_glacier_error_response(self):
+        body = (b'{"code":"AccessDeniedException","type":"Client","message":'
+                b'"Access denied"}')
+        headers = {
+             'x-amzn-requestid': 'request-id'
+        }
+        parser = parsers.RestJSONParser()
+        parsed = parser.parse(
+            {'body': body, 'headers': headers, 'status_code': 400}, None)
+        self.assertEqual(parsed['Error'], {'Message': 'Access denied',
+                                           'Code': 'AccessDeniedException'})
+
+    def test_can_parse_restjson_error_code(self):
+        body = b'''{
+            "status": "error",
+            "errors": [{"message": "[*Deprecated*: blah"}],
+            "adds": 0,
+            "__type": "#WasUnableToParseThis",
+            "message": "blah",
+            "deletes": 0}'''
+        headers = {
+             'x-amzn-requestid': 'request-id'
+        }
+        parser = parsers.RestJSONParser()
+        parsed = parser.parse(
+            {'body': body, 'headers': headers, 'status_code': 400}, None)
+        self.assertEqual(parsed['Error'], {'Message': 'blah',
+                                           'Code': 'WasUnableToParseThis'})
+
+    def test_can_parse_with_case_insensitive_keys(self):
+        body = (b'{"Code":"AccessDeniedException","type":"Client","Message":'
+                b'"Access denied"}')
+        headers = {
+             'x-amzn-requestid': 'request-id'
+        }
+        parser = parsers.RestJSONParser()
+        parsed = parser.parse(
+            {'body': body, 'headers': headers, 'status_code': 400}, None)
+        self.assertEqual(parsed['Error'], {'Message': 'Access denied',
+                                           'Code': 'AccessDeniedException'})
+
+    def test_can_parse_route53_with_missing_message(self):
+        # The message isn't always in the XML response (or even the headers).
+        # We should be able to handle this gracefully and still at least
+        # populate a "Message" key so that consumers don't have to
+        # conditionally check for this.
+        body =  (
+            '<ErrorResponse>'
+            '  <Error>'
+            '    <Type>Sender</Type>'
+            '    <Code>InvalidInput</Code>'
+            '  </Error>'
+            '  <RequestId>id</RequestId>'
+            '</ErrorResponse>'
+        ).encode('utf-8')
+        parser = parsers.RestXMLParser()
+        parsed = parser.parse({
+            'body': body, 'headers': {}, 'status_code': 400}, None)
+        error = parsed['Error']
+        self.assertEqual(error['Code'], 'InvalidInput')
+        # Even though there's no <Message /> we should
+        # still populate an empty string.
+        self.assertEqual(error['Message'], '')