From 77d4a32744faf558fd0ddfc66c232e4beeacc6dc Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Mon, 7 Dec 2015 23:19:11 +0200 Subject: [PATCH 1/4] revert: fix($resource): allow XHR request to be cancelled via timeout promise This reverts commit 7170f9d9ca765c578f8d3eb4699860a9330a0a11. Fixes part of #13393. --- src/ngResource/resource.js | 13 ++----------- test/ngResource/resourceSpec.js | 32 +------------------------------- 2 files changed, 3 insertions(+), 42 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index a0f7274e9a50..e67cd01dce47 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -568,17 +568,8 @@ angular.module('ngResource', ['ng']). undefined; forEach(action, function(value, key) { - switch (key) { - default: - httpConfig[key] = copy(value); - break; - case 'params': - case 'isArray': - case 'interceptor': - break; - case 'timeout': - httpConfig[key] = value; - break; + if (key != 'params' && key != 'isArray' && key != 'interceptor') { + httpConfig[key] = copy(value); } }); diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 2ac1454bd48b..79450e4f24ea 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1308,7 +1308,7 @@ describe("resource", function() { }); describe('resource', function() { - var $httpBackend, $resource, $q; + var $httpBackend, $resource; beforeEach(module(function($exceptionHandlerProvider) { $exceptionHandlerProvider.mode('log'); @@ -1319,7 +1319,6 @@ describe('resource', function() { beforeEach(inject(function($injector) { $httpBackend = $injector.get('$httpBackend'); $resource = $injector.get('$resource'); - $q = $injector.get('$q'); })); @@ -1357,34 +1356,5 @@ describe('resource', function() { ); }); - it('should cancel the request if timeout promise is resolved', function() { - var canceler = $q.defer(); - - $httpBackend.when('GET', '/CreditCard').respond({data: '123'}); - - var CreditCard = $resource('/CreditCard', {}, { - query: { - method: 'GET', - timeout: canceler.promise - } - }); - - CreditCard.query(); - - canceler.resolve(); - expect($httpBackend.flush).toThrow(new Error("No pending request to flush !")); - - canceler = $q.defer(); - CreditCard = $resource('/CreditCard', {}, { - query: { - method: 'GET', - timeout: canceler.promise - } - }); - - CreditCard.query(); - expect($httpBackend.flush).not.toThrow(); - }); - }); From a758d589aac8552a2a9dd54c07047ea494978849 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 8 Dec 2015 15:09:24 +0200 Subject: [PATCH 2/4] refactor($resource): change if-block to switch-block for readability --- src/ngResource/resource.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index e67cd01dce47..aed5033edc5f 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -568,8 +568,14 @@ angular.module('ngResource', ['ng']). undefined; forEach(action, function(value, key) { - if (key != 'params' && key != 'isArray' && key != 'interceptor') { - httpConfig[key] = copy(value); + switch (key) { + default: + httpConfig[key] = copy(value); + break; + case 'params': + case 'isArray': + case 'interceptor': + break; } }); From 0c587eaeb19ba725526424f518496064034e46fe Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 8 Dec 2015 15:17:01 +0200 Subject: [PATCH 3/4] docs($resource): add note about _promises as timeout_ not being supported Fixes part of #13393. --- src/ngResource/resource.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index aed5033edc5f..19bc84dd9233 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -150,8 +150,11 @@ function shallowClearAndCopy(src, dst) { * GET request, otherwise if a cache instance built with * {@link ng.$cacheFactory $cacheFactory}, this cache will be used for * caching. - * - **`timeout`** – `{number|Promise}` – timeout in milliseconds, or {@link ng.$q promise} that - * should abort the request when resolved. + * - **`timeout`** – `{number}` – timeout in milliseconds.
+ * **Note:** In contrast to {@link ng.$http#usage $http.config}, {@link ng.$q promises} are + * **not** supported in $resource, because the same value would be used for multiple requests. + * If you need support for cancellable $resource actions, you should upgrade to version 1.5 or + * higher. * - **`withCredentials`** - `{boolean}` - whether to set the `withCredentials` flag on the * XHR object. See * [requests with credentials](https://developer.mozilla.org/en/http_access_control#section_5) From 6d5141ee03cb4e296e09a1f8a428295abda48b4d Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 8 Dec 2015 15:23:17 +0200 Subject: [PATCH 4/4] fix($resource): don't allow using promises as `timeout` and log a warning Promises never worked correctly as values for `timeout` in `$resource`, because the same value has to be re-used for multiple requests (and it is not possible to `angular.copy()` a promise). Now (in addition to ignoring a non-numeric `timeout`), a warning is logged to the console using `$log.debug()`. Partly fixes #13393. --- src/ngResource/resource.js | 12 +++++++++- test/ngResource/resourceSpec.js | 42 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 19bc84dd9233..514e4c81b72b 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -368,7 +368,7 @@ angular.module('ngResource', ['ng']). } }; - this.$get = ['$http', '$q', function($http, $q) { + this.$get = ['$http', '$log', '$q', function($http, $log, $q) { var noop = angular.noop, forEach = angular.forEach, @@ -579,6 +579,16 @@ angular.module('ngResource', ['ng']). case 'isArray': case 'interceptor': break; + case 'timeout': + if (value && !angular.isNumber(value)) { + $log.debug('ngResource:\n' + + ' Only numeric values are allowed as `timeout`.\n' + + ' Promises are not supported in $resource, because the same value would ' + + 'be used for multiple requests.\n' + + ' If you need support for cancellable $resource actions, you should ' + + 'upgrade to version 1.5 or higher.'); + } + break; } }); diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 79450e4f24ea..560f71ac1f3d 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1355,6 +1355,48 @@ describe('resource', function() { /^\[\$resource:badcfg\] Error in resource configuration for action `get`\. Expected response to contain an object but got an array \(Request: GET \/Customer\/123\)/ ); }); +}); + +describe('resource with promises as `timeout`', function() { + var httpSpy; + var $httpBackend; + var $resource; + + beforeEach(module('ngResource', function($provide) { + $provide.decorator('$http', function($delegate) { + httpSpy = jasmine.createSpy('$http').andCallFake($delegate); + return httpSpy; + }); + })); + + beforeEach(inject(function(_$httpBackend_, _$resource_) { + $httpBackend = _$httpBackend_; + $resource = _$resource_; + })); + it('should ignore non-numeric timeouts in actions and log a $debug message', + inject(function($log, $q) { + spyOn($log, 'debug'); + $httpBackend.whenGET('/CreditCard').respond({}); + + var CreditCard = $resource('/CreditCard', {}, { + get: { + method: 'GET', + timeout: $q.defer().promise + } + }); + + CreditCard.get(); + $httpBackend.flush(); + expect(httpSpy).toHaveBeenCalledOnce(); + expect(httpSpy.calls[0].args[0].timeout).toBeUndefined(); + expect($log.debug).toHaveBeenCalledOnceWith('ngResource:\n' + + ' Only numeric values are allowed as `timeout`.\n' + + ' Promises are not supported in $resource, because the same value would ' + + 'be used for multiple requests.\n' + + ' If you need support for cancellable $resource actions, you should ' + + 'upgrade to version 1.5 or higher.'); + }) + ); });