axum icon indicating copy to clipboard operation
axum copied to clipboard

Published 20 hours ago • verified publisher icon tokio-rs

Middleware that change the request body have poor usability

Open davidpdrsn opened this issue 3 years ago • 6 comments

We've heard from several people who try to use tower_http::limit::RequestBodyLimitLayer that the implications are hard to understand.

For example breaking your Router into functions is quite common. Something like:

#[tokio::main]
async fn main() {
    let app = my_routes();

    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    axum::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

fn my_routes() -> Router {
    Router::new()
}

Adding RequestBodyLimitLayer like so

let app = my_routes().layer(RequestBodyLimitLayer::new(1024));

should just work but produces a rather confusing error:

    Checking example-hello-world v0.1.0 (/Users/davidpdrsn/dev/major/axum/examples/hello-world)
error[E0277]: the trait bound `Route: tower_service::Service<Request<http_body::limited::Limited<_>>>` is not satisfied
  --> hello-world/src/main.rs:13:27
   |
13 |     let app = my_routes().layer(RequestBodyLimitLayer::new(1024));
   |                           ^^^^^ the trait `tower_service::Service<Request<http_body::limited::Limited<_>>>` is not implemented for `Route`
   |
   = help: the following implementations were found:
             <Route<B, E> as tower_service::Service<Request<B>>>
   = note: required because of the requirements on the impl of `tower_service::Service<Request<_>>` for `RequestBodyLimit<Route>`

The solution is instead to write

fn my_routes() -> Router<http_body::Limited<Body>> {
    Router::new()
}

but that isn't obvious and ideally shouldn't be necessary.

We should think about ways to improve that experience.

davidpdrsn avatar Jun 20 '22 13:06 davidpdrsn

I think this is closely related to https://github.com/tokio-rs/axum/pull/1154. If axum had its own body type, and a way to construct that from any B: http_body::Body then we could use that everywhere, even if you applied middleware.

That is something we'll have to explore once the development of hyper 1.0 (and hyper-utils) is further along. Hyper 1.0 will require a breaking release of axum anyway, so I think we should wait with this until we're closer to that happening.

I'm gonna remove this from the 0.6 milestone.

davidpdrsn avatar Aug 22 '22 12:08 davidpdrsn

why does ContentLengthLimit exist in Axum when we have tower_http::limit::RequestBodyLimitLayer in Tower, on which Axum relies anyway? What are their biggest differences other than that the former has the tradeoff that its argument needs to be a const?

avdb13 avatar Sep 15 '22 16:09 avdb13

why does ContentLengthLimit exist in Axum when we have tower_http::limit::RequestBodyLimitLayer in Tower, on which Axum relies anyway? What are their biggest differences other than that the former has the tradeoff that its argument needs to be a const?

Mostly history. ContentLengthLimit was made before RequestBodyLimitLayer existed. It also has a few differences

  • It doesn't change the type of your Router. Adding RequestBodyLimitLayer changes the Router type to Router<_, http_body::Limited<_>>. Thats what this issue is about.
  • It doesn't permit streaming requests, which RequestBodyLimitLayer does.

I tried previously to implement ContentLengthLimit on top of http_body::Limited but couldn't do it because of limitations in 0.5. However in 0.6 FromRequest gets an owned Request<B> which it can take a part and change the body type of. So it might be possible to bridge the cap further between the two and make ContentLengthLimit support streaming requests.

Edit: Its possible https://github.com/tokio-rs/axum/pull/1389!

davidpdrsn avatar Sep 18 '22 18:09 davidpdrsn

I'm trying to use tower_http::limit::RequestBodyLimitLayer on a single handler but I get the following error:

error[E0277]: the trait bound `axum::handler::Layered<RequestBodyLimit<IntoService<fn(Extension<Arc<App>>, Multipart) -> impl Future<Output = axum::response::Redirect> {handlers::creat
e_post}, (Extension<Arc<App>>, Multipart), _>>, (Extension<Arc<App>>, Multipart)>: Handler<_, _>` is not satisfied
   --> src/routes.rs:27:32
    |
27  |         .route("/:board", post(create_post))
    |                           ---- ^^^^^^^^^^^ the trait `Handler<_, _>` is not implemented for `axum::handler::Layered<RequestBodyLimit<IntoService<fn(Extension<Arc<App>>, Multipart
) -> impl Future<Output = axum::response::Redirect> {handlers::create_post}, (Extension<Arc<App>>, Multipart), _>>, (Extension<Arc<App>>, Multipart)>`
    |                           |
    |                           required by a bound introduced by this call
    |
    = help: the trait `Handler<T, ReqBody>` is implemented for `axum::handler::Layered<S, T>`
note: required by a bound in `post`
   --> /home/mikoto/.cargo/registry/src/github.com-1ecc6299db9ec823/axum-0.5.13/src/routing/method_routing.rs:400:1
    |
400 | top_level_handler_fn!(post, POST);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `post`
    = note: this error originates in the macro `top_level_handler_fn` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `Router<http_body::Limited<axum::body::Body>>: tower_service::Service<Request<axum::body::Body>>` is not satisfied
  --> src/main.rs:71:16
   |
71 |         .serve(router.into_make_service())
   |          ----- ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `tower_service::Service<Request<axum::body::Body>>` is not implemented for `Router<http_body::Limited<axum::body::Body>>`
   |          |
   |          required by a bound introduced by this call
   |
   = help: the trait `tower_service::Service<Request<B>>` is implemented for `Router<B>`
   = note: required because of the requirements on the impl of `hyper::service::http::HttpService<axum::body::Body>` for `Router<http_body::Limited<axum::body::Body>>`
   = note: required because of the requirements on the impl of `hyper::service::make::MakeServiceRef<hyper::server::tcp::addr_stream::AddrStream, axum::body::Body>` for `IntoMakeServic
e<Router<http_body::Limited<axum::body::Body>>>`

error[E0277]: the trait bound `hyper::common::exec::Exec: hyper::common::exec::NewSvcExec<hyper::server::tcp::addr_stream::AddrStream, IntoMakeServiceFuture<Router<http_body::Limited<a
xum::body::Body>>>, Router<http_body::Limited<axum::body::Body>>, hyper::common::exec::Exec, hyper::server::server::NoopWatcher>` is not satisfied
  --> src/main.rs:71:43
   |
71 |           .serve(router.into_make_service())
   |  ___________________________________________^
72 | |         .await
   | |______________^ the trait `hyper::common::exec::NewSvcExec<hyper::server::tcp::addr_stream::AddrStream, IntoMakeServiceFuture<Router<http_body::Limited<axum::body::Body>>>, Route
r<http_body::Limited<axum::body::Body>>, hyper::common::exec::Exec, hyper::server::server::NoopWatcher>` is not implemented for `hyper::common::exec::Exec`
   |
   = help: the trait `hyper::common::exec::NewSvcExec<I, N, S, E, W>` is implemented for `hyper::common::exec::Exec`
   = note: required because of the requirements on the impl of `Future` for `Server<hyper::server::tcp::AddrIncoming, IntoMakeService<Router<http_body::Limited<axum::body::Body>>>>`
   = note: required because of the requirements on the impl of `IntoFuture` for `Server<hyper::server::tcp::AddrIncoming, IntoMakeService<Router<http_body::Limited<axum::body::Body>>>>
`
help: remove the `.await`
   |
71 -         .serve(router.into_make_service())
71 +         .serve(router.into_make_service())
   |

error[E0277]: the trait bound `Router<http_body::Limited<axum::body::Body>>: tower_service::Service<Request<axum::body::Body>>` is not satisfied
  --> src/main.rs:71:10
   |
71 |         .serve(router.into_make_service())
   |          ^^^^^ the trait `tower_service::Service<Request<axum::body::Body>>` is not implemented for `Router<http_body::Limited<axum::body::Body>>`
   |
   = help: the trait `tower_service::Service<Request<B>>` is implemented for `Router<B>`
   = note: required because of the requirements on the impl of `hyper::service::http::HttpService<axum::body::Body>` for `Router<http_body::Limited<axum::body::Body>>`
   = note: required because of the requirements on the impl of `hyper::service::make::MakeServiceRef<hyper::server::tcp::addr_stream::AddrStream, axum::body::Body>` for `IntoMakeServic
e<Router<http_body::Limited<axum::body::Body>>>`

This is what my router looks like:

pub fn routes(app: Arc<App>) -> Router<http_body::Limited<Body>> {
    let create_post = handlers::create_post.layer(RequestBodyLimitLayer::new(
        (app.config.security.validate_upload_limit()).unwrap(),
    ));

    Router::new()
        ...
        .route("/:board", post(create_post))
        .layer(TraceLayer::new_for_http())
        .layer(Extension(app))
        .fallback(handlers::fallback.into_service())
}

avdb13 avatar Sep 22 '22 12:09 avdb13

The following compiles fine:

pub fn routes(app: Arc<App>) -> Router {
    let create_post = handlers::create_post;

    Router::new()
        ...
        .route("/:board", post(create_post))
        .layer(TraceLayer::new_for_http())
        // .layer(RequestBodyLimitLayer::new(
        //     (app.config.security.validate_upload_limit()).unwrap(),
        // ))
        .layer(Extension(app))
        .fallback(handlers::fallback.into_service())
}

avdb13 avatar Sep 22 '22 13:09 avdb13

@avdb13 please open a separate issue/discussion about that and please include a minimal reproducible example that we can run.

davidpdrsn avatar Sep 22 '22 15:09 davidpdrsn

I'd say this has been solved by https://github.com/tokio-rs/axum/pull/1751. TLDR: in axum 0.7 the B type parameter has been removed and we instead use our own Body type. So adding new middleware never changes the request body type.

davidpdrsn avatar Mar 20 '23 20:03 davidpdrsn