Repository: danjac/pyramid_storage

Issue:
Switch to boto3 for Amazon S3 backend
https://github.com/boto/boto3

See https://github.com/danjac/pyramid_storage/blob/a3fe9f6c00b1032e198b53e69eef0a28f06ec5c6/pyramid_storage/s3.py#L62-L65


Discussion:
leplatrem: r? @alexcottner

Patch:
diff --git a/docs/index.rst b/docs/index.rst
index 7ae5c8a..4cc981f 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -84,7 +84,7 @@ Setting                Default                Description
 **aws.access_key**     **required**           AWS access key
 **aws.secret_key**     **required**           AWS secret key
 **aws.bucket_name**    **required**           AWS bucket
-**aws.acl**            ``public-read``        `AWS ACL permissions <https://github.com/boto/boto/blob/v2.13.2/boto/s3/acl.py#L25-L28>`_
+**aws.acl**            ``public-read``        `AWS ACL permissions <https://boto3.amazonaws.com/v1/documentation/api/latest/guide/migrations3.html#access-controls>`_
 **base_url**                                  Relative or absolute base URL for uploads; must end in slash ("/")
 **extensions**         ``default``            List of extensions or extension groups (see below)
 **name**               ``storage``            Name of property added to request, e.g. **request.storage**
@@ -230,7 +230,7 @@ Usage: s3 file storage
 ----------------------
 
 .. warning::
-    S3 support requires you install the `Boto`_ library separately (e.g. ``pip install boto``).
+    S3 support requires you install the `Boto`_ library separately (e.g. ``pip install boto3``).
 
     Alternatively you can install **pyramid_storage** with the mandatory extra dependencies: ``pip install pyramid_storage[s3]``
 
@@ -361,7 +361,7 @@ API
 .. autoclass:: DummyFileStorage
    :members:
 
-.. _Boto: http://pypi.python.org/pypi/boto/
+.. _Boto3: http://pypi.python.org/pypi/boto3/
 .. _Pyramid: http://pypi.python.org/pypi/pyramid/
 .. _Github: https://github.com/danjac/pyramid_storage
 .. _google-cloud-storage: https://github.com/googleapis/google-cloud-python
diff --git a/pyproject.toml b/pyproject.toml
index e618c69..d26713f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -11,8 +11,6 @@ classifiers = [
     "Topic :: Internet :: WWW/HTTP :: WSGI :: Application",
     "License :: OSI Approved :: BSD License",
     "Intended Audience :: Developers",
-    "Programming Language :: Python :: Implementation :: CPython",
-    "Programming Language :: Python :: Implementation :: Pypi",
     "Framework :: Pyramid",
     "Operating System :: OS Independent",
 ]
@@ -45,7 +43,7 @@ docs = [
     "repoze.sphinx.autointerface",
 ]
 s3 = [
-    "boto",
+    "boto3",
 ]
 gcloud = [
     "google-cloud-storage",
diff --git a/pyramid_storage/gcloud.py b/pyramid_storage/gcloud.py
index 09652c7..365c34f 100644
--- a/pyramid_storage/gcloud.py
+++ b/pyramid_storage/gcloud.py
@@ -242,7 +242,7 @@ class GoogleCloudStorage(object):
         extensions=None,
         acl=None,
         replace=False,
-        headers=None,
+        headers={},
     ):
         """
         :param filename: local filename
@@ -251,6 +251,7 @@ class GoogleCloudStorage(object):
         :param randomize: randomize the filename
         :param extensions: iterable of allowed extensions, if not default
         :param acl: ACL policy (if None then uses default)
+        :param headers: dict request headers (used to specify for content-type)
         :returns: modified filename
         """
         extensions = extensions or self.extensions
@@ -266,7 +267,9 @@ class GoogleCloudStorage(object):
         if folder:
             filename = folder + "/" + filename
 
-        content_type, _ = mimetypes.guess_type(filename)
+        content_type = headers.get("Content-Type")
+        if content_type is None:
+            content_type, _ = mimetypes.guess_type(filename)
         content_type = content_type or "application/octet-stream"
 
         blob = self.get_bucket(bucket_name).get_blob(filename)
@@ -284,7 +287,7 @@ class GoogleCloudStorage(object):
 
         kwargs = {
             "rewind": True,
-            "content_type": content_type or "application/octet-stream",
+            "content_type": content_type,
         }
 
         if not self.uniform_bucket_level_access:
