From aadb8468dc583b6448795afbc1d62fa071152578 Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 11 Sep 2026 15:46:08 +0200 Subject: [PATCH 1/2] fix: CREATE TABLE IF NOT EXISTS ignores its guard -- duplicate sqlite_master row + leaked page (#697) CREATE TABLE/INDEX/VIEW IF NOT EXISTS and DROP TABLE/INDEX IF EXISTS parsed their guard flag but compile_statement never consulted it: a second CREATE TABLE IF NOT EXISTS against an existing table appended a second sqlite_master row and allocated (then abandoned) a root page, which stock sqlite3 reports as "Page N: never used" -- corrupting a database this crate itself created on the very next startup, since "IF NOT EXISTS on catalog bootstrap" is the idiom's whole purpose. Also: a duplicate CREATE TABLE without the guard silently succeeded too, which it must not. compile_statement now checks schemas/views (or the index list, for CREATE/DROP INDEX) before emitting: a satisfied guard compiles to a new compile_noop() (Init -> Halt, no page allocated, no schema-cookie bump); an unsatisfied guard on a CREATE still errors, matching the oracle's wording ("table t already exists" / "index i already exists" / "view v already exists"); DROP ... IF EXISTS on a missing object is now the same no-op instead of propagating NoSuchTable/NoSuchIndex. Refs: 010/Req-8, #678, #695 --- .../specs/010-vdbe-write-opcodes/spec.md | 15 ++ CHANGELOG.md | 14 ++ src/codegen/dispatch.rs | 117 ++++++++-- tests/corpus/ddl_guard_test.rs | 216 ++++++++++++++++++ tests/corpus/main.rs | 1 + 5 files changed, 345 insertions(+), 18 deletions(-) create mode 100644 tests/corpus/ddl_guard_test.rs diff --git a/.openspec/specs/010-vdbe-write-opcodes/spec.md b/.openspec/specs/010-vdbe-write-opcodes/spec.md index df79c43c..6c0f06b3 100644 --- a/.openspec/specs/010-vdbe-write-opcodes/spec.md +++ b/.openspec/specs/010-vdbe-write-opcodes/spec.md @@ -445,6 +445,21 @@ by `src/codegen/stmt/insert.rs::emit_unique_check` **Tests:** `tests/corpus/create_table_autoindex_test.rs::rowid_alias_primary_key_gains_no_autoindex`, `tests/corpus/create_table_autoindex_test.rs::without_rowid_primary_key_gains_no_autoindex` +#### Scenario: A guarded CREATE/DROP is a clean no-op when its guard condition already holds + +- GIVEN a table (or index, or view) already registered in `sqlite_master` +- WHEN `CREATE TABLE IF NOT EXISTS`/`CREATE INDEX IF NOT EXISTS`/`CREATE VIEW + IF NOT EXISTS` names it again, or `DROP TABLE IF EXISTS`/`DROP INDEX IF + EXISTS` names an object that does not exist +- THEN the statement succeeds (rc 0) without allocating a page, writing a + second `sqlite_master` row, or bumping the schema cookie — the oracle's + `PRAGMA integrity_check` and page count are unchanged, and without the + guard the same duplicate create still fails with the oracle's own wording + (`table t already exists`, `index i already exists`, `view v already + exists`) + +**Tests:** `tests/corpus/ddl_guard_test.rs::create_table_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_table_if_not_exists_twice_without_a_constraint_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_table_without_guard_still_fails_on_a_duplicate`, `tests/corpus/ddl_guard_test.rs::create_index_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_view_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::drop_table_if_exists_on_a_missing_table_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::drop_index_if_exists_on_a_missing_index_is_a_clean_no_op` + ## Related regimes - Tier suite: `tests/tiers/tier2.rs::t2_crud_round_trips_on_rowid_tables` diff --git a/CHANGELOG.md b/CHANGELOG.md index 36cb57ff..da2b5da0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,20 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep no number. `btree::MasterEntry::sql` is now `Option` so a `NULL` `sql` column can be represented at all, rather than an empty string (#687). +- `CREATE TABLE IF NOT EXISTS`/`CREATE INDEX IF NOT EXISTS`/`CREATE VIEW + IF NOT EXISTS` and `DROP TABLE IF EXISTS`/`DROP INDEX IF EXISTS` + parsed their guard but never consulted it before emitting: a second + `CREATE TABLE IF NOT EXISTS` against an existing table appended a + second `sqlite_master` row and leaked its root page, which stock + `sqlite3` then reported as `Page N: never used` (plus a stale + autoindex entry-count mismatch for a composite key) — corrupting a + database this crate itself created on its second run, the exact + startup idiom `IF NOT EXISTS` exists for. `compile_statement` now + checks the catalog first: a satisfied guard compiles to a no-op + (`Init -> Halt`, no page allocated, no schema-cookie bump); an + unguarded duplicate create still fails, matching the oracle's + wording (`table t already exists`, `index i already exists`, `view v + already exists`) (#697). ## [0.18.10] - 2026-08-31 diff --git a/src/codegen/dispatch.rs b/src/codegen/dispatch.rs index d9c7f3ed..b1451fa6 100644 --- a/src/codegen/dispatch.rs +++ b/src/codegen/dispatch.rs @@ -14,13 +14,14 @@ use crate::parser::error::{ parse_pragma, parse_rollback, parse_update, }; use crate::schema::{TableSchema, ViewSchema}; -use crate::vdbe::Program; +use crate::vdbe::{Instruction, Opcode, Program}; use super::{ compile_analyze, compile_begin, compile_commit, compile_create_index, compile_create_table, compile_create_view, compile_delete_with_catalog, compile_drop_index, compile_drop_table, compile_insert, compile_pragma, compile_rollback, compile_update_with_catalog, - expand_with_clause, resolve_from_table_schema, resolve_views, CodegenError, ExpandViews, + expand_with_clause, resolve_from_table_schema, resolve_views, CodegenError, Emitter, + ExpandViews, }; /// Failure compiling one dispatched statement — everything @@ -35,6 +36,18 @@ pub enum DispatchError { /// The statement referenced an index not present in the schema catalog. NoSuchIndex(String), + /// `CREATE TABLE` (without `IF NOT EXISTS`) named a table that already + /// exists (#697). + TableAlreadyExists(String), + + /// `CREATE INDEX` (without `IF NOT EXISTS`) named an index that already + /// exists (#697). + IndexAlreadyExists(String), + + /// `CREATE VIEW` (without `IF NOT EXISTS`) named a view that already + /// exists (#697). + ViewAlreadyExists(String), + /// The leading keyword(s) didn't match any statement kind this /// dispatcher knows how to parse/compile. Unrecognized(String), @@ -54,6 +67,9 @@ impl std::fmt::Display for DispatchError { match self { DispatchError::NoSuchTable(name) => write!(f, "no such table: {name}"), DispatchError::NoSuchIndex(name) => write!(f, "no such index: {name}"), + DispatchError::TableAlreadyExists(name) => write!(f, "table {name} already exists"), + DispatchError::IndexAlreadyExists(name) => write!(f, "index {name} already exists"), + DispatchError::ViewAlreadyExists(name) => write!(f, "view {name} already exists"), DispatchError::Unrecognized(kw) => { write!(f, "unsupported or unrecognized statement: {kw:?} ...") } @@ -116,6 +132,22 @@ fn parse_error(other: ParseOutcome) -> DispatchError { DispatchError::ParseFailed(format!("{other:?}")) } +/// A statement that does nothing: `Init -> Halt`, no other opcode. What a +/// guarded `CREATE ... IF NOT EXISTS`/`DROP ... IF EXISTS` compiles to when +/// its guard condition is already satisfied — the oracle reports success +/// (rc 0) and touches neither the schema nor any b-tree page, so this must +/// not allocate a page, write a `sqlite_master` row, or bump the schema +/// cookie either (#697). +fn compile_noop() -> Program { + let mut em = Emitter::new(); + let init_addr = em.emit(Instruction::new(Opcode::Init, 0, 0, 0)); + let body_start = em.new_label(); + em.place(body_start); + em.patch_p2(init_addr, body_start); + em.emit(Instruction::new(Opcode::Halt, 0, 0, 0)); + em.finish() +} + /// Parses `sql`, picks the compiler for its leading keyword(s), and /// compiles it against `schemas` — the `exec ""` CLI /// subcommand's core (#215's write-path CLI surface), shared by any @@ -133,15 +165,6 @@ pub fn compile_statement( .find(|s| s.name.eq_ignore_ascii_case(name)) .ok_or_else(|| DispatchError::NoSuchTable(name.to_string())) }; - let find_index_root = |name: &str| -> Result { - schemas - .iter() - .flat_map(|s| &s.indexes) - .find(|idx| idx.name.eq_ignore_ascii_case(name)) - .map(|idx| idx.root_page) - .ok_or_else(|| DispatchError::NoSuchIndex(name.to_string())) - }; - let mut words = sql.split_whitespace(); let first_word = words.next().unwrap_or(""); let head = canonical(first_word); @@ -268,31 +291,89 @@ pub fn compile_statement( other => Err(parse_error(other)), }, "CREATE" if second == "TABLE" => match parse_create_table(sql) { - ParseOutcome::Accepted(create) => Ok(compile_create_table(&create, sql)?), + ParseOutcome::Accepted(create) => { + let exists = schemas + .iter() + .any(|s| s.name.eq_ignore_ascii_case(&create.name)) + || views + .iter() + .any(|v| v.name.eq_ignore_ascii_case(&create.name)); + if exists { + if create.if_not_exists { + Ok(compile_noop()) + } else { + Err(DispatchError::TableAlreadyExists(create.name)) + } + } else { + Ok(compile_create_table(&create, sql)?) + } + } other => Err(parse_error(other)), }, "CREATE" if second == "VIEW" => match parse_create_view(sql) { - ParseOutcome::Accepted(create) => Ok(compile_create_view(&create, sql)?), + ParseOutcome::Accepted(create) => { + let exists = schemas + .iter() + .any(|s| s.name.eq_ignore_ascii_case(&create.name)) + || views + .iter() + .any(|v| v.name.eq_ignore_ascii_case(&create.name)); + if exists { + if create.if_not_exists { + Ok(compile_noop()) + } else { + Err(DispatchError::ViewAlreadyExists(create.name)) + } + } else { + Ok(compile_create_view(&create, sql)?) + } + } other => Err(parse_error(other)), }, "CREATE" if second == "INDEX" || second == "UNIQUE" => match parse_create_index(sql) { ParseOutcome::Accepted(ci) => { let schema = find_schema(&ci.table)?; - Ok(compile_create_index(&ci, schema, sql)?) + let exists = schema + .indexes + .iter() + .any(|idx| idx.name.eq_ignore_ascii_case(&ci.name)); + if exists { + if ci.if_not_exists { + Ok(compile_noop()) + } else { + Err(DispatchError::IndexAlreadyExists(ci.name)) + } + } else { + Ok(compile_create_index(&ci, schema, sql)?) + } } other => Err(parse_error(other)), }, "DROP" if second == "TABLE" => match parse_drop_table(sql) { ParseOutcome::Accepted(drop) => { - let schema = find_schema(&drop.name)?; - Ok(compile_drop_table(&drop, schema)?) + match schemas + .iter() + .find(|s| s.name.eq_ignore_ascii_case(&drop.name)) + { + Some(schema) => Ok(compile_drop_table(&drop, schema)?), + None if drop.if_exists => Ok(compile_noop()), + None => Err(DispatchError::NoSuchTable(drop.name)), + } } other => Err(parse_error(other)), }, "DROP" if second == "INDEX" => match parse_drop_index(sql) { ParseOutcome::Accepted(di) => { - let root_page = find_index_root(&di.name)?; - Ok(compile_drop_index(&di, root_page)?) + let existing_root = schemas + .iter() + .flat_map(|s| &s.indexes) + .find(|idx| idx.name.eq_ignore_ascii_case(&di.name)) + .map(|idx| idx.root_page); + match existing_root { + Some(root_page) => Ok(compile_drop_index(&di, root_page)?), + None if di.if_exists => Ok(compile_noop()), + None => Err(DispatchError::NoSuchIndex(di.name)), + } } other => Err(parse_error(other)), }, diff --git a/tests/corpus/ddl_guard_test.rs b/tests/corpus/ddl_guard_test.rs new file mode 100644 index 00000000..f1cb023f --- /dev/null +++ b/tests/corpus/ddl_guard_test.rs @@ -0,0 +1,216 @@ +// Copyright 2026 Schuberg Philis +// SPDX-License-Identifier: Apache-2.0 +//! #697 acceptance: `CREATE ... IF NOT EXISTS` / `DROP ... IF EXISTS` +//! guards were parsed but never consulted before emission — a second +//! `CREATE TABLE IF NOT EXISTS` against an existing table appended a +//! second `sqlite_master` row and leaked its root page, corrupting the +//! file (`PRAGMA integrity_check` then reports `Page N: never used` and, +//! for a composite key, `wrong # of entries in index sqlite_autoindex_*` +//! on top). `CREATE TABLE IF NOT EXISTS` is the idiom SQE's catalog +//! bootstrap runs on every startup, so this corrupted a self-created +//! database on its second run. +//! +//! Covers all three `CREATE ... IF NOT EXISTS` forms, both `DROP ... +//! IF EXISTS` forms, and the mirror claim that dropping/creating +//! *without* the guard still behaves like stock SQLite (fails on a +//! duplicate create, fails on a missing drop). + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::oracle::{assert_integrity_check_ok, pinned_oracle, skip_no_oracle}; + +const CLI: &str = env!("CARGO_BIN_EXE_sqlite-rs"); + +fn scratch_db(label: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "sqlite-rs-ddl-guard-{label}-{}-{n}", + std::process::id() + )); + std::fs::remove_dir_all(&dir).ok(); + std::fs::create_dir_all(&dir).unwrap(); + dir.join("scratch.db") +} + +fn run_exec(db: &Path, sql: &str) -> Output { + Command::new(CLI) + .arg("exec") + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running {CLI} exec {} {sql:?}: {e}", db.display())) +} + +fn exec_ok(db: &Path, sql: &str) { + let output = run_exec(db, sql); + assert!( + output.status.success(), + "exec {sql:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn oracle_scalar(oracle: &Path, db: &Path, sql: &str) -> String { + let output = Command::new(oracle) + .arg(db) + .arg(sql) + .output() + .unwrap_or_else(|e| panic!("running oracle on {}: {e}", db.display())); + assert!( + output.status.success(), + "oracle query {sql:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).trim().to_string() +} + +/// `CREATE TABLE IF NOT EXISTS` run twice: one `sqlite_master` row, no +/// error, no leaked page, prior data intact, and the oracle still +/// considers the file sound. The issue's headline composite-PK repro. +#[test] +fn create_table_if_not_exists_twice_is_a_clean_no_op() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("ddl_guard"); + return; + }; + let db = scratch_db("create-table-twice"); + let ddl = "CREATE TABLE IF NOT EXISTS t (a TEXT, b TEXT, PRIMARY KEY (a, b))"; + exec_ok(&db, ddl); + exec_ok(&db, "INSERT INTO t VALUES ('x', 'y')"); + let page_count_before = oracle_scalar(&oracle, &db, "PRAGMA page_count"); + + exec_ok(&db, ddl); + + let table_rows = oracle_scalar( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE type = 'table' AND name = 't'", + ); + assert_eq!(table_rows, "1", "expected exactly one sqlite_master row"); + let page_count_after = oracle_scalar(&oracle, &db, "PRAGMA page_count"); + assert_eq!( + page_count_before, page_count_after, + "second no-op create must not leak a page" + ); + assert_integrity_check_ok(&oracle, &db); + let row_count = oracle_scalar(&oracle, &db, "SELECT count(*) FROM t"); + assert_eq!(row_count, "1", "data between the two runs must survive"); +} + +/// The single-column, no-constraint shape from the issue's isolation +/// table: same clean no-op, no composite key involved. +#[test] +fn create_table_if_not_exists_twice_without_a_constraint_is_a_clean_no_op() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("ddl_guard"); + return; + }; + let db = scratch_db("create-table-twice-simple"); + let ddl = "CREATE TABLE IF NOT EXISTS n (a TEXT)"; + exec_ok(&db, ddl); + exec_ok(&db, ddl); + let table_rows = oracle_scalar( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE type = 'table' AND name = 'n'", + ); + assert_eq!(table_rows, "1"); + assert_integrity_check_ok(&oracle, &db); +} + +/// Without the guard, a duplicate `CREATE TABLE` must still fail, with +/// the oracle's own wording. +#[test] +fn create_table_without_guard_still_fails_on_a_duplicate() { + let db = scratch_db("create-table-no-guard"); + exec_ok(&db, "CREATE TABLE t (a TEXT)"); + let output = run_exec(&db, "CREATE TABLE t (a TEXT)"); + assert!( + !output.status.success(), + "expected a duplicate CREATE TABLE to fail" + ); + assert!( + String::from_utf8_lossy(&output.stderr).contains("table t already exists"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// `CREATE INDEX IF NOT EXISTS` run twice is a clean no-op; without the +/// guard it fails on the duplicate. +#[test] +fn create_index_if_not_exists_twice_is_a_clean_no_op() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("ddl_guard"); + return; + }; + let db = scratch_db("create-index-twice"); + exec_ok(&db, "CREATE TABLE t (a TEXT)"); + let ddl = "CREATE INDEX IF NOT EXISTS i ON t (a)"; + exec_ok(&db, ddl); + exec_ok(&db, ddl); + let index_rows = oracle_scalar( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE type = 'index' AND name = 'i'", + ); + assert_eq!(index_rows, "1"); + assert_integrity_check_ok(&oracle, &db); + + let output = run_exec(&db, "CREATE INDEX i ON t (a)"); + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stderr).contains("index i already exists")); +} + +/// `CREATE VIEW IF NOT EXISTS` run twice is a clean no-op; without the +/// guard it fails on the duplicate. +#[test] +fn create_view_if_not_exists_twice_is_a_clean_no_op() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("ddl_guard"); + return; + }; + let db = scratch_db("create-view-twice"); + exec_ok(&db, "CREATE TABLE t (a TEXT)"); + let ddl = "CREATE VIEW IF NOT EXISTS v AS SELECT * FROM t"; + exec_ok(&db, ddl); + exec_ok(&db, ddl); + let view_rows = oracle_scalar( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE type = 'view' AND name = 'v'", + ); + assert_eq!(view_rows, "1"); + assert_integrity_check_ok(&oracle, &db); + + let output = run_exec(&db, "CREATE VIEW v AS SELECT * FROM t"); + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stderr).contains("view v already exists")); +} + +/// `DROP TABLE IF EXISTS` on a table that was never there is a clean +/// no-op (rc 0); without the guard it still fails as before. +#[test] +fn drop_table_if_exists_on_a_missing_table_is_a_clean_no_op() { + let db = scratch_db("drop-table-missing"); + exec_ok(&db, "DROP TABLE IF EXISTS nope"); + + let output = run_exec(&db, "DROP TABLE nope"); + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stderr).contains("no such table")); +} + +/// `DROP INDEX IF EXISTS` on an index that was never there is a clean +/// no-op (rc 0); without the guard it still fails as before. +#[test] +fn drop_index_if_exists_on_a_missing_index_is_a_clean_no_op() { + let db = scratch_db("drop-index-missing"); + exec_ok(&db, "DROP INDEX IF EXISTS nope"); + + let output = run_exec(&db, "DROP INDEX nope"); + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stderr).contains("no such index")); +} diff --git a/tests/corpus/main.rs b/tests/corpus/main.rs index c9049478..42518650 100644 --- a/tests/corpus/main.rs +++ b/tests/corpus/main.rs @@ -28,6 +28,7 @@ mod cli_write_test; mod crash_torture_test; mod create_table_autoindex_test; mod cte_test; +mod ddl_guard_test; mod declared_collate_test; mod dump_oracle_test; mod expr_vectors_test; From 53f1dc68a0375c8498f1344c70a7329fd5ea58e8 Mon Sep 17 00:00:00 2001 From: Diederik Siderius Date: Fri, 11 Sep 2026 17:31:51 +0200 Subject: [PATCH 2/2] fix: an already-exists error names the existing object's kind (#697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #717's catalog existence checks named the kind of the *statement* (CREATE TABLE/VIEW/INDEX) rather than the kind of the *existing* object, so a cross-kind clash reported the wrong noun in both directions (tables and views share a namespace). It now names the existing object's kind, oracle-matched against 3.53.4 for all four table/view directions. Also fixes the previously-missing index-namespace checks: CREATE TABLE/VIEW against an existing index now reports "there is already an index named X", and CREATE INDEX against an existing table or view reports "there is already a table named X" (the oracle's wording even when the clash is with a view) — both measured to NOT be suppressed by IF NOT EXISTS, unlike the same-kind clash it does guard. spend: matched estimate (small oracle-diff/message fix) --- .../specs/010-vdbe-write-opcodes/spec.md | 2 +- CHANGELOG.md | 11 +- src/codegen/dispatch.rs | 89 +++++++-- tests/corpus/ddl_guard_test.rs | 183 ++++++++++++++++++ 4 files changed, 264 insertions(+), 21 deletions(-) diff --git a/.openspec/specs/010-vdbe-write-opcodes/spec.md b/.openspec/specs/010-vdbe-write-opcodes/spec.md index 6c0f06b3..aeb689fa 100644 --- a/.openspec/specs/010-vdbe-write-opcodes/spec.md +++ b/.openspec/specs/010-vdbe-write-opcodes/spec.md @@ -458,7 +458,7 @@ by `src/codegen/stmt/insert.rs::emit_unique_check` (`table t already exists`, `index i already exists`, `view v already exists`) -**Tests:** `tests/corpus/ddl_guard_test.rs::create_table_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_table_if_not_exists_twice_without_a_constraint_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_table_without_guard_still_fails_on_a_duplicate`, `tests/corpus/ddl_guard_test.rs::create_index_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_view_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::drop_table_if_exists_on_a_missing_table_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::drop_index_if_exists_on_a_missing_index_is_a_clean_no_op` +**Tests:** `tests/corpus/ddl_guard_test.rs::create_table_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_table_if_not_exists_twice_without_a_constraint_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_table_without_guard_still_fails_on_a_duplicate`, `tests/corpus/ddl_guard_test.rs::create_index_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_view_if_not_exists_twice_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::drop_table_if_exists_on_a_missing_table_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::drop_index_if_exists_on_a_missing_index_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::cross_kind_table_view_clash_without_guard_names_the_existing_kind`, `tests/corpus/ddl_guard_test.rs::cross_kind_table_view_clash_with_guard_is_a_clean_no_op`, `tests/corpus/ddl_guard_test.rs::create_index_name_clash_with_table_or_view_without_guard`, `tests/corpus/ddl_guard_test.rs::create_index_if_not_exists_does_not_suppress_table_or_view_name_clash`, `tests/corpus/ddl_guard_test.rs::create_table_or_view_name_clash_with_index_without_guard`, `tests/corpus/ddl_guard_test.rs::create_table_or_view_if_not_exists_does_not_suppress_index_name_clash` ## Related regimes diff --git a/CHANGELOG.md b/CHANGELOG.md index da2b5da0..2ce9391c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,16 @@ All notable changes to sqlite-rs. Format follows [Keep a Changelog](https://keep (`Init -> Halt`, no page allocated, no schema-cookie bump); an unguarded duplicate create still fails, matching the oracle's wording (`table t already exists`, `index i already exists`, `view v - already exists`) (#697). + already exists`) (#697). Follow-up: the error named the kind of the + *statement* rather than the kind of the *existing* object, so a + cross-kind clash (tables and views share a namespace) reported the + wrong noun in both directions. It now names the existing object's + kind, and the same check covers the index namespace: `CREATE + TABLE`/`CREATE VIEW` against an existing index reports "there is + already an index named X" (guard does not suppress it — different + namespace), and `CREATE INDEX` against an existing table or view + reports "there is already a table named X" (also not suppressed), + matching the oracle verbatim in all four directions. ## [0.18.10] - 2026-08-31 diff --git a/src/codegen/dispatch.rs b/src/codegen/dispatch.rs index b1451fa6..9c1e9257 100644 --- a/src/codegen/dispatch.rs +++ b/src/codegen/dispatch.rs @@ -36,18 +36,33 @@ pub enum DispatchError { /// The statement referenced an index not present in the schema catalog. NoSuchIndex(String), - /// `CREATE TABLE` (without `IF NOT EXISTS`) named a table that already - /// exists (#697). + /// `CREATE TABLE`/`CREATE VIEW` (without `IF NOT EXISTS`) named an + /// object that already exists as a table (#697). Tables and views + /// share one namespace, so this also covers `CREATE VIEW` clashing + /// with an existing table. TableAlreadyExists(String), /// `CREATE INDEX` (without `IF NOT EXISTS`) named an index that already /// exists (#697). IndexAlreadyExists(String), - /// `CREATE VIEW` (without `IF NOT EXISTS`) named a view that already - /// exists (#697). + /// `CREATE TABLE`/`CREATE VIEW` (without `IF NOT EXISTS`) named an + /// object that already exists as a view (#697). Tables and views + /// share one namespace, so this also covers `CREATE TABLE` clashing + /// with an existing view. ViewAlreadyExists(String), + /// `CREATE TABLE`/`CREATE VIEW` named an object that already exists as + /// an index — a different namespace, so `IF NOT EXISTS` does not + /// suppress this one (oracle-measured followup to #697). + NameTakenByIndex(String), + + /// `CREATE INDEX` named an object that already exists as a table or a + /// view — a different namespace, so `IF NOT EXISTS` does not suppress + /// this one (oracle-measured followup to #697). Matches the oracle's + /// own wording, which says "table" even when the clash is with a view. + NameTakenByTable(String), + /// The leading keyword(s) didn't match any statement kind this /// dispatcher knows how to parse/compile. Unrecognized(String), @@ -70,6 +85,12 @@ impl std::fmt::Display for DispatchError { DispatchError::TableAlreadyExists(name) => write!(f, "table {name} already exists"), DispatchError::IndexAlreadyExists(name) => write!(f, "index {name} already exists"), DispatchError::ViewAlreadyExists(name) => write!(f, "view {name} already exists"), + DispatchError::NameTakenByIndex(name) => { + write!(f, "there is already an index named {name}") + } + DispatchError::NameTakenByTable(name) => { + write!(f, "there is already a table named {name}") + } DispatchError::Unrecognized(kw) => { write!(f, "unsupported or unrecognized statement: {kw:?} ...") } @@ -292,18 +313,28 @@ pub fn compile_statement( }, "CREATE" if second == "TABLE" => match parse_create_table(sql) { ParseOutcome::Accepted(create) => { - let exists = schemas + let existing_view = views + .iter() + .any(|v| v.name.eq_ignore_ascii_case(&create.name)); + let existing_table = schemas .iter() - .any(|s| s.name.eq_ignore_ascii_case(&create.name)) - || views - .iter() - .any(|v| v.name.eq_ignore_ascii_case(&create.name)); - if exists { + .any(|s| s.name.eq_ignore_ascii_case(&create.name)); + if existing_table || existing_view { if create.if_not_exists { Ok(compile_noop()) + } else if existing_view { + Err(DispatchError::ViewAlreadyExists(create.name)) } else { Err(DispatchError::TableAlreadyExists(create.name)) } + } else if schemas + .iter() + .flat_map(|s| &s.indexes) + .any(|idx| idx.name.eq_ignore_ascii_case(&create.name)) + { + // Different namespace than table/view, so `IF NOT + // EXISTS` does not suppress this one (oracle-measured). + Err(DispatchError::NameTakenByIndex(create.name)) } else { Ok(compile_create_table(&create, sql)?) } @@ -312,18 +343,28 @@ pub fn compile_statement( }, "CREATE" if second == "VIEW" => match parse_create_view(sql) { ParseOutcome::Accepted(create) => { - let exists = schemas + let existing_view = views + .iter() + .any(|v| v.name.eq_ignore_ascii_case(&create.name)); + let existing_table = schemas .iter() - .any(|s| s.name.eq_ignore_ascii_case(&create.name)) - || views - .iter() - .any(|v| v.name.eq_ignore_ascii_case(&create.name)); - if exists { + .any(|s| s.name.eq_ignore_ascii_case(&create.name)); + if existing_table || existing_view { if create.if_not_exists { Ok(compile_noop()) - } else { + } else if existing_view { Err(DispatchError::ViewAlreadyExists(create.name)) + } else { + Err(DispatchError::TableAlreadyExists(create.name)) } + } else if schemas + .iter() + .flat_map(|s| &s.indexes) + .any(|idx| idx.name.eq_ignore_ascii_case(&create.name)) + { + // Different namespace than table/view, so `IF NOT + // EXISTS` does not suppress this one (oracle-measured). + Err(DispatchError::NameTakenByIndex(create.name)) } else { Ok(compile_create_view(&create, sql)?) } @@ -333,16 +374,26 @@ pub fn compile_statement( "CREATE" if second == "INDEX" || second == "UNIQUE" => match parse_create_index(sql) { ParseOutcome::Accepted(ci) => { let schema = find_schema(&ci.table)?; - let exists = schema + let index_exists = schema .indexes .iter() .any(|idx| idx.name.eq_ignore_ascii_case(&ci.name)); - if exists { + if index_exists { if ci.if_not_exists { Ok(compile_noop()) } else { Err(DispatchError::IndexAlreadyExists(ci.name)) } + } else if schemas + .iter() + .any(|s| s.name.eq_ignore_ascii_case(&ci.name)) + || views.iter().any(|v| v.name.eq_ignore_ascii_case(&ci.name)) + { + // Different namespace than index, so `IF NOT EXISTS` + // does not suppress this one (oracle-measured). The + // oracle says "table" even when the clash is with a + // view — matched verbatim, not guessed. + Err(DispatchError::NameTakenByTable(ci.name)) } else { Ok(compile_create_index(&ci, schema, sql)?) } diff --git a/tests/corpus/ddl_guard_test.rs b/tests/corpus/ddl_guard_test.rs index f1cb023f..2ddc16a0 100644 --- a/tests/corpus/ddl_guard_test.rs +++ b/tests/corpus/ddl_guard_test.rs @@ -191,6 +191,189 @@ fn create_view_if_not_exists_twice_is_a_clean_no_op() { assert!(String::from_utf8_lossy(&output.stderr).contains("view v already exists")); } +/// Followup to #697: tables and views share one namespace, and the +/// already-exists error must name the kind of the *existing* object, not +/// the kind of the statement. Oracle-measured (3.53.4): +/// `CREATE TABLE v(a)` against an existing view `v` reports +/// "view v already exists"; `CREATE VIEW t AS ...` against an existing +/// table `t` reports "table t already exists". +#[test] +fn cross_kind_table_view_clash_without_guard_names_the_existing_kind() { + let db = scratch_db("cross-kind-no-guard"); + exec_ok(&db, "CREATE TABLE t (a)"); + exec_ok(&db, "CREATE VIEW v AS SELECT a FROM t"); + + let output = run_exec(&db, "CREATE TABLE v(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("view v already exists"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_exec(&db, "CREATE VIEW t AS SELECT 1"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("table t already exists"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// The table/view namespace clash above is the same namespace `IF NOT +/// EXISTS` guards, so (oracle-measured) the guard suppresses it in both +/// directions: a clean no-op, schema untouched. +#[test] +fn cross_kind_table_view_clash_with_guard_is_a_clean_no_op() { + let Some(oracle) = pinned_oracle() else { + skip_no_oracle("ddl_guard"); + return; + }; + let db = scratch_db("cross-kind-guard"); + exec_ok(&db, "CREATE TABLE t (a)"); + exec_ok(&db, "CREATE VIEW v AS SELECT a FROM t"); + + exec_ok(&db, "CREATE TABLE IF NOT EXISTS v(a)"); + let view_rows = oracle_scalar( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE name = 'v'", + ); + assert_eq!( + view_rows, "1", + "guarded cross-kind create must not add a row" + ); + let view_type = oracle_scalar( + &oracle, + &db, + "SELECT type FROM sqlite_master WHERE name = 'v'", + ); + assert_eq!(view_type, "view", "v must still be a view, not a table"); + + exec_ok(&db, "CREATE VIEW IF NOT EXISTS t AS SELECT 1"); + let table_rows = oracle_scalar( + &oracle, + &db, + "SELECT count(*) FROM sqlite_master WHERE name = 't'", + ); + assert_eq!( + table_rows, "1", + "guarded cross-kind create must not add a row" + ); + let table_type = oracle_scalar( + &oracle, + &db, + "SELECT type FROM sqlite_master WHERE name = 't'", + ); + assert_eq!(table_type, "table", "t must still be a table, not a view"); + assert_integrity_check_ok(&oracle, &db); +} + +/// `CREATE INDEX` naming an existing table or view is a different +/// namespace clash than index-vs-index — oracle-measured wording is +/// "there is already a table named X" in both directions (even when the +/// clash is with a view, not a table). +#[test] +fn create_index_name_clash_with_table_or_view_without_guard() { + let db = scratch_db("index-vs-table-no-guard"); + exec_ok(&db, "CREATE TABLE t (a)"); + exec_ok(&db, "CREATE VIEW v AS SELECT a FROM t"); + + let output = run_exec(&db, "CREATE INDEX t ON t(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already a table named t"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_exec(&db, "CREATE INDEX v ON t(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already a table named v"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// The index-vs-table/view clash is a different namespace than the one +/// `IF NOT EXISTS` guards on `CREATE INDEX`, so (oracle-measured) the +/// guard does NOT suppress it — still an error. +#[test] +fn create_index_if_not_exists_does_not_suppress_table_or_view_name_clash() { + let db = scratch_db("index-vs-table-guard"); + exec_ok(&db, "CREATE TABLE t (a)"); + exec_ok(&db, "CREATE VIEW v AS SELECT a FROM t"); + + let output = run_exec(&db, "CREATE INDEX IF NOT EXISTS t ON t(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already a table named t"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_exec(&db, "CREATE INDEX IF NOT EXISTS v ON t(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already a table named v"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// `CREATE TABLE`/`CREATE VIEW` naming an existing index is a different +/// namespace clash — oracle-measured wording is "there is already an +/// index named X". +#[test] +fn create_table_or_view_name_clash_with_index_without_guard() { + let db = scratch_db("table-vs-index-no-guard"); + exec_ok(&db, "CREATE TABLE t (a)"); + exec_ok(&db, "CREATE INDEX ix ON t(a)"); + + let output = run_exec(&db, "CREATE TABLE ix(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already an index named ix"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_exec(&db, "CREATE VIEW ix AS SELECT 1"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already an index named ix"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// The table/view-vs-index clash is a different namespace than the one +/// `IF NOT EXISTS` guards on `CREATE TABLE`/`CREATE VIEW`, so +/// (oracle-measured) the guard does NOT suppress it — still an error. +#[test] +fn create_table_or_view_if_not_exists_does_not_suppress_index_name_clash() { + let db = scratch_db("table-vs-index-guard"); + exec_ok(&db, "CREATE TABLE t (a)"); + exec_ok(&db, "CREATE INDEX ix ON t(a)"); + + let output = run_exec(&db, "CREATE TABLE IF NOT EXISTS ix(a)"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already an index named ix"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = run_exec(&db, "CREATE VIEW IF NOT EXISTS ix AS SELECT 1"); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("there is already an index named ix"), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + /// `DROP TABLE IF EXISTS` on a table that was never there is a clean /// no-op (rc 0); without the guard it still fails as before. #[test]