DATABASE_URL parsing isn't very robust

[Timmmm]

I haven't quite diagnosed this, but on Windows using PostgreSQL, this works:

diesel setup --database-url=postgres://postgres:myadminpass@localhost/my_db

But if I put DATABASE_URL=postgres://postgres:myadminpass@localhost/my_db in .env and run diesel setup it prints:

>diesel setup
Creating database: localhost:5432
could not translate host name "postgres" to address: Unknown host

That... doesn't seem right.

[killercup]

Is this only the case with diesel_cli? Otherwise you should probably also open an issue at https://github.com/slapresta/rust-dotenv :)

…

Am 21.01.2017 um 17:44 schrieb Tim ***@***.***>: I haven't quite diagnosed this, but on Windows using PostgreSQL, this works: diesel setup ***@***.***/my_db But if I put ***@***.***/my_db in .env and run diesel setup it prints: >diesel setup Creating database: localhost:5432 could not translate host name "postgres" to address: Unknown host That... doesn't seem right. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[Timmmm]

Yeah I'm not sure. I'll build a little test app to try it.

[Timmmm]

Actually scratch that, it does work. The problem was I also had an actual environment variable set and that apparently takes precedence over .env. It was set to postgresql://localhost:5432/. Seems like a bug in the URL parser.

In any case, this was a bit hard to diagnose. A simple way to make it a lot easier would be to print the URL that it is trying to connect, at least for diesel setup, and ideally fix the URL parsing.

Looks like this is the code.

Looks like it splits on / and gets the last component, which is why it failed. Why not do proper URL parsing? Here is some nice code I have lovingly crafted using Servo's url parsing crate.

/// Split a postgres URL into the address part and the database name, and appends "postgres" to the
/// end for some reason (TODO: seems weird; why?)
///
/// # Examples
///
/// ```
/// assert_eq!(split_pg_connection_string("postgres://user:pass@domain:port/database".to_string()),
///            Ok(("database".to_string(), "postgres://user:pass@domain:port/postgres".to_string()));
/// assert_eq!(split_pg_connection_string("postgres://user:pass@domain:port/database".to_string()),
///            Err(?));
/// ```
//#[cfg(feature = "postgres")]
fn split_pg_connection_string(database_url: &str) -> Result<(String, String), String> {

    let mut pg_url = Url::parse(database_url).map_err(|_| "URL parse error".to_string())?;
    
    if pg_url.scheme() != "postgres" && pg_url.scheme() != "postgresql" {
        return Err("Scheme must be postgres or postresql".to_string());
    }

    let database = {
        let path: Vec<_> = match pg_url.path_segments() {
            None => return Err("Scheme cannot have paths (this should never happen)".to_string()),
            Some(x) => x.collect(),
        };

        if path.len() != 1 || path[0].is_empty() {
            return Err("You must specify a single path element, e.g. postgres://.../database".to_string())
        }
        path[0].to_owned()
    };

    pg_url.set_path("postgres");

    Ok((database, pg_url.into_string()))
}

Also it would be nice if diesel setup reported the URL it tries to connect to.