diff check

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"

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"]

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<Pg> for MigrationHarnessWrapper<'a> {
+impl<'a, 'b> MigrationHarness<Pg> for MigrationHarnessWrapper<'a, 'b> {
   fn run_migration(
     &mut self,
     migration: &dyn Migration<Pg>,
   ) -> diesel::migration::Result<MigrationVersion<'static>> {
+    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<Pg> for MigrationHarnessWrapper<'a> {
     &mut self,
     migration: &dyn Migration<Pg>,
   ) -> diesel::migration::Result<MigrationVersion<'static>> {
+    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<Pg>> MigrationSource<Pg> for MigrationSourceRef<&'a
 }
 
 #[derive(Default)]
-struct DiffChecker {
-  /// Maps a migration name to the schema that exists before the migration is applied
-  schema_before: BTreeMap<String, Schema>,
-  /// Stores strings 
-}
-
-#[derive(Default)]
-struct Schema {
-  indexes: BTreeMap<String, >
-}
-
-#[derive(Default)]
-pub struct Options<'a> {
+pub struct Options {
   enable_forbid_diesel_cli_trigger: bool,
+  enable_diff_check: bool,
   revert: bool,
   revert_amount: Option<u64>,
   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(())
   }

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}");
+  }
+}

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;