From 22ac8c5bfc1e882b943d164c907e9929438b4afb Mon Sep 17 00:00:00 2001 From: Dull Bananas Date: Sun, 12 May 2024 22:10:31 +0000 Subject: [PATCH] use trigger on migrations table --- crates/db_schema/src/schema.rs | 1 + crates/db_schema/src/schema_setup.rs | 36 ++++--------------- .../000000000000000_forbid_diesel_cli/up.sql | 6 ---- .../down.sql | 2 ++ .../up.sql | 29 +++++++++++++++ 5 files changed, 39 insertions(+), 35 deletions(-) delete mode 100644 migrations/000000000000000_forbid_diesel_cli/up.sql create mode 100644 migrations/2024-04-29-012112_forbid_diesel_cli/down.sql create mode 100644 migrations/2024-04-29-012112_forbid_diesel_cli/up.sql diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index aff17e5f6..2adba2ab2 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -1099,6 +1099,7 @@ diesel::allow_tables_to_appear_in_same_query!( post_read, post_report, post_saved, + previously_run_sql, private_message, private_message_report, received_activity, diff --git a/crates/db_schema/src/schema_setup.rs b/crates/db_schema/src/schema_setup.rs index 57fcfa05d..6e4f05293 100644 --- a/crates/db_schema/src/schema_setup.rs +++ b/crates/db_schema/src/schema_setup.rs @@ -19,7 +19,7 @@ use lemmy_utils::error::{LemmyError, LemmyResult}; use std::time::Instant; use tracing::info; -const EMBEDDED_MIGRATIONS: EmbeddedMigrations = embed_migrations!(); +const MIGRATIONS: EmbeddedMigrations = embed_migrations!(); /// This SQL code sets up the `r` schema, which contains things that can be safely dropped and replaced /// instead of being changed using migrations. It may not create or modify things outside of the `r` schema @@ -30,34 +30,10 @@ const REPLACEABLE_SCHEMA: &[&str] = &[ include_str!("../replaceable_schema/triggers.sql"), ]; -const REVERT_REPLACEABLE_SCHEMA: &str = "DROP SCHEMA IF EXISTS r CASCADE;"; - -const LOCK_STATEMENT: &str = "LOCK __diesel_schema_migrations IN SHARE UPDATE EXCLUSIVE MODE;"; - -struct Migrations; - -impl MigrationSource for Migrations { - fn migrations(&self) -> diesel::migration::Result>>> { - let mut migrations = EMBEDDED_MIGRATIONS.migrations()?; - let skipped_migration = if migrations.is_empty() { - None - } else { - Some(migrations.remove(0)) - }; - - debug_assert_eq!( - skipped_migration.map(|m| m.name().to_string()), - Some("000000000000000_forbid_diesel_cli".to_string()) - ); - - Ok(migrations) - } -} - fn get_pending_migrations(conn: &mut PgConnection) -> LemmyResult>>> { Ok( conn - .pending_migrations(Migrations) + .pending_migrations(MIGRATIONS) .map_err(|e| anyhow::anyhow!("Couldn't determine pending migrations: {e}"))?, ) } @@ -97,14 +73,16 @@ pub fn run(db_url: &str) -> LemmyResult<()> { // lemmy_server processes from running this transaction concurrently. This lock does not block // `MigrationHarness::pending_migrations` (`SELECT`) or `MigrationHarness::run_migration` (`INSERT`). info!("Waiting for lock..."); - conn.batch_execute(LOCK_STATEMENT)?; + conn.batch_execute("LOCK __diesel_schema_migrations IN SHARE UPDATE EXCLUSIVE MODE;")?; info!("Running Database migrations (This may take a long time)..."); // Check pending migrations again after locking let pending_migrations = get_pending_migrations(conn)?; - // Run migrations, without stuff from replaceable_schema - conn.batch_execute(REVERT_REPLACEABLE_SCHEMA)?; + // Drop `r` schema and disable the trigger that prevents the Diesel CLI from running migrations + conn.batch_execute( + "DROP SCHEMA IF EXISTS r CASCADE; SET LOCAL lemmy.enable_migrations TO 'on';", + )?; for migration in &pending_migrations { let name = migration.name(); diff --git a/migrations/000000000000000_forbid_diesel_cli/up.sql b/migrations/000000000000000_forbid_diesel_cli/up.sql deleted file mode 100644 index 48e3f4287..000000000 --- a/migrations/000000000000000_forbid_diesel_cli/up.sql +++ /dev/null @@ -1,6 +0,0 @@ -DO $$ -BEGIN - RAISE 'migrations must be managed using lemmy_server instead of diesel CLI'; -END -$$; - diff --git a/migrations/2024-04-29-012112_forbid_diesel_cli/down.sql b/migrations/2024-04-29-012112_forbid_diesel_cli/down.sql new file mode 100644 index 000000000..d6e8132be --- /dev/null +++ b/migrations/2024-04-29-012112_forbid_diesel_cli/down.sql @@ -0,0 +1,2 @@ +DROP FUNCTION forbid_diesel_cli CASCADE; + diff --git a/migrations/2024-04-29-012112_forbid_diesel_cli/up.sql b/migrations/2024-04-29-012112_forbid_diesel_cli/up.sql new file mode 100644 index 000000000..e2e54f5bd --- /dev/null +++ b/migrations/2024-04-29-012112_forbid_diesel_cli/up.sql @@ -0,0 +1,29 @@ +-- This trigger prevents using the Diesel CLI to run or revert migrations, so the custom migration runner +-- can drop and recreate the `r` schema for new migrations. +-- +-- This migration being seperate from the next migration (created in the same PR) guarantees that the +-- Diesel CLI will fail to bring the number of pending migrations to 0, which is one of the conditions +-- required to skip running replaceable_schema. +-- +-- If the Diesel CLI could run or revert migrations, this scenario would be possible: +-- +-- Run `diesel migration redo` when the newest migration has a new table with triggers. End up with triggers +-- being dropped and not replaced because triggers are created outside of up.sql. The custom migration runner +-- sees that there are no pending migrations and the value in the `previously_run_sql` trigger is correct, so +-- it doesn't rebuild the `r` schema. There is now incorrect behavior but no error messages. +CREATE FUNCTION forbid_diesel_cli () + RETURNS TRIGGER + LANGUAGE plpgsql + AS $$ +BEGIN + IF current_setting('lemmy.enable_migrations', TRUE) IS DISTINCT FROM 'on' THEN + RAISE 'migrations must be managed using lemmy_server instead of diesel CLI'; + END IF; + RETURN NULL; +END; +$$; + +CREATE TRIGGER forbid_diesel_cli + BEFORE INSERT OR UPDATE OR DELETE OR TRUNCATE ON __diesel_schema_migrations + EXECUTE FUNCTION forbid_diesel_cli (); +