From 8ee086419bf2887c140d6980f8e6db777e9cf4fa Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Wed, 26 Mar 2014 15:04:16 -0300 Subject: [PATCH 1/2] fix(ngResource): do not call callback with a promise Call the success and failure callbacks for an action with the response object. Never call it with a promise object. Closes #6731 --- src/ngResource/resource.js | 4 ++- test/ngResource/resourceSpec.js | 56 +++++++++++++++++++++++++++++++-- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 8927a2664c16..66e192847708 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -559,7 +559,9 @@ angular.module('ngResource', ['ng']). promise = promise.then( function(response) { var value = responseInterceptor(response); - (success||noop)(value, response.headers); + $q.when(value).then(function(value) { + (success||noop)(value, response.headers); + }); return value; }, responseErrorInterceptor); diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 13317155fa21..e3f98b4f3372 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -600,11 +600,12 @@ describe("resource", function() { describe('promise api', function() { - var $rootScope; + var $rootScope, $q; - beforeEach(inject(function(_$rootScope_) { + beforeEach(inject(function(_$rootScope_, _$q_) { $rootScope = _$rootScope_; + $q = _$q_; })); @@ -939,6 +940,57 @@ describe("resource", function() { expect(response.status).toBe(404); expect(response.config).toBeDefined(); }); + + it('should call the success callback with the object returned from the interceptor, not a promise', function() { + CreditCard = $resource('/CreditCard', {}, { + query: { + method: 'get', + isArray: true, + interceptor: { + response: function(response) { + return $q.when(response.data); + } + } + } + }); + + $httpBackend.expect('GET', '/CreditCard').respond([{id: 1}]); + + CreditCard.query(callback); + + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); + + var response = callback.mostRecentCall.args[0]; + expect(response.then).not.toBeDefined(); + expect(response[0].id).toEqual(1); + }); + + it('should call the failure callback with the object returned from the interceptor, not a promise', function() { + CreditCard = $resource('/CreditCard', {}, { + query: { + method: 'get', + isArray: true, + interceptor: { + responseError: function(response) { + return $q.when(response); + } + } + } + }); + + $httpBackend.expect('GET', '/CreditCard').respond(404); + + CreditCard.query(function() {}, callback); + + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); + + var response = callback.mostRecentCall.args[0]; + expect(response.then).not.toBeDefined(); + expect(response.status).toEqual(404); + expect(response.config).toBeDefined(); + }); }); From ecdaf6611a152cea2989a334ed0bad57b8a5eb69 Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Mon, 31 Mar 2014 09:47:50 -0300 Subject: [PATCH 2/2] fix(ngResource): make success and error callbacks behave more expectedly Call the success and error callbacks with the values returned by the response and responseError interceptors, respectively. If the interceptor returns a promise, call the appropriate callback with the resolution or rejection value of that promise. Previously, the error callback was called with the server response before calling the interceptor, regardless of whether the error interceptor handled the error, and regardless of what it returned. Also, if the server response was successful, but the response interceptor returned a rejection, no callback was called. --- src/ngResource/resource.js | 22 +++++- test/ngResource/resourceSpec.js | 130 ++++++++++++++++++++++---------- 2 files changed, 111 insertions(+), 41 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 66e192847708..044efc78c060 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -551,20 +551,36 @@ angular.module('ngResource', ['ng']). }, function(response) { value.$resolved = true; - (error||noop)(response); - return $q.reject(response); }); promise = promise.then( function(response) { var value = responseInterceptor(response); + $q.when(value).then(function(value) { (success||noop)(value, response.headers); + }, function(response) { + (error||noop)(response); }); + return value; }, - responseErrorInterceptor); + function(response) { + if (responseErrorInterceptor) { + response = responseErrorInterceptor(response); + } else { + response = $q.reject(response); + } + + $q.when(response).then(function(value) { + (success||noop)(value, response.headers); + }, function(response) { + (error||noop)(response); + }); + + return response; + }); if (!isInstanceCall) { // we are creating instance / collection diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index e3f98b4f3372..59d559cee350 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -600,12 +600,11 @@ describe("resource", function() { describe('promise api', function() { - var $rootScope, $q; + var $rootScope; - beforeEach(inject(function(_$rootScope_, _$q_) { + beforeEach(inject(function(_$rootScope_) { $rootScope = _$rootScope_; - $q = _$q_; })); @@ -941,55 +940,110 @@ describe("resource", function() { expect(response.config).toBeDefined(); }); - it('should call the success callback with the object returned from the interceptor, not a promise', function() { - CreditCard = $resource('/CreditCard', {}, { - query: { - method: 'get', - isArray: true, - interceptor: { - response: function(response) { - return $q.when(response.data); + describe('when the response interceptor returns a promise', function() { + var makeCreditCard, $q; + + beforeEach(inject(function(_$q_) { + makeCreditCard = function(interceptor) { + return $resource('/CreditCard', {}, { + query: { + method: 'get', + isArray: true, + interceptor: interceptor } + }); + }; + + $q = _$q_; + + $httpBackend.expect('GET', '/CreditCard').respond([{id: 1}]); + })); + + it('should call the success callback with the resolution value of the promise', function() { + CreditCard = makeCreditCard({ + response: function(response) { + return $q.when(response.data); } - } - }); + }); - $httpBackend.expect('GET', '/CreditCard').respond([{id: 1}]); + CreditCard.query(callback); + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); - CreditCard.query(callback); + var response = callback.mostRecentCall.args[0]; + expect(response.then).not.toBeDefined(); + expect(response[0].id).toEqual(1); + }); - $httpBackend.flush(); - expect(callback).toHaveBeenCalledOnce(); + it('should call the error callback with the rejection value of the promise', function() { + CreditCard = makeCreditCard({ + response: function(response) { + return $q.reject(response); + } + }); - var response = callback.mostRecentCall.args[0]; - expect(response.then).not.toBeDefined(); - expect(response[0].id).toEqual(1); + CreditCard.query(function() {}, callback); + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); + + var response = callback.mostRecentCall.args[0]; + expect(response.then).not.toBeDefined(); + expect(response.status).toEqual(200); + expect(response.config).toBeDefined(); + }); }); - it('should call the failure callback with the object returned from the interceptor, not a promise', function() { - CreditCard = $resource('/CreditCard', {}, { - query: { - method: 'get', - isArray: true, - interceptor: { - responseError: function(response) { - return $q.when(response); + describe('when the responseError interceptor returns a promise', function() { + var makeCreditCard, $q; + + beforeEach(inject(function(_$q_) { + makeCreditCard = function(interceptor) { + return $resource('/CreditCard', {}, { + query: { + method: 'get', + isArray: true, + interceptor: interceptor } + }); + }; + + $q = _$q_; + + $httpBackend.expect('GET', '/CreditCard').respond(404); + })); + + it('should call the success callback with the resolution value of the promise', function() { + CreditCard = makeCreditCard({ + responseError: function(response) { + return $q.when({id: 1}); } - } - }); + }); - $httpBackend.expect('GET', '/CreditCard').respond(404); + CreditCard.query(callback); + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); - CreditCard.query(function() {}, callback); + var response = callback.mostRecentCall.args[0]; + expect(response.then).not.toBeDefined(); + expect(response.id).toEqual(1); + }); - $httpBackend.flush(); - expect(callback).toHaveBeenCalledOnce(); + it('should call the error callback with the rejection value of the promise', function() { + CreditCard = makeCreditCard({ + responseError: function(response) { + return $q.reject(response); + } + }); - var response = callback.mostRecentCall.args[0]; - expect(response.then).not.toBeDefined(); - expect(response.status).toEqual(404); - expect(response.config).toBeDefined(); + CreditCard.query(function() {}, callback); + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnce(); + + var response = callback.mostRecentCall.args[0]; + expect(response.then).not.toBeDefined(); + expect(response.status).toEqual(404); + expect(response.config).toBeDefined(); + }); }); });