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

Adding barrel feature #1573

Merged
merged 69 commits into from
Apr 17, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
7f841e5
Optionally adding `barrel` crate to diesel_migrations
spacekookie Feb 15, 2018
5739101
Initial support for barrel migrations in diesel cli
spacekookie Feb 15, 2018
f258e56
Using the barrel `diesel` feature
spacekookie Feb 15, 2018
439620e
Changing barrel call signature to build Migration on diesel side
spacekookie Feb 18, 2018
4cd7b79
Connecting barrel to diesel migration layouts
spacekookie Feb 20, 2018
25267f9
Formatting with rustfmt
spacekookie Feb 20, 2018
9b95920
Changing generated migration function signatures
spacekookie Feb 21, 2018
b4bacf4
Cleaning up migration process slightly
spacekookie Feb 22, 2018
67fd97e
Pointing barrel to github version
spacekookie Feb 22, 2018
4f64f21
Implementing requested changes in diesel_cli
spacekookie Feb 22, 2018
e46570d
Fix git repository link
spacekookie Feb 22, 2018
7f4b65b
Preliminarily commiting refactoring changes
spacekookie Feb 22, 2018
487404d
Moving Migration trait to core diesel library
spacekookie Feb 27, 2018
a4cb6dd
Adjusting the module comment formatting to let CI pass
spacekookie Feb 27, 2018
f0b153f
Moving MigrationName back to migrations_internals
spacekookie Feb 27, 2018
1f74f90
Adding new import to fix some Migration tests
spacekookie Mar 2, 2018
c4cee3d
Changing the migration generation function signature
spacekookie Mar 2, 2018
c62e6ea
Another clippy change
spacekookie Mar 2, 2018
1aa7232
Renaming external feature flag. Changing barrel version to non-git
spacekookie Mar 4, 2018
5f4b402
Changing barrel version to non-git in diesel_cli
spacekookie Mar 4, 2018
7da5525
Adjust the diesel feature flag on barrel
spacekookie Mar 6, 2018
1887333
Re-exporting diesel::migration::* to avoid a breaking change
spacekookie Mar 6, 2018
bac30c0
Cleaning up diesel_migration exports
spacekookie Mar 6, 2018
1aaaf5c
Adjusting a few function calls into barrel to the v0.2.0 API
spacekookie Mar 6, 2018
a3275f9
Re-adding more re-exports to diesel_migrations
spacekookie Mar 6, 2018
6f374e3
Changing feature and type flag names in diesel_cli
spacekookie Mar 23, 2018
e0e97d6
Making error enums non exhaustive. Only exporting speific error types
spacekookie Mar 24, 2018
ed2b8a1
Breaking formatting
spacekookie Mar 24, 2018
b650a55
Fixing formatting again
spacekookie Mar 24, 2018
bc1607c
Touching code to make github see changes
spacekookie Mar 24, 2018
a27d1d4
Changing formatting back to the way it's meant to be
spacekookie Mar 24, 2018
4fdfb3d
Handling __NonExaustive options for migration errors
spacekookie Mar 24, 2018
8c025b6
Not exposing the entire mirations/error module anymore
spacekookie Mar 24, 2018
feed83f
Fixing some formatting issues
spacekookie Mar 25, 2018
df26c2b
Optionally adding `barrel` crate to diesel_migrations
spacekookie Feb 15, 2018
4e2668b
Initial support for barrel migrations in diesel cli
spacekookie Feb 15, 2018
3c992c4
Changing barrel call signature to build Migration on diesel side
spacekookie Feb 18, 2018
8468bc2
Connecting barrel to diesel migration layouts
spacekookie Feb 20, 2018
9a9b01b
Formatting with rustfmt
spacekookie Feb 20, 2018
56234c8
Changing generated migration function signatures
spacekookie Feb 21, 2018
daa998d
Cleaning up migration process slightly
spacekookie Feb 22, 2018
a14287b
Pointing barrel to github version
spacekookie Feb 22, 2018
3101a99
Implementing requested changes in diesel_cli
spacekookie Feb 22, 2018
d5330d2
Preliminarily commiting refactoring changes
spacekookie Feb 22, 2018
a7cd615
Moving Migration trait to core diesel library
spacekookie Feb 27, 2018
9dd24cf
Adjusting the module comment formatting to let CI pass
spacekookie Feb 27, 2018
8960d05
Moving MigrationName back to migrations_internals
spacekookie Feb 27, 2018
cfc453d
Adding new import to fix some Migration tests
spacekookie Mar 2, 2018
2fc3637
Changing the migration generation function signature
spacekookie Mar 2, 2018
6b902d4
Another clippy change
spacekookie Mar 2, 2018
051661f
Renaming external feature flag. Changing barrel version to non-git
spacekookie Mar 4, 2018
b4a2b3b
Changing barrel version to non-git in diesel_cli
spacekookie Mar 4, 2018
5029b64
Adjust the diesel feature flag on barrel
spacekookie Mar 6, 2018
4eebacf
Re-exporting diesel::migration::* to avoid a breaking change
spacekookie Mar 6, 2018
03e0248
Cleaning up diesel_migration exports
spacekookie Mar 6, 2018
b7bd404
Adjusting a few function calls into barrel to the v0.2.0 API
spacekookie Mar 6, 2018
3bb1233
Re-adding more re-exports to diesel_migrations
spacekookie Mar 6, 2018
a5d375d
Changing feature and type flag names in diesel_cli
spacekookie Mar 23, 2018
e8bd6f5
Making error enums non exhaustive. Only exporting speific error types
spacekookie Mar 24, 2018
6693e87
Breaking formatting
spacekookie Mar 24, 2018
13e8caa
Fixing formatting again
spacekookie Mar 24, 2018
0dc23b8
Touching code to make github see changes
spacekookie Mar 24, 2018
8595def
Changing formatting back to the way it's meant to be
spacekookie Mar 24, 2018
f84f394
Handling __NonExaustive options for migration errors
spacekookie Mar 24, 2018
c1a217f
Not exposing the entire mirations/error module anymore
spacekookie Mar 24, 2018
5168672
Fixing some formatting issues
spacekookie Mar 25, 2018
2086b99
Cleaning a bad merge artefact
Apr 16, 2018
ed20737
Resolving rebase differences
Apr 16, 2018
ef98ddc
Fixing a bad rebase
Apr 16, 2018
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
1 change: 1 addition & 0 deletions diesel/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ pub mod serialize;
pub mod sql_types;
pub mod types;
pub mod row;
pub mod migration;

