Parse json: Issue #51 #53

Merged
merged 4 commits into from Aug 2, 2011

Projects

None yet

5 participants

@benatkin
Contributor
benatkin commented Aug 2, 2011

Here's a fix for #51. It parses the JSON if the following conditions are met:

  • the json option is truth
  • the response content type is application/json
  • the response body isn't empty

Both response.body and the body parameter in the callback are set to the parsed JSON. I wasn't sure if I should leave response.body unparsed and only parse the body that's passed to the function. I chose to make them both parsed. The raw JSON can be obtained by setting the request header and body manually instead of using the JSON option.

Another thing to consider is passing on a JSON parse exception to the callback if the JSON doesn't parse, instead of letting the exception be thrown. Should I be doing that?

@mikeal
Member
mikeal commented Aug 2, 2011

i like.

hrm.....

some people write lazy servers that don't return the right content-type. should we maybe just wrap the JSON.parser in a try block and always attempt it when json:true?

@mikeal mikeal and 1 other commented on an outdated diff Aug 2, 2011
main.js
@@ -289,7 +289,10 @@ Request.prototype.request = function () {
})
options.on("end", function () {
response.body = buffer
- options.callback(null, response, buffer)
+ if (options.json && response.headers['content-type'] === 'application/json' && response.body != '') {
@mikeal
mikeal Aug 2, 2011 Member

might want to do (response.body.length) as the check rather than a string compare. it's a little faster and probably safer since there is no coercion to worry about.

@benatkin
benatkin Aug 2, 2011 Contributor

oops - I meant to do !==. But since we know for sure it's going to be a string, response.length works! (null.length throws an exception)

@benatkin
benatkin Aug 2, 2011 Contributor

However, that check is no longer necessary if I wrap JSON.parse with a try block and silently keep the old value if it doesn't parse.

@benatkin
Contributor
benatkin commented Aug 2, 2011

Yes! That's better...since this is a simple interface rather than a more direct HTTP interface. As I said above, if I want direct, I can set headers and body instead of json and parse it myself!

@benatkin
Contributor
benatkin commented Aug 2, 2011

I updated it. Oops...said write instead of right in the commit message.

@mikeal mikeal merged commit 4bf5861 into request:master Aug 2, 2011
@mikeal
Member
mikeal commented Aug 2, 2011

looks great, thanks!

@aseemk
aseemk commented on 68c17f6 Aug 14, 2011

Gah, this change totally broke our site, and it took a crazy long time to find and debug this...

I like the change, but it's really not nice to introduce breaking changes in a bugfix update! =/

@rgrove

What if the content-type is "application/json;charset=utf-8"?

Contributor

Hmm...my update to that code didn't get merged. We were just going to try parsing all responses to a request with a json property as JSON (search for "might want to"). If I were going to detect a JSON content type I might use this test: if it starts with application/json or contains +json, it's JSON.

@timoxley

+1 I'm currently having issues with returning a string for content-type application/json;charset=utf-8, which is a bit frustrating since I believe this is being set by a 3rd party library and I don't want to have to hack it just to get a consistent content-type

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