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