From 4e0596721c6a7d00622488edaed0b530c3cbf68f Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Fri, 21 Aug 2026 23:20:08 -0400 Subject: [PATCH] [FIX] filestore: five defects a real repair session brought out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deduplicating by file was right for COUNTING and wrong for DELETING: twenty-two rows shared two files, so the purge only ever offered one at a time and had to be replayed. It now gathers every row whose own field is dead, and never a live row sharing the same file. "root" held the root of all filestores instead of this database's directory, so tidying looked for a nested folder at /filestore/filestore and answered "nothing to tidy" in front of 1168 stranded files. It now finds 112 to move up and 1056 duplicates. limit=0 means "no cap" everywhere else, but the slice [:0] is empty: "Show every entry" printed "… 3 more" and showed none of them. The report was captured once, so after a purge it replayed the state from before -- one then purged rows already gone, believing the work unfinished. A repair now says whether it did something, and the report is re-read when it did. "DELETE 0" was announced as a success. The count now comes from what PostgreSQL said, and saying nothing is not the same as deleting nothing. --- FR --- Dédupliquer par fichier était juste pour COMPTER et faux pour EFFACER : vingt-deux lignes partageaient deux fichiers, la purge n'en offrait qu'une à la fois. Elle prend maintenant toutes les lignes dont le champ est mort, et jamais une ligne vivante partageant le même fichier. « root » portait la racine de tous les filestores au lieu du dossier de la base : le rangement cherchait à /filestore/filestore et répondait « rien à ranger » devant 1168 fichiers échoués. Il en trouve 112 à remonter et 1056 doublons. limit=0 veut dire « tout » partout ailleurs, mais [:0] est vide : « Tout afficher » annonçait « … 3 de plus » sans en montrer un seul. Le rapport n'était lu qu'une fois : après une purge il rejouait l'état d'avant, et l'on repurgeait des lignes déjà effacées. Une réparation dit maintenant si elle a fait quelque chose, et le rapport est relu alors. « DELETE 0 » passait pour un succès. Le compte vient de ce que PostgreSQL a annoncé, et ne rien dire n'est pas ne rien supprimer. Assisted-by: Claude Opus 5 --- script/analyse/check_filestore.py | 40 ++++- script/todo/todo.py | 46 ++++-- test/test_check_filestore.py | 251 +++++++++++++++++++++++++++--- 3 files changed, 294 insertions(+), 43 deletions(-) diff --git a/script/analyse/check_filestore.py b/script/analyse/check_filestore.py index 23ba9db..2a38bf9 100755 --- a/script/analyse/check_filestore.py +++ b/script/analyse/check_filestore.py @@ -388,6 +388,10 @@ def audit(database, config_path=None, backups=None): return {"unavailable": True, "database": database} racine = filestore_root(config_path) mien = os.path.join(racine, database) + # `root` désigne le dossier de CETTE base. Y mettre la racine de tous + # les filestores faisait chercher le dossier imbriqué à + # `/filestore/filestore` — inexistant — et l'outil + # répondait « rien à ranger » devant 1168 fichiers échoués. present = set() if os.path.isdir(mien): for prefixe in os.listdir(mien): @@ -403,12 +407,19 @@ def audit(database, config_path=None, backups=None): groupes = {verdict: [] for verdict in VERDICTS} vus = set() + morts = [] for piece in pieces: verdict = classify( piece, present, ailleurs, niches, sauvegardes, champs_vivants ) if not verdict: continue + # La déduplication qui suit sert à compter des FICHIERS. Pour + # effacer des LIGNES il les faut toutes : vingt-deux lignes + # partageaient deux fichiers, et la purge n'en offrait qu'une à + # la fois — il fallait la relancer vingt-deux fois. + if verdict[0] == "dead_field" and str(piece.get("id", "")).isdigit(): + morts.append(int(piece["id"])) # Plusieurs pièces jointes partagent un fichier quand leur contenu # est identique : le compter une fois par ligne gonflerait le # rapport sans ajouter un seul fichier à récupérer. @@ -426,7 +437,8 @@ def audit(database, config_path=None, backups=None): "missing": len(vus), "groups": groupes, "nested_total": len(niches), - "root": racine, + "root": mien, + "dead_ids": morts, } @@ -461,7 +473,7 @@ def render(rapport, limit=20): if len(apercu) > 4: lignes.append(f" … {len(apercu) - 4} {t('more')}") continue - for piece in groupe[:limit]: + for piece in groupe[: limit or None]: ou = ( f"{piece['model']} #{piece['res_id']}" if piece["model"] @@ -473,7 +485,7 @@ def render(rapport, limit=20): f" {piece['size'] // 1024:>6} ko {ou}{champ}" f" {piece['created']} {alive_mark(piece)}" ) - if len(groupe) > limit: + if limit and len(groupe) > limit: lignes.append(f" … {len(groupe) - limit} {t('more')}") return lignes + render_nested(rapport) @@ -523,17 +535,29 @@ def purge_dead_sql(rapport): porte, et rejouer ce raisonnement en SQL laisserait la porte ouverte à effacer autre chose que ce qui a été montré. """ - ids = sorted( - int(piece["id"]) - for piece in rapport["groups"]["dead_field"] - if str(piece.get("id", "")).isdigit() - ) + ids = sorted(set(rapport.get("dead_ids") or [])) if not ids: return "" liste = ", ".join(str(i) for i in ids) return f"DELETE FROM ir_attachment WHERE id IN ({liste})" +def rows_deleted(sortie): + """Le nombre annoncé par « DELETE n », ou None si rien ne le dit. + + None n'est pas zéro : « la commande n'a rien annoncé » et « elle n'a + rien supprimé » appellent des mots différents. Les confondre ferait + taire une panne ou inventer un succès. + """ + lignes = sortie if isinstance(sortie, list) else str(sortie).splitlines() + for ligne in reversed([str(x).strip() for x in lignes]): + if ligne.startswith("DELETE "): + reste = ligne[len("DELETE ") :].strip() + if reste.isdigit(): + return int(reste) + return None + + def nested_dir(rapport): """Le dossier imbriqué de cette base, s'il en existe un.""" chemin = os.path.join(rapport["root"], "filestore") diff --git a/script/todo/todo.py b/script/todo/todo.py index d9ccbb0..d8ea5c6 100755 --- a/script/todo/todo.py +++ b/script/todo/todo.py @@ -10290,18 +10290,32 @@ class TODO: if rapport.get("unavailable"): print(f"❌ {t('Cannot read the database: ')}{database}") return + etat = {"rapport": rapport} print("\n".join(filestore.render(rapport, limit=20))) + def relire(): + """Relire APRÈS une réparation. + + Sans cela « Tout afficher » rejouait le rapport d'avant : + on purgeait, on relisait, et l'on voyait encore ce qui + venait de disparaître. Pire, on repurgeait des lignes déjà + effacées en croyant le travail inachevé. + """ + print(f"⧖ {t('Scanning filestores and backups…')}") + etat["rapport"] = filestore.audit(database) + def handler(rank): if rank == 1: - print("\n".join(filestore.render(rapport, limit=0))) + print("\n".join(filestore.render(etat["rapport"], limit=0))) elif rank == 2: - self._filestore_purge_dead(database, rapport) + if self._filestore_purge_dead(database, etat["rapport"]): + relire() elif rank == 3: - self._filestore_tidy_nested(rapport) + if self._filestore_tidy_nested(etat["rapport"]): + relire() else: self._analyse_export_json( - rapport, os.path.basename(database), "filestore" + etat["rapport"], os.path.basename(database), "filestore" ) self._analyse_follow_up( @@ -10338,7 +10352,7 @@ class TODO: sql = filestore.purge_dead_sql(rapport) if not sql: print(f"ℹ️ {t('Nothing to purge.')}") - return + return False print() for texte in filestore.summarise(lignes): print(f" {texte}") @@ -10352,16 +10366,25 @@ class TODO: "o", ): print(f"ℹ️ {t('Nothing was deleted.')}") - return - status = self.execute.exec_command_live( + return False + status, sortie = self.execute.exec_command_live( f'psql -d {database} -c "{sql}"', source_erplibre=False, single_source_erplibre=True, + return_status_and_output=True, ) if status: print(f"❌ {t('The purge failed.')}") - return - print(f"✅ {len(lignes)} {t('attachment row(s) deleted.')}") + return False + # Le nombre ANNONCÉ par PostgreSQL, pas celui qu'on espérait : + # rejouer une purge déjà faite rendait « DELETE 0 » et l'outil + # se félicitait quand même d'avoir supprimé. + efface = filestore.rows_deleted(sortie) + if efface is None: + print(f"⚠ {t('The purge ran but said nothing.')}") + return True + print(f"✅ {efface} {t('attachment row(s) deleted.')}") + return True def _filestore_tidy_nested(self, rapport): """Remonter ce qui manque, effacer les doublons purs. @@ -10376,7 +10399,7 @@ class TODO: remonter, doublons = filestore.tidy_nested_plan(rapport) if not remonter and not doublons: print(f"ℹ️ {t('No nested filestore to tidy.')}") - return + return False print() print(f" {len(remonter)} {t('file(s) to move up')}") print(f" {len(doublons)} {t('pure duplicate(s) to delete')}") @@ -10385,7 +10408,7 @@ class TODO: f"💬 {t('Go ahead?')} (y/N) : ", default="n" ).strip().lower() not in ("y", "yes", "o"): print(f"ℹ️ {t('Nothing was moved.')}") - return + return False deplaces, effaces = 0, 0 for source, cible in remonter: os.makedirs(os.path.dirname(cible), exist_ok=True) @@ -10401,6 +10424,7 @@ class TODO: f"✅ {deplaces} {t('moved up')}, {effaces}" f" {t('duplicate(s) removed')}." ) + return True def _analyse_offer_install(self, database, rapport): """Proposer d'installer ce qui manque, quand c'est installable. diff --git a/test/test_check_filestore.py b/test/test_check_filestore.py index 0afd69a..412185b 100644 --- a/test/test_check_filestore.py +++ b/test/test_check_filestore.py @@ -231,6 +231,40 @@ class TestTheAudit(unittest.TestCase): fs.attachments = lambda base: None self.assertTrue(fs.audit("db")["unavailable"]) + def test_every_row_sharing_a_dead_file_is_listed_for_deletion(self): + # Vingt-deux lignes partageaient deux fichiers : la purge n'en + # offrait qu'une à la fois, et il fallait la relancer vingt-deux + # fois. Compter des FICHIERS et effacer des LIGNES ne sont pas la + # même opération. + fs.attachments = lambda base: [ + piece("aa/bb", "res.country", "image", res_id="1", pid="10"), + piece("aa/bb", "res.country", "image", res_id="2", pid="11"), + piece("cc/dd", "res.country", "image", res_id="3", pid="12"), + ] + rapport = fs.audit("db") + self.assertEqual(rapport["missing"], 2) + self.assertEqual(len(rapport["groups"]["dead_field"]), 2) + self.assertEqual(sorted(rapport["dead_ids"]), [10, 11, 12]) + self.assertIn("(10, 11, 12)", fs.purge_dead_sql(rapport)) + + def test_a_live_row_sharing_a_file_is_never_swept_along(self): + # Deux lignes, un seul fichier, mais un seul champ mort : effacer + # les deux emporterait une pièce jointe bien vivante. + fs.live_fields = lambda base: {"project.task.attachment"} + fs.attachments = lambda base: [ + piece("aa/bb", "res.country", "image", pid="10"), + piece("aa/bb", "project.task", "attachment", pid="11"), + ] + rapport = fs.audit("db") + self.assertEqual(rapport["dead_ids"], [10]) + + def test_the_root_points_at_this_database_directory(self): + # Y mettre la racine de tous les filestores faisait chercher le + # dossier imbriqué à `/filestore/filestore` : l'outil + # répondait « rien à ranger » devant 1168 fichiers échoués. + fs.attachments = lambda base: [] + self.assertEqual(fs.audit("ma_base")["root"], "/nulle/part/ma_base") + def test_a_dead_field_never_lands_in_lost(self): # Le garde du champ disparu doit VRAIMENT couper : sans lui, ces # lignes gonfleraient les pertes réelles. @@ -271,6 +305,34 @@ class TestTheReport(unittest.TestCase): self.assertNotIn("recuperable.png", texte) self.assertIn("res.country / image", texte) + def test_limit_zero_shows_EVERYTHING(self): + # « Tout afficher » passe limit=0. Une tranche [:0] est vide : + # l'écran annonçait « … 3 de plus » et ne montrait rien du tout. + groupes = {v: [] for v in fs.VERDICTS} + groupes["lost"] = [ + piece(f"a/{i}", "project.task", name=f"perdu{i}.png") + for i in range(3) + ] + texte = "\n".join( + fs.render(self.rapport(missing=3, groups=groupes), limit=0) + ) + for i in range(3): + self.assertIn(f"perdu{i}.png", texte) + self.assertNotIn(todo_i18n.t("more"), texte) + + def test_a_limit_still_caps_and_says_how_many_were_hidden(self): + groupes = {v: [] for v in fs.VERDICTS} + groupes["lost"] = [ + piece(f"a/{i}", "project.task", name=f"perdu{i}.png") + for i in range(5) + ] + texte = "\n".join( + fs.render(self.rapport(missing=5, groups=groupes), limit=2) + ) + self.assertIn("perdu0.png", texte) + self.assertNotIn("perdu4.png", texte) + self.assertIn(f"3 {todo_i18n.t('more')}", texte) + def test_the_nested_pile_is_reported(self): texte = "\n".join(fs.render(self.rapport(nested_total=1168))) self.assertIn("1168", texte) @@ -349,27 +411,45 @@ class TestTheLivingRecord(unittest.TestCase): class TestTheRepairs(unittest.TestCase): - def rapport(self, morts=(), racine="/fs/db"): + def rapport(self, ids=(), racine="/fs/db"): base = {v: [] for v in fs.VERDICTS} - base["dead_field"] = list(morts) - return {"root": racine, "groups": base} + return {"root": racine, "groups": base, "dead_ids": list(ids)} def test_the_purge_deletes_by_id_only(self): # Par IDENTIFIANT, jamais par un domaine reconstruit : rejouer le # raisonnement en SQL ouvrirait la porte à effacer autre chose # que ce qui a été montré. - sql = fs.purge_dead_sql( - self.rapport([piece("a/1", pid="3"), piece("b/2", pid="9")]) - ) + sql = fs.purge_dead_sql(self.rapport([9, 3])) self.assertIn("WHERE id IN (3, 9)", sql) self.assertNotIn("res_model", sql) def test_nothing_dead_gives_no_sql(self): self.assertEqual(fs.purge_dead_sql(self.rapport()), "") - def test_a_row_without_an_id_is_never_deleted(self): - sql = fs.purge_dead_sql(self.rapport([piece("a/1", pid="")])) - self.assertEqual(sql, "") + def test_a_report_without_the_key_gives_no_sql(self): + self.assertEqual(fs.purge_dead_sql({"groups": {}}), "") + + +class TestReadingWhatPostgresSaid(unittest.TestCase): + def test_it_reads_the_count(self): + self.assertEqual(fs.rows_deleted(["DELETE 250"]), 250) + + def test_zero_is_zero_not_success(self): + # « DELETE 0 » se félicitait d'avoir supprimé : rejouer une purge + # déjà faite annonçait un travail qui n'avait pas eu lieu. + self.assertEqual(fs.rows_deleted(["DELETE 0"]), 0) + + def test_it_takes_the_LAST_word(self): + self.assertEqual(fs.rows_deleted(["DELETE 5", "bruit", "DELETE 7"]), 7) + + def test_silence_is_not_zero(self): + # « rien annoncé » et « rien supprimé » appellent des mots + # différents : les confondre tait une panne. + self.assertIsNone(fs.rows_deleted(["ERROR: boom"])) + self.assertIsNone(fs.rows_deleted([])) + + def test_a_plain_string_works_too(self): + self.assertEqual(fs.rows_deleted("DELETE 3\n"), 3) class TestTidyingTheNested(unittest.TestCase): @@ -422,16 +502,14 @@ class TestTheRepairMenu(unittest.TestCase): self.vrai_ask = auto_ask.ask self.lancees = [] self.obj = todo_module.TODO.__new__(todo_module.TODO) - self.obj.execute = type( - "E", - (), - { - "exec_command_live": lambda _s, cmd, **k: self.lancees.append( - cmd - ) - or 0 - }, - )() + + def faux_exec(_self, cmd, **kw): + self.lancees.append(cmd) + if kw.get("return_status_and_output"): + return 0, ["DELETE 1"] + return 0 + + self.obj.execute = type("E", (), {"exec_command_live": faux_exec})() def tearDown(self): self.auto_ask.ask = self.vrai_ask @@ -445,29 +523,33 @@ class TestTheRepairMenu(unittest.TestCase): self.auto_ask.ask = faux - def rapport(self, morts=()): + def rapport(self, morts=(), ids=()): groupes = {v: [] for v in fs.VERDICTS} groupes["dead_field"] = list(morts) - return {"root": "/fs/db", "groups": groupes} + return {"root": "/fs/db", "groups": groupes, "dead_ids": list(ids)} def test_saying_no_deletes_nothing(self): self.repond("n") with redirect_stdout(io.StringIO()): - self.obj._filestore_purge_dead("db", self.rapport([piece("a/1")])) + self.obj._filestore_purge_dead( + "db", self.rapport([piece("a/1")], [3]) + ) self.assertEqual(self.lancees, []) def test_pressing_enter_deletes_nothing(self): # Le défaut d'une SUPPRESSION doit être de ne rien faire. self.repond("") with redirect_stdout(io.StringIO()): - self.obj._filestore_purge_dead("db", self.rapport([piece("a/1")])) + self.obj._filestore_purge_dead( + "db", self.rapport([piece("a/1")], [3]) + ) self.assertEqual(self.lancees, []) def test_accepting_runs_a_command_that_actually_exists(self): self.repond("y") with redirect_stdout(io.StringIO()): self.obj._filestore_purge_dead( - "db", self.rapport([piece("a/1", pid="3")]) + "db", self.rapport([piece("a/1")], [3]) ) self.assertEqual(len(self.lancees), 1) self.assertIn("psql -d db", self.lancees[0]) @@ -497,6 +579,127 @@ class TestTheRepairMenu(unittest.TestCase): # qui empêche Entrée de supprimer. self.assertIn('auto_ask.ask(question, default="n")', src) + def test_a_delete_that_changed_nothing_is_NOT_a_success(self): + # « DELETE 0 » se félicitait d'avoir supprimé. Rejouer une purge + # déjà faite annonçait donc un travail qui n'avait pas eu lieu. + def rien(_self, cmd, **kw): + self.lancees.append(cmd) + return ( + (0, ["DELETE 0"]) if kw.get("return_status_and_output") else 0 + ) + + self.obj.execute = type("E", (), {"exec_command_live": rien})() + self.repond("y") + tampon = io.StringIO() + with redirect_stdout(tampon): + self.obj._filestore_purge_dead( + "db", self.rapport([piece("a/1")], [3]) + ) + self.assertIn("0 ", tampon.getvalue()) + self.assertNotIn("✅ 1 ", tampon.getvalue()) + + def test_a_silent_command_is_flagged_not_counted(self): + def muet(_self, cmd, **kw): + self.lancees.append(cmd) + return ( + (0, ["rien du tout"]) + if kw.get("return_status_and_output") + else 0 + ) + + self.obj.execute = type("E", (), {"exec_command_live": muet})() + self.repond("y") + tampon = io.StringIO() + with redirect_stdout(tampon): + self.obj._filestore_purge_dead( + "db", self.rapport([piece("a/1")], [3]) + ) + self.assertIn( + todo_i18n.t("The purge ran but said nothing."), tampon.getvalue() + ) + + def test_a_repair_reports_whether_it_did_something(self): + # C'est ce booléen qui déclenche la relecture du rapport : sans + # lui, « Tout afficher » rejouerait l'état d'avant la purge. + self.repond("n") + with redirect_stdout(io.StringIO()): + refus = self.obj._filestore_purge_dead( + "db", self.rapport([piece("a/1")], [3]) + ) + self.repond("y") + with redirect_stdout(io.StringIO()): + fait = self.obj._filestore_purge_dead( + "db", self.rapport([piece("a/1")], [3]) + ) + self.assertFalse(refus) + self.assertTrue(fait) + + +class TestTheReportIsRereadAfterARepair(unittest.TestCase): + """« Tout afficher » doit montrer l'APRÈS, pas l'avant. + + Sans relecture on purgeait, on relisait, et l'on voyait encore ce + qui venait de disparaître — puis on repurgeait des lignes déjà + effacées en croyant le travail inachevé. C'est arrivé en vrai. + """ + + def setUp(self): + from script.todo import auto_ask + from script.todo import todo as todo_module + + self.auto_ask = auto_ask + self.vrai_ask = auto_ask.ask + self.vrai_audit = fs.audit + self.appels = [] + self.obj = todo_module.TODO.__new__(todo_module.TODO) + self.obj._analyse_select_database = lambda: "db" + self.obj._filestore_purge_dead = lambda base, rapport: True + self.obj._filestore_tidy_nested = lambda rapport: True + + def audit(base, *a, **k): + self.appels.append(base) + return { + "database": base, + "attachments": 1, + "files_present": 1, + "missing": 0, + "nested_total": 0, + "root": "/fs/db", + "groups": {v: [] for v in fs.VERDICTS}, + "dead_ids": [], + } + + fs.audit = audit + self.auto_ask.ask = lambda p, default="", seconds=None: "n" + + def tearDown(self): + self.auto_ask.ask = self.vrai_ask + fs.audit = self.vrai_audit + + def joue(self, rang): + self.obj._analyse_follow_up = lambda choix, handler: handler(rang) + with redirect_stdout(io.StringIO()): + self.obj.execute_analyse_filestore() + + def test_a_purge_triggers_a_fresh_read(self): + self.joue(2) + self.assertEqual(len(self.appels), 2) + + def test_a_tidy_triggers_a_fresh_read(self): + self.joue(3) + self.assertEqual(len(self.appels), 2) + + def test_merely_displaying_does_not(self): + # Relire pour afficher coûterait un balayage complet des + # filestores et des zips à chaque coup d'œil. + self.joue(1) + self.assertEqual(len(self.appels), 1) + + def test_a_refused_repair_does_not_reread(self): + self.obj._filestore_purge_dead = lambda base, rapport: False + self.joue(2) + self.assertEqual(len(self.appels), 1) + class TestTidyingForReal(unittest.TestCase): def setUp(self):