From c243f68ff45261213af4ca903a769e702907e2e2 Mon Sep 17 00:00:00 2001 From: len Date: Sun, 10 Dec 2023 20:11:35 +0100 Subject: [PATCH] [FIX] fs_attachment: broken links when nonempty paths Let "image.png" be an image. Thumbnails might be created with a path such as "38/5b/image-128.png". Before, the path was forgotten when updating the attachment, so accessing the file would crash, making the record inaccessible. --- fs_attachment/README.rst | 3 +- fs_attachment/models/ir_attachment.py | 2 +- fs_attachment/readme/CONTRIBUTORS.rst | 1 + fs_attachment/readme/newsfragments/312.bugfix | 1 + fs_attachment/static/description/index.html | 5 ++- fs_attachment/tests/common.py | 23 +++++++++- fs_attachment/tests/test_fs_attachment.py | 45 +++++++++++++++++++ 7 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 fs_attachment/readme/newsfragments/312.bugfix diff --git a/fs_attachment/README.rst b/fs_attachment/README.rst index df4133537f..b58346c6d5 100644 --- a/fs_attachment/README.rst +++ b/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 Laurent Mignon Marie Lejeune Wolfgang Pichler +Nans Lefebvre Maintainers ~~~~~~~~~~~ diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 8774ed77aa..51dd959af5 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/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): diff --git a/fs_attachment/readme/CONTRIBUTORS.rst b/fs_attachment/readme/CONTRIBUTORS.rst index 886c390695..d8537f5671 100644 --- a/fs_attachment/readme/CONTRIBUTORS.rst +++ b/fs_attachment/readme/CONTRIBUTORS.rst @@ -11,3 +11,4 @@ Stephane Mangin Laurent Mignon Marie Lejeune Wolfgang Pichler +Nans Lefebvre diff --git a/fs_attachment/readme/newsfragments/312.bugfix b/fs_attachment/readme/newsfragments/312.bugfix new file mode 100644 index 0000000000..36fd06d0d5 --- /dev/null +++ b/fs_attachment/readme/newsfragments/312.bugfix @@ -0,0 +1 @@ +Fix the error retrieving attachment files when the storage is set to optimize directory paths. diff --git a/fs_attachment/static/description/index.html b/fs_attachment/static/description/index.html index d3571bb3d1..9a09c966e9 100644 --- a/fs_attachment/static/description/index.html +++ b/fs_attachment/static/description/index.html @@ -367,7 +367,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 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -->

Beta License: AGPL-3 OCA/storage Translate me on Weblate Try me on Runboat

In some cases, you need to store attachment in another system that the Odoo’s @@ -740,7 +740,8 @@

Contributors

Stephane Mangin <stephane.mangin@camptocamp.com> Laurent Mignon <laurent.mignon@acsone.eu> Marie Lejeune <marie.lejeune@acsone.eu> -Wolfgang Pichler <wpichler@callino.at>

+Wolfgang Pichler <wpichler@callino.at> +Nans Lefebvre <len@lambdao.dev>

Maintainers

diff --git a/fs_attachment/tests/common.py b/fs_attachment/tests/common.py index 95ea76d006..076717a90b 100644 --- a/fs_attachment/tests/common.py +++ b/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): diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index 7ba0728b9e..e1f1fde313 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/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"