From 7ca3d80ae7a5c1fa9ddfcfcfcad94610f19fa68a Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 28 Jun 2016 23:29:31 +0300 Subject: [PATCH 1/3] test($resource): add some tests wrt handling response errors The test currently fail on master, but should pass on 1.5.x. A subsequent commit will fix the regressions on master. (This commit should be backportable to 1.5.x.) Related to #14837. --- test/ngResource/resourceSpec.js | 71 ++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index a848f06d1244..6716756f7f2b 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -30,7 +30,7 @@ describe("basic usage", function() { } }); - callback = jasmine.createSpy(); + callback = jasmine.createSpy('callback'); })); @@ -907,6 +907,7 @@ describe("basic usage", function() { expect(cc.url).toBe('/new-id'); }); + it('should pass the same transformed value to success callbacks and to promises', function() { $httpBackend.expect('GET', '/CreditCard').respond(200, { value: 'original' }); @@ -1024,6 +1025,7 @@ describe("basic usage", function() { }); }); + it('should allow per action response interceptor that gets full response', function() { CreditCard = $resource('/CreditCard', {}, { query: { @@ -1079,6 +1081,46 @@ describe("basic usage", function() { expect(response.status).toBe(404); expect(response.config).toBeDefined(); }); + + + it('should fulfill the promise with the value returned by the responseError interceptor', + inject(function($q) { + CreditCard = $resource('/CreditCard', {}, { + test1: { + method: 'GET', + interceptor: {responseError: function() { return 'foo'; }} + }, + test2: { + method: 'GET', + interceptor: {responseError: function() { return $q.resolve('bar'); }} + }, + test3: { + method: 'GET', + interceptor: {responseError: function() { return $q.reject('baz'); }} + } + }); + + $httpBackend.whenGET('/CreditCard').respond(404); + + callback.calls.reset(); + CreditCard.test1().$promise.then(callback); + $httpBackend.flush(); + + expect(callback).toHaveBeenCalledOnceWith('foo'); + + callback.calls.reset(); + CreditCard.test2().$promise.then(callback); + $httpBackend.flush(); + + expect(callback).toHaveBeenCalledOnceWith('bar'); + + callback.calls.reset(); + CreditCard.test3().$promise.then(null, callback); + $httpBackend.flush(); + + expect(callback).toHaveBeenCalledOnceWith('baz'); + }) + ); }); @@ -1414,6 +1456,33 @@ describe('errors', function() { }); }); +describe('handling rejections', function() { + var $httpBackend; + var $resource; + + beforeEach(module('ngResource')); + + beforeEach(inject(function(_$httpBackend_, _$resource_) { + $httpBackend = _$httpBackend_; + $resource = _$resource_; + + $httpBackend.whenGET('/CreditCard').respond(404); + })); + + + it('should reject the promise even when there is an error callback', function() { + var errorCb1 = jasmine.createSpy('errorCb1'); + var errorCb2 = jasmine.createSpy('errorCb2'); + var CreditCard = $resource('/CreditCard'); + + CreditCard.get(noop, errorCb1).$promise.catch(errorCb2); + $httpBackend.flush(); + + expect(errorCb1).toHaveBeenCalledOnce(); + expect(errorCb2).toHaveBeenCalledOnce(); + }); +}); + describe('cancelling requests', function() { var httpSpy; var $httpBackend; From 3d40936bf34bbf69468ffbee8650a220739248af Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 28 Jun 2016 23:32:28 +0300 Subject: [PATCH 2/3] fix($resource): fulfill promise with the correct value on error This fixes a regression introduced with 71cf28c. See #14837 for more info. Fixes #14837 --- src/ngResource/resource.js | 19 ++++++--- test/ngResource/resourceSpec.js | 71 ++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 31a594421e50..3d45079b25fc 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -695,6 +695,8 @@ angular.module('ngResource', ['ng']). defaultResponseInterceptor; var responseErrorInterceptor = action.interceptor && action.interceptor.responseError || undefined; + var hasError = !!error; + var hasResponseErrorInterceptor = !!responseErrorInterceptor; var timeoutDeferred; var numericTimeoutPromise; @@ -776,13 +778,18 @@ angular.module('ngResource', ['ng']). (success || noop)(value, response.headers); return value; }, - responseErrorInterceptor || error ? + (hasError || hasResponseErrorInterceptor) ? function(response) { - (error || noop)(response); - (responseErrorInterceptor || noop)(response); - return response; - } - : undefined); + if (hasError) error(response); + return hasResponseErrorInterceptor ? + responseErrorInterceptor(response) : + $q.reject(response); + } : + undefined); + if (hasError && !hasResponseErrorInterceptor) { + // Avoid PUR, but still reject the returned promise + promise.catch(noop); + } if (!isInstanceCall) { // we are creating instance / collection diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 6716756f7f2b..f3f83682ea55 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1457,12 +1457,17 @@ describe('errors', function() { }); describe('handling rejections', function() { + var $exceptionHandler; var $httpBackend; var $resource; beforeEach(module('ngResource')); + beforeEach(module(function($exceptionHandlerProvider) { + $exceptionHandlerProvider.mode('log'); + })); - beforeEach(inject(function(_$httpBackend_, _$resource_) { + beforeEach(inject(function(_$exceptionHandler_, _$httpBackend_, _$resource_) { + $exceptionHandler = _$exceptionHandler_; $httpBackend = _$httpBackend_; $resource = _$resource_; @@ -1481,6 +1486,70 @@ describe('handling rejections', function() { expect(errorCb1).toHaveBeenCalledOnce(); expect(errorCb2).toHaveBeenCalledOnce(); }); + + + it('should report a PUR when no error callback or responseError interceptor is provided', + function() { + var CreditCard = $resource('/CreditCard'); + + CreditCard.get(); + $httpBackend.flush(); + + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Possibly unhandled rejection/); + } + ); + + + it('should not report a PUR when an error callback or responseError interceptor is provided', + function() { + var CreditCard = $resource('/CreditCard', {}, { + test1: { + method: 'GET' + }, + test2: { + method: 'GET', + interceptor: {responseError: function() { return {}; }} + } + }); + + // With error callback + CreditCard.test1(noop, noop); + $httpBackend.flush(); + + expect($exceptionHandler.errors.length).toBe(0); + + // With responseError interceptor + CreditCard.test2(); + $httpBackend.flush(); + + expect($exceptionHandler.errors.length).toBe(0); + + // With error callback and responseError interceptor + CreditCard.test2(noop, noop); + $httpBackend.flush(); + + expect($exceptionHandler.errors.length).toBe(0); + } + ); + + + it('should report a PUR when the responseError interceptor returns a rejected promise', + inject(function($q) { + var CreditCard = $resource('/CreditCard', {}, { + test: { + method: 'GET', + interceptor: {responseError: function() { return $q.reject({}); }} + } + }); + + CreditCard.test(); + $httpBackend.flush(); + + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Possibly unhandled rejection/); + }) + ); }); describe('cancelling requests', function() { From 64fc798749535b4207abde1e2a18c4baf659c65c Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Mon, 18 Jul 2016 13:48:24 +0300 Subject: [PATCH 3/3] fixup! fix($resource): fulfill promise with the correct value on error --- src/ngResource/resource.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 3d45079b25fc..ae629d0b16fa 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -787,7 +787,8 @@ angular.module('ngResource', ['ng']). } : undefined); if (hasError && !hasResponseErrorInterceptor) { - // Avoid PUR, but still reject the returned promise + // Avoid `Possibly Unhandled Rejection` error, + // but still fulfill the returned promise with a rejection promise.catch(noop); }