From aa49fa3ae6ae0bfbb0f9efa6304cf109eca1c402 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Mar 2026 09:18:11 +0000 Subject: [PATCH] feat(db): add migration chain CI validation, pre-commit hook, script template, docs, and tests Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- .github/workflows/ci.yml | 14 +- .pre-commit-config.yaml | 10 + docs/DatabaseConfiguration.md | 21 ++ docs/MigrationWorkflow.md | 358 +++++++++++++++++++++++++ migrations/script.py.mako | 40 +++ scripts/check_alembic_migrations.py | 10 +- tests/test_check_alembic_migrations.py | 196 ++++++++++++++ 7 files changed, 641 insertions(+), 8 deletions(-) create mode 100644 docs/MigrationWorkflow.md create mode 100644 migrations/script.py.mako create mode 100644 tests/test_check_alembic_migrations.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0c206c46..45777958 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,18 @@ jobs: - run: ruff check app/ tests/ - run: ruff format --check app/ tests/ + migration-chain: + name: Alembic Migration Chain Check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.11" + - name: Validate migration chain + run: python scripts/check_alembic_migrations.py + html-lint: name: HTML Accessibility Lint runs-on: ubuntu-latest @@ -138,7 +150,7 @@ jobs: build: name: Build & Push Docker Image runs-on: ubuntu-latest - needs: [run-tests, mypy, dependency-scan, html-lint] + needs: [run-tests, mypy, dependency-scan, html-lint, migration-chain] if: github.event_name == 'push' steps: - name: Checkout Code diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1416b15c..773edaa9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -48,6 +48,16 @@ repos: .env.demo )$ + # Alembic migration chain validation + - repo: local + hooks: + - id: check-alembic-migrations + name: Check Alembic migration chain + entry: python scripts/check_alembic_migrations.py + language: python + pass_filenames: false + files: ^migrations/versions/.*\.py$ + # Conventional commits validation - repo: https://github.com/compilerla/conventional-pre-commit rev: v3.0.0 diff --git a/docs/DatabaseConfiguration.md b/docs/DatabaseConfiguration.md index 9eb1fece..4930820f 100644 --- a/docs/DatabaseConfiguration.md +++ b/docs/DatabaseConfiguration.md @@ -287,6 +287,27 @@ alembic revision --autogenerate -m "describe your change" Review the generated file in `migrations/versions/` before applying it. +> **Tip:** For detailed guidance on naming conventions, idempotent patterns, parallel-branch workflows, and resolving merge conflicts, see the [Migration Workflow Guide](MigrationWorkflow.md). + +### Validating the Migration Chain + +A CI check and pre-commit hook validate that the migration chain has no broken +references, duplicate revisions, or diverged heads. Run the check locally: + +```bash +python scripts/check_alembic_migrations.py +python scripts/check_alembic_migrations.py --verbose # extra detail +``` + +If you see **"Multiple migration heads detected"**, two branches added +migrations from the same parent. Create a merge migration: + +```bash +alembic merge heads -m "merge_parallel_branches" +``` + +For a complete walk-through, see the [Migration Workflow Guide](MigrationWorkflow.md). + ### Automating Migrations in Docker Compose Add a short-lived `migrate` service that runs before the API and Worker: diff --git a/docs/MigrationWorkflow.md b/docs/MigrationWorkflow.md new file mode 100644 index 00000000..9a3603f6 --- /dev/null +++ b/docs/MigrationWorkflow.md @@ -0,0 +1,358 @@ +# Migration Workflow + +This guide explains how to create, test, and merge Alembic database migrations in DocuElevate — especially when **multiple feature branches** add migrations in parallel. + +## Table of Contents + +- [Quick Reference](#quick-reference) +- [Creating a New Migration](#creating-a-new-migration) +- [Migration Naming Convention](#migration-naming-convention) +- [Idempotent Migration Patterns](#idempotent-migration-patterns) +- [Parallel Branch Development](#parallel-branch-development) +- [Resolving Migration Conflicts](#resolving-migration-conflicts) +- [CI Validation](#ci-validation) +- [Pre-commit Hook](#pre-commit-hook) +- [Troubleshooting](#troubleshooting) + +--- + +## Quick Reference + +```bash +# Create a new migration after editing app/models.py +alembic revision --autogenerate -m "add_foobar_column" + +# Apply all pending migrations +alembic upgrade head + +# Check current database version +alembic current + +# View migration history +alembic history --verbose + +# Detect multiple heads (diverged branches) +alembic heads + +# Create a merge migration to resolve multiple heads +alembic merge heads -m "merge_parallel_branches" + +# Validate migration chain integrity (CI script) +python scripts/check_alembic_migrations.py +python scripts/check_alembic_migrations.py --verbose +``` + +--- + +## Creating a New Migration + +1. **Edit `app/models.py`** — add or modify SQLAlchemy model classes. + +2. **Generate the migration** from the repo root: + + ```bash + alembic revision --autogenerate -m "add_my_new_table" + ``` + + Alembic uses the `migrations/script.py.mako` template to generate the file. The template includes inline comments about idempotent patterns — read them. + +3. **Rename the file** to follow the [naming convention](#migration-naming-convention): + + ```bash + # Alembic generates a hash-based name by default. + # Rename to the sequential numbering scheme: + mv migrations/versions/_add_my_new_table.py \ + migrations/versions/037_add_my_new_table.py + ``` + + Update the `revision` variable inside the file to match: + + ```python + revision: str = "037_add_my_new_table" + ``` + +4. **Review the generated code** — autogenerate is helpful but not perfect. Check: + - Are new tables and columns detected correctly? + - Does the `downgrade()` reverse all changes? + - Are SQLite-incompatible operations wrapped in `batch_alter_table()`? + +5. **Test the migration** against a fresh database: + + ```bash + # Apply + alembic upgrade head + + # Rollback + alembic downgrade -1 + + # Re-apply + alembic upgrade head + ``` + +6. **Run the chain validation**: + + ```bash + python scripts/check_alembic_migrations.py + ``` + +--- + +## Migration Naming Convention + +All migration files follow a **sequential numeric prefix** scheme: + +``` +NNN_short_description.py +``` + +| Component | Rule | +|-----------|------| +| `NNN` | Three-digit zero-padded number, incrementing from the previous migration | +| `short_description` | Lowercase snake_case summary of the change | + +The **`revision`** variable inside the file **must match the filename stem** exactly: + +```python +# File: migrations/versions/037_add_classification_rules.py +revision: str = "037_add_classification_rules" +down_revision: Union[str, None] = "036_add_document_translation_fields" +``` + +The CI check (`scripts/check_alembic_migrations.py`) enforces this consistency. + +--- + +## Idempotent Migration Patterns + +Migrations should be **idempotent** — safe to run even if the change already exists. This is critical for SQLite compatibility and for recovering from partial failures. + +### Add a Column (only if missing) + +```python +def upgrade() -> None: + conn = op.get_bind() + inspector = sa.inspect(conn) + + if "my_table" in inspector.get_table_names(): + existing = {c["name"] for c in inspector.get_columns("my_table")} + if "new_col" not in existing: + with op.batch_alter_table("my_table") as batch_op: + batch_op.add_column(sa.Column("new_col", sa.String(128), nullable=True)) +``` + +### Create a Table (only if missing) + +```python +def upgrade() -> None: + conn = op.get_bind() + inspector = sa.inspect(conn) + + if "new_table" not in inspector.get_table_names(): + op.create_table( + "new_table", + sa.Column("id", sa.Integer(), primary_key=True), + sa.Column("name", sa.String(255), nullable=False), + ) +``` + +### Drop a Column (only if present) + +```python +def downgrade() -> None: + conn = op.get_bind() + inspector = sa.inspect(conn) + + if "my_table" in inspector.get_table_names(): + existing = {c["name"] for c in inspector.get_columns("my_table")} + if "new_col" in existing: + with op.batch_alter_table("my_table") as batch_op: + batch_op.drop_column("new_col") +``` + +### Use `batch_alter_table` for SQLite + +SQLite does not support `ALTER TABLE DROP COLUMN` or `ALTER TABLE RENAME COLUMN` natively. Alembic's `batch_alter_table` context manager works around this by recreating the table: + +```python +with op.batch_alter_table("users") as batch_op: + batch_op.add_column(sa.Column("phone", sa.String(20), nullable=True)) + batch_op.drop_column("fax") +``` + +--- + +## Parallel Branch Development + +When two feature branches both add migrations from the same parent, the migration chain **diverges** into multiple heads. This is normal and expected — Alembic supports it — but the heads must be merged before the code reaches `main`. + +### Example + +``` +main: 001 → 002 → 003 + ↘ Branch A: 004_add_widgets + ↘ Branch B: 004_add_gadgets ← two heads! +``` + +### How to Avoid Conflicts + +1. **Coordinate** — if two developers are both adding migrations, assign different sequence numbers (e.g., `037_` and `038_`). Even if both depend on `036_`, different numbers prevent filename collisions. + +2. **Rebase early** — before opening a PR, rebase your branch onto the latest `main`: + + ```bash + git fetch origin main + git rebase origin/main + ``` + + If `main` now has a new migration `037_*`, renumber yours to `038_*` and update `down_revision` to point at `037_*`. + +3. **Check for multiple heads** locally: + + ```bash + python scripts/check_alembic_migrations.py + # or + alembic heads + ``` + +--- + +## Resolving Migration Conflicts + +If your PR's CI check reports **"Multiple migration heads detected"**, follow these steps: + +### Step 1 — Update Your Branch + +```bash +git fetch origin main +git merge origin/main +# or +git rebase origin/main +``` + +### Step 2 — Check Heads + +```bash +python scripts/check_alembic_migrations.py --verbose +``` + +The output lists the conflicting heads. + +### Step 3 — Create a Merge Migration + +```bash +alembic merge heads -m "merge_parallel_branches" +``` + +This generates a new migration with **two parents** (a merge point): + +```python +down_revision = ("037_add_widgets", "037_add_gadgets") +``` + +### Step 4 — Rename and Validate + +Rename the merge migration to the next sequence number: + +```bash +mv migrations/versions/_merge_parallel_branches.py \ + migrations/versions/038_merge_parallel_branches.py +``` + +Update the `revision` inside to match, then validate: + +```bash +python scripts/check_alembic_migrations.py +``` + +### Step 5 — Test + +```bash +alembic upgrade head +alembic downgrade -1 +alembic upgrade head +``` + +--- + +## CI Validation + +The CI pipeline (`.github/workflows/ci.yml`) includes a **migration-chain** job that runs: + +```bash +python scripts/check_alembic_migrations.py +``` + +This script checks for: + +| Check | Description | +|-------|-------------| +| Multiple heads | Diverged migration chains that need a merge migration | +| Broken references | A `down_revision` that points to a non-existent revision | +| Duplicate revisions | Two files declaring the same `revision` identifier | +| Filename mismatches | The `revision` variable doesn't match the filename stem | + +The job runs in Stage 1 (fast-fail gates) alongside lint checks. If it fails, the build is blocked until the migration chain is fixed. + +--- + +## Pre-commit Hook + +A local pre-commit hook is configured in `.pre-commit-config.yaml` that runs the same check whenever you commit a change to `migrations/versions/`: + +```yaml +- repo: local + hooks: + - id: check-alembic-migrations + name: Check Alembic migration chain + entry: python scripts/check_alembic_migrations.py + language: python + pass_filenames: false + files: ^migrations/versions/.*\.py$ +``` + +Install the hook: + +```bash +pip install pre-commit +pre-commit install +``` + +--- + +## Troubleshooting + +### "Multiple migration heads detected" + +See [Resolving Migration Conflicts](#resolving-migration-conflicts) above. + +### "Broken chain: revision X references down_revision Y which does not exist" + +You removed or renamed a migration that another migration depends on. Either restore the missing file or update the dependent migration's `down_revision`. + +### "Filename mismatch: file declares revision=X but filename stem is Y" + +The `revision` string inside the Python file must match the filename (without `.py`). Rename the file or update the variable. + +### "relation already exists" when running `alembic upgrade head` + +The database has a table that a pending migration tries to create. Stamp the current state: + +```bash +alembic stamp head +``` + +### Autogenerate doesn't detect my changes + +Ensure all models are imported in `migrations/env.py`. The `from app.models import ...` block at the top must include your new model class. + +### SQLite "no such column" after downgrade + +SQLite has limited `ALTER TABLE` support. Always use `op.batch_alter_table()` for column operations on existing tables. + +--- + +## Further Reading + +- [Alembic Tutorial](https://alembic.sqlalchemy.org/en/latest/tutorial.html) +- [Alembic Branch / Merge](https://alembic.sqlalchemy.org/en/latest/branches.html) +- [Database Configuration Guide](DatabaseConfiguration.md) diff --git a/migrations/script.py.mako b/migrations/script.py.mako new file mode 100644 index 00000000..fb8a9c55 --- /dev/null +++ b/migrations/script.py.mako @@ -0,0 +1,40 @@ +"""${message} + +Revision ID: ${up_revision} +Revises: ${down_revision | comma,n} +Create Date: ${create_date} + +""" + +from typing import Union + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = ${repr(up_revision)} +down_revision: Union[str, None] = ${repr(down_revision)} +depends_on: Union[str, None] = None + + +def upgrade() -> None: + """${message}.""" + # Use ``op.batch_alter_table()`` for SQLite compatibility. + # Always check whether the table/column already exists before altering + # to keep migrations idempotent (safe to re-run). + # + # Example – add a column only if it is missing: + # + # conn = op.get_bind() + # inspector = sa.inspect(conn) + # if "my_table" in inspector.get_table_names(): + # existing = {c["name"] for c in inspector.get_columns("my_table")} + # if "new_col" not in existing: + # with op.batch_alter_table("my_table") as batch_op: + # batch_op.add_column(sa.Column("new_col", sa.String(128), nullable=True)) + ${upgrades if upgrades else "pass"} + + +def downgrade() -> None: + """Reverse ${message}.""" + ${downgrades if downgrades else "pass"} diff --git a/scripts/check_alembic_migrations.py b/scripts/check_alembic_migrations.py index 08f85b45..dee4050a 100644 --- a/scripts/check_alembic_migrations.py +++ b/scripts/check_alembic_migrations.py @@ -42,7 +42,7 @@ from pathlib import Path _REVISION_RE = re.compile(r'^revision\s*(?::\s*str\s*)?=\s*["\'](.+?)["\']', re.MULTILINE) _DOWN_REV_RE = re.compile( - r'^down_revision\s*(?::\s*Union\[str,\s*(?:None|tuple)\]\s*)?=\s*(.+)', + r"^down_revision\s*(?::\s*Union\[str,\s*(?:None|tuple)\]\s*)?=\s*(.+)", re.MULTILINE, ) @@ -120,11 +120,7 @@ def check_migrations(versions_dir: Path, *, verbose: bool = False) -> list[str]: # Check 1 – duplicate revision IDs if rev in migrations: - errors.append( - f"Duplicate revision '{rev}' in:\n" - f" - {migrations[rev]['path'].name}\n" - f" - {path.name}" - ) + errors.append(f"Duplicate revision '{rev}' in:\n - {migrations[rev]['path'].name}\n - {path.name}") else: migrations[rev] = info @@ -157,7 +153,7 @@ def check_migrations(versions_dir: Path, *, verbose: bool = False) -> list[str]: errors.append( f"Multiple migration heads detected ({len(heads)}). " f"Create a merge migration to resolve:\n{head_details}\n\n" - f" Fix: alembic merge heads -m \"merge_parallel_branches\"" + f' Fix: alembic merge heads -m "merge_parallel_branches"' ) # Check 4 – revision / filename consistency ----------------------------- diff --git a/tests/test_check_alembic_migrations.py b/tests/test_check_alembic_migrations.py new file mode 100644 index 00000000..611bf647 --- /dev/null +++ b/tests/test_check_alembic_migrations.py @@ -0,0 +1,196 @@ +"""Tests for scripts/check_alembic_migrations.py.""" + +# The script lives outside of the ``app`` package, so we import it by path. +import importlib.util +import textwrap +from pathlib import Path + +import pytest + +_SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "check_alembic_migrations.py" +_spec = importlib.util.spec_from_file_location("check_alembic_migrations", _SCRIPT) +assert _spec and _spec.loader +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) # type: ignore[union-attr] + +check_migrations = _mod.check_migrations +main = _mod.main + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +def _write_migration(directory: Path, filename: str, revision: str, down_revision: str | None) -> Path: + """Helper to create a minimal migration file.""" + if down_revision is None: + down_rev_str = "None" + elif isinstance(down_revision, tuple): + down_rev_str = repr(down_revision) + else: + down_rev_str = f'"{down_revision}"' + + content = textwrap.dedent(f'''\ + """Test migration.""" + from typing import Union + revision: str = "{revision}" + down_revision: Union[str, None] = {down_rev_str} + depends_on: Union[str, None] = None + def upgrade() -> None: + pass + def downgrade() -> None: + pass + ''') + path = directory / filename + path.write_text(content) + return path + + +@pytest.fixture +def versions_dir(tmp_path: Path) -> Path: + """Return a temporary versions directory.""" + d = tmp_path / "versions" + d.mkdir() + return d + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +class TestCheckMigrations: + """Tests for the check_migrations function.""" + + def test_valid_linear_chain(self, versions_dir: Path) -> None: + """A simple linear chain should pass with no errors.""" + _write_migration(versions_dir, "001_initial.py", "001_initial", None) + _write_migration(versions_dir, "002_add_col.py", "002_add_col", "001_initial") + _write_migration(versions_dir, "003_add_table.py", "003_add_table", "002_add_col") + + errors = check_migrations(versions_dir) + assert errors == [] + + def test_valid_merge_migration(self, versions_dir: Path) -> None: + """A chain with a merge point should pass.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + _write_migration(versions_dir, "002_a.py", "002_a", "001_base") + _write_migration(versions_dir, "002_b.py", "002_b", "001_base") + + # Merge file with tuple down_revision + content = textwrap.dedent('''\ + """Merge.""" + from typing import Union + revision: str = "003_merge" + down_revision: Union[str, tuple] = ("002_a", "002_b") + depends_on: Union[str, None] = None + def upgrade() -> None: + pass + def downgrade() -> None: + pass + ''') + (versions_dir / "003_merge.py").write_text(content) + + errors = check_migrations(versions_dir) + assert errors == [] + + def test_multiple_heads_detected(self, versions_dir: Path) -> None: + """Two unmerged branches should report multiple heads.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + _write_migration(versions_dir, "002_a.py", "002_a", "001_base") + _write_migration(versions_dir, "002_b.py", "002_b", "001_base") + + errors = check_migrations(versions_dir) + assert len(errors) == 1 + assert "Multiple migration heads" in errors[0] + assert "002_a" in errors[0] + assert "002_b" in errors[0] + + def test_broken_down_revision(self, versions_dir: Path) -> None: + """A migration pointing to a non-existent parent should be flagged.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + _write_migration(versions_dir, "002_orphan.py", "002_orphan", "NONEXISTENT") + + errors = check_migrations(versions_dir) + assert any("Broken chain" in e for e in errors) + assert any("NONEXISTENT" in e for e in errors) + + def test_duplicate_revision(self, versions_dir: Path) -> None: + """Two files declaring the same revision should be flagged.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + _write_migration(versions_dir, "002_first.py", "002_dup", "001_base") + _write_migration(versions_dir, "002_second.py", "002_dup", "001_base") + + errors = check_migrations(versions_dir) + assert any("Duplicate revision" in e for e in errors) + + def test_filename_mismatch(self, versions_dir: Path) -> None: + """A file whose revision doesn't match its filename should be flagged.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + # filename stem is "002_wrong_name" but revision says "002_correct_name" + _write_migration(versions_dir, "002_wrong_name.py", "002_correct_name", "001_base") + + errors = check_migrations(versions_dir) + assert any("Filename mismatch" in e for e in errors) + + def test_empty_directory(self, versions_dir: Path) -> None: + """An empty versions directory should report an error.""" + errors = check_migrations(versions_dir) + assert len(errors) == 1 + assert "No migration files found" in errors[0] + + def test_init_py_is_skipped(self, versions_dir: Path) -> None: + """__init__.py files should be ignored.""" + (versions_dir / "__init__.py").write_text("") + _write_migration(versions_dir, "001_base.py", "001_base", None) + + errors = check_migrations(versions_dir) + assert errors == [] + + def test_non_migration_file_skipped(self, versions_dir: Path) -> None: + """A .py file without a revision variable should be silently skipped.""" + (versions_dir / "helper.py").write_text("# just a helper\nx = 1\n") + _write_migration(versions_dir, "001_base.py", "001_base", None) + + errors = check_migrations(versions_dir) + assert errors == [] + + +@pytest.mark.unit +class TestMainCLI: + """Tests for the CLI entry-point.""" + + def test_success_returns_zero(self, versions_dir: Path) -> None: + """Valid chain should exit 0.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + rc = main(["--versions-dir", str(versions_dir)]) + assert rc == 0 + + def test_failure_returns_one(self, versions_dir: Path) -> None: + """Invalid chain should exit 1.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + _write_migration(versions_dir, "002_a.py", "002_a", "001_base") + _write_migration(versions_dir, "002_b.py", "002_b", "001_base") + + rc = main(["--versions-dir", str(versions_dir)]) + assert rc == 1 + + def test_missing_directory_returns_two(self, tmp_path: Path) -> None: + """Non-existent versions directory should exit 2.""" + rc = main(["--versions-dir", str(tmp_path / "does_not_exist")]) + assert rc == 2 + + def test_verbose_flag(self, versions_dir: Path) -> None: + """The --verbose flag should not crash.""" + _write_migration(versions_dir, "001_base.py", "001_base", None) + rc = main(["--versions-dir", str(versions_dir), "--verbose"]) + assert rc == 0 + + def test_real_migrations(self) -> None: + """Smoke test against the actual project migrations.""" + real_dir = Path(__file__).resolve().parent.parent / "migrations" / "versions" + if real_dir.is_dir(): + rc = main(["--versions-dir", str(real_dir)]) + assert rc == 0