seanmonstar / warp

A super-easy, composable, web server framework for warp speeds.
https://seanmonstar.com/post/176530511587/warp
MIT License
9.5k stars 713 forks source link

Rejecting with warp::reply::not_found results in a 405 Method Not Allowed error #77

Open Geal opened 6 years ago

Geal commented 6 years ago

Hello, with the following code, I would get a 405 error while I expect a 404:

fn main() {
  let routes = warp::post2().map(warp::reply).or(warp::get2().and(warp::path("test").and_then(send_test)));
  warp::serve(routes).run(([127, 0, 0, 1], 3000));
}

fn send_test() -> Result<String, warp::Rejection> {
  Err(warp::reject::not_found())
}

Is there something I am doing wrong?

seanmonstar commented 6 years ago

No, you aren't wrong in thinking that. Currently, the rejections in warp collect and the highest priority one is chosen to render as a response. It's assumed that Not Found is the lowest priority, so the method rejection from the post plus the not found rejection you create means the 405 is shown.

The priority of these could maybe need adjustment.

Geal commented 6 years ago

maybe there could be a way to cut backtracking? ie, if we know we got to a specific prefix or argument, we're in the right route, and should return the error from this route

blm768 commented 6 years ago

In principle, if you want a particular error to override all others, you can return an Ok(http::Response) with that error code. Maybe not the most ergonomic way to do it, but it's accepting the mapping of route to resource and then rejecting the request, so it kind of makes sense in an abstract way. Alternatively, in this case, you could probably move the get2() farther down the chain.

algermissen commented 6 years ago

Can you explain how I could return such a response from a header filter? I think that is not possible right now if I am not mistaken?

seanmonstar commented 6 years ago

Can you explain how I could return such a response from a header filter?

You could make use of Filter::recover to alter the rejection, or Filter::or_else can be useful to provide a "default" value overriding any rejection.

mexus commented 6 years ago

My vote is for some way to cut the backtracking. Currently I'm using the approach proposed by @blm768 , but as he mentioned it's not quite ergonomic. IMHO any reject provided by a user's code should by default stop the processing and just return the rejection, and probably it's not a too bad idea to have a special reject like reject::try_something_different() (bad naming, but I hope you'll get the point) that should be returned by the combinators to go for furthers ors.

ghost commented 6 years ago

There's an example to customize errors as described by @blm768 for anyone interested: examples/errors.rs

algermissen commented 6 years ago

What do other's think about the need for context-specific rejection payload? I still see no way to do that with the rejection design as-is. Or I am missing something.

My use case is 401 Unauthorized, where it is usually only known in that specific route, what exactly to return as Challenge parameters. Another use case would be HTTP Problems (https://tools.ietf.org/html/rfc7807).

Thoughts?

boustrophedon commented 5 years ago

+1 for this behavior being unexpected.

As an example, in the todos example when you build the filters which all contain paths under /todos: https://github.com/seanmonstar/warp/blob/f334f51dc33cf61f145c8d693a554958a9c6ab61/examples/todos.rs#L93

if you add the lines

let req = warp::test::request().path("/");
assert_eq!(req.reply(&api).status(), 404);

the assertion fails with 405 even though you'd expect to get a 404 since we have no routes on /

The workaround is as you mentioned above, you can add an .or_else(|_| Err(warp::reject::not_found())); to the end of the filter and the assertion will pass. However, now you don't get proper 405 errors when doing get requests on post-only paths.

I don't know what the best way to solve this in general is though.

Timmmm commented 4 years ago

Presumably there should be:

yuttie commented 4 years ago

Considering the comments from @Timmmm, I guess we should distinguish parse errors and application errors.

mvolkmann commented 3 years ago

Is there a recommended way to handle this case now? For example, I have a GET /dog/{id} route that needs to return JSON when a matching dog is found and return a 404 when it is now. I have not been able to find a way to accomplish this.

luke-brown commented 3 years ago

With the caveat that I'm new here and am certainly not authoritative, for what it's worth I've been able to address these issues by using what I think of as route-specific recover() handlers. Here's my interpretation of a solution to what you're describing, in context of the original example above: https://gist.github.com/luke-brown/fbaa1392964befd9a285ca310f0b270a That returns a 404 instead of a 405 when it doesn't match (randomized there).

mvolkmann commented 3 years ago

Thanks! I think warp should support an easier way to do this, but at least it works. Somehow my POST /dog route stopped working and I now get a 404. Can you spot why? https://github.com/mvolkmann/rust-rest/blob/c7627eca2f66def33c5a9741c333c200d7e8bdda/warp/src/main.rs#L80

luke-brown commented 3 years ago

I think so: my original gist was oversimplified for that case. I imagine putting get_dog last in routes would technically fix it, since a blanket Ok(404) response from a recover handler prevents or() from looking for more routes. The simplest design might be just building a response in get_dog that handles both success and the error case for that lookup. Another alternative that should work is using a custom rejection type, and recovering specifically from that. I updated this with both of those alternatives: https://gist.github.com/luke-brown/fbaa1392964befd9a285ca310f0b270a That second option probably looks more complicated than the "inline" option--here it's certainly a lot more code--but so far I've found that using custom rejection types makes handling complex handlers easy to reject early from.

mvolkmann commented 3 years ago

Thanks so much for your help on this!

SpyMachine commented 3 years ago

I wonder if 404 could have two rejections. A publicly exposed one with a higher priority and one that is the catch all with lowest priority. That may help solve the issue here.

meredian commented 2 years ago

Not happy with that as well. Really would like to have better return code without much workarounds 🙂

My workaround was to create custom rejection for this non-interceptable "Not Found" case: https://gist.github.com/meredian/9dff0dc056fc4c3e61252576ae86a41b