Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Router::with_state and impl Service for Router<()> #1552

Merged
merged 11 commits into from Nov 24, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Nov 20, 2022

Alright this is hopefully the final change to state stuff before 0.6.0 🤞

This changes Router::with_state to

impl<S, B> Router<S, B> {
    pub fn with_state<S2>(self, state: S) -> Router<S2, B> {
        // ...
    }
}

i.e. you get back a new Router and you get decide what state it needs.

This means you can now nest and merge routers with different state by calling with_state:

// a router the requires an `A`
Router::new()
    // calling `with_state` on the inner router returns a new routes whose state
    // will be inferred to `A`
    .merge(Router::new().with_state(B))
    .with_state(A);

// same works for `nest`
Router::new()
    .nest("/foo", Router::new().with_state(B))
    .with_state(A);

Additionally Router<()> now implements Service and RouterService has been removed. This means libraries (such as tonic) can now more easily expose Service compatible interfaces without providing their own .into_service() - as was necessary before (see hyperium/tonic#1155.)

If you do Router::new().with_state(A).call(request) the state of the new router is inferred to () and it all works nicely.

Note that MethodRouter gets the same treatment.

The only downside is that calling a router where with_state has not been called (because it doesn't need any state) doesn't give us a chance to convert everything into Routes ahead of time. That means we need to do it for each request. I haven't measured the perf impact of this but it will at least cause some additional allocations. Router::into_make_service works around this so it shouldn't impact most users but I've still documented it.

Fixes #1547

@davidpdrsn
Copy link
Member Author

So I've been wondering if this plus adding impl Service for Router<()> would be enough to remove RouterService.

Having to call Router::into_service to get a Service cascades into libraries that use axum internally, such as tonic. See hyperium/tonic#1155. I don't like how that requires tonic to change its public API 😕

@jplatte
Copy link
Member

jplatte commented Nov 21, 2022

I don't have a strong opinion on that. I kind of like how into_service erases some of the details of the Router that are no longer relevant after all routes have been added, but I don't think that's a strong reason to keep things as they are.

@jimmycuadra
Copy link
Contributor

It might be clearer to use the name with_state for this new provide_state method, and change the current with_state method into something that makes it clear the type is being changed from a Router to a RouterService. The type change caused by the current with_state is obvious if you're looking at the API docs for the method, but the name of the method doesn't really make the type change apparent, like when you're just looking at an example of its use. Maybe into_stateful_service would work? So you'd end up with:

Router::new()
    .merge(router_with_other_state.with_state(other_state))
    .into_stateful_service(outer_route_state)

@davidpdrsn davidpdrsn changed the title Add Router::provide_state and MethodRouter::provide_state Change Router::with_state and impl Service for Router<()> Nov 23, 2022
@davidpdrsn davidpdrsn marked this pull request as ready for review November 23, 2022 19:25
@davidpdrsn
Copy link
Member Author

@jplatte this is ready for review :)

Copy link
Contributor

@jimmycuadra jimmycuadra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really nice improvement! Great work! I found a couple small things you might want to change in the docs before merging.

axum/src/docs/routing/merge.md Show resolved Hide resolved
axum/src/docs/routing/nest.md Show resolved Hide resolved
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
axum/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from possible doc improvements, this seems good. I really like this solution to the state inheritance problem!

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 24, 2022 14:34
@davidpdrsn davidpdrsn added this to the 0.6 milestone Nov 24, 2022
@davidpdrsn davidpdrsn merged commit 0b26411 into main Nov 24, 2022
@davidpdrsn davidpdrsn deleted the provide-state branch November 24, 2022 14:43
@cooperwalbrun
Copy link

After reading the release notes and the discussion above, I am still not quite sure how to serve stateful routers. This results in a compilation error for me:

async fn serve(router: Router<MyState>) {
    let socket = SocketAddr::new(IpAddr::V4("0.0.0.0"), 1337);
    axum::Server::bind(&socket)
        .serve(router.into_make_service())
        .await
        .unwrap()
}

This is the error:

error[E0599]: the method `into_make_service` exists for struct `axum::Router<MyState>`, but its trait bounds were not satisfied
  --> src\main.rs:60:23
   |
60 |         .serve(router.into_make_service())
   |                       ^^^^^^^^^^^^^^^^^
   |
   |
64 | pub struct Router<S = (), B = Body> {
   | -----------------------------------
   | |
   | doesn't satisfy `_: axum::ServiceExt<_>`
   | doesn't satisfy `_: tower::Service<_>`
   |
   = note: the following trait bounds were not satisfied:
           `axum::Router<MyState>: tower::Service<_>`
           which is required by `axum::Router<MyState>: axum::ServiceExt<_>`
           `&axum::Router<MyState>: tower::Service<_>`
           which is required by `&axum::Router<MyState>: axum::ServiceExt<_>`
           `&mut axum::Router<MyState>: tower::Service<_>`
           which is required by `&mut axum::Router<MyState>: axum::ServiceExt<_>`

What am I missing here? Is the expectation that we need to implement these traits by hand if we use a custom state?

@jplatte
Copy link
Member

jplatte commented Nov 26, 2022

Your Router must have S = () before you can run it. As of the final release, with_state "erases" the state type of the router so you can assign the result of that call to a variable of type Router (all-default generic parameters) or Router<(), B> (with a custom body type) to then call .into_make_service() on it.

If you are still having trouble, please open a new Discussion instead of posting to a merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore Router::merge_service for merging routers with different states
4 participants