fix(cli): test migrate.ignored-chars, fix bugs

Author: abonander

sqlx-cli/src/migrate.rs

@@ -3,7 +3,7 @@ use crate::opt::{AddMigrationOpts, ConnectOpts, MigrationSourceOpt};
 use anyhow::{bail, Context};
 use console::style;
 use sqlx::migrate::{
-    AppliedMigration, Migrate, MigrateError, MigrationType, Migrator, ResolveWith,
+    AppliedMigration, Migrate, MigrateError, MigrationType, Migrator,
 };
 use sqlx::Connection;
 use std::borrow::Cow;
@@ -16,11 +16,11 @@ use std::time::Duration;
 pub async fn add(opts: AddMigrationOpts) -> anyhow::Result<()> {
     let config = opts.config.load_config().await?;
 
-    let source = opts.source.resolve(config);
+    let source = opts.source.resolve_path(config);
 
     fs::create_dir_all(source).context("Unable to create migrations directory")?;
 
-    let migrator = Migrator::new(Path::new(source)).await?;
+    let migrator = opts.source.resolve(config).await?;
 
     let version_prefix = opts.version_prefix(config, &migrator);
 
@@ -130,13 +130,8 @@ pub async fn info(
     migration_source: &MigrationSourceOpt,
     connect_opts: &ConnectOpts,
 ) -> anyhow::Result<()> {
-    let source = migration_source.resolve(config);
+    let migrator = migration_source.resolve(config).await?;
 
-    let migrator = Migrator::new(ResolveWith(
-        Path::new(source),
-        config.migrate.to_resolve_config(),
-    ))
-    .await?;
     let mut conn = crate::connect(connect_opts).await?;
 
     // FIXME: we shouldn't actually be creating anything here
@@ -228,9 +223,8 @@ pub async fn run(
     ignore_missing: bool,
     target_version: Option<i64>,
 ) -> anyhow::Result<()> {
-    let source = migration_source.resolve(config);
-
-    let migrator = Migrator::new(Path::new(source)).await?;
+    let migrator = migration_source.resolve(config).await?;
+    
     if let Some(target_version) = target_version {
         if !migrator.version_exists(target_version) {
             bail!(MigrateError::VersionNotPresent(target_version));
@@ -331,8 +325,8 @@ pub async fn revert(
     ignore_missing: bool,
     target_version: Option<i64>,
 ) -> anyhow::Result<()> {
-    let source = migration_source.resolve(config);
-    let migrator = Migrator::new(Path::new(source)).await?;
+    let migrator = migration_source.resolve(config).await?;
+
     if let Some(target_version) = target_version {
         if target_version != 0 && !migrator.version_exists(target_version) {
             bail!(MigrateError::VersionNotPresent(target_version));
@@ -432,7 +426,7 @@ pub fn build_script(
     migration_source: &MigrationSourceOpt,
     force: bool,
 ) -> anyhow::Result<()> {
-    let source = migration_source.resolve(config);
+    let source = migration_source.resolve_path(config);
 
     anyhow::ensure!(
         Path::new("Cargo.toml").exists(),

sqlx-cli/src/opt.rs

@@ -8,7 +8,7 @@ use clap::{
 };
 #[cfg(feature = "completions")]
 use clap_complete::Shell;
-use sqlx::migrate::Migrator;
+use sqlx::migrate::{MigrateError, Migrator, ResolveWith};
 use std::env;
 use std::ops::{Deref, Not};
 use std::path::PathBuf;
@@ -342,13 +342,19 @@ pub struct MigrationSourceOpt {
 }
 
 impl MigrationSourceOpt {
-    pub fn resolve<'a>(&'a self, config: &'a Config) -> &'a str {
+    pub fn resolve_path<'a>(&'a self, config: &'a Config) -> &'a str {
         if let Some(source) = &self.source {
             return source;
         }
 
         config.migrate.migrations_dir()
     }
+    
+    pub async fn resolve(&self, config: &Config) -> Result<Migrator, MigrateError> {
+        Migrator::new(
+            ResolveWith(self.resolve_path(config), config.migrate.to_resolve_config())
+        ).await
+    }
 }
 
 /// Argument for the database URL.

sqlx-cli/tests/common/mod.rs

@@ -3,75 +3,89 @@ use assert_cmd::{assert::Assert, Command};
 use sqlx::_unstable::config::Config;
 use sqlx::{migrate::Migrate, Connection, SqliteConnection};
 use std::{
-    env::temp_dir,
-    fs::remove_file,
+    env, fs,
     path::{Path, PathBuf},
 };
 
 pub struct TestDatabase {
     file_path: PathBuf,
-    migrations: String,
+    migrations_path: PathBuf,
+    pub config_path: Option<PathBuf>,
 }
 
 impl TestDatabase {
     pub fn new(name: &str, migrations: &str) -> Self {
-        let migrations_path = Path::new("tests").join(migrations);
-        let file_path = Path::new(&temp_dir()).join(format!("test-{}.db", name));
-        let ret = Self {
+        // Note: only set when _building_
+        let temp_dir = option_env!("CARGO_TARGET_TMPDIR").map_or_else(env::temp_dir, PathBuf::from);
+
+        let test_dir = temp_dir.join("migrate");
+
+        fs::create_dir_all(&test_dir)
+            .unwrap_or_else(|e| panic!("error creating directory: {test_dir:?}: {e}"));
+
+        let file_path = test_dir.join(format!("test-{name}.db"));
+
+        if file_path.exists() {
+            fs::remove_file(&file_path)
+                .unwrap_or_else(|e| panic!("error deleting test database {file_path:?}: {e}"));
+        }
+
+        let this = Self {
             file_path,
-            migrations: String::from(migrations_path.to_str().unwrap()),
+            migrations_path: Path::new("tests").join(migrations),
+            config_path: None,
         };
+
         Command::cargo_bin("cargo-sqlx")
             .unwrap()
             .args([
                 "sqlx",
                 "database",
                 "create",
                 "--database-url",
-                &ret.connection_string(),
+                &this.connection_string(),
             ])
             .assert()
             .success();
-        ret
+        this
+    }
+
+    pub fn set_migrations(&mut self, migrations: &str) {
+        self.migrations_path = Path::new("tests").join(migrations);
     }
 
     pub fn connection_string(&self) -> String {
         format!("sqlite://{}", self.file_path.display())
     }
 
     pub fn run_migration(&self, revert: bool, version: Option<i64>, dry_run: bool) -> Assert {
-        let ver = match version {
-            Some(v) => v.to_string(),
-            None => String::from(""),
-        };
-        Command::cargo_bin("cargo-sqlx")
-            .unwrap()
-            .args(
-                [
-                    vec![
-                        "sqlx",
-                        "migrate",
-                        match revert {
-                            true => "revert",
-                            false => "run",
-                        },
-                        "--database-url",
-                        &self.connection_string(),
-                        "--source",
-                        &self.migrations,
-                    ],
-                    match version {
-                        Some(_) => vec!["--target-version", &ver],
-                        None => vec![],
-                    },
-                    match dry_run {
-                        true => vec!["--dry-run"],
-                        false => vec![],
-                    },
-                ]
-                .concat(),
-            )
-            .assert()
+        let mut command = Command::cargo_bin("sqlx").unwrap();
+        command
+            .args([
+                "migrate",
+                match revert {
+                    true => "revert",
+                    false => "run",
+                },
+                "--database-url",
+                &self.connection_string(),
+                "--source",
+            ])
+            .arg(&self.migrations_path);
+
+        if let Some(config_path) = &self.config_path {
+            command.arg("--config").arg(config_path);
+        }
+
+        if let Some(version) = version {
+            command.arg("--target-version").arg(version.to_string());
+        }
+
+        if dry_run {
+            command.arg("--dry-run");
+        }
+
+        command.assert()
     }
 
     pub async fn applied_migrations(&self) -> Vec<i64> {
@@ -88,10 +102,34 @@ impl TestDatabase {
             .map(|m| m.version)
             .collect()
     }
+
+    pub fn migrate_info(&self) -> Assert {
+        let mut command = Command::cargo_bin("sqlx").unwrap();
+        command
+            .args([
+                "migrate",
+                "info",
+                "--database-url",
+                &self.connection_string(),
+                "--source",
+            ])
+            .arg(&self.migrations_path);
+
+        if let Some(config_path) = &self.config_path {
+            command.arg("--config").arg(config_path);
+        }
+
+        command.assert()
+    }
 }
 
 impl Drop for TestDatabase {
     fn drop(&mut self) {
-        remove_file(&self.file_path).unwrap();
+        // Only remove the database if there isn't a failure.
+        if !std::thread::panicking() {
+            fs::remove_file(&self.file_path).unwrap_or_else(|e| {
+                panic!("error deleting test database {:?}: {e}", self.file_path)
+            });
+        }
     }
 }

sqlx-cli/tests/ignored-chars/BOM/.gitattributes

@@ -0,0 +1 @@
+*.sql text eol=lf

sqlx-cli/tests/ignored-chars/BOM/1_user.sql

@@ -0,0 +1,6 @@
+create table user
+(
+    -- integer primary keys are the most efficient in SQLite
+    user_id  integer primary key,
+    username text unique not null
+);

sqlx-cli/tests/ignored-chars/BOM/2_post.sql

@@ -0,0 +1,10 @@
+create table post
+(
+    post_id    integer primary key,
+    user_id    integer not null references user (user_id),
+    content    text    not null,
+    -- Defaults have to be wrapped in parenthesis
+    created_at datetime default (datetime('now'))
+);
+
+create index post_created_at on post (created_at desc);

sqlx-cli/tests/ignored-chars/BOM/3_comment.sql

@@ -0,0 +1,10 @@
+create table comment
+(
+    comment_id integer primary key,
+    post_id    integer not null references post (post_id),
+    user_id    integer not null references "user" (user_id),
+    content    text    not null,
+    created_at datetime default (datetime('now'))
+);
+
+create index comment_created_at on comment (created_at desc);

sqlx-cli/tests/ignored-chars/CRLF/.gitattributes

@@ -0,0 +1 @@
+*.sql text eol=crlf

sqlx-cli/tests/ignored-chars/CRLF/1_user.sql

@@ -0,0 +1,6 @@
+create table user
+(
+    -- integer primary keys are the most efficient in SQLite
+    user_id  integer primary key,
+    username text unique not null
+);

sqlx-cli/tests/ignored-chars/CRLF/2_post.sql

@@ -0,0 +1,10 @@
+create table post
+(
+    post_id    integer primary key,
+    user_id    integer not null references user (user_id),
+    content    text    not null,
+    -- Defaults have to be wrapped in parenthesis
+    created_at datetime default (datetime('now'))
+);
+
+create index post_created_at on post (created_at desc);

sqlx-cli/tests/ignored-chars/CRLF/3_comment.sql

@@ -0,0 +1,10 @@
+create table comment
+(
+    comment_id integer primary key,
+    post_id    integer not null references post (post_id),
+    user_id    integer not null references "user" (user_id),
+    content    text    not null,
+    created_at datetime default (datetime('now'))
+);
+
+create index comment_created_at on comment (created_at desc);

sqlx-cli/tests/ignored-chars/LF/.gitattributes

@@ -0,0 +1 @@
+*.sql text eol=lf

sqlx-cli/tests/ignored-chars/LF/1_user.sql

@@ -0,0 +1,6 @@
+create table user
+(
+    -- integer primary keys are the most efficient in SQLite
+    user_id  integer primary key,
+    username text unique not null
+);

sqlx-cli/tests/ignored-chars/LF/2_post.sql

@@ -0,0 +1,10 @@
+create table post
+(
+    post_id    integer primary key,
+    user_id    integer not null references user (user_id),
+    content    text    not null,
+    -- Defaults have to be wrapped in parenthesis
+    created_at datetime default (datetime('now'))
+);
+
+create index post_created_at on post (created_at desc);

sqlx-cli/tests/ignored-chars/LF/3_comment.sql

@@ -0,0 +1,10 @@
+create table comment
+(
+    comment_id integer primary key,
+    post_id    integer not null references post (post_id),
+    user_id    integer not null references "user" (user_id),
+    content    text    not null,
+    created_at datetime default (datetime('now'))
+);
+
+create index comment_created_at on comment (created_at desc);

sqlx-cli/tests/ignored-chars/oops-all-tabs/.gitattributes

@@ -0,0 +1 @@
+*.sql text eol=lf

sqlx-cli/tests/ignored-chars/oops-all-tabs/1_user.sql

@@ -0,0 +1,6 @@
+create table user
+(
+	-- integer primary keys are the most efficient in SQLite
+	user_id  integer primary key,
+	username text unique not null
+);

sqlx-cli/tests/ignored-chars/oops-all-tabs/2_post.sql

@@ -0,0 +1,10 @@
+create table post
+(
+	post_id	integer primary key,
+	user_id	integer not null references user (user_id),
+	content	text	not null,
+	-- Defaults have to be wrapped in parenthesis
+	created_at datetime default (datetime('now'))
+);
+
+create index post_created_at on post (created_at desc);

sqlx-cli/tests/ignored-chars/oops-all-tabs/3_comment.sql

@@ -0,0 +1,10 @@
+create table comment
+(
+	comment_id integer primary key,
+	post_id	integer not null references post (post_id),
+	user_id	integer not null references "user" (user_id),
+	content	text	not null,
+	created_at datetime default (datetime('now'))
+);
+
+create index comment_created_at on comment (created_at desc);

sqlx-cli/tests/ignored-chars/sqlx.toml

@@ -0,0 +1,7 @@
+[migrate]
+# Ignore common whitespace characters (beware syntatically significant whitespace!)
+# Space, tab, CR, LF, zero-width non-breaking space (U+FEFF)
+#
+# U+FEFF is added by some editors as a magic number at the beginning of a text file indicating it is UTF-8 encoded,
+# where it is known as a byte-order mark (BOM): https://en.wikipedia.org/wiki/Byte_order_mark
+ignored-chars = [" ", "\t", "\r", "\n", "\uFEFF"]