[16.0] fs_attachment: fix meaningful name rewriting

fs_attachment/README.rst

@@ -7,7 +7,7 @@ Base Attachment Object Store
    !! This file is generated by oca-gen-addon-readme !!
    !! changes will be overwritten.                   !!
    !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-   !! source digest: sha256:05b68b54a5be6b982d8c997577edfced5e945460759b3869f559cdf1ee5a9a49
+   !! source digest: sha256:a1e2408096e0d95e31cbee39d95122596fd4e0b807750de9a8d28386205f270c
    !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
 .. |badge1| image:: https://img.shields.io/badge/maturity-Beta-yellow.png
@@ -400,6 +400,7 @@ Stephane Mangin <stephane.mangin@camptocamp.com>
 Laurent Mignon <laurent.mignon@acsone.eu>
 Marie Lejeune <marie.lejeune@acsone.eu>
 Wolfgang Pichler <wpichler@callino.at>
+Nans Lefebvre <len@lambdao.dev>
 
 Maintainers
 ~~~~~~~~~~~

fs_attachment/models/ir_attachment.py

@@ -463,7 +463,7 @@ def _enforce_meaningful_storage_filename(self) -> None:
             # we need to update the store_fname with the new filename by
             # calling the write method of the field since the write method
             # of ir_attachment prevent normal write on store_fname
-            attachment._force_write_store_fname(f"{storage}://{new_filename}")
+            attachment._force_write_store_fname(f"{storage}://{new_filename_with_path}")
             self._fs_mark_for_gc(attachment.store_fname)
 
     def _force_write_store_fname(self, store_fname):

fs_attachment/readme/CONTRIBUTORS.rst

@@ -11,3 +11,4 @@ Stephane Mangin <stephane.mangin@camptocamp.com>
 Laurent Mignon <laurent.mignon@acsone.eu>
 Marie Lejeune <marie.lejeune@acsone.eu>
 Wolfgang Pichler <wpichler@callino.at>
+Nans Lefebvre <len@lambdao.dev>

fs_attachment/readme/newsfragments/312.bugfix

@@ -0,0 +1 @@
+Fix the error retrieving attachment files when the storage is set to optimize directory paths.

fs_attachment/static/description/index.html

@@ -367,7 +367,7 @@ <h1 class="title">Base Attachment Object Store</h1>
 !! This file is generated by oca-gen-addon-readme !!
 !! changes will be overwritten.                   !!
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-!! source digest: sha256:05b68b54a5be6b982d8c997577edfced5e945460759b3869f559cdf1ee5a9a49
+!! source digest: sha256:a1e2408096e0d95e31cbee39d95122596fd4e0b807750de9a8d28386205f270c
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -->
 <p><a class="reference external image-reference" href="https://odoo-community.org/page/development-status"><img alt="Beta" src="https://img.shields.io/badge/maturity-Beta-yellow.png" /></a> <a class="reference external image-reference" href="http://www.gnu.org/licenses/agpl-3.0-standalone.html"><img alt="License: AGPL-3" src="https://img.shields.io/badge/licence-AGPL--3-blue.png" /></a> <a class="reference external image-reference" href="https://github.com/OCA/storage/tree/16.0/fs_attachment"><img alt="OCA/storage" src="https://img.shields.io/badge/github-OCA%2Fstorage-lightgray.png?logo=github" /></a> <a class="reference external image-reference" href="https://translation.odoo-community.org/projects/storage-16-0/storage-16-0-fs_attachment"><img alt="Translate me on Weblate" src="https://img.shields.io/badge/weblate-Translate%20me-F47D42.png" /></a> <a class="reference external image-reference" href="https://runboat.odoo-community.org/builds?repo=OCA/storage&amp;target_branch=16.0"><img alt="Try me on Runboat" src="https://img.shields.io/badge/runboat-Try%20me-875A7B.png" /></a></p>
 <p>In some cases, you need to store attachment in another system that the Odoo’s
@@ -740,7 +740,8 @@ <h2><a class="toc-backref" href="#toc-entry-15">Contributors</a></h2>
 Stephane Mangin &lt;<a class="reference external" href="mailto:stephane.mangin&#64;camptocamp.com">stephane.mangin&#64;camptocamp.com</a>&gt;
 Laurent Mignon &lt;<a class="reference external" href="mailto:laurent.mignon&#64;acsone.eu">laurent.mignon&#64;acsone.eu</a>&gt;
 Marie Lejeune &lt;<a class="reference external" href="mailto:marie.lejeune&#64;acsone.eu">marie.lejeune&#64;acsone.eu</a>&gt;
