[FIX] core, *: BaseModel overrides signatures

*: base, account, crm, hr, hr_attendance, test_access_rights

The various public methods of the ORM can be override in other models,
those overrides sometime don't implement the exact same signature as the
original method in the ORM. In this work we sanitize all the overrides
to ensure a better compatibility. The background objective is to make it
possible to call any public method using kwarg: `search(domain=[...])`.

* `search`, the first parameter was renamed from `args` to `domain` in
  0e9adf7 but the overrides were not updated.
* `invalidate_models` and `invalidate_recordset`, a new `flush=True`
  parameter was introduced in 9c3b9a4 but the overrides were not
  updated.
* `update`, there is a clash between the `update` method responsible for
  writing on a record and `update` in bus responsible to update the user
  presence. The bus method has been renamed so it doesn't clash with the
  ORM.

This sanitization comes with a new linter that verifies that all
overrides of BaseModel public methods share a compatible signature. The
linter has been disabled for `create`, `write` and `default_get` as too
many overrides don't respect the signature of BaseModel.

closes odoo/odoo#106999

Related: odoo/enterprise#34991
Signed-off-by: Julien Castiaux <juc@odoo.com>

addons/account/models/account_move_line.py

@@ -1251,7 +1251,7 @@ class AccountMoveLine(models.Model):
     # -------------------------------------------------------------------------
     # CRUD/ORM
     # -------------------------------------------------------------------------
-    def invalidate_model(self, fnames=None):
+    def invalidate_model(self, fnames=None, flush=True):
         # Invalidate cache of related moves
         if fnames is None or 'move_id' in fnames:
             field = self._fields['move_id']
@@ -1259,16 +1259,16 @@ class AccountMoveLine(models.Model):
             move_ids = {id_ for id_ in self.env.cache.get_values(lines, field) if id_}
             if move_ids:
                 self.env['account.move'].browse(move_ids).invalidate_recordset()
-        return super().invalidate_model(fnames)
+        return super().invalidate_model(fnames, flush)
 
-    def invalidate_recordset(self, fnames=None):
+    def invalidate_recordset(self, fnames=None, flush=True):
         # Invalidate cache of related moves
         if fnames is None or 'move_id' in fnames:
             field = self._fields['move_id']
             move_ids = {id_ for id_ in self.env.cache.get_values(self, field) if id_}
             if move_ids:
                 self.env['account.move'].browse(move_ids).invalidate_recordset()
-        return super().invalidate_recordset(fnames)
+        return super().invalidate_recordset(fnames, flush)
 
     @api.model
     def search_read(self, domain=None, fields=None, offset=0, limit=None, order=None):

addons/bus/models/bus_presence.py

@@ -17,7 +17,7 @@ AWAY_TIMER = 1800  # 30 minutes
 class BusPresence(models.Model):
     """ User Presence
         Its status is 'online', 'away' or 'offline'. This model should be a one2one, but is not
-        attached to res_users to avoid database concurrence errors. Since the 'update' method is executed
+        attached to res_users to avoid database concurrence errors. Since the 'update_presence' method is executed
         at each poll, if the user have multiple opened tabs, concurrence errors can happend, but are 'muted-logged'.
     """
 
@@ -34,7 +34,7 @@ class BusPresence(models.Model):
         self.env.cr.execute("CREATE UNIQUE INDEX IF NOT EXISTS bus_presence_user_unique ON %s (user_id) WHERE user_id IS NOT NULL" % self._table)
 
     @api.model
-    def update(self, inactivity_period, identity_field, identity_value):
+    def update_presence(self, inactivity_period, identity_field, identity_value):
         """ Updates the last_poll and last_presence of the current user
             :param inactivity_period: duration in milliseconds
         """
@@ -44,7 +44,7 @@ class BusPresence(models.Model):
             # Hide transaction serialization errors, which can be ignored, the presence update is not essential
             # The errors are supposed from presence.write(...) call only
             with tools.mute_logger('odoo.sql_db'):