#[cfg(feature = "mysql")]
pub mod mysql;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,27 @@
use diesel::result;
//! Error types that represent migration errors.
//!
//! These are split into multiple segments, depending on
//! where in the migration process an error occurs.

use std::convert::From;
use std::{fmt, io};
use std::path::PathBuf;
use std::error::Error;

use result;

/// Errors that occur while preparing to run migrations
#[derive(Debug)]
pub enum MigrationError {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make this non-exhaustive now that it's public, right?

/// The migration directory wasn't found
MigrationDirectoryNotFound,
/// Provided migration was in an unknown format
UnknownMigrationFormat(PathBuf),
/// General system IO error
IoError(io::Error),
/// Provided migration had an incompatible version number
UnknownMigrationVersion(String),
/// No migrations had to be/ could be run
NoMigrationRun,
}

Expand Down Expand Up @@ -63,11 +74,15 @@ impl From<io::Error> for MigrationError {
}
}

/// Errors that occur while running migrations
#[derive(Debug, PartialEq)]
#[cfg_attr(feature = "clippy", allow(enum_variant_names))]
pub enum RunMigrationsError {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make this non-exhaustive now that it's public, right?

/// A general migration error occured
MigrationError(MigrationError),
/// The provided migration included an invalid query
QueryError(result::Error),
/// The provided migration was empty
EmptyMigration,
}

Expand Down
86 changes: 86 additions & 0 deletions diesel/src/migration/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
//!

pub mod errors;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public or is pub(crate) enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be public, it's re-exported from diesel_migrations

pub use self::errors::*;
Copy link
Member

Choose a reason for hiding this comment

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

This looks super dangerous adds a hard to see amount of items to our public API!

I does in fact only expose MigrationError and RunMigrationsError. So let's pub use those specifically if needed.


use connection::SimpleConnection;
use std::path::Path;
use std::fmt;

/// Represents a migration that interacts with diesel
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that's it's super hard to see what's up with SimpleConnection – it's not public and the only way aside from reading the code where you can find it is in the docs for Connection (which it's a supertrait of). Not something for this PR to fix but seemed worth commenting on.

pub trait Migration {
/// Get the migration version
fn version(&self) -> &str;
/// Apply this migration
fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError>;
/// Revert this migration
fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError>;
/// Get the migration file path
fn file_path(&self) -> Option<&Path> {
None
}
}

/// A migration name
Copy link
Member

Choose a reason for hiding this comment

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

Can we document what this actually does rather than just repeating the name of the type?

#[allow(missing_debug_implementations)]
#[derive(Clone, Copy)]
pub struct MigrationName<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to move?

/// Wraps around a migration
Copy link
Member

Choose a reason for hiding this comment

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

Can we provide more information than just repeating the type?

pub migration: &'a Migration,
}

