Fix permissions ordering in _iter_sql_files() itself, not a caller-side replay - #32
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aqh5NvxzHH2ysU4ziSuwov
…de 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aqh5NvxzHH2ysU4ziSuwov
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.