From 0c0ed07aa81c20eb073262c6f46f3a3d135e8d79 Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Mon, 17 Aug 2026 17:45:04 -0400 Subject: [PATCH] [FIX] migration: the cleanup installs its own module, and survives a refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects, all mine, all found on a real run. Creating a wizard could fail and leave the transaction ABORTED. The next name read died on it, outside any guard, and the whole script stopped with no report at all — only a traceback. Creating and reading the names now happen inside the savepoint, and a report is printed whatever happens: knowing what was done matters more than the trace of what broke. « No orphaned models found » is a UserError: the module signals the EMPTY by raising. Counting it as a failure made a healthy database look broken, four warnings out of five kinds. The module was installed at step 3, after being used at step 2, so the first run had no wizard at all and answered « nothing to do ». It is now installed by the tool itself, and step 3 no longer asks to redo by hand what has just been done automatically. --- FR --- [FIX] migration : le nettoyage pose son module, et survit à un refus Trois défauts, tous à moi, tous trouvés sur une vraie exécution. Créer un assistant pouvait échouer en laissant la transaction AVORTÉE. La lecture de nom suivante mourait dessus, hors de tout garde, et le script s'arrêtait sans aucun rapport — juste une trace. La création et la lecture des noms sont désormais dans le point de reprise, et un rapport est imprimé quoi qu'il arrive : savoir ce qui a été fait vaut mieux que la trace de ce qui a cassé. « No orphaned models found » est une UserError : le module signale le VIDE en levant. Le compter comme un échec faisait passer une base saine pour cassée, quatre avertissements sur cinq catégories. Le module était installé à l'étape 3, après avoir servi à l'étape 2 : le premier passage n'avait donc aucun assistant et répondait « rien à faire ». L'outil le pose lui-même, et l'étape 3 ne demande plus de refaire à la main ce qui vient d'être fait. Assisted-by: Claude Opus 5 --- script/odoo/migration/database_cleanup.py | 175 ++++++++++++++++----- script/todo/todo_i18n.py | 12 ++ script/todo/todo_upgrade.py | 30 +--- test/test_database_cleanup.py | 177 +++++++++++++++++++++- 4 files changed, 329 insertions(+), 65 deletions(-) diff --git a/script/odoo/migration/database_cleanup.py b/script/odoo/migration/database_cleanup.py index 4e6f7fd..4bdaa65 100755 --- a/script/odoo/migration/database_cleanup.py +++ b/script/odoo/migration/database_cleanup.py @@ -79,53 +79,90 @@ END = "ERPLIBRE_CLEANUP_END" SHELL_SCRIPT = """ import json + +try: + from odoo.exceptions import UserError +except Exception: + class UserError(Exception): + pass + ORDER = %(order)r MAX_ROUND = %(max_round)d DRY_RUN = %(dry_run)s report = {"rounds": [], "missing": [], "failed": []} -for index in range(MAX_ROUND): - this_round = [] - purged_this_round = 0 - for label, model in ORDER: - if model not in env: - if label not in report["missing"]: - report["missing"].append(label) - continue - ok = 0 - errors = [] - would = [] - try: - wizard = env[model].create({}) - lines = wizard.purge_line_ids - except Exception as exc: - report["failed"].append([label, "-", str(exc)[:200]]) - continue - for line in lines: - name = line.name or str(line.id) - if DRY_RUN: - would.append(name) + + +def note(label, name, exc): + report["failed"].append([label, name, str(exc)[:200]]) + + +try: + for index in range(MAX_ROUND): + this_round = [] + purged_this_round = 0 + for label, model in ORDER: + if model not in env: + if label not in report["missing"]: + report["missing"].append(label) continue + ok = 0 + errors = [] + would = [] try: - # Un point de reprise par ENTRÉE : un refus n'emporte que la - # sienne, et la passe continue. Sans cela, le premier échec - # ferait perdre tout ce que la passe avait réparé. + # La CRÉATION et la lecture des noms sont dans le point de + # reprise, pas seulement la purge. Un échec ici laissait la + # transaction avortée ; la lecture suivante mourait dessus, + # hors de tout garde, et le rapport entier était perdu. + # Les noms sont matérialisés tout de suite : après un retour + # arrière, les relire relancerait une requête. with env.cr.savepoint(): - line.purge() - ok += 1 + wizard = env[model].create({}) + todo = [ + (line, line.name or str(line.id)) + for line in wizard.purge_line_ids + ] + except UserError: + # « Aucun modèle orphelin trouvé » : le module signale le + # VIDE par une exception. Le compter comme un échec faisait + # passer une base saine pour une base cassée. + this_round.append({"kind": label, "purged": 0, + "errors": [], "would": []}) + continue except Exception as exc: - errors.append([name, str(exc)[:160]]) - if not DRY_RUN and ok: - env.cr.commit() - purged_this_round += ok - this_round.append({"kind": label, "purged": ok, - "errors": errors, "would": would}) - report["rounds"].append(this_round) - # On s'arrête quand une passe ENTIÈRE n'a plus rien réparé : ce qui - # résistait au tour d'avant résistera encore. En simulation, une seule - # passe suffit — rien ne change, donc rien ne se libère. - if purged_this_round == 0 or DRY_RUN: - break + note(label, "-", exc) + continue + for line, name in todo: + if DRY_RUN: + would.append(name) + continue + try: + # Un point de reprise par ENTRÉE : un refus n'emporte que + # la sienne, et la passe continue. Sans cela, le premier + # échec ferait perdre tout ce que la passe avait réparé. + with env.cr.savepoint(): + line.purge() + ok += 1 + except Exception as exc: + errors.append([name, str(exc)[:160]]) + if not DRY_RUN and ok: + try: + env.cr.commit() + except Exception as exc: + note(label, "commit", exc) + purged_this_round += ok + this_round.append({"kind": label, "purged": ok, + "errors": errors, "would": would}) + report["rounds"].append(this_round) + # On s'arrête quand une passe ENTIÈRE n'a plus rien réparé : ce qui + # résistait au tour d'avant résistera encore. En simulation, une + # seule passe suffit — rien ne change, donc rien ne se libère. + if purged_this_round == 0 or DRY_RUN: + break +except Exception as exc: + # Le rapport de CE QUI A ÉTÉ FAIT vaut plus que la trace de ce qui a + # cassé : sans lui, on ne sait même pas si la base a été touchée. + note("*", "fatal", exc) print("%(start)s") print(json.dumps(report)) @@ -200,6 +237,47 @@ def require_matching_version(database): return None +def module_state(database, module="database_cleanup"): + """L'état du module dans cette base, ou None si on ne peut pas lire.""" + env = os.environ.copy() + env["PGOPTIONS"] = "-c default_transaction_read_only=on" + env["PSQLRC"] = "" + done = subprocess.run( + [ + "psql", + "-X", + "-w", + "-d", + database, + "-tAc", + f"SELECT state FROM ir_module_module WHERE name='{module}';", + ], + capture_output=True, + text=True, + env=env, + ) + if done.returncode: + return None + return done.stdout.strip() or None + + +def install_module(database, module="database_cleanup", timeout=1800): + """Poser le module avant de s'en servir. + + Sans lui, aucun assistant n'existe et l'outil rend « rien à faire » sur + une base qui en aurait eu besoin — un silence qu'on prend pour un + succès. La migration l'installait plus tard, à l'étape 3 ; l'attendre + revenait à nettoyer trop tard. + """ + done = subprocess.run( + ["./script/addons/install_addons.sh", database, module], + capture_output=True, + text=True, + timeout=timeout, + ) + return done.returncode, done.stdout + done.stderr + + def run_shell(database, config_path, script, timeout=3600): """Pousser le script dans « odoo-bin shell » et rendre son rapport. @@ -267,6 +345,15 @@ def render(report, database): by_kind[kind] = by_kind.get(kind, 0) + 1 for kind, count in by_kind.items(): lines.append(f" - {t(LABEL.get(kind, kind))} : {count}") + # Les échecs comptent AUSSI en simulation : une catégorie qui ne + # s'ouvre même pas est une information, pas un silence. + for kind, name, message in report.get("failed", []): + lines.append(f" ⚠ [{kind}] {name} : {message[:90]}") + for kind in report.get("missing", []): + lines.append( + f" ℹ {t(LABEL.get(kind, kind))} :" + f" {t('no such wizard in this version, skipped.')}" + ) return "\n".join(lines) + "\n" total = 0 for index, this_round in enumerate(report.get("rounds", []), start=1): @@ -333,6 +420,18 @@ def main(argv=None): print(f"⛔ {mismatch}") return 2 + state = module_state(config.database) + if state != "installed": + print( + f"⧖ {t('database_cleanup is not installed on this base;')}" + f" {t('installing it first.')}" + ) + code, output = install_module(config.database) + if code: + print(output.strip()[-1500:]) + print(f"❌ {t('Could not install database_cleanup.')}") + return 2 + print(f"⧖ {t('Cleaning')} '{config.database}'…") try: report = run_shell( diff --git a/script/todo/todo_i18n.py b/script/todo/todo_i18n.py index 70b1f72..d24cee4 100644 --- a/script/todo/todo_i18n.py +++ b/script/todo/todo_i18n.py @@ -5266,6 +5266,18 @@ TRANSLATIONS = { "fr": "(défaut)", "en": "(default)", }, + "database_cleanup is not installed on this base;": { + "fr": "database_cleanup n'est pas installé sur cette base ;", + "en": "database_cleanup is not installed on this base;", + }, + "installing it first.": { + "fr": "installation préalable.", + "en": "installing it first.", + }, + "Could not install database_cleanup.": { + "fr": "Impossible d'installer database_cleanup.", + "en": "Could not install database_cleanup.", + }, "Nothing to decide yet": { "fr": "Rien à décider pour l'instant", "en": "Nothing to decide yet", diff --git a/script/todo/todo_upgrade.py b/script/todo/todo_upgrade.py index 7aec353..123104e 100755 --- a/script/todo/todo_upgrade.py +++ b/script/todo/todo_upgrade.py @@ -1767,32 +1767,12 @@ class TodoUpgrade: self.print_step(msg) self.add_comment_progression(msg) - if not self.dct_progression.get("state_3_install_clean_database"): - status, cmd_executed = self.todo_upgrade_execute( - f"./script/addons/install_addons.sh {database_name} database_cleanup", - single_source_odoo=True, - ) - if not status: - self.dct_progression["state_3_install_clean_database"] = True - self.write_config() - + # Le nettoyage est fait à l'étape 2, par l'outil, et il pose lui-même + # le module s'il manque. Redemander ici « avez-vous fini de nettoyer + # dans l'interface ? » ferait faire à la main ce qui vient d'être + # fait — et l'installation qui suivait arrivait APRÈS l'usage. if not self.dct_progression.get("state_3_clean_database"): - print( - f"✨ {t('Go to Settings / Technical / Cleanup... / Purge and')}" - f" {t('purge the obsolete modules')}" - ) - status = self.ask_gate( - "💬 Did you finish to clean database? Press y/Y to open" - " server with selenium, else ignore it" - f" {t('(b = go back to a previous step)')} : " - ).strip() - - if status.lower().strip() == "y": - self.todo.prompt_execute_selenium_and_run_db(database_name) - status = self.ask( - f"💬 {t('Press enter to continue step 3')} : " - ).strip() - + self.dct_progression["state_3_install_clean_database"] = True self.dct_progression["state_3_clean_database"] = True self.write_config() diff --git a/test/test_database_cleanup.py b/test/test_database_cleanup.py index 37d2587..f57ca3f 100755 --- a/test/test_database_cleanup.py +++ b/test/test_database_cleanup.py @@ -89,6 +89,39 @@ class FakeEnv(dict): self.cr = FakeCursor(journal) +class UserError(Exception): + """Celle que le script poussé importera : on fournit odoo.exceptions. + + Refaire une classe de son côté ne servirait à rien — « except » compare + des identités, pas des noms, et le test passerait à côté. + """ + + +def install_fake_odoo_exceptions(case): + """Rendre `from odoo.exceptions import UserError` possible ici.""" + import types + + odoo = sys.modules.get("odoo") or types.ModuleType("odoo") + exceptions = types.ModuleType("odoo.exceptions") + exceptions.UserError = UserError + avant_odoo = sys.modules.get("odoo") + avant_exc = sys.modules.get("odoo.exceptions") + sys.modules["odoo"] = odoo + sys.modules["odoo.exceptions"] = exceptions + + def remettre(): + for nom, valeur in ( + ("odoo", avant_odoo), + ("odoo.exceptions", avant_exc), + ): + if valeur is None: + sys.modules.pop(nom, None) + else: + sys.modules[nom] = valeur + + case.addCleanup(remettre) + + def run_script(models, max_round=10, dry_run=False): """Exécuter le script réellement poussé, et rendre (rapport, journal).""" import json @@ -146,7 +179,9 @@ class TestOneEntryCannotSinkThePass(unittest.TestCase): ] models = {cleanup.ORDER[0][1]: FakeModel(lines)} _report, got = run_script(models, max_round=1) - self.assertEqual(got.count(("savepoint", "enter")), 2) + # Un point de reprise par entrée, PLUS un pour la création et la + # lecture des noms : c'est là que la transaction s'était avortée. + self.assertEqual(got.count(("savepoint", "enter")), 3) def test_a_refusal_rolls_back_only_its_own(self): journal = [] @@ -161,7 +196,8 @@ class TestOneEntryCannotSinkThePass(unittest.TestCase): self.assertEqual(entry["purged"], 2) self.assertEqual([name for name, _msg in entry["errors"]], ["bad"]) self.assertEqual(got.count(("savepoint", "rollback")), 1) - self.assertEqual(got.count(("savepoint", "release")), 2) + # Deux entrées purgées + la création : trois relâchements. + self.assertEqual(got.count(("savepoint", "release")), 3) def test_a_wizard_that_cannot_even_be_created_is_recorded(self): models = {cleanup.ORDER[0][1]: FakeModel([], raise_on_create="boom")} @@ -170,6 +206,55 @@ class TestOneEntryCannotSinkThePass(unittest.TestCase): self.assertIn("boom", report["failed"][0][2]) +class TestTheReportSurvivesAnything(unittest.TestCase): + """Sans rapport, on ne sait même pas si la base a été touchée. + + Vécu : `create({})` échouait, l'erreur était notée mais la transaction + restait AVORTÉE. La lecture de nom suivante mourait dessus, hors de tout + garde, et le script entier s'arrêtait — aucun rapport, juste une trace. + """ + + def test_reading_the_names_is_inside_the_savepoint(self): + # C'est la lecture des noms qui déclenche la requête, pas la + # création : la laisser dehors était le défaut. + source = cleanup.build_script(1, False) + creation = source.index("wizard = env[model].create({})") + garde = source.rindex("with env.cr.savepoint():", 0, creation) + noms = source.index("line.name or str(line.id)") + self.assertLess(garde, creation) + self.assertLess(creation, noms) + + def test_a_failure_on_create_does_not_kill_the_run(self): + models = { + cleanup.ORDER[0][1]: FakeModel([], raise_on_create="boom"), + cleanup.ORDER[2][1]: FakeModel([FakeLine("colonne")]), + } + report, _got = run_script(models, max_round=1) + # La catégorie suivante a bien travaillé malgré l'échec de la + # première. + purged = {e["kind"]: e["purged"] for e in report["rounds"][0]} + self.assertEqual(purged.get("columns"), 1) + self.assertEqual(report["failed"][0][0], "models") + + def test_an_unexpected_failure_still_yields_a_report(self): + class Explosive(dict): + def __init__(self, journal): + super().__init__() + self.cr = FakeCursor(journal) + + def __contains__(self, key): + raise RuntimeError("registre en miettes") + + import json as _json + + journal = [] + namespace = {"env": Explosive(journal)} + exec(cleanup.build_script(1, False), namespace) # noqa: S102 + report = _json.loads(_json.dumps(namespace["report"])) + self.assertEqual(report["failed"][0][:2], ["*", "fatal"]) + self.assertIn("miettes", report["failed"][0][2]) + + class TestGoingRoundAgain(unittest.TestCase): def test_what_one_pass_refused_the_next_may_take(self): # LE point, et la raison même de boucler : une entrée refuse TANT QUE @@ -385,6 +470,94 @@ class TestTheMigrationRunsItBeforeTheSmokeTest(unittest.TestCase): self.assertIn("-d db_upgrade_18", lst_cmd[0]) +class TestNothingToPurgeIsNotAFailure(unittest.TestCase): + """Le module signale le VIDE par une exception. + + `raise UserError("No orphaned models found")` : le compter comme un + échec faisait passer une base saine pour une base cassée, avec quatre + avertissements sur cinq catégories. + """ + + def test_the_pushed_script_separates_it(self): + source = cleanup.build_script(1, False) + self.assertIn("except UserError:", source) + vide = source.index("except UserError:") + echec = source.index("except Exception as exc:\n note") + self.assertLess(vide, echec, "l'ordre des except décide") + + def test_it_is_reported_as_zero_not_as_an_error(self): + install_fake_odoo_exceptions(self) + + class Empty(FakeModel): + def create(self, values): + raise UserError("No orphaned models found") + + models = {cleanup.ORDER[0][1]: Empty([])} + report, _got = run_script(models, max_round=1) + entry = report["rounds"][0][0] + self.assertEqual(entry["purged"], 0) + self.assertEqual(entry["errors"], []) + self.assertEqual(report["failed"], []) + + +class TestTheModuleIsInstalledFirst(unittest.TestCase): + """Sans le module, aucun assistant n'existe et l'outil dit « rien ». + + Ce silence se lit comme un succès. La migration l'installait à l'étape + 3, donc APRÈS le nettoyage de l'étape 2 : l'ordre rendait l'outil + inutile au premier passage. + """ + + def test_it_checks_the_state_before_cleaning(self): + import inspect + + source = inspect.getsource(cleanup.main) + self.assertIn("module_state(", source) + self.assertLess( + source.index("module_state("), source.index("run_shell(") + ) + + def test_it_installs_when_absent(self): + import inspect + + source = inspect.getsource(cleanup.main) + self.assertIn('state != "installed"', source) + self.assertIn("install_module(", source) + + def test_a_failed_install_stops_there(self): + # Nettoyer sans le module rendrait « rien à faire » sur une base qui + # en avait besoin. + import inspect + + source = inspect.getsource(cleanup.main) + install = source.index("install_module(") + self.assertIn("return 2", source[install : install + 400]) + + +class TestTheMigrationNoLongerAsksTwice(unittest.TestCase): + def test_the_manual_cleanup_prompt_is_gone(self): + # Le faire à la main après l'avoir fait automatiquement. + import inspect + + from script.todo.todo_upgrade import TodoUpgrade + + source = inspect.getsource(TodoUpgrade.execute_odoo_upgrade) + self.assertNotIn("Did you finish to clean database", source) + self.assertNotIn("Go to Settings / Technical / Cleanup", source) + + def test_the_late_install_is_gone_too(self): + # Elle arrivait à l'étape 3, après l'usage de l'étape 2 : l'outil + # pose désormais le module lui-même, au bon moment. + import inspect + + from script.todo.todo_upgrade import TodoUpgrade + + source = inspect.getsource(TodoUpgrade.execute_odoo_upgrade) + self.assertNotIn( + "install_addons.sh {database_name} database_cleanup", source + ) + + class TestItRefusesTheWrongOdooVersion(unittest.TestCase): """Un Odoo plus ancien sur une base plus récente ÉCRIT avant d'échouer."""