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

Request body can't be querystring.parse(str) #212

Closed
bisubus opened this issue Jul 14, 2016 · 8 comments
Closed

Request body can't be querystring.parse(str) #212

bisubus opened this issue Jul 14, 2016 · 8 comments

Comments

@bisubus
Copy link
Contributor

bisubus commented Jul 14, 2016

When the request is supplied with body that differs from { ... } object, it throws

options.body must be a ReadableStream, string, Buffer or plain Object

is-plain-object expects an object to inherit from either null or Object directly.

This makes

got.post(url, { data: querystring.parse(query) });

to fail, because querystring.parse(...) object inherits from empty object that inherits from null.

This looks more like a bug rather than zealous sanitization.

@floatdrop
Copy link
Contributor

@bisubus I see positive side in this – you should explicitly set fields, that you are sending.

P.s. why you are passing result of querystring.parse to body? Why not just pass query as is?

@bisubus
Copy link
Contributor Author

bisubus commented Aug 4, 2016

@floatdrop Why to not just get own enumerable properties from the object in this case? As for the example above, the workaround should be Object.assign({}, querystring.parse(query)), it doesn't add much clarity.

I'm quite sure (don't have this piece of code at hand at this moment) that this was done because got.post(url, { data: query }) makes a text/plain request and not application/x-www-form-urlencoded, not a very good thing for POST request (maybe a subject for another issue?)

@floatdrop
Copy link
Contributor

makes a text/plain request and not application/x-www-form-urlencoded, not a very good thing for POST request (maybe a subject for another issue?)

This is right behaviour, if you pass String to body. Maybe we should move from is-plain-obj to is-obj.

@bisubus mind to make a PR?

@bisubus
Copy link
Contributor Author

bisubus commented Aug 4, 2016

@floatdrop

Sure, will do this. I see that is-obj allows functions. Wouldn't it be better to stick to util.isObject instead?

@sindresorhus
Copy link
Owner

@bisubus Functions are objects. util.isObject is deprecated.

@bisubus
Copy link
Contributor Author

bisubus commented Aug 4, 2016

@sindresorhus That's correct. This requires this spec to be fixed but otherwise it looks fine. Function type shouldn't be reserved for future use as callbacks here, should it?

@sindresorhus
Copy link
Owner

@bisubus We could always just do isObj(body) && body !== 'function'.

@bisubus
Copy link
Contributor Author

bisubus commented Aug 4, 2016

@sindresorhus My guess that the code cannot gain much from is-obj dependency then, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants