Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($http): JSON parse failure #15724

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/content/error/$http/baddata.ngdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
@ngdoc error
@name $http:baddata
@fullName Bad JSON Data
@description

The default @{link ng.$http#default-transformations `transformResponse`} will try to parse the
response as JSON if the `Content-Type` header is `application/json` or the response looks like a
valid JSON-stringified object or array.
This error occurs when that data is not a valid JSON object.

The error message should provide additional context such as the actual response.

To resolve this error, make sure you pass valid JSON data to `transformResponse` or use an
appropriate `Content-Type` header for non-JSON data.
7 changes: 6 additions & 1 deletion src/ng/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ function defaultHttpResponseTransform(data, headers) {
if (tempData) {
var contentType = headers('Content-Type');
if ((contentType && (contentType.indexOf(APPLICATION_JSON) === 0)) || isJsonLike(tempData)) {
data = fromJson(tempData);
try {
data = fromJson(tempData);
} catch (e) {
throw $httpMinErr('baddata', 'Data must be a valid JSON object. Received: "{0}". ' +
'Parse error: "{1}"', data, e);
}
}
}
}
Expand Down
10 changes: 4 additions & 6 deletions test/ng/httpSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1369,17 +1369,15 @@ describe('$http', function() {
}
);

it('should forward json deserialization errors to the http error handler',
function() {
it('should return JSON data with error message if JSON is invalid', function() {
var errCallback = jasmine.createSpy('error');

$httpBackend.expect('GET', '/url').respond('abcd', {'Content-Type': 'application/json'});
$http({method: 'GET', url: '/url'}).then(callback).catch(errCallback);
$httpBackend.expect('GET', '/url').respond('{abcd}', {'Content-Type': 'application/json'});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replaced the it function of forwarding json deserialization errors with mine. The above highlighted statements are pretty much similar to the old code except that we're using an object '{abcd}' within the string instead of a direct string 'abcd'. Are you referring to that string or something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine with me. @gkalpak wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chirag64, yes, I am referring to the string. Why change abcd to {abcd}?
I am mostly curious - I don't think it makes any difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to make it look clear that we're passing an invalid JSON instead of passing a string.

$http.get('/url').then(callback).catch(errCallback);
$httpBackend.flush();

expect(callback).not.toHaveBeenCalled();
expect(errCallback).toHaveBeenCalledOnce();
expect(errCallback.calls.mostRecent().args[0]).toEqual(jasmine.any(SyntaxError));
expect(errCallback.calls.mostRecent().args[0]).toEqualMinErr('$http', 'baddata');
});

});
Expand Down