Refactor middleware interface

[hjr3]

BREAKING CHANGES - bumps the crate to 0.2.0

• Remove Arc wrapper around the Pool. The Pool internally handles this.
• PostgresMiddleware::new() now only accepts a Postgres URL and uses
  sensible defaults to setup the connection pool.
• Add PostgresMiddleware::with_pool() to accept an already connected
  pool when creating the middleware.
• Rename db_conn() to pg_conn(). This method is now safer and provides
  more a better error message if used without first registering the
  middleware.
• Original example has been updated to reflect these changes. A new
  example has been added to show how with_pool() works.

[hjr3]

@Ryman please let me know if you want to accept this patch. I am using this now in a personal project. I will commit to adding some more documentation around how to use this if accepted.

I still think there is value in middleware for this. nickel uses move closures for routes, so the connection can not be directly reference from the closure if multiple routes are specified. The only other option is to use the shared data functionality that nickel provides. I would like to reserve the shared data functionality for things that are awkward to put in middleware. A database is so core to the application that it should not take up the valuable shared data slot and be more of a first class citizen of the framework.

[flosse]

+1

I refactored nickel-sqlite as well :)

[hjr3]

@flosse awesome! having a consistent interface across these is good. i was almost considering having a very generic r2d2 middleware using only the traits. that is dynamic dispatch though and i have no way to perf test the differences at the moment.

[flosse]

i was almost considering having a very generic r2d2 middleware using only the traits.

that sounds good :)

[Ryman]

(Sorry for the delay and the size of reply!)

I'm happy to accept something along the lines of this patch but I think it's worth discussing a little about the benefits of the library as mentioned in the thread.

Simplify API by accepting an already connected pool when creating the middleware. This should allow us to maintain a stable interface even if more options are added to the r2d2 Config.

I'm unsure if there's going to be any real stability benefit here as it relies on the user's crate versions aligning with the versions specified in this crate which leads to confusing errors (as if you use nickel and an incompatible version of hyper). I do think it's a definite win to simplify the constructor though!

Perhaps it's worth calling the current constructor with_pool which gives the user total configurability, while adding other "good defaults" constructors such as new(db_url: &str) and new_with_ssl(db_url: &str, sslMode: SslMode). The latter could mean the user only needs to have nickel_postgres as a dependency instead of having to figure out what they need to import as we can re-export what appears in our api (as noted in your (fantastic) blogpost that SslMode was coming from an unexpected place and the user had to import several crates). The latter would not need to be introduced in this PR, I'd just like to get alternative viewpoints on the idea.

The db_conn() method is now safer. The lone unwrap is caused by a developer setting the middleware up wrong, so it should be fine to panic.

I think this is a good change, thanks! It might be worth wrapping the error type though so Into<NickelError> could be implemented so that let mut db = try_with(res, req.db_conn()) would work. Turning the unwrap into an expect would also be more helpful to the user since it should only happen as you've said whenever they've made a setup error.

I still think there is value in middleware for this. nickel uses move closures for routes, so the connection can not be directly reference from the closure if multiple routes are specified. The only other option is to use the shared data functionality that nickel provides. I would like to reserve the shared data functionality for things that are awkward to put in middleware.

This relates to that unfortunate unwrap in db_conn! You should be able to share the pool between multiple routes using an Arc but that doesn't help much if the user wants to split their app into functions rather than a block of closures (at least until impl Fn works, or they thread the router around).

The benefit to doing something using ServerData is that you get a compile-time guarantee, similar to in the cookies middleware that you can't have secure cookies without giving it a key.

Overall though, I think it's good to have options on how to deal with this stuff and the middleware does make things easier in a lot of cases :)

i was almost considering having a very generic r2d2 middleware using only the traits.

