Skip to content

[FIX] merge_models: psycopg2 param style + double-AND typo (marketing_card UPDATE)#450

Merged
MiquelRForgeFlow merged 1 commit into
OCA:masterfrom
ledoent:fix-merge-models-marketing-card
May 21, 2026
Merged

[FIX] merge_models: psycopg2 param style + double-AND typo (marketing_card UPDATE)#450
MiquelRForgeFlow merged 1 commit into
OCA:masterfrom
ledoent:fix-merge-models-marketing-card

Conversation

@dnplkndll
Copy link
Copy Markdown
Contributor

Bug

The merge_models() helper's card_campaign cross-reference UPDATE (active when marketing_card is installed) has two issues at openupgradelib/openupgrade.py:1210-1226:

query = """
    UPDATE card_card c
    SET res_id = mt.id, campaign_id = cc2.id
    FROM {model_table} mt, card_campaign cc, card_campaign cc2
    WHERE cc.res_model = {old_model} AND cc2.res_model = {new_model}    # ← bug 1
        AND mt.{ref_field} = c.res_id AND AND c.campaign_id = cc.id"""  # ← bug 2
logged_query(
    cr,
    sql.SQL(query).format(
        model_table=sql.Identifier(model_table),
        ref_field=sql.Identifier(ref_field),    # ← only 2 of 4 placeholders passed
    ),
    {"old_model": old_model, "new_model": new_model},   # ← psycopg2 query params, not sql.SQL.format kwargs
)
  1. {old_model} / {new_model} are written as sql.SQL().format() placeholders, but they're passed via the psycopg2 query-params dict instead. Result at runtime: KeyError: 'old_model' from psycopg2/sql.py:254 in sql.SQL.format().
  2. AND mt.{ref_field} = c.res_id AND AND c.campaign_id = cc.id — redundant AND (SQL syntax error if the first bug ever got past the formatter).

Source

Introduced by #436 ([FIX] merge_models: check table columns exist, merged 2026-02-09 by @MiquelRForgeFlow, approved by @pedrobaeza; @hbrunn requested changes on an unrelated base_automation concern that was resolved). Commit f7f5eab7[FIX] merge_models: correctly handle marketing_card. ~3.4 months old at time of report.

Why nobody caught it: the if is_module_installed(cr, 'marketing_card') gate means the buggy code path only fires when both marketing_card is installed and a model-merge migration runs. The OCA test seed at the time apparently didn't combine those.

Reproduction

Real-world hit: OCA/OpenUpgrade PR #5651 — when marketing_card is added to MODULES_NEW (so the seed has it installed), hr_recruitment/19.0.1.1/pre-migration.py:14's call to openupgrade.merge_models(env.cr, "hr.candidate", "hr.applicant", "candidate_id") triggers this code path and crashes with KeyError: 'old_model'.

Minimal psycopg2 repro (verified locally in the lab):

from psycopg2 import sql
buggy = '''
    WHERE cc.res_model = {old_model} AND cc2.res_model = {new_model}
        AND mt.{ref_field} = c.res_id AND AND c.campaign_id = cc.id'''
sql.SQL(buggy).format(ref_field=sql.Identifier("candidate_id"))
# → KeyError: 'old_model'

Fix

Switch the two value placeholders to psycopg2 param style (%(old_model)s / %(new_model)s, matching the dict already being passed) and drop the redundant AND:

query = """
    UPDATE card_card c
    SET res_id = mt.id, campaign_id = cc2.id
    FROM {model_table} mt, card_campaign cc, card_campaign cc2
    WHERE cc.res_model = %(old_model)s AND cc2.res_model = %(new_model)s
        AND mt.{ref_field} = c.res_id AND c.campaign_id = cc.id"""

Verification

  • Bug reproduced standalone with psycopg2's sql.SQL().format()KeyError: 'old_model'
  • Fixed template formats cleanly → %(old_model)s / %(new_model)s remain as query params for cursor.execute, ref_field becomes a quoted identifier
  • No-op functional change in behavior; same fields still pinned, same parameters still passed

…ng_card UPDATE

The card_campaign cross-reference UPDATE inside merge_models() (active when
marketing_card is installed) had two bugs:

1. Used `{old_model}` / `{new_model}` as sql.SQL().format() placeholders but
   passed them via the psycopg2 params dict — raising
   `KeyError: 'old_model'` from psycopg2/sql.py at runtime.
2. Double `AND` in WHERE clause:
   `AND mt.{ref_field} = c.res_id AND AND c.campaign_id = cc.id`

Both fixed: switch to `%(old_model)s` / `%(new_model)s` (matching the params
dict) and remove the redundant AND.

Triggered in practice by any model-merge migration when marketing_card is
installed (e.g., OCA/OpenUpgrade hr_recruitment hr.candidate -> hr.applicant
merge on 18.0 -> 19.0).
@dnplkndll dnplkndll marked this pull request as ready for review May 21, 2026 13:08
@MiquelRForgeFlow MiquelRForgeFlow merged commit 4029001 into OCA:master May 21, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants