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.

[Narretz]

@chirag64 Yes, we definitely need a test for this. The tests are in httpSpec.js, this should be in the describe block for json parsing. Once you have cloned the repo you can run grunt autotest to start the karma server, which will then run all the tests. You can isolate a single test by changing the it to fit.

[chirag64]

@Narretz So inside the httpSpec.js file, I created a new describe block for json parsing, and inside an it function, I'm trying to run defaultHttpResponseTransform but the tests keep erroring out saying defaultHttpResponseTransform is not defined no-undef. I checked the httpSpec file to see how we're getting the other functions to work but couldn't find a pattern. Any hints on this?

[anupbelambe]

@chirag64 in your forked repository 529cf2f, can you throw tempData instead of data as data would contain empty JS once exception is encountered.

      throw minErr('$http')('baddata', 'Data must be a valid JSON object. Received: {0}', **data**);

[Narretz]

@chirag64

There's actually a section about the json already in the httpSpec.js:

angular.js/test/ng/httpSpec.js

Line 1186 in 71f437c

describe('response', function() {

There's a test about JSON deserialize errors:

angular.js/test/ng/httpSpec.js

Line 1372 in 71f437c

it('should forward json deserialization errors to the http error handler',

Instead of a SyntaxError, we have to match for a minErr now:

angular.js/test/helpers/matchers.js

Line 308 in 71f437c

toEqualMinErr: function() {

It's also better to open a PR, so we can see the difference right away.

@anupbelambe I don't really understand what you mean.

[Narretz]

A problem I see is that with this behavior, you won't have access to the raw response string (requested e.g. here #12012). Basically, there's two different requirements: a) specific error when parsing fails for debug reasons b) access to the original string for error recovery

Can we achieve this together with a minErr? Wdyt @gkalpak?

[gkalpak]

Sounds reasonable. We could embed the response (as string) into the minErr (and hope it is not huge 😛).

[chirag64]

Still unsure how to call defaultHttpResponseTransform in httpSpec.js for unit testing. Would appreciate some help with that and comments on the checked in code and documentation.

[anupbelambe]

Thanks @gkalpak and @chirag64.

I see this has been added to backlog milestone but in which Angular release would this fix be available.

[gkalpak]

v1.6.4