Skip to content
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

Should named parameters with hyphens work? #63

Closed
whitlockjc opened this issue Oct 17, 2015 · 5 comments
Closed

Should named parameters with hyphens work? #63

whitlockjc opened this issue Oct 17, 2015 · 5 comments

Comments

@whitlockjc
Copy link

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
Copy link

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
Copy link
Author

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

@blakeembrey
Copy link
Member

@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
Copy link
Author

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
Copy link

webron commented Oct 18, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants