New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wildcard in route parameter regexp #2495

Open
bevacqua opened this Issue Jan 12, 2015 · 29 comments

Comments

Projects
None yet
10 participants
@bevacqua

bevacqua commented Jan 12, 2015

Route '/:username([a-z][a-z-]*)'
Route '/:username([a-z][a-z-]*)/:project([a-z][a-z0-9-]*)'

URL /someone/project

Expectation is that the second route should match.
Actual result is the first route matches because the * is considered as all the things rather than "zero or more" [a-z-].

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 12, 2015

Member

It's a limitation of the current version of path-to-regexp we use. A work-around would be to use the following route: /:username([a-z][a-z-]{0,}) (i.e. use {0,} in place of *).

Member

dougwilson commented Jan 12, 2015

It's a limitation of the current version of path-to-regexp we use. A work-around would be to use the following route: /:username([a-z][a-z-]{0,}) (i.e. use {0,} in place of *).

@dougwilson dougwilson self-assigned this Jan 12, 2015

@dougwilson dougwilson added 4.x question and removed investigate labels Jan 12, 2015

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 12, 2015

Member

I'm going to check if the most recent (non-backwards-compatible) version of path-to-regexp handles this better.

Member

dougwilson commented Jan 12, 2015

I'm going to check if the most recent (non-backwards-compatible) version of path-to-regexp handles this better.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 12, 2015

Member

It does. So this should be fixed in 5.0, but as for 4.x, you'll probably have to just use the old syntax :/

Member

dougwilson commented Jan 12, 2015

It does. So this should be fixed in 5.0, but as for 4.x, you'll probably have to just use the old syntax :/

@bevacqua

This comment has been minimized.

Show comment
Hide comment
@bevacqua

bevacqua Jan 12, 2015

Thanks for the quick feedback.

bevacqua commented Jan 12, 2015

Thanks for the quick feedback.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 12, 2015

Member

Thanks for the quick feedback.

No problem :) Especially for @bevacqua

Member

dougwilson commented Jan 12, 2015

Thanks for the quick feedback.

No problem :) Especially for @bevacqua

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph May 7, 2015

So is path-to-regexp not going to be updated until version 5.0? That's something I would really like to see as soon as possible. Also the website is misleading because it points to path-to-regexp and its regexp tester but it is currently very out of date.

charlierudolph commented May 7, 2015

So is path-to-regexp not going to be updated until version 5.0? That's something I would really like to see as soon as possible. Also the website is misleading because it points to path-to-regexp and its regexp tester but it is currently very out of date.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson May 7, 2015

Member

It's not that we don't want to upgrade path-to-regexp before 5.0, but all versions above where we are at have breaking changes and thus must wait until 5.0. We cannot upgrade path-to-regexp is a 4.x minor release if people's existing 4.x routes break.

As for website issues, please file them at https://github.com/strongloop/expressjs.com

Member

dougwilson commented May 7, 2015

It's not that we don't want to upgrade path-to-regexp before 5.0, but all versions above where we are at have breaking changes and thus must wait until 5.0. We cannot upgrade path-to-regexp is a 4.x minor release if people's existing 4.x routes break.

As for website issues, please file them at https://github.com/strongloop/expressjs.com

@bevacqua

This comment has been minimized.

Show comment
Hide comment
@bevacqua

bevacqua May 7, 2015

For what it's worth, I've been using {0,} without any inconvenients for a long while now.

bevacqua commented May 7, 2015

For what it's worth, I've been using {0,} without any inconvenients for a long while now.

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph May 7, 2015

I want the upgrade to path-to-regexp for named parameters that can include slashes /assets/:asset+. Do you know of a good workaround for that?

charlierudolph commented May 7, 2015

I want the upgrade to path-to-regexp for named parameters that can include slashes /assets/:asset+. Do you know of a good workaround for that?

@bevacqua

This comment has been minimized.

Show comment
Hide comment
@bevacqua

bevacqua May 7, 2015

Don't you need to use /assets/:asset(*) for that use case?

bevacqua commented May 7, 2015

Don't you need to use /assets/:asset(*) for that use case?

@charlierudolph

This comment has been minimized.

Show comment
Hide comment
@charlierudolph

charlierudolph May 7, 2015

Perfect, thank you @bevacqua

charlierudolph commented May 7, 2015

Perfect, thank you @bevacqua

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jun 19, 2015

Member

So I looked really hard at this issue for Express 4.x, and it can actually be fixed in path-to-regexp 0.1.x series. I'll look into issuing a PR there, but even then, the current 0.1.5 is buggy, so unless the buggyness is reverted or fixed, even if said PR is accepted, we still wouldn't be able to upgrade until the "index issue" is addressed.

Member

dougwilson commented Jun 19, 2015

