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

fix refs components decoding #90

Merged
merged 1 commit into from
Jul 24, 2018
Merged

fix refs components decoding #90

merged 1 commit into from
Jul 24, 2018

Conversation

RomanHotsiy
Copy link
Contributor

The following ref was incorrectly resolved by the Pointer.parse method:

Pointer.parse("#/paths/~1user/application~1vnd.example%2Bjson/schema")
// returns ["paths", "/user", "application/vnd.example%2Bjson", "schema"]

Note that + in application/vnd.example%2Bjson is not decoded.
The reason is usage of decodeURI. Instead, decodeURIComponent should be used (tokens are components of the pointer)

use decodeURIComponent instead of decodeURI
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.288% when pulling 5fe2988 on RomanGotsiy:patch-2 into dba71e2 on BigstickCarpet:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.288% when pulling 5fe2988 on RomanGotsiy:patch-2 into dba71e2 on BigstickCarpet:master.

@coveralls
Copy link

coveralls commented Jun 20, 2018

Coverage Status

Coverage remained the same at 95.288% when pulling 5fe2988 on RomanGotsiy:patch-2 into dba71e2 on BigstickCarpet:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.288% when pulling 5fe2988 on RomanGotsiy:patch-2 into dba71e2 on BigstickCarpet:master.

@RomanHotsiy
Copy link
Contributor Author

Hey @BigstickCarpet 🙌!
Could you please take a look at this?

Thanks in advance!

@JamesMessinger
Copy link
Member

Hi @RomanGotsiy - sorry for taking so long to look at this. I've been super busy at work and haven't had any time.

I'm hesitant to merge this without a test, or at least an example of a schema that doesn't work without this change.

This block of code...
https://github.com/BigstickCarpet/json-schema-ref-parser/blob/9420095f8d717d1ec8372b66a01bbc0d6f903b50/lib/pointer.js#L168-L171

...is directly related to this block of code:
https://github.com/BigstickCarpet/json-schema-ref-parser/blob/9420095f8d717d1ec8372b66a01bbc0d6f903b50/lib/pointer.js#L193-L199

It seems like either both of them should change, or neither of them, but not just one or the other.

@RomanHotsiy
Copy link
Contributor Author

Hey @BigstickCarpet!
No worries! I will add a test a bit later today!

Thanks

@JamesMessinger
Copy link
Member

JamesMessinger commented Jul 24, 2018

@RomanGotsiy - I reviewed the spec and found that you're correct. It should be encodeURIComponent and decodeURIComponent, rather than encodeURI and decodeURI. I extended the "special characters" tests to test for this. As expected, the tests failed. Once I merge this PR, they'll pass

@JamesMessinger JamesMessinger merged commit 9965c26 into APIDevTools:master Jul 24, 2018
@RomanHotsiy
Copy link
Contributor Author

Nice! Thanks a lot for your work! 🙇

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