diff --git a/skill/SKILL.md b/skill/SKILL.md index 5ea2d76..1784d25 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -89,6 +89,15 @@ single DML statement), then **rolls back** — nothing changes. Preview first, show the human the affected count/sample, then re-run with `--commit` to apply. `--commit` is the only flag that writes; you can't forget to preview. +The statement executes **exactly once** under both flags. A dry run is a real +trial run, so a statement that *can't* succeed (unique/constraint violation, +type error) **fails the preview** — exit `10` with `ROLLED BACK — nothing +changed`, not a clean `DRY RUN` report. That's a feature: the preview tells you +the write would have failed before you reach `--commit`. Likewise a `--commit` +whose statement aborts is reported honestly — exit `10`, `ROLLED BACK`, never a +false `COMMITTED` — including deferred-constraint violations that only surface at +commit time. + Writes that hide inside a query are caught, not just bare `INSERT`/`UPDATE`/ `DELETE`: writable CTEs (`WITH d AS (DELETE … RETURNING *) SELECT * FROM d`), leading-CTE DML, `SELECT … INTO new_table`, and `EXPLAIN ANALYZE ` (which diff --git a/src/commands/sql_cmd.rs b/src/commands/sql_cmd.rs index d6cdeb9..1fba21d 100644 --- a/src/commands/sql_cmd.rs +++ b/src/commands/sql_cmd.rs @@ -247,18 +247,31 @@ async fn run_write( .await .context("begin transaction")?; - // From here on, ensure we always close the transaction even on error. + // From here on, ensure we always close the transaction even on error. Any + // error inside the transaction must roll back and propagate loudly — a + // swallowed abort that still reports success is the worst failure for a + // trust tool (see PGC-104). let outcome = execute_in_tx(client, sql, parsed, opts).await; let (results, rows_affected) = match outcome { Ok(v) => v, Err(e) => { let _ = client.batch_execute("ROLLBACK").await; - return Err(e); + return Err(rolled_back(e)); } }; if opts.commit { + // Force any DEFERRED constraint checks to run now, while we can still + // observe a violation as an error. Without this, a deferred violation + // surfaces only at COMMIT, where Postgres silently downgrades COMMIT to + // ROLLBACK and returns the `ROLLBACK` tag with no protocol error — the + // exact shape that let an aborted transaction report COMMITTED. + if let Err(e) = client.simple_query("SET CONSTRAINTS ALL IMMEDIATE").await { + let _ = client.batch_execute("ROLLBACK").await; + return Err(rolled_back(anyhow::Error::new(e))); + } + client .batch_execute("COMMIT") .await @@ -275,62 +288,131 @@ async fn run_write( Ok(()) } -/// Run the write inside the open transaction, gathering command tags and (when -/// safe) a sample of affected rows via a RETURNING wrapper. +/// Wrap a transaction error in the explicit, human-facing "ROLLED BACK — +/// nothing changed" framing. The chained source carries the original DB error; +/// `main` propagates this to exit code 10 for both human and JSON output. +fn rolled_back(e: anyhow::Error) -> anyhow::Error { + e.context("ROLLED BACK — nothing changed") +} + +/// Run the write inside the open transaction **exactly once**, gathering the +/// affected count and (when safe) a sample of affected rows. +/// +/// The single-DML-without-RETURNING case is the dangerous one: previously it +/// ran the bare statement for the count *and* a RETURNING-wrapped copy for the +/// sample, double-applying every effect in the same transaction (PGC-104). Now +/// the wrap is the *only* execution — it yields both count and sample in one +/// pass — so each user statement executes once. async fn execute_in_tx( client: &Client, sql: &str, parsed: &ParsedSql, opts: &SqlOptions, ) -> Result<(Vec, u64)> { - let messages = client.simple_query(sql).await.context("execute SQL")?; - let mut results = collect_results(messages, usize::MAX); - let rows_affected = total_affected(&results); - - // Sample rows only when it's safe and worthwhile: a single DML statement - // without its own RETURNING. We wrap it so the rows would-be-affected can - // be shown. The wrapper still rolls back with the outer transaction. - if parsed.single_dml_no_returning && rows_affected > 0 { - if let Some(sample) = sample_affected(client, sql, opts).await? { - results.push(sample); - } + // Single DML without its own RETURNING: one execution that returns both the + // true affected count and a bounded sample of affected rows. + if parsed.single_dml_no_returning { + return execute_dml_with_sample(client, sql, opts).await; } + // Everything else (DML with RETURNING, DDL, multi-statement, writable CTEs): + // one plain execution. No wrapping, no second pass. + let messages = client.simple_query(sql).await.context("execute SQL")?; + let results = collect_results(messages, usize::MAX); + let rows_affected = total_affected(&results); Ok((results, rows_affected)) } -/// Wrap a single DML statement to capture a few affected rows for preview. -/// Returns `None` (silently) if wrapping fails — sampling is best-effort. -async fn sample_affected( +/// Execute a single INSERT/UPDATE/DELETE (no RETURNING) exactly once, deriving +/// the affected count and a bounded sample from one CTE-wrapped query. +/// +/// `WITH x AS ( RETURNING *) SELECT (SELECT count(*) FROM x) AS affected, +/// (SELECT json_agg(t) FROM (SELECT * FROM x LIMIT N) t) AS samples` +/// +/// The statement runs once inside the writable CTE; `count(*)` over `x` gives +/// the exact affected count and `json_agg` over a bounded slice gives the +/// sample. Any DB error (e.g. a unique violation) propagates from this one +/// call, so an aborted transaction can never be reported as COMMITTED. +async fn execute_dml_with_sample( client: &Client, sql: &str, opts: &SqlOptions, -) -> Result> { +) -> Result<(Vec, u64)> { let stmt = sql.trim().trim_end_matches(';'); let limit = match opts.limit { Some(0) => DRY_RUN_SAMPLE_LIMIT, // uncapped reads still bound the preview Some(n) => n.min(DRY_RUN_SAMPLE_LIMIT), None => DRY_RUN_SAMPLE_LIMIT, }; + // `row_to_json` (text `json`, not `jsonb`) preserves the statement's column + // order; `jsonb` would re-sort keys and scramble the preview. `json_agg` + // collects the slice; the keys array carries the column order explicitly so + // Rust never has to infer it from a sorted map. let wrapped = format!( - "WITH __pgcrate_preview AS ({stmt} RETURNING *) \ - SELECT * FROM __pgcrate_preview LIMIT {limit}" + "WITH __pgcrate_w AS ({stmt} RETURNING *), \ + __pgcrate_s AS (SELECT * FROM __pgcrate_w LIMIT {limit}), \ + __pgcrate_first AS (SELECT * FROM __pgcrate_s LIMIT 1) \ + SELECT (SELECT count(*) FROM __pgcrate_w) AS __pgcrate_affected, \ + (SELECT json_agg(row_to_json(__s)) FROM __pgcrate_s __s) AS __pgcrate_samples, \ + (SELECT array_agg(k) FROM ( \ + SELECT json_object_keys(row_to_json(__f)) AS k \ + FROM __pgcrate_first __f \ + ) __k) AS __pgcrate_columns" ); - match client.simple_query(&wrapped).await { - Ok(messages) => { - let collected = collect_results(messages, limit); - // The wrapped statement produces exactly one query result. - let found = collected.into_iter().find_map(|r| match r { - SqlResult::Query { columns, rows, .. } => Some((columns, rows)), - _ => None, - }); - Ok(found.map(|(columns, rows)| SqlResult::Sample { columns, rows })) - } - // Wrapping isn't always valid (e.g. statement already errors, or a - // construct RETURNING can't express). Fall back to count-only. - Err(_) => Ok(None), + let row = client + .query_one(&wrapped, &[]) + .await + .context("execute SQL")?; + + let affected: i64 = row.get("__pgcrate_affected"); + let rows_affected = affected.max(0) as u64; + + let columns: Option> = row.get("__pgcrate_columns"); + + let mut results = vec![SqlResult::CommandComplete { + rows: rows_affected, + }]; + if let Some(sample) = sample_from_json(row.get("__pgcrate_samples"), columns) { + results.push(sample); } + Ok((results, rows_affected)) +} + +/// Turn the `json_agg(row_to_json(...))` sample column into a `Sample` result. +/// `columns` carries the statement's column order (from `array_agg` of +/// `json_object_keys`); cells are stringified to match the simple-query text +/// representation used elsewhere. +fn sample_from_json( + samples: Option, + columns: Option>, +) -> Option { + let rows = samples?.as_array()?.clone(); + // Prefer the SQL-provided order; fall back to the first row's keys only if + // it's somehow absent (e.g. all-NULL column array). + let columns = columns + .filter(|c| !c.is_empty()) + .or_else(|| Some(rows.first()?.as_object()?.keys().cloned().collect()))?; + + let table_rows: Vec>> = rows + .iter() + .filter_map(|r| r.as_object()) + .map(|obj| { + columns + .iter() + .map(|col| match obj.get(col) { + None | Some(serde_json::Value::Null) => None, + Some(serde_json::Value::String(s)) => Some(s.clone()), + Some(other) => Some(other.to_string()), + }) + .collect() + }) + .collect(); + + Some(SqlResult::Sample { + columns, + rows: table_rows, + }) } /// EXPLAIN the input and return the estimated total cost (relative to the diff --git a/tests/commands/sql.rs b/tests/commands/sql.rs index 1442adc..7816a41 100644 --- a/tests/commands/sql.rs +++ b/tests/commands/sql.rs @@ -500,6 +500,357 @@ fn test_sql_select_into_dry_runs() { ); } +// ============================================================================ +// PGC-104: exactly-once execution + honest abort reporting +// +// These probes use NON-IDEMPOTENT statements (INSERT against a UNIQUE +// constraint). The original bug double-executed single-DML writes — once for +// the count, once for the RETURNING-wrapped sample — and swallowed the abort +// that the second execution triggered, reporting COMMITTED with zero rows +// written. An UPDATE masks this (idempotent under re-execution); an INSERT +// against a unique index does not. +// ============================================================================ + +/// `--commit` of a single INSERT must execute the statement EXACTLY ONCE. The +/// double-execution bug inserted two rows for `INSERT … SELECT 1`; here a +/// single-value insert into a unique column must leave exactly one row. +#[test] +fn test_sql_commit_inserts_exactly_once() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + + let output = project.run_pgcrate_ok(&[ + "sql", + "-c", + "INSERT INTO users (email, name) SELECT 'once@test.com', 'Once'", + "--commit", + ]); + assert!( + stdout(&output).to_uppercase().contains("COMMITTED"), + "commit should be announced: {}", + stdout(&output) + ); + + // Exactly one row — double execution would have inserted two (or aborted on + // the unique violation and falsely reported COMMITTED). + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email = 'once@test.com'"), + "1", + "single INSERT --commit must insert exactly one row" + ); +} + +/// The headline failure: a `--commit` write whose second (phantom) execution +/// hit a unique violation used to report `COMMITTED` while the transaction had +/// silently rolled back. Now any abort must propagate: exit 10, an explicit +/// "ROLLED BACK" message, never COMMITTED, and the table left untouched. +/// +/// Reproduced here by inserting a row that already exists, so the very first +/// (and only) execution violates the unique constraint. +#[test] +fn test_sql_commit_aborted_write_reports_failure_not_committed() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + db.run_sql_ok("INSERT INTO users (email, name) VALUES ('dup@test.com', 'Existing')"); + + let output = project.run_pgcrate(&[ + "sql", + "-c", + "INSERT INTO users (email, name) VALUES ('dup@test.com', 'Phantom')", + "--commit", + ]); + + // Exit 10 (operational failure), not 0. + assert_eq!( + output.status.code(), + Some(10), + "aborted commit must exit 10: stdout={}, stderr={}", + stdout(&output), + stderr(&output) + ); + let combined = format!("{}{}", stdout(&output), stderr(&output)); + assert!( + combined.to_uppercase().contains("ROLLED BACK"), + "must announce ROLLED BACK: {}", + combined + ); + assert!( + !combined.to_uppercase().contains("COMMITTED"), + "must NEVER report COMMITTED on an aborted write: {}", + combined + ); + + // The phantom row was never written and the original survives unchanged. + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email = 'dup@test.com'"), + "1", + "aborted commit must leave the table unchanged" + ); + assert_eq!( + db.query("SELECT name FROM users WHERE email = 'dup@test.com'"), + "Existing", + "the pre-existing row must be untouched" + ); +} + +/// The aborted-commit failure surfaces in JSON too: ok=false, exit 10, the +/// error message mentions the rollback, and there is no committed=true outcome. +#[test] +fn test_sql_commit_aborted_write_json_reports_failure() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + db.run_sql_ok("INSERT INTO users (email, name) VALUES ('dupj@test.com', 'Existing')"); + + let output = project.run_pgcrate(&[ + "sql", + "-c", + "INSERT INTO users (email, name) VALUES ('dupj@test.com', 'Phantom')", + "--commit", + "--json", + ]); + assert_eq!( + output.status.code(), + Some(10), + "aborted commit (json) must exit 10: {}", + stdout(&output) + ); + + let json = parse_json(&output); + assert_eq!( + json.get("ok").and_then(|v| v.as_bool()), + Some(false), + "json must report ok=false on an aborted write: {}", + json + ); + let blob = serde_json::to_string(&json).unwrap().to_uppercase(); + assert!( + blob.contains("ROLLED BACK"), + "json error must mention ROLLED BACK: {}", + json + ); + + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email = 'dupj@test.com'"), + "1", + "aborted commit must leave the table unchanged" + ); +} + +/// A `--commit` insert that affects MANY rows must still execute once and +/// commit exactly that many — the count comes from the same single execution as +/// the sample, not a separate pass. Mirrors the solitaire repro shape (a bulk +/// `INSERT … SELECT` into a uniquely-indexed table). +#[test] +fn test_sql_commit_bulk_insert_executes_once() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + + // 50 unique emails in one INSERT … SELECT. + let output = project.run_pgcrate_ok(&[ + "sql", + "-c", + "INSERT INTO users (email, name) \ + SELECT 'bulk' || g || '@test.com', 'B' FROM generate_series(1, 50) g", + "--commit", + ]); + assert!( + stdout(&output).contains("50 row(s) affected"), + "commit should report 50 rows affected: {}", + stdout(&output) + ); + + // Exactly 50 rows — a double execution would have hit the unique constraint + // on the second pass and aborted (which must now report failure, not 100 + // rows or a false COMMITTED). + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email LIKE 'bulk%@test.com'"), + "50", + "bulk INSERT --commit must insert exactly 50 rows, once" + ); +} + +/// The dry-run (`--allow-write`) path must also execute the statement once and +/// roll back. A single INSERT previewed under --allow-write must report the +/// would-be effect and leave the table empty. +#[test] +fn test_sql_dry_run_insert_executes_once_and_rolls_back() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + + let output = project.run_pgcrate_ok(&[ + "sql", + "-c", + "INSERT INTO users (email, name) VALUES ('preview@test.com', 'Preview')", + "--allow-write", + ]); + let out = stdout(&output); + assert!( + out.to_uppercase().contains("DRY RUN"), + "dry run should be announced: {}", + out + ); + assert!( + out.contains("1 row(s) would be affected"), + "dry run should report one would-be row: {}", + out + ); + + // Rolled back — nothing persisted. + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email = 'preview@test.com'"), + "0", + "dry-run INSERT must roll back (no row persisted)" + ); +} + +/// A single INSERT (no RETURNING) under --allow-write must still surface a +/// sample of the would-be-affected rows in JSON, derived from the SAME single +/// execution that produced the count (not a second wrapped run). +#[test] +fn test_sql_dry_run_insert_json_sample_from_single_exec() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + + let output = project.run_pgcrate(&[ + "sql", + "-c", + "INSERT INTO users (email, name) VALUES ('sample@test.com', 'Sampled')", + "--allow-write", + "--json", + ]); + assert!(output.status.success(), "dry-run JSON should succeed"); + + let json = parse_json(&output); + let write = json.get("write").expect("write outcome present"); + assert_eq!( + write.get("committed").and_then(|v| v.as_bool()), + Some(false), + "dry run must report committed=false: {}", + json + ); + assert_eq!( + write.get("rows_affected").and_then(|v| v.as_u64()), + Some(1), + "dry run must report rows_affected=1: {}", + json + ); + + // The sample carries the would-be values (email + name). + let has_sample = json + .get("results") + .and_then(|r| r.as_array()) + .map(|arr| { + arr.iter().any(|r| { + r.get("type").and_then(|t| t.as_str()) == Some("sample") + && serde_json::to_string(r) + .unwrap() + .contains("sample@test.com") + }) + }) + .unwrap_or(false); + assert!( + has_sample, + "dry-run INSERT should include a sample row with the inserted values: {}", + json + ); + + // And nothing was persisted. + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email = 'sample@test.com'"), + "0", + "dry-run INSERT must not persist" + ); +} + +/// A DML statement that carries its OWN RETURNING goes through the plain +/// single-execution path (no CTE wrap). It too must execute exactly once and +/// report honestly under --commit. +#[test] +fn test_sql_commit_insert_with_returning_executes_once() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + + let output = project.run_pgcrate_ok(&[ + "sql", + "-c", + "INSERT INTO users (email, name) VALUES ('ret@test.com', 'Ret') RETURNING id", + "--commit", + ]); + assert!( + stdout(&output).to_uppercase().contains("COMMITTED"), + "commit should be announced: {}", + stdout(&output) + ); + assert_eq!( + db.query("SELECT count(*) FROM users WHERE email = 'ret@test.com'"), + "1", + "INSERT … RETURNING --commit must insert exactly one row" + ); +} + +/// The dry-run sample must present columns in the statement's natural order +/// (table-definition order for `INSERT … RETURNING *`), not alphabetized. The +/// CTE wrap derives the sample from `row_to_json` (order-preserving text JSON) +/// plus an explicit column-name array; a regression to `to_jsonb`/sorted-map +/// key inference would scramble this to `email, id, is_admin, name`. +#[test] +fn test_sql_dry_run_sample_preserves_column_order() { + skip_if_no_db!(); + let db = TestDatabase::new(); + let project = TestProject::from_fixture("with_migrations", &db); + project.run_pgcrate_ok(&["migrate", "up"]); + + let output = project.run_pgcrate(&[ + "sql", + "-c", + "INSERT INTO users (email, name, is_admin) VALUES ('order@test.com', 'Ord', true)", + "--allow-write", + "--json", + ]); + assert!(output.status.success(), "dry-run JSON should succeed"); + + let json = parse_json(&output); + let sample = json + .get("results") + .and_then(|r| r.as_array()) + .and_then(|arr| { + arr.iter() + .find(|r| r.get("type").and_then(|t| t.as_str()) == Some("sample")) + }) + .expect("sample result present"); + + let columns: Vec<&str> = sample + .get("columns") + .and_then(|c| c.as_array()) + .expect("sample columns present") + .iter() + .map(|c| c.as_str().unwrap()) + .collect(); + + // users is defined as (id, email, name, is_admin, created_at); RETURNING * + // yields that exact order — not the alphabetical email/id/is_admin/name. + assert_eq!( + columns, + vec!["id", "email", "name", "is_admin", "created_at"], + "sample columns must follow table-definition order: {}", + json + ); +} + // ============================================================================ // Row caps // ============================================================================