-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add fallback inheritance for nested routers #1521
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
Conversation
use tower::Service; | ||
|
||
/// A [`Router`] converted into a [`Service`]. | ||
#[derive(Debug)] | ||
pub struct RouterService<B = Body> { | ||
routes: HashMap<RouteId, Route<B>>, | ||
node: Arc<Node>, | ||
fallback: Route<B>, | ||
fallback: FallbackRoute<B>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of this still being a thing at the service level (and using request extensions to make things work). Can we not copy the fallback handler to nested routers that don't have a custom one when building the RouterService
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I don't think so. When calling nest
we might not have the fallback yet
Router::new()
// nested Router added, no fallback yet
.nest(
"/foo",
Router::new(),
)
// now the fallback comes but the nested
// router has already been turned into a RouterService
.fallback(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we could store nested routers (that want to inherit state) differently from nested services, same as we already do for method routers vs. services, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost 😅 we get into trouble when we have to apply middleware to such nested routers. When applying layers to such routers we have to call Router::layer
, since they don't implement Service
so we cannot apply the layer around the whole thing like we do today. But doing that subtly changes the behavior:
Router::new()
.nest(
"/foo",
Router::new().route("/bar", ...),
)
.layer(log_url)
Here if /foo/bar
is called log_url
will see the URL with the /foo
prefix, as it should because its applied around the whole router.
We would instead end up doing something equivalent to calling Router::layer
on the inner router:
Router::new()
.nest(
"/foo",
Router::new().route("/bar", ...).layer(log_url),
)
But now log_url
will see the URL with /foo
removed, i.e. /bar
.
We also cannot store the layer and apply it later, since it changes the request body and introduces new generic parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I'm wondering if we can pull something similar to what we're doing for boxed handlers 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but failed 😞 Ran into some issues that I dunno how to resolve. See #1531
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards merging this as is. We can always refactor things later.
This together with #1532 might be the final release candidate 🤞
For the record we agreed to get this in now as is and refactor the internals later. See #1531. |
That was actually easier than I had feared 😅
Fixes #1416