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

Add type safe state extractor #1155

Merged
merged 50 commits into from
Aug 17, 2022
Merged

Add type safe state extractor #1155

merged 50 commits into from
Aug 17, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Jul 11, 2022

This fixes #1142 by introducing a type safe State extractor. Attempting to
extract states that haven't seen set results in compile errors, rather than
runtime errors.

Example usage:

#[derive(Clone)]
struct AppState {}

async fn handler(
    State(app_state): State<AppState>,
) {
    ...
}

let app = Router::with_state(AppState { ... }).route("/", get(handler));

So the state is set with Router::with_state and extracted with State.

How it works

This works by changing Router::route to accept a MethodRouter, rather than
any Service. MethodRouter, FromRequest, RequestParts, and Handler then
all have a new type parameter for the state extracted with State.

All extractors just requires some generic state S but State fixes that to be
some specific type.

Most of the changes in this PR comes from having to add the new parameter
everywhere. I'm dreading all the merge conflicts but oh well...

Extracting substates

Substates can be extracted by implementing From:

#[derive(Clone)]
struct AppState {
    sub: Substate,
}

#[derive(Clone)]
struct Substate {}

impl From<AppState> for Substate { ... }

async fn handler(
    // this works even through we give an `AppState` to `Router::with_state`
    State(substate): State<Substate>,
) {
    ...
}

let app = Router::with_state(AppState { ... }).route("/", get(handler));

I think this gives most of the benefits of Extension while being type safe.

State defaults to ()

By default the state type param is (). So unless you're explicitly using state
you shouldn't have to worry about it:

// state is `()` and request body is `Body`
fn routes() -> Router {
    Router::new()
}

// same for `Handler` and `MethodRouter`

Note that S does not default to () on FromRequest and RequestParts. I'd be worried that library authors would accidentally bind their extractors to () rather than a generic S. So therefore these types requires specifying the state type.

MethodRouter and Handler as Service

Only Router accepts the state in its constructor so only it can implement
Service directly.

However MethodRouter and Handler needs the state provided to implement
Service. Providing that is done like so:

let method_router_service = get(|_: State<String>| async {})
    // we have to provide the state with `.with_state(...)`
    .with_state(String::new());

// this also implements `Service` since the state is `()`
let method_router_service = get(|| async {});

// same for handlers
let handler_service = (|_: State<String>| async {})
    // provide the state and get something that implements `Service`
    .with_state(String::new());

// there is also a method to get a `MakeService`
let handler_make_service = (|_: State<String>| async {})
    .with_state(String::new())
    .into_make_service();

let handler_service = (|| async {})
    // this method exists on an extension trait thats only implemented
    // for Handlers where the state is `()`
    .into_service();

Known limitations

No good way to provide the state automatically to nested Routers. You have to
something like this:

Router::with_state(state.clone()).nest("/api", Router::with_state(state));

Given all the discussions here I
think this is the best we can do.

Middleware cannot extract states since Router::layer just receives some L: Layer. It doesn't know anything about the state. We cannot pull the same trick
I did with Router::route since there are many ways to create Layers.

This setup might not be a great fit for optional state that might only exist in
some environments. I think those use cases are rare so IMO we can live with
this. Extension still works well for such use cases.

Same goes for routing to generic Services, there is no good way to access the
state. I think thats fine because. In my experience you're either routing to
handlers (which know about state) or Services like ServeDir which don't need
the state.

Follow-ups

These are follow-up improvements I intend to make once this is merged:

  • Support telling #[debug_handler] which state you use. Currently it has to
    assume you're using ().
  • Same goes for #[derive(FromRequest)].
  • Change cookie extractors to use get the signing key via a substate rather than
    extensions, since its required.
  • Write a new #[derive(State)] macro which adds the From impls for all
    fields in a struct and adds FromRequest impls that delegate to State. This
    should take care of all the boilerplate if you have a lot of state.
  • Think about allowing middlware to extract state via Extension.

@jplatte
Copy link
Member

jplatte commented Jul 19, 2022

I'm not a fan of this change. It increases complexity of both the API surface and the implementation while only serving some use cases. Notably, the key for SignedCookieJar still uses Extension and I don't think there's an easy way to migrate it to this new extractor.

@davidpdrsn
Copy link
Member Author

I think sharing state across all/most handlers is a very common use case. It's not specific to cookies and haven't thought about migrating that. But I still think it makes sense.

Have a look at all the examples. Those got way nicer and more type safe.

I do agree re adding more complexity but if/when we remove the request body type param I think that balances things out.

@jplatte
Copy link
Member

jplatte commented Jul 19, 2022

It's not specific to cookies and haven't thought about migrating that.

I didn't want to say cookies are a special case. In general, I feel like extractors that expect some sort of "configuration" in an extension currently are not going to be easy to integrate into this new pattern. The same applies to routes not tied to a particular service (e.g. a metrics route implementation in its own library), I think.

Have a look at all the examples. Those got way nicer and more type safe.

I had a look. They certainly did get more type safe, but how are they way nicer?

