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

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

Open
0b10011 opened this issue Sep 20, 2021 · 2 comments · May be fixed by #855
Open

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

0b10011 opened this issue Sep 20, 2021 · 2 comments · May be fixed by #855
Labels
bug Something isn't working

Comments

@0b10011
Copy link

0b10011 commented Sep 20, 2021

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
Copy link
Author

0b10011 commented Sep 20, 2021

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(())
}

@yoshuawuyts yoshuawuyts added the bug Something isn't working label Oct 8, 2021
@u5surf
Copy link
Contributor

u5surf commented Oct 9, 2021

@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 u5surf linked a pull request Oct 19, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants