[FIX] fs_attachment: Fix attachment store after upgrade

lmignon · lmignon · commit 777c4592c1be · 2023-11-25T17:28:32.000+01:00
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.
diff --git 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
@@ -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
@@ -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
@@ -367,7 +367,7 @@ <h1 class="title">Base Attachment Object Store</h1>
 !! This file is generated by oca-gen-addon-readme !!
 !! changes will be overwritten.                   !!
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-!! source digest: sha256:bdd0b96b994e4dca01425e6a7bd363884a2789cca0bc9fe64d07cd047ee9ff71
+!! source digest: sha256:61a58639f0f0fe76909909b616893271d9e4ff2cdd569885eae7648b0d8dfcf5
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -->
 <p><a class="reference external image-reference" href="https://odoo-community.org/page/development-status"><img alt="Beta" src="https://img.shields.io/badge/maturity-Beta-yellow.png" /></a> <a class="reference external image-reference" href="http://www.gnu.org/licenses/agpl-3.0-standalone.html"><img alt="License: AGPL-3" src="https://img.shields.io/badge/licence-AGPL--3-blue.png" /></a> <a class="reference external image-reference" href="https://github.com/OCA/storage/tree/16.0/fs_attachment"><img alt="OCA/storage" src="https://img.shields.io/badge/github-OCA%2Fstorage-lightgray.png?logo=github" /></a> <a class="reference external image-reference" href="https://translation.odoo-community.org/projects/storage-16-0/storage-16-0-fs_attachment"><img alt="Translate me on Weblate" src="https://img.shields.io/badge/weblate-Translate%20me-F47D42.png" /></a> <a class="reference external image-reference" href="https://runboat.odoo-community.org/builds?repo=OCA/storage&amp;target_branch=16.0"><img alt="Try me on Runboat" src="https://img.shields.io/badge/runboat-Try%20me-875A7B.png" /></a></p>
 <p>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
@@ -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}")
