From 982a063507f649bde84b727dcc2cefcf3e0bbcd3 Mon Sep 17 00:00:00 2001 From: Kyle Wuolle Date: Thu, 19 Jan 2017 12:19:01 -0800 Subject: [PATCH 1/4] fix(ngResource): do not eat exceptions in success callback move the promise catch noop call inside of error callback handler so that errors inside of the success callback are not eaten added two tests to prove functionality No breaking changes --- src/ngResource/resource.js | 20 +++++++++---------- test/ngResource/resourceSpec.js | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index d1d20b04f483..45d146fcb209 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -784,18 +784,18 @@ angular.module('ngResource', ['ng']). return value; }, (hasError || hasResponseErrorInterceptor) ? - function(response) { - if (hasError) error(response); - return hasResponseErrorInterceptor ? + function(response) { + if (hasError && !hasResponseErrorInterceptor) { + // Avoid `Possibly Unhandled Rejection` error, + // but still fulfill the returned promise with a rejection + promise.catch(noop); + } + if (hasError) error(response); + return hasResponseErrorInterceptor ? responseErrorInterceptor(response) : $q.reject(response); - } : - undefined); - if (hasError && !hasResponseErrorInterceptor) { - // Avoid `Possibly Unhandled Rejection` error, - // but still fulfill the returned promise with a rejection - promise.catch(noop); - } + } : + undefined); if (!isInstanceCall) { // we are creating instance / collection diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index cae8d56fff5b..32288a8434ec 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1978,4 +1978,39 @@ describe('configuring `cancellable` on the provider', function() { expect(creditCard3.$cancelRequest).toBeDefined(); }); }); + + describe('exception handling in callbacks', function() { + var $resource; + var $httpBackend; + var CreditCard; + beforeEach(module('ngResource')); + + beforeEach(inject(function($injector) { + $httpBackend = $injector.get('$httpBackend'); + $resource = $injector.get('$resource'); + CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); + })); + + it('should throw exception in success callback when error callback provided', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); + + var cc = CreditCard.get({ id: 123 }, + function(res) { + throw new Error('should be caught'); + }, function() {}); + + expect($httpBackend.flush).toThrow(new Error('should be caught')); + }); + + it('should throw exception in success callback when error callback not provided', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); + + var cc = CreditCard.get({ id: 123 }, + function(res) { + throw new Error('should be caught'); + }); + + expect($httpBackend.flush).toThrow(new Error('should be caught')); + }); + }); }); From c94d3d7ddd2c6b1466b586d63d29a60473ecee38 Mon Sep 17 00:00:00 2001 From: Kyle Wuolle Date: Fri, 3 Feb 2017 09:20:57 -0800 Subject: [PATCH 2/4] * fix(ngResource): moved the unit tests to the proper location and added two more cases to cover all the scenarios. No breaking changes --- test/ngResource/resourceSpec.js | 108 +++++++++++++++++++++----------- 1 file changed, 73 insertions(+), 35 deletions(-) diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 32288a8434ec..ac9988614655 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1709,6 +1709,79 @@ describe('handling rejections', function() { expect($exceptionHandler.errors[0]).toMatch(/^Possibly unhandled rejection/); }) ); + + + it('should throw exception in success callback when error callback provided and no responseErrorInterceptor', function () { + $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); + var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); + var cc = CreditCard.get({ id: 123 }, + function (res) { + throw new Error('should be caught'); + }, function () { }); + + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); + }); + + + it('should throw exception in success callback when error callback not provided and no responseErrorInterceptor', function () { + $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); + var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); + var cc = CreditCard.get({ id: 123 }, + function (res) { + throw new Error('should be caught'); + }); + + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); + }); + + + it('should throw exception in success callback when error callback provided and has responseErrorInterceptor', function () { + inject(function ($q) { + + $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); + var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { + 'get': { + method: 'GET', + interceptor: { responseError: function () { return $q.reject({}); } } + } + }); + + var cc = CreditCard.get({ id: 123 }, + function (res) { + throw new Error('should be caught'); + }, function () { }); + + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); + }); + }); + + + it('should throw exception in success callback when error callback not provided and has responseErrorInterceptor', function () { + inject(function ($q) { + + $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); + var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { + 'get': { + method: 'GET', + interceptor: { responseError: function () { return $q.reject({}); } } + } + }); + var cc = CreditCard.get({ id: 123 }, + function (res) { + throw new Error('should be caught'); + }); + + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); + }); + }); }); describe('cancelling requests', function() { @@ -1978,39 +2051,4 @@ describe('configuring `cancellable` on the provider', function() { expect(creditCard3.$cancelRequest).toBeDefined(); }); }); - - describe('exception handling in callbacks', function() { - var $resource; - var $httpBackend; - var CreditCard; - beforeEach(module('ngResource')); - - beforeEach(inject(function($injector) { - $httpBackend = $injector.get('$httpBackend'); - $resource = $injector.get('$resource'); - CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); - })); - - it('should throw exception in success callback when error callback provided', function() { - $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); - - var cc = CreditCard.get({ id: 123 }, - function(res) { - throw new Error('should be caught'); - }, function() {}); - - expect($httpBackend.flush).toThrow(new Error('should be caught')); - }); - - it('should throw exception in success callback when error callback not provided', function() { - $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); - - var cc = CreditCard.get({ id: 123 }, - function(res) { - throw new Error('should be caught'); - }); - - expect($httpBackend.flush).toThrow(new Error('should be caught')); - }); - }); }); From 2c7280d871c5c50a9b76a729ea7f3a8a6251cb52 Mon Sep 17 00:00:00 2001 From: Kyle Wuolle Date: Fri, 3 Feb 2017 09:36:45 -0800 Subject: [PATCH 3/4] * fix(ngResource): fix up the linting issues --- test/ngResource/resourceSpec.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index ac9988614655..ffa5b36b59a0 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1711,13 +1711,13 @@ describe('handling rejections', function() { ); - it('should throw exception in success callback when error callback provided and no responseErrorInterceptor', function () { + it('should throw exception in success callback when error callback provided and no responseErrorInterceptor', function() { $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); var cc = CreditCard.get({ id: 123 }, - function (res) { + function(res) { throw new Error('should be caught'); - }, function () { }); + }, function() { }); $httpBackend.flush(); expect($exceptionHandler.errors.length).toBe(1); @@ -1725,11 +1725,11 @@ describe('handling rejections', function() { }); - it('should throw exception in success callback when error callback not provided and no responseErrorInterceptor', function () { + it('should throw exception in success callback when error callback not provided and no responseErrorInterceptor', function() { $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); var cc = CreditCard.get({ id: 123 }, - function (res) { + function(res) { throw new Error('should be caught'); }); @@ -1739,21 +1739,21 @@ describe('handling rejections', function() { }); - it('should throw exception in success callback when error callback provided and has responseErrorInterceptor', function () { - inject(function ($q) { + it('should throw exception in success callback when error callback provided and has responseErrorInterceptor', function() { + inject(function($q) { $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { 'get': { method: 'GET', - interceptor: { responseError: function () { return $q.reject({}); } } + interceptor: { responseError: function() { return $q.reject({}); } } } }); var cc = CreditCard.get({ id: 123 }, - function (res) { + function(res) { throw new Error('should be caught'); - }, function () { }); + }, function() { }); $httpBackend.flush(); expect($exceptionHandler.errors.length).toBe(1); @@ -1762,18 +1762,18 @@ describe('handling rejections', function() { }); - it('should throw exception in success callback when error callback not provided and has responseErrorInterceptor', function () { - inject(function ($q) { + it('should throw exception in success callback when error callback not provided and has responseErrorInterceptor', function() { + inject(function($q) { $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { 'get': { method: 'GET', - interceptor: { responseError: function () { return $q.reject({}); } } + interceptor: { responseError: function() { return $q.reject({}); } } } }); var cc = CreditCard.get({ id: 123 }, - function (res) { + function(res) { throw new Error('should be caught'); }); From d7520b3bf89703a1dcc559660dec49df49879f34 Mon Sep 17 00:00:00 2001 From: Kyle Wuolle Date: Sat, 4 Feb 2017 09:01:23 -0800 Subject: [PATCH 4/4] fix(ngResource): clean up some test descriptions and change response interceptors to non rejecting ones --- test/ngResource/resourceSpec.js | 77 +++++++++++++++------------------ 1 file changed, 36 insertions(+), 41 deletions(-) diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index ffa5b36b59a0..49d405dcc170 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -1711,9 +1711,9 @@ describe('handling rejections', function() { ); - it('should throw exception in success callback when error callback provided and no responseErrorInterceptor', function() { - $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); - var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); + it('should not swallow exception in success callback when error callback provided', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond(null); + var CreditCard = $resource('/CreditCard/:id'); var cc = CreditCard.get({ id: 123 }, function(res) { throw new Error('should be caught'); @@ -1725,9 +1725,9 @@ describe('handling rejections', function() { }); - it('should throw exception in success callback when error callback not provided and no responseErrorInterceptor', function() { - $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); - var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }); + it('should not swallow exception in success callback when error callback not provided', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond(null); + var CreditCard = $resource('/CreditCard/:id'); var cc = CreditCard.get({ id: 123 }, function(res) { throw new Error('should be caught'); @@ -1739,48 +1739,43 @@ describe('handling rejections', function() { }); - it('should throw exception in success callback when error callback provided and has responseErrorInterceptor', function() { - inject(function($q) { - - $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); - var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { - 'get': { - method: 'GET', - interceptor: { responseError: function() { return $q.reject({}); } } - } - }); + it('should not swallow exception in success callback when error callback provided and has responseError interceptor', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond(null); + var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { + 'get': { + method: 'GET', + interceptor: { responseError: function() { return {}; }} + } + }); - var cc = CreditCard.get({ id: 123 }, - function(res) { - throw new Error('should be caught'); - }, function() { }); + var cc = CreditCard.get({ id: 123 }, + function(res) { + throw new Error('should be caught'); + }, function() { }); - $httpBackend.flush(); - expect($exceptionHandler.errors.length).toBe(1); - expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); - }); + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); }); - it('should throw exception in success callback when error callback not provided and has responseErrorInterceptor', function() { - inject(function($q) { - - $httpBackend.expect('GET', '/CreditCard/123').respond({ id: 123, number: '9876' }); - var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { - 'get': { - method: 'GET', - interceptor: { responseError: function() { return $q.reject({}); } } - } - }); - var cc = CreditCard.get({ id: 123 }, - function(res) { - throw new Error('should be caught'); - }); + it('should not swallow exception in success callback when error callback not provided and has responseError interceptor', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond(null); + var CreditCard = $resource('/CreditCard/:id:verb', { id: '@id.key' }, { + 'get': { + method: 'GET', + interceptor: { responseError: function() { return {}; }} + } + }); - $httpBackend.flush(); - expect($exceptionHandler.errors.length).toBe(1); - expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); + var cc = CreditCard.get({ id: 123 }, + function(res) { + throw new Error('should be caught'); }); + + $httpBackend.flush(); + expect($exceptionHandler.errors.length).toBe(1); + expect($exceptionHandler.errors[0]).toMatch(/^Error: should be caught/); }); });