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 URI encoding #1539

Merged
merged 1 commit into from
Feb 17, 2020
Merged

Fix URI encoding #1539

merged 1 commit into from
Feb 17, 2020

Conversation

johnf
Copy link
Contributor

@johnf johnf commented Jan 23, 2020

I was having issues with an oData (https://www.odata.org/) based API. oData uses queries like
http://example.com/todo?$skip=100&$top=50

Superagent will currently encode this to http://example.com/todo?%24skip=100&%24top=50

According to HTTP spec this isn't technically correct. While values of query parameters should have fill encoding applied, the list of reserved characters is smaller for the overall URL.

A correct implementation would be to use encodeURIComponent over the query values and then encodeURI over the entire URL. However, the approach I've taken is equivalent.

Some of the answers to https://stackoverflow.com/questions/4540753/should-i-use-encodeuri-or-encodeuricomponent-for-encoding-urls provide some more explanation. I can provide full detail from HTTP and HTML specs if required.

I tried adding a test but the serialize tests seem to be broken. I could only get them to run if I set NODETESTS=tests/client/serialize.js

According to HTTP spec keys should be encoded slightly differently to
values.
@niftylettuce
Copy link
Collaborator

Thanks @johnf for your work here. I understand what you're saying - and it doesn't seem like your commit broke any tests judging from Travis. I would be curious if you could figure out the serialize tests, and what the issue is with them not working?

@johnf
Copy link
Contributor Author

johnf commented Feb 8, 2020

@niftylettuce if you check the Makefile it looks like all the tests inside clients are only run in the browser

@niftylettuce niftylettuce merged commit c7a10e2 into ladjs:master Feb 17, 2020
@niftylettuce
Copy link
Collaborator

released to npm as v5.2.1

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.

None yet

2 participants