Handle case where S3 returns a 200 error response

botocore/handlers.py

@@ -37,6 +37,31 @@
 
 
 
+def check_for_200_error(response, operation, **kwargs):
+    # From: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html
+    # There are two opportunities for a copy request to return an error. One
+    # can occur when Amazon S3 receives the copy request and the other can
+    # occur while Amazon S3 is copying the files. If the error occurs before
+    # the copy operation starts, you receive a standard Amazon S3 error. If the
+    # error occurs during the copy operation, the error response is embedded in
+    # the 200 OK response. This means that a 200 OK response can contain either
+    # a success or an error. Make sure to design your application to parse the
+    # contents of the response and handle it appropriately.
+    #
+    # So this handler checks for this case.  Even though the server sends a
+    # 200 response, conceptually this should be handled exactly like a
+    # 500 response (with respect to raising exceptions, retries, etc.)
+    # We're connected *before* all the other retry logic handlers, so as long
+    # as we switch the error code to 500, we'll retry the error as expected.
+    http_response, parsed = response
+    if http_response.status_code == 200:
+        if 'Errors' in parsed:
+            logger.debug("Error found for response with 200 status code, "
+                         "operation: %s, errors: %s, changing status code to "
+                         "500.", operation, parsed)
+            http_response.status_code = 500
+
+
 def decode_console_output(event_name, shape, value, **kwargs):
     try:
         value = base64.b64decode(six.b(value)).decode('utf-8')
@@ -239,6 +264,9 @@ def copy_snapshot_encrypted(operation, params, **kwargs):
     ('before-call.s3.CopyObject', quote_source_header),
     ('before-call.ec2.CopySnapshot', copy_snapshot_encrypted),
     ('before-auth.s3', fix_s3_host),
+    ('needs-retry.s3.UploadPartCopy', check_for_200_error),
+    ('needs-retry.s3.CopyObject', check_for_200_error),
+    ('needs-retry.s3.CompleteMultipartUpload', check_for_200_error),
     ('service-created', register_retries_for_service),
     ('creating-endpoint.s3', maybe_switch_to_s3sigv4),
     ('creating-endpoint.ec2', maybe_switch_to_sigv4),

tests/unit/test_endpoint.py

@@ -347,9 +347,9 @@ def test_retry_on_socket_errors(self):
         self.assertEqual(self.total_calls, 3)
 
 
-class TestResetStreamOnRetry(unittest.TestCase):
+class TestS3ResetStreamOnRetry(unittest.TestCase):
     def setUp(self):
-        super(TestResetStreamOnRetry, self).setUp()
+        super(TestS3ResetStreamOnRetry, self).setUp()
         self.total_calls = 0
         self.auth = Mock()
         self.session = create_session(include_builtin_handlers=False)
@@ -393,6 +393,42 @@ def test_reset_stream_on_retry(self):
         self.assertEqual(payload.literal_value.total_resets, 2)
 
 
+class TestS3Retry200SpecialCases(unittest.TestCase):
+    def setUp(self):
+        super(TestS3Retry200SpecialCases, self).setUp()
+        self.total_calls = 0
+        self.auth = Mock()
+        self.session = create_session(include_builtin_handlers=True)
+        self.service = Mock()
+        self.service.endpoint_prefix = 's3'
+        self.service.session = self.session
+        self.endpoint = RestEndpoint(
+            self.service, 'us-east-1', 'https://s3.amazonaws.com/',
+            auth=self.auth)
+        self.http_session = Mock()
+        self.endpoint.http_session = self.http_session
+        self.get_response_patch = patch('botocore.response.get_response')
+        self.get_response = self.get_response_patch.start()
+        self.retried_on_exception = None
+
+    def tearDown(self):
+        self.get_response_patch.stop()
+
+    def test_retry_special_case_s3_200_response(self):
+        # Test for:
+        # http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html
+        op = Mock()
+        op.name = 'CopyObject'
+        op.http = {'uri': '', 'method': 'PUT'}
+        http_response = Mock()
+        http_response.status_code = 200
+        parsed = {'Errors': [{'Code': 'InternalError', 'Message': 'foo'}]}
+        self.get_response.return_value = (http_response, parsed)
+        self.endpoint.make_request(op, {'headers': {}, 'payload': None})
+        # The response code should have been switched to 500.
+        self.assertEqual(http_response.status_code, 500)
+
+
 class TestRestEndpoint(unittest.TestCase):
 
     def test_encode_uri_params_unicode(self):

tests/unit/test_handlers.py

@@ -21,6 +21,7 @@
 from botocore.hooks import first_non_none_response
 from botocore.compat import quote
 from botocore.handlers import copy_snapshot_encrypted
+from botocore.handlers import check_for_200_error
 
 
 class TestHandlers(BaseSessionTest):
@@ -98,6 +99,67 @@ def test_copy_snapshot_encrypted(self):
         # We created an endpoint in the source region.
         operation.service.get_endpoint.assert_called_with('us-west-2')
 
+    def test_500_status_code_set_for_200_response(self):
+        http_response = mock.Mock()
+        http_response.status_code = 200
+        parsed = {
+            'Errors': [{
+                "HostId": "hostid",
+                "Message": "An internal error occurred.",
+                "Code": "InternalError",
+                "RequestId": "123456789"
+            }]
+        }
+        check_for_200_error((http_response, parsed), 'MyOperationName')
+        self.assertEqual(http_response.status_code, 500)
+
+    def test_200_response_with_no_error_left_untouched(self):
+        http_response = mock.Mock()
+        http_response.status_code = 200
+        parsed = {
+            'NotAnError': [{
+                'foo': 'bar'
+            }]
+        }
+        check_for_200_error((http_response, parsed), 'MyOperationName')
+        # We don't touch the status code since there are no errors present.
+        self.assertEqual(http_response.status_code, 200)
+
+
+class TestRetryHandlerOrder(BaseSessionTest):
+    def get_handler_names(self, responses):
+        names = []
+        for response in responses:
+            handler = response[0]
+            if hasattr(handler, '__name__'):
+                names.append(handler.__name__)
+            elif hasattr(handler, '__class__'):
+                names.append(handler.__class__.__name__)
+            else:
+                names.append(str(handler))
+        return names
+
+    def test_s3_special_case_is_before_other_retry(self):
+        service = self.session.get_service('s3')
+        operation = service.get_operation('CopyObject')
+        responses = self.session.emit(
+            'needs-retry.s3.CopyObject',
+            response=(mock.Mock(), mock.Mock()), endpoint=mock.Mock(), operation=operation,
+            attempts=1, caught_exception=None)
+        # This is implementation specific, but we're trying to verify that
+        # the check_for_200_error is before any of the retry logic in
+        # botocore.retryhandlers.
+        # Technically, as long as the relative order is preserved, we don't
+        # care about the absolute order.
+        names = self.get_handler_names(responses)
+        self.assertIn('check_for_200_error', names)
+        self.assertIn('RetryHandler', names)
+        s3_200_handler = names.index('check_for_200_error')
+        general_retry_handler = names.index('RetryHandler')
+        self.assertTrue(s3_200_handler < general_retry_handler,
+                        "S3 200 error handler was suppose to be before "
+                        "the general retry handler, but it was not.")
+
 
 if __name__ == '__main__':
     unittest.main()