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

PoC change behavior of nesting with regards to trailing slashes and root route #2730

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions axum/src/routing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ macro_rules! panic_on_err {
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub(crate) struct RouteId(u32);

// TODO Docs - in short sets where a root route (`/`) of a nested router will be available.
// For example if a router is nested at `/foo`, the root route can be available at `/foo`, at `/foo/`, or at both locations.
// We could also extend this with variants like `PermanentRedirectToEmpty` and `PermanentRedirectToSlash`.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum NestedRootRouteBehavior {
OnlyEmpty,
OnlySlash,
EmptyAndSlash,
}

/// The router type for composing handlers and services.
#[must_use]
pub struct Router<S = ()> {
Expand Down Expand Up @@ -187,6 +197,11 @@ where
#[doc = include_str!("../docs/routing/nest.md")]
#[track_caller]
pub fn nest(self, path: &str, router: Router<S>) -> Self {
self.nest_with_root(path, router, NestedRootRouteBehavior::EmptyAndSlash)
}


pub fn nest_with_root(self, path: &str, router: Router<S>, root_behavior: NestedRootRouteBehavior) -> Self {
let RouterInner {
path_router,
fallback_router,
Expand All @@ -198,10 +213,12 @@ where
} = router.into_inner();

self.tap_inner_mut(|this| {
panic_on_err!(this.path_router.nest(path, path_router));
panic_on_err!(this.path_router.nest(path, path_router, root_behavior));

if !default_fallback {
panic_on_err!(this.fallback_router.nest(path, fallback_router));
panic_on_err!(this
.fallback_router
.nest(path, fallback_router, root_behavior));
}
})
}
Expand Down
65 changes: 41 additions & 24 deletions axum/src/routing/path_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use tower_service::Service;

use super::{
future::RouteFuture, not_found::NotFound, strip_prefix::StripPrefix, url_params, Endpoint,
MethodRouter, Route, RouteId, FALLBACK_PARAM_PATH, NEST_TAIL_PARAM,
MethodRouter, NestedRootRouteBehavior, Route, RouteId, FALLBACK_PARAM_PATH, NEST_TAIL_PARAM,
};

pub(super) struct PathRouter<S, const IS_FALLBACK: bool> {
Expand Down Expand Up @@ -161,6 +161,7 @@ where
&mut self,
path_to_nest_at: &str,
router: PathRouter<S, IS_FALLBACK>,
root_behavior: NestedRootRouteBehavior,
) -> Result<(), Cow<'static, str>> {
let prefix = validate_nest_path(path_to_nest_at);

Expand All @@ -176,18 +177,18 @@ where
.get(&id)
.expect("no path for route id. This is a bug in axum. Please file an issue");

let path = path_for_nested_route(prefix, inner_path);

let layer = (
StripPrefix::layer(prefix),
SetNestedPath::layer(path_to_nest_at),
);
match endpoint.layer(layer) {
Endpoint::MethodRouter(method_router) => {
self.route(&path, method_router)?;
}
Endpoint::Route(route) => {
self.route_endpoint(&path, Endpoint::Route(route))?;
for path in paths_for_nested_route(prefix, inner_path, root_behavior) {
let layer = (
StripPrefix::layer(prefix),
SetNestedPath::layer(path_to_nest_at),
);
match endpoint.clone().layer(layer) {
Endpoint::MethodRouter(method_router) => {
self.route(&path, method_router)?;
}
Endpoint::Route(route) => {
self.route_endpoint(&path, Endpoint::Route(route))?;
}
}
}
}
Expand All @@ -208,11 +209,7 @@ where
let path = validate_nest_path(path_to_nest_at);
let prefix = path;

let path = if path.ends_with('/') {
format!("{path}*{NEST_TAIL_PARAM}")
} else {
format!("{path}/*{NEST_TAIL_PARAM}")
};
let path = format!("{path}/*{NEST_TAIL_PARAM}");

let layer = (
StripPrefix::layer(prefix),
Expand Down Expand Up @@ -470,18 +467,38 @@ fn validate_nest_path(path: &str) -> &str {
panic!("Invalid route: nested routes cannot contain wildcards (*)");
}

if path.ends_with('/') {
panic!("Invalid route: nested routes cannot end with `/`");
}

path
}

pub(crate) fn path_for_nested_route<'a>(prefix: &'a str, path: &'a str) -> Cow<'a, str> {
/// This returns all paths that should be registered for a given prefix and
/// path. This usually returns exactly one path, but may return two for root
/// path with [`RootRouteBehavior::EmptyAndSlash`].
pub(crate) fn paths_for_nested_route<'a>(
prefix: &'a str,
path: &'a str,
root_behavior: NestedRootRouteBehavior,
) -> impl Iterator<Item = Cow<'a, str>> {
debug_assert!(prefix.starts_with('/'));
debug_assert!(!prefix.ends_with('/'));
debug_assert!(path.starts_with('/'));

if prefix.ends_with('/') {
format!("{prefix}{}", path.trim_start_matches('/')).into()
} else if path == "/" {
prefix.into()
if path == "/" {
match root_behavior {
NestedRootRouteBehavior::OnlyEmpty => Some(prefix.into()).into_iter().chain(None),
NestedRootRouteBehavior::OnlySlash => {
Some(format!("{prefix}/").into()).into_iter().chain(None)
}
NestedRootRouteBehavior::EmptyAndSlash => Some(prefix.into())
.into_iter()
.chain(Some(format!("{prefix}/").into())),
}
} else {
format!("{prefix}{path}").into()
Some(format!("{prefix}{path}").into())
.into_iter()
.chain(None)
}
}
30 changes: 15 additions & 15 deletions axum/src/routing/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
response::{IntoResponse, Response},
routing::{
delete, get, get_service, on, on_service, patch, patch_service,
path_router::path_for_nested_route, post, MethodFilter,
path_router::paths_for_nested_route, post, MethodFilter, NestedRootRouteBehavior,
},
test_helpers::{
tracing_helpers::{capture_tracing, TracingEvent},
Expand Down Expand Up @@ -888,20 +888,20 @@ fn method_router_fallback_with_state() {
.with_state(state);
}

#[test]
fn test_path_for_nested_route() {
assert_eq!(path_for_nested_route("/", "/"), "/");

assert_eq!(path_for_nested_route("/a", "/"), "/a");
assert_eq!(path_for_nested_route("/", "/b"), "/b");
assert_eq!(path_for_nested_route("/a/", "/"), "/a/");
assert_eq!(path_for_nested_route("/", "/b/"), "/b/");

assert_eq!(path_for_nested_route("/a", "/b"), "/a/b");
assert_eq!(path_for_nested_route("/a/", "/b"), "/a/b");
assert_eq!(path_for_nested_route("/a", "/b/"), "/a/b/");
assert_eq!(path_for_nested_route("/a/", "/b/"), "/a/b/");
}
// #[test]
// fn test_path_for_nested_route() {
// assert_eq!(path_for_nested_route("/", "/"), "/");

// assert_eq!(path_for_nested_route("/a", "/"), "/a");
// assert_eq!(path_for_nested_route("/", "/b"), "/b");
// assert_eq!(path_for_nested_route("/a/", "/"), "/a/");
// assert_eq!(path_for_nested_route("/", "/b/"), "/b/");

// assert_eq!(path_for_nested_route("/a", "/b"), "/a/b");
// assert_eq!(path_for_nested_route("/a/", "/b"), "/a/b");
// assert_eq!(path_for_nested_route("/a", "/b/"), "/a/b/");
// assert_eq!(path_for_nested_route("/a/", "/b/"), "/a/b/");
// }

#[crate::test]
async fn state_isnt_cloned_too_much() {
Expand Down
84 changes: 84 additions & 0 deletions axum/src/routing/tests/nest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,90 @@ async fn nest_with_and_without_trailing() {
assert_eq!(res.status(), StatusCode::OK);
}

#[crate::test]
async fn nest_root_default_behavior() {
// Default behavior -- is the same as before
let app = Router::new().nest("/foo", Router::new().route("/", get(|| async {})));

let client = TestClient::new(app);

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("/foo/").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
}

#[tokio::test]
async fn nest_root_behavior() {
{
let app = Router::new().nest_with_root(
"/foo",
Router::new().route("/", get(|| async {})),
NestedRootRouteBehavior::OnlyEmpty,
);

let client = TestClient::new(app);

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("/foo/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/foo//").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
}
{
let app = Router::new().nest_with_root(
"/foo",
Router::new().route("/", get(|| async {})),
NestedRootRouteBehavior::OnlySlash,
);

let client = TestClient::new(app);

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/foo/").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/foo//").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
}
{
let app = Router::new().nest_with_root(
"/foo",
Router::new().route("/", get(|| async {})),
NestedRootRouteBehavior::EmptyAndSlash,
);

let client = TestClient::new(app);

let res = client.get("/foo").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("/foo/").await;
assert_eq!(res.status(), StatusCode::OK);

let res = client.get("/").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/foo//").await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
}
}

#[tokio::test]
async fn nesting_with_root_inner_router() {
let app = Router::new()
Expand Down
Loading