/// Get the name of a migration
Copy link
Member

Choose a reason for hiding this comment

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

Can we provide more information than just repeating the function/argument names? Maybe at least a usage example?

pub fn name(migration: &Migration) -> MigrationName {
MigrationName {
migration: migration,
}
}

impl<'a> fmt::Display for MigrationName<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let file_name = self.migration
.file_path()
.and_then(|file_path| file_path.file_name())
.and_then(|file| file.to_str());
if let Some(name) = file_name {
f.write_str(name)?;
} else {
f.write_str(self.migration.version())?;
}
Ok(())
}
}

impl Migration for Box<Migration> {
fn version(&self) -> &str {
(&**self).version()
}

fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).run(conn)
}

fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).revert(conn)
}
fn file_path(&self) -> Option<&Path> {
(&**self).file_path()
}
}

impl<'a> Migration for &'a Migration {
fn version(&self) -> &str {
(&**self).version()
}

fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).run(conn)
}

fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).revert(conn)
}
fn file_path(&self) -> Option<&Path> {
(&**self).file_path()
}
}
2 changes: 2 additions & 0 deletions diesel_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ infer_schema_internals = { version = "1.1.0" }
clippy = { optional = true, version = "=0.0.185" }
migrations_internals = { version = "1.1.0" }
url = { version = "1.4.0", optional = true }
barrel = { git = "https://github.com/spacekookie/barrel", optional = true, features = ["diesel"] }
Copy link
Member

Choose a reason for hiding this comment

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

Are you planning on releasing before this is merged? I'm not comfortable putting a git dependency here.


[dev-dependencies]
difference = "1.0"
Expand All @@ -36,6 +37,7 @@ lint = ["clippy"]
postgres = ["diesel/postgres", "infer_schema_internals/postgres", "url"]
sqlite = ["diesel/sqlite", "infer_schema_internals/sqlite"]
mysql = ["diesel/mysql", "infer_schema_internals/mysql", "url"]
barrels = ["migrations_internals/barrel", "barrel"]
Copy link
Member

Choose a reason for hiding this comment

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

Why barrels?


[[test]]
name = "tests"
Expand Down
9 changes: 9 additions & 0 deletions diesel_cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ pub fn build_cli() -> App<'static, 'static> {
for most use cases.",
)
.takes_value(true),
)
.arg(
Arg::with_name("TYPE")
.long("type")
.short("t")
.possible_values(&["sql", "barrel"])
Copy link
Member

Choose a reason for hiding this comment

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

I'd either go with "raw" + "barrel", or "sql" + "rust".

.default_value("sql")
.takes_value(true)
.help("Specify the migration type."),
Copy link
Member

Choose a reason for hiding this comment

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

Can we add .possible_values(&["sql", "barrel"]) and .default_value("sql")?

),
)
.setting(AppSettings::SubcommandRequiredElseHelp);
Expand Down
54 changes: 33 additions & 21 deletions diesel_cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ extern crate migrations_internals;
#[cfg(feature = "url")]
extern crate url;

#[cfg(feature = "barrel")]
extern crate barrel;

mod database_error;
#[macro_use]
mod database;
Expand Down Expand Up @@ -105,38 +108,47 @@ fn run_migration_command(matches: &ArgMatches) {
println!("{:?}", result.unwrap());
}
("generate", Some(args)) => {
use std::io::Write;

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(matches).join(versioned_name);
let migration_type: Option<&str> = args.value_of("TYPE");
fs::create_dir(&migration_dir).unwrap();

let migration_dir_relative =
convert_absolute_path_to_relative(&migration_dir, &env::current_dir().unwrap());

let up_path = migration_dir.join("up.sql");
println!(
"Creating {}",
migration_dir_relative.join("up.sql").display()
);
let mut up = fs::File::create(up_path).unwrap();
up.write_all(b"-- Your SQL goes here").unwrap();

let down_path = migration_dir.join("down.sql");
println!(
"Creating {}",
migration_dir_relative.join("down.sql").display()
);
let mut down = fs::File::create(down_path).unwrap();
down.write_all(b"-- This file should undo anything in `up.sql`")
.unwrap();
match migration_type {
#[cfg(feature = "barrel")]
Some("barrel") => barrel::diesel::generate_initial(migration_dir),
_ => generate_sql_migration(migration_dir),
}
}
_ => unreachable!("The cli parser should prevent reaching here"),
}
}

