diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d6e136f..192bade 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -49,6 +49,8 @@ jobs: uv sync --all-extras --all-groups --no-install-project uv run alembic upgrade head uv run pytest . + uv run alembic downgrade base + uv run pytest tests/migrations --override-ini=addopts= env: SERVICE_ENVIRONMENT: ci PYTHONDONTWRITEBYTECODE: 1 diff --git a/CLAUDE.md b/CLAUDE.md index 8b2f4e8..7bcb684 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,6 +46,18 @@ on the host). Inside the container, raw commands look like `uv run pytest - `just test` cycles the DB (downgrade to `base`, upgrade to `head`) before pytest and tears the stack down before and after. Pass pytest args through, e.g. `just test tests/use_cases/test_create_chat.py -k race -x`. +- `just test-migrations` runs the `pytest-alembic` suite (`tests/migrations/`): + single head, upgrade, per-revision up/down consistency, and + model-definitions-match-DDL — the last being `alembic check` as a test. That + directory is excluded from `just test` (`--ignore` in `addopts`, and from + `coverage`'s `omit`) because it cycles the schema out from under the + transaction-rollback fixture, so it needs the `--override-ini=addopts=` the + recipe passes. CI runs both. +- Enum columns are native Postgres enums, and `alembic-postgresql-enum` is + imported by `migrations/env.py` for its autogenerate hooks — that is what + renders `CREATE TYPE` / `ALTER TYPE ... ADD VALUE` / `op.sync_enum_values` + instead of silently missing them. Add or rename an enum value and + `just migration` writes the type change for you. - `just migration "message"` takes a **single positional argument** — not a `-m` flag — quoted so a multi-word message survives as one token (the recipe shell-quotes it with `quote()` before handing it to `alembic diff --git a/Justfile b/Justfile index b02f2b0..34edc2e 100644 --- a/Justfile +++ b/Justfile @@ -12,6 +12,9 @@ test *args: down && down run: docker compose run --service-ports api sh -c "sleep 1 && uv run alembic upgrade head && uv run python -m app.api" +test-migrations *args: down && down + docker compose run api sh -c "sleep 1 && uv run alembic downgrade base && uv run pytest tests/migrations --override-ini=addopts= {{ args }}" + migration message: && down # `message` is a single named parameter, shell-quoted via quote() so a multi-word message # survives intact - a variadic *args parameter only ever joins tokens with spaces when diff --git a/alembic.ini b/alembic.ini index 455a1fa..9c8d7f5 100644 --- a/alembic.ini +++ b/alembic.ini @@ -11,6 +11,10 @@ file_template = %%(year)d-%%(month).2d-%%(day).2d_%%(slug)s # defaults to the current working directory. prepend_sys_path = . +# separator for lists such as prepend_sys_path; without it Alembic falls back to +# legacy splitting and warns, which filterwarnings = ["error"] turns into a failure. +path_separator = os + # timezone to use when rendering the date # within the migration file as well as the filename. # string value is passed to dateutil.tz.gettz() diff --git a/app/database/tables.py b/app/database/tables.py index 22983ca..27776d5 100644 --- a/app/database/tables.py +++ b/app/database/tables.py @@ -43,8 +43,7 @@ class ChatsTable(BigIntAuditBase): chat_type: orm.Mapped[ChatType] = orm.mapped_column( sa.Enum( ChatType, - native_enum=False, - create_constraint=True, + name="chattype", values_callable=lambda enum_cls: [member.value for member in enum_cls], ) ) diff --git a/app/settings.py b/app/settings.py index 3eb2d28..a5a771d 100644 --- a/app/settings.py +++ b/app/settings.py @@ -60,6 +60,11 @@ def ensure_jwt_secret_is_configured(self) -> None: def db_dsn_parsed(self) -> URL: return make_url(self.db_dsn) + @property + def sync_db_dsn_parsed(self) -> URL: + # Alembic drives psycopg2, not asyncpg. + return self.db_dsn_parsed.set(drivername="postgresql") + @property def api_bootstrapper_config(self) -> LitestarConfig: return LitestarConfig( diff --git a/migrations/env.py b/migrations/env.py index c5fb8a6..86f1311 100644 --- a/migrations/env.py +++ b/migrations/env.py @@ -1,14 +1,16 @@ from logging.config import fileConfig +import alembic_postgresql_enum from alembic import context -from sqlalchemy import URL, create_engine +from sqlalchemy import create_engine from app.database.tables import METADATA from app.settings import settings -def get_dsn() -> URL: - return settings.db_dsn_parsed.set(drivername="postgresql") +# Imported for its side effect: registering the autogenerate hooks that render CREATE TYPE / +# ALTER TYPE ... ADD VALUE for native Postgres enums. +_ = alembic_postgresql_enum config = context.config @@ -21,7 +23,7 @@ def get_dsn() -> URL: def run_migrations_offline() -> None: context.configure( - url=get_dsn(), + url=settings.sync_db_dsn_parsed, target_metadata=target_metadata, literal_binds=True, dialect_opts={"paramstyle": "named"}, @@ -31,7 +33,7 @@ def run_migrations_offline() -> None: def run_migrations_online() -> None: - connectable = create_engine(get_dsn()) + connectable = create_engine(settings.sync_db_dsn_parsed) with connectable.connect() as connection: context.configure(connection=connection, target_metadata=target_metadata) with context.begin_transaction(): diff --git a/migrations/versions/2026-08-21_chat_type_native_enum.py b/migrations/versions/2026-08-21_chat_type_native_enum.py new file mode 100644 index 0000000..db76f0d --- /dev/null +++ b/migrations/versions/2026-08-21_chat_type_native_enum.py @@ -0,0 +1,50 @@ +"""chat type native enum. + +Revision ID: fa15d87677c3 +Revises: 1be68642e392 +Create Date: 2026-08-21 18:45:32.661982 + +""" + +import sqlalchemy as sa +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "fa15d87677c3" +down_revision = "1be68642e392" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + sa.Enum("direct", "group", name="chattype").create(op.get_bind()) + op.alter_column( + "chats", + "chat_type", + existing_type=sa.VARCHAR(length=6), + type_=sa.Enum("direct", "group", name="chattype"), + existing_nullable=False, + postgresql_using="chat_type::chattype", + ) + op.drop_constraint(op.f("ck_chats_chattype"), "chats", type_="check") + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.create_check_constraint( + op.f("ck_chats_chattype"), + "chats", + "chat_type::text = ANY (ARRAY['direct'::character varying, 'group'::character varying]::text[])", + ) + op.alter_column( + "chats", + "chat_type", + existing_type=sa.Enum("direct", "group", name="chattype"), + type_=sa.VARCHAR(length=6), + existing_nullable=False, + ) + sa.Enum("direct", "group", name="chattype").drop(op.get_bind()) + # ### end Alembic commands ### diff --git a/migrations/versions/2026-08-21_messages.py b/migrations/versions/2026-08-21_messages.py index 1470192..8a5b45a 100644 --- a/migrations/versions/2026-08-21_messages.py +++ b/migrations/versions/2026-08-21_messages.py @@ -38,13 +38,6 @@ def upgrade() -> None: op.create_index("ix_messages_chat_id_id", "messages", ["chat_id", "id"], unique=False) op.create_index(op.f("ix_messages_user_id"), "messages", ["user_id"], unique=False) # ### end Alembic commands ### - # NOTE: autogenerate also proposed `op.drop_constraint(op.f('ck_chats_chattype'), 'chats', type_='check')` - # here. That's a known Alembic false positive for `sa.Enum(native_enum=False, create_constraint=True)` - # columns: Postgres reflects the CHECK constraint body back as `chat_type::text = ANY (ARRAY[...])`, - # which never textually matches what Alembic renders from the model, so every autogenerate run - # "detects" this same constraint as removed even though nothing about `chats.chat_type` changed. - # Dropping it here would be unrelated to this migration's purpose (adding `messages`) and would - # silently remove enum validation from `chats.chat_type`, so it is intentionally omitted. def downgrade() -> None: diff --git a/planning/changes/2026-08-21.03-native-enum-and-migration-tests.md b/planning/changes/2026-08-21.03-native-enum-and-migration-tests.md new file mode 100644 index 0000000..f56bdd2 --- /dev/null +++ b/planning/changes/2026-08-21.03-native-enum-and-migration-tests.md @@ -0,0 +1,93 @@ +--- +summary: Converted chats.chat_type to a native Postgres enum with alembic-postgresql-enum and added a pytest-alembic suite, turning a permanently-dirty autogenerate into a green CI gate. +--- + +# Design: Native chat_type enum and a pytest-alembic migration suite + +## Summary + +`chats.chat_type` becomes a native Postgres enum instead of `VARCHAR` plus a +CHECK constraint, `migrations/env.py` imports `alembic-postgresql-enum`, and +`tests/migrations/` runs the four `pytest-alembic` built-ins. `alembic check` is +clean for the first time, and migration health is now enforced by CI rather than +by a comment asking readers to ignore a diff. + +## Motivation + +`sa.Enum(ChatType, native_enum=False, create_constraint=True)` produced a +permanent autogenerate false positive: Postgres reflects the CHECK body back as +`chat_type::text = ANY (ARRAY[...])`, which never textually matches what Alembic +renders from the model, so every run proposed +`op.drop_constraint('ck_chats_chattype')`. The cost was not cosmetic — it meant +`alembic check` could never be a drift gate, and every `just migration` produced +a spurious operation a human had to remember to delete. A note in +`migrations/versions/2026-08-21_messages.py` documented that trap rather than +removing it. + +The private `rchat` service already solved this: native `postgresql.ENUM` +columns plus `alembic-postgresql-enum`, with `pytest-alembic` covering migration +health in a separate CI job. This repo exists to show that stack working, so it +should show that part too. + +## Design + +**Native enum.** `sa.Enum(ChatType, name="chattype", values_callable=...)` — the +`values_callable` stays, so the type's labels remain the lowercase member values. +`alembic-postgresql-enum`, imported for its side effect in `migrations/env.py`, +autogenerated the whole conversion including the `USING` clause and a working +downgrade: + +```python +sa.Enum("direct", "group", name="chattype").create(op.get_bind()) +op.alter_column("chats", "chat_type", ..., postgresql_using="chat_type::chattype") +op.drop_constraint(op.f("ck_chats_chattype"), "chats", type_="check") +``` + +Producing that by hand is exactly the work the library exists to remove, and it +is what makes a future value addition a one-command change. + +**Migration tests.** `tests/migrations/test_migrations.py` imports +`test_single_head_revision`, `test_upgrade`, `test_up_down_consistency` and +`test_model_definitions_match_ddl` from `pytest_alembic.tests`. The last is +`alembic check` as a test; the other three are coverage `just test` never had — +it only ran `downgrade base && upgrade head`, which proves neither per-revision +reversibility nor the absence of branched heads. A local `alembic_engine` +fixture replaces pytest-alembic's default in-memory SQLite engine with the real +Postgres DSN. + +The suite is excluded from the default run — `--ignore=tests/migrations` in +`addopts`, `tests/migrations/*` in coverage's `omit` — because it cycles the +schema out from under `db_session`'s transaction-rollback fixture. `just +test-migrations` runs it with `--override-ini=addopts=`, and CI runs both. + +**Incidental.** `Settings.sync_db_dsn_parsed` now owns the +`postgresql+asyncpg` → `postgresql` rewrite, which `migrations/env.py` and the +`alembic_engine` fixture both need. `alembic.ini` gains `path_separator = os`: +without it Alembic emits a `DeprecationWarning` that `filterwarnings = ["error"]` +turns into a test failure. + +## Non-goals + +- Converting anything else. `chat_type` is the only enum column in the schema. +- Suppressing the diff with an `include_object` filter in `env.py`. It would + have been two lines, but it hides a class of real diffs and keys on a + constraint name that the next enum column would not share. + +## Testing + +- `just test` — 109 passed, 100% coverage. +- `just test-migrations` — 4 passed. +- `just lint` — clean. +- `alembic upgrade head && alembic check` — clean, where it previously reported + `remove_constraint ck_chats_chattype`. `alembic downgrade -1 && alembic + upgrade head` round-trips. + +## Risk + +- **Adding an enum value is now a migration.** That is the point — it was + previously an unenforced CHECK constraint edit — but it does mean a value + added to `ChatType` without running `just migration` fails + `test_model_definitions_match_ddl` rather than passing silently. +- **The conversion is not free on a large table.** `ALTER COLUMN ... TYPE` + rewrites `chats`. It is cheap now, while the table is effectively empty, and + gets more expensive the longer it waits. diff --git a/pyproject.toml b/pyproject.toml index 1d6308d..e416bff 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,6 +17,7 @@ dependencies = [ "db-retry", # database "alembic", + "alembic-postgresql-enum", "psycopg2", "sqlalchemy[asyncio]", "asyncpg", @@ -34,6 +35,7 @@ dev = [ "pytest-asyncio", "asgi_lifespan", "modern-di-pytest>=3,<4", + "pytest-alembic", ] lint = ["ruff", "ty", "eof-fixer"] @@ -70,7 +72,7 @@ isort.no-lines-before = ["standard-library", "local-folder"] "migrations/*.py" = ["ERA001"] [tool.pytest.ini_options] -addopts = "--cov=. --cov-report term-missing --cov-fail-under=100" +addopts = "--cov=. --cov-report term-missing --cov-fail-under=100 --ignore=tests/migrations" asyncio_mode = "auto" asyncio_default_fixture_loop_scope = "function" filterwarnings = ["error"] @@ -83,6 +85,9 @@ concurrency = ["thread", "greenlet"] disable_warnings = ["couldnt-parse"] omit = [ "migrations/*", + # Excluded from the default pytest run (see addopts) because they cycle the schema; they + # run under `just test-migrations` instead, where coverage is off. + "tests/migrations/*", "app/api/__main__.py", # No __init__.py under planning/ (see the comment atop planning/index.py), so coverage's # package walk never reaches this file on its own - omit it explicitly rather than relying diff --git a/tests/migrations/__init__.py b/tests/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/migrations/conftest.py b/tests/migrations/conftest.py new file mode 100644 index 0000000..4f49702 --- /dev/null +++ b/tests/migrations/conftest.py @@ -0,0 +1,15 @@ +import typing + +import pytest +from sqlalchemy import Engine, create_engine + +from app.settings import settings + + +@pytest.fixture +def alembic_engine() -> typing.Iterator[Engine]: + # Overrides pytest-alembic's default in-memory SQLite engine: these tests are only + # meaningful against the Postgres the migrations actually target. + engine: typing.Final = create_engine(settings.sync_db_dsn_parsed) + yield engine + engine.dispose() diff --git a/tests/migrations/test_migrations.py b/tests/migrations/test_migrations.py new file mode 100644 index 0000000..1b937f7 --- /dev/null +++ b/tests/migrations/test_migrations.py @@ -0,0 +1,12 @@ +from pytest_alembic.tests import ( + test_model_definitions_match_ddl, + test_single_head_revision, + test_up_down_consistency, + test_upgrade, +) + + +_ = test_single_head_revision +_ = test_upgrade +_ = test_model_definitions_match_ddl +_ = test_up_down_consistency diff --git a/tests/test_settings.py b/tests/test_settings.py index 87e3209..b424dbd 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -9,6 +9,12 @@ def test_db_dsn_parsed_exposes_driver() -> None: assert settings.db_dsn_parsed.database == "dbname" +def test_sync_db_dsn_parsed_drops_the_async_driver() -> None: + settings = Settings(db_dsn="postgresql+asyncpg://user:pw@host/dbname") + assert settings.sync_db_dsn_parsed.drivername == "postgresql" + assert settings.sync_db_dsn_parsed.database == "dbname" + + def test_api_bootstrapper_config_carries_service_identity() -> None: settings = Settings(service_name="svc", service_version="9.9.9") config = settings.api_bootstrapper_config