vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(#2279): idempotent column-guard migrations, drop invalid ADD COLUMN IF NOT EXISTS

Browse source
This commit is contained in:
damocles 2026-07-10 16:51:50 +02:00 • committed by mara
commit dfbf198ef3
5 changed files with 188 additions and 121 deletions
Download patch file Download diff file Expand all files Collapse all files

171
hive-c0re/src/stores/db.rs
View file

@ -11,10 +11,12 @@
//! tracks the applied count in a `schema_versions` table (one row per
//! `subsystem` key). Only migrations past the stored version run.
//!
//! Legacy databases (written before versioning) are detected via
//! `legacy_column = (table, column)` — a column that exists only in
//! a fully-migrated legacy DB. Present → skip all migrations. Absent →
//! start from 0. See each store's `MIGRATIONS` constant for the list.
//! Legacy databases (written before versioning) need no special marker:
//! each column-adding migration carries the `(table, column)` it
//! introduces, so re-walking a pre-versioning DB from version 0 simply
//! skips the migrations whose column already exists — converging a fully-
//! or partially-migrated legacy DB without ever re-running an `ADD COLUMN`
//! against an existing column. See each store's `MIGRATIONS` constant.
use std::path::Path;
use std::time::Duration;
@ -33,36 +35,57 @@ CREATE TABLE IF NOT EXISTS schema_versions (
version INTEGER NOT NULL DEFAULT 0
)";
/// One numbered schema migration.
///
/// `sql` runs once, at the index whose value is the current stored version.
/// `adds_column`, when set, is the `(table, column)` this migration
/// introduces: if that column already exists — a pre-versioning DB migrated
/// by the old try-and-ignore path, or a partially-migrated one — `sql` is
/// skipped and only the version counter advances. This makes column-adding
/// migrations idempotent *without* `SQLite`'s unsupported `ALTER TABLE … ADD
/// COLUMN IF NOT EXISTS`, so re-walking a legacy DB from version 0 never
/// re-runs an `ADD COLUMN` against a column that is already there (which
/// would fail with "duplicate column name" and wedge startup permanently).
///
/// Multi-statement migrations wrap their `ALTER` + backfill / index rebuild
/// in an explicit `BEGIN; … COMMIT;` batch, so "the added column exists" is a
/// faithful proxy for "this whole migration committed".
pub struct Migration {
/// SQL for this version — a single statement or a `BEGIN; … COMMIT;` batch.
pub sql: &'static str,
/// `(table, column)` this migration adds, if any. Present → the migration
/// is skipped when the column already exists (idempotent replay).
pub adds_column: Option<(&'static str, &'static str)>,
}
/// Apply numbered schema migrations for `subsystem`, tracking progress in
/// the shared `schema_versions` table.
///
/// - `subsystem` — unique key for this store in `schema_versions`
/// (e.g. `"approvals"`, `"broker"`).
/// - `legacy_column` — `(table_name, column_name)` of a column that exists
/// in a fully-migrated pre-versioning database. When there is no
/// `schema_versions` row yet and this column is present, all `migrations`
/// are skipped (they were previously applied via the old try-and-ignore
/// approach). When the column is absent, we start at version 0.
/// - `migrations` — ordered list of SQL batches; each batch runs once, at
/// the index whose value is the current version.
/// - `migrations` — ordered list of [`Migration`]s; migration `i` runs when
/// the stored version is `i`. A pre-versioning DB (no `schema_versions`
/// row) starts at version 0 and re-walks every migration, but each
/// column-adding migration is skipped when its `adds_column` is already
/// present — so a fully- or partially-migrated legacy DB converges without
/// re-running an `ADD COLUMN` against an existing column.
///
/// # Errors
///
/// Returns an error if any migration statement fails or the version update
/// fails. A failed migration leaves the DB at the last successfully
/// committed version (each step is its own implicit transaction via
/// `execute_batch`).
/// fails. Each migration + its version bump is a single `execute_batch`
/// (multi-statement migrations wrap themselves in `BEGIN; … COMMIT;`), so a
/// failure leaves the DB at the last successfully committed version.
pub fn apply_versioned_migrations(
conn: &Connection,
subsystem: &str,
legacy_column: (&str, &str),
migrations: &[&str],
migrations: &[Migration],
) -> Result<()> {
// Ensure the version-tracking table exists (idempotent).
conn.execute_batch(SCHEMA_VERSIONS_DDL)
.context("create schema_versions table")?;
// Look up the stored version for this subsystem.
// Current version for this subsystem — 0 (and a fresh row) when absent.
let stored: Option<i64> = conn
.query_row(
"SELECT version FROM schema_versions WHERE store = ?1",
@ -71,43 +94,32 @@ pub fn apply_versioned_migrations(
)
.optional()
.context("read schema_versions")?;
let version = if let Some(v) = stored {
usize::try_from(v.max(0)).unwrap_or(0)
} else {
// No entry yet. Check the legacy-column to decide where to start.
// If it exists, all known migrations were already applied by the
// old try-and-ignore path — skip them. If it doesn't exist, start
// at 0 (fresh install or partial state).
let (legacy_table, legacy_col) = legacy_column;
// SAFETY: `legacy_table` is always a static table-name literal
// supplied by each store's `open` (e.g. `"messages"`). Dynamic
// or untrusted table names must not be passed here — if that
// ever changes, switch to `pragma_table_info(?1)` with two
// bound parameters.
let is_legacy: bool = conn
.prepare(&format!(
"SELECT 1 FROM pragma_table_info('{legacy_table}') WHERE name = ?1"
))
.context("prepare legacy-column probe")?
.exists([legacy_col])
.context("check legacy column")?;
let initial = if is_legacy { migrations.len() } else { 0 };
conn.execute(
"INSERT INTO schema_versions (store, version) VALUES (?1, ?2)",
params![subsystem, i64::try_from(initial).unwrap_or(0)],
"INSERT INTO schema_versions (store, version) VALUES (?1, 0)",
[subsystem],
)
.context("insert schema_versions row")?;
initial
0
};
if version >= migrations.len() {
return Ok(());
}
for (i, stmt) in migrations.iter().enumerate().skip(version) {
conn.execute_batch(stmt)
.with_context(|| format!("{subsystem} migration v{} failed: {stmt}", i + 1))?;
for (i, mig) in migrations.iter().enumerate().skip(version) {
// Skip a column-adding migration whose column is already present:
// a legacy DB migrated by the old path, or a resumed partial one.
let already_applied = match mig.adds_column {
Some((table, column)) => column_exists(conn, table, column)?,
None => false,
};
if !already_applied {
conn.execute_batch(mig.sql)
.with_context(|| format!("{subsystem} migration v{} failed: {}", i + 1, mig.sql))?;
}
let next = i64::try_from(i + 1).unwrap_or(i64::MAX);
conn.execute(
"UPDATE schema_versions SET version = ?1 WHERE store = ?2",
@ -118,6 +130,17 @@ pub fn apply_versioned_migrations(
Ok(())
}
/// Whether `column` exists on `table` in this connection's schema. Both
/// arguments are bound parameters — `pragma_table_info(?1)` accepts the table
/// name as a bound value — so this is not a SQL-injection surface even if a
/// caller ever passes a non-static name.
fn column_exists(conn: &Connection, table: &str, column: &str) -> Result<bool> {
conn.prepare("SELECT 1 FROM pragma_table_info(?1) WHERE name = ?2")
.context("prepare column probe")?
.exists(params![table, column])
.context("check column existence")
}
/// Open a connection to the sqlite file at `path`, creating the parent
/// directory if needed. `subsystem` labels error contexts (`"broker"`,
/// `"approvals"`, …).
@ -147,17 +170,26 @@ mod tests {
}
const TEST_SCHEMA: &str = "CREATE TABLE IF NOT EXISTS things (id INTEGER PRIMARY KEY)";
const TEST_MIGRATIONS: &[&str] = &[
"ALTER TABLE things ADD COLUMN IF NOT EXISTS alpha TEXT",
"ALTER TABLE things ADD COLUMN IF NOT EXISTS beta INTEGER NOT NULL DEFAULT 0",
"ALTER TABLE things ADD COLUMN IF NOT EXISTS gamma TEXT",
const TEST_MIGRATIONS: &[Migration] = &[
Migration {
sql: "ALTER TABLE things ADD COLUMN alpha TEXT",
adds_column: Some(("things", "alpha")),
},
Migration {
sql: "ALTER TABLE things ADD COLUMN beta INTEGER NOT NULL DEFAULT 0",
adds_column: Some(("things", "beta")),
},
Migration {
sql: "ALTER TABLE things ADD COLUMN gamma TEXT",
adds_column: Some(("things", "gamma")),
},
];
#[test]
fn fresh_install_runs_all_migrations() {
let conn = open_tmp();
conn.execute_batch(TEST_SCHEMA).unwrap();
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
apply_versioned_migrations(&conn, "things", TEST_MIGRATIONS).unwrap();
// All three columns should exist.
for col in ["alpha", "beta", "gamma"] {
let exists: bool = conn
@ -190,7 +222,7 @@ mod tests {
alpha TEXT, beta INTEGER NOT NULL DEFAULT 0, gamma TEXT)",
)
.unwrap();
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
apply_versioned_migrations(&conn, "things", TEST_MIGRATIONS).unwrap();
// Version should be set to migrations.len() (3), not 0.
let v: i64 = conn
.query_row(
@ -212,7 +244,7 @@ mod tests {
INSERT INTO schema_versions (store, version) VALUES ('things', 1)",
)
.unwrap();
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
apply_versioned_migrations(&conn, "things", TEST_MIGRATIONS).unwrap();
// Only beta and gamma should have been added (alpha already existed).
for col in ["alpha", "beta", "gamma"] {
let exists: bool = conn
@ -241,9 +273,9 @@ mod tests {
fn already_at_latest_is_noop() {
let conn = open_tmp();
conn.execute_batch(TEST_SCHEMA).unwrap();
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
apply_versioned_migrations(&conn, "things", TEST_MIGRATIONS).unwrap();
// Second call is a no-op.
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
apply_versioned_migrations(&conn, "things", TEST_MIGRATIONS).unwrap();
let v: i64 = conn
.query_row(
"SELECT version FROM schema_versions WHERE store = 'things'",
@ -265,15 +297,19 @@ mod tests {
apply_versioned_migrations(
&conn,
"store_a",
("a", "col1"),
&["ALTER TABLE a ADD COLUMN col1 TEXT"],
&[Migration {
sql: "ALTER TABLE a ADD COLUMN col1 TEXT",
adds_column: Some(("a", "col1")),
}],
)
.unwrap();
apply_versioned_migrations(
&conn,
"store_b",
("b", "colx"),
&["ALTER TABLE b ADD COLUMN colx INTEGER NOT NULL DEFAULT 0"],
&[Migration {
sql: "ALTER TABLE b ADD COLUMN colx INTEGER NOT NULL DEFAULT 0",
adds_column: Some(("b", "colx")),
}],
)
.unwrap();
// Each store tracks independently.
@ -296,23 +332,20 @@ mod tests {
}
/// A partially-migrated legacy database: some columns from the old
/// try-and-ignore path exist, but the legacy marker column (used to
/// fast-forward fresh schema_versions entries to `len`) is absent.
/// With idempotent `ADD COLUMN IF NOT EXISTS` migrations this must
/// succeed — the already-present columns are skipped and only the
/// missing ones are added.
/// try-and-ignore path exist, with no `schema_versions` row yet. The
/// per-migration `adds_column` guard must skip the already-present
/// columns and add only the missing ones — no "duplicate column name".
#[test]
fn partial_legacy_completes_without_error() {
let conn = open_tmp();
// Simulate a DB that has `alpha` (v1) but not `beta` (v2) or
// `gamma` (v3). No schema_versions row exists yet, and `gamma`
// (the legacy marker) is absent → the function cannot fast-forward
// and must start from v0 with idempotent migrations.
// `gamma` (v3). No schema_versions row exists → start from v0 and
// guard-skip alpha while adding beta + gamma.
conn.execute_batch(
"CREATE TABLE IF NOT EXISTS things (id INTEGER PRIMARY KEY, alpha TEXT)",
)
.unwrap();
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
apply_versioned_migrations(&conn, "things", TEST_MIGRATIONS).unwrap();
// All three columns must now exist.
for col in ["alpha", "beta", "gamma"] {
let exists: bool = conn
@ -322,7 +355,10 @@ mod tests {
.unwrap()
.exists([])
.unwrap();
assert!(exists, "column {col} missing after partial-legacy migration");
assert!(
exists,
"column {col} missing after partial-legacy migration"
);
}
// Version must be at the latest.
let v: i64 = conn
@ -332,6 +368,9 @@ mod tests {
|r| r.get(0),
)
.unwrap();
assert_eq!(v, 3, "must reach latest version after completing partial legacy");
assert_eq!(
v, 3,
"must reach latest version after completing partial legacy"
);
}
}