fn generate_sql_migration(path: PathBuf) {
use std::io::Write;

let migration_dir_relative =
convert_absolute_path_to_relative(&path, &env::current_dir().unwrap());

let up_path = path.join("up.sql");
println!(
"Creating {}",
migration_dir_relative.join("up.sql").display()
);
let mut up = fs::File::create(up_path).unwrap();
up.write_all(b"-- Your SQL goes here").unwrap();

let down_path = path.join("down.sql");
println!(
"Creating {}",
migration_dir_relative.join("down.sql").display()
);
let mut down = fs::File::create(down_path).unwrap();
down.write_all(b"-- This file should undo anything in `up.sql`")
.unwrap();
}

use std::fmt::Display;
fn migration_version<'a>(matches: &'a ArgMatches) -> Box<Display + 'a> {
matches
Expand Down
1 change: 1 addition & 0 deletions diesel_migrations/migrations_internals/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ homepage = "http://diesel.rs"
[dependencies]
clippy = { optional = true, version = "=0.0.185" }
diesel = { version = "1.1.0", default-features = false }
barrel = { git = "https://github.com/spacekookie/barrel", optional = true, features = ["diesel_filled"] }

[dev-dependencies]
tempdir = "0.3.4"
Expand Down
9 changes: 6 additions & 3 deletions diesel_migrations/migrations_internals/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,22 +77,25 @@
#[macro_use]
extern crate diesel;

mod migration;
#[cfg(feature = "barrel")]
extern crate barrel;

pub mod migration;
#[doc(hidden)]
pub mod connection;
mod migration_error;
#[doc(hidden)]
pub mod schema;

#[doc(inline)]
pub use self::connection::MigrationConnection;
#[doc(inline)]
pub use self::migration::*;
pub use self::migration_error::*;
// pub use self::migration_error::*;
Copy link
Member

Choose a reason for hiding this comment

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

?


use std::fs::DirEntry;
use std::io::{stdout, Write};

use diesel::migration::*;
use diesel::expression_methods::*;
use diesel::{Connection, QueryDsl, QueryResult, RunQueryDsl};
use self::schema::__diesel_schema_migrations::dsl::*;
Expand Down
77 changes: 6 additions & 71 deletions diesel_migrations/migrations_internals/src/migration.rs
Original file line number Diff line number Diff line change
@@ -1,46 +1,15 @@
use diesel::connection::SimpleConnection;
use super::{MigrationError, RunMigrationsError};
use diesel::migration::*;

use std::path::{Path, PathBuf};
use std::fmt;

pub trait Migration {
fn version(&self) -> &str;
fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError>;
fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError>;
fn file_path(&self) -> Option<&Path> {
None
}
}

#[allow(missing_debug_implementations)]
#[derive(Clone, Copy)]
pub struct MigrationName<'a> {
pub migration: &'a Migration,
}

pub fn name(migration: &Migration) -> MigrationName {
MigrationName {
migration: migration,
}
}

impl<'a> fmt::Display for MigrationName<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let file_name = self.migration
.file_path()
.and_then(|file_path| file_path.file_name())
.and_then(|file| file.to_str());
if let Some(name) = file_name {
f.write_str(name)?;
} else {
f.write_str(self.migration.version())?;
}
Ok(())
pub fn migration_from(path: PathBuf) -> Result<Box<Migration>, MigrationError> {
#[cfg(feature = "barrel")]
match ::barrel::diesel::migration_from(&path) {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about changing this to if let Some(migration) = ::barrel::diesel::migration_from(path)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be more elegant although rustc tells me attributes are not yet allowed on 'if' expressions. The stabilisation is being tracked here: rust-lang/rust#15701.

I'd suggest we change the syntax when the feature becomes stable, as it is more elegant.

Some(migration) => return Ok(migration),
None => {}
}
}

pub fn migration_from(path: PathBuf) -> Result<Box<Migration>, MigrationError> {
if valid_sql_migration_directory(&path) {
let version = try!(version_from_path(&path));
Ok(Box::new(SqlFileMigration(path, version)))
Expand All @@ -49,40 +18,6 @@ pub fn migration_from(path: PathBuf) -> Result<Box<Migration>, MigrationError> {
}
}

impl Migration for Box<Migration> {
fn version(&self) -> &str {
(&**self).version()
}

fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).run(conn)
}

fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).revert(conn)
}
fn file_path(&self) -> Option<&Path> {
(&**self).file_path()
}
}

impl<'a> Migration for &'a Migration {
fn version(&self) -> &str {
(&**self).version()
}

fn run(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).run(conn)
}

fn revert(&self, conn: &SimpleConnection) -> Result<(), RunMigrationsError> {
(&**self).revert(conn)
}
fn file_path(&self) -> Option<&Path> {
(&**self).file_path()
}
}

fn valid_sql_migration_directory(path: &Path) -> bool {
file_names(path)
.map(|files| files.contains(&"down.sql".into()) && files.contains(&"up.sql".into()))
Expand Down
Loading