Change Router::nest to flatten the routes - #1711
Merged
Merged
Conversation
davidpdrsn
commented
Jan 20, 2023
jplatte
reviewed
Jan 20, 2023
| debug_assert!(path.starts_with('/')); | ||
|
|
||
| if prefix.ends_with('/') { | ||
| format!("{prefix}{}", path.trim_start_matches('/')).into() |
Contributor
There was a problem hiding this comment.
Now I'm wondering how multiple successive slashes behave elsewhere 🤔
Member
Author
There was a problem hiding this comment.
Like if you do .nest("////", _)?
Contributor
There was a problem hiding this comment.
Yeah, that or .nest("///foo/", _).
Member
Author
There was a problem hiding this comment.
You've nerd sniped me 😜 I'll make a note and do some tests.
davidpdrsn
marked this pull request as ready for review
April 7, 2023 19:43
Member
Author
|
@jplatte I finally got fallback inheritance working properly 🎉 |
jplatte
reviewed
Apr 11, 2023
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn can_extract_nested_matched_path_in_middleware_via_extension_using_nest() { |
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
jplatte
approved these changes
Apr 11, 2023
yanns
reviewed
Apr 11, 2023
| } | ||
|
|
||
| fn call(&mut self, _req: Request<B>) -> Self::Future { | ||
| println!("NotFound hit"); |
Member
Author
There was a problem hiding this comment.
It should not 😅 removed in v0.6.14
davidpdrsn
added a commit
that referenced
this pull request
Apr 14, 2023
With the introduction of `PathRouter` in #1711 I forgot that fallbacks shouldn't be able to extract `MatchedPath`. The extension for it was interested regardless if the `PathRouter` was used as a fallback or not. It doesn't make sense to extract `MatchedPath` in a fallback because there was no matched route. Turns out it also fixes a panic with a specifical fallback/nest setup. Fixes #1931
davidpdrsn
added a commit
that referenced
this pull request
Apr 14, 2023
With the introduction of `PathRouter` in #1711 I forgot that fallbacks shouldn't be able to extract `MatchedPath`. The extension for it was interested regardless if the `PathRouter` was used as a fallback or not. It doesn't make sense to extract `MatchedPath` in a fallback because there was no matched route. Turns out it also fixes a panic with a specifical fallback/nest setup. Fixes #1931
|
Thanks for this! I think this is responsible for fixing an issue we had where the telemetry we had for nested routers didn't get their |
Member
Author
|
Yep that most likely #1441 |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes
Router::nestto "flatten" the routes. By flattening I mean justmoving the routes from one router to another by calling
.route(nest_path, nested_endpoint). That means there aren't anyRoutersnested inside each other and instead we just have one flattened
Routerwhichperforms better.
The main tricky part was fallback inheritance. The main reason we didn't flatten
routes in 0.6 previously was specifically because of fallback inheritance.
The solution I went for here was to make an internal
PathRoutertype. It hasmost routing functions from
Router, except fallbacks. ARouterthencontains two
PathRouters: One for the actual routes and another forfallbacks. Then when you add a fallback to the root router that adds
/and/*routes to the fallbackPathRouter. Nesting is then pretty straightforward: We just nest the routes from each
PathRouter.Fixes #1624
Fixes #1441
TODO
MatchedPathDon't believe there are any docs changes to make.