From ad57fad1eaaf40741054d5b29d65a75fbd6303d0 Mon Sep 17 00:00:00 2001 From: Albrecht Degering Date: Mon, 3 Aug 2026 15:04:18 +0200 Subject: [PATCH] Reject duplicate module migration revisions --- docs/MODULE_LIFECYCLE_RECOVERY.md | 6 +++ src/govoplan_core/db/migrations.py | 65 ++++++++++++++++++++++++++++++ tests/test_database_migrations.py | 22 ++++++++++ 3 files changed, 93 insertions(+) diff --git a/docs/MODULE_LIFECYCLE_RECOVERY.md b/docs/MODULE_LIFECYCLE_RECOVERY.md index d7dc5f5..2705302 100644 --- a/docs/MODULE_LIFECYCLE_RECOVERY.md +++ b/docs/MODULE_LIFECYCLE_RECOVERY.md @@ -1,5 +1,11 @@ # Module Lifecycle Recovery +## Migration Revision Namespace + +All enabled module migration directories are assembled into one Alembic graph. Revision IDs are therefore global across Core and every module even though each module owns a separate `migrations/versions` directory. Core validates literal revision declarations before constructing the graph and rejects duplicates with both file paths. A module must assign a new globally unique revision ID; reusing another module's ID can otherwise make Alembic treat an unrelated schema change as already applied or report an ancestor/head overlap. + +When correcting a collision that has already reached a database, first verify the schema objects that identify which migration actually ran. Rename the unapplied migration, or transactionally translate the corresponding `alembic_version` row when the applied owner is unambiguous. Never add both colliding IDs as heads or blindly stamp the database. + Package changes and live module-graph changes use Core's durable recovery ledger. The local `install.lock` still prevents duplicate work in one runtime directory; the database lease `core:module-lifecycle:deployment` is the diff --git a/src/govoplan_core/db/migrations.py b/src/govoplan_core/db/migrations.py index 14b869a..f7b610a 100644 --- a/src/govoplan_core/db/migrations.py +++ b/src/govoplan_core/db/migrations.py @@ -1,5 +1,6 @@ from __future__ import annotations +import ast from collections.abc import Iterable, Mapping from dataclasses import dataclass, replace import json @@ -574,9 +575,73 @@ def alembic_config( config.attributes["enabled_modules"] = tuple(enabled_modules) if manifest_factories: config.attributes["manifest_factories"] = tuple(manifest_factories) + validate_unique_migration_revisions(config) return config +def validate_unique_migration_revisions(config: Config) -> None: + """Reject duplicate revision IDs before Alembic assembles the shared graph. + + Module migrations use separate version directories, but Alembic revision IDs + still occupy one global namespace. Alembic can otherwise resolve a duplicate + to the wrong module and report a misleading ancestor/head overlap. + """ + + locations = tuple( + Path(value).resolve() + for value in config.get_main_option("version_locations", "").split(os.pathsep) + if value.strip() + ) + owners: dict[str, list[Path]] = {} + for location in locations: + if not location.is_dir(): + continue + for path in sorted(location.glob("*.py")): + revision = _literal_migration_revision(path) + if revision: + owners.setdefault(revision, []).append(path) + + duplicates = { + revision: paths + for revision, paths in owners.items() + if len(paths) > 1 + } + if not duplicates: + return + + details = "; ".join( + f"{revision}: {', '.join(str(path) for path in paths)}" + for revision, paths in sorted(duplicates.items()) + ) + raise ValueError( + "Alembic revision IDs are global across enabled modules; duplicate " + f"revision declarations found: {details}" + ) + + +def _literal_migration_revision(path: Path) -> str | None: + try: + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + except (OSError, SyntaxError, UnicodeError): + return None + for statement in tree.body: + value: ast.expr | None = None + if isinstance(statement, ast.Assign) and any( + isinstance(target, ast.Name) and target.id == "revision" + for target in statement.targets + ): + value = statement.value + elif ( + isinstance(statement, ast.AnnAssign) + and isinstance(statement.target, ast.Name) + and statement.target.id == "revision" + ): + value = statement.value + if isinstance(value, ast.Constant) and isinstance(value.value, str): + return value.value.strip() or None + return None + + def database_revision(database_url: str | None = None) -> str | None: url = database_url or settings.database_url engine = create_engine(url) diff --git a/tests/test_database_migrations.py b/tests/test_database_migrations.py index d8b13bb..fadd970 100644 --- a/tests/test_database_migrations.py +++ b/tests/test_database_migrations.py @@ -1,11 +1,13 @@ from __future__ import annotations import logging +import os import tempfile import unittest from pathlib import Path from alembic import command +from alembic.config import Config from alembic.runtime.migration import MigrationContext from alembic.script import ScriptDirectory from sqlalchemy import create_engine, inspect, text @@ -16,6 +18,7 @@ from govoplan_core.db.migrations import ( migrate_database, reconcile_change_sequence_retention_floor_drift, reconcile_namespace_table_drift, + validate_unique_migration_revisions, ) @@ -40,6 +43,25 @@ def database_migration_heads(connection) -> set[str]: class DatabaseMigrationTests(unittest.TestCase): + def test_duplicate_module_revision_ids_are_rejected_with_file_provenance(self) -> None: + with tempfile.TemporaryDirectory(prefix="govoplan-duplicate-revision-test-") as directory: + root = Path(directory) + first = root / "first" + second = root / "second" + first.mkdir() + second.mkdir() + (first / "first.py").write_text('revision = "duplicate123"\n', encoding="utf-8") + (second / "second.py").write_text('revision: str = "duplicate123"\n', encoding="utf-8") + config = Config() + config.set_main_option("version_locations", os.pathsep.join((str(first), str(second)))) + + with self.assertRaisesRegex(ValueError, "duplicate123") as raised: + validate_unique_migration_revisions(config) + + message = str(raised.exception) + self.assertIn("first.py", message) + self.assertIn("second.py", message) + def test_migration_logging_keeps_application_loggers_enabled(self) -> None: logger = logging.getLogger("govoplan.request") previous_disabled = logger.disabled