http-rs / tide

Fast and friendly HTTP server framework for async Rust
https://docs.rs/tide
Apache License 2.0
5.06k stars 322 forks source link

Nested router does not work when path ends with slash #857

Open sorcix opened 3 years ago

sorcix commented 3 years ago

While using a nested router I got unexpected 404 responses when requesting /foo/ on a router nested in /foo.

Test:

#[async_std::test]
async fn nested_with_slash_path() -> tide::Result<()> {
    let mut outer = tide::new();
    let mut inner = tide::new();
    inner.at("/").get(|_| async { Ok("nested") });
    outer.at("/").get(|_| async { Ok("root") });
    outer.at("/foo").nest(inner);

    assert_eq!(outer.get("/foo").recv_string().await?, "nested");
    assert_eq!(outer.get("/foo/").recv_string().await?, "nested");
    assert_eq!(outer.get("/").recv_string().await?, "root");
    Ok(())
}

Failing result:

$ cargo test --features unstable nested_with_slash_path

running 1 test
test nested_with_slash_path ... FAILED

failures:

---- nested_with_slash_path stdout ----
thread 'nested_with_slash_path' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"nested"`', tests/nested.rs:75:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    nested_with_slash_path

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 3 filtered out; finished in 0.00s

Test in https://github.com/http-rs/tide/commit/3a285bbf1c382054149ed2a3c3db21391108fbe1

u5surf commented 2 years ago

Hi, I'm interested in this issues. and I've dived in it slightly.

At first, simply(although may be stupidly?), I just added path.trim_end_matches('/'); both router.route and server.at. Of course this test which @sorcix had indicated has succeeded.

diff --git a/src/router.rs b/src/router.rs 
index e59e260..36ac513 100644
--- a/src/router.rs
+++ b/src/router.rs
@@ -46,6 +46,7 @@ impl<State: Clone + Send + Sync + 'static> Router<State> {
     }

     pub(crate) fn route(&self, path: &str, method: http_types::Method) -> Selection<'_, State> {
+        let path = path.trim_end_matches('/');
         if let Some(Match { handler, params }) = self
             .method_map
             .get(&method)
diff --git a/src/server.rs b/src/server.rs
index 1e6f8c1..c96f7c1 100644
--- a/src/server.rs
+++ b/src/server.rs
@@ -163,6 +163,7 @@ where
     /// match or not, which means that the order of adding resources has no
     /// effect.
     pub fn at<'a>(&'a mut self, path: &str) -> Route<'a, State> {
+        let path = path.trim_end_matches('/');
         let router = Arc::get_mut(&mut self.router)
             .expect("Registering routes is not possible after the Server has started");
         Route::new(router, path.to_owned())

But there are one conventional test which been failed.

     Running tests/wildcard.rs (target/debug/deps/wildcard-2df3af0ed73f684a)

running 11 tests
test invalid_segment_error ... ok
test ambiguous_router_wildcard_vs_star ... ok
test multi_wildcard ... ok
test nameless_internal_wildcard2 ... ok
test nameless_internal_wildcard ... ok
test not_found_error ... ok
test nameless_wildcard ... ok
test invalid_wildcard ... FAILED
test wild_last_segment ... ok
test wildcard ... ok
test wild_path ... ok

failures:

---- invalid_wildcard stdout ----
thread 'invalid_wildcard' panicked at 'assertion failed: `(left == right)`
  left: `Ok`,
 right: `NotFound`', tests/wildcard.rs:102:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    invalid_wildcard

test result: FAILED. 10 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

error: test failed, to rerun pass '--test wildcard'

./tests/wildcard.rs

#[async_std::test]
async fn invalid_wildcard() -> tide::Result<()> {
    let mut app = tide::new();
    app.at("/echo/*path/:one/").get(echo_path);
    assert_eq!(
        app.get("/echo/one/two").await?.status(),
        StatusCode::NotFound
    );
    Ok(())
}

Conventionally, server.at has not removed their last slash which can use to be a last blank segment. Thus, /echo/*path/:one/ which is used in invalid_wildcard could be divided echo, *path, :one, `. /echo/one/twowas recognized asecho,one,two, it returns StatusCode::NotFound cause it cannot be matchedecho,path,:one, . Although, my stupidly proposed changes has removed last segment, in other words segments has been dividedecho,path,:one. Unfortunately, it can be matched segments. and then it has returnedOk`.

@yoshuawuyts Could tell us whether last blank segment has necessary or expected expression?