From cdc018ad0798ebd497f76d26f8c84138c9cf1d79 Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Mon, 18 Apr 2022 01:19:38 +0300 Subject: [PATCH] apply data migration in the same transaction block with schema migration --- src/ahriman/core/database/data/__init__.py | 12 ++++++------ src/ahriman/core/database/data/package_statuses.py | 3 --- src/ahriman/core/database/data/patches.py | 2 -- src/ahriman/core/database/data/users.py | 2 -- src/ahriman/core/database/migrations/__init__.py | 14 +++++++++++--- src/ahriman/core/database/sqlite.py | 8 +------- tests/ahriman/core/database/data/test_data_init.py | 7 +++---- .../core/database/data/test_package_statuses.py | 4 ---- tests/ahriman/core/database/data/test_patches.py | 1 - tests/ahriman/core/database/data/test_users.py | 1 - tests/ahriman/core/database/migrations/conftest.py | 6 ++++-- .../database/migrations/test_migrations_init.py | 7 +++++-- tests/ahriman/core/database/test_sqlite.py | 6 +----- 13 files changed, 31 insertions(+), 42 deletions(-) diff --git a/src/ahriman/core/database/data/__init__.py b/src/ahriman/core/database/data/__init__.py index 380bd85f..8624672a 100644 --- a/src/ahriman/core/database/data/__init__.py +++ b/src/ahriman/core/database/data/__init__.py @@ -24,11 +24,10 @@ from ahriman.core.database.data.package_statuses import migrate_package_statuses from ahriman.core.database.data.patches import migrate_patches from ahriman.core.database.data.users import migrate_users_data from ahriman.models.migration_result import MigrationResult -from ahriman.models.repository_paths import RepositoryPaths -def migrate_data(result: MigrationResult, connection: Connection, - configuration: Configuration, paths: RepositoryPaths) -> None: +def migrate_data( + result: MigrationResult, connection: Connection, configuration: Configuration) -> None: """ perform data migration @@ -36,10 +35,11 @@ def migrate_data(result: MigrationResult, connection: Connection, result(MigrationResult): result of the schema migration connection(Connection): database connection configuration(Configuration): configuration instance - paths(RepositoryPaths): repository paths instance """ # initial data migration + repository_paths = configuration.repository_paths + if result.old_version <= 0: - migrate_package_statuses(connection, paths) - migrate_patches(connection, paths) + migrate_package_statuses(connection, repository_paths) + migrate_patches(connection, repository_paths) migrate_users_data(connection, configuration) diff --git a/src/ahriman/core/database/data/package_statuses.py b/src/ahriman/core/database/data/package_statuses.py index 7a766d79..98f3bd44 100644 --- a/src/ahriman/core/database/data/package_statuses.py +++ b/src/ahriman/core/database/data/package_statuses.py @@ -77,6 +77,3 @@ def migrate_package_statuses(connection: Connection, paths: RepositoryPaths) -> status = BuildStatus.from_json(item["status"]) insert_base(package, status) insert_packages(package) - - connection.commit() - cache_path.unlink() diff --git a/src/ahriman/core/database/data/patches.py b/src/ahriman/core/database/data/patches.py index 487e8bfe..3e5c6d72 100644 --- a/src/ahriman/core/database/data/patches.py +++ b/src/ahriman/core/database/data/patches.py @@ -42,5 +42,3 @@ def migrate_patches(connection: Connection, paths: RepositoryPaths) -> None: connection.execute( """insert into patches (package_base, patch) values (:package_base, :patch)""", {"package_base": package.name, "patch": content}) - - connection.commit() diff --git a/src/ahriman/core/database/data/users.py b/src/ahriman/core/database/data/users.py index bd51a8eb..dc5787b8 100644 --- a/src/ahriman/core/database/data/users.py +++ b/src/ahriman/core/database/data/users.py @@ -38,5 +38,3 @@ def migrate_users_data(connection: Connection, configuration: Configuration) -> connection.execute( """insert into users (username, access, password) values (:username, :access, :password)""", {"username": option.lower(), "access": access, "password": value}) - - connection.commit() diff --git a/src/ahriman/core/database/migrations/__init__.py b/src/ahriman/core/database/migrations/__init__.py index 8d7597f4..f94cf1ba 100644 --- a/src/ahriman/core/database/migrations/__init__.py +++ b/src/ahriman/core/database/migrations/__init__.py @@ -27,6 +27,8 @@ from pkgutil import iter_modules from sqlite3 import Connection from typing import List, Type +from ahriman.core.configuration import Configuration +from ahriman.core.database.data import migrate_data from ahriman.models.migration import Migration from ahriman.models.migration_result import MigrationResult @@ -37,32 +39,36 @@ class Migrations: idea comes from https://www.ash.dev/blog/simple-migration-system-in-sqlite/ Attributes: + configuration(Configuration): configuration instance connection(Connection): database connection logger(logging.Logger): class logger """ - def __init__(self, connection: Connection) -> None: + def __init__(self, connection: Connection, configuration: Configuration) -> None: """ default constructor Args: connection(Connection): database connection + configuration(Configuration): configuration instance """ self.connection = connection + self.configuration = configuration self.logger = logging.getLogger("database") @classmethod - def migrate(cls: Type[Migrations], connection: Connection) -> MigrationResult: + def migrate(cls: Type[Migrations], connection: Connection, configuration: Configuration) -> MigrationResult: """ perform migrations implicitly Args: connection(Connection): database connection + configuration(Configuration): configuration instance Returns: MigrationResult: current schema version """ - return cls(connection).run() + return cls(connection, configuration).run() def migrations(self) -> List[Migration]: """ @@ -112,6 +118,8 @@ class Migrations: cursor.execute(statement) self.logger.info("migration %s at index %s has been applied", migration.name, migration.index) + migrate_data(result, self.connection, self.configuration) + cursor.execute(f"pragma user_version = {expected_version}") # no support for ? placeholders except Exception: self.logger.exception("migration failed with exception") diff --git a/src/ahriman/core/database/sqlite.py b/src/ahriman/core/database/sqlite.py index 1d3505dd..ebe61e1b 100644 --- a/src/ahriman/core/database/sqlite.py +++ b/src/ahriman/core/database/sqlite.py @@ -23,11 +23,9 @@ import json import sqlite3 from pathlib import Path -from sqlite3 import Connection from typing import Type from ahriman.core.configuration import Configuration -from ahriman.core.database.data import migrate_data from ahriman.core.database.migrations import Migrations from ahriman.core.database.operations.auth_operations import AuthOperations from ahriman.core.database.operations.build_operations import BuildOperations @@ -83,9 +81,5 @@ class SQLite(AuthOperations, BuildOperations, PackageOperations, PatchOperations paths = configuration.repository_paths - def run(connection: Connection) -> None: - result = Migrations.migrate(connection) - migrate_data(result, connection, configuration, paths) - - self.with_connection(run) + self.with_connection(lambda conn: Migrations.migrate(conn, configuration)) paths.chown(self.path) diff --git a/tests/ahriman/core/database/data/test_data_init.py b/tests/ahriman/core/database/data/test_data_init.py index a02440fb..a44e57fd 100644 --- a/tests/ahriman/core/database/data/test_data_init.py +++ b/tests/ahriman/core/database/data/test_data_init.py @@ -16,20 +16,19 @@ def test_migrate_data_initial(connection: Connection, configuration: Configurati patches = mocker.patch("ahriman.core.database.data.migrate_patches") users = mocker.patch("ahriman.core.database.data.migrate_users_data") - migrate_data(MigrationResult(old_version=0, new_version=900), connection, configuration, repository_paths) + migrate_data(MigrationResult(old_version=0, new_version=900), connection, configuration) packages.assert_called_once_with(connection, repository_paths) patches.assert_called_once_with(connection, repository_paths) users.assert_called_once_with(connection, configuration) -def test_migrate_data_skip(connection: Connection, configuration: Configuration, - repository_paths: RepositoryPaths, mocker: MockerFixture) -> None: +def test_migrate_data_skip(connection: Connection, configuration: Configuration, mocker: MockerFixture) -> None: """ must not migrate data if version is up-to-date """ packages = mocker.patch("ahriman.core.database.data.migrate_package_statuses") users = mocker.patch("ahriman.core.database.data.migrate_users_data") - migrate_data(MigrationResult(old_version=900, new_version=900), connection, configuration, repository_paths) + migrate_data(MigrationResult(old_version=900, new_version=900), connection, configuration) packages.assert_not_called() users.assert_not_called() diff --git a/tests/ahriman/core/database/data/test_package_statuses.py b/tests/ahriman/core/database/data/test_package_statuses.py index 5bb8cebb..45e401a8 100644 --- a/tests/ahriman/core/database/data/test_package_statuses.py +++ b/tests/ahriman/core/database/data/test_package_statuses.py @@ -19,10 +19,8 @@ def test_migrate_package_statuses(connection: Connection, package_ahriman: Packa mocker.patch("pathlib.Path.is_file", return_value=True) mocker.patch("pathlib.Path.open") mocker.patch("json.load", return_value=response) - unlink_mock = mocker.patch("pathlib.Path.unlink") migrate_package_statuses(connection, repository_paths) - unlink_mock.assert_called_once_with() connection.execute.assert_has_calls([ mock.call(pytest.helpers.anyvar(str, strict=True), pytest.helpers.anyvar(int)), mock.call(pytest.helpers.anyvar(str, strict=True), pytest.helpers.anyvar(int)), @@ -30,7 +28,6 @@ def test_migrate_package_statuses(connection: Connection, package_ahriman: Packa connection.executemany.assert_has_calls([ mock.call(pytest.helpers.anyvar(str, strict=True), pytest.helpers.anyvar(int)), ]) - connection.commit.assert_called_once_with() def test_migrate_package_statuses_skip(connection: Connection, repository_paths: RepositoryPaths, @@ -40,4 +37,3 @@ def test_migrate_package_statuses_skip(connection: Connection, repository_paths: """ mocker.patch("pathlib.Path.is_file", return_value=False) migrate_package_statuses(connection, repository_paths) - connection.commit.assert_not_called() diff --git a/tests/ahriman/core/database/data/test_patches.py b/tests/ahriman/core/database/data/test_patches.py index 0abaa3d7..5e184d43 100644 --- a/tests/ahriman/core/database/data/test_patches.py +++ b/tests/ahriman/core/database/data/test_patches.py @@ -23,7 +23,6 @@ def test_migrate_patches(connection: Connection, repository_paths: RepositoryPat iterdir_mock.assert_called_once_with() read_mock.assert_called_once_with(encoding="utf8") connection.execute.assert_called_once_with(pytest.helpers.anyvar(str, strict=True), pytest.helpers.anyvar(int)) - connection.commit.assert_called_once_with() def test_migrate_patches_skip(connection: Connection, repository_paths: RepositoryPaths, diff --git a/tests/ahriman/core/database/data/test_users.py b/tests/ahriman/core/database/data/test_users.py index 1903fbd8..4bd69849 100644 --- a/tests/ahriman/core/database/data/test_users.py +++ b/tests/ahriman/core/database/data/test_users.py @@ -19,4 +19,3 @@ def test_migrate_users_data(connection: Connection, configuration: Configuration mock.call(pytest.helpers.anyvar(str, strict=True), pytest.helpers.anyvar(int)), mock.call(pytest.helpers.anyvar(str, strict=True), pytest.helpers.anyvar(int)), ]) - connection.commit.assert_called_once_with() diff --git a/tests/ahriman/core/database/migrations/conftest.py b/tests/ahriman/core/database/migrations/conftest.py index 3e86360d..5de0a16e 100644 --- a/tests/ahriman/core/database/migrations/conftest.py +++ b/tests/ahriman/core/database/migrations/conftest.py @@ -2,18 +2,20 @@ import pytest from sqlite3 import Connection +from ahriman.core.configuration import Configuration from ahriman.core.database.migrations import Migrations @pytest.fixture -def migrations(connection: Connection) -> Migrations: +def migrations(connection: Connection, configuration: Configuration) -> Migrations: """ fixture for migrations object Args: connection(Connection): sqlite connection fixture + configuration(Configuration): configuration fixture Returns: Migrations: migrations test instance """ - return Migrations(connection) + return Migrations(connection, configuration) diff --git a/tests/ahriman/core/database/migrations/test_migrations_init.py b/tests/ahriman/core/database/migrations/test_migrations_init.py index f0fec1fd..21c3aa64 100644 --- a/tests/ahriman/core/database/migrations/test_migrations_init.py +++ b/tests/ahriman/core/database/migrations/test_migrations_init.py @@ -5,17 +5,18 @@ from sqlite3 import Connection from unittest import mock from unittest.mock import MagicMock +from ahriman.core.configuration import Configuration from ahriman.core.database.migrations import Migrations from ahriman.models.migration import Migration from ahriman.models.migration_result import MigrationResult -def test_migrate(connection: Connection, mocker: MockerFixture) -> None: +def test_migrate(connection: Connection, configuration: Configuration, mocker: MockerFixture) -> None: """ must perform migrations """ run_mock = mocker.patch("ahriman.core.database.migrations.Migrations.run") - Migrations.migrate(connection) + Migrations.migrate(connection, configuration) run_mock.assert_called_once_with() @@ -46,6 +47,7 @@ def test_run(migrations: Migrations, mocker: MockerFixture) -> None: return_value=[Migration(0, "test", ["select 1"])]) migrations.connection.cursor.return_value = cursor validate_mock = mocker.patch("ahriman.models.migration_result.MigrationResult.validate") + migrate_data_mock = mocker.patch("ahriman.core.database.migrations.migrate_data") migrations.run() validate_mock.assert_called_once_with() @@ -56,6 +58,7 @@ def test_run(migrations: Migrations, mocker: MockerFixture) -> None: mock.call("commit"), ]) cursor.close.assert_called_once_with() + migrate_data_mock.assert_called_once_with(MigrationResult(0, 1), migrations.connection, migrations.configuration) def test_run_migration_exception(migrations: Migrations, mocker: MockerFixture) -> None: diff --git a/tests/ahriman/core/database/test_sqlite.py b/tests/ahriman/core/database/test_sqlite.py index 07d02454..378d18f6 100644 --- a/tests/ahriman/core/database/test_sqlite.py +++ b/tests/ahriman/core/database/test_sqlite.py @@ -20,9 +20,5 @@ def test_init(database: SQLite, configuration: Configuration, mocker: MockerFixt must run migrations on init """ migrate_schema_mock = mocker.patch("ahriman.core.database.migrations.Migrations.migrate") - migrate_data_mock = mocker.patch("ahriman.core.database.sqlite.migrate_data") - database.init(configuration) - migrate_schema_mock.assert_called_once_with(pytest.helpers.anyvar(int)) - migrate_data_mock.assert_called_once_with( - pytest.helpers.anyvar(int), pytest.helpers.anyvar(int), configuration, configuration.repository_paths) + migrate_schema_mock.assert_called_once_with(pytest.helpers.anyvar(int), configuration)