So I looked really hard at this issue for Express 4.x, and it can actually be fixed in path-to-regexp 0.1.x series. I'll look into issuing a PR there, but even then, the current 0.1.5 is buggy, so unless the buggyness is reverted or fixed, even if said PR is accepted, we still wouldn't be able to upgrade until the "index issue" is addressed.

@tunniclm

This comment has been minimized.

Show comment
Hide comment
@tunniclm

tunniclm Mar 1, 2016

Contributor

Looking through the related PR (pillarjs/path-to-regexp#57), it looks like it would not be possible to fix this in 4.x in a back-compatible way. Is that correct? Or is it just a matter of fixing it in express rather than path-to-regexp?

Contributor

tunniclm commented Mar 1, 2016

Looking through the related PR (pillarjs/path-to-regexp#57), it looks like it would not be possible to fix this in 4.x in a back-compatible way. Is that correct? Or is it just a matter of fixing it in express rather than path-to-regexp?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Mar 1, 2016

Member

Sorry, I never updated this issue. Yes, that is correct in that we can't get it fixed in Express 4.x. If you want to take a stab at it, please go ahead! A fresh look never hurts, because it feels like something that should be possible to fix.

Member

dougwilson commented Mar 1, 2016

Sorry, I never updated this issue. Yes, that is correct in that we can't get it fixed in Express 4.x. If you want to take a stab at it, please go ahead! A fresh look never hurts, because it feels like something that should be possible to fix.

@tunniclm

This comment has been minimized.

Show comment
Hide comment
@tunniclm

tunniclm Mar 2, 2016

Contributor

So if I understand correctly, to fix the original issue we want to prevent translation of * within the () part of a token definition so that it will work as expected within the regexp (instread of taking the special meaning of applying to the token).

However, we can't do this because we found an example that documents /files/:file(*) as a valid usage where the * applies to the token, and we don't want to break anyone that reasonably expects documented and working behaviour not be broken by a minor or patch release.

Is that a correct summary?

Contributor

tunniclm commented Mar 2, 2016

So if I understand correctly, to fix the original issue we want to prevent translation of * within the () part of a token definition so that it will work as expected within the regexp (instread of taking the special meaning of applying to the token).

However, we can't do this because we found an example that documents /files/:file(*) as a valid usage where the * applies to the token, and we don't want to break anyone that reasonably expects documented and working behaviour not be broken by a minor or patch release.

Is that a correct summary?

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Mar 2, 2016

Member

Yes.

Edit: It's possible people also relying on that feature now, so removing (and fixing this issue) would break. I know I've had at least one app in the future abusing the * feature within () and not just as (*).

Member

blakeembrey commented Mar 2, 2016

Yes.

Edit: It's possible people also relying on that feature now, so removing (and fixing this issue) would break. I know I've had at least one app in the future abusing the * feature within () and not just as (*).

@tunniclm

This comment has been minimized.

Show comment
Hide comment
@tunniclm

tunniclm Mar 2, 2016

Contributor

The only reasonable way to make a back-compatible fix that I can think of would be to add an option you could enable to make Express replace *s inside the () with {0,} before sending it to path-to-regexp.

You could push that option up into a patch on path-to-regexp version 0.x instead if that sits better.

I'm happy to have a stab at that change if it's something that we think is desirable. There is a bit of a philosophy question there -- how keen are we on adding options that will be removed next major version to fix bugs that may cause breakage?

Otherwise, perhaps we should label this issue with something like "wontfix", "limitation" or "low-priority" to indicate we can't fix it in 4.x?

Contributor

tunniclm commented Mar 2, 2016

The only reasonable way to make a back-compatible fix that I can think of would be to add an option you could enable to make Express replace *s inside the () with {0,} before sending it to path-to-regexp.

You could push that option up into a patch on path-to-regexp version 0.x instead if that sits better.

I'm happy to have a stab at that change if it's something that we think is desirable. There is a bit of a philosophy question there -- how keen are we on adding options that will be removed next major version to fix bugs that may cause breakage?

Otherwise, perhaps we should label this issue with something like "wontfix", "limitation" or "low-priority" to indicate we can't fix it in 4.x?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Mar 2, 2016

Member

The labels are accurate: it's a bug in 4.x, but wont be fixed until 5.0 due to limitations of the implementation we have in 4.x, for better or worse. We keep this issue open for two reasons:

  1. To remind us it needs to be addressed until there is a 5.0 release on npm with this addressed.
  2. Capture the conversation regarding an active bug for users to find when they have the same issue. They can also subscribe to this issue to know when it is fixed in a published version.

As for the potential solution above, any changes to building a regular expression need to land in our own dependency, otherwise we are not only defeating the purpose of owning our own dependencies, but we are cheating the community out of building complex applications, since they would have to copy and paste into their code instead of updating a dependency.

An option, to me, sounds crazy, because if the change would have to be behind a flag, then it sounds like it's not backwards compatible. The way the paths work is an intrinsic property of express, and so a flag to change this behavior at runtime means a community module can no longer know it's routes will just work on a major version of Express, rather they have to instruct users to change this flag or tough luck.

Member

dougwilson commented Mar 2, 2016

The labels are accurate: it's a bug in 4.x, but wont be fixed until 5.0 due to limitations of the implementation we have in 4.x, for better or worse. We keep this issue open for two reasons:

  1. To remind us it needs to be addressed until there is a 5.0 release on npm with this addressed.
  2. Capture the conversation regarding an active bug for users to find when they have the same issue. They can also subscribe to this issue to know when it is fixed in a published version.

As for the potential solution above, any changes to building a regular expression need to land in our own dependency, otherwise we are not only defeating the purpose of owning our own dependencies, but we are cheating the community out of building complex applications, since they would have to copy and paste into their code instead of updating a dependency.

An option, to me, sounds crazy, because if the change would have to be behind a flag, then it sounds like it's not backwards compatible. The way the paths work is an intrinsic property of express, and so a flag to change this behavior at runtime means a community module can no longer know it's routes will just work on a major version of Express, rather they have to instruct users to change this flag or tough luck.

@tunniclm

This comment has been minimized.

Show comment
Hide comment
@tunniclm

tunniclm Mar 3, 2016

Contributor

I wasn't thinking we would remove any labels or close the issue, I just wanted to add an extra label so it is easier to see the status/use in a query. Perhaps something like "fix-in-next-release" would capture it?

For the potential solution, yes, it was a bit of a stretch idea and certainly far from ideal and, on reflection, the existence of a simple workaround makes it unattractive.

I discounted other potential solutions (like special casing (*) or some subset of regexp-likes containing *) since I didn't find docs or tests that explained clearly how * should work inside (). So a reasonable user could form a conclusion about how it works (which would be backed up by the implementation) that would be broken by changing it at all by default.

It's a shame, because I think it is quite surprising behaviour.

Contributor

tunniclm commented Mar 3, 2016

I wasn't thinking we would remove any labels or close the issue, I just wanted to add an extra label so it is easier to see the status/use in a query. Perhaps something like "fix-in-next-release" would capture it?

For the potential solution, yes, it was a bit of a stretch idea and certainly far from ideal and, on reflection, the existence of a simple workaround makes it unattractive.

I discounted other potential solutions (like special casing (*) or some subset of regexp-likes containing *) since I didn't find docs or tests that explained clearly how * should work inside (). So a reasonable user could form a conclusion about how it works (which would be backed up by the implementation) that would be broken by changing it at all by default.

It's a shame, because I think it is quite surprising behaviour.

@clakech

This comment has been minimized.

Show comment
Hide comment
@clakech

clakech Feb 1, 2017

Thanks for documenting this in the issue, that helps a lot

What is the best workaround: using {0,} or (*) ?

both work but which one is a smaller technical debt ?

clakech commented Feb 1, 2017

Thanks for documenting this in the issue, that helps a lot

What is the best workaround: using {0,} or (*) ?

both work but which one is a smaller technical debt ?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 1, 2017

Member

Using {0,} should work the same with 5.x comes out while (*) will likely change meaning in 5, I believe.

Member

dougwilson commented Feb 1, 2017

Using {0,} should work the same with 5.x comes out while (*) will likely change meaning in 5, I believe.

@ttencate

This comment has been minimized.

Show comment
Hide comment
@ttencate

ttencate May 15, 2017

I spent hours debugging until I found this issue. I understand the reasoning behind leaving the bug in 4.x, but maybe it (and the {0,} workaround) could at least be documented on http://expressjs.com/en/4x/api.html#path-examples or some other appropriate place?

ttencate commented May 15, 2017

I spent hours debugging until I found this issue. I understand the reasoning behind leaving the bug in 4.x, but maybe it (and the {0,} workaround) could at least be documented on http://expressjs.com/en/4x/api.html#path-examples or some other appropriate place?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson May 15, 2017

Member

Hi @ttencate nothing wrong with documenting! At the bottom of every page on the site, there is a "Edit this page on GitHub." link; just click it and make the edit you think should be good and open a pull request : ) ! Alternatively, you can also file an issue on the website issue tracker.

Member

dougwilson commented May 15, 2017

Hi @ttencate nothing wrong with documenting! At the bottom of every page on the site, there is a "Edit this page on GitHub." link; just click it and make the edit you think should be good and open a pull request : ) ! Alternatively, you can also file an issue on the website issue tracker.

