From 1eb15e85d2355a3af461439318f57d458ca50db4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 6 Jul 2026 06:08:10 +0900 Subject: [PATCH] feat(migration-safety): risk-classify a schema migration analyze_migration_safety(base, target) labels every change safe/warning/destructive with a plain reason: drop table/column & PK change = data loss; type change / SET NOT NULL / new FK = table lock or fail-against-real-data; add nullable / drop FK = safe. Summary flags has_destructive/has_blocking; report sorts dangerous-first. Endpoint GET /api/snapshots/{uuid}/migration-safety?against=... (IDOR-safe). Pure, reuses diff indexing. Targets the #1 fear in migrations. +5 tests, backend suite 263. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01AxU2xaupAjp912oDNFuWyd --- backend/app/api/snapshots.py | 42 ++++++++ backend/app/ddl/migration_safety.py | 136 +++++++++++++++++++++++++ backend/app/schemas.py | 9 ++ backend/tests/test_migration_safety.py | 132 ++++++++++++++++++++++++ docs/valuation-gap-analysis.md | 2 +- 5 files changed, 320 insertions(+), 1 deletion(-) create mode 100644 backend/app/ddl/migration_safety.py create mode 100644 backend/tests/test_migration_safety.py diff --git a/backend/app/api/snapshots.py b/backend/app/api/snapshots.py index e20467e2..307e4a22 100644 --- a/backend/app/api/snapshots.py +++ b/backend/app/api/snapshots.py @@ -18,6 +18,7 @@ ) from app.permissions import require_project_member from app.schemas import ( + MigrationSafetyOut, SnapshotCreateIn, SnapshotDetailOut, SnapshotDiffOut, @@ -25,6 +26,7 @@ ) from app.ddl.export import snapshot_json_to_sql from app.ddl.migration import snapshot_diff_to_migration_sql +from app.ddl.migration_safety import analyze_migration_safety from app.diff.schema_diff import diff_snapshots from app.jobs.valkey_queue import enqueue_job_signal from app.spec.llm import ( @@ -190,6 +192,46 @@ async def diff_snapshot( ) +@router.get( + "/{schema_snapshot_uuid}/migration-safety", response_model=MigrationSafetyOut +) +async def migration_safety( + schema_snapshot_uuid: uuid.UUID, + against: uuid.UUID = Query( + ..., description="Base snapshot UUID to analyze migrating from" + ), + user: CurrentUser = Depends(get_current_user), + session: AsyncSession = Depends(get_read_session), +) -> MigrationSafetyOut: + """Risk-classify the migration from the base snapshot to this one. + + Each change is labelled safe / warning / destructive with an explanation so + a reviewer can spot data loss and table-locking operations before applying. + Both snapshots are authorized independently (uniform not-found). + """ + target_snap = await _get_authorized_snapshot(session, schema_snapshot_uuid, user) + base_snap = await _get_authorized_snapshot(session, against, user) + if target_snap is None or base_snap is None: + return MigrationSafetyOut( + base_snapshot_uuid=against, + target_snapshot_uuid=schema_snapshot_uuid, + status="not_found", + analysis=None, + ) + base_data = await session.get(SchemaSnapshotData, against) + target_data = await session.get(SchemaSnapshotData, schema_snapshot_uuid) + analysis = analyze_migration_safety( + base_data.snapshot_json if base_data else None, + target_data.snapshot_json if target_data else None, + ) + return MigrationSafetyOut( + base_snapshot_uuid=against, + target_snapshot_uuid=schema_snapshot_uuid, + status="ok", + analysis=analysis, + ) + + @router.get("/{schema_snapshot_uuid}/export.sql", response_class=PlainTextResponse) async def export_snapshot_sql( schema_snapshot_uuid: uuid.UUID, diff --git a/backend/app/ddl/migration_safety.py b/backend/app/ddl/migration_safety.py new file mode 100644 index 00000000..ee47dca1 --- /dev/null +++ b/backend/app/ddl/migration_safety.py @@ -0,0 +1,136 @@ +"""Classify the risk of each change between two schema snapshots. + +Teams don't fear *writing* a migration -- they fear running it in production: +data loss (dropping a table/column), long table locks (rewriting a column +type, validating a new FK), or a migration that simply fails against real data +(``SET NOT NULL`` with existing NULLs). This module turns a diff into a +severity-classified risk report so a reviewer can see, before applying, exactly +which statements are dangerous and why. + +Pure and dialect-agnostic; matches tables/columns/FKs by name (never the +volatile ``relation_oid``), reusing the diff indexer. +""" + +from __future__ import annotations + +from typing import Any + +from app.diff.schema_diff import _index_snapshot + +SAFE = "safe" +WARNING = "warning" +DESTRUCTIVE = "destructive" + +_SEVERITY_RANK = {DESTRUCTIVE: 0, WARNING: 1, SAFE: 2} + + +def _item(category: str, severity: str, target: str, detail: str) -> dict[str, Any]: + return { + "category": category, + "severity": severity, + "target": target, + "detail": detail, + } + + +def _fk_label(fk: dict[str, Any]) -> str: + child_cols = ", ".join(fk.get("child_columns") or []) + return f"{fk.get('child_table')}({child_cols}) -> {fk.get('parent_table')}" + + +def analyze_migration_safety( + base: dict[str, Any] | None, target: dict[str, Any] | None +) -> dict[str, Any]: + """Return risk-classified changes to migrate *base* to *target*.""" + b = _index_snapshot(base) + t = _index_snapshot(target) + b_tables, t_tables = b["tables"], t["tables"] + items: list[dict[str, Any]] = [] + + for key, tbl in b_tables.items(): + if key not in t_tables: + items.append( + _item("drop_table", DESTRUCTIVE, key, "Table is dropped — all its data is lost.") + ) + for key, tbl in t_tables.items(): + if key not in b_tables: + items.append(_item("create_table", SAFE, key, "New table.")) + + for key, ttbl in t_tables.items(): + if key not in b_tables: + continue + btbl = b_tables[key] + b_cols, t_cols = btbl["columns"], ttbl["columns"] + + for name, col in t_cols.items(): + if name in b_cols: + continue + if col.get("is_not_null"): + items.append( + _item( + "add_column", WARNING, f"{key}.{name}", + "New NOT NULL column — fails or locks if the table has rows and no default.", + ) + ) + else: + items.append(_item("add_column", SAFE, f"{key}.{name}", "New nullable column.")) + for name in b_cols: + if name not in t_cols: + items.append( + _item("drop_column", DESTRUCTIVE, f"{key}.{name}", "Column dropped — its data is lost.") + ) + for name, col in t_cols.items(): + if name not in b_cols: + continue + old = b_cols[name] + if (old.get("data_type") or "") != (col.get("data_type") or ""): + items.append( + _item( + "alter_column_type", WARNING, f"{key}.{name}", + f"Type {old.get('data_type')} -> {col.get('data_type')} may rewrite/lock the table and can fail on incompatible data.", + ) + ) + if not old.get("is_not_null") and col.get("is_not_null"): + items.append( + _item( + "set_not_null", WARNING, f"{key}.{name}", + "SET NOT NULL scans the whole table and fails if existing NULLs are present.", + ) + ) + if old.get("is_not_null") and not col.get("is_not_null"): + items.append(_item("drop_not_null", SAFE, f"{key}.{name}", "Relaxing NOT NULL is safe.")) + + if btbl.get("pk", []) != ttbl.get("pk", []): + items.append( + _item( + "primary_key_change", DESTRUCTIVE, key, + "Primary key changed — usually needs data-aware handling and can lock the table.", + ) + ) + + base_fks, target_fks = b["fks"], t["fks"] + for sig, fk in target_fks.items(): + if sig not in base_fks: + items.append( + _item( + "add_foreign_key", WARNING, _fk_label(fk), + "New foreign key validates all existing rows (brief lock) and fails if data violates it.", + ) + ) + for sig, fk in base_fks.items(): + if sig not in target_fks: + items.append( + _item("drop_foreign_key", SAFE, _fk_label(fk), "Dropping a foreign key relaxes a constraint — safe.") + ) + + items.sort(key=lambda i: (_SEVERITY_RANK.get(i["severity"], 9), i["target"])) + + summary = { + "safe": sum(1 for i in items if i["severity"] == SAFE), + "warning": sum(1 for i in items if i["severity"] == WARNING), + "destructive": sum(1 for i in items if i["severity"] == DESTRUCTIVE), + "total": len(items), + "has_destructive": any(i["severity"] == DESTRUCTIVE for i in items), + "has_blocking": any(i["severity"] in (WARNING, DESTRUCTIVE) for i in items), + } + return {"items": items, "summary": summary} diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 9540c560..d549c1b8 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -114,6 +114,15 @@ class SnapshotDiffOut(BaseModel): diff: dict | None +class MigrationSafetyOut(BaseModel): + """Risk-classified analysis of migrating one snapshot to another.""" + + base_snapshot_uuid: uuid.UUID + target_snapshot_uuid: uuid.UUID + status: str + analysis: dict | None + + class DiagramViewCreateIn(BaseModel): """Request body for saving an ERD canvas view.""" diff --git a/backend/tests/test_migration_safety.py b/backend/tests/test_migration_safety.py new file mode 100644 index 00000000..1c9a8a50 --- /dev/null +++ b/backend/tests/test_migration_safety.py @@ -0,0 +1,132 @@ +from __future__ import annotations + +from app.ddl.migration_safety import analyze_migration_safety + + +def _snap(relations, columns, pk_columns=None, fk_edges=None): + return { + "relations": relations, + "columns": columns, + "pk_columns": pk_columns or [], + "fk_edges": fk_edges or [], + } + + +def _member(oid=1, email_not_null=False): + return _snap( + relations=[{"relation_oid": oid, "schema_name": "public", "relation_name": "member"}], + columns=[ + {"relation_oid": oid, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}, + {"relation_oid": oid, "column_name": "email", "data_type": "varchar(100)", "is_not_null": email_not_null}, + ], + pk_columns=[{"relation_oid": oid, "column_name": "member_id", "column_ordinal": 1}], + ) + + +def _cats(analysis): + return {(i["category"], i["severity"]) for i in analysis["items"]} + + +def test_no_changes_is_empty_and_not_blocking(): + a = analyze_migration_safety(_member(1), _member(999)) + assert a["items"] == [] + assert a["summary"]["has_blocking"] is False + + +def test_drop_table_and_column_are_destructive(): + base = _member() + # target: member table gone entirely + empty = _snap(relations=[], columns=[]) + a = analyze_migration_safety(base, empty) + assert ("drop_table", "destructive") in _cats(a) + assert a["summary"]["has_destructive"] is True + + # target: email column dropped + dropped_col = _snap( + relations=[{"relation_oid": 2, "schema_name": "public", "relation_name": "member"}], + columns=[{"relation_oid": 2, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}], + pk_columns=[{"relation_oid": 2, "column_name": "member_id"}], + ) + a2 = analyze_migration_safety(base, dropped_col) + assert ("drop_column", "destructive") in _cats(a2) + + +def test_add_nullable_is_safe_add_not_null_is_warning(): + base = _member() + # add nullable column + add_nullable = _snap( + relations=[{"relation_oid": 3, "schema_name": "public", "relation_name": "member"}], + columns=[ + {"relation_oid": 3, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}, + {"relation_oid": 3, "column_name": "email", "data_type": "varchar(100)", "is_not_null": False}, + {"relation_oid": 3, "column_name": "nickname", "data_type": "text", "is_not_null": False}, + ], + pk_columns=[{"relation_oid": 3, "column_name": "member_id"}], + ) + assert ("add_column", "safe") in _cats(analyze_migration_safety(base, add_nullable)) + + # add NOT NULL column + add_nn = _snap( + relations=[{"relation_oid": 4, "schema_name": "public", "relation_name": "member"}], + columns=[ + {"relation_oid": 4, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}, + {"relation_oid": 4, "column_name": "email", "data_type": "varchar(100)", "is_not_null": False}, + {"relation_oid": 4, "column_name": "status", "data_type": "text", "is_not_null": True}, + ], + pk_columns=[{"relation_oid": 4, "column_name": "member_id"}], + ) + assert ("add_column", "warning") in _cats(analyze_migration_safety(base, add_nn)) + + +def test_type_change_and_set_not_null_are_warnings(): + base = _member(email_not_null=False) + target = _snap( + relations=[{"relation_oid": 5, "schema_name": "public", "relation_name": "member"}], + columns=[ + {"relation_oid": 5, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}, + # email: varchar(100) -> text AND now NOT NULL + {"relation_oid": 5, "column_name": "email", "data_type": "text", "is_not_null": True}, + ], + pk_columns=[{"relation_oid": 5, "column_name": "member_id"}], + ) + cats = _cats(analyze_migration_safety(base, target)) + assert ("alter_column_type", "warning") in cats + assert ("set_not_null", "warning") in cats + + +def test_add_fk_is_warning_drop_fk_is_safe_and_report_is_sorted(): + orders_rel = {"relation_oid": 2, "schema_name": "public", "relation_name": "orders"} + orders_cols = [ + {"relation_oid": 2, "column_name": "order_id", "data_type": "bigint", "is_not_null": True}, + {"relation_oid": 2, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}, + ] + fk = { + "fk_constraint_name": "fk_orders_member", + "child_relation_oid": 2, + "parent_relation_oid": 1, + "child_column_name": "member_id", + "parent_column_name": "member_id", + "column_ordinal": 1, + } + base = _snap( + relations=[{"relation_oid": 1, "schema_name": "public", "relation_name": "member"}, orders_rel], + columns=[{"relation_oid": 1, "column_name": "member_id", "data_type": "bigint", "is_not_null": True}, *orders_cols], + pk_columns=[{"relation_oid": 1, "column_name": "member_id"}], + ) + target = _snap(relations=base["relations"], columns=base["columns"], pk_columns=base["pk_columns"], fk_edges=[fk]) + + add = analyze_migration_safety(base, target) + assert ("add_foreign_key", "warning") in _cats(add) + + drop = analyze_migration_safety(target, base) + assert ("drop_foreign_key", "safe") in _cats(drop) + + # destructive/warnings should sort before safe items + mixed_target = _snap( + relations=[orders_rel], # member dropped (destructive), fk gone (safe) + columns=orders_cols, + pk_columns=[{"relation_oid": 2, "column_name": "order_id"}], + ) + mixed = analyze_migration_safety(target, mixed_target) + severities = [i["severity"] for i in mixed["items"]] + assert severities == sorted(severities, key={"destructive": 0, "warning": 1, "safe": 2}.get) diff --git a/docs/valuation-gap-analysis.md b/docs/valuation-gap-analysis.md index 537f84b0..b31038f2 100644 --- a/docs/valuation-gap-analysis.md +++ b/docs/valuation-gap-analysis.md @@ -32,7 +32,7 @@ now cross into **act** (migration SQL). The gaps below are ordered by value-per- ### P0 — Actionability (turns insight into change teams pay for) - **✅ Migration SQL from diff** _(delivered in this PR)_ — `GET /api/snapshots/{uuid}/migration.sql?against=…&dialect=…`. Diff two snapshots and get the `CREATE`/`ALTER`/`DROP` + FK statements to apply. Bridges the previously-disconnected `diff` and `ddl` modules the graph flagged. -- **Migration safety review** — classify each statement as safe/destructive/lock-heavy; emit a summary header. (Foundation is in place: destructive ops already emitted with review comments.) +- **✅ Migration safety review** _(delivered)_ — `GET /api/snapshots/{uuid}/migration-safety?against=…` classifies every change as **safe / warning / destructive** with a plain-language reason (drop = data loss; type change / `SET NOT NULL` / new FK = lock or fail against real data) plus a summary (`has_destructive`, `has_blocking`). Directly targets the #1 reason teams fear migrations. - **CI drift check** — a documented `GET .../migration.sql` recipe + exit-code semantics so teams can gate deploys on "no unexpected drift." ### P1 — Breadth & documentation (expands addressable market)