Skip to content

Commit

Permalink
fix(common): attempt to JSON.parse errors for JSON responses (#19773)
Browse files Browse the repository at this point in the history
Prior behavior for HttpClient was to parse the body as JSON when
responseType was set to 'json', even if the response was
unsuccessful. This changed due to a recent bugfix, and
unsuccessful responses had their bodies treated as strings.

There is no guarantee that if a service returns JSON in the
successful case that it will do so for errors. However, users
indicate that most APIs in the wild do work this way. Therefore,
this commit changes the error case behavior to attempt a JSON
parsing of the response body, and falls back on returning it as
a string if that fails.

PR Close #19773
  • Loading branch information
alxhub authored and tbosch committed Oct 18, 2017
1 parent 6e6c866 commit 269f5ac
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 1 deletion.
8 changes: 8 additions & 0 deletions packages/common/http/src/xhr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,14 @@ export class HttpXhrBackend implements HttpBackend {
// The parse error contains the text of the body that failed to parse.
body = { error, text: body } as HttpJsonParseError;
}
} else if (!ok && req.responseType === 'json' && typeof body === 'string') {
try {
// Attempt to parse the body as JSON.
body = JSON.parse(body);
} catch (error) {
// Cannot be certain that the body was meant to be parsed as JSON.
// Leave the body as a string.
}
}

if (ok) {
Expand Down
9 changes: 8 additions & 1 deletion packages/common/http/test/xhr_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {MockXhrFactory} from './xhr_mock';

function trackEvents(obs: Observable<HttpEvent<any>>): HttpEvent<any>[] {
const events: HttpEvent<any>[] = [];
obs.subscribe(event => events.push(event));
obs.subscribe(event => events.push(event), err => events.push(err));
return events;
}

Expand Down Expand Up @@ -92,6 +92,13 @@ export function main() {
const res = events[1] as HttpResponse<{data: string}>;
expect(res.body !.data).toBe('some data');
});
it('handles a json error response', () => {
const events = trackEvents(backend.handle(TEST_POST.clone({responseType: 'json'})));
factory.mock.mockFlush(500, 'Error', JSON.stringify({data: 'some data'}));
expect(events.length).toBe(2);
const res = events[1] as any as HttpErrorResponse;
expect(res.error !.data).toBe('some data');
});
it('handles a json string response', () => {
const events = trackEvents(backend.handle(TEST_POST.clone({responseType: 'json'})));
expect(factory.mock.responseType).toEqual('text');
Expand Down

0 comments on commit 269f5ac

Please sign in to comment.