Remove file from filestore when setting a Binary field to None [PREVIEW]

trytond/doc/ref/filestore.rst

@@ -37,6 +37,15 @@ FileStore
 
    Store the sequence of data and return a list of identifiers.
 
+.. method:: FileStore.delete(id[, prefix])
+
+   Remove the data identified by ``id`` in the prefixed directory.
+
+.. method:: FileStore.delete_many(ids[, prefix])
+
+   Remove the sequence of data identified by the sequence of ``ids`` in the
+   prefixed directory.
+
 .. note::
    The class can be overridden by setting a fully qualified name of a
    alternative class defined in the configuration ``class`` of the ``database``

trytond/trytond/filestore.py

@@ -1,14 +1,22 @@
 # This file is part of Tryton.  The COPYRIGHT file at the top level of
 # this repository contains the full copyright notices and license terms.
-import hashlib
+
 import os
+import pathlib
+import uuid
 
 from trytond.config import config
+from trytond.exceptions import UserWarning
+from trytond.i18n import gettext
 from trytond.tools import resolve
 
 __all__ = ['filestore']
 
 
+class OldFileStoreDeletion(UserWarning):
+    pass
+
+
 class FileStore(object):
 
     def get(self, id, prefix=''):
@@ -32,20 +40,9 @@ def set(self, data, prefix=''):
         filename = self._filename(id, prefix)
         dirname = os.path.dirname(filename)
         os.makedirs(dirname, mode=0o770, exist_ok=True)
-
-        collision = 0
-        while True:
-            basename = os.path.basename(filename)
-            if os.path.exists(filename):
-                if data != self.get(basename, prefix):
-                    collision += 1
-                    filename = self._filename(
-                        '%s-%s' % (id, collision), prefix)
-                    continue
-            else:
-                with open(filename, 'wb')as fp:
-                    fp.write(data)
-            return basename
+        with open(filename, 'wb')as fp:
+            fp.write(data)
+        return os.path.basename(filename)
 
     def setmany(self, data, prefix=''):
         return [self.set(d, prefix) for d in data]
@@ -59,7 +56,25 @@ def _filename(self, id, prefix):
         return filename
 
     def _id(self, data):
-        return hashlib.md5(data).hexdigest()
+        return str(uuid.uuid4())
+
+    def delete(self, id, prefix=''):
+        if '-' not in id:
+            raise OldFileStoreDeletion(
+                f'filestore_deletion_{id}',
+                gettext('ir.msg_filestore_old_deletion'))
+
+        file = pathlib.Path(self._filename(id, prefix))
+        file.unlink(missing_ok=True)
+        for parent in file.parents[:2]:
+            try:
+                parent.rmdir()
+            except OSError:
+                break
+
+    def delete_many(self, ids, prefix=''):
+        for id_ in ids:
+            self.delete(id_, prefix)
 
 
 if config.get('database', 'class'):

trytond/trytond/ir/__init__.py

@@ -3,9 +3,9 @@
 from trytond.pool import Pool
 
 from . import (
-    action, attachment, avatar, cache, calendar_, configuration, cron, date,
-    email_, error, export, lang, message, model, module, note, queue_, routes,
-    rule, sequence, session, translation, trigger, ui)
+    action, attachment, avatar, binary, cache, calendar_, configuration, cron,
+    date, email_, error, export, lang, message, model, module, note, queue_,
+    routes, rule, sequence, session, translation, trigger, ui)
 
 __all__ = ['register', 'routes']
 
@@ -88,6 +88,7 @@ def register():
         email_.EmailTemplate,
         email_.EmailTemplate_Report,
         error.Error,
