querystring: fix state machine in querystring.parse

[watilde]

From this comment: #10454 (comment). The posIdx should save the current position instead of the last position, and I've added the test cases.

Fixed cases:

• a&&b => { 'a': '', 'b': '' }
• a=a&&b=b => { 'a': 'a', 'b': 'b' }

Fixes #10454

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

querystring

[TimothyGu]

Some more bad cases, even after the patch (sorry...):

> querystring.parse('&a')
{ '': '', a: '' }
// { a: '' }
> querystring.parse('&=')
{ '': [ '', '' ] }
// { '': '' }
> querystring.parse('a&a&')
{ a: '' }
// { a: [ '', '' ] }
> querystring.parse('a&a&a&')
{ a: '' }
// { a: [ '', '', '' ] }
> querystring.parse('a&a&a&a&')
{ a: '' }
// { a: [ '', '', '', '' ] }

[watilde]

@TimothyGu That's a great catch! Thanks<3 I've added a commit to supporting the cases.
The problem was the logic didn't add the value to obj when curValue is '' in else if (curValue). It should be else. Plus, I noticed that it became unnecessary to evaluate i directly and also delete obj[key], because of the logic updated with the first commit in this PR :)

[mscdex]

This is why I wanted to take a look at the original PR before landing as the delete should never have been necessary in the first place.

On a more general note, I'm not particularly keen on changing querystring.parse() (and slowing it down) just to satisfy URLSearchParams. URLSearchParams should just have its own implementation if it requires different behavior.

[joyeecheung]

I think the fastest way we can fix this in URLSearchParams without affecting querystring is to simply copy the files over, and then make changes in that copy? We can completely reimplement the querystring part later, but right now we are focusing on spec compliance so this is the fastest way to catch up on the spec(not necessarily the best way for sure).

cc @jasnell

[joyeecheung]

Also if we do that, when we start to redo the parsing/serialization bit of URLSearchParams we will be refactoring a spec-complaint fork of querystring instead of reimplementing it from scratch.

[domenic]

It does seem like creating a separate spec-complaint parser for URL queries might be a good path. The algorithm doesn't seem that hard to just implement from scratch: https://url.spec.whatwg.org/#concept-urlencoded-parser

(Although, the spec's pipeline of JS string --UTF-8 encode--> bytes --percent decode--> bytes --UTF-8 decode without BOM--> strings seems a bit inefficient, and hopefully you can the same results by just staying in string-land.)

[jasnell]

yeah, I'll look at creating a separate parser this next week.

[TimothyGu]

@jasnell, you might find my C++ implementation to be interesting, since the spec is fairly declarative on what needs to be done. Though performance-wise I think a JS implementation might be faster.

[mscdex]

I've benchmarked the combined changes in #10967 and this PR against master prior to either of these commits, and on at least one of the benchmark combinations (multicharsep) there is ~5.5% performance regression with a high "confidence" rating.

I am working on and benchmarking an alternative solution now which should hopefully avoid the performance regression(s).

[stevenvachon]

Another useful test case:

> require("querystring").parse("a=&a=value&a=")
{ a: [ '', '' ] }
// { a: [ '', 'value', '' ] }

… and already solved by this patch.

[mscdex]

Alright, I've tested my much simpler changes which should cover the needs of the original PR and this PR, including the tests they introduce (and @stevenvachon's test), and after adjusting the benchmarks to allow the function "priming" to occur more than once to ensure any re-optimizing ("definitely") stays outside of the timed loop, there does not seem to be any high "confidence" negative regressions anymore. Also just judging by this new code change alone, I'm confident that the code will not negatively affect the other code paths.

I will submit a PR as soon as the more thorough benchmarking that I currently have running finishes and agrees with my preliminary results.

[watilde]

@mscdex That sounds great! I have a keen interest in it, let's see what will happen :)
Probably, this PR will be closed depending on your patch.

[mscdex]

My proposed solution has now been pushed: #11234

[jasnell]

@mscdex ... to be clear, #11234 is an alternative to this one, yes?

[watilde]

I think so, and that patch was great! I will close this to jump to #11234. Thanks 😃

[mscdex]

@jasnell Yes