1.6.0: querystring.stringify does not work with number literals

[dpatti]

1.5.1:

> querystring.stringify({ foo: 1 })
'foo=1'

1.6.0:

> querystring.stringify({ foo: 1 })
'foo='

[thedufer]

85a92a3 appears to be at fault - QueryString.escape no longer works properly on numbers, but the calling code expects it to.

[mikeal]

this is bad.

[mikeal]

@mscdex @trevnorris pinging you since this is because of 85a92a3

[cjihrig]

This seems like something that there should have been an existing test for.

[kenany]

Would this patch suffice?

diff --git a/lib/querystring.js b/lib/querystring.js
index af320cf..601ed16 100644
--- a/lib/querystring.js
+++ b/lib/querystring.js
@@ -145,7 +145,7 @@ QueryString.escape = function(str) {

 var stringifyPrimitive = function(v) {
   if (typeof v === 'string' || (typeof v === 'number' && isFinite(v)))
-    return v;
+    return String(v);
   if (typeof v === 'boolean')
     return v ? 'true' : 'false';
   return '';
diff --git a/test/parallel/test-querystring.js b/test/parallel/test-querystring.js
index e2591d7..6ebb85e 100644
--- a/test/parallel/test-querystring.js
+++ b/test/parallel/test-querystring.js
@@ -14,6 +14,7 @@ var qsTestCases = [
   ['foo=bar', 'foo=bar', {'foo': 'bar'}],
   ['foo=bar&foo=quux', 'foo=bar&foo=quux', {'foo': ['bar', 'quux']}],
   ['foo=1&bar=2', 'foo=1&bar=2', {'foo': '1', 'bar': '2'}],
+  ['foo=1&bar=2', 'foo=1&bar=2', {'foo': 1, 'bar': 2}],
   ['my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F',
    'my%20weird%20field=q1!2%22\'w%245%267%2Fz8)%3F',
    {'my weird field': 'q1!2"\'w$5&7/z8)?' }],

[thedufer]

If we're so worried about optimizing this, it seems wrong to run strings through String.

[kenany]

@thedufer Alright,

diff --git a/lib/querystring.js b/lib/querystring.js
index af320cf..0a5e2d1 100644
--- a/lib/querystring.js
+++ b/lib/querystring.js
@@ -144,8 +144,10 @@ QueryString.escape = function(str) {
 };

 var stringifyPrimitive = function(v) {
-  if (typeof v === 'string' || (typeof v === 'number' && isFinite(v)))
+  if (typeof v === 'string')
     return v;
+  if (typeof v === 'number' && isFinite(v))
+    return String(v);
   if (typeof v === 'boolean')
     return v ? 'true' : 'false';
   return '';

[mscdex]

Looks like we should use '' + v instead of the String constructor.

[thedufer]

Seems reasonable.

However, this is dependent on the expectations of QueryString.escape. The docs indicate that it is exported solely for the purpose of allowing it to be overridden. So do we care that someone out there is probably using it (maybe on numbers) and this will still break their code? And second, we now pass strings to escape, even if a number was passed in to stringify - it's possible for someone to have overridden escape it in such a way that it operates on numbers differently from strings. Again, do we care that we've potentially broken their code?

[Fishrock123]

Looks like all the current querystring test cases are strings: https://github.com/iojs/io.js/blob/v1.x/test/parallel/test-querystring.js

[jbergstroem]

@Fishrock123 There doesn't seem to be a case where it passes a number. Try suggested test by @kenany above.

[rvagg]

Can someone get a PR in for this and we'll push a 1.6.1 today, @mscdex are you in a position to deal with this one or should someone else step up?

[Fishrock123]

Fixed in a89f5c2 and c9aec2b