diff --git a/pyramid_storage/s3.py b/pyramid_storage/s3.py
index 8fcde70..a35841f 100644
--- a/pyramid_storage/s3.py
+++ b/pyramid_storage/s3.py
@@ -53,46 +53,44 @@ class S3FileStorage(object):
         self.extensions = resolve_extensions(extensions)
         self.conn_options = conn_options
 
-    def get_connection(self):
+    @property
+    def s3_client(self):
         try:
-            import boto
+            import boto3
         except ImportError:
-            raise RuntimeError("You must have boto installed to use s3")
-
-        from boto.s3 import connect_to_region
-        from boto.s3.connection import OrdinaryCallingFormat
-
-        options = self.conn_options.copy()
-        options["is_secure"] = asbool(options["is_secure"])
-
-        if options["port"]:
-            options["port"] = int(options["port"])
-        else:
-            del options["port"]
-
-        if not options["host"]:
-            del options["host"]
-
-        if asbool(options.pop("use_path_style")):
-            options["calling_format"] = OrdinaryCallingFormat()
-
-        num_retries = int(options.pop("num_retries"))
-        timeout = float(options.pop("timeout"))
-
-        region = options.pop("region")
-        if region:
-            del options["host"]
-            del options["port"]
-            conn = connect_to_region(region, **options)
+            raise RuntimeError("You must have boto3 installed to use s3")
+        from botocore.config import Config
+        from botocore.exceptions import NoCredentialsError
+
+        timeout = float(self.conn_options.pop("timeout"))
+        conn_config = {"connect_timeout": timeout}
+
+        num_retries = int(self.conn_options.pop("num_retries"))
+        if num_retries > 1:
+            conn_config["retries"] = {"max_attempts": num_retries, "mode": "standard"}
+        if asbool(self.conn_options.get("use_path_style")):
+            conn_config["s3"] = {"addressing_style": "path"}
+
+        client_kwargs = {
+            "aws_access_key_id": self.conn_options.get("aws_access_key_id"),
+            "aws_secret_access_key": self.conn_options.get("aws_secret_access_key"),
+        }
+        if self.conn_options["region"] is not None:
+            client_kwargs["region_name"] = self.conn_options.get("region")
         else:
-            conn = boto.connect_s3(**options)
+            protocol = "http" if self.conn_options.get("is_secure") else "https"
+            host = self.conn_options["host"]
+            port = self.conn_options["port"]
+            client_kwargs["endpoint"] = f"{protocol}://{host}:{port}"
 
-        conn.num_retries = num_retries
-        conn.http_connection_kwargs["timeout"] = timeout
-        return conn
-
-    def get_bucket(self, bucket_name=None):
-        return self.get_connection().get_bucket(bucket_name or self.bucket_name)
+        try:
+            return boto3.client(
+                "s3",
+                config=Config(**conn_config),
+                **client_kwargs,
+            )
+        except NoCredentialsError:
+            raise RuntimeError("AWS credentials are missing or incorrect")
 
     def url(self, filename):
         """Returns entire URL of the filename, joined to the base_url
@@ -102,7 +100,11 @@ class S3FileStorage(object):
         return urllib.parse.urljoin(self.base_url, filename)
 
     def exists(self, filename, bucket_name=None):
-        return self.get_bucket(bucket_name).new_key(filename).exists()
+        try:
+            self.s3_client.head_object(Bucket=bucket_name or self.bucket_name, Key=filename)
+            return True
+        except self.s3_client.exceptions.ClientError:
+            return False
 
     def delete(self, filename, bucket_name=None):
         """Deletes the filename. Filename is resolved with the
@@ -112,7 +114,7 @@ class S3FileStorage(object):
         :param filename: base name of file
         :param bucket_name: name of the bucket, if not default
         """
-        self.get_bucket(bucket_name).delete_key(filename)
+        self.s3_client.delete_object(Bucket=bucket_name or self.bucket_name, Key=filename)
 
     def filename_allowed(self, filename, extensions=None):
         """Checks if a filename has an allowed extension
@@ -222,15 +224,15 @@ class S3FileStorage(object):
         content_type = headers.get("Content-Type")
         if content_type is None:
             content_type, _ = mimetypes.guess_type(filename)
-            content_type = content_type or "application/octet-stream"
-            headers["Content-Type"] = content_type
-
-        bucket = self.get_bucket(bucket_name)
-
-        key = bucket.get_key(filename) or bucket.new_key(filename)
+        content_type = content_type or "application/octet-stream"
 
         file.seek(0)
 
-        key.set_contents_from_file(file, headers=headers, policy=acl, replace=replace, rewind=True)
-
+        self.s3_client.put_object(
+            Bucket=bucket_name or self.bucket_name,
+            Key=filename,
+            Body=file,
+            ACL=acl,
+            ContentType=content_type,
+        )
         return filename
