diff --git a/script/odoo/migration/smoke_public_url.py b/script/odoo/migration/smoke_public_url.py index c0555b8..624142e 100755 --- a/script/odoo/migration/smoke_public_url.py +++ b/script/odoo/migration/smoke_public_url.py @@ -33,6 +33,8 @@ Exit codes: 0 every URL answered, 1 some failed, 2 the tool failed. import argparse import os import re +import signal +import socket import subprocess import sys import time @@ -54,14 +56,44 @@ except Exception: # pragma: no cover - repli si i18n indisponible RE_LOC = re.compile(r"\s*([^<\s]+)\s*", re.I) +# « [view_id: 3282, xml_id: n/a, model: n/a, parent_id: 3281] ». Seule la +# phrase qui précède est traduite ; cette ligne-ci ne l'est pas, donc la lire +# marche dans les deux langues. Le PARENT est le coupable : c'est la copie +# figée dans laquelle l'enfant ne trouve plus son xpath. +RE_CONTEXT = re.compile(r"\[view_id: (\d+),.*?parent_id: (\d+)\]") + # Un port à part : la migration tourne souvent à côté d'une instance vivante, # et lui voler 8069 ferait échouer le test pour une raison sans rapport. DEFAULT_PORT = 8169 -def start_server(database, port, config_path="./config.conf"): - """Démarrer Odoo sur la base, sans écrire dans le terminal appelant.""" - return subprocess.Popen( +def run_psql(database, sql): + """Lecture seule, garantie par le serveur PostgreSQL lui-même.""" + env = os.environ.copy() + env["PGOPTIONS"] = "-c default_transaction_read_only=on" + env["PSQLRC"] = "" + done = subprocess.run( + ["psql", "-X", "-w", "-d", database, "-tAF", "\x1f", "-c", sql], + capture_output=True, + text=True, + env=env, + ) + if done.returncode: + return [] + return [line.split("\x1f") for line in done.stdout.splitlines() if line] + + +def start_server(database, port, config_path="./config.conf", log_path=None): + """Démarrer Odoo, son journal dans un FICHIER. + + Pas un tube : Python bufferise par blocs quand sa sortie n'est pas un + terminal, et un fil de lecture court alors après des lignes qui n'ont + pas encore été écrites. Mesuré — 24 lignes vues sur 198, et la trace qui + nomme la vue fautive faisait partie des absentes. Un fichier se relit + entièrement, quand on veut. + """ + handle = open(log_path, "w", encoding="utf-8") if log_path else None + server = subprocess.Popen( [ "./run.sh", "-c", @@ -72,10 +104,26 @@ def start_server(database, port, config_path="./config.conf"): str(port), "--log-level=warn", ], - stdout=subprocess.PIPE, + stdout=handle or subprocess.DEVNULL, stderr=subprocess.STDOUT, text=True, + # Son propre groupe de processus : « ./run.sh » est un script bash + # qui ne transmet rien à son enfant. Un terminate() sur lui tuait le + # script et laissait odoo-bin vivant, tenant le port. L'essai suivant + # démarrait alors un serveur qui ne pouvait pas se lier, et + # interrogeait sans le savoir CELUI D'AVANT — mesuré, deux fois. + start_new_session=True, ) + server.erplibre_log = handle + return server + + +def read_log(log_path): + """Le journal du serveur, tel qu'écrit jusqu'ici.""" + if not log_path or not os.path.isfile(log_path): + return [] + with open(log_path, "r", encoding="utf-8", errors="replace") as handle: + return handle.read().splitlines() def fetch(url, timeout=30): @@ -138,17 +186,84 @@ def local_url(base_url, loc): return base_url.rstrip("/") + path +# Odoo écrit sa trace APRÈS avoir répondu, et par le tube d'un script shell : +# mesuré, elle peut arriver près de trois secondes plus tard. Une demi-seconde +# concluait « aucune vue en cause » sur des pages qui en nommaient une. +LOG_DELAY = 3.0 + + +def attach_missing_parents(lst_failure, lst_log): + """Repêcher les contextes arrivés trop tard pour leur tranche. + + Le découpage par URL est une commodité, pas une garantie : le journal est + asynchrone. Ce second passage lit TOUT ce qui a été capturé et rattache + ce qui n'avait été rattaché à rien — mieux vaut un coupable mal attribué + qu'un coupable perdu. + """ + known = {pid for _u, _s, lst in lst_failure for pid in lst} + extra = [] + for line in lst_log: + for _view_id, parent_id in RE_CONTEXT.findall(line): + if parent_id not in known and parent_id not in extra: + extra.append(parent_id) + if not extra: + return lst_failure + rebuilt = [] + placed = False + for url, status, lst_parent in lst_failure: + if not lst_parent and not placed: + lst_parent = list(extra) + placed = True + rebuilt.append((url, status, lst_parent)) + if not placed and rebuilt: + url, status, lst_parent = rebuilt[0] + rebuilt[0] = (url, status, lst_parent + extra) + return rebuilt + + def check_urls(lst_url, timeout=30): - """[(url, statut)] pour celles qui ont échoué.""" + """[(url, statut, [])] pour celles qui ont échoué. + + Les vues en cause sont rattachées après coup, en relisant le journal du + serveur : elles y arrivent quand Odoo vide son tampon, pas quand la + requête revient. + """ lst_failure = [] for url in lst_url: status, _body = fetch(url, timeout=timeout) if status == 0 or status >= 400: - lst_failure.append((url, status)) + lst_failure.append((url, status, [])) return lst_failure -def render(lst_url, lst_failure): +def culprit_keys(database, lst_failure): + """Les clés des vues parentes mises en cause, sans doublon. + + C'est ce qu'on passe à reset_stale_cow_views : il travaille par clé, et + relever un identifiant dans une trace pour le traduire à la main est + exactement la recopie où l'on se trompe. + """ + lst_id = [] + for _url, _status, lst_parent in lst_failure: + for parent_id in lst_parent: + if parent_id not in lst_id: + lst_id.append(parent_id) + if not lst_id: + return [] + ids = ",".join(str(int(x)) for x in lst_id) + rows = run_psql( + database, + f"SELECT id, key, website_id FROM ir_ui_view WHERE id IN ({ids})" + " AND key IS NOT NULL ORDER BY id;", + ) + lst_key = [] + for row in rows: + if len(row) >= 2 and row[1] and row[1] not in lst_key: + lst_key.append(row[1]) + return lst_key + + +def render(lst_url, lst_failure, lst_key=None): if not lst_url: return f"⚠️ {t('The sitemap listed no URL: nothing was tested.')}\n" if not lst_failure: @@ -160,9 +275,21 @@ def render(lst_url, lst_failure): f"❌ {len(lst_failure)} {t('of')} {len(lst_url)}" f" {t('public URL(s) failed')} :" ] - for url, status in lst_failure: + for url, status, lst_parent in lst_failure: label = status or t("no answer") lines.append(f" [{label}] {url}") + if lst_parent: + lines.append( + f" {t('parent view(s) in cause')} :" + f" {', '.join(lst_parent)}" + ) + if lst_key: + lines.append( + f" {t('Those parents are copies frozen on an older version;')}" + f" {t('resetting them onto the module view is the fix')} :" + ) + for key in lst_key: + lines.append(f" {key}") lines.append( f" {t('A page listed for search engines that does not answer is')}" f" {t('a page your visitors do not reach either.')}" @@ -170,10 +297,104 @@ def render(lst_url, lst_failure): return "\n".join(lines) + "\n" -def run(database, port, config_path, limit=None, timeout=30, boot=180): - """Démarrer, interroger, arrêter. Rend (urls, échecs).""" +def apply_reset(database, lst_key): + """Réinitialiser ces copies sur leur vue module. ÉCRIT en base. + + On délègue à reset_stale_cow_views : il sauvegarde l'arch précédente + avant d'écrire, et c'est déjà lui qu'on documente partout ailleurs. En + refaire une seconde version ici, c'est se donner deux comportements à + tenir d'accord. + """ + cmd = [ + sys.executable, + os.path.join( + "script", "odoo", "migration", "reset_stale_cow_views.py" + ), + "-d", + database, + ] + for key in lst_key: + cmd += ["--reset", key] + cmd.append("--apply") + done = subprocess.run(cmd, capture_output=True, text=True) + return done.returncode, done.stdout + done.stderr + + +def prompt(database, lst_failure, lst_key, ask=input): + """Proposer de corriger, puis dire ce qu'il reste. Rend les clés traitées. + + Détecter sans offrir le geste, c'est laisser relever des identifiants + dans une trace pour les traduire en clés à la main — au moment précis où + l'on veut juste que la page réponde. + """ + if not lst_key: + print(f"ℹ -> {t('No parent view named: nothing to offer.')}") + return [] + print(f"\n✨ {t('Copies to reset onto their module view')} :") + for index, key in enumerate(lst_key, start=1): + print(f" [{index}] {key}") + print(f" [a] {t('All of the list above')}") + answer = ( + ask( + f"💬 {t('Which one(s) to reset?')}" + f" ({t('numbers separated by commas, a = all, empty =')}" + f" {t('nothing')}) : " + ) + .strip() + .lower() + ) + if not answer: + print(f"ℹ -> {t('Kept. Nothing was reset.')}") + return [] + if answer == "a": + lst_chosen = list(lst_key) + else: + lst_chosen = [] + for part in answer.replace(" ", "").split(","): + if part.isdigit() and 1 <= int(part) <= len(lst_key): + lst_chosen.append(lst_key[int(part) - 1]) + if not lst_chosen: + print(f"⚠️ {t('Unknown choice, nothing was reset.')}") + return [] + status, output = apply_reset(database, lst_chosen) + print(output.strip()[-2000:]) + if status == 2: + print(f"❌ {t('Reset failed, nothing was changed.')}") + return [] + return lst_chosen + + +def run( + database, + port, + config_path, + limit=None, + timeout=30, + boot=180, + interactive=False, + auto_apply=False, + ask=input, +): + """Démarrer, interroger, arrêter, LIRE, éventuellement corriger, revérifier. + + L'ordre porte tout le correctif : un serveur qui écrit dans un fichier + bufferise et ne vide qu'en s'arrêtant. Lire avant l'arrêt donnait + vingt-quatre lignes de démarrage et zéro trace — donc « aucune vue en + cause » sur des pages qui en nommaient une. Mesuré deux fois avant d'être + compris. + """ + import tempfile + base_url = f"http://127.0.0.1:{port}" - server = start_server(database, port, config_path) + log_path = os.path.join( + tempfile.gettempdir(), f"erplibre_smoke_{database}_{port}.log" + ) + if port_is_taken(port): + raise RuntimeError( + f"{t('Something already listens on port')} {port} :" + f" {t('it would be tested instead of this database.')}" + ) + server = start_server(database, port, config_path, log_path=log_path) try: if not wait_ready(base_url, timeout=boot): raise RuntimeError( @@ -184,13 +405,78 @@ def run(database, port, config_path, limit=None, timeout=30, boot=180): raise RuntimeError(f"{t('Could not read')} {base_url}/sitemap.xml") if limit: lst_url = lst_url[:limit] - return lst_url, check_urls(lst_url, timeout=timeout) + lst_failure = check_urls(lst_url, timeout=timeout) finally: - server.terminate() + stop_server(server) + + if lst_failure: + lst_failure = attach_missing_parents(lst_failure, read_log(log_path)) + lst_key = culprit_keys(database, lst_failure) + if not lst_failure or not (interactive or auto_apply): + return lst_url, lst_failure, lst_key, None + + print(render(lst_url, lst_failure, lst_key)) + if auto_apply: + lst_done = lst_key + if lst_done: + code, output = apply_reset(database, lst_done) + print(output.strip()[-2000:]) + if code == 2: + lst_done = [] + else: + lst_done = prompt(database, lst_failure, lst_key, ask=ask) + if not lst_done: + return lst_url, lst_failure, lst_key, None + + server = start_server(database, port, config_path, log_path=log_path) + try: + if not wait_ready(base_url, timeout=boot): + raise RuntimeError( + f"{t('The server never answered on')} {base_url}" + ) + lst_again = check_urls( + [url for url, _s, _p in lst_failure], timeout=timeout + ) + finally: + stop_server(server) + return lst_url, lst_failure, lst_key, lst_again + + +def stop_server(server): + """Arrêter tout le GROUPE : sinon odoo-bin survit à son script.""" + try: + group = os.getpgid(server.pid) + except OSError: + group = None + if group is not None: try: - server.wait(timeout=30) - except subprocess.TimeoutExpired: - server.kill() + os.killpg(group, signal.SIGTERM) + except OSError: + pass + try: + server.wait(timeout=30) + except subprocess.TimeoutExpired: + if group is not None: + try: + os.killpg(group, signal.SIGKILL) + except OSError: + pass + server.kill() + handle = getattr(server, "erplibre_log", None) + if handle: + handle.close() + + +def port_is_taken(port, host="127.0.0.1"): + """Quelqu'un écoute-t-il déjà là ? + + Sans cette question, un serveur resté d'un essai précédent répond à + « le serveur est-il prêt ? », et l'on teste sa base à lui en croyant + tester la sienne. C'est exactement ce qui est arrivé ici. + """ + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: + sock.settimeout(1) + return sock.connect_ex((host, port)) == 0 def main(argv=None): @@ -207,6 +493,16 @@ def main(argv=None): "--limit", type=int, default=None, help="test only the first N URLs" ) parser.add_argument("--timeout", type=int, default=30) + parser.add_argument( + "--apply", + action="store_true", + help="reset the views in cause without asking (WRITES)", + ) + parser.add_argument( + "--report-only", + action="store_true", + help="never ask anything, even in front of a terminal", + ) parser.add_argument( "--boot-timeout", type=int, @@ -219,20 +515,31 @@ def main(argv=None): f"⧖ {t('Starting Odoo on')} '{config.database}'" f" ({t('port')} {config.port})…" ) + interactive = not config.report_only and sys.stdin.isatty() try: - lst_url, lst_failure = run( + lst_url, lst_failure, lst_key, lst_again = run( config.database, config.port, config.config, limit=config.limit, timeout=config.timeout, boot=config.boot_timeout, + interactive=interactive, + auto_apply=config.apply, ) except RuntimeError as exc: print(f"❌ {exc}") return 2 - print(render(lst_url, lst_failure)) - return 1 if lst_failure else 0 + if lst_again is None: + print(render(lst_url, lst_failure, lst_key)) + return 1 if lst_failure else 0 + # Après correction on ne redit pas le diagnostic : on dit ce qu'il RESTE. + print( + f"\n↻ {t('Re-checked the')} {len(lst_failure)}" + f" {t('failing URL(s) after the reset')} :" + ) + print(render([url for url, _s, _p in lst_failure], lst_again, None)) + return 1 if lst_again else 0 if __name__ == "__main__": diff --git a/script/todo/todo_i18n.py b/script/todo/todo_i18n.py index f9937f8..5e9537c 100644 --- a/script/todo/todo_i18n.py +++ b/script/todo/todo_i18n.py @@ -5022,6 +5022,46 @@ TRANSLATIONS = { "fr": "port", "en": "port", }, + "parent view(s) in cause": { + "fr": "vue(s) parente(s) en cause", + "en": "parent view(s) in cause", + }, + "Those parents are copies frozen on an older version;": { + "fr": "Ces parents sont des copies figées sur une version antérieure ;", + "en": "Those parents are copies frozen on an older version;", + }, + "resetting them onto the module view is the fix": { + "fr": "les réinitialiser sur la vue module est le correctif", + "en": "resetting them onto the module view is the fix", + }, + "No parent view named: nothing to offer.": { + "fr": "Aucune vue parente nommée : rien à proposer.", + "en": "No parent view named: nothing to offer.", + }, + "Copies to reset onto their module view": { + "fr": "Copies à réinitialiser sur leur vue module", + "en": "Copies to reset onto their module view", + }, + "Which one(s) to reset?": { + "fr": "Laquelle ou lesquelles réinitialiser ?", + "en": "Which one(s) to reset?", + }, + "Re-checked the": { + "fr": "Revérification des", + "en": "Re-checked the", + }, + "failing URL(s) after the reset": { + "fr": "URL en échec après la réinitialisation", + "en": "failing URL(s) after the reset", + }, + "Something already listens on port": { + "fr": "Quelque chose écoute déjà sur le port", + "en": "Something already listens on port", + }, + "it would be tested instead of this database.": { + "fr": "il serait testé à la place de cette base.", + "en": "it would be tested instead of this database.", + }, "Nothing to decide yet": { "fr": "Rien à décider pour l'instant", "en": "Nothing to decide yet", diff --git a/test/test_smoke_public_url.py b/test/test_smoke_public_url.py index 242068e..3ebad84 100755 --- a/test/test_smoke_public_url.py +++ b/test/test_smoke_public_url.py @@ -107,7 +107,7 @@ class TestWhatCountsAsAFailure(unittest.TestCase): def test_a_500_fails(self): self.answers["http://h/bad"] = (500, "") self.assertEqual( - smoke.check_urls(["http://h/bad"]), [("http://h/bad", 500)] + smoke.check_urls(["http://h/bad"]), [("http://h/bad", 500, [])] ) def test_a_404_fails_too(self): @@ -119,7 +119,7 @@ class TestWhatCountsAsAFailure(unittest.TestCase): def test_no_answer_at_all_fails(self): self.answers["http://h/dead"] = (0, "") self.assertEqual( - smoke.check_urls(["http://h/dead"]), [("http://h/dead", 0)] + smoke.check_urls(["http://h/dead"]), [("http://h/dead", 0, [])] ) def test_a_200_passes(self): @@ -141,7 +141,7 @@ class TestTheReport(unittest.TestCase): self.assertIn("✅", text) def test_a_failure_shows_the_status_and_the_url(self): - text = smoke.render(["a"], [("http://h/blog/x", 500)]) + text = smoke.render(["a"], [("http://h/blog/x", 500, [])]) self.assertIn("500", text) self.assertIn("http://h/blog/x", text) @@ -169,10 +169,18 @@ class TestTheServerIsAlwaysStopped(unittest.TestCase): original_start = smoke.start_server original_wait = smoke.wait_ready - smoke.start_server = lambda db, port, cfg="./config.conf": FakeServer() + original_stop = smoke.stop_server + original_port = smoke.port_is_taken + smoke.start_server = ( + lambda db, port, cfg="./config.conf", log_path=None: FakeServer() + ) smoke.wait_ready = lambda base, timeout=180, sleep=2: False + smoke.stop_server = lambda server: stopped.append("terminate") + smoke.port_is_taken = lambda port, host="127.0.0.1": False self.addCleanup(setattr, smoke, "start_server", original_start) self.addCleanup(setattr, smoke, "wait_ready", original_wait) + self.addCleanup(setattr, smoke, "stop_server", original_stop) + self.addCleanup(setattr, smoke, "port_is_taken", original_port) with self.assertRaises(RuntimeError): smoke.run("db", 8169, "./config.conf") self.assertEqual(stopped, ["terminate"]) @@ -183,6 +191,130 @@ class TestTheServerIsAlwaysStopped(unittest.TestCase): self.assertNotEqual(smoke.DEFAULT_PORT, 8069) +class TestTheCulpritViewsAreNamed(unittest.TestCase): + """Détecter sans nommer laisse relever des ids dans une trace à la main.""" + + def test_the_parent_is_read_from_the_error_context(self): + # C'est le PARENT le coupable : la copie figée dans laquelle + # l'enfant ne trouve plus son xpath. + line = "[view_id: 3282, xml_id: n/a, model: n/a, parent_id: 3281]" + self.assertEqual(smoke.RE_CONTEXT.findall(line), [("3282", "3281")]) + + def test_the_context_line_is_not_translated(self): + # La phrase qui précède l'est, celle-ci non : la lire marche donc + # sur un système français comme anglais. + self.assertEqual( + smoke.RE_CONTEXT.findall("[view_id: 1, parent_id: 2]"), + [("1", "2")], + ) + + def test_a_late_context_is_still_attached(self): + # Odoo vide son tampon à l'arrêt : le journal se lit APRÈS, et rien + # ne doit dépendre du moment où la ligne est apparue. + lst_failure = [("http://h/a", 500, [])] + log = ["[view_id: 3288, model: n/a, parent_id: 2841]"] + rebuilt = smoke.attach_missing_parents(lst_failure, log) + self.assertEqual(rebuilt[0][2], ["2841"]) + + def test_an_already_attributed_parent_is_not_duplicated(self): + lst_failure = [("http://h/a", 500, ["2841"])] + log = ["[view_id: 3288, model: n/a, parent_id: 2841]"] + rebuilt = smoke.attach_missing_parents(lst_failure, log) + self.assertEqual(rebuilt[0][2], ["2841"]) + + +class TestTheServerIsKilledForReal(unittest.TestCase): + """« ./run.sh » est un script bash : il ne transmet pas les signaux.""" + + def test_it_starts_its_own_process_group(self): + # Sans cela, terminate() tue l'enveloppe et laisse odoo-bin vivant. + # Mesuré : six serveurs orphelins, un par essai, et les essais + # suivants interrogeaient sans le savoir celui d'avant. + import inspect + + source = inspect.getsource(smoke.start_server) + self.assertIn("start_new_session=True", source) + + def test_it_kills_the_group_not_just_the_wrapper(self): + import inspect + + source = inspect.getsource(smoke.stop_server) + self.assertIn("killpg", source) + + def test_a_taken_port_is_refused_not_tested(self): + # Un serveur resté d'un essai précédent répond « prêt » : on + # testerait sa base à lui en croyant tester la sienne. + original = smoke.port_is_taken + smoke.port_is_taken = lambda port, host="127.0.0.1": True + self.addCleanup(setattr, smoke, "port_is_taken", original) + with self.assertRaises(RuntimeError) as caught: + smoke.run("db", 8169, "./config.conf") + self.assertIn("8169", str(caught.exception)) + + +class TestOfferingTheFix(unittest.TestCase): + def setUp(self): + from script.todo import todo_i18n + + self.addCleanup( + setattr, todo_i18n, "_current_lang", todo_i18n._current_lang + ) + todo_i18n._current_lang = "en" + self.applied = [] + original = smoke.apply_reset + smoke.apply_reset = lambda db, keys: ( + self.applied.append(keys), + (0, "ok"), + )[1] + self.addCleanup(setattr, smoke, "apply_reset", original) + + def run_prompt(self, answer, lst_key=None): + import contextlib + import io + + if lst_key is None: + lst_key = ["website_blog.blog_post_complete", "website_form.x"] + out = io.StringIO() + with contextlib.redirect_stdout(out): + done = smoke.prompt( + "db", + [("http://h/a", 500, ["2841"])], + lst_key, + ask=lambda prompt: answer, + ) + return done, out.getvalue() + + def test_every_key_is_numbered(self): + _done, text = self.run_prompt("") + self.assertIn("[1] website_blog.blog_post_complete", text) + self.assertIn("[a]", text) + + def test_a_number_applies_only_that_one(self): + done, _text = self.run_prompt("1") + self.assertEqual(done, ["website_blog.blog_post_complete"]) + self.assertEqual(self.applied, [["website_blog.blog_post_complete"]]) + + def test_a_applies_them_all(self): + done, _text = self.run_prompt("a") + self.assertEqual(len(done), 2) + + def test_empty_applies_nothing(self): + done, text = self.run_prompt("") + self.assertEqual(done, []) + self.assertEqual(self.applied, []) + self.assertIn("Kept", text) + + def test_an_unknown_answer_applies_nothing(self): + done, text = self.run_prompt("9") + self.assertEqual(self.applied, []) + self.assertIn("Unknown choice", text) + + def test_no_key_found_says_so(self): + done, text = self.run_prompt("a", lst_key=[]) + self.assertEqual(done, []) + self.assertIn("nothing to offer", text) + + class TestTheMigrationOffersIt(unittest.TestCase): def setUp(self): from script.todo import todo_i18n