From 0787f150b18633cd9387fb80d2515ab9e53fcf9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Wed, 21 Sep 2022 14:56:28 +0000 Subject: [PATCH] [FIX] core: make index naming without conflicts There are two issues with the index naming convention used by the ORM: Problem 1: it is possible to have naming conflict for indexes. For instance, the name 'slide_channel_tag_group_sequence_index' is used for both fields slide.channel.tag.group.sequence and slide.channel.tag.group_sequence. Only the first index will be created. Solution 1: we separate the model and field names with a double underscore instead of a single one, which is the same strategy as with LEFT JOIN aliases. This is correct because model names don't contain such double underscores or underscores as prefix or suffix (it is not forbiden but model name should follow the 'dot notation'.) Problem 2: index names can be longer than 63 chars, but PostgreSQL silently truncates it. This doesn't actually break anything (PostgreSQL also truncates values when we check the existence of indexes) but it can lead to using the same name twice. There is hopefully not any example in our code. Solution 2: if the name is too large, we truncate it and pad it with a hash of the complete name to match 63 characters, which is also the strategy used for LEFT JOIN aliases. task-2984730 closes odoo/odoo#100736 Related: odoo/upgrade#3957 Signed-off-by: Raphael Collet --- .../test_new_api/models/test_new_api.py | 4 ++ odoo/addons/test_new_api/tests/test_schema.py | 37 +++++++++++++++++++ odoo/modules/registry.py | 6 +-- odoo/tools/query.py | 12 ++---- odoo/tools/sql.py | 20 +++++++++- 5 files changed, 65 insertions(+), 14 deletions(-) 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 51b3bad97f8..06bc99a571f 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -530,6 +530,7 @@ class Order(models.Model): _name = _description = 'test_new_api.order' line_ids = fields.One2many('test_new_api.order.line', 'order_id') + line_short_field_name = fields.Integer(index=True) class OrderLine(models.Model): @@ -538,6 +539,9 @@ class OrderLine(models.Model): order_id = fields.Many2one('test_new_api.order', required=True, ondelete='cascade') product = fields.Char() reward = fields.Boolean() + short_field_name = fields.Integer(index=True) + very_very_very_very_very_long_field_name_1 = fields.Integer(index=True) + very_very_very_very_very_long_field_name_2 = fields.Integer(index=True) def unlink(self): # also delete associated reward lines diff --git a/odoo/addons/test_new_api/tests/test_schema.py b/odoo/addons/test_new_api/tests/test_schema.py index 90bdc1836b4..fe648e74e96 100644 --- a/odoo/addons/test_new_api/tests/test_schema.py +++ b/odoo/addons/test_new_api/tests/test_schema.py @@ -577,3 +577,40 @@ class TestSchema(common.TransactionCase): (field.relation, field.column1, model._table, 'id', 'CASCADE'), (field.relation, field.column2, comodel._table, 'id', 'CASCADE'), ]) + + def test_20_unique_indexes(self): + """ Test uniqueness of indexes: + - test_new_api.order.line_short_field_name + - test_new_api.order.line.short_field_name + """ + tablenames = ('test_new_api_order', 'test_new_api_order_line') + self.env.cr.execute(""" + SELECT tablename + FROM pg_indexes + WHERE tablename IN %s AND indexdef LIKE %s + """, [tablenames, '%short_field_name%']) + tables = {table for table, in self.env.cr.fetchall()} + self.assertEqual(tables, {'test_new_api_order', 'test_new_api_order_line'}) + + def test_21_too_long_indexes(self): + """ Test too long indexes name: + + Both indexes share same truncated name + 'test_new_api_order_line__very_very_very_very_very_long_field_nam' + if no strategy is done to avoid duplicate too long index names + + - test_new_api.order.line.very_very_very_very_very_long_field_name_1 + -> test_new_api_order_line__very_very_very_very_very_long_field_name_1_index + => test_new_api_order_line__very_very_very_very_very_long_ea4b39c9 + + - test_new_api.order.line.very_very_very_very_very_long_field_name_2 + -> test_new_api_order_line__very_very_very_very_very_long_field_name_2_index + => test_new_api_order_line__very_very_very_very_very_long_dba32354 + """ + self.env.cr.execute(""" + SELECT COUNT(*) + FROM pg_indexes + WHERE tablename = 'test_new_api_order_line' AND indexdef LIKE %s + """, ['%very_very_very_very_long_field_name%']) + nb_field_index, = self.env.cr.fetchone() + self.assertEqual(nb_field_index, 2) diff --git a/odoo/modules/registry.py b/odoo/modules/registry.py index 74137c17dc2..dea7f3a6f76 100644 --- a/odoo/modules/registry.py +++ b/odoo/modules/registry.py @@ -479,8 +479,9 @@ class Registry(Mapping): def check_indexes(self, cr, model_names): """ Create or drop column indexes for the given models. """ + expected = [ - (f"{Model._table}_{field.name}_index", Model._table, field, getattr(field, 'unaccent', False)) + (sql.make_index_name(Model._table, field.name), Model._table, field, getattr(field, 'unaccent', False)) for model_name in model_names for Model in [self.models[model_name]] if Model._auto and not Model._abstract @@ -496,12 +497,11 @@ class Registry(Mapping): existing = dict(cr.fetchall()) for indexname, tablename, field, unaccent in expected: - column_expression = f'"{field.name}"' index = field.index assert index in ('btree', 'btree_not_null', 'trigram', True, False, None) if index and indexname not in existing and \ ((not field.translate and index != 'trigram') or (index == 'trigram' and self.has_trigram)): - + column_expression = f'"{field.name}"' if index == 'trigram': if field.translate: column_expression = f'''(jsonb_path_query_array({column_expression}, '$.*')::text)''' diff --git a/odoo/tools/query.py b/odoo/tools/query.py index 7fc4233e149..522d7c639e3 100644 --- a/odoo/tools/query.py +++ b/odoo/tools/query.py @@ -3,7 +3,8 @@ import re import warnings -from zlib import crc32 + +from odoo.tools.sql import make_identifier from .func import lazy_property @@ -37,14 +38,7 @@ def _generate_table_alias(src_table_alias, link): :param str link: field name :return str: alias """ - alias = "%s__%s" % (src_table_alias, link) - # Use an alternate alias scheme if length exceeds the PostgreSQL limit - # of 63 characters. - if len(alias) >= 64: - # We have to fit a crc32 hash and one underscore into a 63 character - # alias. The remaining space we can use to add a human readable prefix. - alias = "%s_%08x" % (alias[:54], crc32(alias.encode('utf-8'))) - return alias + return make_identifier(f"{src_table_alias}__{link}") class Query(object): diff --git a/odoo/tools/sql.py b/odoo/tools/sql.py index 99bfdf369a8..2d20fede732 100644 --- a/odoo/tools/sql.py +++ b/odoo/tools/sql.py @@ -3,15 +3,14 @@ # pylint: disable=sql-injection +from binascii import crc32 import logging import json import re import psycopg2 from psycopg2.sql import SQL, Identifier -import odoo.sql_db from collections import defaultdict -from contextlib import closing _schema = logging.getLogger('odoo.schema') @@ -421,3 +420,20 @@ def pattern_to_translated_trigram_pattern(pattern): # replace the original wildcard characters by % return f"%{'%'.join(wildcard_escaped)}%" if wildcard_escaped else "%" + + +def make_identifier(identifier: str) -> str: + """ Return ``identifier``, possibly modified to fit PostgreSQL's identifier size limitation. + If too long, ``identifier`` is truncated and padded with a hash to make it mostly unique. + """ + # if length exceeds the PostgreSQL limit of 63 characters. + if len(identifier) > 63: + # We have to fit a crc32 hash and one underscore into a 63 character + # alias. The remaining space we can use to add a human readable prefix. + return f"{identifier[:54]}_{crc32(identifier.encode()):08x}" + return identifier + + +def make_index_name(table_name: str, column_name: str) -> str: + """ Return an index name according to conventions for the given table and column. """ + return make_identifier(f"{table_name}__{column_name}_index")