From 89966f792cdda6db080f3daf81a5b815ddff1be0 Mon Sep 17 00:00:00 2001 From: Hector Vior Date: Mon, 28 Sep 2026 14:33:49 +0200 Subject: [PATCH] [FIX] fs_attachment: don't upload files forced to DB Since Odoo 19, ir.attachment create() and _set_attachment_data() write the file after _get_datas_related_values() for every attachment when the storage is not 'db', including the ones fs_attachment forces to the database through force_db_for_default_attachment_rules. The file is then uploaded to the storage for nothing: it is referenced by no attachment and only removed later by the fs.file.gc, and the write fails when the storage is unreachable, so an attachment meant to be kept in the database cannot be written while the storage is down. Skip the write in _file_write when the content was forced to the database in the current transaction and no attachment references the file. Contents shared with an attachment kept in the storage, and the files written by AttachmentFileLikeAdapter, are still written. Assisted-by: Claude Opus 5.5 --- fs_attachment/models/ir_attachment.py | 33 ++++++++++ .../newsfragments/forced_db_upload.bugfix | 3 + fs_attachment/tests/test_fs_attachment.py | 62 +++++++++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 fs_attachment/readme/newsfragments/forced_db_upload.bugfix diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 3227f7c7c7..c0c819d67f 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -237,6 +237,10 @@ def _get_datas_related_values(self, data, mimetype): "db_datas": data, } ) + # see _is_forced_to_db_and_unreferenced + self.env.cr.cache.setdefault( + "fs_attachment_forced_to_db_checksums", set() + ).add(values["checksum"]) else: # Uses the full object storage path; standard Odoo uses a relative path. path = self._get_fs_path(storage, data) @@ -358,11 +362,40 @@ def _file_read(self, fname, size=None): def _file_write(self, bin_data, checksum): location = self.env.context.get("storage_location") or self._storage() if location in self._get_storage_codes(): + if self._is_forced_to_db_and_unreferenced(location, bin_data, checksum): + return False filename = self._storage_file_write(bin_data) else: filename = super()._file_write(bin_data, checksum) return filename + @api.model + def _is_forced_to_db_and_unreferenced(self, location, bin_data, checksum): + """Return whether writing the file into the storage can be skipped + + Since Odoo 19, ``create`` and ``_set_attachment_data`` write the file + after ``_get_datas_related_values`` for every attachment when the + storage is not 'db', including the ones this module forces to the + database. Uploading them only leaves an orphan file in the storage + and fails when the storage is unreachable. + + The write is skipped only for a content forced to the database in + this transaction that no attachment references, so a file shared with + an attachment kept in the storage is still written, as are the files + written for another purpose (e.g. ``AttachmentFileLikeAdapter``). + """ + if checksum not in self.env.cr.cache.get( + "fs_attachment_forced_to_db_checksums", () + ): + return False + store_fname = f"{location}://{self._get_fs_path(location, bin_data)}" + self.flush_model(["store_fname"]) + self.env.cr.execute( + "SELECT 1 FROM ir_attachment WHERE store_fname = %s LIMIT 1", + (store_fname,), + ) + return not self.env.cr.fetchone() + @api.model def _file_delete(self, fname) -> None: # pylint: disable=missing-return if self._is_file_from_a_storage(fname): diff --git a/fs_attachment/readme/newsfragments/forced_db_upload.bugfix b/fs_attachment/readme/newsfragments/forced_db_upload.bugfix new file mode 100644 index 0000000000..7b29c73200 --- /dev/null +++ b/fs_attachment/readme/newsfragments/forced_db_upload.bugfix @@ -0,0 +1,3 @@ +Attachments forced to the database by `force_db_for_default_attachment_rules` +are no longer also written into the storage, which left an orphan file and +failed when the storage was unreachable. diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index 42dd4cd734..78de89d902 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -402,6 +402,68 @@ def test_force_storage_to_fs_ignore_missing_file(self): # the file should not have been moved self.assertEqual(attachment.store_fname, store_fname, "store_fname not changed") + def test_force_attachment_in_db_rules_no_file_written(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.force_db_for_default_attachment_rules = '{"text/plain": 0}' + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + attachment.write({"raw": b"new content"}) + self.env.flush_all() + self.assertFalse(attachment.store_fname) + self.assertEqual(attachment.db_datas, b"new content") + # nothing uploaded to the storage, nothing left for the gc + self.assertEqual(os.listdir(self.temp_dir), []) + self.assertFalse( + self.gc_file_model.search([("fs_storage_code", "=", "tmp_dir")]) + ) + + def test_force_attachment_in_db_rules_storage_unreachable(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.force_db_for_default_attachment_rules = '{"text/plain": 0}' + with mock.patch.object( + type(self.ir_attachment_model), + "_storage_file_write", + side_effect=MyException("storage unreachable"), + ): + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.env.flush_all() + self.assertEqual(attachment.db_datas, b"content") + + def test_force_attachment_in_db_rules_shared_content(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.force_db_for_default_attachment_rules = '{"text/plain": 0}' + in_db = self.ir_attachment_model.create({"name": "a.txt", "raw": b"content"}) + # same content in the same transaction, not forced to the database: + # the file must still be written + self.temp_backend.force_db_for_default_attachment_rules = "{}" + in_storage = self.ir_attachment_model.create( + {"name": "b.txt", "raw": b"content"} + ) + self.env.flush_all() + self.assertFalse(in_db.store_fname) + self.assertTrue(in_storage.store_fname.startswith("tmp_dir://")) + self.assertEqual(os.listdir(self.temp_dir), [in_storage.fs_filename]) + self.env.invalidate_all() + self.assertEqual(in_storage.raw, b"content") + + def test_force_attachment_in_db_rules_content_already_in_storage(self): + self.temp_backend.use_as_default_for_attachments = True + self.temp_backend.force_db_for_default_attachment_rules = "{}" + in_storage = self.ir_attachment_model.create( + {"name": "a.txt", "raw": b"content"} + ) + self.env.flush_all() + self.temp_backend.force_db_for_default_attachment_rules = '{"text/plain": 0}' + in_db = self.ir_attachment_model.create({"name": "b.txt", "raw": b"content"}) + self.env.flush_all() + self.assertFalse(in_db.store_fname) + self.assertEqual(os.listdir(self.temp_dir), [in_storage.fs_filename]) + self.env.invalidate_all() + self.assertEqual(in_storage.raw, b"content") + def test_storage_use_filename_obfuscation(self): self.temp_backend.base_url = "https://acsone.eu/media" self.temp_backend.use_as_default_for_attachments = True