Diesel: impl std::fmt::Debug for diesel::pg::PgConnection

[Urhengulas]

Feature Description

impl std::fmt::Debug for diesel::pg::PgConnection {
    // ...
}

Possible solution

My idea would be to just store the database_url on PgConnection::establish(database_url) in the PgConnection and use it as the debug message.
But I am open to include additional information.

Happy to implement it myself 😁

[weiznich]

What is your actual usecase for this? Printing out the connection url is likely not a good idea as it can contain potential secret credentials.

[Urhengulas]

Thank you for coming back to me @weiznich

You are right, I didn't think about the credentials getting potentially exposed.

My usecase is mainly convenience (😬). What I'd like to do is:

pub static DB: OnceCell<PgConnection> = OnceCell::new();
// actually a `r2d2::Pool<...>`, but let's ignore that

fn main() {
    let conn = PgConnection::establish("conn_str").unwrap();
    DB.set(conn).unwrap(); // this is not possible
}

In the system I am working on we are using a global database connection in a OnceCell. I am not able to call OnceCell::set(conn).unwrap(), since it wants to print the debug message of PgConnection in the error case. I am very sure that it won't fail, so I am fine with calling unwrap instead of explicitly handling it.

---

The potential gain is just one or two lines less code, but since I though the effort is also low we could still do it. What do you think about dropping the password in the stored string and saving the rest?

E.g.:

"postgres://user:[email protected]:5678/db_name" -> "postgres://[email protected]:5678/db_name

[weiznich]

"Just" dropping the password/username there would mean that you have to parse the url. That's quite a lot of complexity for such a simple feature. Additionally I'm not sure that the password field is the only potential confidential field there. What's for example with ssl based authentication?

[Urhengulas]

I was thinking, that there actually shouldn't be too much of a security problem with giving the whole conn_str as debug message, because only the application establishing the PgConnection will ever see the string.
Thereby it is no secret to it, right?

[pksunkara]

We don't need to implement this because you can use expect instead of unwrap. You can see an example here, https://github.com/pksunkara/reign/blob/master/reign_model/src/plugin.rs#L32-L38

[Urhengulas]

We don't need to implement this because you can use expect instead of unwrap. You can see an example here, https://github.com/pksunkara/reign/blob/master/reign_model/src/plugin.rs#L32-L38

Thank you for your input @pksunkara :D

But as far as I understand both .unwrap() and .expect() require the error to implement std::fmt::Debug (see core/result.rs#910).

And in your example you are still doing:

if DB.set(pool).is_err() {
    panic!("Unable to store the database connection");
}

But I'd like to have it as:

DB.set(pool).unwrap();
// OR
DB.set(pool).expect("Unable to store the database connection");

[weiznich]

@Urhengulas Both require that the Error type implements Debug, not the Ok type. (https://doc.rust-lang.org/std/result/enum.Result.html#method.expect), so either of those solutions already work today.

Closed as I consider the underlying problem to be solved.

[pksunkara]

Alternative, DB.set(pool).map_err(...).unwrap()

[Urhengulas]

@Urhengulas Both require that the Error type implements Debug, not the Ok type. (https://doc.rust-lang.org/std/result/enum.Result.html#method.expect), so either of those solutions already work today.

Closed as I consider the underlying problem to be solved.

@weiznich I wouldn't consider it solved.

You are right that the error type needs to implement Debug, BUT in case of OnceCell::set(PgConnection) the PgConnection is the Error type! (see https://docs.rs/once_cell/1.5.2/once_cell/sync/struct.OnceCell.html#method.set).

---

Alternative, DB.set(pool).map_err(...).unwrap()

@pksunkara Thanks for the workaround.

[weiznich]

Ok then let me word it that way: I would be happy to merge a PR implementing Debug for all connection types as long as it does not reveal potentially confidential information. I consider database url's to be potential confidential here.

[Urhengulas]

Ok then let me word it that way: I would be happy to merge a PR implementing Debug for all connection types as long as it does not reveal potentially confidential information. I consider database url's to be potential confidential here.

That sounds good 😄

Let me think what other valuable information one could put in there. Most straight forward would be a static "Connection to [postgres, mysql, sqlite] database", but maybe we can include a bit more.

[Mingun]

@Urhengulas maybe you can use a wrapper on the connection type that implements Debug + Error?

[Urhengulas]

@Urhengulas maybe you can use a wrapper on the connection type that implements Debug + Error?

@Mingun That would work. Thank you for the input.

Nevertheless I am looking for a more general upstream solution which simplifies the code of multiple people, because I think the OnceCell<Pool<ConnectionManager<PgConnection>>> is a common pattern.

[pksunkara]

You would need to change r2d2 too and forward the debug IIUC

[Urhengulas]

You would need to change r2d2 too and forward the debug IIUC

Pool and ConnectionManager already implement Debug, but require the enclosed type to also implement it.

[Urhengulas]

fyi: with #2572 the debug message of Pool<ConnectionManager<PgConnection>>> now looks like:

Pool {
    state: State {
        connections: 10,
        idle_connections: 10,
    },
    config: Config {
        max_size: 10,
        min_idle: None,
        test_on_check_out: true,
        max_lifetime: Some(
            1800s,
        ),
        idle_timeout: Some(
            600s,
        ),
        connection_timeout: 30s,
        error_handler: LoggingErrorHandler,
        event_handler: NopEventHandler,
        connection_customizer: NopConnectionCustomizer,
    },
    manager: ConnectionManager {
        database_url: "postgres://postgres:password@localhost",
        _marker: PhantomData,
    },
}

The debug message of the connection actually gets omitted and just says PhantomData, but is required to be implemented nevertheless.

[pksunkara]

I would personally still not recommend to do expect or unwrap because ConnectionManager has the database url with all the secrets.

[pksunkara]

And If we can update ConnectionManager in r2d2 and implement a Debug for it manually, then we don't need Debug for these connections here in diesel.

[Urhengulas]

I would personally still not recommend to do expect or unwrap because ConnectionManager has the database url with all the secrets.

@pksunkara I see that one should keep the database_url secret, but the application having the ConnectionManager did establish the connection and thereby already knows about the database_url.

So I don't see the issue.

[pksunkara]

It's not about the application knowing the database_url but about the secrets being logged because of unwrap or expect

[Urhengulas]

It's not about the application knowing the database_url but about the secrets being logged because of unwrap or expect

True, Then I would suggest to change the debug message of ConnectioManager in a 2nd PR.

[pksunkara]

Yeah, we do that by implementing a custom Debug for ConnectionManager which means we can skip the Connection debug there which means there is no reason for this issue and related PR to exist.

[Urhengulas]

Yeah, we do that by implementing a custom Debug for ConnectionManager which means we can skip the Connection debug there which means there is no reason for this issue and related PR to exist.

I agree that we should have a custom Debug for ConnectionManager<T> which doesn't need T: Debug. Thereby we enable easier use of static DB: OnceCell: Pool<ConnectionManager<PgConnection>>>.

But static DB: OnceCell<PgConnection> would not work. If this isn't a common pattern we can drop #2572, but I am unsure about that. Need your experience here.. --> Just made a github-wide code search and only could find usage of static DB: Pool<...>. Going to close PR.