[ADD] no-raise-unlink: Check if there is raise sentence in unlink met…

…hod (#458)

Raising errors inside unlink(), especially when inherited can leave
stale data behind, therefore they are prohibited. This lint checks
for that.

Closes OCA/odoo-pre-commit-hooks#73.
Context: odoo/odoo@1c8a958

README.md

@@ -43,6 +43,7 @@ method-required-super | Missing `super` call in "%s" method. | W8106
 method-search | Name of search method should start with "_search_" | C8109
 missing-readme | Missing ./README.rst file. Template here: %s | C8112
 missing-return | Missing `return` (`super` is used) in method %s. | W8110
+no-raise-unlink | No exceptions should be raised inside unlink() functions | E8140
 no-wizard-in-models | No wizard class for model directory. See the complete structure https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#complete-structure | C8113
 no-write-in-compute | Compute method calling `write`. Use `update` instead. | E8135
 odoo-addons-relative-import | Same Odoo module absolute import. You should use relative import with "." instead of "odoo.addons.%s" | W8150
@@ -247,6 +248,11 @@ Checks valid only for odoo <= 13.0
 
     - https://github.com/OCA/pylint-odoo/blob/v8.0.19/testing/resources/test_repo/broken_module/pylint_oca_broken.py#L24 Missing `return` (`super` is used) in method inherited_method.
 
+ * no-raise-unlink
+
+    - https://github.com/OCA/pylint-odoo/blob/v8.0.19/testing/resources/test_repo/test_module/res_partner_unlink.py#L9 No exceptions should be raised inside unlink() functions
+    - https://github.com/OCA/pylint-odoo/blob/v8.0.19/testing/resources/test_repo/test_module/sale_order_unlink.py#L14 No exceptions should be raised inside unlink() functions
+
  * no-wizard-in-models
 
     - https://github.com/OCA/pylint-odoo/blob/v8.0.19/testing/resources/test_repo/broken_module/models/broken_model.py#L812 No wizard class for model directory. See the complete structure https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#complete-structure

src/pylint_odoo/checkers/odoo_addons.py

@@ -105,7 +105,7 @@
 from collections import Counter, defaultdict
 
 import validators
-from astroid import nodes
+from astroid import ClassDef, FunctionDef, NodeNG, nodes
 from pylint.checkers import BaseChecker, utils
 
 from .. import misc
@@ -177,6 +177,11 @@
     ),
     "E8130": ("Test folder imported in module %s", "test-folder-imported", CHECK_DESCRIPTION),
     "E8135": ("Compute method calling `write`. Use `update` instead.", "no-write-in-compute", CHECK_DESCRIPTION),
+    "E8140": (
+        "No exceptions should be raised inside unlink() functions",
+        "no-raise-unlink",
+        "Use @api.ondelete to add any constraints instead",
+    ),
     "F8101": ('File "%s": "%s" not found.', "resource-not-exist", CHECK_DESCRIPTION),
     "R8101": (
         "`odoo.exceptions.Warning` is a deprecated alias to `odoo.exceptions.UserError` "
@@ -519,6 +524,7 @@ class OdooAddons(OdooBaseChecker, BaseChecker):
         "translation-contains-variable": {
             "odoo_maxversion": "13.0",
         },
+        "no-raise-unlink": {"odoo_minversion": "15.0"},
     }
 
     def close(self):
@@ -1183,16 +1189,33 @@ def get_func_lib(self, node):
             return node.expr.name
         return ""
 
