JSON parse failure in defaultHttpResponseTransform

[anupbelambe]

function defaultHttpResponseTransform(data, headers) {
  if (isString(data)) {
    // Strip json vulnerability protection prefix and trim whitespace
    var tempData = data.replace(JSON_PROTECTION_PREFIX, '').trim();

    if (tempData) {
      var contentType = headers('Content-Type');
      **if ((contentType && (contentType.indexOf(APPLICATION_JSON) === 0)) || isJsonLike(tempData))** {
        data = fromJson(tempData);
      }
    }
  }

  return data;
}

In defaultHttpResponseTransform() above, the response data is parsed as json, if either content type is application/json or is JSON like. In this case the data is unable to parsed as JSON in fromJson(), the exception is thrown at JSON.parse, which causes request to be rejected with empty object.
This makes it hard to debug response issues in production especially if response is dynamically generated, as no response data can be captured in this case.
My suggestion would be to reject with original response data in such cases and let client handle the failure.

[gkalpak]

I assume that by "debug [...] in production" you mean logging client errors to the server and analysing them to find issues. For that usecase, this makes sense.

Of cource, you can create your own transform that captures that, but if someone wants to create a PR, I we can consider making this change in core.

[Narretz]

That's interesting, because prior to 1.6.0, a parse error wouldn't even be forwarded to the rejection handler / application code. Are people now finding new bugs in their apps because we let $q reject errors?

[gkalpak]

I think it would (but in a slightly different way).

Before 1.6:

1. Error
2. Promise rejection + $exceptionHandler

After 1.6:

1. Error
2. Promise rejection
3. If rejection not handled: $exceptionHandler (Possibly Unhandled Rejection)

[anupbelambe]

I assume that by "debug [...] in production" you mean logging client errors to the server and analysing them to find issues. For that usecase, this makes sense.

Yes @gkalpak

[chirag64]

Hi, I'm interested in attempting a PR for this. Is there any suggested approach you want me to follow to fix this?

[anupbelambe]

Can we catch the exception in defaultHttpResponseTransform and reject with original data in cases where JSON data cannot be parsed?

[gkalpak]

Off the top of my head, a reasonable approach would be to wrap the relevant code in a try-catch block, catch the error and throw a more informative error (containing the original).

This should be a minErr error though, so it needs an error page etc. Take a look here for an example of how this may look like.

[gkalpak]

@chirag64: Thx for stepping up btw 👍

[chirag64]

@gkalpak Thanks for the help, I've made some changes on my forked repository - 529cf2f. I'm not sure how I can test my changes or how I can call this function in a unit test (if a unit test is needed). Would appreciate your help on this. Thanks.