-                self._update(inactivity_period=inactivity_period, identity_field=identity_field, identity_value=identity_value)
+                self._update_presence(inactivity_period=inactivity_period, identity_field=identity_field, identity_value=identity_value)
                 # commit on success
                 self.env.cr.commit()
         except OperationalError as e:
@@ -54,7 +54,7 @@ class BusPresence(models.Model):
             raise
 
     @api.model
-    def _update(self, inactivity_period, identity_field, identity_value):
+    def _update_presence(self, inactivity_period, identity_field, identity_value):
         presence = self.search([(identity_field, '=', identity_value)], limit=1)
         # compute last_presence timestamp
         last_presence = datetime.datetime.now() - datetime.timedelta(milliseconds=inactivity_period)

addons/bus/models/ir_websocket.py

@@ -40,7 +40,7 @@ class IrWebsocket(models.AbstractModel):
 
     def _update_bus_presence(self, inactivity_period, im_status_ids_by_model):
         if self.env.user and not self.env.user._is_public():
-            self.env['bus.presence'].update(
+            self.env['bus.presence'].update_presence(
                 inactivity_period,
                 identity_field='user_id',
                 identity_value=self.env.uid

addons/crm/models/crm_lead.py

@@ -774,7 +774,7 @@ class Lead(models.Model):
         return result
 
     @api.model
-    def search(self, args, offset=0, limit=None, order=None, count=False):
+    def search(self, domain, offset=0, limit=None, order=None, count=False):
         """ Override to support ordering on my_activity_date_deadline.
 
         Ordering through web client calls search_read with an order parameter set.
@@ -805,7 +805,7 @@ class Lead(models.Model):
         side effects. Search_count is not affected by this override.
         """
         if count or not order or 'my_activity_date_deadline' not in order:
-            return super(Lead, self).search(args, offset=offset, limit=limit, order=order, count=count)
+            return super(Lead, self).search(domain, offset=offset, limit=limit, order=order, count=count)
         order_items = [order_item.strip().lower() for order_item in (order or self._order).split(',')]
 
         # Perform a read_group on my activities to get a mapping lead_id / deadline
@@ -820,7 +820,7 @@ class Lead(models.Model):
         )
         my_lead_mapping = dict((item['res_id'], item['date_deadline']) for item in my_lead_activities)
         my_lead_ids = list(my_lead_mapping.keys())
-        my_lead_domain = expression.AND([[('id', 'in', my_lead_ids)], args])
+        my_lead_domain = expression.AND([[('id', 'in', my_lead_ids)], domain])
         my_lead_order = ', '.join(item for item in order_items if 'my_activity_date_deadline' not in item)
 
         # Search leads linked to those activities and order them. See docstring
@@ -848,7 +848,7 @@ class Lead(models.Model):
         lead_order = ', '.join(item for item in order_items if 'my_activity_date_deadline' not in item)
 
         other_lead_res = super(Lead, self).search(
-            expression.AND([[('id', 'not in', my_lead_ids_skip)], args]),
+            expression.AND([[('id', 'not in', my_lead_ids_skip)], domain]),
             offset=lead_offset, limit=lead_limit, order=lead_order, count=count
         )
         return self.browse(my_lead_ids_keep) + other_lead_res

addons/hr/models/hr_employee.py

@@ -219,10 +219,10 @@ class HrEmployeePrivate(models.Model):
                     user_id=responsible_user_id)
         employees_scheduled.write({'work_permit_scheduled_activity': True})
 
-    def read(self, fields, load='_classic_read'):
+    def read(self, fields=None, load='_classic_read'):
         if self.check_access_rights('read', raise_exception=False):
             return super(HrEmployeePrivate, self).read(fields, load=load)
-        private_fields = set(fields).difference(self.env['hr.employee.public']._fields.keys())
+        private_fields = set(fields or self._fields).difference(self.env['hr.employee.public']._fields)
         if private_fields:
             raise AccessError(_('The fields "%s" you try to read is not available on the public employee profile.') % (','.join(private_fields)))
         return self.env['hr.employee.public'].browse(self.ids).read(fields, load=load)

addons/hr_attendance/models/hr_attendance.py

