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

Update matchit and fix nesting inconsistencies #1086

Merged
merged 23 commits into from Aug 11, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2ddd0a2
Break `Router::nest` into `nest` and `nest_service` methods
davidpdrsn Jun 11, 2022
23721f2
Merge branch 'main' into separate-nesting-opaque-services
davidpdrsn Jun 29, 2022
2c54bb5
fix doc tests
davidpdrsn Jun 29, 2022
014067e
update docs
davidpdrsn Jun 29, 2022
79daaf0
fix
davidpdrsn Jun 29, 2022
276a950
Only accept `Router` in `Resource::{nest, nest_collection}`
davidpdrsn Jun 29, 2022
b220778
update changelog
davidpdrsn Jun 29, 2022
fb5219f
fix docs
davidpdrsn Jun 29, 2022
657a089
fix `MatchedPath` with `Router`s nested with `nest_service`
davidpdrsn Jun 29, 2022
bbf1852
Apply suggestions from code review
davidpdrsn Jun 29, 2022
7ed35d2
Merge branch 'main' into separate-nesting-opaque-services
davidpdrsn Jun 29, 2022
ca95dd6
Merge branch 'main' into separate-nesting-opaque-services
davidpdrsn Jul 3, 2022
581b0d9
adjust docs for fallbacks
davidpdrsn Jul 4, 2022
f18df8a
Always nest services as opaque
davidpdrsn Jul 26, 2022
a8b0608
Merge branch 'main' into separate-nesting-opaque-services
davidpdrsn Jul 26, 2022
b166548
fix old docs reference
davidpdrsn Jul 26, 2022
4630476
more tests for `MatchedPath` with nested handlers
davidpdrsn Jul 26, 2022
f6db084
Merge branch 'main' into separate-nesting-opaque-services
davidpdrsn Jul 26, 2022
32e67aa
minor clean up
davidpdrsn Aug 11, 2022
4c39785
Merge branch 'main' into separate-nesting-opaque-services
davidpdrsn Aug 11, 2022
be864ea
use identifier captures in format strings
davidpdrsn Aug 11, 2022
171171d
Apply suggestions from code review
davidpdrsn Aug 11, 2022
35c616d
fix test
davidpdrsn Aug 11, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions axum-extra/CHANGELOG.md
Expand Up @@ -9,10 +9,14 @@ and this project adheres to [Semantic Versioning].

