From 3ba36956426ee2d5338c3e459673eefc61b0ec77 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Tue, 22 Mar 2022 12:54:21 +0000 Subject: [PATCH] [FIX] core: avoid NULLs in subselects generated for one2many fields This fixes a problem that occurs with conditions on one2many fields, where most conditions are translated with a subquery like "model".id IN ( SELECT "comodel"."model_id" FROM "comodel" WHERE ... ) The issue occurs when the subquery return NULL values, i.e., when the field "model_id" in the subquery above can be NULL. The fix simply consists in adding the condition "model_id IS NOT NULL" in the WHERE clause of the subquery. The problem with NULL values returned by subqueries is the fact that they make the SQL condition undetermined instead of false, and this discards some of the results returned by a query. Simply consider: id NOT IN (1, 2, 3) The value id=3 makes the condition false, while i=4 makes it true. Now consider that the set also includes a NULL value, like: id NOT IN (1, 2, 3, NULL) The value id=3 makes the condition false, but the value id=4 makes it undetermined. Therefore this condition is actually undetermined for all possible values of id, and a query containing that condition simply returns nothing! closes odoo/odoo#87285 X-original-commit: 6d680bac0333c6c7ce9f056cba0e1f7e9fa22f9c Signed-off-by: Vincent Schippefilt (vsc) Signed-off-by: Raphael Collet --- odoo/addons/base/tests/test_expression.py | 26 ++++++++++++- .../test_new_api/tests/test_new_fields.py | 6 +-- odoo/osv/expression.py | 38 +++++++++++-------- 3 files changed, 51 insertions(+), 19 deletions(-) diff --git a/odoo/addons/base/tests/test_expression.py b/odoo/addons/base/tests/test_expression.py index 66a8170fd84..8923d1c8281 100644 --- a/odoo/addons/base/tests/test_expression.py +++ b/odoo/addons/base/tests/test_expression.py @@ -1496,7 +1496,7 @@ class TestOne2many(TransactionCase): SELECT "res_partner_bank"."partner_id" FROM "res_partner_bank" WHERE ("res_partner_bank"."sanitized_acc_number"::text LIKE %s) - )) + )) AND "res_partner"."parent_id" IS NOT NULL )) ORDER BY "res_partner"."display_name", "res_partner"."id" ''']): @@ -1637,6 +1637,30 @@ class TestOne2many(TransactionCase): ''']): self.Partner.search([('bank_ids', 'like', '12')]) + def test_empty(self): + self.Partner.search([('bank_ids', '!=', False)], order='id') + self.Partner.search([('bank_ids', '=', False)], order='id') + + with self.assertQueries([''' + SELECT "res_partner".id + FROM "res_partner" + WHERE ("res_partner"."id" IN ( + SELECT "partner_id" FROM "res_partner_bank" WHERE "partner_id" IS NOT NULL + )) + ORDER BY "res_partner"."id" + ''']): + self.Partner.search([('bank_ids', '!=', False)], order='id') + + with self.assertQueries([''' + SELECT "res_partner".id + FROM "res_partner" + WHERE ("res_partner"."id" NOT IN ( + SELECT "partner_id" FROM "res_partner_bank" WHERE "partner_id" IS NOT NULL + )) + ORDER BY "res_partner"."id" + ''']): + self.Partner.search([('bank_ids', '=', False)], order='id') + class TestMany2many(TransactionCase): def setUp(self): 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 2524a2e67f7..05e2ade2b32 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -2844,12 +2844,12 @@ class TestX2many(common.TransactionCase): result = recs.search([('id', 'in', recs.ids), ('lines', 'not in', [])]) self.assertEqual(result, recs) - # these cases are weird + # test 'not in' where the lines contain NULL values result = recs.search([('id', 'in', recs.ids), ('lines', 'not in', (line1 + line0).ids)]) - self.assertEqual(result, recs.browse()) + self.assertEqual(result, recs - recX) result = recs.search([('id', 'in', recs.ids), ('lines', 'not in', line0.ids)]) - self.assertEqual(result, recs.browse()) + self.assertEqual(result, recs) # special case: compare with False result = recs.search([('id', 'in', recs.ids), ('lines', '=', False)]) diff --git a/odoo/osv/expression.py b/odoo/osv/expression.py index 393e7fdfe9f..be9ce5f3de4 100644 --- a/odoo/osv/expression.py +++ b/odoo/osv/expression.py @@ -765,7 +765,8 @@ class expression(object): elif field.type == 'one2many': domain = field.get_domain_list(model) - inverse_is_int = comodel._fields[field.inverse_name].type in ('integer', 'many2one_reference') + inverse_field = comodel._fields[field.inverse_name] + inverse_is_int = inverse_field.type in ('integer', 'many2one_reference') unwrap_inverse = (lambda ids: ids) if inverse_is_int else (lambda recs: recs.ids) if right is not False: @@ -781,36 +782,43 @@ class expression(object): if inverse_is_int and domain: ids2 = comodel._search([('id', 'in', ids2)] + domain, order='id') - if isinstance(ids2, Query) and comodel._fields[field.inverse_name].store: - op1 = 'not inselect' if operator in NEGATIVE_TERM_OPERATORS else 'inselect' - subquery, subparams = ids2.subselect('"%s"."%s"' % (comodel._table, field.inverse_name)) - push(('id', op1, (subquery, subparams)), model, alias, internal=True) - elif ids2 and comodel._fields[field.inverse_name].store: - op1 = 'not inselect' if operator in NEGATIVE_TERM_OPERATORS else 'inselect' - subquery = 'SELECT "%s" FROM "%s" WHERE "id" IN %%s' % (field.inverse_name, comodel._table) - subparams = [tuple(ids2)] - push(('id', op1, (subquery, subparams)), model, alias, internal=True) + if inverse_field.store: + # In the condition, one must avoid subqueries to return + # NULL values, since it makes the IN test NULL instead + # of FALSE. This may discard expected results, as for + # instance "id NOT IN (42, NULL)" is never TRUE. + in_ = 'NOT IN' if operator in NEGATIVE_TERM_OPERATORS else 'IN' + if isinstance(ids2, Query): + if not inverse_field.required: + ids2.add_where(f'"{comodel._table}"."{inverse_field.name}" IS NOT NULL') + subquery, subparams = ids2.subselect(f'"{comodel._table}"."{inverse_field.name}"') + else: + subquery = f'SELECT "{inverse_field.name}" FROM "{comodel._table}" WHERE "id" IN %s' + if not inverse_field.required: + subquery += f' AND "{inverse_field.name}" IS NOT NULL' + subparams = [tuple(ids2) or (None,)] + push_result(f'("{alias}"."id" {in_} ({subquery}))', subparams) else: # determine ids1 in model related to ids2 recs = comodel.browse(ids2).sudo().with_context(prefetch_fields=False) - ids1 = unwrap_inverse(recs.mapped(field.inverse_name)) + ids1 = unwrap_inverse(recs.mapped(inverse_field.name)) # rewrite condition in terms of ids1 op1 = 'not in' if operator in NEGATIVE_TERM_OPERATORS else 'in' push(('id', op1, ids1), model, alias) else: - if comodel._fields[field.inverse_name].store and not (inverse_is_int and domain): + if inverse_field.store and not (inverse_is_int and domain): # rewrite condition to match records with/without lines op1 = 'inselect' if operator in NEGATIVE_TERM_OPERATORS else 'not inselect' - subquery = 'SELECT "%s" FROM "%s" where "%s" is not null' % (field.inverse_name, comodel._table, field.inverse_name) + subquery = f'SELECT "{inverse_field.name}" FROM "{comodel._table}" WHERE "{inverse_field.name}" IS NOT NULL' push(('id', op1, (subquery, [])), model, alias, internal=True) else: - comodel_domain = [(field.inverse_name, '!=', False)] + comodel_domain = [(inverse_field.name, '!=', False)] if inverse_is_int and domain: comodel_domain += domain recs = comodel.search(comodel_domain, order='id').sudo().with_context(prefetch_fields=False) # determine ids1 = records with lines - ids1 = unwrap_inverse(recs.mapped(field.inverse_name)) + ids1 = unwrap_inverse(recs.mapped(inverse_field.name)) # rewrite condition to match records with/without lines op1 = 'in' if operator in NEGATIVE_TERM_OPERATORS else 'not in' push(('id', op1, ids1), model, alias)