Skip to content

Commit

Permalink
update implementation and clean up tests
Browse files Browse the repository at this point in the history
  • Loading branch information
alexgromero committed Oct 17, 2024
1 parent 7ad46f6 commit b08fd43
Show file tree
Hide file tree
Showing 11 changed files with 126 additions and 169 deletions.
5 changes: 0 additions & 5 deletions botocore/endpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,6 @@ def _do_get_response(self, request, operation_model, context):
response_dict, operation_model.output_shape
)
parsed_response.update(customized_response_dict)
if updated_status_code := customized_response_dict.get(
'updated_status_code'
):
http_response.status_code = updated_status_code
del parsed_response['updated_status_code']
# Do a second parsing pass to pick up on any modeled error fields
# NOTE: Ideally, we would push this down into the parser classes but
# they currently have no reference to the operation or service model
Expand Down
37 changes: 27 additions & 10 deletions botocore/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1240,28 +1240,44 @@ def document_expires_shape(section, event_name, **kwargs):
)


def _handle_200_error(
operation_model, response_dict, customized_response_dict, **kwargs
):
if (
not response_dict
or operation_model.has_streaming_output
or not operation_model.has_modeled_body_output
):
def _handle_200_error(operation_model, response_dict, **kwargs):
# S3 can return a 200 OK response with an error embedded in the body.
# Conceptually, this should be handled like a 500 response in terms of
# raising exceptions and retries which we handle in _retry_200_error.
# This handler converts the 200 response to a 500 response to ensure
# correct error handling.
if not response_dict or operation_model.has_streaming_output:
# Operations with streaming response blobs should be excluded as they
# may contain customer content which mimics the form of an S3 error.
return
if _looks_like_special_case_error(
response_dict['status_code'], response_dict['body']
):
# The response_dict status code must be changed to be parsed as a 500 response.
response_dict['status_code'] = 500
logger.debug(
f"Error found for response with 200 status code: {response_dict['body']}. "
f"Changing status code to 500."
f"Changing the http_response status code to 500 will be propagated in "
f"the _retry_200_error handler."
)


def _retry_200_error(response, **kwargs):
# Adjusts the HTTP status code for responses that may contain errors
# embedded in a 200 OK response body. The _handle_200_error function
# modifies the parsed response status code to 500 if it detects an error.
# This function checks if the HTTP status code differs from the parsed
# status code and updates the HTTP response accordingly, ensuring
# correct handling for retries.
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.
Expand Down Expand Up @@ -1336,6 +1352,7 @@ def _handle_200_error(
('before-call.ec2.CopySnapshot', inject_presigned_url_ec2),
('request-created', add_retry_headers),
('request-created.machinelearning.Predict', switch_host_machinelearning),
('needs-retry.s3.*', _retry_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
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,7 @@ def assert_endpoint_url_used_for_operation(
):
http_stubber = ClientHTTPStubber(client)
http_stubber.start()
http_stubber.add_response(
body=(b'<Test/>' if operation == 'list_buckets' else None)
)
http_stubber.add_response()

# Call an operation on the client
getattr(client, operation)(**params)
Expand Down
4 changes: 2 additions & 2 deletions tests/functional/test_credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ def test_token_chosen_from_provider(self):
session = Session(profile='sso-test')
with SessionHTTPStubber(session) as stubber:
self.add_credential_response(stubber)
stubber.add_response(body=b'<Test/>')
stubber.add_response()
with mock.patch.object(
SSOTokenProvider, 'DEFAULT_CACHE_CLS', MockCache
):
Expand Down Expand Up @@ -1155,7 +1155,7 @@ def test_credential_context_override(self):
with SessionHTTPStubber(session) as stubber:
s3 = session.create_client('s3')
s3.meta.events.register('before-sign', self._add_fake_creds)
stubber.add_response(body=b'<Test/>')
stubber.add_response()
s3.list_buckets()
request = stubber.requests[0]
assert self.ACCESS_KEY in str(request.headers.get('Authorization'))
4 changes: 2 additions & 2 deletions tests/functional/test_regions.py
Original file line number Diff line number Diff line change
Expand Up @@ -502,15 +502,15 @@ def create_stubbed_client(self, service_name, region_name, **kwargs):

def test_regionalized_client_endpoint_resolution(self):
client, stubber = self.create_stubbed_client('s3', 'us-east-2')
stubber.add_response(body=b'<Test/>')
stubber.add_response()
client.list_buckets()
self.assertEqual(
stubber.requests[0].url, 'https://s3.us-east-2.amazonaws.com/'
)

def test_regionalized_client_with_unknown_region(self):
client, stubber = self.create_stubbed_client('s3', 'not-real')
stubber.add_response(body=b'<Test/>')
stubber.add_response()
client.list_buckets()
# Validate we don't fall back to partition endpoint for
# regionalized services.
Expand Down
37 changes: 18 additions & 19 deletions tests/functional/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,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",
Expand Down Expand Up @@ -551,7 +550,7 @@ def test_accesspoint_arn_with_custom_endpoint(self):
self.client, http_stubber = self.create_stubbed_s3_client(
endpoint_url="https://custom.com"
)
http_stubber.add_response(body=b'<Test/>')
http_stubber.add_response()
self.client.list_objects(Bucket=accesspoint_arn)
expected_endpoint = "myendpoint-123456789012.custom.com"
self.assert_endpoint(http_stubber.requests[0], expected_endpoint)
Expand All @@ -564,7 +563,7 @@ def test_accesspoint_arn_with_custom_endpoint_and_dualstack(self):
endpoint_url="https://custom.com",
config=Config(s3={"use_dualstack_endpoint": True}),
)
http_stubber.add_response(body=b'<Test/>')
http_stubber.add_response()
self.client.list_objects(Bucket=accesspoint_arn)
expected_endpoint = "myendpoint-123456789012.custom.com"
self.assert_endpoint(http_stubber.requests[0], expected_endpoint)
Expand Down Expand Up @@ -613,7 +612,7 @@ def test_signs_with_arn_region(self):
self.client, self.http_stubber = self.create_stubbed_s3_client(
region_name="us-east-1"
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=accesspoint_arn)
self.assert_signing_region(self.http_stubber.requests[0], "us-west-2")

Expand Down Expand Up @@ -737,7 +736,7 @@ def test_basic_outpost_arn(self):
self.client, self.http_stubber = self.create_stubbed_s3_client(
region_name="us-east-1"
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=outpost_arn)
request = self.http_stubber.requests[0]
self.assert_signing_name(request, "s3-outposts")
Expand All @@ -759,7 +758,7 @@ def test_basic_outpost_arn_custom_endpoint(self):
self.client, self.http_stubber = self.create_stubbed_s3_client(
endpoint_url="https://custom.com", region_name="us-east-1"
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=outpost_arn)
request = self.http_stubber.requests[0]
self.assert_signing_name(request, "s3-outposts")
Expand Down Expand Up @@ -963,7 +962,7 @@ def test_s3_object_lambda_arn_with_us_east_1(self):
region_name="us-east-1",
config=Config(s3={"use_arn_region": False}),
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=s3_object_lambda_arn)
request = self.http_stubber.requests[0]
self.assert_signing_name(request, "s3-object-lambda")
Expand All @@ -981,7 +980,7 @@ def test_basic_s3_object_lambda_arn(self):
self.client, self.http_stubber = self.create_stubbed_s3_client(
region_name="us-east-1"
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=s3_object_lambda_arn)
request = self.http_stubber.requests[0]
self.assert_signing_name(request, "s3-object-lambda")
Expand Down Expand Up @@ -1049,7 +1048,7 @@ def test_accesspoint_with_global_regions(self):
config=Config(s3={"use_arn_region": True}),
)

