[FIX] util/records: deduplicate before batch update

src/base/tests/test_util.py

@@ -389,6 +389,62 @@ def test_adapt_domain_view(self):
         self.assertIn("courriel", view_search_2.arch)
 
 
+@unittest.skipUnless(
+    util.version_gte("13.0"), "This test is incompatible with old style odoo.addons.base.maintenance.migrations.util"
+)
+class TestReplaceReferences(UnitTestCase):
+    def setUp(self):
+        super().setUp()
+        self.env.cr.execute(
+            """
+            CREATE TABLE dummy_model(
+                             id serial PRIMARY KEY,
+                             res_id int,
+                             res_model varchar,
+                             extra varchar,
+                             CONSTRAINT uniq_constr UNIQUE(res_id, res_model, extra)
+                         );
+
+            INSERT INTO dummy_model(res_model, res_id, extra)
+                 VALUES -- the target is there with same res_id
+                        ('res.users', 1, 'x'),
+                        ('res.partner', 1, 'x'),
+
+                        -- two with same target and the target is there
+                        ('res.users', 2, 'x'),
+                        ('res.users', 3, 'x'),
+                        ('res.partner', 2, 'x'),
+
+                        -- two with same target and the target is not there
+                        ('res.users', 4, 'x'),
+                        ('res.users', 5, 'x'),
+
+                        -- target is there different res_id
+                        ('res.users', 6, 'x'),
+                        ('res.partner', 4, 'x')
+            """
+        )
+
+    def _ir_dummy(self, cr, bound_only=True):
+        yield util.IndirectReference("dummy_model", "res_model", "res_id")
+
+    def test_replace_record_references_batch__full_unique(self):
+        cr = self.env.cr
+        mapping = {1: 1, 2: 2, 3: 2, 4: 3, 5: 3, 6: 4}
+        with mock.patch("odoo.upgrade.util.records.indirect_references", self._ir_dummy):
+            util.replace_record_references_batch(cr, mapping, "res.users", "res.partner")
+
+        cr.execute("SELECT res_model, res_id, extra FROM dummy_model ORDER BY res_id, res_model")
+        data = cr.fetchall()
+        expected = [
+            ("res.partner", 1, "x"),
+            ("res.partner", 2, "x"),
+            ("res.partner", 3, "x"),
+            ("res.partner", 4, "x"),
+        ]
+        self.assertEqual(data, expected)
+
+
 class TestRemoveFieldDomains(UnitTestCase):
     @parametrize(
         [

src/util/indirect_references.py

@@ -29,80 +29,80 @@ def model_filter(self, prefix="", placeholder="%s"):
 # By default, there is no `res_id`, no `res_model_id` and it is deleted when the linked model is removed
 # warning: defaults are from the last fields in the namedtuple
 IndirectReference.__new__.__defaults__ = (None, None, False, None)  # https://stackoverflow.com/a/18348004
+_IR = IndirectReference
+
+INDIRECT_REFERENCES = [
+    _IR("ir_attachment", "res_model", "res_id"),
+    _IR("ir_cron", "model", None, set_unknown=True),
+    _IR("ir_act_report_xml", "model", None, set_unknown=True),
+    _IR("ir_act_window", "res_model", "res_id"),
+    _IR("ir_act_window", "res_model", None),
+    _IR("ir_act_window", "src_model", None),
+    _IR("ir_act_server", "wkf_model_name", None),
+    _IR("ir_act_server", "crud_model_name", None),
+    _IR("ir_act_server", "model_name", None, "model_id", set_unknown=True),
+    _IR("ir_act_client", "res_model", None, set_unknown=True),
+    _IR("ir_embedded_actions", "parent_res_model", "parent_res_id"),
+    _IR("ir_model", "model", None),
+    _IR("ir_model_fields", "model", None),
+    _IR("ir_model_fields", "relation", None),  # destination of a relation field
+    _IR("ir_model_data", "model", "res_id"),
+    _IR("ir_filters", "model_id", None, set_unknown=True),  # YUCK!, not an id
+    # duplicated for versions where the `res_id` column does not exists
+    _IR("ir_filters", "model_id", "embedded_parent_res_id"),
+    _IR("ir_exports", "resource", None),
+    _IR("ir_ui_view", "model", None, set_unknown=True),
+    _IR("ir_values", "model", "res_id"),
+    _IR("wkf_transition", "trigger_model", None),
+    _IR("wkf_triggers", "model", None),
+    _IR("ir_model_fields_anonymization", "model_name", None),
+    _IR("ir_model_fields_anonymization_migration_fix", "model_name", None),
+    _IR("base_import_mapping", "res_model", None),
+    _IR("calendar_event", "res_model", "res_id"),  # new in saas~18
+    _IR("data_cleaning_model", "res_model_name", None),
+    _IR("data_cleaning_record", "res_model_name", "res_id"),
+    _IR("data_cleaning_rule", "res_model_name", None),
+    _IR("data_merge_group", "res_model_name", None),
+    _IR("data_merge_model", "res_model_name", None),
+    _IR("data_merge_record", "res_model_name", "res_id"),
+    _IR("documents_document", "res_model", "res_id"),
+    _IR("email_template", "model", None, set_unknown=True),  # stored related
+    _IR("iap_extracted_words", "res_model", "res_id"),
+    _IR("mail_template", "model", None, set_unknown=True),  # model renamed in saas~6
+    _IR("mail_activity", "res_model", "res_id", "res_model_id"),
+    _IR("mail_activity_type", "res_model", None),
+    _IR("mail_alias", None, "alias_force_thread_id", "alias_model_id"),
+    _IR("mail_alias", None, "alias_parent_thread_id", "alias_parent_model_id"),
+    _IR("mail_followers", "res_model", "res_id"),
+    _IR("mail_message_subtype", "res_model", None),
+    _IR("mail_message", "model", "res_id"),
+    _IR("mail_compose_message", "model", "res_id"),
+    _IR("mail_wizard_invite", "res_model", "res_id"),
+    _IR("mail_mail_statistics", "model", "res_id"),
+    _IR("mailing_trace", "model", "res_id"),
+    _IR("mail_mass_mailing", "mailing_model", None, "mailing_model_id", set_unknown=True),
+    _IR("mailing_mailing", None, None, "mailing_model_id", set_unknown=True),
+    _IR("marketing_campaign", "model_name", None, set_unknown=True),  # stored related
+    _IR("marketing_participant", "model_name", "res_id", "model_id", set_unknown=True),
+    _IR("payment_transaction", None, "callback_res_id", "callback_model_id"),
+    _IR("project_project", "alias_model", None, set_unknown=True),
+    # IR("pos_blackbox_be_log", "model_name", None),  # ACTUALLY NOT. We need to keep records intact, even when renaming a model  # noqa: ERA001
+    _IR("quality_point", "worksheet_model_name", None),
+    _IR("rating_rating", "res_model", "res_id", "res_model_id"),
+    _IR("rating_rating", "parent_res_model", "parent_res_id", "parent_res_model_id"),
+    _IR("snailmail_letter", "model", "res_id", set_unknown=True),
+    _IR("sms_template", "model", None),
+    _IR("studio_approval_rule", "model_name", None),
+    _IR("spreadsheet_revision", "res_model", "res_id"),
+    _IR("studio_approval_entry", "model", "res_id"),
+    _IR("timer_timer", "res_model", "res_id"),
+    _IR("timer_timer", "parent_res_model", "parent_res_id"),
+    _IR("worksheet_template", "res_model", None),
+]
 
 
 def indirect_references(cr, bound_only=False):
-    IR = IndirectReference
-    each = [
-        IR("ir_attachment", "res_model", "res_id"),
-        IR("ir_cron", "model", None, set_unknown=True),
-        IR("ir_act_report_xml", "model", None, set_unknown=True),
-        IR("ir_act_window", "res_model", "res_id"),
-        IR("ir_act_window", "res_model", None),
-        IR("ir_act_window", "src_model", None),
-        IR("ir_act_server", "wkf_model_name", None),
-        IR("ir_act_server", "crud_model_name", None),
-        IR("ir_act_server", "model_name", None, "model_id", set_unknown=True),
-        IR("ir_act_client", "res_model", None, set_unknown=True),
-        IR("ir_embedded_actions", "parent_res_model", "parent_res_id"),
-        IR("ir_model", "model", None),
-        IR("ir_model_fields", "model", None),
-        IR("ir_model_fields", "relation", None),  # destination of a relation field
-        IR("ir_model_data", "model", "res_id"),
-        IR("ir_filters", "model_id", None, set_unknown=True),  # YUCK!, not an id
-        # duplicated for versions where the `res_id` column does not exists
-        IR("ir_filters", "model_id", "embedded_parent_res_id"),
-        IR("ir_exports", "resource", None),
-        IR("ir_ui_view", "model", None, set_unknown=True),
-        IR("ir_values", "model", "res_id"),
-        IR("wkf_transition", "trigger_model", None),
-        IR("wkf_triggers", "model", None),
-        IR("ir_model_fields_anonymization", "model_name", None),
-        IR("ir_model_fields_anonymization_migration_fix", "model_name", None),
-        IR("base_import_mapping", "res_model", None),
-        IR("calendar_event", "res_model", "res_id"),  # new in saas~18
-        IR("data_cleaning_model", "res_model_name", None),
-        IR("data_cleaning_record", "res_model_name", "res_id"),
-        IR("data_cleaning_rule", "res_model_name", None),
-        IR("data_merge_group", "res_model_name", None),
-        IR("data_merge_model", "res_model_name", None),
-        IR("data_merge_record", "res_model_name", "res_id"),
-        IR("documents_document", "res_model", "res_id"),
-        IR("email_template", "model", None, set_unknown=True),  # stored related
-        IR("iap_extracted_words", "res_model", "res_id"),
-        IR("mail_template", "model", None, set_unknown=True),  # model renamed in saas~6
-        IR("mail_activity", "res_model", "res_id", "res_model_id"),
-        IR("mail_activity_type", "res_model", None),
-        IR("mail_alias", None, "alias_force_thread_id", "alias_model_id"),
-        IR("mail_alias", None, "alias_parent_thread_id", "alias_parent_model_id"),
-        IR("mail_followers", "res_model", "res_id"),
-        IR("mail_message_subtype", "res_model", None),
-        IR("mail_message", "model", "res_id"),
-        IR("mail_compose_message", "model", "res_id"),
-        IR("mail_wizard_invite", "res_model", "res_id"),
-        IR("mail_mail_statistics", "model", "res_id"),
-        IR("mailing_trace", "model", "res_id"),
-        IR("mail_mass_mailing", "mailing_model", None, "mailing_model_id", set_unknown=True),
-        IR("mailing_mailing", None, None, "mailing_model_id", set_unknown=True),
-        IR("marketing_campaign", "model_name", None, set_unknown=True),  # stored related
-        IR("marketing_participant", "model_name", "res_id", "model_id", set_unknown=True),
-        IR("payment_transaction", None, "callback_res_id", "callback_model_id"),
-        IR("project_project", "alias_model", None, set_unknown=True),
-        # IR("pos_blackbox_be_log", "model_name", None),  # ACTUALLY NOT. We need to keep records intact, even when renaming a model  # noqa: ERA001
-        IR("quality_point", "worksheet_model_name", None),
-        IR("rating_rating", "res_model", "res_id", "res_model_id"),
-        IR("rating_rating", "parent_res_model", "parent_res_id", "parent_res_model_id"),
-        IR("snailmail_letter", "model", "res_id", set_unknown=True),
-        IR("sms_template", "model", None),
-        IR("studio_approval_rule", "model_name", None),
-        IR("spreadsheet_revision", "res_model", "res_id"),
-        IR("studio_approval_entry", "model", "res_id"),
-        IR("timer_timer", "res_model", "res_id"),
-        IR("timer_timer", "parent_res_model", "parent_res_id"),
-        IR("worksheet_template", "res_model", None),
-    ]
-
-    for ir in each:
+    for ir in INDIRECT_REFERENCES:
         if bound_only and not ir.res_id:
             continue
         if ir.res_id and not column_exists(cr, ir.table, ir.res_id):
@@ -130,7 +130,7 @@ def indirect_references(cr, bound_only=False):
             """,
         )
         for model_name, column_name, comodel_name in cr.fetchall():
-            yield IR(table_of_model(cr, model_name), None, column_name, company_dependent_comodel=comodel_name)
+            yield _IR(table_of_model(cr, model_name), None, column_name, company_dependent_comodel=comodel_name)
 
     # XXX Once we will get the model field of `many2one_reference` fields in the database, we should get them also
     # (and filter the one already hardcoded)

src/util/records.py

@@ -1458,18 +1458,22 @@ def replace_record_references_batch(cr, id_mapping, model_src, model_dst=None, r
     :param list(str) ignores: list of **table** names to skip when updating the referenced
                               values
     """
-    assert id_mapping
-    assert all(isinstance(v, int) and isinstance(k, int) for k, v in id_mapping.items())
-
     _validate_model(model_src)
     if model_dst is None:
         model_dst = model_src
     else:
         _validate_model(model_dst)
 
+    if model_src == model_dst:
+        same_ids = {k: v for k, v in id_mapping.items() if k == v}
+        if same_ids:
+            _logger.warning("Replace references in model `%s`, ignoring same-id mapping `%s`", model_src, same_ids)
+            id_mapping = {k: v for k, v in id_mapping if k != v}
+
+    assert id_mapping
+    assert all(isinstance(v, int) and isinstance(k, int) for k, v in id_mapping.items())
+
     id_update = any(k != v for k, v in id_mapping.items())
-    nop = (not id_update) and (model_src == model_dst)
-    assert nop is False
 
     ignores = [_validate_table(table) for table in ignores]
     if not replace_xmlid:
@@ -1685,6 +1689,7 @@ def replace_record_references_batch(cr, id_mapping, model_src, model_dst=None, r
                SET {upd}
                    {jmap_expr_upd}
               FROM _upgrade_rrr
+              {{extra_join}}
              WHERE {whr}
                AND _upgrade_rrr.old = t.{res_id}
             """,
@@ -1701,19 +1706,43 @@ def replace_record_references_batch(cr, id_mapping, model_src, model_dst=None, r
         if ir.res_model_id:
             unique_indexes += _get_unique_indexes_with(cr, ir.table, ir.res_id, ir.res_model_id)
         if unique_indexes:
+            query = format_query(
+                cr,
+                query,
+                extra_join=format_query(
+                    cr,
+                    """
+         LEFT JOIN _upgrade_rrr AS _upgrade_rrr2
+                ON _upgrade_rrr.new = _upgrade_rrr2.new
+               AND _upgrade_rrr.old < _upgrade_rrr2.old
+         LEFT JOIN {table} t2
+                ON _upgrade_rrr2.old = t2.{res_id}
+                    """,
+                    table=ir.table,
+                    res_id=ir.res_id,
+                ),
+            )
             conditions = []
             for _, uniq_cols in unique_indexes:
                 uniq_cols = set(uniq_cols) - {ir.res_id, ir.res_model, ir.res_model_id}  # noqa: PLW2901
                 conditions.append(
                     format_query(
                         cr,
-                        "NOT EXISTS(SELECT 1 FROM {table} WHERE {res_model_whr} AND {jmap_expr} AND {where_clause})",
+                        """
+                         -- there is no target already present within the same constraint
+                        NOT EXISTS(SELECT 1 FROM {table} WHERE {res_model_whr} AND {jmap_expr} AND {where_clause})
+                         -- there is no other entry with the same target within the same constraint
+                        AND NOT (t2 IS NOT NULL AND {where_clause2})
+                        """,
                         table=ir.table,
                         res_model_whr=res_model_whr,
                         jmap_expr=jmap_expr,
                         where_clause=SQLStr(" AND ".join(format_query(cr, "{0}=t.{0}", col) for col in uniq_cols))
                         if uniq_cols
                         else SQLStr("True"),
+                        where_clause2=SQLStr(" AND ".join(format_query(cr, "t2.{0}=t.{0}", col) for col in uniq_cols))
+                        if uniq_cols
+                        else SQLStr("True"),
                     )
                 )
             query = format_query(
@@ -1730,7 +1759,7 @@ def replace_record_references_batch(cr, id_mapping, model_src, model_dst=None, r
             )
             cr.execute(query, locals())
         else:
-            fmt_query = cr.mogrify(query.format(**locals()), locals()).decode()
+            fmt_query = cr.mogrify(format_query(cr, query, extra_join=SQLStr("")), locals()).decode()
             parallel_execute(cr, explode_query_range(cr, fmt_query, table=ir.table, alias="t"))
 
     # reference fields