- **added:** Add `RouterExt::route_with_tsr` for adding routes with an
additional "trailing slash redirect" route ([#1119])
- **breaking:** `Resource::nest` and `Resource::nest_collection` has been
removed. You can instead convert the `Resource` into a `Router` and
add additional routes as necessary ([#1086])
- **changed:** For methods that accept some `S: Service`, the bounds have been
relaxed so the response type must implement `IntoResponse` rather than being a
literal `Response`

[#1086]: https://github.com/tokio-rs/axum/pull/1086
[#1119]: https://github.com/tokio-rs/axum/pull/1119

# 0.3.5 (27. June, 2022)
Expand Down
60 changes: 2 additions & 58 deletions axum-extra/src/routing/resource.rs
Expand Up @@ -31,18 +31,7 @@ use tower_service::Service;
/// // `PUT or PATCH /users/:users_id`
/// .update(|Path(user_id): Path<u64>| async {})
/// // `DELETE /users/:users_id`
/// .destroy(|Path(user_id): Path<u64>| async {})
/// // Nest another router at the "member level"
/// // This defines a route for `GET /users/:users_id/tweets`
/// .nest(Router::new().route(
/// "/tweets",
/// get(|Path(user_id): Path<u64>| async {}),
/// ))
/// // Nest another router at the "collection level"
/// // This defines a route for `GET /users/featured`
/// .nest_collection(
/// Router::new().route("/featured", get(|| async {})),
/// );
/// .destroy(|Path(user_id): Path<u64>| async {});
///
/// let app = Router::new().merge(users);
/// # let _: Router<axum::body::Body> = app;
Expand Down Expand Up @@ -136,34 +125,6 @@ where
self.route(&path, delete(handler))
}

/// Nest another route at the "member level".
///
/// The routes will be nested at `/{resource_name}/:{resource_name}_id`.
pub fn nest<T>(mut self, svc: T) -> Self
jplatte marked this conversation as resolved.
Show resolved Hide resolved
where
T: Service<Request<B>, Error = Infallible> + Clone + Send + 'static,
T::Response: IntoResponse,
T::Future: Send + 'static,
{
let path = self.show_update_destroy_path();
self.router = self.router.nest(&path, svc);
self
}

/// Nest another route at the "collection level".
///
/// The routes will be nested at `/{resource_name}`.
pub fn nest_collection<T>(mut self, svc: T) -> Self
where
T: Service<Request<B>, Error = Infallible> + Clone + Send + 'static,
T::Response: IntoResponse,
T::Future: Send + 'static,
{
let path = self.index_create_path();
self.router = self.router.nest(&path, svc);
self
}

fn index_create_path(&self) -> String {
format!("/{}", self.name)
}
Expand Down Expand Up @@ -214,14 +175,7 @@ mod tests {
.show(|Path(id): Path<u64>| async move { format!("users#show id={}", id) })
.edit(|Path(id): Path<u64>| async move { format!("users#edit id={}", id) })
.update(|Path(id): Path<u64>| async move { format!("users#update id={}", id) })
.destroy(|Path(id): Path<u64>| async move { format!("users#destroy id={}", id) })
.nest(Router::new().route(
"/tweets",
get(|Path(id): Path<u64>| async move { format!("users#tweets id={}", id) }),
))
.nest_collection(
Router::new().route("/featured", get(|| async move { "users#featured" })),
);
.destroy(|Path(id): Path<u64>| async move { format!("users#destroy id={}", id) });

let mut app = Router::new().merge(users);

Expand Down Expand Up @@ -264,16 +218,6 @@ mod tests {
call_route(&mut app, Method::DELETE, "/users/1").await,
"users#destroy id=1"
);

assert_eq!(
call_route(&mut app, Method::GET, "/users/1/tweets").await,
"users#tweets id=1"
);

assert_eq!(
call_route(&mut app, Method::GET, "/users/featured").await,
"users#featured"
);
}

async fn call_route(app: &mut Router, method: Method, uri: &str) -> String {
Expand Down
10 changes: 10 additions & 0 deletions axum/CHANGELOG.md
Expand Up @@ -12,6 +12,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
Use `axum::middleware::from_extractor` instead ([#1077])
- **breaking:** Allow `Error: Into<Infallible>` for `Route::{layer, route_layer}` ([#924])
- **breaking:** `MethodRouter` now panics on overlapping routes ([#1102])
- **breaking:** Nested `Router`s will no longer delegate to the outer `Router`'s
fallback. Instead you must explicitly set a fallback on the inner `Router` ([#1086])
jplatte marked this conversation as resolved.
Show resolved Hide resolved
- **breaking:** The request `/foo/` no longer matches `/foo/*rest`. If you want
to match `/foo/` you have to add a route specifically for that ([#1086])
- **breaking:** Path params for wildcard routes no longer include the prefix
`/`. e.g. `/foo.js` will match `/*filepath` with a value of `foo.js`, _not_
`/foo.js` ([#1086])
davidpdrsn marked this conversation as resolved.
Show resolved Hide resolved
- **fixed:** Routes like `/foo` and `/*rest` are no longer considered
overlapping. `/foo` will take priority ([#1086])
- **breaking:** Remove trailing slash redirects. Previously if you added a route
for `/foo`, axum would redirect calls to `/foo/` to `/foo` (or vice versa for
`/foo/`). That is no longer supported and such requests will now be sent to
Expand All @@ -35,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

[#1171]: https://github.com/tokio-rs/axum/pull/1171
[#1077]: https://github.com/tokio-rs/axum/pull/1077
[#1086]: https://github.com/tokio-rs/axum/pull/1086
[#1088]: https://github.com/tokio-rs/axum/pull/1088
[#1102]: https://github.com/tokio-rs/axum/pull/1102
[#1119]: https://github.com/tokio-rs/axum/pull/1119
Expand Down
2 changes: 1 addition & 1 deletion axum/Cargo.toml
Expand Up @@ -34,7 +34,7 @@ http = "0.2.5"
http-body = "0.4.4"
hyper = { version = "0.14.14", features = ["server", "tcp", "stream"] }
itoa = "1.0.1"
matchit = "0.5.0"
matchit = "0.6"
memchr = "2.4.1"
mime = "0.3.16"
percent-encoding = "2.1"
Expand Down
94 changes: 60 additions & 34 deletions axum/src/docs/routing/nest.md
@@ -1,4 +1,4 @@
Nest a group of routes (or a [`Service`]) at some path.
Nest a [`Service`] at some path.

This allows you to break your application into smaller pieces and compose
them together.
Expand Down Expand Up @@ -64,36 +64,6 @@ let app = Router::new().nest("/:version/api", users_api);
# };
```

# Nesting services

`nest` also accepts any [`Service`]. This can for example be used with
[`tower_http::services::ServeDir`] to serve static files from a directory:

```rust
use axum::{
Router,
routing::get_service,
http::StatusCode,
error_handling::HandleErrorLayer,
};
use std::{io, convert::Infallible};
use tower_http::services::ServeDir;

// Serves files inside the `public` directory at `GET /public/*`
let serve_dir_service = get_service(ServeDir::new("public"))
.handle_error(|error: io::Error| async move {
(
StatusCode::INTERNAL_SERVER_ERROR,
format!("Unhandled internal error: {}", error),
)
});

let app = Router::new().nest("/public", serve_dir_service);
# async {
# axum::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap();
# };
```

# Differences to wildcard routes

Nested routes are similar to wildcard routes. The difference is that
Expand All @@ -103,18 +73,73 @@ the prefix stripped:
```rust
use axum::{routing::get, http::Uri, Router};

let nested_router = Router::new()
.route("/", get(|uri: Uri| async {
// `uri` will _not_ contain `/bar`
}));

let app = Router::new()
.route("/foo/*rest", get(|uri: Uri| async {
// `uri` will contain `/foo`
}))
.nest("/bar", get(|uri: Uri| async {
// `uri` will _not_ contain `/bar`
}));
.nest("/bar", nested_router);
# async {
# axum::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap();
# };
```

# Fallbacks

If a request matches the prefix but the nested router doesn't have matching
davidpdrsn marked this conversation as resolved.
Show resolved Hide resolved
route the outer fallback will _not_ be called:
davidpdrsn marked this conversation as resolved.
Show resolved Hide resolved

```rust
use axum::{routing::get, http::StatusCode, handler::Handler, Router};

async fn fallback() -> (StatusCode, &'static str) {
(StatusCode::NOT_FOUND, "Not Found")
}

let api_routes = Router::new().nest("/users", get(|| async {}));

let app = Router::new()
.nest("/api", api_routes)
.fallback(fallback.into_service());
# let _: Router = app;
```

Here requests like `GET /api/not-found` will go into `api_routes` and then to
the fallback of `api_routes` which will return an empty `404 Not Found`
response. The outer fallback declared on `app` will _not_ be called.

Think of nested services as swallowing requests that matches the prefix and
not falling back to outer router even if they don't have a matching route.

You can still add separate fallbacks to nested routers:

```rust
use axum::{routing::get, http::StatusCode, handler::Handler, Json, Router};
use serde_json::{json, Value};

async fn fallback() -> (StatusCode, &'static str) {
(StatusCode::NOT_FOUND, "Not Found")
}

async fn api_fallback() -> (StatusCode, Json<Value>) {
(StatusCode::NOT_FOUND, Json(json!({ "error": "Not Found" })))
}

let api_routes = Router::new()
.nest("/users", get(|| async {}))
// add dedicated fallback for requests starting with `/api`
.fallback(api_fallback.into_service());

let app = Router::new()
.nest("/api", api_routes)
.fallback(fallback.into_service());
# let _: Router = app;
```

# Panics

- If the route overlaps with another route. See [`Router::route`]
Expand All @@ -125,3 +150,4 @@ for more details.
`Router` only allows a single fallback.

[`OriginalUri`]: crate::extract::OriginalUri
[fallbacks]: Router::fallback
18 changes: 2 additions & 16 deletions axum/src/docs/routing/route.md
Expand Up @@ -51,6 +51,8 @@ Examples:
- `/:id/:repo/*tree`

Wildcard captures can also be extracted using [`Path`](crate::extract::Path).
Note that the leading slash is not included, i.e. for the route `/foo/*rest` and
the path `/foo/bar/baz` the value of `rest` will be `bar/baz`.

# Accepting multiple methods

Expand Down Expand Up @@ -184,22 +186,6 @@ let app = Router::new()
The static route `/foo` and the dynamic route `/:key` are not considered to
overlap and `/foo` will take precedence.

Take care when using [`Router::nest`] as it behaves like a wildcard route.
Therefore this setup panics:

```rust,should_panic
use axum::{routing::get, Router};

let app = Router::new()
// this is similar to `/api/*`
.nest("/api", get(|| async {}))
// which overlaps with this route
.route("/api/users", get(|| async {}));
# async {
# axum::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap();
# };
```

Also panics if `path` is empty.

## Nesting
Expand Down
43 changes: 36 additions & 7 deletions axum/src/extract/matched_path.rs
Expand Up @@ -85,8 +85,9 @@ where
mod tests {
use super::*;
use crate::{extract::Extension, handler::Handler, routing::get, test_helpers::*, Router};
use http::Request;
use http::{Request, StatusCode};
use std::task::{Context, Poll};
use tower::layer::layer_fn;
use tower_service::Service;

#[derive(Clone)]
Expand Down Expand Up @@ -139,33 +140,45 @@ mod tests {
)
}

let app = Router::new()
let app: _ = Router::new()
davidpdrsn marked this conversation as resolved.
Show resolved Hide resolved
.route(
"/:key",
get(|path: MatchedPath| async move { path.as_str().to_owned() }),
)
.nest("/api", api)
.nest(
"/public",
Router::new().route("/assets/*path", get(handler)),
Router::new()
.route("/assets/*path", get(handler))
// have to set the middleware here since otherwise the
// matched path is just `/public/*` since we're nesting
// this router
.layer(layer_fn(SetMatchedPathExtension)),
)
.nest("/foo", handler.into_service())
.layer(tower::layer::layer_fn(SetMatchedPathExtension));
.layer(layer_fn(SetMatchedPathExtension));

let client = TestClient::new(app);

let res = client.get("/foo").send().await;
assert_eq!(res.text().await, "/:key");

let res = client.get("/api/users/123").send().await;
assert_eq!(res.text().await, "/api/users/:id");

// the router nested at `/public` doesn't handle `/`
let res = client.get("/public").send().await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);

let res = client.get("/public/assets/css/style.css").send().await;
assert_eq!(
res.text().await,
"extractor = /public/assets/*path, middleware = /public/assets/*path"
);

let res = client.get("/foo").send().await;
assert_eq!(res.text().await, "extractor = /foo, middleware = /foo");

let res = client.get("/foo/").send().await;
assert_eq!(res.text().await, "extractor = /foo/, middleware = /foo/");

let res = client.get("/foo/bar/baz").send().await;
assert_eq!(
res.text().await,
Expand All @@ -176,4 +189,20 @@ mod tests {
),
);
}

#[tokio::test]
async fn nested_opaque_routers_append_to_matched_path() {
let app = Router::new().nest(
"/:a",
Router::new().route(
"/:b",
get(|path: MatchedPath| async move { path.as_str().to_owned() }),
),
);

let client = TestClient::new(app);

let res = client.get("/foo/bar").send().await;
assert_eq!(res.text().await, "/:a/:b");
}
}
4 changes: 2 additions & 2 deletions axum/src/extract/path/mod.rs
Expand Up @@ -499,10 +499,10 @@ mod tests {
let client = TestClient::new(app);

let res = client.get("/foo/bar/baz").send().await;
assert_eq!(res.text().await, "/bar/baz");
assert_eq!(res.text().await, "bar/baz");

let res = client.get("/bar/baz/qux").send().await;
assert_eq!(res.text().await, "/baz/qux");
assert_eq!(res.text().await, "baz/qux");
}

#[tokio::test]
Expand Down