I also think this is likely a good idea (although I don't get the point about why it requires dynamic dispatch?)

[hjr3]

@Ryman thank you for the thoughtful response!
I will certainly update the crate with your suggestions about error handling.

I agree with you about the crate versions needing to change. With regards to the stable interface, I only meant that we only had to support the r2d2::Pool interface. As we saw, r2d2::PostgresConnectionManager changed to use a different SslMode enum. That being said, I do think that useful defaults are a good thing. I am happy to support both if we think that is best.

With regards to a nickel-r2d2 middleware, I might be wrong about the dynamic dispatch. I was assuming that Rust would have to look up the proper implementation at runtime, but maybe it will monomorphize at compile time. Something for me to look into.

[hjr3]

Not sure I am fully understanding the Into<NickelError> suggestion now. I wrapped the error type, but where are expecting that code that converts the wrapped error type to NickelError to exist?

I would want to implement something like:

impl<'_> From<(Response<'_>, PostgresMiddlewareError)> for NickelError<'_> {
    fn from((res, err): (Response<'_>, PostgresMiddlewareError)) -> NickelError<'_> {
        NickelError::new(res, format!("{}", err), StatusCode::InternalServerError)
    }
}

but cannot because the middleware does not own that NickelError type.

[ghost]

@hjr3, you can return Result<PooledConnection<PostgresConnectionManager>, (StatusCode, PostgresMiddlewareError)>.

[hjr3]

@kilte thank you for the hint 😄 i removed the PostgresConnectionManager enum as GetTimeout already converts to std::error::Error.

@Ryman al discussed changes are in.

[ghost]

What do you think about renaming db_conn to pg_conn?
People can use more than one middleware which provides a database connection and it would be nice if each method name will reflect a type of provided connection.

[hjr3]

@kilte I think that is a great idea. Added a new commit.

Prior to merge I will be squashing and updating the commit message with the changes discussed.

[Ryman]

@hjr3 This looks great, thanks for following up on the changes!

What do you think about renaming db_conn to pg_conn?

I don't really mind about this, happy enough to change it. Though perhaps adding an alias for the explicit form could be done (that just implements in terms of the other). So that in the general case db_conn is clearer than pg_conn?

return Result<PooledConnection, (StatusCode, PostgresMiddlewareError)>.

Damn, this reminds me @flosse raised an issue about this months ago and I totally forgot to follow up on. There's a thread nickel-org/nickel.rs#304 (with a linked branch with a suggested change to fix the coherence problem) if anyone wants to push a better suggestion for changes. I think it also may work with the wrapped crate-local error type if Into<Box<Error>> is implemented or might just fix itself with specialization but I'm not certain about that!

[Ryman]

Are these crate imports still required in this example?

[hjr3]

nope, i am not sure why this was not triggering warnings.

[Ryman]

Just a few minor nits remaining. Feel free to squash and rebase as you like, multiple commits is fine (encouraged even) as long as they're not wip.

[hjr3]

@Ryman

What do you think about renaming db_conn to pg_conn?

I don't really mind about this, happy enough to change it. Though perhaps adding an alias for the explicit form could be done (that just implements in terms of the other). So that in the general case db_conn is clearer than pg_conn?

Sorry, I am not clear what you are suggesting here.

[Ryman]

Sorry, I am not clear what you are suggesting here.

Something along the lines of this:

pub trait PostgresRequestExtensions {
    fn pg_conn(&self) -> Result<PooledConnection<PostgresConnectionManager>, (StatusCode, GetTimeout)>;

    /// Alias for users who don't want to be explicit about their database brand
    fn db_conn(&self) -> Result<PooledConnection<PostgresConnectionManager>, (StatusCode, GetTimeout)> {
        self.pg_conn()
    }
}

I'm not sure if it's a good idea or not though. It might make more sense in a more generalized trait similar to the one you proposed earlier in the thread.

[hjr3]

@Ryman I think it should be a separate trait. Let me try and get that more generalized version working after we get this in.

I squashed down into a single commit and updated the commit message. I was going to try and make separate commits, but my changes were all tangled together. I will be more conscious about it in the future.

[Ryman]

Thanks @hjr3, I look forward to seeing what you come up with in future!

ping @MatthewBentley: I tried to publish this update but it seems that you have ownership of the crate name on crates.io, would you mind adding the nickel-org organisation as an additional crate owner?

[mtbentley]

I went ahead and added @Ryman, since I was getting errors trying to add nickel-org. You can do whatever you need.

[hjr3]

I still see https://crates.io/crates/nickel_postgres at 0.1.0. @Ryman please publish the new crate when you get a moment.

[Ryman]

Published 0.2, thanks @hjr3 @MatthewBentley 👍

[flosse]

I published https://github.com/flosse/nickel-sqlite v0.2 that should behave similar :)