From db51a4c820410ddc9bc6a23aeab90a581f7a5a62 Mon Sep 17 00:00:00 2001 From: Adrian Ehrsam Date: Tue, 8 Sep 2026 14:27:20 +0200 Subject: [PATCH 1/2] Re-apply permissions files after every delayed-retry converges apply_schema()'s _iter_sql_files() sorts files by (type_order, filename) and does a first-pass-then-delayed-retry resolution for anything with an unresolved SQL dependency at first attempt. A `permissions`-type file (type_order=100, e.g. `grant select, insert, update, delete on all tables in schema x to role;`) has no SQL dependencies of its own -- GRANT statements aren't parsed as depending on the tables they name -- so it always applies in the very first pass, at its sorted position. But a table with an FK to another table in a different layer directory whose filename happens to sort later fails its own first-pass attempt and only actually gets created via the delayed-retry loop, which runs after every non-delayed file -- permissions included. A blanket "grant ... on all tables in schema X" is a one-time snapshot: it silently never covers a table created after it ran. Confirmed this is real, not theoretical: in a downstream project's ~200-file database/ tree, a live _iter_sql_files() dry run found 25 separate table/view files landing after its permissions file in actual apply order, each a genuine missing-grant bug that only surfaced once something connected as a restricted role instead of an admin/superuser connection. Fix: re-execute every permissions-type file (detected generically via _get_type_order(file) == _TYPE_ORDER["permissions"], not hardcoded filenames -- works for env-tagged variants like permissions.local_test.sql too) a second time, after the whole apply (delayed retries included) has fully converged. GRANT is idempotent, so this is a no-op for anything the first pass already covered, and correctly grants anything created only via delayed-retry. Applied to both the Postgres path (apply_schema()) and the MSSQL path (mssql/api.py's _apply(), which has its own separate apply loop with no outer retry of its own) for consistency. Explicitly not extended to `indexes` (type_order=101): CREATE INDEX ... ON tablename already has a real, correctly-tracked per-object dependency via the existing scanner, unlike a schema-wide GRANT. Deliberately not implemented as ALTER DEFAULT PRIVILEGES -- that approach is scoped to "whatever role ran the ALTER DEFAULT PRIVILEGES statement" and silently breaks once a different identity later creates objects in the same schema (confirmed by a downstream project's own real incident history). Re-running the actual GRANT statements has no such assumption. New regression test (test_apply_schema_reapplies_permissions_for_a_delayed_table) with a fixture pair (app.event's filename sorts before its own FK target app.event_kind's filename, forcing the exact delayed-retry scenario) and a permissions/grants.sql blanket grant -- verified this fails with psycopg.errors.InsufficientPrivilege without the fix and passes with it, connecting as the granted role and querying the delayed table directly (not just asserting apply_schema() doesn't raise). Full testdb suite (78 tests, temporarily running with @requires_podman's Docker-API check bypassed against this sandbox's already-reachable local Postgres) passes; the one pre-existing failure (test_container.py, which genuinely needs Docker/Podman itself) is unrelated and expected. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Aqh5NvxzHH2ysU4ziSuwov --- docs/database-layout.md | 12 ++++++++ pgdevkit/testdb/mssql/api.py | 26 +++++++++++++---- pgdevkit/testdb/schema.py | 27 ++++++++++++++++++ pyproject.toml | 2 +- .../fixtures/database/app/tables/event.sql | 11 ++++++++ .../database/app/tables/event_kind.sql | 4 +++ .../fixtures/database/permissions/grants.sql | 10 +++++++ tests/testdb/test_schema.py | 28 +++++++++++++++++++ uv.lock | 2 +- 9 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 tests/testdb/fixtures/database/app/tables/event.sql create mode 100644 tests/testdb/fixtures/database/app/tables/event_kind.sql create mode 100644 tests/testdb/fixtures/database/permissions/grants.sql diff --git a/docs/database-layout.md b/docs/database-layout.md index 95302d6..04b624b 100644 --- a/docs/database-layout.md +++ b/docs/database-layout.md @@ -63,6 +63,18 @@ both together. One object per file: `tables/user.sql`, `views/all_edits.sql`, `types/measurement_unit.sql`. +**`permissions` files apply twice.** A blanket `GRANT ... ON ALL TABLES IN +SCHEMA x TO role;` only covers what exists at the moment it runs — but a +table with a cross-file dependency that isn't resolved on the first pass +(e.g. an FK to a table in another layer directory whose filename happens to +sort later) can end up created *after* every `permissions` file already ran, +via `pgdb testdb`'s delayed-retry resolution. So every `permissions` file is +re-applied a second time, once the whole tree (delayed retries included) has +fully converged — closing that gap. GRANT is idempotent, so the second pass +is a no-op for anything the first pass already covered; write `permissions` +files as if they always ran last, since — after this second pass — they +effectively do. + --- ## File-naming conventions diff --git a/pgdevkit/testdb/mssql/api.py b/pgdevkit/testdb/mssql/api.py index fbc0a96..9a9f40c 100644 --- a/pgdevkit/testdb/mssql/api.py +++ b/pgdevkit/testdb/mssql/api.py @@ -12,7 +12,7 @@ from ...envtag import strip_env_suffix from .. import query from ..config import ProjectConfig -from ..schema import _iter_sql_files, _strip_layer_prefix +from ..schema import _TYPE_ORDER, _get_type_order, _iter_sql_files, _strip_layer_prefix from . import constants from .container import ensure_mssql_container @@ -121,10 +121,8 @@ def _connect() -> Any: database_dir = config.root / config.database_dir if not database_dir.is_dir(): return - for file, sql in _iter_sql_files( - database_dir, MSSQL, - env=env, areas=areas, exclude_areas=exclude_areas, schemas=schemas, exclude_schemas=exclude_schemas, - ): + + async def _apply_file(file: Path, sql: str) -> None: for batch in query.split_tsql_batches(sql): def _exec(batch: str = batch) -> None: @@ -138,6 +136,24 @@ def _exec(batch: str = batch) -> None: await _insert_test_data( json_file, f"{schema_name}.{_strip_layer_prefix(table_stem)}", force_reset, conn ) + + permissions_files: list[tuple[Path, str]] = [] + for file, sql in _iter_sql_files( + database_dir, MSSQL, + env=env, areas=areas, exclude_areas=exclude_areas, schemas=schemas, exclude_schemas=exclude_schemas, + ): + if _get_type_order(file) == _TYPE_ORDER["permissions"]: + permissions_files.append((file, sql)) + await _apply_file(file, sql) + + # Same fix as the Postgres path (see apply_schema()'s docstring/comment in + # ..schema): a permissions-type file has no SQL dependencies of its own, so + # it always applies at its sorted position in the pass above -- even before + # a table whose own creation got deferred by a same-type file elsewhere. Re- + # running permissions files now, after everything genuinely exists, closes + # that gap; GRANT is idempotent. + for file, sql in permissions_files: + await _apply_file(file, sql) finally: await asyncio.to_thread(conn.close) diff --git a/pgdevkit/testdb/schema.py b/pgdevkit/testdb/schema.py index d81c82b..30b2ff5 100644 --- a/pgdevkit/testdb/schema.py +++ b/pgdevkit/testdb/schema.py @@ -276,6 +276,14 @@ async def apply_schema( """Apply every .sql file under database_dir (in dependency-safe order) and seed any matching .test_data.json files. Safe to call repeatedly. + Every `permissions`-type file (see `_TYPE_ORDER`) is applied a second time + after the whole tree has been applied (delayed-retry included) — a blanket + "grant ... on all tables in schema X" only covers what exists at the moment + it runs, and a table whose own creation got deferred to the delayed-retry + path (an FK to a same-type file that happens to sort later) would otherwise + silently miss it. GRANT is idempotent, so the second pass is a no-op for + anything that already had the grant. + A file tagged for another environment (e.g. `grants.prod.sql` when `env="local_test"`) is skipped — see pgdevkit.envtag. @@ -305,6 +313,7 @@ async def _apply(file: Path, sql: str) -> None: ) failures: list[tuple[Path, str]] = [] + permissions_files: list[tuple[Path, str]] = [] for file, sql in _iter_sql_files( database_dir, dialect, @@ -314,6 +323,8 @@ async def _apply(file: Path, sql: str) -> None: schemas=schemas, exclude_schemas=exclude_schemas, ): + if _get_type_order(file) == _TYPE_ORDER["permissions"]: + permissions_files.append((file, sql)) try: await _apply(file, sql) except Exception as e: # noqa: BLE001 @@ -338,3 +349,19 @@ async def _apply(file: Path, sql: str) -> None: assert last_error is not None raise last_error failures = still_failing + + # A "permissions"-type file (e.g. `grant select, insert, update, delete on all + # tables in schema editing to app_role;`) has no SQL dependencies of its own — + # GRANT statements aren't parsed as depending on the tables they name — so it + # always applies in the very first pass above, at its sorted position. But a + # *table* with an unresolved dependency at that point (e.g. an FK to a table + # in a different layer directory whose filename happens to sort later) gets + # deferred to the delayed-retry loop inside _iter_sql_files() and only + # actually exists *after* the permissions file already ran — so a blanket + # "grant ... on all tables in schema X", being a one-time snapshot, silently + # never covers it. Re-running every permissions file now, after the full + # apply above (delayed retries included) has converged and every object + # genuinely exists, closes that gap. GRANT is idempotent, so this is a no-op + # for anything that already had the grant from the first pass. + for file, sql in permissions_files: + await _apply(file, sql) diff --git a/pyproject.toml b/pyproject.toml index cdd3408..fee7c56 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,7 @@ packages = ["pgdevkit"] [project] name = "pgdevkit" -version = "0.6.0" +version = "0.6.1" description = "A helper for developing with Postgres" readme = "README.md" requires-python = ">=3.14" diff --git a/tests/testdb/fixtures/database/app/tables/event.sql b/tests/testdb/fixtures/database/app/tables/event.sql new file mode 100644 index 0000000..cec9e17 --- /dev/null +++ b/tests/testdb/fixtures/database/app/tables/event.sql @@ -0,0 +1,11 @@ +-- Filename ("event.sql") deliberately sorts alphabetically before its own FK +-- target's filename ("event_kind.sql") -- '.' (0x2E) sorts before '_' (0x5F) -- +-- so this file's first apply attempt always fails (app.event_kind doesn't +-- exist yet) and it only actually gets created via _iter_sql_files()'s +-- delayed-retry pass, which runs after every non-delayed file (permissions +-- included) has already been attempted. See +-- test_apply_schema_reapplies_permissions_for_a_delayed_table. +CREATE TABLE IF NOT EXISTS app.event ( + id serial PRIMARY KEY, + event_kind_id int NOT NULL REFERENCES app.event_kind(id) +); diff --git a/tests/testdb/fixtures/database/app/tables/event_kind.sql b/tests/testdb/fixtures/database/app/tables/event_kind.sql new file mode 100644 index 0000000..25bfe4b --- /dev/null +++ b/tests/testdb/fixtures/database/app/tables/event_kind.sql @@ -0,0 +1,4 @@ +CREATE TABLE IF NOT EXISTS app.event_kind ( + id serial PRIMARY KEY, + name text NOT NULL +); diff --git a/tests/testdb/fixtures/database/permissions/grants.sql b/tests/testdb/fixtures/database/permissions/grants.sql new file mode 100644 index 0000000..0666122 --- /dev/null +++ b/tests/testdb/fixtures/database/permissions/grants.sql @@ -0,0 +1,10 @@ +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'pgdevkit_test_reader') THEN + CREATE ROLE pgdevkit_test_reader LOGIN PASSWORD 'testpwd'; + END IF; +END +$$; + +GRANT USAGE ON SCHEMA app TO pgdevkit_test_reader; +GRANT SELECT ON ALL TABLES IN SCHEMA app TO pgdevkit_test_reader; diff --git a/tests/testdb/test_schema.py b/tests/testdb/test_schema.py index eaa1c7e..a146463 100644 --- a/tests/testdb/test_schema.py +++ b/tests/testdb/test_schema.py @@ -4,6 +4,7 @@ import psycopg import pytest +from psycopg.conninfo import make_conninfo from pgdevkit.testdb import constants from pgdevkit.testdb.container import ensure_container @@ -97,6 +98,33 @@ async def test_apply_schema_resolves_view_to_view_dependency(schema_test_db): assert rows == [(1, "sprocket")] +@requires_podman +async def test_apply_schema_reapplies_permissions_for_a_delayed_table(schema_test_db): + # permissions/grants.sql (a blanket "grant select on all tables in schema app") + # has no SQL dependencies of its own, so it always applies in the first pass, at + # its sorted position -- before app.event, whose own filename ("event.sql") + # sorts alphabetically before its FK target's filename ("event_kind.sql") and + # so always falls to the delayed-retry pass, landing (chronologically) after + # permissions/grants.sql already ran. A blanket GRANT is a one-time snapshot: on + # its own it would never cover app.event. Confirms apply_schema() closes this by + # re-running every permissions-type file again after the whole tree (delayed + # retries included) has converged. + async with await psycopg.AsyncConnection.connect(_db_dsn(), autocommit=True) as con: + await apply_schema(con, FIXTURES) # must not raise + + # permissions/grants.sql creates this login role itself -- a fixed, + # non-secret password, matching pgdevkit's own testdb container convention. + reader_dsn = make_conninfo( + host=constants.HOST, port=str(constants.PORT), dbname=TEST_DB, + user="pgdevkit_test_reader", password="testpwd", + ) + async with await psycopg.AsyncConnection.connect(reader_dsn, autocommit=True) as con: + async with con.cursor() as cur: + await cur.execute("SELECT count(*) FROM app.event") # must not raise InsufficientPrivilege + (count,) = await cur.fetchone() + assert count == 0 + + @requires_podman async def test_apply_schema_seeds_composite_enum_and_jsonb_columns(schema_test_db): # gadget.mood is an enum, gadget.size a composite type, gadget.tags diff --git a/uv.lock b/uv.lock index 38b213e..dd4e331 100644 --- a/uv.lock +++ b/uv.lock @@ -313,7 +313,7 @@ wheels = [ [[package]] name = "pgdevkit" -version = "0.6.0" +version = "0.6.1" source = { editable = "." } dependencies = [ { name = "docker" }, From a9fb2cb3ea569a0a77e36a6699c8305b842bbc6b Mon Sep 17 00:00:00 2001 From: Adrian Ehrsam Date: Tue, 8 Sep 2026 14:40:24 +0200 Subject: [PATCH 2/2] Fix permissions ordering in _iter_sql_files() itself, not a caller-side replay Code review of the previous commit (collect permissions-type files during the main apply loop, then re-execute them a second time after everything converges) found the "second pass" approach had three real problems: 1. The replay in both apply_schema() (schema.py) and MSSQL's _apply() called _apply() directly with no try/except, unlike the first pass's protected failures/retry loop -- a transient error on replay (lock timeout, dropped connection) would now abort the whole apply even though every object and the first-pass GRANT already succeeded, a regression versus applying permissions files exactly once. 2. The fix was duplicated near-verbatim at two call sites instead of living once in _iter_sql_files(), which already owns the delayed/delivered convergence loop -- any future caller (a new engine backend, another entry point) wouldn't know to replicate the pattern and would silently reintroduce the exact bug being fixed. 3. Every apply_schema()/ensure_testdb() call now paid a wholly redundant extra pass of GRANT round-trips even in the common case where nothing was ever actually delayed. Fixed properly instead: _iter_sql_files() now holds every permissions-type file back unconditionally during its main loop (regardless of the file's own -- always empty -- dependency set) and yields them only after its existing `while delayed:` retry loop has fully converged. Nothing ever legitimately depends on a permissions file (it creates no table/view/ schema of its own), so holding every one back is always safe. Both apply_schema() and MSSQL's _apply() need zero changes at all now -- they just consume _iter_sql_files() in order, with their existing (and, for MSSQL, existing-and-unchanged) per-file error handling automatically covering permissions files the same as everything else. Applied exactly once, in the correct position, for every caller, with no code duplication and no extra cost on the common path. Also fixes a real race the review caught in the new test fixture: permissions/grants.sql's `CREATE ROLE pgdevkit_test_reader` used an `IF NOT EXISTS (SELECT ...) THEN CREATE` check-then-act pattern -- roles are cluster-wide, not per-database, so two pytest processes (e.g. two worktrees, this repo's own normal workflow) hitting the same shared testdb container concurrently could both pass the check before either commits, and the second CREATE ROLE would raise duplicate_object. Now uses `CREATE ROLE ... EXCEPTION WHEN duplicate_object THEN NULL` instead, mirroring the same atomic-create-and-catch pattern pgdevkit/testdb/api.py's _ensure_database() already uses for the equivalent CREATE DATABASE race (`except psycopg.errors.DuplicateDatabase: pass`). Renamed the regression test (test_apply_schema_reapplies_permissions_for_a_delayed_table -> test_apply_schema_applies_permissions_after_a_delayed_table) and its comments to describe the new mechanism. Re-verified against the true pre-fix baseline (git show HEAD~1:pgdevkit/testdb/schema.py): fails with psycopg.errors.InsufficientPrivilege without either fix, passes with this one. Full testdb suite (78 tests, @requires_podman temporarily bypassed against this sandbox's already-reachable local Postgres) passes; the one pre-existing failure (test_container.py, needs real Docker/Podman) is unrelated and expected. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Aqh5NvxzHH2ysU4ziSuwov --- docs/database-layout.md | 21 ++++---- pgdevkit/testdb/mssql/api.py | 26 ++-------- pgdevkit/testdb/schema.py | 48 +++++++++---------- .../fixtures/database/app/tables/event.sql | 5 +- .../fixtures/database/permissions/grants.sql | 14 ++++-- tests/testdb/test_schema.py | 18 +++---- 6 files changed, 59 insertions(+), 73 deletions(-) diff --git a/docs/database-layout.md b/docs/database-layout.md index 04b624b..8ae7ad2 100644 --- a/docs/database-layout.md +++ b/docs/database-layout.md @@ -63,17 +63,16 @@ both together. One object per file: `tables/user.sql`, `views/all_edits.sql`, `types/measurement_unit.sql`. -**`permissions` files apply twice.** A blanket `GRANT ... ON ALL TABLES IN -SCHEMA x TO role;` only covers what exists at the moment it runs — but a -table with a cross-file dependency that isn't resolved on the first pass -(e.g. an FK to a table in another layer directory whose filename happens to -sort later) can end up created *after* every `permissions` file already ran, -via `pgdb testdb`'s delayed-retry resolution. So every `permissions` file is -re-applied a second time, once the whole tree (delayed retries included) has -fully converged — closing that gap. GRANT is idempotent, so the second pass -is a no-op for anything the first pass already covered; write `permissions` -files as if they always ran last, since — after this second pass — they -effectively do. +**`permissions` files always apply last, genuinely.** A blanket `GRANT ... +ON ALL TABLES IN SCHEMA x TO role;` only covers what exists at the moment it +runs — so if a table with a cross-file dependency that isn't resolved on the +first pass (e.g. an FK to a table in another layer directory whose filename +happens to sort later) got created *after* a `permissions` file, that grant +would silently never cover it. `pgdb testdb` holds every `permissions` file +back unconditionally and applies it only once the whole rest of the tree +(delayed-retry resolution included) has actually finished — so write +`permissions` files as if every table/view they reference is guaranteed to +already exist, because it is. --- diff --git a/pgdevkit/testdb/mssql/api.py b/pgdevkit/testdb/mssql/api.py index 9a9f40c..fbc0a96 100644 --- a/pgdevkit/testdb/mssql/api.py +++ b/pgdevkit/testdb/mssql/api.py @@ -12,7 +12,7 @@ from ...envtag import strip_env_suffix from .. import query from ..config import ProjectConfig -from ..schema import _TYPE_ORDER, _get_type_order, _iter_sql_files, _strip_layer_prefix +from ..schema import _iter_sql_files, _strip_layer_prefix from . import constants from .container import ensure_mssql_container @@ -121,8 +121,10 @@ def _connect() -> Any: database_dir = config.root / config.database_dir if not database_dir.is_dir(): return - - async def _apply_file(file: Path, sql: str) -> None: + for file, sql in _iter_sql_files( + database_dir, MSSQL, + env=env, areas=areas, exclude_areas=exclude_areas, schemas=schemas, exclude_schemas=exclude_schemas, + ): for batch in query.split_tsql_batches(sql): def _exec(batch: str = batch) -> None: @@ -136,24 +138,6 @@ def _exec(batch: str = batch) -> None: await _insert_test_data( json_file, f"{schema_name}.{_strip_layer_prefix(table_stem)}", force_reset, conn ) - - permissions_files: list[tuple[Path, str]] = [] - for file, sql in _iter_sql_files( - database_dir, MSSQL, - env=env, areas=areas, exclude_areas=exclude_areas, schemas=schemas, exclude_schemas=exclude_schemas, - ): - if _get_type_order(file) == _TYPE_ORDER["permissions"]: - permissions_files.append((file, sql)) - await _apply_file(file, sql) - - # Same fix as the Postgres path (see apply_schema()'s docstring/comment in - # ..schema): a permissions-type file has no SQL dependencies of its own, so - # it always applies at its sorted position in the pass above -- even before - # a table whose own creation got deferred by a same-type file elsewhere. Re- - # running permissions files now, after everything genuinely exists, closes - # that gap; GRANT is idempotent. - for file, sql in permissions_files: - await _apply_file(file, sql) finally: await asyncio.to_thread(conn.close) diff --git a/pgdevkit/testdb/schema.py b/pgdevkit/testdb/schema.py index 30b2ff5..9cf70b6 100644 --- a/pgdevkit/testdb/schema.py +++ b/pgdevkit/testdb/schema.py @@ -132,6 +132,19 @@ def _iter_sql_files( ): """Yield (Path, sql_content) pairs in dependency-safe execution order. + Every `permissions`-type file (see `_TYPE_ORDER`) is held back + unconditionally and yielded only after every other file -- including + anything resolved via the delayed-retry loop below -- has actually been + delivered. A blanket "grant ... on all tables in schema X" has no real + per-object dependency of its own to track, but it still must apply + *after* every table/view it's meant to cover; a table whose own creation + got deferred to the retry loop (e.g. an FK to a same-type file that + happens to sort later) would otherwise silently miss that grant, since + a schema-wide GRANT is a one-time snapshot of whatever exists when it + runs. Nothing ever legitimately depends on a permissions file (it + creates no table/view/schema of its own), so holding every one back is + always safe. + A file tagged for another environment (see pgdevkit.envtag) is dropped during the initial file walk, before dependency ordering even sees it. @@ -154,6 +167,7 @@ def _iter_sql_files( delivered: set[str] = set() delayed: list[tuple[str | None, Path, str]] = [] all_declared: set[str] = set() + permissions_files: list[tuple[Path, str]] = [] for file in sorted(files, key=lambda p: (_get_type_order(p), p.name)): content = file.read_text(encoding="utf-8") @@ -163,6 +177,9 @@ def _iter_sql_files( sql_schemas(content, dialect), only=schemas, exclude=exclude_schemas ): continue + if _get_type_order(file) == _TYPE_ORDER["permissions"]: + permissions_files.append((file, content)) + continue deps = _get_sql_deps(content, dialect) if file.parent.name in _SCHEMA_QUALIFIED_TYPES: schema = _strip_layer_prefix(file.parent.parent.name) @@ -196,6 +213,8 @@ def _iter_sql_files( if not progressed: raise ValueError(f"Circular or missing SQL dependencies: {[f[1] for f in delayed]}") + yield from permissions_files + async def _insert_test_data( json_file: Path, @@ -276,13 +295,9 @@ async def apply_schema( """Apply every .sql file under database_dir (in dependency-safe order) and seed any matching .test_data.json files. Safe to call repeatedly. - Every `permissions`-type file (see `_TYPE_ORDER`) is applied a second time - after the whole tree has been applied (delayed-retry included) — a blanket - "grant ... on all tables in schema X" only covers what exists at the moment - it runs, and a table whose own creation got deferred to the delayed-retry - path (an FK to a same-type file that happens to sort later) would otherwise - silently miss it. GRANT is idempotent, so the second pass is a no-op for - anything that already had the grant. + Every `permissions`-type file (see `_TYPE_ORDER`) is guaranteed to apply + after every table/view it's meant to cover, including anything resolved + via delayed-retry -- see `_iter_sql_files()`'s docstring. A file tagged for another environment (e.g. `grants.prod.sql` when `env="local_test"`) is skipped — see pgdevkit.envtag. @@ -313,7 +328,6 @@ async def _apply(file: Path, sql: str) -> None: ) failures: list[tuple[Path, str]] = [] - permissions_files: list[tuple[Path, str]] = [] for file, sql in _iter_sql_files( database_dir, dialect, @@ -323,8 +337,6 @@ async def _apply(file: Path, sql: str) -> None: schemas=schemas, exclude_schemas=exclude_schemas, ): - if _get_type_order(file) == _TYPE_ORDER["permissions"]: - permissions_files.append((file, sql)) try: await _apply(file, sql) except Exception as e: # noqa: BLE001 @@ -349,19 +361,3 @@ async def _apply(file: Path, sql: str) -> None: assert last_error is not None raise last_error failures = still_failing - - # A "permissions"-type file (e.g. `grant select, insert, update, delete on all - # tables in schema editing to app_role;`) has no SQL dependencies of its own — - # GRANT statements aren't parsed as depending on the tables they name — so it - # always applies in the very first pass above, at its sorted position. But a - # *table* with an unresolved dependency at that point (e.g. an FK to a table - # in a different layer directory whose filename happens to sort later) gets - # deferred to the delayed-retry loop inside _iter_sql_files() and only - # actually exists *after* the permissions file already ran — so a blanket - # "grant ... on all tables in schema X", being a one-time snapshot, silently - # never covers it. Re-running every permissions file now, after the full - # apply above (delayed retries included) has converged and every object - # genuinely exists, closes that gap. GRANT is idempotent, so this is a no-op - # for anything that already had the grant from the first pass. - for file, sql in permissions_files: - await _apply(file, sql) diff --git a/tests/testdb/fixtures/database/app/tables/event.sql b/tests/testdb/fixtures/database/app/tables/event.sql index cec9e17..7f7c51f 100644 --- a/tests/testdb/fixtures/database/app/tables/event.sql +++ b/tests/testdb/fixtures/database/app/tables/event.sql @@ -2,9 +2,8 @@ -- target's filename ("event_kind.sql") -- '.' (0x2E) sorts before '_' (0x5F) -- -- so this file's first apply attempt always fails (app.event_kind doesn't -- exist yet) and it only actually gets created via _iter_sql_files()'s --- delayed-retry pass, which runs after every non-delayed file (permissions --- included) has already been attempted. See --- test_apply_schema_reapplies_permissions_for_a_delayed_table. +-- delayed-retry pass, which runs after every non-delayed file has already +-- been attempted. See test_apply_schema_applies_permissions_after_a_delayed_table. CREATE TABLE IF NOT EXISTS app.event ( id serial PRIMARY KEY, event_kind_id int NOT NULL REFERENCES app.event_kind(id) diff --git a/tests/testdb/fixtures/database/permissions/grants.sql b/tests/testdb/fixtures/database/permissions/grants.sql index 0666122..40b8c0c 100644 --- a/tests/testdb/fixtures/database/permissions/grants.sql +++ b/tests/testdb/fixtures/database/permissions/grants.sql @@ -1,8 +1,16 @@ +-- Roles are cluster-wide, not per-database, so this can race against another +-- pytest process (e.g. a second worktree) hitting the same shared testdb +-- container concurrently -- both can pass the IF NOT EXISTS check before +-- either commits. Catching duplicate_object (mirroring _ensure_database()'s +-- own DuplicateDatabase guard in pgdevkit/testdb/api.py) is race-safe in a +-- way "IF NOT EXISTS (SELECT ...) THEN CREATE" is not, since it lets +-- Postgres's own uniqueness check -- not a separate lookup -- be the source +-- of truth. DO $$ BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'pgdevkit_test_reader') THEN - CREATE ROLE pgdevkit_test_reader LOGIN PASSWORD 'testpwd'; - END IF; + CREATE ROLE pgdevkit_test_reader LOGIN PASSWORD 'testpwd'; +EXCEPTION WHEN duplicate_object THEN + NULL; END $$; diff --git a/tests/testdb/test_schema.py b/tests/testdb/test_schema.py index a146463..5efbfd0 100644 --- a/tests/testdb/test_schema.py +++ b/tests/testdb/test_schema.py @@ -99,16 +99,16 @@ async def test_apply_schema_resolves_view_to_view_dependency(schema_test_db): @requires_podman -async def test_apply_schema_reapplies_permissions_for_a_delayed_table(schema_test_db): +async def test_apply_schema_applies_permissions_after_a_delayed_table(schema_test_db): # permissions/grants.sql (a blanket "grant select on all tables in schema app") - # has no SQL dependencies of its own, so it always applies in the first pass, at - # its sorted position -- before app.event, whose own filename ("event.sql") - # sorts alphabetically before its FK target's filename ("event_kind.sql") and - # so always falls to the delayed-retry pass, landing (chronologically) after - # permissions/grants.sql already ran. A blanket GRANT is a one-time snapshot: on - # its own it would never cover app.event. Confirms apply_schema() closes this by - # re-running every permissions-type file again after the whole tree (delayed - # retries included) has converged. + # has no SQL dependencies of its own -- app.event, whose own filename + # ("event.sql") sorts alphabetically before its FK target's filename + # ("event_kind.sql"), always falls to the delayed-retry pass instead of + # resolving on the first attempt. Without holding permissions files back + # unconditionally, a blanket GRANT (a one-time snapshot of whatever exists + # when it runs) would apply before app.event exists and never cover it. + # Confirms _iter_sql_files() always yields permissions-type files last, + # genuinely after the whole tree (delayed retries included) has converged. async with await psycopg.AsyncConnection.connect(_db_dsn(), autocommit=True) as con: await apply_schema(con, FIXTURES) # must not raise