From 544ce8cd837332765218f681695c129e7de0f45c Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 30 Jul 2013 22:48:56 +0100 Subject: [PATCH 1/2] fix(resource): check whether response matches action.isArray When using $resource you must setup your actions carefully based on what the server returns. If the server responds to a request with an array then you must configure the action with `isArray:true` and vice versa. The built-in `get` action defaults to `isArray:false` and the `query` action defaults to `isArray:true`, which is must be changed if the server does not do this. Before the error message was an exception inside angular.copy, which didn't explain what the real problem was. Rather than changing the way that angular.copy works, this change ensures that a better error message is provided to the programmer if they do not set up their resource actions correctly. Closes #2255, #1044 --- docs/content/error/ngResource/badcfg.ngdoc | 4 ++ src/ngResource/resource.js | 5 +++ test/ngResource/resourceSpec.js | 49 +++++++++++++++++++++- 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 docs/content/error/ngResource/badcfg.ngdoc diff --git a/docs/content/error/ngResource/badcfg.ngdoc b/docs/content/error/ngResource/badcfg.ngdoc new file mode 100644 index 000000000000..c27dc7c0ec3f --- /dev/null +++ b/docs/content/error/ngResource/badcfg.ngdoc @@ -0,0 +1,4 @@ +@ngdoc error +@name ngResource:badcfg +@fullName Response does not match configured parameter +@description diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 92cbcd161bf4..003b41f18cce 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -473,6 +473,11 @@ angular.module('ngResource', ['ng']). promise = value.$promise; if (data) { + if ( angular.isArray(data) != !!action.isArray ) { + throw ngResourceMinErr('badcfg', 'Error in resource configuration. Expected response' + + ' to contain an {0} but got an {1}', + action.isArray?'array':'object', angular.isArray(data)?'array':'object'); + } if (action.isArray) { value.length = 0; forEach(data, function(item) { diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 52395297b896..19456f01360b 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -807,7 +807,6 @@ describe("resource", function() { }); }); - it('should transform request/response', function() { var Person = $resource('/Person/:id', {}, { save: { @@ -1034,3 +1033,51 @@ describe("resource", function() { }); }); }); + +describe('resource', function() { + var $httpBackend, $resource; + + beforeEach(module(function($exceptionHandlerProvider) { + $exceptionHandlerProvider.mode('log'); + })); + + beforeEach(module('ngResource')); + + beforeEach(inject(function($injector) { + $httpBackend = $injector.get('$httpBackend'); + $resource = $injector.get('$resource'); + })); + + + it('should fail if action expects an object but response is an array', function() { + var successSpy = jasmine.createSpy('successSpy'); + var failureSpy = jasmine.createSpy('failureSpy'); + + $httpBackend.expect('GET', '/Customer/123').respond({id: 'abc'}); + + $resource('/Customer/123').query() + .$promise.then(successSpy, function(e) { failureSpy(e.message); }); + $httpBackend.flush(); + + expect(successSpy).not.toHaveBeenCalled(); + expect(failureSpy).toHaveBeenCalledWith( + '[ngResource:badcfg] Error in resource configuration. Expected response to contain an array but got an object'); + }); + + it('should fail if action expects an array but response is an object', function() { + var successSpy = jasmine.createSpy('successSpy'); + var failureSpy = jasmine.createSpy('failureSpy'); + + $httpBackend.expect('GET', '/Customer/123').respond([1,2,3]); + + $resource('/Customer/123').get() + .$promise.then(successSpy, function(e) { failureSpy(e.message); }); + $httpBackend.flush(); + + expect(successSpy).not.toHaveBeenCalled(); + expect(failureSpy).toHaveBeenCalledWith( + '[ngResource:badcfg] Error in resource configuration. Expected response to contain an object but got an array'); + }); + + +}); \ No newline at end of file From 5878cebf15021386747f79f593ecf8518f119a8f Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 31 Jul 2013 10:09:23 +0100 Subject: [PATCH 2/2] fix($q): call `reject()` even if `$exceptionHandler` rethrows Normally $exceptionHandler doesn't throw an exception. It is normally used just for logging and so on. But if an application developer implemented a version that did throw an exception then $q would never have called reject() when converting an exception thrown inside a `then` handler into a rejected promise. --- src/ng/q.js | 4 ++-- test/ng/qSpec.js | 51 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/ng/q.js b/src/ng/q.js index fe05b37f5659..b68660d54bd9 100644 --- a/src/ng/q.js +++ b/src/ng/q.js @@ -237,8 +237,8 @@ function qFactory(nextTick, exceptionHandler) { try { result.resolve((callback || defaultCallback)(value)); } catch(e) { - exceptionHandler(e); result.reject(e); + exceptionHandler(e); } }; @@ -246,8 +246,8 @@ function qFactory(nextTick, exceptionHandler) { try { result.resolve((errback || defaultErrback)(reason)); } catch(e) { - exceptionHandler(e); result.reject(e); + exceptionHandler(e); } }; diff --git a/test/ng/qSpec.js b/test/ng/qSpec.js index 316f8add68a1..6d08cb15ec03 100644 --- a/test/ng/qSpec.js +++ b/test/ng/qSpec.js @@ -160,6 +160,7 @@ describe('q', function() { mockNextTick.queue.push(task); }, queue: [], + logExceptions: true, flush: function() { if (!mockNextTick.queue.length) throw new Error('Nothing to be flushed!'); while (mockNextTick.queue.length) { @@ -169,7 +170,9 @@ describe('q', function() { try { task(); } catch(e) { - dump('exception in mockNextTick:', e, e.name, e.message, task); + if ( mockNextTick.logExceptions ) { + dump('exception in mockNextTick:', e, e.name, e.message, task); + } } }); } @@ -1393,4 +1396,50 @@ describe('q', function() { }); }); }); + + + describe('when exceptionHandler rethrows exceptions, ', function() { + var originalLogExceptions, deferred, errorSpy, exceptionExceptionSpy; + + beforeEach(function() { + // Turn off exception logging for these particular tests + originalLogExceptions = mockNextTick.logExceptions; + mockNextTick.logExceptions = false; + + // Set up spies + exceptionExceptionSpy = jasmine.createSpy('rethrowExceptionHandler') + .andCallFake(function rethrowExceptionHandler(e) { + throw e; + }); + errorSpy = jasmine.createSpy('errorSpy'); + + + q = qFactory(mockNextTick.nextTick, exceptionExceptionSpy); + deferred = q.defer(); + }); + + + afterEach(function() { + // Restore the original exception logging mode + mockNextTick.logExceptions = originalLogExceptions; + }); + + + it('should still reject the promise, when exception is thrown in success handler, even if exceptionHandler rethrows', function() { + deferred.promise.then(function() { throw 'reject'; }).then(null, errorSpy); + deferred.resolve('resolve'); + mockNextTick.flush(); + expect(exceptionExceptionSpy).toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalled(); + }); + + + it('should still reject the promise, when exception is thrown in success handler, even if exceptionHandler rethrows', function() { + deferred.promise.then(null, function() { throw 'reject again'; }).then(null, errorSpy); + deferred.reject('reject'); + mockNextTick.flush(); + expect(exceptionExceptionSpy).toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalled(); + }); + }); });