diff --git a/requirements.txt b/requirements.txt
index c5f80eb..e8a0e8c 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -2,7 +2,7 @@
 # This file is autogenerated by pip-compile with Python 3.9
 # by the following command:
 #
-#    pip-compile --generate-hashes requirements.in
+#    pip-compile --allow-unsafe --generate-hashes requirements.in
 #
 hupper==1.12 \
     --hash=sha256:18b1653d9832c9f8e7d3401986c7e7af2ae6783616be0bc406bfe0b14134a5c6 \
@@ -81,7 +81,11 @@ zope-interface==6.1 \
     --hash=sha256:fddbab55a2473f1d3b8833ec6b7ac31e8211b0aa608df5ab09ce07f3727326de
     # via pyramid
 
-# WARNING: The following packages were not pinned, but pip requires them to be
-# pinned when the requirements file includes hashes and the requirement is not
-# satisfied by a package already installed. Consider using the --allow-unsafe flag.
-# setuptools
+# The following packages are considered to be unsafe in a requirements file:
+setuptools==75.1.0 \
+    --hash=sha256:35ab7fd3bcd95e6b7fd704e4a1539513edad446c097797f2985e0e4b960772f2 \
+    --hash=sha256:d59a21b17a275fb872a9c3dae73963160ae079f1049ed956880cd7c09b120538
+    # via
+    #   pyramid
+    #   zope-deprecation
+    #   zope-interface


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 76b67d8..49774e0 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -20,7 +20,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        python-version: ["3.8", "3.9", "3.10", "3.11"]
+        python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
     steps:
       - uses: actions/checkout@v3
         with:
diff --git a/tests/test_gcloud.py b/tests/test_gcloud.py
index 5391e29..cfc058d 100644
--- a/tests/test_gcloud.py
+++ b/tests/test_gcloud.py
@@ -216,6 +216,26 @@ def test_save_if_randomize():
     assert name != "test.jpg"
 
 
+def test_save_with_content_type():
+    from pyramid_storage import gcloud
+
+    fs = mock.Mock()
+    fs.filename = "test.jpg"
+
+    g = gcloud.GoogleCloudStorage(
+        credentials="/secrets/credentials.json", bucket_name="my_bucket", extensions="images"
+    )
+
+    with mock.patch(
+        "pyramid_storage.gcloud.GoogleCloudStorage.get_connection", _get_mock_gcloud_connection
+    ):
+        with mock.patch("pyramid_storage.gcloud.Blob") as mocked_new_blob:
+            g.save(fs, headers={"Content-Type": "image/png"})
+            mocked_new_blob.return_value.upload_from_file.assert_called_with(
+                mock.ANY, rewind=True, content_type="image/png", predefined_acl="publicRead"
+            )
+
+
 def test_save_in_folder():
     from pyramid_storage import gcloud
 
diff --git a/tests/test_s3.py b/tests/test_s3.py
index 3520c43..80405a3 100644
--- a/tests/test_s3.py
+++ b/tests/test_s3.py
@@ -6,21 +6,10 @@ import pytest
 from pyramid import exceptions as pyramid_exceptions
 
 
-class MockS3Connection(object):
-    def get_bucket(self, bucket_name):
-        return mock.Mock()
-
-
-def _get_mock_s3_connection(self):
-    return MockS3Connection()
-
-
-def _mock_open(name="test", mode="wb"):
-    obj = mock.Mock()
-    obj.__enter__ = mock.Mock()
-    obj.__enter__.return_value = mock.Mock()
-    obj.__exit__ = mock.Mock()
-    return obj
+@pytest.fixture
+def mock_s3_client():
+    with mock.patch("pyramid_storage.s3.S3FileStorage.s3_client") as mocked:
+        yield mocked
 
 
 def test_extension_allowed_if_any():
@@ -97,7 +86,7 @@ def test_save_if_file_not_allowed():
         s.save(fs)
 
 
-def test_save_if_file_allowed():
+def test_save_if_file_allowed(mock_s3_client):
     from pyramid_storage import s3
 
     fs = mock.Mock()
@@ -107,46 +96,36 @@ def test_save_if_file_allowed():
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection", _get_mock_s3_connection):
-        name = s.save(fs)
+    name = s.save(fs)
     assert name == "test.jpg"
 
 
