Options must allow to provide options to queryparser

[expressjs]

Options must allow to provide options to queryparser

[jonathanong]

@dougwilson

in the future, extended should be replaced by something like parse: qs.parse where parse is an actual function or something. then people could wrap qs.parse() with a function and their own options.

unless you want to support options. i'm not sure waht to call it. maybe just pass options itself to it? it's not like we'd support qs forever, so i'd rather not namespace the options to avoid api changes

[lazdmx]

+1 for accepting parse option with actual parser-function.

[dougwilson]

@lazutkin please check open issues first :) #41 is already about this. That issue had me open an issue with qs and then until yesterday, it wasn't even possible to change the limits in qs.

I would have had the options added in yesterday, but I can only work so fast between home and the hospital :)

@jonathanong it won't really work, especially since the core querystring module doesn't accept the options as an options object, but rather three different arguments (ugh). We will support qs forever because everyone seems to use it. If you look at the urlencoded code, you can see it does like nothing, so accepting a parsing option is dumb; #22 would be the real solution to using your own query string parser.

[dougwilson]

So as a FYI here, I implemented something that would allow changing the parameterLimit for qs, but it turned out that it doesn't make sense, because just giving the module 2000 parameters to parse (the default is a 1000 limit), it takes ~1710ms to parse. This means qs pretty much tops out at 1000 parameters (which takes 80ms to parse).

[lazdmx]

Heh, pretty long time)) Nevertheless, I think that it depends on projects so at least users should have a way to change options for what is appropriate to projects.

[dougwilson]

This module only works with web servers, and you cannot hang you web server for 2s, so no. You can always directly use qs or, since I submitted the slowness report to the qs owners, if it is fixed I will allow increasing it here.

Also note, I am taking about parameterLimit here, not arrayLimit like your original post was about. I don't let have any problems letting people change that option (doing some tests first to figure out a max. This limit is what prevents a DoS on servers. Without some limit for this, you can instantly crash a server due to OOM with a tiny packet).

[dougwilson]

@lazutkin it looks like qs is going to land the pref fix.

[dougwilson]

So the parameterLimit configuration has landed. Personally, w.r.t. the arrayLimit option, I would like to just set it to 0 so arrays just end up as objects always (really, a user can abuse your assumption that the thing is an array by adding a random [foo] to your array). A user can even send you something like a[0]=0&a[2]=2&a[length]=30 and you may think there are 30 items in the array.

[lazdmx]

Yes, reasonable!

[dougwilson]

@lazutkin since setting it to 0 is not very backwards-compatible at this time, I'm going to up the default value to 50, over the default of 20 provided by the qs library. How does that sound?

[dougwilson]

The main reason I'm not very keen on allowing directly passing in any options to the underlying query parser is then if the parser itself is switched by this module, it would change the public API of this module. Plus it doesn't allow me to normalize options where the two different parsers will take the same option names (like parameterLimit, which is maxKeys for the built-in parser).