From 7349e23837e8433bedc134c6bd2c33e92b033209 Mon Sep 17 00:00:00 2001 From: Christophe Simonis Date: Thu, 20 Aug 2015 11:44:04 +0200 Subject: [PATCH] [FIX] models: correct ORDER BY onm many2one fields Now the we follow many2one when generating ORDER BY, we need to keep track of visited relations to avoid loops. Closes #8114 --- openerp/addons/base/tests/test_search.py | 60 ++++++++++++++++++++++-- openerp/models.py | 34 +++++++++----- 2 files changed, 77 insertions(+), 17 deletions(-) diff --git a/openerp/addons/base/tests/test_search.py b/openerp/addons/base/tests/test_search.py index af14fc88e85..6cb8490546d 100644 --- a/openerp/addons/base/tests/test_search.py +++ b/openerp/addons/base/tests/test_search.py @@ -5,6 +5,18 @@ import openerp.tests.common as common class test_search(common.TransactionCase): + def patch_order(self, model, order): + m_e = self.env[model] + m_r = self.registry(model) + + old_order = m_e._order + + @self.addCleanup + def cleanup(): + m_r._order = type(m_e)._order = old_order + + m_r._order = type(m_e)._order = order + def test_00_search_order(self): registry, cr, uid = self.registry, self.cr, self.uid @@ -96,15 +108,13 @@ class test_search(common.TransactionCase): self.assertEqual(test_user_ids, expected_ids, 'search on res_users did not provide expected ids or expected order') # Do: order on many2one, but not by specifying in order parameter of search, but by overriding _order of res_users - old_order = users_obj._order - users_obj._order = 'country_id desc, name asc, login desc' + self.patch_order('res.users', 'country_id desc, name asc, login desc') user_ids = users_obj.search(cr, search_user, []) expected_ids = [search_user, c, b, a] test_user_ids = filter(lambda x: x in expected_ids, user_ids) self.assertEqual(test_user_ids, expected_ids, 'search on res_users did not provide expected ids or expected order') - users_obj._order = old_order - def test_11_indirect_inherits_m2order(self): + def test_11_indirect_inherits_m2o_order(self): registry, cr, uid = self.registry, self.cr, self.uid Cron = registry('ir.cron') Users = registry('res.users') @@ -119,6 +129,48 @@ class test_search(common.TransactionCase): expected_ids = [cron_ids[l] for l in 'ABC'] self.assertEqual(ids, expected_ids) + def test_12_m2o_order_loop_self(self): + registry, cr, uid = self.registry, self.cr, self.uid + + Cats = registry('ir.module.category') + ids = {} + def create(name, **kw): + ids[name] = Cats.create(cr, uid, dict(kw, name=name)) + + self.patch_order('ir.module.category', 'parent_id desc, name') + + create('A') + create('B', parent_id=ids['A']) + create('C', parent_id=ids['A']) + create('D') + create('E', parent_id=ids['D']) + create('F', parent_id=ids['D']) + + expected_order = [ids[x] for x in 'ADEFBC'] + domain = [('id', 'in', ids.values())] + search_result = Cats.search(cr, uid, domain) + self.assertEqual(search_result, expected_order) + + def test_13_m2o_order_loop_multi(self): + Users = self.env['res.users'] + + # will sort by login desc of the creator, then by name + self.patch_order('res.partner', 'create_uid, name') + self.patch_order('res.users', 'partner_id, login desc') + + kw = dict(groups_id=[(6, 0, [self.ref('base.group_system'), + self.ref('base.group_partner_manager')])]) + + u1 = Users.create(dict(name='Q', login='m', **kw)).id + u2 = Users.sudo(user=u1).create(dict(name='B', login='f', **kw)).id + u3 = Users.create(dict(name='C', login='c', **kw)).id + u4 = Users.sudo(user=u2).create(dict(name='D', login='z', **kw)).id + + expected_order = [u2, u4, u3, u1] + + domain = [('id', 'in', [u1, u2, u3, u4])] + search_result = list(Users.search(domain)._ids) + self.assertEqual(search_result, expected_order) if __name__ == '__main__': unittest2.main() diff --git a/openerp/models.py b/openerp/models.py index 081bf307271..35944ff4873 100644 --- a/openerp/models.py +++ b/openerp/models.py @@ -4526,7 +4526,7 @@ class BaseModel(object): apply_rule(rule_where_clause, rule_where_clause_params, rule_tables, parent_model=inherited_model) - def _generate_m2o_order_by(self, alias, order_field, query, reverse_direction): + def _generate_m2o_order_by(self, alias, order_field, query, reverse_direction, seen): """ Add possibly missing JOIN to ``query`` and generate the ORDER BY clause for m2o fields, either native m2o fields or function/related fields that are stored, including @@ -4536,18 +4536,18 @@ class BaseModel(object): """ if order_field not in self._columns and order_field in self._inherit_fields: # also add missing joins for reaching the table containing the m2o field - qualified_field = self._inherits_join_calc(alias, order_field, query) order_field_column = self._inherit_fields[order_field][2] + qualified_field = self._inherits_join_calc(alias, order_field, query) + alias, order_field = qualified_field.replace('"', '').split('.', 1) else: - qualified_field = '"%s"."%s"' % (alias, order_field) order_field_column = self._columns[order_field] assert order_field_column._type == 'many2one', 'Invalid field passed to _generate_m2o_order_by()' if not order_field_column._classic_write and not getattr(order_field_column, 'store', False): - _logger.debug("Many2one function/related fields must be stored " \ - "to be used as ordering fields! Ignoring sorting for %s.%s", - self._name, order_field) - return + _logger.debug("Many2one function/related fields must be stored " + "to be used as ordering fields! Ignoring sorting for %s.%s", + self._name, order_field) + return [] # figure out the applicable order_by for the m2o dest_model = self.pool[order_field_column._obj] @@ -4558,12 +4558,14 @@ class BaseModel(object): # Join the dest m2o table if it's not joined yet. We use [LEFT] OUTER join here # as we don't want to exclude results that have NULL values for the m2o - src_table, src_field = qualified_field.replace('"', '').split('.', 1) - dst_alias, dst_alias_statement = query.add_join((src_table, dest_model._table, src_field, 'id', src_field), implicit=False, outer=True) + join = (alias, dest_model._table, order_field, 'id', order_field) + dst_alias, dst_alias_statement = query.add_join(join, implicit=False, outer=True) return dest_model._generate_order_by_inner(dst_alias, m2o_order, query, - reverse_direction=reverse_direction) + reverse_direction=reverse_direction, seen=seen) - def _generate_order_by_inner(self, alias, order_spec, query, reverse_direction=False): + def _generate_order_by_inner(self, alias, order_spec, query, reverse_direction=False, seen=None): + if seen is None: + seen = set() order_by_elements = [] self._check_qorder(order_spec) for order_part in order_spec.split(','): @@ -4584,7 +4586,10 @@ class BaseModel(object): inner_clauses = ['"%s"."%s"' % (alias, order_field)] add_dir = True elif order_column._type == 'many2one': - inner_clauses = self._generate_m2o_order_by(alias, order_field, query, do_reverse) + key = (self._name, order_column._obj, order_field) + if key not in seen: + seen.add(key) + inner_clauses = self._generate_m2o_order_by(alias, order_field, query, do_reverse, seen) else: continue # ignore non-readable or "non-joinable" fields elif order_field in self._inherit_fields: @@ -4594,7 +4599,10 @@ class BaseModel(object): inner_clauses = [self._inherits_join_calc(alias, order_field, query)] add_dir = True elif order_column._type == 'many2one': - inner_clauses = self._generate_m2o_order_by(alias, order_field, query, do_reverse) + key = (parent_obj._name, order_column._obj, order_field) + if key not in seen: + seen.add(key) + inner_clauses = self._generate_m2o_order_by(alias, order_field, query, do_reverse, seen) else: continue # ignore non-readable or "non-joinable" fields else: