From b8da632e6e8f008a630a4e43029cd7cbf4bf38a9 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 13 Sep 2021 13:25:08 +0000 Subject: [PATCH] [IMP] core, base: allow disabling unaccent on a per-field basis For searches, Odoo uses `unaccent` if it's available. On some technical fields this is completely unnecessary and precludes the use of indexes. This PR provides: * an opt-out (`unaccent = False`) on `String` and `Text` fields * a warning if `unaccent` is enabled on *parent_path* fields as their performance can be rather critical and not using the index is quite an issue (note: the check that `parent_path` fields have been moved outside of the check for their existence as we want to check that the field is declared and correctly configured in all cases, probably) Task 2627454 Part-of: odoo/odoo#76436 --- odoo/addons/base/tests/test_expression.py | 32 ++++++++++++++++++----- odoo/fields.py | 1 + odoo/models.py | 17 +++++++----- odoo/osv/expression.py | 24 ++++++++++------- 4 files changed, 51 insertions(+), 23 deletions(-) diff --git a/odoo/addons/base/tests/test_expression.py b/odoo/addons/base/tests/test_expression.py index ba91408cec7..84dc8e66b9a 100644 --- a/odoo/addons/base/tests/test_expression.py +++ b/odoo/addons/base/tests/test_expression.py @@ -1,5 +1,7 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +import unittest +from unittest.mock import patch import psycopg2 @@ -672,13 +674,29 @@ class TestExpression(SavepointCaseWithUserDemo): def test_accent(self): if not self.registry.has_unaccent: - return - Company = self.env['res.company'] - helene = Company.create({'name': u'Hélène'}) - self.assertEqual(helene, Company.search([('name','ilike','Helene')])) - self.assertEqual(helene, Company.search([('name','ilike','hélène')])) - self.assertNotIn(helene, Company.search([('name','not ilike','Helene')])) - self.assertNotIn(helene, Company.search([('name','not ilike','hélène')])) + raise unittest.SkipTest("unaccent not enabled") + + Model = self.env['res.partner.category'] + helen = Model.create({'name': 'Hélène'}) + self.assertEqual(helen, Model.search([('name', 'ilike', 'Helene')])) + self.assertEqual(helen, Model.search([('name', 'ilike', 'hélène')])) + self.assertNotIn(helen, Model.search([('name', 'not ilike', 'Helene')])) + self.assertNotIn(helen, Model.search([('name', 'not ilike', 'hélène')])) + + hermione, nicostratus = Model.create([ + {'name': 'Hermione', 'parent_id': helen.id}, + {'name': 'Nicostratus', 'parent_id': helen.id} + ]) + self.assertEqual(nicostratus.parent_path, f'{helen.id}/{nicostratus.id}/') + + with patch('odoo.osv.expression.get_unaccent_wrapper') as w: + w().side_effect = lambda x: x + rs = Model.search([('parent_path', 'like', f'{helen.id}/%')], order='id asc') + self.assertEqual(rs, helen | hermione | nicostratus) + # the result of `get_unaccent_wrapper()` is the wrapper and that's + # what should not be called + w().assert_not_called() + def test_pure_function(self): orig_false = expression.FALSE_DOMAIN.copy() diff --git a/odoo/fields.py b/odoo/fields.py index c6b3ef25f76..db5b27f3a9d 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -1536,6 +1536,7 @@ class _String(Field): """ Abstract class for string fields. """ translate = False # whether the field is translated prefetch = None + unaccent = True def __init__(self, string=Default, **kwargs): # translate is either True, False, or a callable diff --git a/odoo/models.py b/odoo/models.py index 8c519fdc52f..451836f362c 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -2817,8 +2817,9 @@ class BaseModel(metaclass=MetaModel): if self._parent_store: if not tools.column_exists(cr, self._table, 'parent_path'): - self._create_parent_columns() + tools.create_column(self._cr, self._table, 'parent_path', 'VARCHAR') parent_path_compute = True + self._check_parent_path() if not must_create_table: self._check_removed_columns(log=False) @@ -2861,12 +2862,14 @@ class BaseModel(metaclass=MetaModel): """ pass - def _create_parent_columns(self): - tools.create_column(self._cr, self._table, 'parent_path', 'VARCHAR') - if 'parent_path' not in self._fields: - _logger.error("add a field parent_path on model %s: parent_path = fields.Char(index=True)", self._name) - elif not self._fields['parent_path'].index: - _logger.error('parent_path field on model %s must be indexed! Add index=True to the field definition)', self._name) + def _check_parent_path(self): + field = self._fields.get('parent_path') + if field is None: + _logger.error("add a field parent_path on model %r: `parent_path = fields.Char(index=True, unaccent=False)`.", self._name) + elif not field.index: + _logger.error('parent_path field on model %r should be indexed! Add index=True to the field definition.', self._name) + elif field.unaccent: + _logger.warning("parent_path field on model %r should have unaccent disabled. Add `unaccent=False` to the field definition.", self._name) def _add_sql_constraints(self): """ diff --git a/odoo/osv/expression.py b/odoo/osv/expression.py index 7b83a59f6bc..ddbf68f4a4f 100644 --- a/odoo/osv/expression.py +++ b/odoo/osv/expression.py @@ -436,7 +436,7 @@ class expression(object): :attr result: the result of the parsing, as a pair (query, params) :attr query: Query object holding the final result """ - self._unaccent = get_unaccent_wrapper(model._cr) + self._unaccent_wrapper = get_unaccent_wrapper(model._cr) self.root_model = model self.root_alias = alias or model._table @@ -449,6 +449,11 @@ class expression(object): # parse the domain expression self.parse() + def _unaccent(self, field, _id=lambda x: x): + if getattr(field, 'unaccent', False): + return self._unaccent_wrapper + return _id + # ---------------------------------------- # Leafs management # ---------------------------------------- @@ -930,7 +935,7 @@ class expression(object): if sql_operator in ('in', 'not in'): right = tuple(right) - unaccent = self._unaccent if sql_operator.endswith('like') else lambda x: x + unaccent = self._unaccent(field) if sql_operator.endswith('like') else lambda x: x left = unaccent(model._generate_translated_field(alias, left, self.query)) instr = unaccent('%s') @@ -1043,16 +1048,18 @@ class expression(object): query, params = self.__leaf_to_sql((left, '=', right), model, alias) else: + field = model._fields.get(left) + if field is None: + raise ValueError("Invalid field %r in domain term %r" % (left, leaf)) + need_wildcard = operator in ('like', 'ilike', 'not like', 'not ilike') sql_operator = {'=like': 'like', '=ilike': 'ilike'}.get(operator, operator) - cast = '::text' if sql_operator.endswith('like') else '' + cast = '::text' if sql_operator.endswith('like') else '' - if left not in model: - raise ValueError("Invalid field %r in domain term %r" % (left, leaf)) - format = '%s' if need_wildcard else model._fields[left].column_format - unaccent = self._unaccent if sql_operator.endswith('like') else lambda x: x + column_format = '%s' if need_wildcard else field.column_format + unaccent = self._unaccent(field) if sql_operator.endswith('like') else lambda x: x column = '%s.%s' % (table_alias, _quote(left)) - query = '(%s %s %s)' % (unaccent(column + cast), sql_operator, unaccent(format)) + query = '(%s %s %s)' % (unaccent(column + cast), sql_operator, unaccent(column_format)) if (need_wildcard and not right) or (right and operator in NEGATIVE_TERM_OPERATORS): query = '(%s OR %s."%s" IS NULL)' % (query, table_alias, left) @@ -1060,7 +1067,6 @@ class expression(object): if need_wildcard: params = ['%%%s%%' % pycompat.to_text(right)] else: - field = model._fields[left] params = [field.convert_to_column(right, model, validate=False)] return query, params