From 2dccc0d031d7609be11a93de98721a2fab4989c8 Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Fri, 26 Aug 2022 16:05:45 +0000 Subject: [PATCH] [IMP] base: faster get_bindings The cache key of _get_bindings was not super efficient. The result of _get_bindings is cached, but its performance was altered by the cache key which requires to fetch the user groups for each call to _get_bindings. Besides, as there is a lot of possible group combination, this resulted in a lot of possible cache keys, and therefore a lot of cached values. This revision aims to make _get_bindings more efficient by: - do not use the groups in the cache keys (less cached values) - filter out actions not available to the user groups after retrieving them from the cache - use has_group to do the above, which is itself cached as well, and therefore do not need to fetch the user groups at each call to get_bindings. In addition, move get_bindings from `get_view` to `get_views`. If there was 3 views asked by `get_views` (let's say kanban, list, form) `get_bindings` was being called 3 times, through `get_view` with each time the same arguments and therefore the same result :-). Moving it to `get_views` allows to call it only once for all view types requested, and for the web client it doesn't change much, as it always request the toolbar/get_bindings through `get_views` only. In addition, add the lang to the cache of _get_bindings. it was actually a bug not to put it: if you had 2 users with the same group set, using 2 different languages, the user accessing first the get_bindings would cache the action names within his language, and then the second user would see the action name within the language of the first user :-). Before ```py In [1]: %time for i in range(1000): self.env['ir.actions.actions'].get_bindings('res.partner'); self.env.invalidate_all(); CPU times: user 790 ms, sys: 104 ms, total: 893 ms Wall time: 1.7 s ``` After ```py In [1]: %time for i in range(1000): self.env['ir.actions.actions'].get_bindings('res.partner'); self.env.invalidate_all(); CPU times: user 23.5 ms, sys: 9.12 ms, total: 32.7 ms Wall time: 36.9 ms ``` Part-of: odoo/odoo#99417 --- addons/board/models/board.py | 12 ++-- addons/hr/models/res_users.py | 9 --- addons/hr/tests/test_self_user_access.py | 4 +- odoo/addons/base/models/ir_actions.py | 58 ++++++++++++------- odoo/addons/base/models/ir_module.py | 10 ++-- odoo/addons/base/models/res_users.py | 20 +++++++ .../tests/test_bindings.py | 16 ++--- odoo/models.py | 38 ++++++------ 8 files changed, 98 insertions(+), 69 deletions(-) diff --git a/addons/board/models/board.py b/addons/board/models/board.py index 9dc2978fd8e..08675445949 100644 --- a/addons/board/models/board.py +++ b/addons/board/models/board.py @@ -32,10 +32,14 @@ class Board(models.AbstractModel): if custom_view: res.update({'custom_view_id': custom_view.id, 'arch': custom_view.arch}) - res.update({ - 'arch': self._arch_preprocessing(res['arch']), - 'toolbar': {'print': [], 'action': [], 'relate': []} - }) + res['arch'] = self._arch_preprocessing(res['arch']) + return res + + @api.model + def get_views(self, views, options=None): + res = super().get_views(views, options) + for view in res['views'].values(): + view['toolbar'] = {'print': [], 'action': [], 'relate': []} return res @api.model diff --git a/addons/hr/models/res_users.py b/addons/hr/models/res_users.py index e102b99ca8b..49138a9d919 100644 --- a/addons/hr/models/res_users.py +++ b/addons/hr/models/res_users.py @@ -179,18 +179,9 @@ class User(models.Model): # Note: limit the `sudo` to the only action of "editing own profile" action in order to # avoid breaking `groups` mecanism on res.users form view. profile_view = self.env.ref("hr.res_users_view_form_profile") - original_user = self.env.user if profile_view and view_id == profile_view.id: self = self.with_user(SUPERUSER_ID) result = super(User, self).get_view(view_id, view_type, **options) - # Due to using the SUPERUSER the result will contain action that the user may not have access too - # here we filter out actions that requires special implicit rights to avoid having unusable actions - # in the dropdown menu. - if options.get('toolbar') and self.env.user != original_user: - self = self.with_user(original_user.id) - if not self.user_has_groups("base.group_erp_manager"): - change_password_action = self.env.ref("base.change_password_wizard_action") - result['toolbar']['action'] = [act for act in result['toolbar']['action'] if act['id'] != change_password_action.id] return result @api.model_create_multi diff --git a/addons/hr/tests/test_self_user_access.py b/addons/hr/tests/test_self_user_access.py index 36213ff912a..a3cb1c63075 100644 --- a/addons/hr/tests/test_self_user_access.py +++ b/addons/hr/tests/test_self_user_access.py @@ -86,7 +86,7 @@ class TestSelfAccessProfile(TestHrCommon): 'user_id': james.id, }) view = self.env.ref('hr.res_users_view_form_profile') - available_actions = james.get_view(view.id, toolbar=True)['toolbar']['action'] + available_actions = james.get_views([(view.id, 'form')], {'toolbar': True})['views']['form']['toolbar']['action'] change_password_action = self.env.ref("base.change_password_wizard_action") self.assertFalse(any(x['id'] == change_password_action.id for x in available_actions)) @@ -99,7 +99,7 @@ class TestSelfAccessProfile(TestHrCommon): 'user_id': john.id, }) view = self.env.ref('hr.res_users_view_form_profile') - available_actions = john.get_view(view.id, toolbar=True)['toolbar']['action'] + available_actions = john.get_views([(view.id, 'form')], {'toolbar': True})['views']['form']['toolbar']['action'] self.assertTrue(any(x['id'] == change_password_action.id for x in available_actions)) diff --git a/odoo/addons/base/models/ir_actions.py b/odoo/addons/base/models/ir_actions.py index 23c10df836c..9a90d60f737 100644 --- a/odoo/addons/base/models/ir_actions.py +++ b/odoo/addons/base/models/ir_actions.py @@ -4,6 +4,7 @@ import odoo from odoo import api, fields, models, tools, _, Command from odoo.exceptions import MissingError, ValidationError, AccessError +from odoo.tools import frozendict from odoo.tools.safe_eval import safe_eval, test_python_expr from odoo.tools.float_utils import float_compare from odoo.http import request @@ -87,24 +88,40 @@ class IrActions(models.Model): @api.model def get_bindings(self, model_name): - return self._get_bindings(model_name, bool(request) and request.session.debug) - - @tools.ormcache('frozenset(self.env.user.groups_id.ids)', 'model_name', 'debug') - def _get_bindings(self, model_name, debug=False): """ Retrieve the list of actions bound to the given model. :return: a dict mapping binding types to a list of dict describing actions, where the latter is given by calling the method ``read`` on the action record. """ + result = {} + for action_type, all_actions in self._get_bindings(model_name).items(): + actions = [] + for action in all_actions: + action = dict(action) + groups = action.pop('groups_id', None) + if groups and not self.user_has_groups(groups): + # the user may not perform this action + continue + res_model = action.pop('res_model', None) + if res_model and not self.env['ir.model.access'].check( + res_model, + mode='read', + raise_exception=False + ): + # the user won't be able to read records + continue + actions.append(action) + if actions: + result[action_type] = actions + return result + + @tools.ormcache('model_name', 'self.env.lang') + def _get_bindings(self, model_name): cr = self.env.cr - IrModelAccess = self.env['ir.model.access'] # discard unauthorized actions, and read action definitions result = defaultdict(list) - user_groups = self.env.user.groups_id - if not debug: - user_groups -= self.env.ref('base.group_no_one') self.env.flush_all() cr.execute(""" @@ -117,25 +134,22 @@ class IrActions(models.Model): for action_id, action_model, binding_type in cr.fetchall(): try: action = self.env[action_model].sudo().browse(action_id) - action_groups = getattr(action, 'groups_id', ()) - action_model = getattr(action, 'res_model', False) - if action_groups and not action_groups & user_groups: - # the user may not perform this action - continue - if action_model and not IrModelAccess.check(action_model, mode='read', raise_exception=False): - # the user won't be able to read records - continue fields = ['name', 'binding_view_types'] - if 'sequence' in action._fields: - fields.append('sequence') - result[binding_type].append(action.read(fields)[0]) - except (AccessError, MissingError): + for field in ('groups_id', 'res_model', 'sequence'): + if field in action._fields: + fields.append(field) + action = action.read(fields)[0] + if action.get('groups_id'): + groups = self.env['res.groups'].browse(action['groups_id']) + action['groups_id'] = ','.join(ext_id for ext_id in groups._ensure_xml_id().values()) + result[binding_type].append(frozendict(action)) + except (MissingError): continue # sort actions by their sequence if sequence available if result.get('action'): - result['action'] = sorted(result['action'], key=lambda vals: vals.get('sequence', 0)) - return result + result['action'] = tuple(sorted(result['action'], key=lambda vals: vals.get('sequence', 0))) + return frozendict(result) @api.model def _for_xml_id(self, full_xml_id): diff --git a/odoo/addons/base/models/ir_module.py b/odoo/addons/base/models/ir_module.py index 9c912c945af..5ded93cbe82 100644 --- a/odoo/addons/base/models/ir_module.py +++ b/odoo/addons/base/models/ir_module.py @@ -159,12 +159,12 @@ class Module(models.Model): _order = 'application desc,sequence,name' @api.model - def get_view(self, view_id=None, view_type='form', **options): - res = super().get_view(view_id, view_type, **options) - if view_type == 'form' and res.get('toolbar',False): + def get_views(self, views, options=None): + res = super().get_views(views, options) + if res['views'].get('form', {}).get('toolbar'): install_id = self.env.ref('base.action_server_module_immediate_install').id - action = [rec for rec in res['toolbar']['action'] if rec.get('id', False) != install_id] - res['toolbar'] = {'action': action} + action = [rec for rec in res['views']['form']['toolbar']['action'] if rec.get('id', False) != install_id] + res['views']['form']['toolbar'] = {'action': action} return res @classmethod diff --git a/odoo/addons/base/models/res_users.py b/odoo/addons/base/models/res_users.py index 403f16d5af3..a601778afff 100644 --- a/odoo/addons/base/models/res_users.py +++ b/odoo/addons/base/models/res_users.py @@ -215,6 +215,26 @@ class Groups(models.Model): self.env['ir.model.access'].call_cache_clearing_methods() return super(Groups, self).write(vals) + def _ensure_xml_id(self): + """Return the groups external identifiers, creating the external identifier for groups missing one""" + result = self.get_external_id() + missings = {group_id: f'__custom__.group_{group_id}' for group_id, ext_id in result.items() if not ext_id} + if missings: + self.env['ir.model.data'].create( + [ + { + 'name': name.split('.')[1], + 'model': 'res.groups', + 'res_id': group_id, + 'module': name.split('.')[0], + } + for group_id, name in missings.items() + ] + ) + result.update(missings) + + return result + class ResUsersLog(models.Model): _name = 'res.users.log' diff --git a/odoo/addons/test_action_bindings/tests/test_bindings.py b/odoo/addons/test_action_bindings/tests/test_bindings.py index e7bdcc66f0b..cb283b15443 100644 --- a/odoo/addons/test_action_bindings/tests/test_bindings.py +++ b/odoo/addons/test_action_bindings/tests/test_bindings.py @@ -9,8 +9,8 @@ class TestActionBindings(common.TransactionCase): # first make sure there is no bound action self.env.ref('base.action_partner_merge').unlink() bindings = Actions.get_bindings('res.partner') - self.assertFalse(bindings['action']) - self.assertFalse(bindings['report']) + self.assertFalse(bindings.get('action')) + self.assertFalse(bindings.get('report')) # create action bindings, and check the returned bindings action1 = self.env.ref('base.action_attachment') @@ -53,19 +53,19 @@ class TestBindingViewFilters(common.TransactionCase): def test_act_window(self): A = self.env['tab.a'] - form_act = A.get_view(toolbar=True)['toolbar']['action'] + form_act = A.get_views([(False, 'form')], {'toolbar': True})['views']['form']['toolbar']['action'] self.assertEqual( [a['name'] for a in form_act], ['Action 1', 'Action 2', 'Action 3'], "forms should have all actions") - list_act = A.get_view(view_type='tree', toolbar=True)['toolbar']['action'] + list_act = A.get_views([(False, 'list')], {'toolbar': True})['views']['list']['toolbar']['action'] self.assertEqual( [a['name'] for a in list_act], ['Action 1', 'Action 3'], "lists should not have the form-only action") - kanban_act = A.get_view(view_type='kanban', toolbar=True)['toolbar']['action'] + kanban_act = A.get_views([(False, 'kanban')], {'toolbar': True})['views']['kanban']['toolbar']['action'] self.assertEqual( [a['name'] for a in kanban_act], ['Action 1'], @@ -74,19 +74,19 @@ class TestBindingViewFilters(common.TransactionCase): def test_act_record(self): B = self.env['tab.b'] - form_act = B.get_view(toolbar=True)['toolbar']['action'] + form_act = B.get_views([(False, 'form')], {'toolbar': True})['views']['form']['toolbar']['action'] self.assertEqual( [a['name'] for a in form_act], ['Record 1', 'Record 2', 'Record 3'], "forms should have all actions") - list_act = B.get_view(view_type='tree', toolbar=True)['toolbar']['action'] + list_act = B.get_views([(False, 'list')], {'toolbar': True})['views']['list']['toolbar']['action'] self.assertEqual( [a['name'] for a in list_act], ['Record 1', 'Record 3'], "lists should not have the form-only action") - kanban_act = B.get_view(view_type='kanban', toolbar=True)['toolbar']['action'] + kanban_act = B.get_views([(False, 'kanban')], {'toolbar': True})['views']['kanban']['toolbar']['action'] self.assertEqual( [a['name'] for a in kanban_act], ['Record 1'], diff --git a/odoo/models.py b/odoo/models.py index 48c84673333..6f138bd8dc5 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -1657,6 +1657,24 @@ class BaseModel(metaclass=MetaModel): result['models'] = {model: self.env[model].fields_get() for model in models} + # Add related action information if asked + if options.get('toolbar'): + for view in result['views'].values(): + view['toolbar'] = {} + + bindings = self.env['ir.actions.actions'].get_bindings(self._name) + for action_type, key in (('report', 'print'), ('action', 'action')): + for action in bindings.get(action_type, []): + view_types = ( + action['binding_view_types'].split(',') + if action.get('binding_view_types') + else result['views'].keys() + ) + for view_type in view_types: + view_type = view_type if view_type != 'tree' else 'list' + if view_type in result['views']: + result['views'][view_type]['toolbar'].setdefault(key, []).append(action) + return result @api.model @@ -1671,7 +1689,6 @@ class BaseModel(metaclass=MetaModel): - bool load_filters: returns the model's filters (for search views) - bool mobile: true if the web client is currently using the responsive mobile view (to use kanban views instead of list views for x2many fields) - - bool toolbar: true to include contextual actions :return: architecture of the view as an etree node, and the browse record of the view used :rtype: tuple :raise AttributeError: @@ -1729,7 +1746,6 @@ class BaseModel(metaclass=MetaModel): - load_filters: returns the model's filters (for search views) - mobile: true if the web client is currently using the responsive mobile view (to use kanban views instead of list views for x2many fields) - - toolbar: true to include contextual actions :return: composition of the requested view (including inherited views and extensions) :rtype: dict :raise AttributeError: @@ -1760,22 +1776,6 @@ class BaseModel(metaclass=MetaModel): 'models': models, } - # Add related action information if asked - if options.get('toolbar') and view_type != 'search': - vt = 'list' if view_type == 'tree' else view_type - bindings = self.env['ir.actions.actions'].get_bindings(self._name) - resreport = [action - for action in bindings['report'] - if vt in (action.get('binding_view_types') or vt).split(',')] - resaction = [action - for action in bindings['action'] - if vt in (action.get('binding_view_types') or vt).split(',')] - - result['toolbar'] = { - 'print': resreport, - 'action': resaction, - } - if options.get('load_filters') and view_type == 'search': result['filters'] = self.env['ir.filters'].get_filters(self._name, options.get('action_id')) @@ -1823,7 +1823,7 @@ class BaseModel(metaclass=MetaModel): 'Method `fields_view_get` is deprecated, use `get_view` instead', DeprecationWarning, stacklevel=2, ) - result = self.get_view(view_id, view_type, toolbar=toolbar, submenu=submenu) + result = self.get_views([(view_id, view_type)], {'toolbar': toolbar, 'submenu': submenu})['views'][view_type] node = etree.fromstring(result['arch']) view_fields = set(el.get('name') for el in node.xpath('.//field[not(ancestor::field)]')) result['fields'] = self.fields_get(view_fields)