tokio-rs / axum

Ergonomic and modular web framework built with Tokio, Tower, and Hyper
19.11k stars 1.06k forks source link

Implement "names" for Routes #298

Closed Flowneee closed 3 years ago

Flowneee commented 3 years ago

Feature Request

Motivation

Having name, associated with route, allow easier implementation of middlewares services, which should work with multiple routes, and also can be used for logging purposes, for example

Proposal

Route can have optional name, which will be set for all requests, passed to this route, and all services and handlers can access this (optional) name. Actix-web implements it like this (and this).

Obstacles

  1. I think this might be impossible in current implementation of Router and might require this issue to be done;
  2. It is unclear, should name be defined for all methods of path or for each methodes separately (actix-web allow to set name for resource, which can be one or more methods, and multiple resources can have same path).

Drawbacks

  1. It adds a little memory usage (additional string per request);
  2. This will require wrapper around http::Request (or dirty way - store name in extensions).

Alternatives

Alternative is to set for request "path pattern" (like /users/:id/things from route("/users/:id/things", get(get_user_things))), used for routing (like in actix-web). It also should have 2 variants like Uri/OriginalUri for handling nested routers. It is harder to use, but it should be easier to implement.

davidpdrsn commented 3 years ago

This feels like an XY problem. What are you actually trying to do that makes you think you need named routes?

You can already add middleware to individual routes like this. Is that the kinda thing you're looking for?

Flowneee commented 3 years ago

What are you actually trying to do that makes you think you need named routes?

For example: I have application, built with actix-web, and it have 2 middlewares:

App::new()
        ...
        .wrap(auth::AuthMiddlewareFactory::new(app_state.clone()))
        .wrap(logging::LoggingMiddlewareFactory::new(app_state))
        ...

(there is actually more middlewares, some of them also uses names), where

You can already add middleware to individual routes

Yes, but imagine, like, 30+ routes, and you have to write middlewares for every one of them. And not like here, you also should provide some data for each of them (not only some shared state).

Of course I can write a wrapper around Router like this (I hope I can write this, didn't tried yet):

struct RouterWrapper(Router);

impl RouterWrapper {
    fn route<T>(&mut self, uri: &str, svc: T, param1: String, param2: String, ...) {
        self.0.route(uri, svc.layer(AuthLayer::new(param1)).layer(LoggingLayer::new(param2)))
    }
}
// maybe Deref/Mut as well

But this is kind of ugly IMO and I think it is nice to add layer once somewhere in the "root" router.

davidpdrsn commented 3 years ago

I'm still not seeing what you're missing. You can add middleware to multiple routes like this.

3olkin commented 3 years ago

@Flowneee you can do it easily using gin-like approach. Here is simple router:

pub fn app(state: AppState) -> Router<BoxRoute> {
    use handlers::*;

    let middleware_stack = ServiceBuilder::new()
        .layer(AddExtensionLayer::new(state.clone()))
        .into_inner();

    Router::new()
        .nest(
            "/api",
            Router::new()
                .route("/login", post(auth::login))
                .boxed()
                .route("/distros", get(distros::index).post(distros::create))
                .boxed()
                .route(
                    "/distros/:id",
                    put(distros::update).delete(distros::destroy),
                )
                .boxed()
                .route("/packages", get(packages::index).post(packages::create))
                .boxed()
        )
        .layer(middleware_stack)
        .boxed()
}

Let's imagine that we want wrap /distros and /distros/:id into tower_http::trace::TraceLayer. We can do it this way:

pub fn app(state: AppState) -> Router<BoxRoute> {
    use handlers::*;

    let middleware_stack = ServiceBuilder::new()
        .layer(AddExtensionLayer::new(state.clone()))
        .into_inner();

    Router::new()
        .nest(
            "/api",
            Router::new()
                .route("/login", post(auth::login))
                .boxed()
                .nest("/", wrapped_routes(state.clone()))
                .boxed()
                .route("/packages", get(packages::index).post(packages::create))
                .boxed()
        )
        .layer(middleware_stack)
        .boxed()
}

pub fn wrapped_routes(state: AppState) -> Router<BoxRoute> {
    use handlers::distros;

    let middleware_stack = ServiceBuilder::new()
        .layer(TraceLayer::new_for_http())
        .layer(AddExtensionLayer::new(state))
        .into_inner();

    Router::new()
        .route("/distros", get(distros::index).post(distros::create))
        .boxed()
        .route(
            "/distros/:id",
            put(distros::update).delete(distros::destroy),
        ).boxed()
        .layer(middleware_stack)
        .boxed()
}
Flowneee commented 3 years ago

I'm still not seeing what you're missing. You can add middleware to multiple routes like this.

Yeah, but I can't say for sure which route was used. For "static" paths this is fine, but if there is "catch" in path, there is a problem. If I have route like /user/:name, I must write some kind of "router", which will match /user/name1 with /user/:name, thus doing this operation twice (first - in axum router, second - in my own inside layers), because request itself does not carry information about pattern, used for routing.

And another option - add layers for each route.

@Z4RX this is close to what I suggested previously, with Router wrapper (your solution is much clearer). But your middleware currently cannot determine that /distros/1 and /distros/2 is a same route (and uses same handler). So .layer() should be called for each route (this way Layer will work with only one route and can log all requests as same route regardless of URI, as well as add metrics for this requests in single counter/histogram/whatever).


Since it is technically possible to implement my case with current functionality (though with ton of boilerplate) and I have no support here, I think I will close this issue (but I'm sure there will be other feature requests like this). Anyway thx for explaining me things)