From 472cefed5577fd09060ca81b94dab9d916f0f9bb Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:39:46 +0200 Subject: [PATCH] test(migrations)+fix: widen drift check to column shape; notifications.created_at NOT NULL (S2 review #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drift check dropped every diff except table/column presence, so a column type or nullability change would slip through. Add modify_type/modify_nullable to the ops it fails on. That immediately surfaced a real pre-existing drift: notifications.created_at was created NULLABLE by migration 0017 but the model has always declared it NOT NULL (Mapped[datetime] + server_default=func.now()). Migration 0066 enforces NOT NULL to match the model — safe, since the server_default has filled every row since day one. --- .../versions/0066_notif_created_at_notnull.py | 24 +++++++++++++++++++ backend/tests/test_migrations.py | 18 ++++++++------ 2 files changed, 35 insertions(+), 7 deletions(-) create mode 100644 backend/alembic/versions/0066_notif_created_at_notnull.py diff --git a/backend/alembic/versions/0066_notif_created_at_notnull.py b/backend/alembic/versions/0066_notif_created_at_notnull.py new file mode 100644 index 00000000..486f55b9 --- /dev/null +++ b/backend/alembic/versions/0066_notif_created_at_notnull.py @@ -0,0 +1,24 @@ +"""notifications.created_at NOT NULL (align schema to the model) + +The model has always declared `Notification.created_at: Mapped[datetime]` (NOT NULL) with +`server_default=func.now()`, but migration 0017 created the column without `nullable=False`, so the live +schema allowed NULL — a model/schema drift the new migration drift-test (E1 S2) surfaced. The +server_default has filled every row since day one, so no NULLs exist; enforcing NOT NULL just makes the +schema match the contract the model already promises. Pure DDL, reversible. +""" +from typing import Sequence, Union + +from alembic import op + +revision: str = "0066_notif_created_at_notnull" +down_revision: Union[str, None] = "0065_plex_detail_fields" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.alter_column("notifications", "created_at", nullable=False) + + +def downgrade() -> None: + op.alter_column("notifications", "created_at", nullable=True) diff --git a/backend/tests/test_migrations.py b/backend/tests/test_migrations.py index 373e4413..eec06b69 100644 --- a/backend/tests/test_migrations.py +++ b/backend/tests/test_migrations.py @@ -9,11 +9,12 @@ Two things the prod boot never checks: Both use the `db` fixture, which has already brought the schema to head via `_schema`. -Scope note: we assert on STRUCTURAL drift only — tables and columns present in the model but not the -schema, or vice versa. `compare_metadata` also reports index/constraint/sequence/type differences, but -those are dominated by migration-only objects (partial/expression/GIN indexes, CHECK constraints, SERIAL -sequences) that can't be expressed on `Base.metadata`, so comparing them wholesale is noise. Missing/extra -columns and tables — the "someone changed a model without a migration" case — are the signal worth gating. +Scope note: we gate on COLUMN/TABLE drift — a table or column present on one side and not the other, or +a column whose type/nullability disagrees. `compare_metadata` also reports index/constraint/sequence +differences, but those are dominated by migration-only objects (partial/expression/GIN indexes, CHECK +constraints, SERIAL sequences) that can't be expressed on `Base.metadata`, so comparing them wholesale is +noise; they're filtered out. The column/table signal is the "someone changed a model without a migration" +case worth failing on. """ from alembic import command from alembic.autogenerate import compare_metadata @@ -22,8 +23,11 @@ from alembic.migration import MigrationContext from app.db import Base, engine -# The diff ops that mean a real table/column is present on one side and not the other. -_DRIFT_OPS = {"add_table", "remove_table", "add_column", "remove_column"} +# The diff ops that mean a real table/column disagreement (presence AND shape) — not the index/ +# constraint/sequence noise from migration-only objects. +_DRIFT_OPS = { + "add_table", "remove_table", "add_column", "remove_column", "modify_type", "modify_nullable", +} def _structural_drift():