diff --git a/.changes/next-release/enhancement-s3-47846.json b/.changes/next-release/enhancement-s3-47846.json new file mode 100644 index 0000000000..a549b5e190 --- /dev/null +++ b/.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." +} diff --git a/botocore/handlers.py b/botocore/handlers.py index 9cb1d052c0..9a7f78acef 100644 --- a/botocore/handlers.py +++ b/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', http_response.status_code + ) + 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), diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index 04fc32aa7b..613ef1c016 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -28,7 +28,6 @@ UnsupportedS3AccesspointConfigurationError, UnsupportedS3ConfigurationError, ) -from botocore.parsers import ResponseParserError from tests import ( BaseSessionTest, ClientHTTPStubber, @@ -435,12 +434,12 @@ 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'\n\n\n' complete_body = ( b'\n\n' b"2020-04-21T21:03:31.000Z" b""s0mEcH3cK5uM"" ) - - 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'\n\n\n' - self.http_stubber.add_response(status=200, body=incomplete_body) - with self.assertRaises(ResponseParserError): + error_body = ( + b"" + b"SlowDown" + b"Please reduce your request rate." + b"" + ) + # 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"") + + 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"") + 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): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index 7e28356442..924abed8af 100644 --- a/tests/unit/test_handlers.py +++ b/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,