-Wolfgang Pichler &lt;<a class="reference external" href="mailto:wpichler&#64;callino.at">wpichler&#64;callino.at</a>&gt;</p>
+Wolfgang Pichler &lt;<a class="reference external" href="mailto:wpichler&#64;callino.at">wpichler&#64;callino.at</a>&gt;
+Nans Lefebvre &lt;<a class="reference external" href="mailto:len&#64;lambdao.dev">len&#64;lambdao.dev</a>&gt;</p>
 </div>
 <div class="section" id="maintainers">
 <h2><a class="toc-backref" href="#toc-entry-16">Maintainers</a></h2>

fs_attachment/tests/common.py

@@ -21,6 +21,15 @@ def setUpClass(cls):
                 "directory_path": temp_dir,
             }
         )
+        cls.backend_optimized = cls.env["fs.storage"].create(
+            {
+                "name": "Temp Optimized FS Storage",
+                "protocol": "file",
+                "code": "tmp_opt",
+                "directory_path": temp_dir,
+                "optimizes_directory_path": True,
+            }
+        )
         cls.temp_dir = temp_dir
         cls.gc_file_model = cls.env["fs.file.gc"]
         cls.ir_attachment_model = cls.env["ir.attachment"]
@@ -40,12 +49,24 @@ def setUp(self):
                 "directory_path": self.temp_dir,
             }
         )
+        self.backend_optimized.write(
+            {
+                "protocol": "file",
+                "code": "tmp_opt",
+                "directory_path": self.temp_dir,
+                "optimizes_directory_path": True,
+            }
+        )
 
     def tearDown(self) -> None:
         super().tearDown()
         # empty the temp dir
         for f in os.listdir(self.temp_dir):
-            os.remove(os.path.join(self.temp_dir, f))
+            full_path = os.path.join(self.temp_dir, f)
+            if os.path.isfile(full_path):
+                os.remove(full_path)
+            else:  # using optimizes_directory_path, we'll have a directory
+                shutil.rmtree(full_path)
 
 
 class MyException(Exception):

fs_attachment/tests/test_fs_attachment.py

@@ -30,6 +30,51 @@ def test_create_attachment_explicit_location(self):
             f.write(b"new")
         self.assertEqual(attachment.raw, b"new")
 
+    def test_create_attachment_with_meaningful_name(self):
+        """In this test we use a backend with 'optimizes_directory_path',
+        which rewrites the filename to be a meaningful name.
+        We ensure that the rewritten path is consistently used,
+        meaning we can read the file after.
+        """
+        content = b"This is a test attachment"
+        attachment = (
+            self.env["ir.attachment"]
+            .with_context(
+                storage_location=self.backend_optimized.code,
+                force_storage_key="test.txt",
+            )
+            .create({"name": "test.txt", "raw": content})
+        )
+        # the expected store_fname is made of the storage code,
+        # a random middle part, and the filename
+        # example: tmp_opt://te/st/test-198-0.txt
+        # The storage root is NOT part of the store_fname
+        self.assertFalse("tmp/" in attachment.store_fname)
+
+        # remove protocol and file name to keep the middle part
+        sub_path = os.path.dirname(attachment.store_fname.split("://")[1])
+        # the subpath is consistently 'te/st' because the file storage key is forced
+        # if it's arbitrary we might get a random name (3fbc5er....txt), in which case
+        # the middle part would also be 'random', in our example 3f/bc
+        self.assertEqual(sub_path, "te/st")
+
+        # we can read the file, so storage finds it correctly
+        with attachment.open("rb") as f:
+            self.assertEqual(f.read(), content)
+
+        new_content = b"new content"
+        with attachment.open("wb") as f:
+            f.write(new_content)
+
+        # the store fname should have changed, as its version number has increased
+        # e.g. tmp_opt://te/st/test-1766-0.txt to tmp_opt://te/st/test-1766-1.txt
+        # but the protocol and sub path should be the same
+        new_sub_path = os.path.dirname(attachment.store_fname.split("://")[1])
+        self.assertEqual(sub_path, new_sub_path)
+
+        with attachment.open("rb") as f:
+            self.assertEqual(f.read(), new_content)
+
     def test_open_attachment_in_db(self):
         self.env["ir.config_parameter"].sudo().set_param("ir_attachment.location", "db")
         content = b"This is a test attachment in db"