From 6a89f5b4f932b167a445427e0b3ee97be3cc8b44 Mon Sep 17 00:00:00 2001 From: Petar Najman Date: Fri, 24 Mar 2023 10:18:59 +0000 Subject: [PATCH] [IMP] core: add support for NULLS {FIRST | LAST} in ORDER BY clauses Recent Odoo versions require modern postgres (e.g. use of jsonb), the `NULLS {FIRST | LAST}` clause was added in 8.3 so should be well supported. While Odoo's use of nulls is not always consistent, the NULLS ordering clauses can be quite useful especially when sorting `DESC`: `NULLS FIRST` and `NULLS LAST` are literal positions so they put nulls at that location regardless of sort order whereas the default Postgres ordering is to consider nulls larger than every other value so they appear first when sorting DESC, which is often undesirable (putting nulls first when sorting ASC can also be useful to fill records). Update `_generate_order_by_inner` to correctly process `NULLS` clauses: - Use `regex_order` to ensure we parse orderings correctly and consistently, also update `regex_order` to use named groups to make the relevant bits clearer (and VERBOSE for readability). - Given `a_id DESC` and `_fields[a_id].relation._order = 'xxx NULLS LAST'`, reversing the clause should order by `xxx DESC NULLS FIRST` to correctly flip the original as `NULLS` clauses are not relative to the sorting order (they put the `NULL`-valued records at the specified location regardless). Therefore apply `reverse_direction` to `NULLS`. - Manually expand `NULLS` clauses on m2o fields: propagating the clause through the join would yield unexpected and illogical results when mixing NULL m2o fields and non-NULL m2o fields with NULL `_order` fields (as they would get mixed rather than clearly layered / separated). However because SQL booleans are ordered the usual way (`true > false`) the `NULLS` clause is fundamentally equivalent to sorting on the field being NULL (if `NULLS LAST`) or not (if `NULLS FIRST`). So we can prepend the expanded m2o's ordering with such a clause to get the correct behavior. Replaces #116464 Supersedes #116664 closes odoo/odoo#117439 Signed-off-by: Raphael Collet Co-authored-by: Petar Najman --- odoo/addons/base/tests/test_search.py | 74 +++++++++++++++++++++++++++ odoo/models.py | 40 ++++++++++----- 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/odoo/addons/base/tests/test_search.py b/odoo/addons/base/tests/test_search.py index 53fa5dd455b..758176376ac 100644 --- a/odoo/addons/base/tests/test_search.py +++ b/odoo/addons/base/tests/test_search.py @@ -60,6 +60,80 @@ class test_search(TransactionCase): id_desc_active_desc = Partner.search([('name', 'like', 'test_search_order%'), '|', ('active', '=', True), ('active', '=', False)], order="id desc, active desc") self.assertEqual([e, ab, b, a, d, c], list(id_desc_active_desc), "Search with 'ID DESC, ACTIVE DESC' order failed.") + a.ref = "ref1" + c.ref = "ref2" + ids = (a | b | c).ids + for order, result in [ + ('ref', a | c | b), + ('ref desc', b | c | a), + ('ref asc nulls first', b | a | c), + ('ref asc nulls last', a | c | b), + ('ref desc nulls first', b | c | a), + ('ref desc nulls last', c | a | b) + ]: + with self.subTest(order): + self.assertEqual( + Partner.search([('id', 'in', ids)], order=order).mapped('name'), + result.mapped('name')) + + # sorting by an m2o should alias to the natural order of the m2o + self.patch_order('res.country', 'phone_code') + a.country_id, c.country_id = self.env['res.country'].create([{ + 'name': "Country 1", + 'code': 'C1', + 'phone_code': '01', + }, { + 'name': 'Country 2', + 'code': 'C2', + 'phone_code': '02' + }]) + + for order, result in [ + ('country_id', a | c | b), + ('country_id desc', b | c | a), + ('country_id asc nulls first', b | a | c), + ('country_id asc nulls last', a | c | b), + ('country_id desc nulls first', b | c | a), + ('country_id desc nulls last', c | a | b) + ]: + with self.subTest(order): + self.assertEqual( + Partner.search([('id', 'in', ids)], order=order).mapped('name'), + result.mapped('name')) + + # NULLS applies to the m2o itself, not its sub-fields, so a null `phone_code` + # will sort normally (larger than non-null codes) + b.country_id = self.env['res.country'].create({'name': "Country X", 'code': 'C3'}) + + for order, result in [ + ('country_id', a | c | b), + ('country_id desc', b | c | a), + ('country_id asc nulls first', a | c | b), + ('country_id asc nulls last', a | c | b), + ('country_id desc nulls first', b | c | a), + ('country_id desc nulls last', b | c | a) + ]: + with self.subTest(order): + self.assertEqual( + Partner.search([('id', 'in', ids)], order=order).mapped('name'), + result.mapped('name')) + + # a field DESC should reverse the nested behaviour (and thus the inner + # NULLS clauses), but the outer NULLS clause still has no effect + self.patch_order('res.country', 'phone_code NULLS FIRST') + for order, result in [ + ('country_id', b | a | c), + ('country_id desc', c | a | b), + ('country_id asc nulls first', b | a | c), + ('country_id asc nulls last', b | a | c), + ('country_id desc nulls first', c | a | b), + ('country_id desc nulls last', c | a | b) + ]: + with self.subTest(order): + self.assertEqual( + Partner.search([('id', 'in', ids)], order=order).mapped('name'), + result.mapped('name')) + def test_10_inherits_m2order(self): Users = self.env['res.users'] diff --git a/odoo/models.py b/odoo/models.py index 9764c2eae56..df3e7649116 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -61,7 +61,6 @@ from .tools import ( get_lang, LastOrderedSet, lazy_classproperty, OrderedSet, ormcache, partition, populate, Query, ReversedIterable, split_every, unique, ) -from .tools.func import frame_codeinfo from .tools.lru import LRU from .tools.translate import _, _lt @@ -69,7 +68,18 @@ _logger = logging.getLogger(__name__) _unlink = logging.getLogger(__name__ + '.unlink') regex_alphanumeric = re.compile(r'^[a-z0-9_]+$') -regex_order = re.compile(r'^(\s*([a-z0-9:_]+|"[a-z0-9:_]+")(\.[a-z0-9]+)?(\s+(desc|asc))?\s*(,|$))+(?[a-z0-9:_]+|"[a-z0-9:_]+")(\.(?P[a-z0-9_]+))?) + (\s+(?Pdesc|asc))? + (\s+(?Pnulls\ first|nulls\ last))? + \s* + (,|$) + )+ + (? '{property_name}')" - order_by_elements.append("%s %s" % (qualifield_name, order_direction)) + order_by_elements.append(f"{qualifield_name} {order_direction} {order_nulls}") else: _logger.warning("Model %r cannot be sorted on field %r (not a column)", self._name, order_field) continue # ignore non-readable or "non-joinable" fields