From 777c4592c1be3b46983d8c04f27560d0154f5829 Mon Sep 17 00:00:00 2001 From: "Laurent Mignon (ACSONE)" Date: Sat, 25 Nov 2023 16:33:16 +0100 Subject: [PATCH] [FIX] fs_attachment: Fix attachment store after upgrade Prerequisites: * The fs_attachment addon is installed, and specific filesystem storage is configured. * All the attachments are already stored in the appropriate filesystem storage. (We assume that if you've installed the addon on an existing database, you'vealready migrated the existing attachments appropriately. Context: * You run an upgrade command on your Odoo server. Problem: Some attachments could be created during the upgrade process before the load of the fs_attachment module or the configuration of the filesystem storage backends. This is especially true for the attachments created by addons to store the app icons or even attachments from XML data files. As a result, you end up with attachments.that could be found by some Odoo instances if, for example, you have multiple Odoo instances running on different servers since these attachments are stored in the odoo filestore of the server that created them. Solution: A solution to this problem is to hook the end of the upgrade process through the implementation of the *_register_hook* method on our *ir.attachment* model. Since This method is called after the completion of the upgrade process and the full initialization of the odoo registry, we can safely expect that all the information We need to know where to store the attachments that are available. From there, we can simply search for all the attachments that are not linked to any specific storage and store them in the appropriate storage. --- fs_attachment/README.rst | 2 +- fs_attachment/models/ir_attachment.py | 175 ++++++++++-------- fs_attachment/readme/newsfragments/304.bugfix | 33 ++++ fs_attachment/static/description/index.html | 2 +- fs_attachment/tests/test_fs_attachment.py | 8 +- 5 files changed, 142 insertions(+), 78 deletions(-) create mode 100644 fs_attachment/readme/newsfragments/304.bugfix diff --git a/fs_attachment/README.rst b/fs_attachment/README.rst index 649b73cff1..b70dd547ff 100644 --- a/fs_attachment/README.rst +++ b/fs_attachment/README.rst @@ -7,7 +7,7 @@ Base Attachment Object Store !! This file is generated by oca-gen-addon-readme !! !! changes will be overwritten. !! !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! - !! source digest: sha256:bdd0b96b994e4dca01425e6a7bd363884a2789cca0bc9fe64d07cd047ee9ff71 + !! source digest: sha256:61a58639f0f0fe76909909b616893271d9e4ff2cdd569885eae7648b0d8dfcf5 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! .. |badge1| image:: https://img.shields.io/badge/maturity-Beta-yellow.png diff --git a/fs_attachment/models/ir_attachment.py b/fs_attachment/models/ir_attachment.py index 8b34a6b9c3..5ad2554e1c 100644 --- a/fs_attachment/models/ir_attachment.py +++ b/fs_attachment/models/ir_attachment.py @@ -4,6 +4,7 @@ from __future__ import annotations +import inspect import io import logging import mimetypes @@ -13,7 +14,6 @@ from contextlib import closing, contextmanager import fsspec # pylint: disable=missing-manifest-dependency -import psycopg2 from slugify import slugify # pylint: disable=missing-manifest-dependency import odoo @@ -37,23 +37,6 @@ def is_true(strval): return bool(strtobool(strval or "0")) -def clean_fs(files): - _logger.info("cleaning old files from filestore") - for full_path in files: - if os.path.exists(full_path): - try: - os.unlink(full_path) - except OSError: - _logger.info( - "_file_delete could not unlink %s", full_path, exc_info=True - ) - except IOError: - # Harmless and needed for race conditions - _logger.info( - "_file_delete could not unlink %s", full_path, exc_info=True - ) - - class IrAttachment(models.Model): _inherit = "ir.attachment" @@ -96,6 +79,46 @@ class IrAttachment(models.Model): ondelete="restrict", ) + def init(self): + res = super().init() + # This partial index is used by the register hook to find attachments + # to migrate from the odoo filestore to a fs storage + query = """ + CREATE INDEX IF NOT EXISTS + ir_attachment_no_fs_storage_code + ON ir_attachment (fs_storage_code) + WHERE fs_storage_code IS NULL; + """ + self.env.cr.execute(query) + return res + + def _register_hook(self): + super()._register_hook() + fs_storage_codes = self._get_storage_codes() + # ignore if we are not using an object storages + if not fs_storage_codes: + return + curframe = inspect.currentframe() + calframe = inspect.getouterframes(curframe, 2) + # the caller of _register_hook is 'load_modules' in + # odoo/modules/loading.py + load_modules_frame = calframe[1][0] + # 'update_module' is an argument that 'load_modules' receives with a + # True-ish value meaning that an install or upgrade of addon has been + # done during the initialization. We need to move the attachments that + # could have been created or updated in other addons before this addon + # was loaded + update_module = load_modules_frame.f_locals.get("update_module") + + # We need to call the migration on the loading of the model because + # when we are upgrading addons, some of them might add attachments. + # To be sure they are migrated to the storage we need to call the + # migration here. + # Typical example is images of ir.ui.menu which are updated in + # ir.attachment at every upgrade of the addons + if update_module: + self.sudo()._force_storage_to_object_storage() + @api.depends("name") def _compute_internal_url(self) -> None: for rec in self: @@ -673,7 +696,7 @@ def _move_attachment_to_store(self): } ) _logger.info("moved %s on the object storage", fname) - return self._full_path(fname) + self._mark_for_gc(fname) elif self.db_datas: _logger.info("moving on the object storage from database") self.write({"datas": self.datas}) @@ -760,67 +783,73 @@ def force_storage_to_db_for_special_fields(self, new_cr=False): ) @api.model - def _force_storage_to_object_storage(self, new_cr=False): + def _force_storage_to_object_storage(self): _logger.info("migrating files to the object storage") - storage = self.env.context.get("storage_location") or self._storage() - if self._is_storage_disabled(storage): + is_disabled = is_true(os.environ.get("DISABLE_ATTACHMENT_STORAGE")) + if is_disabled: 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", "{}://%".format(storage)), + all_storages = self.env["fs.storage"].search([]) + self._force_storage_for_specific_fields(all_storages) + self._force_storage_for_specific_models(all_storages) + self._force_storage_for_attachments(all_storages) + + @property + def _default_domain_for_force_storage(self): + return [ + ("fs_storage_code", "=", False), + ("store_fname", "!=", False), "|", ("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"] - ids = model_env.search(domain).ids - files_to_clean = [] - for attachment_id in ids: - try: - with new_env.cr.savepoint(): - # check that no other transaction has - # locked the row, don't send a file to storage - # in that case - self.env.cr.execute( - "SELECT id " - "FROM ir_attachment " - "WHERE id = %s " - "FOR UPDATE NOWAIT", - (attachment_id,), - log_exceptions=False, - ) - - # This is a trick to avoid having the 'datas' - # function fields computed for every attachment on - # each iteration of the loop. The former issue - # being that it reads the content of the file of - # ALL the attachments on each loop. - new_env.clear() - attachment = model_env.browse(attachment_id) - path = attachment._move_attachment_to_store() - if path: - files_to_clean.append(path) - except psycopg2.OperationalError: - _logger.error( - "Could not migrate attachment %s to S3", attachment_id - ) - # delete the files from the filesystem once we know the changes - # have been committed in ir.attachment - if files_to_clean: - new_env.cr.commit() - clean_fs(files_to_clean) + @api.model + def _force_storage_for_specific_fields(self, fs_storages): + """Migrate attachments linked to model's fields for which a fs storage + is configured and no fs_storage_code is set on the attachment + """ + domain = self._default_domain_for_force_storage + fields = fs_storages.mapped("field_ids") + fields_domain = [] + for field in fields: + fields_domain.append( + [("res_field", "=", field.name), ("res_model", "=", field.model_name)] + ) + domain = AND([domain, OR(fields_domain)]) + for attachment in self.search(domain): + attachment._move_attachment_to_store() + + @api.model + def _force_storage_for_specific_models(self, fs_storages): + """Migrate attachments linked to models for which a fs storage + is configured and no fs_storage_code is set on the attachment. + This method MUST be called after _force_storage_for_specific_fields + to be sure that all the attachments linked to fields are migrated + before migrating the attachments linked to models otherwise we + will move some attachment with specific fs storage defined for + its field to the default fs storage defined for its model. + """ + domain = self._default_domain_for_force_storage + model_names = fs_storages.mapped("model_ids.model") + domain = AND([domain, [("res_model", "in", model_names)]]) + for attachment in self.search(domain): + attachment._move_attachment_to_store() + + @api.model + def _force_storage_for_attachments(self, fs_storages): + """Migrate attachments not stored into a filesystem storage if a + filesystem storage is configured for attachments. + + This method MUST be called after _force_storage_for_specific_fields + and _force_storage_for_specific_models + + """ + if not self.env["fs.storage"].get_default_storage_code_for_attachments(): + # no default storage configured for attachments + return + domain = self._default_domain_for_force_storage + for attachment in self.search(domain): + attachment._move_attachment_to_store() class AttachmentFileLikeAdapter(object): diff --git a/fs_attachment/readme/newsfragments/304.bugfix b/fs_attachment/readme/newsfragments/304.bugfix new file mode 100644 index 0000000000..c6820e1c57 --- /dev/null +++ b/fs_attachment/readme/newsfragments/304.bugfix @@ -0,0 +1,33 @@ +Fix attachments stored in Odoo after an addon upgrade. + +Prerequisites: + +* The fs_attachment addon is installed, and specific filesystem storage is + configured. +* All the attachments are already stored in the appropriate filesystem storage. + (We assume that if you've installed the addon on an existing database, + you'vealready migrated the existing attachments appropriately. + +Context: + +* You run an upgrade command on your Odoo server. + +Problem: + +Some attachments could be created during the upgrade process before the load of +the fs_attachment module or the configuration of the filesystem storage backends. +This is especially true for the attachments created by addons to store the app +icons or even attachments from XML data files. As a result, you end up with +attachments that could not be found by some Odoo instances if, for example, you have +multiple Odoo instances running on different servers since these attachments are stored +in the odoo filestore of the server that created them. + +Solution: + +A solution to this problem is to hook the end of the upgrade process through the +implementation of the *_register_hook* method on our *ir.attachment* model. Since +this method is called after the completion of the upgrade process and the full +initialization of the odoo registry, we can safely expect that all the information +we need to know where to store the attachments are available. +From there, we can simply search for all the attachments that are not linked to +any specific storage and store them in the appropriate storage. diff --git a/fs_attachment/static/description/index.html b/fs_attachment/static/description/index.html index b066e5d503..4fe61ed967 100644 --- a/fs_attachment/static/description/index.html +++ b/fs_attachment/static/description/index.html @@ -367,7 +367,7 @@

Base Attachment Object Store

!! This file is generated by oca-gen-addon-readme !! !! changes will be overwritten. !! !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -!! source digest: sha256:bdd0b96b994e4dca01425e6a7bd363884a2789cca0bc9fe64d07cd047ee9ff71 +!! source digest: sha256:61a58639f0f0fe76909909b616893271d9e4ff2cdd569885eae7648b0d8dfcf5 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -->

Beta License: AGPL-3 OCA/storage Translate me on Weblate Try me on Runboat

In some cases, you need to store attachment in another system that the Odoo’s diff --git a/fs_attachment/tests/test_fs_attachment.py b/fs_attachment/tests/test_fs_attachment.py index 7ba0728b9e..cc99ef7dff 100644 --- a/fs_attachment/tests/test_fs_attachment.py +++ b/fs_attachment/tests/test_fs_attachment.py @@ -317,11 +317,13 @@ def test_force_storage_to_fs(self): self.assertEqual(os.listdir(self.temp_dir), []) # we decide to force the storage in the filestore self.temp_backend.use_as_default_for_attachments = True - with mock.patch.object(self.env.cr, "commit"), mock.patch( - "odoo.addons.fs_attachment.models.ir_attachment.clean_fs" + with mock.patch.object(self.env.cr, "commit"), mock.patch.object( + type(self.ir_attachment_model), "_mark_for_gc" ) as clean_fs: + fname = attachment.store_fname self.ir_attachment_model.force_storage() - clean_fs.assert_called_once() + clean_fs.assert_any_call(fname) + # files into the filestore must be moved to our filesystem storage filename = f"test-{attachment.id}-0.txt" self.assertEqual(attachment.store_fname, f"tmp_dir://{filename}")