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/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` | 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"] },