Skip to content

refactor: bind schema hash to migration #2445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
juan518munoz wants to merge 2 commits into
base: next
Choose a base branch
from
Open

refactor: bind schema hash to migration #2445

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b123c86
refactor: bind schema hash to migration
juan518munoz Aug 25, 2026
0677793
chore: address comment & add changelog entry
juan518munoz Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions crates/sqlite-store/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,15 @@ Upgrades are forward-only. There are no down migrations.

1. Add `src/migrations/000N_short_name.sql` with the next unused prefix. Never edit an existing
file, including its comments.
2. Append `SqliteMigration::new(include_str!("../migrations/000N_short_name.sql"))` to
`CLIENT_MIGRATIONS` in `src/db_management/migration.rs`. Nothing scans the directory, so a file
that is not listed here is never applied. Use `SqliteMigration::with_hook` instead if the upgrade
also needs Rust, as described below.
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.
2. Append `SqliteMigration::new(include_str!("../migrations/000N_short_name.sql"), "0x...")` to
`CLIENT_MIGRATIONS` in `src/db_management/migration.rs`, where the second argument is the
fingerprint of the schema the new version builds. Nothing scans the directory, so a file that is
not listed here is never applied. Use `SqliteMigration::with_hook` instead if the upgrade also
needs Rust, as described below.
3. Pin the fingerprint. Start from any placeholder, run
`cargo test -p miden-client-sqlite-store --lib migration_schema_hashes_are_stable`, and replace
it with the hash the failure reports as `found`. Leave the entries before it alone. If one of
them is what fails, 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
Expand All @@ -61,7 +62,7 @@ with the old type and re-encoding it with the new one. SQLite has no way to do t
Such a migration pairs its `.sql` file with a hook, a `fn(&Transaction<'_>) -> HookResult`:

```rust
SqliteMigration::with_hook(include_str!("../migrations/000N_short_name.sql"), reencode_rows)
SqliteMigration::with_hook(include_str!("../migrations/000N_short_name.sql"), reencode_rows, "0x...")
```

Per migration the library runs the SQL, then the foreign key check, then the hook. Three
Expand All @@ -71,8 +72,8 @@ consequences are worth knowing before writing one:
the end, so a hook returning an error rolls back the whole upgrade, not just its own version.
- A hook runs *after* its migration's foreign key check, so rows it writes itself are not covered by
that check. It has to leave the database referentially whole on its own.
- A hook also runs while the fingerprint of each version is being derived, against an empty
database, so it has to tolerate finding no rows.
- A hook also runs against an empty database whenever the schema is built from scratch, on a new
store and in `migration_schema_hashes_are_stable`, so it has to tolerate finding no rows.

The fingerprint check each version ends with is itself such a hook, wrapped around the migration's
own one, which is what lets a rejected upgrade roll back. A migration's hook therefore always runs
Expand Down
159 changes: 56 additions & 103 deletions crates/sqlite-store/src/db_management/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ use super::schema::SchemaHash;
// CLIENT MIGRATIONS
// ================================================================================================

/// The migrations that build the store schema, in the order they are applied.
pub(crate) const CLIENT_MIGRATIONS: [SqliteMigration; 1] =
[SqliteMigration::new(include_str!("../migrations/0001_init.sql"))];
/// The migrations that build the store schema, in the order they are applied, each pinned to the
/// fingerprint the schema has once it has been applied.
pub(crate) const CLIENT_MIGRATIONS: [SqliteMigration; 1] = [SqliteMigration::new(
include_str!("../migrations/0001_init.sql"),
"0xd02b6d09378d300dd92bfc44a2ce15f5852d76eec336b204f87e0d3a916cfa08",
)];

/// The migrations this client ships.
///
/// Building this replays every migration to derive the fingerprint each version produces, so it is
/// built once per process rather than once per store.
static CLIENT_MIGRATOR: LazyLock<SqliteMigrator> =
LazyLock::new(|| SqliteMigrator::new(&CLIENT_MIGRATIONS));

