Reject duplicate module migration revisions
This commit is contained in:
@@ -1,5 +1,11 @@
|
|||||||
# Module Lifecycle Recovery
|
# 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
|
Package changes and live module-graph changes use Core's durable recovery
|
||||||
ledger. The local `install.lock` still prevents duplicate work in one runtime
|
ledger. The local `install.lock` still prevents duplicate work in one runtime
|
||||||
directory; the database lease `core:module-lifecycle:deployment` is the
|
directory; the database lease `core:module-lifecycle:deployment` is the
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import ast
|
||||||
from collections.abc import Iterable, Mapping
|
from collections.abc import Iterable, Mapping
|
||||||
from dataclasses import dataclass, replace
|
from dataclasses import dataclass, replace
|
||||||
import json
|
import json
|
||||||
@@ -574,9 +575,73 @@ def alembic_config(
|
|||||||
config.attributes["enabled_modules"] = tuple(enabled_modules)
|
config.attributes["enabled_modules"] = tuple(enabled_modules)
|
||||||
if manifest_factories:
|
if manifest_factories:
|
||||||
config.attributes["manifest_factories"] = tuple(manifest_factories)
|
config.attributes["manifest_factories"] = tuple(manifest_factories)
|
||||||
|
validate_unique_migration_revisions(config)
|
||||||
return 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:
|
def database_revision(database_url: str | None = None) -> str | None:
|
||||||
url = database_url or settings.database_url
|
url = database_url or settings.database_url
|
||||||
engine = create_engine(url)
|
engine = create_engine(url)
|
||||||
|
|||||||
@@ -1,11 +1,13 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import logging
|
import logging
|
||||||
|
import os
|
||||||
import tempfile
|
import tempfile
|
||||||
import unittest
|
import unittest
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
from alembic import command
|
from alembic import command
|
||||||
|
from alembic.config import Config
|
||||||
from alembic.runtime.migration import MigrationContext
|
from alembic.runtime.migration import MigrationContext
|
||||||
from alembic.script import ScriptDirectory
|
from alembic.script import ScriptDirectory
|
||||||
from sqlalchemy import create_engine, inspect, text
|
from sqlalchemy import create_engine, inspect, text
|
||||||
@@ -16,6 +18,7 @@ from govoplan_core.db.migrations import (
|
|||||||
migrate_database,
|
migrate_database,
|
||||||
reconcile_change_sequence_retention_floor_drift,
|
reconcile_change_sequence_retention_floor_drift,
|
||||||
reconcile_namespace_table_drift,
|
reconcile_namespace_table_drift,
|
||||||
|
validate_unique_migration_revisions,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -40,6 +43,25 @@ def database_migration_heads(connection) -> set[str]:
|
|||||||
|
|
||||||
|
|
||||||
class DatabaseMigrationTests(unittest.TestCase):
|
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:
|
def test_migration_logging_keeps_application_loggers_enabled(self) -> None:
|
||||||
logger = logging.getLogger("govoplan.request")
|
logger = logging.getLogger("govoplan.request")
|
||||||
previous_disabled = logger.disabled
|
previous_disabled = logger.disabled
|
||||||
|
|||||||
Reference in New Issue
Block a user