Trailing slash

[secretfader]

When creating REST APIs with Tide, I expect code like the following will be fairly common:

let mut router = App::new();
router.at("/resource").nest(|router| {
  router.at("/").get(async move |_| "Slash");
  router.at("").get(async move |_| "Blank");
});

I don't think developers will type both routes often, but in the wild, it's highly likely that we'll find both techniques in use. Being a bit more explicit about how routes are added the looked up may be an option to prevent confusion going forward. When looking at the example, it isn't entirely clear how an incoming request would be handled.

As of this writing, Tide renders two different responses: "Slash" appears at the path with a trailing slash (/resources/), and "Blank" at /resources. Which leads to the following questions:

• Should Tide allow both a blank and slashed path in cases where the two are extremely similar?
• Does Tide require a configuration option to differentiate between matching paths with trailing slashes and empty paths? Do we enable this option by default?

I'm sure there are other considerations I haven't accounted for yet, and aware that this change could affect future work in route-recognizer. The above is merely a starting point for this discussion, and I'd like to hear what others think.

[yoshuawuyts]

Should Tide allow both a blank and slashed path in cases where the two are extremely similar?

We should probably treat the two as equivalent. There's even an argument to be made for disallowing empty paths "", and require folks type "/" so confusion can't occur in the first place.

Does Tide require a configuration option to differentiate between matching paths with trailing slashes and empty paths?

I don't think we should allow configuration of this. This is the kind of decision that feels like a bit of a bikeshed, and I see one of the benefits of frameworks over ad-hoc module collections is that you don't need to think about these kinds of decisions.

To me picking one option, and documenting it seems like the right way to go to.

[secretfader]

I agree that we should choose one path, @yoshuawuyts. My take is that we shouldn't allow "" at all, in favor of /. (It is admittedly a bit of a bikeshed, but not nearly as critical now as it will become. In my view, it's best to answer this now while the framework is developing.)

[enners]

I like the current strict path matching principles of tide, only one match, no magic, compliant.

As weird as this may be, http://acme.com/resource/ and http://acme.com/resource are different in http. @yoshuawuyts "We should probably treat the two as equivalent. " -> I think it is perfect as it is currently and vote for closing this one.

Anyways, see #492 for a similar issue of myself I want to suggest for improvement.

BTW: Google suggests to redirect the one to the other in case a dumb client needs it, that should be easily doable with tide - but manually, without magic, please :)

[prabirshrestha]

I'm having similar issues trying to capture trailing slash where curl http://localhost:8080/webdav/ -v is returning 404 in tide 0.16.
I'm trying to implement webdav based on async-vfs.

use tide::{Request, Result, Server};

#[async_std::main]
async fn main() -> Result<()> {
    let mut app = tide::new();
    app.at("/webdav").nest(get_app());
    app.listen("0.0.0.0:8080").await?;
    Ok(())
}

fn get_app() -> Server<()> {
    let mut app = tide::new();
    app.at("/").get(|_| async { Ok("root: /") });
    app.at("/*path").get(|req: Request<()>| async move {
        Ok(format!("path: {}", req.param("path").unwrap_or("")))
    });
    app
}

Ruby on rails seems to have a flag to for enforcing. https://stackoverflow.com/questions/16218670/enforce-trailing-slash-in-rails-routing.

config.action_controller.default_url_options = { :trailing_slash => true }

Sinatra seems to allow via ?. https://benl.wordpress.com/2009/04/17/sinatra-matching-routes-with-or-without-trailing-slashes/

get "/test/?" do
    'in test'
end

ASP.NET has the AppendTrailingSlash flag. https://docs.microsoft.com/en-us/dotnet/api/system.web.routing.routecollection.appendtrailingslash?view=netframework-4.8

Personally I prefer the sinatra with /? as the user has the full power.

It would also be good if I could do something like this where the wildcard is optional so I don't need to register both .at("/") and .at("/*path").

fn get_app() -> Server<()> {
    let mut app = tide::new();
    app.at("/*?path").get(|req: Request<()>| async move {
        Ok(format!("path: {}", req.param("path").unwrap_or("")))
    });
    app
}

[jbr]

This is addressed in #802, and routefinder also opens up the possibility of alternative route syntaxes that route-recognizer does not support

[jbr]

I guess actually, to make that more actionable: I believe that #802 addresses this, but it would be extremely useful for people to try that branch out on their projects and provide feedback. The primary thing that's keeping us from merging it is that it's a bunch of brand new code and I don't trust the author (me)

[prabirshrestha]

@jbr #802 does seem to fix my issue.

use anyhow::Result;
use tide::{Request, Server};

#[async_std::main]
async fn main() -> Result<()> {
    let mut app = tide::new();
    app.at("/webdav").nest(webdav());
    app.listen("0.0.0.0:3000").await?;
    Ok(())
}

fn webdav() -> Server<()> {
    let mut app = tide::new();
    app.at("*").get(|req: Request<()>| async move {
        let path = req.url().path();
        let wildcard = req.wildcard().unwrap_or("empty");
        Ok(format!("wildcard: {}\npath: {}\n", wildcard, path).to_owned())
    });
    app
}

$ curl http://localhost:3000/webdav
wildcard:
path: /
/home/prabirshrestha/code/others/tide-wildcard$ curl http://localhost:3000/webdav/
wildcard:
path: /
$ curl http://localhost:3000/webdav/hi
wildcard: hi
path: /hi
$ curl http://localhost:3000/webdav/hi/
wildcard: hi
path: /hi

Unfortunately I don't have too many tide projects to test it. Another idea could be to define some sort of metadata to define routes in test. Here is an example that I came up.

GET / -> 1
ALL /webdav -> 2
    GET / -> 3
    OPTIONS / -> 4
    GET * -> 5
---
GET /webdav -> 3
    params = {}
    path = ""
    query = {}
    wildcard = None
GET /webdav/ -> 3
    params = {}
    path = ""
    query = {}
    wildcard = None
GET /webdav/home -> 5
    params = {}
    path = "/home"
    query = {}
    wildcard = "home"
GET /webdav/home/photos -> 5
    params = {}
    path = "/home/photos"
    query = {}
    wildcard = "home/photos"

the upper half before --- defines the route and tab/4 spaces defines nesting of an app. below half after --- defines the test cases and params. you can then have a function like assert_routes(metadata) which would programmatically setup the tide server and register routes and auto return the params/path/query/wildcard as part of json body or response header which can then be asserted. once you have the test case you can easily swap the tide router in future without much work.

Or might be all we need is to just add nested trailing slash test cases in #802.

[jbr]

As far as testing, routefinder has quite a few tests that work exactly like this, expressed in rust: https://github.com/jbr/routefinder/blob/main/tests/tests.rs (as well as doctests throughout the docs).
Additional tests of that sort would be very welcome.

I personally believe that each unit of code (crates in this circumstance) should test the code it is responsible for, and to avoid duplicating test coverage except for a few "toothpick through the sandwich" tests. In this case, route precedence is the exclusive responsibility of routefinder/route-recognizer, and so the test coverage should exist where it is implemented.