Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix #296 - Only set Content-Type if body exists #332

Merged
merged 1 commit into from Oct 21, 2012

Conversation

Projects
None yet
3 participants
Contributor

Marsup commented Sep 25, 2012

As discussed in the original issue, here is a quick fix, I hope it's what you had in mind.

Owner

mikeal commented Sep 25, 2012

this doesn't covering streaming.

Contributor

Marsup commented Sep 25, 2012

Sorry for the rushed PR, this time it should be ok hopefully.

It works on streams, I'm just wondering if it's the behavior we should expect or if json should override content-type.

+1 to being able to set json: true but only have it set a content-type if there's a body.

I was about to send a pull request then saw this one. My approach was pretty similar: HenrikJoreteg/request@1421be7

The reason I noticed this at all is that the body parser in Connect 2.5 tries to parse the body as JSON any time it sees a content-type of application/json so it blows up even when doing a GET with the json: true flag set in request unless you also send JSON in the body, which is silly. Although it's weird and not semantic to send a body on a GET, apparently it's not technically invalid.

I'm not sure it's completely practical to for connect to try to parse empty bodies on GET requests. But arguably TJ's right that it's invalid to set a JSON content type when there's no body (senchalabs/connect#415 (comment)).

There's obviously other workarounds, in that I could configure request different depending on if there's a body or not. Or not use the connect body parser... but it's kinda nice to just be able to set defaults to json: true and be stupid and happy.

Cheers!

@HenrikJoreteg HenrikJoreteg referenced this pull request in senchalabs/connect Oct 4, 2012

Closed

bodyParser should check for empty body #415

Contributor

Marsup commented Oct 20, 2012

@mikeal Is it good now ?

mikeal added a commit that referenced this pull request Oct 21, 2012

Merge pull request #332 from Marsup/issue-296
Fix #296 - Only set Content-Type if body exists

@mikeal mikeal merged commit 59cf5e6 into request:master Oct 21, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment