[16.0][FIX] fs_attachment: Fix attachment store after upgrade

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

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):

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.

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

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}")