Skip to content

Commit

Permalink
Return user-readable drift summary in migrate dev
Browse files Browse the repository at this point in the history
  • Loading branch information
tomhoule committed Jun 9, 2021
1 parent 17ed782 commit d2459f9
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 37 deletions.
2 changes: 1 addition & 1 deletion libs/datamodel/core/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub(crate) use renderer::Renderer;
#[derive(Debug, Clone, PartialEq)]
pub struct SchemaAst {
/// All models, enums, datasources, generators or type aliases
pub tops: Vec<Top>,
pub(crate) tops: Vec<Top>,
}

impl SchemaAst {
Expand Down
3 changes: 3 additions & 0 deletions migration-engine/connectors/migration-connector/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ pub trait DatabaseMigrationMarker: Debug + Send + Sync {

/// Is the migration empty?
fn is_empty(&self) -> bool;

/// Render a human-readable summary of the migrations content, for drift diagnostics.
fn summary(&self) -> String;
}

/// Shorthand for a [Result](https://doc.rust-lang.org/std/result/enum.Result.html) where the error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,22 @@ impl DatabaseMigrationMarker for MongoDbMigration {
fn is_empty(&self) -> bool {
self.steps.is_empty()
}

fn summary(&self) -> String {
let mut out = String::with_capacity(self.steps.len() * 10);

for step in &self.steps {
match step {
MongoDbMigrationStep::CreateCollection(collection) => {
out.push_str("- Added collection `");
out.push_str(collection);
out.push_str("`\n");
}
}
}

out
}
}

#[derive(Debug)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,11 @@ impl SqlMigration {
DriftType::RemovedTable => out.push_str("\n[-] Removed tables\n"),
DriftType::RemovedUdt => out.push_str("\n[-] Removed UDTs\n"),
DriftType::RemovedView => out.push_str("\n[-] Removed views\n"),
DriftType::RedefinedTable => out.push_str("\n[*] Redefined tables\n"),
DriftType::RedefinedTable => {
out.push_str("\n[*] Redefined table `");
out.push_str(item_name);
out.push_str("`\n")
}
DriftType::ChangedEnum => {
out.push_str("\n[*] Changed the `");
out.push_str(item_name);
Expand Down Expand Up @@ -373,6 +377,10 @@ impl DatabaseMigrationMarker for SqlMigration {
fn is_empty(&self) -> bool {
self.steps.is_empty()
}

fn summary(&self) -> String {
self.drift_summary()
}
}

// The order of the variants matters for sorting. The steps are sorted _first_
Expand Down
9 changes: 4 additions & 5 deletions migration-engine/core/src/commands/dev_diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,10 @@ fn check_for_reset_conditions(output: &DiagnoseMigrationHistoryOutput) -> Option
))
}

