Repository: RaRe-Technologies/smart_open
Pull request #767 (2023-04-06 05:35:33)

Avoid overfilling buffer when reading from Azure
#### Title

when read size > chunk size, return read size and not chunk size.

```
bugfix: read(MAX_READ_SIZE) returns read(CHUNK_SIZE) when read size > chunk size
```

#### Motivation

The read function expects to return the maximum size it can read from a stream and not the chunk size. This causes serious bugs when reading with a parameter such as x.read(MAX_READ_SIZE), which does not expect you to re-read the buffer until you get to MAX_READ_SIZE.

This bug does not happen when the read size is not specified.

If you're fixing a bug, link to the issue number like so:

```
- Fixes #{issue_number}
```

If you're adding a new feature, then consider opening a ticket and discussing it with the maintainers before you actually do the hard work.

#### Tests

If you're fixing a bug, consider [test-driven development](https://en.wikipedia.org/wiki/Test-driven_development):

1. Create a unit test that demonstrates the bug. The test should **fail**.
2. Implement your bug fix.
3. The test you created should now **pass**.

If you're implementing a new feature, include unit tests for it.

Make sure all existing unit tests pass.
You can run them locally using:

    pytest smart_open

If there are any failures, please fix them before creating the PR (or mark it as WIP, see below).

#### Work in progress

If you're still working on your PR, include "WIP" in the title.
We'll skip reviewing it for the time being.
Once you're ready to review, remove the "WIP" from the title, and ping one of the maintainers (e.g. mpenkov).

#### Checklist

Before you create the PR, please make sure you have:

- [ ] Picked a concise, informative and complete title
- [ ] Clearly explained the motivation behind the PR
- [ ] Linked to any existing issues that your PR will be solving
- [ ] Included tests for any new functionality
- [ ] Checked that all unit tests pass

#### Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.



Patch:
diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml
index c2cb5b3..767a691 100644
--- a/.github/workflows/python-package.yml
+++ b/.github/workflows/python-package.yml
@@ -6,10 +6,10 @@ jobs:
     steps:
       - uses: actions/checkout@v2
 
-      - name: Setup up Python 3.10
+      - name: Setup up Python 3.11
         uses: actions/setup-python@v2
         with:
-          python-version: "3.10"
+          python-version: "3.11"
 
       - name: Update pip
         run: python -m pip install -U pip
@@ -26,15 +26,15 @@ jobs:
     strategy:
       matrix:
         include:
-          - {python: '3.7', os: ubuntu-20.04}
           - {python: '3.8', os: ubuntu-20.04}
           - {python: '3.9', os: ubuntu-20.04}
           - {python: '3.10', os: ubuntu-20.04}
+          - {python: '3.11', os: ubuntu-20.04}
 
-          - {python: '3.7', os: windows-2019}
           - {python: '3.8', os: windows-2019}
           - {python: '3.9', os: windows-2019}
           - {python: '3.10', os: windows-2019}
+          - {python: '3.11', os: windows-2019}
     steps:
       - uses: actions/checkout@v2
 
@@ -63,10 +63,10 @@ jobs:
     strategy:
       matrix:
         include:
-          - {python: '3.7', os: ubuntu-20.04}
           - {python: '3.8', os: ubuntu-20.04}
           - {python: '3.9', os: ubuntu-20.04}
           - {python: '3.10', os: ubuntu-20.04}
+          - {python: '3.11', os: ubuntu-20.04}
 
           #
           # Some of the doctests don't pass on Windows because of Windows-specific
@@ -105,10 +105,10 @@ jobs:
     strategy:
       matrix:
         include:
-          - {python: '3.7', os: ubuntu-20.04, moto_server: true}
           - {python: '3.8', os: ubuntu-20.04}
           - {python: '3.9', os: ubuntu-20.04}
           - {python: '3.10', os: ubuntu-20.04}
+          - {python: '3.11', os: ubuntu-20.04}
 
           # Not sure why we exclude these, perhaps for historical reasons?
           #
@@ -159,10 +159,10 @@ jobs:
     strategy:
       matrix:
         include:
-          - {python: '3.7', os: ubuntu-20.04}
           - {python: '3.8', os: ubuntu-20.04}
           - {python: '3.9', os: ubuntu-20.04}
           - {python: '3.10', os: ubuntu-20.04}
+          - {python: '3.11', os: ubuntu-20.04}
 
           # - {python: '3.7', os: windows-2019}
           # - {python: '3.8', os: windows-2019}
diff --git a/setup.py b/setup.py
index 57ad9e6..8c08b8e 100644
--- a/setup.py
+++ b/setup.py
@@ -95,6 +95,7 @@ setup(
         'Programming Language :: Python :: 3.8',
         'Programming Language :: Python :: 3.9',
         'Programming Language :: Python :: 3.10',
+        'Programming Language :: Python :: 3.11',
         'Topic :: System :: Distributed Computing',
         'Topic :: Database :: Front-Ends',
     ],
diff --git a/smart_open/azure.py b/smart_open/azure.py
index 96f944a..ccc1905 100644
--- a/smart_open/azure.py
+++ b/smart_open/azure.py
@@ -306,7 +306,7 @@ class Reader(io.BufferedIOBase):
         if self._position == self._size:
             return self._read_from_buffer()
 
-        self._fill_buffer()
+        self._fill_buffer(size)
         return self._read_from_buffer(size)
 
     def read1(self, size=-1):
