Skip to content

[IMP] fs_attachment: simplify domain for attachment search by skipping res_field checks #585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CRogos wants to merge 1 commit into
base: 18.0
Choose a base branch
from
Open

[IMP] fs_attachment: simplify domain for attachment search by skipping res_field checks #585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions fs_attachment/models/fs_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,19 +310,14 @@ def recompute_urls(self) -> None:
in staging are done in a different directory and will not impact the
production.
"""
# The weird "res_field = False OR res_field != False" domain
# is required! It's because of an override of _search in ir.attachment
# which adds ('res_field', '=', False) when the domain does not
# contain 'res_field'.
# https://github.com/odoo/odoo/blob/9032617120138848c63b3cfa5d1913c5e5ad76db/
# odoo/addons/base/ir/ir_attachment.py#L344-L347
domain = [
("fs_storage_id", "in", self.ids),
"|",
("res_field", "=", False),
("res_field", "!=", False),
]
attachments = self.env["ir.attachment"].search(domain)
attachments = (
self.env["ir.attachment"]
.with_context(skip_res_field_check=True)
.search(domain)
)
Comment on lines 313 to +320
Copy link
Copy Markdown
Contributor Author

@CRogos CRogos Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be covered by this test:

https://github.com/oca/storage/blob/18.0/fs_attachment/tests/test_fs_storage.py#L245-L279

attachments._compute_fs_url()
attachments._compute_fs_url_path()

Expand Down
20 changes: 4 additions & 16 deletions fs_attachment/models/ir_attachment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,19 +767,16 @@ def force_storage_to_db_for_special_fields(
normalize_domain(
[
("store_fname", "=like", f"{storage}://%"),
# for res_field, see comment in
# _force_storage_to_object_storage
"|",
("res_field", "=", False),
("res_field", "!=", False),
]
),
normalize_domain(self._store_in_db_instead_of_object_storage_domain()),
)
)

with self._do_in_new_env(new_cr=new_cr) as new_env:
model_env = new_env["ir.attachment"].with_context(prefetch_fields=False)
model_env = new_env["ir.attachment"].with_context(
prefetch_fields=False, skip_res_field_check=True
)
attachment_ids = model_env.search(domain).ids
if not attachment_ids:
return
Expand Down Expand Up @@ -819,25 +816,16 @@ def _force_storage_to_object_storage(self, new_cr=False):
storage = self.env.context.get("storage_location") or self._storage()
if self._is_storage_disabled(storage):
return
# The weird "res_field = False OR res_field != False" domain
# is required! It's because of an override of _search in ir.attachment
# which adds ('res_field', '=', False) when the domain does not
# contain 'res_field'.
# https://github.com/odoo/odoo/blob/9032617120138848c63b3cfa5d1913c5e5ad76db/
# odoo/addons/base/ir/ir_attachment.py#L344-L347
domain = [
"!",
("store_fname", "=like", f"{storage}://%"),
"|",
("res_field", "=", False),
("res_field", "!=", False),
]
# We do a copy of the environment so we can workaround the cache issue
# below. We do not create a new cursor by default because it causes
# serialization issues due to concurrent updates on attachments during
# the installation
with self._do_in_new_env(new_cr=new_cr) as new_env:
model_env = new_env["ir.attachment"]
model_env = new_env["ir.attachment"].with_context(skip_res_field_check=True)
ids = model_env.search(domain).ids
files_to_clean = []
for attachment_id in ids:
Expand Down
25 changes: 25 additions & 0 deletions fs_attachment/tests/test_fs_attachment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,24 +337,44 @@ def test_force_storage_to_db(self):
attachment = self.ir_attachment_model.create(
{"name": "test.txt", "raw": b"content"}
)
attachment_res_field = self.ir_attachment_model.create(
{
"name": "field.txt",
"raw": "Attachment linked to a field",
"res_model": "res.partner",
"res_field": "name",
}
)
self.env.flush_all()
self.assertTrue(attachment.store_fname)
self.assertFalse(attachment.db_datas)
store_fname = attachment.store_fname
store_fname_res_field = attachment_res_field.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_res_field.store_fname)
self.assertEqual(attachment_res_field.db_datas, b"Attachment linked to a field")
# 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)
self.assertIn(store_fname_res_field, gc_files)

@mute_logger("odoo.addons.fs_attachment.models.ir_attachment")
def test_force_storage_to_fs(self):
attachment = self.ir_attachment_model.create(
{"name": "test.txt", "raw": b"content"}
)
attachment_res_field = self.ir_attachment_model.create(
{
"name": "field.txt",
"raw": "Attachment linked to a field",
"res_model": "res.partner",
"res_field": "name",
}
)
self.env.flush_all()
fs_path = self.ir_attachment_model._filestore() + "/" + attachment.store_fname
self.assertTrue(os.path.exists(fs_path))
Expand All @@ -371,8 +391,13 @@ def test_force_storage_to_fs(self):
clean_fs.assert_called_once()
# files into the filestore must be moved to our filesystem storage
filename = f"test-{attachment.id}-0.txt"
filename_res_field = f"field-{attachment_res_field.id}-0.txt"
self.assertEqual(attachment.store_fname, f"tmp_dir://{filename}")
self.assertEqual(
attachment_res_field.store_fname, f"tmp_dir://{filename_res_field}"
)
self.assertIn(filename, os.listdir(self.temp_dir))
self.assertIn(filename_res_field, os.listdir(self.temp_dir))

def test_storage_use_filename_obfuscation(self):
self.temp_backend.base_url = "https://acsone.eu/media"
Expand Down
Loading