auto parse application/json #99

Closed
timoxley opened this Issue Oct 27, 2011 · 10 comments

Comments

Projects
None yet
4 participants
@timoxley

Is there a reason why request doesn't (or isn't) automatically JSON.parsing the body if it detects the application/json* content-type header (or perhaps if the url ends in .json?

my current json header is:

Content-Type: application/json; charset=utf-8

eg:

 request('http://localhost:4000/datas.json'}, function(error, response, body) {
    console.log(typeof body) // = string! but the server returned JSON
}
request({url: 'http://localhost:4000/datas.json', json: true}, function(error, response, body) {
    console.log(typeof body) // = object. great, but the json:true could be inferred.
}
@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Oct 27, 2011

Owner

It's just dangerous.

Auto-parsing it means that if a server changes it's content-type your application will start throwing exceptions even though no code changes happened on your side. You can get json parsing with json:true

var r = request.defaults({json:true})
r('http://localhost:4000/data.json')
Owner

mikeal commented Oct 27, 2011

It's just dangerous.

Auto-parsing it means that if a server changes it's content-type your application will start throwing exceptions even though no code changes happened on your side. You can get json parsing with json:true

var r = request.defaults({json:true})
r('http://localhost:4000/data.json')
@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Nov 10, 2011

Owner

closing.

Owner

mikeal commented Nov 10, 2011

closing.

@mikeal mikeal closed this Nov 10, 2011

@aseemk

This comment has been minimized.

Show comment Hide comment
@aseemk

aseemk Feb 24, 2013

Hey @mikeal,

Auto-parsing it means that if a server changes it's content-type your application will start throwing exceptions even though no code changes happened on your side.

Won't that happen even with the current way?

var r = request.defaults({json:true})
r('http://localhost:4000/data.json')
// this will throw an exception if the server changes its content-type to no longer be JSON

aseemk commented Feb 24, 2013

Hey @mikeal,

Auto-parsing it means that if a server changes it's content-type your application will start throwing exceptions even though no code changes happened on your side.

Won't that happen even with the current way?

var r = request.defaults({json:true})
r('http://localhost:4000/data.json')
// this will throw an exception if the server changes its content-type to no longer be JSON
@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Feb 24, 2013

Owner

nope, {json:true} attempts to decode the JSON body, if it fails it does not throw. this is intentional, many servers do not encode the body to JSON when errors occur. this is also why many people don't use the json option in request, and why i won't assume it. it's more efficient to do

request(url, function (e, r, b) { if (r.statusCode === 200) JSON.parse(b); else cb(new Error(resp.statusCode))})
Owner

mikeal commented Feb 24, 2013

nope, {json:true} attempts to decode the JSON body, if it fails it does not throw. this is intentional, many servers do not encode the body to JSON when errors occur. this is also why many people don't use the json option in request, and why i won't assume it. it's more efficient to do

request(url, function (e, r, b) { if (r.statusCode === 200) JSON.parse(b); else cb(new Error(resp.statusCode))})
@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Feb 24, 2013

Owner

to sum this up, {json:true} is a convenience, and its useful but its a trade off. what i won't do is sacrifice predictability in request's defaults for the convenience.

Owner

mikeal commented Feb 24, 2013

to sum this up, {json:true} is a convenience, and its useful but its a trade off. what i won't do is sacrifice predictability in request's defaults for the convenience.

@aseemk

This comment has been minimized.

Show comment Hide comment
@aseemk

aseemk Feb 24, 2013

Hmm, okay. If this won't change, alas. =D Thanks for the consideration.

Just wanted to point out that you can still keep the "attempt" part (try-catching JSON.parse), just do it based on the response's Content-Type rather than tying it to "send my request as JSON too".

I ask because I've run into cases, too, where I want to send a non-JSON request (either POST form data or PUT a binary file), but I expect JSON responses.

Not a huge deal. You're right that it's for convenience. The current option just feels lacking in this regard.

aseemk commented Feb 24, 2013

Hmm, okay. If this won't change, alas. =D Thanks for the consideration.

Just wanted to point out that you can still keep the "attempt" part (try-catching JSON.parse), just do it based on the response's Content-Type rather than tying it to "send my request as JSON too".

I ask because I've run into cases, too, where I want to send a non-JSON request (either POST form data or PUT a binary file), but I expect JSON responses.

Not a huge deal. You're right that it's for convenience. The current option just feels lacking in this regard.

@aseemk

This comment has been minimized.

Show comment Hide comment
@aseemk

aseemk Feb 24, 2013

@magicdawn

This comment has been minimized.

Show comment Hide comment
@magicdawn

magicdawn Nov 4, 2016

Yeah, superagent supports auto parse

Yeah, superagent supports auto parse

@magicdawn

This comment has been minimized.

Show comment Hide comment
@magicdawn

magicdawn Nov 4, 2016

And using a { form: { xxx }, json: true } option will generate content-type: application/x-www-form-urlencoded request

I just do not understand, we use form / formData to get urlencoded / multipart content-type request, but why json: true will control the request header?

I think json just control the response, and parse the body as json, the request content-type depends ontypeof options.body is better.

  • string -> text/plain
  • object -> application/json

magicdawn commented Nov 4, 2016

And using a { form: { xxx }, json: true } option will generate content-type: application/x-www-form-urlencoded request

I just do not understand, we use form / formData to get urlencoded / multipart content-type request, but why json: true will control the request header?

I think json just control the response, and parse the body as json, the request content-type depends ontypeof options.body is better.

  • string -> text/plain
  • object -> application/json
@magicdawn

This comment has been minimized.

Show comment Hide comment
@magicdawn

magicdawn Nov 4, 2016

Anyway. switching to request-promise

Anyway. switching to request-promise

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