From e682e2dd729afa5c7b33ae3152a5aca1c0590e17 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Mon, 30 Jan 2023 14:38:48 -0800 Subject: [PATCH] Changed SQLez migrations to be executed eagerly Added fix for terminal working directory's sometimes getting lost co-authored-by: Kay --- Cargo.lock | 1 + crates/sqlez/Cargo.toml | 5 +- crates/sqlez/src/migrations.rs | 71 ++++++++++++++++++++++++- crates/sqlez/src/typed_statements.rs | 36 +++++++++++++ crates/terminal_view/src/persistence.rs | 20 +++++++ 5 files changed, 130 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 914e61226d..8c7f08bf9b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6015,6 +6015,7 @@ dependencies = [ "libsqlite3-sys", "parking_lot 0.11.2", "smol", + "sqlez_macros", "thread_local", "uuid 1.2.2", ] diff --git a/crates/sqlez/Cargo.toml b/crates/sqlez/Cargo.toml index 716ec76644..f247f3e537 100644 --- a/crates/sqlez/Cargo.toml +++ b/crates/sqlez/Cargo.toml @@ -15,4 +15,7 @@ thread_local = "1.1.4" lazy_static = "1.4" parking_lot = "0.11.1" futures = "0.3" -uuid = { version = "1.1.2", features = ["v4"] } \ No newline at end of file +uuid = { version = "1.1.2", features = ["v4"] } + +[dev-dependencies] +sqlez_macros = { path = "../sqlez_macros"} \ No newline at end of file diff --git a/crates/sqlez/src/migrations.rs b/crates/sqlez/src/migrations.rs index 41c505f85b..b3aaef95b1 100644 --- a/crates/sqlez/src/migrations.rs +++ b/crates/sqlez/src/migrations.rs @@ -4,12 +4,36 @@ // to creating a new db?) // Otherwise any missing migrations are run on the connection -use anyhow::{anyhow, Result}; +use std::ffi::CString; + +use anyhow::{anyhow, Context, Result}; use indoc::{formatdoc, indoc}; +use libsqlite3_sys::sqlite3_exec; use crate::connection::Connection; impl Connection { + fn eager_exec(&self, sql: &str) -> anyhow::Result<()> { + let sql_str = CString::new(sql).context("Error creating cstr")?; + unsafe { + sqlite3_exec( + self.sqlite3, + sql_str.as_c_str().as_ptr(), + None, + 0 as *mut _, + 0 as *mut _, + ); + } + self.last_error() + .with_context(|| format!("Prepare call failed for query:\n{}", sql))?; + + Ok(()) + } + + /// Migrate the database, for the given domain. + /// Note: Unlike everything else in SQLez, migrations are run eagerly, without first + /// preparing the SQL statements. This makes it possible to do multi-statement schema + /// updates in a single string without running into prepare errors. pub fn migrate(&self, domain: &'static str, migrations: &[&'static str]) -> Result<()> { self.with_savepoint("migrating", || { // Setup the migrations table unconditionally @@ -47,7 +71,7 @@ impl Connection { } } - self.exec(migration)?()?; + self.eager_exec(migration)?; store_completed_migration((domain, index, *migration))?; } @@ -59,6 +83,7 @@ impl Connection { #[cfg(test)] mod test { use indoc::indoc; + use sqlez_macros::sql; use crate::connection::Connection; @@ -257,4 +282,46 @@ mod test { // Verify new migration returns error when run assert!(second_migration_result.is_err()) } + + #[test] + fn test_create_alter_drop() { + let connection = Connection::open_memory(Some("test_create_alter_drop")); + + connection + .migrate( + "first_migration", + &[sql!( CREATE TABLE table1(a TEXT) STRICT; )], + ) + .unwrap(); + + connection + .exec(sql!( INSERT INTO table1(a) VALUES ("test text"); )) + .unwrap()() + .unwrap(); + + connection + .migrate( + "second_migration", + &[sql!( + CREATE TABLE table2(b TEXT) STRICT; + + INSERT INTO table2 (b) + SELECT a FROM table1; + + DROP TABLE table1; + + ALTER TABLE table2 RENAME TO table1; + )], + ) + .unwrap(); + + let res = &connection + .select::(sql!( + SELECT b FROM table1 + )) + .unwrap()() + .unwrap()[0]; + + assert_eq!(res, "test text"); + } } diff --git a/crates/sqlez/src/typed_statements.rs b/crates/sqlez/src/typed_statements.rs index df4a2987b5..488ee27c0c 100644 --- a/crates/sqlez/src/typed_statements.rs +++ b/crates/sqlez/src/typed_statements.rs @@ -7,11 +7,23 @@ use crate::{ }; impl Connection { + /// Prepare a statement which has no bindings and returns nothing. + /// + /// Note: If there are multiple statements that depend upon each other + /// (such as those which make schema changes), preparation will fail. + /// Use a true migration instead. pub fn exec<'a>(&'a self, query: &str) -> Result Result<()>> { let mut statement = Statement::prepare(self, query)?; Ok(move || statement.exec()) } + /// Prepare a statement which takes a binding, but returns nothing. + /// The bindings for a given invocation should be passed to the returned + /// closure + /// + /// Note: If there are multiple statements that depend upon each other + /// (such as those which make schema changes), preparation will fail. + /// Use a true migration instead. pub fn exec_bound<'a, B: Bind>( &'a self, query: &str, @@ -20,6 +32,11 @@ impl Connection { Ok(move |bindings| statement.with_bindings(bindings)?.exec()) } + /// Prepare a statement which has no bindings and returns a `Vec`. + /// + /// Note: If there are multiple statements that depend upon each other + /// (such as those which make schema changes), preparation will fail. + /// Use a true migration instead. pub fn select<'a, C: Column>( &'a self, query: &str, @@ -28,6 +45,11 @@ impl Connection { Ok(move || statement.rows::()) } + /// Prepare a statement which takes a binding and returns a `Vec`. + /// + /// Note: If there are multiple statements that depend upon each other + /// (such as those which make schema changes), preparation will fail. + /// Use a true migration instead. pub fn select_bound<'a, B: Bind, C: Column>( &'a self, query: &str, @@ -36,6 +58,13 @@ impl Connection { Ok(move |bindings| statement.with_bindings(bindings)?.rows::()) } + /// Prepare a statement that selects a single row from the database. + /// Will return none if no rows are returned and will error if more than + /// 1 row + /// + /// Note: If there are multiple statements that depend upon each other + /// (such as those which make schema changes), preparation will fail. + /// Use a true migration instead. pub fn select_row<'a, C: Column>( &'a self, query: &str, @@ -44,6 +73,13 @@ impl Connection { Ok(move || statement.maybe_row::()) } + /// Prepare a statement which takes a binding and selects a single row + /// from the database. WIll return none if no rows are returned and will + /// error if more than 1 row is returned. + /// + /// Note: If there are multiple statements that depend upon each other + /// (such as those which make schema changes), preparation will fail. + /// Use a true migration instead. pub fn select_row_bound<'a, B: Bind, C: Column>( &'a self, query: &str, diff --git a/crates/terminal_view/src/persistence.rs b/crates/terminal_view/src/persistence.rs index 26bd0931fe..0da9ed4729 100644 --- a/crates/terminal_view/src/persistence.rs +++ b/crates/terminal_view/src/persistence.rs @@ -14,6 +14,26 @@ define_connection! { FOREIGN KEY(workspace_id) REFERENCES workspaces(workspace_id) ON DELETE CASCADE ) STRICT; + ), + // Remove the unique constraint on the item_id table + // SQLite doesn't have a way of doing this automatically, so + // we have to do this silly copying. + sql!( + CREATE TABLE terminals2 ( + workspace_id INTEGER, + item_id INTEGER, + working_directory BLOB, + PRIMARY KEY(workspace_id, item_id), + FOREIGN KEY(workspace_id) REFERENCES workspaces(workspace_id) + ON DELETE CASCADE + ) STRICT; + + INSERT INTO terminals2 (workspace_id, item_id, working_directory) + SELECT workspace_id, item_id, working_directory FROM terminals; + + DROP TABLE terminals; + + ALTER TABLE terminals2 RENAME TO terminals; )]; }