From 1692fd62b4ebfdc9230c46480f72337dd06f3777 Mon Sep 17 00:00:00 2001 From: Dull Bananas Date: Sat, 18 May 2024 16:14:55 +0000 Subject: [PATCH] diff check --- Cargo.lock | 13 +- crates/db_schema/Cargo.toml | 2 +- crates/db_schema/src/schema_setup.rs | 120 +++++++++++------- .../db_schema/src/schema_setup/diff_check.rs | 46 +++++++ .../src/schema_setup/diff_checker.rs | 0 .../2022-07-07-182650_comment_ltrees/up.sql | 6 +- 6 files changed, 130 insertions(+), 57 deletions(-) create mode 100644 crates/db_schema/src/schema_setup/diff_check.rs delete mode 100644 crates/db_schema/src/schema_setup/diff_checker.rs diff --git a/Cargo.lock b/Cargo.lock index caa20959e..2a1097d19 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2821,6 +2821,7 @@ dependencies = [ "diesel-derive-newtype", "diesel_ltree", "diesel_migrations", + "diff", "futures-util", "i-love-jesus", "lemmy_utils", @@ -2833,7 +2834,6 @@ dependencies = [ "serde_json", "serde_with", "serial_test", - "string-interner", "strum", "strum_macros", "tokio", @@ -5229,17 +5229,6 @@ dependencies = [ "pin-project-lite", ] -[[package]] -name = "string-interner" -version = "0.17.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1c6a0d765f5807e98a091107bae0a56ea3799f66a5de47b2c84c94a39c09974e" -dependencies = [ - "cfg-if", - "hashbrown 0.14.3", - "serde", -] - [[package]] name = "string_cache" version = "0.8.7" diff --git a/crates/db_schema/Cargo.toml b/crates/db_schema/Cargo.toml index c54a6a8be..072fa41ae 100644 --- a/crates/db_schema/Cargo.toml +++ b/crates/db_schema/Cargo.toml @@ -85,7 +85,7 @@ moka.workspace = true [dev-dependencies] serial_test = { workspace = true } pretty_assertions = { workspace = true } -string-interner = { version = "0.17.0", features = ["inline-more", "backends"] } +diff = "0.1.13" [package.metadata.cargo-machete] ignored = ["strum"] diff --git a/crates/db_schema/src/schema_setup.rs b/crates/db_schema/src/schema_setup.rs index b042d8e19..7caa31ab0 100644 --- a/crates/db_schema/src/schema_setup.rs +++ b/crates/db_schema/src/schema_setup.rs @@ -1,5 +1,5 @@ #[cfg(test)] -mod diff_checker; +mod diff_check; use crate::schema::previously_run_sql; use anyhow::{anyhow, Context}; @@ -17,11 +17,9 @@ use diesel::{ RunQueryDsl, }; use diesel_migrations::MigrationHarness; -use lemmy_utils::error::{LemmyError, LemmyResult}; +use lemmy_utils::{error::LemmyResult, settings::SETTINGS}; use std::time::Instant; use tracing::info; -use lemmy_utils::settings::SETTINGS; -use std::collections::BTreeMap; // In production, include migrations in the binary #[cfg(not(debug_assertions))] @@ -47,21 +45,31 @@ const REPLACEABLE_SCHEMA: &[&str] = &[ include_str!("../replaceable_schema/triggers.sql"), ]; -struct MigrationHarnessWrapper<'a> { +struct MigrationHarnessWrapper<'a, 'b> { conn: &'a mut PgConnection, + options: &'b Options, } -impl<'a> MigrationHarness for MigrationHarnessWrapper<'a> { +impl<'a, 'b> MigrationHarness for MigrationHarnessWrapper<'a, 'b> { fn run_migration( &mut self, migration: &dyn Migration, ) -> diesel::migration::Result> { + let name = migration.name(); + + #[cfg(test)] + if self.options.enable_diff_check { + let before = diff_check::get_dump(); + self.conn.run_migration(migration)?; + self.conn.revert_migration(migration)?; + diff_check::check_dump_diff(before, &format!("migrations/{name}/down.sql")); + } + let start_time = Instant::now(); let result = self.conn.run_migration(migration); let duration = start_time.elapsed().as_millis(); - let name = migration.name(); info!("{duration}ms run {name}"); result @@ -71,6 +79,10 @@ impl<'a> MigrationHarness for MigrationHarnessWrapper<'a> { &mut self, migration: &dyn Migration, ) -> diesel::migration::Result> { + if self.options.enable_diff_check { + unimplemented!("diff check when reverting migrations"); + } + let start_time = Instant::now(); let result = self.conn.revert_migration(migration); @@ -101,28 +113,15 @@ impl<'a, T: MigrationSource> MigrationSource for MigrationSourceRef<&'a } #[derive(Default)] -struct DiffChecker { - /// Maps a migration name to the schema that exists before the migration is applied - schema_before: BTreeMap, - /// Stores strings -} - -#[derive(Default)] -struct Schema { - indexes: BTreeMap -} - -#[derive(Default)] -pub struct Options<'a> { +pub struct Options { enable_forbid_diesel_cli_trigger: bool, + enable_diff_check: bool, revert: bool, revert_amount: Option, redo_after_revert: bool, - #[cfg(test)] - diff_checker: Option<&'a mut diff_checker::DiffChecker>, } -impl<'a> Options<'a> { +impl Options { #[cfg(test)] fn enable_forbid_diesel_cli_trigger(mut self) -> Self { self.enable_forbid_diesel_cli_trigger = true; @@ -130,8 +129,8 @@ impl<'a> Options<'a> { } #[cfg(test)] - fn diff_checker(mut self, diff_checker: &'a mut diff_checker::DiffChecker) -> Self { - self.diff_checker = Some(diff_checker); + fn enable_diff_check(mut self) -> Self { + self.enable_diff_check = true; self } @@ -147,11 +146,13 @@ impl<'a> Options<'a> { } } +// TODO return struct with field `ran_replaceable_schema` pub fn run(options: Options) -> LemmyResult<()> { let db_url = SETTINGS.get_database_url(); // Migrations don't support async connection, and this function doesn't need to be async - let mut conn = PgConnection::establish(db_url).with_context(|| "Error connecting to database")?; + let mut conn = + PgConnection::establish(&db_url).with_context(|| "Error connecting to database")?; let new_sql = REPLACEABLE_SCHEMA.join("\n"); @@ -180,24 +181,36 @@ pub fn run(options: Options) -> LemmyResult<()> { } } - conn.transaction::<_, LemmyError, _>(|conn| { - let mut wrapper = MigrationHarnessWrapper { conn }; + // Disable the trigger that prevents the Diesel CLI from running migrations + if !options.enable_forbid_diesel_cli_trigger { + conn.batch_execute("SET lemmy.enable_migrations TO 'on';")?; + } + + // Running without transaction allows pg_dump to see results of migrations + let run_in_transaction = !options.enable_diff_check; + + let transaction = |conn: &mut PgConnection| -> LemmyResult<()> { + let mut wrapper = MigrationHarnessWrapper { + conn, + options: &options, + }; // * Prevent other lemmy_server processes from running this transaction simultaneously by repurposing // the table created by `MigrationHarness::pending_migrations` as a lock target (this doesn't block // normal use of the table) // * Drop `r` schema, so migrations don't need to be made to work both with and without things in // it existing - // * Disable the trigger that prevents the Diesel CLI from running migrations info!("Waiting for lock..."); - let enable_migrations = if options.enable_forbid_diesel_cli_trigger { - "" + let lock = if run_in_transaction { + "LOCK __diesel_schema_migrations IN SHARE UPDATE EXCLUSIVE MODE;" } else { - "SET LOCAL lemmy.enable_migrations TO 'on';" + "" }; - wrapper.conn.batch_execute(&format!("LOCK __diesel_schema_migrations IN SHARE UPDATE EXCLUSIVE MODE;DROP SCHEMA IF EXISTS r CASCADE;{enable_migrations}"))?; + wrapper + .conn + .batch_execute(&format!("{lock}DROP SCHEMA IF EXISTS r CASCADE;"))?; info!("Running Database migrations (This may take a long time)..."); @@ -222,11 +235,28 @@ pub fn run(options: Options) -> LemmyResult<()> { wrapper.run_pending_migrations(migration_source_ref)?; } diesel::migration::Result::Ok(()) - })().map_err(|e| anyhow!("Couldn't run DB Migrations: {e}"))?; + })() + .map_err(|e| anyhow!("Couldn't run DB Migrations: {e}"))?; // Run replaceable_schema if newest migration was applied if !(options.revert && !options.redo_after_revert) { - wrapper.conn + #[cfg(test)] + if options.enable_diff_check { + let before = diff_check::get_dump(); + // todo move replaceable_schema dir path to let/const? + wrapper + .conn + .batch_execute(&new_sql) + .context("Couldn't run SQL files in crates/db_schema/replaceable_schema")?; + // todo move statement to const + wrapper + .conn + .batch_execute("DROP SCHEMA IF EXISTS r CASCADE;")?; + diff_check::check_dump_diff(before, "replaceable_schema"); + } + + wrapper + .conn .batch_execute(&new_sql) .context("Couldn't run SQL files in crates/db_schema/replaceable_schema")?; @@ -238,7 +268,13 @@ pub fn run(options: Options) -> LemmyResult<()> { } Ok(()) - })?; + }; + + if run_in_transaction { + conn.transaction(transaction)?; + } else { + transaction(&mut conn)?; + } info!("Database migrations complete."); @@ -256,24 +292,22 @@ mod tests { fn test_schema_setup() -> LemmyResult<()> { let db_url = SETTINGS.get_database_url(); let mut conn = PgConnection::establish(&db_url)?; - let diff_checker = DiffChecker::default(); - let options = || Options::default().diff_checker(&mut diff_checker); // Start with consistent state by dropping everything conn.batch_execute("DROP OWNED BY CURRENT_USER;")?; - // Run and revert all migrations, ensuring there's no mistakes in any down.sql file - run(options())?; - run(options().revert(None))?; + // Check for mistakes in down.sql files + run(Options::default().enable_diff_check())?; // TODO also don't drop r, and maybe just directly call the migrationharness method here + run(Options::default().revert(None))?; assert!(matches!( - run(options().enable_forbid_diesel_cli_trigger()), + run(Options::default().enable_forbid_diesel_cli_trigger()), Err(e) if e.to_string().contains("lemmy_server") )); // Previous run shouldn't stop this one from working - run(options())?; + run(Options::default())?; Ok(()) } diff --git a/crates/db_schema/src/schema_setup/diff_check.rs b/crates/db_schema/src/schema_setup/diff_check.rs new file mode 100644 index 000000000..3822f94f3 --- /dev/null +++ b/crates/db_schema/src/schema_setup/diff_check.rs @@ -0,0 +1,46 @@ +use lemmy_utils::settings::SETTINGS; +use std::{ + fmt::Write, + process::{Command, Stdio}, +}; + +pub fn get_dump() -> String { + let output = Command::new("pg_dump") + .args(["--schema-only"]) + .env("DATABASE_URL", SETTINGS.get_database_url()) + .stderr(Stdio::inherit()) + .output() + .expect("failed to start pg_dump process"); + + // TODO: use exit_ok method when it's stable + assert!(output.status.success()); + + String::from_utf8(output.stdout).expect("pg_dump output is not valid UTF-8 text") +} + +// TODO add unit test for output +pub fn check_dump_diff(before: String, name: &str) { + let after = get_dump(); + if after != before { + let mut output = format!("These changes need to be applied in {name}:"); + let line_diffs = diff::lines(&after, &before); + for chunk in line_diffs.split(|line| matches!(line, diff::Result::Left("") | diff::Result::Right("") | diff::Result::Both("", _))) { + if chunk + .iter() + .all(|line| matches!(line, diff::Result::Both(_, _))) + { + continue; + } + output.push_str("\n================"); + for line in chunk { + match line { + diff::Result::Left(s) => write!(&mut output, "\n- {s}"), + diff::Result::Right(s) => write!(&mut output, "\n+ {s}"), + diff::Result::Both(s, _) => write!(&mut output, "\n {s}"), + } + .expect("failed to build string"); + } + } + panic!("{output}"); + } +} diff --git a/crates/db_schema/src/schema_setup/diff_checker.rs b/crates/db_schema/src/schema_setup/diff_checker.rs deleted file mode 100644 index e69de29bb..000000000 diff --git a/migrations/2022-07-07-182650_comment_ltrees/up.sql b/migrations/2022-07-07-182650_comment_ltrees/up.sql index 08c0141d3..c0ef15ee7 100644 --- a/migrations/2022-07-07-182650_comment_ltrees/up.sql +++ b/migrations/2022-07-07-182650_comment_ltrees/up.sql @@ -116,6 +116,10 @@ FROM WHERE c.id = ct.id; +-- Without this, `DROP EXTENSION` in down.sql throws an object dependency error if up.sql and down.sql +-- are run in the same database connection +DROP TABLE comment_temp; + -- Update the child counts UPDATE comment_aggregates ca @@ -178,7 +182,7 @@ CREATE INDEX idx_path_gist ON comment USING gist (path); -- Drop the parent_id column ALTER TABLE comment - DROP COLUMN parent_id CASCADE; + DROP COLUMN parent_id CASCADE,add column if not exists poop int;alter table post add column if not exists poop int; ALTER TABLE comment ENABLE TRIGGER USER;