Skip to content

Conversation

@gwicke
Copy link

@gwicke gwicke commented Mar 9, 2015

Slashes in path parameters should be encoded in order to produce valid URLs.

Fixes: #1013

Slashes in path parameters should be encoded in order to produce valid URLs.

Fixes: swagger-api#1013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done with a much simpler approach:

return pathParam.toString().split('/').map(encodeURIComponent).join('/')

Please update the code and and add test if possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is to encode slashes as well.

@fehguy
Copy link
Contributor

fehguy commented Mar 9, 2015

Two thoughts. First, this is in the underlying javascript library, swagger-js. The PR would really belong there.

Second, we have been back and forth on how this should work. Some people want to support slashes in the param and some want them encoded. The current code intentionally supports slashes so "splat" params are valid. I'm not sure what the right answer is other than to make it configurable because there's no one way to do it.

@gwicke
Copy link
Author

gwicke commented Mar 9, 2015

@fehguy, IMHO the current encoding code is just a hacky and inconsistent way to work around the lack of level 2 URI templates. The encoding path pretends that parameters were actually specified as {+splat_param}, while according to the spec they are actually just {single_path_component}s. This patch makes the encoding path consistent with the current swagger spec 2.0.

I hope that proper RFC 6570 support will make its way into the 2.1 spec, so that {+splat_path} can be used to encode "splat" paths where they are actually desired, without breaking the encoding semantics for {single_path_component}.

@gwicke
Copy link
Author

gwicke commented Mar 9, 2015

Created another PR against swagger-js at swagger-api/swagger-js#280.

@gwicke
Copy link
Author

gwicke commented Mar 9, 2015

See also: OAI/OpenAPI-Specification#93

@mohsen1
Copy link
Contributor

mohsen1 commented Mar 11, 2015

Closing in favor of swagger-api/swagger-js#280

@mohsen1 mohsen1 closed this Mar 11, 2015
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

Successfully merging this pull request may close these issues.

3 participants