Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix global cli argument handling #1304

Merged
merged 4 commits into from
Nov 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion diesel_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ path = "src/main.rs"

[dependencies]
chrono = "0.4"
clap = "2.20.3"
clap = ">=2.27.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diesel CLI 0.99 is going to break when a new version of clap is released...

diesel = { version = "0.16.0", default-features = false }
dotenv = ">=0.8, <0.11"
diesel_infer_schema = "0.16.0"
Expand Down
30 changes: 20 additions & 10 deletions diesel_cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,32 +54,32 @@ fn main() {

fn run_migration_command(matches: &ArgMatches) {
match matches.subcommand() {
("run", Some(args)) => {
("run", Some(_)) => {
let database_url = database::database_url(matches);
let dir = migrations_dir(args);
let dir = migrations_dir(matches);
call_with_conn!(
database_url,
migrations::run_pending_migrations_in_directory(&dir, &mut stdout())
).unwrap_or_else(handle_error);
}
("revert", Some(args)) => {
("revert", Some(_)) => {
let database_url = database::database_url(matches);
let dir = migrations_dir(args);
let dir = migrations_dir(matches);
call_with_conn!(
database_url,
migrations::revert_latest_migration_in_directory(&dir)
).unwrap_or_else(handle_error);
}
("redo", Some(args)) => {
("redo", Some(_)) => {
let database_url = database::database_url(matches);
let dir = migrations_dir(args);
let dir = migrations_dir(matches);
call_with_conn!(database_url, redo_latest_migration(&dir));
}
("list", Some(args)) => {
("list", Some(_)) => {
use std::ffi::OsStr;

let database_url = database::database_url(matches);
let dir = migrations_dir(args);
let dir = migrations_dir(matches);
let migrations =
call_with_conn!(database_url, migrations::mark_migrations_in_directory(&dir))
.unwrap_or_else(handle_error);
Expand Down Expand Up @@ -118,7 +118,7 @@ fn run_migration_command(matches: &ArgMatches) {
let migration_name = args.value_of("MIGRATION_NAME").unwrap();
let version = migration_version(args);
let versioned_name = format!("{}_{}", version, migration_name);
let migration_dir = migrations_dir(args).join(versioned_name);
let migration_dir = migrations_dir(matches).join(versioned_name);
fs::create_dir(&migration_dir).unwrap();

let migration_dir_relative =
Expand Down Expand Up @@ -153,10 +153,20 @@ fn migration_version<'a>(matches: &'a ArgMatches) -> Box<Display + 'a> {
.unwrap_or_else(|| Box::new(Utc::now().format(TIMESTAMP_FORMAT)))
}

fn migrations_dir(matches: &ArgMatches) -> PathBuf {
fn migrations_dir_from_cli(matches: &ArgMatches) -> Option<PathBuf> {
matches
.value_of("MIGRATION_DIRECTORY")
.map(PathBuf::from)
.or_else(|| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weiznich - I'm still becoming familiar with rust idioms and clap! 😅. From what I understand, Pathbuf::from indicates that you expect the MIGRATION_DIRECTORY value will always convert without fail. Why do we need to chain .or_else and then match on the subcommand (name?, not sure which value of the tuple 1 represents)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, Pathbuf::from indicates that you expect the MIGRATION_DIRECTORY value will always convert without fail.

As far as I understood the stdlib docs/code this conversion is guaranteed to succeed. If the user passes a nonsense path a PathBuf pointing to a non existing (the nonsense path) location is generated and the following operations executed by diesel will end with a error message indication a invalid/non existing path.

Why do we need to chain .or_else and then match on the subcommand (name?, not sure which value of the tuple 1 represents)?

The or_else is needed to workaround some architectural decisions/limitations in clap. A clap app consists of several subcommands (migration, setup, print-schema , … for diesel_cli). Each subcommand could also consists of several subcommands (list, run, … for diesel migration). Arguments are defined in a subcommand and are valid for the defined subcommand and all child subcommands. ArgMatches contains all arguments passed to a certain subcommand. Arguments are stored in that ArgMatches to which they are passed, not in which they are defined. So if someone is calling diesel migration --migration_dir /some/dir list the migration_dir argument is stored in the ArgMatches of migration. It is also possible to call diesel migration list --migration_dir /some/dir where migration_dir is stored in the ArgMatches of list. (There is a global option that allows a argument globally, but that does not solve this problem here). Therefore we need to traverse the results for each subcommand. This is done by recursively calling this method on each subcommand. (See the clap docs for why the we access .1 on the tuple)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!! 😄 Thanks so much! I'll finish review ASAP this week (unless someone else beats me to it).

matches
.subcommand()
.1
.and_then(|s| migrations_dir_from_cli(s))
})
}

fn migrations_dir(matches: &ArgMatches) -> PathBuf {
migrations_dir_from_cli(matches)
.or_else(|| env::var("MIGRATION_DIRECTORY").map(PathBuf::from).ok())
.unwrap_or_else(|| {
migrations::find_migrations_directory().unwrap_or_else(handle_error)
Expand Down
140 changes: 140 additions & 0 deletions diesel_cli/tests/migration_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,143 @@ fn any_pending_migrations_after_running_and_creating() {

assert!(result.stdout().contains("true\n"));
}

#[test]
fn migration_run_runs_pending_migrations_custom_database_url_1() {
let p = project("migration_run_custom_db_url_1")
.folder("migrations")
.build();
let db_url = p.database_url();
let db = database(&db_url);

// Make sure the project is setup
p.command("setup").run();

p.create_migration(
"12345_create_users_table",
"CREATE TABLE users ( id INTEGER )",
"DROP TABLE users",
);

assert!(!db.table_exists("users"));

let result = p.command_without_database_url("migration")
.arg("run")
.arg("--database-url")
.arg(db_url)
.run();

assert!(result.is_success(), "Result was unsuccessful {:?}", result);
assert!(
result.stdout().contains("Running migration 12345"),
"Unexpected stdout {}",
result.stdout()
);
assert!(db.table_exists("users"));
}

#[test]
fn migration_run_runs_pending_migrations_custom_database_url_2() {
let p = project("migration_run_custom_db_url_2")
.folder("migrations")
.build();
let db_url = p.database_url();
let db = database(&db_url);

// Make sure the project is setup
p.command("setup").run();

p.create_migration(
"12345_create_users_table",
"CREATE TABLE users ( id INTEGER )",
"DROP TABLE users",
);

assert!(!db.table_exists("users"));

let result = p.command_without_database_url("migration")
.arg("--database-url")
.arg(db_url)
.arg("run")
.run();

assert!(result.is_success(), "Result was unsuccessful {:?}", result);
assert!(
result.stdout().contains("Running migration 12345"),
"Unexpected stdout {}",
result.stdout()
);
assert!(db.table_exists("users"));
}


#[test]
fn migration_run_runs_pending_migrations_custom_migration_dir_1() {
let p = project("migration_run_custom_migration_dir_1")
.folder("custom_migrations")
.build();
let db_url = p.database_url();
let db = database(&db_url);

// Make sure the project is setup
p.command("setup").run();

p.create_migration_in_directory(
"custom_migrations",
"12345_create_users_table",
"CREATE TABLE users ( id INTEGER )",
"DROP TABLE users",
);

assert!(!db.table_exists("users"));

let result = p.command("migration")
.arg("run")
.arg("--migration-dir")
.arg(p.migration_dir_in_directory("custom_migrations"))
.run();

assert!(result.is_success(), "Result was unsuccessful {:?}", result);
assert!(
result.stdout().contains("Running migration 12345"),
"Unexpected stdout {}",
result.stdout()
);
assert!(db.table_exists("users"));
}


#[test]
fn migration_run_runs_pending_migrations_custom_migration_dir_2() {
let p = project("migration_run_custom_migration_dir_2")
.folder("custom_migrations")
.build();
let db_url = p.database_url();
let db = database(&db_url);

// Make sure the project is setup
p.command("setup").run();

p.create_migration_in_directory(
"custom_migrations",
"12345_create_users_table",
"CREATE TABLE users ( id INTEGER )",
"DROP TABLE users",
);

assert!(!db.table_exists("users"));

let result = p.command("migration")
.arg("--migration-dir")
.arg(p.migration_dir_in_directory("custom_migrations"))
.arg("run")
.run();

assert!(result.is_success(), "Result was unsuccessful {:?}", result);
assert!(
result.stdout().contains("Running migration 12345"),
"Unexpected stdout {}",
result.stdout()
);
assert!(db.table_exists("users"));
}
5 changes: 5 additions & 0 deletions diesel_cli/tests/support/project_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ impl Project {
fs::remove_dir_all(file).unwrap();
}

pub fn migration_dir_in_directory(&self, directory: &str) -> String {
let migration_path = self.directory.path().join(directory);
migration_path.display().to_string()
}

pub fn create_migration(&self, name: &str, up: &str, down: &str) {
self.create_migration_in_directory("migrations", name, up, down);
}
Expand Down