querystring: use `String.prototype.split`'s `limit` #2288

manvalls · 2015-08-02T07:36:05Z

There's no need to add extra logic for it, String.prototype.split already has a limit argument. By using this argument we avoid keeping the whole array in memory, and V8 doesn't have to process the entire string.

targos · 2015-08-03T19:01:01Z

The changes LGTM.
I'm not sure about the style. Should the if...else be in 4 lines or is it fine like that ?

CI: https://jenkins-iojs.nodesource.com/job/node-test-commit/90/

thefourtheye · 2015-08-03T19:06:47Z

Split is not in Array but in String

manvalls · 2015-08-03T19:11:05Z

@targos ok, fixed

@thefourtheye that's some serious lapse on my side :(

ronkorving · 2015-08-12T02:37:10Z

lib/querystring.js

@@ -209,18 +209,20 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) {
  }

  var regexp = /\+/g;
-  qs = qs.split(sep);

  var maxKeys = 1000;


Unrelated, but what's with this really arbitrary value?

It's in the docs, changing it would mean a major version upgrade. I don't know the reasoning behind it, I guess 1000 is a safe but large enough number of key value pairs.

jasnell · 2015-11-16T16:50:21Z

@manvalls ... can you take a look at @ronkorving's question here: #2288 (comment)

@thefourtheye @targos ... looks like the conversation on this stalled out. Any further thoughts?

targos · 2015-11-16T17:40:39Z

I forgot about it, sorry ! This still LGTM.
We will just have to fix the commit message (to String.prototype.split) if we land it.

jasnell · 2015-11-16T21:28:16Z

@manvalls ... could you squash the commits and fix up the commit message?

thefourtheye · 2015-11-17T00:48:15Z

LGTM

manvalls · 2015-11-17T07:36:56Z

@jasnell done

targos · 2015-11-17T07:54:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/761/

targos · 2016-01-31T14:12:20Z

@manvalls I'd like to land this. Could you rebase on master?

There's no need to add extra logic for it, `String.prototype.split` already has a `limit` argument. By using this argument we avoid keeping the whole array in memory, and V8 doesn't have to process the entire string.

manvalls · 2016-01-31T15:12:30Z

@targos done

targos · 2016-01-31T16:02:27Z

Thanks a lot.
CI: https://ci.nodejs.org/job/node-test-commit/2005/

There's no need to add extra logic for it, `String.prototype.split` already has a `limit` argument. By using this argument we avoid keeping the whole array in memory, and V8 doesn't have to process the entire string. PR-URL: #2288 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

targos · 2016-01-31T16:33:33Z

Landed in 27def4f

MylesBorins · 2016-02-03T22:43:05Z

@targos it looks like this commit is breaking body-parser. specifically when maxKeys is set to Infinity

MylesBorins · 2016-02-03T23:30:44Z

There is a very subtle change in behavior introduced with this patch.

String.prototype.split(value, Infinity) will always return an empty array.

The spec documents that the second argument must be an integer. Infinity is being interpreted as 0... instead of MAX_INT (perhaps this should be a fix upstream to v8).

In the past if querystring.parse was given Infinity, the split operation was unaffected.

body-parser is depending on this functionality, at the very least in their tests. We should likely find out how much querystring.parse is being called with infinity to see how much this will break.

I'm not 100% that the changes present here justify the edge case

MylesBorins · 2016-02-03T23:31:52Z

Here is a gist for a test if you want to experiment
https://gist.github.com/9b27b9320820d7bcdcfd

MylesBorins · 2016-02-03T23:34:02Z

It is worth mentioning that the documentation for querystring.parse does not mention anything about maxKeys not supporting Infinity

rvagg · 2016-02-04T00:08:13Z

@nodejs/ctc please tune briefly in to this, interesting result from smoke testing work that @thealphanerd has been doing, see nodejs/citgm#74

For the sake of being conservative about breaking changes, particularly when we have examples of people using it in the wild (and if there's one, there's bound to be more, particularly on a prominent project that people use as a reference), I'd +1 a special case here for checking Infinity. However, it does look like an happy accident that Infinity is being interpreted as 0.

