From 896aec5976e1db3a013bd1115b47610b89b30302 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 15:59:16 +0200 Subject: [PATCH] Ensure that migration of files is commited before deleting files When moving attachments from the filestore to an object storage, the filesystem files will be deleted only after the commit, so if the transaction is rollbacked, we still have the local files for another try. --- .../models/ir_attachment.py | 51 ++++++++++++------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/base_attachment_object_storage/models/ir_attachment.py b/base_attachment_object_storage/models/ir_attachment.py index 9063cc6..22899b5 100644 --- a/base_attachment_object_storage/models/ir_attachment.py +++ b/base_attachment_object_storage/models/ir_attachment.py @@ -15,6 +15,25 @@ from odoo import api, exceptions, models, _ _logger = logging.getLogger(__name__) +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' @@ -177,22 +196,7 @@ class IrAttachment(models.Model): # on assets 'mimetype': self.mimetype}) _logger.info('moved %s on the object storage', fname) - full_path = self._full_path(fname) - _logger.info('cleaning fs self') - 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 - ) + return self._full_path(fname) elif self.db_datas: _logger.info('moving on the object storage from database') self.write({'datas': self.datas}) @@ -222,11 +226,12 @@ class IrAttachment(models.Model): 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 S3 + # locked the row, don't send a file to storage # in that case self.env.cr.execute("SELECT id " "FROM ir_attachment " @@ -242,11 +247,21 @@ class IrAttachment(models.Model): # ALL the attachments on each loop. new_env.clear() attachment = model_env.browse(attachment_id) - attachment._move_attachment_to_store() + 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) + def clean(): + clean_fs(files_to_clean) + + # delete the files from the filesystem once we know the changes + # have been committed in ir.attachment + if files_to_clean: + new_env.cr.after('commit', clean) + def _get_stores(self): """ To get the list of stores activated in the system """ return []