From 368c4141312da78292b0a019cd9c5e2512513c18 Mon Sep 17 00:00:00 2001 From: Adrian Ehrsam Date: Fri, 17 Jul 2026 09:35:44 +0200 Subject: [PATCH 1/2] Remove migrations/ replay from apply_schema database/migrations/*.sql files are for one-time manual application against real (already-provisioned) databases, not for building a fresh schema -- that was never their intended purpose. Replaying them against apply_schema()'s freshly-built base schema is the wrong mechanism: it requires classifying which migration statements are "safe" to re-run purely from source text, which is unreliable in practice (e.g. a legitimate add-column-then-drop-old-column retirement migration has no sound way to distinguish itself from a genuinely destructive one using a substring heuristic alone). The correct fix for a project whose migrations/ dir has drifted ahead of its base table files is to update the base files directly so they reflect the current, final schema on their own -- apply_schema()'s existing behavior (apply every non-migrations .sql file in dependency order) is then sufficient with no special-casing needed. Bump to 0.2.1 since this reverts published migration-replay behavior from the just-released 0.2.0. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU --- pgdevkit/testdb/schema.py | 59 +++---------------- pyproject.toml | 2 +- .../002_unsafe_drop_gadget_note.sql | 3 - tests/testdb/test_schema.py | 49 +++------------ uv.lock | 2 +- 5 files changed, 16 insertions(+), 99 deletions(-) delete mode 100644 tests/testdb/fixtures/database/app/migrations/002_unsafe_drop_gadget_note.sql diff --git a/pgdevkit/testdb/schema.py b/pgdevkit/testdb/schema.py index 9633ea2..3b9bb99 100644 --- a/pgdevkit/testdb/schema.py +++ b/pgdevkit/testdb/schema.py @@ -172,54 +172,19 @@ async def _insert_test_data( await cur.executemany(insert_sql, rows) -_UNSAFE_MIGRATION_PATTERNS = ( - "DROP COLUMN", - "DROP TABLE", - "TRUNCATE", - "RENAME COLUMN", - "RENAME TO", - "DELETE FROM", - "OWNER TO", -) - - -def _is_additive_migration(sql: str) -> bool: - """Only pure-additive migrations (ADD COLUMN/CREATE ... IF NOT EXISTS, - etc.) are safe to (re-)apply against a schema that already reflects - later migrations — skip anything that could drop, rename, truncate, or - delete existing structure/data, alter an existing column, or change - ownership. This is a substring heuristic, not a SQL parser, so it errs - towards skipping (a false negative here means real data loss replayed - on every apply_schema() call) rather than trying to be exhaustive.""" - normalized = sql.upper() - has_alter_column = "ALTER COLUMN" in normalized and "ADD COLUMN" not in normalized - has_unsafe_pattern = any(pattern in normalized for pattern in _UNSAFE_MIGRATION_PATTERNS) - return not (has_alter_column or has_unsafe_pattern) - - -def _iter_migration_files(database_dir: Path): - """Yield every `*.sql` file in any `migrations/` subdirectory, in - filename order (the project's naming convention — numeric or date - prefixes — sorts chronologically).""" - for root, _, files in os.walk(database_dir): - if Path(root).name != "migrations": - continue - for file in sorted(files): - if file.endswith(".sql"): - yield Path(root) / file - - async def apply_schema( con: psycopg.AsyncConnection, database_dir: Path, extensions: tuple[str, ...] = (), force_reset: bool = False, ) -> None: - """Apply every .sql file under database_dir (in dependency-safe order), - seed any matching .test_data.json files, then apply purely-additive - `migrations/*.sql` files (see `_is_additive_migration`) so a schema that - ships changes via migration files rather than editing the base object - files stays in sync. Safe to call repeatedly.""" + """Apply every .sql file under database_dir (in dependency-safe order) + and seed any matching .test_data.json files. Safe to call repeatedly. + + `migrations/` subdirectories are never applied here — they're for + one-time manual application against real (already-provisioned) + databases, not for building a fresh schema. The base object files under + `database_dir` must reflect the current, final schema on their own.""" await con.set_autocommit(True) for extension in extensions: await con.execute(SQL("CREATE EXTENSION IF NOT EXISTS {e}").format(e=Identifier(extension))) @@ -243,13 +208,3 @@ async def _apply(file: Path, sql: str) -> None: for file, sql in failures: await _apply(file, sql) - - for migration_file in _iter_migration_files(database_dir): - content = migration_file.read_text(encoding="utf-8") - if not _is_additive_migration(content): - continue - try: - await con.execute(cast(Any, content)) - logger.info("Applied migration %s", migration_file.name) - except Exception as e: # noqa: BLE001 - logger.debug("Migration %s skipped (likely already applied): %s", migration_file.name, e) diff --git a/pyproject.toml b/pyproject.toml index 8e3e4f8..0583815 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,7 @@ packages = ["pgdevkit"] [project] name = "pgdevkit" -version = "0.2.0" +version = "0.2.1" description = "A helper for developing with Postgres" readme = "README.md" requires-python = ">=3.14" diff --git a/tests/testdb/fixtures/database/app/migrations/002_unsafe_drop_gadget_note.sql b/tests/testdb/fixtures/database/app/migrations/002_unsafe_drop_gadget_note.sql deleted file mode 100644 index c9f21ad..0000000 --- a/tests/testdb/fixtures/database/app/migrations/002_unsafe_drop_gadget_note.sql +++ /dev/null @@ -1,3 +0,0 @@ --- Deliberately destructive: must be skipped by apply_schema's additive-only --- filter, not applied. See test_apply_schema_skips_unsafe_migrations. -ALTER TABLE app.gadget DROP COLUMN note; diff --git a/tests/testdb/test_schema.py b/tests/testdb/test_schema.py index 8f71404..bf7c8fe 100644 --- a/tests/testdb/test_schema.py +++ b/tests/testdb/test_schema.py @@ -7,34 +7,13 @@ from pgdevkit.testdb import constants from pgdevkit.testdb.container import ensure_container -from pgdevkit.testdb.schema import _is_additive_migration, apply_schema +from pgdevkit.testdb.schema import apply_schema from tests.testdb.conftest import RUN_SUFFIX, requires_podman FIXTURES = Path(__file__).parent / "fixtures" / "database" TEST_DB = f"pgdevkit_schema_selftest_{RUN_SUFFIX}" -@pytest.mark.parametrize( - "sql,expected", - [ - ("ALTER TABLE t ADD COLUMN IF NOT EXISTS x text;", True), - ("CREATE TABLE IF NOT EXISTS t (id serial primary key);", True), - ("CREATE INDEX IF NOT EXISTS idx ON t(x);", True), - ("ALTER TABLE t ALTER COLUMN x TYPE text;", False), - ("ALTER TABLE t DROP COLUMN x;", False), - ("DROP TABLE t;", False), - ("DROP TABLE IF EXISTS t;", False), - ("TRUNCATE t;", False), - ("ALTER TABLE t RENAME COLUMN x TO y;", False), - ("ALTER TABLE t RENAME TO t2;", False), - ("DELETE FROM t WHERE id = 1;", False), - ("ALTER TABLE t OWNER TO someone;", False), - ], -) -def test_is_additive_migration(sql, expected): - assert _is_additive_migration(sql) is expected - - def _admin_dsn() -> str: return constants.conninfo("postgres") @@ -136,25 +115,11 @@ async def test_apply_schema_seeds_composite_enum_and_jsonb_columns(schema_test_d @requires_podman -async def test_apply_schema_applies_additive_migrations(schema_test_db): - # app/migrations/001_add_gadget_note.sql adds a column not present in - # gadget.sql itself — only reachable via the migrations pass. - async with await psycopg.AsyncConnection.connect(_db_dsn(), autocommit=True) as con: - await apply_schema(con, FIXTURES) - async with con.cursor() as cur: - await cur.execute( - "SELECT column_name FROM information_schema.columns " - "WHERE table_schema = 'app' AND table_name = 'gadget' AND column_name = 'note'" - ) - row = await cur.fetchone() - assert row is not None - - -@requires_podman -async def test_apply_schema_skips_unsafe_migrations(schema_test_db): - # app/migrations/002_unsafe_drop_gadget_note.sql drops the column that - # 001 added — apply_schema must skip it (not execute it), so the column - # added by 001 must still be present afterwards. +async def test_apply_schema_never_applies_migrations_dir(schema_test_db): + # migrations/ is for one-time manual application against real databases, + # not for building a fresh schema — the base table file is the only + # source of truth apply_schema uses. app/migrations/001_add_gadget_note.sql + # adds a column that gadget.sql itself doesn't have; it must NOT appear. async with await psycopg.AsyncConnection.connect(_db_dsn(), autocommit=True) as con: await apply_schema(con, FIXTURES) async with con.cursor() as cur: @@ -163,4 +128,4 @@ async def test_apply_schema_skips_unsafe_migrations(schema_test_db): "WHERE table_schema = 'app' AND table_name = 'gadget' AND column_name = 'note'" ) row = await cur.fetchone() - assert row is not None + assert row is None diff --git a/uv.lock b/uv.lock index d7d03c3..a8c9f62 100644 --- a/uv.lock +++ b/uv.lock @@ -267,7 +267,7 @@ wheels = [ [[package]] name = "pgdevkit" -version = "0.2.0" +version = "0.2.1" source = { editable = "." } dependencies = [ { name = "psycopg", extra = ["binary"] }, From 9a2b6d57d3ca979b3c36c18ac45d9c75100b2b25 Mon Sep 17 00:00:00 2001 From: Adrian Ehrsam Date: Fri, 17 Jul 2026 09:56:47 +0200 Subject: [PATCH 2/2] Add explicit skill note: migrations/ is never replayed by apply_schema() Call out the "why" directly where an agent would look (right after ensure_testdb()'s description in SKILL.md), since this was tried and reverted once already -- docs/database-layout.md already documented the rule ("Skipped by pgdb testdb") but that alone didn't stop it from being violated. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Ad1oGCCUheJVhcmwxHWsdU --- skills/pgdevkit/SKILL.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/skills/pgdevkit/SKILL.md b/skills/pgdevkit/SKILL.md index b785e18..f7a79f7 100644 --- a/skills/pgdevkit/SKILL.md +++ b/skills/pgdevkit/SKILL.md @@ -75,6 +75,8 @@ def _testdb_env(): `ensure_testdb()` starts the shared `pgdevkit-postgres` container if needed (via `podman`), creates a database scoped to this project+branch (so different worktrees/branches never collide), and applies every `.sql` file under `database_dir` in dependency order, seeding any `.test_data.json` sidecar files. +**`migrations/` is never applied here, on purpose.** If the test schema is missing something, that's a sign the base `tables/`/`views`/... file has drifted behind a migration that was only ever run manually against a real database — fix the base file, don't add migration-replay to `apply_schema()` (tried once, reverted: a migration can't be judged "safe to re-run" from its SQL text alone — see `docs/database-layout.md`'s Migrations section). + | What do you need? | Command | |---|---| | First-time setup / apply new files | `pgdb testdb up` |