MylesBorins · 2016-02-04T00:18:37Z

@rvagg I have submitted a PR to check for Infinity

#5066

MylesBorins · 2016-02-05T22:00:36Z

fix for this landed in 878bcd4

There's no need to add extra logic for it, `String.prototype.split` already has a `limit` argument. By using this argument we avoid keeping the whole array in memory, and V8 doesn't have to process the entire string. PR-URL: #2288 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

There's no need to add extra logic for it, `String.prototype.split` already has a `limit` argument. By using this argument we avoid keeping the whole array in memory, and V8 doesn't have to process the entire string. PR-URL: nodejs#2288 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

thefourtheye added the querystring Issues and PRs related to the built-in querystring module. label Aug 2, 2015

manvalls changed the title ~~querystring: use Array.prototype.split's limit~~ querystring: use String.prototype.split's limit Aug 3, 2015

mscdex changed the title ~~querystring: use String.prototype.split's limit~~ querystring: use Array.prototype.split's limit Aug 3, 2015

Fishrock123 changed the title ~~querystring: use Array.prototype.split's limit~~ querystring: use String.prototype.split's limit Aug 11, 2015

ronkorving reviewed Aug 12, 2015
View reviewed changes

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015

manvalls force-pushed the patch-1 branch from 2b78000 to 2fec309 Compare November 17, 2015 07:33

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

querystring: use String.prototype.split's limit

91a6cab

There's no need to add extra logic for it, `String.prototype.split` already has a `limit` argument. By using this argument we avoid keeping the whole array in memory, and V8 doesn't have to process the entire string.

manvalls force-pushed the patch-1 branch from 2fec309 to 91a6cab Compare January 31, 2016 15:09

targos closed this Jan 31, 2016

jasnell added lts-watch-v4.x and removed stalled Issues and PRs that are stalled. labels Feb 1, 2016

MylesBorins mentioned this pull request Feb 3, 2016

body-praser broken on node.js master nodejs/citgm#74

Closed

MylesBorins added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 3, 2016

MylesBorins added the dont-land-on-v4.x label Feb 3, 2016

MylesBorins removed lts-watch-v4.x semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 5, 2016

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

querystring: use `String.prototype.split`'s `limit` #2288

querystring: use `String.prototype.split`'s `limit` #2288

manvalls commented Aug 2, 2015

targos commented Aug 3, 2015

thefourtheye commented Aug 3, 2015

manvalls commented Aug 3, 2015

ronkorving Aug 12, 2015

manvalls Nov 16, 2015

jasnell commented Nov 16, 2015

targos commented Nov 16, 2015

jasnell commented Nov 16, 2015

thefourtheye commented Nov 17, 2015

manvalls commented Nov 17, 2015

targos commented Nov 17, 2015

targos commented Jan 31, 2016

manvalls commented Jan 31, 2016

targos commented Jan 31, 2016

targos commented Jan 31, 2016

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

rvagg commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

MylesBorins commented Feb 5, 2016

querystring: use String.prototype.split's limit #2288

querystring: use String.prototype.split's limit #2288

Conversation

manvalls commented Aug 2, 2015

targos commented Aug 3, 2015

thefourtheye commented Aug 3, 2015

manvalls commented Aug 3, 2015

ronkorving Aug 12, 2015

Choose a reason for hiding this comment

manvalls Nov 16, 2015

Choose a reason for hiding this comment

jasnell commented Nov 16, 2015

targos commented Nov 16, 2015

jasnell commented Nov 16, 2015

thefourtheye commented Nov 17, 2015

manvalls commented Nov 17, 2015

targos commented Nov 17, 2015

targos commented Jan 31, 2016

manvalls commented Jan 31, 2016

targos commented Jan 31, 2016

targos commented Jan 31, 2016

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

MylesBorins commented Feb 3, 2016

rvagg commented Feb 4, 2016

MylesBorins commented Feb 4, 2016

MylesBorins commented Feb 5, 2016

querystring: use `String.prototype.split`'s `limit` #2288

querystring: use `String.prototype.split`'s `limit` #2288