Routing Paths with Query Strings

[hleinone]

Routes that contain query strings are dysfunctional. I've fixed this in my branch, but as I'm quite inexperienced with the codebase I have no idea if my fix is fool-proof. In addition to ?, I allowed . character in the route pattern.

[olivernn]

Do you have a test case for this, I'm not sure I understand what you mean by 'routes that contain query strings'.

[hleinone]

For instance, if after processing a post request I want to do this: req.redirect("/result?data=foo). I need the appropriate route handler this.get("/result?data=:data", ...). I tried this with the Greeter demo, switching the one / to ? but it didn't work out-of-the-box, hence the issue.

[olivernn]

I don't think you need to have a route that matches the query string. The data in the query string should just be available in the params in the get route handler.

this.post('/foo', function (req) {
  req.redirect('/bar?someData=123')
})

this.get('/bar', function (req) {
  console.log(req.params['someData'])  // prints 123
})

Let me know if this fixes your problem.

[hleinone]

At least there's a problem in redirecting the user to a URL containing query strings, since only Request.path is used.

[olivernn]

Requests are routed based on the pathname only, any query params are not taken into account. When you redirect to a url with query params, the pathname of that url is used to match against a route, then the params from the redirect url are available in that route handler as request params.

I think this is fairly standard amongst routing libraries, a route is a route, regardless of what params you are passing in a query string. E.g. /foo and /foo?bar=baz will both be matched by a route /foo.

[hleinone]

You're right, the routing works as intended. The problem still is that in redirecting the query string does not appear in the URL.

[olivernn]

Oh I see now, I'll take a look at your branch and see about the best way to do this.

[hleinone]

Check out this commit since I first fixed that only. Then I thought that maybe I need the routing to support that and did the another commit.

[hleinone]

I changed a bit of my branch's history and now there's only the Request.location change.

[hleinone]

Also when doing a normal GET request to say /foo?bar=baz the value of bar is not accessible as req.params["bar"].

[hleinone]

I tried to fix that by modifying the history.current to return also the query string if present, and now the parameters are correctly set, but it seems that the history.current is also used in route matching thus my route not being matched.

[hleinone]

Never mind the last one, it was a trivial bug fixed in my branch.

[olivernn]

Your fix is in the right direction.

Instead of re-combining the path and the query string in the location method perhaps just add it as a parameter on the request object anyway, it can be pulled straight from the object that is passed to Davis.Request constructor anyway.

[hleinone]

Not sure if I understand correctly... Do you mean that in the Request object there would be a field that'd contain both the path and the queryString? And the Request.location function would return that instead of this.path?

[olivernn]

Yeah, because somewhere else has already done the work of combining a query string and the path, no point in re-implementing that work.

[hleinone]

Like this?

[hleinone]

Okay, now it should be fine...

[olivernn]

Looks good, if you could add some tests and then wrap up that change into a pull request I can merge into master.

[olivernn]

I've merged this patch and it is now available in the latest release, v0.6.2.

[tj]

people try this with express all the time, IMO query-strings should have nothing to do with routing, it makes them dependant on order etc, IMO it's best just to have req.query or similar

[olivernn]

Definitely agree, query params are not part of routing. The issue was actually that the query string wasn't being put into the url bar when a route was being ran, so /foo?bar=baz would be routed correctly to /foo but the url bar would only have /foo.