From ef709fd92b7b22a58e60715c9ff7ab4476a83ca5 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 18 Jun 2020 13:22:23 +0000 Subject: [PATCH] [IMP] core, base: avoid multiple xids when importing records w/ inherits Before this change, we create an xid for every parent of a record being imported, regardless of whether it already has an xid, or if it's being created implicitly through the child. This generates unnecessary extra xids on pre-existing objects e.g. update 5 product variants -> the product gets 5 new xids despite already having one. We should *only* set a xid on parent records which are being implicitly created by the creation of a child with a specified xid. That is, we should never set a xid on the parent if it exists before the child is created. Update _process_end to try and see if "non-loaded" xids correspond to an automatically generated "parent" xid: we're still setting a xid on implicitly created records (if the child is created with a xid) so they're properly removed if e.g. the module is uninstalled, but because we're only doing so at creation these xids will not be visited during update and _process_end will try to delete them. A special case can be added to check that "unknown" parent xids don't have children which _inherit them, in which case we want to protect them. Task 2251039 closes odoo/odoo#53283 Related: odoo/enterprise#12023 Signed-off-by: Xavier Morel (xmo) --- addons/fleet/data/fleet_demo.xml | 2 +- odoo/addons/base/models/ir_model.py | 97 +++++++------ odoo/addons/base/tests/test_ir_model.py | 17 +-- odoo/addons/test_impex/ir.model.access.csv | 2 + odoo/addons/test_impex/models.py | 12 ++ odoo/addons/test_impex/tests/test_load.py | 156 +++++++++++++++++++++ odoo/models.py | 38 +++-- 7 files changed, 252 insertions(+), 72 deletions(-) diff --git a/addons/fleet/data/fleet_demo.xml b/addons/fleet/data/fleet_demo.xml index 30de2acc4d4..87f06a636be 100644 --- a/addons/fleet/data/fleet_demo.xml +++ b/addons/fleet/data/fleet_demo.xml @@ -463,7 +463,7 @@ Black Grand-Rosiere 5 - + kilometers diff --git a/odoo/addons/base/models/ir_model.py b/odoo/addons/base/models/ir_model.py index c766451d5c3..18f113d0cac 100644 --- a/odoo/addons/base/models/ir_model.py +++ b/odoo/addons/base/models/ir_model.py @@ -1970,12 +1970,6 @@ class IrModelData(models.Model): return result - def _generate_xmlids(self, xml_id, model): - """ Return all the XML ids to create for the given model. """ - yield xml_id - for parent_model in model._inherits: - yield '%s_%s' % (xml_id, parent_model.replace('.', '_')) - @api.model def _update_xmlids(self, data_list, update=False): """ Create or update the given XML ids. @@ -1992,14 +1986,6 @@ class IrModelData(models.Model): prefix, suffix = data['xml_id'].split('.', 1) record = data['record'] noupdate = bool(data.get('noupdate')) - # First create XML ids for parent records, then create XML id for - # record. The order reflects their actual creation order. This order - # is relevant for the uninstallation process: the record must be - # deleted before its parent records. - for parent_model, parent_field in record._inherits.items(): - parent = record[parent_field] - puffix = suffix + '_' + parent_model.replace('.', '_') - rows.add((prefix, puffix, parent._name, parent.id, noupdate)) rows.add((prefix, suffix, record._name, record.id, noupdate)) for sub_rows in self.env.cr.split_for_in_conditions(rows): @@ -2036,8 +2022,6 @@ class IrModelData(models.Model): record = self.xmlid_to_object(xml_id) if record: self.pool.loaded_xmlids.add(xml_id) - for parent_model, parent_field in record._inherits.items(): - self.pool.loaded_xmlids.add(xml_id + '_' + parent_model.replace('.', '_')) return record @api.model @@ -2176,32 +2160,63 @@ class IrModelData(models.Model): """ self._cr.execute(query, (tuple(modules), True)) for (id, xmlid, model, res_id) in self._cr.fetchall(): - if xmlid not in loaded_xmlids: - if model in self.env: - if self.search_count([ - ("model", "=", model), - ("res_id", "=", res_id), - ("id", "!=", id), - ("id", "not in", bad_imd_ids), - ]): - # another external id is still linked to the same record, only deleting the old imd - bad_imd_ids.append(id) - continue + if xmlid in loaded_xmlids: + continue - _logger.info('Deleting %s@%s (%s)', res_id, model, xmlid) - record = self.env[model].browse(res_id) - if record.exists(): - module = xmlid.split('.', 1)[0] - record = record.with_context(module=module) - if record._name == 'ir.model.fields' and not module.startswith('test_'): - _logger.warning( - "Deleting field %s.%s (hint: fields should be" - " explicitly removed by an upgrade script)", - record.model, record.name, - ) - record.unlink() - else: - bad_imd_ids.append(id) + Model = self.env.get(model) + if Model is None: + continue + + # when _inherits parents are implicitly created we give them an + # external id (if their descendant has one) in order to e.g. + # properly remove them when the module is deleted, however this + # generated id is *not* provided during update yet we don't want to + # try and remove either the xid or the record, so check if the + # record has a child we've just updated + keep = False + for inheriting in (self.env[m] for m in Model._inherits_children): + # ignore mixins + if inheriting._abstract: + continue + + parent_field = inheriting._inherits[model] + children = inheriting.with_context(active_test=False).search([(parent_field, '=', res_id)]) + children_xids = { + xid + for xids in (children and children._get_external_ids().values()) + for xid in xids + } + if children_xids & loaded_xmlids: + # at least one child was loaded + keep = True + break + if keep: + continue + + # if the record has other associated xids, only remove the xid + if self.search_count([ + ("model", "=", model), + ("res_id", "=", res_id), + ("id", "!=", id), + ("id", "not in", bad_imd_ids), + ]): + bad_imd_ids.append(id) + continue + + _logger.info('Deleting %s@%s (%s)', res_id, model, xmlid) + record = Model.browse(res_id) + if record.exists(): + module = xmlid.split('.', 1)[0] + record = record.with_context(module=module) + if record._name == 'ir.model.fields' and not module.startswith('test_'): + _logger.warning( + "Deleting field %s.%s (hint: fields should be" + " explicitly removed by an upgrade script)", + record.model, record.name, + ) + record.unlink() + else: + bad_imd_ids.append(id) if bad_imd_ids: self.browse(bad_imd_ids).unlink() diff --git a/odoo/addons/base/tests/test_ir_model.py b/odoo/addons/base/tests/test_ir_model.py index bfa6ea7fd5f..a46550fcf74 100644 --- a/odoo/addons/base/tests/test_ir_model.py +++ b/odoo/addons/base/tests/test_ir_model.py @@ -142,14 +142,15 @@ class TestXMLID(TransactionCase): def test_create_xmlids(self): # create users and assign them xml ids - foo, bar = self.env['res.users'].create([ - {'name': 'Foo', 'login': 'foo'}, - {'name': 'Bar', 'login': 'bar'}, - ]) - self.env['ir.model.data']._update_xmlids([ - dict(xml_id='test_convert.foo', record=foo, noupdate=True), - dict(xml_id='test_convert.bar', record=bar, noupdate=True), - ]) + foo, bar = self.env['res.users']._load_records([{ + 'xml_id': 'test_convert.foo', + 'values': {'name': 'Foo', 'login': 'foo'}, + 'noupdate': True, + }, { + 'xml_id': 'test_convert.bar', + 'values': {'name': 'Bar', 'login': 'bar'}, + 'noupdate': True, + }]) self.assertEqual(foo, self.env.ref('test_convert.foo', raise_if_not_found=False)) self.assertEqual(bar, self.env.ref('test_convert.bar', raise_if_not_found=False)) diff --git a/odoo/addons/test_impex/ir.model.access.csv b/odoo/addons/test_impex/ir.model.access.csv index fd0f7370c67..e051b726705 100644 --- a/odoo/addons/test_impex/ir.model.access.csv +++ b/odoo/addons/test_impex/ir.model.access.csv @@ -24,3 +24,5 @@ access_export_many2many_other,access_export_many2many_other,model_export_many2ma access_export_selection_withdefault,access_export_selection_withdefault,model_export_selection_withdefault,,1,1,1,1 access_export_one2many_recursive,access_export_one2many_recursive,model_export_one2many_recursive,,1,1,1,1 access_export_unique,access_export_unique,model_export_unique,,1,1,1,1 +access_export_inherits_parent,access_export_inherits_parent,model_export_inherits_parent,,1,1,1,1 +access_export_inherits_child,access_export_inherits_child,model_export_inherits_child,,1,1,1,1 diff --git a/odoo/addons/test_impex/models.py b/odoo/addons/test_impex/models.py index f5890aec480..ad75f7d43f4 100644 --- a/odoo/addons/test_impex/models.py +++ b/odoo/addons/test_impex/models.py @@ -164,3 +164,15 @@ class OnlyOne(models.Model): ('value_unique', 'unique (value)', "The value must be unique"), ('pair_unique', 'unique (value2, value3)', "The values must be unique"), ] + +class InheritsParent(models.Model): + _name = _description = 'export.inherits.parent' + + value_parent = fields.Integer() + +class InheritsChild(models.Model): + _name = _description = 'export.inherits.child' + _inherits = {'export.inherits.parent': 'parent_id'} + + parent_id = fields.Many2one('export.inherits.parent', required=True, ondelete='cascade') + value = fields.Integer() diff --git a/odoo/addons/test_impex/tests/test_load.py b/odoo/addons/test_impex/tests/test_load.py index 957d78e505c..1016cd741d5 100644 --- a/odoo/addons/test_impex/tests/test_load.py +++ b/odoo/addons/test_impex/tests/test_load.py @@ -1255,3 +1255,159 @@ class test_unique(ImporterCase): m.group(2).split(', '), ['Value2', 'Value3'] ) + +class test_inherits(ImporterCase): + """ The import process should only assign a new xid (derived from the + childs') if the child is being created and triggers the creation of the + parent + """ + model_name = 'export.inherits.child' + def test_create_no_parent(self): + r = self.import_(['id', 'value_parent', 'value'], [ + ['xxx.child', '0', '1'], + ]) + rec = self.env[self.model_name].browse(r['ids']) + + self.assertEqual(rec.value_parent, 0) + self.assertEqual(rec.value, 1) + self.assertEqual(rec.parent_id.value_parent, 0) + self.assertEqual( + rec._get_external_ids()[rec.id], + ['xxx.child'], + ) + self.assertEqual( + rec.parent_id._get_external_ids()[rec.parent_id.id], + ['xxx.child_export_inherits_parent'], + ) + + def test_create_parent_no_xid(self): + parent = self.env['export.inherits.parent'].create({'value_parent': 0}) + r = self.import_(['id', 'parent_id/.id', 'value'], [ + ['xxx.child', str(parent.id), '1'], + ]) + rec = self.env[self.model_name].browse(r['ids']) + self.assertEqual(rec.value_parent, 0) + self.assertEqual(rec.parent_id, parent) + + self.assertEqual( + rec._get_external_ids()[rec.id], + ['xxx.child'], + ) + self.assertEqual( + rec.parent_id._get_external_ids()[rec.parent_id.id], + [], + "no xid should be created for the parent" + ) + + def test_create_parent_with_xid(self): + parent = self.env['export.inherits.parent'].create({'value_parent': 0}) + pid = self.env['ir.model.data'].create({ + 'model': 'export.inherits.parent', + 'res_id': parent.id, + 'module': 'xxx', + 'name': 'parent', + }) + r = self.import_(['id', 'parent_id/.id', 'value'], [ + ['xxx.child', str(parent.id), '1'], + ]) + rec = self.env[self.model_name].browse(r['ids']) + self.assertEqual(rec.value_parent, 0) + self.assertEqual(rec.parent_id, parent) + self.assertTrue(pid.exists().res_id, parent.id) + + self.assertEqual( + rec._get_external_ids()[rec.id], + ['xxx.child'], + ) + self.assertEqual( + rec.parent_id._get_external_ids()[rec.parent_id.id], + ['xxx.parent'], + ) + + def test_create_parent_by_xid(self): + parent = self.env['export.inherits.parent'].create({'value_parent': 0}) + pid = self.env['ir.model.data'].create({ + 'model': 'export.inherits.parent', + 'res_id': parent.id, + 'module': 'xxx', + 'name': 'parent', + }) + r = self.import_(['id', 'parent_id/id', 'value'], [ + ['xxx.child', 'xxx.parent', '1'], + ]) + rec = self.env[self.model_name].browse(r['ids']) + self.assertEqual(rec.value_parent, 0) + self.assertEqual(rec.parent_id, parent) + self.assertTrue(pid.exists().res_id, parent.id) + + self.assertEqual( + rec._get_external_ids()[rec.id], + ['xxx.child'], + ) + self.assertEqual( + rec.parent_id._get_external_ids()[rec.parent_id.id], + ['xxx.parent'], + ) + + def test_update_parent_no_xid(self): + parent = self.env['export.inherits.parent'].create({'value_parent': 0}) + child = self.env[self.model_name].create({ + 'parent_id': parent.id, + 'value': 1, + }) + self.env['ir.model.data'].create({ + 'model': self.model_name, + 'res_id': child.id, + 'module': 'xxx', + 'name': 'child' + }) + + self.import_(['id', 'value'], [ + ['xxx.child', '42'] + ]) + self.assertEqual(child.value, 42) + self.assertEqual(child.parent_id, parent) + + self.assertEqual( + child._get_external_ids()[child.id], + ['xxx.child'], + ) + self.assertEqual( + parent._get_external_ids()[parent.id], + [], + ) + + def test_update_parent_with_xid(self): + parent = self.env['export.inherits.parent'].create({'value_parent': 0}) + child = self.env[self.model_name].create({ + 'parent_id': parent.id, + 'value': 1, + }) + pid, cid = self.env['ir.model.data'].create([{ + 'model': 'export.inherits.parent', + 'res_id': parent.id, + 'module': 'xxx', + 'name': 'parent', + }, { + 'model': self.model_name, + 'res_id': child.id, + 'module': 'xxx', + 'name': 'child' + }]) + + self.import_(['id', 'value'], [ + ['xxx.child', '42'] + ]) + self.assertEqual(child.value, 42) + self.assertEqual(child.parent_id, parent) + self.assertEqual(pid.exists().res_id, parent.id) + self.assertEqual(cid.exists().res_id, child.id) + + self.assertEqual( + child._get_external_ids()[child.id], + ['xxx.child'], + ) + self.assertEqual( + parent._get_external_ids()[parent.id], + ['xxx.parent'], + ) diff --git a/odoo/models.py b/odoo/models.py index a5371e03327..cf0cbf4c7c3 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -1047,7 +1047,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): for id, xid, record, info in converted: if xid: xid = xid if '.' in xid else "%s.%s" % (current_module, xid) - batch_xml_ids.update(ModelData._generate_xmlids(xid, self)) + batch_xml_ids.add(xid) elif id: record['id'] = id batch.append((xid, record, info)) @@ -4063,6 +4063,7 @@ Fields: # determine which records to create and update to_create = [] # list of data to_update = [] # list of data + imd_data_list = [] # list of data for _update_xmlids() for data in data_list: xml_id = data.get('xml_id') @@ -4080,11 +4081,11 @@ Fields: continue d_id, d_module, d_name, d_model, d_res_id, d_noupdate, r_id = row record = self.browse(d_res_id) - if update and d_noupdate: + if r_id: data['record'] = record - elif r_id: - data['record'] = record - to_update.append(data) + imd_data_list.append(data) + if not (update and d_noupdate): + to_update.append(data) else: imd.browse(d_id).unlink() to_create.append(data) @@ -4093,22 +4094,6 @@ Fields: for data in to_update: data['record']._load_records_write(data['values']) - # determine existing parents for new records - for parent_model, parent_field in self._inherits.items(): - suffix = '_' + parent_model.replace('.', '_') - xml_ids_vals = { - (data['xml_id'] + suffix): data['values'] - for data in to_create - if data.get('xml_id') - } - for row in imd._lookup_xmlids(xml_ids_vals, self.env[parent_model]): - d_id, d_module, d_name, d_model, d_res_id, d_noupdate, r_id = row - if r_id: - xml_id = '%s.%s' % (d_module, d_name) - xml_ids_vals[xml_id][parent_field] = r_id - else: - imd.browse(d_id).unlink() - # check for records to create with an XMLID from another module module = self.env.context.get('install_module') if module: @@ -4121,9 +4106,18 @@ Fields: records = self._load_records_create([data['values'] for data in to_create]) for data, record in zip(to_create, records): data['record'] = record + if data.get('xml_id'): + # add XML ids for parent records that have just been created + for parent_model, parent_field in self._inherits.items(): + if not data['values'].get(parent_field): + imd_data_list.append({ + 'xml_id': f"{data['xml_id']}_{parent_model.replace('.', '_')}", + 'record': record[parent_field], + 'noupdate': data.get('noupdate', False), + }) + imd_data_list.append(data) # create or update XMLIDs - imd_data_list = [data for data in data_list if data.get('xml_id')] imd._update_xmlids(imd_data_list, update) return original_self.concat(*(data['record'] for data in data_list))