How to know if JSON auto-parsing failed? #440

Open
aseemk opened this Issue Feb 24, 2013 · 8 comments

Projects

None yet

6 participants

@aseemk
aseemk commented Feb 24, 2013

Currently, request try-catches its JSON parsing:

https://github.com/mikeal/request/blob/v2.33.1/request.js#L879-L883

          if (self._json) {
            try {
              response.body = JSON.parse(response.body)
            } catch (e) {}
          }

As you mention in #99 (comment), this was intentional, and I understand why.

But it has one problem: I as a caller have no way of knowing whether the JSON has been successfully parsed or not. Specifically, if the response is malformed JSON, response.body remains a string, but if it's valid JSON representing a string, response.body is also a string.

This isn't just a theoretical problem; I ran into this recently when an API server started returning malformed JSON (with a 200 error code, because it's a streaming API) due to an out-of-memory error.

What do you suggest? Thanks again for the help!

(Edited to fix and pin link.)

@aseemk
aseemk commented Feb 24, 2013

One idea: perhaps add a response.json or response.isJSON property if JSON parsing succeeds?

@aseemk
aseemk commented Oct 2, 2013

Just checking in on this.

@benatkin
Contributor
benatkin commented Oct 2, 2013

A string isn't considered to be a JSON document according to the spec, so couldn't you just check that response.body is a string?

@mikeal
Member
mikeal commented Oct 2, 2013

i think we might just want to consider it an error.

@aseemk
aseemk commented Oct 2, 2013

@benatkin: According to the spec, yes, but a lot of JSON APIs return primitives directly since most JSON parsers (Node's included) accept them just fine. (JSON.parse('5') === 5)

@mikeal: That'd work for me. Your comment here #99 (comment) was that a lot of servers don't return JSON on internal server errors, which is true, but no harm in treating that as an error indeed then, instead of as a successful response.

(This relates a bit to #606 maybe, but not sure...)

@mikeal mikeal added the NeedsCode label Aug 28, 2014
@tbuchok
Contributor
tbuchok commented Oct 13, 2014

@mikeal i played around with emitting an error from the JSON.parse's try/catch, and on a GET i can see that being pretty clear. but afraid it might be a bit more confusing if i'm doing this:

READABLE_WITH_JSON.pipe(request(API_URL))
  .on('error', function(err) {
     // oops, the api server didn't reply with valid json back to me! is that really an error?
  }) 
;
  • how do you feel about enforcing { json: true } in the opts all the time -- GET, PUT, WHATEVR? does it take a bit of the convenience out to the library?
  • does erring ONLY if opts.json was explicitly provided help -- or is that even more confusing?

i think both would probably need to go in 3.0 as that behavior would be breaking?

here's a stab at response.json = true if the json parsing is correct, otherwise it'll be undefined:

tbuchok@61aed9f

@curiouslybrown

I've experimented with a lot of different approaches to this and concluded:

The only fool proof way to implement this would be to pass in an expected JSON schema. If the response doesn't validate against the schema, then error out.

@bblack
bblack commented Feb 10, 2016

i think we might just want to consider it an error.

this seems like the obvious solution. if i open such a pull request, will it be accepted?

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