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.

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

'querystring.parse' works a inconsistent way with URLSearchParams(chrome) and python's urlparse.parse_qs #10454

Closed
AbnerZheng opened this issue Dec 26, 2016 · 4 comments
Labels
querystring Issues and PRs related to the built-in querystring module. url Issues and PRs related to the legacy built-in url module.

Comments

@AbnerZheng
Copy link

  • Version: 7.3.0
  • Platform:Mac
  • Subsystem: 10.12.1

when parse a paramsString like "&&q=test&topic=api", where '&&' appear together, Node will get the result like this:

querystring.parse("&&q=test&topic=api") //=>{ '': [ '', '' ], q: 'test', topic: 'api' }

But in other environment, such as Chrome console, we will get the result below:

var paramsString = "&&q=test&topic=api"
var searchParams = new URLSearchParams(paramsString);
for(let p of searchParams){ console.log(p);} //=>["q", "test"],["topic", "api"]

The same as python.

@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Dec 26, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 26, 2016

The querystring module is not intended to be 100% compatible with what browsers are doing.

If you're actually parsing a url, have you tried using the experimental WHATWG URL object in recent versions of node (require('url').URL)?

It doesn't seem that the WHATWG implementation is exporting its URLSearchParams, so currently it's only used when parsing a valid url. Perhaps that might be worth exporting though. /cc @jasnell

@TimothyGu
Copy link
Member

Currently even URLSearchParams is backed by querystring.parse. Tracking issue is at #10821.

/cc @nodejs/url

@watilde
Copy link
Member

watilde commented Jan 23, 2017

Like @TimothyGu said, we need to update the querystring.parse to fix them because of:

node/lib/internal/url.js

Lines 614 to 618 in 6b6123c

// Reused by the URL parse function invoked by
// the href setter, and the URLSearchParams constructor
function initSearchParams(url, init) {
url[searchParams] = getParamsFromObject(querystring.parse(init));
}

I opened a PR for it on #10967 😃

@joyeecheung joyeecheung added the querystring Issues and PRs related to the built-in querystring module. label Jan 24, 2017
watilde added a commit to watilde/node that referenced this issue Feb 1, 2017
+ update state machine in parse
  + repeated sep should be adjusted
  + `&=&=` should be `{ '': [ '', '' ] }`
+ add test cases for querystring and URLSearchParams

Fixes: nodejs#10454
watilde added a commit to watilde/node that referenced this issue Feb 1, 2017
+ update state machine in parse
  + repeated sep should be adjusted
  + `&=&=` should be `{ '': [ '', '' ] }`
+ add test cases for querystring and URLSearchParams

Fixes: nodejs#10454
@jasnell jasnell closed this as completed in 4e259b2 Feb 2, 2017
@TimothyGu TimothyGu reopened this Feb 4, 2017
@TimothyGu
Copy link
Member

Unfortunately, some cases still do not work as expected even after @watilde's fix:

> querystring.parse('a&&b')
{ a: '', '': '', b: '' }
> querystring.parse('a=a&&b=b')
{ a: 'a', '': '', b: 'b' }

watilde added a commit to watilde/node that referenced this issue Feb 4, 2017
- posIdx should save the current position instead of the last position.

Fixed cases:
- `a&&b` => `{ 'a': '', 'b': '' }`
- `a=a&&b=b` => `{ 'a': 'a', 'b': 'b' }`

Fixes nodejs#10454
mscdex added a commit to mscdex/io.js that referenced this issue Feb 8, 2017
This commit fixes handling of empty pairs that occur before the end
of the query string so that they are also ignored.

Additionally, some optimizations have been made, including:

* Avoid unnecessary code execution where possible
* Use a lookup table when checking for hex characters
* Avoid forced decoding when '+' characters are encountered and we
are using the default decoder

Fixes: nodejs#10454
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
* update state machine in parse
  * repeated sep should be adjusted
  * `&=&=` should be `{ '': [ '', '' ] }`
* add test cases for querystring and URLSearchParams

Fixes: nodejs#10454
PR-URL: nodejs#10967
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

No branches or pull requests

5 participants