pr feedback

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}.'
                     )
                 )
 

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}'

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.
     """

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',

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']