From 9d365a307ef642709c25ddb660c48f9c1ed6afc4 Mon Sep 17 00:00:00 2001 From: abdulrahman-d1 Date: Mon, 5 Oct 2026 20:38:09 +0400 Subject: [PATCH] [FIX] fs_attachment: don't move a file shared with other attachments With filename obfuscation off, storing or renaming an attachment renames its file. Attachments stored while obfuscation was on share one file per content, so that rename moved the file away from the others and they lost their content. Copy the file when other attachments still use it, and move it only when none does. One grouped query per call counts the attachments on each file. It also covers duplicates created in one call, so the special case for them is gone. --- fs_attachment/models/ir_attachment.py | 36 +++++++++---- fs_attachment/readme/newsfragments/678.bugfix | 6 +++ fs_attachment/tests/test_fs_attachment.py | 53 +++++++++++++++++++ 3 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 fs_attachment/readme/newsfragments/678.bugfix diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 3227f7c7c7..0250ad0365 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -479,7 +479,7 @@ def _enforce_meaningful_storage_filename(self) -> None: Keeping the same meaning and mimetype is important to also ease to provide a meaningful and SEO friendly URL to the file in the filesystem storage. """ - renamed_attachments = {} + to_rename = self.browse() for attachment in self: if not self._is_file_from_a_storage(attachment.store_fname): continue @@ -487,22 +487,27 @@ def _enforce_meaningful_storage_filename(self) -> None: if self.env["fs.storage"]._must_use_filename_obfuscation(storage): attachment.fs_filename = filename - continue + else: + to_rename |= attachment + to_rename._set_meaningful_storage_filename() + + def _set_meaningful_storage_filename(self) -> None: + """Give each attachment's file its meaningful name""" + usage = self._count_by_store_fname() + for attachment in self: + fs, storage, filename = attachment._get_fs_parts() new_filename = attachment._build_fs_filename() # we must keep the same full path as the original filename new_filename_with_path = os.path.join( os.path.dirname(filename), new_filename ) - if filename in renamed_attachments: - if renamed_attachments[filename] == new_filename_with_path: - # we already renamed this file, no need to rename it again - continue - else: - fs.copy(renamed_attachments[filename], new_filename_with_path) + if usage[attachment.store_fname] > 1: + # the file stays with the other attachments stored in it + usage[attachment.store_fname] -= 1 + fs.copy(filename, new_filename_with_path) else: fs.rename(filename, new_filename_with_path) - renamed_attachments[filename] = new_filename_with_path attachment.fs_filename = new_filename # we need to update the store_fname with the new filename by @@ -512,6 +517,19 @@ def _enforce_meaningful_storage_filename(self) -> None: attachment._force_write_store_fname(f"{storage}://{new_filename_with_path}") self._fs_mark_for_gc(attachment.store_fname) + def _count_by_store_fname(self) -> dict[str, int]: + """Return the number of attachments stored in each file of self""" + # "res_field = False OR res_field != False" to count the attachments of + # binary fields too, see fs.storage + domain = [ + ("store_fname", "in", self.mapped("store_fname")), + "|", + ("res_field", "=", False), + ("res_field", "!=", False), + ] + groups = self.sudo()._read_group(domain, ["store_fname"], ["__count"]) + return dict(groups) + def _force_write_store_fname(self, store_fname): """Force the write of the store_fname field diff --git a/fs_attachment/readme/newsfragments/678.bugfix b/fs_attachment/readme/newsfragments/678.bugfix new file mode 100644 index 0000000000..6a5bb2d8b1 --- /dev/null +++ b/fs_attachment/readme/newsfragments/678.bugfix @@ -0,0 +1,6 @@ +Don't move a file that other attachments still use. + +With filename obfuscation off, storing or renaming an attachment renames its +file. Attachments stored while obfuscation was on share one file per content, +so the others lost their content. The module now copies such a file and only +moves it when no other attachment uses it. diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index 42dd4cd734..afcdacd5a2 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -502,6 +502,59 @@ def test_create_two_attachments_differnt_call(self): self.assertNotEqual(res[0].store_fname, res2[0].store_fname) self.assertEqual(res[0].raw, res2[0].raw) + def test_create_attachment_same_content_as_obfuscated_one(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.use_filename_obfuscation = True + attachment1 = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.temp_backend.use_filename_obfuscation = False + attachment2 = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.assertNotEqual(attachment1.store_fname, attachment2.store_fname) + self.env.invalidate_all() + self.assertEqual(attachment1.raw, b"content") + self.assertEqual(attachment2.raw, b"content") + + def test_create_attachment_same_content_as_obfuscated_field_one(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.use_filename_obfuscation = True + attachment1 = self.ir_attachment_model.create( + { + "name": "test.txt", + "raw": b"content", + "res_id": self.env.user.partner_id.id, + "res_model": "res.partner", + "res_field": "image_1920", + } + ) + self.temp_backend.use_filename_obfuscation = False + attachment2 = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.assertNotEqual(attachment1.store_fname, attachment2.store_fname) + self.env.invalidate_all() + self.assertEqual(attachment1.raw, b"content") + self.assertEqual(attachment2.raw, b"content") + + def test_write_name_file_shared_with_obfuscated_attachment(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.use_filename_obfuscation = True + attachment1, attachment2 = self.ir_attachment_model.create( + [ + {"name": "test.txt", "raw": b"content"}, + {"name": "test.txt", "raw": b"content"}, + ] + ) + self.assertEqual(attachment1.store_fname, attachment2.store_fname) + self.temp_backend.use_filename_obfuscation = False + attachment1.name = "test2.txt" + self.assertNotEqual(attachment1.store_fname, attachment2.store_fname) + self.env.invalidate_all() + self.assertEqual(attachment1.raw, b"content") + self.assertEqual(attachment2.raw, b"content") + def test_update_png_to_svg(self): b64_data_png = ( b"iVBORw0KGgoAAAANSUhEUgAAADMAAAAhCAIAAAD73QTtAAAAA3NCSVQICAjb4U/gAA"