@@ -293,5 +293,5 @@ class HrAttendance(models.Model):
         self._update_overtime(attendances_dates)
 
     @api.returns('self', lambda value: value.id)
-    def copy(self):
+    def copy(self, default=None):
         raise exceptions.UserError(_('You cannot duplicate an attendance.'))

addons/mail/models/ir_websocket.py

@@ -46,4 +46,4 @@ class IrWebsocket(models.AbstractModel):
             guest_sudo = self.env['mail.guest']._get_guest_from_request(req).sudo()
             if not guest_sudo:
                 return
-            guest_sudo.env['bus.presence'].update(inactivity_period, identity_field='guest_id', identity_value=guest_sudo.id)
+            guest_sudo.env['bus.presence'].update_presence(inactivity_period, identity_field='guest_id', identity_value=guest_sudo.id)

odoo/addons/test_access_rights/models.py

@@ -43,10 +43,10 @@ class ObjCateg(models.Model):
 
     name = fields.Char(required=True)
 
-    def search(self, args, **kwargs):
+    def search(self, domain, *args, **kwargs):
         if self.env.context.get('only_media'):
-            args += [('name', '=', 'Media')]
-        return super(ObjCateg, self).search(args, **kwargs)
+            domain += [('name', '=', 'Media')]
+        return super(ObjCateg, self).search(domain, **kwargs)
 
 
 class FakeTicket(models.Model):

odoo/addons/test_lint/tests/__init__.py

@@ -8,5 +8,6 @@ from . import test_l10n
 from . import test_manifests
 from . import test_markers
 from . import test_onchange_domains
+from . import test_override_signatures
 from . import test_pofile
 from . import test_pylint

odoo/addons/test_lint/tests/test_override_signatures.py

