Implement Reply for Result<impl Reply, impl Reply>

[cjbassi]

This fixes the problems I've been having with error handling as described in #388. I don't think this is a breaking change since http::Error can be added to a result and you get the same effect as before. Let me know what you think of this. I also added an example demonstrating how this would be used.

Close #388

[cjbassi]

The one issue with this is that in the example, rejections are turned into plain text error responses instead of json but this should be easy to fix once #451 is implemented.

[cjbassi]

Actually it seems like this doesn't work with async functions since map doesn't work with async functions... :(

edit: you could use and_then and wrap the function with an async closure like .and_then(async move |num, denom| Ok(div_by(num, denom)));, except async closures are unstable

edit2: I'm stumped on how you would do this if sticking to the Future combinators methodology. Maybe we just need to add another combinator called handler or something?

edit3: How about adding a map_async combinator? I think that would work really well. It would also clean up the todos example so that the handlers don't have to return a result.

edit4: I added map_async and refactored some examples with it.

[jxs]

Hi, there's a PR in the pipeline to impl<T: Reject> From<T> for Rejection which will then allow to use the ? operator on Result<T: Reply, E: Reject> will that help your case?

warp Filters are similar to the futures api, that's why map composes the filter with a function that maps the extracted value, while and_then composes the filter with a function that returns a TryFuture which can be an async fn that returns Result<impl Reply, Rejection>

[cjbassi]

I hadn't seen that pull request but it turns out that I implemented the same thing in my codebase and unfortunately it did not do the trick for me since I still had to add a whole bunch of .map_error(MyError::from) to convert external errors to my error type first before using ? which was pretty unergonomic.

Yah, I ended up studying the Futures API quite a bit yesterday to see if there was a way to do an async map with the current API :) But it doesn't look possible so I had to add a new method.

[cjbassi]

Another idea: what if instead of having to return a Rejection struct we returned the Reject trait? That might work instead. So return values would be Result<impl Reply, impl Reject>. The Reject trait would probably have to be reworked. Idk if that would work, just throwing stuff out there.

[sidju]

I am very interested in this.

Implementing Reply for Result where both implement reply seems trivial and a reasonable thing to add in general. When also combined with some kind of async .map it would suddenly enable me to write handlers as I would like, since handlers should infallibly return a response when executed and should fail fast on errors.

Of all the solutions mentioned in #388 this one seems the best.

(It seems to me that the clash with the futures API here lies in the fact that warp has declared the error type for us and given it special meaning.

It works great in the context of routing and makes writing your own filters a breeze, but it also requires the user to interact with this error type even when it is irrelevant.

This causes frustration when the ? operator could normally be used but the error type is strictly set by warp (and not recommended for returning from a handler, since it was the right route).

Due to this (and the odd nature of request handlers compared to other "normal" async operations) I would say there is a need for a .map_async or .handler)

[sidju]

I am no expert in the internals of this project, but the code looks good and seems to do what I wish for. I will certainly pull down and use this fork until a official fix makes an appearance.

[kaj]

This looks good! The changes in src/reply.rs is pretty much identical to a PR i did before seeing this (and closed as duplicate on finding this). I'm not familiar enough with warp internals to say if the contents of filter/map_sync.rs are correct, but i am familiar enough with using warp to say that the FilterBase::map_async function is dearly wanted.

Of course, sitting since February there is merge conflicts to resolve, and the map_async function does need documentation. Is @cjbassi still around and interested in doing that? Or should I clone the branch, do the rebase and doc, and submit a new PR?

Question: Is map_async a good name for that function? I think it is, but can argue for some other names as well:

• map_async(f) since it is the async version of map.
• then(f), since it is kind of an "unconditional" version of and_then.
• handle(f), since it is used to terminate the matching logic and actually call a handler.

Thoughts?

[PPakalns]

Resolved merge conflicts with the current master branch for this PR in PPakalns/warp as I needed an up to date version. Can be used as a reference to fix conflicts in this PR or to make a new PR.

I am interested that functionality similar to this is added to warp, if needed I can work on the PR.

[jxs]

superseeded by #878

[sidju]

On the contrary, #878 doesn't implement Reply for Result<Reply, Reply>. Whilst the .map_async is indeed covered the Reply implementation may still be of relevance.