Skip to content

Commit 1aa3193

Browse files
croc100croc100
authored
feat: actionable error messages, 3 new patterns (27→30), false-positive suite, API stability doc (#9)
Actionable error messages (schema.py, seeder.py): Before: 'Table users missing after rollback' After: 'Table users missing after rollback. Fix: add op.create_table(...) to downgrade()' All 5 failure types now include specific remediation steps 3 new Alembic static analysis patterns (detector.py): #28 DROP FOREIGN KEY without restore op.drop_constraint(type_='foreignkey') without create_foreign_key in downgrade → referential integrity silently lost on rollback #29 CREATE TRIGGER without DROP TRIGGER op.execute('CREATE TRIGGER ...') without matching DROP in downgrade → dangling trigger on rolled-back schema causes DML errors #30 CREATE TYPE without DROP TYPE op.execute('CREATE TYPE ... AS ENUM') without DROP TYPE in downgrade → re-running upgrade fails with 'type already exists' Total patterns: 27 → 30 (v1.0 target met) False-positive test suite (tests/test_false_positives.py): 16 tests covering known-safe migrations that must produce zero errors Covers: create/drop table, nullable add/drop, NOT NULL with default, rename with reverse, FK create/drop, data migration with reverse, batch alter, CONCURRENTLY index, trigger + type with proper DROP Plugin API stability (docs/plugin-api.md): Declares stable vs internal API, stable since versions, exit codes, version policy, and deprecation guarantee Added to mkdocs nav
1 parent b4adccd commit 1aa3193

7 files changed

Lines changed: 787 additions & 8 deletions

File tree

‎docs/plugin-api.md‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
# Plugin API — Stability Guarantee
2+
3+
This page documents which parts of pytest-mrt are **stable public API** vs **internal implementation details**.
4+
5+
Starting from v0.8, we follow [Semantic Versioning](https://semver.org/). Any breaking change to a stable API increments the major version.
6+
7+
---
8+
9+
## Stable public API (guaranteed no breaking changes in 0.x)
10+
11+
### `MRTConfig`
12+
13+
```python
14+
from pytest_mrt import MRTConfig
15+
```
16+
17+
All constructor parameters are stable:
18+
19+
| Parameter | Type | Stable since |
20+
|---|---|---|
21+
| `alembic_ini` | `str` | v0.4 |
22+
| `db_url` | `str` | v0.4 |
23+
| `seed_rows` | `int` | v0.5 |
24+
| `skip` | `dict[str, str]` | v0.5 |
25+
| `severity_overrides` | `dict[str, str]` | v0.7 |
26+
| `custom_seeds` | `dict[str, Callable]` | v0.6 |
27+
| `custom_checks` | `list[Callable]` | v0.7 |
28+
| `migration_timeout` | `int \| None` | v0.8 |
29+
30+
### `MRTFixture` methods (via `mrt` fixture)
31+
32+
```python
33+
def test_example(mrt):
34+
mrt.upgrade("001")
35+
mrt.downgrade()
36+
mrt.assert_all_reversible()
37+
```
38+
39+
| Method | Stable since |
40+
|---|---|
41+
| `upgrade(revision)` | v0.4 |
42+
| `downgrade(revision)` | v0.4 |
43+
| `seed(table, rows, pk_col)` | v0.5 |
44+
| `check_static(versions_dir)` | v0.7 |
45+
| `assert_no_static_errors(versions_dir)` | v0.7 |
46+
| `check_revision(revision)` | v0.5 |
47+
| `check_all()` | v0.5 |
48+
| `assert_reversible(revision)` | v0.5 |
49+
| `assert_all_reversible()` | v0.4 |
50+
| `assert_data_intact()` | v0.5 |
51+
| `reset()` | v0.5 |
52+
53+
### `RevisionResult` attributes
54+
55+
| Attribute | Type | Stable since |
56+
|---|---|---|
57+
| `revision` | `str` | v0.5 |
58+
| `passed` | `bool` | v0.5 |
59+
| `skipped` | `bool` | v0.5 |
60+
| `skip_reason` | `str` | v0.5 |
61+
| `failures` | `list[str]` | v0.5 |
62+
| `risk_score` | `int` | v0.6 |
63+
| `failure_summary()` | `str` | v0.5 |
64+
65+
### `RiskWarning` attributes
66+
67+
| Attribute | Type | Stable since |
68+
|---|---|---|
69+
| `revision` | `str` | v0.5 |
70+
| `file` | `str` | v0.5 |
71+
| `pattern` | `str` | v0.5 |
72+
| `message` | `str` | v0.5 |
73+
| `severity` | `str` | v0.5 |
74+
| `line` | `int \| None` | v0.6 |
75+
76+
### `MigrationAST` (custom checks API)
77+
78+
Functions registered via `MRTConfig(custom_checks=[fn])` receive a `MigrationAST`:
79+
80+
```python
81+
from pytest_mrt.core.ast_analyzer import MigrationAST
82+
from pytest_mrt.core.detector import RiskWarning
83+
84+
def my_check(m: MigrationAST) -> list[RiskWarning]:
85+
...
86+
```
87+
88+
Stable attributes and methods:
89+
90+
| Member | Type | Stable since |
91+
|---|---|---|
92+
| `m.revision` | `str` | v0.7 |
93+
| `m.filename` | `str` | v0.7 |
94+
| `m.source` | `str` | v0.7 |
95+
| `m._parse_error` | `Exception \| None` | v0.7 |
96+
| `m.upgrade_calls()` | `list[CallInfo]` | v0.7 |
97+
| `m.downgrade_calls()` | `list[CallInfo]` | v0.7 |
98+
| `m.upgrade_methods()` | `set[str]` | v0.7 |
99+
| `m.downgrade_methods()` | `set[str]` | v0.7 |
100+
| `m.str_arg(call, index)` | `str \| None` | v0.7 |
101+
102+
`CallInfo` stable attributes:
103+
104+
| Attribute | Type |
105+
|---|---|
106+
| `method` | `str` — the `op.*` method name |
107+
| `kw` | `dict[str, Any]` — keyword arguments |
108+
| `node` | `ast.Call` — AST node (has `.lineno`) |
109+
110+
### `__version__`
111+
112+
```python
113+
from pytest_mrt import __version__
114+
```
115+
116+
Always a PEP 440 version string. Stable since v0.4.
117+
118+
---
119+
120+
## CLI exit codes (stable since v0.8)
121+
122+
```
123+
mrt check <versions_dir>
124+
```
125+
126+
| Exit code | Meaning |
127+
|---|---|
128+
| `0` | No issues detected |
129+
| `1` | Warnings found (review before next release) |
130+
| `2` | Errors found — migrations have rollback risks |
131+
| `2` | Warnings found with `--strict` |
132+
133+
These exit codes are stable and will not change.
134+
135+
---
136+
137+
## Not stable (internal, may change)
138+
139+
The following are implementation details. Do not import or depend on them:
140+
141+
| Module / symbol | Reason |
142+
|---|---|
143+
| `pytest_mrt.core.runner.MigrationRunner` | Internal adapter for Alembic |
144+
| `pytest_mrt.core.seeder.SmartSeeder` | Internal test data layer |
145+
| `pytest_mrt.core.schema.*` | Internal schema diffing |
146+
| `pytest_mrt.core.verifier.RollbackVerifier` | Internal verification engine |
147+
| `pytest_mrt.core.detector._*` | All private check functions |
148+
| `pytest_mrt.adapters.django_detector.*` | Internal Django adapter |
149+
| `pytest_mrt.reporter.*` | Internal Rich console output |
150+
151+
If you find yourself needing to import from these modules, [open an issue](https://github.com/croc100/pytest-mrt/issues/new/choose) — we may promote it to the stable API.
152+
153+
---
154+
155+
## Version policy
156+
157+
| Version bump | What it means |
158+
|---|---|
159+
| `0.x.y` patch | Bug fixes, no API changes |
160+
| `0.x+1.0` minor | New features, fully backwards-compatible |
161+
| `1.0.0` | Stable API declaration, SemVer from here |
162+
| `2.0.0` | Breaking API change (will have deprecation warnings first) |
163+
164+
Deprecations are announced at least **one minor version** before removal.

‎mkdocs.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ nav:
3333
- Risk Patterns: patterns.md
3434
- CLI & Fixture Reference: cli.md
3535
- API Reference: api.md
36+
- Plugin API & Stability: plugin-api.md
3637
- How It Works: how-it-works.md
3738
- Best Practices: best-practices.md
3839
- FAQ: faq.md

‎pytest_mrt/core/detector.py‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,111 @@ def _check_context_execute(m: MigrationAST) -> list[RiskWarning]:
611611
]
612612

613613

614+
def _check_drop_foreign_key(m: MigrationAST) -> list[RiskWarning]:
615+
"""
616+
op.drop_constraint(type_='foreignkey') in upgrade without op.create_foreign_key in downgrade.
617+
618+
Dropping a FK constraint loses referential integrity. If the downgrade does not recreate the
619+
constraint, rolling back leaves the database without FK protection on that column pair.
620+
"""
621+
warnings = []
622+
fk_drops = [
623+
c
624+
for c in m.upgrade_calls()
625+
if c.method == "drop_constraint"
626+
and (m.kwarg_str(c.node, "type_") or "").lower() == "foreignkey"
627+
]
628+
if not fk_drops:
629+
return []
630+
631+
down_creates_fk = any(c.method == "create_foreign_key" for c in m.downgrade_calls())
632+
if down_creates_fk:
633+
return []
634+
635+
for c in fk_drops:
636+
table = m.str_arg(c.node, 1) or "?"
637+
warnings.append(
638+
_warn(
639+
m,
640+
"DROP FOREIGN KEY without restore",
641+
f"op.drop_constraint(type_='foreignkey') on '{table}' — "
642+
"referential integrity is lost unless op.create_foreign_key(...) is called in downgrade(). "
643+
"Fix: add op.create_foreign_key(...) to downgrade() to restore the constraint.",
644+
"error",
645+
line=c.node.lineno,
646+
)
647+
)
648+
return warnings
649+
650+
651+
def _check_create_trigger_without_drop(m: MigrationAST) -> list[RiskWarning]:
652+
"""
653+
op.execute('CREATE TRIGGER ...') in upgrade without DROP TRIGGER in downgrade.
654+
655+
Triggers created via raw SQL are invisible to schema diffing. If downgrade does not drop
656+
the trigger, rolling back leaves a dangling trigger that references potentially removed tables
657+
or columns, causing unexpected errors on future DML.
658+
"""
659+
import re as _re
660+
661+
# Extract string literals from upgrade execute calls
662+
upgrade_sql = " ".join(
663+
m.str_arg(c.node, 0) or "" for c in m.upgrade_calls() if c.method == "execute"
664+
)
665+
if not _re.search(r"CREATE\s+TRIGGER", upgrade_sql, _re.IGNORECASE):
666+
return []
667+
668+
downgrade_sql = " ".join(
669+
m.str_arg(c.node, 0) or "" for c in m.downgrade_calls() if c.method == "execute"
670+
)
671+
if _re.search(r"DROP\s+TRIGGER", downgrade_sql, _re.IGNORECASE):
672+
return []
673+
674+
return [
675+
_warn(
676+
m,
677+
"CREATE TRIGGER without DROP TRIGGER",
678+
"upgrade() creates a trigger via SQL but downgrade() does not DROP it. "
679+
"Fix: add op.execute('DROP TRIGGER IF EXISTS <name> ON <table>') to downgrade().",
680+
"error",
681+
)
682+
]
683+
684+
685+
def _check_create_type_without_drop(m: MigrationAST) -> list[RiskWarning]:
686+
"""
687+
op.execute('CREATE TYPE ...') in upgrade without DROP TYPE in downgrade.
688+
689+
PostgreSQL custom types (including ENUMs created via op.execute) cannot be dropped
690+
while any column references them. If downgrade does not drop the type, re-running
691+
the upgrade later will fail with 'type already exists'.
692+
"""
693+
import re as _re
694+
695+
upgrade_sql = " ".join(
696+
m.str_arg(c.node, 0) or "" for c in m.upgrade_calls() if c.method == "execute"
697+
)
698+
if not _re.search(r"CREATE\s+TYPE", upgrade_sql, _re.IGNORECASE):
699+
return []
700+
701+
downgrade_sql = " ".join(
702+
m.str_arg(c.node, 0) or "" for c in m.downgrade_calls() if c.method == "execute"
703+
)
704+
if _re.search(r"DROP\s+TYPE", downgrade_sql, _re.IGNORECASE):
705+
return []
706+
707+
return [
708+
_warn(
709+
m,
710+
"CREATE TYPE without DROP TYPE",
711+
"upgrade() creates a custom type via SQL but downgrade() does not DROP it. "
712+
"Fix: add op.execute('DROP TYPE IF EXISTS <typename>') to downgrade(). "
713+
"Ensure all columns using the type are dropped first.",
714+
"error",
715+
)
716+
]
717+
718+
614719
_PER_FILE_CHECKS = [
615720
_check_batch_alter_drop, # first: batch context needs special handling
616721
_check_downgrade_exists,
@@ -638,6 +743,9 @@ def _check_context_execute(m: MigrationAST) -> list[RiskWarning]:
638743
_check_not_null_raw_sql,
639744
_check_bulk_insert_no_reverse,
640745
_check_context_execute,
746+
_check_drop_foreign_key,
747+
_check_create_trigger_without_drop,
748+
_check_create_type_without_drop,
641749
]
642750

643751

‎pytest_mrt/core/schema.py‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,22 @@ def verify_restored(
9999
restored_t = set(after_rollback.tables)
100100

101101
for t in before_t - restored_t:
102-
issues.append(SchemaIssue(t, f"Table '{t}' missing after rollback", "error"))
102+
issues.append(
103+
SchemaIssue(
104+
t,
105+
f"Table '{t}' missing after rollback. "
106+
f"Fix: add op.create_table('{t}', ...) to downgrade(). "
107+
"If this table is intentionally dropped, use MRTConfig(skip={{...}}) with a reason.",
108+
"error",
109+
)
110+
)
103111

104112
for t in restored_t - before_t:
105113
issues.append(
106114
SchemaIssue(
107115
t,
108-
f"Table '{t}' still exists after rollback — downgrade is incomplete",
116+
f"Table '{t}' still exists after rollback — downgrade() is incomplete. "
117+
f"Fix: add op.drop_table('{t}') to downgrade().",
109118
"error",
110119
)
111120
)
@@ -115,13 +124,19 @@ def verify_restored(
115124
restored_c = set(after_rollback.tables[t].columns)
116125
for col in before_c - restored_c:
117126
issues.append(
118-
SchemaIssue(t, f"Column '{t}.{col}' missing after rollback", "error")
127+
SchemaIssue(
128+
t,
129+
f"Column '{t}.{col}' missing after rollback. "
130+
f"Fix: add op.add_column('{t}', sa.Column('{col}', ...)) to downgrade().",
131+
"error",
132+
)
119133
)
120134
for col in restored_c - before_c:
121135
issues.append(
122136
SchemaIssue(
123137
t,
124-
f"Column '{t}.{col}' still present after rollback — downgrade is incomplete",
138+
f"Column '{t}.{col}' still present after rollback — downgrade() is incomplete. "
139+
f"Fix: add op.drop_column('{t}', '{col}') to downgrade().",
125140
"warning",
126141
)
127142
)

‎pytest_mrt/core/seeder.py‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,11 @@ def verify(self) -> list[str]:
319319
tname = seeded.table
320320

321321
if tname not in existing_tables:
322-
failures.append(f"Table '{tname}' no longer exists after rollback — all data lost")
322+
failures.append(
323+
f"Table '{tname}' no longer exists after rollback — all seeded rows lost. "
324+
f"Fix: add op.create_table('{tname}', ...) to downgrade(), or "
325+
"use MRTConfig(skip={...}) with a documented reason if the data loss is intentional."
326+
)
323327
continue
324328

325329
with self.engine.connect() as conn:
@@ -334,7 +338,10 @@ def verify(self) -> list[str]:
334338

335339
if row is None:
336340
failures.append(
337-
f"Table '{tname}': row {seeded.pk_col}={seeded.pk_val!r} lost after rollback"
341+
f"Table '{tname}': row {seeded.pk_col}={seeded.pk_val!r} lost after rollback. "
342+
"This migration deletes or truncates data. "
343+
"Fix: ensure downgrade() restores the rows, or use "
344+
"MRTConfig(skip={...}) with a documented reason if the data loss is intentional."
338345
)
339346
continue
340347

@@ -345,8 +352,9 @@ def verify(self) -> list[str]:
345352
actual = current[col]
346353
if _normalize_for_compare(actual) != _normalize_for_compare(expected):
347354
failures.append(
348-
f"Table '{tname}': column '{col}' value changed after rollback "
349-
f"(expected {expected!r}, got {actual!r})"
355+
f"Table '{tname}': column '{col}' changed after rollback "
356+
f"(expected {expected!r}, got {actual!r}). "
357+
"Fix: ensure downgrade() restores the original value of this column."
350358
)
351359

352360
return failures

0 commit comments

Comments
 (0)