@ttencate

This comment has been minimized.

Show comment
Hide comment
@ttencate

ttencate May 15, 2017

Thanks, I somehow overlooked that link :) expressjs/expressjs.com#809

ttencate commented May 15, 2017

Thanks, I somehow overlooked that link :) expressjs/expressjs.com#809

@rjmunro

This comment has been minimized.

Show comment
Hide comment
@rjmunro

rjmunro Nov 22, 2017

So what's horrible here is not the "bug". Either syntax is fine. It's the fact that if you follow the documentation, it says to look at path-to-regexp and express-route-tester to for more information, but those are both at a much newer version that behaves differently.

The documentation should be updated to point at this version of express-route-tester which lets you select different versions and defaults to the one that express 4 is currently using.

For the next versions of express, (both 4 and 5) there should be an option to select the old or new style routing.

rjmunro commented Nov 22, 2017

So what's horrible here is not the "bug". Either syntax is fine. It's the fact that if you follow the documentation, it says to look at path-to-regexp and express-route-tester to for more information, but those are both at a much newer version that behaves differently.

The documentation should be updated to point at this version of express-route-tester which lets you select different versions and defaults to the one that express 4 is currently using.

For the next versions of express, (both 4 and 5) there should be an option to select the old or new style routing.

@VAggrippino

