From 1c8a95809852baefa851985507a07ac7f6465dc7 Mon Sep 17 00:00:00 2001 From: Adrian Torres Date: Wed, 26 Aug 2020 10:20:26 +0000 Subject: [PATCH] [IMP] core: introduce `api.ondelete` decorator With this commit, a new ORM api decorator is introduced: `api.ondelete(*, at_uninstall)`. This decorator is to be applied to Model methods that check for specific business conditions when attempting to unlink a record via the interface. E.g. trying to unlink a validated journal entry This decorator allows this logic to exist outside the `BaseModel.unlink` method and is automatically bypassed when in uninstall mode, this means that during an uninstall any and all data related to a module can and will be removed easily and cleanly while still being able to apply business logic to manual deletion of records. This feature opens the gates to solving a very big problem with uninstalls: records and tables that remain in a database despite the relevant module being uninstalled, because if an override to unlink raises an error, the data will never be deleted from the database. Henceforth, overrides of unlink shall solely be used for data-cleaning purposes, i.e. deletion or modification of data that is related to the one currently being deleted but cannot be automatically deleted because there are no proper SQL relations. In certain very specific, low-level scenarios an unlink may be overridden to raise an error, but this should only be done if you know what the fuck you're doing, most of the time you'll want to resort to `@api.ondelete`. Note that this new decorator includes a keyword-only, required argument called `at_uninstall`, in most business cases this argument shall be False as this argument dictates whether or not this method should be executed in the `unlink` call during uninstall. It should only be set to True if you are certain of all the implications which most likely means that the records of the model in which this ondelete function is defined will NOT be removed during uninstall, they will forever linger in the DB until manual intervention, this in turn can mean a wide range of undefined problems due to crap left on the database. Following commits will replace any `unlink` overrides that raise business errors by methods decorated with `api.ondelete`, another commit will introduce a pylint checker that will raise a warning any time that an unlink override raises an error. --- .../test_new_api/models/test_new_api.py | 20 +++++++ .../test_new_api/security/ir.model.access.csv | 1 + .../test_new_api/tests/test_new_fields.py | 39 ++++++++++++ odoo/api.py | 60 +++++++++++++++++++ odoo/models.py | 19 ++++++ 5 files changed, 139 insertions(+) diff --git a/odoo/addons/test_new_api/models/test_new_api.py b/odoo/addons/test_new_api/models/test_new_api.py index 3041daeadc0..70bbe1ecd77 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -1179,3 +1179,23 @@ class ComputeEditableLine(models.Model): def _compute_edit(self): for line in self: line.edit = line.value + + +class ConstrainedUnlinks(models.Model): + _name = 'test_new_api.model_constrained_unlinks' + _description = 'Model with unlink override that is constrained' + + foo = fields.Char() + bar = fields.Integer() + + @api.ondelete(at_uninstall=False) + def _unlink_except_bar_gt_five(self): + for rec in self: + if rec.bar and rec.bar > 5: + raise ValueError("Nooooooooo bar can't be greater than five!!") + + @api.ondelete(at_uninstall=True) + def _unlink_except_prosciutto(self): + for rec in self: + if rec.foo and rec.foo == 'prosciutto': + raise ValueError("You didn't say if you wanted it crudo or cotto...") diff --git a/odoo/addons/test_new_api/security/ir.model.access.csv b/odoo/addons/test_new_api/security/ir.model.access.csv index 021d7127b05..7b313690904 100644 --- a/odoo/addons/test_new_api/security/ir.model.access.csv +++ b/odoo/addons/test_new_api/security/ir.model.access.csv @@ -68,3 +68,4 @@ access_test_new_api_compute_container,access_test_new_api_compute_container,mode access_test_new_api_compute_member,access_test_new_api_compute_member,model_test_new_api_compute_member,,1,1,1,1 access_test_new_api_compute_editable,access_test_new_api_compute_editable,model_test_new_api_compute_editable,,1,1,1,1 access_test_new_api_compute_editable_line,access_test_new_api_compute_editable_line,model_test_new_api_compute_editable_line,,1,1,1,1 +access_test_new_api_model_constrained_unlinks,access_test_new_api_model_constrained_unlinks,model_test_new_api_model_constrained_unlinks,,1,1,1,1 diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index 76be6c6f9b5..d980c7a020a 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -3109,3 +3109,42 @@ class test_shared_cache(TransactionCaseWithUserDemo): line.amount = 2 # The new value for total_amount, should be 3, not 2. self.assertEqual(task_form.total_amount, 2) + + +@common.tagged('unlink_constraints') +class TestUnlinkConstraints(common.TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + MODEL = cls.env['test_new_api.model_constrained_unlinks'] + + cls.deletable_bar = MODEL.create({'bar': 5}) + cls.undeletable_bar = MODEL.create({'bar': 6}) + cls.deletable_foo = MODEL.create({'foo': 'formaggio'}) + cls.undeletable_foo = MODEL.create({'foo': 'prosciutto'}) + + from odoo.addons.base.models.ir_model import MODULE_UNINSTALL_FLAG + uninstall = {MODULE_UNINSTALL_FLAG: True} + cls.undeletable_bar_uninstall = cls.undeletable_bar.with_context(**uninstall) + cls.undeletable_foo_uninstall = cls.undeletable_foo.with_context(**uninstall) + + def test_unlink_constraint_manual_bar(self): + self.assertTrue(self.deletable_bar.unlink()) + with self.assertRaises(ValueError, msg="Nooooooooo bar can't be greater than five!!"): + self.undeletable_bar.unlink() + + def test_unlink_constraint_uninstall_bar(self): + self.assertTrue(self.deletable_bar.unlink()) + # should succeed since it's at_uninstall=False + self.assertTrue(self.undeletable_bar_uninstall.unlink()) + + def test_unlink_constraint_manual_foo(self): + self.assertTrue(self.deletable_foo.unlink()) + with self.assertRaises(ValueError, msg="You didn't say if you wanted it crudo or cotto..."): + self.undeletable_foo.unlink() + + def test_unlink_constraint_uninstall_foo(self): + self.assertTrue(self.deletable_foo) + # should fail since it's at_uninstall=True + with self.assertRaises(ValueError, msg="You didn't say if you wanted it crudo or cotto..."): + self.undeletable_foo_uninstall.unlink() diff --git a/odoo/api.py b/odoo/api.py index fd3caf9eb61..c7087add14f 100644 --- a/odoo/api.py +++ b/odoo/api.py @@ -37,6 +37,7 @@ _logger = logging.getLogger(__name__) # - method._returns: set by @returns, specifies return model # - method._onchange: set by @onchange, specifies onchange fields # - method.clear_cache: set by @ormcache, used to clear the cache +# - method._ondelete: set by @ondelete, used to raise errors for unlink operations # # On wrapping method only: # - method._api: decorator function, used for re-applying decorator @@ -130,6 +131,65 @@ def constrains(*args): return attrsetter('_constrains', args) +def ondelete(*, at_uninstall): + """ + Mark a method to be executed during :meth:`~odoo.models.BaseModel.unlink`. + + The goal of this decorator is to allow client-side errors when unlinking + records if, from a business point of view, it does not make sense to delete + such records. For instance, a user should not be able to delete a validated + sales order. + + While this could be implemented by simply overriding the method ``unlink`` + on the model, it has the drawback of not being compatible with module + uninstallation. When uninstalling the module, the override could raise user + errors, but we shouldn't care because the module is being uninstalled, and + thus **all** records related to the module should be removed anyway. + + This means that by overriding ``unlink``, there is a big chance that some + tables/records may remain as leftover data from the uninstalled module. This + leaves the database in an inconsistent state. Moreover, there is a risk of + conflicts if the module is ever reinstalled on that database. + + Methods decorated with ``@ondelete`` should raise an error following some + conditions, and by convention, the method should be named either + ``_unlink_if_`` or ``_unlink_except_``. + + .. code-block:: python + + @api.ondelete(at_uninstall=False) + def _unlink_if_user_inactive(self): + if any(user.active for user in self): + raise UserError("Can't delete an active user!") + + # same as above but with _unlink_except_* as method name + @api.ondelete(at_uninstall=False) + def _unlink_except_active_user(self): + if any(user.active for user in self): + raise UserError("Can't delete an active user!") + + :param bool at_uninstall: Whether the decorated method should be called if + the module that implements said method is being uninstalled. Should + almost always be ``False``, so that module uninstallation does not + trigger those errors. + + .. danger:: + The parameter ``at_uninstall`` should only be set to ``True`` if the + check you are implementing also applies when uninstalling the module. + + For instance, it doesn't matter if when uninstalling ``sale``, validated + sales orders are being deleted because all data pertaining to ``sale`` + should be deleted anyway, in that case ``at_uninstall`` should be set to + ``False``. + + However, it makes sense to prevent the removal of the default language + if no other languages are installed, since deleting the default language + will break a lot of basic behavior. In this case, ``at_uninstall`` + should be set to ``True``. + """ + return attrsetter('_ondelete', at_uninstall) + + def onchange(*args): """Return a decorator to decorate an onchange method for given fields. diff --git a/odoo/models.py b/odoo/models.py index bfe6f3d09cd..84e4b16b399 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -672,6 +672,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): # reset properties memoized on cls cls._constraint_methods = BaseModel._constraint_methods + cls._ondelete_methods = BaseModel._ondelete_methods cls._onchange_methods = BaseModel._onchange_methods @property @@ -695,6 +696,18 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): cls._constraint_methods = methods return methods + @property + def _ondelete_methods(self): + """ Return a list of methods implementing checks before unlinking. """ + def is_ondelete(func): + return callable(func) and hasattr(func, '_ondelete') + + cls = type(self) + methods = [func for _, func in getmembers(cls, is_ondelete)] + # optimization: memoize results on cls, it will not be recomputed + cls._ondelete_methods = methods + return methods + @property def _onchange_methods(self): """ Return a dictionary mapping field names to onchange methods. """ @@ -3413,6 +3426,12 @@ Fields: self.check_access_rights('unlink') self._check_concurrency() + from odoo.addons.base.models.ir_model import MODULE_UNINSTALL_FLAG + for func in self._ondelete_methods: + # func._ondelete is True if it should be called during uninstallation + if func._ondelete or not self._context.get(MODULE_UNINSTALL_FLAG): + func(self) + # mark fields that depend on 'self' to recompute them after 'self' has # been deleted (like updating a sum of lines after deleting one line) self.flush()