Thinking more about this, here is another pattern I've used before that wouldn't integrate well with this: I used to work on a webservice that integrated with a bunch of external services (i.e. stuff like S3). For some routes, having these configured was necessary for them to do their work. I set up the server such that setting specific env vars would add specific extensions to the app-level ServiceBuilder. This allowed me to do simple dev builds without spinning up loads of docker containers if I only wanted to test new functionality that didn't depend on external services; calling one of the routes that needed it would result in an internal server error but everything else worked fine.

@davidpdrsn
Copy link
Member Author

I actually think requiring the state used with private and signed cookies to implement From <AppState> for cookie::Key {} is kinda nice. That also avoids runtime errors because you cannot forget the key.

I had a look. They certainly did get more type safe, but how are they way nicer?

I meant the type safety made them way nicer 😛

Thinking more about this, here is another pattern I've used before that wouldn't integrate well with this

So you just wouldn't call those routes during development because the extensions would be missing?

Hm yeah not sure how to port that to this pattern 🤔 Maybe your state could have options for those configs that are Some if the env vars exist. And then an extractor for the configs that fails if they're None.

But extensions aren't going away either so you could also just keep doing what you did. Any solutions I've heard for checking that extensions exist quickly dissolve in hlists which I really wanna avoid.

Maybe if Router and FromRequest used () by default for the state? Then I don't think the changes here would even be visible outside of some new methods.

@jplatte
Copy link
Member

jplatte commented Jul 19, 2022

Maybe if Router and FromRequest used () by default for the state? Then I don't think the changes here would even be visible outside of some new methods.

Yeah, I guess this could be a good compromise. Though it does break my proposed FromRequestOnce design that doesn't need the weird dummy Mut / Once type param, which would be possible if we were to merge #1154 but not this. I admit this is a pretty weak motivation.

@jplatte
Copy link
Member

jplatte commented Jul 20, 2022

Nested routers can't inherit the state somehow, can they? That was actually something I considered a bit ugly when seeing it – fn admin_routes() -> Router becoming fn admin_routes(state: SharedState) -> Router<SharedState> in the key-value-store example.

@davidpdrsn
Copy link
Member Author

Yep that is annoying for sure. I did actually have a previous version where the state would be lazily applied so you didn't have to pass it around. But it was pretty complicated so I abandoned it.

@davidpdrsn davidpdrsn changed the title Router state extractor Add type safe state extractor Jul 26, 2022
axum-extra/src/routing/mod.rs Show resolved Hide resolved
axum-extra/src/routing/mod.rs Show resolved Hide resolved
axum/src/routing/mod.rs Show resolved Hide resolved
axum/src/routing/tests/mod.rs Outdated Show resolved Hide resolved
axum/src/routing/tests/mod.rs Outdated Show resolved Hide resolved
examples/testing/src/main.rs Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn mentioned this pull request Aug 17, 2022
* Add `FromRef` trait

* Remove unnecessary type params

* format

* fix docs link

* format examples
@davidpdrsn davidpdrsn enabled auto-merge (squash) August 17, 2022 15:09
@davidpdrsn davidpdrsn merged commit 423308d into main Aug 17, 2022
@davidpdrsn davidpdrsn deleted the router-state-extractor branch August 17, 2022 15:13
ttys3 added a commit to ttys3/static-server that referenced this pull request Aug 22, 2022
…to what get, post, etc accept). Use the new Router::fallback_service for setting any Service as the fallback (#1155)

Refs: tokio-rs/axum#1155
ttys3 added a commit to ttys3/static-server that referenced this pull request Aug 22, 2022
Comment on lines +204 to +205
//! You should prefer using [`State`] if possible since it's more type safe. The downside is that
//! its less dynamic than request extensions.
Copy link

@pickfire pickfire Oct 3, 2022

Choose a reason for hiding this comment

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

I was trying a similar experiment to have an AnyMap within State to allow using dynamic Extension that is not type-safe. But the issue with that is that the impl crashes due to impl FromRef<T> for State crashes with those specific implementations of FromRef. Here separating it out seemed to work quite well.

I believe this is also more performant since we no longer have to go through a HashMap of types (AnyMap) and can access the fields directly. Not sure if we want to write it down but I will try to do a benchmark and see what's the difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have a benchmark of extension vs state in the repo.

Comment on lines +71 to +76
/// // this impl tells `SignedCookieJar` how to access the key from our state
/// impl FromRef<AppState> for Key {
/// fn from_ref(state: &AppState) -> Self {
/// state.key.clone()
/// }
/// }
Copy link

@pickfire pickfire Oct 4, 2022

Choose a reason for hiding this comment

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

Is it a good idea to recommend implementing FromRef for an owned version that requires .clone() instead of a borrowed version? The FromRef implementations from the docs seemed to always have .clone().

Copy link
Member Author

Choose a reason for hiding this comment

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

How else would you do it?

Copy link

Choose a reason for hiding this comment

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

Maybe recommend the borrowed version like impl FromRef<AppState> for &Key in cases where clone is heavy and user does not need the owned version? Like show an example.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried that? I'm pretty damn sure it isn't going to work. The only way we could make borrowed extractors work is by adding a lifetime to FromRequest, which would be extra complexity for little practical benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah exactly. I don't think it'll actually work.

@wangluyi1982
Copy link

looking forward to the 0.6 release. this is wonderful!

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

Successfully merging this pull request may close these issues.

Missing extensions are runtime errors rather than compile errors
9 participants