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"