From c1d39d6dbdb2f2bbea3b6643f969d86c487f5c7a Mon Sep 17 00:00:00 2001 From: Don Kendall Date: Tue, 22 Sep 2026 10:12:43 -0400 Subject: [PATCH] [20.0][OU-ADD] gate the constraints the upgrade could not apply When migrated data does not satisfy a constraint the new version declares, Odoo logs one INFO line and carries on. The upgrade still exits 0, and checklog-odoo only reacts to WARNING and above, so nothing noticed that the run which first exited 0 left TWELVE constraints unapplied -- one of them payment_method's provider_id, behind which every company but one had lost its payment methods. The log cannot be the verdict on its own. The line is emitted while the table is reshaped, before post- and end-migration run, so a script that repairs the data cannot stop it being written; and a module that owns a stricter rule replaces the one being attempted a moment later. account cannot build its broad account_move_unique_name while LATAM vendor bills are present, and then l10n_latam_invoice_document installs the narrower index that is the real rule. So the log says which constraints to look at and the schema says how each one ended: nulls are counted, and a named check constraint or unique index is looked up. All three shapes Odoo reports are answerable, so the allowlist is empty. It fails closed four ways, each verified: a log with no upgrade in it, a database it cannot reach, a column that no longer exists, and a message it does not recognise all report rather than pass. The upgrade is piped through tee to produce the log, so the step sets pipefail; without it the step would report tee's status and a failed upgrade would pass. Assisted-by: Claude Opus 5 --- .../scripts/check_unapplied_constraints.py | 210 ++++++++++++++++++ .github/workflows/test-migration.yml | 24 +- 2 files changed, 229 insertions(+), 5 deletions(-) create mode 100644 .github/scripts/check_unapplied_constraints.py diff --git a/.github/scripts/check_unapplied_constraints.py b/.github/scripts/check_unapplied_constraints.py new file mode 100644 index 00000000000..8b10ac11b1d --- /dev/null +++ b/.github/scripts/check_unapplied_constraints.py @@ -0,0 +1,210 @@ +#!/usr/bin/env python3 +"""Fail when the upgrade could not establish a constraint the new version declares. + +When migrated data does not satisfy a constraint, Odoo logs one INFO line and +carries on: + + Constraint not added: column "provider_id" of relation "payment_method" + contains null values + +The upgrade still exits 0, so neither the migration job nor checklog-odoo, which +only reacts to WARNING and above, notices. Every such line is a 20.0 invariant +the database does not hold -- a field the migration should have filled, a rename +it should have performed, or rows it should have reconciled. On the run that +first exited 0 there were twelve, and behind one of them was every company but +one losing its payment methods. + +The log line alone is not the verdict. It is written while the table is being +reshaped, which is *before* post-migration and end-migration run, so a script +that repairs the data cannot stop the line being emitted. Reading the log and +nothing else would report a fixed defect as broken for ever, and the natural +reaction -- allowlist it -- would blind the check to a real regression later. + +So the log only says which constraints to look at; the database says whether +they hold now. All three shapes Odoo reports are answerable from the schema: + + * "contains null values" -- count the nulls that are left. + * "check constraint N ... is violated by some row" -- ask whether N exists. + * "could not create unique index N" -- ask whether N exists. + +The last two matter more than they look. A module that owns a stricter rule than +the one being attempted replaces it a moment later in the same run, and the +replacement succeeds: `account` cannot build its broad account_move_unique_name +while LATAM vendor bills are present, and then l10n_latam_invoice_document +installs the narrower index that is the real rule. The log records the first +attempt; only the schema knows how it ended. + +ALLOWED is a last resort for a constraint that is genuinely absent and genuinely +acceptable. It should stay empty; an entry needs a reason saying why the database +is correct without the constraint, never merely to quiet a failure. + +Usage: check_unapplied_constraints.py [--dsn DSN] + +The connection comes from --dsn, else from the standard libpq environment +(PGHOST, PGDATABASE, PGUSER, ...). It is required: a run that cannot reach the +database fails rather than degrading to a log-only check, because a check that +quietly gets weaker is indistinguishable from one that passes. +""" + +import argparse +import re +import sys + +PATTERN = re.compile(r"Constraint not added: (.+?)\s*$", re.MULTILINE) +NULL_COLUMN = re.compile( + r'^column "(?P[^"]+)" of relation "(?P[^"]+)" contains null values$' +) +CHECK_CONSTRAINT = re.compile( + r'^check constraint "(?P[^"]+)" of relation "(?P
[^"]+)" ' + r"is violated by some row$" +) +UNIQUE_INDEX = re.compile(r'^could not create unique index "(?P[^"]+)"$') + +# Message payload -> why it is acceptable that the constraint is genuinely +# absent. Keep this empty if at all possible. +ALLOWED = {} + + +def connect(dsn): + try: + import psycopg2 + except ImportError: + print("::error::psycopg2 is needed to verify the migrated database") + return None + try: + return psycopg2.connect(dsn) if dsn else psycopg2.connect("") + except Exception as exc: # noqa: BLE001 - any failure here is fatal alike + print(f"::error::cannot reach the migrated database: {exc}") + return None + + +def count_nulls(cur, table, column): + """Rows still holding NULL, or None when the column no longer exists. + + A missing column is not "no nulls": it means the upgrade went somewhere this + check does not understand, and saying so beats reporting a pass. + """ + cur.execute( + """ + SELECT 1 FROM information_schema.columns + WHERE table_name = %s AND column_name = %s + """, + (table, column), + ) + if not cur.fetchone(): + return None + cur.execute(f'SELECT count(*) FROM "{table}" WHERE "{column}" IS NULL') + return cur.fetchone()[0] + + +def constraint_exists(cur, table, name): + cur.execute( + """ + SELECT 1 FROM pg_constraint + WHERE conname = %s AND conrelid = %s::regclass + """, + (name, table), + ) + return bool(cur.fetchone()) + + +def index_exists(cur, name): + cur.execute("SELECT 1 FROM pg_indexes WHERE indexname = %s", (name,)) + return bool(cur.fetchone()) + + +def verdict(cur, payload): + """(resolved?, why) for one 'Constraint not added' payload.""" + match = NULL_COLUMN.match(payload) + if match: + table, column = match.group("table"), match.group("column") + nulls = count_nulls(cur, table, column) + if nulls is None: + return False, f"{table}.{column} does not exist after the upgrade" + if nulls: + return False, f"{nulls} row(s) still NULL" + return True, "0 rows NULL after the full run" + + match = CHECK_CONSTRAINT.match(payload) + if match: + table, name = match.group("table"), match.group("name") + if constraint_exists(cur, table, name): + return True, "the constraint is present in the final schema" + return False, "the constraint is absent from the final schema" + + match = UNIQUE_INDEX.match(payload) + if match: + name = match.group("name") + if index_exists(cur, name): + return True, "the index is present in the final schema" + return False, "the index is absent from the final schema" + + if payload in ALLOWED: + return True, ALLOWED[payload] + return False, "unrecognised message and not allowlisted" + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("log", help="the migration log to read") + parser.add_argument( + "--dsn", + help="libpq connection string; defaults to the PG* environment", + ) + opts = parser.parse_args() + dsn = opts.dsn + + try: + with open(opts.log, encoding="utf-8", errors="replace") as fobj: + log = fobj.read() + except OSError as exc: + print(f"::error::cannot read the migration log: {exc}") + return 2 + + # A log with no upgrade in it must not pass for lack of findings. + if "Modules loaded." not in log: + print( + "::error::the log has no 'Modules loaded.' line, so no upgrade ran " + "and this check proves nothing" + ) + return 2 + + found = [] + for match in PATTERN.finditer(log): + payload = match.group(1) + if payload not in found: + found.append(payload) + if not found: + print("OK: the upgrade applied every constraint it declares.") + return 0 + + conn = connect(dsn) + if conn is None: + return 2 + + resolved, failures = [], [] + with conn, conn.cursor() as cur: + for payload in found: + ok, why = verdict(cur, payload) + (resolved if ok else failures).append((payload, why)) + + for payload, why in resolved: + print(f"resolved: {payload}\n -> {why}") + + if not failures: + print(f"\nOK: {len(resolved)} reported, all resolved by the end of the run.") + return 0 + + print() + for payload, why in failures: + print(f"::error::{payload} -- {why}") + print( + f"\n{len(failures)} constraint(s) the new version declares are missing from " + f"the migrated database, and the data still does not satisfy them. Each one " + f"is data the migration should have produced and did not." + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/test-migration.yml b/.github/workflows/test-migration.yml index 9f41d52ad16..c5b5a24e7dd 100644 --- a/.github/workflows/test-migration.yml +++ b/.github/workflows/test-migration.yml @@ -128,12 +128,14 @@ jobs: sed -i -E "s/(gevent==)21\.8\.0( ; sys_platform != 'win32' and python_version == '3.10')/\122.10.2\2/;s/(greenlet==)1.1.2( ; sys_platform != 'win32' and python_version == '3.10')/\12.0.2\2/" odoo/requirements.txt pip install -q -r odoo/requirements.txt pip install -r ./openupgrade/requirements.txt - # TEMPORARY pin to the fork branch carrying the get_many2one_references - # fix (ledoent/openupgradelib#4). Upstream master selects + # TEMPORARY pin to the fork branch carrying the two 20.0 fixes + # (ledoent/openupgradelib#4). Upstream master selects # ir_model_fields.relation_model_field, which only exists in 20.0, while a # pre-migration runs against the 19.0 schema -- every script calling - # rename_models or rename_fields dies on UndefinedColumn. Drop this back to - # oca/openupgradelib once the fix is released. + # rename_models or rename_fields dies on UndefinedColumn. It also reaches + # convert_xml_import through odoo.tools, which 20.0 no longer re-exports, + # so every load_data call ends the upgrade. Drop this back to + # oca/openupgradelib once the fixes are released. pip install -U git+https://github.com/ledoent/openupgradelib@20.0-fix-many2one-references # this is for v18 l10n_eg_edi_eta which crashes without it pip install asn1crypto @@ -157,6 +159,10 @@ jobs: - name: OpenUpgrade test id: run_migration run: | + # The upgrade is piped through tee so the constraint check below can + # read it. Without pipefail the step would report tee's status and a + # failed upgrade would pass. + set -o pipefail # select modules and perform the upgrade MODULES_OLD=$(\ sed -n '/^+========/,$p' \ @@ -208,7 +214,15 @@ jobs: --log-handler odoo.models.unlink:WARNING \ --stop-after-init \ --without-demo=$MODULES_NEW \ - --update=$MODULES_NEW + --update=$MODULES_NEW \ + 2>&1 | tee "$GITHUB_WORKSPACE/migration.log" + - name: Check the constraints the upgrade could not apply + if: ${{ steps.run_migration.outputs.modules_new != '' }} + run: | + PGHOST=$DB_HOST PGPORT=$DB_PORT PGUSER=$DB_USERNAME \ + PGPASSWORD=$DB_PASSWORD PGDATABASE=$DB \ + python3 openupgrade/.github/scripts/check_unapplied_constraints.py \ + "$GITHUB_WORKSPACE/migration.log" - name: Generate coverage.xml if: ${{ steps.run_migration.outputs.modules_new != '' }} run: coverage xml