Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S3 upload_file, download file to support path-lib objects #2259

Merged
merged 31 commits into from Dec 16, 2022
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
ada17c6
Update transfer.py
alanyee Jan 8, 2020
ef141cb
Update transfer.py
alanyee Jan 9, 2020
b25cdff
Add exhaustive ValueError message
alanyee Jan 8, 2021
fa917ec
Update test_transfer.py
alanyee Jan 8, 2021
86053f2
Fix test for download
alanyee Jan 8, 2021
6b84c21
Correct file path for Windows
alanyee Jan 9, 2021
351a7a5
Add os import for unit tests
alanyee Jan 9, 2021
049b10a
Fix for Windows
alanyee Jan 9, 2021
6cf1f6c
Fix for Windows path
alanyee Jan 9, 2021
aa4d6bf
Merge branch 'develop' into patch-1
alanyee Jan 15, 2022
ab9643b
Merge branch 'develop' into patch-1
alanyee Oct 4, 2022
eb43aaf
lint: fix style
alanyee Oct 4, 2022
be73d95
lint: fix style
alanyee Oct 4, 2022
6108d6f
lint: fix style
alanyee Oct 4, 2022
0970707
lint: fix style
alanyee Oct 4, 2022
38962ec
refactor: remove python2 compatibility
alanyee Dec 4, 2022
ff0af1b
fix: add os import when needed
alanyee Dec 4, 2022
7f67c6f
test: add more unit and integration tests
alanyee Dec 8, 2022
3d1bdc4
test: fix dupe function call
alanyee Dec 8, 2022
1d05811
lint: fix linting issues
alanyee Dec 8, 2022
39b183d
lint: add ending comma
alanyee Dec 8, 2022
fc84da5
fix: correct path import
alanyee Dec 9, 2022
bc6cae2
fix: add os check on os-dependent tests
alanyee Dec 9, 2022
6257495
test: remove system dependent tests
alanyee Dec 15, 2022
2efe07e
lint: remove unused import os
alanyee Dec 15, 2022
5a7f0a8
feat: use feature that ensures system independent solution
alanyee Dec 15, 2022
ec0fde5
Revert "feat: use feature that ensures system independent solution"
jonemo Dec 15, 2022
4ef8361
use NamedTempFile as source of realistic file name
jonemo Dec 15, 2022
b5da19b
actually use Path object in test_upload_via_path
jonemo Dec 16, 2022
8a6365e
Merge branch 'develop' into patch-1
jonemo Dec 16, 2022
b12faf9
changelog
jonemo Dec 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions boto3/s3/transfer.py
Expand Up @@ -122,6 +122,8 @@ def __call__(self, bytes_amount):


"""
from os import PathLike, fspath

from botocore.exceptions import ClientError
from s3transfer.exceptions import (
RetriesExceededError as S3TransferRetriesExceededError,
Expand Down Expand Up @@ -277,8 +279,10 @@ def upload_file(
:py:meth:`S3.Client.upload_file`
:py:meth:`S3.Client.upload_fileobj`
"""
if isinstance(filename, PathLike):
filename = fspath(filename)
if not isinstance(filename, str):
raise ValueError('Filename must be a string')
raise ValueError('Filename must be a string or a path-like object')

