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