if let Some(DriftDiagnostic::DriftDetected { rollback }) = &output.drift {
tracing::info!(rollback = rollback.as_str(), "DriftDetected diagnostic");

reset_reasons
.push("Drift detected: Your database schema is not in sync with your migration history.".to_owned())
if let Some(DriftDiagnostic::DriftDetected { summary }) = &output.drift {
let mut reason = format!("Drift detected: Your database schema is not in sync with your migration history.\n");
reason.push_str(summary);
reset_reasons.push(reason);
}

match &output.history {
Expand Down
12 changes: 5 additions & 7 deletions migration-engine/core/src/commands/diagnose_migration_history.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,12 @@ impl<'a> MigrationCommand for DiagnoseMigrationHistoryCommand {
let (drift, error_in_unapplied_migration) = {
if input.opt_in_to_shadow_database {
let drift = match connector
.diff(DiffTarget::Database, DiffTarget::Migrations(&applied_migrations))
.diff(DiffTarget::Migrations(&applied_migrations), DiffTarget::Database)
.await
.map(|mig| if mig.is_empty() { None } else { Some(mig) })
{
Ok(Some(rollback)) => Some(DriftDiagnostic::DriftDetected {
rollback: connector
.database_migration_step_applier()
.render_script(&rollback, &connector.destructive_change_checker().pure_check(&rollback)),
summary: rollback.summary(),
}),
Err(error) => Some(DriftDiagnostic::MigrationFailedToApply { error }),
_ => None,
Expand Down Expand Up @@ -290,8 +288,8 @@ pub enum DriftDiagnostic {
/// The database schema of the current database does not match what would be
/// expected at its stage in the migration history.
DriftDetected {
/// A database script to correct the drift by reverting to the expected schema.
rollback: String,
/// The human-readable contents of the drift.
summary: String,
},
/// When a migration fails to apply cleanly to a shadow database.
MigrationFailedToApply {
Expand All @@ -304,7 +302,7 @@ impl DriftDiagnostic {
/// For tests.
pub fn unwrap_drift_detected(self) -> String {
match self {
DriftDiagnostic::DriftDetected { rollback } => rollback,
DriftDiagnostic::DriftDetected { summary } => summary,
other => panic!("unwrap_drift_detected on {:?}", other),
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub use expect_test::expect;
pub use test_macros::test_connector;
pub use test_setup::{BitFlags, Capabilities, Tags};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,8 @@ fn dev_diagnostic_detects_drift(api: TestApi) {

let DevDiagnosticOutput { action } = api.dev_diagnostic(&directory).send().into_output();

assert_eq!(
action.as_reset(),
Some("Drift detected: Your database schema is not in sync with your migration history.")
);
let expected_start = "Drift detected: Your database schema is not in sync with your migration history.";
assert!(action.as_reset().unwrap().starts_with(expected_start));
}

#[test_connector(exclude(Postgres, Mssql))]
Expand Down Expand Up @@ -200,7 +198,12 @@ fn dev_diagnostic_can_detect_when_the_migrations_directory_is_behind(api: TestAp

let DevDiagnosticOutput { action } = api.dev_diagnostic(&directory).send().into_output();

assert_eq!(action.as_reset(), Some(format!("- Drift detected: Your database schema is not in sync with your migration history.\n- The following migration(s) are applied to the database but missing from the local migrations directory: {}\n", name)).as_deref());
let message = action.as_reset().unwrap();
assert!(message.contains("- Drift detected: Your database schema is not in sync with your migration history"));
assert!(message.contains(&format!(
"The following migration(s) are applied to the database but missing from the local migrations directory: {}",
name
)));
}

#[test_connector]
Expand Down Expand Up @@ -258,13 +261,10 @@ fn dev_diagnostic_can_detect_when_history_diverges(api: TestApi) {

let DevDiagnosticOutput { action } = api.dev_diagnostic(&directory).send().into_output();

let expected_message = format!(
"- Drift detected: Your database schema is not in sync with your migration history.\n- The migrations recorded in the database diverge from the local migrations directory. Last common migration: `{}`. Migrations applied to the database but absent from the migrations directory are: {}\n",
first_migration_name,
deleted_migration_name,
);
let message = action.as_reset().unwrap();

assert_eq!(action.as_reset(), Some(expected_message.as_str()));
assert!(message.contains("Drift detected: Your database schema is not in sync with your migration history"));
assert!(message.contains(&format!("- The migrations recorded in the database diverge from the local migrations directory. Last common migration: `{}`. Migrations applied to the database but absent from the migrations directory are: {}", first_migration_name, deleted_migration_name)));
}

#[test_connector]
Expand Down Expand Up @@ -462,7 +462,7 @@ fn with_an_invalid_unapplied_migration_should_report_it(api: TestApi) {
}

#[test_connector(tags(Postgres))]
fn drift_can_be_detected_without_migrations_table(api: TestApi) {
fn drift_can_be_detected_without_migrations_table_dev(api: TestApi) {
let directory = api.create_migrations_directory();

api.raw_cmd("CREATE TABLE \"cat\" (\nid SERIAL PRIMARY KEY\n);");
Expand All @@ -477,10 +477,14 @@ fn drift_can_be_detected_without_migrations_table(api: TestApi) {

let DevDiagnosticOutput { action } = api.dev_diagnostic(&directory).send().into_output();

assert_eq!(
action.as_reset(),
Some("Drift detected: Your database schema is not in sync with your migration history.")
);
let expect = expect![[r#"
Drift detected: Your database schema is not in sync with your migration history.
[+] Added tables
- cat
"#]];

expect.assert_eq(action.as_reset().unwrap());
}

#[test_connector(tags(Mysql8), exclude(Vitess))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,15 @@ fn diagnose_migration_history_with_opt_in_to_shadow_database_calculates_drift(ap
.send_sync()
.into_output();

let expected_rollback_warnings =
format!("/*\n Warnings:\n\n - You are about to drop the column `fluffiness` on the `{}` table. All the data in the column will be lost.\n\n*/", api.normalize_identifier("Cat"));

let rollback = drift.unwrap().unwrap_drift_detected();

assert!(rollback.starts_with(&expected_rollback_warnings), "{}", rollback);
let snapshot = expect_test::expect![[r#"
[*] Changed the `Cat` table
[+] Added column `fluffiness`
"#]];

snapshot.assert_eq(&rollback);

assert!(history.is_none());
assert!(failed_migration_names.is_empty());
Expand Down Expand Up @@ -324,7 +327,7 @@ fn diagnose_migrations_history_can_detect_when_the_folder_is_behind(api: TestApi

assert!(failed_migration_names.is_empty());
assert!(edited_migration_names.is_empty());
assert!(matches!(drift, Some(DriftDiagnostic::DriftDetected { rollback: _ })));
assert!(matches!(drift, Some(DriftDiagnostic::DriftDetected { summary: _ })));
assert_eq!(
history,
Some(HistoryDiagnostic::MigrationsDirectoryIsBehind {
Expand Down Expand Up @@ -407,7 +410,7 @@ fn diagnose_migrations_history_can_detect_when_history_diverges(api: TestApi) {

assert!(failed_migration_names.is_empty());
assert!(edited_migration_names.is_empty());
assert!(matches!(drift, Some(DriftDiagnostic::DriftDetected { rollback: _ })));
assert!(matches!(drift, Some(DriftDiagnostic::DriftDetected { summary: _ })));
assert_eq!(
history,
Some(HistoryDiagnostic::HistoriesDiverge {
Expand Down Expand Up @@ -734,7 +737,7 @@ fn drift_can_be_detected_without_migrations_table(api: TestApi) {
.send_sync()
.into_output();

assert!(matches!(drift, Some(DriftDiagnostic::DriftDetected { rollback: _ })));
assert!(matches!(drift, Some(DriftDiagnostic::DriftDetected { summary: _ })));
assert!(
matches!(history, Some(HistoryDiagnostic::DatabaseIsBehind { unapplied_migration_names: migs }) if migs.len() == 1)
);
Expand Down

0 comments on commit d2459f9

Please sign in to comment.