From 4ef0c00b4b37d75b78c5cd30dba004ffd0f8b0ac Mon Sep 17 00:00:00 2001
From: Vincent Schippefilt <vsc@odoo.com>
Date: Fri, 25 Feb 2022 13:02:25 +0000
Subject: [PATCH] [ADD] models: add _read_group method to avoid sorting on the
 names of many2one

 The public read_group method is used by back-end code as well as RPC calls to receive grouped data.
 Most of the time, if the data isn't shown to the user, it doesn't need to be sorted on the names of
 the many2one, and can just be sorted on their ID instead, avoiding extra joins in the resulting queries

 This is a first commit that will reduce the features of _read_group to the strict minimum necessary
 to obtain correct grouped data that is never shown to a user

Task-id: 2479334
Part-of: odoo/odoo#84908
---
 odoo/addons/base/tests/test_base.py | 111 +++++++++++++++++++++++++++-
 odoo/models.py                      |  32 +++++++-
 2 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/odoo/addons/base/tests/test_base.py b/odoo/addons/base/tests/test_base.py
index b060f3abd3a..4a24948bfb2 100644
--- a/odoo/addons/base/tests/test_base.py
+++ b/odoo/addons/base/tests/test_base.py
@@ -196,7 +196,7 @@ class TestBase(TransactionCase):
         self.assertEqual(p1.street, p1street, 'Address fields must not be synced after turning sync off')
         self.assertNotEqual(ghoststep.street, p1street, 'Parent address must never be touched')
 
-        # turn on sync again       
+        # turn on sync again
         p1.write({'type': 'contact'})
         self.assertEqual(p1.street, ghoststep.street, 'Address fields must be synced again')
         self.assertEqual(p1.phone, p1phone, 'Phone should be preserved after address sync')
@@ -411,7 +411,7 @@ class TestBase(TransactionCase):
         title_lady = self.env['res.partner.title'].create({'name': 'Lady...'})
         user_vals_list = [
             {'name': 'Alice', 'login': 'alice', 'color': 1, 'function': 'Friend', 'date': '2015-03-28', 'title': title_lady.id},
-            {'name': 'Alice', 'login': 'alice2', 'color': 0, 'function': 'Friend',  'date': '2015-01-28', 'title': title_lady.id},
+            {'name': 'Alice', 'login': 'alice2', 'color': 0, 'function': 'Friend', 'date': '2015-01-28', 'title': title_lady.id},
             {'name': 'Bob', 'login': 'bob', 'color': 2, 'function': 'Friend', 'date': '2015-03-02', 'title': title_sir.id},
             {'name': 'Eve', 'login': 'eve', 'color': 3, 'function': 'Eavesdropper', 'date': '2015-03-20', 'title': title_lady.id},
             {'name': 'Nab', 'login': 'nab', 'color': -3, 'function': '5$ Wrench', 'date': '2014-09-10', 'title': title_sir.id},
@@ -507,6 +507,113 @@ class TestBase(TransactionCase):
         self.assertEqual([g['title_count'] for g in groups_data], [2, 4], 'Incorrect number of results')
         self.assertEqual([g['color'] for g in groups_data], [-1, 10], 'Incorrect aggregation of int column')
 
+    def test_61_private_read_group(self):
+        """
+        the _read_group should behave exactly like read_group (public method) except for sorting the one2many on ID
+        instead of name, so avoiding the join on the "to many" table to get the name
+        """
+        title_sir = self.env['res.partner.title'].create({'name': 'Sir...'})
+        title_lady = self.env['res.partner.title'].create({'name': 'Lady...'})
+        user_vals_list = [
+            {'name': 'Alice', 'login': 'alice', 'color': 1, 'function': 'Friend', 'date': '2015-03-28', 'title': title_lady.id},
+            {'name': 'Alice', 'login': 'alice2', 'color': 0, 'function': 'Friend', 'date': '2015-01-28', 'title': title_lady.id},
+            {'name': 'Bob', 'login': 'bob', 'color': 2, 'function': 'Friend', 'date': '2015-03-02', 'title': title_sir.id},
+            {'name': 'Eve', 'login': 'eve', 'color': 3, 'function': 'Eavesdropper', 'date': '2015-03-20', 'title': title_lady.id},
+            {'name': 'Nab', 'login': 'nab', 'color': -3, 'function': '5$ Wrench', 'date': '2014-09-10', 'title': title_sir.id},
+            {'name': 'Nab', 'login': 'nab-she', 'color': 6, 'function': '5$ Wrench', 'date': '2014-01-02', 'title': title_lady.id},
+        ]
+        res_users = self.env['res.users']
+        users = res_users.create(user_vals_list)
+        domain = [('id', 'in', users.ids)]
+
+        # group on local char field without domain and without active_test (-> empty WHERE clause)
+        groups_data = res_users.with_context(active_test=False)._read_group([], fields=['login'], groupby=['login'], orderby='login DESC')
+        self.assertGreater(len(groups_data), 6, "Incorrect number of results when grouping on a field")
+
+        # group on local char field with limit
+        groups_data = res_users._read_group(domain, fields=['login'], groupby=['login'], orderby='login DESC', limit=3, offset=3)
+        self.assertEqual(len(groups_data), 3, "Incorrect number of results when grouping on a field with limit")
+        self.assertEqual(['bob', 'alice2', 'alice'], [g['login'] for g in groups_data], 'Result mismatch')
+
+        # group on inherited char field, aggregate on int field (second groupby ignored on purpose)
+        groups_data = res_users._read_group(domain, fields=['name', 'color', 'function'], groupby=['function', 'login'])
+        self.assertEqual(len(groups_data), 3, "Incorrect number of results when grouping on a field")
+        self.assertEqual([g['function'] for g in groups_data], ['5$ Wrench', 'Eavesdropper', 'Friend'], 'incorrect _read_group order')
+        for group_data in groups_data:
+            self.assertIn('color', group_data, "Aggregated data for the column 'color' is not present in _read_group return values")
+            self.assertEqual(group_data['color'], 3, "Incorrect sum for aggregated data for the column 'color'")
+
+        # group on inherited char field, reverse order
+        groups_data = res_users._read_group(domain, fields=['name', 'color'], groupby='name', orderby='name DESC')
+        self.assertEqual([g['name'] for g in groups_data], ['Nab', 'Eve', 'Bob', 'Alice'], 'Incorrect ordering of the list')
+
+        # group on int field, default ordering
+        groups_data = res_users._read_group(domain, fields=['color'], groupby='color')
+        self.assertEqual([g['color'] for g in groups_data], [-3, 0, 1, 2, 3, 6], 'Incorrect ordering of the list')
+
+        # multi group, second level is int field, should still be summed in first level grouping
+        groups_data = res_users._read_group(domain, fields=['name', 'color'], groupby=['name', 'color'], orderby='name DESC')
+        self.assertEqual([g['name'] for g in groups_data], ['Nab', 'Eve', 'Bob', 'Alice'], 'Incorrect ordering of the list')
+        self.assertEqual([g['color'] for g in groups_data], [3, 3, 2, 1], 'Incorrect ordering of the list')
+
+        # group on inherited char field, multiple orders with directions
+        groups_data = res_users._read_group(domain, fields=['name', 'color'], groupby='name', orderby='color DESC, name')
+        self.assertEqual(len(groups_data), 4, "Incorrect number of results when grouping on a field")
+        self.assertEqual([g['name'] for g in groups_data], ['Eve', 'Nab', 'Bob', 'Alice'], 'Incorrect ordering of the list')
+        self.assertEqual([g['name_count'] for g in groups_data], [1, 2, 1, 2], 'Incorrect number of results')
+
+        # group on inherited date column (res_partner.date) -> Year-Month, default ordering
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'date'], groupby=['date'])
+        self.assertEqual(len(groups_data), 4, "Incorrect number of results when grouping on a field")
+        self.assertEqual([g['date'] for g in groups_data], ['January 2014', 'September 2014', 'January 2015', 'March 2015'], 'Incorrect ordering of the list')
+        self.assertEqual([g['date_count'] for g in groups_data], [1, 1, 1, 3], 'Incorrect number of results')
+
+        # group on inherited date column (res_partner.date) specifying the :year -> Year default ordering
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'date'], groupby=['date:year'])
+        self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
+        self.assertEqual([g['date:year'] for g in groups_data], ['2014', '2015'], 'Incorrect ordering of the list')
+        self.assertEqual([g['date_count'] for g in groups_data], [2, 4], 'Incorrect number of results')
+
+        # group on inherited date column (res_partner.date) -> Year-Month, custom order
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'date'], groupby=['date'], orderby='date DESC')
+        self.assertEqual(len(groups_data), 4, "Incorrect number of results when grouping on a field")
+        self.assertEqual([g['date'] for g in groups_data], ['March 2015', 'January 2015', 'September 2014', 'January 2014'], 'Incorrect ordering of the list')
+        self.assertEqual([g['date_count'] for g in groups_data], [3, 1, 1, 1], 'Incorrect number of results')
+
+        # group on inherited many2one (res_partner.title), default order
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'title'], groupby=['title'])
+        self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
+        # m2o is returned as a (id, label) pair
+        # here the order of the titles is by ID
+        self.assertEqual([g['title'] for g in groups_data], [(title_sir.id, 'Sir...'), (title_lady.id, 'Lady...')], 'Incorrect ordering of the list')
+        self.assertEqual([g['title_count'] for g in groups_data], [2, 4], 'Incorrect number of results')
+        self.assertEqual([g['color'] for g in groups_data], [-1, 10], 'Incorrect aggregation of int column')
+
+        # group on inherited many2one (res_partner.title), reversed natural order
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'title'], groupby=['title'], orderby="title desc")
+        self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
+        # m2o is returned as a (id, label) pair
+        # here the order of the titles is by ID DESC
+        self.assertEqual([g['title'] for g in groups_data], [(title_sir.id, 'Sir...'), (title_lady.id, 'Lady...')], 'Incorrect ordering of the list')
+        self.assertEqual([g['title_count'] for g in groups_data], [2, 4], 'Incorrect number of results')
+        self.assertEqual([g['color'] for g in groups_data], [-1, 10], 'Incorrect aggregation of int column')
+
+        # group on inherited many2one (res_partner.title), multiple orders with m2o in second position
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'title'], groupby=['title'], orderby="color desc, title desc")
+        self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
+        # m2o is returned as a (id, label) pair
+        self.assertEqual([g['title'] for g in groups_data], [(title_lady.id, 'Lady...'), (title_sir.id, 'Sir...')], 'Incorrect ordering of the list')
+        self.assertEqual([g['title_count'] for g in groups_data], [4, 2], 'Incorrect number of results')
+        self.assertEqual([g['color'] for g in groups_data], [10, -1], 'Incorrect aggregation of int column')
+
+        # group on inherited many2one (res_partner.title), ordered by other inherited field (color)
+        groups_data = res_users._read_group(domain, fields=['function', 'color', 'title'], groupby=['title'], orderby='color')
+        self.assertEqual(len(groups_data), 2, "Incorrect number of results when grouping on a field")
+        # m2o is returned as a (id, label) pair
+        self.assertEqual([g['title'] for g in groups_data], [(title_sir.id, 'Sir...'), (title_lady.id, 'Lady...')], 'Incorrect ordering of the list')
+        self.assertEqual([g['title_count'] for g in groups_data], [2, 4], 'Incorrect number of results')
+        self.assertEqual([g['color'] for g in groups_data], [-1, 10], 'Incorrect aggregation of int column')
+
     def test_70_archive_internal_partners(self):
         test_partner = self.env['res.partner'].create({'name':'test partner'})
         test_user = self.env['res.users'].create({
diff --git a/odoo/models.py b/odoo/models.py
index 221fecc9913..87b6c0d3196 100644
--- a/odoo/models.py
+++ b/odoo/models.py
@@ -68,7 +68,7 @@ from .tools.lru import LRU
 _logger = logging.getLogger(__name__)
 _unlink = logging.getLogger(__name__ + '.unlink')
 
-regex_order = re.compile(r'^(\s*([a-z0-9:_]+|"[a-z0-9:_]+")(\s+(desc|asc))?\s*(,|$))+(?<!,)$', re.I)
+regex_order = re.compile(r'^(\s*([a-z0-9:_]+|"[a-z0-9:_]+")(\.id)?(\s+(desc|asc))?\s*(,|$))+(?<!,)$', re.I)
 regex_object_name = re.compile(r'^[a-z0-9_.]+$')
 regex_pg_name = re.compile(r'^[a-z_][a-z0-9_$]*$', re.I)
 regex_field_agg = re.compile(r'(\w+)(?::(\w+)(?:\((\w+)\))?)?')
@@ -2246,11 +2246,16 @@ class BaseModel(metaclass=MetaModel):
             for key in ('field', 'groupby')
         }
         for order_part in orderby.split(','):
-            order_split = order_part.split()
+            order_split = order_part.split()  # potentially ["field:group_func", "desc"]
             order_field = order_split[0]
+            is_many2one_id = order_field.endswith(".id")
+            if is_many2one_id:
+                order_field = order_field[:-3]
             if order_field == 'id' or order_field in groupby_fields:
-                if self._fields[order_field.split(':')[0]].type == 'many2one':
-                    order_clause = self._generate_order_by(order_part, query).replace('ORDER BY ', '')
+                order_field_name = order_field.split(':')[0]
+                if self._fields[order_field_name].type == 'many2one' and not is_many2one_id:
+                    order_clause = self._generate_order_by(order_part, query)
+                    order_clause = order_clause.replace('ORDER BY ', '')
                     if order_clause:
                         orderby_terms.append(order_clause)
                         groupby_terms += [order_term.split()[0] for order_term in order_clause.split(',')]
@@ -2413,6 +2418,25 @@ class BaseModel(metaclass=MetaModel):
         del data['id']
         return data
 
+    @api.model
+    def _read_group(self, domain, fields, groupby, offset=0, limit=None, orderby=False, lazy=True):
+        """
+        Executes exactly what the public read_group() does, except it doesn't
+        order many2one fields on their comodel's order but on their ID instead.
+        """
+        if not orderby:
+            if isinstance(groupby, str):
+                groupby = [groupby]
+            groupby_list = groupby[:1] if lazy else groupby
+            order_list = []
+            for order_spec in groupby_list:
+                field_name = order_spec.split(":")[0]  # field name could be formatted like "field:group_func"
+                if self._fields[field_name].type == 'many2one':
+                    order_spec = f"{field_name}.id"  # do not order by comodel's order
+                order_list.append(order_spec)
+            orderby = ','.join(order_list)
+        return self.read_group(domain, fields, groupby, offset, limit, orderby, lazy)
+
     @api.model
     def read_group(self, domain, fields, groupby, offset=0, limit=None, orderby=False, lazy=True):
         """Get the list of records in list view grouped by the given ``groupby`` fields.