From a01cf9f618461e6c777e2b69e2df8c00eadbe9a8 Mon Sep 17 00:00:00 2001 From: abdulrahman-d1 Date: Mon, 5 Oct 2026 20:38:09 +0400 Subject: [PATCH 1/9] [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 b75ca583c7..71669ec8a0 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -478,7 +478,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 @@ -486,22 +486,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 @@ -511,6 +516,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 f6a57d6904..abb9abd154 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -479,6 +479,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" From 0a362105f7ad045990d5628b2b954165c3f500ff Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:21:49 +0200 Subject: [PATCH 2/9] [FIX] fs_attachment: adapt _count_by_store_fname to the 16.0 _read_group API Fix backport of https://github.com/OCA/storage/pull/678 from 19.0 --- fs_attachment/models/ir_attachment.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 71669ec8a0..e2131ddefc 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -526,8 +526,10 @@ def _count_by_store_fname(self) -> dict[str, int]: ("res_field", "=", False), ("res_field", "!=", False), ] - groups = self.sudo()._read_group(domain, ["store_fname"], ["__count"]) - return dict(groups) + groups = self.sudo()._read_group( + domain, ["store_fname"], ["store_fname"], lazy=False + ) + return {group["store_fname"]: group["__count"] for group in groups} def _force_write_store_fname(self, store_fname): """Force the write of the store_fname field From 773ccf436e6cfa6a242b5c34cae492fcb470e09a Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:23:46 +0200 Subject: [PATCH 3/9] [FIX] fs_attachment: don't move a file not written by the transaction If a file not written into the current transaction is moved or renamed if will be lost in case of rollback. In place, we copy the original one and make it to GC so in case of rollback the content is not lost and file unreferenced... --- fs_attachment/models/ir_attachment.py | 24 ++++++++++++-- fs_attachment/tests/test_fs_attachment.py | 40 +++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index e2131ddefc..00df153ef6 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -414,6 +414,7 @@ def _storage_file_write(self, bin_data: bytes) -> str: with fs.open(path, "wb", **kwargs) as f: f.write(bin_data) self._fs_mark_for_gc(fname) + self._fs_get_files_written_in_transaction().add(fname) return fname @api.model @@ -493,18 +494,27 @@ def _enforce_meaningful_storage_filename(self) -> None: def _set_meaningful_storage_filename(self) -> None: """Give each attachment's file its meaningful name""" usage = self._count_by_store_fname() + written_files = self._fs_get_files_written_in_transaction() for attachment in self: fs, storage, filename = attachment._get_fs_parts() + old_store_fname = attachment.store_fname 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 usage[attachment.store_fname] > 1: + if usage[old_store_fname] > 1: # the file stays with the other attachments stored in it - usage[attachment.store_fname] -= 1 + usage[old_store_fname] -= 1 fs.copy(filename, new_filename_with_path) + elif old_store_fname not in written_files: + # The file has not been written by the current transaction + # We can't rename it to ensure that in case of rollback + # the original one still exists... In place we copy + # the original and we mark it to GC + fs.copy(filename, new_filename_with_path) + self._fs_mark_for_gc(old_store_fname) else: fs.rename(filename, new_filename_with_path) @@ -515,6 +525,16 @@ def _set_meaningful_storage_filename(self) -> None: # flake8: noqa: E231 attachment._force_write_store_fname(f"{storage}://{new_filename_with_path}") self._fs_mark_for_gc(attachment.store_fname) + written_files.add(attachment.store_fname) + + @api.model + def _fs_get_files_written_in_transaction(self) -> set[str]: + """Return the store_fname of the files written into a filesystem + storage by the current transaction. + """ + return self.env.cr.postcommit.data.setdefault( + "fs_attachment.written_files", set() + ) def _count_by_store_fname(self) -> dict[str, int]: """Return the number of attachments stored in each file of self""" diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index abb9abd154..90c2727373 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -532,6 +532,46 @@ def test_write_name_file_shared_with_obfuscated_attachment(self): self.assertEqual(attachment1.raw, b"content") self.assertEqual(attachment2.raw, b"content") + def test_write_name_transactional(self): + """If the transaction is rolled back after a change of name, the + attachment must still reference an existing file""" + self.temp_backend.use_as_default_for_attachments = True + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + self.env.flush_all() + initial_filename = f"test-{attachment.id}-0.txt" + initial_store_fname = attachment.store_fname + self.assertEqual(initial_store_fname, f"tmp_dir://{initial_filename}") + # simulate the commit of the transaction that created the file + self.env.cr.postcommit.data.pop("fs_attachment.written_files", None) + try: + with self.env.cr.savepoint(): + attachment.name = "test2.txt" + new_filename = f"test2-{attachment.id}-1.txt" + self.assertEqual(attachment.store_fname, f"tmp_dir://{new_filename}") + # the original file is copied, not moved + self.assertEqual( + set(os.listdir(self.temp_dir)), {initial_filename, new_filename} + ) + gc_files = self.gc_file_model.search([]).mapped("store_fname") + self.assertIn(initial_store_fname, gc_files) + raise MyException("dummy exception") + except MyException: + ... + self.assertEqual(attachment.store_fname, initial_store_fname) + self.assertEqual(attachment.raw, b"content") + + def test_write_name_of_new_attachment_renames_file(self): + """A file written by the current transaction is renamed, not copied""" + self.temp_backend.use_as_default_for_attachments = True + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"content"} + ) + attachment.name = "test2.txt" + self.env.flush_all() + self.assertEqual(os.listdir(self.temp_dir), [f"test2-{attachment.id}-1.txt"]) + def test_update_png_to_svg(self): b64_data_png = ( b"iVBORw0KGgoAAAANSUhEUgAAADMAAAAhCAIAAAD73QTtAAAAA3NCSVQICAjb4U/gAA" From 6e62bd31802b1c376543bca17ce035fe33dc2685 Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:25:13 +0200 Subject: [PATCH 4/9] [FIX] fs_attachment: apply missing cache policy to the FsStream responses Apply missing cache policy for non public attachment as it's done in odoo by default --- fs_attachment/fs_stream.py | 20 ++++++++++++++++---- fs_attachment/tests/test_stream.py | 25 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/fs_attachment/fs_stream.py b/fs_attachment/fs_stream.py index 28d3ea9690..64908201cc 100644 --- a/fs_attachment/fs_stream.py +++ b/fs_attachment/fs_stream.py @@ -28,6 +28,7 @@ def from_fs_attachment(cls, attachment: IrAttachment) -> FsStream: type="fs", size=attachment.file_size, last_modified=attachment["__last_update"], + public=attachment.public, fs_attachment=attachment, ) @@ -46,7 +47,10 @@ def get_response( ): if self.type != "fs": return super().get_response( - as_attachment=as_attachment, immutable=immutable, **send_file_kwargs + as_attachment=as_attachment, + immutable=immutable, + content_security_policy=content_security_policy, + **send_file_kwargs, ) if as_attachment is None: as_attachment = self.as_attachment @@ -64,6 +68,7 @@ def get_response( "max_age": STATIC_CACHE_LONG if immutable else self.max_age, "environ": request.httprequest.environ, "response_class": Response, + **send_file_kwargs, } use_x_sendfile = self.fs_attachment._fs_use_x_sendfile() # The file will be closed by werkzeug... @@ -81,12 +86,19 @@ def get_response( res.headers["X-Sendfile"] = x_sendfile_path res.headers["Content-Length"] = 0 - if immutable and res.cache_control: - res.cache_control["immutable"] = None - res.headers["X-Content-Type-Options"] = "nosniff" if content_security_policy: res.headers["Content-Security-Policy"] = content_security_policy + # apply same cache policy as odoo.http.Stream.get_response + if self.public: + if (res.cache_control.max_age or 0) > 0: + res.cache_control.public = True + else: + res.cache_control.pop("public", "") + res.cache_control.private = True + if immutable: + res.cache_control["immutable"] = None + return res diff --git a/fs_attachment/tests/test_stream.py b/fs_attachment/tests/test_stream.py index 5f5c32288b..ab11e31509 100644 --- a/fs_attachment/tests/test_stream.py +++ b/fs_attachment/tests/test_stream.py @@ -162,6 +162,31 @@ def test_response_csp_header(self): }, ) + def test_response_cache_control_private(self): + self.authenticate("admin", "admin") + for url in ( + f"/web/content/{self.attachment_binary.id}", + # immutable content must not become cacheable by proxies + f"/web/content/{self.attachment_binary.id}?unique=1", + ): + res = self.assertDownload( + url, headers={}, assert_status_code=200, assert_headers={} + ) + cache_control = res.headers.get("Cache-Control", "") + self.assertIn("private", cache_control) + self.assertNotIn("public", cache_control) + + def test_response_cache_control_public(self): + self.attachment_binary.public = True + url = f"/web/content/{self.attachment_binary.id}?unique=1" + res = self.assertDownload( + url, headers={}, assert_status_code=200, assert_headers={} + ) + cache_control = res.headers.get("Cache-Control", "") + self.assertIn("public", cache_control) + self.assertIn("immutable", cache_control) + self.assertNotIn("private", cache_control) + def test_serving_field_image(self): self.authenticate("admin", "admin") demo_partner = self.env.ref("base.partner_demo") From deb5af91c49f3a5033f63390c2281ba25cee5ea2 Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:26:02 +0200 Subject: [PATCH 5/9] [FIX] fs_attachment: don't reload the registry to get a new cursor --- fs_attachment/models/ir_attachment.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 00df153ef6..4eccca38eb 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -17,7 +17,6 @@ import psycopg2 from slugify import slugify # pylint: disable=missing-manifest-dependency -import odoo from odoo import _, api, fields, models from odoo.exceptions import AccessError, UserError from odoo.osv.expression import AND, OR, normalize_domain @@ -718,8 +717,7 @@ def _do_in_new_env(self, new_cr=False): Using a new Odoo Environment thus a new PG transaction. """ if new_cr: - registry = odoo.modules.registry.Registry.new(self.env.cr.dbname) - with closing(registry.cursor()) as cr: + with closing(self.env.registry.cursor()) as cr: try: yield self.env(cr=cr) except Exception: From 1da5e9e3a237c7e7aa5d81260106f9a8f353d0bf Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:26:55 +0200 Subject: [PATCH 6/9] [FIX] fs_attachment: Ensure attachment moved to db no more linked to fs storage recompute fs_storage_id when store_fname changes fs_storage_id is computed from store_fname but only depended on fs_filename. When the content of an attachment is moved from a filesystem storage to the database (e.g. by force_storage_to_db_for_special_fields), store_fname is reset while fs_filename is unchanged. This fix ensure that the the attachment moved to database is no more linked to the filesystem storage. --- fs_attachment/models/ir_attachment.py | 2 +- fs_attachment/tests/test_fs_attachment.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 4eccca38eb..850bd5f8e6 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -130,7 +130,7 @@ def _compute_fs_url_path(self) -> None: rec, exclude_base_url=True ) - @api.depends("fs_filename") + @api.depends("store_fname", "fs_filename") def _compute_fs_storage_id(self): for rec in self: if rec.store_fname: diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index 90c2727373..89132bc2cd 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -347,12 +347,15 @@ def test_force_storage_to_db(self): self.env.flush_all() self.assertTrue(attachment.store_fname) self.assertFalse(attachment.db_datas) + self.assertEqual(attachment.fs_storage_id, self.temp_backend) store_fname = attachment.store_fname # we change the rules to force the storage in db for text/plain self.temp_backend.force_db_for_default_attachment_rules = '{"text/plain": 0}' attachment.force_storage_to_db_for_special_fields() self.assertFalse(attachment.store_fname) self.assertEqual(attachment.db_datas, b"content") + self.assertFalse(attachment.fs_storage_id) + self.assertFalse(attachment.fs_storage_code) # we check that the file is marked for GC gc_files = self.gc_file_model.search([]).mapped("store_fname") self.assertIn(store_fname, gc_files) From 1dc00e5a6c0bb34d74eb0f40212cf4fa8d371220 Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:28:43 +0200 Subject: [PATCH 7/9] [FIX] fs_attachment: update checksum and file_size when writing with open() --- fs_attachment/models/ir_attachment.py | 18 ++++++++++++++++++ .../test_fs_attachment_file_like_adapter.py | 12 ++++++++++++ 2 files changed, 30 insertions(+) diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 850bd5f8e6..0a897b5aab 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -559,6 +559,16 @@ def _force_write_store_fname(self, store_fname): """ self._fields["store_fname"].write(self, store_fname) + def _force_write_file_values(self, vals): + """Force the write of the fields computed from the file content + + The write method of ir_attachment ignores the fields 'checksum' and + 'file_size'. This method bypass this limitation by calling the write + method of the fields directly. + """ + for field_name, value in vals.items(): + self._fields[field_name].write(self, value) + @api.model def _get_fs_storage_for_code( self, @@ -1170,7 +1180,15 @@ def _file_close(self): and self._is_stored_in_db ): attachment_data["raw"] = self._file.getvalue() + # the values computed from the content are removed from the + # values by ir.attachment.write + file_values = { + "checksum": attachment_data["checksum"], + "file_size": attachment_data["file_size"], + } self.attachment.write(attachment_data) + if "raw" not in attachment_data: + self.attachment._force_write_file_values(file_values) if self.new_version and self._new_store_fname: self.attachment._force_write_store_fname(self._new_store_fname) self.attachment._enforce_meaningful_storage_filename() diff --git a/fs_attachment/tests/test_fs_attachment_file_like_adapter.py b/fs_attachment/tests/test_fs_attachment_file_like_adapter.py index bac729c136..edcbaad580 100644 --- a/fs_attachment/tests/test_fs_attachment_file_like_adapter.py +++ b/fs_attachment/tests/test_fs_attachment_file_like_adapter.py @@ -17,6 +17,7 @@ def prepareClass(cls): def prepare(self): self.attachment = self._create_attachment() + self.initial_checksum = self.attachment.checksum def open(self, attachment=None, mode="rb", new_version=False, **kwargs): return AttachmentFileLikeAdapter( @@ -26,6 +27,13 @@ def open(self, attachment=None, mode="rb", new_version=False, **kwargs): **kwargs, ) + def assertFileValues(self, content): + """The values computed from the content must be up to date""" + # the checksum is used as ETag and must change with the content + self.assertTrue(self.attachment.checksum) + self.assertNotEqual(self.attachment.checksum, self.initial_checksum) + self.assertEqual(self.attachment.file_size, len(content)) + def test_read(self): with self.open(mode="rb") as f: self.assertEqual(f.read(), self.initial_content) @@ -34,18 +42,21 @@ def test_write(self): with self.open(mode="wb") as f: f.write(self.new_content) self.assertEqual(self.new_content, self.attachment.raw) + self.assertFileValues(self.new_content) def test_write_append(self): self.assertEqual(self.initial_content, self.attachment.raw) with self.open(mode="ab") as f: f.write(self.new_content) self.assertEqual(self.initial_content + self.new_content, self.attachment.raw) + self.assertFileValues(self.initial_content + self.new_content) def test_write_new_version(self): initial_fname = self.attachment.store_fname with self.open(mode="wb", new_version=True) as f: f.write(self.new_content) self.assertEqual(self.new_content, self.attachment.raw) + self.assertFileValues(self.new_content) if initial_fname: self.assertNotEqual(self.attachment.store_fname, initial_fname) @@ -54,6 +65,7 @@ def test_write_append_new_version(self): with self.open(mode="ab", new_version=True) as f: f.write(self.new_content) self.assertEqual(self.initial_content + self.new_content, self.attachment.raw) + self.assertFileValues(self.initial_content + self.new_content) if initial_fname: self.assertNotEqual(self.attachment.store_fname, initial_fname) From c7d42b37642d041f3662807f6736a4f06cebe9ed Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Fri, 16 Oct 2026 14:29:54 +0200 Subject: [PATCH 8/9] [FIX] fs_attachment: open a new version of a content stored elsewhere Fix mismatch in storage location when an attachment change its storage --- fs_attachment/models/ir_attachment.py | 58 ++++++++++++++++++----- fs_attachment/tests/test_fs_attachment.py | 23 +++++++++ 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 0a897b5aab..ea24686636 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -9,6 +9,7 @@ import mimetypes import os import re +import shutil import time from contextlib import closing, contextmanager from pathlib import Path @@ -1137,19 +1138,9 @@ def _file_open(self) -> io.IOBase: attachment_res_model=self.attachment.res_model, attachment_res_field=self.attachment.res_field, )._file_write(content, checksum) - if self.attachment._is_file_from_a_storage(new_store_fname): - ( - filesystem, - _storage, - new_filepath, - ) = self.attachment._fs_parse_store_fname(new_store_fname) - _fs, _storage, old_filepath = self.attachment._get_fs_parts() - else: - new_filepath = self.attachment._full_path(new_store_fname) - old_filepath = self.attachment._full_path(self.attachment.store_fname) - filesystem = fsspec.filesystem("file") + filesystem, new_filepath = self._get_filesystem_and_path(new_store_fname) if "a" in self.mode: - filesystem.cp_file(old_filepath, new_filepath) + self._copy_current_content(filesystem, new_filepath, new_store_fname) the_file = filesystem.open( new_filepath, mode=self.mode, @@ -1162,6 +1153,49 @@ def _file_open(self) -> io.IOBase: self._new_store_fname = new_store_fname self._file = the_file + def _get_filesystem_and_path( + self, store_fname: str + ) -> tuple[fsspec.AbstractFileSystem, str]: + """Return the filesystem and the path of the file for the given + store_fname (filesystem storage or odoo filestore)""" + if self.attachment._is_file_from_a_storage(store_fname): + filesystem, _storage, path = self.attachment._fs_parse_store_fname( + store_fname + ) + return filesystem, path + return fsspec.filesystem("file"), self.attachment._full_path(store_fname) + + def _get_location(self, store_fname: str) -> str: + """Return the code of the filesystem storage where the file is + stored or 'file' for the odoo filestore""" + if self.attachment._is_file_from_a_storage(store_fname): + return store_fname.partition("://")[0] + return "file" + + def _copy_current_content( + self, + filesystem: fsspec.AbstractFileSystem, + new_filepath: str, + new_store_fname: str, + ) -> None: + """Copy the current content of the attachment into the new file + + The current content can be stored in the database, in the odoo + filestore or in a filesystem storage different from the one of the + new file. + """ + old_store_fname = self.attachment.store_fname + if old_store_fname and self._get_location( + old_store_fname + ) == self._get_location(new_store_fname): + # same location: copy the file + _filesystem, old_filepath = self._get_filesystem_and_path(old_store_fname) + filesystem.cp_file(old_filepath, new_filepath) + return + with AttachmentFileLikeAdapter(self.attachment, mode="rb") as src: + with filesystem.open(new_filepath, "wb") as dst: + shutil.copyfileobj(src, dst) + def _gen_random_content(self, size=256): """Generate a random content of size bytes""" return os.urandom(size) diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index 89132bc2cd..7db5571e43 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -535,6 +535,29 @@ def test_write_name_file_shared_with_obfuscated_attachment(self): self.assertEqual(attachment1.raw, b"content") self.assertEqual(attachment2.raw, b"content") + def test_open_new_version_content_stored_elsewhere(self): + """Open a new version of a file into a filesystem storage while the + current content is stored in the database or in the odoo filestore""" + for location in ("db", "file"): + for mode, expected in ( + ("wb", b" new"), + ("ab", b"initial new"), + ): + with self.subTest(location=location, mode=mode): + self.temp_backend.use_as_default_for_attachments = False + self.env["ir.config_parameter"].sudo().set_param( + "ir_attachment.location", location + ) + attachment = self.ir_attachment_model.create( + {"name": "test.txt", "raw": b"initial"} + ) + self.assertFalse(attachment.fs_storage_id) + self.temp_backend.use_as_default_for_attachments = True + with attachment.open(mode, new_version=True) as f: + f.write(b" new") + self.assertEqual(attachment.raw, expected) + self.assertTrue(attachment.store_fname.startswith("tmp_dir://")) + def test_write_name_transactional(self): """If the transaction is rolled back after a change of name, the attachment must still reference an existing file""" From b8be3d115fd3118dce03358410188119ae5fd86a Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Mon, 19 Oct 2026 10:34:23 +0200 Subject: [PATCH 9/9] [FIX] fs_image: Fix test due to changes in #678 --- fs_image/tests/test_fs_image.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs_image/tests/test_fs_image.py b/fs_image/tests/test_fs_image.py index fb9a9b8f64..1520f53ed9 100644 --- a/fs_image/tests/test_fs_image.py +++ b/fs_image/tests/test_fs_image.py @@ -228,7 +228,7 @@ def test_generated_sql_commands(self): # if the number of SQL queries changes into the logs. They # are to help us keep track of the number of SQL queries generated # by the module. - with self.assertQueryCount(__system__=3): + with self.assertQueryCount(__system__=4): instance = self.env["test.image.model"].create( {"fs_image": FSImageValue(name=self.filename, value=self.image_w)} )