From 3a865b061c6b8b77f602ce208c9965af5b2cf0d9 Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Tue, 8 Oct 2024 12:47:41 -0400 Subject: [PATCH 01/20] S3 200 errors implementation --- botocore/endpoint.py | 1 + botocore/handlers.py | 36 ++-- .../test_configured_endpoint_url.py | 4 +- tests/functional/test_credentials.py | 4 +- tests/functional/test_regions.py | 4 +- tests/functional/test_s3.py | 53 +++--- tests/functional/test_s3express.py | 2 +- tests/functional/test_useragent.py | 2 +- tests/unit/test_handlers.py | 155 ++++++++++-------- tests/unit/test_s3_addressing.py | 28 +++- 10 files changed, 173 insertions(+), 116 deletions(-) diff --git a/botocore/endpoint.py b/botocore/endpoint.py index 1c2cee068b..bdacbb5802 100644 --- a/botocore/endpoint.py +++ b/botocore/endpoint.py @@ -308,6 +308,7 @@ def _do_get_response(self, request, operation_model, context): operation_model=operation_model, response_dict=response_dict, customized_response_dict=customized_response_dict, + http_response=http_response, ) parser = self._response_parser_factory.create_parser(protocol) parsed_response = parser.parse( diff --git a/botocore/handlers.py b/botocore/handlers.py index 9cb1d052c0..dcebda2405 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -131,7 +131,9 @@ def escape_xml_payload(params, **kwargs): params['body'] = body -def check_for_200_error(response, **kwargs): +def check_for_200_error( + operation_model, response_dict, http_response, **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 @@ -147,19 +149,27 @@ def check_for_200_error(response, **kwargs): # 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. - if response is None: + if ( + http_response is None + or operation_model.has_streaming_output + or not _has_modeled_body(operation_model) + ): # A None response can happen if an exception is raised while # trying to retrieve the response. See Endpoint._get_response(). + # Operations with streaming response blobs should be excluded as they + # may contain customer content which mimics the form of an S3 error. return - http_response, parsed = response if _looks_like_special_case_error(http_response): logger.debug( "Error found for response with 200 status code, " "errors: %s, changing status code to " "500.", - parsed, + http_response.content, ) http_response.status_code = 500 + # The response_dict status code must also be changed + # for it to be parsed as a 500 response. + response_dict['status_code'] = 500 def _looks_like_special_case_error(http_response): @@ -180,6 +190,16 @@ def _looks_like_special_case_error(http_response): return False +def _has_modeled_body(operation_model): + if output_shape := operation_model.output_shape: + for member_shape in output_shape.members.values(): + if not member_shape.serialization.get('location'): + # If any member is not bound to a location, + # we can expect a body + return True + return False + + def set_operation_specific_signer(context, signing_name, **kwargs): """Choose the operation-specific signer. @@ -1268,6 +1288,7 @@ def document_expires_shape(section, event_name, **kwargs): ('after-call.ec2.GetConsoleOutput', decode_console_output), ('after-call.cloudformation.GetTemplate', json_decode_template_body), ('after-call.s3.GetBucketLocation', parse_get_bucket_location), + ('before-parse.s3.*', check_for_200_error, REGISTER_FIRST), ('before-parse.s3.*', handle_expires_header), ('before-parameter-build', generate_idempotent_uuid), ('before-parameter-build.s3', validate_bucket_name), @@ -1312,13 +1333,6 @@ 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, - ), ('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/configured_endpoint_urls/test_configured_endpoint_url.py b/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py index 40cc17e26d..0a067981d0 100644 --- a/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py +++ b/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py @@ -147,7 +147,9 @@ def assert_endpoint_url_used_for_operation( ): http_stubber = ClientHTTPStubber(client) http_stubber.start() - http_stubber.add_response() + http_stubber.add_response( + body=(b'' if operation == 'list_buckets' else None) + ) # Call an operation on the client getattr(client, operation)(**params) diff --git a/tests/functional/test_credentials.py b/tests/functional/test_credentials.py index a6ba1f8b4b..1e957f3d6d 100644 --- a/tests/functional/test_credentials.py +++ b/tests/functional/test_credentials.py @@ -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() + stubber.add_response(body=b'') with mock.patch.object( SSOTokenProvider, 'DEFAULT_CACHE_CLS', MockCache ): @@ -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() + stubber.add_response(body=b'') s3.list_buckets() request = stubber.requests[0] assert self.ACCESS_KEY in str(request.headers.get('Authorization')) diff --git a/tests/functional/test_regions.py b/tests/functional/test_regions.py index 11a882f91f..be5f25087d 100644 --- a/tests/functional/test_regions.py +++ b/tests/functional/test_regions.py @@ -502,7 +502,7 @@ 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() + stubber.add_response(body=b'') client.list_buckets() self.assertEqual( stubber.requests[0].url, 'https://s3.us-east-2.amazonaws.com/' @@ -510,7 +510,7 @@ def test_regionalized_client_endpoint_resolution(self): def test_regionalized_client_with_unknown_region(self): client, stubber = self.create_stubbed_client('s3', 'not-real') - stubber.add_response() + stubber.add_response(body=b'') client.list_buckets() # Validate we don't fall back to partition endpoint for # regionalized services. diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index 04fc32aa7b..a4d85db6d4 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, @@ -462,19 +461,35 @@ 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): + def test_s3_copy_object_with_200_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"" + ) + self.http_stubber.add_response(status=200, body=error_body) + self.http_stubber.add_response(status=200, body=error_body) + self.http_stubber.add_response(status=200, body=error_body) + self.http_stubber.add_response(status=200, body=error_body) + 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" + ) class TestAccesspointArn(BaseS3ClientConfigurationTest): @@ -538,7 +553,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() + http_stubber.add_response(body=b'') self.client.list_objects(Bucket=accesspoint_arn) expected_endpoint = "myendpoint-123456789012.custom.com" self.assert_endpoint(http_stubber.requests[0], expected_endpoint) @@ -551,7 +566,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() + http_stubber.add_response(body=b'') self.client.list_objects(Bucket=accesspoint_arn) expected_endpoint = "myendpoint-123456789012.custom.com" self.assert_endpoint(http_stubber.requests[0], expected_endpoint) @@ -600,7 +615,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=accesspoint_arn) self.assert_signing_region(self.http_stubber.requests[0], "us-west-2") @@ -724,7 +739,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=outpost_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-outposts") @@ -746,7 +761,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=outpost_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-outposts") @@ -950,7 +965,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_object_lambda_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-object-lambda") @@ -968,7 +983,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_object_lambda_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-object-lambda") @@ -1036,7 +1051,7 @@ def test_accesspoint_with_global_regions(self): config=Config(s3={"use_arn_region": True}), ) - self.http_stubber.add_response() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] expected_endpoint = ( @@ -1050,7 +1065,7 @@ def test_accesspoint_with_global_regions(self): region_name="s3-external-1", ) - self.http_stubber.add_response() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] expected_endpoint = ( @@ -1530,7 +1545,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() + self.http_stubber.add_response(body=b'') with self.http_stubber: self.client.list_objects(Bucket="foo") sent_headers = self.get_sent_headers() @@ -1548,7 +1563,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() + self.http_stubber.add_response(body=b'') with self.http_stubber: self.client.list_objects(Bucket=bucket) sent_headers = self.get_sent_headers() @@ -2197,7 +2212,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() + stub.add_response(body=b'') call = getattr(client, operation) call(**operation_kwargs) assert "Content-MD5" in stub.requests[-1].headers @@ -3643,7 +3658,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() + self.http_stubber.add_response(body=b'') with self.http_stubber: self.client.delete_objects( Bucket="mybucket", diff --git a/tests/functional/test_s3express.py b/tests/functional/test_s3express.py index 390721ee12..0cf2307071 100644 --- a/tests/functional/test_s3express.py +++ b/tests/functional/test_s3express.py @@ -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() + stubber.add_response(body=b'') default_s3_client.delete_objects( Bucket=S3EXPRESS_BUCKET, diff --git a/tests/functional/test_useragent.py b/tests/functional/test_useragent.py index 79290459ab..660a325318 100644 --- a/tests/functional/test_useragent.py +++ b/tests/functional/test_useragent.py @@ -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() # expect exactly one request + self.add_response(body=b'') # expect exactly one request @property def captured_ua_string(self): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index 7e28356442..b62577a406 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -501,33 +501,6 @@ def test_presigned_url_casing_changed_for_rds(self): ) self.assertIn('X-Amz-Signature', params['PreSignedUrl']) - def test_500_status_code_set_for_200_response(self): - http_response = mock.Mock() - http_response.status_code = 200 - http_response.content = """ - - AccessDenied - Access Denied - id - hostid - - """ - handlers.check_for_200_error((http_response, {})) - 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 - http_response.content = "" - handlers.check_for_200_error((http_response, {})) - # We don't touch the status code since there are no errors present. - self.assertEqual(http_response.status_code, 200) - - def test_500_response_can_be_none(self): - # A 500 response can raise an exception, which means the response - # object is None. We need to handle this case. - handlers.check_for_200_error(None) - def test_route53_resource_id(self): event = 'before-parameter-build.route53.GetHostedZone' params = { @@ -1175,49 +1148,6 @@ def test_non_ascii_characters(self): ) -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): - client = self.session.create_client('s3') - service_model = self.session.get_service_model('s3') - operation = service_model.operation_model('CopyObject') - responses = client.meta.events.emit( - 'needs-retry.s3.CopyObject', - request_dict={'context': {}}, - 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 supposed to be before " - "the general retry handler, but it was not.", - ) - - class BaseMD5Test(BaseSessionTest): def setUp(self, **environ): super().setUp(**environ) @@ -1944,3 +1874,88 @@ def test_document_response_params_without_expires(document_expires_mocks): mocks['section'].get_section.assert_not_called() mocks['param_section'].add_new_section.assert_not_called() mocks['doc_section'].write.assert_not_called() + + +@pytest.fixture() +def operation_model_for_200_error(): + operation_model = mock.Mock() + operation_model.has_streaming_output = False + operation_model.output_shape = mock.Mock() + operation_model.output_shape.members = {'member': mock.Mock()} + operation_model.output_shape.members['member'].serialization = {} + return operation_model + + +@pytest.fixture() +def http_response(): + http_response = mock.Mock() + http_response.status_code = 200 + http_response.content = "" + return http_response + + +@pytest.fixture() +def response_dict(): + return {'status_code': 200} + + +def test_500_status_code_set_for_200_response( + operation_model_for_200_error, response_dict, http_response +): + http_response.content = """ + + AccessDenied + Access Denied + id + hostid + + """ + handlers.check_for_200_error( + operation_model_for_200_error, response_dict, http_response + ) + assert http_response.status_code == 500 + assert response_dict['status_code'] == 500 + + +def test_200_response_with_no_error_left_untouched( + operation_model_for_200_error, response_dict, http_response +): + http_response.content = "" + handlers.check_for_200_error( + operation_model_for_200_error, response_dict, http_response + ) + # We don't touch the status code since there are no errors present. + assert http_response.status_code == 200 + assert response_dict['status_code'] == 200 + + +def test_200_response_with_streaming_output_left_untouched( + response_dict, http_response +): + operation_model = mock.Mock() + operation_model.has_streaming_output = True + http_response.content = "" + handlers.check_for_200_error(operation_model, response_dict, http_response) + # We don't touch the status code on streaming operations + assert http_response.status_code == 200 + assert response_dict['status_code'] == 200 + + +def test_200_response_with_no_body_left_untouched( + operation_model_for_200_error, response_dict, http_response +): + operation_model_for_200_error.output_shape.members[ + 'member' + ].serialization = {'location': 'header'} + handlers.check_for_200_error( + operation_model_for_200_error, response_dict, http_response + ) + assert http_response.status_code == 200 + assert response_dict['status_code'] == 200 + + +def test_500_response_can_be_none(): + # A 500 response can raise an exception, which means the response + # object is None. We need to handle this case. + operation_model = mock.Mock() + handlers.check_for_200_error(operation_model, None, None) diff --git a/tests/unit/test_s3_addressing.py b/tests/unit/test_s3_addressing.py index 7150d5b4ce..d3f6d4f3f8 100644 --- a/tests/unit/test_s3_addressing.py +++ b/tests/unit/test_s3_addressing.py @@ -31,12 +31,14 @@ def setUp(self): set_list_objects_encoding_type_url, ) - def get_prepared_request(self, operation, params, force_hmacv1=False): + def get_prepared_request( + self, operation, params, force_hmacv1=False, body=None + ): if force_hmacv1: self.session.register('choose-signer', self.enable_hmacv1) client = self.session.create_client('s3', self.region_name) with ClientHTTPStubber(client) as http_stubber: - http_stubber.add_response() + http_stubber.add_response(body=body) getattr(client, operation)(**params) # Return the request that was sent over the wire. return http_stubber.requests[0] @@ -47,7 +49,7 @@ def enable_hmacv1(self, **kwargs): def test_list_objects_dns_name(self): params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True + 'list_objects', params, force_hmacv1=True, body=b'' ) self.assertEqual( prepared_request.url, 'https://safename.s3.amazonaws.com/' @@ -56,7 +58,7 @@ def test_list_objects_dns_name(self): def test_list_objects_non_dns_name(self): params = {'Bucket': 'un_safe_name'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True + 'list_objects', params, force_hmacv1=True, body=b'' ) self.assertEqual( prepared_request.url, 'https://s3.amazonaws.com/un_safe_name' @@ -66,7 +68,7 @@ def test_list_objects_dns_name_non_classic(self): self.region_name = 'us-west-2' params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True + 'list_objects', params, force_hmacv1=True, body=b'' ) self.assertEqual( prepared_request.url, @@ -78,7 +80,9 @@ def test_list_objects_unicode_query_string_eu_central_1(self): params = OrderedDict( [('Bucket', 'safename'), ('Marker', '\xe4\xf6\xfc-01.txt')] ) - prepared_request = self.get_prepared_request('list_objects', params) + prepared_request = self.get_prepared_request( + 'list_objects', params, body=b'' + ) self.assertEqual( prepared_request.url, ( @@ -90,7 +94,9 @@ def test_list_objects_unicode_query_string_eu_central_1(self): def test_list_objects_in_restricted_regions(self): self.region_name = 'us-gov-west-1' params = {'Bucket': 'safename'} - prepared_request = self.get_prepared_request('list_objects', params) + prepared_request = self.get_prepared_request( + 'list_objects', params, body=b'' + ) # Note how we keep the region specific endpoint here. self.assertEqual( prepared_request.url, @@ -100,7 +106,9 @@ def test_list_objects_in_restricted_regions(self): def test_list_objects_in_fips(self): self.region_name = 'fips-us-gov-west-1' params = {'Bucket': 'safename'} - prepared_request = self.get_prepared_request('list_objects', params) + prepared_request = self.get_prepared_request( + 'list_objects', params, body=b'' + ) # Note how we keep the region specific endpoint here. self.assertEqual( prepared_request.url, @@ -110,7 +118,9 @@ def test_list_objects_in_fips(self): def test_list_objects_non_dns_name_non_classic(self): self.region_name = 'us-west-2' params = {'Bucket': 'un_safe_name'} - prepared_request = self.get_prepared_request('list_objects', params) + prepared_request = self.get_prepared_request( + 'list_objects', params, body=b'' + ) self.assertEqual( prepared_request.url, 'https://s3.us-west-2.amazonaws.com/un_safe_name', From c0e9871f214324d69e8ad3badf5f2c58a6f5964d Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Tue, 8 Oct 2024 13:15:26 -0400 Subject: [PATCH 02/20] add changelog --- .changes/next-release/enhancement-s3-47846.json | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/next-release/enhancement-s3-47846.json diff --git a/.changes/next-release/enhancement-s3-47846.json b/.changes/next-release/enhancement-s3-47846.json new file mode 100644 index 0000000000..2c93767355 --- /dev/null +++ b/.changes/next-release/enhancement-s3-47846.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "``s3``", + "description": "Adds logic to gracefully handle HTTP 200 OK responses that contain error information in the response body." +} From e18167062a3e89c1d3f052ff33a7d0edf466704e Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Tue, 8 Oct 2024 16:47:59 -0400 Subject: [PATCH 03/20] move _has_modeled_body function and update tests that need bodies --- botocore/handlers.py | 12 +-- botocore/model.py | 20 +++++ .../test_configured_endpoint_url.py | 2 +- tests/functional/test_credentials.py | 4 +- tests/functional/test_regions.py | 4 +- tests/functional/test_s3.py | 30 +++---- tests/functional/test_s3express.py | 2 +- tests/functional/test_useragent.py | 2 +- tests/unit/test_handlers.py | 8 +- tests/unit/test_model.py | 85 +++++++++++++++++++ tests/unit/test_s3_addressing.py | 14 +-- 11 files changed, 137 insertions(+), 46 deletions(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index dcebda2405..fe6b3c252e 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -152,7 +152,7 @@ def check_for_200_error( if ( http_response is None or operation_model.has_streaming_output - or not _has_modeled_body(operation_model) + or not operation_model.has_modeled_body_output ): # A None response can happen if an exception is raised while # trying to retrieve the response. See Endpoint._get_response(). @@ -190,16 +190,6 @@ def _looks_like_special_case_error(http_response): return False -def _has_modeled_body(operation_model): - if output_shape := operation_model.output_shape: - for member_shape in output_shape.members.values(): - if not member_shape.serialization.get('location'): - # If any member is not bound to a location, - # we can expect a body - return True - return False - - def set_operation_specific_signer(context, signing_name, **kwargs): """Choose the operation-specific signer. diff --git a/botocore/model.py b/botocore/model.py index 677266c8d2..f4070fb6ee 100644 --- a/botocore/model.py +++ b/botocore/model.py @@ -707,6 +707,26 @@ def _get_streaming_body(self, shape): return payload_shape return None + @CachedProperty + def has_modeled_body_input(self): + return self._has_modeled_body(self.input_shape) + + @CachedProperty + def has_modeled_body_output(self): + return self._has_modeled_body(self.output_shape) + + def _has_modeled_body(self, shape): + """ + Determines if an operation has a modeled body. + If any member is not bound to a location, we can expect a body. + """ + if shape is None: + return False + for member_shape in shape.members.values(): + if not member_shape.serialization.get('location'): + return True + return False + def __repr__(self): return f'{self.__class__.__name__}(name={self.name})' diff --git a/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py b/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py index 0a067981d0..51649d5bbc 100644 --- a/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py +++ b/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py @@ -148,7 +148,7 @@ def assert_endpoint_url_used_for_operation( http_stubber = ClientHTTPStubber(client) http_stubber.start() http_stubber.add_response( - body=(b'' if operation == 'list_buckets' else None) + body=(b'' if operation == 'list_buckets' else None) ) # Call an operation on the client diff --git a/tests/functional/test_credentials.py b/tests/functional/test_credentials.py index 1e957f3d6d..a45a73f14e 100644 --- a/tests/functional/test_credentials.py +++ b/tests/functional/test_credentials.py @@ -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'') + stubber.add_response(body=b'') with mock.patch.object( SSOTokenProvider, 'DEFAULT_CACHE_CLS', MockCache ): @@ -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'') + stubber.add_response(body=b'') s3.list_buckets() request = stubber.requests[0] assert self.ACCESS_KEY in str(request.headers.get('Authorization')) diff --git a/tests/functional/test_regions.py b/tests/functional/test_regions.py index be5f25087d..6e22cc75e5 100644 --- a/tests/functional/test_regions.py +++ b/tests/functional/test_regions.py @@ -502,7 +502,7 @@ 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'') + stubber.add_response(body=b'') client.list_buckets() self.assertEqual( stubber.requests[0].url, 'https://s3.us-east-2.amazonaws.com/' @@ -510,7 +510,7 @@ def test_regionalized_client_endpoint_resolution(self): def test_regionalized_client_with_unknown_region(self): client, stubber = self.create_stubbed_client('s3', 'not-real') - stubber.add_response(body=b'') + stubber.add_response(body=b'') client.list_buckets() # Validate we don't fall back to partition endpoint for # regionalized services. diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index a4d85db6d4..8a777a6f9a 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -553,7 +553,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'') + http_stubber.add_response(body=b'') self.client.list_objects(Bucket=accesspoint_arn) expected_endpoint = "myendpoint-123456789012.custom.com" self.assert_endpoint(http_stubber.requests[0], expected_endpoint) @@ -566,7 +566,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'') + http_stubber.add_response(body=b'') self.client.list_objects(Bucket=accesspoint_arn) expected_endpoint = "myendpoint-123456789012.custom.com" self.assert_endpoint(http_stubber.requests[0], expected_endpoint) @@ -615,7 +615,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'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=accesspoint_arn) self.assert_signing_region(self.http_stubber.requests[0], "us-west-2") @@ -739,7 +739,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'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=outpost_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-outposts") @@ -761,7 +761,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'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=outpost_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-outposts") @@ -965,7 +965,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'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_object_lambda_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-object-lambda") @@ -983,7 +983,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'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_object_lambda_arn) request = self.http_stubber.requests[0] self.assert_signing_name(request, "s3-object-lambda") @@ -1051,7 +1051,7 @@ def test_accesspoint_with_global_regions(self): config=Config(s3={"use_arn_region": True}), ) - self.http_stubber.add_response(body=b'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] expected_endpoint = ( @@ -1065,7 +1065,7 @@ def test_accesspoint_with_global_regions(self): region_name="s3-external-1", ) - self.http_stubber.add_response(body=b'') + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] expected_endpoint = ( @@ -1154,7 +1154,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] self._assert_sigv4a_used(request.headers) @@ -1241,7 +1241,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() + self.http_stubber.add_response(body=b'') self.client.list_objects(Bucket=arn) request = self.http_stubber.requests[0] self.assert_endpoint(request, expected) @@ -1545,7 +1545,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'') + self.http_stubber.add_response(body=b'') with self.http_stubber: self.client.list_objects(Bucket="foo") sent_headers = self.get_sent_headers() @@ -1563,7 +1563,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'') + self.http_stubber.add_response(body=b'') with self.http_stubber: self.client.list_objects(Bucket=bucket) sent_headers = self.get_sent_headers() @@ -2212,7 +2212,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'') + stub.add_response(body=b'') call = getattr(client, operation) call(**operation_kwargs) assert "Content-MD5" in stub.requests[-1].headers @@ -3658,7 +3658,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'') + self.http_stubber.add_response(body=b'') with self.http_stubber: self.client.delete_objects( Bucket="mybucket", diff --git a/tests/functional/test_s3express.py b/tests/functional/test_s3express.py index 0cf2307071..a6e04bf661 100644 --- a/tests/functional/test_s3express.py +++ b/tests/functional/test_s3express.py @@ -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'') + stubber.add_response(body=b'') default_s3_client.delete_objects( Bucket=S3EXPRESS_BUCKET, diff --git a/tests/functional/test_useragent.py b/tests/functional/test_useragent.py index 660a325318..90386e5157 100644 --- a/tests/functional/test_useragent.py +++ b/tests/functional/test_useragent.py @@ -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'') # expect exactly one request + self.add_response(body=b'') # expect exactly one request @property def captured_ua_string(self): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index b62577a406..79fbc497eb 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -1880,9 +1880,7 @@ def test_document_response_params_without_expires(document_expires_mocks): def operation_model_for_200_error(): operation_model = mock.Mock() operation_model.has_streaming_output = False - operation_model.output_shape = mock.Mock() - operation_model.output_shape.members = {'member': mock.Mock()} - operation_model.output_shape.members['member'].serialization = {} + operation_model.has_modeled_body_output = True return operation_model @@ -1944,9 +1942,7 @@ def test_200_response_with_streaming_output_left_untouched( def test_200_response_with_no_body_left_untouched( operation_model_for_200_error, response_dict, http_response ): - operation_model_for_200_error.output_shape.members[ - 'member' - ].serialization = {'location': 'header'} + operation_model_for_200_error.has_modeled_body_output = False handlers.check_for_200_error( operation_model_for_200_error, response_dict, http_response ) diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index da95a18bea..f5c75cb0cc 100644 --- a/tests/unit/test_model.py +++ b/tests/unit/test_model.py @@ -737,6 +737,91 @@ def test_not_streaming_output_for_operation(self): self.assertEqual(operation.get_streaming_output(), None) +class TestOperationModelBody(unittest.TestCase): + def setUp(self): + super().setUp() + self.model = { + 'operations': { + 'OperationName': { + 'name': 'OperationName', + 'input': { + 'shape': 'OperationRequest', + }, + 'output': { + 'shape': 'OperationResponse', + }, + }, + 'NoBodyOperation': { + 'name': 'NoBodyOperation', + 'input': {'shape': 'NoBodyOperationRequest'}, + 'output': {'shape': 'NoBodyOperationResponse'}, + }, + }, + 'shapes': { + 'OperationRequest': { + 'type': 'structure', + 'members': { + 'String': { + 'shape': 'stringType', + }, + "Body": { + 'shape': 'blobType', + }, + }, + 'payload': 'Body', + }, + 'OperationResponse': { + 'type': 'structure', + 'members': { + 'String': { + 'shape': 'stringType', + }, + "Body": { + 'shape': 'blobType', + }, + }, + 'payload': 'Body', + }, + 'NoBodyOperationRequest': { + 'type': 'structure', + 'members': { + 'data': { + 'location': 'header', + 'locationName': 'x-amz-data', + 'shape': 'stringType', + } + }, + }, + 'NoBodyOperationResponse': { + 'type': 'structure', + 'members': { + 'data': { + 'location': 'header', + 'locationName': 'x-amz-data', + 'shape': 'stringType', + } + }, + }, + 'stringType': { + 'type': 'string', + }, + 'blobType': {'type': 'blob'}, + }, + } + + def test_modeled_body_for_operation_with_body(self): + service_model = model.ServiceModel(self.model) + operation = service_model.operation_model('OperationName') + self.assertTrue(operation.has_modeled_body_input) + self.assertTrue(operation.has_modeled_body_output) + + def test_modeled_body_for_operation_with_no_body(self): + service_model = model.ServiceModel(self.model) + operation = service_model.operation_model('NoBodyOperation') + self.assertFalse(operation.has_modeled_body_input) + self.assertFalse(operation.has_modeled_body_output) + + class TestDeepMerge(unittest.TestCase): def setUp(self): self.shapes = { diff --git a/tests/unit/test_s3_addressing.py b/tests/unit/test_s3_addressing.py index d3f6d4f3f8..1629dbf880 100644 --- a/tests/unit/test_s3_addressing.py +++ b/tests/unit/test_s3_addressing.py @@ -49,7 +49,7 @@ def enable_hmacv1(self, **kwargs): def test_list_objects_dns_name(self): params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True, body=b'' + 'list_objects', params, force_hmacv1=True, body=b'' ) self.assertEqual( prepared_request.url, 'https://safename.s3.amazonaws.com/' @@ -58,7 +58,7 @@ def test_list_objects_dns_name(self): def test_list_objects_non_dns_name(self): params = {'Bucket': 'un_safe_name'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True, body=b'' + 'list_objects', params, force_hmacv1=True, body=b'' ) self.assertEqual( prepared_request.url, 'https://s3.amazonaws.com/un_safe_name' @@ -68,7 +68,7 @@ def test_list_objects_dns_name_non_classic(self): self.region_name = 'us-west-2' params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True, body=b'' + 'list_objects', params, force_hmacv1=True, body=b'' ) self.assertEqual( prepared_request.url, @@ -81,7 +81,7 @@ def test_list_objects_unicode_query_string_eu_central_1(self): [('Bucket', 'safename'), ('Marker', '\xe4\xf6\xfc-01.txt')] ) prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' + 'list_objects', params, body=b'' ) self.assertEqual( prepared_request.url, @@ -95,7 +95,7 @@ def test_list_objects_in_restricted_regions(self): self.region_name = 'us-gov-west-1' params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' + 'list_objects', params, body=b'' ) # Note how we keep the region specific endpoint here. self.assertEqual( @@ -107,7 +107,7 @@ def test_list_objects_in_fips(self): self.region_name = 'fips-us-gov-west-1' params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' + 'list_objects', params, body=b'' ) # Note how we keep the region specific endpoint here. self.assertEqual( @@ -119,7 +119,7 @@ def test_list_objects_non_dns_name_non_classic(self): self.region_name = 'us-west-2' params = {'Bucket': 'un_safe_name'} prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' + 'list_objects', params, body=b'' ) self.assertEqual( prepared_request.url, From aa936facbd8387698ad6c488ce1e744c1b03a908 Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Tue, 8 Oct 2024 17:15:51 -0400 Subject: [PATCH 04/20] fix test comment --- tests/unit/test_handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index 79fbc497eb..2b8a3ebea5 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -1934,7 +1934,7 @@ def test_200_response_with_streaming_output_left_untouched( operation_model.has_streaming_output = True http_response.content = "" handlers.check_for_200_error(operation_model, response_dict, http_response) - # We don't touch the status code on streaming operations + # We don't touch the status code on streaming operations. assert http_response.status_code == 200 assert response_dict['status_code'] == 200 From 9f975e625dc53a3b77550ab0960c4a62952a03df Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Tue, 15 Oct 2024 10:40:10 -0400 Subject: [PATCH 05/20] deprecate original handler. update implementation and tests --- botocore/endpoint.py | 6 ++- botocore/handlers.py | 50 +++++++++++++-------- tests/unit/test_handlers.py | 88 ++++++++++++++++++------------------- 3 files changed, 79 insertions(+), 65 deletions(-) diff --git a/botocore/endpoint.py b/botocore/endpoint.py index bdacbb5802..54442f4ad7 100644 --- a/botocore/endpoint.py +++ b/botocore/endpoint.py @@ -308,13 +308,17 @@ def _do_get_response(self, request, operation_model, context): operation_model=operation_model, response_dict=response_dict, customized_response_dict=customized_response_dict, - http_response=http_response, ) parser = self._response_parser_factory.create_parser(protocol) parsed_response = parser.parse( 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 diff --git a/botocore/handlers.py b/botocore/handlers.py index fe6b3c252e..18bbb79f28 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -131,9 +131,8 @@ def escape_xml_payload(params, **kwargs): params['body'] = body -def check_for_200_error( - operation_model, response_dict, http_response, **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 @@ -149,36 +148,28 @@ def check_for_200_error( # 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. - if ( - http_response is None - or operation_model.has_streaming_output - or not operation_model.has_modeled_body_output - ): + if response is None: # A None response can happen if an exception is raised while # trying to retrieve the response. See Endpoint._get_response(). - # Operations with streaming response blobs should be excluded as they - # may contain customer content which mimics the form of an S3 error. return + http_response, parsed = response if _looks_like_special_case_error(http_response): logger.debug( "Error found for response with 200 status code, " "errors: %s, changing status code to " "500.", - http_response.content, + parsed, ) http_response.status_code = 500 - # The response_dict status code must also be changed - # for it to be parsed as a 500 response. - response_dict['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: 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 @@ -1249,6 +1240,29 @@ 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 + ): + # 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 + customized_response_dict['updated_status_code'] = 500 + logger.debug( + f"Error found for response with 200 status code: {response_dict['body']}. " + f"Changing status code to 500." + ) + + # This is a list of (event_name, handler). # When a Session is created, everything in this list will be # automatically registered with that Session. @@ -1278,8 +1292,8 @@ def document_expires_shape(section, event_name, **kwargs): ('after-call.ec2.GetConsoleOutput', decode_console_output), ('after-call.cloudformation.GetTemplate', json_decode_template_body), ('after-call.s3.GetBucketLocation', parse_get_bucket_location), - ('before-parse.s3.*', check_for_200_error, REGISTER_FIRST), ('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), diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index 2b8a3ebea5..f4f1ec19c4 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -1885,73 +1885,69 @@ def operation_model_for_200_error(): @pytest.fixture() -def http_response(): - http_response = mock.Mock() - http_response.status_code = 200 - http_response.content = "" - return http_response - - -@pytest.fixture() -def response_dict(): - return {'status_code': 200} +def response_dict_for_200_error(): + return { + 'status_code': 200, + 'body': ( + b"" + b"SlowDown" + b"Please reduce your request rate." + b"" + ), + } def test_500_status_code_set_for_200_response( - operation_model_for_200_error, response_dict, http_response + operation_model_for_200_error, response_dict_for_200_error ): - http_response.content = """ - - AccessDenied - Access Denied - id - hostid - - """ - handlers.check_for_200_error( - operation_model_for_200_error, response_dict, http_response + customized_response_dict = {} + handlers._handle_200_error( + operation_model_for_200_error, + response_dict_for_200_error, + customized_response_dict, ) - assert http_response.status_code == 500 - assert response_dict['status_code'] == 500 + assert response_dict_for_200_error['status_code'] == 500 + assert customized_response_dict.get('updated_status_code') == 500 def test_200_response_with_no_error_left_untouched( - operation_model_for_200_error, response_dict, http_response + operation_model_for_200_error, response_dict_for_200_error ): - http_response.content = "" - handlers.check_for_200_error( - operation_model_for_200_error, response_dict, http_response + response_dict_for_200_error['body'] = b"" + customized_response_dict = {} + handlers._handle_200_error( + operation_model_for_200_error, + response_dict_for_200_error, + customized_response_dict, ) # We don't touch the status code since there are no errors present. - assert http_response.status_code == 200 - assert response_dict['status_code'] == 200 + assert response_dict_for_200_error['status_code'] == 200 + assert customized_response_dict == {} def test_200_response_with_streaming_output_left_untouched( - response_dict, http_response + response_dict_for_200_error, ): operation_model = mock.Mock() operation_model.has_streaming_output = True - http_response.content = "" - handlers.check_for_200_error(operation_model, response_dict, http_response) + customized_response_dict = {} + handlers._handle_200_error( + operation_model, response_dict_for_200_error, customized_response_dict + ) # We don't touch the status code on streaming operations. - assert http_response.status_code == 200 - assert response_dict['status_code'] == 200 + assert response_dict_for_200_error['status_code'] == 200 + assert customized_response_dict == {} def test_200_response_with_no_body_left_untouched( - operation_model_for_200_error, response_dict, http_response + operation_model_for_200_error, response_dict_for_200_error ): operation_model_for_200_error.has_modeled_body_output = False - handlers.check_for_200_error( - operation_model_for_200_error, response_dict, http_response + customized_response_dict = {} + handlers._handle_200_error( + operation_model_for_200_error, + response_dict_for_200_error, + customized_response_dict, ) - assert http_response.status_code == 200 - assert response_dict['status_code'] == 200 - - -def test_500_response_can_be_none(): - # A 500 response can raise an exception, which means the response - # object is None. We need to handle this case. - operation_model = mock.Mock() - handlers.check_for_200_error(operation_model, None, None) + assert response_dict_for_200_error['status_code'] == 200 + assert customized_response_dict == {} From 5948f9bec961a715e4b8d8aa0337d44fc1a37cc4 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Wed, 16 Oct 2024 17:57:48 -0400 Subject: [PATCH 06/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 18bbb79f28..4daf14bc35 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -164,7 +164,7 @@ def check_for_200_error(response, **kwargs): def _looks_like_special_case_error(status_code, body): - if status_code == 200: + if status_code == 200 and body: try: parser = ETree.XMLParser( target=ETree.TreeBuilder(), encoding='utf-8' From e258a3f9ec20b4979239af48b0feadca8e9417db Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Wed, 16 Oct 2024 18:03:59 -0400 Subject: [PATCH 07/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 4daf14bc35..3639608652 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1256,7 +1256,6 @@ def _handle_200_error( ): # The response_dict status code must be changed to be parsed as a 500 response. response_dict['status_code'] = 500 - customized_response_dict['updated_status_code'] = 500 logger.debug( f"Error found for response with 200 status code: {response_dict['body']}. " f"Changing status code to 500." From 6269bf5eac8900756c134e55fbd407f467969b6e Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Wed, 16 Oct 2024 18:16:41 -0400 Subject: [PATCH 08/20] Update botocore/model.py Co-authored-by: Nate Prewitt --- botocore/model.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/botocore/model.py b/botocore/model.py index f4070fb6ee..677266c8d2 100644 --- a/botocore/model.py +++ b/botocore/model.py @@ -707,26 +707,6 @@ def _get_streaming_body(self, shape): return payload_shape return None - @CachedProperty - def has_modeled_body_input(self): - return self._has_modeled_body(self.input_shape) - - @CachedProperty - def has_modeled_body_output(self): - return self._has_modeled_body(self.output_shape) - - def _has_modeled_body(self, shape): - """ - Determines if an operation has a modeled body. - If any member is not bound to a location, we can expect a body. - """ - if shape is None: - return False - for member_shape in shape.members.values(): - if not member_shape.serialization.get('location'): - return True - return False - def __repr__(self): return f'{self.__class__.__name__}(name={self.name})' From 44ad31dd5dd716d3d9cce355d4b187915a5169d3 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Wed, 16 Oct 2024 18:16:51 -0400 Subject: [PATCH 09/20] Update tests/functional/test_s3.py Co-authored-by: Nate Prewitt --- tests/functional/test_s3.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index 8a777a6f9a..422e74a455 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -471,11 +471,9 @@ def test_s3_copy_object_with_200_error_response(self): b"Please reduce your request rate." b"" ) - self.http_stubber.add_response(status=200, body=error_body) - self.http_stubber.add_response(status=200, body=error_body) - self.http_stubber.add_response(status=200, body=error_body) - self.http_stubber.add_response(status=200, body=error_body) - self.http_stubber.add_response(status=200, body=error_body) + # Populate 5 retries for SlowDown + 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", From bd5d4ad719e52cfe40c163ffe7b5ccdc54767191 Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Thu, 17 Oct 2024 10:32:14 -0400 Subject: [PATCH 10/20] update implementation and clean up tests --- botocore/endpoint.py | 5 -- botocore/handlers.py | 37 +++++--- .../test_configured_endpoint_url.py | 4 +- tests/functional/test_credentials.py | 4 +- tests/functional/test_regions.py | 4 +- tests/functional/test_s3.py | 37 ++++---- tests/functional/test_s3express.py | 2 +- tests/functional/test_useragent.py | 2 +- tests/unit/test_handlers.py | 87 ++++++++++++++----- tests/unit/test_model.py | 85 ------------------ tests/unit/test_s3_addressing.py | 28 ++---- 11 files changed, 126 insertions(+), 169 deletions(-) diff --git a/botocore/endpoint.py b/botocore/endpoint.py index 54442f4ad7..1c2cee068b 100644 --- a/botocore/endpoint.py +++ b/botocore/endpoint.py @@ -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 diff --git a/botocore/handlers.py b/botocore/handlers.py index 3639608652..2d17875cd3 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -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. @@ -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), diff --git a/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py b/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py index 51649d5bbc..40cc17e26d 100644 --- a/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py +++ b/tests/functional/configured_endpoint_urls/test_configured_endpoint_url.py @@ -147,9 +147,7 @@ def assert_endpoint_url_used_for_operation( ): http_stubber = ClientHTTPStubber(client) http_stubber.start() - http_stubber.add_response( - body=(b'' if operation == 'list_buckets' else None) - ) + http_stubber.add_response() # Call an operation on the client getattr(client, operation)(**params) diff --git a/tests/functional/test_credentials.py b/tests/functional/test_credentials.py index a45a73f14e..a6ba1f8b4b 100644 --- a/tests/functional/test_credentials.py +++ b/tests/functional/test_credentials.py @@ -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'') + stubber.add_response() with mock.patch.object( SSOTokenProvider, 'DEFAULT_CACHE_CLS', MockCache ): @@ -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'') + stubber.add_response() s3.list_buckets() request = stubber.requests[0] assert self.ACCESS_KEY in str(request.headers.get('Authorization')) diff --git a/tests/functional/test_regions.py b/tests/functional/test_regions.py index 6e22cc75e5..11a882f91f 100644 --- a/tests/functional/test_regions.py +++ b/tests/functional/test_regions.py @@ -502,7 +502,7 @@ 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'') + stubber.add_response() client.list_buckets() self.assertEqual( stubber.requests[0].url, 'https://s3.us-east-2.amazonaws.com/' @@ -510,7 +510,7 @@ def test_regionalized_client_endpoint_resolution(self): def test_regionalized_client_with_unknown_region(self): client, stubber = self.create_stubbed_client('s3', 'not-real') - stubber.add_response(body=b'') + stubber.add_response() client.list_buckets() # Validate we don't fall back to partition endpoint for # regionalized services. diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index 422e74a455..04908b0551 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -434,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", @@ -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'') + 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) @@ -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'') + 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) @@ -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'') + self.http_stubber.add_response() self.client.list_objects(Bucket=accesspoint_arn) self.assert_signing_region(self.http_stubber.requests[0], "us-west-2") @@ -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'') + 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") @@ -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'') + 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") @@ -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'') + 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") @@ -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'') + 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") @@ -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'') + self.http_stubber.add_response() self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] expected_endpoint = ( @@ -1063,7 +1062,7 @@ def test_accesspoint_with_global_regions(self): region_name="s3-external-1", ) - self.http_stubber.add_response(body=b'') + self.http_stubber.add_response() self.client.list_objects(Bucket=s3_accesspoint_arn) request = self.http_stubber.requests[0] expected_endpoint = ( @@ -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'') + 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) @@ -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'') + self.http_stubber.add_response() self.client.list_objects(Bucket=arn) request = self.http_stubber.requests[0] self.assert_endpoint(request, expected) @@ -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'') + self.http_stubber.add_response() with self.http_stubber: self.client.list_objects(Bucket="foo") sent_headers = self.get_sent_headers() @@ -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'') + self.http_stubber.add_response() with self.http_stubber: self.client.list_objects(Bucket=bucket) sent_headers = self.get_sent_headers() @@ -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'') + stub.add_response() call = getattr(client, operation) call(**operation_kwargs) assert "Content-MD5" in stub.requests[-1].headers @@ -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'') + self.http_stubber.add_response() with self.http_stubber: self.client.delete_objects( Bucket="mybucket", diff --git a/tests/functional/test_s3express.py b/tests/functional/test_s3express.py index a6e04bf661..390721ee12 100644 --- a/tests/functional/test_s3express.py +++ b/tests/functional/test_s3express.py @@ -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'') + stubber.add_response() default_s3_client.delete_objects( Bucket=S3EXPRESS_BUCKET, diff --git a/tests/functional/test_useragent.py b/tests/functional/test_useragent.py index 90386e5157..79290459ab 100644 --- a/tests/functional/test_useragent.py +++ b/tests/functional/test_useragent.py @@ -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'') # expect exactly one request + self.add_response() # expect exactly one request @property def captured_ua_string(self): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index f4f1ec19c4..f75eb1cb1c 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -1148,6 +1148,49 @@ def test_non_ascii_characters(self): ) +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): + client = self.session.create_client('s3') + service_model = self.session.get_service_model('s3') + operation = service_model.operation_model('CopyObject') + responses = client.meta.events.emit( + 'needs-retry.s3.CopyObject', + request_dict={'context': {}}, + 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 _retry_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('_retry_200_error', names) + self.assertIn('RetryHandler', names) + s3_200_handler = names.index('_retry_200_error') + general_retry_handler = names.index('RetryHandler') + self.assertTrue( + s3_200_handler < general_retry_handler, + "S3 200 error handler was supposed to be before " + "the general retry handler, but it was not.", + ) + + class BaseMD5Test(BaseSessionTest): def setUp(self, **environ): super().setUp(**environ) @@ -1880,7 +1923,6 @@ def test_document_response_params_without_expires(document_expires_mocks): def operation_model_for_200_error(): operation_model = mock.Mock() operation_model.has_streaming_output = False - operation_model.has_modeled_body_output = True return operation_model @@ -1900,54 +1942,55 @@ def response_dict_for_200_error(): def test_500_status_code_set_for_200_response( operation_model_for_200_error, response_dict_for_200_error ): - customized_response_dict = {} handlers._handle_200_error( - operation_model_for_200_error, - response_dict_for_200_error, - customized_response_dict, + operation_model_for_200_error, response_dict_for_200_error ) assert response_dict_for_200_error['status_code'] == 500 - assert customized_response_dict.get('updated_status_code') == 500 def test_200_response_with_no_error_left_untouched( operation_model_for_200_error, response_dict_for_200_error ): response_dict_for_200_error['body'] = b"" - customized_response_dict = {} handlers._handle_200_error( - operation_model_for_200_error, - response_dict_for_200_error, - customized_response_dict, + operation_model_for_200_error, response_dict_for_200_error ) # We don't touch the status code since there are no errors present. assert response_dict_for_200_error['status_code'] == 200 - assert customized_response_dict == {} def test_200_response_with_streaming_output_left_untouched( + operation_model_for_200_error, response_dict_for_200_error, ): - operation_model = mock.Mock() - operation_model.has_streaming_output = True - customized_response_dict = {} + operation_model_for_200_error.has_streaming_output = True handlers._handle_200_error( - operation_model, response_dict_for_200_error, customized_response_dict + operation_model_for_200_error, response_dict_for_200_error ) # We don't touch the status code on streaming operations. assert response_dict_for_200_error['status_code'] == 200 - assert customized_response_dict == {} def test_200_response_with_no_body_left_untouched( operation_model_for_200_error, response_dict_for_200_error ): - operation_model_for_200_error.has_modeled_body_output = False - customized_response_dict = {} + response_dict_for_200_error['body'] = b"" handlers._handle_200_error( - operation_model_for_200_error, - response_dict_for_200_error, - customized_response_dict, + operation_model_for_200_error, response_dict_for_200_error ) assert response_dict_for_200_error['status_code'] == 200 - assert customized_response_dict == {} + + +def test_http_status_code_updated_to_retry_200_response(): + http_response = mock.Mock() + http_response.status_code = 200 + parsed = {} + parsed.setdefault('ResponseMetadata', {})['HTTPStatusCode'] = 500 + handlers._retry_200_error((http_response, parsed)) + assert http_response.status_code == 500 + + +def test_500_response_can_be_none(): + # A 500 response can raise an exception, which means the response + # object is None. We need to handle this case. + handlers._retry_200_error(None) diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index f5c75cb0cc..da95a18bea 100644 --- a/tests/unit/test_model.py +++ b/tests/unit/test_model.py @@ -737,91 +737,6 @@ def test_not_streaming_output_for_operation(self): self.assertEqual(operation.get_streaming_output(), None) -class TestOperationModelBody(unittest.TestCase): - def setUp(self): - super().setUp() - self.model = { - 'operations': { - 'OperationName': { - 'name': 'OperationName', - 'input': { - 'shape': 'OperationRequest', - }, - 'output': { - 'shape': 'OperationResponse', - }, - }, - 'NoBodyOperation': { - 'name': 'NoBodyOperation', - 'input': {'shape': 'NoBodyOperationRequest'}, - 'output': {'shape': 'NoBodyOperationResponse'}, - }, - }, - 'shapes': { - 'OperationRequest': { - 'type': 'structure', - 'members': { - 'String': { - 'shape': 'stringType', - }, - "Body": { - 'shape': 'blobType', - }, - }, - 'payload': 'Body', - }, - 'OperationResponse': { - 'type': 'structure', - 'members': { - 'String': { - 'shape': 'stringType', - }, - "Body": { - 'shape': 'blobType', - }, - }, - 'payload': 'Body', - }, - 'NoBodyOperationRequest': { - 'type': 'structure', - 'members': { - 'data': { - 'location': 'header', - 'locationName': 'x-amz-data', - 'shape': 'stringType', - } - }, - }, - 'NoBodyOperationResponse': { - 'type': 'structure', - 'members': { - 'data': { - 'location': 'header', - 'locationName': 'x-amz-data', - 'shape': 'stringType', - } - }, - }, - 'stringType': { - 'type': 'string', - }, - 'blobType': {'type': 'blob'}, - }, - } - - def test_modeled_body_for_operation_with_body(self): - service_model = model.ServiceModel(self.model) - operation = service_model.operation_model('OperationName') - self.assertTrue(operation.has_modeled_body_input) - self.assertTrue(operation.has_modeled_body_output) - - def test_modeled_body_for_operation_with_no_body(self): - service_model = model.ServiceModel(self.model) - operation = service_model.operation_model('NoBodyOperation') - self.assertFalse(operation.has_modeled_body_input) - self.assertFalse(operation.has_modeled_body_output) - - class TestDeepMerge(unittest.TestCase): def setUp(self): self.shapes = { diff --git a/tests/unit/test_s3_addressing.py b/tests/unit/test_s3_addressing.py index 1629dbf880..7150d5b4ce 100644 --- a/tests/unit/test_s3_addressing.py +++ b/tests/unit/test_s3_addressing.py @@ -31,14 +31,12 @@ def setUp(self): set_list_objects_encoding_type_url, ) - def get_prepared_request( - self, operation, params, force_hmacv1=False, body=None - ): + def get_prepared_request(self, operation, params, force_hmacv1=False): if force_hmacv1: self.session.register('choose-signer', self.enable_hmacv1) client = self.session.create_client('s3', self.region_name) with ClientHTTPStubber(client) as http_stubber: - http_stubber.add_response(body=body) + http_stubber.add_response() getattr(client, operation)(**params) # Return the request that was sent over the wire. return http_stubber.requests[0] @@ -49,7 +47,7 @@ def enable_hmacv1(self, **kwargs): def test_list_objects_dns_name(self): params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True, body=b'' + 'list_objects', params, force_hmacv1=True ) self.assertEqual( prepared_request.url, 'https://safename.s3.amazonaws.com/' @@ -58,7 +56,7 @@ def test_list_objects_dns_name(self): def test_list_objects_non_dns_name(self): params = {'Bucket': 'un_safe_name'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True, body=b'' + 'list_objects', params, force_hmacv1=True ) self.assertEqual( prepared_request.url, 'https://s3.amazonaws.com/un_safe_name' @@ -68,7 +66,7 @@ def test_list_objects_dns_name_non_classic(self): self.region_name = 'us-west-2' params = {'Bucket': 'safename'} prepared_request = self.get_prepared_request( - 'list_objects', params, force_hmacv1=True, body=b'' + 'list_objects', params, force_hmacv1=True ) self.assertEqual( prepared_request.url, @@ -80,9 +78,7 @@ def test_list_objects_unicode_query_string_eu_central_1(self): params = OrderedDict( [('Bucket', 'safename'), ('Marker', '\xe4\xf6\xfc-01.txt')] ) - prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' - ) + prepared_request = self.get_prepared_request('list_objects', params) self.assertEqual( prepared_request.url, ( @@ -94,9 +90,7 @@ def test_list_objects_unicode_query_string_eu_central_1(self): def test_list_objects_in_restricted_regions(self): self.region_name = 'us-gov-west-1' params = {'Bucket': 'safename'} - prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' - ) + prepared_request = self.get_prepared_request('list_objects', params) # Note how we keep the region specific endpoint here. self.assertEqual( prepared_request.url, @@ -106,9 +100,7 @@ def test_list_objects_in_restricted_regions(self): def test_list_objects_in_fips(self): self.region_name = 'fips-us-gov-west-1' params = {'Bucket': 'safename'} - prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' - ) + prepared_request = self.get_prepared_request('list_objects', params) # Note how we keep the region specific endpoint here. self.assertEqual( prepared_request.url, @@ -118,9 +110,7 @@ def test_list_objects_in_fips(self): def test_list_objects_non_dns_name_non_classic(self): self.region_name = 'us-west-2' params = {'Bucket': 'un_safe_name'} - prepared_request = self.get_prepared_request( - 'list_objects', params, body=b'' - ) + prepared_request = self.get_prepared_request('list_objects', params) self.assertEqual( prepared_request.url, 'https://s3.us-west-2.amazonaws.com/un_safe_name', From 0fdbd8220dc8d77206ba43698de55189b6ebc8bf Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 13:31:05 -0400 Subject: [PATCH 11/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 2d17875cd3..d14ef7facb 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1255,9 +1255,7 @@ def _handle_200_error(operation_model, response_dict, **kwargs): ): response_dict['status_code'] = 500 logger.debug( - f"Error found for response with 200 status code: {response_dict['body']}. " - f"Changing the http_response status code to 500 will be propagated in " - f"the _retry_200_error handler." + f"Error found for response with 200 status code: {response_dict['body']}." ) From 46d0f2e4bebae2937b8ad8fa0ebebbbdc457ce86 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 13:31:34 -0400 Subject: [PATCH 12/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index d14ef7facb..4f4a60e01b 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1247,8 +1247,8 @@ def _handle_200_error(operation_model, response_dict, **kwargs): # 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. + # 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'] From d7dcfa17cf306e522bcefaca336e4df4f70e3077 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 13:31:52 -0400 Subject: [PATCH 13/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 4f4a60e01b..881d010b6a 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1241,11 +1241,8 @@ def document_expires_shape(section, event_name, **kwargs): 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. + # S3 can return a 200 response with an error embedded in the body. + # Convert the 200 to a 500 for retry resolution in ``_retry_200_error``. 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. From d21ce1b030319d92044d308e926f806c39982fe6 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 13:32:00 -0400 Subject: [PATCH 14/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 881d010b6a..585fd2bb13 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1257,12 +1257,8 @@ def _handle_200_error(operation_model, response_dict, **kwargs): 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. + # 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 From 9cfa05963b1d3954190d8157081328b7cfa4bc94 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 14:35:47 -0400 Subject: [PATCH 15/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 585fd2bb13..48014423da 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1256,7 +1256,7 @@ def _handle_200_error(operation_model, response_dict, **kwargs): ) -def _retry_200_error(response, **kwargs): +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: From 4a51dc96019a7c14618422f54e2340f8fc64d101 Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 18:02:33 -0400 Subject: [PATCH 16/20] Update .changes/next-release/enhancement-s3-47846.json Co-authored-by: Nate Prewitt --- .changes/next-release/enhancement-s3-47846.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/next-release/enhancement-s3-47846.json b/.changes/next-release/enhancement-s3-47846.json index 2c93767355..a549b5e190 100644 --- a/.changes/next-release/enhancement-s3-47846.json +++ b/.changes/next-release/enhancement-s3-47846.json @@ -1,5 +1,5 @@ { "type": "enhancement", "category": "``s3``", - "description": "Adds logic to gracefully handle HTTP 200 OK responses that contain error information in the response body." + "description": "Handle HTTP 200 responses with error information for all supported s3 operations." } From ee4dda54c57718d6437b65ef00fa54cd9c04742f Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Thu, 17 Oct 2024 18:35:05 -0400 Subject: [PATCH 17/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index 48014423da..de7b7e1383 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1242,7 +1242,7 @@ 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 ``_retry_200_error``. + # 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. From 1e16e8bdade0af64e86ca60ce1dc4853b762b411 Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Fri, 18 Oct 2024 10:58:00 -0400 Subject: [PATCH 18/20] drop unit tests in favor of functional tests --- botocore/handlers.py | 6 +- tests/functional/test_s3.py | 57 ++++++++++++++++++- tests/unit/test_handlers.py | 110 ++++++++++-------------------------- 3 files changed, 89 insertions(+), 84 deletions(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index de7b7e1383..c60ae31656 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -153,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 " @@ -1343,7 +1345,7 @@ 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.*', _retry_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 04908b0551..613ef1c016 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -460,7 +460,20 @@ def test_s3_copy_object_with_incomplete_response(self): self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200) self.assertTrue("CopyObjectResult" in response) - def test_s3_copy_object_with_200_error_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" ) @@ -470,7 +483,8 @@ def test_s3_copy_object_with_200_error_response(self): b"Please reduce your request rate." b"" ) - # Populate 5 retries for SlowDown + # 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: @@ -488,6 +502,45 @@ def test_s3_copy_object_with_200_error_response(self): 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): def setUp(self): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index f75eb1cb1c..f71b250289 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -501,6 +501,33 @@ def test_presigned_url_casing_changed_for_rds(self): ) self.assertIn('X-Amz-Signature', params['PreSignedUrl']) + def test_500_status_code_set_for_200_response(self): + http_response = mock.Mock() + http_response.status_code = 200 + http_response.content = """ + + AccessDenied + Access Denied + id + hostid + + """ + handlers.check_for_200_error((http_response, {})) + 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 + http_response.content = "" + handlers.check_for_200_error((http_response, {})) + # We don't touch the status code since there are no errors present. + self.assertEqual(http_response.status_code, 200) + + def test_500_response_can_be_none(self): + # A 500 response can raise an exception, which means the response + # object is None. We need to handle this case. + handlers.check_for_200_error(None) + def test_route53_resource_id(self): event = 'before-parameter-build.route53.GetHostedZone' params = { @@ -1175,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 _retry_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('_retry_200_error', names) + self.assertIn('_update_status_code', names) self.assertIn('RetryHandler', names) - s3_200_handler = names.index('_retry_200_error') + s3_200_handler = names.index('_update_status_code') general_retry_handler = names.index('RetryHandler') self.assertTrue( s3_200_handler < general_retry_handler, @@ -1917,80 +1944,3 @@ def test_document_response_params_without_expires(document_expires_mocks): mocks['section'].get_section.assert_not_called() mocks['param_section'].add_new_section.assert_not_called() mocks['doc_section'].write.assert_not_called() - - -@pytest.fixture() -def operation_model_for_200_error(): - operation_model = mock.Mock() - operation_model.has_streaming_output = False - return operation_model - - -@pytest.fixture() -def response_dict_for_200_error(): - return { - 'status_code': 200, - 'body': ( - b"" - b"SlowDown" - b"Please reduce your request rate." - b"" - ), - } - - -def test_500_status_code_set_for_200_response( - operation_model_for_200_error, response_dict_for_200_error -): - handlers._handle_200_error( - operation_model_for_200_error, response_dict_for_200_error - ) - assert response_dict_for_200_error['status_code'] == 500 - - -def test_200_response_with_no_error_left_untouched( - operation_model_for_200_error, response_dict_for_200_error -): - response_dict_for_200_error['body'] = b"" - handlers._handle_200_error( - operation_model_for_200_error, response_dict_for_200_error - ) - # We don't touch the status code since there are no errors present. - assert response_dict_for_200_error['status_code'] == 200 - - -def test_200_response_with_streaming_output_left_untouched( - operation_model_for_200_error, - response_dict_for_200_error, -): - operation_model_for_200_error.has_streaming_output = True - handlers._handle_200_error( - operation_model_for_200_error, response_dict_for_200_error - ) - # We don't touch the status code on streaming operations. - assert response_dict_for_200_error['status_code'] == 200 - - -def test_200_response_with_no_body_left_untouched( - operation_model_for_200_error, response_dict_for_200_error -): - response_dict_for_200_error['body'] = b"" - handlers._handle_200_error( - operation_model_for_200_error, response_dict_for_200_error - ) - assert response_dict_for_200_error['status_code'] == 200 - - -def test_http_status_code_updated_to_retry_200_response(): - http_response = mock.Mock() - http_response.status_code = 200 - parsed = {} - parsed.setdefault('ResponseMetadata', {})['HTTPStatusCode'] = 500 - handlers._retry_200_error((http_response, parsed)) - assert http_response.status_code == 500 - - -def test_500_response_can_be_none(): - # A 500 response can raise an exception, which means the response - # object is None. We need to handle this case. - handlers._retry_200_error(None) From 9594dca6b121fed36ece01c3bfd284b1bb51a89a Mon Sep 17 00:00:00 2001 From: Alessandra Romero Date: Fri, 18 Oct 2024 11:05:34 -0400 Subject: [PATCH 19/20] fix extra space in handlers test --- tests/unit/test_handlers.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index f71b250289..924abed8af 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -505,13 +505,13 @@ def test_500_status_code_set_for_200_response(self): http_response = mock.Mock() http_response.status_code = 200 http_response.content = """ - - AccessDenied - Access Denied - id - hostid - - """ + + AccessDenied + Access Denied + id + hostid + + """ handlers.check_for_200_error((http_response, {})) self.assertEqual(http_response.status_code, 500) From 017bacf494eac0e40945fb2a0304ee980b3c0d7a Mon Sep 17 00:00:00 2001 From: Alessandra Romero <24320222+alexgromero@users.noreply.github.com> Date: Fri, 18 Oct 2024 11:36:47 -0400 Subject: [PATCH 20/20] Update botocore/handlers.py Co-authored-by: Nate Prewitt --- botocore/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/botocore/handlers.py b/botocore/handlers.py index c60ae31656..9a7f78acef 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1265,7 +1265,7 @@ def _update_status_code(response, **kwargs): return http_response, parsed = response parsed_status_code = parsed.get('ResponseMetadata', {}).get( - 'HTTPStatusCode' + 'HTTPStatusCode', http_response.status_code ) if http_response.status_code != parsed_status_code: http_response.status_code = parsed_status_code