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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fallback inheritance for nested routers #1521

Merged
merged 4 commits into from Nov 18, 2022
Merged

Conversation

davidpdrsn
Copy link
Member

That was actually easier than I had feared 馃槄

Fixes #1416

@davidpdrsn davidpdrsn added this to the 0.6 milestone Nov 11, 2022
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>,
Copy link
Member

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?

Copy link
Member Author

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(...)

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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 馃

Copy link
Member Author

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

Copy link
Member Author

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 馃

@davidpdrsn
Copy link
Member Author

For the record we agreed to get this in now as is and refactor the internals later. See #1531.

@davidpdrsn davidpdrsn enabled auto-merge (squash) November 18, 2022 10:21
@davidpdrsn davidpdrsn merged commit 7090649 into main Nov 18, 2022
@davidpdrsn davidpdrsn deleted the fallback-inheritance branch November 18, 2022 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out fallback inheritance
2 participants