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

Incorrect documented use of qs when dealing with complex query parameters #4058

Closed
tundebabzy opened this issue Feb 21, 2020 · 7 comments
Closed
Assignees
Labels
support Questions, discussions, and general support

Comments

@tundebabzy
Copy link

Context

  • node version: 12.15.0
  • module version: 19.1.0

What are you trying to achieve or the steps to reproduce ?

I was trying to use qs as recommended by hapi's documentation (see https://hapi.dev/tutorials/routing/?lang=en_US#query) however unsurprisingly, it did not work.

The documentation recommends something like this:

const Hapi = require('@hapi/hapi');
const Qs = require('qs');

const server = Hapi.server({
    port: 3000,
    host: 'localhost',
    query: {
        parser: (query) => Qs.parse(query)
    }
});
...

The problem is that the query parameter in server.options.query.parser is an object not a string (See). For that reason, Qs.parse(query) is wrong since Qs.parse expects a string.

To get things working, I am manually building a query string; something like:

const query = [];
for (const key in request.query) {
  query.push(`${key}=${request.query[key]}`);
}
const opts = parse(query.join('&'));

Is there an undocumented way to receive the raw query string or to convert request.query to a string?

I already opened an issue in the hapi.dev repo (hapijs/hapi.dev#297) but I was directed to report here.

@tundebabzy tundebabzy added the documentation Non-code related changes label Feb 21, 2020
@yadomi
Copy link

yadomi commented Feb 27, 2020

I'm having the exact same issue, after update @hapi/hapi to v19.0.1 I can't access any of my GET routes because Joi will fail when expecting an array but since all values were converted to string I now receive literally a string with a square bracket inside "[]" (or any value that was passed in the request)

I've created a gist here to easily recreate the problem, when reaching http://localhost:3333/test?status=a,b

Without any validator, request.query is partially parsed. Which seems to be the normal behavior as explained here in the documentation but later it say to use server.options.query.parser with qs. However as @tundebabzy said, qs.parse expect a string, not an object but the argument passed to the parser function is cleary an object (see)

The problem seems to happen somewhere here:

hapi/lib/request.js

Lines 169 to 171 in 5796cd9

url.pathname = source.slice(0, q);
const query = h === -1 ? source.slice(q + 1) : source.slice(q + 1, h);
url.searchParams = Querystring.parse(query);

as url.searchParams is then passed to this._parseQuery here

this.query = this._parseQuery(url.searchParams);

Hope this help.

@ghost
Copy link

ghost commented Mar 13, 2020

Having the same issue. According to ljharb/qs#357 the qs confirms to support only strings for parsing parameters.

Maybe there could be an extra string parameter to parse function in addition to an object?

For example, changing the _setUrl function

hapi/lib/request.js

Lines 144 to 149 in d064f9c

_setUrl(source, stripTrailingSlash, { fast }) {
const url = this._parseUrl(source, { stripTrailingSlash, _fast: fast });
this.query = this._parseQuery(url.searchParams);
this.path = url.pathname;
}

like this

this._parseQuery(url.searchParams, url.search);

And add then add search parameter to the caller here

query = parser(query);

@ljharb
Copy link

ljharb commented Mar 13, 2020

note: qs.parse does in fact support an object parameter (although the docs only talk about a string, since that's the primary use case) but it assumes that in the object form, all the values have been parsed already (meaning, if you use the "comma" option, the object's values need to already have been split into arrays)

@hueniverse
Copy link
Contributor

hueniverse commented Mar 14, 2020

const Hapi = require('@hapi/hapi');
const Qs = require('qs');

const server = Hapi.server({
    port: 3000,
    host: 'localhost',
    query: {
        parser: (query) => Qs.parse(query)
    }
});

server.route({ path: '/', method: 'GET', handler: (request) => request.query });

server.start();

and...

$ curl http://localhost:3000/?s=4
> {"s":"4"}
$ curl http://localhost:3000/?s[a]=4
> {"s":{"a":"4"}}

I don't see a problem.

@hueniverse hueniverse added support Questions, discussions, and general support and removed documentation Non-code related changes labels Mar 14, 2020
@hueniverse hueniverse self-assigned this Mar 14, 2020
@ljharb
Copy link

ljharb commented Mar 14, 2020

I believe the issue is if you parse with the “comma” array format enabled, fwiw.

@ghost
Copy link

ghost commented Mar 14, 2020

Yes, it doesn't work for parsing arrays, even with

Qs.parse(query, {comma: true})
http://localhost:3000/?s=1,2,3,4,5

@hueniverse
Copy link
Contributor

Thanks @ljharb.

I consider this a qs/usage issue. hapi is not going to implement custom logic. If qs does not want to properly support its own API where you can pass either a string or an object with each key=value pair literals, then someone has to write a wrapper that implements the comma stuff...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
support Questions, discussions, and general support
Projects
None yet
Development

No branches or pull requests

4 participants