+        binary.RemovedFile,
         module='ir', type_='model')
     Pool.register(
         translation.TranslationSet,

trytond/trytond/ir/binary.py

@@ -0,0 +1,44 @@
+# This file is part of Tryton.  The COPYRIGHT file at the top level of
+# this repository contains the full copyright notices and license terms.
+
+from collections import defaultdict
+from itertools import groupby
+from operator import itemgetter
+
+from trytond.filestore import filestore
+from trytond.model import ModelSQL, fields
+from trytond.pool import Pool
+from trytond.transaction import Transaction
+
+
+class RemovedFile(ModelSQL):
+    "Removed File"
+    __name__ = 'ir.removed_file'
+
+    file_id = fields.Char("File ID", required=True)
+    model = fields.Char("Model", required=True)
+    field = fields.Char("Field", required=True)
+
+    @classmethod
+    def remove(cls):
+        pool = Pool()
+
+        table = cls.__table__()
+        cursor = Transaction().connection.cursor()
+        cursor.execute(*table.select(
+                table.file_id, table.model, table.field,
+                order_by=[table.model, table.field]))
+
+        to_remove = defaultdict(list)
+        for (model, fname), deleted in groupby(cursor, key=itemgetter(1, 2)):
+            Model = pool.get(model)
+            field = Model._fields[fname]
+
+            deleted_ids = {d[0] for d in deleted}
+            active_ids = {getattr(r, field.file_id) for r in Model.search([
+                        (field.file_id, 'in', list(deleted_ids)),
+                        ])}
+            to_remove[field.store_prefix].extend(deleted_ids - active_ids)
+
+        for prefix, to_delete in to_remove.items():
+            filestore.delete_many(to_delete, prefix=prefix)

trytond/trytond/ir/binary.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+<tryton>
+    <data noupdate="1">
+        <record model="ir.cron" id="cron_removed_file_remove">
+            <field name="method">ir.removed_file|remove</field>
+            <field name="interval_number" eval="1"/>
+            <field name="interval_type">months</field>
+        </record>
+    </data>
+</tryton>

trytond/trytond/ir/message.xml

@@ -423,5 +423,8 @@ this repository contains the full copyright notices and license terms. -->
         <record model="ir.message" id="msg_field_model_name">
             <field name="text">%(field)s (model name)</field>
         </record>
+        <record model="ir.message" id="msg_filestore_old_deletion">
+            <field name="text">Deleting an old file from the filestore</field>
+        </record>
     </data>
 </tryton>

trytond/trytond/model/fields/binary.py

@@ -3,6 +3,7 @@
 from sql import Column, Null
 
 from trytond.filestore import filestore
+from trytond.pool import Pool
 from trytond.tools import cached_property, grouped_slice, reduce_ids
 from trytond.transaction import Transaction
 
@@ -106,6 +107,9 @@ def store_func(id, prefix):
         return res
 
     def set(self, Model, name, ids, value, *args):
+        pool = Pool()
+        RemovedFile = pool.get('ir.removed_file')
+
         transaction = Transaction()
         table = Model.__table__()
         cursor = transaction.connection.cursor()
@@ -114,18 +118,33 @@ def set(self, Model, name, ids, value, *args):
         if prefix is None:
             prefix = transaction.database.name
 
+        removed = []
         args = iter((ids, value) + args)
         for ids, value in zip(args, args):
             if self.file_id:
-                columns = [Column(table, self.file_id), Column(table, name)]
-                values = [
-                    filestore.set(value, prefix) if value else None, None]
+                new_file_id = filestore.set(value, prefix) if value else None
+                fileid_col = Column(table, self.file_id)
+                cursor.execute(*table.select(
+                        table.id, fileid_col,
+                        where=(reduce_ids(table.id, ids)
+                            & (fileid_col != Null)
+                            & (fileid_col != new_file_id))))
+                for record_id, file_id in cursor:
+                    removed.append(RemovedFile(
+                            file_id=file_id,
+                            model=Model.__name__,
+                            field=name))
+                columns = [fileid_col, Column(table, name)]
+                values = [new_file_id, None]
             else:
                 columns = [Column(table, name)]
                 values = [self.sql_format(value)]
             cursor.execute(*table.update(columns, values,
                     where=reduce_ids(table.id, ids)))
 
+        if removed:
+            RemovedFile.save(removed)
+
     def definition(self, model, language):
         definition = super().definition(model, language)
         definition['searchable'] = False

trytond/trytond/model/modelstorage.py

@@ -342,6 +342,14 @@ def delete(cls, records):
         if cls._must_log():
             cls.log(records, 'delete')
 
+        record_ids = [r.id for r in records]
+        for fname, field in cls._fields.items():
+            if not isinstance(field, fields.Binary):
+                continue
+            if not field.file_id:
+                continue
+            field.set(cls, fname, record_ids, None)
+
         # Increase transaction counter
         transaction.counter += 1
 

trytond/trytond/tests/field_binary.py

@@ -30,7 +30,7 @@ class BinaryRequired(ModelSQL):
 class BinaryFileStorage(ModelSQL):
     "Binary in FileStorage"
     __name__ = 'test.binary_filestorage'
-    binary = fields.Binary('Binary', file_id='binary_id')
+    binary = fields.Binary('Binary', file_id='binary_id', store_prefix='test')
     binary_id = fields.Char('Binary ID')
 
 

trytond/trytond/tests/test_field_binary.py

@@ -7,6 +7,7 @@
 from sql import Literal
 
 from trytond.config import config
+from trytond.filestore import filestore
 from trytond.model import fields
 from trytond.model.exceptions import RequiredValidationError
 from trytond.pool import Pool
@@ -250,3 +251,37 @@ def test_copy_with_filestorage_default(self):
         copy, = Binary.copy([binary], default={'binary': b'bar'})
 
         self.assertEqual(copy.binary, b'bar')
+
+    @with_transaction()
+    def test_removal(self):
+        "Test the removal method of binary fields"
+        pool = Pool()
+        RemovedFile = pool.get('ir.removed_file')
+        Binary = pool.get('test.binary_filestorage')
+
+        binary = Binary(binary=b'foo')
+        binary.save()
+        file_id = binary.binary_id
+
+        Binary.delete([binary])
+        RemovedFile.remove()
+
+        with self.assertRaises(IOError):
+            filestore.get(file_id, prefix='test')
+
+    @with_transaction()
+    def test_removal_copy(self):
+        "Test the removal method of copied binary fields"
+        pool = Pool()
+        RemovedFile = pool.get('ir.removed_file')
+        Binary = pool.get('test.binary_filestorage')
+
+        binary = Binary(binary=b'foo')
+        binary.save()
+        file_id = binary.binary_id
+        copy, = Binary.copy([binary])
+
+        Binary.delete([binary])
+        RemovedFile.remove()
+
+        self.assertTrue(filestore.get(file_id, prefix='test'))

trytond/trytond/tests/test_filestore.py

@@ -4,7 +4,6 @@
 import shutil
 import tempfile
 import unittest
-from unittest.mock import patch
 
 from trytond.config import config
 from trytond.filestore import filestore
@@ -73,24 +72,23 @@ def test_bad_prefix(self):
         with self.assertRaises(ValueError):
             filestore.set(self.data(), prefix='../')
 
-    def test_duplicate(self):
-        "Test duplicate"
+    def test_delete(self):
+        "Test deletion"
         data = self.data()
-        id = filestore.set(data, prefix='test')
-        self.assertEqual(filestore.set(data, prefix='test'), id)
-
-    def test_collision(self):
-        "Test collision"
-        data1 = self.data()
-        data2 = self.data()
-
-        id1 = filestore.set(data1, prefix='test')
+        id_ = filestore.set(data, prefix='test')
+        filestore.delete(id_, prefix='test')
+        with self.assertRaises(IOError):
+            filestore.get(id_, prefix='test')
 
-        with patch.object(filestore, '_id') as _id:
-            _id.return_value = id1
+    def test_delete_many(self):
+        "Test multiple deletions"
+        id1, id2, id3 = filestore.setmany(
+            [self.data(), self.data(), self.data()],
+            prefix='test')
+        filestore.delete_many([id1, id2], prefix='test')
 
-            id2 = filestore.set(data2, prefix='test')
+        for id_ in [id1, id2]:
+            with self.assertRaises(IOError):
+                filestore.get(id_, prefix='test')
 
-        self.assertNotEqual(id1, id2)
-        self.assertEqual(filestore.get(id1, prefix='test'), data1)
-        self.assertEqual(filestore.get(id2, prefix='test'), data2)
+        self.assertTrue(filestore.get(id3, prefix='test'))