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.

Sign up for GitHub

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

Jump to bottom

querystring: parse numbers correctly #1213

Merged
merged 2 commits into from
Mar 20, 2015
Merged

querystring: parse numbers correctly #1213

merged 2 commits into from
Mar 20, 2015

Conversation

Fishrock123
Copy link
Contributor

Fixes: #1208

I dunno if the test cases need to be any more thorough.

cc @rvagg @mscdex

@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2015

LGTM

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

perhaps stick a decimal in there for completeness, -0 is also an interesting case.

@kenany
Copy link
Contributor

kenany commented Mar 19, 2015

From @thedufer's comment I was under the impression that coercing the number to a string should happen in QueryString.escape. That would match previous behavior, wherein numbers can be passed to custom escape functions.

@Fishrock123
Copy link
Contributor Author

Pushed new tests. Looking into the escape() thing.

@Fishrock123
Copy link
Contributor Author

@kenany interesting. I think stringifyPrimitive()'s previous functionaility was effectively broken though. It wasn't stringifying a primitive (numbers).

See the original commit: 422d3c9#diff-82accdcd8cb0f24f4b05d365c996d64fR22

It appears this passed through because encodeURIComponent() didn't really care.

@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2015

The check for a number could be done at the top of Querystring.escape() I suppose, but with a name like stringifyPrimitive() it seems like all output should have been a string to begin with...

@mscdex
Copy link
Contributor

mscdex commented Mar 19, 2015

Also, I'm not sure why converting the number to a string before passing it to the encoder function would cause a problem. Even if a custom encoder was being used, they should have already been supporting strings right? The only difference is that they get a string for numbers instead of a number now.

@kenany
Copy link
Contributor

kenany commented Mar 19, 2015

Actually yeah the function's name is pretty clear that we should coerce in there :)

@Fishrock123
Copy link
Contributor Author

@rvagg Thoughts? (and PTAL at the tests again)

Technically,escape() not accepting numbers could be breaking if people are for some reason using it.

If we need to be safe, we can make escape() check it and just change it to be proper in 2.x, but I'd call the behavior of stringifyPrimitive() broken.

@mscdex one would hope so.

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

priority is to feature-match what was there before, don't go fixing other things in this, separate PR for that and separate consideration re semver-major

this LGTM if @mscdex and others are on board

@Fishrock123
Copy link
Contributor Author

To clarify: this fixes the behavior of stringifyPrimitive(), but does not match how escape() coerced numbers before.

@mscdex
Copy link
Contributor

mscdex commented Mar 20, 2015

@Fishrock123 I guess if we want to be absolutely 100% backwards compatible, we need to move the change from stringifyPrimitive() to the top of Querystring.escape(). Then after that lands, we can move it back with a semver-major tag I guess.

I have a hard time believing the current solution would actually affect anyone (for reasons I previously described). FWIW I actually just manually searched through ~1200 search results for querystring.escape on github, and all of the instances where people are actually replacing that method with another function, they're either directly or indirectly calling encodeURIComponent() on the value (much less writing their own version of encodeURIComponent). Most instances are people wrapping encodeURIComponent() and then replacing their own custom characters afterwards or they are replacing escape() with one from the querystring npm module which also uses encodeURIComponent() internally.

EDIT: I also searched through another ~800 search results for qs.escape on github and found similar results as with querystring.escape FWIW...

@rvagg
Copy link
Member

rvagg commented Mar 20, 2015

ok, so what's it to be? I don't have a strong opinion beyond not making this a breaking change for people, it was supposed to be semver-patch so let's try and keep it that way -- internal changes with no external API change should be the goal. So fix escape() too?

@Fishrock123
Copy link
Contributor Author

I'm going to fix both in two commits, incoming momentarily.

@Fishrock123
Copy link
Contributor Author

@rvagg @mscdex updated PTAL

Fishrock123
Fishrock123 reviewed Mar 20, 2015
View reviewed changes
lib/querystring.js
@@ -90,6 +90,9 @@ var hexTable = new Array(256);
for (var i = 0; i < 256; ++i)
hexTable[i] = '%' + ((i < 16 ? '0' : '') + i.toString(16)).toUpperCase();
QueryString.escape = function(str) {
// replaces encodeURIComponent
// http://www.ecma-international.org/ecma-262/5.1/#sec-15.1.3.4
str = '' + str;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what encodeURIComponent does on any input

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with that, it looks like if it's already a string, there's no performance penalty involved.

@rvagg
Copy link
Member

rvagg commented Mar 20, 2015

lgtm, good work

@mscdex
Copy link
Contributor

mscdex commented Mar 20, 2015

LGTM too.

@Fishrock123
querystring: parse numbers correctly
a89f5c2 
Fixes a number parsing regression introduced in 85a92a3

Fixes: nodejs#1208
PR-URL: nodejs#1213
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Brian White <[email protected]>
@Fishrock123
querystring: fix broken stringifyPrimitive
c9aec2b 
stringifyPrimitive has always failed to stringify numbers since its
introduction in 422d3c9. This went uncaught due to encodeURIComponent's
string coercion.

Fixes: nodejs#1208
PR-URL: nodejs#1213
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Brian White <[email protected]>
@Fishrock123 Fishrock123 merged commit c9aec2b into nodejs:v1.x Mar 20, 2015
@Fishrock123
Copy link
Contributor Author

Landed in a89f5c2 and c9aec2b

@rvagg rvagg mentioned this pull request Mar 20, 2015
Closed
rvagg added a commit that referenced this pull request Mar 20, 2015
@rvagg
2015-03-20 io.js v1.6.1 Release
4b91d50 
Notable Changes:

* path: New type-checking on path.resolve()
  <#1153> uncovered some edge-cases
  being relied upon in the wild, most notably path.dirname(undefined).
  Type-checking has been loosened for path.dirname(), path.basename(),
  and path.extname(), (Colin Ihrig)
  <#1216>.
* querystring: Internal optimizations in querystring.parse() and
  querystring.stringify() <#847>
  prevented Number literals from being properly converted via
  querystring.escape() <#1208>,
  exposing a blind-spot in the test suite. The bug and the tests have
  now been fixed (Jeremiah Senkpiel)
  <#1213>.
@julianduque julianduque mentioned this pull request Mar 20, 2015
Closed
@yosuke-furukawa yosuke-furukawa mentioned this pull request Mar 21, 2015
Merged
@Fishrock123 Fishrock123 deleted the querystring-fix branch March 26, 2015 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Fishrock123 @mscdex @rvagg @kenany