tide icon indicating copy to clipboard operation
tide copied to clipboard

`reset_middleware()` doesn't prevent middleware from running for route

Open 0b10011 opened this issue 2 years ago • 2 comments

reset_middleware() doesn't affect which middleware is ran for a route. With the following code, I'd expect a 200 response with the text "Success!". Instead, the log shows:

thread 'async-std/runtime' panicked at 'Middleware panicked.', src\main.rs:25:9

cargo.toml

[package]
name = "tide-middleware-test"
version = "0.1.0"
edition = "2018"
publish = false

[dependencies]
async-std = { version = "1.6.0", features = ["attributes"] }
tide = "0.16.0"

src/main.rs

use tide::{Middleware, Next, Request, Response, Result};

#[async_std::main]
async fn main() -> std::result::Result<(), Box<dyn std::error::Error>> {
    let mut app = tide::new();

    app.with(PanicMiddleware {});

    app.at("/").reset_middleware().get(page);

    app.listen("0.0.0.0:80").await?;

    Ok(())
}

async fn page(_request: Request<()>) -> tide::Result {
    Ok("Success!".into())
}

struct PanicMiddleware {}

#[tide::utils::async_trait]
impl<State: Clone + Send + Sync + 'static> Middleware<State> for PanicMiddleware {
    async fn handle(&self, _request: Request<State>, _next: Next<'_, State>) -> Result<Response> {
        panic!("Middleware panicked.");
    }
}

0b10011 avatar Sep 20 '21 02:09 0b10011

It looks like this is specifically due to adding the middleware to Server instead of Route. Here's a test for this issue:



#[async_std::test]
async fn app_middleware_reset() -> tide::Result<()> {
    let mut app = tide::new();
    app.with(TestMiddleware::with_header_name("X-Root", "root"));
    app.at("/foo")
        .reset_middleware()
        .with(TestMiddleware::with_header_name("X-Foo", "foo"))
        .get(echo_path);

    let res = app.get("/foo").await?;
    assert!(res.header("X-Root").is_none());
    assert_eq!(res["X-Foo"], "foo");
    Ok(())
}

0b10011 avatar Sep 20 '21 02:09 0b10011

@jnicklas @0b10011 Hi, I have a simple resolution for this issue.

It looks like this is specifically due to adding the middleware to Server instead of Route. Here's a test for this issue:

For example, implement server.reset_middleware instead of route.reset_middleware, we are possible to reset middleware which server has.

For usages:

#[async_std::test]
async fn app_middleware_reset() -> tide::Result<()> {
    let mut app = tide::new();
    app.with(TestMiddleware::with_header_name("X-Root", "root"));
    app.reset_middleware(); // reset this!
    app.at("/foo")
        .with(TestMiddleware::with_header_name("X-Foo", "foo"))
        .get(echo_path);

    let res = app.get("/foo").await?;
    assert!(res.header("X-Root").is_none());
    assert_eq!(res["X-Foo"], "foo");
    Ok(())
}

According to implementation of server.reset_middleware in my local, it seems to be passed this tests.

@yoshuawuyts Can I work on this issue if you approved to resolve this way?

u5surf avatar Oct 09 '21 01:10 u5surf