From b79b10f710388f8383adad71aa0c55370827bac9 Mon Sep 17 00:00:00 2001 From: brinkflew Date: Sun, 26 Jul 2026 23:05:45 +0200 Subject: [PATCH] [FIX] databases: neutralize with the custom addons of the database `odev quickstart` neutralized databases without ever running the neutralization scripts shipped by their custom modules, and ran `odoo-bin neutralize` with an `--addons-path` that did not contain the custom repository at all. Three separate defects had to line up for that: - `quickstart` linked the repository to the new database *after* restoring it, but neutralization runs from within `restore`. At that point the database had no repository, so no addons path could be derived from it. The repository is now linked before the restore, and again after it, since `restore` drops and recreates the database and clears its entry in the data store. - `additional_addons_paths` returned the repository root as-is. A repository keeping its modules in subdirectories is not itself a valid addons path, so it was filtered out downstream and never reached `--addons-path`. The lookup for the directories actually holding modules only existed in `OdoobinCommand`, which `odev neutralize` does not inherit from; it now lives on `OdoobinProcess` as `expand_addons_paths` and is shared by both. - The neutralization scripts of custom modules were looked up by intersecting the installed module names with the *directory names of the addons paths*. An addons path is a directory containing modules, never a module itself, so the intersection was always empty and the spinner reported "0 installed modules". The lookup now builds the module map from the subdirectories of each addons path. Also fixes an infinite loop in `LocalDatabase.neutralize`: the retry counter was reset by a walrus assignment evaluated on every iteration of the loop it was guarding, so `odev neutralize` on a database odev cannot start never returned. Closes #98 --- odev/_version.py | 2 +- odev/commands/database/quickstart.py | 29 ++++-- odev/common/commands/odoobin.py | 9 +- odev/common/databases/local.py | 56 +++++++---- odev/common/odoobin.py | 35 +++++-- .../test-addons/addon_01/data/neutralize.sql | 2 + tests/tests/commands/test_quickstart.py | 96 +++++++++++++++++++ tests/tests/common/test_local_database.py | 79 +++++++++++++++ tests/tests/common/test_odoobin.py | 84 ++++++++++++++++ 9 files changed, 348 insertions(+), 44 deletions(-) create mode 100644 tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql create mode 100644 tests/tests/commands/test_quickstart.py create mode 100644 tests/tests/common/test_local_database.py create mode 100644 tests/tests/common/test_odoobin.py diff --git a/odev/_version.py b/odev/_version.py index 7306664df..12aa2f183 100644 --- a/odev/_version.py +++ b/odev/_version.py @@ -22,4 +22,4 @@ # or merged change. # ------------------------------------------------------------------------------ -__version__ = "4.29.9" +__version__ = "4.29.10" diff --git a/odev/commands/database/quickstart.py b/odev/commands/database/quickstart.py index c014ee0bc..f56b25183 100644 --- a/odev/commands/database/quickstart.py +++ b/odev/commands/database/quickstart.py @@ -64,16 +64,31 @@ def run(self): raise self.error(f"Database {self._database.name!r} could not be restored") new_database = LocalDatabase(self.args.name or self._database.name) + + # Link the repository before restoring: `restore` neutralizes the database, and + # neutralization looks for `data/neutralize.sql` scripts in the custom modules found + # under the addons paths derived from the repository linked to the database. + self.link_repository(new_database) self.odev.run_command("restore", dump_file.as_posix(), database=new_database) - if self._database.repository: - if isinstance(self._database.repository, Repository): - repo_org = self._database.repository.organization - repo_name = self._database.repository.name - else: - repo_org, repo_name = self._database.repository.name.split("/") + # `restore` drops and recreates the database, clearing its entry in the data store. + self.link_repository(new_database) + + def link_repository(self, database: LocalDatabase) -> None: + """Link the repository of the source database to the target local database. + + :param database: The database to link the repository to. + """ + if not self._database.repository: + return + + if isinstance(self._database.repository, Repository): + repo_org = self._database.repository.organization + repo_name = self._database.repository.name + else: + repo_org, repo_name = self._database.repository.name.split("/") - new_database.repository = Repository(repo_name, repo_org) + database.repository = Repository(repo_name, repo_org) def get_dump_filename_kwargs(self) -> MutableMapping[str, Any]: """Return the keyword arguments to pass to Database.get_dump_filename().""" diff --git a/odev/common/commands/odoobin.py b/odev/common/commands/odoobin.py index a6088f567..31a6ad242 100644 --- a/odev/common/commands/odoobin.py +++ b/odev/common/commands/odoobin.py @@ -178,14 +178,7 @@ def _guess_addons_paths(self) -> list[Path]: def _set_addons_paths(self) -> None: """Find additional addons paths from the database repository if any.""" - addons_paths = self._guess_addons_paths() - - globs = [path.glob(f"**/__{manifest}__.py") for path in addons_paths for manifest in ["manifest", "openerp"]] - addons_paths = [ - path.parents[1] for path in (p for g in globs for p in g) if self.odoobin.check_addons_path(path.parents[1]) - ] - - self.odoobin.additional_addons_paths = sorted(set(addons_paths)) + self.odoobin.additional_addons_paths = self.odoobin.expand_addons_paths(self._guess_addons_paths()) self.odoobin.save_database_repository() def _set_odoobin_process(self, force=False) -> None: diff --git a/odev/common/databases/local.py b/odev/common/databases/local.py index d9a732646..b320a00c4 100644 --- a/odev/common/databases/local.py +++ b/odev/common/databases/local.py @@ -6,7 +6,7 @@ import re import shutil import tempfile -from collections.abc import Generator, Mapping +from collections.abc import Generator, Mapping, MutableMapping from datetime import datetime from functools import cached_property from pathlib import Path @@ -468,12 +468,13 @@ def drop(self) -> bool: def neutralize(self): """Neutralize the database.""" max_retries = 5 + retries = 0 with self.connector.nocache(): # Artificially wait for SQL transaction to be committed and for the process to be ready # before running the neutralize command # This is not clean but it works, I guess - while (not self.process or not self.version) and (retries := 0) < max_retries: + while (not self.process or not self.version) and retries < max_retries: retries += 1 sleep(0.2) @@ -500,24 +501,7 @@ def neutralize(self): self.process.run(["-d", self.name], subcommand="neutralize") self.console.print() - installed_modules: set[str] = set(self.installed_modules) & { - path.name for path in self.process.additional_addons_paths - } - scripts: list[Path] = [self.odev.static_path / "neutralize-pre.sql"] - - with progress.spinner(f"Looking up neutralization scripts in {len(installed_modules)} installed modules"): - for addon in self.process.additional_addons_paths: - for module in installed_modules: - neutralize_path: Path = addon / module / "data" / "neutralize.sql" - - if neutralize_path.is_file(): - scripts.append(neutralize_path) - - scripts.append(self.odev.static_path / "neutralize-post.sql") - - if self.version < NEUTRALIZE_BEFORE_ODOO_VERSION: - scripts.append(self.odev.static_path / "neutralize-post-before-15.0.sql") - + scripts = self._neutralize_scripts() tracker = progress.Progress() task = tracker.add_task(f"Running {len(scripts)} neutralization scripts", total=len(scripts)) @@ -529,6 +513,38 @@ def neutralize(self): tracker.stop() + def _neutralize_scripts(self) -> list[Path]: + """Return the neutralization scripts to run, including those shipped by the custom modules + installed in the database. + + :return: The paths to the neutralization scripts, in the order they must be run. + :rtype: List[Path] + """ + addons_paths = self.process.additional_addons_paths if self.process else [] + modules: MutableMapping[str, Path] = { + module.name: module + for path in addons_paths + if path.is_dir() + for module in path.iterdir() + if module.is_dir() + } + installed_modules: set[str] = set(self.installed_modules) & modules.keys() + scripts: list[Path] = [self.odev.static_path / "neutralize-pre.sql"] + + with progress.spinner(f"Looking up neutralization scripts in {len(installed_modules)} installed modules"): + scripts.extend( + script + for module in sorted(installed_modules) + if (script := modules[module] / "data" / "neutralize.sql").is_file() + ) + + scripts.append(self.odev.static_path / "neutralize-post.sql") + + if self.version < NEUTRALIZE_BEFORE_ODOO_VERSION: + scripts.append(self.odev.static_path / "neutralize-post-before-15.0.sql") + + return scripts + def dump(self, filestore: bool = False, path: Path | None = None) -> Path: if path is None: path = self.odev.dumps_path diff --git a/odev/common/odoobin.py b/odev/common/odoobin.py index 8b0e881d0..50cade59f 100644 --- a/odev/common/odoobin.py +++ b/odev/common/odoobin.py @@ -148,8 +148,10 @@ def __init__( self.repository: GitConnector = GitConnector("odoo/odoo") """Github repository of Odoo.""" - self._additional_addons_paths: list[Path] = [] - """List of additional addons paths to use when starting the Odoo process.""" + self._additional_addons_paths: list[Path] | None = None + """List of additional addons paths to use when starting the Odoo process. + None until derived from the repository linked to the database, or assigned explicitly. + """ self._venv: PythonEnv | None = None """Cached python virtual environment used by the Odoo installation.""" @@ -282,12 +284,14 @@ def odoo_support_repository(self) -> GitConnector: @property def additional_addons_paths(self) -> list[Path]: - """Return the list of additional addons paths.""" - if not self._additional_addons_paths and self.database.repository: - repository = GitConnector(self.database.repository.full_name) - - if repository.path not in self._additional_addons_paths: - self._additional_addons_paths.append(repository.path) + """Return the list of additional addons paths. + Derived once from the repository linked to the database, unless assigned explicitly. + """ + if self._additional_addons_paths is None: + repository = self.database.repository + self._additional_addons_paths = ( + self.expand_addons_paths([GitConnector(repository.full_name).path]) if repository else [] + ) return self._additional_addons_paths @@ -821,6 +825,21 @@ def check_addons_path(cls, path: Path) -> bool: manifest.is_file() and (manifest.parent / "__init__.py").is_file() for glob in globs for manifest in glob ) + @classmethod + def expand_addons_paths(cls, paths: Sequence[Path]) -> list[Path]: + """Expand paths to the actual Odoo addons directories they contain. + + Modules are looked up recursively so that repositories keeping their modules in + subdirectories are handled the same way as those keeping them at their root. + + :param paths: Paths to expand. + :return: Sorted list of unique valid addons paths. + :rtype: List[Path] + """ + globs = (path.glob(f"**/__{manifest}__.py") for path in paths for manifest in ["manifest", "openerp"]) + candidates = {manifest.parents[1] for glob in globs for manifest in glob} + return sorted(path for path in candidates if cls.check_addons_path(path)) + @classmethod def check_addon_path(cls, path: Path) -> bool: """Return whether the given path is a valid Odoo addon. diff --git a/tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql b/tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql new file mode 100644 index 000000000..2ff641944 --- /dev/null +++ b/tests/resources/repositories/test/test-addons/addon_01/data/neutralize.sql @@ -0,0 +1,2 @@ +-- Neutralization script shipped by a custom module, used by tests. +SELECT 1; diff --git a/tests/tests/commands/test_quickstart.py b/tests/tests/commands/test_quickstart.py new file mode 100644 index 000000000..ff79fc4ea --- /dev/null +++ b/tests/tests/commands/test_quickstart.py @@ -0,0 +1,96 @@ +from argparse import Namespace +from pathlib import Path +from unittest.mock import MagicMock + +from odev.commands.database.quickstart import QuickStartCommand +from odev.common.databases import LocalDatabase, Repository + +from tests.fixtures import OdevTestCase + + +class TestQuickStartLinksRepository(OdevTestCase): + """The repository must be linked before the dump is restored, and still be linked afterwards.""" + + def make_command(self, repository: Repository | None) -> QuickStartCommand: + command = QuickStartCommand.__new__(QuickStartCommand) + command._framework = self.odev + command.args = Namespace(branch=None, version=None, name="quickstart-target", filestore=False) + + source = MagicMock() + source.name = "quickstart-source" + source.repository = repository + source.platform.name = "remote" + source._get_dump_filename.return_value = "dump.zip" + command._database = source + return command + + def make_dump_file(self) -> Path: + dump_file = self.run_path / "dump.zip" + dump_file.parent.mkdir(parents=True, exist_ok=True) + dump_file.touch() + return dump_file + + def test_repository_is_linked_before_restore(self): + """Regression for #98: neutralization runs from within `restore`, so a repository linked + only after the restore call is invisible to it. + """ + repository = Repository("psbe-project", "odoo-ps") + command = self.make_command(repository) + target = MagicMock(spec=LocalDatabase) + target.repository = None + seen: list[Repository | None] = [] + + def fake_run_command(name, *_args, database=None, **_kwargs): + if name == "restore": + seen.append(database.repository) + return True + + dump_file = self.make_dump_file() + + with ( + self.patch(self.odev, "run_command", side_effect=fake_run_command), + self.patch_property(type(self.odev), "dumps_path", dump_file.parent), + self.patch("odev.commands.database.quickstart", "LocalDatabase", return_value=target), + ): + command.run() + + self.assertEqual(seen, [Repository("psbe-project", "odoo-ps")]) + + def test_repository_is_still_linked_after_restore(self): + """`restore` drops and recreates the database, which clears its entry in the data store.""" + repository = Repository("psbe-project", "odoo-ps") + command = self.make_command(repository) + target = MagicMock(spec=LocalDatabase) + target.repository = None + + def fake_run_command(name, *_args, database=None, **_kwargs): + if name == "restore": + database.repository = None # `restore` wipes the store entry + return True + + dump_file = self.make_dump_file() + + with ( + self.patch(self.odev, "run_command", side_effect=fake_run_command), + self.patch_property(type(self.odev), "dumps_path", dump_file.parent), + self.patch("odev.commands.database.quickstart", "LocalDatabase", return_value=target), + ): + command.run() + + self.assertEqual(target.repository, Repository("psbe-project", "odoo-ps")) + + def test_no_repository_on_source_links_nothing(self): + command = self.make_command(None) + target = MagicMock(spec=LocalDatabase) + target.repository = None + + dump_file = self.make_dump_file() + + with ( + self.patch(self.odev, "run_command", return_value=True), + self.patch_property(type(self.odev), "dumps_path", dump_file.parent), + self.patch("odev.commands.database.quickstart", "LocalDatabase", return_value=target), + ): + command.run() + + self.assertIsNone(target.repository) diff --git a/tests/tests/common/test_local_database.py b/tests/tests/common/test_local_database.py new file mode 100644 index 000000000..d7e3b93b9 --- /dev/null +++ b/tests/tests/common/test_local_database.py @@ -0,0 +1,79 @@ +from pathlib import Path +from unittest.mock import MagicMock + +from odev.common.databases import LocalDatabase +from odev.common.version import OdooVersion + +from tests.fixtures import OdevTestCase + + +class TestNeutralizeScripts(OdevTestCase): + """Neutralization must pick up the `data/neutralize.sql` scripts shipped by custom modules.""" + + def setUp(self): + super().setUp() + self.addons_path = self.res_path / "repositories" / "test" / "test-addons" + + def make_database(self, addons_paths: list[Path] | None = None) -> LocalDatabase: + database = LocalDatabase.__new__(LocalDatabase) + database._framework = self.odev + database._process = MagicMock() + database._process.additional_addons_paths = [self.addons_path] if addons_paths is None else addons_paths + return database + + def neutralize_scripts( + self, + installed_modules: list[str], + addons_paths: list[Path] | None = None, + version: str = "18.0", + ) -> list[Path]: + database = self.make_database(addons_paths) + + with ( + self.patch_property(LocalDatabase, "installed_modules", installed_modules), + self.patch_property(LocalDatabase, "process", database._process), + self.patch_property(LocalDatabase, "version", OdooVersion(version)), + ): + return database._neutralize_scripts() + + def test_custom_module_script_is_collected(self): + """Regression for #98: the module scripts were filtered against the *addons directory* + names rather than the module names, so no custom script was ever collected. + """ + scripts = self.neutralize_scripts(["base", "addon_01"]) + self.assertEqual( + scripts, + [ + self.odev.static_path / "neutralize-pre.sql", + self.addons_path / "addon_01" / "data" / "neutralize.sql", + self.odev.static_path / "neutralize-post.sql", + ], + ) + + def test_module_without_script_is_skipped(self): + scripts = self.neutralize_scripts(["addon_02"], addons_paths=[self.addons_path / "submodule"]) + self.assertEqual( + scripts, + [ + self.odev.static_path / "neutralize-pre.sql", + self.odev.static_path / "neutralize-post.sql", + ], + ) + + def test_module_not_installed_is_skipped(self): + scripts = self.neutralize_scripts(["base"]) + self.assertNotIn(self.addons_path / "addon_01" / "data" / "neutralize.sql", scripts) + + def test_no_addons_paths_yields_static_scripts_only(self): + scripts = self.neutralize_scripts(["addon_01"], addons_paths=[]) + self.assertEqual( + scripts, + [ + self.odev.static_path / "neutralize-pre.sql", + self.odev.static_path / "neutralize-post.sql", + ], + ) + + def test_older_versions_get_the_legacy_script(self): + scripts = self.neutralize_scripts(["base"], version="14.0") + self.assertIn(self.odev.static_path / "neutralize-post-before-15.0.sql", scripts) diff --git a/tests/tests/common/test_odoobin.py b/tests/tests/common/test_odoobin.py new file mode 100644 index 000000000..b6c5cce73 --- /dev/null +++ b/tests/tests/common/test_odoobin.py @@ -0,0 +1,84 @@ +import shutil +import tempfile +from pathlib import Path + +from odev.common.databases import Repository +from odev.common.odoobin import OdoobinProcess + +from tests.fixtures import OdevTestCase + + +class FakeDatabase: + """Minimal stand-in for a database, exposing only what the addons paths derivation reads.""" + + def __init__(self, repository: Repository | None = None): + self.repository = repository + + +class TestExpandAddonsPaths(OdevTestCase): + """`expand_addons_paths` resolves a directory to the addons directories it contains.""" + + @property + def addons_path(self) -> Path: + return self.res_path / "repositories" / "test" / "test-addons" + + def test_finds_modules_nested_in_subdirectories(self): + expanded = OdoobinProcess.expand_addons_paths([self.addons_path]) + self.assertEqual(expanded, [self.addons_path, self.addons_path / "submodule"]) + + def test_ignores_directories_without_modules(self): + empty = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, empty, ignore_errors=True) + self.assertEqual(OdoobinProcess.expand_addons_paths([empty]), []) + + def test_deduplicates_overlapping_inputs(self): + expanded = OdoobinProcess.expand_addons_paths([self.addons_path, self.addons_path / "submodule"]) + self.assertEqual(expanded, [self.addons_path, self.addons_path / "submodule"]) + + def test_no_paths_yields_no_addons_paths(self): + self.assertEqual(OdoobinProcess.expand_addons_paths([]), []) + + +class TestAdditionalAddonsPaths(OdevTestCase): + """The addons paths derived from the linked repository must be expanded, and derived only once.""" + + def setUp(self): + super().setUp() + self.odev.config.paths.repositories = self.res_path / "repositories" + self.addons_path = self.res_path / "repositories" / "test" / "test-addons" + + def make_process(self, repository: Repository | None = None) -> OdoobinProcess: + process = OdoobinProcess.__new__(OdoobinProcess) + process.database = FakeDatabase(repository) # type: ignore [assignment] + process._additional_addons_paths = None + return process + + def test_repository_is_expanded_to_its_addons_directories(self): + """Regression for #98: the bare repository root is not a valid addons path when the + modules live in subdirectories, so it must be expanded before being used. + """ + process = self.make_process(Repository("test-addons", "test")) + self.assertEqual(process.additional_addons_paths, [self.addons_path, self.addons_path / "submodule"]) + + def test_without_repository_no_addons_paths(self): + self.assertEqual(self.make_process().additional_addons_paths, []) + + def test_derived_only_once(self): + process = self.make_process(Repository("test-addons", "test")) + calls: list[object] = [] + original = OdoobinProcess.expand_addons_paths + + def counting_expand(paths): + calls.append(paths) + return original(paths) + + with self.patch(OdoobinProcess, "expand_addons_paths", side_effect=counting_expand): + for _ in range(3): + _ = process.additional_addons_paths + + self.assertEqual(len(calls), 1) + + def test_explicit_assignment_is_not_overridden(self): + process = self.make_process(Repository("test-addons", "test")) + process.additional_addons_paths = [] + self.assertEqual(process.additional_addons_paths, [])