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

Type safe state inheritance #1532

Merged
merged 16 commits into from Nov 18, 2022
Merged

Type safe state inheritance #1532

merged 16 commits into from Nov 18, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Nov 15, 2022

Lately I've been wanting to explore a more type safe way to passing state to routers, that don't run things like #1516. It think its a little unfortunate that axum 0.6's headline feature is type safe state, only for inherit_state to not actually be type safe.

This changes the API such that Router::with_state requires the state which is then passed to everything when building the final RouterService:

let service: RouterService = Router::new()
    .route(...)
    .with_state(state);

The upside of this approach is that we don't need additional type parameters or runtime stuff to track whether the state has been supplied or not.

Nesting (and merging) is only possible with a Router which doesn't have the state so it becomes more type safe. You can still use nest_service but that requires turning the Router into a RouterService by supplying the state.

@jplatte what do you think?

Fixes #1516

TODO

  • Update docs
  • Changelog

@davidpdrsn davidpdrsn added this to the 0.6 milestone Nov 15, 2022
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Nov 15, 2022

You can still use nest_service but that requires turning the Router into a RouterService by supplying the state.

I think the reason I didn't do it this way is that you then can't have fallback inheritance for nested routers that have a different state type.

@davidpdrsn
Copy link
Member Author

I think the reason I didn't do it this way is that you then can't have fallback inheritance for nested routers that have a different state type.

Yeah I ran into that here. I changed nest and merge to require the same state type. I think thats an okay compromise. I think I'd rather have type safety.

@davidpdrsn davidpdrsn marked this pull request as ready for review November 17, 2022 12:41
@davidpdrsn davidpdrsn changed the title Make state type safe by setting is as the last thing Type safe sate inheritance Nov 17, 2022
@davidpdrsn davidpdrsn changed the title Type safe sate inheritance Type safe state inheritance Nov 17, 2022
@jplatte
Copy link
Member

jplatte commented Nov 18, 2022

Well, I still somewhat strongly prefer the previous compromise because while you could be surprised about that too, the surprise with nesting a RouterService and expecting fallback inheritance to work same as with nesting a Router can happen while the service is already in prod. That's not the case with the panic from attempting to turn an inheriting Router into a service, whether directly or indirectly.

axum/CHANGELOG.md Outdated Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Nov 18, 2022

We discussed this on Discord (privately, sorry) and because of how state inheritance is implemented now, this can't actually cause the problem that I was worried about.

I guess it makes it impossible to merge two routers with different state though? Maybe that's fine. It's at least not something that you notice when the service is already running 🙂

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 18, 2022 10:58
@davidpdrsn davidpdrsn merged commit 64960bb into main Nov 18, 2022
@davidpdrsn davidpdrsn deleted the provide-router-state-late branch November 18, 2022 11:03
@jplatte
Copy link
Member

jplatte commented Nov 18, 2022

The changelog for this is very weird, I only noticed now. There is a double : and it talks about inherit_fallback and that you should use with_state (rather than just new).

@davidpdrsn
Copy link
Member Author

@jplatte oh good catch! Fixed in 2e3000f

jimmycuadra added a commit to jimmycuadra/axum that referenced this pull request Nov 19, 2022
I was going to improve the discoverability of docs for how state works
with `Router::merge` and `Router::nest` based on my comment in tokio-rs#1313,
but tokio-rs#1532 removes `Router::inherit_state` so I discovered that the
change I originally suggested isn't necessary anymore.

I decided to make a few other minor documentation fixes while I was
reading, however:

* Corrected the docs for `Router::nest` to say that it nests another
  `Router` rather than a `Service`. There is a separate function
  (`Router::nest_service`) for the latter.
* Changed one of the headings in nest.md to use more idiomatic English.
* Changed awkward phrasing under the "Sharing state with handlers"
  heading of the root documentation page.
* Removed a trailing period from one of three list items for
  consistency.
@zvkemp
Copy link

zvkemp commented Nov 25, 2022

This change seems to have broken into_make_service() for routers not using the default state; was that decision was intentional? As IntoMakeService::new() is crate-private, it's unclear how we are now supposed to serve routers with a custom state.

@davidpdrsn
Copy link
Member Author

@zvkemp it didn't. You gotta call Router::with_state to provide the state. See the examples in the docs or in the repo.

@zvkemp
Copy link

zvkemp commented Nov 26, 2022

Yes, we are calling with_state (the newer version as of 0.6.0.rc5).

This method (as well as into_make_service_with_connect_info) is only implemented on Router<(), B>, not Router<S, B> as it was previously.

https://docs.rs/axum/0.6.0/axum/routing/struct.Router.html#method.into_make_service

@zvkemp
Copy link

zvkemp commented Nov 26, 2022

Oh actually I think I understand what is happening — I didn't realize that the signature of with_state doesn't specify that the state argument will become the router's state, and we had a type hint specifying our router would be built as a Router<MyState, _> rather than the required Router<(), _> for it to be a Service. Removing that type hint fixes the issue.

@jplatte
Copy link
Member

jplatte commented Nov 26, 2022

Exactly. Next time if you have an issue though, please open a discussion with a reproducible example 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't turn a Router that wants to inherit state into a service
3 participants