This comment has been minimized.

Show comment
Hide comment
@VAggrippino

VAggrippino Mar 23, 2018

Why doesn't .* work?

From reading this thread, I understand that * matches everything rather than just zero or more of the preceding character, but shouldn't .* match a single character followed by everything else in the same way that [a-z][a-z-]* matches a [letter] followed by a [letter or a dash] followed by [everything else]?

Does this represent a separate bug from path-to-regexp for the .?

I used the following to test:
https://gist.github.com/VAggrippino/796114a27fe449fff7c45b278d9de74b

VAggrippino commented Mar 23, 2018

Why doesn't .* work?

From reading this thread, I understand that * matches everything rather than just zero or more of the preceding character, but shouldn't .* match a single character followed by everything else in the same way that [a-z][a-z-]* matches a [letter] followed by a [letter or a dash] followed by [everything else]?

Does this represent a separate bug from path-to-regexp for the .?

I used the following to test:
https://gist.github.com/VAggrippino/796114a27fe449fff7c45b278d9de74b

@rjmunro

This comment has been minimized.

Show comment
Hide comment
@rjmunro

rjmunro Mar 26, 2018

@VAggrippino There are two different versions of path-to-regexp with different behaviours - as far as I know there isn't an actual bug in code. Express uses the old one, but the documentation doesn't explain this clearly enough.

Have a play with this version of Express Route Tester to see the differences.

rjmunro commented Mar 26, 2018

@VAggrippino There are two different versions of path-to-regexp with different behaviours - as far as I know there isn't an actual bug in code. Express uses the old one, but the documentation doesn't explain this clearly enough.

Have a play with this version of Express Route Tester to see the differences.

@VAggrippino

This comment has been minimized.

Show comment
Hide comment
@VAggrippino

VAggrippino Mar 26, 2018

@rjmunro I really like the express route tester you linked to, but it doesn't help me understand this problem. In express routes, * and . don't work as they do in regular expressions. Why isn't this considered a bug, or at least a documentation error? ... but I suppose that's really a question for the path-to-regexp folks.

If this is the intended functionality, maybe the description of the path argument in the documentation should be changed? The route tester you linked to makes this more clear. It doesn't call the route a regular expression pattern as it does in the documentation. It says "will compile your route into the following regular expression".

VAggrippino commented Mar 26, 2018

@rjmunro I really like the express route tester you linked to, but it doesn't help me understand this problem. In express routes, * and . don't work as they do in regular expressions. Why isn't this considered a bug, or at least a documentation error? ... but I suppose that's really a question for the path-to-regexp folks.

If this is the intended functionality, maybe the description of the path argument in the documentation should be changed? The route tester you linked to makes this more clear. It doesn't call the route a regular expression pattern as it does in the documentation. It says "will compile your route into the following regular expression".

@rjmunro

This comment has been minimized.

Show comment
Hide comment
@rjmunro

rjmunro Mar 28, 2018

@VAggrippino I read the page you linked to as saying you can either specify "A string representing a path" or "A path pattern" (which is presumably passed to path-to-regexp), or you can specify a "A regular expression pattern" which I interpret to mean a regular expression object, i.e. something like:

app.get(/foo/, func);

Which I have never tried, but I guess could work.

rjmunro commented Mar 28, 2018

@VAggrippino I read the page you linked to as saying you can either specify "A string representing a path" or "A path pattern" (which is presumably passed to path-to-regexp), or you can specify a "A regular expression pattern" which I interpret to mean a regular expression object, i.e. something like:

app.get(/foo/, func);

Which I have never tried, but I guess could work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment