[go: up one dir, main page]

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

"matchit" based router #363

Merged
merged 17 commits into from
Oct 24, 2021
Merged

"matchit" based router #363

merged 17 commits into from
Oct 24, 2021

Conversation

davidpdrsn
Copy link
Member
@davidpdrsn davidpdrsn commented Oct 3, 2021
  • Routing is now done by matchit internally.
  • Adds support for wildcard routes namely things like .route("/foo/*, _).
  • Performance stays the same as you add more routes. I've benchmarked 1, 10, and 100 routes and saw no significant increase in response times.
  • Overlapping routes now cause a panic when being added.
    • This doesn't work across ors since or takes any tower::Service. I think thats fine and want to keep or this general.
  • nest still accepts any tower::Service which is how I think we should keep it.
  • The order routes are added in no longer matter.
  • Doesn't change method routing.

TODO

  • Update changelog
  • Document panics from route and nest when adding conflicting route
  • nest cannot contain wildcards because axum internally makes it a wildcard
  • Document /foo and /:key now conflicting
  • Does cloning Node impact performance?
  • Look into if this fixes Middleware that return early break Router::or #380 (comment)
  • Make sure middleware still only apply to the routes defined above

Fixes #240
Fixes #259
Fixes #109

@davidpdrsn davidpdrsn added this to the 0.3 milestone Oct 3, 2021
@davidpdrsn davidpdrsn added breaking change A PR that makes a breaking change. C-enhancement Category: A PR with an enhancement labels Oct 3, 2021
@davidpdrsn davidpdrsn marked this pull request as ready for review October 3, 2021 14:35
@davidpdrsn
Copy link
Member Author

cc @ibraheemdev 😊

@ibraheemdev
Copy link
Contributor

👀

I've been meaning to fix the fact that routes like /a/static and /a/:dynamic are conflicting, I'll try to get a fix out soon because that will probably be a cause of complaint.

@davidpdrsn
Copy link
Member Author

I've been meaning to fix the fact that routes like /a/static and /a/:dynamic are conflicting, I'll try to get a fix out soon because that will probably be a cause of complaint.

Probably yeah but I don't think its a big issue tbh.

Copy link
Member
@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I've noticed that doesn't relate to a specific code location: There doesn't seem to be any #[should_panic] tests for overlapping routes.

CHANGELOG.md Outdated Show resolved Hide resolved
src/routing/mod.rs Show resolved Hide resolved
@davidpdrsn
Copy link
Member Author

One thing I've noticed that doesn't relate to a specific code location: There doesn't seem to be any #[should_panic] tests for overlapping routes.

I'm adding those as docs tests right this moment.

CHANGELOG.md Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@jplatte
Copy link
Member
jplatte commented Oct 7, 2021

Have you thought about how #257 would interact with this, by the way? For that, overlapping routes would have to be allowedin some form. Personally I think one reasonable way to allow them would be by requiring users to explicitly "order" / "rank" the otherwise-overlapping routes, and trying them in the specified order. This would also allow both /:key and /foo naturally, though at a slight ergonomics cost (i.e. only works when you explicitly give the /:key route a lower precedence).

See also my comment on another issue where I mentioned how Rocket does this kind of thing.

@davidpdrsn
Copy link
Member Author

No I haven't thought about that 🤔 I think extractors being able to cause some kind of fallback is nice but also a bit worried that it can lead to complex routing setups that are hard to understand.

Regardless I don't think this makes adding such a feature impossible since as you mention some sort of ranking could be added to matchit. So routes would only be considered overlapping if they had the same rank. I guess that could work.

@davidpdrsn
Copy link
Member Author

I think this is in a pretty decent state now. Will read through the changes during the weekend and hopefully merge :)

@davidpdrsn davidpdrsn modified the milestones: 0.3, 0.4 Oct 24, 2021
@davidpdrsn davidpdrsn merged commit 1a78a3f into main Oct 24, 2021
@davidpdrsn davidpdrsn deleted the matchit-router branch October 24, 2021 13:22
davidpdrsn added a commit that referenced this pull request Oct 24, 2021
In #363 I added `MaybeSharedNode` which makes `Router` faster to clone
_if_ you're using `into_make_service`. However I would like instead like
to find a solution that works even when you're not using
`into_make_service`.

Using an `Arc<RwLock<Node<_>>>` is probably the only solution but would
like to experiment. For now I'm gonna rollback `MaybeSharedNode`.
Changing it wont require user facing changes.
davidpdrsn added a commit that referenced this pull request Oct 24, 2021
In #363 I added `MaybeSharedNode` which makes `Router` faster to clone
_if_ you're using `into_make_service`. However I would like instead like
to find a solution that works even when you're not using
`into_make_service`.

