[FIX] core: fix remove constraints at uninstall
This patch aims to fix multiple issues with the removal of table
constraints at module uninstall.
1. We cannot remove `ir.model.constraint` records before calling
`_module_data_uninstall` on them. Otherwise we either won't find them
when performing the search
`self.env['ir.model.constraint'].search([('module', 'in',
modules.ids)]` or, if we somehow keep the ids and use `browse`
instead, would get an error because `_module_data_uninstall` tries to
access field values of records already removed. Note, although not an
issue, the removal is redundant for non FK constraints since
`_model_data_uninstall` already unlinks the record.
2. When a constraint has a name longer than 63 characters (Postgres
default) we would fail the check for the existence of the constraint
since the names are truncated.
3. When checking for the presence of a constraint we assumed its type
would be `u` in `pg_constraint` because for us that means non FK
(i.e. not `f` type). That's incorrect since there are many more
types. Here we propose to handle `c,u,x` types.
For bullet 2 we use `tools.make_identifier` that hashes the name and
ensures it fits in the 63 chars limit.
Revert "[IMP] models: warn if constraint key len exceed 63"
The check from commit 823d9e10dc is no
longer needed since the name is ensured to fit length limit.
[IMP] code: improve uninstall tests
Perform extra checks for removal of SQL constraints. Note the test is
commented out in `__init__.py`. It can be uncommented locally for
testing. It's kept commented out to avoid random errors in runbot.
closes odoo/odoo#129084
X-original-commit: af288b7178c25261329dd85a2e64b9dd635cd9e1
Signed-off-by: Raphael Collet <rco@odoo.com>
This commit is contained in:
Alvaro Fuentes
@@ -1633,18 +1633,23 @@ class IrModelConstraint(models.Model):
|
||||
self._cr.execute(
|
||||
sql.SQL('ALTER TABLE {} DROP CONSTRAINT {}').format(
|
||||
sql.Identifier(table),
|
||||
sql.Identifier(name)
|
||||
sql.Identifier(name[:63])
|
||||
))
|
||||
_logger.info('Dropped FK CONSTRAINT %s@%s', name, data.model.model)
|
||||
|
||||
if typ == 'u':
|
||||
hname = tools.make_identifier(name)
|
||||
# test if constraint exists
|
||||
# Since type='u' means any "other" constraint, to avoid issues we limit to
|
||||
# 'c' -> check, 'u' -> unique, 'x' -> exclude constraints, effective leaving
|
||||
# out 'p' -> primary key and 'f' -> foreign key, constraints.
|
||||
# See: https://www.postgresql.org/docs/9.5/catalog-pg-constraint.html
|
||||
self._cr.execute("""SELECT 1 from pg_constraint cs JOIN pg_class cl ON (cs.conrelid = cl.oid)
|
||||
WHERE cs.contype=%s and cs.conname=%s and cl.relname=%s""",
|
||||
('u', name, table))
|
||||
WHERE cs.contype in ('c', 'u', 'x') and cs.conname=%s and cl.relname=%s""",
|
||||
(hname, table))
|
||||
if self._cr.fetchone():
|
||||
self._cr.execute(sql.SQL('ALTER TABLE {} DROP CONSTRAINT {}').format(
|
||||
sql.Identifier(table), sql.Identifier(name)))
|
||||
sql.Identifier(table), sql.Identifier(hname)))
|
||||
_logger.info('Dropped CONSTRAINT %s@%s', name, data.model.model)
|
||||
|
||||
self.unlink()
|
||||
@@ -2286,9 +2291,9 @@ class IrModelData(models.Model):
|
||||
modules._remove_copied_views()
|
||||
|
||||
# remove constraints
|
||||
delete(self.env['ir.model.constraint'].browse(unique(constraint_ids)))
|
||||
constraints = self.env['ir.model.constraint'].search([('module', 'in', modules.ids)])
|
||||
constraints._module_data_uninstall()
|
||||
delete(self.env['ir.model.constraint'].browse(unique(constraint_ids)))
|
||||
|
||||
# If we delete a selection field, and some of its values have ondelete='cascade',
|
||||
# we expect the records with that value to be deleted. If we delete the field first,
|
||||
|
||||
@@ -41,7 +41,7 @@ from . import test_res_config
|
||||
from . import test_res_lang
|
||||
from . import test_search
|
||||
from . import test_translate
|
||||
#import test_uninstall # loop
|
||||
# from . import test_uninstall # loop
|
||||
from . import test_user_has_group
|
||||
from . import test_views
|
||||
from . import test_xmlrpc
|
||||
|
||||
@@ -46,6 +46,17 @@ class TestUninstall(BaseCase):
|
||||
self.assertTrue(env['ir.model.data'].search([('module', '=', MODULE)]))
|
||||
self.assertTrue(env['ir.model.fields'].search([('model', '=', MODEL)]))
|
||||
|
||||
env.cr.execute(
|
||||
r"""
|
||||
SELECT conname
|
||||
FROM pg_constraint
|
||||
WHERE conrelid = 'res_users'::regclass
|
||||
AND conname LIKE 'res\_users\_test\_uninstall\_res\_user\_%'
|
||||
"""
|
||||
)
|
||||
existing_constraints = [r[0] for r in env.cr.fetchall()]
|
||||
self.assertTrue(len(existing_constraints) == 4, existing_constraints)
|
||||
|
||||
def test_02_uninstall(self):
|
||||
""" Check a few things showing the module is uninstalled. """
|
||||
with environment() as env:
|
||||
@@ -59,6 +70,17 @@ class TestUninstall(BaseCase):
|
||||
self.assertFalse(env['ir.model.data'].search([('module', '=', MODULE)]))
|
||||
self.assertFalse(env['ir.model.fields'].search([('model', '=', MODEL)]))
|
||||
|
||||
env.cr.execute(
|
||||
r"""
|
||||
SELECT conname
|
||||
FROM pg_constraint
|
||||
WHERE conrelid = 'res_users'::regclass
|
||||
AND conname LIKE 'res\_users\_test\_uninstall\_res\_user\_%'
|
||||
"""
|
||||
)
|
||||
remaining_constraints = [r[0] for r in env.cr.fetchall()]
|
||||
self.assertFalse(remaining_constraints)
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
|
||||
@@ -18,3 +18,13 @@ class test_uninstall_model(models.Model):
|
||||
_sql_constraints = [
|
||||
('name_uniq', 'unique (name)', 'Each name must be unique.'),
|
||||
]
|
||||
|
||||
class ResUsers(models.Model):
|
||||
_inherit = 'res.users'
|
||||
|
||||
_sql_constraints = [
|
||||
('test_uninstall_res_user_unique_constraint', 'unique (password)', 'Test uninstall unique constraint'),
|
||||
('test_uninstall_res_user_check_constraint', 'check (true)', 'Test uninstall check constraint'),
|
||||
('test_uninstall_res_user_exclude_constraint', 'exclude (password with =)', 'Test uninstall exclude constraint'),
|
||||
('test_uninstall_res_user_exclude_constraint_looooooooooooong_name', 'exclude (password with =)', 'Test uninstall exclude constraint'),
|
||||
]
|
||||
|
||||
+7
-9
@@ -795,15 +795,8 @@ class BaseModel(metaclass=MetaModel):
|
||||
for mname, fnames in base._depends.items():
|
||||
depends.setdefault(mname, []).extend(fnames)
|
||||
|
||||
for constraint in base._sql_constraints:
|
||||
constraint_key = constraint[0]
|
||||
if len(cls._table) + len(constraint_key) + 1 > 63:
|
||||
_logger.warning(
|
||||
'Constrains `%s` combined to model table will have more than 63 character '
|
||||
'and could be truncated leading to unexpected results',
|
||||
constraint_key
|
||||
)
|
||||
cls._sql_constraints[constraint_key] = constraint
|
||||
for cons in base._sql_constraints:
|
||||
cls._sql_constraints[cons[0]] = cons
|
||||
|
||||
cls._sql_constraints = list(cls._sql_constraints.values())
|
||||
|
||||
@@ -2852,6 +2845,11 @@ class BaseModel(metaclass=MetaModel):
|
||||
|
||||
for (key, definition, message) in self._sql_constraints:
|
||||
conname = '%s_%s' % (self._table, key)
|
||||
if len(conname) > 63:
|
||||
hashed_conname = tools.make_identifier(conname)
|
||||
_logger.info("Constraint name %r has more than 63 characters, internal PG identifier is %r", conname, hashed_conname)
|
||||
conname = hashed_conname
|
||||
|
||||
current_definition = tools.constraint_definition(cr, self._table, conname)
|
||||
if current_definition == definition:
|
||||
continue
|
||||
|
||||
Reference in New Issue
Block a user