subscribers = self._get_subscribers(callback)
future = self._manager.upload(
Expand Down Expand Up @@ -309,8 +313,10 @@ def download_file(
:py:meth:`S3.Client.download_file`
:py:meth:`S3.Client.download_fileobj`
"""
if isinstance(filename, PathLike):
filename = fspath(filename)
if not isinstance(filename, str):
raise ValueError('Filename must be a string')
raise ValueError('Filename must be a string or a path-like object')

subscribers = self._get_subscribers(callback)
future = self._manager.download(
Expand Down
8 changes: 8 additions & 0 deletions tests/integration/test_s3.py
Expand Up @@ -387,6 +387,14 @@ def test_download_fileobj(self):

self.assertEqual(fileobj.getvalue(), b'beach')

def test_upload_via_path(self):
transfer = self.create_s3_transfer()
filename = self.files.create_file_with_size('foo.txt', filesize=1024)
transfer.upload_file(filename, self.bucket_name, 'foo.txt')
self.addCleanup(self.delete_object, 'foo.txt')

self.assertTrue(self.object_exists('foo.txt'))

def test_upload_below_threshold(self):
config = boto3.s3.transfer.TransferConfig(
multipart_threshold=2 * 1024 * 1024
Expand Down
99 changes: 99 additions & 0 deletions tests/unit/s3/test_transfer.py
Expand Up @@ -10,6 +10,9 @@
# 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 os
import pathlib

import pytest
from s3transfer.futures import NonThreadedExecutor
from s3transfer.manager import TransferManager
Expand Down Expand Up @@ -148,6 +151,83 @@ def test_upload_file(self):
'smallfile', 'bucket', 'key', extra_args, None
)

def test_upload_file_via_path(self):
extra_args = {'ACL': 'public-read'}
self.transfer.upload_file(
pathlib.Path('smallfile'), 'bucket', 'key', extra_args=extra_args
)
self.manager.upload.assert_called_with(
'smallfile', 'bucket', 'key', extra_args, None
)

def test_upload_file_via_purepath(self):
extra_args = {'ACL': 'public-read'}
self.transfer.upload_file(
pathlib.PurePath('smallfile'),
'bucket',
'key',
extra_args=extra_args,
)
self.manager.upload.assert_called_with(
'smallfile', 'bucket', 'key', extra_args, None
)

def test_upload_file_via_pureposixpath(self):
if os.name != 'posix':
pytest.skip("Unsupported system for method tested")
extra_args = {'ACL': 'public-read'}
self.transfer.upload_file(
pathlib.PurePosixPath('smallfile'),
'bucket',
'key',
extra_args=extra_args,
)
self.manager.upload.assert_called_with(
'smallfile', 'bucket', 'key', extra_args, None
)

def test_upload_file_via_posixpath(self):
if os.name != 'posix':
pytest.skip("Unsupported system for method tested")
extra_args = {'ACL': 'public-read'}
self.transfer.upload_file(
pathlib.PosixPath('smallfile'),
'bucket',
'key',
extra_args=extra_args,
)
self.manager.upload.assert_called_with(
'smallfile', 'bucket', 'key', extra_args, None
)

def test_upload_file_via_purewindowpath(self):
if os.name != 'nt':
pytest.skip("Unsupported system for method tested")
extra_args = {'ACL': 'public-read'}
self.transfer.upload_file(
pathlib.PureWindowsPath('smallfile'),
'bucket',
'key',
extra_args=extra_args,
)
self.manager.upload.assert_called_with(
'smallfile', 'bucket', 'key', extra_args, None
)

def test_upload_file_via_windowpath(self):
if os.name != 'nt':
pytest.skip("Unsupported system for method tested")
extra_args = {'ACL': 'public-read'}
self.transfer.upload_file(
pathlib.WindowsPath('smallfile'),
'bucket',
'key',
extra_args=extra_args,
)
self.manager.upload.assert_called_with(
'smallfile', 'bucket', 'key', extra_args, None
)

def test_download_file(self):
extra_args = {
'SSECustomerKey': 'foo',
Expand All @@ -160,6 +240,25 @@ def test_download_file(self):
'bucket', 'key', '/tmp/smallfile', extra_args, None
)

def test_download_file_via_path(self):
extra_args = {
'SSECustomerKey': 'foo',
'SSECustomerAlgorithm': 'AES256',
}
self.transfer.download_file(
'bucket',
'key',
pathlib.Path('/tmp/smallfile'),
extra_args=extra_args,
)
self.manager.download.assert_called_with(
'bucket',
'key',
os.path.normpath('/tmp/smallfile'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is os.path.normpath needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it now, but the keys can be messed up on Windows:

Expected: download('bucket', 'key', '/tmp/smallfile', {'SSECustomerKey': 'foo', 'SSECustomerAlgorithm': 'AES256'}, None)
E           Actual: download('bucket', 'key', '\\tmp\\smallfile'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use an absolute path, make sure the first character is not a slash ('/') Use pathlib or the os.path.normpath() function to ensure that paths are properly encoded for Win32.

I will add normpath to ensure it is properly encoded independent of the system.

extra_args,
None,
)

def test_upload_wraps_callback(self):
self.transfer.upload_file(
'smallfile', 'bucket', 'key', callback=self.callback
Expand Down