self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=s3_accesspoint_arn)
request = self.http_stubber.requests[0]
expected_endpoint = (
Expand All @@ -1063,7 +1062,7 @@ def test_accesspoint_with_global_regions(self):
region_name="s3-external-1",
)

self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=s3_accesspoint_arn)
request = self.http_stubber.requests[0]
expected_endpoint = (
Expand Down Expand Up @@ -1152,7 +1151,7 @@ def test_mrap_signing_algorithm_is_sigv4a(self):
self.client, self.http_stubber = self.create_stubbed_s3_client(
region_name="us-west-2"
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=s3_accesspoint_arn)
request = self.http_stubber.requests[0]
self._assert_sigv4a_used(request.headers)
Expand Down Expand Up @@ -1239,7 +1238,7 @@ def _assert_mrap_endpoint(
self.client, self.http_stubber = self.create_stubbed_s3_client(
region_name=region, endpoint_url=endpoint_url, config=config
)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
self.client.list_objects(Bucket=arn)
request = self.http_stubber.requests[0]
self.assert_endpoint(request, expected)
Expand Down Expand Up @@ -1543,7 +1542,7 @@ def test_content_sha256_set_if_config_value_not_set_list_objects(self):
"s3", self.region, config=config
)
self.http_stubber = ClientHTTPStubber(self.client)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
with self.http_stubber:
self.client.list_objects(Bucket="foo")
sent_headers = self.get_sent_headers()
Expand All @@ -1561,7 +1560,7 @@ def test_content_sha256_set_s3_on_outpost(self):
"s3", self.region, config=config
)
self.http_stubber = ClientHTTPStubber(self.client)
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
with self.http_stubber:
self.client.list_objects(Bucket=bucket)
sent_headers = self.get_sent_headers()
Expand Down Expand Up @@ -2210,7 +2209,7 @@ def test_checksums_included_in_expected_operations(
"""Validate expected calls include Content-MD5 header"""
client = _create_s3_client()
with ClientHTTPStubber(client) as stub:
stub.add_response(body=b'<Test/>')
stub.add_response()
call = getattr(client, operation)
call(**operation_kwargs)
assert "Content-MD5" in stub.requests[-1].headers
Expand Down Expand Up @@ -3656,7 +3655,7 @@ def assert_correct_content_md5(self, request):
self.assertEqual(content_md5, request.headers["Content-MD5"])

def test_escape_keys_in_xml_delete_objects(self):
self.http_stubber.add_response(body=b'<Test/>')
self.http_stubber.add_response()
with self.http_stubber:
self.client.delete_objects(
Bucket="mybucket",
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/test_s3express.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ def test_delete_objects_injects_correct_checksum(

with ClientHTTPStubber(default_s3_client) as stubber:
stubber.add_response(body=CREATE_SESSION_RESPONSE)
stubber.add_response(body=b'<Test/>')
stubber.add_response()

default_s3_client.delete_objects(
Bucket=S3EXPRESS_BUCKET,
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/test_useragent.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class UACapHTTPStubber(ClientHTTPStubber):

def __init__(self, obj_with_event_emitter):
super().__init__(obj_with_event_emitter, strict=False)
self.add_response(body=b'<Test/>') # expect exactly one request
self.add_response() # expect exactly one request

@property
def captured_ua_string(self):
Expand Down
Loading

0 comments on commit b08fd43

Please sign in to comment.