S3 200 errors implementation

.changes/next-release/enhancement-s3-47846.json

@@ -0,0 +1,5 @@
+{
+  "type": "enhancement",
+  "category": "``s3``",
+  "description": "Handle HTTP 200 responses with error information for all supported s3 operations."
+}

botocore/handlers.py

@@ -132,6 +132,7 @@ def escape_xml_payload(params, **kwargs):
 
 
 def check_for_200_error(response, **kwargs):
+    """This function has been deprecated, but is kept for backwards compatibility."""
     # 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
@@ -152,7 +153,9 @@ def check_for_200_error(response, **kwargs):
         # trying to retrieve the response.  See Endpoint._get_response().
         return
     http_response, parsed = response
-    if _looks_like_special_case_error(http_response):
+    if _looks_like_special_case_error(
+        http_response.status_code, http_response.content
+    ):
         logger.debug(
             "Error found for response with 200 status code, "
             "errors: %s, changing status code to "
@@ -162,13 +165,13 @@ def check_for_200_error(response, **kwargs):
         http_response.status_code = 500
 
 
-def _looks_like_special_case_error(http_response):
-    if http_response.status_code == 200:
+def _looks_like_special_case_error(status_code, body):
+    if status_code == 200 and body:
         try:
             parser = ETree.XMLParser(
                 target=ETree.TreeBuilder(), encoding='utf-8'
             )
-            parser.feed(http_response.content)
+            parser.feed(body)
             root = parser.close()
         except XMLParseError:
             # In cases of network disruptions, we may end up with a partial
@@ -1239,6 +1242,35 @@ def document_expires_shape(section, event_name, **kwargs):
         )
 
 
+def _handle_200_error(operation_model, response_dict, **kwargs):
+    # S3 can return a 200 response with an error embedded in the body.
+    # Convert the 200 to a 500 for retry resolution in ``_update_status_code``.
+    if not response_dict or operation_model.has_streaming_output:
+        # Operations with streaming response blobs are excluded as they
+        # can't be reliably distinguished from an S3 error.
+        return
+    if _looks_like_special_case_error(
+        response_dict['status_code'], response_dict['body']
+    ):
+        response_dict['status_code'] = 500
+        logger.debug(
+            f"Error found for response with 200 status code: {response_dict['body']}."
+        )
+
+
+def _update_status_code(response, **kwargs):
+    # Update the http_response status code when the parsed response has been
+    # modified in a handler. This enables retries for cases like ``_handle_200_error``.
+    if response is None:
+        return
+    http_response, parsed = response
+    parsed_status_code = parsed.get('ResponseMetadata', {}).get(
+        'HTTPStatusCode'
+    )
+    if http_response.status_code != parsed_status_code:
+        http_response.status_code = parsed_status_code
+
+
 # This is a list of (event_name, handler).
 # When a Session is created, everything in this list will be
 # automatically registered with that Session.
@@ -1269,6 +1301,7 @@ def document_expires_shape(section, event_name, **kwargs):
     ('after-call.cloudformation.GetTemplate', json_decode_template_body),
     ('after-call.s3.GetBucketLocation', parse_get_bucket_location),
     ('before-parse.s3.*', handle_expires_header),
+    ('before-parse.s3.*', _handle_200_error, REGISTER_FIRST),
     ('before-parameter-build', generate_idempotent_uuid),
     ('before-parameter-build.s3', validate_bucket_name),
     ('before-parameter-build.s3', remove_bucket_from_url_paths_from_model),
@@ -1312,13 +1345,7 @@ def document_expires_shape(section, event_name, **kwargs):
     ('before-call.ec2.CopySnapshot', inject_presigned_url_ec2),
     ('request-created', add_retry_headers),
     ('request-created.machinelearning.Predict', switch_host_machinelearning),
-    ('needs-retry.s3.UploadPartCopy', check_for_200_error, REGISTER_FIRST),
-    ('needs-retry.s3.CopyObject', check_for_200_error, REGISTER_FIRST),
-    (
-        'needs-retry.s3.CompleteMultipartUpload',
-        check_for_200_error,
-        REGISTER_FIRST,
-    ),
+    ('needs-retry.s3.*', _update_status_code, REGISTER_FIRST),
     ('choose-signer.cognito-identity.GetId', disable_signing),
     ('choose-signer.cognito-identity.GetOpenIdToken', disable_signing),
     ('choose-signer.cognito-identity.UnlinkIdentity', disable_signing),

tests/functional/test_s3.py

@@ -28,7 +28,6 @@
     UnsupportedS3AccesspointConfigurationError,
     UnsupportedS3ConfigurationError,
 )
