Easier customization of the connection pool in `rocket_sync_db_pools`

[ralpha]

Description

When testing my code I want to be able to test out my API endpoint, but not keep the changes in the database. I can do this in Diesel by using conn.begin_test_transaction(). But to use this with rocket is fairly complicated right now. Some small changes could make this much easier to do. The code below is based on a bunch of thing, including this: https://github.com/diesel-rs/diesel/issues/2733

How could it work (does not work currently)

impl Poolable for diesel::PgConnection {
    type Manager = diesel::r2d2::ConnectionManager<diesel::PgConnection>;
    type Error = std::convert::Infallible;

    fn pool(db_name: &str, rocket: &Rocket<Build>) -> PoolResult<Self> {
        let config = Config::from(db_name, rocket)?;
        let manager = diesel::r2d2::ConnectionManager::new(config.url);
        Ok(r2d2::Pool::builder()
            .connection_customizer(Box::new(TestConnectionCustomizer))
            .max_size(config.pool_size)
            .connection_timeout(Duration::from_secs(config.timeout as u64))
            .build(manager)?)
    }
}

// With the test connection customizer:
#[derive(Debug, Clone, Copy)]
pub struct TestConnectionCustomizer;

impl<C, E> CustomizeConnection<C, E> for TestConnectionCustomizer
where
    C: diesel::Connection,
{
    fn on_acquire(&self, conn: &mut C) -> Result<(), E> {
        conn.begin_test_transaction()
            .expect("Failed to start test transaction");

        Ok(())
    }
}

But this does not work because of either Poolable or diesel::PgConnection needs to be implemented in the current crate. So to get around this you can always implement your own object and use that. Like this:

pub struct MyConn(diesel::PgConnection);
// And implement the trait for that.
impl Poolable for MyConn {
    type Manager = diesel::r2d2::ConnectionManager<diesel::PgConnection>;
    type Error = std::convert::Infallible;

    fn pool(db_name: &str, rocket: &Rocket<Build>) -> PoolResult<Self> {
        // Same a above code
    }
}

But this does not work because of the following constraint: type Manager: ManageConnection<Connection=Self>; This constraint is reasonable in most cases but does not allow for a different struct in these cases. If this constraint could be removed or be made generic then this would allow for custom poolable connections.

This would make it much easier to implement this. Right now you have to manually implement #[rocket_contrib::database("databasename")] And re-implement functions like run, fairing, get, ... and FromRequest. So this is about 250 lines of code instead of the 28 from above.

It would be a very welcome change. I can create a MR for this if this is something that you would like to see added.

Environment:

• OS Distribution and Kernel: any
• Rocket Version: 0.5.0-rc.1

[Alken0]

If you are using sqlite, you may use the given in-memory database like this:

// function to call in tests to get Rocket instance for Client (using diesel)
pub fn rocket() -> rocket::Rocket<rocket::Build> {
    let rocket = crate::rocket();
    let config = rocket
        .figment()
        .clone()
        .merge((Config::LOG_LEVEL, "off"))
        .merge(("databases.diesel.url", ":memory:"));
    return rocket.configure(config);
}

[jebrosen]

When testing my code I want to be able to test out my API endpoint, but not keep the changes in the database.

This seems pretty unusual - if you don't keep and then verify changes to the database, are you really testing that the routes work?

More often we've seen a need for #1197 - to make changes, observe changes, keep changes, and discard them all after a series of routes have been exercised.

If this constraint could be removed or be made generic then this would allow for custom poolable connections.

In the case of rocket_sync_db_pools we could also consider exposing connection_customizer in some way, which reduces the need to duplicate the code in Poolable::pool().

[ralpha]

When testing my code I want to be able to test out my API endpoint, but not keep the changes in the database.

This seems pretty unusual - if you don't keep and then verify changes to the database, are you really testing that the routes work?

More often we've seen a need for #1197 - to make changes, observe changes, keep changes, and discard them all after a series of routes have been exercised.

Both will work in my case. Because the data that is returned from the database is the data that has been updated/changed/created/... so we know the endpoint works (mostly) correctly. For our usecase this is currently more then enough. #1197 would be better but is much more difficult to implement at this stage. So having something simple/good enough is better then nothing.

If this constraint could be removed or be made generic then this would allow for custom poolable connections.

In the case of rocket_sync_db_pools we could also consider exposing connection_customizer in some way, which reduces the need to duplicate the code in Poolable::pool().

If that solves the problem or reduces the code needed to pull this off it would be step in the right direction.

But in type Manager: ManageConnection<Connection=Self>; is the constraint Connection=Self needed or not? Otherwise this would be the easiest fix I think. With just a few lines I can change the connection_customizer.

If I can just directly change the connection_customizer for the Poolable instance that would give the same result. Just don't know how this code would look at the moment.

[jebrosen]

But in type Manager: ManageConnection<Connection=Self>; is the constraint Connection=Self needed or not? Otherwise this would be the easiest fix I think. With just a few lines I can change the connection_customizer.

The constraint is required within the current design; to meet this requirement you could additionally implement your own type that implements Manager. The main alternative I can think of would be to replace that constraint, and/or add another constraint or function, in terms of the connection type instead -- this was the approach chosen for the newer, async-first, rocket_db_pools crate, but it might or might not work in rocket_sync_db_pools or require additional changes.

[SergioBenitez]

In rocket_db_pools, the suggested functionality can be implemented via an implementation of Poolable that mostly forwards to an existing implementation but customizes as needed. The specific request would look something like:

use rocket::figment::Figment;
use rocket_db_pools::{Pool, Config, Error};
use rocket::futures::future::TryFutureExt;

struct MyPool(PgPool);

#[rocket::async_trait]
impl Pool for MyPool {
    type Connection = <PgPool as Pool>::Connection;
    type Error = <PgPool as Pool>::Error;

    async fn init(figment: &Figment) -> Result<Self, Self::Error> {
        <PgPool as Pool>::init(figment).map_ok(Self).await
    }

    async fn get(&self) -> Result<Self::Connection, Self::Error> {
        let conn = <PgPool as Pool>::get(&self.0).await
        conn.begin_transaction().await?;
        Ok(conn)
    }

    async fn close(&self) {
        <PgPool as Pool>::close(&self.0).await
    }
}

I wouldn't mind a rewrite of rocket_sync_db_pools to look like rocket_db_pools, but I think at this point we should consider rocket_db_pools to be the recommended choice for database use in Rocket. It seems we'll soon-ish get diesel_async as well, which would make exactly what is being proposed possible via the above. Given this, I think we can close out this issue.