.get does not identify valid route request after it returned correct response

[s-a]

Bug Report

Serving static data with plugin restify.plugins.serveStatic alternating returns 405

Restify Version

6.3.4

Node.js Version

v8.9.0

Expected behaviour

Static resource request should be served with status 200

Actual behaviour

Sometimes static resource request will be served with status 405

Repro case

• git clone https://github.com/s-a/restify-routes-start.git
• cd restify-routes-start
• npm install
• npm start
• Browse to http://localhost:8001 and open devtools.

This example request a static JSON file 3 times. In this case 2 of them returns with 405

Update:
I tried to take a look at the code but cannot identify what is going wrong here. The wrong exit point for my requests is here /lib/router.js#L581. Hard to understand because there are so many exit points in Router.prototype.find but if I understand correct the program should not reach this code area. So I think the main error occurs before matchURL where re parameter changes while runtime.

re.exec fails with /\bv?(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-[\da-z-]+(?:\.[\da-z-]+)*)?(?:\+[\da-z-]+(?:\.[\da-z-]+)*)?\/?.*\b/gi

re.exec succeed with /\bv?(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-[\da-z-]+(?:\.[\da-z-]+)*)?(?:\+[\da-z-]+(?:\.[\da-z-]+)*)?\/?.*\b/ which is the original regular expression i passed to .get function.

[DonutEspresso]

Thanks for the bug report, this definitely seems problematic. Will try to take a look later this week.

[s-a]

Thank you. Please let me know if I can help you with something.

[Junkern]

@DonutEspresso
Wow. I also just encountered the error. Not sure if it is exactly the same one (my regexp does not change) but it seems really closely related.

tl;dr: It's probably due to having/using the /g flag in the regexp.

Longer Version:

I wrote a small service which basically acts as a "key-value" database. You can put anything, and if you call the same route, it returns the data you have putted.

So I did

this.server.get(/.*/g, this.getData)
this.server.put(/.*/g, this.upsertData)

Although, I have a PUT handler for any route, restify did return PUT not allowed (which really confused me :D)

Now to the exciting part:

matchUrl reuses the regexp passed to .put(). The documentation for .exec() mentions a lastIndex property on the regexp object:

The index at which to start the next match. When "g" is absent, this will remain as 0.

Remember, g is not absent in my case (and also not in the original issue)

Now, my first PUT was for /someroute/which/is/very/long.
After executing re.exec('/someroute/which/is/very/long'): re.lastIndex === 29

The second PUT was for /ashorterroute. The regexp wants to start at re.lastIndex, but the string is shorter than 29! So it just bails and returns null, and in this case matchUrl returns false.

But that also means, that if the "shorter" route is called first, the re.lastIndex number is still a valid index of the second "longer route", hence both PUT requests would be served fine.

Possible things to do:

My fix was to simply remove the g flag from my regexp.

However, to safe other people from that, I would probably suggest adding some documentation to warn of g flag usage.
Another option would be to improve the documentation to implementation for a set g flag?

[s-a]

@Junkern did you found the code in node-restify where the regular expression changes? I d Like to use this in production but it is blocked by this issue. 😢

[retrohacker]

An update on this issue:

We are in the process of moving to a new router, find-my-way. This is a breaking change that will change the supported routing semantics, but it looks like the new semantics will fit your use case much better!

Open PR: #1561

[hekike]

Now that Restify 7.x is release could you please check your use case with it?

[s-a]

There is a reproducable example in this issue . We have this on storyboard in a few weeks. Still looking for a solution. We will take a look. May take some time if it needs migration. Am 10.04.2018 20:20 schrieb "Peter Marton" <notifications@github.com>: Now that Restify 7.x is release could you please check your use case with it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1581 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNjhr-B7nXwcAru8e21KiS1xFb6y000ks5tnPf0gaJpZM4Q5sZs> .

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[DonutEspresso]

@s-a just to close the loop here - the API has changed a bit in 7.x so your repro case doesn't work out of the box anymore. I tried to take a stab at it locally but seeing as the new router doesn't support free from regex's anymore I think we may need to take a different approach. The new versioning changes can be found here:
http://restify.com/docs/6to7/

Take a look and let us know!

[s-a]

Sorry for the delay friends. We migrated to latest restify version and changed very much the internal process. So let us close this issue since it does not matter anymore for me. Thanks to all for your efforts.