[FIX] core,account: no join for grouping by m2o id

When using account.root (through account.account().root_id or
account.move.line().account_root_id) in read_group, we can have
inconsistent results because of how account.root is defined.

account_root is a view with the id field computed out of account codes,
but there can be the same account for several companies, so we can have
several same ID for different rows => this is not expected by the ORM
who expects one record by ID => in result, we get for example the values
in the pivot table of journal items be multiplied by the number of
companies if we group by "Account Root".

With this changeset, we add a small optimisation in ORM so if a group by
is ordered by a many2one, if the order of the many2one is "id" we don't
add a left join for ordering.

note: without the change, the added tests failed:

- in account, with 1000 as balance, and 2 as number of root with id=90090
- in test_read_group with a query containing a left join to o2m table
- in test_new_api with a query containing a left join to o2m table

opw-2282699
opw-2289440
opw-3288390

closes odoo/odoo#143257

X-original-commit: 674bdd1ac2cf9a4a3748b85f71ebbd01137485ce
Signed-off-by: Rémy Voet (ryv) <ryv@odoo.com>

addons/account/tests/test_account_move_entry.py

@@ -1093,3 +1093,40 @@ class TestAccountMove(AccountTestInvoicingCommon):
         move = move_form.save()
         tax_line = move.line_ids.filtered('tax_repartition_line_id')
         self.assertEqual(tax_line.debit, 721.43)
+
+    def test_account_root_multiple_companies(self):
+        account = self.env['account.account'].create({
+            'name': 'account',
+            'code': 'ZZ',
+            'account_type': 'asset_current',
+            'company_id': self.env.company.id,
+        })
+        other_company = self.env['res.company'].create({'name': 'other company'})
+        self.env['account.account'].create({
+            'name': 'other account',
+            'code': 'ZZ',
+            'account_type': 'asset_current',
+            'company_id': other_company.id,
+        })
+        self.env['account.move'].create({
+            'move_type': 'entry',
+            'date': fields.Date.from_string('2016-01-01'),
+            'line_ids': [
+                (0, None, {
+                    'name': 'revenue line 1',
+                    'account_id': account.id,
+                    'debit': 500.0,
+                    'credit': 0.0,
+                }),
+                (0, None, {
+                    'name': 'revenue line 1',
+                    'account_id': self.company_data['default_account_tax_sale'].id,
+                    'debit': 0.0,
+                    'credit': 500.0,
+                }),
+            ]
+        })
+        balance = self.env["account.move.line"].read_group(
+            [("account_id", "=", account.id)], ["balance:sum"], ["account_root_id"]
+        )[0]["balance"]
+        self.assertEqual(balance, 500)

odoo/addons/test_new_api/tests/test_new_fields.py

@@ -2265,6 +2265,16 @@ class TestFields(TransactionCaseWithUserDemo):
             [('author_partner.name', '=', 'Marc Demo')])
         self.assertEqual(messages, self.env.ref('test_new_api.message_0_1'))
 
+    def test_51_search_many2one_ordered(self):
+        """ test search on many2one ordered by id """
+        with self.assertQueries(['''
+            SELECT "test_new_api_message"."id" FROM "test_new_api_message"
+            WHERE ("test_new_api_message"."active" = %s)
+            ORDER BY  "test_new_api_message"."discussion"
+        ''']):
+            self.env['test_new_api.message'].search([], order='discussion')
+
+
     def test_60_one2many_domain(self):
         """ test the cache consistency of a one2many field with a domain """
         discussion = self.env.ref('test_new_api.discussion_0')

odoo/addons/test_read_group/tests/test_private_read_group.py

@@ -686,3 +686,20 @@ class TestPrivateReadGroup(common.TransactionCase):
                 (User, ["Donkey Kong"]),                            # tasks of nobody
             ],
         )
+
+    def test_order_by_many2one_id(self):
+        # ordering by a many2one ordered itself by id does not use useless join
+        expected_query = '''
+            SELECT "test_read_group_order_line"."order_id", COUNT(*)
+            FROM "test_read_group_order_line"
+            GROUP BY "test_read_group_order_line"."order_id"
+            ORDER BY "test_read_group_order_line"."order_id"
+        '''
+        with self.assertQueries([expected_query + ' ASC']):
+            self.env["test_read_group.order.line"].read_group(
+                [], ["order_id"], "order_id"
+            )
+        with self.assertQueries([expected_query + ' DESC']):
+            self.env["test_read_group.order.line"].read_group(
+                [], ["order_id"], "order_id", orderby="order_id DESC"
+            )

odoo/models.py

@@ -2078,7 +2078,10 @@ class BaseModel(metaclass=MetaModel):
                 continue
 
             field = self._fields.get(term)
-            if traverse_many2one and field and field.type == 'many2one':
+            if (
+                traverse_many2one and field and field.type == 'many2one'
+                and self.env[field.comodel_name]._order != 'id'
+            ):
                 # this generates an extra clause to add in the group by
                 sql_order = self._order_to_sql(f'{term} {direction} {nulls}', query)
                 orderby_terms.append(sql_order)
@@ -5188,6 +5191,17 @@ class BaseModel(metaclass=MetaModel):
                 return
             self = self.with_context(__m2o_order_seen=frozenset((field, *seen)))
 
+            # figure out the applicable order_by for the m2o
+            comodel = self.env[field.comodel_name]
+            coorder = comodel._order
+            if not regex_order.match(coorder):
+                # _order is complex, can't use it here, so we default to _rec_name
+                coorder = comodel._rec_name
+
+            if coorder == 'id':
+                sql_field = self._field_to_sql(alias, field_name, query)
+                return SQL("%s %s %s", sql_field, direction, nulls)
+
             # instead of ordering by the field's raw value, use the comodel's
             # order on many2one values
             terms = []
@@ -5197,7 +5211,6 @@ class BaseModel(metaclass=MetaModel):
                 terms.append(SQL("%s IS NULL", self._field_to_sql(alias, field_name, query)))
 
             # LEFT JOIN the comodel table, in order to include NULL values, too
-            comodel = self.env[field.comodel_name]
             coalias = query.make_alias(alias, field_name)
             query.add_join('LEFT JOIN', coalias, comodel._table, SQL(
                 "%s = %s",
@@ -5205,12 +5218,6 @@ class BaseModel(metaclass=MetaModel):
                 SQL.identifier(coalias, 'id'),
             ))
 
-            # figure out the applicable order_by for the m2o
-            coorder = comodel._order
-            if not regex_order.match(coorder):
-                # _order is complex, can't use it here, so we default to _rec_name
-                coorder = comodel._rec_name
-
             # delegate the order to the comodel
             reverse = direction.code == 'DESC'
             term = comodel._order_to_sql(coorder, query, alias=coalias, reverse=reverse)