Skip to content

Commit

Permalink
Revert "S3 200 errors implementation (boto#3276)"
Browse files Browse the repository at this point in the history 
This reverts commit d64e956.
  • Loading branch information
@alexgromero
alexgromero committed Oct 23, 2024
1 parent b725262 commit 1b50e32
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 116 deletions.
49 changes: 11 additions & 38 deletions botocore/handlers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ 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
Expand All @@ -153,9 +152,7 @@ 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.status_code, http_response.content
):
if _looks_like_special_case_error(http_response):
logger.debug(
"Error found for response with 200 status code, "
"errors: %s, changing status code to "
Expand All @@ -165,13 +162,13 @@ def check_for_200_error(response, **kwargs):
http_response.status_code = 500


def _looks_like_special_case_error(status_code, body):
if status_code == 200 and body:
def _looks_like_special_case_error(http_response):
if http_response.status_code == 200:
try:
parser = ETree.XMLParser(
target=ETree.TreeBuilder(), encoding='utf-8'
)
parser.feed(body)
parser.feed(http_response.content)
root = parser.close()
except XMLParseError:
# In cases of network disruptions, we may end up with a partial
Expand Down Expand Up @@ -1242,35 +1239,6 @@ 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.
Expand Down Expand Up @@ -1301,7 +1269,6 @@ def _update_status_code(response, **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),
Expand Down Expand Up @@ -1345,7 +1312,13 @@ def _update_status_code(response, **kwargs):
('before-call.ec2.CopySnapshot', inject_presigned_url_ec2),
('request-created', add_retry_headers),
('request-created.machinelearning.Predict', switch_host_machinelearning),
('needs-retry.s3.*', _update_status_code, REGISTER_FIRST),
('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,
),
('choose-signer.cognito-identity.GetId', disable_signing),
('choose-signer.cognito-identity.GetOpenIdToken', disable_signing),
('choose-signer.cognito-identity.UnlinkIdentity', disable_signing),
Expand Down
85 changes: 10 additions & 75 deletions tests/functional/test_s3.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
UnsupportedS3AccesspointConfigurationError,
UnsupportedS3ConfigurationError,
)
from botocore.parsers import ResponseParserError
from tests import (
BaseSessionTest,
ClientHTTPStubber,
Expand Down Expand Up @@ -434,20 +435,21 @@ def create_stubbed_s3_client(self, **kwargs):
http_stubber.start()
return client, http_stubber

def test_s3_copy_object_with_incomplete_response(self):
def test_s3_copy_object_with_empty_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'
empty_body = b""
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=incomplete_body)

self.http_stubber.add_response(status=200, body=empty_body)
self.http_stubber.add_response(status=200, body=complete_body)
response = self.client.copy_object(
Bucket="bucket",
Expand All @@ -460,86 +462,19 @@ def test_s3_copy_object_with_incomplete_response(self):
self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
self.assertTrue("CopyObjectResult" in response)


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):
def test_s3_copy_object_with_incomplete_response(self):
self.client, self.http_stubber = self.create_stubbed_s3_client(
region_name="us-east-1"
)
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:

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):
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):
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/test_handlers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 _update_status_code is before any of the retry logic in
# 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('_update_status_code', names)
self.assertIn('check_for_200_error', names)
self.assertIn('RetryHandler', names)
s3_200_handler = names.index('_update_status_code')
s3_200_handler = names.index('check_for_200_error')
general_retry_handler = names.index('RetryHandler')
self.assertTrue(
s3_200_handler < general_retry_handler,
Expand Down

0 comments on commit 1b50e32

Please sign in to comment.