thruster-rs / Thruster

A fast, middleware based, web framework written in Rust
MIT License
1.06k stars 47 forks source link

Trailing slash not checked? #196

Closed rapodaca closed 2 years ago

rapodaca commented 2 years ago

An example taken from the examples directory:

use hyper::Body;
use log::info;
use thruster::context::basic_hyper_context::{
    generate_context, BasicHyperContext as Ctx, HyperRequest,
};
use thruster::hyper_server::HyperServer;
use thruster::{m, middleware_fn};
use thruster::{App, ThrusterServer};
use thruster::{MiddlewareNext, MiddlewareResult};

#[middleware_fn]
async fn plaintext(mut context: Ctx, _next: MiddlewareNext<Ctx>) -> MiddlewareResult<Ctx> {
    let val = "Hello, World!";
    context.body = Body::from(val);
    Ok(context)
}

fn main() {
    env_logger::init();
    info!("Starting server...");

    let mut app = App::<HyperRequest, Ctx, ()>::create(generate_context, ());
    app.get("/plaintext", m![plaintext]);

    let server = HyperServer::new(app);
    server.start("0.0.0.0", 4321);
}

I expect /plaintext to match and it does. But /plaintext/ also matches, which is unexpected.

If I change the route to /plaintext/, then '/plaintext` matches, which I also didn't expect. I looked for this in documentation, an even source, but couldn't find anything.

Is the trailing slash matching part of Thruster, or one of the underlying libraries? What should be the supported behavior?

trezm commented 2 years ago

That's correct! We model somewhat after express here, in that /route/ == /route in terms of matching. This is something I could update though, is there a use case you're after?

rapodaca commented 2 years ago

I'm not sure the current behavior is correct. See these two items:

Bottom line: treating /foo/ and /foo as synonyms for the same resource can interfere with the use of relative URLs. A redirect, which can be applied as a top-level middleware by those users who want it, solves the problem without introducing another. At least that's my understanding.

trezm commented 2 years ago

I'm not sure I fully agree with some of the points in that article, but I do fully agree that it should be the developer's choice. I'll work on adding a configuration for it! Thanks for raising the issue :-)

On Thu, Sep 9, 2021, 9:35 AM Richard Apodaca @.***> wrote:

I'm not sure the current behavior is correct. See these two items:

Bottom line: treating /foo/ and /foo as synonyms for the same resource can interfere with the use of relative URLs. A redirect, which can be applied as a top-level middleware by those users who want it, solves the problem without introducing another. At least that's my understanding.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/thruster-rs/Thruster/issues/196#issuecomment-916104227, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJWOLL5RULR4TJHQMY2L4DUBCZZTANCNFSM5DWFCYOQ .

rapodaca commented 2 years ago

It looks like this has been resolved in v1.1.10. Looking forward to trying it out!