From 31dd432ebe14c2ae2a8b23117c382f83af09b629 Mon Sep 17 00:00:00 2001 From: James Kominick Date: Tue, 4 Aug 2026 08:47:14 -0400 Subject: [PATCH 1/3] add migration checksums, enforce apply ordering, and rework the settings and migrator API - add `id`, `checksum`, and `applied_at` columns to `__migrant_migrations`; record a sha256 of each migration's up SQL (`None` for `FnMigration`) via `Migratable::checksum`, and revert the last-applied migration by recorded order on `Down` - error on an applied tag missing from the defined set or an out-of-order apply; add `Migrator::allow_unknown_tags` and `allow_out_of_order` to opt out - sort discovered migrations by `(stamp, tag)` for a deterministic total order, so same-second migrations no longer resolve nondeterministically - make `SqliteSettingsBuilder`/`ServerSettingsBuilder` `database_path` and `migration_location` infallible, deferring path validation to `build()`, and reject non-UTF-8 paths in `init` - rename `migrant_lib::new` to `create_migration` returning `NewMigration`; move `list` to `migrant_lib::cli::list` - take `Direction` by value in `Migratable::description` and `cli::edit`; seal `MigratableClone` - add the `sha2` dependency The `__migrant_migrations` schema change is a breaking on-disk change; existing databases need the table upgrade noted in CHANGELOG.md. --- Cargo.lock | 1 + Cargo.toml | 1 + migrant_lib/Cargo.toml | 1 + migrant_lib/README.md | 2 +- .../examples/embedded_cli_compatible.rs | 6 +- migrant_lib/examples/embedded_programmable.rs | 6 +- .../examples/migrant_cli_compatible.rs | 8 +- .../examples/settings_file_programmable.rs | 8 +- migrant_lib/src/config/builders.rs | 116 ++++- migrant_lib/src/config/init.rs | 190 +++++++- migrant_lib/src/config/mod.rs | 39 +- migrant_lib/src/drivers/mod.rs | 63 ++- migrant_lib/src/drivers/mysql.rs | 33 +- migrant_lib/src/drivers/pg.rs | 41 +- migrant_lib/src/drivers/sqlite.rs | 50 +- migrant_lib/src/errors.rs | 11 + migrant_lib/src/lib.rs | 10 +- migrant_lib/src/migratable.rs | 26 +- migrant_lib/src/migration.rs | 116 ++++- migrant_lib/src/migrator.rs | 410 +++++++++++++--- migrant_lib/src/ops.rs | 150 +++++- migrant_lib/tests/server_dbs.rs | 189 ++++++++ migrant_lib/tests/sqlite.rs | 453 +++++++++++++++++- 23 files changed, 1733 insertions(+), 197 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e993336..8ad1609 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1605,6 +1605,7 @@ dependencies = [ "postgres-native-tls", "rusqlite", "serde", + "sha2 0.10.9", "tempfile", "thiserror 2.0.18", "toml", diff --git a/Cargo.toml b/Cargo.toml index 3883d36..ba409d1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,7 @@ percent-encoding = "2" postgres = "0.19" postgres-native-tls = "0.5" serde = { version = "1", features = ["derive"] } +sha2 = "0.10" tempfile = "3" thiserror = "2" toml = "0.9" diff --git a/migrant_lib/Cargo.toml b/migrant_lib/Cargo.toml index 8666874..ada3689 100644 --- a/migrant_lib/Cargo.toml +++ b/migrant_lib/Cargo.toml @@ -22,6 +22,7 @@ chrono = { workspace = true } log = { workspace = true } percent-encoding = { workspace = true } serde = { workspace = true } +sha2 = { workspace = true } thiserror = { workspace = true } toml = { workspace = true } url = { workspace = true } diff --git a/migrant_lib/README.md b/migrant_lib/README.md index 2fa0deb..fa98268 100644 --- a/migrant_lib/README.md +++ b/migrant_lib/README.md @@ -98,7 +98,7 @@ config.setup()?; Migration management identical to the [`migrant`](https://github.com/jaemk/migrant) CLI tool can also be embedded. This method only supports file-based migrations (so `FileMigration`s or `EmbeddedMigration`s using `include_str!`) and those migration files names must be timestamped with the format `[0-9]{14}_[a-z0-9-]+`, -Properly named files can be generated by `migrant_lib::new` or the `migrant` CLI tool. +Properly named files can be generated by `migrant_lib::create_migration` or the `migrant` CLI tool. This is required because migration order is implied by file names which must follow a specific format and contain a valid timestamp. diff --git a/migrant_lib/examples/embedded_cli_compatible.rs b/migrant_lib/examples/embedded_cli_compatible.rs index 7167cc0..3d5dab5 100644 --- a/migrant_lib/examples/embedded_cli_compatible.rs +++ b/migrant_lib/examples/embedded_cli_compatible.rs @@ -20,7 +20,7 @@ use std::env; fn run() -> Result<(), Box> { let path = env::current_dir()?; let path = path.join("db/embedded_example.db"); - let settings = Settings::configure_sqlite().database_path(&path)?.build()?; + let settings = Settings::configure_sqlite().database_path(&path).build()?; let mut config = Config::with_settings(settings); @@ -65,7 +65,7 @@ fn run() -> Result<(), Box> { .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; println!("\nUnapplying migrations..."); Migrator::with_config(&config) @@ -74,7 +74,7 @@ fn run() -> Result<(), Box> { .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; Ok(()) } diff --git a/migrant_lib/examples/embedded_programmable.rs b/migrant_lib/examples/embedded_programmable.rs index 13fd6c5..6755803 100644 --- a/migrant_lib/examples/embedded_programmable.rs +++ b/migrant_lib/examples/embedded_programmable.rs @@ -61,7 +61,7 @@ mod migrations { fn run() -> Result<(), Box> { let path = env::current_dir()?; let path = path.join("db/embedded_example.db"); - let settings = Settings::configure_sqlite().database_path(&path)?.build()?; + let settings = Settings::configure_sqlite().database_path(&path).build()?; let mut config = Config::with_settings(settings); config.setup()?; @@ -105,7 +105,7 @@ fn run() -> Result<(), Box> { .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; println!("\nUnapplying migrations..."); Migrator::with_config(&config) @@ -114,7 +114,7 @@ fn run() -> Result<(), Box> { .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; Ok(()) } diff --git a/migrant_lib/examples/migrant_cli_compatible.rs b/migrant_lib/examples/migrant_cli_compatible.rs index 8862e3e..a3d7fa1 100644 --- a/migrant_lib/examples/migrant_cli_compatible.rs +++ b/migrant_lib/examples/migrant_cli_compatible.rs @@ -26,8 +26,8 @@ fn run() -> Result<(), migrant_lib::Error> { Config::init_in(&dir) .with_sqlite_options( SqliteSettingsBuilder::empty() - .database_path("db/db.db")? - .migration_location("migrations/managed")?, + .database_path("db/db.db") + .migration_location("migrations/managed"), ) // .with_postgres_options( // PostgresSettingsBuilder::empty() @@ -65,7 +65,7 @@ fn run() -> Result<(), migrant_lib::Error> { .all(true) .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; println!("Unapplying all migrations..."); migrant_lib::Migrator::with_config(&config) @@ -73,7 +73,7 @@ fn run() -> Result<(), migrant_lib::Error> { .all(true) .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; Ok(()) } diff --git a/migrant_lib/examples/settings_file_programmable.rs b/migrant_lib/examples/settings_file_programmable.rs index f2954fb..69775c6 100644 --- a/migrant_lib/examples/settings_file_programmable.rs +++ b/migrant_lib/examples/settings_file_programmable.rs @@ -23,8 +23,8 @@ fn run() -> Result<(), Box> { Config::init_in(&dir) .with_sqlite_options( SqliteSettingsBuilder::empty() - .database_path("db/db.db")? - .migration_location("migrations/managed")?, + .database_path("db/db.db") + .migration_location("migrations/managed"), ) .interactive(false) .initialize()?; @@ -59,7 +59,7 @@ fn run() -> Result<(), Box> { .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; println!("\nUnapplying migrations..."); Migrator::with_config(&config) @@ -68,7 +68,7 @@ fn run() -> Result<(), Box> { .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; Ok(()) } diff --git a/migrant_lib/src/config/builders.rs b/migrant_lib/src/config/builders.rs index ce0d161..8a28f5e 100644 --- a/migrant_lib/src/config/builders.rs +++ b/migrant_lib/src/config/builders.rs @@ -19,8 +19,8 @@ fn path_to_string(p: &Path) -> Result { /// Sqlite settings builder #[derive(Debug, Clone, Default)] pub struct SqliteSettingsBuilder { - pub(crate) database_path: Option, - pub(crate) migration_location: Option, + pub(crate) database_path: Option, + pub(crate) migration_location: Option, } impl SqliteSettingsBuilder { @@ -38,9 +38,12 @@ impl SqliteSettingsBuilder { /// `Config::init_in(...).with_sqlite_options(...)`, a relative path is also accepted -- /// it is written into the generated settings file and resolved relative to that /// settings file's directory when the config is later loaded. - pub fn database_path>(mut self, p: T) -> Result { - self.database_path = Some(path_to_string(p.as_ref())?); - Ok(self) + /// + /// The path is validated (must be valid UTF-8, and absolute when used with + /// `build()`) when `build()` is called. + pub fn database_path>(mut self, p: T) -> Self { + self.database_path = Some(p.as_ref().to_path_buf()); + self } /// Use an in-memory database. @@ -49,7 +52,7 @@ impl SqliteSettingsBuilder { /// (shared by all clones of the built `Config`) so migrations and /// application queries all see the same database. pub fn memory(mut self) -> Self { - self.database_path = Some(SQLITE_MEMORY_PATH.to_string()); + self.database_path = Some(PathBuf::from(SQLITE_MEMORY_PATH)); self } @@ -58,17 +61,18 @@ impl SqliteSettingsBuilder { /// This can be an absolute or relative path. An absolute path should be preferred. /// If a relative path is provided, the path will be assumed relative to either the /// settings file's directory if a settings file exists, or the current directory. - pub fn migration_location>(mut self, p: T) -> Result { - self.migration_location = Some(path_to_string(p.as_ref())?); - Ok(self) + pub fn migration_location>(mut self, p: T) -> Self { + self.migration_location = Some(p.as_ref().to_path_buf()); + self } /// Build a `Settings` object pub fn build(&self) -> Result { let database_path = self .database_path - .clone() + .as_deref() .ok_or_else(|| err!(Config, "Missing `database_path` parameter"))?; + let database_path = path_to_string(database_path)?; if database_path != SQLITE_MEMORY_PATH && !Path::new(&database_path).is_absolute() { bail!( Config, @@ -76,9 +80,14 @@ impl SqliteSettingsBuilder { database_path ) } + let migration_location = self + .migration_location + .as_deref() + .map(path_to_string) + .transpose()?; Ok(Settings::new(DbSettings::Sqlite(SqliteSettings { database_path, - migration_location: self.migration_location.clone(), + migration_location, }))) } } @@ -93,7 +102,7 @@ pub(crate) struct ServerSettingsBuilder { pub(crate) database_port: Option, pub(crate) database_params: Option>, pub(crate) ssl_cert_file: Option, - pub(crate) migration_location: Option, + pub(crate) migration_location: Option, } impl ServerSettingsBuilder { @@ -115,7 +124,11 @@ impl ServerSettingsBuilder { database_port: self.database_port.clone(), database_params: self.database_params.clone(), ssl_cert_file: self.ssl_cert_file.clone(), - migration_location: self.migration_location.clone(), + migration_location: self + .migration_location + .as_deref() + .map(path_to_string) + .transpose()?, }) } @@ -172,9 +185,11 @@ macro_rules! server_builder_methods { /// This can be an absolute or relative path. An absolute path should be preferred. /// If a relative path is provided, the path will be assumed relative to either the /// settings file's directory if a settings file exists, or the current directory. - pub fn migration_location>(mut self, p: T) -> Result { - self.inner.migration_location = Some(path_to_string(p.as_ref())?); - Ok(self) + /// + /// A non-UTF-8 path surfaces as an error when `build()` is called. + pub fn migration_location>(mut self, p: T) -> Self { + self.inner.migration_location = Some(p.as_ref().to_path_buf()); + self } }; } @@ -229,16 +244,15 @@ impl MySqlSettingsBuilder { mod tests { use super::*; - // The owned (`self -> Self` / `self -> Result`) setters must chain - // without an intermediate `mut` binding and carry every value through. + // The owned (`self -> Self`) setters must chain without an intermediate + // `mut` binding and carry every value through. The path setters are now + // infallible; validation is deferred to `build()`. #[test] fn sqlite_owned_setters_chain_and_build() { let settings = SqliteSettingsBuilder::empty() .database_path("/abs/path/to/my.db") - .unwrap() .migration_location("/abs/migrations") - .unwrap() .build() .unwrap(); match settings.inner { @@ -254,7 +268,10 @@ mod tests { fn sqlite_memory_owned_setter_chains() { // `memory()` consumes and returns owned self, chainable into `build()`. let builder = SqliteSettingsBuilder::empty().memory(); - assert_eq!(builder.database_path.as_deref(), Some(SQLITE_MEMORY_PATH)); + assert_eq!( + builder.database_path.as_deref(), + Some(Path::new(SQLITE_MEMORY_PATH)) + ); let settings = builder.build().unwrap(); match settings.inner { DbSettings::Sqlite(s) => assert_eq!(s.database_path, SQLITE_MEMORY_PATH), @@ -273,7 +290,6 @@ mod tests { .database_params(&[("sslmode", "require")]) .ssl_cert_file("/certs/db.pem") .migration_location("/abs/migrations") - .unwrap() .build() .unwrap(); match settings.inner { @@ -294,6 +310,62 @@ mod tests { } } + #[test] + fn sqlite_relative_database_path_is_rejected_at_build_time() { + // The setter itself is infallible; the absolute-path requirement is + // enforced when `build()` runs. + let builder = SqliteSettingsBuilder::empty().database_path("relative/path/my.db"); + let err = builder.build().unwrap_err(); + assert!(err.is_config(), "expected a Config error, got {:?}", err); + assert!( + err.to_string().contains("must be absolute"), + "error should explain the absolute-path requirement: {}", + err + ); + } + + #[cfg(unix)] + #[test] + fn sqlite_non_utf8_database_path_is_rejected_at_build_time() { + // A non-UTF-8 path is accepted by the infallible setter but rejected by + // `build()` (paths are stored as strings in the settings file). + let bad = non_utf8_path(); + let builder = SqliteSettingsBuilder::empty().database_path(&bad); + let err = builder.build().unwrap_err(); + assert!( + matches!(err, Error::PathError(_)), + "expected a PathError for a non-utf8 path, got {:?}", + err + ); + } + + #[cfg(unix)] + #[test] + fn server_non_utf8_migration_location_is_rejected_at_build_time() { + let bad = non_utf8_path(); + let err = PostgresSettingsBuilder::empty() + .database_name("mydb") + .database_user("me") + .database_password("secret") + .migration_location(&bad) + .build() + .unwrap_err(); + assert!( + matches!(err, Error::PathError(_)), + "expected a PathError for a non-utf8 migration location, got {:?}", + err + ); + } + + /// A path whose bytes are not valid UTF-8, for validating `build()`-time + /// UTF-8 checks. + #[cfg(unix)] + fn non_utf8_path() -> PathBuf { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + PathBuf::from(OsStr::from_bytes(&[0x2f, 0x66, 0x6f, 0x80, 0x6f])) + } + #[test] fn mysql_owned_server_setters_chain_and_build() { let settings = MySqlSettingsBuilder::empty() diff --git a/migrant_lib/src/config/init.rs b/migrant_lib/src/config/init.rs index 5d40845..ecef940 100644 --- a/migrant_lib/src/config/init.rs +++ b/migrant_lib/src/config/init.rs @@ -90,34 +90,49 @@ pub struct SettingsFileInitializer { } /// Template value resolution: explicit value, `env:VAR` placeholder, or fallback -fn value_or(explicit: Option<&String>, with_env: bool, env_var: &str, fallback: &str) -> String { +fn value_or(explicit: Option<&str>, with_env: bool, env_var: &str, fallback: &str) -> String { match explicit { - Some(v) => v.clone(), + Some(v) => v.to_string(), None if with_env => format!("env:{}", env_var), None => fallback.to_string(), } } +/// Resolve an optional path option to its UTF-8 string form for template +/// substitution. +/// +/// An explicitly-provided non-UTF-8 path is rejected with a `PathError`, mirroring +/// the `SqliteSettingsBuilder::build()` / `ServerSettingsBuilder::build()` +/// validation so `init` and `build()` agree. A genuinely-absent option yields +/// `None`, so the normal env/fallback default still applies. +fn path_opt_str(opt: Option<&Path>) -> Result> { + opt.map(|p| { + p.to_str() + .ok_or_else(|| err!(PathError, "Unicode path error: {:?}", p)) + }) + .transpose() +} + /// Render the pg/mysql shared template values fn render_server_template( template: &str, opts: &ServerSettingsBuilder, with_env: bool, default_port: &str, -) -> String { +) -> Result { let mut content = template .replace( "__DB_NAME__", - &value_or(opts.database_name.as_ref(), with_env, "DATABASE_NAME", ""), + &value_or(opts.database_name.as_deref(), with_env, "DATABASE_NAME", ""), ) .replace( "__DB_USER__", - &value_or(opts.database_user.as_ref(), with_env, "DATABASE_USER", ""), + &value_or(opts.database_user.as_deref(), with_env, "DATABASE_USER", ""), ) .replace( "__DB_PASS__", &value_or( - opts.database_password.as_ref(), + opts.database_password.as_deref(), with_env, "DATABASE_PASSWORD", "", @@ -126,7 +141,7 @@ fn render_server_template( .replace( "__DB_HOST__", &value_or( - opts.database_host.as_ref(), + opts.database_host.as_deref(), with_env, "DATABASE_HOST", "localhost", @@ -135,7 +150,7 @@ fn render_server_template( .replace( "__DB_PORT__", &value_or( - opts.database_port.as_ref(), + opts.database_port.as_deref(), with_env, "DATABASE_PORT", default_port, @@ -144,7 +159,7 @@ fn render_server_template( .replace( "__MIG_LOC__", &value_or( - opts.migration_location.as_ref(), + path_opt_str(opts.migration_location.as_deref())?, with_env, "MIGRATION_LOCATION", "migrations", @@ -159,7 +174,7 @@ fn render_server_template( None => content.push('\n'), } content.push('\n'); - content + Ok(content) } impl SettingsFileInitializer { @@ -197,7 +212,7 @@ impl SettingsFileInitializer { /// Config::init_in(env::current_dir()?) /// .with_sqlite_options( /// SqliteSettingsBuilder::empty() - /// .database_path("/abs/path/to/my.db")?) + /// .database_path("/abs/path/to/my.db")) /// .initialize()?; /// # Ok(()) /// # } @@ -297,15 +312,15 @@ impl SettingsFileInitializer { .map_err(|_| err!(Config, "unsupported database type: {}", db_kind))?; Ok(match db_kind { DbKind::Sqlite => { - let options = SqliteSettingsBuilder::empty().migration_location("migrations")?; + let options = SqliteSettingsBuilder::empty().migration_location("migrations"); DatabaseConfigOptions::Sqlite(options) } DbKind::Postgres => { - let options = PostgresSettingsBuilder::empty().migration_location("migrations")?; + let options = PostgresSettingsBuilder::empty().migration_location("migrations"); DatabaseConfigOptions::Postgres(options) } DbKind::MySql => { - let options = MySqlSettingsBuilder::empty().migration_location("migrations")?; + let options = MySqlSettingsBuilder::empty().migration_location("migrations"); DatabaseConfigOptions::MySql(options) } }) @@ -357,13 +372,13 @@ impl SettingsFileInitializer { &opts.inner, self.with_env_defaults, "5432", - ), + )?, DatabaseConfigOptions::MySql(ref opts) => render_server_template( MYSQL_CONFIG_TEMPLATE, &opts.inner, self.with_env_defaults, "3306", - ), + )?, DatabaseConfigOptions::Sqlite(ref opts) => { let config_dir = config_path.parent().and_then(Path::to_str).ok_or_else(|| { err!( @@ -377,7 +392,7 @@ impl SettingsFileInitializer { .replace( "__DB_PATH__", &value_or( - opts.database_path.as_ref(), + path_opt_str(opts.database_path.as_deref())?, self.with_env_defaults, "DATABASE_PATH", "", @@ -386,7 +401,7 @@ impl SettingsFileInitializer { .replace( "__MIG_LOC__", &value_or( - opts.migration_location.as_ref(), + path_opt_str(opts.migration_location.as_deref())?, self.with_env_defaults, "MIGRATION_LOCATION", "migrations", @@ -423,3 +438,142 @@ impl SettingsFileInitializer { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + /// The template renderer's `value_or(... Path::to_str ...)` resolution. + #[test] + fn value_or_resolves_explicit_env_and_fallback() { + assert_eq!(value_or(Some("v"), false, "VAR", "fallback"), "v"); + // env-defaults on and no explicit value -> `env:VAR` placeholder + assert_eq!(value_or(None, true, "VAR", "fallback"), "env:VAR"); + // no explicit value, no env-defaults -> the fallback + assert_eq!(value_or(None, false, "VAR", "fallback"), "fallback"); + // an explicit value wins even with env-defaults on + assert_eq!(value_or(Some("v"), true, "VAR", "fallback"), "v"); + } + + /// A non-UTF-8 path explicitly provided to sqlite `init` is rejected with a + /// `PathError`, mirroring `SqliteSettingsBuilder::build()`, rather than + /// silently falling back to a default. No settings file is written. + #[cfg(unix)] + #[test] + fn sqlite_init_rejects_non_utf8_paths() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let bad = std::path::PathBuf::from(OsStr::from_bytes(&[0x2f, 0x66, 0x6f, 0x80, 0x6f])); + let dir = tempfile::tempdir().unwrap(); + + // A non-UTF-8 database_path is rejected. + let err = SettingsFileInitializer::new(dir.path()) + .interactive(false) + .with_sqlite_options(SqliteSettingsBuilder::empty().database_path(&bad)) + .initialize() + .unwrap_err(); + assert!( + matches!(err, Error::PathError(_)), + "non-utf8 database_path must be rejected with a PathError, got {err:?}" + ); + + // A non-UTF-8 migration_location is rejected too (with a valid db path). + let err = SettingsFileInitializer::new(dir.path()) + .interactive(false) + .with_sqlite_options( + SqliteSettingsBuilder::empty() + .database_path("/abs/db.db") + .migration_location(&bad), + ) + .initialize() + .unwrap_err(); + assert!( + matches!(err, Error::PathError(_)), + "non-utf8 migration_location must be rejected with a PathError, got {err:?}" + ); + + // The failed init did not leave a settings file behind. + assert!( + !dir.path().join(crate::CONFIG_FILE).exists(), + "no settings file should be written when a path is rejected" + ); + } + + /// The same rejection applies to the shared pg/mysql server template's + /// `migration_location`. + #[cfg(unix)] + #[test] + fn server_init_rejects_non_utf8_migration_location() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let bad = std::path::PathBuf::from(OsStr::from_bytes(&[0x2f, 0x66, 0x6f, 0x80, 0x6f])); + let dir = tempfile::tempdir().unwrap(); + + let err = SettingsFileInitializer::new(dir.path()) + .interactive(false) + .with_postgres_options( + PostgresSettingsBuilder::empty() + .database_name("db") + .database_user("me") + .database_password("secret") + .migration_location(&bad), + ) + .initialize() + .unwrap_err(); + assert!( + matches!(err, Error::PathError(_)), + "non-utf8 server migration_location must be rejected with a PathError, got {err:?}" + ); + assert!( + !dir.path().join(crate::CONFIG_FILE).exists(), + "no settings file should be written when a path is rejected" + ); + } + + /// Ordinary UTF-8 paths -- and omitted path options -- still render and write + /// the settings file successfully, using explicit values where given and the + /// normal defaults where absent. + #[test] + fn init_writes_template_for_utf8_and_omitted_paths() { + // sqlite with explicit UTF-8 paths + let dir = tempfile::tempdir().unwrap(); + SettingsFileInitializer::new(dir.path()) + .interactive(false) + .with_sqlite_options( + SqliteSettingsBuilder::empty() + .database_path("db/db.db") + .migration_location("migrations/managed"), + ) + .initialize() + .expect("utf8 sqlite init succeeds"); + let written = std::fs::read_to_string(dir.path().join(crate::CONFIG_FILE)).unwrap(); + assert!( + written.contains(r#"database_path = "db/db.db""#), + "{written}" + ); + assert!( + written.contains(r#"migration_location = "migrations/managed""#), + "{written}" + ); + + // postgres with migration_location omitted -> normal default is used + let dir = tempfile::tempdir().unwrap(); + SettingsFileInitializer::new(dir.path()) + .interactive(false) + .with_postgres_options( + PostgresSettingsBuilder::empty() + .database_name("db") + .database_user("me") + .database_password("secret"), + ) + .initialize() + .expect("postgres init with omitted migration_location succeeds"); + let written = std::fs::read_to_string(dir.path().join(crate::CONFIG_FILE)).unwrap(); + assert!( + written.contains(r#"migration_location = "migrations""#), + "omitted migration_location uses its default: {written}" + ); + } +} diff --git a/migrant_lib/src/config/mod.rs b/migrant_lib/src/config/mod.rs index 2912500..3476cec 100644 --- a/migrant_lib/src/config/mod.rs +++ b/migrant_lib/src/config/mod.rs @@ -14,7 +14,7 @@ use crate::drivers::DbConnection; use crate::errors::*; use crate::macros::{bail, err}; use crate::migratable::Migratable; -use crate::{tags, DbKind, DT_FORMAT, SQLITE_MEMORY_PATH}; +use crate::{tags, DbKind, SQLITE_MEMORY_PATH}; mod builders; mod init; @@ -80,8 +80,8 @@ impl Config { /// # fn main() { run().unwrap(); } /// # fn run() -> Result<(), Box> { /// let settings = Settings::configure_sqlite() - /// .database_path("/absolute/path/to/db.db")? - /// .migration_location("/absolute/path/to/migration_dir")? + /// .database_path("/absolute/path/to/db.db") + /// .migration_location("/absolute/path/to/migration_dir") /// .build()?; /// let config = Config::with_settings(settings); /// // Setup migrations table @@ -342,7 +342,7 @@ impl Config { /// Check that migration tags conform to naming requirements. /// If CLI compatibility is enabled, then tags must be prefixed with a timestamp /// following: `[0-9]{14}_[a-z0-9-]+` which is the format generated by the migrant - /// CLI tool and `migrant_lib::new`. When CLI compatibility is disabled (default), + /// CLI tool and `migrant_lib::create_migration`. When CLI compatibility is disabled (default), /// tags may only contain `[a-z0-9-]`, but can still be optionally prefixed with /// a timestamp following: `([0-9]{14}_)?[a-z0-9-]+`. fn check_saved_tag(&self, tag: &str) -> Result<()> { @@ -407,7 +407,11 @@ impl Config { Ok(()) } - /// Load the applied migrations from the database migration table + /// Load the applied migrations from the database migration table. + /// + /// The tags are returned in recorded application order (`order by id`), + /// which is authoritative -- no re-sorting is done. Each tag is still + /// validated against the active naming rules. pub(crate) fn load_applied(&self) -> Result> { if !self.migration_table_exists()? { bail!( @@ -420,23 +424,7 @@ impl Config { for tag in &applied { self.check_saved_tag(tag)?; } - if !self.cli_compatible { - return Ok(applied); - } - // Applied cli-compatible (timestamp-prefixed) tags are ordered chronologically - let mut stamped = applied - .into_iter() - .map(|tag| { - let stamp = tag - .split('_') - .next() - .ok_or_else(|| err!(TagError, "Invalid tag format: {:?}", tag))?; - let stamp = chrono::NaiveDateTime::parse_from_str(stamp, DT_FORMAT)?; - Ok((stamp, tag)) - }) - .collect::>>()?; - stamped.sort_by_key(|(stamp, _)| *stamp); - Ok(stamped.into_iter().map(|(_, tag)| tag).collect()) + Ok(applied) } /// Check if a `__migrant_migrations` table exists @@ -444,9 +432,10 @@ impl Config { self.with_conn(|conn| conn.migration_table_exists()) } - /// Insert given tag into database migration table - pub(crate) fn insert_migration_tag(&self, tag: &str) -> Result<()> { - self.with_conn(|conn| conn.insert_tag(tag)) + /// Insert given tag (and its optional checksum) into the database migration + /// table. `applied_at` is populated by the column default. + pub(crate) fn insert_migration_tag(&self, tag: &str, checksum: Option<&str>) -> Result<()> { + self.with_conn(|conn| conn.insert_tag(tag, checksum)) } /// Remove a given tag from the database migration table diff --git a/migrant_lib/src/drivers/mod.rs b/migrant_lib/src/drivers/mod.rs index 210370e..e5c3297 100644 --- a/migrant_lib/src/drivers/mod.rs +++ b/migrant_lib/src/drivers/mod.rs @@ -14,15 +14,22 @@ use crate::DbKind; #[allow(dead_code)] // per-backend statements are unused when their feature is disabled pub(crate) mod sql { - pub static CREATE_TABLE: &str = "create table __migrant_migrations(tag text unique);"; - pub static MYSQL_CREATE_TABLE: &str = - "create table __migrant_migrations(tag varchar(512) unique);"; - - pub static GET_MIGRATIONS: &str = "select tag from __migrant_migrations;"; + // The bookkeeping table carries, besides the migration `tag`: a surrogate + // `id` whose ascending order is the authoritative recorded application + // order, an optional `checksum` (lowercase hex sha256 of the raw up-direction + // SQL, NULL for programmatic migrations that have no SQL to hash), and an + // `applied_at` timestamp populated by the column default. + pub static PG_CREATE_TABLE: &str = "create table __migrant_migrations(id serial primary key, tag text unique not null, checksum text, applied_at timestamptz not null default now());"; + pub static SQLITE_CREATE_TABLE: &str = "create table __migrant_migrations(id integer primary key autoincrement, tag text unique not null, checksum text, applied_at timestamp not null default current_timestamp);"; + pub static MYSQL_CREATE_TABLE: &str = "create table __migrant_migrations(id integer primary key auto_increment, tag varchar(512) unique not null, checksum text null, applied_at timestamp not null default current_timestamp);"; + + // Recorded application order is authoritative, so order by the surrogate id. + pub static GET_MIGRATIONS: &str = "select tag from __migrant_migrations order by id;"; pub static INSERT_MIGRATION_PG_SQLITE: &str = - "insert into __migrant_migrations (tag) values ($1)"; + "insert into __migrant_migrations (tag, checksum) values ($1, $2)"; pub static REMOVE_MIGRATION_PG_SQLITE: &str = "delete from __migrant_migrations where tag = $1"; - pub static INSERT_MIGRATION_MYSQL: &str = "insert into __migrant_migrations (tag) values (?)"; + pub static INSERT_MIGRATION_MYSQL: &str = + "insert into __migrant_migrations (tag, checksum) values (?, ?)"; pub static REMOVE_MIGRATION_MYSQL: &str = "delete from __migrant_migrations where tag = ?"; pub static SQLITE_MIGRATION_TABLE_EXISTS: &str = "select exists(select 1 from sqlite_master where type = 'table' and name = '__migrant_migrations');"; @@ -139,9 +146,10 @@ impl DbConnection { dispatch!(self, c => c.applied_tags()) } - /// Record a migration tag as applied - pub(crate) fn insert_tag(&mut self, tag: &str) -> Result<()> { - dispatch!(self, c => c.insert_tag(tag)) + /// Record a migration tag as applied, along with its optional checksum + /// (`applied_at` is populated by the column default) + pub(crate) fn insert_tag(&mut self, tag: &str, checksum: Option<&str>) -> Result<()> { + dispatch!(self, c => c.insert_tag(tag, checksum)) } /// Remove a migration tag from the applied set @@ -199,4 +207,39 @@ mod tests { sql::MYSQL_MIGRATION_TABLE_EXISTS ); } + + /// Every backend's create-table statement must carry the `tag`, `checksum`, + /// and `applied_at` columns, and applied tags must be selected in recorded + /// order (`order by id`) so the recorded application order is authoritative. + #[test] + fn create_table_statements_carry_checksum_and_applied_at() { + for (name, ddl) in [ + ("pg", sql::PG_CREATE_TABLE), + ("sqlite", sql::SQLITE_CREATE_TABLE), + ("mysql", sql::MYSQL_CREATE_TABLE), + ] { + assert!( + ddl.contains("tag"), + "{name} ddl must have a tag column: {ddl}" + ); + assert!( + ddl.contains("checksum"), + "{name} ddl must have a checksum column: {ddl}" + ); + assert!( + ddl.contains("applied_at"), + "{name} ddl must have an applied_at column: {ddl}" + ); + } + assert!( + sql::GET_MIGRATIONS.contains("order by id"), + "GET_MIGRATIONS must order by id: {}", + sql::GET_MIGRATIONS + ); + assert!( + sql::INSERT_MIGRATION_PG_SQLITE.contains("checksum") + && sql::INSERT_MIGRATION_MYSQL.contains("checksum"), + "inserts must carry the checksum column" + ); + } } diff --git a/migrant_lib/src/drivers/mysql.rs b/migrant_lib/src/drivers/mysql.rs index 8b47bd8..8d138f8 100644 --- a/migrant_lib/src/drivers/mysql.rs +++ b/migrant_lib/src/drivers/mysql.rs @@ -51,8 +51,9 @@ impl MySqlConn { Ok(self.conn.query(sql::GET_MIGRATIONS)?) } - pub(crate) fn insert_tag(&mut self, tag: &str) -> Result<()> { - self.conn.exec_drop(sql::INSERT_MIGRATION_MYSQL, (tag,))?; + pub(crate) fn insert_tag(&mut self, tag: &str, checksum: Option<&str>) -> Result<()> { + self.conn + .exec_drop(sql::INSERT_MIGRATION_MYSQL, (tag, checksum))?; Ok(()) } @@ -142,10 +143,30 @@ mod tests { assert!(!conn.setup_migration_table().unwrap(), "setup idempotent"); assert!(conn.migration_table_exists().unwrap(), "table exists"); - conn.insert_tag("initial").unwrap(); - conn.insert_tag("alter1").unwrap(); - conn.insert_tag("alter2").unwrap(); - assert_eq!(3, conn.applied_tags().unwrap().len()); + conn.insert_tag("initial", Some("abc123")).unwrap(); + conn.insert_tag("alter1", None).unwrap(); + conn.insert_tag("alter2", Some("def456")).unwrap(); + // Recorded order is authoritative: tags come back in insertion (id) order. + assert_eq!( + vec!["initial", "alter1", "alter2"], + conn.applied_tags().unwrap() + ); + + // The checksum column carries the inserted value (NULL where None). + let checksums: Vec> = conn + .conn + .query("select checksum from __migrant_migrations order by id") + .unwrap(); + assert_eq!( + vec![Some("abc123".to_string()), None, Some("def456".to_string())], + checksums + ); + // `applied_at` is populated by the column default. + let stamped: Option = conn + .conn + .query_first("select count(*) from __migrant_migrations where applied_at is not null") + .unwrap(); + assert_eq!(Some(3), stamped); conn.remove_tag("alter2").unwrap(); assert_eq!(2, conn.applied_tags().unwrap().len()); diff --git a/migrant_lib/src/drivers/pg.rs b/migrant_lib/src/drivers/pg.rs index b2e3ff3..e4352df 100644 --- a/migrant_lib/src/drivers/pg.rs +++ b/migrant_lib/src/drivers/pg.rs @@ -94,7 +94,7 @@ impl PgConn { if self.migration_table_exists()? { return Ok(false); } - self.client.execute(sql::CREATE_TABLE, &[])?; + self.client.execute(sql::PG_CREATE_TABLE, &[])?; Ok(true) } @@ -103,9 +103,9 @@ impl PgConn { Ok(rows.iter().map(|row| row.get(0)).collect()) } - pub(crate) fn insert_tag(&mut self, tag: &str) -> Result<()> { + pub(crate) fn insert_tag(&mut self, tag: &str, checksum: Option<&str>) -> Result<()> { self.client - .execute(sql::INSERT_MIGRATION_PG_SQLITE, &[&tag])?; + .execute(sql::INSERT_MIGRATION_PG_SQLITE, &[&tag, &checksum])?; Ok(()) } @@ -229,10 +229,37 @@ mod tests { assert!(!conn.setup_migration_table().unwrap(), "setup idempotent"); assert!(conn.migration_table_exists().unwrap(), "table exists"); - conn.insert_tag("initial").unwrap(); - conn.insert_tag("alter1").unwrap(); - conn.insert_tag("alter2").unwrap(); - assert_eq!(3, conn.applied_tags().unwrap().len()); + conn.insert_tag("initial", Some("abc123")).unwrap(); + conn.insert_tag("alter1", None).unwrap(); + conn.insert_tag("alter2", Some("def456")).unwrap(); + // Recorded order is authoritative: tags come back in insertion (id) order. + assert_eq!( + vec!["initial", "alter1", "alter2"], + conn.applied_tags().unwrap() + ); + + // The checksum column carries the value we inserted (and NULL where None). + let checksums: Vec> = conn + .client + .query("select checksum from __migrant_migrations order by id", &[]) + .unwrap() + .iter() + .map(|row| row.get(0)) + .collect(); + assert_eq!( + vec![Some("abc123".to_string()), None, Some("def456".to_string())], + checksums + ); + // `applied_at` is populated by the column default. + let stamped: i64 = conn + .client + .query_one( + "select count(*) from __migrant_migrations where applied_at is not null", + &[], + ) + .unwrap() + .get(0); + assert_eq!(3, stamped); conn.remove_tag("alter2").unwrap(); assert_eq!(2, conn.applied_tags().unwrap().len()); diff --git a/migrant_lib/src/drivers/sqlite.rs b/migrant_lib/src/drivers/sqlite.rs index b93e736..165327d 100644 --- a/migrant_lib/src/drivers/sqlite.rs +++ b/migrant_lib/src/drivers/sqlite.rs @@ -59,7 +59,7 @@ impl SqliteConn { if self.migration_table_exists()? { return Ok(false); } - self.lock().execute(sql::CREATE_TABLE, [])?; + self.lock().execute(sql::SQLITE_CREATE_TABLE, [])?; Ok(true) } @@ -72,9 +72,11 @@ impl SqliteConn { Ok(tags) } - pub(crate) fn insert_tag(&self, tag: &str) -> Result<()> { - self.lock() - .execute(sql::INSERT_MIGRATION_PG_SQLITE, [tag])?; + pub(crate) fn insert_tag(&self, tag: &str, checksum: Option<&str>) -> Result<()> { + self.lock().execute( + sql::INSERT_MIGRATION_PG_SQLITE, + rusqlite::params![tag, checksum], + )?; Ok(()) } @@ -144,10 +146,42 @@ mod tests { assert!(!conn.setup_migration_table().unwrap(), "setup idempotent"); assert!(conn.migration_table_exists().unwrap(), "table exists"); - conn.insert_tag("initial").unwrap(); - conn.insert_tag("alter1").unwrap(); - conn.insert_tag("alter2").unwrap(); - assert_eq!(3, conn.applied_tags().unwrap().len()); + conn.insert_tag("initial", Some("abc123")).unwrap(); + conn.insert_tag("alter1", None).unwrap(); + conn.insert_tag("alter2", Some("def456")).unwrap(); + // Recorded order is authoritative: tags come back in insertion (id) order. + assert_eq!( + vec!["initial", "alter1", "alter2"], + conn.applied_tags().unwrap() + ); + + // The checksum column carries the inserted value (NULL where None). + let checksums: Vec> = { + let guard = conn.lock(); + let mut stmt = guard + .prepare("select checksum from __migrant_migrations order by id") + .unwrap(); + let rows = stmt + .query_map([], |row| row.get::<_, Option>(0)) + .unwrap() + .collect::, _>>() + .unwrap(); + rows + }; + assert_eq!( + vec![Some("abc123".to_string()), None, Some("def456".to_string())], + checksums + ); + // `applied_at` is populated by the column default. + let stamped: i64 = conn + .lock() + .query_row( + "select count(*) from __migrant_migrations where applied_at is not null", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(3, stamped); conn.remove_tag("alter2").unwrap(); assert_eq!(2, conn.applied_tags().unwrap().len()); diff --git a/migrant_lib/src/errors.rs b/migrant_lib/src/errors.rs index f037687..c529402 100644 --- a/migrant_lib/src/errors.rs +++ b/migrant_lib/src/errors.rs @@ -21,6 +21,11 @@ pub enum Error { #[error("MigrationNotFound: {0}")] MigrationNotFound(String), + /// An applied migration was recorded out of order relative to the + /// definition order of the available migrations + #[error("MigrationOrdering: {0}")] + MigrationOrdering(String), + /// Failure while running an external command (editor, database shell) #[error("ShellCommandError: {0}")] ShellCommand(String), @@ -89,6 +94,11 @@ impl Error { matches!(self, Error::MigrationNotFound(_)) } + /// `true` for [`Error::MigrationOrdering`] + pub fn is_migration_ordering(&self) -> bool { + matches!(self, Error::MigrationOrdering(_)) + } + /// `true` for [`Error::ShellCommand`] pub fn is_shell_command(&self) -> bool { matches!(self, Error::ShellCommand(_)) @@ -118,6 +128,7 @@ mod tests { fn predicates_match_their_variant() { assert!(Error::TagError("dup".to_string()).is_tag_error()); assert!(Error::MigrationNotFound("x".to_string()).is_migration_not_found()); + assert!(Error::MigrationOrdering("y".to_string()).is_migration_ordering()); assert!(Error::FeatureRequired("sqlite").is_feature_required()); } diff --git a/migrant_lib/src/lib.rs b/migrant_lib/src/lib.rs index 4800853..8943e18 100644 --- a/migrant_lib/src/lib.rs +++ b/migrant_lib/src/lib.rs @@ -106,7 +106,7 @@ config.setup()?; Migration management identical to the [`migrant`](https://github.com/jaemk/migrant) CLI tool can also be embedded. This method only supports file-based migrations (so `FileMigration`s or `EmbeddedMigration`s using `include_str!`) and those migration files names must be timestamped with the format `[0-9]{14}_[a-z0-9-]+`, -Properly named files can be generated by `migrant_lib::new` or the `migrant` CLI tool. +Properly named files can be generated by `migrant_lib::create_migration` or the `migrant` CLI tool. This is required because migration order is implied by file names which must follow a specific format and contain a valid timestamp. @@ -149,15 +149,17 @@ pub use crate::migratable::Migratable; pub use crate::migration::{noop, EmbeddedMigration, FileMigration, FnMigration}; pub use crate::migrator::{Direction, ForceMode, Migrator, Report}; pub use crate::ops::{ - list, migration_statuses, new, pending_migrations, search_for_settings_file, MigrationStatus, + create_migration, migration_statuses, pending_migrations, search_for_settings_file, + MigrationStatus, NewMigration, }; /// Interactive, terminal-oriented operations used by the `migrant` CLI. /// /// These prompt on stdin and/or spawn external programs (a database shell, an -/// editor), so they are kept out of the crate root and grouped here. +/// editor), print human-readable status to stdout, so they are kept out of the +/// crate root and grouped here. pub mod cli { - pub use crate::ops::{edit, shell}; + pub use crate::ops::{edit, list, shell}; } /// Re-export of the `rusqlite` crate used by this library, so downstream diff --git a/migrant_lib/src/migratable.rs b/migrant_lib/src/migratable.rs index 4b914ce..3d40b6e 100644 --- a/migrant_lib/src/migratable.rs +++ b/migrant_lib/src/migratable.rs @@ -6,8 +6,18 @@ use std::fmt; use crate::migrator::Direction; use crate::Config; -/// Helper trait so boxed `Migratable` trait objects can be cloned -pub trait MigratableClone { +mod private { + /// Sealing marker: only types in this crate that satisfy the blanket impl + /// below can implement [`MigratableClone`](super::MigratableClone). + pub trait Sealed {} +} +impl private::Sealed for T {} + +/// Helper trait so boxed `Migratable` trait objects can be cloned. +/// +/// This trait is sealed: it is implemented automatically for every +/// `'static + Migratable + Clone` type and cannot be implemented directly. +pub trait MigratableClone: private::Sealed { /// Clone into a new boxed trait object fn clone_migratable_box(&self) -> Box; } @@ -35,8 +45,18 @@ pub trait Migratable: MigratableClone { /// A unique identifying tag fn tag(&self) -> String; + /// The lowercase hex sha256 of this migration's raw up-direction SQL bytes, + /// recorded in the `checksum` column of `__migrant_migrations` when the + /// migration is applied. + /// + /// Defaults to `None`. Programmatic migrations (`FnMigration` and custom + /// implementations) have no SQL to hash, so they store NULL by design. + fn checksum(&self) -> Option { + None + } + /// Optional migration description. Defaults to `Migratable::tag` - fn description(&self, _: &Direction) -> String { + fn description(&self, _: Direction) -> String { self.tag() } diff --git a/migrant_lib/src/migration.rs b/migrant_lib/src/migration.rs index a6ef729..80e343e 100644 --- a/migrant_lib/src/migration.rs +++ b/migrant_lib/src/migration.rs @@ -5,6 +5,7 @@ use std::borrow::Cow; use std::path::{Path, PathBuf}; use chrono::{DateTime, Utc}; +use sha2::{Digest, Sha256}; use crate::config::Config; use crate::connection::ConnConfig; @@ -24,6 +25,20 @@ use crate::DT_FORMAT; /// ``` pub(crate) const NO_TRANSACTION_DIRECTIVE: &str = "migrant:no-transaction"; +/// Compute the lowercase hex sha256 of the given raw bytes. Used to fingerprint +/// a migration's up-direction SQL for the `checksum` bookkeeping column. +pub(crate) fn sha256_hex(bytes: &[u8]) -> String { + let mut hasher = Sha256::new(); + hasher.update(bytes); + let digest = hasher.finalize(); + let mut out = String::with_capacity(digest.len() * 2); + for byte in digest { + use std::fmt::Write; + let _ = write!(out, "{:02x}", byte); + } + out +} + /// Return `true` if `sql` carries the [`NO_TRANSACTION_DIRECTIVE`] on a comment /// line. It is matched case-insensitively as the first token of a `--` line /// comment, so a trailing explanation is allowed @@ -156,7 +171,15 @@ impl Migratable for FileMigration { } } - fn description(&self, direction: &Direction) -> String { + fn checksum(&self) -> Option { + // Hash the raw bytes of the up file. A missing or unreadable file yields + // `None`; the apply path surfaces any real read error. + let up = self.up.as_ref()?; + let bytes = std::fs::read(up).ok()?; + Some(sha256_hex(&bytes)) + } + + fn description(&self, direction: Direction) -> String { let file = match direction { Direction::Up => &self.up, Direction::Down => &self.down, @@ -293,7 +316,12 @@ impl Migratable for EmbeddedMigration { self.tag.to_owned() } - fn description(&self, _: &Direction) -> String { + fn checksum(&self) -> Option { + // Hash the raw bytes of the up-direction SQL string, if any. + self.up.as_ref().map(|up| sha256_hex(up.as_bytes())) + } + + fn description(&self, _: Direction) -> String { self.tag() } @@ -413,7 +441,7 @@ where self.tag.to_owned() } - fn description(&self, _: &Direction) -> String { + fn description(&self, _: Direction) -> String { self.tag() } @@ -463,6 +491,88 @@ mod tests { assert!(!sql_opts_out_of_transaction("")); } + #[test] + fn embedded_checksum_is_sha256_hex_of_up_sql() { + let m = EmbeddedMigration::with_tag("m") + .up("create table t (x integer);") + .down("drop table t;"); + // Known sha256 of the exact up-direction bytes (lowercase hex). + assert_eq!( + Some(sha256_hex(b"create table t (x integer);")), + m.checksum() + ); + // The value is a 64-char lowercase hex string. + let sum = m.checksum().unwrap(); + assert_eq!(64, sum.len()); + assert!(sum + .chars() + .all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase())); + } + + #[test] + fn embedded_checksum_none_without_up() { + let m = EmbeddedMigration::with_tag("m").down("drop table t;"); + assert_eq!(None, m.checksum()); + } + + #[test] + fn checksum_is_deterministic_and_distinguishes_up_sql() { + // Same up-SQL -> identical checksum, regardless of tag or down-SQL: the + // checksum fingerprints only the up direction and is stable. + let a = EmbeddedMigration::with_tag("one") + .up("create table t (x integer);") + .down("drop table t;"); + let b = EmbeddedMigration::with_tag("two") + .up("create table t (x integer);") + .down("select 1;"); + assert_eq!(a.checksum(), b.checksum()); + + // Different up-SQL -> different checksum. + let c = EmbeddedMigration::with_tag("three").up("create table u (x integer);"); + assert_ne!(a.checksum(), c.checksum()); + + // A whitespace-only difference is still a different fingerprint (the raw + // bytes are hashed, no normalization). + let d = EmbeddedMigration::with_tag("four").up("create table t (x integer); "); + assert_ne!(a.checksum(), d.checksum()); + + // A `FileMigration` hashing the identical bytes yields the identical + // checksum as the embedded one (both hash raw up bytes the same way). + let dir = tempfile::tempdir().unwrap(); + let up = dir.path().join("up.sql"); + std::fs::write(&up, b"create table t (x integer);").unwrap(); + let filed = FileMigration::with_tag("filed").up(&up); + assert_eq!(a.checksum(), filed.checksum()); + } + + #[test] + fn fn_migration_checksum_is_none() { + let m = FnMigration::with_tag("f").up(noop).down(noop); + assert_eq!(None, m.checksum()); + } + + #[test] + fn file_checksum_hashes_up_file_bytes_and_none_when_missing() { + let dir = tempfile::tempdir().unwrap(); + let up = dir.path().join("up.sql"); + let down = dir.path().join("down.sql"); + std::fs::write(&up, b"select 1;").unwrap(); + std::fs::write(&down, b"select 2;").unwrap(); + + let m = FileMigration::with_tag("filed").up(&up).down(&down); + assert_eq!(Some(sha256_hex(b"select 1;")), m.checksum()); + + // A missing up file yields `None` (the apply path surfaces read errors). + let missing = FileMigration::with_tag("gone") + .up(dir.path().join("nope.sql")) + .down(&down); + assert_eq!(None, missing.checksum()); + + // No up file at all is also `None`. + let no_up = FileMigration::with_tag("noup").down(&down); + assert_eq!(None, no_up.checksum()); + } + #[test] fn embedded_use_transaction_is_per_direction_and_directive_wins() { // `up` opts out via directive, `down` does not: transactionality differs diff --git a/migrant_lib/src/migrator.rs b/migrant_lib/src/migrator.rs index 689ad3d..13979c4 100644 --- a/migrant_lib/src/migrator.rs +++ b/migrant_lib/src/migrator.rs @@ -149,6 +149,8 @@ pub struct Migrator { all: bool, show_output: bool, synchronized: bool, + allow_unknown_tags: bool, + allow_out_of_order: bool, } impl Migrator { @@ -162,6 +164,8 @@ impl Migrator { all: false, show_output: true, synchronized: true, + allow_unknown_tags: false, + allow_out_of_order: false, } } @@ -222,6 +226,33 @@ impl Migrator { self } + /// Allow applied migration tags that are not among the available + /// migrations. Default is `false`. + /// + /// By default an `Up` run aborts with [`Error::MigrationNotFound`] if the + /// database records a tag that is not in the managed/available set -- for + /// example a migration that was applied and later removed from the codebase, + /// which usually signals the wrong migration set or database. Set this to + /// `true` to tolerate such unknown tags and apply the remaining available + /// migrations anyway. + pub fn allow_unknown_tags(mut self, allow: bool) -> Self { + self.allow_unknown_tags = allow; + self + } + + /// Allow applied migrations that are out of order relative to definition + /// order. Default is `false`. + /// + /// By default an `Up` run aborts with [`Error::MigrationOrdering`] if a later + /// migration (in definition order) was applied while an earlier one was not + /// -- the situation that arises when a migration is merged behind others that + /// already ran. Set this to `true` to apply the intervening un-applied + /// migrations anyway. + pub fn allow_out_of_order(mut self, allow: bool) -> Self { + self.allow_out_of_order = allow; + self + } + /// Apply migrations using the current configuration. /// /// Returns a [`Report`] of the migration tags whose bookkeeping this run @@ -321,45 +352,111 @@ impl Migrator { } /// Return the next available up or down migration, excluding any tags - /// skipped earlier in this run (`ForceMode::SkipFailures`) + /// skipped earlier in this run (`ForceMode::SkipFailures`). + /// + /// For an `Up` run this first enforces the strictness checks (unknown applied + /// tags, out-of-order application) unless they have been opted out of. fn next_available<'a>( direction: Direction, available: &'a [Box], applied: &[String], skipped: &HashSet, + allow_unknown_tags: bool, + allow_out_of_order: bool, ) -> Result> { Ok(match direction { - Direction::Up => available - .iter() - .find(|m| !applied.contains(&m.tag()) && !skipped.contains(&m.tag())) - .map(AsRef::as_ref), + Direction::Up => { + Self::check_applied_consistency( + available, + applied, + skipped, + allow_unknown_tags, + allow_out_of_order, + )?; + available + .iter() + .find(|m| !applied.contains(&m.tag()) && !skipped.contains(&m.tag())) + .map(AsRef::as_ref) + } Direction::Down => { - if applied.is_empty() { - None - } else { - // Select the Down target by definition order: the last - // migration in `available` order whose tag is applied. The - // `applied` slice may be unordered (it comes from an - // unordered `select tag from __migrant_migrations` unless - // running in cli-compatible mode), so we must not rely on - // `applied.last()`. - if !available.iter().any(|m| applied.contains(&m.tag())) { - bail!( + // Recorded application order is authoritative, so the most + // recently applied migration is `applied.last()`. Walk backwards, + // skipping tags that failed earlier this run, and return the + // corresponding available migration. A target tag absent from the + // available set is a hard error, matching the previous behavior. + for tag in applied.iter().rev() { + if skipped.contains(tag) { + continue; + } + match available.iter().find(|m| &m.tag() == tag) { + Some(m) => return Ok(Some(m.as_ref())), + None => bail!( MigrationNotFound, "Applied migration not found in available migrations: {}", - applied[0] - ) + tag + ), } - available - .iter() - .rev() - .find(|m| applied.contains(&m.tag()) && !skipped.contains(&m.tag())) - .map(AsRef::as_ref) } + None } }) } + /// Enforce the `Up`-run strictness checks against the applied set. Tags in + /// `skipped` (failed earlier this run under `ForceMode::SkipFailures`) count + /// as neither applied nor blocking, so a `skip-failures` run is not + /// self-defeating. + fn check_applied_consistency( + available: &[Box], + applied: &[String], + skipped: &HashSet, + allow_unknown_tags: bool, + allow_out_of_order: bool, + ) -> Result<()> { + if !allow_unknown_tags { + for tag in applied { + if skipped.contains(tag) { + continue; + } + if !available.iter().any(|m| &m.tag() == tag) { + bail!( + MigrationNotFound, + "Applied migration `{}` is not among the available migrations. \ + Pass `allow_unknown_tags(true)` to ignore unknown applied tags.", + tag + ) + } + } + } + if !allow_out_of_order { + // Walk definition order tracking the first un-applied (and un-skipped) + // migration. An applied migration appearing after it was applied out + // of order. + let mut first_unapplied: Option = None; + for m in available { + let tag = m.tag(); + if skipped.contains(&tag) { + continue; + } + if applied.contains(&tag) { + if let Some(ref earlier) = first_unapplied { + bail!( + MigrationOrdering, + "Migration `{}` was applied out of order: it comes after `{}` \ + in definition order, which has not been applied. Pass \ + `allow_out_of_order(true)` to apply the intervening migrations.", + tag, + earlier + ) + } + } else if first_unapplied.is_none() { + first_unapplied = Some(tag); + } + } + } + Ok(()) + } + /// Try applying the next available migration in the specified `Direction` fn apply_next( &self, @@ -368,23 +465,29 @@ impl Migrator { lock_generation: Option, ) -> Result { let migrations = Self::available_migrations(config)?; - let next = - match Self::next_available(self.direction, &migrations, &config.applied, skipped)? { - Some(next) => next, - None => return Ok(Step::Complete), - }; + let next = match Self::next_available( + self.direction, + &migrations, + &config.applied, + skipped, + self.allow_unknown_tags, + self.allow_out_of_order, + )? { + Some(next) => next, + None => return Ok(Step::Complete), + }; self.print(&format!( "Applying[{}]: {}", self.direction, - next.description(&self.direction) + next.description(self.direction) )); let tag = next.tag(); if self.fake { self.println(" ✓ (fake)"); - self.record_tag(config, &tag)?; + self.record_tag(config, next)?; return Ok(Step::Applied(tag)); } @@ -396,7 +499,7 @@ impl Migrator { config.begin_transaction()?; } - match self.apply_and_record(config, next, &tag) { + match self.apply_and_record(config, next) { Ok(()) => { if transactional { config.commit_transaction()?; @@ -426,7 +529,7 @@ impl Migrator { self.check_lock_still_held(config, lock_generation)?; // The transaction (if any) was rolled back, so this // bookkeeping row stands alone. - self.record_tag(config, &tag)?; + self.record_tag(config, next)?; Ok(Step::Applied(tag)) } ForceMode::SkipFailures => { @@ -451,22 +554,23 @@ impl Migrator { &self, config: &Config, next: &dyn Migratable, - tag: &str, ) -> std::result::Result<(), String> { match self.direction { Direction::Up => next.apply_up(config), Direction::Down => next.apply_down(config), } .map_err(|e| e.to_string())?; - self.record_tag(config, tag).map_err(|e| e.to_string()) + self.record_tag(config, next).map_err(|e| e.to_string()) } /// Record the migration as applied (`Up`) or un-applied (`Down`) in the - /// `__migrant_migrations` table. - fn record_tag(&self, config: &Config, tag: &str) -> Result<()> { + /// `__migrant_migrations` table. An `Up` record carries the migration's + /// checksum (`None` for programmatic migrations, stored as NULL). + fn record_tag(&self, config: &Config, next: &dyn Migratable) -> Result<()> { + let tag = next.tag(); match self.direction { - Direction::Up => config.insert_migration_tag(tag), - Direction::Down => config.delete_migration_tag(tag), + Direction::Up => config.insert_migration_tag(&tag, next.checksum().as_deref()), + Direction::Down => config.delete_migration_tag(&tag), } } @@ -528,6 +632,16 @@ mod tests { strs.iter().map(|s| (*s).to_owned()).collect() } + /// Strict selection with both opt-outs off -- the default the migrator uses. + fn next_strict<'a>( + direction: Direction, + available: &'a [Box], + applied: &[String], + skipped: &HashSet, + ) -> Result> { + Migrator::next_available(direction, available, applied, skipped, false, false) + } + #[test] fn owned_setters_chain_and_apply_each_value() { // The setters take owned `self` and return owned `Self`, so a full @@ -544,20 +658,36 @@ mod tests { .fake(true) .all(true) .show_output(false) - .synchronized(false); + .synchronized(false) + .allow_unknown_tags(true) + .allow_out_of_order(true); assert_eq!(migrator.direction, Direction::Down); assert_eq!(migrator.force, ForceMode::AcceptFailures); assert!(migrator.fake); assert!(migrator.all); assert!(!migrator.show_output); assert!(!migrator.synchronized); + assert!(migrator.allow_unknown_tags); + assert!(migrator.allow_out_of_order); + } + + #[test] + fn strictness_defaults_are_false() { + let settings = crate::config::Settings::configure_sqlite() + .memory() + .build() + .unwrap(); + let config = Config::with_settings(settings); + let migrator = Migrator::with_config(&config); + assert!(!migrator.allow_unknown_tags); + assert!(!migrator.allow_out_of_order); } #[test] fn up_picks_first_unapplied_in_definition_order() { let avail = available(&["a", "b", "c"]); let applied = tags(&["a"]); - let next = Migrator::next_available(Direction::Up, &avail, &applied, &no_skips()) + let next = next_strict(Direction::Up, &avail, &applied, &no_skips()) .unwrap() .expect("expected an un-applied migration"); assert_eq!(next.tag(), "b"); @@ -567,7 +697,7 @@ mod tests { fn up_returns_none_when_all_applied() { let avail = available(&["a", "b"]); let applied = tags(&["a", "b"]); - let next = Migrator::next_available(Direction::Up, &avail, &applied, &no_skips()).unwrap(); + let next = next_strict(Direction::Up, &avail, &applied, &no_skips()).unwrap(); assert!(next.is_none()); } @@ -576,7 +706,7 @@ mod tests { let avail = available(&["a", "b", "c"]); let applied = tags(&["a"]); // `b` failed under skip-failures earlier in the run: `c` is next. - let next = Migrator::next_available(Direction::Up, &avail, &applied, &skips(&["b"])) + let next = next_strict(Direction::Up, &avail, &applied, &skips(&["b"])) .unwrap() .expect("expected an un-applied migration"); assert_eq!(next.tag(), "c"); @@ -586,22 +716,22 @@ mod tests { fn up_with_all_remaining_skipped_returns_none() { let avail = available(&["a", "b"]); let applied = tags(&["a"]); - let next = - Migrator::next_available(Direction::Up, &avail, &applied, &skips(&["b"])).unwrap(); + let next = next_strict(Direction::Up, &avail, &applied, &skips(&["b"])).unwrap(); assert!(next.is_none()); } #[test] - fn down_picks_last_applied_in_definition_order_even_when_applied_shuffled() { + fn down_picks_last_applied_in_recorded_order() { let avail = available(&["a", "b", "c", "d"]); - // `applied` is intentionally shuffled and does not include the final - // migration `d`. The Down target must be `c` (the last applied tag in - // definition order), not `applied.last()` which would be `a`. - let applied = tags(&["b", "c", "a"]); - let next = Migrator::next_available(Direction::Down, &avail, &applied, &no_skips()) + // Recorded application order is authoritative: `b` is the most recently + // applied migration (last in the recorded list) even though it comes + // before `c` in definition order. Down must target `applied.last()` = `b`, + // not the definition-order-last applied tag `c`. + let applied = tags(&["a", "c", "b"]); + let next = next_strict(Direction::Down, &avail, &applied, &no_skips()) .unwrap() .expect("expected a down migration"); - assert_eq!(next.tag(), "c"); + assert_eq!(next.tag(), "b"); } #[test] @@ -609,7 +739,7 @@ mod tests { let avail = available(&["a", "b", "c"]); let applied = tags(&["a", "b", "c"]); // `c`'s down failed under skip-failures: `b` is next. - let next = Migrator::next_available(Direction::Down, &avail, &applied, &skips(&["c"])) + let next = next_strict(Direction::Down, &avail, &applied, &skips(&["c"])) .unwrap() .expect("expected a down migration"); assert_eq!(next.tag(), "b"); @@ -619,8 +749,7 @@ mod tests { fn down_with_all_applied_skipped_returns_none() { let avail = available(&["a", "b"]); let applied = tags(&["a", "b"]); - let next = Migrator::next_available(Direction::Down, &avail, &applied, &skips(&["a", "b"])) - .unwrap(); + let next = next_strict(Direction::Down, &avail, &applied, &skips(&["a", "b"])).unwrap(); assert!(next.is_none()); } @@ -628,8 +757,7 @@ mod tests { fn down_with_empty_applied_returns_none() { let avail = available(&["a", "b"]); let applied: Vec = Vec::new(); - let next = - Migrator::next_available(Direction::Down, &avail, &applied, &no_skips()).unwrap(); + let next = next_strict(Direction::Down, &avail, &applied, &no_skips()).unwrap(); assert!(next.is_none()); } @@ -637,10 +765,178 @@ mod tests { fn down_with_applied_tags_absent_from_available_errors() { let avail = available(&["a", "b"]); let applied = tags(&["x", "y"]); - match Migrator::next_available(Direction::Down, &avail, &applied, &no_skips()) { + match next_strict(Direction::Down, &avail, &applied, &no_skips()) { Err(Error::MigrationNotFound(_)) => {} Err(other) => panic!("expected MigrationNotFound, got: {:?}", other), Ok(_) => panic!("expected MigrationNotFound error, got Ok"), } } + + #[test] + fn up_unknown_applied_tag_errors_by_default() { + let avail = available(&["a", "b"]); + // `x` is applied but not among the available migrations. + let applied = tags(&["a", "x"]); + match next_strict(Direction::Up, &avail, &applied, &no_skips()).map(|o| o.map(|m| m.tag())) + { + Err(Error::MigrationNotFound(_)) => {} + other => panic!("expected MigrationNotFound, got: {:?}", other), + } + } + + #[test] + fn up_unknown_applied_tag_allowed_when_opted_out() { + let avail = available(&["a", "b"]); + let applied = tags(&["a", "x"]); + // With `allow_unknown_tags`, the unknown `x` is ignored and the next + // available migration `b` is selected. + let next = + Migrator::next_available(Direction::Up, &avail, &applied, &no_skips(), true, false) + .unwrap() + .expect("expected an un-applied migration"); + assert_eq!(next.tag(), "b"); + } + + #[test] + fn up_out_of_order_applied_tag_errors_by_default() { + let avail = available(&["a", "b", "c"]); + // `c` is applied while the earlier `b` is not: out of order. + let applied = tags(&["a", "c"]); + match next_strict(Direction::Up, &avail, &applied, &no_skips()).map(|o| o.map(|m| m.tag())) + { + Err(Error::MigrationOrdering(msg)) => { + assert!( + msg.contains("c"), + "message should name the out-of-order tag: {msg}" + ); + assert!( + msg.contains("b"), + "message should name the earlier unapplied tag: {msg}" + ); + } + other => panic!("expected MigrationOrdering, got: {:?}", other), + } + } + + #[test] + fn up_out_of_order_allowed_when_opted_out() { + let avail = available(&["a", "b", "c"]); + let applied = tags(&["a", "c"]); + // With `allow_out_of_order`, the intervening `b` is selected next. + let next = + Migrator::next_available(Direction::Up, &avail, &applied, &no_skips(), false, true) + .unwrap() + .expect("expected an un-applied migration"); + assert_eq!(next.tag(), "b"); + } + + #[test] + fn up_skipped_tags_do_not_trigger_ordering_or_unknown_errors() { + // A `skip-failures` run must not be self-defeating: a tag in the skipped + // set counts as neither applied (so no ordering violation) nor blocking. + let avail = available(&["a", "b", "c"]); + let applied = tags(&["a"]); + // `b` failed and was skipped this run; selecting past it to `c` must not + // raise an out-of-order error even though `b` (unapplied) precedes `c`. + let next = next_strict(Direction::Up, &avail, &applied, &skips(&["b"])) + .unwrap() + .expect("expected an un-applied migration"); + assert_eq!(next.tag(), "c"); + } + + #[test] + fn up_unknown_takes_precedence_over_out_of_order() { + // The applied set contains *both* an unknown tag (`x`, not among the + // available migrations) and an out-of-order condition (`c` applied while + // the earlier `a`/`b` are not). With both checks enabled (the default), + // the unknown-tag check runs first, so `MigrationNotFound` -- not + // `MigrationOrdering` -- is the error that surfaces. + let avail = available(&["a", "b", "c"]); + let applied = tags(&["x", "c"]); + match next_strict(Direction::Up, &avail, &applied, &no_skips()) { + Err(Error::MigrationNotFound(_)) => {} + other => panic!( + "unknown-tag check must take precedence over ordering, got: {:?}", + other.map(|o| o.map(|m| m.tag())) + ), + } + } + + #[test] + fn up_allow_unknown_still_enforces_ordering_independently() { + // Opting out of the unknown-tag check must not also disable the ordering + // check: with the same set, once `x` is tolerated the still-active + // ordering check catches `c` applied ahead of the earlier migrations. + let avail = available(&["a", "b", "c"]); + let applied = tags(&["x", "c"]); + match Migrator::next_available(Direction::Up, &avail, &applied, &no_skips(), true, false) { + Err(Error::MigrationOrdering(_)) => {} + other => panic!( + "ordering check must remain active when only unknown tags are allowed, got: {:?}", + other.map(|o| o.map(|m| m.tag())) + ), + } + } + + #[test] + fn up_allow_out_of_order_still_enforces_unknown_independently() { + // The mirror case: opting out of the ordering check must not disable the + // unknown-tag check. `x` is unknown and must still raise + // `MigrationNotFound` even with `allow_out_of_order`. + let avail = available(&["a", "b"]); + let applied = tags(&["a", "x"]); + match Migrator::next_available(Direction::Up, &avail, &applied, &no_skips(), false, true) { + Err(Error::MigrationNotFound(_)) => {} + other => panic!( + "unknown-tag check must remain active when only ordering is allowed, got: {:?}", + other.map(|o| o.map(|m| m.tag())) + ), + } + } + + #[test] + fn up_skipped_unknown_tag_does_not_raise_not_found() { + // A tag in the `skipped` set is excluded from the unknown-tag check too + // (not only the ordering check): a skipped tag that happens not to be + // among the available migrations must not raise `MigrationNotFound`. + let avail = available(&["a", "b"]); + let applied = tags(&["a", "ghost"]); + let next = next_strict(Direction::Up, &avail, &applied, &skips(&["ghost"])) + .unwrap() + .expect("expected an un-applied migration"); + assert_eq!(next.tag(), "b"); + } + + #[test] + fn down_does_not_run_the_up_consistency_checks() { + // The strictness checks are `Up`-only. A `Down` run against an + // out-of-order applied set must not raise `MigrationOrdering`; it simply + // targets the most-recently-applied migration by recorded order. + let avail = available(&["a", "b", "c"]); + // `c` was applied while `b` was not: an out-of-order set for an Up run. + let applied = tags(&["a", "c"]); + let next = next_strict(Direction::Down, &avail, &applied, &no_skips()) + .unwrap() + .expect("expected a down migration"); + assert_eq!(next.tag(), "c"); + } + + #[test] + fn down_last_applied_skipped_earlier_unknown_errors() { + // Down walks recorded order backwards skipping the run-skipped tags. When + // the most-recently-applied tag is skipped and the next-back tag is not + // among the available migrations, that unknown tag is the selection + // target and Down errors with `MigrationNotFound` (Down does not consult + // the Up-only unknown-tag opt-out). + let avail = available(&["a", "b"]); + // Recorded order: `x` (unknown) then `b`; `b` was skipped this run. + let applied = tags(&["x", "b"]); + match next_strict(Direction::Down, &avail, &applied, &skips(&["b"])) { + Err(Error::MigrationNotFound(_)) => {} + other => panic!( + "expected MigrationNotFound for the unknown down target, got: {:?}", + other.map(|o| o.map(|m| m.tag())) + ), + } + } } diff --git a/migrant_lib/src/ops.rs b/migrant_lib/src/ops.rs index 6541b6b..7d0b70f 100644 --- a/migrant_lib/src/ops.rs +++ b/migrant_lib/src/ops.rs @@ -114,8 +114,13 @@ pub(crate) fn search_for_migrations(mig_root: &Path) -> Result Result<()> { Ok(()) } -/// Create a new migration with the given tag +/// The migration directory and files created by [`create_migration`]. +#[derive(Debug, Clone)] +pub struct NewMigration { + dir: PathBuf, + up: PathBuf, + down: PathBuf, +} + +impl NewMigration { + /// The created migration directory (`/_`) + pub fn dir(&self) -> &Path { + &self.dir + } + + /// The created `up.sql` file path + pub fn up_path(&self) -> &Path { + &self.up + } + + /// The created `down.sql` file path + pub fn down_path(&self) -> &Path { + &self.down + } +} + +/// Create a new migration with the given tag, returning the paths that were +/// created. /// /// Generated tags will follow the format `{DT-STAMP}_{TAG}` /// /// Intended only for use when running in "migrant CLI compatibility mode" /// where migrations (`FileMigration`s) are all files with names following /// the expected timestamp formatted name. -pub fn new(config: &Config, tag: &str) -> Result<()> { +pub fn create_migration(config: &Config, tag: &str) -> Result { if !tags::is_valid_simple_tag(tag) { bail!( Migration, @@ -225,10 +256,15 @@ pub fn new(config: &Config, tag: &str) -> Result<()> { let mig_dir = config.migration_location()?.join(folder); fs::create_dir_all(&mig_dir)?; - for name in ["up.sql", "down.sql"] { - fs::File::create(mig_dir.join(name))?; - } - Ok(()) + let up = mig_dir.join("up.sql"); + let down = mig_dir.join("down.sql"); + fs::File::create(&up)?; + fs::File::create(&down)?; + Ok(NewMigration { + dir: mig_dir, + up, + down, + }) } /// Open a repl connection to the given `Config` settings @@ -404,10 +440,10 @@ fn select_from_matches<'a>(tag: &str, matches: &'a [FileMigration]) -> Result<&' /// In the case of ambiguous names, the user will be prompted for a selection. /// /// Intended only for use with `FileMigration`s that were created by -/// `migrant_lib::new` or `migrant` CLI (migration files with names that -/// follow the expected timestamp format), NOT those managed directly in source -/// with `Config::use_migrations`. -pub fn edit(config: &Config, tag: &str, up_down: &Direction) -> Result<()> { +/// `migrant_lib::create_migration` or `migrant` CLI (migration files with names +/// that follow the expected timestamp format), NOT those managed directly in +/// source with `Config::use_migrations`. +pub fn edit(config: &Config, tag: &str, up_down: Direction) -> Result<()> { let mig_dir = config.migration_location()?; let available = search_for_migrations(&mig_dir)?; @@ -483,6 +519,50 @@ mod tests { assert!(!unapplied.applied()); } + #[test] + fn create_migration_returns_created_paths() { + // Use an absolute migration_location so no settings file is needed. + let dir = tempfile::tempdir().unwrap(); + let settings = crate::config::Settings::configure_sqlite() + .database_path("/abs/some.db") + .migration_location(dir.path()) + .build() + .unwrap(); + let config = Config::with_settings(settings); + + let created = create_migration(&config, "add-widgets").unwrap(); + + // The returned paths are the ones that now exist on disk. + assert!(created.dir().is_dir(), "the migration dir must be created"); + assert!(created.up_path().is_file(), "up.sql must be created"); + assert!(created.down_path().is_file(), "down.sql must be created"); + assert_eq!(created.up_path().file_name().unwrap(), "up.sql"); + assert_eq!(created.down_path().file_name().unwrap(), "down.sql"); + assert_eq!(created.up_path().parent().unwrap(), created.dir()); + assert_eq!(created.down_path().parent().unwrap(), created.dir()); + // The generated folder is `<14-digit-stamp>_`. + let folder = created.dir().file_name().unwrap().to_str().unwrap(); + assert!( + folder.ends_with("_add-widgets"), + "folder must be stamped and tagged: {folder}" + ); + let (stamp, _) = folder.split_once('_').unwrap(); + assert_eq!(14, stamp.len(), "stamp must be 14 digits: {stamp}"); + assert!(stamp.chars().all(|c| c.is_ascii_digit())); + } + + #[test] + fn create_migration_rejects_invalid_tag() { + let dir = tempfile::tempdir().unwrap(); + let settings = crate::config::Settings::configure_sqlite() + .database_path("/abs/some.db") + .migration_location(dir.path()) + .build() + .unwrap(); + let config = Config::with_settings(settings); + assert!(create_migration(&config, "Bad Tag!").is_err()); + } + #[test] fn migration_search_finds_and_sorts() { let dir = tempfile::tempdir().unwrap(); @@ -502,6 +582,51 @@ mod tests { assert_eq!("20200101000000_second", migs[1].tag()); } + #[test] + fn migration_search_orders_same_second_migrations_deterministically() { + // Several migrations sharing the same timestamp second. Their discovery + // order must be a total, deterministic order (stamp then tag), not the + // random HashMap iteration order -- otherwise repeated searches within a + // run disagree and the migrator's strict ordering check spuriously fails. + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + // Same second `20200101000000`, tags intentionally not in sorted order + // on disk-creation order. + let tags_in = [ + "20200101000000_delta", + "20200101000000_alpha", + "20200101000000_charlie", + "20200101000000_bravo", + ]; + for folder in tags_in { + let d = root.join(folder); + fs::create_dir_all(&d).unwrap(); + fs::write(d.join("up.sql"), "select 1;").unwrap(); + fs::write(d.join("down.sql"), "select 1;").unwrap(); + } + + let expected = vec![ + "20200101000000_alpha".to_string(), + "20200101000000_bravo".to_string(), + "20200101000000_charlie".to_string(), + "20200101000000_delta".to_string(), + ]; + + // Repeated independent searches (each builds a fresh HashMap with a new + // random seed) must all agree on the tag-sorted total order. + for _ in 0..16 { + let order = search_for_migrations(root) + .unwrap() + .into_iter() + .map(|m| m.tag()) + .collect::>(); + assert_eq!( + expected, order, + "same-second migrations must resolve to a deterministic tag-sorted order" + ); + } + } + #[test] fn migration_search_requires_up_and_down() { let dir = tempfile::tempdir().unwrap(); @@ -623,7 +748,6 @@ mod tests { fn sqlite_shell_opens_database_path() { let settings = crate::config::Settings::configure_sqlite() .database_path("/tmp/some.db") - .unwrap() .build() .unwrap(); let config = Config::with_settings(settings); diff --git a/migrant_lib/tests/server_dbs.rs b/migrant_lib/tests/server_dbs.rs index c2787df..dfdd4d8 100644 --- a/migrant_lib/tests/server_dbs.rs +++ b/migrant_lib/tests/server_dbs.rs @@ -118,6 +118,9 @@ fn postgres_end_to_end() { drop_pg_migration_table(&conn_str); apply_and_unapply(&settings); drop_pg_migration_table(&conn_str); + // schema (checksum/applied_at) + recorded-order phase, same database + assert_pg_schema_records_checksum_and_order(&conn_str, &settings); + drop_pg_migration_table(&conn_str); // atomic-rollback phase runs against the same database (see the helper doc) assert_failed_migration_rolls_back(&conn_str, &settings); drop_pg_migration_table(&conn_str); @@ -175,6 +178,101 @@ fn assert_unsynchronized_run_skips_lock(conn_str: &str, settings: &Settings) { .unwrap(); } +/// After applying two embedded migrations, the `__migrant_migrations` table on +/// postgres carries the new bookkeeping columns: `applied_at` is populated by +/// the column default, `checksum` holds the sha256 of each migration's up SQL, +/// and `order by id` reflects the recorded application order. Shares the +/// postgres database with `postgres_end_to_end`, so it runs as one of its phases. +#[cfg(feature = "postgres")] +fn assert_pg_schema_records_checksum_and_order(conn_str: &str, settings: &Settings) { + use sha2::{Digest, Sha256}; + + fn sha256_hex(bytes: &[u8]) -> String { + let mut h = Sha256::new(); + h.update(bytes); + h.finalize().iter().map(|b| format!("{:02x}", b)).collect() + } + + let up_a = "create table users (name varchar(64));"; + let up_b = "insert into users (name) values ('james');"; + + let mut config = Config::with_settings(settings.clone()); + config + .use_migrations(&[ + EmbeddedMigration::with_tag("create-users") + .up(up_a) + .down("drop table users;") + .boxed(), + EmbeddedMigration::with_tag("seed-users") + .up(up_b) + .down("delete from users where name = 'james';") + .boxed(), + ]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + Migrator::with_config(&config) + .all(true) + .show_output(false) + .apply() + .unwrap(); + + let mut client = postgres::Client::connect(conn_str, postgres::NoTls).unwrap(); + + // The new columns exist. + for col in ["id", "tag", "checksum", "applied_at"] { + let exists: bool = client + .query_one( + "select exists(select 1 from information_schema.columns \ + where table_name = '__migrant_migrations' and column_name = $1)", + &[&col], + ) + .unwrap() + .get(0); + assert!( + exists, + "column `{}` must exist on __migrant_migrations", + col + ); + } + + // Recorded order (order by id) matches application order, and checksums are + // the sha256 of each up SQL. + let rows = client + .query( + "select tag, checksum, applied_at is not null \ + from __migrant_migrations order by id", + &[], + ) + .unwrap(); + let recorded: Vec<(String, Option, bool)> = rows + .iter() + .map(|r| (r.get(0), r.get(1), r.get(2))) + .collect(); + assert_eq!( + vec![ + ( + "create-users".to_string(), + Some(sha256_hex(up_a.as_bytes())), + true + ), + ( + "seed-users".to_string(), + Some(sha256_hex(up_b.as_bytes())), + true + ), + ], + recorded, + ); + + Migrator::with_config(&config) + .direction(Direction::Down) + .all(true) + .show_output(false) + .apply() + .unwrap(); +} + /// A migration whose SQL fails partway is rolled back atomically on postgres: /// the partial DDL is undone and the bookkeeping row is never written. /// @@ -297,4 +395,95 @@ fn mysql_end_to_end() { drop_mysql_migration_table(&conn_str); apply_and_unapply(&settings); drop_mysql_migration_table(&conn_str); + // schema (checksum/applied_at) + recorded-order phase, same database + assert_mysql_schema_records_checksum_and_order(&conn_str, &settings); + drop_mysql_migration_table(&conn_str); +} + +/// After applying two embedded migrations, the `__migrant_migrations` table on +/// mysql carries the new bookkeeping columns: `applied_at` is populated by the +/// column default, `checksum` holds the sha256 of each migration's up SQL, and +/// `order by id` reflects the recorded application order. Shares the mysql +/// database with `mysql_end_to_end`, so it runs as one of its phases. +#[cfg(feature = "mysql")] +fn assert_mysql_schema_records_checksum_and_order(conn_str: &str, settings: &Settings) { + use mysql::prelude::Queryable; + use sha2::{Digest, Sha256}; + + fn sha256_hex(bytes: &[u8]) -> String { + let mut h = Sha256::new(); + h.update(bytes); + h.finalize().iter().map(|b| format!("{:02x}", b)).collect() + } + + let up_a = "create table users (name varchar(64));"; + let up_b = "insert into users (name) values ('james');"; + + let mut config = Config::with_settings(settings.clone()); + config + .use_migrations(&[ + EmbeddedMigration::with_tag("create-users") + .up(up_a) + .down("drop table users;") + .boxed(), + EmbeddedMigration::with_tag("seed-users") + .up(up_b) + .down("delete from users where name = 'james';") + .boxed(), + ]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + Migrator::with_config(&config) + .all(true) + .show_output(false) + .apply() + .unwrap(); + + let opts = mysql::Opts::from_url(conn_str).unwrap(); + let mut conn = mysql::Conn::new(opts).unwrap(); + + // The new columns exist. + for col in ["id", "tag", "checksum", "applied_at"] { + let exists: Option = conn + .exec_first( + "select count(*) from information_schema.columns \ + where table_name = '__migrant_migrations' \ + and table_schema = database() and column_name = ?", + (col,), + ) + .unwrap(); + assert_eq!(Some(1), exists, "column `{}` must exist", col); + } + + // Recorded order (order by id) matches application order, and checksums are + // the sha256 of each up SQL. + let recorded: Vec<(String, Option, i64)> = conn + .query( + "select tag, checksum, (applied_at is not null) \ + from __migrant_migrations order by id", + ) + .unwrap(); + assert_eq!( + vec![ + ( + "create-users".to_string(), + Some(sha256_hex(up_a.as_bytes())), + 1 + ), + ( + "seed-users".to_string(), + Some(sha256_hex(up_b.as_bytes())), + 1 + ), + ], + recorded, + ); + + Migrator::with_config(&config) + .direction(Direction::Down) + .all(true) + .show_output(false) + .apply() + .unwrap(); } diff --git a/migrant_lib/tests/sqlite.rs b/migrant_lib/tests/sqlite.rs index eb09d33..24576a4 100644 --- a/migrant_lib/tests/sqlite.rs +++ b/migrant_lib/tests/sqlite.rs @@ -4,7 +4,7 @@ use migrant_lib::{ Config, ConnConfig, Direction, EmbeddedMigration, FileMigration, FnMigration, ForceMode, - Migrator, Settings, + Migratable, Migrator, Settings, }; fn seed_users(conn: ConnConfig) -> Result<(), Box> { @@ -65,6 +65,36 @@ fn table_exists(config: &Config, name: &str) -> bool { .unwrap() } +/// Read the `(tag, checksum)` bookkeeping rows in recorded (`order by id`) order. +fn recorded_rows(config: &Config) -> Vec<(String, Option)> { + let handle = config.sqlite_connection().unwrap(); + let conn = handle.lock().unwrap(); + let mut stmt = conn + .prepare("select tag, checksum from __migrant_migrations order by id") + .unwrap(); + let rows = stmt + .query_map([], |r| { + Ok((r.get::<_, String>(0)?, r.get::<_, Option>(1)?)) + }) + .unwrap(); + rows.collect::, _>>().unwrap() +} + +/// Insert a raw bookkeeping tag directly (no checksum), simulating a tag the +/// database records that the running code does not manage. +fn raw_insert_tag(config: &Config, tag: &str) { + let handle = config.sqlite_connection().unwrap(); + let conn = handle.lock().unwrap(); + conn.execute("insert into __migrant_migrations (tag) values (?1)", [tag]) + .unwrap(); +} + +fn is_hex64(s: &str) -> bool { + s.len() == 64 + && s.chars() + .all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase()) +} + /// Build an in-memory config with a single migration whose `up` creates a table /// and then runs an invalid statement, so application fails partway through. fn failing_migration_config(no_transaction: bool) -> Config { @@ -147,12 +177,27 @@ fn force_skip_failures_leaves_failed_migration_unrecorded() { "skip-failures must not record the failed migration" ); - // The skipped migration is retried on the next run (and fails again - // without force). - let res = Migrator::with_config(&config).show_output(false).apply(); + // The skipped `bad` migration was left unrecorded, so `later` is now applied + // ahead of the still-pending `bad`. Under the strict default a following run + // surfaces that gap as an ordering error rather than silently proceeding. + let err = Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap_err(); + assert!( + err.is_migration_ordering(), + "the unrecorded skipped migration leaves an out-of-order gap on the next run, got: {err:?}" + ); + + // Opting out of the ordering check retries the skipped `bad`, which fails + // again without force. + let res = Migrator::with_config(&config) + .allow_out_of_order(true) + .show_output(false) + .apply(); assert!( res.is_err(), - "the skipped migration must be selected and fail on the next run" + "the skipped migration must be selected and fail again on retry" ); } @@ -387,7 +432,6 @@ fn file_database_end_to_end() { let db_path = dir.path().join("test.db"); let settings = Settings::configure_sqlite() .database_path(&db_path) - .unwrap() .build() .unwrap(); let config = migrations_config(&settings); @@ -405,3 +449,400 @@ fn file_database_end_to_end() { assert_eq!(2, applied_tags(&config).len()); assert_eq!(1, user_count(&config)); } + +#[test] +fn checksum_recorded_for_embedded_null_for_fn_and_ordered_by_id() { + // `create-users` is an embedded (SQL) migration and gets a checksum; + // `seed-users` is a function migration and stores NULL by design. The rows + // come back in recorded application order via `order by id`. + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let config = migrations_config(&settings); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + Migrator::with_config(&config) + .all(true) + .show_output(false) + .apply() + .unwrap(); + + let rows = recorded_rows(&config); + assert_eq!(2, rows.len()); + assert_eq!("create-users", rows[0].0); + assert_eq!("seed-users", rows[1].0); + let embedded_sum = rows[0] + .1 + .as_deref() + .expect("embedded migration has a checksum"); + assert!( + is_hex64(embedded_sum), + "checksum must be 64 hex chars: {embedded_sum}" + ); + assert_eq!(None, rows[1].1, "function migrations store NULL checksum"); +} + +#[test] +fn file_migration_checksum_is_recorded() { + let dir = tempfile::tempdir().unwrap(); + let up = dir.path().join("up.sql"); + let down = dir.path().join("down.sql"); + std::fs::write(&up, "create table filed (x integer);").unwrap(); + std::fs::write(&down, "drop table filed;").unwrap(); + + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let mut config = Config::with_settings(settings); + config + .use_migrations(&[FileMigration::with_tag("filed").up(&up).down(&down).boxed()]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap(); + + let rows = recorded_rows(&config); + assert_eq!(1, rows.len()); + let sum = rows[0].1.as_deref().expect("file migration has a checksum"); + assert!(is_hex64(sum), "checksum must be 64 hex chars: {sum}"); +} + +#[test] +fn down_reverts_last_applied_by_recorded_order() { + // Recorded order is authoritative: a single Down after a full Up reverts the + // most recently applied migration (`seed-users`), leaving the earlier one. + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let config = migrations_config(&settings); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + Migrator::with_config(&config) + .all(true) + .show_output(false) + .apply() + .unwrap(); + + Migrator::with_config(&config) + .direction(Direction::Down) + .show_output(false) + .apply() + .unwrap(); + + let config = config.reload().unwrap(); + assert_eq!(vec!["create-users".to_string()], applied_tags(&config)); +} + +#[test] +fn unknown_applied_tag_errors_and_allow_unknown_tags_opts_out() { + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let config = migrations_config(&settings); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + // Apply the first migration, then record a tag the code does not manage. + Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap(); + raw_insert_tag(&config, "ghost"); + + // A default Up run aborts on the unknown applied tag. + let err = Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap_err(); + assert!( + err.is_migration_not_found(), + "unknown applied tag must raise MigrationNotFound, got: {err:?}" + ); + + // Opting out lets the run ignore `ghost` and apply the remaining migration. + Migrator::with_config(&config) + .allow_unknown_tags(true) + .all(true) + .show_output(false) + .apply() + .unwrap(); + let config = config.reload().unwrap(); + let applied = applied_tags(&config); + assert!(applied.contains(&"create-users".to_string())); + assert!(applied.contains(&"seed-users".to_string())); +} + +#[test] +fn same_second_file_migrations_apply_deterministically_under_strict_checks() { + // Regression: file migrations sharing a timestamp second must have a total, + // deterministic discovery order. The migrator re-runs discovery on every + // step and treats the order as authoritative; without a stamp-tie tiebreak + // the random HashMap order could differ between steps and the default strict + // ordering check would spuriously abort a legitimate `apply` of same-second + // migrations. A full apply must succeed and leave every migration applied. + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + // Four migrations, all the same second, folder-creation order deliberately + // unsorted relative to the resolved (tag-sorted) order. + for (folder, tbl) in [ + ("20200101000000_delta", "t_delta"), + ("20200101000000_alpha", "t_alpha"), + ("20200101000000_charlie", "t_charlie"), + ("20200101000000_bravo", "t_bravo"), + ] { + let d = root.join(folder); + std::fs::create_dir_all(&d).unwrap(); + std::fs::write(d.join("up.sql"), format!("create table {tbl} (x integer);")).unwrap(); + std::fs::write(d.join("down.sql"), format!("drop table {tbl};")).unwrap(); + } + + // In-memory database, migrations discovered from the on-disk location (so + // `available_migrations` goes through `search_for_migrations`, not explicit + // `use_migrations`). + let settings = Settings::configure_sqlite() + .memory() + .migration_location(root) + .build() + .unwrap(); + let config = Config::with_settings(settings); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + // Must not raise `MigrationOrdering` under the default strict checks. + Migrator::with_config(&config) + .all(true) + .show_output(false) + .apply() + .expect("same-second migrations must apply cleanly under strict ordering"); + + let config = config.reload().unwrap(); + assert_eq!( + vec![ + "20200101000000_alpha".to_string(), + "20200101000000_bravo".to_string(), + "20200101000000_charlie".to_string(), + "20200101000000_delta".to_string(), + ], + applied_tags(&config), + "applied in the deterministic tag-sorted order" + ); + for tbl in ["t_alpha", "t_bravo", "t_charlie", "t_delta"] { + assert!(table_exists(&config, tbl), "{tbl} must have been created"); + } +} + +#[test] +fn out_of_order_applied_tag_errors_and_allow_out_of_order_opts_out() { + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let mut config = Config::with_settings(settings); + config + .use_migrations(&[ + EmbeddedMigration::with_tag("a") + .up("create table a (x integer);") + .down("drop table a;") + .boxed(), + EmbeddedMigration::with_tag("b") + .up("create table b (x integer);") + .down("drop table b;") + .boxed(), + EmbeddedMigration::with_tag("c") + .up("create table c (x integer);") + .down("drop table c;") + .boxed(), + ]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + // Apply `a`, then record `c` as applied while `b` is still pending: `c` is + // out of order relative to definition order. + Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap(); + raw_insert_tag(&config, "c"); + + let err = Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap_err(); + assert!( + err.is_migration_ordering(), + "out-of-order application must raise MigrationOrdering, got: {err:?}" + ); + + // Opting out applies the intervening `b`. + Migrator::with_config(&config) + .allow_out_of_order(true) + .show_output(false) + .apply() + .unwrap(); + assert!( + table_exists(&config, "b"), + "the intervening migration must run" + ); +} + +/// A `FileMigration` whose `up` file does not exist must fail to apply and record +/// *nothing* -- it must not silently record a NULL checksum (which `checksum()` +/// returns for an unreadable file) and report success. This guards the divergence +/// between the `checksum()` read (best-effort `None`) and the apply-path read +/// (which surfaces the real error). +#[test] +fn file_migration_missing_up_file_fails_and_records_nothing() { + let dir = tempfile::tempdir().unwrap(); + // `down` exists but `up` points at a file that was never created. + let down = dir.path().join("down.sql"); + std::fs::write(&down, "select 1;").unwrap(); + let missing_up = dir.path().join("does_not_exist_up.sql"); + + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let mut config = Config::with_settings(settings); + config + .use_migrations(&[FileMigration::with_tag("filed") + .up(&missing_up) + .down(&down) + .boxed()]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + let res = Migrator::with_config(&config).show_output(false).apply(); + assert!( + res.is_err(), + "applying a migration whose up file is missing must error, not silently succeed" + ); + + let config = config.reload().unwrap(); + assert!( + applied_tags(&config).is_empty(), + "an unreadable up file must not record the tag" + ); + assert!( + recorded_rows(&config).is_empty(), + "no bookkeeping row (and so no NULL-checksum row) may be written for a failed read" + ); +} + +/// Recorded application order (`order by id`) is authoritative even when a +/// migration is applied out of definition order: the intervening migration gets +/// the *latest* id, so `order by id` lists it last, and a subsequent default +/// `Down` reverts it first (the most recently applied by recorded order). +#[test] +fn recorded_order_reflects_out_of_order_run_and_down_reverts_last_applied() { + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let mut config = Config::with_settings(settings); + config + .use_migrations(&[ + EmbeddedMigration::with_tag("a") + .up("create table a (x integer);") + .down("drop table a;") + .boxed(), + EmbeddedMigration::with_tag("b") + .up("create table b (x integer);") + .down("drop table b;") + .boxed(), + EmbeddedMigration::with_tag("c") + .up("create table c (x integer);") + .down("drop table c;") + .boxed(), + ]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + // Apply `a`, then record `c` (as if it ran on another branch), then apply the + // intervening `b` with the out-of-order opt-out. `b` is inserted last. + Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap(); + raw_insert_tag(&config, "c"); + let config = config.reload().unwrap(); + Migrator::with_config(&config) + .allow_out_of_order(true) + .show_output(false) + .apply() + .unwrap(); + + let config = config.reload().unwrap(); + // `order by id` reflects the true application order: a, c, then b (applied + // last), *not* definition order a, b, c. + let order: Vec = recorded_rows(&config) + .into_iter() + .map(|(tag, _)| tag) + .collect(); + assert_eq!( + vec!["a".to_string(), "c".to_string(), "b".to_string()], + order + ); + + // A default Down reverts the most recently applied by recorded order: `b`. + Migrator::with_config(&config) + .direction(Direction::Down) + .show_output(false) + .apply() + .unwrap(); + let config = config.reload().unwrap(); + let remaining: Vec = recorded_rows(&config) + .into_iter() + .map(|(tag, _)| tag) + .collect(); + assert_eq!(vec!["a".to_string(), "c".to_string()], remaining); + assert!(!table_exists(&config, "b"), "down must have reverted `b`"); +} + +/// A user-defined `Migratable` (only `'static + Clone`, using the trait's default +/// `checksum`) must be usable end to end: the sealed `MigratableClone` blanket +/// impl clones it into the boxed set, it applies, and -- inheriting the default +/// `checksum` of `None` -- records a NULL checksum. +#[test] +fn custom_migratable_applies_and_records_null_checksum() { + #[derive(Clone)] + struct Custom { + tag: String, + } + impl Migratable for Custom { + fn apply_up(&self, config: &Config) -> Result<(), Box> { + let handle = config.sqlite_connection()?; + let conn = handle.lock().unwrap(); + conn.execute_batch("create table custom_made (x integer);")?; + Ok(()) + } + fn apply_down(&self, config: &Config) -> Result<(), Box> { + let handle = config.sqlite_connection()?; + let conn = handle.lock().unwrap(); + conn.execute_batch("drop table custom_made;")?; + Ok(()) + } + fn tag(&self) -> String { + self.tag.clone() + } + } + + let settings = Settings::configure_sqlite().memory().build().unwrap(); + let mut config = Config::with_settings(settings); + config + .use_migrations(&[Box::new(Custom { + tag: "custom".to_string(), + }) as Box]) + .unwrap(); + config.setup().unwrap(); + let config = config.reload().unwrap(); + + Migrator::with_config(&config) + .show_output(false) + .apply() + .unwrap(); + + let config = config.reload().unwrap(); + assert!( + table_exists(&config, "custom_made"), + "the custom migration's up must have run" + ); + let rows = recorded_rows(&config); + assert_eq!(1, rows.len()); + assert_eq!("custom", rows[0].0); + assert_eq!( + None, rows[0].1, + "a custom migration inherits the default `checksum` of None (NULL)" + ); +} From 105d09bdce6dd8673e370349f043e4d37b161bbd Mon Sep 17 00:00:00 2001 From: James Kominick Date: Tue, 4 Aug 2026 08:47:19 -0400 Subject: [PATCH 2/3] apply all pending migrations by default, add `--step` and ordering opt-out flags - apply all pending migrations on `migrant apply`; add `--step N` for a bounded run and remove `--all` from `apply` (`redo` keeps it). `--down` stays single-step by default - add `--allow-unknown-tags` and `--allow-out-of-order` to `apply` and `redo` - call `create_migration` and print the created file paths; use `migrant_lib::cli::list` --- src/cli.rs | 50 ++++- src/main.rs | 66 ++++-- tests/migrant.rs | 568 +++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 644 insertions(+), 40 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index f159f5a..c035403 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -30,6 +30,28 @@ fn no_sync_arg() -> Arg { ) } +/// `--allow-unknown-tags`: tolerate applied tags absent from the available set. +fn allow_unknown_tags_arg() -> Arg { + Arg::new("allow-unknown-tags") + .long("allow-unknown-tags") + .action(ArgAction::SetTrue) + .help( + "Tolerate applied migration tags that are not among the available migrations, \ + instead of aborting", + ) +} + +/// `--allow-out-of-order`: tolerate applied migrations out of definition order. +fn allow_out_of_order_arg() -> Arg { + Arg::new("allow-out-of-order") + .long("allow-out-of-order") + .action(ArgAction::SetTrue) + .help( + "Tolerate an applied migration that is out of order relative to definition \ + order, instead of aborting", + ) +} + pub fn build_cli() -> Command { Command::new("migrant") .version(env!("CARGO_PKG_VERSION")) @@ -122,7 +144,12 @@ pub fn build_cli() -> Command { ) .subcommand( Command::new("apply") - .about("Moves up or down (applies up/down.sql) one migration. Default direction is up unless specified with `-d/--down`.") + .about( + "Applies pending migrations. Default direction is up, applying all pending \ + migrations in one run; use `-d/--down` to revert the latest applied \ + migration instead, or `--step N` to limit either direction to N \ + migrations.", + ) .arg( Arg::new("down") .long("down") @@ -131,11 +158,14 @@ pub fn build_cli() -> Command { .help("Applies `down.sql` migrations"), ) .arg( - Arg::new("all") - .long("all") - .short('a') - .action(ArgAction::SetTrue) - .help("Applies all remaining migrations in the chosen direction (un-applies all with --down)"), + Arg::new("step") + .long("step") + .value_parser(clap::value_parser!(u64).range(1..)) + .value_name("N") + .help( + "Apply at most N migrations in the chosen direction, stopping early \ + if none remain", + ), ) .arg(force_arg()) .arg( @@ -144,7 +174,9 @@ pub fn build_cli() -> Command { .action(ArgAction::SetTrue) .help("Updates the migration table without running the migration"), ) - .arg(no_sync_arg()), + .arg(no_sync_arg()) + .arg(allow_unknown_tags_arg()) + .arg(allow_out_of_order_arg()), ) .subcommand( Command::new("redo") @@ -157,7 +189,9 @@ pub fn build_cli() -> Command { .help("Re-applies (down, then up) all applied migrations instead of only the latest"), ) .arg(force_arg()) - .arg(no_sync_arg()), + .arg(no_sync_arg()) + .arg(allow_unknown_tags_arg()) + .arg(allow_out_of_order_arg()), ) .subcommand( Command::new("new") diff --git a/src/main.rs b/src/main.rs index bb5b119..416c096 100644 --- a/src/main.rs +++ b/src/main.rs @@ -92,7 +92,7 @@ fn run(dir: &Path, matches: &clap::ArgMatches) -> Result<()> { // load applied migrations from the database let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; } Some(("status", matches)) => { // load applied migrations from the database @@ -110,8 +110,10 @@ fn run(dir: &Path, matches: &clap::ArgMatches) -> Result<()> { let config = config.reload()?; let tag = matches.get_one::("tag").expect("required arg"); - migrant_lib::new(&config, tag)?; - migrant_lib::list(&config)?; + let new_migration = migrant_lib::create_migration(&config, tag)?; + println!("Created: {}", new_migration.up_path().display()); + println!("Created: {}", new_migration.down_path().display()); + migrant_lib::cli::list(&config)?; } Some(("apply", matches)) => { // load applied migrations from the database @@ -119,24 +121,52 @@ fn run(dir: &Path, matches: &clap::ArgMatches) -> Result<()> { let force = force_mode(matches)?; let fake = matches.get_flag("fake"); - let all = matches.get_flag("all"); let no_sync = matches.get_flag("no-sync"); + let allow_unknown_tags = matches.get_flag("allow-unknown-tags"); + let allow_out_of_order = matches.get_flag("allow-out-of-order"); let direction = if matches.get_flag("down") { Direction::Down } else { Direction::Up }; - Migrator::with_config(&config) - .direction(direction) - .force(force) - .fake(fake) - .all(all) - .synchronized(!no_sync) - .apply()?; + match matches.get_one::("step") { + Some(&step) => { + // Loop single-step runs, stopping early once a run + // reports nothing left to do. + for _ in 0..step { + let report = Migrator::with_config(&config) + .direction(direction) + .force(force) + .fake(fake) + .all(false) + .synchronized(!no_sync) + .allow_unknown_tags(allow_unknown_tags) + .allow_out_of_order(allow_out_of_order) + .apply()?; + if report.is_empty() { + break; + } + } + } + None => { + // Up defaults to applying every pending migration; down + // defaults to reverting only the latest applied one. + let all = direction == Direction::Up; + Migrator::with_config(&config) + .direction(direction) + .force(force) + .fake(fake) + .all(all) + .synchronized(!no_sync) + .allow_unknown_tags(allow_unknown_tags) + .allow_out_of_order(allow_out_of_order) + .apply()?; + } + } let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; } Some(("redo", matches)) => { // load applied migrations from the database @@ -145,24 +175,30 @@ fn run(dir: &Path, matches: &clap::ArgMatches) -> Result<()> { let force = force_mode(matches)?; let all = matches.get_flag("all"); let no_sync = matches.get_flag("no-sync"); + let allow_unknown_tags = matches.get_flag("allow-unknown-tags"); + let allow_out_of_order = matches.get_flag("allow-out-of-order"); Migrator::with_config(&config) .direction(Direction::Down) .force(force) .all(all) .synchronized(!no_sync) + .allow_unknown_tags(allow_unknown_tags) + .allow_out_of_order(allow_out_of_order) .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; Migrator::with_config(&config) .direction(Direction::Up) .force(force) .all(all) .synchronized(!no_sync) + .allow_unknown_tags(allow_unknown_tags) + .allow_out_of_order(allow_out_of_order) .apply()?; let config = config.reload()?; - migrant_lib::list(&config)?; + migrant_lib::cli::list(&config)?; } Some(("shell", _)) => { migrant_lib::cli::shell(&config)?; @@ -174,7 +210,7 @@ fn run(dir: &Path, matches: &clap::ArgMatches) -> Result<()> { } else { Direction::Up }; - migrant_lib::cli::edit(&config, tag, &up_down)?; + migrant_lib::cli::edit(&config, tag, up_down)?; } Some(("which-config", _)) => { let path = config_path diff --git a/tests/migrant.rs b/tests/migrant.rs index 4b8a426..562fe81 100644 --- a/tests/migrant.rs +++ b/tests/migrant.rs @@ -18,14 +18,16 @@ fn migrant() -> Command { #[test] fn kitchen_sink() { - // make sure we're setup and back to no applied migrations + // make sure we're setup and back to no applied migrations. `--step` with + // a count comfortably larger than the number of migrations reverts + // everything and stops early once nothing remains. migrant().arg("setup").assert().success(); - let _ = migrant().args(["apply", "-ad"]).assert(); + let _ = migrant().args(["apply", "-d", "--step", "100"]).assert(); // A down run with nothing left to un-apply is not an error; it succeeds and // reports the (all-unapplied) status. migrant() - .args(["apply", "-ad"]) + .args(["apply", "-d", "--step", "100"]) .assert() .success() .stdout(contains("[ ] 20170812145327_initial")) @@ -39,8 +41,9 @@ fn kitchen_sink() { .stdout(contains("[ ] 20170812145327_initial")) .stdout(contains("[ ] 20171126194042_second")); + // `apply` with no flags applies all pending migrations in one invocation. migrant() - .args(["apply", "-a"]) + .arg("apply") .assert() .success() .stdout(contains("Applying[Up]:")) @@ -88,7 +91,7 @@ fn kitchen_sink() { .success() .stdout(contains("Migrant.toml")); - let _ = migrant().args(["apply", "-ad"]).assert(); + let _ = migrant().args(["apply", "-d", "--step", "100"]).assert(); } // CLIMIG-6: `status` reports every managed migration in text and json. @@ -113,13 +116,15 @@ fn status_reports_text_and_json() { "drop table status_b;", ); - // apply one migration so we have one applied, one pending. The two `new` - // migrations can share a timestamp (created in the same second), so their - // order is not guaranteed; assert on the mixed state and counts, not on - // which specific tag ends up applied. + // `apply` with no flags now applies every pending migration, so use + // `--step 1` to leave one applied and one pending. `new_migration` + // guarantees the two migrations landed in distinct seconds, but the + // status/list output is still not committed to a specific tag ordering + // here, so assert on the mixed state and counts rather than which + // specific tag ends up applied. migrant() .current_dir(dir.path()) - .arg("apply") + .args(["apply", "--step", "1"]) .assert() .success(); @@ -178,6 +183,14 @@ fn sqlite_project() -> tempfile::TempDir { } /// Create a migration via `migrant new` and overwrite its up/down files. +/// +/// No inter-migration timing dance is needed: `migrant new`'s generated tag has +/// second-resolution timestamps, but the library now discovers/orders +/// migrations by a deterministic total order (timestamp, then tag), so +/// migrations created back-to-back in the same second still have a stable +/// definition order. (See `same_second_migrations_apply_deterministically` for +/// the regression that guards this.) Callers that assert on relative apply order +/// pick tags whose intended order they control. fn new_migration(dir: &std::path::Path, tag: &str, up: &str, down: &str) { migrant() .current_dir(dir) @@ -198,6 +211,319 @@ fn new_migration(dir: &std::path::Path, tag: &str, up: &str, down: &str) { std::fs::write(mig_dir.join("down.sql"), down).expect("write down.sql"); } +/// Create a migration directory directly on disk with a caller-chosen 14-digit +/// timestamp `stamp` and `tag`, bypassing `migrant new`. This is the only way +/// to force two migrations to share the exact same timestamp second (which +/// `migrant new` + [`wait_for_distinct_migration_second`] deliberately avoids). +fn raw_migration(dir: &std::path::Path, stamp: &str, tag: &str, up: &str, down: &str) { + let mig_dir = dir.join("migrations").join(format!("{}_{}", stamp, tag)); + std::fs::create_dir_all(&mig_dir).expect("create migration dir"); + std::fs::write(mig_dir.join("up.sql"), up).expect("write up.sql"); + std::fs::write(mig_dir.join("down.sql"), down).expect("write down.sql"); +} + +/// Delete the on-disk migration directory whose name ends in `_`, so its +/// applied tag becomes "unknown" (recorded as applied but absent from the +/// available set). +fn remove_migration(dir: &std::path::Path, tag: &str) { + let migrations = dir.join("migrations"); + let mig_dir = std::fs::read_dir(&migrations) + .expect("read migrations dir") + .map(|e| e.expect("dir entry").path()) + .find(|p| { + p.file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| n.ends_with(&format!("_{}", tag))) + }) + .unwrap_or_else(|| panic!("migration dir for `{}` not found", tag)); + std::fs::remove_dir_all(&mig_dir).expect("remove migration dir"); +} + +// CLIMIG: `apply --step N --down` reverts at most N applied migrations +// (newest-first) and stops early once nothing remains, mirroring the up +// direction. Only the up direction was exercised before. +#[test] +fn apply_step_down_reverts_limited_and_stops_early() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + new_migration( + dir.path(), + "one", + "create table step_down_a (x integer);", + "drop table step_down_a;", + ); + new_migration( + dir.path(), + "two", + "create table step_down_b (x integer);", + "drop table step_down_b;", + ); + new_migration( + dir.path(), + "three", + "create table step_down_c (x integer);", + "drop table step_down_c;", + ); + + // Apply everything up first. + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .success(); + + // `--step 2 --down` reverts exactly the two most-recently applied + // (`three`, then `two`), leaving only `one` applied. + migrant() + .current_dir(dir.path()) + .args(["apply", "--step", "2", "--down"]) + .assert() + .success(); + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_one").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[ \] \d{14}_two").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[ \] \d{14}_three").expect("valid regex")); + + // `--step 100 --down` with only one applied reverts it and stops early + // instead of erroring once nothing remains. + migrant() + .current_dir(dir.path()) + .args(["apply", "--step", "100", "--down"]) + .assert() + .success(); + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[ \] \d{14}_one").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[ \] \d{14}_two").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[ \] \d{14}_three").expect("valid regex")); +} + +// CLIMIG-4: end-to-end unknown-applied-tag strictness. With an applied tag that +// is no longer among the available migrations, a plain `apply` aborts; the same +// run with `--allow-unknown-tags` succeeds. (Flag *acceptance* is covered +// elsewhere; this drives the actual error path and the opt-out.) +#[test] +fn apply_unknown_tag_rejected_then_allowed() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + new_migration( + dir.path(), + "solo", + "create table unknown_solo (x integer);", + "drop table unknown_solo;", + ); + + // Apply it, then delete its files so the applied tag becomes unknown. + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .success(); + remove_migration(dir.path(), "solo"); + + // A plain up run enforces the unknown-tag check and aborts. + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .failure() + .stderr(contains("MigrationNotFound")) + .stderr(contains("is not among the available migrations")); + + // Opting out lets the run proceed (nothing left to apply). + migrant() + .current_dir(dir.path()) + .args(["apply", "--allow-unknown-tags"]) + .assert() + .success(); +} + +// CLIMIG-4: end-to-end out-of-order strictness. `--force=skip-failures` leaves a +// later migration applied ahead of an earlier, still-pending one; a plain +// `apply` then aborts as out-of-order, while `--allow-out-of-order` proceeds. +#[test] +fn apply_out_of_order_rejected_then_allowed() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + // `a-bad` is defined first (earlier second) but its SQL fails; `b-good` + // is defined second and succeeds. + new_migration( + dir.path(), + "a-bad", + "insert into does_not_exist values (1);", + "select 1;", + ); + new_migration( + dir.path(), + "b-good", + "create table ooo_good (x integer);", + "drop table ooo_good;", + ); + + // skip-failures leaves `b-good` applied ahead of the still-pending `a-bad`. + migrant() + .current_dir(dir.path()) + .args(["apply", "--force=skip-failures"]) + .assert() + .success(); + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[ \] \d{14}_a-bad").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_b-good").expect("valid regex")); + + // A plain up run detects the out-of-order applied set and aborts. + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .failure() + .stderr(contains("MigrationOrdering")) + .stderr(contains("out of order")); + + // Opting out lets the run proceed past the intervening migration. + // (`a-bad` still fails, so `--force` records it and the run completes.) + migrant() + .current_dir(dir.path()) + .args(["apply", "--allow-out-of-order", "--force"]) + .assert() + .success(); + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_a-bad").expect("valid regex")); +} + +// CLIMIG-3: `new` reports the created up/down file paths on stdout. +#[test] +fn new_reports_created_paths() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + + migrant() + .current_dir(dir.path()) + .args(["new", "reported"]) + .assert() + .success() + .stdout( + predicates::str::is_match(r"Created: .*_reported[\\/]+up\.sql").expect("valid regex"), + ) + .stdout( + predicates::str::is_match(r"Created: .*_reported[\\/]+down\.sql").expect("valid regex"), + ); +} + +// REGRESSION (migrant_lib same-second ordering fix): migrations that share the +// exact same timestamp second must have a deterministic definition order. +// +// The library previously discovered migrations via a randomly-seeded `HashMap` +// and a *stable* `sort_by_key(|m| m.stamp)`, so same-second ties kept +// nondeterministic HashMap iteration order. Because each `apply_next` re-runs +// discovery, a single default `migrant apply` of same-second migrations could +// see one order on one step and a different order on the next, and the item-4 +// out-of-order check would then spuriously abort a perfectly legitimate set +// with `MigrationOrdering` (~50% of runs). `migrant_lib::ops` now sorts by a +// total order (timestamp, then tag), so this test drives the default strict +// path (no `--allow-*` flags) over same-second migrations and asserts it +// succeeds deterministically -- applying all of them in a stable tag order -- +// across repeated runs. +#[test] +fn same_second_migrations_apply_deterministically() { + let dir = sqlite_project(); + // Three legitimate migrations sharing the exact same timestamp second. The + // deterministic tiebreak is by tag, so definition order is aaa < bbb < ccc. + raw_migration( + dir.path(), + "20200101000000", + "aaa", + "create table ss_aaa (x integer);", + "drop table ss_aaa;", + ); + raw_migration( + dir.path(), + "20200101000000", + "bbb", + "create table ss_bbb (x integer);", + "drop table ss_bbb;", + ); + raw_migration( + dir.path(), + "20200101000000", + "ccc", + "create table ss_ccc (x integer);", + "drop table ss_ccc;", + ); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + + // Repeat: a default `apply` (strict checks, no allow-flags) must succeed + // every time and apply all three. Under the old bug this aborted with + // `MigrationOrdering` on roughly half the runs; the fix makes it stable. + for _ in 0..10 { + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .success(); + + let out = migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(contains("[✓] 20200101000000_aaa")) + .stdout(contains("[✓] 20200101000000_bbb")) + .stdout(contains("[✓] 20200101000000_ccc")); + + // Definition/list order is the deterministic tiebreak: aaa, bbb, ccc. + let stdout = String::from_utf8(out.get_output().stdout.clone()).expect("utf8 stdout"); + let a = stdout.find("20200101000000_aaa").expect("aaa listed"); + let b = stdout.find("20200101000000_bbb").expect("bbb listed"); + let c = stdout.find("20200101000000_ccc").expect("ccc listed"); + assert!( + a < b && b < c, + "same-second order must be stable (aaa,bbb,ccc)" + ); + + // Reset to all-unapplied for the next iteration. + migrant() + .current_dir(dir.path()) + .args(["apply", "-d", "--step", "100"]) + .assert() + .success(); + } +} + // CLIPRO-3: without a config, commands error and point at `init` instead of // silently starting the interactive config-creation flow. #[test] @@ -319,7 +645,7 @@ fn force_modes_through_the_cli() { migrant() .current_dir(dir.path()) - .args(["apply", "--all", "--force=skip-failures"]) + .args(["apply", "--force=skip-failures"]) .assert() .success() .stdout(contains("skip-failures")); @@ -331,10 +657,13 @@ fn force_modes_through_the_cli() { .stdout(predicates::str::is_match(r"\[ \] \d{14}_a-bad").expect("valid regex")) .stdout(predicates::str::is_match(r"\[✓\] \d{14}_b-good").expect("valid regex")); - // Bare `--force` records the still-failing migration as applied. + // The skip-failures run above left `b-good` applied ahead of the still + // unapplied (and earlier-defined) `a-bad`, so the next run needs + // `--allow-out-of-order` to proceed past that state. Bare `--force` + // then records the still-failing migration as applied. migrant() .current_dir(dir.path()) - .args(["apply", "--all", "--force"]) + .args(["apply", "--force", "--allow-out-of-order"]) .assert() .success(); migrant() @@ -345,9 +674,9 @@ fn force_modes_through_the_cli() { .stdout(predicates::str::is_match(r"\[✓\] \d{14}_a-bad").expect("valid regex")); } -// CLIMIG: `apply --all --no-sync` is accepted and applies migrations -// normally. On sqlite the advisory lock is a no-op, so this proves the flag -// is wired end-to-end (accepted + migrations applied). +// CLIMIG: `apply --no-sync` is accepted and applies migrations normally. On +// sqlite the advisory lock is a no-op, so this proves the flag is wired +// end-to-end (accepted + migrations applied). #[test] fn apply_no_sync_applies_migrations() { let dir = sqlite_project(); @@ -371,7 +700,7 @@ fn apply_no_sync_applies_migrations() { migrant() .current_dir(dir.path()) - .args(["apply", "--all", "--no-sync"]) + .args(["apply", "--no-sync"]) .assert() .success() .stdout(contains("Applying[Up]:")); @@ -401,3 +730,208 @@ fn apply_no_sync_applies_migrations() { .stdout(predicates::str::is_match(r"\[✓\] \d{14}_first").expect("valid regex")) .stdout(predicates::str::is_match(r"\[✓\] \d{14}_second").expect("valid regex")); } + +// CLIMIG: `apply` with no flags applies every pending migration in a single +// invocation (previously only the next one ran unless `-a/--all` was given). +#[test] +fn apply_default_applies_all_pending() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + new_migration( + dir.path(), + "one", + "create table apply_all_a (x integer);", + "drop table apply_all_a;", + ); + new_migration( + dir.path(), + "two", + "create table apply_all_b (x integer);", + "drop table apply_all_b;", + ); + new_migration( + dir.path(), + "three", + "create table apply_all_c (x integer);", + "drop table apply_all_c;", + ); + + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .success(); + + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_one").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_two").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_three").expect("valid regex")); +} + +// CLIMIG: `apply --step N` applies at most N migrations and stops early once +// none remain, instead of erroring. +#[test] +fn apply_step_limits_and_stops_early() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + new_migration( + dir.path(), + "one", + "create table apply_step_a (x integer);", + "drop table apply_step_a;", + ); + new_migration( + dir.path(), + "two", + "create table apply_step_b (x integer);", + "drop table apply_step_b;", + ); + + // `--step 1` applies exactly one, leaving the other pending. + migrant() + .current_dir(dir.path()) + .args(["apply", "--step", "1"]) + .assert() + .success(); + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_one").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[ \] \d{14}_two").expect("valid regex")); + + // `--step 5`, with only one migration left, applies it and stops early + // instead of erroring once nothing remains. + migrant() + .current_dir(dir.path()) + .args(["apply", "--step", "5"]) + .assert() + .success(); + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_one").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_two").expect("valid regex")); +} + +// CLIMIG: `apply --down` without `--step` still reverts a single migration +// by default. +#[test] +fn apply_down_default_reverts_one() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + new_migration( + dir.path(), + "one", + "create table apply_down_a (x integer);", + "drop table apply_down_a;", + ); + new_migration( + dir.path(), + "two", + "create table apply_down_b (x integer);", + "drop table apply_down_b;", + ); + + migrant() + .current_dir(dir.path()) + .arg("apply") + .assert() + .success(); + + migrant() + .current_dir(dir.path()) + .args(["apply", "--down"]) + .assert() + .success(); + + migrant() + .current_dir(dir.path()) + .arg("list") + .assert() + .success() + .stdout(predicates::str::is_match(r"\[✓\] \d{14}_one").expect("valid regex")) + .stdout(predicates::str::is_match(r"\[ \] \d{14}_two").expect("valid regex")); +} + +// CLIMIG-4: `--allow-unknown-tags` / `--allow-out-of-order` are accepted by +// both `apply` and `redo`. The full unknown-tag/out-of-order scenarios are +// covered in the library's own test suite; here we only prove the CLI wires +// the flags through without rejecting them. +#[test] +fn allow_flags_are_accepted_by_apply_and_redo() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + new_migration( + dir.path(), + "one", + "create table allow_flags_a (x integer);", + "drop table allow_flags_a;", + ); + + migrant() + .current_dir(dir.path()) + .args(["apply", "--allow-unknown-tags", "--allow-out-of-order"]) + .assert() + .success(); + + migrant() + .current_dir(dir.path()) + .args(["redo", "--allow-unknown-tags", "--allow-out-of-order"]) + .assert() + .success(); +} + +// CLIMIG: `--all`/`-a` is no longer accepted by `apply` (only `redo` keeps +// it). +#[test] +fn apply_rejects_all_flag_but_redo_still_accepts_it() { + let dir = sqlite_project(); + migrant() + .current_dir(dir.path()) + .arg("setup") + .assert() + .success(); + + migrant() + .current_dir(dir.path()) + .args(["apply", "--all"]) + .assert() + .failure() + .stderr(contains("--all")); + + migrant() + .current_dir(dir.path()) + .args(["apply", "-a"]) + .assert() + .failure(); + + migrant() + .current_dir(dir.path()) + .args(["redo", "--all"]) + .assert() + .success(); +} From fc45276b015ae7b2f64dcde39adfd74d82da3085 Mon Sep 17 00:00:00 2001 From: James Kominick Date: Tue, 4 Aug 2026 08:47:28 -0400 Subject: [PATCH 3/3] update changelog and spec for the bookkeeping and API changes --- CHANGELOG.md | 48 ++++++++++++++++++++++++++++++++ README.md | 2 +- migrant_lib/CHANGELOG.md | 28 +++++++++++++++++++ spec/cli-migration-management.md | 21 ++++++++++---- spec/database-backends.md | 7 +++++ spec/library-config-api.md | 20 +++++++++++++ spec/migration-types.md | 11 ++++++++ spec/migrator-api.md | 17 ++++++++++- spec/settings-builders.md | 16 ++++++++--- 9 files changed, 158 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea6d9b4..8160d27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,53 @@ # Changelog +## [Unreleased] +### Added +- `apply` accepts `--step N` to apply exactly N migrations, in either direction +- `apply` and `redo` accept `--allow-unknown-tags` to permit a run when the database has an + applied tag not present in the defined migration set, and `--allow-out-of-order` to permit + applying migrations out of their defined order. Both are off by default: an unknown applied + tag or an out-of-order pending migration now aborts the run with an error + +### Changed +- `apply` applies all pending migrations by default, instead of just the next one. Use + `--step 1`, or `--down` (which remains single-step by default), to move one migration at a + time +- The `__migrant_migrations` bookkeeping table is now multi-column (`id`, `tag`, `checksum`, + `applied_at`) instead of a single `tag` column. Each applied migration now records a sha256 + checksum of its up-SQL (null for programmatic migrations) and an applied-at timestamp, and + applied order is tracked by `id` rather than inferred from file/tag order. `redo` and + `apply --down` now target the most-recently-applied migration by this recorded order. + + This is a one-time breaking change to the on-disk schema. If you don't need to preserve + applied history, the simplest upgrade is to drop the existing `__migrant_migrations` table + and let `migrant setup` recreate it. To upgrade an existing table in place instead, add the + missing columns and backfill them (existing rows can be left with `checksum` null): + + ```sql + -- postgres + alter table __migrant_migrations add column id bigserial; + alter table __migrant_migrations add column checksum text; + alter table __migrant_migrations add column applied_at timestamptz not null default now(); + + -- sqlite + alter table __migrant_migrations add column id integer; + alter table __migrant_migrations add column checksum text; + alter table __migrant_migrations add column applied_at text not null default (datetime('now')); + + -- mysql + alter table __migrant_migrations add column id bigint unsigned auto_increment unique; + alter table __migrant_migrations add column checksum text; + alter table __migrant_migrations add column applied_at timestamp not null default current_timestamp; + ``` + + Since applied order was not previously tracked, `id` will not necessarily reflect the + original apply order after an in-place upgrade; reconstruct it manually (e.g. from your own + deployment history) if order matters, or prefer the fresh-table path above. + +### Removed +- `apply --all`. Applying all pending migrations is now the default; use `--step N` if you + need to control how many migrations move + ## [1.0.0-rc.2] ### Added - `migrant status` reports every managed migration's applied/pending state with summary counts, diff --git a/README.md b/README.md index b8c5ec1..23c07bc 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,7 @@ When run interactively (without `--no-confirm`), `setup` will be run automatical `migrant status [--format ]` - Report every managed migration's applied/pending state with summary counts, as pretty text (default) or JSON. -`migrant apply [--down, --all, --force, --fake, --no-sync]` - Apply the next available migration[s]. +`migrant apply [--down, --step N, --force, --fake, --no-sync]` - Apply all pending migrations. `--down` reverses direction and applies a single step by default; `--step N` applies exactly N steps in either direction. `migrant redo [--all, --force, --no-sync]` - Re-apply the latest migration (down then up). diff --git a/migrant_lib/CHANGELOG.md b/migrant_lib/CHANGELOG.md index 108fb2a..dda9950 100644 --- a/migrant_lib/CHANGELOG.md +++ b/migrant_lib/CHANGELOG.md @@ -1,5 +1,33 @@ # Changelog +## [Unreleased] +### Added +- `Migratable::checksum()` returns a sha256 checksum of a migration's up-SQL (`None` for + `FnMigration`), recorded in the bookkeeping table when the migration is applied +- `Migrator::allow_unknown_tags(bool)` (default `false`) lets a run proceed when the database + has an applied tag not present in the defined migration set, instead of erroring +- `Migrator::allow_out_of_order(bool)` (default `false`) lets a run apply migrations out of + their defined order, instead of erroring + +### Changed +- `migrant_lib::new` is renamed to `migrant_lib::create_migration` and now returns a + `NewMigration` (with `dir()`/`up_path()`/`down_path()` accessors) instead of `()` +- `migrant_lib::list` moved to `migrant_lib::cli::list` +- `SqliteSettingsBuilder`/`PostgresSettingsBuilder`/`MySqlSettingsBuilder` setters + `database_path`/`migration_location` are now infallible: they take and return `Self` instead + of `Result`. Validation is deferred to `build()` +- `Migratable::description` and `cli::edit` now take `Direction` by value instead of by + reference +- `MigratableClone` is now a sealed trait and can no longer be implemented outside this crate +- The `__migrant_migrations` bookkeeping table is now multi-column (`id`, `tag`, `checksum`, + `applied_at`) instead of a single `tag` column. Applied order is tracked by `id` and is + authoritative; `Down` targets the most-recently-applied migration by this recorded order, + not by scanning the defined migration list. This is a breaking change to the on-disk schema; + see the `migrant` CLI changelog for an upgrade note +- A run now errors by default if the database records an applied tag absent from the defined + migration set, or if it would apply migrations out of order. Opt out with + `Migrator::allow_unknown_tags(true)` / `Migrator::allow_out_of_order(true)` + ## [1.0.0-rc.2] Breaking pre-1.0 release, continuing the API cleanup from rc.1. diff --git a/spec/cli-migration-management.md b/spec/cli-migration-management.md index 3953a91..b9c9188 100644 --- a/spec/cli-migration-management.md +++ b/spec/cli-migration-management.md @@ -18,12 +18,14 @@ file instead. ## CLIMIG-4 -`migrant apply` applies the next unapplied migration. Flags: `--all` applies all remaining, -`--down` reverses direction (unapplies), `--fake` marks migrations applied/unapplied without -executing their SQL. `--force[=]` continues past failed migrations: bare `--force` (or -`--force=accept-failures`) records a failed migration as applied so it is not retried; -`--force=skip-failures` leaves it unrecorded, skips it for the rest of the run, and retries -it on the next run. +`migrant apply` applies all pending migrations by default. Flags: `--step N` applies exactly +N migrations instead of all of them; `--down` reverses direction (unapplies) and defaults to +a single step unless `--step N` is also given; `--fake` marks migrations applied/unapplied +without executing their SQL. `--force[=]` continues past failed migrations: bare +`--force` (or `--force=accept-failures`) records a failed migration as applied so it is not +retried; `--force=skip-failures` leaves it unrecorded, skips it for the rest of the run, and +retries it on the next run. `apply` no longer has an `--all` flag: applying all pending +migrations is the default behavior. ## CLIMIG-5 @@ -37,6 +39,13 @@ counts (total, applied, pending). `--format text` (the default) prints a summary by a `[✓]`/`[ ]` row per migration; `--format json` prints the same data as pretty-printed JSON (`{ total, applied, pending, migrations: [{ tag, applied }] }`) for scripting. +## CLIMIG-7 + +`apply` and `redo` accept `--allow-unknown-tags` and `--allow-out-of-order`, both off by +default. `--allow-unknown-tags` permits a run when the database has an applied tag not +present in the defined migration set, instead of erroring. `--allow-out-of-order` permits a +run to apply migrations out of their defined order, instead of erroring. + Coverage: `tests/migrant.rs` (kitchen_sink, new_rejects_invalid_tag, apply_fake_records_without_running, force_modes_through_the_cli, status_reports_text_and_json), backend integration tests, unit tests in `migrant_lib/src/ops.rs` and `src/status.rs`. diff --git a/spec/database-backends.md b/spec/database-backends.md index b112bae..44463c4 100644 --- a/spec/database-backends.md +++ b/spec/database-backends.md @@ -24,6 +24,13 @@ Connections are established lazily on first use and kept alive per `Config`. Invoking an operation whose backend feature is disabled returns `Error::FeatureRequired` rather than panicking. +## BACKEND-6 + +The `__migrant_migrations` bookkeeping table has four columns on all three backends: `id` +(an auto-incrementing key recording applied order), `tag` (the migration tag), `checksum` +(a sha256 checksum of the migration's up-SQL, null for programmatic migrations), and +`applied_at` (a timestamp of when the migration was applied). + Coverage: `migrant_lib/tests/sqlite.rs`; `server_dbs.rs` (postgres/mysql end-to-end, gated on POSTGRES_TEST_CONN_STR/MYSQL_TEST_CONN_STR, run via `test.sh` against docker databases). diff --git a/spec/library-config-api.md b/spec/library-config-api.md index 0cc6e58..4ae0abf 100644 --- a/spec/library-config-api.md +++ b/spec/library-config-api.md @@ -40,6 +40,26 @@ template. Its setters take and return an owned `Self` so calls chain by value: ` `initialize()` renders and writes the template. Without a database type set it either prompts (interactive) or errors (non-interactive). +## LIBRAR-7 + +`migrant_lib::create_migration(...)` (renamed from `migrant_lib::new`) generates a new up/down +migration file pair and returns a `NewMigration`, with `dir()`, `up_path()`, and `down_path()` +accessors, instead of `()`. + +## LIBRAR-8 + +`migrant_lib::cli::list` (moved from the crate root, where it was `migrant_lib::list`) displays +all managed migrations with their applied status. + +## LIBRAR-9 + +`Migratable::description(Direction)` and `cli::edit(..., Direction)` take `Direction` by value +instead of by reference. + +## LIBRAR-10 + +`MigratableClone` is a sealed trait: it can no longer be implemented outside this crate. + Coverage: `tests/migrant.rs` (init_non_interactive_creates_config, init_rejects_invalid_database_type, init --default-from-env); doc examples in `migrant_lib/src/config/init.rs`. diff --git a/spec/migration-types.md b/spec/migration-types.md index 600e54d..667f9f0 100644 --- a/spec/migration-types.md +++ b/spec/migration-types.md @@ -31,4 +31,15 @@ out (the directive takes precedence); `FnMigration` never runs in a migrator-managed transaction. See [transactional-migrations.md](transactional-migrations.md). +## MIGTYPE-6 + +`Migratable::checksum()` returns a sha256 checksum of a migration's up-SQL, recorded in the +bookkeeping table when the migration is applied. `FileMigration` and `EmbeddedMigration` +compute it from their up-SQL; `FnMigration` (a programmatic migration with no SQL) returns +`None`, recorded as a null checksum. + +## MIGTYPE-7 + +`Migratable::description(Direction)` takes `Direction` by value instead of by reference. + Coverage: `migrant_lib/tests/sqlite.rs`, `server_dbs.rs`, `reload_memory.rs`. diff --git a/spec/migrator-api.md b/spec/migrator-api.md index 84447af..07f9db5 100644 --- a/spec/migrator-api.md +++ b/spec/migrator-api.md @@ -19,7 +19,10 @@ apply returns an empty `Report`, not an error. ## MIGRATOR-2 `direction(Direction::Up|Down)` sets the migration direction; `all(bool)` applies every -remaining migration in that direction instead of just the next one. +remaining migration in that direction instead of just the next one. Applied order is +tracked in the bookkeeping table (by an `id` recorded per migration) and is authoritative +for a `Direction::Down` run: it targets the most-recently-applied migration by this +recorded order, not by scanning the defined migration list. ## MIGRATOR-3 @@ -65,5 +68,17 @@ Migrator::with_config(&config) `with_config(&Config)` and `apply(&self)` are unchanged. +## MIGRATOR-7 + +By default, a run errors before applying anything if either check fails: + +- Unknown tags: the database records an applied tag that is not present in the defined + migration set. +- Out of order: a pending migration would apply out of the order defined by the migration + set, given what is already recorded as applied. + +`Migrator::allow_unknown_tags(bool)` (default `false`) and `Migrator::allow_out_of_order(bool)` +(default `false`) each opt out of the corresponding check. + Coverage: `migrant_lib/tests/sqlite.rs`, `server_dbs.rs`, `reload_memory.rs`, `tests/migrant.rs`; unit tests in `migrant_lib/src/migrator.rs`. diff --git a/spec/settings-builders.md b/spec/settings-builders.md index ef8576d..1301f63 100644 --- a/spec/settings-builders.md +++ b/spec/settings-builders.md @@ -6,7 +6,8 @@ Typed builders for sqlite, postgres, and mysql settings. `Settings::configure_sqlite()` returns a `SqliteSettingsBuilder` with `database_path` (absolute, or relative to the config file), `memory()` for an in-memory database, and -`migration_location`. +`migration_location`. `database_path` and `migration_location` are infallible setters; +invalid values (e.g. an empty path) surface as an error from `build()`, not from the setter. ## SETTIN-2 @@ -28,18 +29,25 @@ characters in passwords and params are safe. ## SETTIN-5 The fluent setters on `SqliteSettingsBuilder`, `PostgresSettingsBuilder`, and -`MySqlSettingsBuilder` (e.g. `database_path(self) -> Result`, `memory(self) -> Self`, +`MySqlSettingsBuilder` (e.g. `database_path(self) -> Self`, `memory(self) -> Self`, `database_name(self) -> Self`) take and return an owned `Self`, not `&mut self`, so calls chain by value: ```rust Settings::configure_sqlite() - .database_path("/abs/path/to/my.db")? - .migration_location("migrations")? + .database_path("/abs/path/to/my.db") + .migration_location("migrations") .build()?; ``` `build(&self)` still takes `&self` and does not consume the builder. +## SETTIN-6 + +`database_path` and `migration_location` are infallible: they take and return `Self` directly +(not `Result`), so they can sit anywhere in a builder chain without an intervening `?`. +Validation of their values (e.g. an empty path) is deferred to `build()`, which still returns +`Result`. + Coverage: `migrant_lib/tests/server_dbs.rs`, `sqlite.rs`; unit tests in `migrant_lib/src/config/builders.rs`. `ssl_cert_file` has no dedicated test.