Implemented Reply for Result<impl Reply, impl Reply> #909
Conversation
Good to hear - And now; we wait
No, since pub trait Reply: BoxedReply + Send { ... } |
There was a problem hiding this comment.
Not a maintainer, but I rebased these changes on my own branch and the new impl works wonderfully for removing the match statements everywhere in my handlers and replacing them with concise ?s.
Using this impl I could define a new error type and impl Reply on it, as well as From for library error types:
use thiserror::Error;
use warp::reply::Response;
use warp::Reply;
#[derive(Error, Debug)]
pub enum ApiError {
#[error("Database error")]
DatabaseError,
#[error("Object not found")]
NotFound,
#[error("Object could not be serialized into JSON")]
JsonSerializeError,
}
impl Reply for ApiError {
fn into_response(self) -> Response {
self.to_string().into_response()
}
}
impl From<sea_orm::DbErr> for ApiError {
fn from(_err: sea_orm::DbErr) -> ApiError {
ApiError::DatabaseError
}
}
impl From<serde_json::Error> for ApiError {
fn from(_err: serde_json::Error) -> ApiError {
ApiError::JsonSerializeError
}
}The resulting handler looks like this:
pub async fn get_node(db: Db) -> Result<impl Reply, ApiError> {
// Each use of ? concisely calls into one of the From impls
let node = node::Entity::find()
.filter(node::Column::UserId.eq(1))
.one(&*db)
.await?
.ok_or(ApiError::NotFound)?;
let json = serde_json::to_string(&node)?;
Ok(json)
}Which allowed me to terminate without any recovers, Rejections, or other unintended constructs:
let route = warp::path("node")
.and(warp::post())
.and(with_db(db.clone()))
.and(warp::body::json())
.then(v1::get_node);Rebasing on master had 0 conflicts so it should be super quick, but if @gtsiam is busy I can make a fresh PR.
Thank you for implementing this!
|
I don't like pinging like this, but @jxs @seanmonstar: Since there seems to be interest in this, could someone take a look? I intentionally made the change as simple as humanly possible to make merging easy. |
|
I guess you could do: and then on all your routes: no? But yes, seems pretty obvious that |
|
Iterating on your idea @jakajancar, it turns out that if you call Given e.g. a handler that returns a #[derive(Error, Debug)]
pub enum ApiError {
#[error("Database error")]
DatabaseError,
#[error("Object not found")]
NotFound,
}
impl Reply for ApiError {
fn into_response(self) -> Response<Body> {
Response::builder()
.status(StatusCode::INTERNAL_SERVER_ERROR)
.body(self.to_string().into())
.expect("Could not construct Response")
}
}
#[derive(Serialize)]
pub struct NodeInfo {
pub pubkey: String,
pub num_channels: usize,
}
pub async fn node_info(
channel_manager: Arc<ChannelManagerType>,
peer_manager: Arc<PeerManagerType>,
) -> Result<NodeInfo, ApiError> {
// do stuff that builds the NodeInfo
let resp = NodeInfo {
pubkey,
num_channels,
};
Ok(resp)
}One can have a helper like this: use http::response::Response;
use serde::Serialize;
use warp::hyper::Body;
use warp::{reply, Reply};
/// Converts Result<S, E> into Response<Body>, avoiding the need to call
/// reply::json(&resp) in every handler or to implement warp::Reply manually
fn into_response<S: Serialize, E: Reply>(
reply_res: Result<S, E>,
) -> Response<Body> {
match reply_res {
Ok(resp) => reply::json(&resp).into_response(),
Err(err) => err.into_response(),
}
}With a route that looks like this: let node_info = warp::path("node_info")
.and(warp::get())
.and(inject::channel_manager(channel_manager))
.and(inject::peer_manager(peer_manager))
.then(node_info)
.map(into_response);I think it is pretty clean, and it works with warp today. It also allows testing handlers as pure functions. |
- A handler function, instead of returning a Warp reply/rejection, returns a
`Result<Reply, ApiError>.`
- This is because rejections are meant to say "this filter can't handle this
request, but maybe some other can" (see
seanmonstar/warp#388 (comment)).
- A rejection means Warp will fall through to another filter and ultimately
hit a rejection handler, with people reporting rejections take way too long
to process with more routes.
- In our case, the error in our handler function is final and we also would
like to be able to use the ? operator to bail out of the handler if an error
exists, which using a Result type handles for us.
- ApiError knows how to convert itself to an HTTP response + status code
(error-specific), allowing us to implement Reply for ApiError.
- We can't implement `Reply` for `Result<Reply, Reply>` (we didn't make the
`Result` type), so we have to add a final function `into_response` that
converts our `Result` into a Response. We won't need to do this when
seanmonstar/warp#909 is merged:
```
.then(my_handler_func)
.map(into_response)
```
- A handler function, instead of returning a Warp reply/rejection, returns a
`Result<Reply, ApiError>.`
- This is because rejections are meant to say "this filter can't handle this
request, but maybe some other can" (see
seanmonstar/warp#388 (comment)).
- A rejection means Warp will fall through to another filter and ultimately
hit a rejection handler, with people reporting rejections take way too long
to process with more routes.
- In our case, the error in our handler function is final and we also would
like to be able to use the ? operator to bail out of the handler if an error
exists, which using a Result type handles for us.
- ApiError knows how to convert itself to an HTTP response + status code
(error-specific), allowing us to implement Reply for ApiError.
- We can't implement `Reply` for `Result<Reply, Reply>` (we didn't make the
`Result` type), so we have to add a final function `into_response` that
converts our `Result` into a Response. We won't need to do this when
seanmonstar/warp#909 is merged:
```
.then(my_handler_func)
.map(into_response)
```
- A handler function, instead of returning a Warp reply/rejection, returns a
`Result<Reply, ApiError>.`
- This is because rejections are meant to say "this filter can't handle this
request, but maybe some other can" (see
seanmonstar/warp#388 (comment)).
- A rejection means Warp will fall through to another filter and ultimately
hit a rejection handler, with people reporting rejections take way too long
to process with more routes.
- In our case, the error in our handler function is final and we also would
like to be able to use the ? operator to bail out of the handler if an error
exists, which using a Result type handles for us.
- ApiError knows how to convert itself to an HTTP response + status code
(error-specific), allowing us to implement Reply for ApiError.
- We can't implement `Reply` for `Result<Reply, Reply>` (we didn't make the
`Result` type), so we have to add a final function `into_response` that
converts our `Result` into a Response. We won't need to do this when
seanmonstar/warp#909 is merged:
```
.then(my_handler_func)
.map(into_response)
```
- A handler function, instead of returning a Warp reply/rejection, returns a
`Result<Reply, ApiError>.`
- This is because rejections are meant to say "this filter can't handle this
request, but maybe some other can" (see
seanmonstar/warp#388 (comment)).
- A rejection means Warp will fall through to another filter and ultimately
hit a rejection handler, with people reporting rejections take way too long
to process with more routes.
- In our case, the error in our handler function is final and we also would
like to be able to use the ? operator to bail out of the handler if an error
exists, which using a Result type handles for us.
- ApiError knows how to convert itself to an HTTP response + status code
(error-specific), allowing us to implement Reply for ApiError.
- We can't implement `Reply` for `Result<Reply, Reply>` (we didn't make the
`Result` type), so we have to add a final function `into_response` that
converts our `Result` into a Response. We won't need to do this when
seanmonstar/warp#909 is merged:
```
.then(my_handler_func)
.map(into_response)
```
|
I iterated towards a similar solution -- it would be nice if infrastructure like |
|
Indeed. But at this point my experience contributing to warp indicates that it is simply abandonware. Generally I've come to expect that any library by @seanmonstar that isn't hyper is or will become abandonware at this point. If you want a good framework, go with axum. If you already have a large codebase, just write up a quick |
|
It is true that I can spend less attention on warp. My previous job filled up my time with corporate overhead, and a prioritization towards things the company specifically used. It's not a great excuse, it's just the reason. I'm sorry that makes contributing frustrating. I am happy to onboard others who can pay more attention, as I did with @jxs a couple years ago. The way I'm able to build trust like that is when someone starts doing reviews, and reviews in a way that aligns with the previous issues/reviews and goals of the project. With regards to this PR, I remember seeing it a while ago and feeling uncertain about |
|
Thank you @gtsiam for the PR, and for reminding me existed! |
|
That was fast! @seanmonstar If you're looking for maintainers, consider myself volunteered. |
#878 was originally meant to work with this change, however #458 which originally provided it was closed after #878 was merged. And so, I'm providing a minimal subset of #458 to merge. I'm leaving the example changes from #458 up to a future pr, since those could be subject to discussions unrelated to this particular change.
Changes:
ReplyforResult<impl Reply, impl Reply>.