From 7c7f927befa5f3d080f765147a98bb04df1333e2 Mon Sep 17 00:00:00 2001 From: Nicolas Lempereur Date: Thu, 4 May 2023 13:28:41 +0000 Subject: [PATCH] [FIX] core,account: no join for grouping by m2o id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../account/tests/test_account_move_entry.py | 37 +++++++++++++++++++ .../test_new_api/tests/test_new_fields.py | 10 +++++ .../tests/test_private_read_group.py | 17 +++++++++ odoo/models.py | 23 ++++++++---- 4 files changed, 79 insertions(+), 8 deletions(-) diff --git a/addons/account/tests/test_account_move_entry.py b/addons/account/tests/test_account_move_entry.py index ef4bcb17bbf..1bc06830fea 100644 --- a/addons/account/tests/test_account_move_entry.py +++ b/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) diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index 1c3e71dd137..556ada90124 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/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') diff --git a/odoo/addons/test_read_group/tests/test_private_read_group.py b/odoo/addons/test_read_group/tests/test_private_read_group.py index 85b948d6573..5750bdba9ab 100644 --- a/odoo/addons/test_read_group/tests/test_private_read_group.py +++ b/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" + ) diff --git a/odoo/models.py b/odoo/models.py index cf9cfd2dbff..d084926c51f 100644 --- a/odoo/models.py +++ b/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)