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

Panic instead of silently discarding fallbacks #529

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Conversation

davidpdrsn
Copy link
Member

This introduces two new possible panics when constructing routers:

  • If merging two routers that each have a fallback. Previously that left
    side fallback would be silently discarded.
  • If nesting a router that has a fallback. Previously it would be
    silently discarded.

Overall this should make things more explicit and users shouldn't have
to worry "why isn't my fallback" working.

Fixes #488

@davidpdrsn davidpdrsn added this to the 0.4 milestone Nov 16, 2021
This introduces two new possible panics when constructing routers:

- If merging two routers that each have a fallback. Previously that left
  side fallback would be silently discarded.
- If nesting a router that has a fallback. Previously it would be
  silently discarded.

Overall this should make things more explicit and users shouldn't have
to worry "why isn't my fallback" working.

Fixes #488
@jplatte
Copy link
Member

jplatte commented Nov 18, 2021

If nesting a router that has a fallback.

Why is this? It should be possible to have a fallback for /foo/unrecognized that's different from /unrecognized, no?

@davidpdrsn
Copy link
Member Author

Its unfortunately no so straight forward with how nest is currently implemented:

If the service you're trying to nest is actually a Router then we simply clone all the routes from the nested router to the outer router. That means we end up one "flat" router in the end which removes pretty much all the complexity nesting otherwise leads to.

Adding a wildcard route like .route("/nest-path/*axum_fallback", nest_fallback) doesn't work since that conflicts with the nested routes.

@jplatte
Copy link
Member

jplatte commented Nov 18, 2021

Adding a wildcard route like .route("/nest-path/*axum_fallback", nest_fallback) doesn't work since that conflicts with the nested routes.

Oh, so while /foo and /:var don't conflict, /foo and /*wildcard do?

@davidpdrsn
Copy link
Member Author

davidpdrsn commented Nov 18, 2021

Yep. That was the source of the whole nest("/", ServeDir::new(...)) thing.

@davidpdrsn davidpdrsn merged commit 1d94d75 into main Nov 18, 2021
@davidpdrsn davidpdrsn deleted the panic-on-fallbacks branch November 18, 2021 21:19
EdorianDark pushed a commit to EdorianDark/axum that referenced this pull request Nov 24, 2021
This introduces two new possible panics when constructing routers:

- If merging two routers that each have a fallback. Previously that left
  side fallback would be silently discarded.
- If nesting a router that has a fallback. Previously it would be
  silently discarded.

Overall this should make things more explicit and users shouldn't have
to worry "why isn't my fallback" working.

Fixes tokio-rs#488
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.

Panic if merging two routers that each have a fallback
2 participants