From 4b2f6bbee9928a3c330301230d0c1c3cb6ba5283 Mon Sep 17 00:00:00 2001 From: Evgenii Alekseev Date: Wed, 4 Sep 2024 21:31:52 +0300 Subject: [PATCH] bug: fix removal of the packages It has been broken since reporter improvements, because it effectivelly 1) didn't call remove functions in database 2) used empty repository identifier for web service With those changes it also raises exception when you try to call id on empty identifier --- src/ahriman/application/lock.py | 4 +++- src/ahriman/core/database/sqlite.py | 16 ++++++++++------ src/ahriman/core/status/local_client.py | 2 +- src/ahriman/core/status/watcher.py | 1 - src/ahriman/models/repository_id.py | 5 ++++- tests/ahriman/application/test_lock.py | 3 +++ tests/ahriman/core/database/test_sqlite.py | 17 ++++++++++------- tests/ahriman/core/status/test_local_client.py | 2 +- tests/ahriman/core/status/test_watcher.py | 2 -- tests/ahriman/models/test_repository_id.py | 9 ++++++++- tests/ahriman/web/views/test_view_base.py | 2 +- 11 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/ahriman/application/lock.py b/src/ahriman/application/lock.py index 56aab2d9..023296bf 100644 --- a/src/ahriman/application/lock.py +++ b/src/ahriman/application/lock.py @@ -75,7 +75,9 @@ class Lock(LazyLogging): """ self.path: Path | None = None if args.lock is not None: - self.path = args.lock.with_stem(f"{args.lock.stem}_{repository_id.id}") + self.path = args.lock + if not repository_id.is_empty: + self.path = self.path.with_stem(f"{args.lock.stem}_{repository_id.id}") if not self.path.is_absolute(): # prepend full path to the lock file self.path = Path("/") / "run" / "ahriman" / self.path diff --git a/src/ahriman/core/database/sqlite.py b/src/ahriman/core/database/sqlite.py index e6d26d7e..57ab6bb5 100644 --- a/src/ahriman/core/database/sqlite.py +++ b/src/ahriman/core/database/sqlite.py @@ -27,6 +27,7 @@ from ahriman.core.configuration import Configuration from ahriman.core.database.migrations import Migrations from ahriman.core.database.operations import AuthOperations, BuildOperations, ChangesOperations, \ DependenciesOperations, LogsOperations, PackageOperations, PatchOperations +from ahriman.models.repository_id import RepositoryId # pylint: disable=too-many-ancestors @@ -102,23 +103,26 @@ class SQLite( self.with_connection(lambda connection: Migrations.migrate(connection, configuration)) paths.chown(self.path) - def package_clear(self, package_base: str) -> None: + def package_clear(self, package_base: str, repository_id: RepositoryId | None = None) -> None: """ completely remove package from all tables Args: package_base(str): package base to remove + repository_id(RepositoryId, optional): repository unique identifier override (Default value = None) Examples: This method completely removes the package from all tables and must be used, e.g. on package removal:: >>> database.package_clear("ahriman") """ - self.build_queue_clear(package_base) - self.patches_remove(package_base, []) - self.logs_remove(package_base, None) - self.changes_remove(package_base) - self.dependencies_remove(package_base) + self.build_queue_clear(package_base, repository_id) + self.patches_remove(package_base, None) + self.logs_remove(package_base, None, repository_id) + self.changes_remove(package_base, repository_id) + self.dependencies_remove(package_base, repository_id) + + self.package_remove(package_base, repository_id) # remove local cache too self._repository_paths.tree_clear(package_base) diff --git a/src/ahriman/core/status/local_client.py b/src/ahriman/core/status/local_client.py index e0900805..bd0d2250 100644 --- a/src/ahriman/core/status/local_client.py +++ b/src/ahriman/core/status/local_client.py @@ -184,7 +184,7 @@ class LocalClient(Client): Args: package_base(str): package base to remove """ - self.database.package_clear(package_base) + self.database.package_clear(package_base, self.repository_id) def package_status_update(self, package_base: str, status: BuildStatusEnum) -> None: """ diff --git a/src/ahriman/core/status/watcher.py b/src/ahriman/core/status/watcher.py index a3d704ff..6b8f5a5a 100644 --- a/src/ahriman/core/status/watcher.py +++ b/src/ahriman/core/status/watcher.py @@ -140,7 +140,6 @@ class Watcher(LazyLogging): with self._lock: self._known.pop(package_base, None) self.client.package_remove(package_base) - self.package_logs_remove(package_base, None) def package_status_update(self, package_base: str, status: BuildStatusEnum) -> None: """ diff --git a/src/ahriman/models/repository_id.py b/src/ahriman/models/repository_id.py index b8abfe6c..47c775c4 100644 --- a/src/ahriman/models/repository_id.py +++ b/src/ahriman/models/repository_id.py @@ -41,9 +41,12 @@ class RepositoryId: Returns: str: unique id for this repository + + Raises: + ValueError: if repository identifier is empty """ if self.is_empty: - return "" + raise ValueError("Repository ID is called on empty repository identifier") return f"{self.architecture}-{self.name}" # basically the same as used for command line @property diff --git a/tests/ahriman/application/test_lock.py b/tests/ahriman/application/test_lock.py index 2db2c8cd..f9c77285 100644 --- a/tests/ahriman/application/test_lock.py +++ b/tests/ahriman/application/test_lock.py @@ -14,6 +14,7 @@ from ahriman.core.configuration import Configuration from ahriman.core.exceptions import DuplicateRunError, UnsafeRunError from ahriman.models.build_status import BuildStatus, BuildStatusEnum from ahriman.models.internal_status import InternalStatus +from ahriman.models.repository_id import RepositoryId def test_path(args: argparse.Namespace, configuration: Configuration) -> None: @@ -30,6 +31,8 @@ def test_path(args: argparse.Namespace, configuration: Configuration) -> None: args.lock = Path("ahriman.pid") assert Lock(args, repository_id, configuration).path == Path("/run/ahriman/ahriman_x86_64-aur-clone.pid") + assert Lock(args, RepositoryId("", ""), configuration).path == Path("/run/ahriman/ahriman.pid") + with pytest.raises(ValueError): args.lock = Path("/") assert Lock(args, repository_id, configuration).path # special case diff --git a/tests/ahriman/core/database/test_sqlite.py b/tests/ahriman/core/database/test_sqlite.py index ef30badf..f3bdf53d 100644 --- a/tests/ahriman/core/database/test_sqlite.py +++ b/tests/ahriman/core/database/test_sqlite.py @@ -4,6 +4,7 @@ from pytest_mock import MockerFixture from ahriman.core.configuration import Configuration from ahriman.core.database import SQLite +from ahriman.models.repository_id import RepositoryId def test_load(configuration: Configuration, mocker: MockerFixture) -> None: @@ -35,7 +36,7 @@ def test_init_skip_migration(database: SQLite, configuration: Configuration, moc migrate_schema_mock.assert_not_called() -def test_package_clear(database: SQLite, mocker: MockerFixture) -> None: +def test_package_clear(database: SQLite, repository_id: RepositoryId, mocker: MockerFixture) -> None: """ must clear package data """ @@ -44,12 +45,14 @@ def test_package_clear(database: SQLite, mocker: MockerFixture) -> None: logs_mock = mocker.patch("ahriman.core.database.SQLite.logs_remove") changes_mock = mocker.patch("ahriman.core.database.SQLite.changes_remove") dependencies_mock = mocker.patch("ahriman.core.database.SQLite.dependencies_remove") + package_mock = mocker.patch("ahriman.core.database.SQLite.package_remove") tree_clear_mock = mocker.patch("ahriman.models.repository_paths.RepositoryPaths.tree_clear") - database.package_clear("package") - build_queue_mock.assert_called_once_with("package") - patches_mock.assert_called_once_with("package", []) - logs_mock.assert_called_once_with("package", None) - changes_mock.assert_called_once_with("package") - dependencies_mock.assert_called_once_with("package") + database.package_clear("package", repository_id) + build_queue_mock.assert_called_once_with("package", repository_id) + patches_mock.assert_called_once_with("package", None) + logs_mock.assert_called_once_with("package", None, repository_id) + changes_mock.assert_called_once_with("package", repository_id) + dependencies_mock.assert_called_once_with("package", repository_id) + package_mock.assert_called_once_with("package", repository_id) tree_clear_mock.assert_called_once_with("package") diff --git a/tests/ahriman/core/status/test_local_client.py b/tests/ahriman/core/status/test_local_client.py index 8e3db4f4..8266f48e 100644 --- a/tests/ahriman/core/status/test_local_client.py +++ b/tests/ahriman/core/status/test_local_client.py @@ -158,7 +158,7 @@ def test_package_remove(local_client: LocalClient, package_ahriman: Package, moc """ package_mock = mocker.patch("ahriman.core.database.SQLite.package_clear") local_client.package_remove(package_ahriman.base) - package_mock.assert_called_once_with(package_ahriman.base) + package_mock.assert_called_once_with(package_ahriman.base, local_client.repository_id) def test_package_status_update(local_client: LocalClient, package_ahriman: Package, mocker: MockerFixture) -> None: diff --git a/tests/ahriman/core/status/test_watcher.py b/tests/ahriman/core/status/test_watcher.py index 34e9c182..620e918f 100644 --- a/tests/ahriman/core/status/test_watcher.py +++ b/tests/ahriman/core/status/test_watcher.py @@ -101,13 +101,11 @@ def test_package_remove(watcher: Watcher, package_ahriman: Package, mocker: Mock must remove package base """ cache_mock = mocker.patch("ahriman.core.status.local_client.LocalClient.package_remove") - logs_mock = mocker.patch("ahriman.core.status.watcher.Watcher.package_logs_remove", create=True) watcher._known = {package_ahriman.base: (package_ahriman, BuildStatus())} watcher.package_remove(package_ahriman.base) assert not watcher._known cache_mock.assert_called_once_with(package_ahriman.base) - logs_mock.assert_called_once_with(package_ahriman.base, None) def test_package_remove_unknown(watcher: Watcher, package_ahriman: Package, mocker: MockerFixture) -> None: diff --git a/tests/ahriman/models/test_repository_id.py b/tests/ahriman/models/test_repository_id.py index f18973c0..a96d3c1d 100644 --- a/tests/ahriman/models/test_repository_id.py +++ b/tests/ahriman/models/test_repository_id.py @@ -7,10 +7,17 @@ def test_id() -> None: """ must correctly generate id """ - assert RepositoryId("", "").id == "" assert RepositoryId("arch", "repo").id == "arch-repo" +def test_id_empty() -> None: + """ + must raise exception on empty identifier + """ + with pytest.raises(ValueError): + assert RepositoryId("", "").id + + def test_is_empty() -> None: """ must check if repository id is empty or not diff --git a/tests/ahriman/web/views/test_view_base.py b/tests/ahriman/web/views/test_view_base.py index 901e095c..5279a98d 100644 --- a/tests/ahriman/web/views/test_view_base.py +++ b/tests/ahriman/web/views/test_view_base.py @@ -201,7 +201,7 @@ def test_service_not_found(base: BaseView) -> None: must raise HTTPNotFound if no repository found """ with pytest.raises(HTTPNotFound): - base.service(RepositoryId("", "")) + base.service(RepositoryId("repo", "arch")) def test_service_package(base: BaseView, repository_id: RepositoryId, mocker: MockerFixture) -> None: