From d4bbb0ebc3b814c43109c713aace306588991c1d Mon Sep 17 00:00:00 2001 From: davidlm Date: Wed, 12 Jul 2023 10:27:23 -0400 Subject: [PATCH] pr feedback --- botocore/args.py | 22 ++++++++--- botocore/compress.py | 79 +++++++++++++++++++------------------ botocore/config.py | 4 +- botocore/configprovider.py | 2 - tests/unit/test_compress.py | 42 ++++++++++++++++---- 5 files changed, 95 insertions(+), 54 deletions(-) diff --git a/botocore/args.py b/botocore/args.py index 7164e8975b..86e2efe478 100644 --- a/botocore/args.py +++ b/botocore/args.py @@ -59,6 +59,10 @@ # Maximum allowed length of the ``user_agent_appid`` config field. Longer # values result in a warning-level log message. USERAGENT_APPID_MAXLEN = 50 +# Minimum and maximum allowed value of the `request_min_compression_size_bytes` +# config field. Lower or higher values result in an InvalidConfigError raised. +REQUEST_MIN_COMPRESSION_SIZE_BYTES_MIN = 0 +REQUEST_MIN_COMPRESSION_SIZE_BYTES_MAX = 10485760 class ClientArgsCreator: @@ -575,19 +579,27 @@ def _validate_min_compression_size(self, min_size): if min_size is not None: error_msg_base = ( f'Invalid value "{min_size}" for ' - 'request_min_compression_size_bytes. ' + 'request_min_compression_size_bytes.' ) try: min_size = int(min_size) except ValueError: raise botocore.exceptions.InvalidConfigError( - error_msg=f'{error_msg_base}Value must be an integer.' + error_msg=( + f'{error_msg_base} Value must be an integer.' + f' Received {type(min_size)} instead.' + ) ) - if not 0 <= min_size <= 1048576: + if ( + not REQUEST_MIN_COMPRESSION_SIZE_BYTES_MIN + <= min_size + <= REQUEST_MIN_COMPRESSION_SIZE_BYTES_MAX + ): raise botocore.exceptions.InvalidConfigError( error_msg=( - f'{error_msg_base}Value must ' - 'be between 0 and 1048576.' + f'{error_msg_base} Value must be between ' + f'{REQUEST_MIN_COMPRESSION_SIZE_BYTES_MIN} and ' + f'{REQUEST_MIN_COMPRESSION_SIZE_BYTES_MAX}.' ) ) diff --git a/botocore/compress.py b/botocore/compress.py index 4900236f86..8c6f3cf8f1 100644 --- a/botocore/compress.py +++ b/botocore/compress.py @@ -25,33 +25,47 @@ class RequestCompressor: @classmethod def compress(cls, config, request_dict, operation_model): - """Compresses the request body using the specified encodings. - - Check if the request should be compressed based on the contents of its - body and config settings. Set or append the `Content-Encoding` header - with the matched encoding if not present. - """ + """Attempt to compress the request body using the modeled encodings.""" body = request_dict['body'] if cls._should_compress_request(config, body, operation_model): - encodings = operation_model.request_compression['encodings'] - headers = request_dict['headers'] - for encoding in encodings: + for i, encoding in enumerate( + operation_model.request_compression['encodings'] + ): encoder = getattr(cls, f'_{encoding}_compress_body', None) if encoder is not None: - ce_header = headers.get('Content-Encoding') - if ce_header is None: - headers['Content-Encoding'] = encoding - elif encoding not in ce_header.split(','): - headers['Content-Encoding'] = f'{ce_header},{encoding}' logger.debug( - 'Compressing request with %s encoding', encoding + 'Compressing request with %s encoding.', encoding ) request_dict['body'] = encoder(body) + cls._set_compression_header( + request_dict['headers'], encoding + ) + return else: logger.debug( - 'Unsupported compression encoding: %s' % encoding + 'Unsupported compression encoding: %s', encoding ) + @classmethod + def _should_compress_request(cls, config, body, operation_model): + if ( + config.disable_request_compression is not True + and config.signature_version != 'v2' + and operation_model.request_compression is not None + ): + # Request is compressed no matter the content length if it has a streaming input. + # However, if the stream has the `requiresLength` trait it is NOT compressed. + if operation_model.has_streaming_input: + return ( + 'requiresLength' + not in operation_model.get_streaming_input().metadata + ) + return ( + config.request_min_compression_size_bytes + <= cls._get_body_size(body) + ) + return False + @classmethod def _gzip_compress_body(cls, body): if isinstance(body, str): @@ -80,33 +94,22 @@ def _gzip_compress_fileobj(body): compressed_obj.seek(0) return compressed_obj - @classmethod - def _should_compress_request(cls, config, body, operation_model): - if ( - config.disable_request_compression is not True - and config.signature_version != 'v2' - and operation_model.request_compression is not None - ): - # Request is compressed no matter the content length if it has a streaming input. - # However, if the stream has the `requiresLength` trait it is NOT compressed. - if operation_model.has_streaming_input: - return ( - 'requiresLength' - not in operation_model.get_streaming_input().metadata - ) - return ( - config.request_min_compression_size_bytes - <= cls._get_body_size(body) - ) - return False - @staticmethod def _get_body_size(body): size = determine_content_length(body) if size is None: logger.debug( - 'Unable to get length of the request body: %s. Not compressing.' - % body + 'Unable to get length of the request body: %s. ' + 'Skipping compression.', + body, ) return -1 return size + + @staticmethod + def _set_compression_header(headers, encoding): + ce_header = headers.get('Content-Encoding') + if ce_header is None: + headers['Content-Encoding'] = encoding + elif encoding not in ce_header.split(','): + headers['Content-Encoding'] = f'{ce_header},{encoding}' diff --git a/botocore/config.py b/botocore/config.py index de31321d85..1abea5cee7 100644 --- a/botocore/config.py +++ b/botocore/config.py @@ -217,8 +217,8 @@ class Config: Defaults to None. :type disable_request_compression: bool - :param disable_request_compression: Disables the request body compression - if set to True. + :param disable_request_compression: Disables request body compression if + set to True. Defaults to None. """ diff --git a/botocore/configprovider.py b/botocore/configprovider.py index 778507775d..1f9155e320 100644 --- a/botocore/configprovider.py +++ b/botocore/configprovider.py @@ -147,8 +147,6 @@ # whatever the defaults are in _retry.json. 'max_attempts': ('max_attempts', 'AWS_MAX_ATTEMPTS', None, int), 'user_agent_appid': ('sdk_ua_app_id', 'AWS_SDK_UA_APP_ID', None, None), - # This must be a parsable integer between 0 and 1048576, but validation - # is performed during client initialization instead of here. 'request_min_compression_size_bytes': ( 'request_min_compression_size_bytes', 'AWS_REQUEST_MIN_COMPRESSION_SIZE_BYTES', diff --git a/tests/unit/test_compress.py b/tests/unit/test_compress.py index 0d7f294aff..7a65c0e9ab 100644 --- a/tests/unit/test_compress.py +++ b/tests/unit/test_compress.py @@ -10,6 +10,7 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import gzip import io import pytest @@ -26,6 +27,13 @@ def _op_with_compression(): return op +def _op_with_multiple_compressions(): + op = mock.Mock() + op.request_compression = {'encodings': ['gzip', 'gzip']} + op.has_streaming_input = False + return op + + def _op_unknown_compression(): op = mock.Mock() op.request_compression = {'encodings': ['foo']} @@ -94,15 +102,25 @@ def request_dict_with_content_encoding_header(): } -COMPRESSION_HEADERS = {'gzip': b'\x1f\x8b'} +COMPRESSION_INFO = { + 'gzip': {'headers': b'\x1f\x8b', 'decompress_method': gzip.decompress} +} -def _assert_compression(is_compressed, body, encoding): +def _assert_compression(is_compressed, body, maybe_compressed_body, encoding): if hasattr(body, 'read'): - header = body.read(2) - else: - header = body[:2] - assert is_compressed == (header == COMPRESSION_HEADERS.get(encoding)) + body = body.read() + maybe_compressed_body = maybe_compressed_body.read() + if isinstance(body, str): + body = body.encode('utf-8') + compression_info = COMPRESSION_INFO.get(encoding, {}) + assert is_compressed == ( + maybe_compressed_body[:2] == compression_info.get('headers') + ) + decompress_method = compression_info.get( + 'decompress_method', lambda body: body + ) + assert decompress_method(maybe_compressed_body) == body @pytest.mark.parametrize( @@ -142,6 +160,13 @@ def _assert_compression(is_compressed, body, encoding): True, 'gzip', ), + ( + COMPRESSION_CONFIG_128_BYTES, + request_dict(), + _op_with_multiple_compressions(), + True, + 'gzip', + ), ( DEFAULT_COMPRESSION_CONFIG, request_dict(), @@ -221,8 +246,11 @@ def test_compress( is_compressed, encoding, ): + original_body = request_dict['body'] RequestCompressor.compress(config, request_dict, operation_model) - _assert_compression(is_compressed, request_dict['body'], encoding) + _assert_compression( + is_compressed, original_body, request_dict['body'], encoding + ) assert ( 'headers' in request_dict and 'Content-Encoding' in request_dict['headers']