Introduce `db_pools` contrib crate, for asynchronous database connection pooling #1696

jebrosen · 2021-06-14T01:47:33Z

Resolves #1117.

This implementation is similar to sync_db_pools and derives most of its documentation from that crate, but there are a few notable differences:

Database types are declared via a #[derive(Database)] in combination with the #[database(name = "")] attribute, which makes it more straightforward to see what methods are available on the decorated type.
Databases are declared in terms of the pool type, not the connection type. This helps avoid requiring adapters to go through a particular pool implementation such as r2d2, and allows us to use built-in connection pooling support that is provided by some database drivers.
The decorated database type is placed directly in managed state by its fairing, so that .pool() and .get() are easily accessible after attaching the fairing and from within routes. This is especially useful for integrations that provide additional methods on the pool type which would otherwise be inaccessible.
Direct access to the underlying connection pool is available (Expose pool directly from generated database pools #1187), via the .pool() method.
All relevant APIs are async, and no additional wrapper (a la run()) is necessary to bridge between async and blocking code.

TODO:

Solidify the configuration API. In sync_db_pools, each implementation of Poolable deserializes its config from a Rocket instance. In this implementation, the configuration is restricted any DeserializeOwned type and the deserialization is done by the database fairing - this makes it easier to correctly implement Pool since it is no longer responsible for looking up its configuration, but it is impossible to query the worker count when choosing a pool size. I would appreciate any ideas on how to best reconcile this.
pool_size - Different database pools accept different combinations of min or max pool size, and currently most Pool implementations use the default.
Configurable timeout for connection retrieval.
Tests.
Documentation additions and corrections, especially the guide. Most of the documentation here has simply been copied from sync_db_pools and adjusted for the different trait names and provided integrations.

SergioBenitez · 2021-06-14T04:08:30Z

I am so stoked to see this! This is huge for 0.5. Will prioritize reviewing this ASAP. Thank you!

SergioBenitez

I've left some comments. The biggest issues I see are:

Config needs to be redone. I've laid out a structure in the comments.
Libraries aren't being used to their fullest extent. Use devise and figment.
Lots of items are fully qualified, increasing the verbosity of code unnecessarily.
The effective need to declare a type alias is unfortunate. Maybe an associated type helps.

contrib/db_pools/codegen/src/database.rs

contrib/db_pools/codegen/src/lib.rs

contrib/db_pools/lib/Cargo.toml

contrib/db_pools/lib/src/config.rs

SergioBenitez · 2021-06-25T13:15:23Z

contrib/db_pools/lib/src/config.rs

+/// [`Pool::initialize()`]: crate::Pool::initialize
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+#[serde(crate = "rocket::serde")]
+pub struct Config {


We shouldn't need this structure at all. Each implementation should define their own config structure.

See my other comments in config.

SergioBenitez · 2021-06-25T13:17:40Z

contrib/db_pools/lib/src/pool.rs

+    /// The error type returned by `initialize`.
+    type InitError: std::error::Error;
+
+    /// The error type returned by `get`.
+    type GetError: std::error::Error;


This feels wrong to me. Why isn't there just one Error?

InitError is rocket_db_pools::Error, which represents either bad configuration or the database-specific error. GetError could be the same error type, but it would always be the non-configuration variant.

For some database drivers (e.g. deadpool-postgres), the error type returned by initialization vs by get() are actually different. In that case one error type is a superset of the other, but I wouldn't want to assume that about future drivers.

Sure, but that doesn't mean the pool integration can't wrap those into one type.

I think there's a lot of effort right now spent on trying to creates these reusable structures like Config and Error (the least common denominators) when, in reality, each database driver (at least those from each library) will be fairly distinct. I would advocate for eschewing both of these types entirely and instead creating an Error and Config for each database variant/library as needed. With Config specifically, this is pretty important.

when, in reality, each database driver (at least those from each library) will be fairly distinct

So, I tried using a separate error type for each implementation and ran into every implementation of Pool::initialize needing to return a figment::Error. I feel again like I'm completely misunderstanding something, because the single Error type with 3 variants now seems like a lot less effort than writing and documenting the same 3 variants of a separate error enum for each database.

contrib/db_pools/lib/src/pool.rs

SergioBenitez · 2021-06-25T13:24:54Z

examples/databases/src/sqlx.rs

-        Err(e) => {
-            error!("Failed to read SQLx config: {}", e);
-            return Err(rocket);
+    match rocket.state::<Db>() {


This feels like an abstraction has just leaked. I think we want something like https://api.rocket.rs/v0.5-rc/rocket/struct.State.html#method.get. Maybe Db::pool(&rocket); and Db::conn(&rocket).await;.

Leaking the abstraction in this particular way (putting the derived database type directly in managed state) was intentional, because I thought it was much simpler to understand. Db::pool and Db::conn already take &self, so this would logically be adding Db::pool_from_rocket(&rocket) and Db::conn_from_rocket(&rocket).await. Happy to make that change, though.

The problem is that this presents two different interfaces: an abstracted Db::fairing() and now a Db in managed state. If the user had instead been asked to do .manage(Db::init()?) or something to that effect, this would be fine. But otherwise, .attach(Db::fairing()) says nothing about something going into managed state.

SergioBenitez

Something feels quite wrong here with the way Database is defined:

It's very weird that there are two error types in Database. There really needs to be one.
We need to specify configs per database. The existence of a common Config type in 0.4 was a mistake rooted in the inability to generalize configuration. With Figment, this mistake doesn't need to be made again. Each database type (or library, or wherever the distinction lies) needs to define its own configuration structure. This immediately resolves all weirdness around configuration and allows the user to configure all aspects of each database.
The Fairing struct is very odd. It effectively demands that the database type is Fn(Self::Pool) -> Self without having that fact be codified or enforced anywhere. Even so, that feels like a strange restriction. It seems we really just want some way to go from a database pool type to a structure. Perhaps the bound we want on Database is From<Self::Pool> (which would imply generating such an implementation), or maybe we want a fn new(pool: Db::Pool) -> Self; on Database. I'm not sure exactly which of these is the right approach, but the current Fairing situation feels like the wrong one.
Something needs to be done about the effective need for a type alias. Whether that's generating an inherent associated type or something similar, I don't know. But it needs to be trivial and concise to get a connection without adding any code.

contrib/db_pools/codegen/src/database.rs

SergioBenitez · 2021-06-26T05:44:46Z

contrib/db_pools/lib/src/database.rs

+    fn pool(&self) -> &Self::Pool;
+
+    /// get().await returns a connection from the pool (or an error)
+    fn get(&self) -> BoxFuture<'_, Result<Connection<Self>, <Self::Pool as Pool>::GetError>> {


I wanted to avoid boxing every time we retrieved a connection. If the pool type implemented an async fn get() method inherently, and codegen could call that method directly, then we would avoid the box. There's no reason to Box manually without using async_trait. This thinking relied on the Db type implementing FromRequest or coherence allowing you to generate a FromRequest for Connection<Db>.

contrib/db_pools/codegen/Cargo.toml

SergioBenitez · 2021-06-27T01:00:54Z

contrib/db_pools/lib/src/database.rs

+    /// Returns a fairing that attaches this connection pool to the server.
+    fn fairing() -> Fairing<Self>;


This Fairing type feels pretty weird to me. It seems to me that a Database is itself a Fairing. Or something like that. There's something not quite right about the setup right now with respect to this fairing, the Fairing type, and the Connection type.

SergioBenitez · 2021-06-27T01:01:14Z

contrib/db_pools/lib/src/database.rs

+    /// Create a new database fairing with the given constructor.  This
+    /// constructor will be called to create an instance of `D` after the pool
+    /// is initialized and before it is placed into managed state.
+    pub fn new(fairing_name: &'static str, ctor: fn(D::Pool) -> D, ) -> Self {


This is really weird.

SergioBenitez · 2021-06-27T01:02:05Z

contrib/db_pools/lib/src/database.rs

+            }
+        };
+
+        Ok(rocket.manage((self.1)(pool)))


This will panic if one is already being managed. We should probably check and emit a nicer error.

SergioBenitez · 2021-06-27T01:04:19Z

contrib/db_pools/lib/src/pool.rs

+    /// The error type returned by `initialize`.
+    type InitError: std::error::Error;
+
+    /// The error type returned by `get`.
+    type GetError: std::error::Error;


Sure, but that doesn't mean the pool integration can't wrap those into one type.

I think there's a lot of effort right now spent on trying to creates these reusable structures like Config and Error (the least common denominators) when, in reality, each database driver (at least those from each library) will be fairly distinct. I would advocate for eschewing both of these types entirely and instead creating an Error and Config for each database variant/library as needed. With Config specifically, this is pretty important.

SergioBenitez · 2021-06-27T01:05:18Z

contrib/db_pools/lib/src/pool.rs

+    async fn initialize(db_name: &str, rocket: &Rocket<Build>)
+        -> Result<Self, Error<Self::InitError>>;


This shouldn't be formatted this way. At worst:

Suggested change

async fn initialize(db_name: &str, rocket: &Rocket<Build>)

-> Result<Self, Error<Self::InitError>>;

async fn initialize(

db_name: &str,

rocket: &Rocket<Build>

) -> Result<Self, Error<Self::InitError>>;

But I really think this needs to be Self::Error.

SergioBenitez · 2021-06-27T01:05:54Z

examples/databases/src/sqlx.rs

+#[database(name = "sqlx")]
+struct Db(sqlx::SqlitePool);
+
+type Connection = rocket_db_pools::Connection<Db>;


Not of the trait, but of the type directly.

SergioBenitez · 2021-06-27T01:07:45Z

examples/databases/src/sqlx.rs

-        Err(e) => {
-            error!("Failed to read SQLx config: {}", e);
-            return Err(rocket);
+    match rocket.state::<Db>() {


The problem is that this presents two different interfaces: an abstracted Db::fairing() and now a Db in managed state. If the user had instead been asked to do .manage(Db::init()?) or something to that effect, this would be fine. But otherwise, .attach(Db::fairing()) says nothing about something going into managed state.

paulzhang5511 · 2021-07-14T08:44:46Z

How to test a query statement with rocket_db_pools?

SergioBenitez · 2021-07-18T21:11:30Z

Merged in f7f068b with design changes in 5b1a04d.

jebrosen requested a review from SergioBenitez June 14, 2021 01:47

JarvisCraft mentioned this pull request Jun 16, 2021

[Feature] Access to managed objects on Rocket shutdown #1707

Closed

SergioBenitez requested changes Jun 25, 2021

View reviewed changes

jebrosen added 5 commits June 26, 2021 17:23

WIP: Add rocket_db_pools crate.

cd83276

Add rocket_db_pools features to test script.

c3540f1

Use rocket_db_pools in sqlx database example.

5b78f67

Set sqlx connection defaults to those used in the sqlx database example.

59e76df

Apply most changes from first review.

055cc1a

jebrosen force-pushed the rocket_db_pools branch from 7b52d46 to 055cc1a Compare June 27, 2021 00:30

SergioBenitez requested changes Jun 27, 2021

View reviewed changes

Quick fixes.

116cb1a

SergioBenitez mentioned this pull request Jun 29, 2021

Add ArangoDB to rocket_contrib databases #1433

Closed

use From instead of passing a ctor function

ab6194c

jebrosen force-pushed the rocket_db_pools branch from 0749a7c to ab6194c Compare July 8, 2021 22:57

SergioBenitez closed this Jul 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Introduce `db_pools` contrib crate, for asynchronous database connection pooling #1696

Introduce `db_pools` contrib crate, for asynchronous database connection pooling #1696

jebrosen commented Jun 14, 2021 •

edited

Loading

SergioBenitez commented Jun 14, 2021

SergioBenitez left a comment

SergioBenitez Jun 25, 2021

SergioBenitez Jun 25, 2021

jebrosen Jun 26, 2021

jebrosen Jun 27, 2021

SergioBenitez Jun 27, 2021 •

edited

Loading

jebrosen Jun 27, 2021

SergioBenitez Jun 25, 2021

jebrosen Jun 26, 2021

SergioBenitez Jun 27, 2021

SergioBenitez left a comment •

edited

Loading

SergioBenitez Jun 26, 2021

SergioBenitez Jun 27, 2021

SergioBenitez Jun 27, 2021

SergioBenitez Jun 27, 2021

SergioBenitez Jun 27, 2021 •

edited

Loading

SergioBenitez Jun 27, 2021

SergioBenitez Jun 27, 2021

SergioBenitez Jun 27, 2021

paulzhang5511 commented Jul 14, 2021

SergioBenitez commented Jul 18, 2021

		/// Returns a fairing that attaches this connection pool to the server.
		fn fairing() -> Fairing<Self>;

		async fn initialize(db_name: &str, rocket: &Rocket<Build>)
		-> Result<Self, Error<Self::InitError>>;

Introduce db_pools contrib crate, for asynchronous database connection pooling #1696

Introduce db_pools contrib crate, for asynchronous database connection pooling #1696

Conversation

jebrosen commented Jun 14, 2021 • edited Loading

SergioBenitez commented Jun 14, 2021

SergioBenitez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulzhang5511 commented Jul 14, 2021

SergioBenitez commented Jul 18, 2021

Introduce `db_pools` contrib crate, for asynchronous database connection pooling #1696

Introduce `db_pools` contrib crate, for asynchronous database connection pooling #1696

jebrosen commented Jun 14, 2021 •

edited

Loading

SergioBenitez Jun 27, 2021 •

edited

Loading

SergioBenitez left a comment •

edited

Loading

SergioBenitez Jun 27, 2021 •

edited

Loading