warp icon indicating copy to clipboard operation
warp copied to clipboard

Document that `filters::method::*` should come between `filters::path::*` and other ones

Open cpick opened this issue 3 years ago • 0 comments

Version warp v0.3.1

Platform Darwin Kernel Version 20.3.0; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64

Description (I'm brand new to warp so please excuse any mistakes.)

There are at least a few places in the documentation and one in an example that mention that path Filters should be placed first.

I believe this is because the reject::preferred() function treats NOT_FOUND as the lowest priority Rejection.

By the same logic, since reject::preferred() treats METHOD_NOT_ALLOWED as the second-lowest priority shouldn't the method Filters be placed after any path Filters but before any others?

For example, in the "todos" example, there's the following comment:

/// DELETE /todos/:id
pub fn todos_delete(
    db: Db,
) -> impl Filter<Extract = impl warp::Reply, Error = warp::Rejection> + Clone {
    // We'll make one of our endpoints admin-only to show how authentication filters are used
    let admin_only = warp::header::exact("authorization", "Bearer admin");

    warp::path!("todos" / u64)
        // It is important to put the auth check _after_ the path filters.
        // If we put the auth check before, the request `PUT /todos/invalid-string`
        // would try this filter and reject because the authorization header doesn't match,
        // rather because the param is wrong for that other path.
        .and(admin_only)
        .and(warp::delete())
        .and(with_db(db))
        .and_then(handlers::delete_todo)
}

That same logic also applies to the warp::delete() filter. Since it's after admin_only issuing a PUT that passes its path+method filters but is rejected by its json_body() filter still produces the wrong error:

$ curl -v -X PUT -H 'Content-Type: application/json' -d '{}' localhost:3030/todos/1
....
< HTTP/1.1 400 Bad Request
...
Missing request header "authorization"* Closing connection 0

The corresponding server log is:

$ RUST_LOG='debug' cargo watch -x 'run --example todos' &
...
 DEBUG hyper::proto::h1::io   > parsed 5 headers 
 DEBUG hyper::proto::h1::conn > incoming body is content-length (2 bytes) 
 DEBUG hyper::proto::h1::conn > incoming body completed 
 DEBUG warp::filters::body    > request json body error: missing field `id` at line 1 column 2 
 INFO  todos                  > 127.0.0.1:49818 "PUT /todos/1 HTTP/1.1" 400 "-" "curl/7.64.1" 256.16µs
 DEBUG warp::filter::service  > rejected: Rejection([MissingHeader { name: "authorization" }, BodyDeserializeError { cause: Error("missing field `id`", line: 1, column: 2) }]) 
 DEBUG hyper::proto::h1::io   > flushed 164 bytes 
 DEBUG hyper::proto::h1::conn > read eof 

Which shows that the "Missing request header..." error is coming from the admin_only filter. If that filter is moved after warp::delete():

    warp::path!("todos" / u64)
        .and(warp::delete()) // moved up before `admin_only`
        .and(admin_only)

Then the HTTP response and status code is correct:

$ curl -v -X PUT -H 'Content-Type: application/json' -d '{}' localhost:3030/todos/1
...
< HTTP/1.1 400 Bad Request
...
Request body deserialize error: missing field `id` at line 1 column 2* Closing connection 0

The server now returns the (lower priority) MethodNotAllowed (as opposed to the higher priority MissingHeader that was previously being returned):

 DEBUG warp::filter::service  > rejected: Rejection([MethodNotAllowed, BodyDeserializeError { cause: Error("missing field `id`", line: 1, column: 2) }]) 

All of this took me a while to understand (hopefully I've got it all right). I think the documentation on the method Filters should also include some notifications explaining what order they should be in and the todos example should be adjusted as noted above, but maybe this needs a more in-depth description somewhere? I didn't find the whole Rejection preference/priority order intuitive until I dug into the code.

cpick avatar Mar 28 '21 22:03 cpick