-def test_save_file():
+def test_save_file(mock_s3_client):
     from pyramid_storage import s3
 
     s = s3.S3FileStorage(
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection", _get_mock_s3_connection):
-        name = s.save_file(mock.Mock(), "test.jpg")
+    name = s.save_file(mock.Mock(), "test.jpg")
     assert name == "test.jpg"
 
 
-def test_save_filename():
+def test_save_filename(mock_s3_client, tmp_path):
     from pyramid_storage import s3
 
     s = s3.S3FileStorage(
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    patches = (
-        mock.patch("builtins.open", _mock_open),
-        mock.patch("pyramid_storage.s3.S3FileStorage.get_connection", _get_mock_s3_connection),
-    )
-
-    for patch in patches:
-        patch.start()
+    p = tmp_path / "hello.jpg"
+    p.write_text("test")
 
-    name = s.save_filename("test.jpg")
-    assert name == "test.jpg"
-
-    for patch in patches:
-        patch.stop()
+    name = s.save_filename(str(p))
+    assert name == "hello.jpg"
 
 
-def test_save_if_randomize():
+def test_save_if_randomize(mock_s3_client):
     from pyramid_storage import s3
 
     fs = mock.Mock()
@@ -156,12 +135,12 @@ def test_save_if_randomize():
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection", _get_mock_s3_connection):
-        name = s.save(fs, randomize=True)
+    name = s.save(fs, randomize=True)
+
     assert name != "test.jpg"
 
 
-def test_save_in_folder():
+def test_save_in_folder(mock_s3_client):
     from pyramid_storage import s3
 
     fs = mock.Mock()
@@ -171,12 +150,12 @@ def test_save_in_folder():
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection", _get_mock_s3_connection):
-        name = s.save(fs, folder="my_folder")
+    name = s.save(fs, folder="my_folder")
+
     assert name == "my_folder/test.jpg"
 
 
-def test_save_with_content_type():
+def test_save_with_content_type(mock_s3_client):
     from pyramid_storage import s3
 
     fs = mock.Mock()
@@ -186,21 +165,23 @@ def test_save_with_content_type():
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="documents"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection") as mocked:
-        s.save(fs, headers={"Content-Type": "text/html"})
-    call = mocked.return_value.get_bucket.return_value.get_key.return_value.set_contents_from_file.call_args_list
-    assert call[0][1]["headers"]["Content-Type"] == "text/html"
+    s.save(fs, headers={"Content-Type": "text/html"})
+
+    assert mock_s3_client.put_object(
+        Bucket="my_bucket", Key="test.doc", Body=mock.ANY, ACL=None, ContentType="text/html"
+    )
 
 
-def test_delete():
+def test_delete(mock_s3_client):
     from pyramid_storage import s3
 
     s = s3.S3FileStorage(
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection", _get_mock_s3_connection):
-        s.delete("test.jpg")
+    s.delete("test.jpg")
+
+    assert mock_s3_client.delete_object(Bucket="my_bucket", Key="test.jpg")
 
 
 def test_from_settings_with_defaults():
@@ -210,6 +191,7 @@ def test_from_settings_with_defaults():
         "storage.aws.access_key": "abc",
         "storage.aws.secret_key": "123",
         "storage.aws.bucket_name": "Attachments",
+        "storage.aws.region": "us2",
     }
     inst = s3.S3FileStorage.from_settings(settings, "storage.")
     assert inst.base_url == ""
@@ -219,12 +201,19 @@ def test_from_settings_with_defaults():
     assert inst.conn_options["aws_secret_access_key"] == "123"
     assert set(("jpg", "txt", "doc")).intersection(inst.extensions)
 
-    with mock.patch("boto.connect_s3") as boto_mocked:
-        boto_mocked.return_value.http_connection_kwargs = {}
-        inst.get_connection()
-        _, boto_options = boto_mocked.call_args_list[0]
-        assert "host" not in boto_options
-        assert "port" not in boto_options
+    with mock.patch("boto3.client") as boto_mocked:
+        inst.s3_client
+
+        boto_mocked.assert_called_with(
+            "s3",
+            config=mock.ANY,
+            aws_access_key_id="abc",
+            aws_secret_access_key="123",
+            region_name="us2",
+        )
+        call = boto_mocked.call_args_list[0]
+        _, kwargs = call
+        assert kwargs["config"].connect_timeout == 5
 
 
 def test_from_settings_if_base_path_missing():
@@ -249,24 +238,22 @@ def test_from_settings_with_additional_options():
         "storage.aws.timeout": "10",
     }
     inst = s3.S3FileStorage.from_settings(settings, "storage.")
-    with mock.patch("boto.connect_s3") as boto_mocked:
-        boto_mocked.return_value.http_connection_kwargs = {}
-        conn = inst.get_connection()
-        assert conn.num_retries == 3
-        assert conn.http_connection_kwargs["timeout"] == 10
-
-        _, boto_options = boto_mocked.call_args_list[0]
 
-        calling_format = boto_options.pop("calling_format")
-        assert calling_format.__class__.__name__ == "OrdinaryCallingFormat"
+    with mock.patch("boto3.client") as boto_mocked:
+        inst.s3_client
 
-        assert boto_options == {
-            "is_secure": False,
-            "host": "localhost",
-            "port": 5000,
-            "aws_access_key_id": "abc",
-            "aws_secret_access_key": "123",
-        }
+        boto_mocked.assert_called_with(
+            "s3",
+            config=mock.ANY,
+            aws_access_key_id="abc",
+            aws_secret_access_key="123",
+            endpoint="http://localhost:5000",
+        )
+        call = boto_mocked.call_args_list[0]
+        _, kwargs = call
+        assert kwargs["config"].connect_timeout == 10.0
+        assert kwargs["config"].retries == {"max_attempts": 3, "mode": "standard"}
+        assert kwargs["config"].s3 == {"addressing_style": "path"}
 
 
 def test_from_settings_with_regional_options_ignores_host_port():
@@ -281,53 +268,49 @@ def test_from_settings_with_regional_options_ignores_host_port():
         "storage.aws.port": "5000",
     }
     inst = s3.S3FileStorage.from_settings(settings, "storage.")