diff --git a/smart_open/ftp.py b/smart_open/ftp.py
index 3dbe26f..7d4a5ad 100644
--- a/smart_open/ftp.py
+++ b/smart_open/ftp.py
@@ -14,6 +14,7 @@ import urllib.parse
 import smart_open.utils
 from ftplib import FTP, FTP_TLS, error_reply
 import types
+
 logger = logging.getLogger(__name__)
 
 SCHEMES = ("ftp", "ftps")
@@ -55,8 +56,13 @@ def open_uri(uri, mode, transport_params):
     uri_path = parsed_uri.pop("uri_path")
     scheme = parsed_uri.pop("scheme")
     secure_conn = True if scheme == "ftps" else False
-    return open(uri_path, mode, secure_connection=secure_conn,
-            transport_params=transport_params, **parsed_uri)
+    return open(
+        uri_path,
+        mode,
+        secure_connection=secure_conn,
+        transport_params=transport_params,
+        **parsed_uri,
+    )
 
 
 def convert_transport_params_to_args(transport_params):
@@ -90,7 +96,9 @@ def _connect(hostname, username, port, password, secure_connection, transport_pa
     try:
         ftp.login(username, password)
     except error_reply as e:
-        logger.error("Unable to login to FTP server: try checking the username and password!")
+        logger.error(
+            "Unable to login to FTP server: try checking the username and password!"
+        )
         raise e
     if secure_connection:
         ftp.prot_p()
@@ -99,7 +107,7 @@ def _connect(hostname, username, port, password, secure_connection, transport_pa
 
 def open(
     path,
-    mode="r",
+    mode="rb",
     host=None,
     user=None,
     password=None,
@@ -146,6 +154,7 @@ def open(
     except KeyError:
         raise ValueError(f"unsupported mode: {mode!r}")
     ftp_mode, file_obj_mode = mode_to_ftp_cmds[mode]
+    conn.voidcmd("TYPE I")
     socket = conn.transfercmd(f"{ftp_mode} {path}")
     fobj = socket.makefile(file_obj_mode)
 
@@ -153,6 +162,7 @@ def open(
         self.orig_close()
         self.socket.close()
         self.conn.close()
+
     fobj.orig_close = fobj.close
     fobj.socket = socket
     fobj.conn = conn
diff --git a/smart_open/s3.py b/smart_open/s3.py
index b211163..7e3fb63 100644
--- a/smart_open/s3.py
+++ b/smart_open/s3.py
@@ -324,10 +324,13 @@ def open(
 
 def _get(client, bucket, key, version, range_string):
     try:
+        params = dict(Bucket=bucket, Key=key)
         if version:
-            return client.get_object(Bucket=bucket, Key=key, VersionId=version, Range=range_string)
-        else:
-            return client.get_object(Bucket=bucket, Key=key, Range=range_string)
+            params["VersionId"] = version
+        if range_string:
+            params["Range"] = range_string
+
+        return client.get_object(**params)
     except botocore.client.ClientError as error:
         wrapped_error = IOError(
             'unable to access bucket: %r key: %r version: %r error: %s' % (
@@ -447,8 +450,19 @@ class _SeekableRawReader(object):
             error_response = _unwrap_ioerror(ioe)
             if error_response is None or error_response.get('Code') != _OUT_OF_RANGE:
                 raise
-            self._position = self._content_length = int(error_response['ActualObjectSize'])
-            self._body = io.BytesIO()
+            try:
+                self._position = self._content_length = int(error_response['ActualObjectSize'])
+                self._body = io.BytesIO()
+            except KeyError:
+                response = _get(
+                    self._client,
+                    self._bucket,
+                    self._key,
+                    self._version_id,
+                    None,
+                )
+                self._position = self._content_length = response["ContentLength"]
+                self._body = response["Body"]
         else:
             #
             # Keep track of how many times boto3's built-in retry mechanism
@@ -461,7 +475,7 @@ class _SeekableRawReader(object):
                 self,
                 response['ResponseMetadata']['RetryAttempts'],
             )
-            units, start, stop, length = smart_open.utils.parse_content_range(response['ContentRange'])
+            _, start, stop, length = smart_open.utils.parse_content_range(response['ContentRange'])
             self._content_length = length
             self._position = start
             self._body = response['Body']


Test patch:
diff --git a/smart_open/tests/test_s3.py b/smart_open/tests/test_s3.py
index a91a731..fa90710 100644
--- a/smart_open/tests/test_s3.py
+++ b/smart_open/tests/test_s3.py
@@ -73,6 +73,8 @@ def patch_invalid_range_response(actual_size):
                 error_response['ActualObjectSize'] = actual_size
                 error_response['Code'] = 'InvalidRange'
                 error_response['Message'] = 'The requested range is not satisfiable'
+            if actual_size is None:
+                error_response.pop('ActualObjectSize', None)
             raise
 
     with mock.patch('smart_open.s3._get', new=mock_get):
@@ -399,6 +401,15 @@ class ReaderTest(BaseTest):
 
         self.assertEqual(data, b'')
 
+    def test_read_empty_file_no_actual_size(self):
+        _resource('s3').Object(BUCKET_NAME, KEY_NAME).put(Body=b'')
+
+        with self.assertApiCalls(GetObject=2), patch_invalid_range_response(None):
+            with smart_open.s3.Reader(BUCKET_NAME, KEY_NAME) as fin:
+                data = fin.read()
+
+        self.assertEqual(data, b'')
+
 
 @moto.mock_s3
 class MultipartWriterTest(unittest.TestCase):