diff --git a/spp_hide_menus_base/README.rst b/spp_hide_menus_base/README.rst index 6ff430a3..05751bd8 100644 --- a/spp_hide_menus_base/README.rst +++ b/spp_hide_menus_base/README.rst @@ -103,6 +103,23 @@ Dependencies Changelog ========= +19.0.2.1.1 +~~~~~~~~~~ + +- fix: make the menu-hiding pass run by ``_register_hook`` (and + ``next()``) best-effort on database errors (#526). ``hide_menus()`` + now runs in its own savepoint and logs and skips any + ``psycopg2.Error`` instead of letting it escape ``_register_hook`` and + abort the registry load — on a poisoned cursor that meant every + restart failed until the database was repaired by hand, the shape of + the 2026-07-30 preprod incident (OpenSPP/odoo-job-worker#22), which + #383 first attributed to the ``spp_base_common`` menu-icon hook. The + caller's own pending writes are flushed before the guard and stay the + caller's error; a missing menu xmlid is skipped through + ``raise_if_not_found=False``. Only ``psycopg2.Error`` is caught, and + the pass stays all-or-nothing: a Python exception here is a bug in + this module and still fails loudly. + 19.0.2.1.0 ~~~~~~~~~~ diff --git a/spp_hide_menus_base/__manifest__.py b/spp_hide_menus_base/__manifest__.py index 79db6476..274d4282 100644 --- a/spp_hide_menus_base/__manifest__.py +++ b/spp_hide_menus_base/__manifest__.py @@ -5,7 +5,7 @@ { "name": "OpenSPP Hide Non-OpenSPP Menus: Base", "category": "OpenSPP", - "version": "19.0.2.1.0", + "version": "19.0.2.1.1", "summary": "Administrators can manage the visibility of OpenSPP navigation menus, streamlining the user interface for specific user groups. The module modifies ir.ui.menu records to control menu visibility, providing a foundation for other modules to selectively hide non-essential navigation items.", "sequence": 1, "author": "OpenSPP.org", diff --git a/spp_hide_menus_base/models/ir_module_module.py b/spp_hide_menus_base/models/ir_module_module.py index d2c0c628..b0e3dba9 100644 --- a/spp_hide_menus_base/models/ir_module_module.py +++ b/spp_hide_menus_base/models/ir_module_module.py @@ -1,5 +1,7 @@ import logging +import psycopg2 + from odoo import models _logger = logging.getLogger(__name__) @@ -58,41 +60,74 @@ class IrModuleModule(models.Model): } def hide_menus(self): + """Hide the root menus of the stock apps listed in MENU_APP. + + Best-effort: this runs from ``_register_hook`` at the end of every + registry load, where a database error that escapes aborts the load + and takes the instance (or a job worker) down with it. A menu left + visible is recoverable, so the whole pass runs in its own savepoint + and is logged and skipped on any ``psycopg2.Error``. + + The caller's pending ORM writes are flushed first so that only the + hiding pass itself is covered by the guard; a failure in the caller's + own writes stays the caller's error. Retryable errors (serialization + failures, deadlocks) are deliberately swallowed too: the registry + load has no retry loop, and on the ``next()`` path a retry would + rebuild the registry for a menu write. + + Two deliberate limits. Only ``psycopg2.Error`` is caught: a Python + exception raised here is a bug in this module and must fail loudly + in tests rather than silently leave menus visible in production. And + the pass is all-or-nothing, as in ``spp_base_common``: database + errors are transaction-wide in practice, so a savepoint per catalog + entry would add cost without adding recoverable cases. + """ + self.env.cr.flush() + try: + with self.env.cr.savepoint(): + self._hide_catalog_menus() + except psycopg2.Error: + _logger.warning( + "Skipping the OpenSPP menu hiding pass because the database reported an error; " + "the menus keep their current visibility and this hook will not be the cause of a failed registry load", + exc_info=True, + ) + + def _hide_catalog_menus(self): for module in self.search([]): menu_info = self.MENU_APP.get(module.name) - if menu_info: - try: - menu = self.env.ref(menu_info["menu_xml_id"]) - except ValueError: - _logger.debug("Menu XML ID not found: %s", menu_info["menu_xml_id"]) - menu = False + if not menu_info: + continue + menu = self.env.ref(menu_info["menu_xml_id"], raise_if_not_found=False) + if not menu: + _logger.debug("Menu XML ID not found: %s", menu_info["menu_xml_id"]) + continue - if menu: - hidden_menus = self.env["spp.hide.menu"].search([("menu_id", "=", menu.id)]) - if not hidden_menus: - hidden_menu = self.env["spp.hide.menu"].create( - { - "menu_id": menu.id, - "xml_id": menu_info["menu_xml_id"], - } - ) - hidden_menu.hide_menu() - continue + hidden_menus = self.env["spp.hide.menu"].search([("menu_id", "=", menu.id)]) + if not hidden_menus: + hidden_menu = self.env["spp.hide.menu"].create( + { + "menu_id": menu.id, + "xml_id": menu_info["menu_xml_id"], + } + ) + hidden_menu.hide_menu() + continue - # Read state off ONE row, never off the search result. This - # method runs from _register_hook, so an Expected singleton - # here aborts the whole registry load and every request 500s - # until the extra row is deleted by hand. UNIQUE(menu_id) - # normally rules that out, but a database that already held - # duplicates when the constraint landed keeps them: the - # registry logs the failed constraint and carries on. - hidden_menu = hidden_menus._primary() - if hidden_menu.state == "show": - hidden_menu.hide_menu() - elif hidden_menu.state == "hide": - # Module upgrade may have reset group_ids via XML - # (noupdate="0"). Re-apply hiding if stale. - hidden_menu._reapply_hide() + # Read state off ONE row, never off the search result. This + # method runs from _register_hook, so an Expected singleton + # here aborts the whole registry load and every request 500s + # until the extra row is deleted by hand. UNIQUE(menu_id) + # normally rules that out, but a database that already held + # duplicates when the constraint landed keeps them: the + # registry logs the failed constraint and carries on. + hidden_menu = hidden_menus._primary() + if hidden_menu.state == "show": + hidden_menu.hide_menu() + elif hidden_menu.state == "hide": + # Module upgrade may have reset group_ids via XML + # (noupdate="0"). Re-apply hiding if stale. + hidden_menu._reapply_hide() def next(self): # Call your menu hiding logic first diff --git a/spp_hide_menus_base/readme/HISTORY.md b/spp_hide_menus_base/readme/HISTORY.md index a56bacf8..ca525306 100644 --- a/spp_hide_menus_base/readme/HISTORY.md +++ b/spp_hide_menus_base/readme/HISTORY.md @@ -1,3 +1,18 @@ +### 19.0.2.1.1 + +- fix: make the menu-hiding pass run by ``_register_hook`` (and ``next()``) + best-effort on database errors (#526). ``hide_menus()`` now runs in its own + savepoint and logs and skips any ``psycopg2.Error`` instead of letting it + escape ``_register_hook`` and abort the registry load — on a poisoned + cursor that meant every restart failed until the database was repaired by + hand, the shape of the 2026-07-30 preprod incident + (OpenSPP/odoo-job-worker#22), which #383 first attributed to the + ``spp_base_common`` menu-icon hook. The caller's own pending writes are + flushed before the guard and stay the caller's error; a missing menu xmlid + is skipped through ``raise_if_not_found=False``. Only ``psycopg2.Error`` is + caught, and the pass stays all-or-nothing: a Python exception here is a + bug in this module and still fails loudly. + ### 19.0.2.1.0 - Enforce ``UNIQUE(menu_id)`` on ``spp.hide.menu``: a second configuration row diff --git a/spp_hide_menus_base/static/description/index.html b/spp_hide_menus_base/static/description/index.html index 47178b92..205214a6 100644 --- a/spp_hide_menus_base/static/description/index.html +++ b/spp_hide_menus_base/static/description/index.html @@ -474,6 +474,24 @@

Changelog

+

19.0.2.1.1

+ +
+

19.0.2.1.0

-
+

19.0.2.0.1

-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_hide_menus_base/tests/__init__.py b/spp_hide_menus_base/tests/__init__.py index 8aa33c32..0cce5ddf 100644 --- a/spp_hide_menus_base/tests/__init__.py +++ b/spp_hide_menus_base/tests/__init__.py @@ -1,3 +1,4 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. from . import test_hide_menu from . import test_migration_dedup_hide_menu +from . import test_register_hook_guard diff --git a/spp_hide_menus_base/tests/test_register_hook_guard.py b/spp_hide_menus_base/tests/test_register_hook_guard.py new file mode 100644 index 00000000..33fa3b9d --- /dev/null +++ b/spp_hide_menus_base/tests/test_register_hook_guard.py @@ -0,0 +1,175 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Regression tests for the database-error guard around ``hide_menus()``. + +``hide_menus()`` runs from ``ir.module.module._register_hook`` at the end of +every registry load (startup, install, upgrade, worker reload). A +``psycopg2.Error`` escaping it aborts the registry load, so on a poisoned +cursor every restart fails until the database is repaired by hand. Hiding a +menu is best-effort there: a menu left visible is recoverable, an aborted +registry load is an outage. +""" + +import contextlib +import functools +from unittest.mock import patch + +import psycopg2 +import psycopg2.errors +import psycopg2.extensions + +from odoo.tests import TransactionCase, tagged +from odoo.tools import mute_logger + +HOOK_LOGGER = "odoo.addons.spp_hide_menus_base.models.ir_module_module" +MISSING_MENU_XMLID = "spp_hide_menus_base.test_526_menu_that_does_not_exist" + + +def _failing_lookup_for(menu_xml_id, original_lookup): + """An ``ir.model.data._xmlid_lookup`` stand-in that aborts the transaction + for one xmlid and behaves normally for every other one.""" + + @functools.wraps(original_lookup) + def _xmlid_lookup(model, xmlid): + if xmlid == menu_xml_id: + model.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist") + return original_lookup(model, xmlid) + + return _xmlid_lookup + + +@tagged("post_install", "-at_install") +class TestHideMenusDatabaseErrorGuard(TransactionCase): + def setUp(self): + super().setUp() + self.IrModule = self.env["ir.module.module"] + self.HideMenu = self.env["spp.hide.menu"] + self.hide_group = self.HideMenu._hide_group() + self.assertTrue(self.hide_group, "precondition: the hide group must exist") + + def _catalog(self, **entries): + """Replace MENU_APP with ``{module_name: menu_xml_id}`` for the test.""" + menu_app = {module: {"menu_xml_id": xml_id} for module, xml_id in entries.items()} + return patch.object(type(self.IrModule), "MENU_APP", menu_app) + + def _menu_without_hide_row(self): + """A menu no spp.hide.menu row points at yet.""" + taken = self.HideMenu.search([]).menu_id.ids + menu = self.env["ir.ui.menu"].search([("id", "not in", taken)], limit=1) + self.assertTrue(menu, "no unconfigured ir.ui.menu left to test against") + return menu + + def _menu_with_external_id(self, name): + """A menu with no hide configuration yet, reachable through a fresh xmlid.""" + menu = self._menu_without_hide_row() + self.env["ir.model.data"].create( + {"module": "spp_hide_menus_base", "name": name, "model": "ir.ui.menu", "res_id": menu.id} + ) + return menu, f"spp_hide_menus_base.{name}" + + def _first_and_last_module(self): + """Two module names at opposite ends of the order hide_menus() walks, + so a failure planted on the last one is reached after the first one + has already done its work.""" + modules = self.IrModule.search([]) + self.assertGreater(len(modules), 1) + return modules[0].name, modules[-1].name + + def _poison_cursor(self): + # Odoo's assertRaises runs its body in a savepoint and rolls it back, + # which would un-poison the cursor; suppress keeps the transaction aborted. + with mute_logger("odoo.sql_db"), contextlib.suppress(psycopg2.Error): + self.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist") + self.assertEqual( + self.env.cr.connection.get_transaction_status(), + psycopg2.extensions.TRANSACTION_STATUS_INERROR, + "precondition: the transaction must be aborted before the hook runs", + ) + + def test_01_missing_menu_xmlid_is_skipped(self): + """A catalog entry whose menu does not exist neither raises nor warns.""" + first, _last = self._first_and_last_module() + before = self.HideMenu.search([]) + + with self._catalog(**{first: MISSING_MENU_XMLID}), self.assertNoLogs(HOOK_LOGGER, level="WARNING"): + self.IrModule._register_hook() + + self.assertEqual(self.HideMenu.search([]), before) + + def test_02_database_error_inside_hook_is_logged_and_skipped(self): + """A SQL failure while hiding is logged, the registry load continues, + and the half-done pass is rolled back as a whole.""" + first, last = self._first_and_last_module() + menu, menu_xml_id = self._menu_with_external_id("test_526_real_menu") + groups_before = menu.group_ids + self.assertNotIn(self.hide_group, groups_before, "precondition: the menu starts visible") + IrModelData = type(self.env["ir.model.data"]) + failing_lookup = _failing_lookup_for(MISSING_MENU_XMLID, IrModelData._xmlid_lookup) + + # Control: the same catalog entry does hide the menu when nothing fails, + # so the "rolled back" assertions below cannot pass by never having run. + with self._catalog(**{first: menu_xml_id}): + self.IrModule._register_hook() + row = self.HideMenu.search([("menu_id", "=", menu.id)]) + self.assertTrue(row, "control: the catalog entry must create a hide row") + self.assertIn(self.hide_group, menu.group_ids, "control: the catalog entry must hide the menu") + row.show_menu() + row.unlink() + self.assertEqual(menu.group_ids, groups_before, "control undone: the menu is visible again") + + with ( + self._catalog(**{first: menu_xml_id, last: MISSING_MENU_XMLID}), + patch.object(IrModelData, "_xmlid_lookup", failing_lookup), + mute_logger("odoo.sql_db"), + self.assertLogs(HOOK_LOGGER, level="WARNING") as captured, + ): + self.IrModule._register_hook() + + self.assertTrue( + any("menu hiding pass" in message for message in captured.output), + f"expected a skipped-hiding warning, got {captured.output}", + ) + # The transaction is still usable: the failure was contained in a savepoint. + self.env.cr.execute("SELECT 1") + self.assertEqual(self.env.cr.fetchone(), (1,)) + # ...and the pass is atomic: the menu hidden before the failure is visible again + # and its configuration row is gone. + self.assertEqual(menu.group_ids, groups_before) + self.assertFalse(self.HideMenu.search([("menu_id", "=", menu.id)])) + + def test_03_hook_survives_already_aborted_transaction(self): + """The hook must not raise when the cursor is already poisoned before it starts.""" + self.env.cr.execute("SAVEPOINT test_526_poisoned_cursor") + try: + self._poison_cursor() + # Driven through _register_hook() as the loader does; this relies on + # super()._register_hook() issuing no SQL of its own. + with mute_logger("odoo.sql_db"), self.assertLogs(HOOK_LOGGER, level="WARNING"): + self.IrModule._register_hook() + finally: + self.env.cr.execute("ROLLBACK TO SAVEPOINT test_526_poisoned_cursor") + self.env.cr.execute("RELEASE SAVEPOINT test_526_poisoned_cursor") + + def test_04_callers_pending_writes_are_not_swallowed(self): + """A failure flushing the caller's own pending writes is the caller's error. + + The guard covers only the hiding pass. Pending ORM writes queued before + it are flushed ahead of the guard, so their failure surfaces where it + belongs instead of being logged as a menu-hiding problem. + """ + menu = self._menu_without_hide_row() + row = self.HideMenu.create({"menu_id": menu.id, "xml_id": "test.pending"}) + self.env.cr.execute("SAVEPOINT test_526_pending_write") + try: + row.write({"xml_id": "test.pending_changed"}) # queued, not flushed yet + self._poison_cursor() + + raised = None + with mute_logger("odoo.sql_db"), self.assertNoLogs(HOOK_LOGGER, level="WARNING"): + try: + self.IrModule.hide_menus() + except psycopg2.Error as exc: + raised = exc + self.assertIsInstance(raised, psycopg2.errors.InFailedSqlTransaction) + finally: + self.env.cr.execute("ROLLBACK TO SAVEPOINT test_526_pending_write") + self.env.cr.execute("RELEASE SAVEPOINT test_526_pending_write")