search

pillarjs / path-to-regexp

Turn a path string such as `/user/:name` into a regular expression
MIT License
8.18k stars 382 forks source link

Should named parameters with hyphens work? #63

Closed whitlockjc closed 9 years ago

whitlockjc commented 9 years ago

Should /some/:object-id be supported? I do not get an error when parsing a path like this but the generated regular expression never matches paths I would expect to like /some/1 or /some/name. Before I assume it's an error, I wanted to ask.

This originally came up in apigee-127/swagger-tools/issues/289.

dougwilson commented 9 years ago

Though I don't see it in the readme, the only supported tokens are those in the JavaScript "\w" set, mainly because the patterns were originally designed in Express 4 for use in JavaScript variables and properties.

I don't have an opinion on supporting the dash, only would note that it would be a breaking change in behavior (think of the route "/:lang-:country/*").

whitlockjc commented 9 years ago

Thanks for responding. I'll let @theganyo read up on this since he was the original reporter and we'll go from there.

blakeembrey commented 9 years ago

@whitlockjc Doug beat me to it, and this should be added to the README, but it's not supported and likely won't be.

As a side note, I don't know how you're technically handling Swagger documents and path syntax, but for RAML I implemented a custom path matcher implementation and eventually a router (since RAML does validation on URI parameters). It's likely you don't need to do that - Swagger is only Level 1 Template URIs right? (@dougwilson, this might be interesting going to back when you mentioned having a router that supported the RFC for template URIs). I couldn't find where you were actually handling the path stuff to create middleware, so this is just a brain dump of information.

Side note: I was reading through https://github.com/apigee-127/swagger-tools/blob/408e252345578716bd52b607948e287a2eb196c2/middleware/swagger-metadata.js#L327-L330 and I think it's a bug? The expressPath never has { since you just replaced all the instances with : a few lines above.

whitlockjc commented 9 years ago

I'm not sure why you couldn't support - but I'm not advocating for it either. I mean, if I wanted to have a parameter named some-path, I don't see why I couldn't have /path/:some-path without breaking /path/:some-:path. But again, I'm not asking for it.

As for the potential bug in swagger-tools' swagger-metadata middleware, I do think you've found a bug and I'll report it. Speaking of Swagger, I wonder what @webron thinks about this. I wonder if there is some undocumented limitations for the variable names in Swagger path templating. Regardless, I can bring it up in swagger-tools.

Thanks a lot for looking into this.

webron commented 9 years ago

Just replied to the linked topic, but for ease I'll quote it here:

The Swagger spec doesn't provide specific details about it.

However, following RFC 6570 it looks like characters other than alphanumeric or underscore cannot be used unless they are decoded. I'd leave the decision on whether to support it up to you, but I believe it's ok to not provide support for it.

For non-Swagger related tools, if you follow support for the RFC, then you probably wouldn't want to add support for it.