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

Update matchit and fix nesting inconsistencies #1086

Merged
merged 23 commits into from Aug 11, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Jun 11, 2022

While updating to the new matchit I ran into a bunch of inconsistencies regarding nest. Specifically cases where nesting a Router or an opaque service had different behavior. That is also something people have asked about on discord before.

This addresses that by always nesting services opaque in nest, i.e. we don't attempt to downcast it to a Router and change the behavior based on that. This makes the behavior more consistent and simplifies the implementation.


Old, outdated, description:

While updating to the new matchit I ran into a bunch of inconsistencies regarding nest. Specifically cases where nesting a Router or an opaque service had different behavior. That is also something people have asked about on discord before.

This addresses that by splitting nest into

  • nest: Which only accepts Routers.
  • nest_service: Which accepts any opaque Service.

Once this is merged I'm gonna change fallback to work how I described in #1053 (comment). If we didn't add nest_service that would introduce yet another inconsistency so wanted to get this in first.

@davidpdrsn davidpdrsn added breaking change A PR that makes a breaking change. A-axum labels Jun 11, 2022
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
@davidpdrsn davidpdrsn force-pushed the axum-next branch 2 times, most recently from f0b6e1d to 415965e Compare June 25, 2022 10:27
@davidpdrsn davidpdrsn added this to the 0.6 milestone Jun 25, 2022
@davidpdrsn davidpdrsn force-pushed the axum-next branch 3 times, most recently from f6dbabe to f6bab0f Compare June 28, 2022 19:35
Base automatically changed from axum-next to main June 28, 2022 20:07
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
@davidpdrsn
Copy link
Member Author

Soon I'll do a more thorough PR description+comments that details the changes a bit better.

@davidpdrsn davidpdrsn marked this pull request as ready for review July 4, 2022 16:56
@davidpdrsn
Copy link
Member Author

@jplatte This is ready for review now. I hope the docs and changelog makes the changes clear but let me know if you have questions :)

@davidpdrsn davidpdrsn removed the breaking change A PR that makes a breaking change. label Jul 13, 2022
axum-extra/CHANGELOG.md Outdated Show resolved Hide resolved
axum/CHANGELOG.md Outdated Show resolved Hide resolved
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
@davidpdrsn
Copy link
Member Author

davidpdrsn commented Jul 26, 2022

I've been thinking about this and believe the right move is not delegate to fallbacks to outer Routers for nested services (like mentioned here). This makes use cases like @lilyball describes more annoying but I believe its the right move overall as it makes nesting more consistent.

If you're nesting a lot of Routers and want the same fallbacks on all of them I think writing something like this isn't too bad

fn router() -> Router {
    Router::new().fallback(...)
}

fn make_some_routes() {
    router().route("/", ...)
}

I've made the changes here and will fix the merge conflicts now so we can start the review process.

axum-extra/src/routing/resource.rs Show resolved Hide resolved
axum/CHANGELOG.md Show resolved Hide resolved
axum/CHANGELOG.md Show resolved Hide resolved
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
axum/src/docs/routing/nest.md Outdated Show resolved Hide resolved
axum/src/extract/matched_path.rs Outdated Show resolved Hide resolved
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
axum/src/routing/mod.rs Outdated Show resolved Hide resolved
@lilyball
Copy link

I've been thinking about this and believe the right move is not delegate to fallbacks to outer Routers for nested services (like mentioned here). This makes use cases like @lilyball describes more annoying but I believe its the right move overall as it makes nesting more consistent.

I still think this is a footgun, where people are going to nest routers and then configure a root fallback and forget to configure the same fallback on every level. Even if they make a fn route() -> Router they may simply forget to use it in one of the nested routers. The expectation of a fn route() -> Router also makes it more complicated to do things like move the nested router generation into other modules.

This also means that splitting up your route definitions into nested groups means it doesn't merge all the routes into a single route set anymore, which was previously being done as an "optimization" and so I assume there is in fact a measurable impact there.

I really don't like that this new design means the best performance is to skip nesting routers and instead set a bunch of routes with identical prefixes.

Have you given any thought to having a separate RouteSet type that can have routes added to it like a Router but no fallback? Then Router can have a nest_routes() method that does the same merging of routes into the same route set that it was doing when nesting routers before, and there's no complication over how to handle fallback behavior.

@davidpdrsn
Copy link
Member Author

I really don't like that this new design means the best performance is to skip nesting routers and instead set a bunch of routes with identical prefixes.

I'm fine with that. I still think it's the right trade-off.

Have you given any thought to having a separate RouteSet type that can have routes added to it like a Router but no fallback? Then Router can have a nest_routes() method that does the same merging of routes into the same route set that it was doing when nesting routers before, and there's no complication over how to handle fallback behavior

I have not but like to not complicate the routing more than it is already. The purpose of those is make nesting simpler so adding more methods goes against that.

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.

None yet

3 participants