-
Notifications
You must be signed in to change notification settings - Fork 111
feat(store): improve SQL migrations #2358
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.
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
base: next
Are you sure you want to change the base?
Changes from 2 commits
8c5b813
e563024
0e10af0
170dec7
4f4dd5d
4897f1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,36 +61,62 @@ 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")]; | ||
|
|
||
| /// The schema fingerprint each migration in [`MIGRATION_SCRIPTS`] produced when it was released. | ||
| const PINNED_SCHEMA_HASHES: [&str; MIGRATION_SCRIPTS.len()] = | ||
| ["0x6300110a9f3efa3476fac4e736f94c33e07935ab7eedf357b38a50f55cabf140"]; | ||
|
|
||
| static MIGRATIONS: LazyLock<Migrations> = LazyLock::new(prepare_migrations); | ||
| static EXPECTED_SCHEMA_HASHES: LazyLock<Vec<Hash>> = LazyLock::new(compute_expected_schema_hashes); | ||
|
|
||
| fn up(s: &'static str) -> M<'static> { | ||
| M::up(s).foreign_key_check() | ||
| } | ||
|
|
||
| /// Applies the migrations to the database. | ||
| /// Brings the database up to the latest schema version, creating it if it is empty. | ||
| pub fn apply_migrations(conn: &mut Connection) -> 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); | ||
| } | ||
| match MIGRATIONS.current_version(conn)? { | ||
| SchemaVersion::NoneSet => {}, | ||
| SchemaVersion::Inside(ver) => { | ||
| let expected = PINNED_SCHEMA_HASHES[ver.get() - 1]; | ||
| let actual = String::from(schema_hash(conn)?); | ||
| if actual != expected { | ||
| return Err(SqliteStoreError::SchemaDrift { | ||
| version: schema_version(ver.get()), | ||
| expected: expected.to_string(), | ||
| actual, | ||
| }); | ||
| } | ||
| }, | ||
| SchemaVersion::Outside(ver) => { | ||
| return Err(SqliteStoreError::SchemaTooNew { | ||
| found: schema_version(ver.get()), | ||
| supported: schema_version(MIGRATION_SCRIPTS.len()), | ||
| }); | ||
| }, | ||
| } | ||
|
|
||
| MIGRATIONS.to_latest(conn)?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// 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. | ||
| #[cfg(test)] | ||
| fn compute_expected_schema_hashes() -> Vec<Hash> { | ||
| let mut conn = | ||
| Connection::open_in_memory().expect("in-memory database creation should not fail"); | ||
|
|
@@ -144,14 +170,83 @@ fn push_field(buf: &mut Vec<u8>, field: &[u8]) { | |
| buf.extend_from_slice(field); | ||
| } | ||
|
|
||
| /// Collapses runs of whitespace to single spaces and trims a trailing semicolon so cosmetic | ||
| /// differences in stored SQL text do not change the fingerprint. | ||
| /// Rewrites the SQL text stored for a schema object into a form that ignores differences `SQLite` | ||
| /// itself ignores, so cosmetic edits do not change the fingerprint. | ||
| fn normalize_sql(sql: &str) -> String { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's revert these changes. I don't think it's code worth maintaining as it becomes instantly quite more complicated. A solution could be to introduce a SQL linter so the normalization is even more trivial.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted |
||
| sql.trim_end() | ||
| .trim_end_matches(';') | ||
| .split_whitespace() | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| let mut out = String::with_capacity(sql.len()); | ||
| let mut chars = sql.chars().peekable(); | ||
|
|
||
| while let Some(ch) = chars.next() { | ||
| match ch { | ||
| // A doubled quote inside a quoted region escapes itself and does not close it. | ||
| '\'' | '"' | '`' => { | ||
| out.push(ch); | ||
| while let Some(inner) = chars.next() { | ||
| out.push(inner); | ||
| if inner == ch { | ||
| if chars.peek() == Some(&ch) { | ||
| out.push(ch); | ||
| chars.next(); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| // Bracketed identifiers do not nest and have no escape sequence. | ||
| '[' => { | ||
| out.push(ch); | ||
| for inner in chars.by_ref() { | ||
| out.push(inner); | ||
| if inner == ']' { | ||
| break; | ||
| } | ||
| } | ||
| }, | ||
| // A comment collapses to a separator rather than to nothing, because `SQLite` does not | ||
| // require whitespace before `--` and fusing the tokens on either side of it would | ||
| // change what the text means. | ||
| '-' if chars.peek() == Some(&'-') => { | ||
| chars.next(); | ||
| while chars.peek().is_some_and(|&inner| inner != '\n') { | ||
| chars.next(); | ||
| } | ||
| push_separator(&mut out); | ||
| }, | ||
| '/' if chars.peek() == Some(&'*') => { | ||
| chars.next(); | ||
| let mut prev = '\0'; | ||
| for inner in chars.by_ref() { | ||
| if prev == '*' && inner == '/' { | ||
| break; | ||
| } | ||
| prev = inner; | ||
| } | ||
| push_separator(&mut out); | ||
| }, | ||
| _ if is_sql_whitespace(ch) => push_separator(&mut out), | ||
| _ => out.push(ch), | ||
| } | ||
| } | ||
|
|
||
| let normalized = out.trim_end().trim_end_matches(';').trim(); | ||
| normalized.to_string() | ||
| } | ||
|
|
||
| /// Appends a single space unless one is already there, so adjacent separators do not stack up. | ||
| fn push_separator(out: &mut String) { | ||
| if !out.is_empty() && !out.ends_with(' ') { | ||
| out.push(' '); | ||
| } | ||
| } | ||
|
|
||
| /// Returns whether `ch` separates tokens for `SQLite`. | ||
| /// | ||
| /// This is deliberately narrower than [`char::is_whitespace`]. `SQLite` treats every byte above | ||
| /// the ASCII range as part of an identifier, so a Unicode space between two tokens makes them one | ||
| /// token and must not be normalized away. | ||
| fn is_sql_whitespace(ch: char) -> bool { | ||
| matches!(ch, ' ' | '\t' | '\n' | '\r' | '\u{0c}') | ||
| } | ||
|
|
||
| pub fn get_setting<T: FromSql>(conn: &mut Connection, name: &str) -> Result<Option<T>, StoreError> { | ||
|
|
@@ -196,9 +291,21 @@ pub fn list_setting_keys(conn: &Connection) -> Result<Vec<String>, StoreError> { | |
| mod tests { | ||
| use rusqlite::Connection; | ||
|
|
||
| use super::{EXPECTED_SCHEMA_HASHES, MIGRATION_SCRIPTS, apply_migrations, schema_hash}; | ||
| use super::{ | ||
| MIGRATION_SCRIPTS, | ||
| PINNED_SCHEMA_HASHES, | ||
| apply_migrations, | ||
| compute_expected_schema_hashes, | ||
| schema_hash, | ||
| }; | ||
| use crate::db_management::errors::SqliteStoreError; | ||
|
|
||
| fn hash_of(schema: &str) -> super::Hash { | ||
| let conn = Connection::open_in_memory().unwrap(); | ||
| conn.execute_batch(schema).unwrap(); | ||
| schema_hash(&conn).unwrap() | ||
| } | ||
|
|
||
| #[test] | ||
| fn honest_database_reopens_without_error() { | ||
| let mut conn = Connection::open_in_memory().unwrap(); | ||
|
|
@@ -217,7 +324,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] | ||
|
|
@@ -241,7 +369,37 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn expected_schema_hash_per_migration() { | ||
| assert_eq!(EXPECTED_SCHEMA_HASHES.len(), MIGRATION_SCRIPTS.len()); | ||
| fn migration_schema_hashes_are_stable() { | ||
| let replayed = compute_expected_schema_hashes() | ||
| .into_iter() | ||
| .map(String::from) | ||
| .collect::<Vec<_>>(); | ||
| 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." | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn schema_hash_ignores_comment_edits() { | ||
| let documented = hash_of( | ||
| "CREATE TABLE items ( | ||
| id INTEGER PRIMARY KEY, -- the identifier | ||
| /* the payload */ | ||
| value TEXT | ||
| );", | ||
| ); | ||
| let reworded = hash_of( | ||
| "CREATE TABLE items ( | ||
| id INTEGER PRIMARY KEY, -- a completely different explanation | ||
| value TEXT | ||
| );", | ||
| ); | ||
|
|
||
| assert_eq!(documented, reworded); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's just inline this function where it's called (it's only one place AFAICT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0e10af0