diff --git a/odev/_version.py b/odev/_version.py index 346ddacf8..69f390745 100644 --- a/odev/_version.py +++ b/odev/_version.py @@ -22,4 +22,4 @@ # or merged change. # ------------------------------------------------------------------------------ -__version__ = "4.30.2" +__version__ = "4.30.3" 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 0a31d961d..c846e7bf1 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 @@ -493,12 +493,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) @@ -525,24 +526,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)) @@ -554,6 +538,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 a41993b41..fa5194913 100644 --- a/odev/common/odoobin.py +++ b/odev/common/odoobin.py @@ -167,8 +167,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.""" @@ -301,12 +303,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 @@ -925,6 +929,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, [])