[SDBELGA-1051] improve(s3): Use ObjectId and store filename in metadata (#3169)

* [SDBELGA-1051] improve(s3): Use ObjectId and store filename in metadata

* fix s3 tests

* remove commented test code

* Update pydocs, convert filename to ASCII, fix test mock args order

* fix unicode filename test

Author: MarkLark86

superdesk/storage/amazon_media_storage.py

@@ -18,11 +18,10 @@
 import time
 import unidecode
 
-from os.path import splitext
-from urllib.parse import urlparse
 from botocore.client import Config
+from bson import ObjectId
 
-from superdesk.media.media_operations import download_file_from_url, guess_media_extension
+from superdesk.media.media_operations import download_file_from_url
 from .superdesk_file import SuperdeskFile
 from superdesk.utc import query_datetime
 from . import SuperdeskMediaStorage
@@ -35,7 +34,7 @@
 
 
 class AmazonObjectWrapper(SuperdeskFile):
-    def __init__(self, s3_object, name, metadata):
+    def __init__(self, s3_object, key, metadata):
         super().__init__()
 
         s3_body = s3_object["Body"]
@@ -48,12 +47,11 @@ def __init__(self, s3_object, name, metadata):
         self.seek(0)
         self.content_type = s3_object["ContentType"]
         self.length = int(s3_object["ContentLength"])
-        self._name = name
-        self.filename = name
-        self.metadata = metadata
+        self.metadata = metadata or {}
+        self._name = self.filename = self.metadata.get("filename") or key
         self.upload_date = s3_object["LastModified"]
         self.md5 = s3_object["ETag"][1:-1]
-        self._id = name
+        self._id = key
 
 
 class AmazonMediaStorage(SuperdeskMediaStorage):
@@ -121,19 +119,21 @@ def get_translation_table():
 
         return unidecode.unidecode(str(_id)).translate(get_translation_table())
 
-    def media_id(self, filename, content_type=None, version=True):
-        """Get the ``media_id`` path for the given ``filename``.
-
-        if filename doesn't have an extension one is guessed,
-        and additional *version* option to have automatic version or not to have,
-        or to send a `string` one.
+    def media_id(self, version: bool | str = True) -> str:
+        """
+        Generates a media ID string using a configurable time-based prefix and a unique identifier.
+
+        This method generates a media ID based on a configurable time granularity or provided
+        custom version. The time granularity settings can be 'hourly', 'daily', or 'none'. A unique
+        identifier is appended to the generated string, ensuring a unique media ID is produced
+        each time the method is called.
+
+        :param version: Determines the versioning format. If True, an automatic time-based
+                        version is generated based on the 'AMAZON_MEDIA_ID_TIME_PREFIX' configuration
+                        from the application. If False, no version prefix is included. If a string is
+                        provided, it is used as a custom prefix after stripping any surrounding slashes.
+        :returns: A unique media ID string composed of the specified versioning format and a unique identifier.
         """
-        path = urlparse(filename).path.split("/")[-1]
-        file_extension = splitext(path)[1]
-
-        extension = ""
-        if not file_extension:
-            extension = str(guess_media_extension(content_type)) if content_type else ""
 
         if version is True:
             folder_granularity = str(self.app.config.get("AMAZON_MEDIA_ID_TIME_PREFIX", "hourly")).lower()
@@ -150,7 +150,7 @@ def media_id(self, filename, content_type=None, version=True):
         else:
             version = "%s/" % version.strip("/")
 
-        return "%s%s%s" % (version, self._make_s3_safe(filename), extension)
+        return f"{version}{ObjectId()}"
 
     def fetch_rendition(self, rendition):
         stream, name, mime = download_file_from_url(rendition.get("href"))
@@ -211,7 +211,9 @@ def _get_all_keys_in_batches(self):
     def extract_metadata_from_headers(self, request_headers):
         headers = {}
         for key, value in request_headers.items():
-            if self.user_metadata_header in key:
+            if key == "filename":
+                headers["filename"] = value
+            elif self.user_metadata_header in key:
                 new_key = key.split(self.user_metadata_header)[1]
                 if value:
                     try:
@@ -258,7 +260,7 @@ def put(
         content_type = self._get_mimetype(content, filename, content_type)
 
         if not _id:
-            _id = self.media_id(filename, content_type=content_type, version=version)
+            _id = self.media_id(version)
 
         if folder:
             _id = "%s/%s" % (folder.rstrip("/"), _id)
@@ -273,6 +275,10 @@ def put(
             # probably better to turn on/off public-read on the bucket instead
             kwargs["ACL"] = acl
 
+        if filename:
+            kwargs.setdefault("Metadata", {})
+            kwargs["Metadata"]["filename"] = self._make_s3_safe(filename)
+
         try:
             self.call("put_object", Key=_id, Body=content, ContentType=content_type, **kwargs)
             return _id

tests/storage/amazon_media_storage_test.py

@@ -28,41 +28,40 @@ def setUp(self):
         p.start()
         self.addCleanup(p.stop)
 
-    def test_media_url(self):
-        filename = "test"
+    @patch("superdesk.storage.amazon_media_storage.ObjectId", return_value="507f1f77bcf86cd799439011")
+    def test_media_url(self, _mock_object_id):
         # automatic version is set on hourly granularity.
         time_id = time.strftime("%Y%m%d%H")
-        media_id = self.amazon.media_id(filename)
-        self.assertEqual("%s/%s" % (time_id, filename), media_id)
+        media_id = self.amazon.media_id()
+        self.assertEqual(f"{time_id}/507f1f77bcf86cd799439011", media_id)
         self.assertEqual(self.amazon.url_for_media(media_id), "https://acname.s3-us-east-1.amazonaws.com/%s" % media_id)
         sub = "test-sub"
         settings = {"AMAZON_S3_SUBFOLDER": sub, "MEDIA_PREFIX": "https://acname.s3-us-east-1.amazonaws.com/" + sub}
         with patch.dict(self.app.config, settings):
-            media_id = self.amazon.media_id(filename)
-            self.assertEqual("%s/%s" % (time_id, filename), media_id)
+            media_id = self.amazon.media_id()
+            self.assertEqual(f"{time_id}/507f1f77bcf86cd799439011", media_id)
             path = "%s/%s" % (sub, media_id)
             self.assertEqual(self.amazon.url_for_media(media_id), "https://acname.s3-us-east-1.amazonaws.com/%s" % path)
             with patch.object(self.amazon, "client") as s3:
                 self.amazon.get(media_id)
                 self.assertTrue(s3.get_object.called)
                 self.assertEqual(s3.get_object.call_args[1], dict(Bucket="acname", Key=path))
 
-    def test_media_id_time_prefix(self):
-        filename = "test"
-
+    @patch("superdesk.storage.amazon_media_storage.ObjectId", return_value="507f1f77bcf86cd799439011")
+    def test_media_id_time_prefix(self, _mock_object_id):
         with patch.dict(self.app.config, {"AMAZON_MEDIA_ID_TIME_PREFIX": "none"}):
-            media_id = self.amazon.media_id(filename)
-            self.assertEqual(filename, media_id)
+            media_id = self.amazon.media_id()
+            self.assertEqual("507f1f77bcf86cd799439011", media_id)
 
         with patch("superdesk.storage.amazon_media_storage.time.strftime", return_value="20260102"):
             with patch.dict(self.app.config, {"AMAZON_MEDIA_ID_TIME_PREFIX": "daily"}):
-                media_id = self.amazon.media_id(filename)
-                self.assertEqual("20260102/%s" % filename, media_id)
+                media_id = self.amazon.media_id()
+                self.assertEqual("20260102/507f1f77bcf86cd799439011", media_id)
 
         with patch("superdesk.storage.amazon_media_storage.time.strftime", return_value="2026010215"):
             with patch.dict(self.app.config, {"AMAZON_MEDIA_ID_TIME_PREFIX": "hourly"}):
-                media_id = self.amazon.media_id(filename)
-                self.assertEqual("2026010215/%s" % filename, media_id)
+                media_id = self.amazon.media_id()
+                self.assertEqual("2026010215/507f1f77bcf86cd799439011", media_id)
 
     def test_put_and_delete(self):
         """Test amazon if configured.
@@ -83,22 +82,24 @@ def test_put_and_delete(self):
         else:
             self.assertTrue(True)
 
-    def test_put_into_folder(self):
+    @patch("superdesk.storage.amazon_media_storage.ObjectId", return_value="507f1f77bcf86cd799439011")
+    @patch("superdesk.storage.amazon_media_storage.time.strftime", return_value="2026010215")
+    def test_put_into_folder(self, mock_time_strftime, _mock_object_id):
         data = b"test data"
         folder = "s3test"
         filename = "abc123.zip"
         content_type = "application/zip"
         self.amazon.client.put_object = Mock()
-        self.amazon.media_id = Mock(return_value=filename)
         self.amazon._check_exists = Mock(return_value=False)
 
         self.amazon.put(data, filename, content_type, folder=folder)
 
         kwargs = {
-            "Key": "{}/{}".format(folder, filename),
+            "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
             "Body": data,
             "Bucket": "acname",
             "ContentType": content_type,
+            "Metadata": {"filename": filename},
         }
         self.amazon.client.put_object.assert_called_once_with(**kwargs)
 
@@ -147,30 +148,28 @@ def test_guess_extension(self):
         # leave empty when there is no extension
         self.assertEqual("", guess_media_extension("audio/foo"))
 
-    def test_media_url_none_utf8(self):
+    @patch("superdesk.storage.amazon_media_storage.ObjectId", return_value="507f1f77bcf86cd799439011")
+    def test_media_url_none_utf8(self, _mock_object_id):
         filename = "[DIARY NOTE] – Victory In The Pacific Day Commemoration - Thursday (1)"
         # automatic version is set on hourly granularity.
         time_id = time.strftime("%Y%m%d%H")
-        media_id = self.amazon.media_id(filename)
-        self.assertEqual(
-            "%s/%s" % (time_id, "DIARY NOTE - Victory In The Pacific Day Commemoration - Thursday (1)"), media_id
-        )
+        media_id = self.amazon.media_id()
+        self.assertEqual(f"{time_id}/507f1f77bcf86cd799439011", media_id)
         self.assertEqual(self.amazon.url_for_media(media_id), "https://acname.s3-us-east-1.amazonaws.com/%s" % media_id)
         sub = "test-sub"
         settings = {"AMAZON_S3_SUBFOLDER": sub, "MEDIA_PREFIX": "https://acname.s3-us-east-1.amazonaws.com/" + sub}
         with patch.dict(self.app.config, settings):
-            media_id = self.amazon.media_id(filename)
-            self.assertEqual(
-                "%s/%s" % (time_id, "DIARY NOTE - Victory In The Pacific Day Commemoration - Thursday (1)"), media_id
-            )
+            media_id = self.amazon.media_id()
+            self.assertEqual(f"{time_id}/507f1f77bcf86cd799439011", media_id)
             path = "%s/%s" % (sub, media_id)
             self.assertEqual(self.amazon.url_for_media(media_id), "https://acname.s3-us-east-1.amazonaws.com/%s" % path)
             with patch.object(self.amazon, "client") as s3:
                 self.amazon.get(media_id)
                 self.assertTrue(s3.get_object.called)
                 self.assertEqual(s3.get_object.call_args[1], dict(Bucket="acname", Key=path))
 
-    def test_put_into_folder_none_utf8(self):
+    @patch("superdesk.storage.amazon_media_storage.ObjectId", return_value="507f1f77bcf86cd799439011")
+    def test_put_into_folder_none_utf8(self, _mock_object_id):
         data = b"test data"
         folder = "s3test"
         filename = "[DIARY NOTE] – Victory In The Pacific Day Commemoration - Thursday (1).pdf"
@@ -181,10 +180,11 @@ def test_put_into_folder_none_utf8(self):
         self.amazon.put(data, filename, content_type, folder=folder, version=False)
 
         kwargs = {
-            "Key": "{}/{}".format(folder, "DIARY NOTE - Victory In The Pacific Day Commemoration - Thursday (1).pdf"),
+            "Key": f"{folder}/507f1f77bcf86cd799439011",
             "Body": data,
             "Bucket": "acname",
             "ContentType": content_type,
+            "Metadata": {"filename": "DIARY NOTE - Victory In The Pacific Day Commemoration - Thursday (1).pdf"},
         }
         self.amazon.client.put_object.assert_called_once_with(**kwargs)
 
@@ -197,22 +197,24 @@ def test_get_none_utf8(self):
         kwargs = {"Bucket": "acname", "Key": "DIARY NOTE - Victory In The Pacific Day Commemoration - Thursday (1).pdf"}
         self.amazon.client.get_object.assert_called_once_with(**kwargs)
 
-    def test_mimetype_detect(self):
+    @patch("superdesk.storage.amazon_media_storage.ObjectId", return_value="507f1f77bcf86cd799439011")
+    @patch("superdesk.storage.amazon_media_storage.time.strftime", return_value="2026010215")
+    def test_mimetype_detect(self, mock_time_strftime, _mock_object_id):
         # keep default mimetype
         content = b"bytes are here"
         filename = "extensionless"
         content_type = "text/css"
         folder = "f1"
         self.amazon.client.put_object = Mock()
         self.amazon._check_exists = Mock(return_value=False)
-        self.amazon.media_id = Mock(return_value=filename)
         self.amazon.put(content, filename, content_type, folder=folder)
         self.amazon.client.put_object.assert_called_once_with(
             **{
-                "Key": "{}/{}".format(folder, filename),
+                "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                 "Body": content,
                 "Bucket": "acname",
                 "ContentType": content_type,
+                "Metadata": {"filename": filename},
             }
         )
 
@@ -223,14 +225,14 @@ def test_mimetype_detect(self):
         folder = "f1"
         self.amazon.client.put_object = Mock()
         self.amazon._check_exists = Mock(return_value=False)
-        self.amazon.media_id = Mock(return_value=filename)
         self.amazon.put(content, filename, content_type, folder=folder)
         self.amazon.client.put_object.assert_called_once_with(
             **{
-                "Key": "{}/{}".format(folder, filename),
+                "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                 "Body": b"bytes are here",
                 "Bucket": "acname",
                 "ContentType": "text/css",
+                "Metadata": {"filename": filename},
             }
         )
 
@@ -240,14 +242,14 @@ def test_mimetype_detect(self):
         folder = "f1"
         self.amazon.client.put_object = Mock()
         self.amazon._check_exists = Mock(return_value=False)
-        self.amazon.media_id = Mock(return_value=filename)
         self.amazon.put(content, filename, content_type, folder=folder)
         self.amazon.client.put_object.assert_called_once_with(
             **{
-                "Key": "{}/{}".format(folder, filename),
+                "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                 "Body": b"bytes are here",
                 "Bucket": "acname",
                 "ContentType": "image/jpeg",
+                "Metadata": {"filename": filename},
             }
         )
 
@@ -260,14 +262,14 @@ def test_mimetype_detect(self):
             folder = "f1"
             self.amazon.client.put_object = Mock()
             self.amazon._check_exists = Mock(return_value=False)
-            self.amazon.media_id = Mock(return_value=filename)
             self.amazon.put(content, filename, content_type, folder=folder)
             self.amazon.client.put_object.assert_called_once_with(
                 **{
-                    "Key": "{}/{}".format(folder, filename),
+                    "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                     "Body": content,
                     "Bucket": "acname",
                     "ContentType": "image/jpeg",
+                    "Metadata": {"filename": filename},
                 }
             )
 
@@ -277,14 +279,14 @@ def test_mimetype_detect(self):
                 folder = "f1"
                 self.amazon.client.put_object = Mock()
                 self.amazon._check_exists = Mock(return_value=False)
-                self.amazon.media_id = Mock(return_value=filename)
                 self.amazon.put(content, filename, content_type, folder=folder)
                 self.amazon.client.put_object.assert_called_once_with(
                     **{
-                        "Key": "{}/{}".format(folder, filename),
+                        "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                         "Body": content,
                         "Bucket": "acname",
                         "ContentType": "application/vnd.ms-excel",
+                        "Metadata": {"filename": filename},
                     }
                 )
 
@@ -294,14 +296,14 @@ def test_mimetype_detect(self):
                 folder = "f1"
                 self.amazon.client.put_object = Mock()
                 self.amazon._check_exists = Mock(return_value=False)
-                self.amazon.media_id = Mock(return_value=filename)
                 self.amazon.put(content, filename, content_type, folder=folder)
                 self.amazon.client.put_object.assert_called_once_with(
                     **{
-                        "Key": "{}/{}".format(folder, filename),
+                        "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                         "Body": content,
                         "Bucket": "acname",
                         "ContentType": "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
+                        "Metadata": {"filename": filename},
                     }
                 )
 
@@ -311,13 +313,13 @@ def test_mimetype_detect(self):
                 folder = "f1"
                 self.amazon.client.put_object = Mock()
                 self.amazon._check_exists = Mock(return_value=False)
-                self.amazon.media_id = Mock(return_value=filename)
                 self.amazon.put(content, filename, content_type, folder=folder)
                 self.amazon.client.put_object.assert_called_once_with(
                     **{
-                        "Key": "{}/{}".format(folder, filename),
+                        "Key": f"{folder}/2026010215/507f1f77bcf86cd799439011",
                         "Body": content,
                         "Bucket": "acname",
                         "ContentType": "application/vnd.openxmlformats-officedocument.wordprocessingml.document",
+                        "Metadata": {"filename": filename},
                     }
                 )