diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5a3a6fab32..0b9e49c194 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -17,6 +17,22 @@ env: RUST_CACHE_KEY: rust-cache-2026.02.18 jobs: + migrations: + name: Migrations are append-only + runs-on: ubuntu-latest + timeout-minutes: 5 + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + with: + fetch-depth: 0 + persist-credentials: false + - name: Check that no released migration was modified + env: + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: ./scripts/check-migrations.sh + shell: bash + unused_deps: name: Check for unused dependencies runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index bb7927b40f..ee079172be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ * [FEATURE][rust,store] Added `NoteFilter::ScriptRoots` to query input notes by their note script root directly at the store level, without loading and screening unrelated notes. The filter doesn't apply to output notes: querying output notes with it returns an empty list ([#2335](https://github.com/0xMiden/rust-sdk/pull/2335)). * [rust] Added `PartialBlockchainUpdates::block_headers_to_store`, which narrows the staged headers to the ones a sync must persist: those marked as relevant, genesis, and the block at the sync height. `block_headers` still yields all staged headers ([#2297](https://github.com/0xMiden/rust-sdk/pull/2297)). * [rust] State sync now authenticates every relevant note block but only persists block headers and MMR authentication nodes for blocks containing notes that remain unspent or that a `NoteObserver` explicitly marks as relevant ([#2297](https://github.com/0xMiden/rust-sdk/pull/2297)). +* [store] The SQLite store's schema is now built from append-only migrations under `crates/sqlite-store/src/migrations/`, starting with the frozen `0001_init.sql`. Opening a store verifies its schema against a fingerprint derived by replaying the migrations, both before migrating and after. A store that is behind is copied before it is migrated and put back from that copy if the migration fails, so a failed upgrade leaves it as it was. A pinned snapshot of the fingerprints and a CI job together reject any pull request that modifies an existing migration file ([#2346](https://github.com/0xMiden/rust-sdk/issues/2346)). * [FEATURE][cli] Added `account --inspect [:]` to list the procedures an account exposes, grouped into resolved procedures (with their names and signatures) and unresolved ones (listed by MAST root). Names and signatures are resolved from the `.masp` packages in the configured packages directory plus any passed via `--package` (`-p`). `--verbose` prints each procedure's MASM disassembly. ([#2312](https://github.com/0xMiden/rust-sdk/issues/2312)). * Improved the output of the `miden-client init` command when a configuration already exists ([#](https://github.com/0xMiden/rust-sdk/pull/2357)). diff --git a/Cargo.lock b/Cargo.lock index 2100c450a0..274f5be37e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2121,6 +2121,7 @@ dependencies = [ "miden-standards", "rusqlite", "rusqlite_migration", + "tempfile", "thiserror", "tokio", ] diff --git a/crates/sqlite-store/Cargo.toml b/crates/sqlite-store/Cargo.toml index 38e4834df1..402946d11b 100644 --- a/crates/sqlite-store/Cargo.toml +++ b/crates/sqlite-store/Cargo.toml @@ -37,3 +37,4 @@ workspace = true # Enable client testing utilities only for tests miden-client = { features = ["testing"], workspace = true } miden-standards = { features = ["testing"], workspace = true } +tempfile = { workspace = true } diff --git a/crates/sqlite-store/README.md b/crates/sqlite-store/README.md index 0de71a65f7..9063eb0608 100644 --- a/crates/sqlite-store/README.md +++ b/crates/sqlite-store/README.md @@ -17,5 +17,47 @@ miden-client = { version = "0.13" } miden-client-sqlite-store = { version = "0.13" } ``` +## Migrations + +The schema is built by replaying the migrations listed in `MIGRATION_SCRIPTS` +(`src/db_management/utils.rs`), which include the files under `src/migrations/` in order. A file's +four-digit prefix is its schema version, which is the value SQLite records in `PRAGMA user_version`. + +Migrations are **append-only**. Every store on a user's disk was built by replaying these exact +files. On open the client replays the migrations against an in-memory database to derive the +fingerprint each version should have, and verifies that the schema it finds on disk matches the one +for the version the database claims. A store that was altered outside the migrations is rejected +rather than migrated further. Unlike chain state, a store holds private notes and account seeds +that cannot be recovered from the network. + +Upgrades are forward-only. There are no down migrations. + +### Adding a migration + +1. Add `src/migrations/000N_short_name.sql` with the next unused prefix. Never edit an existing + file, including its comments. +2. Append `include_str!("../migrations/000N_short_name.sql")` to `MIGRATION_SCRIPTS` in + `src/db_management/utils.rs`. Nothing scans the directory, so a file that is not listed here is + never applied. +3. Append one entry to `PINNED_SCHEMA_HASHES` in that file's test module. Run + `cargo test -p miden-client-sqlite-store --lib migration_schema_hashes_are_stable` and take the + new hash from the failure output. Leave the existing entries alone. If they changed, the + migration edited the schema an older version built. +4. Add a `CHANGELOG.md` entry under `[store]`. + +`scripts/check-migrations.sh` runs in CI and fails a pull request that modifies, renames or deletes +a file that already exists on the base branch. + +### Migrations that transform data + +Some upgrades cannot be expressed in SQL. The store holds serialized protocol objects as blobs, so +a change to how an account, note or transaction is encoded has to be applied by decoding each row +with the old type and re-encoding it with the new one. SQLite has no way to do that. + +`rusqlite_migration` covers this with `up_with_hook`, where the hook is a Rust +closure taking the migration's `&Transaction`. Per migration the library runs the SQL, then the +foreign key check, then the hook, and all of it is inside the transaction the whole upgrade commits +at the end, so a hook that returns an error rolls back the migration exactly like failing SQL does. + ## License This project is licensed under the MIT License. See the [LICENSE](../../LICENSE) file for details. diff --git a/crates/sqlite-store/src/db_management/backup.rs b/crates/sqlite-store/src/db_management/backup.rs new file mode 100644 index 0000000000..0e5223cad1 --- /dev/null +++ b/crates/sqlite-store/src/db_management/backup.rs @@ -0,0 +1,181 @@ +use std::ffi::OsString; +use std::path::{Path, PathBuf}; + +use rusqlite::{Connection, params}; + +use super::errors::SqliteStoreError; + +// PRE-MIGRATION BACKUP +// ================================================================================================ + +/// Suffix appended to the store's filename to name its pre-migration backup. +const BACKUP_SUFFIX: &str = ".pre-migration-backup"; + +/// Files `SQLite` keeps next to the database, which describe the database they were written for and +/// must not outlive it. +const SIDECAR_SUFFIXES: [&str; 3] = ["-journal", "-wal", "-shm"]; + +/// Returns the path of the backup taken before migrating the store at `database_filepath`. +pub fn backup_path(database_filepath: &Path) -> PathBuf { + let mut path = OsString::from(database_filepath); + path.push(BACKUP_SUFFIX); + PathBuf::from(path) +} + +/// Copies the database into `backup_filepath`, replacing a backup left behind by an earlier run. +/// +/// `VACUUM INTO` writes a consistent snapshot even while the connection is open, so this does not +/// depend on the caller quiescing the store. +pub fn create_backup(conn: &Connection, backup_filepath: &Path) -> Result<(), SqliteStoreError> { + // A backup that is still here was left by a run that died mid-migration. Its database was + // already restored or abandoned, and `VACUUM INTO` refuses to write to a file that exists. + discard_backup(backup_filepath)?; + + conn.execute("VACUUM INTO ?1", params![path_argument(backup_filepath)?])?; + + Ok(()) +} + +/// Puts the backup back in place of the database, consuming the backup. +/// +/// The caller must have closed every connection to the database first. Restoring under an open +/// connection would leave that connection reading a file that no longer exists, and on Windows the +/// replacement cannot happen at all. +pub fn restore_backup( + database_filepath: &Path, + backup_filepath: &Path, +) -> Result<(), SqliteStoreError> { + // These describe the database being replaced, not the backup, so leaving one behind would let + // `SQLite` apply it to the restored file. + for suffix in SIDECAR_SUFFIXES { + discard_backup(&sidecar_path(database_filepath, suffix))?; + } + + std::fs::rename(backup_filepath, database_filepath).map_err(|err| { + SqliteStoreError::BackupRestoreFailed { + backup: backup_filepath.display().to_string(), + reason: err.to_string(), + } + }) +} + +/// Removes `backup_filepath` if it exists. +pub fn discard_backup(backup_filepath: &Path) -> Result<(), SqliteStoreError> { + match std::fs::remove_file(backup_filepath) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(SqliteStoreError::BackupFailed { + backup: backup_filepath.display().to_string(), + reason: err.to_string(), + }), + } +} + +/// Renders a path for `SQLite`, which takes filenames as text. +fn path_argument(path: &Path) -> Result<&str, SqliteStoreError> { + path.to_str().ok_or_else(|| SqliteStoreError::BackupFailed { + backup: path.display().to_string(), + reason: String::from("backup path is not valid UTF-8"), + }) +} + +/// Returns the path of the `SQLite` sidecar file for `database_filepath` with the given suffix. +fn sidecar_path(database_filepath: &Path, suffix: &str) -> PathBuf { + let mut path = OsString::from(database_filepath); + path.push(suffix); + PathBuf::from(path) +} + +// TESTS +// ================================================================================================ + +#[cfg(test)] +mod tests { + // `tempfile` rather than `create_test_store_path`: `TempDir` removes the database and any + // backup left behind on drop, so repeated runs do not accumulate files in the system temp + // directory. + + use rusqlite::Connection; + + use super::{backup_path, create_backup, restore_backup, sidecar_path}; + + fn table_names(conn: &Connection) -> Vec { + let mut stmt = conn + .prepare("SELECT name FROM sqlite_schema WHERE type = 'table' ORDER BY name") + .unwrap(); + stmt.query_map([], |row| row.get::<_, String>(0)) + .unwrap() + .collect::, _>>() + .unwrap() + } + + #[test] + fn backup_restores_the_database_as_it_was() { + let dir = tempfile::tempdir().unwrap(); + let database = dir.path().join("store.sqlite3"); + let backup = backup_path(&database); + + let conn = Connection::open(&database).unwrap(); + conn.execute_batch( + "CREATE TABLE items (id INTEGER PRIMARY KEY, value TEXT); + INSERT INTO items (id, value) VALUES (1, 'before');", + ) + .unwrap(); + + create_backup(&conn, &backup).unwrap(); + assert!(backup.exists()); + + // The change the restore is meant to undo. + conn.execute_batch( + "DROP TABLE items; + CREATE TABLE migrated (id INTEGER PRIMARY KEY);", + ) + .unwrap(); + drop(conn); + + restore_backup(&database, &backup).unwrap(); + assert!(!backup.exists(), "a consumed backup should not be left behind"); + + let conn = Connection::open(&database).unwrap(); + assert_eq!(table_names(&conn), vec![String::from("items")]); + let value: String = conn + .query_row("SELECT value FROM items WHERE id = 1", [], |row| row.get(0)) + .unwrap(); + assert_eq!(value, "before"); + } + + #[test] + fn backup_replaces_one_left_by_an_earlier_run() { + let dir = tempfile::tempdir().unwrap(); + let database = dir.path().join("store.sqlite3"); + let backup = backup_path(&database); + std::fs::write(&backup, b"not a database").unwrap(); + + let conn = Connection::open(&database).unwrap(); + conn.execute_batch("CREATE TABLE items (id INTEGER PRIMARY KEY);").unwrap(); + + create_backup(&conn, &backup).unwrap(); + + let restored = Connection::open(&backup).unwrap(); + assert_eq!(table_names(&restored), vec![String::from("items")]); + } + + #[test] + fn restore_removes_sidecars_of_the_replaced_database() { + let dir = tempfile::tempdir().unwrap(); + let database = dir.path().join("store.sqlite3"); + let backup = backup_path(&database); + + let conn = Connection::open(&database).unwrap(); + conn.execute_batch("CREATE TABLE items (id INTEGER PRIMARY KEY);").unwrap(); + create_backup(&conn, &backup).unwrap(); + drop(conn); + + let journal = sidecar_path(&database, "-journal"); + std::fs::write(&journal, b"stale").unwrap(); + + restore_backup(&database, &backup).unwrap(); + + assert!(!journal.exists(), "a sidecar of the replaced database must not survive it"); + } +} diff --git a/crates/sqlite-store/src/db_management/errors.rs b/crates/sqlite-store/src/db_management/errors.rs index 5132aac830..3dac4e4311 100644 --- a/crates/sqlite-store/src/db_management/errors.rs +++ b/crates/sqlite-store/src/db_management/errors.rs @@ -11,21 +11,65 @@ use thiserror::Error; #[derive(Debug, Error)] pub enum SqliteStoreError { #[error("Database error: {0}")] - DatabaseError(String), + Database(String), #[error("Migration error: {0}")] - MigrationError(String), - #[error("Database schema does not match the schema expected for its migration version")] - SchemaHashMismatch, + Migration(String), + #[error( + "stored schema at version {version} does not match the schema this client builds for that version (expected {expected}, found {actual})" + )] + SchemaDrift { + version: u32, + expected: String, + actual: String, + }, + #[error( + "store is at schema version {found}, which is newer than the highest version this client supports ({supported})" + )] + SchemaTooNew { found: u32, supported: u32 }, + #[error( + "migrating to schema version {version} produced a schema this client does not expect (expected {expected}, found {actual})" + )] + MigratedSchemaMismatch { + version: u32, + expected: String, + actual: String, + }, + #[error( + "the database is not empty and does not record a schema version, so it was not created by this client and will not be migrated into a store" + )] + NotAClientStore, + #[error("failed to back up the store to {backup} before migrating it: {reason}")] + BackupFailed { backup: String, reason: String }, + #[error( + "migrating the store failed and it could not be restored from its backup at {backup}: {reason}. The backup holds the store as it was before migrating" + )] + BackupRestoreFailed { backup: String, reason: String }, } impl From for SqliteStoreError { fn from(err: RusqliteError) -> Self { - SqliteStoreError::DatabaseError(err.to_string()) + SqliteStoreError::Database(err.to_string()) } } impl From for SqliteStoreError { + /// Renders a migration failure without reproducing the migration script. fn from(err: MigrationError) -> Self { - SqliteStoreError::MigrationError(err.to_string()) + let message = match &err { + MigrationError::RusqliteError { + err: RusqliteError::SqlInputError { msg, .. }, + .. + } => msg.clone(), + MigrationError::RusqliteError { err, .. } => err.to_string(), + MigrationError::ForeignKeyCheck(violations) => { + format!( + "{} foreign key violation(s) after applying the migration", + violations.len() + ) + }, + other => other.to_string(), + }; + + SqliteStoreError::Migration(message) } } diff --git a/crates/sqlite-store/src/db_management/migration_tests.rs b/crates/sqlite-store/src/db_management/migration_tests.rs index f3be2061ac..3c44cfa615 100644 --- a/crates/sqlite-store/src/db_management/migration_tests.rs +++ b/crates/sqlite-store/src/db_management/migration_tests.rs @@ -114,7 +114,7 @@ fn schema_present_at_version_zero_fails() { )); let err = apply_fixture_migrations(&mut conn).unwrap_err(); - assert!(matches!(err, SqliteStoreError::MigrationError(_))); + assert!(matches!(err, SqliteStoreError::NotAClientStore)); } #[test] @@ -124,7 +124,11 @@ fn user_version_beyond_migrations_fails() { .expect("user_version should update"); let err = apply_fixture_migrations(&mut conn).unwrap_err(); - assert!(matches!(err, SqliteStoreError::MigrationError(_))); + let SqliteStoreError::SchemaTooNew { found, supported } = err else { + panic!("a version beyond the migrations should be reported as too new, got {err:?}"); + }; + assert_eq!(found as usize, FIXTURE_MIGRATION_COUNT + 1); + assert_eq!(supported as usize, FIXTURE_MIGRATION_COUNT); } #[test] @@ -143,7 +147,11 @@ fn partial_migration_schema_drift_is_rejected() { .expect("manual schema change should apply"); let err = apply_fixture_migrations(&mut conn).unwrap_err(); - assert!(matches!(err, SqliteStoreError::SchemaHashMismatch)); + let SqliteStoreError::SchemaDrift { version, expected, actual } = err else { + panic!("a hand-modified schema should be reported as drift, got {err:?}"); + }; + assert_eq!(version, 1); + assert_ne!(expected, actual); } #[test] diff --git a/crates/sqlite-store/src/db_management/mod.rs b/crates/sqlite-store/src/db_management/mod.rs index 3f80965323..e54d615777 100644 --- a/crates/sqlite-store/src/db_management/mod.rs +++ b/crates/sqlite-store/src/db_management/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod backup; pub(crate) mod errors; pub(crate) mod pool_manager; pub(crate) mod utils; diff --git a/crates/sqlite-store/src/db_management/utils.rs b/crates/sqlite-store/src/db_management/utils.rs index 21340da104..b0c338a8df 100644 --- a/crates/sqlite-store/src/db_management/utils.rs +++ b/crates/sqlite-store/src/db_management/utils.rs @@ -61,8 +61,13 @@ type Hash = Blake3Digest<32>; const SCHEMA_HASH_DOMAIN: &[u8] = b"miden-client-sqlite-schema-v1"; -const MIGRATION_SCRIPTS: [&str; 1] = [include_str!("../store.sql")]; +/// The migrations that build the store schema, in the order they are applied. +const MIGRATION_SCRIPTS: [&str; 1] = [include_str!("../migrations/0001_init.sql")]; + static MIGRATIONS: LazyLock = LazyLock::new(prepare_migrations); + +/// The schema fingerprint each migration in [`MIGRATION_SCRIPTS`] produces, obtained by replaying +/// the migrations rather than by trusting a recorded value. pub(crate) static EXPECTED_SCHEMA_HASHES: LazyLock> = LazyLock::new(compute_expected_schema_hashes); @@ -70,37 +75,106 @@ fn up(s: &'static str) -> M<'static> { M::up(s).foreign_key_check() } -/// Applies the given migrations to the database after validating the schema fingerprint for the -/// current migration version. +/// Returns whether the database holds a schema that is behind the latest version. +/// +/// A database with no schema at all is not behind: there is nothing in it to preserve, so opening +/// it builds the latest schema directly. +pub fn has_pending_migrations(conn: &Connection) -> Result { + match MIGRATIONS.current_version(conn)? { + SchemaVersion::Inside(ver) => Ok(ver.get() < MIGRATION_SCRIPTS.len()), + // A version beyond the last migration is rejected when migrating, not backed up. + SchemaVersion::NoneSet | SchemaVersion::Outside(_) => Ok(false), + } +} + +/// Brings the database up to the latest schema version, creating it if it is empty. +pub fn apply_migrations(conn: &mut Connection) -> Result<(), SqliteStoreError> { + apply_migrations_with(conn, &MIGRATIONS, &EXPECTED_SCHEMA_HASHES) +} + +/// [`apply_migrations`] with the migration set and its fingerprints injected, so that tests can +/// drive the paths a single migration cannot reach on its own. pub(crate) fn apply_migrations_with( conn: &mut Connection, migrations: &Migrations, expected_schema_hashes: &[Hash], ) -> Result<(), SqliteStoreError> { - let version_before = migrations.current_version(conn)?; - - if let SchemaVersion::Inside(ver) = version_before { - let actual_hash = schema_hash(conn)?; - if actual_hash != expected_schema_hashes[ver.get() - 1] { - return Err(SqliteStoreError::SchemaHashMismatch); - } + let latest_version = expected_schema_hashes.len(); + + match migrations.current_version(conn)? { + SchemaVersion::NoneSet => { + if !is_empty_database(conn)? { + return Err(SqliteStoreError::NotAClientStore); + } + }, + SchemaVersion::Inside(ver) => { + let expected = expected_schema_hashes[ver.get() - 1]; + let actual = schema_hash(conn)?; + if actual != expected { + return Err(SqliteStoreError::SchemaDrift { + version: schema_version(ver.get()), + expected: String::from(expected), + actual: String::from(actual), + }); + } + }, + SchemaVersion::Outside(ver) => { + return Err(SqliteStoreError::SchemaTooNew { + found: schema_version(ver.get()), + supported: schema_version(latest_version), + }); + }, } migrations.to_latest(conn)?; + verify_migrated_schema(conn, expected_schema_hashes, latest_version) +} + +/// Returns whether the database holds no objects of its own. +fn is_empty_database(conn: &Connection) -> Result { + let objects: u32 = conn.query_row( + "SELECT COUNT(*) FROM sqlite_schema WHERE name NOT GLOB 'sqlite_*'", + [], + |row| row.get(0), + )?; + + Ok(objects == 0) +} + +/// Checks that migrating to `version` built the schema that version is defined to build. +fn verify_migrated_schema( + conn: &Connection, + expected_schema_hashes: &[Hash], + version: usize, +) -> Result<(), SqliteStoreError> { + let expected = expected_schema_hashes[version - 1]; + let actual = schema_hash(conn)?; + + if actual != expected { + return Err(SqliteStoreError::MigratedSchemaMismatch { + version: schema_version(version), + expected: String::from(expected), + actual: String::from(actual), + }); + } + Ok(()) } -/// Applies the migrations to the database. -pub fn apply_migrations(conn: &mut Connection) -> Result<(), SqliteStoreError> { - apply_migrations_with(conn, &MIGRATIONS, &EXPECTED_SCHEMA_HASHES) +/// Narrows a migration index to the width schema versions are reported in. +/// +/// `SQLite` stores the version in `PRAGMA user_version`, which is an `i32`, so a version that does +/// not fit is unreachable. +fn schema_version(version: usize) -> u32 { + u32::try_from(version).expect("schema version should fit in a u32") } fn prepare_migrations() -> Migrations<'static> { Migrations::new(MIGRATION_SCRIPTS.map(up).to_vec()) } -/// Computes the schema fingerprint expected after each migration by replaying the migrations on an +/// Computes the schema fingerprint each migration produces by replaying the migrations on an /// in-memory database. pub(crate) fn compute_expected_schema_hashes_for( migrations: &Migrations, @@ -214,9 +288,18 @@ pub fn list_setting_keys(conn: &Connection) -> Result, StoreError> { mod tests { use rusqlite::Connection; - use super::{EXPECTED_SCHEMA_HASHES, MIGRATION_SCRIPTS, apply_migrations, schema_hash}; + use super::{ + EXPECTED_SCHEMA_HASHES, + MIGRATION_SCRIPTS, + apply_migrations, + schema_hash, + verify_migrated_schema, + }; use crate::db_management::errors::SqliteStoreError; + const PINNED_SCHEMA_HASHES: [&str; MIGRATION_SCRIPTS.len()] = + ["0x749fba4988cae911b43dd2a3efef634ce5f514515ae26687f791fb17612c5b7a"]; + #[test] fn honest_database_reopens_without_error() { let mut conn = Connection::open_in_memory().unwrap(); @@ -226,6 +309,41 @@ mod tests { apply_migrations(&mut conn).unwrap(); } + #[test] + fn fresh_database_is_built_to_the_latest_version() { + let mut conn = Connection::open_in_memory().unwrap(); + apply_migrations(&mut conn).unwrap(); + + let version: usize = conn.query_row("PRAGMA user_version", [], |row| row.get(0)).unwrap(); + assert_eq!(version, MIGRATION_SCRIPTS.len()); + assert_eq!(schema_hash(&conn).unwrap(), EXPECTED_SCHEMA_HASHES[version - 1]); + } + + #[test] + fn unversioned_database_with_contents_is_rejected() { + let mut conn = Connection::open_in_memory().unwrap(); + // A database that is not a store, named by mistake. It records no version, which is what + // an empty file also looks like. + conn.execute_batch("CREATE TABLE somebody_elses (id INTEGER PRIMARY KEY);") + .unwrap(); + + let err = apply_migrations(&mut conn).unwrap_err(); + assert!( + matches!(err, SqliteStoreError::NotAClientStore), + "a foreign database should not be migrated into a store, got {err:?}" + ); + + // Refusing must leave the database alone. + let version: usize = conn.query_row("PRAGMA user_version", [], |row| row.get(0)).unwrap(); + assert_eq!(version, 0); + let tables: u32 = conn + .query_row("SELECT COUNT(*) FROM sqlite_schema WHERE name = 'input_notes'", [], |row| { + row.get(0) + }) + .unwrap(); + assert_eq!(tables, 0); + } + #[test] fn schema_drift_is_rejected() { let mut conn = Connection::open_in_memory().unwrap(); @@ -235,7 +353,28 @@ mod tests { conn.execute("ALTER TABLE input_notes ADD COLUMN injected TEXT", []).unwrap(); let err = apply_migrations(&mut conn).unwrap_err(); - assert!(matches!(err, SqliteStoreError::SchemaHashMismatch)); + let SqliteStoreError::SchemaDrift { version, expected, actual } = err else { + panic!("drifted schema should be reported as drift, got {err:?}"); + }; + assert_eq!(version, 1); + assert_ne!(expected, actual); + } + + #[test] + fn database_from_a_newer_client_is_rejected() { + let mut conn = Connection::open_in_memory().unwrap(); + apply_migrations(&mut conn).unwrap(); + + // A version this client has no migration for, as written by a later release. + let ahead = MIGRATION_SCRIPTS.len() + 3; + conn.pragma_update(None, "user_version", ahead).unwrap(); + + let err = apply_migrations(&mut conn).unwrap_err(); + let SqliteStoreError::SchemaTooNew { found, supported } = err else { + panic!("a database from a newer client should be reported as too new, got {err:?}"); + }; + assert_eq!(found as usize, ahead); + assert_eq!(supported as usize, MIGRATION_SCRIPTS.len()); } #[test] @@ -259,7 +398,34 @@ mod tests { } #[test] - fn expected_schema_hash_per_migration() { - assert_eq!(EXPECTED_SCHEMA_HASHES.len(), MIGRATION_SCRIPTS.len()); + fn migrated_schema_is_verified() { + let mut conn = Connection::open_in_memory().unwrap(); + apply_migrations(&mut conn).unwrap(); + + // Migrating cannot be made to build the wrong schema without a broken migration, so the + // schema is changed under the check instead, which is what such a migration would leave + // behind. + conn.execute("DROP TABLE input_notes", []).unwrap(); + + let err = verify_migrated_schema(&conn, &EXPECTED_SCHEMA_HASHES, MIGRATION_SCRIPTS.len()) + .unwrap_err(); + let SqliteStoreError::MigratedSchemaMismatch { version, expected, actual } = err else { + panic!("an unexpected migrated schema should be reported as a mismatch, got {err:?}"); + }; + assert_eq!(version as usize, MIGRATION_SCRIPTS.len()); + assert_ne!(expected, actual); + } + + #[test] + fn migration_schema_hashes_are_stable() { + let replayed = EXPECTED_SCHEMA_HASHES.iter().copied().map(String::from).collect::>(); + let pinned = PINNED_SCHEMA_HASHES.map(str::to_string).to_vec(); + + assert_eq!( + replayed, pinned, + "a released migration builds a different schema than it did when it was pinned. \ + Append a new migration instead of editing an existing one. If this is a new \ + migration, append its hash rather than rewriting the entries before it." + ); } } diff --git a/crates/sqlite-store/src/lib.rs b/crates/sqlite-store/src/lib.rs index 2d6d22ad24..3df7f27db0 100644 --- a/crates/sqlite-store/src/lib.rs +++ b/crates/sqlite-store/src/lib.rs @@ -6,15 +6,18 @@ use std::boxed::Box; use std::collections::{BTreeMap, BTreeSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::string::{String, ToString}; use std::sync::{Arc, RwLock}; use std::vec::Vec; +use db_management::backup::{backup_path, create_backup, discard_backup, restore_backup}; +use db_management::errors::SqliteStoreError; use db_management::pool_manager::{Pool, SqlitePoolManager}; use db_management::utils::{ apply_migrations, get_setting, + has_pending_migrations, list_setting_keys, remove_setting, set_setting, @@ -76,7 +79,8 @@ pub use builder::ClientBuilderSqliteExt; /// Represents a pool of connections with an `SQLite` database. The pool is used to interact /// concurrently with the underlying database in a safe and efficient manner. /// -/// Current table definitions can be found at `store.sql` migration file. +/// Current table definitions are the result of applying every migration under `migrations/` in +/// order. pub struct SqliteStore { pub(crate) pool: Pool, database_filepath: String, @@ -90,17 +94,12 @@ impl SqliteStore { /// Returns a new instance of [Store] instantiated with the specified configuration options. pub async fn new(database_filepath: PathBuf) -> Result { let database_filepath_str = database_filepath.to_string_lossy().into_owned(); - let sqlite_pool_manager = SqlitePoolManager::new(database_filepath); + let sqlite_pool_manager = SqlitePoolManager::new(database_filepath.clone()); let pool = Pool::builder(sqlite_pool_manager) .build() .map_err(|e| StoreError::DatabaseError(e.to_string()))?; - let conn = pool.get().await.map_err(|e| StoreError::DatabaseError(e.to_string()))?; - - conn.interact(apply_migrations) - .await - .map_err(|e| StoreError::DatabaseError(e.to_string()))? - .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + Self::migrate(&pool, &database_filepath).await?; let store = SqliteStore { pool, @@ -125,6 +124,59 @@ impl SqliteStore { Ok(store) } + /// Brings the database at `database_filepath` up to the latest schema version. + async fn migrate(pool: &Pool, database_filepath: &Path) -> Result<(), StoreError> { + Self::migrate_with(pool, database_filepath, has_pending_migrations, apply_migrations).await + } + + /// [`Self::migrate`] with its two schema steps injected, so that tests can drive the paths a + /// single migration cannot reach on its own. + async fn migrate_with( + pool: &Pool, + database_filepath: &Path, + pending: fn(&Connection) -> Result, + apply: fn(&mut Connection) -> Result<(), SqliteStoreError>, + ) -> Result<(), StoreError> { + let conn = pool.get().await.map_err(|e| StoreError::DatabaseError(e.to_string()))?; + + let upgrading = conn + .interact(move |conn| pending(conn)) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))? + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + + let backup = backup_path(database_filepath); + if upgrading { + let backup = backup.clone(); + conn.interact(move |conn| create_backup(conn, &backup)) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))? + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + } + + let migrated = conn + .interact(apply) + .await + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + + // The database file cannot be replaced while anything is still reading it, so the + // connection goes back to the pool and the pool closes before the backup is put back. + drop(conn); + + let Err(migration_error) = migrated else { + discard_backup(&backup).map_err(|e| StoreError::DatabaseError(e.to_string()))?; + return Ok(()); + }; + + if upgrading { + pool.close(); + restore_backup(database_filepath, &backup) + .map_err(|e| StoreError::DatabaseError(e.to_string()))?; + } + + Err(StoreError::DatabaseError(migration_error.to_string())) + } + /// Interacts with the database by executing the provided function on a connection from the /// pool. /// @@ -616,7 +668,32 @@ pub mod tests { use miden_client::store::Store; use miden_client::testing::common::create_test_store_path; - use super::SqliteStore; + use super::db_management::pool_manager::SqlitePoolManager; + use super::{Pool, SqliteStore, SqliteStoreError, String, backup_path}; + + /// Stands in for a migration that damages the store before failing, which is what the backup + /// exists to undo. + fn failing_migration(conn: &mut rusqlite::Connection) -> Result<(), SqliteStoreError> { + conn.execute_batch("DROP TABLE input_notes;")?; + Err(SqliteStoreError::Migration(String::from("migration failed"))) + } + + #[tokio::test] + async fn failed_migration_leaves_the_store_as_it_was() { + let database_filepath = create_test_store_path(); + drop(SqliteStore::new(database_filepath.clone()).await.unwrap()); + + let pool = Pool::builder(SqlitePoolManager::new(database_filepath.clone())) + .build() + .unwrap(); + SqliteStore::migrate_with(&pool, &database_filepath, |_| Ok(true), failing_migration) + .await + .unwrap_err(); + + assert!(!backup_path(&database_filepath).exists(), "the backup should be consumed"); + // Reopening verifies the schema, so it only succeeds if the dropped table came back. + SqliteStore::new(database_filepath).await.unwrap(); + } fn assert_send_sync() {} diff --git a/crates/sqlite-store/src/store.sql b/crates/sqlite-store/src/migrations/0001_init.sql similarity index 100% rename from crates/sqlite-store/src/store.sql rename to crates/sqlite-store/src/migrations/0001_init.sql diff --git a/scripts/check-migrations.sh b/scripts/check-migrations.sh new file mode 100755 index 0000000000..4b360c278d --- /dev/null +++ b/scripts/check-migrations.sh @@ -0,0 +1,31 @@ +#!/bin/bash +# Fails closed: any error resolving the base branch or running git aborts the script rather than +# reporting success. This is the only automated guard against an edited migration, so a silent +# pass would be worse than a false alarm. +set -euo pipefail + +MIGRATIONS_DIR="${1:-crates/sqlite-store/src/migrations}" +BASE="origin/${BASE_REF:?must be set to the base branch of the pull request}" + +if ! git rev-parse --verify --quiet "${BASE}^{commit}" > /dev/null; then + >&2 echo "Cannot resolve \"${BASE}\". Fetch the base branch before running this check." + exit 1 +fi + +# Compared against the merge base rather than the tip of the base branch, so a migration added on +# the base branch after this one forked is not attributed to this pull request. +CHANGED=$(git diff --name-only --diff-filter=MDR --merge-base "${BASE}" -- "${MIGRATIONS_DIR}") + +if [ -z "${CHANGED}" ]; then + echo "No released migration was modified." + exit 0 +fi + +>&2 echo "The following merged migrations were modified, renamed or deleted:" +>&2 echo "${CHANGED}" +>&2 echo "" +>&2 echo "Migrations are append-only. Add a new file under \"${MIGRATIONS_DIR}\" with the next +version prefix instead, register it in MIGRATION_SCRIPTS and append its schema hash to +PINNED_SCHEMA_HASHES, both in crates/sqlite-store/src/db_management/utils.rs, rather than editing +the existing entries." +exit 1