@@ -0,0 +1,155 @@
+import collections
+import inspect
+import itertools
+
+import odoo
+from odoo.tests.common import get_db_name, tagged
+from .lint_case import LintCase
+
+
+POSITIONAL_ONLY = inspect.Parameter.POSITIONAL_ONLY
+POSITIONAL_OR_KEYWORD = inspect.Parameter.POSITIONAL_OR_KEYWORD
+VAR_POSITIONAL = inspect.Parameter.VAR_POSITIONAL
+KEYWORD_ONLY = inspect.Parameter.KEYWORD_ONLY
+VAR_KEYWORD = inspect.Parameter.VAR_KEYWORD
+
+
+failure_message = """\
+Invalid override of {model}.{method} in {child_module}, {message}.
+
+Original definition in {parent_module}:
+    def {method}{original_signature}
+
+Incompatible definition in {child_module}:
+    def {method}{override_signature}"""
+
+
+methods_to_sanitize = {
+    method_name
+    for method_name in dir(odoo.models.BaseModel)
+    if not method_name.startswith('_')
+} - {
+    # Not yet sanitized...
+    'write', 'create', 'default_get', 'init'
+}
+
+
+class HitMiss:
+    def __init__(self):
+        self.hit = 0
+        self.miss = 0
+    @property
+    def ratio(self):
+        return self.hit / (self.hit + self.miss)
+counter = collections.defaultdict(HitMiss)
+
+
+def get_odoo_module_name(python_module_name):
+    if python_module_name.startswith('odoo.addons.'):
+        return python_module_name.split('.')[2]
+    if python_module_name == 'odoo.models':
+        return 'odoo'
+
+    return python_module_name
+
+
+def assert_valid_override(parent_signature, child_signature):
+    pparams = parent_signature.parameters
+    cparams = child_signature.parameters
+
+    # parent and child have exact same signature
+    if pparams == cparams:
+        return
+
+    # child overrides is exactly (self, *args, **kwargs)
+    if [cp.kind for cp in cparams.values()] == [VAR_POSITIONAL, VAR_KEYWORD]:
+        return
+
+    # parent has *args/**kwargs: child can define new custom args/kwargs
+    parent_has_varargs = any(pp.kind == VAR_POSITIONAL for pp in pparams.values())
+    parent_has_varkwargs = any(pp.kind == VAR_KEYWORD for pp in pparams.values())
+
+    # child has *args/**kwargs: all unknown args/kargs are delegated
+    child_has_varargs = any(cp.kind == VAR_POSITIONAL for cp in cparams.values())
+    child_has_varkwargs = any(cp.kind == VAR_KEYWORD for cp in cparams.values())
+
+    # check positionals
+    pposparams = [(pp_name, pp) for pp_name, pp in pparams.items()
+                  if pp.kind in (POSITIONAL_ONLY, POSITIONAL_OR_KEYWORD)]
+    cposparams = [(cp_name, cp) for cp_name, cp in cparams.items()
+                  if cp.kind in (POSITIONAL_ONLY, POSITIONAL_OR_KEYWORD)]
+    if len(cposparams) < len(pposparams):
+        assert child_has_varargs, "missing positional parameters"
+        assert cposparams == pposparams[:len(cposparams)], "wrong positional parameters"
+    elif len(cposparams) > len(pposparams):
+        assert parent_has_varargs, "too many positional parameters"
+        assert cposparams[:len(pposparams)] == pposparams, "wrong positional parameters"
+    else:
+        assert cposparams == pposparams, "wrong positional parameters"
+
+    # check keywords
+    pkwparams = {(pp_name, pp) for pp_name, pp in pparams.items()
+                 if pp.kind in (POSITIONAL_OR_KEYWORD, KEYWORD_ONLY)}
+    ckwparams = {(cp_name, cp) for cp_name, cp in cparams.items()
+                 if cp.kind in (POSITIONAL_OR_KEYWORD, KEYWORD_ONLY)}
+    if ckwparams < pkwparams:
+        assert child_has_varkwargs, "missing keyword parameters"
+    elif ckwparams > pkwparams:
+        assert parent_has_varkwargs, "too many keyword parameters"
+    elif ckwparams != pkwparams:
+        assert child_has_varkwargs and parent_has_varkwargs, "wrong keyword parameters"
+
+
+@tagged('-at_install', 'post_install')
+class TestLintOverrideSignatures(LintCase):
+    def test_lint_override_signature(self):
+        self.failureException = TypeError
+        registry = odoo.registry(get_db_name())
+        for model_name, model_cls in registry.items():
+            for method_name, _ in inspect.getmembers(model_cls, inspect.isroutine):
+                if method_name not in methods_to_sanitize:
+                    continue
+
+                # Find the original function definition
+                reverse_mro = reversed(model_cls.mro()[1:-1])
+                for parent_class in reverse_mro:
+                    method = getattr(parent_class, method_name, None)
+                    if callable(method):
+                        break
+
+                parent_module = get_odoo_module_name(parent_class.__module__)
+                original_signature = inspect.signature(method)
+
+                iter_parent, iter_children = itertools.tee(reverse_mro)
+                iter_parent = itertools.chain([parent_class], iter_parent)
+
+                # Assert that all child classes correctly override the method
+                for parent_class, child_class in zip(iter_parent, iter_children):
+                    if method_name not in child_class.__dict__:
+                        continue
+                    override = getattr(child_class, method_name)
+
+                    child_module = get_odoo_module_name(child_class.__module__)
+                    override_signature = inspect.signature(override)
+
+                    with self.subTest(module=child_module, model=model_name, method=method_name):
+                        try:
+                            assert_valid_override(original_signature, override_signature)
+                            counter[method_name].hit += 1
+                        except AssertionError as exc:
+                            counter[method_name].miss += 1
+                            raise TypeError(failure_message.format(
+                                message=exc.args[0],
+                                model=model_name,
+                                method=method_name,
+                                child_module=child_module,
+                                parent_module=parent_module,
+                                original_signature=original_signature,
+                                override_signature=override_signature,
+                            )) from None
+
+        #with open('/tmp/odoo-override-signatures.md', 'w') as f:
+        #    f.write('method|hit|miss|ratio\n')
+        #    f.write('------|---|----|-----\n')
+        #    for method_name, hm in sorted(counter.items(), key=lambda x: x[1].miss):
+        #        f.write(f'{method_name}|{hm.hit}|{hm.miss}|{hm.ratio:.3f}\n')