From ee498e1ca798947159a821e10f4de2950ce42cb7 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Fri, 25 Sep 2026 20:23:02 -0400 Subject: [PATCH 1/4] feat: apply desired schemas through migrate Signed-off-by: Armand Parajon --- README.md | 4 +- demo/desired-rls.sql | 6 + demo/seed.sql | 8 + demo/tour.sh | 30 ++++ docs/atomic-row-security.md | 4 +- docs/capabilities.md | 4 +- docs/declarative-row-security.md | 60 ++++++- docs/limitations.md | 2 +- docs/supabase.md | 32 +++- internal/cli/cli.go | 10 +- internal/cli/migrate.go | 6 + internal/cli/migrate_desired.go | 103 +++++++++++ .../cli/migrate_desired_integration_test.go | 167 ++++++++++++++++++ internal/cli/migrate_desired_test.go | 84 +++++++++ internal/cli/migrate_row_security.go | 58 ++++++ .../migrate_row_security_integration_test.go | 79 +++++++++ pkg/capabilities/capabilities.yaml | 10 +- 17 files changed, 647 insertions(+), 20 deletions(-) create mode 100644 demo/desired-rls.sql create mode 100644 internal/cli/migrate_desired.go create mode 100644 internal/cli/migrate_desired_integration_test.go create mode 100644 internal/cli/migrate_desired_test.go create mode 100644 internal/cli/migrate_row_security.go create mode 100644 internal/cli/migrate_row_security_integration_test.go diff --git a/README.md b/README.md index 4853ae2..4737bdb 100644 --- a/README.md +++ b/README.md @@ -67,8 +67,8 @@ refusal — never a silently wrong or incomplete result: - **Copy-and-swap** (genuine table rewrites) is not yet available — those changes refuse rather than fall through to a blocking rewrite. - **Row security** is included in exports when present, with reviewable before/after differences. The - [atomic Go executor](docs/atomic-row-security.md) can apply RLS-only changes to - existing tables; CLI execution and mixed table/policy changes remain unsupported. See the workflow and roadmap in + [atomic executor](docs/atomic-row-security.md), also available through `migrate --desired`, applies RLS-only changes to + existing tables; mixed table/policy changes remain unsupported. See the workflow and roadmap in [declarative-row-security.md](docs/declarative-row-security.md). - **Foreign keys** are out of the declarative model in either direction: desired files cannot declare them, and export refuses both a table that diff --git a/demo/desired-rls.sql b/demo/desired-rls.sql new file mode 100644 index 0000000..cc43bcf --- /dev/null +++ b/demo/desired-rls.sql @@ -0,0 +1,6 @@ +CREATE TABLE demo_rls ( + id bigint PRIMARY KEY, + owner_id bigint NOT NULL +); +ALTER TABLE demo_rls ENABLE ROW LEVEL SECURITY; +CREATE POLICY readers ON demo_rls FOR SELECT USING (owner_id = 7); diff --git a/demo/seed.sql b/demo/seed.sql index f7b47e9..1340fff 100644 --- a/demo/seed.sql +++ b/demo/seed.sql @@ -32,3 +32,11 @@ FROM generate_series(1, 5000) g; -- the --accept-blocking acknowledgement. It lives on orders, which no -- desired-state file describes, so the diff tour's plans do not see it. CREATE INDEX orders_total_idx ON orders (total); + +-- Dedicated fixture for atomic desired-state row security. +CREATE SCHEMA IF NOT EXISTS demo_security; +DROP TABLE IF EXISTS demo_security.demo_rls; +CREATE TABLE demo_security.demo_rls ( + id bigint PRIMARY KEY, + owner_id bigint NOT NULL +); diff --git a/demo/tour.sh b/demo/tour.sh index 4b40268..e9ae0a3 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -362,8 +362,38 @@ run_offline() { fi } +run_rls_apply() { + step "Apply a complete RLS definition" + local out status=0 + out=$("$PGS" migrate --url "$PG_DSN" --schema demo_security --desired desired-rls.sql --dry-run --json) || status=$? + if [ "$CHECK" = 1 ]; then + assert_eq "RLS preview exit" 2 "$status" + assert_eq "RLS preview changes" true "$(jq '.row_security_review.changes | length > 0' <<<"$out")" + else + printf '%s\n' "$out" + fi + status=0 + out=$("$PGS" migrate --url "$PG_DSN" --schema demo_security --desired desired-rls.sql --json) || status=$? + if [ "$CHECK" = 1 ]; then + assert_eq "RLS apply exit" 0 "$status" + assert_eq "RLS apply outcome" executed-natively "$(jq -r '.outcome' <<<"$out")" + assert_eq "RLS committed statements" 3 "$(jq '.executed_sql | length' <<<"$out")" + else + printf '%s\n' "$out" + fi + status=0 + out=$("$PGS" migrate --url "$PG_DSN" --schema demo_security --desired desired-rls.sql --json) || status=$? + if [ "$CHECK" = 1 ]; then + assert_eq "RLS repeat exit" 0 "$status" + assert_eq "RLS repeat statements" 0 "$(jq '.executed_sql // [] | length' <<<"$out")" + else + printf '%s\n' "$out" + fi +} + run_exec() { heading "Real executions against the seeded tables (make demo reseeds each run)" + run_rls_apply # steps fragment execute_native 0 "" "ALTER TABLE users ADD COLUMN bio text" execute_native 1 CONCURRENTLY "CREATE INDEX idx_users_email ON users (email)" diff --git a/docs/atomic-row-security.md b/docs/atomic-row-security.md index 3901f2e..b748a94 100644 --- a/docs/atomic-row-security.md +++ b/docs/atomic-row-security.md @@ -1,8 +1,8 @@ # Atomic row security changes -The dedicated Go executor converges the complete RLS definition of one existing +The dedicated executor, available through the Go API and `migrate --desired`, converges the complete RLS definition of one existing ordinary table. It does not change columns, indexes, or constraints and does not -create missing tables. `diff` remains a preview; no new CLI flags are required. +create missing tables. `diff` remains a preview. See the [CLI workflow](declarative-row-security.md#apply-the-declaration) for commands and output. ## Call the Go API diff --git a/docs/capabilities.md b/docs/capabilities.md index 2fed70f..6671070 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -272,8 +272,8 @@ review the object warrants) · | PL/pgSQL function bodies (`CREATE OR REPLACE FUNCTION`) | ⚪ | — | No — owner tooling | Transactional catalog work that takes no lock on any relation; nothing for an online engine to add. No peer online executor owns it either | | Triggers (`CREATE TRIGGER`) | ⚪ | — | No — owner tooling | Catalog work — no scan, no rewrite — but it takes a brief `SHARE ROW EXCLUSIVE` on the table, queues behind long-running queries, and blocks writers while it waits — run it under a `lock_timeout` | | Extensions (`CREATE EXTENSION`) | ⚪ | — | No — owner tooling | Same: catalog bootstrap, owner tooling | -| Grants, roles, and CLI policy DDL | 🔵 | — | No — provisioning / IaC | Access control changes remain with provisioning. Export includes RLS when present; explicit RLS declarations support comparison and review-only deltas with advisory access warnings. Plain table files leave access control separately managed. The separate [atomic RLS Go executor](atomic-row-security.md) supports RLS-only changes on existing supported tables; it does not route through these CLI front doors. See the [workflow and roadmap](declarative-row-security.md). See [engine-role.md](engine-role.md) for the engine's own role | -| Complete table-local RLS definition (Go API only) | ✅ | native, safer sequence | Yes | The [atomic RLS executor](atomic-row-security.md) locks an existing supported table, refuses structural changes, replaces policies/settings, and verifies convergence before committing. Lock waits and the whole transaction are bounded. No CLI execution or new diff flags | +| Grants, roles, and imperative policy DDL | 🔵 | — | No — provisioning / IaC | Access control changes remain with provisioning. Export includes RLS when present; explicit RLS declarations support comparison and review-only deltas with advisory access warnings. Plain table files leave access control separately managed. The separate [atomic RLS Go executor](atomic-row-security.md) supports RLS-only changes on existing supported tables; `migrate --desired` uses this executor; imperative policy statements remain refused. See the [workflow and roadmap](declarative-row-security.md). See [engine-role.md](engine-role.md) for the engine's own role | +| Complete table-local RLS definition (Go API and migrate --desired) | ✅ | native, safer sequence | Yes | The [atomic RLS executor](atomic-row-security.md) locks an existing supported table, refuses structural changes, replaces policies/settings, and verifies convergence before committing. Lock waits and the whole transaction are bounded. Available through `migrate --desired` without RLS-specific flags; `diff` remains a review-only surface | | Standalone sequences | ⚪ | — | No — owner tooling | Transactional catalog work on an object with no readers-and-writers problem | | Publications, subscriptions | 🔵 | — | No — replication provisioning / IaC | Replication provisioning, not table shape (`ALTER PUBLICATION ... ADD TABLE` also takes `SHARE UPDATE EXCLUSIVE` on the table) | diff --git a/docs/declarative-row-security.md b/docs/declarative-row-security.md index 9dc68d0..5e2f1e4 100644 --- a/docs/declarative-row-security.md +++ b/docs/declarative-row-security.md @@ -4,8 +4,8 @@ You can export a table's RLS settings and policies, keep them alongside its SQL, and verify that the live definition still matches. `pull` includes RLS when the live table has settings or policies; ordinary tables get no extra SQL. `diff` shows the captured definitions and refuses to emit execution SQL for RLS -changes. The dedicated [atomic Go executor](atomic-row-security.md) can apply an -RLS-only declaration to an existing supported table. No new CLI flags are needed. +changes. Use `migrate --desired` to apply an RLS-only declaration to an existing supported table +through the [atomic executor](atomic-row-security.md). No RLS-specific flags are needed. ## Export and compare @@ -46,7 +46,57 @@ optional and defaults to `NO FORCE`. Files without RLS declarations keep their table-only behavior: `diff` leaves access control separately managed. Export preserves policies even when RLS is disabled, and preserves enabled RLS even when there are no policies (default deny). -`fmt`, `lint`, and live desired-state execution do not accept the expanded format yet. +`fmt` and `lint` do not accept the expanded format yet. + +## Apply the declaration + +After reviewing the differences, apply the same file: + +```sh +pg-sprite migrate --url "$PG_DSN" --schema public --desired schema/documents.sql +``` + +The command prints an executed verdict and the SQL that committed. A second apply +prints an already-converged verdict and runs no policy DDL. For scripts, add `--json`: + +```sh +pg-sprite migrate --url "$PG_DSN" --schema public --desired schema/documents.sql --json +``` + +An already-converged response is: + +```json +{ + "outcome": "executed-natively", + "statement": "", + "table": "public.documents", + "detail": "already converged: row security matches; nothing to run" +} +``` + +On a change, `executed_sql` contains the ordered statements committed together. +The input owns the complete policy set: removing a policy from the file removes it +from the table. Disabling RLS or widening a policy changes access deliberately; +review that meaning and test application authorization before applying. + +`--dry-run` uses the same review as `diff`: RLS differences exit 2 and show the +review without executing. That exit code describes the review-only preview, +not a claim that the atomic executor cannot apply an RLS-only difference. +Apply independently reads and validates the current table under its lock; it does +not execute a saved preview or pin the reviewed definition with a fingerprint. + +Apply exits 0 after commit or a no-op, 2 for an unsupported declaration/target, +and 1 for an operational failure. JSON includes the executor's `code` on failure. +`row-security-outcome-unknown` means the commit response was lost or failed: +inspect the live state before retrying; do not assume rollback. The whole RLS +attempt uses `--statement-timeout`, including scratch inspection and lock waits; +`--lock-timeout` bounds lock acquisition. RLS applies do not automatically retry. + +Ordinary files use the existing desired-state sequence instead; their JSON is +`migrate.DesiredResult` with `plan`, `verdicts`, and overall `outcome`. Earlier +committed steps stay committed if a later step fails. Mixed table/RLS changes +are refused as a whole. `--desired` cannot be combined with `--alter`, `--force`, +or `--accept-blocking`. ## Keep the SQL people already use @@ -77,7 +127,7 @@ CREATE POLICY "Create your documents" WITH CHECK ((SELECT auth.uid()) = owner_id); ``` -This file is accepted by `diff` for inspection. The role, helper, +This file is accepted by `diff` for inspection and by `migrate --desired` for execution. The role, helper, and necessary table grants must already exist. These policies cover reads and inserts, not updates or deletes. Application authorization tests remain necessary. @@ -222,7 +272,7 @@ this table-scoped work. creation remains refused. 3. **Execute transitions atomically.** The Go executor now locks, derives, applies, and verifies one table's RLS state in one bounded transaction. Mixed changes and - policy relation dependencies remain unsupported. CLI integration is a follow-up. + policy relation dependencies remain unsupported. The CLI selects this executor for explicit RLS declarations through `migrate --desired`. 4. **Prove application behavior.** The local Supabase harness now checks real PostgREST requests from two authenticated users and anonymous callers, allowed and denied writes, changed visibility, and rollback after a cancelled apply. diff --git a/docs/limitations.md b/docs/limitations.md index ec8e4a7..aa94f26 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -25,7 +25,7 @@ with a typed refusal — never a silently wrong or incomplete result: | Not modeled | Current behavior | | --- | --- | -| Row-level security (RLS) settings and policies | `pull` exports settings, policies, roles, and policy comments; `diff` verifies unchanged definitions and shows review-only RLS deltas before refusing changes. The [atomic Go executor](atomic-row-security.md) supports RLS-only changes to existing supported tables. Policy relation subqueries, mixed table changes, greenfield creation, and CLI policy execution remain refused. Tables without RLS get no extra SQL; table-only desired files leave access control separately managed. See the [RLS workflow and roadmap](declarative-row-security.md). | +| Row-level security (RLS) settings and policies | `pull` exports settings, policies, roles, and policy comments; `diff` verifies unchanged definitions and shows review-only RLS deltas before refusing changes. The [atomic executor](atomic-row-security.md), through the Go API or `migrate --desired`, supports RLS-only changes to existing supported tables. Policy relation subqueries, mixed table changes, and greenfield RLS creation remain refused. Tables without RLS get no extra SQL; table-only desired files leave access control separately managed. See the [RLS workflow and roadmap](declarative-row-security.md). | | Foreign keys (either direction) | Unsupported in the declarative model, in both directions. A desired file cannot declare a `REFERENCES` clause (refused at parse), and export refuses both sides of a foreign-key relationship: a table whose definition carries foreign-key constraints surfaces the parse gate's typed error, and a table that other tables reference refuses with its own typed error — a single-table baseline cannot carry incoming foreign-key topology, so rendering one would silently drop the relationship. Foreign-key **DDL is still supported through the statement front door** (`ADD FOREIGN KEY` routes to the online `NOT VALID` + `VALIDATE` sequence). Because a desired file can never declare a foreign key, `diff` on a live table that carries one plans a **destructive** `DROP CONSTRAINT` for it — gated like every destructive change, never auto-executed — and incoming foreign keys are invisible to a single-table diff entirely, so tables participating in foreign-key relationships in either direction should not be managed declaratively yet. | | Partitioned tables | Partitioned parents and their partitions are introspectable, and the statement front door supports in-place changes on them (see the partitioned-parent rows above), but they cannot be expressed in or exported to a desired file: the model captures the partition key and attachment only to refuse — it does not carry partition bounds or the parent/partition topology. A partitioning mismatch between live and desired is a typed `diff` refusal, never a zero diff. | | Classic table inheritance (`INHERITS`) | Both parents and children are refused during export. The model records classic inheritance edges in both directions only to fail closed: rendering a child would flatten inherited columns and rendering a parent would omit its children. Declarative partitions share `pg_inherits` but remain classified by `relispartition` and use the partition refusal above. | diff --git a/docs/supabase.md b/docs/supabase.md index 1ba07aa..5090c9d 100644 --- a/docs/supabase.md +++ b/docs/supabase.md @@ -163,6 +163,34 @@ exit codes are in [CLI output examples](cli-output-examples.md). A refusal is a stopping point to inspect, not a reason to retry with `--force`. Keep RLS policies, grants, and Supabase-managed schemas outside this workflow. +## Apply your RLS definition + +Use a direct or session-pooled connection with table-owner privileges and database +CREATE permission for scratch inspection. Keep Supabase-managed schemas outside +the target. In the [declarative RLS example](declarative-row-security.md#keep-the-sql-people-already-use), +`authenticated` and `auth.uid()` come from Supabase; your application grants remain +separately managed. + +Save the complete definition as `schema/documents.sql`, matching the existing +table's columns, indexes, and constraints. Review before applying: + +```sh +pg-sprite diff --url "$PG_DSN" --schema public --desired schema/documents.sql +``` + +The preview lists policy/settings differences and access warnings, then exits 2 +because RLS diff output is review-only. Apply the reviewed declaration: + +```sh +pg-sprite migrate --url "$PG_DSN" --schema public --desired schema/documents.sql +``` + +The result lists the statements committed in one transaction. Applying it again +reports that row security already matches. A mixed column/index and RLS edit +refuses without committing either part. See [output and failure handling](declarative-row-security.md#apply-the-declaration). +This CLI route is covered by PostgreSQL integration tests; local Supabase API +coverage uses the same executor. Hosted validation remains a separate step. + ## What works today A disposable local `supabase/postgres:17.6.1.136` database (PostgreSQL 17.6) @@ -188,7 +216,7 @@ hosted-project or complete Supabase stack test. | API and Realtime after each copy-and-swap refusal | Tenant reads and new INSERT events still worked on the original sockets | [Service continuity](../integration/supabase/continuity_test.go) | | Apply a complete RLS declaration through the Go API | PostgREST enforces tenant reads and writes, anonymous denial, changed visibility, and default deny; repeated apply is a no-op | [RLS application behavior](../integration/supabase/row_security_api_test.go) | | Cancel an RLS apply after a live policy is dropped | Transaction rollback restores the catalog and previous API access, including allowed and denied writes | [RLS rollback](../integration/supabase/row_security_api_rollback_test.go) | -| Enable RLS through the statement/CLI entry point | Refused with `unsupported-statement` | [Refusals](../integration/supabase/schema_test.go) | +| Enable RLS through `migrate --alter` | Refused with `unsupported-statement` | [Refusals](../integration/supabase/schema_test.go) | | Supavisor session endpoint | Column addition and concurrent index succeeded; session timeouts verified | [Execution](../integration/supabase/services_test.go), [timeouts](../pkg/dbconn/supabase_integration_test.go) | | Supavisor transaction endpoint | Refused with `ErrNoSessionAffinity`, even with named prepared statements disabled | [Pooler boundary](../pkg/dbconn/supabase_integration_test.go) | | PostgREST after direct/session schema changes | New columns became available through automatic schema-cache reload | [API and tenants](../integration/supabase/services_test.go) | @@ -213,7 +241,7 @@ outcome; they do not assume every unsuccessful change rolls back completely. ## What to keep in mind - RLS declarations can be exported, compared, and applied through the - [atomic Go API](atomic-row-security.md). CLI execution remains unsupported. + [atomic Go API](atomic-row-security.md). Use `migrate --desired` for the same atomic execution from the CLI. Table-only files do not compare access rules. Grants and roles remain separately managed; see [declarative RLS](declarative-row-security.md) - Qualify extension types and functions outside `public`, such as diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 9858301..8382c12 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -104,7 +104,9 @@ type MigrateCmd struct { DBFlags `embed:""` OutputFlags `embed:""` - Alter string `help:"Imperative ALTER statement to run." name:"alter" required:""` + Alter string `help:"Imperative ALTER statement to run." name:"alter"` + Desired string `help:"Converge one table from a desired SQL file; mutually exclusive with --alter." type:"existingfile"` + Schema string `help:"Target schema for --desired." default:"public"` MaxTableSize byteSize `help:"Size threshold above which the optimistic attempt is skipped, measured as the table's full on-disk footprint: heap, indexes, and TOAST, all partitions (binary units: B, KiB, MiB, GiB, TiB). Planner-proven online steps (concurrent index builds, constraint validation) are not size-guarded." default:"1GiB"` IndexBuildTimeout time.Duration `help:"Overall bound (statement_timeout) for one concurrent index build step; expect large tables to need a generous value." default:"30m"` ValidateTimeout time.Duration `help:"Overall bound (statement_timeout) for one VALIDATE CONSTRAINT step; expect large tables to need a generous value." default:"30m"` @@ -131,6 +133,12 @@ type MigrateCmd struct { // supplied" is observable — the value alone cannot tell a typed default // from an inherited one. func (c *MigrateCmd) Validate(kctx *kong.Context) error { + if err := c.validateInput(); err != nil { + return err + } + if c.Desired == "" && flagSupplied(kctx, "schema") { + return errors.New("--schema requires --desired; qualify the table in --alter instead") + } if c.DryRun && c.Force != "" { return errors.New("--force cannot be combined with --dry-run: the dry run reports the unforced plan") } diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go index a306ec8..2bb9466 100644 --- a/internal/cli/migrate.go +++ b/internal/cli/migrate.go @@ -22,6 +22,12 @@ import ( // prefix — and still returns the operational error. --dry-run diverts to // the classify-and-route plan instead. func (c *MigrateCmd) run(ctx context.Context, out io.Writer) error { + if err := c.validateInput(); err != nil { + return err + } + if c.Desired != "" { + return c.runDesired(ctx, out) + } if c.DryRun { return c.runDryRun(ctx, out) } diff --git a/internal/cli/migrate_desired.go b/internal/cli/migrate_desired.go new file mode 100644 index 0000000..faded36 --- /dev/null +++ b/internal/cli/migrate_desired.go @@ -0,0 +1,103 @@ +package cli + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/diffplan" + "github.com/block/pg-sprite/pkg/migrate" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/verdict" +) + +func (c *MigrateCmd) validateInput() error { + if (c.Alter == "") == (c.Desired == "") { + return errors.New("provide exactly one of --alter or --desired") + } + if c.Desired != "" { + if c.Force != "" { + return errors.New("--force cannot be combined with --desired") + } + if c.AcceptBlocking != "" { + return errors.New("--accept-blocking cannot be combined with --desired") + } + } + return nil +} + +func (c *MigrateCmd) runDesired(ctx context.Context, out io.Writer) error { + raw, err := os.ReadFile(c.Desired) + if err != nil { + return fmt.Errorf("read desired schema: %w", err) + } + desired, err := statement.ParseDesired(string(raw)) + if err != nil { + carriesRLS, detectErr := statement.HasRowSecurityDeclaration(string(raw)) + if detectErr == nil && carriesRLS { + return c.runDesiredRowSecurity(ctx, out, string(raw)) + } + return err + } + pool, err := dbconn.NewPool(ctx, c.Config()) + if err != nil { + return err + } + defer pool.Close() + if c.DryRun { + report, err := diffplan.Plan(ctx, pool, diffplan.Request{Schema: c.Schema, Desired: desired}) + if err != nil { + return err + } + // Preview uses desired-state admission: a routed native step can still + // discard live structure, which RunDesired refuses before execution. + for i := range report.Statements { + st := &report.Statements[i] + if st.Destructive { + st.Disposition = router.DispositionRefuse + st.Class = verdict.ClassByDesign + st.Reason = verdict.ReasonDestructiveChange + report.Disposition = router.DispositionRefuse + } + } + diff := DiffCmd{DBFlags: c.DBFlags, OutputFlags: c.OutputFlags, JSON: c.JSON} + return diff.writeDiffReport(out, report) + } + result, runErr := migrate.RunDesired(ctx, pool, migrate.DesiredRequest{Schema: c.Schema, Desired: desired}, c.options(c.diag())) + if result.Outcome == "" { + return runErr + } + if err := c.writeDesiredResult(out, result); err != nil { + return err + } + if runErr != nil { + return runErr + } + if result.Outcome == verdict.OutcomeRefused { + return verdict.ErrRefused + } + return nil +} + +func (c *MigrateCmd) writeDesiredResult(out io.Writer, result migrate.DesiredResult) error { + if c.JSON { + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + if err := enc.Encode(result); err != nil { + return fmt.Errorf("write desired result: %w", err) + } + return nil + } + for _, v := range result.Verdicts { + if err := writeVerdictText(out, c.palette(out), v); err != nil { + return err + } + } + _, err := fmt.Fprintf(out, "%s: %s\n", result.Outcome, result.Detail) + return err +} diff --git a/internal/cli/migrate_desired_integration_test.go b/internal/cli/migrate_desired_integration_test.go new file mode 100644 index 0000000..864c437 --- /dev/null +++ b/internal/cli/migrate_desired_integration_test.go @@ -0,0 +1,167 @@ +package cli + +import ( + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/migrate" + "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/verdict" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func desiredFixture(t *testing.T) (string, string, *pgxpool.Pool) { + t.Helper() + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + t.Cleanup(pool.Close) + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf(`CREATE TABLE %s.documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + );`, schema)) + require.NoError(t, err) + return url, schema, pool +} + +func TestMigrateDesiredRLSAppliesAndConverges(t *testing.T) { + url, schema, pool := desiredFixture(t) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + ); + ALTER TABLE documents ENABLE ROW LEVEL SECURITY; + CREATE POLICY readers ON documents FOR SELECT USING (owner_id = 7);`) + var out strings.Builder + cmd.DryRun = true + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.False(t, before.RowSecurity.Enabled, "preview must not change access") + cmd.DryRun = false + out.Reset() + require.NoError(t, cmd.run(t.Context(), &out)) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + require.Len(t, v.ExecutedSQL, 3) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.True(t, after.RowSecurity.Enabled) + require.Len(t, after.RowSecurity.Policies, 1) + assert.Equal(t, "readers", after.RowSecurity.Policies[0].Name) + assert.Equal(t, "(owner_id = 7)", *after.RowSecurity.Policies[0].Using) + out.Reset() + require.NoError(t, cmd.run(t.Context(), &out)) + v = verdict.Verdict{} + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + assert.Empty(t, v.ExecutedSQL) +} + +func TestMigrateDesiredRLSRefusesMixedChanges(t *testing.T) { + url, schema, pool := desiredFixture(t) + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL, + body text + ); + ALTER TABLE documents ENABLE ROW LEVEL SECURITY; + CREATE POLICY readers ON documents FOR SELECT USING (owner_id = 7);`) + var out strings.Builder + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, string(executor.CodeRowSecurityRefused), v.Code) + assert.Empty(t, v.ExecutedSQL) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Equal(t, before, after) +} + +func TestMigrateDesiredTableAddsColumn(t *testing.T) { + url, schema, pool := desiredFixture(t) + _, err := pool.Exec(t.Context(), fmt.Sprintf(`ALTER TABLE %s.documents ENABLE ROW LEVEL SECURITY; + CREATE POLICY readers ON %s.documents FOR SELECT USING (owner_id = 7);`, schema, schema)) + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL, + body text + );`) + var out strings.Builder + cmd.DryRun = true + require.NoError(t, cmd.run(t.Context(), &out)) + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Len(t, before.Columns, 2) + cmd.DryRun = false + out.Reset() + require.NoError(t, cmd.run(t.Context(), &out)) + var result migrate.DesiredResult + require.NoError(t, json.Unmarshal([]byte(out.String()), &result)) + assert.Equal(t, verdict.OutcomeExecuted, result.Outcome) + require.Len(t, result.Verdicts, 1) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + require.Len(t, after.Columns, 3) + assert.Equal(t, "body", after.Columns[2].Name) + assert.Equal(t, before.RowSecurity, after.RowSecurity, "table-only files leave policies separately managed") +} + +func TestMigrateDesiredTableRefusesDestructiveChange(t *testing.T) { + url, schema, pool := desiredFixture(t) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY + );`) + var out strings.Builder + cmd.DryRun = true + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var preview plan.Report + require.NoError(t, json.Unmarshal([]byte(out.String()), &preview)) + assert.Equal(t, router.DispositionRefuse, preview.Disposition) + require.Len(t, preview.Statements, 1) + assert.Equal(t, verdict.ReasonDestructiveChange, preview.Statements[0].Reason) + cmd.DryRun = false + out.Reset() + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var result migrate.DesiredResult + require.NoError(t, json.Unmarshal([]byte(out.String()), &result)) + assert.Equal(t, verdict.ReasonDestructiveChange, result.Reason) + assert.Empty(t, result.Verdicts) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Len(t, after.Columns, 2) +} + +func TestMigrateDesiredCreatesMissingTable(t *testing.T) { + url, schema, pool := desiredFixture(t) + _, err := pool.Exec(t.Context(), "DROP TABLE "+schema+".documents") + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + );`) + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + var result migrate.DesiredResult + require.NoError(t, json.Unmarshal([]byte(out.String()), &result)) + assert.Equal(t, verdict.OutcomeExecuted, result.Outcome) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Len(t, after.Columns, 2) + assert.Equal(t, "id", after.Columns[0].Name) +} diff --git a/internal/cli/migrate_desired_test.go b/internal/cli/migrate_desired_test.go new file mode 100644 index 0000000..f56d301 --- /dev/null +++ b/internal/cli/migrate_desired_test.go @@ -0,0 +1,84 @@ +package cli + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/alecthomas/kong" + "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/verdict" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func desiredCommand(t *testing.T, url, schema, sql string, args ...string) *MigrateCmd { + t.Helper() + path := filepath.Join(t.TempDir(), "documents.sql") + require.NoError(t, os.WriteFile(path, []byte(sql), 0600)) + root := New("test") + parser, err := kong.New(root, kong.Vars{"version": "test"}) + require.NoError(t, err) + argv := []string{"migrate", "--url", url, "--schema", schema, "--desired", path, "--json"} + _, err = parser.Parse(append(argv, args...)) + require.NoError(t, err) + return &root.Migrate +} + +func TestMigrateDesiredRejectsConflictingInputs(t *testing.T) { + for _, args := range [][]string{ + {"--alter", "ALTER TABLE documents ADD COLUMN body text"}, + {"--force", "public.documents"}, + {"--accept-blocking", "public.documents", "--statement-timeout", "1s"}, + } { + t.Run(args[0], func(t *testing.T) { + file := filepath.Join(t.TempDir(), "documents.sql") + require.NoError(t, os.WriteFile(file, []byte("CREATE TABLE documents (id int PRIMARY KEY);"), 0600)) + root := New("test") + parser, err := kong.New(root, kong.Vars{"version": "test"}) + require.NoError(t, err) + _, err = parser.Parse(append([]string{"migrate", "--url", "postgres://localhost/test", "--desired", file}, args...)) + require.Error(t, err) + }) + } +} + +func TestMigrateDesiredRejectsInvalidDeclarationBeforeConnecting(t *testing.T) { + cmd := desiredCommand(t, "postgres://localhost:1/test", "public", `CREATE TABLE documents ( + id int PRIMARY KEY + ); + CREATE POLICY readers ON documents FOR SELECT USING (true);`) + var out strings.Builder + require.Error(t, cmd.run(t.Context(), &out)) + assert.Empty(t, out.String()) +} + +func TestRowSecurityVerdictPreservesUnknownCommit(t *testing.T) { + err := &executor.RowSecurityOutcomeUnknownError{Err: context.Canceled} + v := rowSecurityVerdict("public.documents", executor.RowSecurityReport{}, err) + assert.Equal(t, verdict.OutcomeFailed, v.Outcome) + assert.Equal(t, string(executor.CodeRowSecurityOutcomeUnknown), v.Code) + assert.Empty(t, v.ExecutedSQL) + assert.Equal(t, err.Error(), v.Detail) +} + +func TestRowSecurityVerdictPreservesCancellation(t *testing.T) { + v := rowSecurityVerdict("public.documents", executor.RowSecurityReport{}, context.Canceled) + assert.Equal(t, verdict.OutcomeFailed, v.Outcome) + assert.Equal(t, string(executor.OutcomeCode(context.Canceled)), v.Code) + assert.Empty(t, v.ExecutedSQL) +} + +func TestMigrateDesiredWriterFailure(t *testing.T) { + cmd := MigrateCmd{JSON: true} + want := errors.New("closed output") + err := cmd.emit(failingDesiredWriter{want}, rowSecurityVerdict("public.documents", executor.RowSecurityReport{}, nil)) + require.ErrorIs(t, err, want) +} + +type failingDesiredWriter struct{ err error } + +func (w failingDesiredWriter) Write([]byte) (int, error) { return 0, w.err } diff --git a/internal/cli/migrate_row_security.go b/internal/cli/migrate_row_security.go new file mode 100644 index 0000000..826bb68 --- /dev/null +++ b/internal/cli/migrate_row_security.go @@ -0,0 +1,58 @@ +package cli + +import ( + "context" + "errors" + "io" + + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/verdict" +) + +func (c *MigrateCmd) runDesiredRowSecurity(ctx context.Context, out io.Writer, sql string) error { + desired, err := statement.ParseDesiredWithRowSecurity(sql) + if err != nil { + return err + } + if c.DryRun { + diff := DiffCmd{DBFlags: c.DBFlags, OutputFlags: c.OutputFlags, Schema: c.Schema, JSON: c.JSON} + return diff.runRowSecurityDiff(ctx, out, sql) + } + pool, err := dbconn.NewPool(ctx, c.Config()) + if err != nil { + return err + } + defer pool.Close() + report, runErr := executor.ExecuteRowSecurity(ctx, pool, c.Schema, desired, executor.Budget{ + LockTimeout: c.LockTimeout, StatementTimeout: c.StatementTimeout, + }) + v := rowSecurityVerdict(c.Schema+"."+desired.Table(), report, runErr) + emitErr := c.emit(out, v) + return errors.Join(emitErr, runErr) +} + +// A failed commit is not evidence of rollback. Preserve its distinct code and +// explanation; never disclose uncommitted statements as executed SQL. +func rowSecurityVerdict(table string, report executor.RowSecurityReport, err error) verdict.Verdict { + v := verdict.Verdict{Table: table, Outcome: verdict.OutcomeExecuted, ExecutedSQL: report.Statements, + Detail: "row security converged: all changes committed in one transaction"} + if err == nil { + if len(report.Statements) == 0 { + v.Detail = "already converged: row security matches; nothing to run" + } + return v + } + v.Outcome = verdict.OutcomeFailed + v.ExecutedSQL = nil + v.Code = string(executor.OutcomeCode(err)) + v.Detail = err.Error() + if errors.Is(err, executor.ErrRowSecurityRefused) { + return v.WithRefusal(verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement)) + } + if errors.Is(err, executor.ErrTableNotFound) { + return v.WithRefusal(verdict.Environmental(verdict.ReasonUnsupportedStatement)) + } + return v +} diff --git a/internal/cli/migrate_row_security_integration_test.go b/internal/cli/migrate_row_security_integration_test.go new file mode 100644 index 0000000..56e5b33 --- /dev/null +++ b/internal/cli/migrate_row_security_integration_test.go @@ -0,0 +1,79 @@ +package cli + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/verdict" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMigrateDesiredRLSLockTimeoutPreservesState(t *testing.T) { + url, schema, pool := desiredFixture(t) + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + holder, err := pool.Begin(t.Context()) + require.NoError(t, err) + defer func() { require.NoError(t, holder.Rollback(context.WithoutCancel(t.Context()))) }() + _, err = holder.Exec(t.Context(), "LOCK TABLE "+schema+".documents IN ACCESS SHARE MODE") + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + ); + ALTER TABLE documents ENABLE ROW LEVEL SECURITY;`, "--lock-timeout", "50ms", "--statement-timeout", "5s") + var out strings.Builder + err = cmd.run(t.Context(), &out) + require.Error(t, err) + require.NotErrorIs(t, err, verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeFailed, v.Outcome) + assert.Equal(t, string(executor.CodeBudgetLockExceeded), v.Code) + assert.Empty(t, v.ExecutedSQL) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Equal(t, before, after) +} + +func TestMigrateDesiredRLSMissingTableIsRefused(t *testing.T) { + url, schema, pool := desiredFixture(t) + _, err := pool.Exec(t.Context(), "DROP TABLE "+schema+".documents") + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + ); + ALTER TABLE documents ENABLE ROW LEVEL SECURITY;`) + var out strings.Builder + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, string(executor.CodeTableNotFound), v.Code) + _, err = schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.ErrorIs(t, err, schemadiff.ErrTableNotFound) +} + +func TestMigrateDesiredRLSRemovesLastPolicy(t *testing.T) { + url, schema, pool := desiredFixture(t) + _, err := pool.Exec(t.Context(), fmt.Sprintf(`ALTER TABLE %s.documents ENABLE ROW LEVEL SECURITY; + CREATE POLICY readers ON %s.documents FOR SELECT USING (true);`, schema, schema)) + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + ); + ALTER TABLE documents ENABLE ROW LEVEL SECURITY;`) + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.True(t, after.RowSecurity.Enabled) + assert.Empty(t, after.RowSecurity.Policies, "enabled without policies is default deny") +} diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 29ce674..983e0d3 100644 --- a/pkg/capabilities/capabilities.yaml +++ b/pkg/capabilities/capabilities.yaml @@ -509,7 +509,7 @@ rows: reason_notes: "Same: catalog bootstrap, owner tooling" - id: grants-roles-row-level-security-policies area: "types_and_non_table_objects" - operation: "Grants, roles, and CLI policy DDL" + operation: "Grants, roles, and imperative policy DDL" tier: "t3" status_mark: "🔵" engine_path: "none" @@ -519,19 +519,19 @@ rows: migrate: refused diff: refused refusal_reason: unsupported-statement - reason_notes: "Access control changes remain with provisioning. Export includes RLS when present; explicit RLS declarations support comparison and review-only deltas with advisory access warnings. Plain table files leave access control separately managed. The separate [atomic RLS Go executor](atomic-row-security.md) supports RLS-only changes on existing supported tables; it does not route through these CLI front doors. See the [workflow and roadmap](declarative-row-security.md). See [engine-role.md](engine-role.md) for the engine's own role" + reason_notes: "Access control changes remain with provisioning. Export includes RLS when present; explicit RLS declarations support comparison and review-only deltas with advisory access warnings. Plain table files leave access control separately managed. The separate [atomic RLS Go executor](atomic-row-security.md) supports RLS-only changes on existing supported tables; `migrate --desired` uses this executor; imperative policy statements remain refused. See the [workflow and roadmap](declarative-row-security.md). See [engine-role.md](engine-role.md) for the engine's own role" - id: declarative-row-security-library area: "types_and_non_table_objects" - operation: "Complete table-local RLS definition (Go API only)" + operation: "Complete table-local RLS definition (Go API and migrate --desired)" tier: "t1" status_mark: "✅" engine_path: "native_safer_sequence" online_safety_problem: true front_doors: - migrate: refused + migrate: supported diff: refused refusal_reason: unsupported-statement - reason_notes: "The [atomic RLS executor](atomic-row-security.md) locks an existing supported table, refuses structural changes, replaces policies/settings, and verifies convergence before committing. Lock waits and the whole transaction are bounded. No CLI execution or new diff flags" + reason_notes: "The [atomic RLS executor](atomic-row-security.md) locks an existing supported table, refuses structural changes, replaces policies/settings, and verifies convergence before committing. Lock waits and the whole transaction are bounded. Available through `migrate --desired` without RLS-specific flags; `diff` remains a review-only surface" - id: standalone-sequences area: "types_and_non_table_objects" operation: "Standalone sequences" From f38f1d30b959952df15ce705b6923783008dbc7a Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Fri, 25 Sep 2026 20:49:29 -0400 Subject: [PATCH 2/4] fix: preserve desired CLI output contracts Signed-off-by: Armand Parajon --- docs/declarative-row-security.md | 7 ++- internal/cli/migrate_desired.go | 12 +---- .../cli/migrate_desired_integration_test.go | 9 +++- internal/cli/migrate_desired_test.go | 16 +++++++ internal/cli/migrate_row_security.go | 2 +- .../migrate_row_security_integration_test.go | 3 +- pkg/plan/desired.go | 19 ++++++++ pkg/plan/desired_test.go | 46 +++++++++++++++++++ 8 files changed, 98 insertions(+), 16 deletions(-) create mode 100644 pkg/plan/desired.go create mode 100644 pkg/plan/desired_test.go diff --git a/docs/declarative-row-security.md b/docs/declarative-row-security.md index 5e2f1e4..c75fbe8 100644 --- a/docs/declarative-row-security.md +++ b/docs/declarative-row-security.md @@ -85,8 +85,11 @@ not a claim that the atomic executor cannot apply an RLS-only difference. Apply independently reads and validates the current table under its lock; it does not execute a saved preview or pin the reviewed definition with a fingerprint. -Apply exits 0 after commit or a no-op, 2 for an unsupported declaration/target, -and 1 for an operational failure. JSON includes the executor's `code` on failure. +Apply exits 0 after commit or a no-op and 2 for executor/target refusals. +Invalid or unsupported input declarations fail admission before execution and +exit 1 with a diagnostic, without a verdict. Operational failures also exit 1; +their JSON verdict includes the executor's `code`. Refusal verdicts use `reason` +and `class`, without a failure code. `row-security-outcome-unknown` means the commit response was lost or failed: inspect the live state before retrying; do not assume rollback. The whole RLS attempt uses `--statement-timeout`, including scratch inspection and lock waits; diff --git a/internal/cli/migrate_desired.go b/internal/cli/migrate_desired.go index faded36..ccf16b6 100644 --- a/internal/cli/migrate_desired.go +++ b/internal/cli/migrate_desired.go @@ -11,7 +11,7 @@ import ( "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/diffplan" "github.com/block/pg-sprite/pkg/migrate" - "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" ) @@ -56,15 +56,7 @@ func (c *MigrateCmd) runDesired(ctx context.Context, out io.Writer) error { } // Preview uses desired-state admission: a routed native step can still // discard live structure, which RunDesired refuses before execution. - for i := range report.Statements { - st := &report.Statements[i] - if st.Destructive { - st.Disposition = router.DispositionRefuse - st.Class = verdict.ClassByDesign - st.Reason = verdict.ReasonDestructiveChange - report.Disposition = router.DispositionRefuse - } - } + plan.RefuseDestructive(&report) diff := DiffCmd{DBFlags: c.DBFlags, OutputFlags: c.OutputFlags, JSON: c.JSON} return diff.writeDiffReport(out, report) } diff --git a/internal/cli/migrate_desired_integration_test.go b/internal/cli/migrate_desired_integration_test.go index 864c437..88a2f6b 100644 --- a/internal/cli/migrate_desired_integration_test.go +++ b/internal/cli/migrate_desired_integration_test.go @@ -8,7 +8,6 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" - "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/migrate" "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/router" @@ -85,7 +84,8 @@ func TestMigrateDesiredRLSRefusesMixedChanges(t *testing.T) { var v verdict.Verdict require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) assert.Equal(t, verdict.OutcomeRefused, v.Outcome) - assert.Equal(t, string(executor.CodeRowSecurityRefused), v.Code) + assert.Equal(t, verdict.ReasonUnsupportedStatement, v.Reason) + assert.Empty(t, v.Code) assert.Empty(t, v.ExecutedSQL) after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") require.NoError(t, err) @@ -135,6 +135,11 @@ func TestMigrateDesiredTableRefusesDestructiveChange(t *testing.T) { assert.Equal(t, router.DispositionRefuse, preview.Disposition) require.Len(t, preview.Statements, 1) assert.Equal(t, verdict.ReasonDestructiveChange, preview.Statements[0].Reason) + assert.Equal(t, verdict.ReasonDestructiveChange, preview.Reason) + assert.Empty(t, preview.Statements[0].Backend) + assert.Empty(t, preview.Statements[0].ExecSQL) + assert.Empty(t, preview.Statements[0].Execution) + assert.Equal(t, plan.Fingerprint(preview.Statements), preview.Fingerprint) cmd.DryRun = false out.Reset() require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) diff --git a/internal/cli/migrate_desired_test.go b/internal/cli/migrate_desired_test.go index f56d301..ae03f96 100644 --- a/internal/cli/migrate_desired_test.go +++ b/internal/cli/migrate_desired_test.go @@ -82,3 +82,19 @@ func TestMigrateDesiredWriterFailure(t *testing.T) { type failingDesiredWriter struct{ err error } func (w failingDesiredWriter) Write([]byte) (int, error) { return 0, w.err } + +func TestMigrateRequiresOneInput(t *testing.T) { + root := New("test") + parser, err := kong.New(root, kong.Vars{"version": "test"}) + require.NoError(t, err) + _, err = parser.Parse([]string{"migrate", "--url", "postgres://localhost/test"}) + require.EqualError(t, err, "migrate: provide exactly one of --alter or --desired") +} + +func TestMigrateAlterRejectsSchemaFlag(t *testing.T) { + root := New("test") + parser, err := kong.New(root, kong.Vars{"version": "test"}) + require.NoError(t, err) + _, err = parser.Parse([]string{"migrate", "--url", "postgres://localhost/test", "--alter", "ALTER TABLE documents ADD COLUMN body text", "--schema", "app"}) + require.EqualError(t, err, "migrate: --schema requires --desired; qualify the table in --alter instead") +} diff --git a/internal/cli/migrate_row_security.go b/internal/cli/migrate_row_security.go index 826bb68..1e4b063 100644 --- a/internal/cli/migrate_row_security.go +++ b/internal/cli/migrate_row_security.go @@ -46,7 +46,6 @@ func rowSecurityVerdict(table string, report executor.RowSecurityReport, err err } v.Outcome = verdict.OutcomeFailed v.ExecutedSQL = nil - v.Code = string(executor.OutcomeCode(err)) v.Detail = err.Error() if errors.Is(err, executor.ErrRowSecurityRefused) { return v.WithRefusal(verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement)) @@ -54,5 +53,6 @@ func rowSecurityVerdict(table string, report executor.RowSecurityReport, err err if errors.Is(err, executor.ErrTableNotFound) { return v.WithRefusal(verdict.Environmental(verdict.ReasonUnsupportedStatement)) } + v.Code = string(executor.OutcomeCode(err)) return v } diff --git a/internal/cli/migrate_row_security_integration_test.go b/internal/cli/migrate_row_security_integration_test.go index 56e5b33..84b9b46 100644 --- a/internal/cli/migrate_row_security_integration_test.go +++ b/internal/cli/migrate_row_security_integration_test.go @@ -55,7 +55,8 @@ func TestMigrateDesiredRLSMissingTableIsRefused(t *testing.T) { require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) var v verdict.Verdict require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) - assert.Equal(t, string(executor.CodeTableNotFound), v.Code) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Empty(t, v.Code) _, err = schemadiff.Introspect(t.Context(), pool, schema, "documents") require.ErrorIs(t, err, schemadiff.ErrTableNotFound) } diff --git a/pkg/plan/desired.go b/pkg/plan/desired.go new file mode 100644 index 0000000..287882c --- /dev/null +++ b/pkg/plan/desired.go @@ -0,0 +1,19 @@ +package plan + +import ( + "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/verdict" +) + +// RefuseDestructive marks destructive steps as non-executable for desired-state +// previews. It preserves existing refusals, withdraws execution metadata, and +// fingerprints the resulting plan. Execution still enforces its own admission. +func RefuseDestructive(report *Report) { + refuseStatements(report, func(i int) (verdict.Refusal, executor.CreateShapeCause) { + if report.Statements[i].Destructive { + return verdict.ByDesign(verdict.ReasonDestructiveChange), "" + } + return verdict.Refusal{}, "" + }) + report.Fingerprint = Fingerprint(report.Statements) +} diff --git a/pkg/plan/desired_test.go b/pkg/plan/desired_test.go new file mode 100644 index 0000000..8d35bf8 --- /dev/null +++ b/pkg/plan/desired_test.go @@ -0,0 +1,46 @@ +package plan_test + +import ( + "testing" + + "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/verdict" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRefuseDestructiveWithdrawsExecutionAndRehashes(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Disposition = router.DispositionExecute + report.Statements = []plan.Statement{{ + SQL: "DROP INDEX idx_documents", Destructive: true, + Backend: router.BackendNative, Disposition: router.DispositionExecute, + ExecSQL: []string{"DROP INDEX CONCURRENTLY idx_documents"}, + Execution: planner.ExecutionAutocommit, + Decisions: []planner.Decision{{SaferSQL: []string{"DROP INDEX CONCURRENTLY idx_documents"}, SaferSQLExecution: planner.ExecutionAutocommit}}, + }, { + SQL: "ALTER TABLE documents ADD COLUMN body text", Backend: router.BackendNative, + Disposition: router.DispositionExecute, + }} + report.Fingerprint = plan.Fingerprint(report.Statements) + before := report.Fingerprint + untouched := report.Statements[1] + plan.RefuseDestructive(&report) + st := report.Statements[0] + assert.Equal(t, router.DispositionRefuse, report.Disposition) + assert.Equal(t, verdict.ReasonDestructiveChange, report.Reason) + assert.Equal(t, verdict.ClassByDesign, report.Class) + assert.Equal(t, router.DispositionRefuse, st.Disposition) + assert.Empty(t, st.Backend) + assert.Empty(t, st.ExecSQL) + assert.Empty(t, st.Execution) + assert.Empty(t, st.Decisions[0].SaferSQL) + assert.Empty(t, st.Decisions[0].SaferSQLExecution) + require.NotNil(t, st.BlockingPassthroughEligible) + assert.False(t, *st.BlockingPassthroughEligible) + assert.Equal(t, untouched, report.Statements[1]) + assert.NotEqual(t, before, report.Fingerprint) + assert.Equal(t, plan.Fingerprint(report.Statements), report.Fingerprint) +} From d099e71dd7ed1525fc6ed82c32f563353365c143 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 26 Sep 2026 01:38:28 -0400 Subject: [PATCH 3/4] fix: align desired preview and refusal contracts Signed-off-by: Armand Parajon --- README.md | 12 ++-- demo/tour.sh | 1 + docs/capabilities.md | 7 +- docs/limitations.md | 4 +- docs/optimistic-attempt.md | 4 +- docs/refusal-classes.md | 12 +++- internal/cli/diff_row_security.go | 3 +- internal/cli/migrate_desired.go | 2 +- .../cli/migrate_desired_integration_test.go | 19 +++++- internal/cli/migrate_row_security.go | 16 ++--- .../migrate_row_security_integration_test.go | 65 +++++++++++++++++++ pkg/executor/row_security.go | 2 +- pkg/executor/row_security_internal_test.go | 9 +++ pkg/executor/row_security_owner.go | 7 +- pkg/migrate/refusal_registry.go | 28 +++++++- pkg/migrate/refusal_registry_test.go | 25 +++++++ pkg/plan/desired.go | 18 +++-- pkg/plan/desired_test.go | 21 +++++- pkg/plan/refusal.go | 11 ++++ pkg/statement/desired_rls.go | 7 ++ pkg/statement/desired_rls_test.go | 7 ++ 21 files changed, 238 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 4737bdb..c26901a 100644 --- a/README.md +++ b/README.md @@ -42,9 +42,8 @@ default when the submitted form blocks (reported in the verdict's `executed_sql` optimistic native attempt otherwise. A gated `--force` runs the submitted form as-is under the same budgets. Changes without an available backend get a structured refusal (exit code 2). Desired-state execution — converging a live table onto a `CREATE TABLE` file, including -creating the table when it does not exist yet — is a Go API today: `migrate.RunDesired` -in [`pkg/migrate`](pkg/migrate/desired.go); the CLI's `migrate` verb takes one imperative -statement. +creating the table when it does not exist yet — is available through `migrate --desired schema.sql` and +`migrate.RunDesired` in [`pkg/migrate`](pkg/migrate/desired.go). The design docs and the phased build plan live in [docs/](docs/) — start with [docs/README.md](docs/README.md); the vision — what pg-sprite is and is not — @@ -79,10 +78,9 @@ refusal — never a silently wrong or incomplete result: - **Unlogged tables and explicit column collations** are outside the declarative model: converging either is a table (or column) rewrite, so export and diff refuse rather than plan one. -- **Desired-state execution has no CLI verb yet** — `migrate.RunDesired` - (including the greenfield `CREATE TABLE` path for a table that does not - exist) is library-only; the CLI's `migrate` takes one imperative - statement. +- **Destructive desired-state plans are refused as a whole** — `migrate --desired` + creates missing tables and converges supported changes, but never infers permission + to discard live structure. - **Invalid-index recovery has no CLI verb yet** — a failed concurrent index build's leftover is reported with a typed state, and the proven removal (`executor.RebuildAbandonedIndex`, or `executor.DropAbandonedIndex` when diff --git a/demo/tour.sh b/demo/tour.sh index e9ae0a3..2e661fc 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -368,6 +368,7 @@ run_rls_apply() { out=$("$PGS" migrate --url "$PG_DSN" --schema demo_security --desired desired-rls.sql --dry-run --json) || status=$? if [ "$CHECK" = 1 ]; then assert_eq "RLS preview exit" 2 "$status" + assert_eq "RLS preview class" capability-boundary "$(jq -r '.class' <<<"$out")" assert_eq "RLS preview changes" true "$(jq '.row_security_review.changes | length > 0' <<<"$out")" else printf '%s\n' "$out" diff --git a/docs/capabilities.md b/docs/capabilities.md index 6671070..c50f4e5 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -30,7 +30,8 @@ refused form would take, what an operator who accepts a maintenance window can d - [Deliberately operator-owned](#deliberately-operator-owned) A ✅ marks an implemented capability; check the front-door columns for CLI access. -The atomic RLS executor is currently a Go API only, with no `migrate` or `diff` execution. +The atomic RLS executor is available through `migrate --desired` and the Go API; +`diff` shows RLS changes for review without emitting executable SQL. ## Query the matrix @@ -52,8 +53,8 @@ pg-sprite capabilities --json | jq '.capabilities[] | select(.tier == "t2")' # What is waiting on the copy engine, across tiers. pg-sprite capabilities --json | jq '.capabilities[] | select(.engine_path == "copy_and_swap")' -# Everything the declarative door refuses. Both doors carry the same disposition on -# every row today; the map exists so they can diverge, so query the door you use. +# Everything diff refuses. Query the door you use: RLS changes are review-only +# in diff, but can execute through migrate --desired. pg-sprite capabilities --json | jq '.capabilities[] | select(.front_doors.diff == "refused")' # Rows another tool class owns: the ⚪ and 🔵 rows. The ❌ rows name no owner, because diff --git a/docs/limitations.md b/docs/limitations.md index aa94f26..4ec0f8a 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -36,12 +36,12 @@ with a typed refusal — never a silently wrong or incomplete result: | Non-table objects | Views, materialized views, standalone sequences, enums, domains, extensions, functions, and triggers are outside the model. A serial column's owned sequence is the one exception: it round-trips through the `serial` pseudo-types — and ownership is verified through the catalog (`pg_depend`), so a hand-written `nextval` default on a standalone sequence that merely carries the serial-style name refuses rather than exporting as `serial` and silently privatizing a shared sequence. A column may *use* an unmanaged type (an enum, a domain) — the type text round-trips — but the type's definition is not managed. | | Multiple tables per file | A desired file is single-table scoped: exactly one `CREATE TABLE` plus `CREATE INDEX` statements on it. Multi-table schemas are managed as one file per table. | | Live tables with no desired file | Not in `diff`'s view: the diff is single-table scoped, so it never plans or executes a `DROP TABLE`, and the imperative front door refuses the statement as unsupported. The set of tables a schema should contain is the whole-schema owner's to enumerate and reconcile — see [Deliberately operator-owned](capabilities.md#deliberately-operator-owned) for the catalog query and its exclusions. | -| Greenfield table creation | Reachable through desired-state execution (`migrate.RunDesired`, library-only today — no CLI verb): a plan whose table does not exist routes to the executor create path (`executor.ExecuteCreate`). Planning refuses every clause that binds to an existing object the absence proof does not cover (`PARTITION OF`, `INHERITS`, `LIKE`, and `OF`), plus `IF NOT EXISTS` and duplicate claimed relation names. Apply re-checks the same shape rules before anything executes. `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse (`statement.ParseDesired`); the create path's admission re-checks them as defense in depth. | +| Greenfield table creation | Reachable through desired-state execution (`migrate --desired` or `migrate.RunDesired`): a plan whose table does not exist routes to the executor create path (`executor.ExecuteCreate`). Planning refuses every clause that binds to an existing object the absence proof does not cover (`PARTITION OF`, `INHERITS`, `LIKE`, and `OF`), plus `IF NOT EXISTS` and duplicate claimed relation names. Apply re-checks the same shape rules before anything executes. `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse (`statement.ParseDesired`); the create path's admission re-checks them as defense in depth. | | Changed index or constraint definition | A redefinition diffs to drop-and-recreate, the drop is destructive, and desired-state execution refuses any plan containing a destructive statement — the whole plan, including the harmless recreate. Run the drop deliberately first (`DROP INDEX CONCURRENTLY` directly against the database; `ALTER TABLE ... DROP CONSTRAINT` through the imperative front door), then rerun — the remaining plan converges the recreate. | ## What desired-state execution converges today -Desired-state execution (`migrate.RunDesired`, library-only today) feeds +Desired-state execution (`migrate --desired` or `migrate.RunDesired`) feeds every planned statement back through the same gates as the imperative front door, so the outcome of an ordinary desired-file edit is the composition of the model boundaries above with those gates. At a glance: diff --git a/docs/optimistic-attempt.md b/docs/optimistic-attempt.md index d923091..6d5e181 100644 --- a/docs/optimistic-attempt.md +++ b/docs/optimistic-attempt.md @@ -154,8 +154,8 @@ What happens to one statement, in order: same gates below. An orchestrator embedding the library (see [schemabot-integration.md](schemabot-integration.md)) enters the same way. The doors differ before the fork, not after it. The declarative door rejects lane F - outright (`ErrForceNotSupported`), is library-only today (`RunDesired`; no CLI - verb — [limitations.md](limitations.md)), and runs a whole-plan admission gate + outright (`ErrForceNotSupported`), is available through `migrate --desired` and + `RunDesired` ([limitations.md](limitations.md)), and runs a whole-plan admission gate before any statement enters the walk: the plan is refused all-or-nothing when the plan derived at execution time is not the pinned one (`plan-fingerprint-mismatch`), any planned statement discards live structure (`destructive-change`), or the plan diff --git a/docs/refusal-classes.md b/docs/refusal-classes.md index dfee1f8..4085ab6 100644 --- a/docs/refusal-classes.md +++ b/docs/refusal-classes.md @@ -236,7 +236,8 @@ from production (`verdict.Reasons()`, `executor.CreateShapeCauses()`, `preflight.PartitionRefusalCauses()`, the admission sentinel sets, and a walk of the remaining refusal sites) and fails if a key is absent, carries the zero class, or violates the owner rule. The registry has two halves: `pkg/plan/refusal.go` classifies the keys that travel with -a planned statement (`CreateShapeRefusal`, `PartitionRefusal`, `RouteRefusal`), and +a planned statement (`CreateShapeRefusal`, `PartitionRefusal`, `RouteRefusal`, +`DestructiveChangeRefusal`, `RowSecurityReviewRefusal`), and `pkg/migrate/refusal_registry.go` classifies statement kinds at the gate, the admission sentinel sets, and the imperative sites; `TestRefusalRegistryIsComplete` in `pkg/migrate` covers both halves. Keying on causes is what gives the test correspondence rather than presence: a registry @@ -244,6 +245,15 @@ keyed on sites alone would go green with `admissionRefusalVerdict` classified `capability-boundary` while it minted a `by-design` refusal for every `CREATE ... IF NOT EXISTS`. +RLS preview uses the plan registry's `capability-boundary` refusal: its deltas +are review-only. Atomic RLS execution uses `migrate.RowSecurityRefusal`: +missing owner or database CREATE privileges (including PostgreSQL SQLSTATE 42501) +carry `insufficient-privileges` / `environmental`; an absent target table carries +`unsupported-statement` / `environmental`; unsupported declarations or target +shapes carry `unsupported-statement` / `capability-boundary`. Operational failures +remain failures. Typed privilege causes take precedence over the executor's general +admission sentinel. The registry tests cover these mappings and refusal classes. + The test also pins a sentinel set of keys it must find — at least one cause from each closed set and the `KindOther` catch-all — so that a change to how refusal verdicts are constructed cannot make the deriver find zero sites and pass vacuously on an empty set. This is the same diff --git a/internal/cli/diff_row_security.go b/internal/cli/diff_row_security.go index 9e5d63e..998ad54 100644 --- a/internal/cli/diff_row_security.go +++ b/internal/cli/diff_row_security.go @@ -10,6 +10,7 @@ import ( "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/diffplan" + "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" @@ -40,7 +41,7 @@ func (c *DiffCmd) runRowSecurityDiff(ctx context.Context, out io.Writer, sql str func (c *DiffCmd) writeRowSecurityRefusal(out io.Writer, cause error) error { var review *diffplan.RowSecurityReviewRequired errors.As(cause, &review) - v := verdict.Verdict{Outcome: verdict.OutcomeRefused, Reason: verdict.ReasonUnsupportedStatement, Detail: cause.Error()} + v := verdict.Verdict{Detail: cause.Error()}.WithRefusal(plan.RowSecurityReviewRefusal()) switch { case c.JSON && review != nil: report := struct { diff --git a/internal/cli/migrate_desired.go b/internal/cli/migrate_desired.go index ccf16b6..e1122d8 100644 --- a/internal/cli/migrate_desired.go +++ b/internal/cli/migrate_desired.go @@ -57,7 +57,7 @@ func (c *MigrateCmd) runDesired(ctx context.Context, out io.Writer) error { // Preview uses desired-state admission: a routed native step can still // discard live structure, which RunDesired refuses before execution. plan.RefuseDestructive(&report) - diff := DiffCmd{DBFlags: c.DBFlags, OutputFlags: c.OutputFlags, JSON: c.JSON} + diff := DiffCmd{DBFlags: c.DBFlags, OutputFlags: c.OutputFlags, Schema: c.Schema, JSON: c.JSON} return diff.writeDiffReport(out, report) } result, runErr := migrate.RunDesired(ctx, pool, migrate.DesiredRequest{Schema: c.Schema, Desired: desired}, c.options(c.diag())) diff --git a/internal/cli/migrate_desired_integration_test.go b/internal/cli/migrate_desired_integration_test.go index 88a2f6b..ee9f224 100644 --- a/internal/cli/migrate_desired_integration_test.go +++ b/internal/cli/migrate_desired_integration_test.go @@ -44,6 +44,9 @@ func TestMigrateDesiredRLSAppliesAndConverges(t *testing.T) { var out strings.Builder cmd.DryRun = true require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var preview verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &preview)) + assert.Equal(t, verdict.ClassCapabilityBoundary, preview.Class) before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") require.NoError(t, err) assert.False(t, before.RowSecurity.Enabled, "preview must not change access") @@ -126,14 +129,24 @@ func TestMigrateDesiredTableRefusesDestructiveChange(t *testing.T) { url, schema, pool := desiredFixture(t) cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( id int PRIMARY KEY - );`) + ); + CREATE INDEX documents_id_idx ON documents (id);`) + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) var out strings.Builder cmd.DryRun = true require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) var preview plan.Report require.NoError(t, json.Unmarshal([]byte(out.String()), &preview)) assert.Equal(t, router.DispositionRefuse, preview.Disposition) - require.Len(t, preview.Statements, 1) + require.Len(t, preview.Statements, 2) + for _, st := range preview.Statements { + assert.Equal(t, router.DispositionRefuse, st.Disposition) + assert.Equal(t, verdict.ReasonDestructiveChange, st.Reason) + assert.Empty(t, st.Backend) + assert.Empty(t, st.ExecSQL) + assert.Empty(t, st.Execution) + } assert.Equal(t, verdict.ReasonDestructiveChange, preview.Statements[0].Reason) assert.Equal(t, verdict.ReasonDestructiveChange, preview.Reason) assert.Empty(t, preview.Statements[0].Backend) @@ -149,7 +162,7 @@ func TestMigrateDesiredTableRefusesDestructiveChange(t *testing.T) { assert.Empty(t, result.Verdicts) after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") require.NoError(t, err) - assert.Len(t, after.Columns, 2) + assert.Equal(t, before, after, "whole-plan admission must leave both columns and indexes unchanged") } func TestMigrateDesiredCreatesMissingTable(t *testing.T) { diff --git a/internal/cli/migrate_row_security.go b/internal/cli/migrate_row_security.go index 1e4b063..cda76a3 100644 --- a/internal/cli/migrate_row_security.go +++ b/internal/cli/migrate_row_security.go @@ -7,19 +7,20 @@ import ( "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/migrate" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" ) func (c *MigrateCmd) runDesiredRowSecurity(ctx context.Context, out io.Writer, sql string) error { - desired, err := statement.ParseDesiredWithRowSecurity(sql) - if err != nil { - return err - } if c.DryRun { diff := DiffCmd{DBFlags: c.DBFlags, OutputFlags: c.OutputFlags, Schema: c.Schema, JSON: c.JSON} return diff.runRowSecurityDiff(ctx, out, sql) } + desired, err := statement.ParseDesiredWithRowSecurity(sql) + if err != nil { + return err + } pool, err := dbconn.NewPool(ctx, c.Config()) if err != nil { return err @@ -47,11 +48,8 @@ func rowSecurityVerdict(table string, report executor.RowSecurityReport, err err v.Outcome = verdict.OutcomeFailed v.ExecutedSQL = nil v.Detail = err.Error() - if errors.Is(err, executor.ErrRowSecurityRefused) { - return v.WithRefusal(verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement)) - } - if errors.Is(err, executor.ErrTableNotFound) { - return v.WithRefusal(verdict.Environmental(verdict.ReasonUnsupportedStatement)) + if refusal, ok := migrate.RowSecurityRefusal(err); ok { + return v.WithRefusal(refusal) } v.Code = string(executor.OutcomeCode(err)) return v diff --git a/internal/cli/migrate_row_security_integration_test.go b/internal/cli/migrate_row_security_integration_test.go index 84b9b46..a918e06 100644 --- a/internal/cli/migrate_row_security_integration_test.go +++ b/internal/cli/migrate_row_security_integration_test.go @@ -4,12 +4,14 @@ import ( "context" "encoding/json" "fmt" + neturl "net/url" "strings" "testing" "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/verdict" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -78,3 +80,66 @@ func TestMigrateDesiredRLSRemovesLastPolicy(t *testing.T) { assert.True(t, after.RowSecurity.Enabled) assert.Empty(t, after.RowSecurity.Policies, "enabled without policies is default deny") } + +// Both privilege failures must tell callers to fix the environment, not wait +// for a new engine capability. Neither may change the live table. +func TestMigrateDesiredRLSPrivilegeRefusals(t *testing.T) { + for _, owner := range []bool{false, true} { + name := "non-owner" + if owner { + name = "owner-without-database-create" + } + t.Run(name, func(t *testing.T) { + url, schema, pool := desiredFixture(t) + role := pgx.Identifier{schema + "_role"}.Sanitize() + _, err := pool.Exec(t.Context(), "CREATE ROLE "+role+" LOGIN") + require.NoError(t, err) + t.Cleanup(func() { + ctx := context.WithoutCancel(t.Context()) + _, err := pool.Exec(ctx, "DROP OWNED BY "+role) + require.NoError(t, err) + _, err = pool.Exec(ctx, "DROP ROLE "+role) + require.NoError(t, err) + }) + _, err = pool.Exec(t.Context(), "GRANT USAGE, CREATE ON SCHEMA "+pgx.Identifier{schema}.Sanitize()+" TO "+role) + require.NoError(t, err) + if owner { + _, err = pool.Exec(t.Context(), "ALTER TABLE "+pgx.Identifier{schema, "documents"}.Sanitize()+" OWNER TO "+role) + require.NoError(t, err) + } + if !owner { + var database string + require.NoError(t, pool.QueryRow(t.Context(), "SELECT current_database()").Scan(&database)) + _, err = pool.Exec(t.Context(), "GRANT CREATE ON DATABASE "+pgx.Identifier{database}.Sanitize()+" TO "+role) + require.NoError(t, err) + } + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + config, err := neturl.Parse(url) + require.NoError(t, err) + query := config.Query() + query.Set("role", schema+"_role") + config.RawQuery = query.Encode() + cmd := desiredCommand(t, config.String(), schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + owner_id int NOT NULL + ); + ALTER TABLE documents ENABLE ROW LEVEL SECURITY;`) + var out strings.Builder + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.ReasonInsufficientPrivileges, v.Reason) + assert.Equal(t, verdict.ClassEnvironmental, v.Class) + if owner { + assert.Contains(t, v.Detail, "requires CREATE on the database") + } else { + assert.Contains(t, v.Detail, "requires owner privileges") + } + assert.Empty(t, v.ExecutedSQL) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Equal(t, before, after) + }) + } +} diff --git a/pkg/executor/row_security.go b/pkg/executor/row_security.go index 51f1950..11c410d 100644 --- a/pkg/executor/row_security.go +++ b/pkg/executor/row_security.go @@ -77,7 +77,7 @@ func rowSecurityError(caller, attempt context.Context, err error, b Budget) erro } var pgErr *pgconn.PgError if errors.As(err, &pgErr) && pgErr.Code == "42501" { - return fmt.Errorf("%w: %w", ErrRowSecurityRefused, err) + return fmt.Errorf("%w: %w: %w", ErrRowSecurityRefused, ErrRowSecurityPrivileges, err) } return err } diff --git a/pkg/executor/row_security_internal_test.go b/pkg/executor/row_security_internal_test.go index 433ee8a..3953416 100644 --- a/pkg/executor/row_security_internal_test.go +++ b/pkg/executor/row_security_internal_test.go @@ -8,6 +8,7 @@ import ( "github.com/block/pg-sprite/pkg/statement" "github.com/jackc/pgx/v5/pgconn" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRowSecurityErrorClassification(t *testing.T) { @@ -52,3 +53,11 @@ func TestRowSecurityAdmissionErrorsArePermanent(t *testing.T) { assert.True(t, OutcomeCode(err).Permanent()) } } + +func TestRowSecurityPermissionDeniedRetainsPrivilegeCause(t *testing.T) { + cause := &pgconn.PgError{Code: "42501"} + err := rowSecurityError(t.Context(), t.Context(), cause, Budget{}) + require.ErrorIs(t, err, ErrRowSecurityRefused) + require.ErrorIs(t, err, ErrRowSecurityPrivileges) + require.ErrorIs(t, err, cause) +} diff --git a/pkg/executor/row_security_owner.go b/pkg/executor/row_security_owner.go index 7204e34..a38ac70 100644 --- a/pkg/executor/row_security_owner.go +++ b/pkg/executor/row_security_owner.go @@ -12,6 +12,9 @@ import ( // change before retrying. Wrapped causes retain the specific admission failure. var ErrRowSecurityRefused = errors.New("row security change refused") +// ErrRowSecurityPrivileges identifies a missing grant, distinct from unsupported declarations. +var ErrRowSecurityPrivileges = errors.New("insufficient row security privileges") + func checkRowSecurityPrivileges(ctx context.Context, tx pgx.Tx, schema, table string) error { var owner, createSchema bool err := tx.QueryRow(ctx, `SELECT pg_catalog.pg_has_role(current_user, c.relowner, 'USAGE'), @@ -26,10 +29,10 @@ func checkRowSecurityPrivileges(ctx context.Context, tx pgx.Tx, schema, table st return fmt.Errorf("check row security owner for %s: %w", target, err) } if !owner { - return fmt.Errorf("row security on %s requires owner privileges: %w", target, ErrRowSecurityRefused) + return fmt.Errorf("row security on %s requires owner privileges: %w: %w", target, ErrRowSecurityRefused, ErrRowSecurityPrivileges) } if !createSchema { - return fmt.Errorf("row security on %s requires CREATE on the database for scratch inspection: %w", target, ErrRowSecurityRefused) + return fmt.Errorf("row security on %s requires CREATE on the database for scratch inspection: %w: %w", target, ErrRowSecurityRefused, ErrRowSecurityPrivileges) } return nil } diff --git a/pkg/migrate/refusal_registry.go b/pkg/migrate/refusal_registry.go index 0492688..a8890ac 100644 --- a/pkg/migrate/refusal_registry.go +++ b/pkg/migrate/refusal_registry.go @@ -54,7 +54,7 @@ func budgetExceededRefusal() verdict.Refusal { // destructiveChangeRefusal: desired-state execution never infers permission // to discard live structure; the deliberate path is the imperative statement. func destructiveChangeRefusal() verdict.Refusal { - return verdict.ByDesign(verdict.ReasonDestructiveChange) + return plan.DestructiveChangeRefusal() } // fingerprintMismatchRefusal: the live table or desired schema moved under @@ -94,6 +94,8 @@ func siteRefusals() []siteRefusal { {"plan-fingerprint-mismatch", fingerprintMismatchRefusal()}, {"create-collision", createCollisionRefusal()}, {"plan-incoherent", planIncoherentRefusal()}, + {"row-security-unsupported", rowSecurityUnsupportedRefusal()}, + {"row-security-missing-table", rowSecurityMissingTableRefusal()}, } } @@ -214,3 +216,27 @@ func isInSentinelSet(err error, set []error) bool { } return false } + +func rowSecurityUnsupportedRefusal() verdict.Refusal { + return verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement) +} + +func rowSecurityMissingTableRefusal() verdict.Refusal { + return verdict.Environmental(verdict.ReasonUnsupportedStatement) +} + +// RowSecurityRefusal classifies typed admission failures from the atomic RLS +// executor. Operational errors are not refusals. Privilege causes take priority +// over the general admission sentinel they also wrap. +func RowSecurityRefusal(err error) (verdict.Refusal, bool) { + if errors.Is(err, executor.ErrRowSecurityPrivileges) { + return insufficientPrivilegesRefusal(), true + } + if errors.Is(err, executor.ErrRowSecurityRefused) { + return rowSecurityUnsupportedRefusal(), true + } + if errors.Is(err, executor.ErrTableNotFound) { + return rowSecurityMissingTableRefusal(), true + } + return verdict.Refusal{}, false +} diff --git a/pkg/migrate/refusal_registry_test.go b/pkg/migrate/refusal_registry_test.go index b5bdbc9..c42cc9d 100644 --- a/pkg/migrate/refusal_registry_test.go +++ b/pkg/migrate/refusal_registry_test.go @@ -71,6 +71,7 @@ func deriveRefusalKeys() (keys []classifiedKey, admitted []string) { keys = append(keys, classifiedKey{"site:" + s.site, s.refusal, true}) } keys = append(keys, classifiedKey{"route", plan.RouteRefusal(), true}) + keys = append(keys, classifiedKey{"row-security-review", plan.RowSecurityReviewRefusal(), true}) return keys, admitted } @@ -357,3 +358,27 @@ func hasKey(keys []classifiedKey, key string) bool { } return false } + +func TestRowSecurityRefusalRegistry(t *testing.T) { + cases := []struct { + name string + cause error + reason verdict.Reason + class verdict.Class + }{ + {"privileges", errors.Join(executor.ErrRowSecurityRefused, executor.ErrRowSecurityPrivileges), verdict.ReasonInsufficientPrivileges, verdict.ClassEnvironmental}, + {"unsupported", executor.ErrRowSecurityRefused, verdict.ReasonUnsupportedStatement, verdict.ClassCapabilityBoundary}, + {"missing-table", executor.ErrTableNotFound, verdict.ReasonUnsupportedStatement, verdict.ClassEnvironmental}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + refusal, ok := RowSecurityRefusal(fmt.Errorf("wrapped: %w", tc.cause)) + require.True(t, ok) + v := verdict.Verdict{}.WithRefusal(refusal) + assert.Equal(t, tc.reason, v.Reason) + assert.Equal(t, tc.class, v.Class) + }) + } + _, ok := RowSecurityRefusal(errors.New("operational failure")) + assert.False(t, ok) +} diff --git a/pkg/plan/desired.go b/pkg/plan/desired.go index 287882c..f730b37 100644 --- a/pkg/plan/desired.go +++ b/pkg/plan/desired.go @@ -5,15 +5,19 @@ import ( "github.com/block/pg-sprite/pkg/verdict" ) -// RefuseDestructive marks destructive steps as non-executable for desired-state -// previews. It preserves existing refusals, withdraws execution metadata, and +// RefuseDestructive marks the whole desired-state preview non-executable when +// any step is destructive. It preserves existing refusals, withdraws execution metadata, and // fingerprints the resulting plan. Execution still enforces its own admission. func RefuseDestructive(report *Report) { - refuseStatements(report, func(i int) (verdict.Refusal, executor.CreateShapeCause) { - if report.Statements[i].Destructive { - return verdict.ByDesign(verdict.ReasonDestructiveChange), "" - } - return verdict.Refusal{}, "" + destructive := false + for _, st := range report.Statements { + destructive = destructive || st.Destructive + } + if !destructive { + return + } + refuseStatements(report, func(_ int) (verdict.Refusal, executor.CreateShapeCause) { + return DestructiveChangeRefusal(), "" }) report.Fingerprint = Fingerprint(report.Statements) } diff --git a/pkg/plan/desired_test.go b/pkg/plan/desired_test.go index 8d35bf8..8d8c72e 100644 --- a/pkg/plan/desired_test.go +++ b/pkg/plan/desired_test.go @@ -26,7 +26,6 @@ func TestRefuseDestructiveWithdrawsExecutionAndRehashes(t *testing.T) { }} report.Fingerprint = plan.Fingerprint(report.Statements) before := report.Fingerprint - untouched := report.Statements[1] plan.RefuseDestructive(&report) st := report.Statements[0] assert.Equal(t, router.DispositionRefuse, report.Disposition) @@ -40,7 +39,25 @@ func TestRefuseDestructiveWithdrawsExecutionAndRehashes(t *testing.T) { assert.Empty(t, st.Decisions[0].SaferSQLExecution) require.NotNil(t, st.BlockingPassthroughEligible) assert.False(t, *st.BlockingPassthroughEligible) - assert.Equal(t, untouched, report.Statements[1]) + assert.Equal(t, router.DispositionRefuse, report.Statements[1].Disposition) + assert.Equal(t, verdict.ReasonDestructiveChange, report.Statements[1].Reason) + assert.Empty(t, report.Statements[1].Backend) assert.NotEqual(t, before, report.Fingerprint) assert.Equal(t, plan.Fingerprint(report.Statements), report.Fingerprint) } + +func TestRefuseDestructiveLeavesNondestructivePlanUnchanged(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Disposition = router.DispositionExecute + report.Statements = []plan.Statement{{ + SQL: "ALTER TABLE documents ADD COLUMN body text", + Backend: router.BackendNative, Disposition: router.DispositionExecute, + }} + report.Fingerprint = plan.Fingerprint(report.Statements) + before := report.Statements[0] + fingerprint := report.Fingerprint + plan.RefuseDestructive(&report) + assert.Equal(t, router.DispositionExecute, report.Disposition) + assert.Equal(t, before, report.Statements[0]) + assert.Equal(t, fingerprint, report.Fingerprint) +} diff --git a/pkg/plan/refusal.go b/pkg/plan/refusal.go index e35fe17..2ddb9f8 100644 --- a/pkg/plan/refusal.go +++ b/pkg/plan/refusal.go @@ -78,3 +78,14 @@ func PartitionRefusal(cause preflight.PartitionRefusalCause) (verdict.Refusal, b func RouteRefusal() verdict.Refusal { return verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement) } + +// DestructiveChangeRefusal classifies desired-state admission of a plan that +// would discard live structure; the operator must choose an explicit statement. +func DestructiveChangeRefusal() verdict.Refusal { + return verdict.ByDesign(verdict.ReasonDestructiveChange) +} + +// RowSecurityReviewRefusal keeps policy deltas review-only in diff output. +func RowSecurityReviewRefusal() verdict.Refusal { + return verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement) +} diff --git a/pkg/statement/desired_rls.go b/pkg/statement/desired_rls.go index e1a3518..5544bc1 100644 --- a/pkg/statement/desired_rls.go +++ b/pkg/statement/desired_rls.go @@ -45,8 +45,12 @@ func ParseDesiredWithRowSecurity(sql string) (DesiredWithRowSecurity, error) { } var tableSQL strings.Builder var security []*pganalyze.Node + hasTable := false for _, raw := range tree.GetStmts() { node := raw.GetStmt() + if node.GetCreateStmt() != nil { + hasTable = true + } if node.GetCreateStmt() != nil || node.GetIndexStmt() != nil { text, err := deparseOne(node) if err != nil { @@ -57,6 +61,9 @@ func ParseDesiredWithRowSecurity(sql string) (DesiredWithRowSecurity, error) { security = append(security, node) } } + if !hasTable { + return DesiredWithRowSecurity{}, fmt.Errorf("desired row security requires a CREATE TABLE definition: %w", ErrRowSecurityDeclaration) + } table, err := ParseDesired(tableSQL.String()) if err != nil { return DesiredWithRowSecurity{}, err diff --git a/pkg/statement/desired_rls_test.go b/pkg/statement/desired_rls_test.go index b3d4bfe..3d72735 100644 --- a/pkg/statement/desired_rls_test.go +++ b/pkg/statement/desired_rls_test.go @@ -111,3 +111,10 @@ func TestRelationWalkHandlesMessageMaps(t *testing.T) { require.NoError(t, err) assert.False(t, messageReadsRelation(value.ProtoReflect())) } + +func TestDesiredRowSecurityRequiresTableDefinition(t *testing.T) { + _, err := ParseDesiredWithRowSecurity(`ALTER TABLE documents ENABLE ROW LEVEL SECURITY; + CREATE POLICY readers ON documents FOR SELECT USING (true);`) + require.ErrorIs(t, err, ErrRowSecurityDeclaration) + assert.Contains(t, err.Error(), "CREATE TABLE") +} From c4e94a7a17930583bc7ddb1a3ef90961d93e8022 Mon Sep 17 00:00:00 2001 From: Armand Parajon Date: Sat, 26 Sep 2026 02:19:45 -0400 Subject: [PATCH 4/4] fix: match preview refusals to desired apply Signed-off-by: Armand Parajon --- docs/cli-output-examples.md | 2 +- docs/refusal-classes.md | 7 ++-- internal/cli/diff_row_security.go | 6 +++- .../cli/migrate_desired_integration_test.go | 33 +++++++++++++++++++ .../migrate_row_security_integration_test.go | 24 +++++++++----- pkg/executor/row_security_owner.go | 10 ++++-- pkg/migrate/refusal_registry.go | 8 ++--- pkg/plan/desired.go | 5 +++ pkg/plan/desired_test.go | 20 +++++++++++ pkg/plan/refusal.go | 6 ++++ pkg/statement/desired_rls.go | 5 ++- pkg/statement/desired_rls_test.go | 2 +- 12 files changed, 104 insertions(+), 24 deletions(-) diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 177355d..b6f114a 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -113,7 +113,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `unsupported-statement` | No safe path is known for the statement — only `ALTER TABLE` and `CREATE INDEX` reach classification — or a greenfield create plan carries a shape the create path refuses (`PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, or a duplicate claimed relation name; the [plan statement's `cause`](plan-report.md#causes-cause-greenfield-create-shape-refusals-only) names which — a create-shape vocabulary distinct from the verdict's budget `cause` below). These greenfield shapes refuse in the plan and are re-checked at apply. | | `index-statement` | Index maintenance (`DROP INDEX`, `REINDEX`) has a native safe idiom (`CONCURRENTLY`) and is never attempted; the verdict's `safer_idiom` names it. | | `not-native-safe-table-too-large` | The size guard skipped the optimistic attempt: the table exceeds the configured bound and the change is not provably metadata-only. | -| `insufficient-privileges` | The connected role lacks the access the change needs; `detail` names the exact missing GRANT (see [engine-role.md](engine-role.md)). | +| `insufficient-privileges` | The connected role lacks the access the change needs; `detail` names the missing grant for preflight checks (see [engine-role.md](engine-role.md)). RLS admission names the required table ownership or database CREATE privilege and its remedy; permission errors reported by PostgreSQL retain the server diagnostic, which may not identify an exact GRANT. | | `unsupported-partitioned-parent` | The routed plan builds an index on a partitioned parent, where PostgreSQL cannot `CREATE INDEX CONCURRENTLY`. | | `not-native-safe-budget-exceeded` | The optimistic attempt exceeded its lock or statement budget and was cancelled; the verdict's `cause` narrows which budget fired. The same `cause` field carries the partitioned-parent shape under `unsupported-partitioned-parent`; both are refusal causes, unrelated to the plan statement's create-shape `cause`. | | `not-native-safe-rewrite-required` | The submitted form blocks and must run as a safer native sequence, but none could be constructed. | diff --git a/docs/refusal-classes.md b/docs/refusal-classes.md index 4085ab6..25df53d 100644 --- a/docs/refusal-classes.md +++ b/docs/refusal-classes.md @@ -237,7 +237,7 @@ from production (`verdict.Reasons()`, `executor.CreateShapeCauses()`, refusal sites) and fails if a key is absent, carries the zero class, or violates the owner rule. The registry has two halves: `pkg/plan/refusal.go` classifies the keys that travel with a planned statement (`CreateShapeRefusal`, `PartitionRefusal`, `RouteRefusal`, -`DestructiveChangeRefusal`, `RowSecurityReviewRefusal`), and +`DestructiveChangeRefusal`, `RowSecurityReviewRefusal`, `RowSecurityMissingTableRefusal`), and `pkg/migrate/refusal_registry.go` classifies statement kinds at the gate, the admission sentinel sets, and the imperative sites; `TestRefusalRegistryIsComplete` in `pkg/migrate` covers both halves. Keying on causes is what gives the test correspondence rather than presence: a registry @@ -245,8 +245,9 @@ keyed on sites alone would go green with `admissionRefusalVerdict` classified `capability-boundary` while it minted a `by-design` refusal for every `CREATE ... IF NOT EXISTS`. -RLS preview uses the plan registry's `capability-boundary` refusal: its deltas -are review-only. Atomic RLS execution uses `migrate.RowSecurityRefusal`: +RLS preview uses the plan registry's `capability-boundary` refusal for review-only +policy deltas. A missing target uses the shared `RowSecurityMissingTableRefusal` +in both preview and apply, with class `environmental`. Atomic RLS execution uses `migrate.RowSecurityRefusal`: missing owner or database CREATE privileges (including PostgreSQL SQLSTATE 42501) carry `insufficient-privileges` / `environmental`; an absent target table carries `unsupported-statement` / `environmental`; unsupported declarations or target diff --git a/internal/cli/diff_row_security.go b/internal/cli/diff_row_security.go index 998ad54..bd4ec6d 100644 --- a/internal/cli/diff_row_security.go +++ b/internal/cli/diff_row_security.go @@ -41,7 +41,11 @@ func (c *DiffCmd) runRowSecurityDiff(ctx context.Context, out io.Writer, sql str func (c *DiffCmd) writeRowSecurityRefusal(out io.Writer, cause error) error { var review *diffplan.RowSecurityReviewRequired errors.As(cause, &review) - v := verdict.Verdict{Detail: cause.Error()}.WithRefusal(plan.RowSecurityReviewRefusal()) + refusal := plan.RowSecurityReviewRefusal() + if errors.Is(cause, schemadiff.ErrTableNotFound) { + refusal = plan.RowSecurityMissingTableRefusal() + } + v := verdict.Verdict{Detail: cause.Error()}.WithRefusal(refusal) switch { case c.JSON && review != nil: report := struct { diff --git a/internal/cli/migrate_desired_integration_test.go b/internal/cli/migrate_desired_integration_test.go index ee9f224..308beef 100644 --- a/internal/cli/migrate_desired_integration_test.go +++ b/internal/cli/migrate_desired_integration_test.go @@ -183,3 +183,36 @@ func TestMigrateDesiredCreatesMissingTable(t *testing.T) { assert.Len(t, after.Columns, 2) assert.Equal(t, "id", after.Columns[0].Name) } + +func TestMigrateDesiredDestructiveAndUnsupportedPreviewMatchesApply(t *testing.T) { + url, schema, pool := desiredFixture(t) + before, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + cmd := desiredCommand(t, url, schema, `CREATE TABLE documents ( + id int PRIMARY KEY, + CONSTRAINT unique_id EXCLUDE USING btree (id WITH =) + );`) + var out strings.Builder + cmd.DryRun = true + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var preview plan.Report + require.NoError(t, json.Unmarshal([]byte(out.String()), &preview)) + require.Len(t, preview.Statements, 2) + assert.Equal(t, verdict.ReasonDestructiveChange, preview.Reason) + assert.Equal(t, verdict.ClassByDesign, preview.Class) + for _, st := range preview.Statements { + assert.Equal(t, router.DispositionRefuse, st.Disposition) + assert.Empty(t, st.ExecSQL) + } + out.Reset() + cmd.DryRun = false + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var result migrate.DesiredResult + require.NoError(t, json.Unmarshal([]byte(out.String()), &result)) + assert.Equal(t, preview.Reason, result.Reason) + assert.Equal(t, preview.Class, result.Class) + assert.Empty(t, result.Verdicts) + after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") + require.NoError(t, err) + assert.Equal(t, before, after) +} diff --git a/internal/cli/migrate_row_security_integration_test.go b/internal/cli/migrate_row_security_integration_test.go index a918e06..11730cb 100644 --- a/internal/cli/migrate_row_security_integration_test.go +++ b/internal/cli/migrate_row_security_integration_test.go @@ -53,12 +53,17 @@ func TestMigrateDesiredRLSMissingTableIsRefused(t *testing.T) { owner_id int NOT NULL ); ALTER TABLE documents ENABLE ROW LEVEL SECURITY;`) - var out strings.Builder - require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) - var v verdict.Verdict - require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) - assert.Equal(t, verdict.OutcomeRefused, v.Outcome) - assert.Empty(t, v.Code) + for _, dryRun := range []bool{true, false} { + cmd.DryRun = dryRun + var out strings.Builder + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonUnsupportedStatement, v.Reason) + assert.Equal(t, verdict.ClassEnvironmental, v.Class) + assert.Empty(t, v.Code) + } _, err = schemadiff.Introspect(t.Context(), pool, schema, "documents") require.ErrorIs(t, err, schemadiff.ErrTableNotFound) } @@ -126,15 +131,16 @@ func TestMigrateDesiredRLSPrivilegeRefusals(t *testing.T) { ); ALTER TABLE documents ENABLE ROW LEVEL SECURITY;`) var out strings.Builder - require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + runErr := cmd.run(t.Context(), &out) + require.ErrorIs(t, runErr, verdict.ErrRefused) var v verdict.Verdict require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) assert.Equal(t, verdict.ReasonInsufficientPrivileges, v.Reason) assert.Equal(t, verdict.ClassEnvironmental, v.Class) if owner { - assert.Contains(t, v.Detail, "requires CREATE on the database") + assert.ErrorIs(t, runErr, executor.ErrRowSecurityDatabaseCreateRequired) } else { - assert.Contains(t, v.Detail, "requires owner privileges") + assert.ErrorIs(t, runErr, executor.ErrRowSecurityOwnerRequired) } assert.Empty(t, v.ExecutedSQL) after, err := schemadiff.Introspect(t.Context(), pool, schema, "documents") diff --git a/pkg/executor/row_security_owner.go b/pkg/executor/row_security_owner.go index a38ac70..116487e 100644 --- a/pkg/executor/row_security_owner.go +++ b/pkg/executor/row_security_owner.go @@ -15,6 +15,12 @@ var ErrRowSecurityRefused = errors.New("row security change refused") // ErrRowSecurityPrivileges identifies a missing grant, distinct from unsupported declarations. var ErrRowSecurityPrivileges = errors.New("insufficient row security privileges") +// ErrRowSecurityOwnerRequired identifies a missing table-ownership privilege. +var ErrRowSecurityOwnerRequired = errors.New("connect as the table owner or a role inheriting its privileges") + +// ErrRowSecurityDatabaseCreateRequired identifies the grant needed for scratch inspection. +var ErrRowSecurityDatabaseCreateRequired = errors.New("grant CREATE on the database to the execution role for scratch inspection") + func checkRowSecurityPrivileges(ctx context.Context, tx pgx.Tx, schema, table string) error { var owner, createSchema bool err := tx.QueryRow(ctx, `SELECT pg_catalog.pg_has_role(current_user, c.relowner, 'USAGE'), @@ -29,10 +35,10 @@ func checkRowSecurityPrivileges(ctx context.Context, tx pgx.Tx, schema, table st return fmt.Errorf("check row security owner for %s: %w", target, err) } if !owner { - return fmt.Errorf("row security on %s requires owner privileges: %w: %w", target, ErrRowSecurityRefused, ErrRowSecurityPrivileges) + return fmt.Errorf("row security on %s requires owner privileges: %w: %w: %w", target, ErrRowSecurityRefused, ErrRowSecurityPrivileges, ErrRowSecurityOwnerRequired) } if !createSchema { - return fmt.Errorf("row security on %s requires CREATE on the database for scratch inspection: %w: %w", target, ErrRowSecurityRefused, ErrRowSecurityPrivileges) + return fmt.Errorf("row security on %s requires CREATE on the database for scratch inspection: %w: %w: %w", target, ErrRowSecurityRefused, ErrRowSecurityPrivileges, ErrRowSecurityDatabaseCreateRequired) } return nil } diff --git a/pkg/migrate/refusal_registry.go b/pkg/migrate/refusal_registry.go index a8890ac..4129e5d 100644 --- a/pkg/migrate/refusal_registry.go +++ b/pkg/migrate/refusal_registry.go @@ -95,7 +95,7 @@ func siteRefusals() []siteRefusal { {"create-collision", createCollisionRefusal()}, {"plan-incoherent", planIncoherentRefusal()}, {"row-security-unsupported", rowSecurityUnsupportedRefusal()}, - {"row-security-missing-table", rowSecurityMissingTableRefusal()}, + {"row-security-missing-table", plan.RowSecurityMissingTableRefusal()}, } } @@ -221,10 +221,6 @@ func rowSecurityUnsupportedRefusal() verdict.Refusal { return verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement) } -func rowSecurityMissingTableRefusal() verdict.Refusal { - return verdict.Environmental(verdict.ReasonUnsupportedStatement) -} - // RowSecurityRefusal classifies typed admission failures from the atomic RLS // executor. Operational errors are not refusals. Privilege causes take priority // over the general admission sentinel they also wrap. @@ -236,7 +232,7 @@ func RowSecurityRefusal(err error) (verdict.Refusal, bool) { return rowSecurityUnsupportedRefusal(), true } if errors.Is(err, executor.ErrTableNotFound) { - return rowSecurityMissingTableRefusal(), true + return plan.RowSecurityMissingTableRefusal(), true } return verdict.Refusal{}, false } diff --git a/pkg/plan/desired.go b/pkg/plan/desired.go index f730b37..0c003f5 100644 --- a/pkg/plan/desired.go +++ b/pkg/plan/desired.go @@ -19,5 +19,10 @@ func RefuseDestructive(report *Report) { refuseStatements(report, func(_ int) (verdict.Refusal, executor.CreateShapeCause) { return DestructiveChangeRefusal(), "" }) + // Whole-plan admission checks destructiveness before routed dispositions. + // Preserve each statement's specific refusal, but give the aggregate the same + // precedence as apply, including when routing already refused the report. + r := DestructiveChangeRefusal() + report.Reason, report.Class, report.Owner = r.Reason(), r.Class(), r.Owner() report.Fingerprint = Fingerprint(report.Statements) } diff --git a/pkg/plan/desired_test.go b/pkg/plan/desired_test.go index 8d8c72e..bf28514 100644 --- a/pkg/plan/desired_test.go +++ b/pkg/plan/desired_test.go @@ -61,3 +61,23 @@ func TestRefuseDestructiveLeavesNondestructivePlanUnchanged(t *testing.T) { assert.Equal(t, before, report.Statements[0]) assert.Equal(t, fingerprint, report.Fingerprint) } + +func TestRefuseDestructiveOverridesAggregateRouteRefusal(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Disposition = router.DispositionRefuse + report.Statements = []plan.Statement{{ + SQL: "ALTER TABLE documents DROP COLUMN body", Destructive: true, + Disposition: router.DispositionExecute, Backend: router.BackendNative, + }, { + SQL: "ALTER TABLE documents ADD CONSTRAINT ex EXCLUDE USING btree (id WITH =)", + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedStatement, Class: verdict.ClassCapabilityBoundary, + }} + plan.RefuseDestructive(&report) + assert.Equal(t, verdict.ReasonDestructiveChange, report.Reason) + assert.Equal(t, verdict.ClassByDesign, report.Class) + assert.Empty(t, report.Owner) + assert.Equal(t, verdict.ReasonUnsupportedStatement, report.Statements[1].Reason) + assert.Equal(t, verdict.ClassCapabilityBoundary, report.Statements[1].Class) + assert.Equal(t, plan.Fingerprint(report.Statements), report.Fingerprint) +} diff --git a/pkg/plan/refusal.go b/pkg/plan/refusal.go index 2ddb9f8..7e3c836 100644 --- a/pkg/plan/refusal.go +++ b/pkg/plan/refusal.go @@ -89,3 +89,9 @@ func DestructiveChangeRefusal() verdict.Refusal { func RowSecurityReviewRefusal() verdict.Refusal { return verdict.CapabilityBoundary(verdict.ReasonUnsupportedStatement) } + +// RowSecurityMissingTableRefusal classifies an absent RLS target in preview +// and apply. The operator must create the table before managing its policies. +func RowSecurityMissingTableRefusal() verdict.Refusal { + return verdict.Environmental(verdict.ReasonUnsupportedStatement) +} diff --git a/pkg/statement/desired_rls.go b/pkg/statement/desired_rls.go index 5544bc1..a912dc1 100644 --- a/pkg/statement/desired_rls.go +++ b/pkg/statement/desired_rls.go @@ -14,6 +14,9 @@ import ( // ErrRowSecurityDeclaration reports an incomplete or contradictory RLS declaration. var ErrRowSecurityDeclaration = errors.New("desired row security requires one explicit ENABLE or DISABLE on the desired table") +// ErrRowSecurityTableDefinitionRequired identifies a missing table definition in an RLS file. +var ErrRowSecurityTableDefinitionRequired = errors.New("desired row security requires a CREATE TABLE definition") + // ErrPolicyRelationDependency refuses policy subqueries that read relations until // desired-state materialization can preserve their dependency identities. var ErrPolicyRelationDependency = errors.New("policy relation dependencies are not supported in desired row security") @@ -62,7 +65,7 @@ func ParseDesiredWithRowSecurity(sql string) (DesiredWithRowSecurity, error) { } } if !hasTable { - return DesiredWithRowSecurity{}, fmt.Errorf("desired row security requires a CREATE TABLE definition: %w", ErrRowSecurityDeclaration) + return DesiredWithRowSecurity{}, fmt.Errorf("%w: %w", ErrRowSecurityTableDefinitionRequired, ErrRowSecurityDeclaration) } table, err := ParseDesired(tableSQL.String()) if err != nil { diff --git a/pkg/statement/desired_rls_test.go b/pkg/statement/desired_rls_test.go index 3d72735..c3518c2 100644 --- a/pkg/statement/desired_rls_test.go +++ b/pkg/statement/desired_rls_test.go @@ -116,5 +116,5 @@ func TestDesiredRowSecurityRequiresTableDefinition(t *testing.T) { _, err := ParseDesiredWithRowSecurity(`ALTER TABLE documents ENABLE ROW LEVEL SECURITY; CREATE POLICY readers ON documents FOR SELECT USING (true);`) require.ErrorIs(t, err, ErrRowSecurityDeclaration) - assert.Contains(t, err.Error(), "CREATE TABLE") + require.ErrorIs(t, err, ErrRowSecurityTableDefinitionRequired) }