http-rs / route-recognizer

Recognizes URL patterns with support for dynamic and wildcard segments
MIT License
100 stars 38 forks source link

`*` should match empty paths #45

Open yoshuawuyts opened 3 years ago

yoshuawuyts commented 3 years ago

Ref: https://github.com/http-rs/tide/issues/783

curl http://localhost:8080/test
ok
curl http://localhost:8080/
404
curl http://localhost:8080
404
jbr commented 3 years ago

happy to take this on, although i'm not sure it's a bug in the semver sense (i think this is 0.x semver-minor in that it's a change in expected behavior as well as actual behavior, and would be semver-major if this were > 0)

willhodges commented 2 years ago

This is still an issue. The wildcard is commonly understood to mean zero or more repetitions of an item, in this case path segments.

willhodges commented 2 years ago

@yoshuawuyts Do you mind if I take a crack at this sometime in the next few weeks?

jbr commented 2 years ago

Is this an issue on head? The router has changed between the last release and head

willhodges commented 2 years ago

@jbr Yes, still an issue on head. The NFA is not configured to allow an empty string for the colon wildcard as a valid acceptance state. I added some quick tests in lib.rs to verify this as well:

    #[test]
    fn empty_wildcard_colon() {
        let mut router = Router::new();

        router.add("/a/:b", "ab".to_string());

        let m = router.recognize("/a").unwrap();
        assert_eq!(*m.handler, "ab".to_string());
    }

    #[test]
    fn empty_wildcard_colon2() {
        let mut router = Router::new();

        router.add("/a/:b", "ab".to_string());

        let m = router.recognize("/a/").unwrap();
        assert_eq!(*m.handler, "ab".to_string());
    }
failures:

---- tests::empty_wildcard_colon stdout ----
thread 'tests::empty_wildcard_colon' panicked at 'called `Result::unwrap()` on an `Err` value: "The string was exhausted before reaching an acceptance state"', src/lib.rs:503:40

---- tests::empty_wildcard_colon2 stdout ----
thread 'tests::empty_wildcard_colon2' panicked at 'called `Result::unwrap()` on an `Err` value: "The string was exhausted before reaching an acceptance state"', src/lib.rs:513:41
jbr commented 2 years ago

Ahh my mistake, I hadn't noticed that we were in the route recognizer crate, not tide. However, this original issue was about star routes, not colon routes. Wanting /a/:b to match /a would be a different issue from this one.

However, tide moved to a different routing crate (routefinder) because route-recognizer is not particularly easy to change. It might be possible to add this behavior to routefinder. If that's something you'd want, feel free to open an issue over there, but it might be a week or two before I get to it, and it might need to be an opt-in feature (probably new syntax for the parser, like /a/:b? or similar).

willhodges commented 2 years ago

No worries.

I'm currently working on a project using Yew, which uses route-recognizer for routing. It might be possible to convince them to switch to routefinder since the route specification syntax is similar (identical?). I would love the ability to specify a /:b? in my routes, as this mirrors the route matching in Rocket, which I'm also using: https://rocket.rs/v0.5-rc/guide/requests/#multiple-segments. Their multi-segment wildcard allows matching an empty path, which simplifies routing in an application that deals with file paths where you'd also want to accept the root path as valid.

jbr commented 2 years ago

@willhodges I'd be very excited to hear if yew was interested in switching to routefinder! Although the syntax is identical, there are several small differences: Routefinder only supports a single wildcard (*) per route, unnamed parameters are not supported, and the prioritization between routes may not be identical (I am not aware of any differences, but retaining route priority wasn't as important as making the priority behave as I thought was desirable). * does match the empty path in routefinder, which is another difference from route-recognizer.