-    @utils.only_required_for_messages("translation-required")
-    def visit_raise(self, node):
-        """Visit raise and search methods with a string parameter
-        without a method.
-        Example wrong: raise UserError('My String')
-        Example done: raise UserError(_('My String'))
-        TODO: Consider the case where is used a variable with string value
-              my_string = 'My String'  # wrong
-              raise UserError(my_string)  # Detect variable string here
+    @staticmethod
+    def _is_unlink(node: FunctionDef) -> bool:
+        parent = getattr(node, "parent", False)
+        return (
+            isinstance(parent, ClassDef)
+            and ("_name" in parent.locals or "_inherit" in parent.locals)
+            and node.name == "unlink"
+        )
+
+    @staticmethod
+    def get_enclosing_function(node: NodeNG, depth=10):
+        parent = getattr(node, "parent", False)
+        for _i in range(depth):
+            if not parent or isinstance(parent, FunctionDef):
+                break
+            parent = parent.parent
+
+        return parent
+
+    def check_translation_required(self, node):
+        """Search methods with an untranslated string parameter.
+        Wrong:  ``raise UserError('My String')``
+        Correct: ``raise UserError(_('My String'))``
         """
+        # TODO: Consider the case where a string variable is used
+        # my_string = 'My String'  # wrong
+        # raise UserError(my_string)  # Detect variable string here
         if node.exc is None:
             # ignore empty raise
             return
@@ -1217,6 +1240,16 @@ def visit_raise(self, node):
         ):
             self.add_message("translation-required", node=node, args=(func_name, "", argument.as_string()))
 
+    @utils.only_required_for_messages("translation-required", "no-raise-unlink")
+    def visit_raise(self, node):
+        if self.linter.is_message_enabled("no-raise-unlink"):
+            function = self.get_enclosing_function(node)
+            if self._is_unlink(function):
+                self.add_message("no-raise-unlink", node=node)
+
+        if self.linter.is_message_enabled("translation-required"):
+            self.check_translation_required(node)
+
     def get_cursor_name(self, node):
         expr_list = []
         node_expr = node.expr

testing/resources/test_repo/test_module/res_partner_unlink.py

@@ -0,0 +1,11 @@
+from odoo import models
+
+
+class ResPartner(models.Model):
+    _inherit = 'res.partner'
+
+    def unlink(self):
+        if self.name == 'explode':
+            raise RuntimeError()
+
+        return super().unlink()

testing/resources/test_repo/test_module/sale_order_unlink.py

@@ -0,0 +1,16 @@
+import platform
+from odoo.models import Model
+
+if platform.system() == 'Windows':
+    raise OSError
+
+
+class SaleOrder(Model):
+    _name = 'sale.order'
+
+    def unlink(self):
+        if self.name == 'maybe':
+            if self.status == 'explosive':
+                raise Exception()
+
+        return super().unlink()

tests/test_main.py

@@ -63,6 +63,7 @@
     "translation-unsupported-format": 1,
     "use-vim-comment": 1,
     "website-manifest-key-not-valid-uri": 1,
+    "no-raise-unlink": 2,
 }
 
 
@@ -75,11 +76,11 @@ def setUp(self):
             "--msg-template={path}:{line} {msg} - [{symbol}]",
             "--rcfile=%s" % os.devnull,
         ]
-        path_modules = os.path.join(
+        self.root_path_modules = os.path.join(
             os.path.dirname(os.path.dirname(os.path.realpath(__file__))), "testing", "resources", "test_repo"
         )
         # Similar to pre-commit way
-        self.paths_modules = glob(os.path.join(path_modules, "**", "*.py"), recursive=True)
+        self.paths_modules = glob(os.path.join(self.root_path_modules, "**", "*.py"), recursive=True)
         self.odoo_namespace_addons_path = os.path.join(
             os.path.dirname(os.path.dirname(os.path.realpath(__file__))),
             "testing",
@@ -147,6 +148,7 @@ def test_25_checks_excluding_by_odoo_version(self):
             "translation-too-few-args",
             "translation-too-many-args",
             "translation-unsupported-format",
+            "no-raise-unlink",
         }
         self.default_extra_params += ["--valid-odoo-versions=13.0"]
         pylint_res = self.run_pylint(self.paths_modules)
@@ -165,6 +167,7 @@ def test_35_checks_emiting_by_odoo_version(self):
         expected_errors = self.expected_errors.copy()
         expected_errors.update({"manifest-version-format": 6})
         excluded_msgs = {
+            "no-raise-unlink",
             "translation-contains-variable",
         }
         for excluded_msg in excluded_msgs:
@@ -363,6 +366,19 @@ def test_160_check_only_disabled_one_check(self):
             expected_errors.pop(disable_error)
             self.assertDictEqual(real_errors, expected_errors)
 
+    def test_165_no_raises_unlink(self):
+        extra_params = ["--disable=all", "--enable=no-raise-unlink"]
+        test_repo = os.path.join(self.root_path_modules, "test_module")
+
+        self.assertDictEqual(
+            self.run_pylint([test_repo], extra_params).linter.stats.by_msg,
+            {"no-raise-unlink": 2},
+        )
+
+        # This check is only valid for Odoo 15.0 and upwards
+        extra_params.append("--valid-odoo-versions=14.0")
+        self.assertFalse(self.run_pylint([test_repo], extra_params).linter.stats.by_msg)
+
     @staticmethod
     def re_replace(sub_start, sub_end, substitution, content):
         re_sub = re.compile(rf"^{re.escape(sub_start)}$.*^{re.escape(sub_end)}$", re.M | re.S)