Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Route: leading * screws named params #1341

Closed
mgurov opened this Issue · 9 comments

4 participants

@mgurov

Mixing plain wildcards with named parameters in routes produces inconsistent results.

works more or less ok when the * is after named parameters:

app.get('/file/:name/*', function(req, res){
    console.log('tailing *', req.params);
    res.end(req.params.name);
});

...
request(app).get('/file/name/blah/fooe').expect('name', done); //succeeds
> tailing * [ 'blah/fooe', name: 'name' ]

while same exercise fails with leading asterisk:

app.get('*/file/:name/', function(req, res){
    console.log('leading *', req.params);
    res.end(req.params.name);
});

...
request(app).get('/context/subcontext/file/name').expect('name', done); //fails
> leading * [ 'name', name: '/context/subcontext' ]
Technical notes

The reason for this behaviour is that the keys parameter is not touched for the plain wildcards in utils.pathRegexp function and captures later are assigned incorrectly at Route.prototype.match (route.js).
Fixing this might be tricky, unless managed to inject .replace(/\*/g, '(.*)') into the main replace statement (utile.pathRegexp). At the very least, detecting such mixture would not be too difficult and issuing warning / error could be an option.

Workaround

Hand-crafted regexp works of course, but at a price of less readability and index-based param referencing:

app.get(/.*\/file\/(.*)/, function(req, res){
    res.end(req.params[0]);
});
...
request(app).get('/context/subcontext/file/name').expect('name', done); //works
@mgurov mgurov referenced this issue from a commit in mgurov/express
@mgurov mgurov test revealing issue #1341 0f44925
@mgurov mgurov referenced this issue from a commit in mgurov/express
@mgurov mgurov potential but not elegant fix to issue #1341 43d7eb7
@antitoxic

Is this a real use case scenario though? I mean matching all urls that have /file/something anywhere inside of them ?

@tj
tj commented

im not too concerned about this personally, we can't possibly account for everything a regexp can do, unfortunately we don't have named capture groups in js! that would be nice

@antitoxic

@visionmedia actually xregexp module enables named capture groups. I've used them in https://github.com/web-napopa/node-reversable-router.

@mgurov i still don't know whether this is real world use. e.g. something in the middle or start or end of anything.

@tj
tj commented

@antitoxic yeah but that's a dirty hack.. I don't include stuff like that in cores of projects

@antitoxic

@visionmedia i haven't looked into the source but performance seems to remain the same. what do you have in mind for the dirty hacks? im really interested as i don't want to be doing work using bad practise! i just thought there some good people promoting this project.

@mgurov

@antitoxic it was a real enough life for me. I am mocking a certain web service, which can potentially be attached to different paths, and for me it was much more convenient not to think about the details of * in */file/:name/. Again, there are multiple workarounds and this particular issue is not a show-stopper for me, but feels a bit of "dirty" due to the inconsistency of the behavior, IMHO.

@tj
tj commented

there is no inconsistent behaviour though, it's always (.*)

@mgurov

Yes, it is always (.*), from the underlying regexp point of view. From the captured groups replacement there is some difference, and to the user of the router who got used to /x/* and /x/:name the current behavior might be unexpected.

@jonathanong

closing as i don't think express will ever support this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.