From ad9bd90d7bed367074a246783d82dd4cdf577f00 Mon Sep 17 00:00:00 2001 From: Julien Castiaux Date: Thu, 1 Dec 2022 14:07:32 +0000 Subject: [PATCH] [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 --- addons/account/models/account_move_line.py | 8 +- addons/bus/models/bus_presence.py | 8 +- addons/bus/models/ir_websocket.py | 2 +- addons/crm/models/crm_lead.py | 8 +- addons/hr/models/hr_employee.py | 4 +- addons/hr_attendance/models/hr_attendance.py | 2 +- addons/mail/models/ir_websocket.py | 2 +- odoo/addons/test_access_rights/models.py | 6 +- odoo/addons/test_lint/tests/__init__.py | 1 + .../tests/test_override_signatures.py | 155 ++++++++++++++++++ 10 files changed, 176 insertions(+), 20 deletions(-) create mode 100644 odoo/addons/test_lint/tests/test_override_signatures.py diff --git a/addons/account/models/account_move_line.py b/addons/account/models/account_move_line.py index 86a527b5150..9760d5f8787 100644 --- a/addons/account/models/account_move_line.py +++ b/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): diff --git a/addons/bus/models/bus_presence.py b/addons/bus/models/bus_presence.py index 72e1983fcb7..4da96e7b9b4 100644 --- a/addons/bus/models/bus_presence.py +++ b/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) diff --git a/addons/bus/models/ir_websocket.py b/addons/bus/models/ir_websocket.py index 22a56297a3e..847d7a76b46 100644 --- a/addons/bus/models/ir_websocket.py +++ b/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 diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index dad5d39eaec..89189d5c4cc 100644 --- a/addons/crm/models/crm_lead.py +++ b/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 diff --git a/addons/hr/models/hr_employee.py b/addons/hr/models/hr_employee.py index 31656094c05..310248674f8 100644 --- a/addons/hr/models/hr_employee.py +++ b/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) diff --git a/addons/hr_attendance/models/hr_attendance.py b/addons/hr_attendance/models/hr_attendance.py index 08d6c714d17..526a803fa2c 100644 --- a/addons/hr_attendance/models/hr_attendance.py +++ b/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.')) diff --git a/addons/mail/models/ir_websocket.py b/addons/mail/models/ir_websocket.py index 9b6b65673cf..9af56e5ed8b 100644 --- a/addons/mail/models/ir_websocket.py +++ b/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) diff --git a/odoo/addons/test_access_rights/models.py b/odoo/addons/test_access_rights/models.py index 01be7059dc5..770f1142e6f 100644 --- a/odoo/addons/test_access_rights/models.py +++ b/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): diff --git a/odoo/addons/test_lint/tests/__init__.py b/odoo/addons/test_lint/tests/__init__.py index 8a1444a60d3..f7908d8a6e3 100644 --- a/odoo/addons/test_lint/tests/__init__.py +++ b/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 diff --git a/odoo/addons/test_lint/tests/test_override_signatures.py b/odoo/addons/test_lint/tests/test_override_signatures.py new file mode 100644 index 00000000000..2211f450e95 --- /dev/null +++ b/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')