diff --git a/.changepacks/changepack_log_fIoUZOkWt-518L5MIjOim.json b/.changepacks/changepack_log_fIoUZOkWt-518L5MIjOim.json new file mode 100644 index 00000000..730912ea --- /dev/null +++ b/.changepacks/changepack_log_fIoUZOkWt-518L5MIjOim.json @@ -0,0 +1 @@ +{"changes":{"crates/vespertide-core/Cargo.toml":"Minor","crates/vespertide-planner/Cargo.toml":"Minor","crates/vespertide-query/Cargo.toml":"Minor","crates/vespertide-cli/Cargo.toml":"Minor","crates/vespertide-lsp/Cargo.toml":"Patch"},"note":"data_migration 액션 추가: MigrationAction::DataMigration(#[non_exhaustive]라 additive), DataMigrationSql/leading_ddl_keyword/sql_preview 공개 API 신설, DDL 가드용 PlannerError::DataMigrationContainsDdl 변형 추가(PlannerError는 exhaustive라 0.x 기준 breaking), find_raw_sql_replay_hazards 신설, diff/status의 raw_sql 리플레이 경고","date":"2026-08-20T11:20:29.9859819Z"} diff --git a/crates/vespertide-cli/src/commands/diff/mod.rs b/crates/vespertide-cli/src/commands/diff/mod.rs index f93c4c8d..c74ad238 100644 --- a/crates/vespertide-cli/src/commands/diff/mod.rs +++ b/crates/vespertide-cli/src/commands/diff/mod.rs @@ -9,8 +9,9 @@ use vespertide_planner::{ find_type_narrowings, plan_next_migration, render_reference_action, schema_from_plans, }; +use super::raw_sql_warning::emit_raw_sql_replay_warning; use crate::utils::{load_config, load_migrations, load_models}; -use vespertide_core::action::truncate_comment; +use vespertide_core::action::{sql_preview, truncate_comment}; use vespertide_core::{MigrationAction, MigrationPlan, TableDef}; pub async fn cmd_diff() -> Result<()> { @@ -21,6 +22,10 @@ pub async fn cmd_diff() -> Result<()> { let plan = plan_next_migration(¤t_models, &applied_plans) .map_err(|e| anyhow::anyhow!("planning error: {e}"))?; + // Emitted before the action list: when replay was incomplete the list + // itself is untrustworthy, so the caveat has to arrive first. + emit_raw_sql_replay_warning(&applied_plans); + if plan.actions.is_empty() { println!( "{} {}", @@ -514,6 +519,16 @@ fn format_action(action: &MigrationAction) -> String { sql.bright_cyan() ) } + MigrationAction::DataMigration { sql, description } => { + let summary = description + .clone() + .unwrap_or_else(|| sql_preview(sql.postgres())); + format!( + "{} {}", + "Data migration:".bright_yellow(), + summary.bright_cyan() + ) + } MigrationAction::AddConstraint { constraint, .. } => { format!( "{} {} {} {}", diff --git a/crates/vespertide-cli/src/commands/diff/tests/mod.rs b/crates/vespertide-cli/src/commands/diff/tests/mod.rs index 4c0c17db..f308fba7 100644 --- a/crates/vespertide-cli/src/commands/diff/tests/mod.rs +++ b/crates/vespertide-cli/src/commands/diff/tests/mod.rs @@ -173,6 +173,14 @@ fn chk_age() -> TableConstraint { MigrationAction::RemapEnumValues { table: "users".into(), column: "status".into(), mapping: { let mut m = std::collections::BTreeMap::new(); m.insert(0, 10); m.insert(1, 20); m } }, format!("{} {}.{} [{}]", "Remap enum values:".bright_yellow(), "users".bright_cyan(), "status".bright_cyan().bold(), "0->10, 1->20".bright_white()) )] +#[case( + MigrationAction::DataMigration { sql: "UPDATE users SET tier = 'pro'".into(), description: Some("promote beta users".into()) }, + format!("{} {}", "Data migration:".bright_yellow(), "promote beta users".bright_cyan()) +)] +#[case( + MigrationAction::DataMigration { sql: "UPDATE users SET tier = 'pro'".into(), description: None }, + format!("{} {}", "Data migration:".bright_yellow(), "UPDATE users SET tier = 'pro'".bright_cyan()) +)] #[serial] fn format_action_cases(#[case] action: MigrationAction, #[case] expected: String) { assert_eq!(format_action(&action), expected); @@ -209,6 +217,63 @@ async fn cmd_diff_when_no_changes() { assert!(result.is_ok()); } +#[rstest] +#[serial] +#[tokio::test] +async fn cmd_diff_warns_when_history_contains_raw_sql() { + let tmp = tempdir().unwrap(); + let _guard = CwdGuard::new(&tmp.path().to_path_buf()); + + write_default_config(); + write_simple_id_model("users"); + fs::create_dir_all("migrations").unwrap(); + fs::write( + "migrations/0001_init.json", + serde_json::to_string_pretty(&MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![MigrationAction::RawSql { + sql: "ALTER TABLE users ADD COLUMN legacy int".into(), + }], + }) + .unwrap(), + ) + .unwrap(); + + assert!(cmd_diff().await.is_ok()); +} + +#[rstest] +#[serial] +#[tokio::test] +async fn cmd_diff_stays_quiet_when_history_uses_data_migration() { + let tmp = tempdir().unwrap(); + let _guard = CwdGuard::new(&tmp.path().to_path_buf()); + + write_default_config(); + write_simple_id_model("users"); + fs::create_dir_all("migrations").unwrap(); + fs::write( + "migrations/0001_init.json", + serde_json::to_string_pretty(&MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![MigrationAction::DataMigration { + sql: "UPDATE users SET id = id".into(), + description: Some("no-op backfill".into()), + }], + }) + .unwrap(), + ) + .unwrap(); + + assert!(cmd_diff().await.is_ok()); +} + #[test] fn test_constraint_display_unnamed_index() { let constraint = TableConstraint::Index { diff --git a/crates/vespertide-cli/src/commands/mod.rs b/crates/vespertide-cli/src/commands/mod.rs index d2caaa22..640d8f8b 100644 --- a/crates/vespertide-cli/src/commands/mod.rs +++ b/crates/vespertide-cli/src/commands/mod.rs @@ -4,6 +4,7 @@ pub mod export; pub mod init; pub mod log; pub mod new; +mod raw_sql_warning; pub mod revision; pub mod sql; pub mod status; diff --git a/crates/vespertide-cli/src/commands/raw_sql_warning.rs b/crates/vespertide-cli/src/commands/raw_sql_warning.rs new file mode 100644 index 00000000..ebaaa1fd --- /dev/null +++ b/crates/vespertide-cli/src/commands/raw_sql_warning.rs @@ -0,0 +1,118 @@ +use std::fmt::Write as _; + +use colored::Colorize; +use vespertide_core::MigrationPlan; +use vespertide_planner::{RawSqlReplayHazard, find_raw_sql_replay_hazards}; + +/// Warn that `raw_sql` in the applied history makes baseline replay incomplete. +/// +/// Shared by `vespertide diff` and `vespertide status`. Silent when the history +/// has no `raw_sql`, which is the common case. +pub(super) fn emit_raw_sql_replay_warning(plans: &[MigrationPlan]) { + let hazards = find_raw_sql_replay_hazards(plans); + if hazards.is_empty() { + return; + } + + println!(); + for line in format_raw_sql_replay_warning(&hazards).lines() { + println!("{line}"); + } +} + +/// Render the warning as a multi-line indented block. +/// Extracted so its output can be unit-tested without going through stdout. +fn format_raw_sql_replay_warning(hazards: &[RawSqlReplayHazard]) -> String { + let versions = hazards + .iter() + .map(|hazard| { + let plural = if hazard.count == 1 { "" } else { "s" }; + format!("{} ({} action{})", hazard.version, hazard.count, plural) + }) + .collect::>() + .join(", "); + + let mut out = format!( + "{} {}", + "⚠".bright_yellow().bold(), + format!( + "{} applied migration(s) use raw_sql — baseline replay may be incomplete:", + hazards.len() + ) + .bright_yellow() + ); + let _ = write!( + out, + "\n {} {}", + "versions:".bright_white(), + versions.bright_cyan().bold() + ); + let _ = write!( + out, + "\n {} replay cannot interpret raw SQL, so any schema change those actions made \ + is missing from the reconstructed baseline — `vespertide diff` may report changes \ + that are already applied", + "why:".bright_white() + ); + let _ = write!( + out, + "\n {} re-express schema changes as typed actions; use `data_migration` for \ + data-only SQL so replay can skip it safely", + "fix:".bright_green() + ); + out +} + +#[cfg(test)] +mod tests { + use super::*; + use vespertide_core::MigrationAction; + + fn plan_of(version: u32, actions: Vec) -> MigrationPlan { + MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version, + actions, + } + } + + fn raw() -> MigrationAction { + MigrationAction::RawSql { + sql: "ALTER TABLE users ADD c int".to_string(), + } + } + + #[test] + fn warning_names_every_affected_version_with_its_action_count() { + let rendered = format_raw_sql_replay_warning(&[ + RawSqlReplayHazard { + version: 3, + count: 1, + }, + RawSqlReplayHazard { + version: 7, + count: 2, + }, + ]); + + assert!( + rendered.contains("2 applied migration(s) use raw_sql"), + "{rendered}" + ); + assert!(rendered.contains("3 (1 action)"), "{rendered}"); + assert!(rendered.contains("7 (2 actions)"), "{rendered}"); + assert!(rendered.contains("data_migration"), "{rendered}"); + } + + #[test] + fn emit_is_silent_for_a_history_without_raw_sql() { + emit_raw_sql_replay_warning(&[plan_of(1, vec![])]); + } + + #[test] + fn emit_prints_for_a_history_with_raw_sql() { + emit_raw_sql_replay_warning(&[plan_of(1, vec![raw()])]); + } +} diff --git a/crates/vespertide-cli/src/commands/status.rs b/crates/vespertide-cli/src/commands/status.rs index 975748bb..ac0d5073 100644 --- a/crates/vespertide-cli/src/commands/status.rs +++ b/crates/vespertide-cli/src/commands/status.rs @@ -2,6 +2,7 @@ use anyhow::Result; use colored::Colorize; use vespertide_planner::schema_from_plans; +use super::raw_sql_warning::emit_raw_sql_replay_warning; use crate::utils::{load_config, load_migrations, load_models}; use std::collections::HashSet; @@ -66,6 +67,7 @@ pub async fn cmd_status() -> Result<()> { ); } } + emit_raw_sql_replay_warning(&applied_plans); println!(); println!( @@ -270,6 +272,33 @@ mod tests { cmd_status().await.unwrap(); } + #[tokio::test] + #[serial] + async fn cmd_status_warns_when_history_contains_raw_sql() { + let tmp = tempdir().unwrap(); + let _guard = CwdGuard::new(&tmp.path().to_path_buf()); + + let cfg = write_default_config(); + write_simple_id_model("users"); + fs::create_dir_all(cfg.migrations_dir()).unwrap(); + let plan = MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![MigrationAction::RawSql { + sql: "ALTER TABLE users ADD COLUMN legacy int".into(), + }], + }; + fs::write( + cfg.migrations_dir().join("0001_init.json"), + serde_json::to_string_pretty(&plan).unwrap(), + ) + .unwrap(); + + cmd_status().await.unwrap(); + } + #[tokio::test] #[serial] async fn cmd_status_model_with_description() { diff --git a/crates/vespertide-core/src/action/data_migration.rs b/crates/vespertide-core/src/action/data_migration.rs new file mode 100644 index 00000000..16cffc11 --- /dev/null +++ b/crates/vespertide-core/src/action/data_migration.rs @@ -0,0 +1,338 @@ +//! Wire format and DDL guard for [`MigrationAction::DataMigration`]. +//! +//! [`MigrationAction::DataMigration`]: super::MigrationAction::DataMigration + +use serde::{Deserialize, Serialize}; + +/// The SQL body of a [`MigrationAction::DataMigration`], either one portable +/// statement or one statement per backend. +/// +/// The wire format is untagged, so both shapes are written naturally in a +/// migration file: +/// +/// ```json +/// { "type": "data_migration", "sql": "UPDATE product SET price = 0 WHERE price IS NULL" } +/// ``` +/// +/// ```json +/// { +/// "type": "data_migration", +/// "sql": { +/// "postgres": "UPDATE t SET j = jsonb_build_object('ko', c)", +/// "mysql": "UPDATE t SET j = JSON_OBJECT('ko', c)", +/// "sqlite": "UPDATE t SET j = json_object('ko', c)" +/// } +/// } +/// ``` +/// +/// All three backend keys are required in the per-backend form. A missing key +/// would silently emit nothing for that backend — exactly the class of silent +/// data loss this action exists to prevent. +/// +/// [`MigrationAction::DataMigration`]: super::MigrationAction::DataMigration +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] +#[serde(untagged)] +pub enum DataMigrationSql { + /// One portable statement executed verbatim on every backend. + Uniform(String), + /// One statement per backend, for data changes that cannot be expressed + /// portably (JSON constructors, string functions, upsert syntax, …). + PerBackend { + /// Statement emitted for `PostgreSQL`. + postgres: String, + /// Statement emitted for `MySQL`. + mysql: String, + /// Statement emitted for `SQLite`. + sqlite: String, + }, +} + +impl DataMigrationSql { + /// The statement emitted for `PostgreSQL`, verbatim. + #[must_use] + pub fn postgres(&self) -> &str { + match self { + Self::Uniform(sql) => sql, + Self::PerBackend { postgres, .. } => postgres, + } + } + + /// The statement emitted for `MySQL`, verbatim. + #[must_use] + pub fn mysql(&self) -> &str { + match self { + Self::Uniform(sql) => sql, + Self::PerBackend { mysql, .. } => mysql, + } + } + + /// The statement emitted for `SQLite`, verbatim. + #[must_use] + pub fn sqlite(&self) -> &str { + match self { + Self::Uniform(sql) => sql, + Self::PerBackend { sqlite, .. } => sqlite, + } + } + + /// Every statement this value can emit, in backend order. + /// + /// The DDL guard checks *all* of them: a per-backend form whose `sqlite` + /// branch smuggles in a `DROP TABLE` is just as fatal to baseline replay + /// as a uniform one. + pub fn statements(&self) -> impl Iterator { + match self { + Self::Uniform(sql) => [Some(sql.as_str()), None, None], + Self::PerBackend { + postgres, + mysql, + sqlite, + } => [ + Some(postgres.as_str()), + Some(mysql.as_str()), + Some(sqlite.as_str()), + ], + } + .into_iter() + .flatten() + } +} + +impl From<&str> for DataMigrationSql { + fn from(sql: &str) -> Self { + Self::Uniform(sql.to_string()) + } +} + +impl From for DataMigrationSql { + fn from(sql: String) -> Self { + Self::Uniform(sql) + } +} + +/// Statement keywords that change *schema* rather than *data*. +/// +/// A `data_migration` starting with any of these breaks the action's +/// schema-neutrality contract, so it is rejected at load and plan time. +const DDL_KEYWORDS: [&str; 4] = ["CREATE", "ALTER", "DROP", "TRUNCATE"]; + +/// Strip leading whitespace and SQL comments (`-- line`, `/* block */`) so the +/// DDL guard sees the first real token. +/// +/// Block comments are treated as non-nesting (ANSI SQL behaviour). A +/// deliberately nested comment can therefore hide a keyword from the guard; +/// that is a false *negative* in a pathological case, never a false positive. +fn strip_leading_trivia(sql: &str) -> &str { + let mut rest = sql.trim_start(); + loop { + rest = if let Some(after) = rest.strip_prefix("--") { + after.split_once('\n').map_or("", |(_, tail)| tail) + } else if let Some(after) = rest.strip_prefix("/*") { + after.split_once("*/").map_or("", |(_, tail)| tail) + } else { + return rest; + }; + rest = rest.trim_start(); + } +} + +/// True when `body` opens with `keyword` as a complete token. +/// +/// Matching is ASCII-case-insensitive and requires a token boundary after the +/// keyword, so `CREATED_AT` is not mistaken for `CREATE`. +fn starts_with_keyword(body: &str, keyword: &str) -> bool { + let bytes = body.as_bytes(); + let keyword = keyword.as_bytes(); + if bytes.len() < keyword.len() { + return false; + } + if !bytes[..keyword.len()].eq_ignore_ascii_case(keyword) { + return false; + } + match bytes.get(keyword.len()) { + None => true, + Some(next) => !(next.is_ascii_alphanumeric() || *next == b'_'), + } +} + +/// Return the DDL keyword a statement opens with, if any. +/// +/// Leading whitespace and comments are trimmed first, and the comparison is +/// case-insensitive, so `/* fix up */ drop table t` is caught just like +/// `DROP TABLE t`. +#[must_use] +pub fn leading_ddl_keyword(sql: &str) -> Option<&'static str> { + let body = strip_leading_trivia(sql); + DDL_KEYWORDS + .into_iter() + .find(|keyword| starts_with_keyword(body, keyword)) +} + +/// Bounded single-line preview of a SQL statement for error and warning text. +/// +/// Leading trivia is dropped, internal whitespace runs collapse to one space, +/// and the result is truncated to 60 characters followed by `...`. +#[must_use] +pub fn sql_preview(sql: &str) -> String { + let collapsed = strip_leading_trivia(sql) + .split_whitespace() + .collect::>() + .join(" "); + if collapsed.char_indices().nth(60).is_some() { + let head: String = collapsed.chars().take(57).collect(); + format!("{head}...") + } else { + collapsed + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + #[test] + fn uniform_sql_wire_format_is_a_bare_string() { + let canonical = r#""UPDATE user SET active = true""#; + let parsed: DataMigrationSql = serde_json::from_str(canonical).expect("parse"); + assert_eq!( + parsed, + DataMigrationSql::Uniform("UPDATE user SET active = true".to_string()) + ); + assert_eq!( + serde_json::to_string(&parsed).expect("serialize"), + canonical + ); + } + + #[test] + fn per_backend_sql_wire_format_is_a_three_key_object() { + let canonical = r#"{"postgres":"UPDATE a","mysql":"UPDATE b","sqlite":"UPDATE c"}"#; + let parsed: DataMigrationSql = serde_json::from_str(canonical).expect("parse"); + assert_eq!( + parsed, + DataMigrationSql::PerBackend { + postgres: "UPDATE a".to_string(), + mysql: "UPDATE b".to_string(), + sqlite: "UPDATE c".to_string(), + } + ); + assert_eq!( + serde_json::to_string(&parsed).expect("serialize"), + canonical + ); + } + + #[test] + fn per_backend_sql_requires_every_backend_key() { + let missing_sqlite = r#"{"postgres":"UPDATE a","mysql":"UPDATE b"}"#; + let parsed: Result = serde_json::from_str(missing_sqlite); + assert!( + parsed.is_err(), + "a per-backend form missing a key must not deserialize: {parsed:?}" + ); + } + + #[test] + fn uniform_sql_is_returned_for_every_backend() { + let sql = DataMigrationSql::from("UPDATE user SET x = 1"); + assert_eq!(sql.postgres(), "UPDATE user SET x = 1"); + assert_eq!(sql.mysql(), "UPDATE user SET x = 1"); + assert_eq!(sql.sqlite(), "UPDATE user SET x = 1"); + assert_eq!( + sql.statements().collect::>(), + ["UPDATE user SET x = 1"] + ); + } + + #[test] + fn per_backend_sql_routes_each_backend_to_its_own_statement() { + let sql = DataMigrationSql::PerBackend { + postgres: "UPDATE pg".to_string(), + mysql: "UPDATE my".to_string(), + sqlite: "UPDATE lite".to_string(), + }; + assert_eq!(sql.postgres(), "UPDATE pg"); + assert_eq!(sql.mysql(), "UPDATE my"); + assert_eq!(sql.sqlite(), "UPDATE lite"); + assert_eq!( + sql.statements().collect::>(), + ["UPDATE pg", "UPDATE my", "UPDATE lite"] + ); + } + + #[test] + fn from_owned_string_builds_the_uniform_form() { + let sql = DataMigrationSql::from("UPDATE user SET x = 1".to_string()); + assert_eq!( + sql, + DataMigrationSql::Uniform("UPDATE user SET x = 1".to_string()) + ); + } + + #[rstest] + #[case::create("CREATE TABLE t (id int)", Some("CREATE"))] + #[case::alter("ALTER TABLE t ADD COLUMN c int", Some("ALTER"))] + #[case::drop("DROP TABLE t", Some("DROP"))] + #[case::truncate("TRUNCATE TABLE t", Some("TRUNCATE"))] + #[case::lowercase("drop table t", Some("DROP"))] + #[case::mixed_case("CrEaTe TABLE t (id int)", Some("CREATE"))] + #[case::leading_whitespace("\n\t DROP TABLE t", Some("DROP"))] + #[case::line_comment("-- clean up\nDROP TABLE t", Some("DROP"))] + #[case::block_comment("/* clean up */ DROP TABLE t", Some("DROP"))] + #[case::stacked_comments("-- one\n/* two */\n-- three\nALTER TABLE t", Some("ALTER"))] + #[case::unterminated_line_comment("-- only a comment", None)] + #[case::unterminated_block_comment("/* never closed", None)] + #[case::update("UPDATE user SET active = true", None)] + #[case::insert("INSERT INTO audit SELECT * FROM user", None)] + #[case::delete("DELETE FROM session WHERE expired", None)] + #[case::with_cte("WITH d AS (SELECT 1) UPDATE t SET x = 1", None)] + #[case::ddl_word_inside_body("UPDATE t SET note = 'DROP TABLE'", None)] + #[case::identifier_prefix("CREATED_AT_FIXUP()", None)] + #[case::empty("", None)] + #[case::whitespace_only(" \n ", None)] + fn leading_ddl_keyword_classifies_statements( + #[case] sql: &str, + #[case] expected: Option<&'static str>, + ) { + assert_eq!(leading_ddl_keyword(sql), expected); + } + + #[test] + fn ddl_keyword_needs_a_token_boundary_not_just_a_prefix() { + // `DROPLET` starts with `DROP` but is a different token. + assert_eq!(leading_ddl_keyword("DROPLET the_table"), None); + // A shorter body than the keyword must not index out of bounds. + assert_eq!(leading_ddl_keyword("DRO"), None); + // End-of-input immediately after the keyword is a valid boundary. + assert_eq!(leading_ddl_keyword("DROP"), Some("DROP")); + // A non-ASCII byte right after the keyword still counts as a boundary. + assert_eq!(leading_ddl_keyword("DROP\u{ad6d}"), Some("DROP")); + } + + #[rstest] + #[case::short("UPDATE t SET x = 1", "UPDATE t SET x = 1")] + #[case::collapses_newlines("UPDATE t\n SET x = 1", "UPDATE t SET x = 1")] + #[case::strips_leading_comment("-- why\nUPDATE t SET x = 1", "UPDATE t SET x = 1")] + fn sql_preview_normalises_short_statements(#[case] sql: &str, #[case] expected: &str) { + assert_eq!(sql_preview(sql), expected); + } + + #[test] + fn sql_preview_truncates_at_the_60_character_boundary() { + let exactly_60 = "0123456789".repeat(6); + assert_eq!(sql_preview(&exactly_60), exactly_60); + + let sixty_one = format!("{exactly_60}X"); + let head: String = sixty_one.chars().take(57).collect(); + assert_eq!(sql_preview(&sixty_one), format!("{head}...")); + } + + #[test] + fn sql_preview_counts_characters_not_bytes() { + let multibyte = "한".repeat(61); + let head: String = multibyte.chars().take(57).collect(); + assert_eq!(sql_preview(&multibyte), format!("{head}...")); + } +} diff --git a/crates/vespertide-core/src/action/display.rs b/crates/vespertide-core/src/action/display.rs index 5175e125..2a66b8c3 100644 --- a/crates/vespertide-core/src/action/display.rs +++ b/crates/vespertide-core/src/action/display.rs @@ -1,4 +1,4 @@ -use super::MigrationAction; +use super::{DataMigrationSql, MigrationAction}; use crate::schema::TableConstraint; use std::borrow::Cow; use std::fmt; @@ -53,6 +53,9 @@ fn write_migration_action(f: &mut fmt::Formatter<'_>, action: &MigrationAction) } MigrationAction::RenameTable { from, to } => write!(f, "RenameTable: {from} -> {to}"), MigrationAction::RawSql { sql } => write_raw_sql_action(f, sql), + MigrationAction::DataMigration { sql, description } => { + write_data_migration_action(f, sql, description.as_deref()) + } MigrationAction::RemapEnumValues { table, column, @@ -137,6 +140,17 @@ fn write_raw_sql_action(f: &mut fmt::Formatter<'_>, sql: &str) -> fmt::Result { } } +fn write_data_migration_action( + f: &mut fmt::Formatter<'_>, + sql: &DataMigrationSql, + description: Option<&str>, +) -> fmt::Result { + match description { + Some(description) => write!(f, "DataMigration: {description}"), + None => write!(f, "DataMigration: {}", super::sql_preview(sql.postgres())), + } +} + fn write_constraint_action( f: &mut fmt::Formatter<'_>, action: &str, diff --git a/crates/vespertide-core/src/action/mod.rs b/crates/vespertide-core/src/action/mod.rs index 18568945..75998994 100644 --- a/crates/vespertide-core/src/action/mod.rs +++ b/crates/vespertide-core/src/action/mod.rs @@ -1,9 +1,11 @@ +mod data_migration; mod display; mod narrowing_strategy; mod prefix; mod remap_mapping_serde; use crate::schema::{ColumnDef, ColumnName, ColumnType, TableConstraint, TableName}; +pub use data_migration::{DataMigrationSql, leading_ddl_keyword, sql_preview}; pub use display::truncate_comment; pub use narrowing_strategy::NarrowingStrategy; use serde::{Deserialize, Serialize}; @@ -43,7 +45,9 @@ pub struct MigrationPlan { /// /// Prefer typed actions over [`MigrationAction::RawSql`]. Raw SQL is an emergency escape hatch: /// it is not portable across backends and is skipped during baseline replay, which means the -/// planner cannot reason about it. +/// planner cannot reason about it. For SQL that only changes *data*, use +/// [`MigrationAction::DataMigration`] instead: it is skipped by replay too, but by contract +/// rather than by ignorance. /// /// This enum is `#[non_exhaustive]`: new variants may be added in future releases. /// Downstream `match` expressions should include a wildcard arm. @@ -207,12 +211,44 @@ pub enum MigrationAction { /// **Emergency escape hatch only.** Raw SQL is not portable across backends and is invisible /// to baseline replay, so the planner cannot reason about schema state after this action. /// Use typed actions whenever possible. + /// + /// For SQL that changes **data only**, use [`MigrationAction::DataMigration`] instead. Both + /// are skipped by baseline replay, but `raw_sql` is skipped because its effect is *unknown* + /// while `data_migration` is skipped because *changing no schema* is its enforced contract. + /// Using `raw_sql` for DDL silently drops that schema change from the reconstructed baseline, + /// after which `vespertide diff` reports the same already-applied changes forever. RawSql { sql: String }, + /// Execute a **data-only** statement verbatim (`UPDATE` / `INSERT` / `DELETE` / …). + /// + /// This is the typed home for backfills that the schema-coupled facilities cannot express: + /// conditional updates of *existing* columns, correlated-subquery backfills, and data + /// reshaping during a format change. Unlike `AddColumn.fill_with` (which only fires for a + /// newly added NOT NULL column with no default) or `ModifyColumnDefault.backfill` (one + /// column, one value, every row), `data_migration` carries an arbitrary DML statement and + /// is not tied to any schema change. + /// + /// **Contract: this action changes no schema.** Baseline replay skips it — not because its + /// effect is unknown, as with [`MigrationAction::RawSql`], but because "no schema change" + /// is guaranteed. The guarantee is enforced: a statement whose first token is `CREATE`, + /// `ALTER`, `DROP`, or `TRUNCATE` is rejected at load and plan time. + /// + /// The SQL is emitted byte-for-byte as written — no case folding, cast rewriting, or + /// reformatting. Set `sql` to a single string for portable SQL, or to an object keyed by + /// `postgres` / `mysql` / `sqlite` when the statement cannot be portable. A `description` + /// is optional but strongly encouraged: it is what `vespertide diff` shows for this action. + DataMigration { + /// The statement(s) to run, emitted verbatim. See [`DataMigrationSql`]. + sql: DataMigrationSql, + /// Why this data change exists, shown in `vespertide diff` output. + #[serde(default, skip_serializing_if = "Option::is_none")] + description: Option, + }, } impl MigrationAction { /// Returns the primary table this action affects, if any. - /// Returns None for actions that don't bind to a single table (e.g. `RawSql`). + /// Returns None for actions that don't bind to a single table + /// (e.g. `RawSql`, `DataMigration`). #[must_use] pub fn table_name(&self) -> Option<&str> { match self { @@ -230,9 +266,23 @@ impl MigrationAction { | Self::ReplaceConstraint { table, .. } | Self::RemapEnumValues { table, .. } => Some(table.as_str()), Self::RenameTable { from, .. } => Some(from.as_str()), - Self::RawSql { .. } => None, + Self::RawSql { .. } | Self::DataMigration { .. } => None, } } + + /// The DDL keyword this action's SQL illegally opens with, if any. + /// + /// Only [`MigrationAction::DataMigration`] carries a schema-neutrality + /// contract, so every other variant returns `None`. Per-backend statements + /// are all checked; the first offender wins. + #[must_use] + pub fn data_migration_ddl_violation(&self) -> Option<(&'static str, &str)> { + let Self::DataMigration { sql, .. } = self else { + return None; + }; + sql.statements() + .find_map(|stmt| leading_ddl_keyword(stmt).map(|keyword| (keyword, stmt))) + } } #[cfg(test)] @@ -436,10 +486,146 @@ mod tests { Some("old_users") )] #[case::raw_sql(MigrationAction::RawSql { sql: "SELECT 1".into() }, None)] + #[case::data_migration( + MigrationAction::DataMigration { sql: "UPDATE t SET x = 1".into(), description: None }, + None + )] fn test_table_name(#[case] action: MigrationAction, #[case] expected: Option<&str>) { assert_eq!(action.table_name(), expected); } + #[test] + fn data_migration_wire_format_round_trip_without_description() { + let canonical = r#"{"type":"data_migration","sql":"UPDATE user SET active = true"}"#; + let parsed: MigrationAction = serde_json::from_str(canonical).expect("parse"); + assert_eq!( + parsed, + MigrationAction::DataMigration { + sql: "UPDATE user SET active = true".into(), + description: None, + } + ); + assert_eq!( + serde_json::to_string(&parsed).expect("serialize"), + canonical, + "wire format MUST be byte-identical" + ); + } + + #[test] + fn data_migration_wire_format_round_trip_with_description() { + let canonical = concat!( + r#"{"type":"data_migration","sql":"UPDATE user SET active = true","#, + r#""description":"activate legacy accounts"}"# + ); + let parsed: MigrationAction = serde_json::from_str(canonical).expect("parse"); + assert_eq!( + serde_json::to_string(&parsed).expect("serialize"), + canonical + ); + } + + #[test] + fn data_migration_per_backend_wire_format_round_trip() { + let canonical = concat!( + r#"{"type":"data_migration","sql":{"postgres":"UPDATE a","#, + r#""mysql":"UPDATE b","sqlite":"UPDATE c"}}"# + ); + let parsed: MigrationAction = serde_json::from_str(canonical).expect("parse"); + assert_eq!( + parsed, + MigrationAction::DataMigration { + sql: DataMigrationSql::PerBackend { + postgres: "UPDATE a".into(), + mysql: "UPDATE b".into(), + sqlite: "UPDATE c".into(), + }, + description: None, + } + ); + assert_eq!( + serde_json::to_string(&parsed).expect("serialize"), + canonical + ); + } + + #[rstest] + #[case::uniform_dml("UPDATE user SET active = true".into(), None)] + #[case::uniform_ddl("DROP TABLE user".into(), Some(("DROP", "DROP TABLE user")))] + #[case::commented_ddl( + "-- tidy up\n truncate table audit".into(), + Some(("TRUNCATE", "-- tidy up\n truncate table audit")) + )] + fn data_migration_ddl_violation_detects_uniform_sql( + #[case] sql: DataMigrationSql, + #[case] expected: Option<(&'static str, &str)>, + ) { + let action = MigrationAction::DataMigration { + sql, + description: None, + }; + assert_eq!(action.data_migration_ddl_violation(), expected); + } + + #[test] + fn data_migration_ddl_violation_scans_every_backend_statement() { + let action = MigrationAction::DataMigration { + sql: DataMigrationSql::PerBackend { + postgres: "UPDATE t SET x = 1".into(), + mysql: "UPDATE t SET x = 1".into(), + sqlite: "ALTER TABLE t RENAME TO t2".into(), + }, + description: None, + }; + assert_eq!( + action.data_migration_ddl_violation(), + Some(("ALTER", "ALTER TABLE t RENAME TO t2")), + "a DDL statement hidden in a non-default backend branch must still be caught" + ); + } + + #[test] + fn ddl_violation_is_none_for_every_non_data_migration_action() { + let action = MigrationAction::RawSql { + sql: "DROP TABLE user".to_string(), + }; + assert_eq!( + action.data_migration_ddl_violation(), + None, + "raw_sql keeps its escape-hatch freedom; only data_migration is constrained" + ); + } + + #[rstest] + #[case::with_description( + MigrationAction::DataMigration { + sql: "UPDATE user SET active = true".into(), + description: Some("activate legacy accounts".into()), + }, + "DataMigration: activate legacy accounts" + )] + #[case::without_description( + MigrationAction::DataMigration { + sql: "UPDATE user SET active = true".into(), + description: None, + }, + "DataMigration: UPDATE user SET active = true" + )] + #[case::per_backend_without_description( + MigrationAction::DataMigration { + sql: DataMigrationSql::PerBackend { + postgres: "UPDATE pg".into(), + mysql: "UPDATE my".into(), + sqlite: "UPDATE lite".into(), + }, + description: None, + }, + "DataMigration: UPDATE pg" + )] + fn test_display_data_migration(#[case] action: MigrationAction, #[case] expected: &str) { + assert_eq!(action.to_string(), expected); + } + #[rstest] #[case::add_constraint_primary_key( MigrationAction::AddConstraint { table: "users".into(), constraint: pk_id() }, diff --git a/crates/vespertide-core/src/action/prefix.rs b/crates/vespertide-core/src/action/prefix.rs index 3dd46fe1..e6b9b6bc 100644 --- a/crates/vespertide-core/src/action/prefix.rs +++ b/crates/vespertide-core/src/action/prefix.rs @@ -51,6 +51,9 @@ fn prefix_migration_action(action: MigrationAction, prefix: &str) -> MigrationAc to: to.with_prefix(prefix), }, MigrationAction::RawSql { sql } => MigrationAction::RawSql { sql }, + MigrationAction::DataMigration { sql, description } => { + MigrationAction::DataMigration { sql, description } + } action => prefix_column_or_constraint_action(action, prefix), } } @@ -169,6 +172,22 @@ mod tests { } } + #[test] + fn data_migration_with_prefix_is_a_noop_on_sql_body() { + let action = MigrationAction::DataMigration { + sql: "UPDATE users SET active = true".into(), + description: Some("activate everyone".to_string()), + }; + let prefixed = action.with_prefix("p_"); + match prefixed { + MigrationAction::DataMigration { sql, description } => { + assert_eq!(sql.postgres(), "UPDATE users SET active = true"); + assert_eq!(description.as_deref(), Some("activate everyone")); + } + other => panic!("expected DataMigration, got {other:?}"), + } + } + #[test] fn raw_sql_within_plan_with_prefix_preserves_sql() { // Drives the same RawSql arm via MigrationPlan::with_prefix. diff --git a/crates/vespertide-core/src/arbitrary/mod.rs b/crates/vespertide-core/src/arbitrary/mod.rs index 3b177c97..3d5de407 100644 --- a/crates/vespertide-core/src/arbitrary/mod.rs +++ b/crates/vespertide-core/src/arbitrary/mod.rs @@ -4,6 +4,7 @@ use proptest::{collection, prelude::*}; use crate::{ MigrationAction, + action::DataMigrationSql, schema::{ ColumnDef, ColumnType, ComplexColumnType, DefaultValue, EnumValues, NumValue, ReferenceAction, SimpleColumnType, StrOrBoolOrArray, StringOrBool, TableConstraint, @@ -313,9 +314,28 @@ pub fn arb_migration_action() -> impl Strategy { to: to.into() }), arb_sql().prop_map(|sql| MigrationAction::RawSql { sql }), + arb_data_migration_action(), ] } +fn arb_data_migration_sql() -> impl Strategy { + prop_oneof![ + arb_sql().prop_map(DataMigrationSql::Uniform), + (arb_sql(), arb_sql(), arb_sql()).prop_map(|(postgres, mysql, sqlite)| { + DataMigrationSql::PerBackend { + postgres, + mysql, + sqlite, + } + }), + ] +} + +fn arb_data_migration_action() -> impl Strategy { + (arb_data_migration_sql(), prop::option::of(arb_comment())) + .prop_map(|(sql, description)| MigrationAction::DataMigration { sql, description }) +} + fn arb_create_table_action() -> impl Strategy { ( arb_safe_ident(), @@ -591,10 +611,24 @@ mod tests { | MigrationAction::ReplaceConstraint { .. } | MigrationAction::RenameTable { .. } | MigrationAction::RawSql { .. } + | MigrationAction::DataMigration { .. } | MigrationAction::RemapEnumValues { .. } => {} } } + #[test] + fn arb_data_migration_action_yields_both_sql_forms( + action in arb_data_migration_action() + ) { + let MigrationAction::DataMigration { sql, .. } = action else { + prop_assert!(false, "expected DataMigration"); + return Ok(()); + }; + match sql { + DataMigrationSql::Uniform(_) | DataMigrationSql::PerBackend { .. } => {} + } + } + /// Direct cover for the four high-fanout helpers that compose /// `arb_migration_action`: each one is its own `impl Strategy` /// returning `MigrationAction`, so calling them and asserting on diff --git a/crates/vespertide-core/src/lib.rs b/crates/vespertide-core/src/lib.rs index 7f967160..85d56f1e 100644 --- a/crates/vespertide-core/src/lib.rs +++ b/crates/vespertide-core/src/lib.rs @@ -11,7 +11,9 @@ pub mod migration; pub mod schema; pub mod sql_escape; -pub use action::{MigrationAction, MigrationPlan, NarrowingStrategy}; +pub use action::{ + DataMigrationSql, MigrationAction, MigrationPlan, NarrowingStrategy, leading_ddl_keyword, +}; pub use migration::{MigrationError, MigrationOptions}; pub use schema::{ CheckViolationStrategy, ColumnDef, ColumnName, ColumnType, ComplexColumnType, ConstraintKind, diff --git a/crates/vespertide-lsp/src/diagnostics/locator.rs b/crates/vespertide-lsp/src/diagnostics/locator.rs index b9d1a817..cb7808e6 100644 --- a/crates/vespertide-lsp/src/diagnostics/locator.rs +++ b/crates/vespertide-lsp/src/diagnostics/locator.rs @@ -44,12 +44,12 @@ impl ErrorLocation { use PlannerError::{ AddColumnWithFkRequiresNullable, BetweenBoundaryReversed, CheckSelfContradiction, ColumnExists, ColumnNotFound, ConstraintColumnNotFound, ConstraintTypeChanged, - DanglingForeignKeyAfterDrop, DefaultViolatesCheck, DuplicateEnumValue, - DuplicateEnumVariantName, DuplicateTableName, EmptyConstraintColumns, - ForeignKeyColumnNotFound, ForeignKeyTableNotFound, IndexColumnNotFound, IndexNotFound, - InvalidAutoIncrement, InvalidEnumDefault, MissingFillWith, MissingPrimaryKey, Multiple, - PrimaryKeyColumnNullable, PrimaryKeyRemovedWithoutReplacement, TableExists, - TableNotFound, TableValidation, + DanglingForeignKeyAfterDrop, DataMigrationContainsDdl, DefaultViolatesCheck, + DuplicateEnumValue, DuplicateEnumVariantName, DuplicateTableName, + EmptyConstraintColumns, ForeignKeyColumnNotFound, ForeignKeyTableNotFound, + IndexColumnNotFound, IndexNotFound, InvalidAutoIncrement, InvalidEnumDefault, + MissingFillWith, MissingPrimaryKey, Multiple, PrimaryKeyColumnNullable, + PrimaryKeyRemovedWithoutReplacement, TableExists, TableNotFound, TableValidation, }; match err { @@ -83,7 +83,10 @@ impl ErrorLocation { | TableNotFound(table) | DuplicateTableName(table) | MissingPrimaryKey(table) => Some(Self::table(table)), - TableValidation(_) => None, + // Neither anchors to a model file: `TableValidation` carries only a + // message, and a `data_migration` DDL violation lives in a migration + // file, which the model-file locator cannot address. + TableValidation(_) | DataMigrationContainsDdl { .. } => None, // Column-anchored errors. F12 Scenario C // (`PrimaryKeyColumnNullable`) is a struct variant rather than // a tuple, so its arm is listed separately even though the diff --git a/crates/vespertide-planner/src/apply/data_migration.rs b/crates/vespertide-planner/src/apply/data_migration.rs new file mode 100644 index 00000000..1e23e025 --- /dev/null +++ b/crates/vespertide-planner/src/apply/data_migration.rs @@ -0,0 +1,10 @@ +/// `DataMigration` changes rows, never schema, so replay has nothing to apply. +/// +/// This is a *different* reason from the sibling [`super::raw_sql`] no-op. +/// `RawSql` is skipped because its effect on the schema is **unknown** — any +/// DDL it performed is silently lost from the reconstructed baseline. +/// `DataMigration` is skipped because **changing no schema is its contract**, +/// enforced by the DDL guard in +/// [`crate::validate::validate_migration_plan`]. Replaying a history that +/// contains one therefore yields a schema identical to the one before it. +pub(super) const fn apply_data_migration() {} diff --git a/crates/vespertide-planner/src/apply/mod.rs b/crates/vespertide-planner/src/apply/mod.rs index de75d6a3..f6b831b4 100644 --- a/crates/vespertide-planner/src/apply/mod.rs +++ b/crates/vespertide-planner/src/apply/mod.rs @@ -1,5 +1,6 @@ mod column_ops; mod constraint_ops; +mod data_migration; mod raw_sql; mod table_ops; @@ -86,6 +87,10 @@ pub fn apply_action( column, mapping, } => column_ops::remap_enum_values(schema, table, column, mapping), + MigrationAction::DataMigration { .. } => { + data_migration::apply_data_migration(); + Ok(()) + } MigrationAction::RawSql { .. } | _ => { raw_sql::apply_raw_sql(); Ok(()) diff --git a/crates/vespertide-planner/src/apply/tests/mod.rs b/crates/vespertide-planner/src/apply/tests/mod.rs index 54b303a2..5e0ed22b 100644 --- a/crates/vespertide-planner/src/apply/tests/mod.rs +++ b/crates/vespertide-planner/src/apply/tests/mod.rs @@ -343,6 +343,35 @@ fn apply_action_success_cases(#[case] case: SuccessCase) { assert_eq!(schema, case.expected); } +#[rstest] +#[case::uniform(vespertide_core::DataMigrationSql::Uniform( + "UPDATE users SET name = 'x' WHERE name IS NULL".to_string() +))] +#[case::per_backend(vespertide_core::DataMigrationSql::PerBackend { + postgres: "UPDATE users SET name = 'x'".to_string(), + mysql: "UPDATE users SET name = 'x'".to_string(), + sqlite: "UPDATE users SET name = 'x'".to_string(), +})] +fn apply_data_migration_leaves_schema_untouched(#[case] sql: vespertide_core::DataMigrationSql) { + let before = vec![table( + "users", + vec![col("id", ColumnType::Simple(SimpleColumnType::Integer))], + vec![idx("ix_users__id", vec!["id"])], + )]; + let mut schema = before.clone(); + + apply_action( + &mut schema, + &MigrationAction::DataMigration { + sql, + description: Some("backfill".to_string()), + }, + ) + .unwrap(); + + assert_eq!(schema, before); +} + #[test] fn apply_rename_table_rewrites_foreign_key_ref_table() { let mut schema = vec![ diff --git a/crates/vespertide-planner/src/error.rs b/crates/vespertide-planner/src/error.rs index b22348be..05c89ba4 100644 --- a/crates/vespertide-planner/src/error.rs +++ b/crates/vespertide-planner/src/error.rs @@ -56,6 +56,24 @@ pub enum PlannerError { EmptyConstraintColumns(String, String), #[error("AddColumn requires fill_with when column is NOT NULL without default: {0}.{1}")] MissingFillWith(String, String), + /// A `data_migration` action whose SQL opens with a DDL keyword. + /// + /// `data_migration` is contractually schema-neutral: baseline replay skips + /// it *because* it changes no schema. Hiding DDL inside one would drop that + /// change from the reconstructed baseline permanently, after which + /// `vespertide diff` reports the same already-applied changes on every run. + /// Rejecting the plan at load time keeps the contract enforceable. + #[error( + "data_migration contains DDL: the statement starts with `{keyword}` ({statement}). \ + `data_migration` promises to change data only — baseline replay skips it on that \ + basis, so schema changes hidden here are lost forever and `vespertide diff` will \ + report phantom pending changes. Express the schema change with a typed action, or \ + use `raw_sql` if you genuinely need the escape hatch." + )] + DataMigrationContainsDdl { + keyword: &'static str, + statement: String, + }, #[error("table validation error: {0}")] TableValidation(String), #[error("table '{0}' must have a primary key")] diff --git a/crates/vespertide-planner/src/lib.rs b/crates/vespertide-planner/src/lib.rs index 9b18b7e1..212e370f 100644 --- a/crates/vespertide-planner/src/lib.rs +++ b/crates/vespertide-planner/src/lib.rs @@ -30,7 +30,7 @@ pub use validate::{ CheckTypeMismatchWarning, ConstraintDropWarning, DanglingFkDrop, DefaultChangeKind, DefaultChangeWarning, EnumFillWithRequired, FillWithRequired, FkOrphanAdditionWarning, FkPolicyChangeWarning, MissingFkSupportingIndex, NarrowingKind, PkAdditionKind, PkKind, - PolicyDelta, PrimaryKeyAdditionWarning, RiskLevel, SequenceExhaustionKind, + PolicyDelta, PrimaryKeyAdditionWarning, RawSqlReplayHazard, RiskLevel, SequenceExhaustionKind, SequenceExhaustionWarning, SequenceRiskLevel, TimezoneConversionDirection, TimezoneConversionWarning, TypeNarrowingWarning, UniqueAdditionFkReference, UniqueAdditionWarning, find_addcolumn_fk_nullable_violations, find_between_boundary_reversals, @@ -39,8 +39,9 @@ pub use validate::{ find_constraint_type_changes, find_dangling_fk_drops, find_default_changes, find_fk_orphan_additions, find_fk_policy_changes, find_missing_enum_fill_with, find_missing_fill_with, find_missing_fk_supporting_indexes, find_plan_violations, - find_primary_key_additions, find_primary_key_removals, find_schema_violations, - find_self_contradictions, find_sequence_exhaustion_risks, find_timezone_conversions, - find_type_narrowings, find_unique_additions, is_narrowing, lex_check_expr, parse_check_expr, - render_reference_action, validate_migration_plan, validate_schema, + find_primary_key_additions, find_primary_key_removals, find_raw_sql_replay_hazards, + find_schema_violations, find_self_contradictions, find_sequence_exhaustion_risks, + find_timezone_conversions, find_type_narrowings, find_unique_additions, is_narrowing, + lex_check_expr, parse_check_expr, render_reference_action, validate_migration_plan, + validate_schema, }; diff --git a/crates/vespertide-planner/src/validate/mod.rs b/crates/vespertide-planner/src/validate/mod.rs index 45c4490c..6c93d01f 100644 --- a/crates/vespertide-planner/src/validate/mod.rs +++ b/crates/vespertide-planner/src/validate/mod.rs @@ -17,6 +17,7 @@ mod fk_policy_changes; mod foreign_keys; mod pk_additions; mod plan; +mod raw_sql_replay; mod schema; mod sequence_exhaustion; mod timezone_conversion; @@ -52,6 +53,7 @@ pub use plan::{ EnumFillWithRequired, FillWithRequired, find_missing_enum_fill_with, find_missing_fill_with, find_plan_violations, validate_migration_plan, }; +pub use raw_sql_replay::{RawSqlReplayHazard, find_raw_sql_replay_hazards}; pub use schema::{find_schema_violations, validate_schema}; pub use sequence_exhaustion::{ SequenceExhaustionKind, SequenceExhaustionWarning, SequenceRiskLevel, diff --git a/crates/vespertide-planner/src/validate/plan.rs b/crates/vespertide-planner/src/validate/plan.rs index 4d290583..38f50e88 100644 --- a/crates/vespertide-planner/src/validate/plan.rs +++ b/crates/vespertide-planner/src/validate/plan.rs @@ -1,7 +1,7 @@ use rayon::prelude::*; use vespertide_core::{ ColumnType, ComplexColumnType, EnumValues, MigrationAction, MigrationPlan, TableConstraint, - TableDef, + TableDef, action::sql_preview, }; use super::enums::validate_enum_value; @@ -22,6 +22,7 @@ use crate::parallel_config::{VALIDATE_PLAN_PAR_ACTION_MIN_LEN, validate_plan_par /// - `AddColumn` actions with NOT NULL columns without default must have `fill_with` /// - `ModifyColumnNullable` actions changing from nullable to non-nullable must have `fill_with` /// - Enum columns with `default/fill_with` values must have valid enum values +/// - `DataMigration` actions must not carry DDL (see [`PlannerError::DataMigrationContainsDdl`]) pub fn validate_migration_plan(plan: &MigrationPlan) -> Result<(), PlannerError> { let mut violations = find_plan_violations(plan); match violations.len() { @@ -130,6 +131,14 @@ fn validate_action(action: &MigrationAction) -> Result<(), PlannerError> { } } } + MigrationAction::DataMigration { .. } => { + if let Some((keyword, statement)) = action.data_migration_ddl_violation() { + return Err(PlannerError::DataMigrationContainsDdl { + keyword, + statement: sql_preview(statement), + }); + } + } _ => {} } diff --git a/crates/vespertide-planner/src/validate/raw_sql_replay.rs b/crates/vespertide-planner/src/validate/raw_sql_replay.rs new file mode 100644 index 00000000..d2aeb121 --- /dev/null +++ b/crates/vespertide-planner/src/validate/raw_sql_replay.rs @@ -0,0 +1,145 @@ +use vespertide_core::{MigrationAction, MigrationPlan}; + +/// One applied migration whose `raw_sql` actions make baseline replay +/// incomplete. +/// +/// [`crate::schema_from_plans`] cannot interpret raw SQL, so it skips those +/// actions entirely. When the SQL was pure DML that is harmless; when it was +/// DDL the reconstructed baseline permanently lacks that schema change and +/// `vespertide diff` reports the same already-applied changes on every run. +/// Nothing in the plan distinguishes the two cases, so every `raw_sql` in the +/// history is reported and the user decides. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RawSqlReplayHazard { + /// Version of the applied migration containing the `raw_sql` action(s). + pub version: u32, + /// How many `raw_sql` actions that migration carries. + pub count: usize, +} + +/// Find applied migrations containing `raw_sql`, in version order. +/// +/// Returns an empty vec when the history is fully replayable, which is the +/// common case — callers should stay silent then rather than emit a warning. +#[must_use] +pub fn find_raw_sql_replay_hazards(plans: &[MigrationPlan]) -> Vec { + plans + .iter() + .filter_map(|plan| { + let count = plan + .actions + .iter() + .filter(|action| matches!(action, MigrationAction::RawSql { .. })) + .count(); + (count > 0).then_some(RawSqlReplayHazard { + version: plan.version, + count, + }) + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use vespertide_core::{ColumnType, DataMigrationSql, SimpleColumnType}; + + fn plan_of(version: u32, actions: Vec) -> MigrationPlan { + MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version, + actions, + } + } + + fn raw(sql: &str) -> MigrationAction { + MigrationAction::RawSql { + sql: sql.to_string(), + } + } + + fn create_users() -> MigrationAction { + MigrationAction::CreateTable { + table: "users".into(), + columns: vec![vespertide_core::ColumnDef::new( + "id", + ColumnType::Simple(SimpleColumnType::Integer), + false, + )], + constraints: vec![], + } + } + + #[test] + fn clean_history_reports_no_hazard() { + let plans = vec![plan_of(1, vec![create_users()])]; + assert!(find_raw_sql_replay_hazards(&plans).is_empty()); + } + + #[test] + fn data_migration_is_not_a_replay_hazard() { + let plans = vec![plan_of( + 1, + vec![MigrationAction::DataMigration { + sql: DataMigrationSql::Uniform("UPDATE users SET active = true".into()), + description: None, + }], + )]; + assert!( + find_raw_sql_replay_hazards(&plans).is_empty(), + "data_migration is skipped by contract, so replay stays complete" + ); + } + + #[test] + fn raw_sql_migrations_are_reported_with_version_and_count() { + let plans = vec![ + plan_of(1, vec![create_users()]), + plan_of(3, vec![raw("CREATE INDEX ix ON users (id)")]), + plan_of(4, vec![create_users()]), + plan_of( + 7, + vec![ + raw("ALTER TABLE users ADD c int"), + raw("UPDATE users SET c = 1"), + ], + ), + ]; + + assert_eq!( + find_raw_sql_replay_hazards(&plans), + vec![ + RawSqlReplayHazard { + version: 3, + count: 1 + }, + RawSqlReplayHazard { + version: 7, + count: 2 + }, + ] + ); + } + + #[test] + fn mixed_migration_counts_only_its_raw_sql_actions() { + let plans = vec![plan_of( + 2, + vec![create_users(), raw("UPDATE users SET x = 1")], + )]; + assert_eq!( + find_raw_sql_replay_hazards(&plans), + vec![RawSqlReplayHazard { + version: 2, + count: 1 + }] + ); + } + + #[test] + fn empty_history_reports_no_hazard() { + assert!(find_raw_sql_replay_hazards(&[]).is_empty()); + } +} diff --git a/crates/vespertide-planner/src/validate/tests/data_migration_ddl.rs b/crates/vespertide-planner/src/validate/tests/data_migration_ddl.rs new file mode 100644 index 00000000..906e0915 --- /dev/null +++ b/crates/vespertide-planner/src/validate/tests/data_migration_ddl.rs @@ -0,0 +1,140 @@ +use super::*; +use vespertide_core::DataMigrationSql; + +fn plan_with(sql: DataMigrationSql) -> MigrationPlan { + MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![MigrationAction::DataMigration { + sql, + description: None, + }], + } +} + +#[rstest] +#[case::update("UPDATE user SET tier = 'pro' WHERE kind = 'internal'")] +#[case::conditional_backfill("UPDATE user SET a = 1, b = 2 WHERE type = 'legacy'")] +#[case::correlated_subquery( + "UPDATE post SET author_id = (SELECT id FROM author a WHERE a.name = post.author_name) \ + WHERE (SELECT count(*) FROM author a WHERE a.name = post.author_name) = 1" +)] +#[case::insert("INSERT INTO audit (kind) SELECT 'backfill' FROM user")] +#[case::delete("DELETE FROM session WHERE expires_at < now()")] +#[case::with_cte("WITH stale AS (SELECT id FROM s) DELETE FROM s USING stale")] +fn data_only_sql_is_accepted(#[case] sql: &str) { + assert!(validate_migration_plan(&plan_with(sql.into())).is_ok()); +} + +#[rstest] +#[case::create("CREATE TABLE t (id int)", "CREATE")] +#[case::alter("ALTER TABLE user ADD COLUMN c int", "ALTER")] +#[case::drop("DROP TABLE user", "DROP")] +#[case::truncate("TRUNCATE TABLE user", "TRUNCATE")] +#[case::lowercase("drop table user", "DROP")] +#[case::leading_comment("-- oops\nCREATE INDEX ix ON user (id)", "CREATE")] +#[case::block_comment("/* oops */ ALTER TABLE user ADD c int", "ALTER")] +fn ddl_sql_is_rejected_with_the_offending_keyword( + #[case] sql: &str, + #[case] expected_keyword: &str, +) { + let err = validate_migration_plan(&plan_with(sql.into())) + .expect_err("DDL inside data_migration must be rejected"); + + match err { + PlannerError::DataMigrationContainsDdl { keyword, statement } => { + assert_eq!(keyword, expected_keyword); + assert!( + !statement.is_empty(), + "the error must quote the offending statement" + ); + } + other => panic!("expected DataMigrationContainsDdl, got {other:?}"), + } +} + +#[test] +fn ddl_error_message_explains_the_replay_contract() { + let err = validate_migration_plan(&plan_with("DROP TABLE user".into())) + .expect_err("DDL must be rejected"); + let message = err.to_string(); + + assert!(message.contains("DROP"), "{message}"); + assert!(message.contains("DROP TABLE user"), "{message}"); + assert!(message.contains("raw_sql"), "{message}"); +} + +#[test] +fn ddl_hidden_in_a_single_backend_branch_is_rejected() { + let plan = plan_with(DataMigrationSql::PerBackend { + postgres: "UPDATE user SET x = 1".into(), + mysql: "UPDATE user SET x = 1".into(), + sqlite: "DROP TABLE user".into(), + }); + + let err = validate_migration_plan(&plan).expect_err("per-backend DDL must be rejected"); + assert!(matches!( + err, + PlannerError::DataMigrationContainsDdl { + keyword: "DROP", + .. + } + )); +} + +#[test] +fn portable_per_backend_data_sql_is_accepted() { + let plan = plan_with(DataMigrationSql::PerBackend { + postgres: "UPDATE t SET j = jsonb_build_object('ko', c)".into(), + mysql: "UPDATE t SET j = JSON_OBJECT('ko', c)".into(), + sqlite: "UPDATE t SET j = json_object('ko', c)".into(), + }); + assert!(validate_migration_plan(&plan).is_ok()); +} + +#[test] +fn raw_sql_keeps_its_ddl_escape_hatch() { + let plan = MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![MigrationAction::RawSql { + sql: "DROP TABLE user".to_string(), + }], + }; + assert!( + validate_migration_plan(&plan).is_ok(), + "the guard must constrain data_migration only" + ); +} + +#[test] +fn every_offending_action_is_reported_not_just_the_first() { + let plan = MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![ + MigrationAction::DataMigration { + sql: "DROP TABLE a".into(), + description: None, + }, + MigrationAction::DataMigration { + sql: "CREATE TABLE b (id int)".into(), + description: None, + }, + ], + }; + + let violations = find_plan_violations(&plan); + assert_eq!(violations.len(), 2); + assert!( + violations + .iter() + .all(|violation| matches!(violation, PlannerError::DataMigrationContainsDdl { .. })) + ); +} diff --git a/crates/vespertide-planner/src/validate/tests/mod.rs b/crates/vespertide-planner/src/validate/tests/mod.rs index 5c44f1eb..cf6e2386 100644 --- a/crates/vespertide-planner/src/validate/tests/mod.rs +++ b/crates/vespertide-planner/src/validate/tests/mod.rs @@ -40,6 +40,7 @@ fn is_missing_pk(err: &PlannerError) -> bool { mod check_default; mod constraint_drops; mod dangling_fk_drops; +mod data_migration_ddl; mod enum_fill_with; mod fill_with; mod fk_policy_changes; diff --git a/crates/vespertide-query/src/sql/data_migration.rs b/crates/vespertide-query/src/sql/data_migration.rs new file mode 100644 index 00000000..6be19b62 --- /dev/null +++ b/crates/vespertide-query/src/sql/data_migration.rs @@ -0,0 +1,75 @@ +use vespertide_core::DataMigrationSql; + +use super::types::{BuiltQuery, RawSql}; + +/// Emit a `DataMigration` action's SQL **verbatim**, per backend. +/// +/// The statement is passed through untouched: no case folding, no cast +/// rewriting, no reformatting. Every backend-normalising helper in this crate +/// (`convert_default_for_backend`, `normalize_fill_with`, …) is deliberately +/// bypassed — the user wrote executable SQL and gets exactly that SQL back. +pub fn build_data_migration(sql: &DataMigrationSql) -> Vec { + vec![BuiltQuery::Raw(RawSql::per_backend( + sql.postgres().to_string(), + sql.mysql().to_string(), + sql.sqlite().to_string(), + ))] +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::sql::types::DatabaseBackend; + use insta::{assert_snapshot, with_settings}; + use rstest::rstest; + + /// SQL deliberately loaded with everything the other emitters rewrite: + /// mixed-case keywords, a `::` cast, a quoted identifier, a string + /// literal, and multi-line formatting. + const HOSTILE_SQL: &str = + "UpDaTe \"User\"\n SET meta = '{\"a\": 1}'::jsonb, n = N + 1\n WHERE Kind = 'Legacy';"; + + #[rstest] + #[case::postgres(DatabaseBackend::Postgres)] + #[case::mysql(DatabaseBackend::MySql)] + #[case::sqlite(DatabaseBackend::Sqlite)] + fn uniform_sql_is_emitted_byte_for_byte(#[case] backend: DatabaseBackend) { + let queries = build_data_migration(&DataMigrationSql::Uniform(HOSTILE_SQL.to_string())); + + assert_eq!(queries.len(), 1); + assert_eq!( + queries[0].build(backend), + HOSTILE_SQL, + "data_migration SQL must survive emission unchanged" + ); + + with_settings!({ snapshot_suffix => format!("data_migration_uniform_{backend:?}") }, { + assert_snapshot!(queries[0].build(backend)); + }); + } + + #[rstest] + #[case::postgres( + DatabaseBackend::Postgres, + "UPDATE t SET j = jsonb_build_object('ko', c)" + )] + #[case::mysql(DatabaseBackend::MySql, "UPDATE t SET j = JSON_OBJECT('ko', c)")] + #[case::sqlite(DatabaseBackend::Sqlite, "UPDATE t SET j = json_object('ko', c)")] + fn per_backend_sql_selects_the_matching_statement( + #[case] backend: DatabaseBackend, + #[case] expected: &str, + ) { + let queries = build_data_migration(&DataMigrationSql::PerBackend { + postgres: "UPDATE t SET j = jsonb_build_object('ko', c)".to_string(), + mysql: "UPDATE t SET j = JSON_OBJECT('ko', c)".to_string(), + sqlite: "UPDATE t SET j = json_object('ko', c)".to_string(), + }); + + assert_eq!(queries.len(), 1); + assert_eq!(queries[0].build(backend), expected); + + with_settings!({ snapshot_suffix => format!("data_migration_per_backend_{backend:?}") }, { + assert_snapshot!(queries[0].build(backend)); + }); + } +} diff --git a/crates/vespertide-query/src/sql/mod.rs b/crates/vespertide-query/src/sql/mod.rs index 10acfa93..0b0ba519 100644 --- a/crates/vespertide-query/src/sql/mod.rs +++ b/crates/vespertide-query/src/sql/mod.rs @@ -1,6 +1,7 @@ pub mod add_column; pub mod add_constraint; pub mod create_table; +pub mod data_migration; pub mod delete_column; pub mod delete_table; pub(crate) mod fill_with; @@ -25,8 +26,9 @@ use vespertide_core::{MigrationAction, TableConstraint, TableDef}; use self::{ add_column::build_add_column, add_constraint::build_add_constraint, - create_table::build_create_table, delete_column::build_delete_column, - delete_table::build_delete_table, modify_column_comment::build_modify_column_comment, + create_table::build_create_table, data_migration::build_data_migration, + delete_column::build_delete_column, delete_table::build_delete_table, + modify_column_comment::build_modify_column_comment, modify_column_default::build_modify_column_default, modify_column_nullable::build_modify_column_nullable, remap_enum_values::build_remap_enum_values, remove_constraint::build_remove_constraint, @@ -53,7 +55,7 @@ pub fn build_action_queries( /// to avoid recreating indexes that will be created by future `AddConstraint` actions. #[expect( clippy::too_many_lines, - reason = "flat 15-variant MigrationAction dispatcher kept inline so the variant→builder mapping stays auditable; extracting individual arms scatters the routing logic" + reason = "flat 16-variant MigrationAction dispatcher kept inline so the variant→builder mapping stays auditable; extracting individual arms scatters the routing logic" )] pub fn build_action_queries_with_pending( backend: DatabaseBackend, @@ -168,6 +170,8 @@ pub fn build_action_queries_with_pending( MigrationAction::RawSql { sql } => Ok(vec![BuiltQuery::Raw(RawSql::uniform(sql.clone()))]), + MigrationAction::DataMigration { sql, .. } => Ok(build_data_migration(sql)), + MigrationAction::AddConstraint { .. } | MigrationAction::RemoveConstraint { .. } | MigrationAction::ReplaceConstraint { .. } => { diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_MySql.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_MySql.snap new file mode 100644 index 00000000..e33976eb --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_MySql.snap @@ -0,0 +1,5 @@ +--- +source: crates/vespertide-query/src/sql/data_migration.rs +expression: "queries[0].build(backend)" +--- +UPDATE t SET j = JSON_OBJECT('ko', c) diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_Postgres.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_Postgres.snap new file mode 100644 index 00000000..315502fb --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_Postgres.snap @@ -0,0 +1,5 @@ +--- +source: crates/vespertide-query/src/sql/data_migration.rs +expression: "queries[0].build(backend)" +--- +UPDATE t SET j = jsonb_build_object('ko', c) diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_Sqlite.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_Sqlite.snap new file mode 100644 index 00000000..7935eef8 --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__per_backend_sql_selects_the_matching_statement@data_migration_per_backend_Sqlite.snap @@ -0,0 +1,5 @@ +--- +source: crates/vespertide-query/src/sql/data_migration.rs +expression: "queries[0].build(backend)" +--- +UPDATE t SET j = json_object('ko', c) diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_MySql.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_MySql.snap new file mode 100644 index 00000000..e51f061e --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_MySql.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/data_migration.rs +expression: "queries[0].build(backend)" +--- +UpDaTe "User" + SET meta = '{"a": 1}'::jsonb, n = N + 1 + WHERE Kind = 'Legacy'; diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_Postgres.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_Postgres.snap new file mode 100644 index 00000000..e51f061e --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_Postgres.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/data_migration.rs +expression: "queries[0].build(backend)" +--- +UpDaTe "User" + SET meta = '{"a": 1}'::jsonb, n = N + 1 + WHERE Kind = 'Legacy'; diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_Sqlite.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_Sqlite.snap new file mode 100644 index 00000000..e51f061e --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__data_migration__tests__uniform_sql_is_emitted_byte_for_byte@data_migration_uniform_Sqlite.snap @@ -0,0 +1,7 @@ +--- +source: crates/vespertide-query/src/sql/data_migration.rs +expression: "queries[0].build(backend)" +--- +UpDaTe "User" + SET meta = '{"a": 1}'::jsonb, n = N + 1 + WHERE Kind = 'Legacy'; diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_MySql.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_MySql.snap new file mode 100644 index 00000000..cf51279f --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_MySql.snap @@ -0,0 +1,5 @@ +--- +source: crates/vespertide-query/src/sql/tests/dispatch.rs +expression: sql +--- +UPDATE "User" SET Tier = 'PRO'::text WHERE Kind = 'internal'; diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_Postgres.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_Postgres.snap new file mode 100644 index 00000000..cf51279f --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_Postgres.snap @@ -0,0 +1,5 @@ +--- +source: crates/vespertide-query/src/sql/tests/dispatch.rs +expression: sql +--- +UPDATE "User" SET Tier = 'PRO'::text WHERE Kind = 'internal'; diff --git a/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_Sqlite.snap b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_Sqlite.snap new file mode 100644 index 00000000..cf51279f --- /dev/null +++ b/crates/vespertide-query/src/sql/snapshots/vespertide_query__sql__tests__dispatch__build_action_queries_data_migration@data_migration_Sqlite.snap @@ -0,0 +1,5 @@ +--- +source: crates/vespertide-query/src/sql/tests/dispatch.rs +expression: sql +--- +UPDATE "User" SET Tier = 'PRO'::text WHERE Kind = 'internal'; diff --git a/crates/vespertide-query/src/sql/tests/dispatch.rs b/crates/vespertide-query/src/sql/tests/dispatch.rs index 8efaf509..8ac2b5a7 100644 --- a/crates/vespertide-query/src/sql/tests/dispatch.rs +++ b/crates/vespertide-query/src/sql/tests/dispatch.rs @@ -565,6 +565,28 @@ fn test_build_action_queries_raw_sql(#[case] backend: DatabaseBackend) { }); } +#[rstest] +#[case::data_migration_postgres(DatabaseBackend::Postgres)] +#[case::data_migration_mysql(DatabaseBackend::MySql)] +#[case::data_migration_sqlite(DatabaseBackend::Sqlite)] +fn test_build_action_queries_data_migration(#[case] backend: DatabaseBackend) { + let action = MigrationAction::DataMigration { + sql: "UPDATE \"User\" SET Tier = 'PRO'::text WHERE Kind = 'internal';".into(), + description: Some("promote internal accounts".into()), + }; + let result = build_action_queries(backend, &action, &[]).unwrap(); + assert_eq!(result.len(), 1); + let sql = result[0].build(backend); + assert_eq!( + sql, + "UPDATE \"User\" SET Tier = 'PRO'::text WHERE Kind = 'internal';" + ); + + with_settings!({ snapshot_path => "../snapshots", snapshot_suffix => format!("data_migration_{:?}", backend) }, { + assert_snapshot!(sql); + }); +} + // Comprehensive index naming tests #[rstest] #[case::add_index_with_custom_name_postgres( diff --git a/schemas/migration.schema.json b/schemas/migration.schema.json index 9b640eb5..c48e9db7 100644 --- a/schemas/migration.schema.json +++ b/schemas/migration.schema.json @@ -278,6 +278,38 @@ } ] }, + "DataMigrationSql": { + "description": "The SQL body of a [`MigrationAction::DataMigration`], either one portable\nstatement or one statement per backend.\n\nThe wire format is untagged, so both shapes are written naturally in a\nmigration file:\n\n```json\n{ \"type\": \"data_migration\", \"sql\": \"UPDATE product SET price = 0 WHERE price IS NULL\" }\n```\n\n```json\n{\n \"type\": \"data_migration\",\n \"sql\": {\n \"postgres\": \"UPDATE t SET j = jsonb_build_object('ko', c)\",\n \"mysql\": \"UPDATE t SET j = JSON_OBJECT('ko', c)\",\n \"sqlite\": \"UPDATE t SET j = json_object('ko', c)\"\n }\n}\n```\n\nAll three backend keys are required in the per-backend form. A missing key\nwould silently emit nothing for that backend — exactly the class of silent\ndata loss this action exists to prevent.\n\n[`MigrationAction::DataMigration`]: super::MigrationAction::DataMigration", + "anyOf": [ + { + "description": "One portable statement executed verbatim on every backend.", + "type": "string" + }, + { + "description": "One statement per backend, for data changes that cannot be expressed\nportably (JSON constructors, string functions, upsert syntax, …).", + "type": "object", + "properties": { + "mysql": { + "description": "Statement emitted for `MySQL`.", + "type": "string" + }, + "postgres": { + "description": "Statement emitted for `PostgreSQL`.", + "type": "string" + }, + "sqlite": { + "description": "Statement emitted for `SQLite`.", + "type": "string" + } + }, + "required": [ + "postgres", + "mysql", + "sqlite" + ] + } + ] + }, "DefaultValue": { "description": "A column default value that can be a boolean, integer, float, or SQL expression string.\n\nIn JSON model files the `\"default\"` field accepts any of these forms:\n- `true` / `false` — boolean literal.\n- `0`, `42` — integer literal.\n- `0.0`, `1.5` — floating-point literal.\n- `\"'pending'\"` — SQL string literal (note the inner single quotes).\n- `\"NOW()\"` — SQL function call (no surrounding quotes).\n\nUse [`DefaultValue::to_sql`] to convert to the SQL representation for DDL generation.\n\n`StringOrBool` is a backwards-compatibility alias for this type.\n\nThis enum is `#[non_exhaustive]`: new variants may be added in future releases.\nDownstream `match` expressions should include a wildcard arm.", "anyOf": [ @@ -431,7 +463,7 @@ ] }, "MigrationAction": { - "description": "A single schema change produced by the planner and consumed by the SQL generator.\n\nThe planner emits a `Vec` when diffing two schemas. The SQL generator\n(`vespertide-query`) translates each action into backend-specific DDL statements.\n\nPrefer typed actions over [`MigrationAction::RawSql`]. Raw SQL is an emergency escape hatch:\nit is not portable across backends and is skipped during baseline replay, which means the\nplanner cannot reason about it.\n\nThis enum is `#[non_exhaustive]`: new variants may be added in future releases.\nDownstream `match` expressions should include a wildcard arm.", + "description": "A single schema change produced by the planner and consumed by the SQL generator.\n\nThe planner emits a `Vec` when diffing two schemas. The SQL generator\n(`vespertide-query`) translates each action into backend-specific DDL statements.\n\nPrefer typed actions over [`MigrationAction::RawSql`]. Raw SQL is an emergency escape hatch:\nit is not portable across backends and is skipped during baseline replay, which means the\nplanner cannot reason about it. For SQL that only changes *data*, use\n[`MigrationAction::DataMigration`] instead: it is skipped by replay too, but by contract\nrather than by ignorance.\n\nThis enum is `#[non_exhaustive]`: new variants may be added in future releases.\nDownstream `match` expressions should include a wildcard arm.", "oneOf": [ { "description": "Create a new table with the given columns and constraints (`CREATE TABLE`).", @@ -828,7 +860,7 @@ ] }, { - "description": "Execute a raw SQL statement verbatim.\n\n**Emergency escape hatch only.** Raw SQL is not portable across backends and is invisible\nto baseline replay, so the planner cannot reason about schema state after this action.\nUse typed actions whenever possible.", + "description": "Execute a raw SQL statement verbatim.\n\n**Emergency escape hatch only.** Raw SQL is not portable across backends and is invisible\nto baseline replay, so the planner cannot reason about schema state after this action.\nUse typed actions whenever possible.\n\nFor SQL that changes **data only**, use [`MigrationAction::DataMigration`] instead. Both\nare skipped by baseline replay, but `raw_sql` is skipped because its effect is *unknown*\nwhile `data_migration` is skipped because *changing no schema* is its enforced contract.\nUsing `raw_sql` for DDL silently drops that schema change from the reconstructed baseline,\nafter which `vespertide diff` reports the same already-applied changes forever.", "type": "object", "properties": { "sql": { @@ -843,6 +875,31 @@ "type", "sql" ] + }, + { + "description": "Execute a **data-only** statement verbatim (`UPDATE` / `INSERT` / `DELETE` / …).\n\nThis is the typed home for backfills that the schema-coupled facilities cannot express:\nconditional updates of *existing* columns, correlated-subquery backfills, and data\nreshaping during a format change. Unlike `AddColumn.fill_with` (which only fires for a\nnewly added NOT NULL column with no default) or `ModifyColumnDefault.backfill` (one\ncolumn, one value, every row), `data_migration` carries an arbitrary DML statement and\nis not tied to any schema change.\n\n**Contract: this action changes no schema.** Baseline replay skips it — not because its\neffect is unknown, as with [`MigrationAction::RawSql`], but because \"no schema change\"\nis guaranteed. The guarantee is enforced: a statement whose first token is `CREATE`,\n`ALTER`, `DROP`, or `TRUNCATE` is rejected at load and plan time.\n\nThe SQL is emitted byte-for-byte as written — no case folding, cast rewriting, or\nreformatting. Set `sql` to a single string for portable SQL, or to an object keyed by\n`postgres` / `mysql` / `sqlite` when the statement cannot be portable. A `description`\nis optional but strongly encouraged: it is what `vespertide diff` shows for this action.", + "type": "object", + "properties": { + "description": { + "description": "Why this data change exists, shown in `vespertide diff` output.", + "type": [ + "string", + "null" + ] + }, + "sql": { + "description": "The statement(s) to run, emitted verbatim. See [`DataMigrationSql`].", + "$ref": "#/$defs/DataMigrationSql" + }, + "type": { + "type": "string", + "const": "data_migration" + } + }, + "required": [ + "type", + "sql" + ] } ] },