-    with mock.patch("boto.s3.connect_to_region") as boto_mocked:
-        boto_mocked.return_value.http_connection_kwargs = {}
-        inst.get_connection()
-        _, boto_options = boto_mocked.call_args_list[0]
-        assert "host" not in boto_options
-        assert "port" not in boto_options
+    with mock.patch("boto3.client") as boto_mocked:
+        inst.s3_client
 
+        boto_mocked.assert_called_with(
+            "s3",
+            config=mock.ANY,
+            aws_access_key_id="abc",
+            aws_secret_access_key="123",
+            region_name="eu-west-1",
+        )
 
-def test_get_bucket():
+
+def test_save_file_to_bucket(mock_s3_client):
     from pyramid_storage import s3
 
     s = s3.S3FileStorage(
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection") as mocked:
-        _my_bucket = s.get_bucket()
-        _other_bucket = s.get_bucket("other_bucket")
-    assert mocked.return_value.get_bucket.call_args_list[0][0][0] == "my_bucket"
-    assert mocked.return_value.get_bucket.call_args_list[1][0][0] == "other_bucket"
+    s.save_file(mock.Mock(), "test.jpg")
 
+    mock_s3_client.put_object(
+        Bucket="my_bucket", Key="test.jpg", Body=mock.ANY, ACL=None, ContentType="image/jpeg"
+    )
 
-def test_save_file_to_bucket():
-    from pyramid_storage import s3
+    s.save_file(mock.Mock(), "test.jpg", bucket_name="other_bucket")
 
-    s = s3.S3FileStorage(
-        access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
+    mock_s3_client.put_object(
+        Bucket="other_bucket", Key="test.jpg", Body=mock.ANY, ACL=None, ContentType="image/jpeg"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection") as mocked:
-        s.save_file(mock.Mock(), "test.jpg")
-        s.save_file(mock.Mock(), "test.jpg", bucket_name="other_bucket")
-    assert mocked.return_value.get_bucket.call_args_list[0][0][0] == "my_bucket"
-    assert mocked.return_value.get_bucket.call_args_list[1][0][0] == "other_bucket"
-    # make sure saving to another bucket doesn't change the default
-    assert s.bucket_name == "my_bucket"
-
 
-def test_delete_from_bucket():
+def test_delete_from_bucket(mock_s3_client):
     from pyramid_storage import s3
 
     s = s3.S3FileStorage(
         access_key="AK", secret_key="SK", bucket_name="my_bucket", extensions="images"
     )
 
-    with mock.patch("pyramid_storage.s3.S3FileStorage.get_connection") as mocked:
-        s.delete("test.jpg")
-        s.delete("test.jpg", bucket_name="other_bucket")
-    assert mocked.return_value.get_bucket.call_args_list[0][0][0] == "my_bucket"
-    assert mocked.return_value.get_bucket.call_args_list[1][0][0] == "other_bucket"
+    s.delete("test.jpg")
+
+    mock_s3_client.delete_object(Bucket="my_bucket", Key="test.jpg")
+
+    s.delete("test.jpg", bucket_name="other_bucket")
+
+    mock_s3_client.delete_object(Bucket="other_bucket", Key="test.jpg")