warp Implemented Reply for Result<impl Reply, impl Reply>

#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:

Implemented Reply for Result<impl Reply, impl Reply>.

Oct 16 '21 23:10 gtsiam

This looks good, I would love to have it in an upcoming warp release.

Good to hear - And now; we wait

Should it require Send for the T and E of the Result<T, E>?

No, since Reply already requires it:

pub trait Reply: BoxedReply + Send { ... }

Oct 22 '21 12:10 gtsiam

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.

Jun 09 '22 11:06 gtsiam

@gtsiam

I guess you could do:

fn result_reply_reply_into_reply<'a>(result: Result<impl warp::Reply + 'a, impl warp::Reply + 'a>) -> impl warp::Reply + 'a {
    match result {
        Ok(ok) => Box::new(ok) as Box<dyn warp::Reply>,
        Err(err) => Box::new(err) as Box<dyn warp::Reply>,
    }
}

and then on all your routes:

let route = warp::path("node")
    .and(warp::post())
    .and(with_db(db.clone()))
    .and(warp::body::json())
    .then(v1::get_node)
    .map(result_reply_reply_into_reply);

no?

But yes, seems pretty obvious that impl Reply should be implemented for Result<impl Reply, impl Reply>, especially now that Filter::then exists. @jxs?

Jun 29 '22 22:06 jakajancar

Iterating on your idea @jakajancar, it turns out that if you call into_response() provided by the Reply trait, you can avoid the heap allocation.

Given e.g. a handler that returns a Result<S, E> where S implements Serialize and E implements Reply

#[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.

Jul 14 '22 20:07 MaxFangX

I iterated towards a similar solution -- it would be nice if infrastructure like fn into_response above, or the impl Reply for Result<...,..>, were in some shared utility crate, or warp itself, since many people are likely to reinvent this and then carry it.

Jul 21 '23 17:07 cbeck88

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 ReplyExt trait for this method and move on with your life :/

Jul 21 '23 18:07 gtsiam

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 Result<impl Reply, impl Reply>, since I feel like the Rejection and Error system in warp is probably not quite as elegant as I had hoped. But then other things crowded that out my mind. Looking again now, I think this is perfectly fine. If at some point the Rejection system were to be redesigned, and this were in conflict, we can always change it then. (Basically, I'm stopping a "what-if-future" from getting in the way of "would-be-nice-now".)

Jul 21 '23 18:07 seanmonstar

Thank you @gtsiam for the PR, and for reminding me existed!

Jul 21 '23 18:07 seanmonstar

That was fast!

@seanmonstar If you're looking for maintainers, consider myself volunteered.

Jul 21 '23 19:07 gtsiam

warp warp copied to clipboard

Implemented Reply for Result<impl Reply, impl Reply>

warp
warp copied to clipboard