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

How to handle error from JSON.parse #61

Closed
mzabriskie opened this issue Apr 15, 2015 · 21 comments
Closed

How to handle error from JSON.parse #61

mzabriskie opened this issue Apr 15, 2015 · 21 comments

Comments

@mzabriskie
Copy link
Member

The transformRequest method attempts to parse JSON, but swallows any error in an empty catch statement. This results in the stringified form of the response body to be returned should the JSON be malformed, and supplied to the response object that is used to resolve the Promise.

Without the try/catch a global error would occur. With the try/catch the code handling the response will error, but in an unpredictable way. Consider:

// Assume a malformed response from the server for the request.
// Missing quotes around property names:
//   {firstName: "Jimmy", lastName: "Page", isBlocked: true}
axios.get('/user/12345').then((res) => {
  var user = res.data;
  if (!user.isBlocked) {
    // Do something potentially dangerous that only an unblocked user should be allowed to do
  }
});

In this case user is a String not an Object as expected. Calling 'some string'.isBlocked will return undefined, which evaluates as falsey. This will cause the if condition to erroneously be entered.

A potential solution would be to reject the Promise if JSON.parse throws an Error. This isn't a straight forward fix though as it would break #55 again.

@jamesseanwright
Copy link

Personally, I think Promise rejection is a good solution. At the very least, one could invoke console.error in the catch block to make developers aware of what has happened.

@demetriusnunes
Copy link

I also think there should be an easy option to retry the request in these cases as it often boils down to a truncated response.

@youurayy
Copy link

Just got burned on this. Instead of correctly rejecting the promise on invalid (truncated) JSON, it simply returns the orignal string object. Bad bad bad. At least put a warning in the docs about this.

@rubennorte
Copy link
Member

I think we should just reject with the parse errors. That'd make axios more predictable and easier to debug.

@batiste
Copy link

batiste commented May 30, 2017

I also spent 1 hour today tryng to understand what is happening with some invalid JSON. Just reject the Promise, or at least console.log something...?

@youurayy
Copy link

imagine first finding out in production - not sure where the logical thinking has gone on this one

@vinnymac
Copy link

vinnymac commented Mar 1, 2018

Not sure how others are avoiding this issue after almost 3 years, but I decided to try to work around it for one of my projects. I would like to see how other people have been doing this as well.

I took notes from here and only raise an error for 2xx response codes.

If people find it useful I can make a PR of my commit. If you need this to raise for other response codes, just remove lines 67 and 71 in defaults.js

Another option for those who don’t want to fork is to just override transformResponse, and detect when data is a string, and parse on your own. Unfortunately this means you’ll do more work, and you can’t check the status code if you do that, but at least you have options.

@thame
Copy link

thame commented May 19, 2018

I also encountered a scenario where this behavior is problematic. If the network connection is interrupted partially through the request, axios would not call an error and my application received malformed JSON which obviously led to lots of errors.

I too think that JSON parse errors should be caught and reported. I wasn't keen on forking or modifying the source files so my solution was to transform the response like this:

axios.get(URL, {
  transformResponse: [(response) => (JSON.parse(response))], 
})

@jmkenz
Copy link

jmkenz commented Aug 9, 2018

My approach to handling this is to wrap all axios requests in a promise, and reject that promise if the response from axios show a response.headers['content-type'] of 'application/json' and a response.data that is a string. From this I infer there was a JSON parsing error.

return new Promise((resolve, reject) => {
  axios.request(options)
    .then(response => {
      if (response.headers && response.headers['content-type'] && response.headers['content-type'].indexOf('application/json') >= 0 && _.isString(response.data)) {
        reject(new Error('Error parsing response data as JSON object'));
      } else {
        resolve(response);
      }
  });
});

@youurayy
Copy link

youurayy commented Aug 9, 2018

@emilyemorehouse is this something you could fix, for the sake of all humanity?

@wujunze
Copy link

wujunze commented Dec 29, 2018

Should throw exception

@soundstep
Copy link

+1 here, swallowing errors is never good

@ethanentW
Copy link

This is among the most disappointing responses to a glaring issue I've seen in an open-source project. This has been an issue for almost 4 years... This is the reason I use my own HTTP package.

