diff --git a/docs/database-layout.md b/docs/database-layout.md index 95302d6..8ae7ad2 100644 --- a/docs/database-layout.md +++ b/docs/database-layout.md @@ -63,6 +63,17 @@ both together. One object per file: `tables/user.sql`, `views/all_edits.sql`, `types/measurement_unit.sql`. +**`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. + --- ## File-naming conventions diff --git a/pgdevkit/testdb/schema.py b/pgdevkit/testdb/schema.py index d81c82b..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,6 +295,10 @@ 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 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. 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..7f7c51f --- /dev/null +++ b/tests/testdb/fixtures/database/app/tables/event.sql @@ -0,0 +1,10 @@ +-- 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 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/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..40b8c0c --- /dev/null +++ b/tests/testdb/fixtures/database/permissions/grants.sql @@ -0,0 +1,18 @@ +-- 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 + CREATE ROLE pgdevkit_test_reader LOGIN PASSWORD 'testpwd'; +EXCEPTION WHEN duplicate_object THEN + NULL; +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..5efbfd0 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_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 -- 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 + + # 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" },