From 6af689fd0a01a0f93050e93c2f0b37bb0636d0e6 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Mon, 22 Nov 2021 16:06:47 +0000 Subject: [PATCH] [FIX] base: fix search on company depend fields This is a bunch of fixes (65 corner cases) for the search on company-dependent fields: Without a default value: - `char` fields: - operators `not like`/`not ilike` don't return records with unset value - `(..., '=', False)` doesn't return records with unset value - `(..., '!=', '')` doesn't return records with unset value - `(..., 'in', [..., False])` doesn't return records with unset value - `(..., 'not in', value)` without `False` inside `value` doesn't return records with unset value - `date` and `datetime` fields: - `(..., '!=', )` doesn't return records with unset value - `(..., '=', False)` doesn't return records with unset value - `many2one` fields: - operators `not like`/`not ilike` don't return records with unset value - `(..., 'in', [..., False])` doesn't return records with unset value - `(..., 'not in', value)` without `False` inside `value` doesn't return records with unset value - `boolean` fields: - `(..., '=', False)` and `(..., '!=', True)` don't return records with unset and `False` values - `integer`/`float` fields: - `(..., '!=', )` doesn't return records with unset value With a truthy default value: - `many2one` fields: - operators `not like`/`not ilike` don't return records with unset value - `(..., '=', False)` returns the record with the default value (which isn't `False`) - `boolean` fields: - `(..., '=', False)` doesn't return records with unset and `False` value - `(..., '!=', False)` returns records with unset and `False` value - `integer`/`float` fields: - all `(..., operator, value)` which include value 0, return all records with unset value, even if the default value does not satisfy the domain closes odoo/odoo#80994 X-original-commit: 3e3be652ece83420782070bdb13da2c3a4930046 Signed-off-by: Raphael Collet --- odoo/addons/base/models/ir_property.py | 81 ++++++------ .../test_new_api/models/test_new_api.py | 3 + .../test_new_api/tests/test_new_fields.py | 125 ++++++++++++++++++ 3 files changed, 172 insertions(+), 37 deletions(-) diff --git a/odoo/addons/base/models/ir_property.py b/odoo/addons/base/models/ir_property.py index cc2aa3cf4fb..525751302d2 100644 --- a/odoo/addons/base/models/ir_property.py +++ b/odoo/addons/base/models/ir_property.py @@ -3,6 +3,7 @@ from odoo import api, fields, models, _ from odoo.exceptions import UserError +from odoo.osv.expression import TERM_OPERATORS_NEGATION from odoo.tools import ormcache TYPE2FIELD = { @@ -381,55 +382,61 @@ class Property(models.Model): def search_multi(self, name, model, operator, value): """ Return a domain for the records that match the given condition. """ default_matches = False - include_zero = False + negate = False + + # For "is set" and "is not set", same logic for all types + if operator == 'in' and False in value: + operator = 'not in' + negate = True + elif operator == 'not in' and False not in value: + operator = 'in' + negate = True + elif operator in ('!=', 'not like', 'not ilike') and value: + operator = TERM_OPERATORS_NEGATION[operator] + negate = True + elif operator == '=' and not value: + operator = '!=' + negate = True field = self.env[model]._fields[name] + if field.type == 'many2one': - comodel = field.comodel_name def makeref(value): - return value and '%s,%s' % (comodel, value) - if operator == "=": - value = makeref(value) - # if searching properties not set, search those not in those set - if value is False: - default_matches = True - elif operator in ('!=', '<=', '<', '>', '>='): + return value and f'{field.comodel_name},{value}' + + if operator in ('=', '!=', '<=', '<', '>', '>='): value = makeref(value) elif operator in ('in', 'not in'): value = [makeref(v) for v in value] elif operator in ('=like', '=ilike', 'like', 'not like', 'ilike', 'not ilike'): # most probably inefficient... but correct - target = self.env[comodel] + target = self.env[field.comodel_name] target_names = target.name_search(value, operator=operator, limit=None) target_ids = [n[0] for n in target_names] operator, value = 'in', [makeref(v) for v in target_ids] + elif field.type in ('integer', 'float'): # No record is created in ir.property if the field's type is float or integer with a value # equal to 0. Then to match with the records that are linked to a property field equal to 0, # the negation of the operator must be taken to compute the goods and the domain returned # to match the searched records is just the opposite. - if value == 0 and operator == '=': - operator = '!=' - include_zero = True - elif value <= 0 and operator == '>=': + value = float(value) if field.type == 'float' else int(value) + if operator == '>=' and value <= 0: operator = '<' - include_zero = True - elif value < 0 and operator == '>': + negate = True + elif operator == '>' and value < 0: operator = '<=' - include_zero = True - elif value >= 0 and operator == '<=': + negate = True + elif operator == '<=' and value >= 0: operator = '>' - include_zero = True - elif value > 0 and operator == '<': + negate = True + elif operator == '<' and value > 0: operator = '>=' - include_zero = True + negate = True + elif field.type == 'boolean': - if not value and operator == '=': - operator = '!=' - include_zero = True - elif value and operator == '!=': - operator = '=' - include_zero = True + # the value must be mapped to an integer value + value = int(value) # retrieve the properties that match the condition domain = self._get_domain(name, model) @@ -441,21 +448,21 @@ class Property(models.Model): good_ids = [] for prop in props: if prop.res_id: - res_model, res_id = prop.res_id.split(',') + __, res_id = prop.res_id.split(',') good_ids.append(int(res_id)) else: default_matches = True - if include_zero: - return [('id', 'not in', good_ids)] - elif default_matches: + if default_matches: # exclude all records with a property that does not match - all_ids = [] props = self.search(domain + [('res_id', '!=', False)]) - for prop in props: - res_model, res_id = prop.res_id.split(',') - all_ids.append(int(res_id)) - bad_ids = list(set(all_ids) - set(good_ids)) - return [('id', 'not in', bad_ids)] + all_ids = {int(res_id.split(',')[1]) for res_id in props.mapped('res_id')} + bad_ids = list(all_ids - set(good_ids)) + if negate: + return [('id', 'in', bad_ids)] + else: + return [('id', 'not in', bad_ids)] + elif negate: + return [('id', 'not in', good_ids)] else: return [('id', 'in', good_ids)] diff --git a/odoo/addons/test_new_api/models/test_new_api.py b/odoo/addons/test_new_api/models/test_new_api.py index a00b6b23dca..ec5b2091012 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -533,6 +533,9 @@ class CompanyDependent(models.Model): date = fields.Date(company_dependent=True) moment = fields.Datetime(company_dependent=True) tag_id = fields.Many2one('test_new_api.multi.tag', company_dependent=True) + truth = fields.Boolean(company_dependent=True) + count = fields.Integer(company_dependent=True) + phi = fields.Float(company_dependent=True, digits=(2, 5)) class CompanyDependentAttribute(models.Model): 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 9f606aef959..48444cbdadb 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -1427,6 +1427,131 @@ class TestFields(TransactionCaseWithUserDemo): company_records = self.env['test_new_api.company'].search([('foo', '=', 'DEF')]) self.assertEqual(len(company_records), 1) + def test_28_company_dependent_search(self): + """ Test the search on company-dependent fields in all corner cases. + This assumes that filtered_domain() correctly filters records when + its domain refers to company-dependent fields. + """ + Property = self.env['ir.property'] + Model = self.env['test_new_api.company'] + + # create 4 records for all cases: two with explicit truthy values, one + # with an explicit falsy value, and one without an explicit value + records = Model.create([{}] * 4) + + # For each field, we assign values to the records, and test a number of + # searches. The search cases are given by comparison operators, and for + # each operator, we test a number of possible operands. Every search() + # returns a subset of the records, and we compare it to an equivalent + # search performed by filtered_domain(). + + def test_field(field_name, truthy_values, operations): + # set ir.properties to all records except the last one + Property._set_multi( + field_name, Model._name, + {rec.id: val for rec, val in zip(records, truthy_values + [False])}, + # Using this sentinel for 'default_value' forces the method to + # create 'ir.property' records for the value False. Without it, + # no property would be created because False is the default + # value. + default_value=object(), + ) + + # test without default value + test_cases(field_name, operations) + + # set default value to False + Property._set_default(field_name, Model._name, False) + Property.flush() + Property.invalidate_cache() + test_cases(field_name, operations, False) + + # set default value to truthy_values[0] + Property._set_default(field_name, Model._name, truthy_values[0]) + Property.flush() + Property.invalidate_cache() + test_cases(field_name, operations, truthy_values[0]) + + def test_cases(field_name, operations, default=None): + for operator, values in operations.items(): + for value in values: + domain = [(field_name, operator, value)] + with self.subTest(domain=domain, default=default): + search_result = Model.search([('id', 'in', records.ids)] + domain) + filter_result = records.filtered_domain(domain) + self.assertEqual( + search_result, filter_result, + f"Got values {[r[field_name] for r in search_result]} " + f"instead of {[r[field_name] for r in filter_result]}", + ) + + # boolean fields + test_field('truth', [True, True], { + '=': (True, False), + '!=': (True, False), + }) + # integer fields + test_field('count', [10, -2], { + '=': (10, -2, 0, False), + '!=': (10, -2, 0, False), + '<': (10, -2, 0), + '>=': (10, -2, 0), + '<=': (10, -2, 0), + '>': (10, -2, 0), + }) + # float fields + test_field('phi', [1.61803, -1], { + '=': (1.61803, -1, 0, False), + '!=': (1.61803, -1, 0, False), + '<': (1.61803, -1, 0), + '>=': (1.61803, -1, 0), + '<=': (1.61803, -1, 0), + '>': (1.61803, -1, 0), + }) + # char fields + test_field('foo', ['qwer', 'azer'], { + 'like': ('qwer', 'azer'), + 'ilike': ('qwer', 'azer'), + 'not like': ('qwer', 'azer'), + 'not ilike': ('qwer', 'azer'), + '=': ('qwer', 'azer', False), + '!=': ('qwer', 'azer', False), + 'not in': (['qwer', 'azer'], ['qwer', False], [False], []), + 'in': (['qwer', 'azer'], ['qwer', False], [False], []), + }) + # date fields + date1, date2 = date(2021, 11, 22), date(2021, 11, 23) + test_field('date', [date1, date2], { + '=': (date1, date2, False), + '!=': (date1, date2, False), + '<': (date1, date2), + '>=': (date1, date2), + '<=': (date1, date2), + '>': (date1, date2), + }) + # datetime fields + moment1, moment2 = datetime(2021, 11, 22), datetime(2021, 11, 23) + test_field('moment', [moment1, moment2], { + '=': (moment1, moment2, False), + '!=': (moment1, moment2, False), + '<': (moment1, moment2), + '>=': (moment1, moment2), + '<=': (moment1, moment2), + '>': (moment1, moment2), + }) + # many2one fields + tag1, tag2 = self.env['test_new_api.multi.tag'].create([{'name': 'one'}, {'name': 'two'}]) + test_field('tag_id', [tag1.id, tag2.id], { + 'like': (tag1.name, tag2.name), + 'ilike': (tag1.name, tag2.name), + 'not like': (tag1.name, tag2.name), + 'not ilike': (tag1.name, tag2.name), + '=': (tag1.id, tag2.id, False), + '!=': (tag1.id, tag2.id, False), + 'in': ([tag1.id, tag2.id], [tag2.id, False], [False], []), + 'not in': ([tag1.id, tag2.id], [tag2.id, False], [False], []), + }) + def test_30_read(self): """ test computed fields as returned by read(). """ discussion = self.env.ref('test_new_api.discussion_0')