@todorone
Copy link

todorone commented Oct 9, 2019

@vinnymac Can You please submit PR of Your solution?

@smakinson
Copy link

Just came across this as well, it seems if I manually add: responseType: 'json' (even though it's the default according to the docs) it at least keeps it form providing the bad data to the result.

@rognstad
Copy link

rognstad commented Sep 21, 2020

I just got bitten by this in production today. The current behavior is really unintuitive and wasted a bunch of my time in debugging. Throwing an error would have pointed me in the right direction immediately and would have been helpfully caught/grouped in our error reporting. Even once I found that there was a disconnect between the type of data I got in the response vs. what the header said it should be, it wasn't trivial to find this issue as the explanation.

@yarcl
Copy link

yarcl commented Dec 24, 2020

Is this responseText a problem? like "24\r\n{"success":"true", "payload":"123456"}\r\n0\r\n\r\n", and sometimes it returns "24\r\n{"success":"true", "payload":"123456"}\r\n". Is there anyone who can help, thanks a lot.

@ouzkhanmete
Copy link

imagine first finding out in production - not sure where the logical thinking has gone on this one

Generally speaking, that was exactly what we faced....

@porlov
Copy link

porlov commented Mar 16, 2021

Guys, any updates on this issue?
This is really confusing behavior that I've faced with recently.

jasonsaayman pushed a commit that referenced this issue Apr 19, 2021
…be passed directly as payload for encoding to JSON #2613, #61, #907 (#3688)

* Draft

* Added support for primitive types to be converted to JSON if the request Content-Type is 'application/json';
Added throwing SyntaxError if JSON parsing failed and responseType is json;
Added transitional option object;
Added options validator to assert transitional options;
Added transitional option `silentJSONParsing= true` for backward compatibility;
Updated README.md;
Updated typings;

* Fixed isOlderVersion helper;
Fixed typo;
Added validator.spec.js;

* Added forcedJSONParsing transitional option #2791

* `transformData` is now called in the default configuration context if the function context is not specified (for tests compatibility);

* Added `transitional.clarifyTimeoutError` to throw ETIMEDOUT error instead of generic ECONNABORTED on request timeouts;
Added support of onloadend handler if available instead of onreadystatechange;
Added xhr timeout test;
Fixed potential bug of xhr adapter with proper handling timeouts&errors (FakeXMLHTTPRequest failed to handle timeouts);
@jasonsaayman
Copy link
Member

Fixed in #3688

@pcone
Copy link

pcone commented Nov 8, 2021

I see this has been closed already, but imo neither of the available behaviours from silentJSONParsing are quite perfect for all use cases.
With silentJSONParsing on you can access the status code of the response, but the JSON parsing error is swallowed which isn't ideal.
But with silentJSONParsing off there's no longer any way to see what the status code of the response was, unless I'm missing something.

Effectively it seems like I have to choose between being able to know the status code or the JSON parsing error, but not both.

mbargiel pushed a commit to mbargiel/axios that referenced this issue Jan 27, 2022
…be passed directly as payload for encoding to JSON axios#2613, axios#61, axios#907 (axios#3688)

* Draft

* Added support for primitive types to be converted to JSON if the request Content-Type is 'application/json';
Added throwing SyntaxError if JSON parsing failed and responseType is json;
Added transitional option object;
Added options validator to assert transitional options;
Added transitional option `silentJSONParsing= true` for backward compatibility;
Updated README.md;
Updated typings;

* Fixed isOlderVersion helper;
Fixed typo;
Added validator.spec.js;

* Added forcedJSONParsing transitional option axios#2791

* `transformData` is now called in the default configuration context if the function context is not specified (for tests compatibility);

* Added `transitional.clarifyTimeoutError` to throw ETIMEDOUT error instead of generic ECONNABORTED on request timeouts;
Added support of onloadend handler if available instead of onreadystatechange;
Added xhr timeout test;
Fixed potential bug of xhr adapter with proper handling timeouts&errors (FakeXMLHTTPRequest failed to handle timeouts);
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

Successfully merging a pull request may close this issue.