Using an `Arc<RwLock<Node<_>>>` is probably the only solution but would
like to experiment. For now I'm gonna rollback `MaybeSharedNode`.
Changing it wont require user facing changes.
davidpdrsn added a commit that referenced this pull request Nov 2, 2021
- Overall:
  - **fixed:** All known compile time issues are resolved, including those with
    `boxed` and those introduced by Rust 1.56 ([#404])
  - **breaking:** The router's type is now always `Router` regardless of how many routes or
    middleware are applied ([#404])

    This means router types are all always nameable:

    ```rust
    fn my_routes() -> Router {
        Router::new().route(
            "/users",
            post(|| async { "Hello, World!" }),
        )
    }
    ```
  - **breaking:** Added feature flags for HTTP1 and JSON. This enables removing a
    few dependencies if your app only uses HTTP2 or doesn't use JSON. Its only a
    breaking change if you depend on axum with `default_features = false`. ([#286])
  - **breaking:** `Route::boxed` and `BoxRoute` have been removed as they're no longer
    necessary ([#404])
  - **breaking:** `Nested`, `Or` types are now private. They no longer had to be
    public because `Router` is internally boxed ([#404])
  - **breaking:** Remove `routing::Layered` as it didn't actually do anything and
    thus wasn't necessary
  - **breaking:** Vendor `AddExtensionLayer` and `AddExtension` to reduce public
    dependencies
  - **breaking:** `body::BoxBody` is now a type alias for
    `http_body::combinators::UnsyncBoxBody` and thus is no longer `Sync`. This
    is because bodies are streams and requiring streams to be `Sync` is
    unnecessary.
  - **added:** Implement `IntoResponse` for `http_body::combinators::UnsyncBoxBody`.
  - **added:** Add `Handler::into_make_service` for serving a handler without a
    `Router`.
  - **added:** Add `Handler::into_make_service_with_connect_info` for serving a
    handler without a `Router`, and storing info about the incoming connection.
  - **breaking:** axum's minimum support rust version is not 1.54
- Routing:
  - Big internal refactoring of routing leading to several improvements ([#363])
    - **added:** Wildcard routes like `.route("/api/users/*rest", service)` are now supported.
    - **fixed:** The order routes are added in no longer matters.
    - **fixed:** Adding a conflicting route will now cause a panic instead of silently making
      a route unreachable.
    - **fixed:** Route matching is faster as number of routes increase.
  - **fixed:** Correctly handle trailing slashes in routes:
    - If a route with a trailing slash exists and a request without a trailing
      slash is received, axum will send a 301 redirection to the route with the
      trailing slash.
    - Or vice versa if a route without a trailing slash exists and a request
      with a trailing slash is received.
    - This can be overridden by explicitly defining two routes: One with and one
      without trailing a slash.
  - **breaking:** Method routing for handlers have been moved from `axum::handler`
    to `axum::routing`. So `axum::handler::get` now lives at `axum::routing::get`
    ([#405])
  - **breaking:** Method routing for services have been moved from `axum::service`
    to `axum::routing`. So `axum::service::get` now lives at, etc.
    `axum::routing::service_method_routing::get`, etc. ([#405])
  - **breaking:** `Router::or` renamed to `Router::merge` and will now panic on
    overlapping routes. It now only accepts `Router`s and not general `Service`s.
    Use `Router::fallback` for adding fallback routes ([#408])
  - **added:** `Router::fallback` for adding handlers for request that didn't
    match any routes. `Router::fallback` must be use instead of `nest("/", _)` ([#408])
  - **breaking:** `EmptyRouter` has been renamed to `MethodNotAllowed` as its only
    used in method routers and not in path routers (`Router`)
  - **breaking:** Remove support for routing based on the `CONNECT` method. An
    example of combining axum with and HTTP proxy can be found [here][proxy] ([#428])
- Extractors:
  - **fixed:** Expand accepted content types for JSON requests ([#378])
  - **fixed:** Support deserializing `i128` and `u128` in `extract::Path`
  - **breaking:** Automatically do percent decoding in `extract::Path`
    ([#272])
  - **breaking:** Change `Connected::connect_info` to return `Self` and remove
    the associated type `ConnectInfo` ([#396])
  - **added:** Add `extract::MatchedPath` for accessing path in router that
    matched the request ([#412])
- Error handling:
  - **breaking:** Simplify error handling model ([#402]):
    - All services part of the router are now required to be infallible.
    - Error handling utilities have been moved to an `error_handling` module.
    - `Router::check_infallible` has been removed since routers are always
      infallible with the error handling changes.
    - Error handling closures must now handle all errors and thus always return
      something that implements `IntoResponse`.

    With these changes handling errors from fallible middleware is done like so:

    ```rust,no_run
    use axum::{
        routing::get,
        http::StatusCode,
        error_handling::HandleErrorLayer,
        response::IntoResponse,
        Router, BoxError,
    };
    use tower::ServiceBuilder;
    use std::time::Duration;

    let middleware_stack = ServiceBuilder::new()
        // Handle errors from middleware
        //
        // This middleware most be added above any fallible
        // ones if you're using `ServiceBuilder`, due to how ordering works
        .layer(HandleErrorLayer::new(handle_error))
        // Return an error after 30 seconds
        .timeout(Duration::from_secs(30));

    let app = Router::new()
        .route("/", get(|| async { /* ... */ }))
        .layer(middleware_stack);

    fn handle_error(_error: BoxError) -> impl IntoResponse {
        StatusCode::REQUEST_TIMEOUT
    }
    ```

    And handling errors from fallible leaf services is done like so:

    ```rust
    use axum::{
        Router, service,
        body::Body,
        routing::service_method_routing::get,
        response::IntoResponse,
        http::{Request, Response},
        error_handling::HandleErrorExt, // for `.handle_error`
    };
    use std::{io, convert::Infallible};
    use tower::service_fn;

    let app = Router::new()
        .route(
            "/",
            get(service_fn(|_req: Request<Body>| async {
                let contents = tokio::fs::read_to_string("some_file").await?;
                Ok::<_, io::Error>(Response::new(Body::from(contents)))
            }))
            .handle_error(handle_io_error),
        );

    fn handle_io_error(error: io::Error) -> impl IntoResponse {
        // ...
    }
    ```
- Misc:
  - `InvalidWebsocketVersionHeader` has been renamed to `InvalidWebSocketVersionHeader` ([#416])
  - `WebsocketKeyHeaderMissing` has been renamed to `WebSocketKeyHeaderMissing` ([#416])

[#339]: #339
[#286]: #286
[#272]: #272
[#378]: #378
[#363]: #363
[#396]: #396
[#402]: #402
[#404]: #404
[#405]: #405
[#408]: #408
[#412]: #412
[#416]: #416
[#428]: #428
[proxy]: https://github.com/tokio-rs/axum/blob/main/examples/http-proxy/src/main.rs
@davidpdrsn davidpdrsn mentioned this pull request Nov 2, 2021
davidpdrsn added a commit that referenced this pull request Nov 2, 2021
- Overall:
  - **fixed:** All known compile time issues are resolved, including those with
    `boxed` and those introduced by Rust 1.56 ([#404])
  - **breaking:** The router's type is now always `Router` regardless of how many routes or
    middleware are applied ([#404])

    This means router types are all always nameable:

    ```rust
    fn my_routes() -> Router {
        Router::new().route(
            "/users",
            post(|| async { "Hello, World!" }),
        )
    }
    ```
  - **breaking:** Added feature flags for HTTP1 and JSON. This enables removing a
    few dependencies if your app only uses HTTP2 or doesn't use JSON. Its only a
    breaking change if you depend on axum with `default_features = false`. ([#286])
  - **breaking:** `Route::boxed` and `BoxRoute` have been removed as they're no longer
    necessary ([#404])
  - **breaking:** `Nested`, `Or` types are now private. They no longer had to be
    public because `Router` is internally boxed ([#404])
  - **breaking:** Remove `routing::Layered` as it didn't actually do anything and
    thus wasn't necessary
  - **breaking:** Vendor `AddExtensionLayer` and `AddExtension` to reduce public
    dependencies
  - **breaking:** `body::BoxBody` is now a type alias for
    `http_body::combinators::UnsyncBoxBody` and thus is no longer `Sync`. This
    is because bodies are streams and requiring streams to be `Sync` is
    unnecessary.
  - **added:** Implement `IntoResponse` for `http_body::combinators::UnsyncBoxBody`.
  - **added:** Add `Handler::into_make_service` for serving a handler without a
    `Router`.
  - **added:** Add `Handler::into_make_service_with_connect_info` for serving a
    handler without a `Router`, and storing info about the incoming connection.
  - **breaking:** axum's minimum support rust version is not 1.54
- Routing:
  - Big internal refactoring of routing leading to several improvements ([#363])
    - **added:** Wildcard routes like `.route("/api/users/*rest", service)` are now supported.
    - **fixed:** The order routes are added in no longer matters.
    - **fixed:** Adding a conflicting route will now cause a panic instead of silently making
      a route unreachable.
    - **fixed:** Route matching is faster as number of routes increase.
  - **fixed:** Correctly handle trailing slashes in routes:
    - If a route with a trailing slash exists and a request without a trailing
      slash is received, axum will send a 301 redirection to the route with the
      trailing slash.
    - Or vice versa if a route without a trailing slash exists and a request
      with a trailing slash is received.
    - This can be overridden by explicitly defining two routes: One with and one
      without trailing a slash.
  - **breaking:** Method routing for handlers have been moved from `axum::handler`
    to `axum::routing`. So `axum::handler::get` now lives at `axum::routing::get`
    ([#405])
  - **breaking:** Method routing for services have been moved from `axum::service`
    to `axum::routing`. So `axum::service::get` now lives at, etc.
    `axum::routing::service_method_routing::get`, etc. ([#405])
  - **breaking:** `Router::or` renamed to `Router::merge` and will now panic on
    overlapping routes. It now only accepts `Router`s and not general `Service`s.
    Use `Router::fallback` for adding fallback routes ([#408])
  - **added:** `Router::fallback` for adding handlers for request that didn't
    match any routes. `Router::fallback` must be use instead of `nest("/", _)` ([#408])
  - **breaking:** `EmptyRouter` has been renamed to `MethodNotAllowed` as its only
    used in method routers and not in path routers (`Router`)
  - **breaking:** Remove support for routing based on the `CONNECT` method. An
    example of combining axum with and HTTP proxy can be found [here][proxy] ([#428])
- Extractors:
  - **fixed:** Expand accepted content types for JSON requests ([#378])
  - **fixed:** Support deserializing `i128` and `u128` in `extract::Path`
  - **breaking:** Automatically do percent decoding in `extract::Path`
    ([#272])
  - **breaking:** Change `Connected::connect_info` to return `Self` and remove
    the associated type `ConnectInfo` ([#396])
  - **added:** Add `extract::MatchedPath` for accessing path in router that
    matched the request ([#412])
- Error handling:
  - **breaking:** Simplify error handling model ([#402]):
    - All services part of the router are now required to be infallible.
    - Error handling utilities have been moved to an `error_handling` module.
    - `Router::check_infallible` has been removed since routers are always
      infallible with the error handling changes.
    - Error handling closures must now handle all errors and thus always return
      something that implements `IntoResponse`.

    With these changes handling errors from fallible middleware is done like so:

    ```rust,no_run
    use axum::{
        routing::get,
        http::StatusCode,
        error_handling::HandleErrorLayer,
        response::IntoResponse,
        Router, BoxError,
    };
    use tower::ServiceBuilder;
    use std::time::Duration;

    let middleware_stack = ServiceBuilder::new()
        // Handle errors from middleware
        //
        // This middleware most be added above any fallible
        // ones if you're using `ServiceBuilder`, due to how ordering works
        .layer(HandleErrorLayer::new(handle_error))
        // Return an error after 30 seconds
        .timeout(Duration::from_secs(30));

    let app = Router::new()
        .route("/", get(|| async { /* ... */ }))
        .layer(middleware_stack);

    fn handle_error(_error: BoxError) -> impl IntoResponse {
        StatusCode::REQUEST_TIMEOUT
    }
    ```

    And handling errors from fallible leaf services is done like so:

    ```rust
    use axum::{
        Router, service,
        body::Body,
        routing::service_method_routing::get,
        response::IntoResponse,
        http::{Request, Response},
        error_handling::HandleErrorExt, // for `.handle_error`
    };
    use std::{io, convert::Infallible};
    use tower::service_fn;

    let app = Router::new()
        .route(
            "/",
            get(service_fn(|_req: Request<Body>| async {
                let contents = tokio::fs::read_to_string("some_file").await?;
                Ok::<_, io::Error>(Response::new(Body::from(contents)))
            }))
            .handle_error(handle_io_error),
        );

    fn handle_io_error(error: io::Error) -> impl IntoResponse {
        // ...
    }
    ```
- Misc:
  - `InvalidWebsocketVersionHeader` has been renamed to `InvalidWebSocketVersionHeader` ([#416])
  - `WebsocketKeyHeaderMissing` has been renamed to `WebSocketKeyHeaderMissing` ([#416])

[#339]: #339
[#286]: #286
[#272]: #272
[#378]: #378
[#363]: #363
[#396]: #396
[#402]: #402
[#404]: #404
[#405]: #405
[#408]: #408
[#412]: #412
[#416]: #416
[#428]: #428
[proxy]: https://github.com/tokio-rs/axum/blob/main/examples/http-proxy/src/main.rs
ttys3 added a commit to ttys3/static-server that referenced this pull request Nov 22, 2021
…registered route: /favicon.ico. Note that `nest("/", _)` conflicts with all routes. Use `Router::fallback` instead

Refs: tokio-rs/axum#363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change. C-enhancement Category: A PR with an enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reverse route ordering Implement routing in Router Ability to do more complex route matching
4 participants