Skip to content

URL encode parameters for OpenApiSpec 3 #1331

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

Merged
merged 5 commits into from
Aug 18, 2018
Merged

URL encode parameters for OpenApiSpec 3 #1331

merged 5 commits into from
Aug 18, 2018

Conversation

artemyarulin
Copy link
Contributor

@artemyarulin artemyarulin commented Jun 22, 2018

URL encode parameters for OpenApiSpec 3

Description

Fixes this issue in swagger-ui swagger-api/swagger-ui#4025

Motivation and Context

I guess it was disabled by accident in #1181 which was about allowing to skip url encode for query params if Paramter.allowReserved is set to true. But specification says clearly:

Determines whether the parameter value SHOULD allow reserved characters, as defined by RFC3986 :/?#[]@!$&'()*+,;= to be included without percent-encoding. This property only applies to parameters with an in value of query. The default value is false.. So it should be applied only to query params and path parameters should use encoding all the time

How Has This Been Tested?

I'm not familiar with your testing strategy, but I've recompiled your library and checked that it works correctly using swagger-ui:

Before:
image
After:
image

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@shockey
Copy link
Contributor

shockey commented Jun 27, 2018

Note to self: the fact that changing false to true here didn't break any tests is concerning - may need to add additional unit tests along with this PR

@artemyarulin
Copy link
Contributor Author

Anything I can do to push it forward? Tests, docs, etc.?

@shockey
Copy link
Contributor

shockey commented Jul 9, 2018

@artemyarulin, yes! I was planning to add some tests myself, but some cases that fail prior to this change would be great, if you're up to the task 😄

@shockey shockey merged commit ec016c6 into swagger-api:master Aug 18, 2018
shockey pushed a commit to shockey/swagger-js that referenced this pull request Nov 9, 2018
* URL escape paramters

* modify tests to check value escaping

* linter fixes

Signed-off-by: Kyle Shockey <[email protected]>
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.

2 participants