Expand All @@ -36,44 +36,52 @@ pub(crate) type MigrationHook = fn(&Transaction<'_>) -> HookResult;
/// [`SqliteMigrator::apply`].
type RejectionReport = Arc<Mutex<Option<SqliteStoreError>>>;

/// One schema version: the SQL that builds it and, optionally, the Rust code that moves data the
/// SQL cannot.
/// One schema version: the SQL that builds it, optionally the Rust code that moves data the SQL
/// cannot, and the fingerprint the schema is defined to have once both have run.
#[derive(Debug, Clone, Copy)]
pub(crate) struct SqliteMigration {
/// The SQL that takes the schema to this version.
sql: &'static str,
/// Rust code applied on top of the SQL.
hook: Option<MigrationHook>,
/// The fingerprint the schema has once this migration has been applied, as [`SchemaHash`]
/// renders it.
expected_hash: &'static str,
}

impl SqliteMigration {
// CONSTRUCTORS
// --------------------------------------------------------------------------------------------

/// Builds a migration that is applied by running `sql`.
pub(crate) const fn new(sql: &'static str) -> Self {
Self { sql, hook: None }
/// Builds a migration that is applied by running `sql`, and that is defined to leave a schema
/// fingerprinting to `expected_hash`.
pub(crate) const fn new(sql: &'static str, expected_hash: &'static str) -> Self {
Self { sql, hook: None, expected_hash }
}

/// Builds a migration that runs `sql` and then `hook`.
/// Builds a migration that runs `sql` and then `hook`, and that is defined to leave a schema
/// fingerprinting to `expected_hash`.
///
/// `hook` receives the transaction the whole upgrade commits at the end, which has two
/// consequences. Returning an error rolls back every migration the upgrade is applying, not
/// just this one. And the hook runs after this migration's foreign key check, so rows it writes
/// itself are not covered by that check and it has to leave the database referentially whole on
/// its own.
#[cfg_attr(not(test), expect(dead_code, reason = "no shipped migration needs a hook yet"))]
pub(crate) const fn with_hook(sql: &'static str, hook: MigrationHook) -> Self {
Self { sql, hook: Some(hook) }
pub(crate) const fn with_hook(
sql: &'static str,
hook: MigrationHook,
expected_hash: &'static str,
) -> Self {
Self { sql, hook: Some(hook), expected_hash }
}

// CONVERSIONS
// --------------------------------------------------------------------------------------------

/// Returns this migration in the form the migration library applies.
///
/// It runs `SQLite`'s foreign key check inside the transaction it is applied in, so a migration
/// whose SQL orphans a row fails instead of committing.
/// Returns this migration in the form the migration library applies, without the fingerprint
/// check [`SqliteMigrator::apply`] wraps it in.
#[cfg(test)]
fn to_library_migration(self) -> M<'static> {
match self.hook {
Some(hook) => M::up_with_hook(self.sql, hook),
Expand All @@ -86,14 +94,11 @@ impl SqliteMigration {
// SQLITE MIGRATOR
// ================================================================================================

/// An ordered set of migrations that build a store schema, paired with the fingerprint the schema
/// has once each of them has been applied.
/// An ordered set of migrations that build a store schema.
#[derive(Debug)]
pub(crate) struct SqliteMigrator {
/// The migrations in the order they are applied, the one for version `v` at index `v - 1`.
migrations: Vec<SqliteMigration>,
/// The fingerprint the schema has once a migration has been applied.
expected_schema_hashes: Vec<SchemaHash>,
}

impl SqliteMigrator {
Expand All @@ -105,47 +110,22 @@ impl SqliteMigrator {
&CLIENT_MIGRATOR
}

/// Builds the migrator for `migrations`, deriving the fingerprint each version produces by
/// replaying them rather than by trusting a recorded value.
/// Builds the migrator that applies `migrations`, in the order they are given.
pub(crate) fn new(migrations: &[SqliteMigration]) -> Self {
let expected_schema_hashes = Self::replay_schema_hashes(migrations);

Self::with_expected_hashes(migrations, expected_schema_hashes)
}

/// Pairs `migrations` with the fingerprint each of their versions builds.
///
/// # Panics
/// If there is not one fingerprint per migration, since every fingerprint is looked up by the
/// version whose index it sits at.
fn with_expected_hashes(
migrations: &[SqliteMigration],
expected_schema_hashes: Vec<SchemaHash>,
) -> Self {
assert_eq!(
migrations.len(),
expected_schema_hashes.len(),
"every migration needs the fingerprint of the schema it builds"
);

Self {
migrations: migrations.to_vec(),
expected_schema_hashes,
}
Self { migrations: migrations.to_vec() }
}

// ACCESSORS
// --------------------------------------------------------------------------------------------

/// Returns the highest schema version these migrations build.
pub(crate) fn latest_version(&self) -> usize {
self.expected_schema_hashes.len()
self.migrations.len()
}

/// Returns the fingerprint each version is defined to build, version `v` at index `v - 1`.
#[cfg(test)]
pub(crate) fn expected_schema_hashes(&self) -> &[SchemaHash] {
&self.expected_schema_hashes
/// Returns the fingerprint `version` is defined to build.
pub(crate) fn expected_hash(&self, version: usize) -> &'static str {
self.migrations[version - 1].expected_hash
}

// MIGRATION
Expand Down Expand Up @@ -210,6 +190,7 @@ impl SqliteMigrator {
// --------------------------------------------------------------------------------------------

/// Builds `migrations` in the form the migration library applies.
#[cfg(test)]
fn library_migrations(migrations: &[SqliteMigration]) -> Migrations<'static> {
Migrations::new(
migrations.iter().copied().map(SqliteMigration::to_library_migration).collect(),
Expand All @@ -222,27 +203,27 @@ impl SqliteMigrator {
let migrations = self
.migrations
.iter()
.zip(&self.expected_schema_hashes)
.enumerate()
.map(|(index, (migration, &expected))| {
.map(|(index, migration)| {
let version = index + 1;
let hook = migration.hook;
let expected = migration.expected_hash;
let rejection = Arc::clone(rejection);

M::up_with_hook(migration.sql, move |tx: &Transaction<'_>| {
if let Some(hook) = hook {
hook(tx)?;
}

let actual = SchemaHash::of(tx).map_err(|err| hook_error(&err))?;
let actual = SchemaHash::of(tx).map_err(|err| hook_error(&err))?.to_string();
if actual == expected {
return Ok(());
}

let mismatch = SqliteStoreError::MigratedSchemaMismatch {
version,
expected: expected.to_string(),
actual: actual.to_string(),
expected: expected.to_owned(),
actual,
};
let message = mismatch.to_string();
*rejection.lock().expect("rejection lock not poisoned") = Some(mismatch);
Expand All @@ -256,36 +237,17 @@ impl SqliteMigrator {
Migrations::new(migrations)
}

/// Computes the fingerprint each version produces by replaying `migrations` on an in-memory
/// database.
fn replay_schema_hashes(migrations: &[SqliteMigration]) -> Vec<SchemaHash> {
let library_migrations = Self::library_migrations(migrations);
let mut conn =
Connection::open_in_memory().expect("in-memory database creation should not fail");
conn.pragma_update(None, "foreign_keys", "ON")
.expect("enabling foreign keys on the reference database should not fail");

(1..=migrations.len())
.map(|version| {
library_migrations
.to_version(&mut conn, version)
.expect("replaying a migration on the reference database should not fail");
SchemaHash::of(&conn).expect("hashing the reference schema should not fail")
})
.collect()
}

/// Returns the fingerprint version `version` is defined to build and the one `conn` holds,
/// rendered for reporting, when the two differ.
fn schema_mismatch_at(
&self,
conn: &Connection,
version: usize,
) -> Result<Option<(String, String)>, SqliteStoreError> {
let expected = self.expected_schema_hashes[version - 1];
let actual = SchemaHash::of(conn)?;
let expected = self.expected_hash(version);
let actual = SchemaHash::of(conn)?.to_string();

Ok((actual != expected).then(|| (expected.to_string(), actual.to_string())))
Ok((actual != expected).then(|| (expected.to_owned(), actual)))
}

/// Returns whether the database holds no objects of its own.
Expand Down Expand Up @@ -323,26 +285,21 @@ pub(crate) mod tests {
use super::{CLIENT_MIGRATIONS, SqliteMigration, SqliteMigrator};
use crate::db_management::errors::SqliteStoreError;

const PINNED_SCHEMA_HASHES: [&str; CLIENT_MIGRATIONS.len()] =
["0xd02b6d09378d300dd92bfc44a2ce15f5852d76eec336b204f87e0d3a916cfa08"];

// FIXTURES
// --------------------------------------------------------------------------------------------

/// The migrations this client ships with one more appended that drops `input_notes`, recorded
/// as building the schema of the version before it.
/// The migrations this client ships with one more appended that drops `input_notes`, pinned to
/// the fingerprint of the version before it.
///
/// Applying it drops the table and is then rejected by the fingerprint check, which is the
/// shape every failure the rollback has to undo takes.
pub(crate) fn damaging_migration() -> SqliteMigrator {
let mut migrations = CLIENT_MIGRATIONS.to_vec();
migrations.push(SqliteMigration::new("DROP TABLE input_notes;"));
let last = *CLIENT_MIGRATIONS.last().expect("the client ships at least one migration");

let mut expected_schema_hashes = SqliteMigrator::client().expected_schema_hashes.clone();
expected_schema_hashes
.push(*expected_schema_hashes.last().expect("the client ships at least one migration"));
let mut migrations = CLIENT_MIGRATIONS.to_vec();
migrations.push(SqliteMigration::new("DROP TABLE input_notes;", last.expected_hash));

SqliteMigrator::with_expected_hashes(&migrations, expected_schema_hashes)
SqliteMigrator::new(&migrations)
}

// TESTS
Expand Down Expand Up @@ -381,18 +338,14 @@ pub(crate) mod tests {

#[test]
fn migration_schema_hashes_are_stable() {
let replayed = SqliteMigrator::client()
.expected_schema_hashes()
.iter()
.map(ToString::to_string)
.collect::<Vec<_>>();
let pinned = PINNED_SCHEMA_HASHES.map(str::to_string).to_vec();
let mut conn = Connection::open_in_memory().unwrap();

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."
);
if let Err(err) = SqliteMigrator::client().apply(&mut conn) {
panic!(
"a migration builds a different schema than the one it is pinned to. Append a new \
migration instead of editing an existing one. If this is a new migration, pin the \
hash reported below as `found` and leave the ones before it alone. {err}"
);
}
}
}
Loading
Loading