From 500a46d5f29d51720c6772273057165b7d593e0c Mon Sep 17 00:00:00 2001 From: Frederik Prijck Date: Tue, 21 Feb 2017 23:42:16 +0100 Subject: [PATCH 1/3] feature($http): allow differentiation between Request Error, Abort and Timeout Fixes #15924 --- src/ng/http.js | 18 +++++----- src/ng/httpBackend.js | 25 ++++++++++---- src/ngMock/angular-mocks.js | 8 ++--- test/ng/httpBackendSpec.js | 57 +++++++++++++++++++++++++++++++- test/ng/httpSpec.js | 34 +++++++++++++++++++ test/ngMock/angular-mocksSpec.js | 52 ++++++++++++++++++----------- 6 files changed, 154 insertions(+), 40 deletions(-) diff --git a/src/ng/http.js b/src/ng/http.js index a4b54f68fad5..7d05914bbbe1 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -431,6 +431,7 @@ function $HttpProvider() { * - **headers** – `{function([headerName])}` – Header getter function. * - **config** – `{Object}` – The configuration object that was used to generate the request. * - **statusText** – `{string}` – HTTP status text of the response. + * - **xhrStatus** – `{string}` – Status of the XMLHttpRequest (`complete`, `error`, `timeout` or `abort`). * * A response status code between 200 and 299 is considered a success status and will result in * the success callback being called. Any response status code outside of that range is @@ -1269,9 +1270,9 @@ function $HttpProvider() { } else { // serving from cache if (isArray(cachedResp)) { - resolvePromise(cachedResp[1], cachedResp[0], shallowCopy(cachedResp[2]), cachedResp[3]); + resolvePromise(cachedResp[1], cachedResp[0], shallowCopy(cachedResp[2]), cachedResp[3], cachedResp[4]); } else { - resolvePromise(cachedResp, 200, {}, 'OK'); + resolvePromise(cachedResp, 200, {}, 'OK', 'complete'); } } } else { @@ -1328,10 +1329,10 @@ function $HttpProvider() { * - resolves the raw $http promise * - calls $apply */ - function done(status, response, headersString, statusText) { + function done(status, response, headersString, statusText, xhrStatus) { if (cache) { if (isSuccess(status)) { - cache.put(url, [status, response, parseHeaders(headersString), statusText]); + cache.put(url, [status, response, parseHeaders(headersString), statusText, xhrStatus]); } else { // remove promise from the cache cache.remove(url); @@ -1339,7 +1340,7 @@ function $HttpProvider() { } function resolveHttpPromise() { - resolvePromise(response, status, headersString, statusText); + resolvePromise(response, status, headersString, statusText, xhrStatus); } if (useApplyAsync) { @@ -1354,7 +1355,7 @@ function $HttpProvider() { /** * Resolves the raw $http promise. */ - function resolvePromise(response, status, headers, statusText) { + function resolvePromise(response, status, headers, statusText, xhrStatus) { //status: HTTP response status code, 0, -1 (aborted by timeout / promise) status = status >= -1 ? status : 0; @@ -1363,12 +1364,13 @@ function $HttpProvider() { status: status, headers: headersGetter(headers), config: config, - statusText: statusText + statusText: statusText, + xhrStatus: xhrStatus }); } function resolvePromiseWithResult(result) { - resolvePromise(result.data, result.status, shallowCopy(result.headers()), result.statusText); + resolvePromise(result.data, result.status, shallowCopy(result.headers()), result.statusText, result.xhrStatus); } function removePendingReq() { diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index 501c1de86c73..7e4cb6d75680 100644 --- a/src/ng/httpBackend.js +++ b/src/ng/httpBackend.js @@ -64,7 +64,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc var jsonpDone = jsonpReq(url, callbackPath, function(status, text) { // jsonpReq only ever sets status to 200 (OK), 404 (ERROR) or -1 (WAITING) var response = (status === 200) && callbacks.getResponse(callbackPath); - completeRequest(callback, status, response, '', text); + completeRequest(callback, status, response, '', text, 'complete'); callbacks.removeCallback(callbackPath); }); } else { @@ -99,18 +99,29 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc status, response, xhr.getAllResponseHeaders(), - statusText); + statusText, + 'complete'); }; var requestError = function() { // The response is always empty // See https://xhr.spec.whatwg.org/#request-error-steps and https://fetch.spec.whatwg.org/#concept-network-error - completeRequest(callback, -1, null, null, ''); + completeRequest(callback, -1, null, null, '', 'error'); + }; + + var requestAborted = function() { + completeRequest(callback, -1, null, null, '', 'abort'); + }; + + var requestTimeout = function() { + // The response is always empty + // See https://xhr.spec.whatwg.org/#request-error-steps and https://fetch.spec.whatwg.org/#concept-network-error + completeRequest(callback, -1, null, null, '', 'timeout'); }; xhr.onerror = requestError; - xhr.onabort = requestError; - xhr.ontimeout = requestError; + xhr.onabort = requestAborted; + xhr.ontimeout = requestTimeout; forEach(eventHandlers, function(value, key) { xhr.addEventListener(key, value); @@ -160,14 +171,14 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc } } - function completeRequest(callback, status, response, headersString, statusText) { + function completeRequest(callback, status, response, headersString, statusText, xhrStatus) { // cancel timeout and subsequent timeout promise resolution if (isDefined(timeoutId)) { $browserDefer.cancel(timeoutId); } jsonpDone = xhr = null; - callback(status, response, headersString, statusText); + callback(status, response, headersString, statusText, xhrStatus); } }; diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 70adeb8f7843..37d42a69f581 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -1346,8 +1346,8 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) { return function() { return angular.isNumber(status) - ? [status, data, headers, statusText] - : [200, status, data, headers]; + ? [status, data, headers, statusText, 'complete'] + : [200, status, data, headers, 'complete']; }; } @@ -1382,14 +1382,14 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) { var response = wrapped.response(method, url, data, headers, wrapped.params(url)); xhr.$$respHeaders = response[2]; callback(copy(response[0]), copy(response[1]), xhr.getAllResponseHeaders(), - copy(response[3] || '')); + copy(response[3] || ''), copy(response[4])); } function handleTimeout() { for (var i = 0, ii = responses.length; i < ii; i++) { if (responses[i] === handleResponse) { responses.splice(i, 1); - callback(-1, undefined, ''); + callback(-1, undefined, '', undefined, 'timeout'); break; } } diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js index fbc58072894f..adb76f60b097 100644 --- a/test/ng/httpBackendSpec.js +++ b/test/ng/httpBackendSpec.js @@ -174,11 +174,12 @@ describe('$httpBackend', function() { }); it('should complete the request on timeout', function() { - callback.and.callFake(function(status, response, headers, statusText) { + callback.and.callFake(function(status, response, headers, statusText, xhrStatus) { expect(status).toBe(-1); expect(response).toBe(null); expect(headers).toBe(null); expect(statusText).toBe(''); + expect(xhrStatus).toBe('timeout'); }); $backend('GET', '/url', null, callback, {}); xhr = MockXhr.$$lastInstance; @@ -189,6 +190,60 @@ describe('$httpBackend', function() { expect(callback).toHaveBeenCalledOnce(); }); + it('should complete the request on abort', function() { + callback.and.callFake(function(status, response, headers, statusText, xhrStatus) { + expect(status).toBe(-1); + expect(response).toBe(null); + expect(headers).toBe(null); + expect(statusText).toBe(''); + expect(xhrStatus).toBe('abort'); + }); + $backend('GET', '/url', null, callback, {}); + xhr = MockXhr.$$lastInstance; + + expect(callback).not.toHaveBeenCalled(); + + xhr.onabort(); + expect(callback).toHaveBeenCalledOnce(); + }); + + it('should complete the request on error', function() { + callback.and.callFake(function(status, response, headers, statusText, xhrStatus) { + expect(status).toBe(-1); + expect(response).toBe(null); + expect(headers).toBe(null); + expect(statusText).toBe(''); + expect(xhrStatus).toBe('error'); + }); + $backend('GET', '/url', null, callback, {}); + xhr = MockXhr.$$lastInstance; + + expect(callback).not.toHaveBeenCalled(); + + xhr.onerror(); + expect(callback).toHaveBeenCalledOnce(); + }); + + it('should complete the request on success', function() { + callback.and.callFake(function(status, response, headers, statusText, xhrStatus) { + expect(status).toBe(200); + expect(response).toBe('response'); + expect(headers).toBe(''); + expect(statusText).toBe(''); + expect(xhrStatus).toBe('complete'); + }); + $backend('GET', '/url', null, callback, {}); + xhr = MockXhr.$$lastInstance; + + expect(callback).not.toHaveBeenCalled(); + + xhr.statusText = ''; + xhr.response = 'response'; + xhr.status = 200; + xhr.onload(); + expect(callback).toHaveBeenCalledOnce(); + }); + it('should abort request on timeout', function() { callback.and.callFake(function(status, response) { expect(status).toBe(-1); diff --git a/test/ng/httpSpec.js b/test/ng/httpSpec.js index 1318a742125a..239d7467cef0 100644 --- a/test/ng/httpSpec.js +++ b/test/ng/httpSpec.js @@ -448,6 +448,28 @@ describe('$http', function() { expect(callback).toHaveBeenCalledOnce(); }); + it('should pass xhrStatus in response object when a request is successful', function() { + $httpBackend.expect('GET', '/url').respond(200, 'SUCCESS', {}, 'OK'); + $http({url: '/url', method: 'GET'}).then(function(response) { + expect(response.xhrStatus).toBe('complete'); + callback(); + }); + + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); + }); + + it('should pass xhrStatus in response object when a request fails', function() { + $httpBackend.expect('GET', '/url').respond(404, 'ERROR', {}, 'Not Found'); + $http({url: '/url', method: 'GET'}).then(null, function(response) { + expect(response.xhrStatus).toBe('complete'); + callback(); + }); + + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); + }); + it('should pass in the response object when a request failed', function() { $httpBackend.expect('GET', '/url').respond(543, 'bad error', {'request-id': '123'}); @@ -1625,6 +1647,17 @@ describe('$http', function() { expect(callback).toHaveBeenCalledOnce(); })); + it('should cache xhrStatus as well', inject(function($rootScope) { + doFirstCacheRequest('GET', 201, null); + callback.and.callFake(function(response) { + expect(response.xhrStatus).toBe('complete'); + }); + + $http({method: 'get', url: '/url', cache: cache}).then(callback); + $rootScope.$digest(); + expect(callback).toHaveBeenCalledOnce(); + })); + it('should use cache even if second request was made before the first returned', function() { $httpBackend.expect('GET', '/url').respond(201, 'fake-response'); @@ -1790,6 +1823,7 @@ describe('$http', function() { function(response) { expect(response.data).toBeUndefined(); expect(response.status).toBe(-1); + expect(response.xhrStatus).toBe('timeout'); expect(response.headers()).toEqual(Object.create(null)); expect(response.config.url).toBe('/some'); callback(); diff --git a/test/ngMock/angular-mocksSpec.js b/test/ngMock/angular-mocksSpec.js index 70b8a97fd98b..e8125c216726 100644 --- a/test/ngMock/angular-mocksSpec.js +++ b/test/ngMock/angular-mocksSpec.js @@ -1274,8 +1274,8 @@ describe('ngMock', function() { hb.flush(); expect(callback).toHaveBeenCalledTimes(2); - expect(callback.calls.argsFor(0)).toEqual([201, 'second', '', '']); - expect(callback.calls.argsFor(1)).toEqual([200, 'first', '', '']); + expect(callback.calls.argsFor(0)).toEqual([201, 'second', '', '', 'complete']); + expect(callback.calls.argsFor(1)).toEqual([200, 'first', '', '', 'complete']); }); @@ -1285,7 +1285,7 @@ describe('ngMock', function() { hb('GET', '/url1', undefined, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'first', 'header: val', 'OK'); + expect(callback).toHaveBeenCalledOnceWith(200, 'first', 'header: val', 'OK', 'complete'); }); it('should default status code to 200', function() { @@ -1308,7 +1308,19 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'first', 'header: val', 'OK'); + expect(callback).toHaveBeenCalledOnceWith(200, 'first', 'header: val', 'OK', 'complete'); + }); + + it('should default xhrStatus to complete', function() { + callback.and.callFake(function(status, response, headers, x, xhrStatus) { + expect(xhrStatus).toBe('complete'); + }); + + hb.expect('GET', '/url1').respond('some-data'); + hb('GET', '/url1', null, callback); + + hb.flush(); + expect(callback).toHaveBeenCalled(); }); it('should take function', function() { @@ -1319,7 +1331,7 @@ describe('ngMock', function() { hb('GET', '/some?q=s', 'data', callback, {a: 'b'}); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(301, 'GET/some?q=s;data;a=b;q=s', 'Connection: keep-alive', 'Moved Permanently'); + expect(callback).toHaveBeenCalledOnceWith(301, 'GET/some?q=s;data;a=b;q=s', 'Connection: keep-alive', 'Moved Permanently', undefined); }); it('should decode query parameters in respond() function', function() { @@ -1331,7 +1343,7 @@ describe('ngMock', function() { hb('GET', '/url?query=l%E2%80%A2ng%20string%20w%2F%20spec%5Eal%20char%24&id=1234&orderBy=-name', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'id=1234;orderBy=-name;query=l•ng string w/ spec^al char$', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'id=1234;orderBy=-name;query=l•ng string w/ spec^al char$', '', '', undefined); }); it('should include regex captures in respond() params when keys provided', function() { @@ -1343,7 +1355,7 @@ describe('ngMock', function() { hb('GET', '/1234/article/cool-angular-article', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'id=1234;name=cool-angular-article', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'id=1234;name=cool-angular-article', '', '', undefined); }); it('should default response headers to ""', function() { @@ -1356,8 +1368,8 @@ describe('ngMock', function() { hb.flush(); expect(callback).toHaveBeenCalledTimes(2); - expect(callback.calls.argsFor(0)).toEqual([200, 'first', '', '']); - expect(callback.calls.argsFor(1)).toEqual([200, 'second', '', '']); + expect(callback.calls.argsFor(0)).toEqual([200, 'first', '', '', 'complete']); + expect(callback.calls.argsFor(1)).toEqual([200, 'second', '', '', 'complete']); }); it('should be able to override response of expect definition', function() { @@ -1367,7 +1379,7 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', '', 'complete'); }); it('should be able to override response of when definition', function() { @@ -1377,7 +1389,7 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', '', 'complete'); }); it('should be able to override response of expect definition with chaining', function() { @@ -1386,7 +1398,7 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', '', 'complete'); }); it('should be able to override response of when definition with chaining', function() { @@ -1395,7 +1407,7 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'second', '', '', 'complete'); }); }); @@ -1588,7 +1600,7 @@ describe('ngMock', function() { canceler(); // simulate promise resolution - expect(callback).toHaveBeenCalledWith(-1, undefined, ''); + expect(callback).toHaveBeenCalledWith(-1, undefined, '', undefined, 'timeout'); hb.verifyNoOutstandingExpectation(); hb.verifyNoOutstandingRequest(); }); @@ -1600,7 +1612,7 @@ describe('ngMock', function() { hb('GET', '/url1', null, callback, null, 200); $timeout.flush(300); - expect(callback).toHaveBeenCalledWith(-1, undefined, ''); + expect(callback).toHaveBeenCalledWith(-1, undefined, '', undefined, 'timeout'); hb.verifyNoOutstandingExpectation(); hb.verifyNoOutstandingRequest(); })); @@ -1746,7 +1758,7 @@ describe('ngMock', function() { hb[shortcut]('/foo').respond('bar'); hb(method, '/foo', undefined, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'bar', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'bar', '', '', 'complete'); }); }); }); @@ -1761,7 +1773,7 @@ describe('ngMock', function() { hb[routeShortcut](this, '/route').respond('path'); hb(this, '/route', undefined, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete'); } ); they('should match colon delimited parameters in ' + routeShortcut + ' $prop method', methods, @@ -1769,7 +1781,7 @@ describe('ngMock', function() { hb[routeShortcut](this, '/route/:id/path/:s_id').respond('path'); hb(this, '/route/123/path/456', undefined, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete'); } ); they('should ignore query param when matching in ' + routeShortcut + ' $prop method', methods, @@ -1777,7 +1789,7 @@ describe('ngMock', function() { hb[routeShortcut](this, '/route/:id').respond('path'); hb(this, '/route/123?q=str&foo=bar', undefined, callback); hb.flush(); - expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete'); } ); }); @@ -2460,7 +2472,7 @@ describe('ngMockE2E', function() { $browser.defer.flush(); expect(realHttpBackend).not.toHaveBeenCalled(); - expect(callback).toHaveBeenCalledOnceWith(200, 'passThrough override', '', ''); + expect(callback).toHaveBeenCalledOnceWith(200, 'passThrough override', '', '', 'complete'); })); it('should pass through to an httpBackend that uses the same $browser service', inject(function($browser) { From 2fcbc76e1008b882d0306e5445999b691d451363 Mon Sep 17 00:00:00 2001 From: "PRIJCK Frederik (FPRJ)" Date: Tue, 4 Jul 2017 13:54:19 +0200 Subject: [PATCH 2/3] fix spaces --- src/ngMock/angular-mocks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 37d42a69f581..06a651c149e2 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -1382,7 +1382,7 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) { var response = wrapped.response(method, url, data, headers, wrapped.params(url)); xhr.$$respHeaders = response[2]; callback(copy(response[0]), copy(response[1]), xhr.getAllResponseHeaders(), - copy(response[3] || ''), copy(response[4])); + copy(response[3] || ''), copy(response[4])); } function handleTimeout() { From f2586063260528e9b790bfea31095d1bfe9972e0 Mon Sep 17 00:00:00 2001 From: "PRIJCK Frederik (FPRJ)" Date: Tue, 4 Jul 2017 14:00:35 +0200 Subject: [PATCH 3/3] fix spaces, try 2 --- src/ngMock/angular-mocks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 06a651c149e2..5d4831283d58 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -1382,7 +1382,7 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) { var response = wrapped.response(method, url, data, headers, wrapped.params(url)); xhr.$$respHeaders = response[2]; callback(copy(response[0]), copy(response[1]), xhr.getAllResponseHeaders(), - copy(response[3] || ''), copy(response[4])); + copy(response[3] || ''), copy(response[4])); } function handleTimeout() {