-from botocore.parsers import ResponseParserError
 from tests import (
     BaseSessionTest,
     ClientHTTPStubber,
@@ -435,21 +434,20 @@ def create_stubbed_s3_client(self, **kwargs):
         http_stubber.start()
         return client, http_stubber
 
-    def test_s3_copy_object_with_empty_response(self):
+    def test_s3_copy_object_with_incomplete_response(self):
         self.client, self.http_stubber = self.create_stubbed_s3_client(
             region_name="us-east-1"
         )
 
-        empty_body = b""
+        incomplete_body = b'<?xml version="1.0" encoding="UTF-8"?>\n\n\n'
         complete_body = (
             b'<?xml version="1.0" encoding="UTF-8"?>\n\n'
             b"<CopyObjectResult "
             b'xmlns="http://s3.amazonaws.com/doc/2006-03-01/">'
             b"<LastModified>2020-04-21T21:03:31.000Z</LastModified>"
             b"<ETag>&quot;s0mEcH3cK5uM&quot;</ETag></CopyObjectResult>"
         )
-
-        self.http_stubber.add_response(status=200, body=empty_body)
+        self.http_stubber.add_response(status=200, body=incomplete_body)
         self.http_stubber.add_response(status=200, body=complete_body)
         response = self.client.copy_object(
             Bucket="bucket",
@@ -462,19 +460,86 @@ def test_s3_copy_object_with_empty_response(self):
         self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
         self.assertTrue("CopyObjectResult" in response)
 
-    def test_s3_copy_object_with_incomplete_response(self):
+
+class TestS3200ErrorResponse(BaseS3OperationTest):
+    def create_s3_client(self, **kwargs):
+        client_kwargs = {"region_name": self.region}
+        client_kwargs.update(kwargs)
+        return self.session.create_client("s3", **client_kwargs)
+
+    def create_stubbed_s3_client(self, **kwargs):
+        client = self.create_s3_client(**kwargs)
+        http_stubber = ClientHTTPStubber(client)
+        http_stubber.start()
+        return client, http_stubber
+
+    def test_s3_200_with_error_response(self):
         self.client, self.http_stubber = self.create_stubbed_s3_client(
             region_name="us-east-1"
         )
-
-        incomplete_body = b'<?xml version="1.0" encoding="UTF-8"?>\n\n\n'
-        self.http_stubber.add_response(status=200, body=incomplete_body)
-        with self.assertRaises(ResponseParserError):
+        error_body = (
+            b"<Error>"
+            b"<Code>SlowDown</Code>"
+            b"<Message>Please reduce your request rate.</Message>"
+            b"</Error>"
+        )
+        # Populate 5 attempts for SlowDown to validate
+        # we reached four max retries and raised an exception.
+        for i in range(5):
+            self.http_stubber.add_response(status=200, body=error_body)
+        with self.assertRaises(botocore.exceptions.ClientError) as context:
             self.client.copy_object(
                 Bucket="bucket",
                 CopySource="other-bucket/test.txt",
                 Key="test.txt",
             )
+        self.assertEqual(len(self.http_stubber.requests), 5)
+        self.assertEqual(
+            context.exception.response["ResponseMetadata"]["HTTPStatusCode"],
+            500,
+        )
+        self.assertEqual(
+            context.exception.response["Error"]["Code"], "SlowDown"
+        )
+
+    def test_s3_200_with_no_error_response(self):
+        self.client, self.http_stubber = self.create_stubbed_s3_client(
+            region_name="us-east-1"
+        )
+        self.http_stubber.add_response(status=200, body=b"<NotAnError/>")
+
+        response = self.client.copy_object(
+            Bucket="bucket",
+            CopySource="other-bucket/test.txt",
+            Key="test.txt",
+        )
+
+        # Validate that the status code remains 200.
+        self.assertEqual(len(self.http_stubber.requests), 1)
+        self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
+
+    def test_s3_200_with_error_response_on_streaming_operation(self):
+        self.client, self.http_stubber = self.create_stubbed_s3_client(
+            region_name="us-east-1"
+        )
+        self.http_stubber.add_response(status=200, body=b"<Error/>")
+        response = self.client.get_object(Bucket="bucket", Key="test.txt")
+
+        # Validate that the status code remains 200 because we don't
+        # process 200-with-error responses on streaming operations.
+        self.assertEqual(len(self.http_stubber.requests), 1)
+        self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
+
+    def test_s3_200_response_with_no_body(self):
+        self.client, self.http_stubber = self.create_stubbed_s3_client(
+            region_name="us-east-1"
+        )
+        self.http_stubber.add_response(status=200)
+        response = self.client.head_object(Bucket="bucket", Key="test.txt")
+
+        # Validate that the status code remains 200 on operations without a body.
+        self.assertEqual(len(self.http_stubber.requests), 1)
+        self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
 
 
 class TestAccesspointArn(BaseS3ClientConfigurationTest):

tests/unit/test_handlers.py

@@ -1202,14 +1202,14 @@ def test_s3_special_case_is_before_other_retry(self):
             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
+        # the _update_status_code 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('_update_status_code', names)
         self.assertIn('RetryHandler', names)
-        s3_200_handler = names.index('check_for_200_error')
+        s3_200_handler = names.index('_update_status_code')
         general_retry_handler = names.index('RetryHandler')
         self.assertTrue(
             s3_200_handler < general_retry_handler,