fix(resource): check whether response matches action.isArray #3054

petebacondarwin · 2013-06-25T12:59:56Z

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 this change, 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

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 angular#2255, angular#1044

btford · 2013-07-12T00:48:05Z

LGTM. Would like to run this by @IgorMinar before merging.

IgorMinar · 2013-07-14T04:51:11Z

src/ngResource/resource.js

@@ -472,6 +472,10 @@ angular.module('ngResource', ['ng']).
                promise = value.$promise;

            if (data) {
+              if ( angular.isArray(data) != !!action.isArray ) {
+                return $q.reject("Error in resource configuration. Expected response to contain an " +
+                  (action.isArray?'array':'object') + " but got an " + (angular.isArray(data)?'array':'object'));


can we throw a minErr instead of explicitly rejecting the promise?

IgorMinar · 2013-07-14T04:52:09Z

Nice fix, but please consider throwing minErr instead

petebacondarwin · 2013-07-14T08:43:17Z

Will throwing the minErr be converted to a $q rejection?

On 14 July 2013 05:52, Igor Minar notifications@github.com wrote:

Nice fix, but please consider throwing minErr instead

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3054#issuecomment-20931696
.

ghost · 2013-07-18T16:51:08Z

bump!

IgorMinar · 2013-07-18T19:21:54Z

@petebacondarwin I spoke with @ksheedlo and asked him to merge this one as well as #3159 in after some modifications

petebacondarwin · 2013-07-18T19:28:36Z

Great!

ksheedlo · 2013-07-19T18:37:24Z

I merged #3159 yesterday, but this one is giving me some trouble. I checked it out and changed the return $q.reject to a throw ngResourceMinErr and I'm getting strange test failures:

Chrome 28.0.1500 (Linux) resource should fail if action expects an object but response is an array FAILED
    Error: [ngResource:nocfg] Error in resource configuration. Expected response to contain an array but got an object
        at /usr/local/google/home/sheedlo/angular/angular.js/src/minErr.js:55:12
        at $http.then.value.$resolved (/usr/local/google/home/sheedlo/angular/angular.js/src/ngResource/resource.js:477:23)
        at wrappedCallback (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/q.js:238:59)
        at wrappedCallback (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/q.js:238:59)
        at /usr/local/google/home/sheedlo/angular/angular.js/src/ng/q.js:319:26
        at Object.Scope.$eval (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:653:28)
        at Object.Scope.$digest (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:509:23)
        at Object.Scope.$apply (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:739:24)
        at done (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/http.js:976:45)
        at handleResponse (/usr/local/google/home/sheedlo/angular/angular.js/src/ngMock/angular-mocks.js:985:9)
    Error: [$rootScope:inprog] $digest already in progress
        at /usr/local/google/home/sheedlo/angular/angular.js/src/minErr.js:55:12
        at beginPhase (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:930:15)
        at Object.Scope.$digest (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:501:9)
        at Function.$httpBackend.verifyNoOutstandingExpectation (/usr/local/google/home/sheedlo/angular/angular.js/src/ngMock/angular-mocks.js:1325:16)
        at null.<anonymous> (/usr/local/google/home/sheedlo/angular/angular.js/test/ngResource/resourceSpec.js:31:18)
Chrome 28.0.1500 (Linux) resource should fail if action expects an array but response is an object FAILED
    Error: [ngResource:nocfg] Error in resource configuration. Expected response to contain an object but got an array
        at /usr/local/google/home/sheedlo/angular/angular.js/src/minErr.js:55:12
        at $http.then.value.$resolved (/usr/local/google/home/sheedlo/angular/angular.js/src/ngResource/resource.js:477:23)
        at wrappedCallback (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/q.js:238:59)
        at wrappedCallback (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/q.js:238:59)
        at /usr/local/google/home/sheedlo/angular/angular.js/src/ng/q.js:319:26
        at Object.Scope.$eval (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:653:28)
        at Object.Scope.$digest (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:509:23)
        at Object.Scope.$apply (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:739:24)
        at done (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/http.js:976:45)
        at handleResponse (/usr/local/google/home/sheedlo/angular/angular.js/src/ngMock/angular-mocks.js:985:9)
    Error: [$rootScope:inprog] $digest already in progress
        at /usr/local/google/home/sheedlo/angular/angular.js/src/minErr.js:55:12
        at beginPhase (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:930:15)
        at Object.Scope.$digest (/usr/local/google/home/sheedlo/angular/angular.js/src/ng/rootScope.js:501:9)
        at Function.$httpBackend.verifyNoOutstandingExpectation (/usr/local/google/home/sheedlo/angular/angular.js/src/ngMock/angular-mocks.js:1325:16)
        at null.<anonymous> (/usr/local/google/home/sheedlo/angular/angular.js/test/ngResource/resourceSpec.js:31:18)

An error thrown inside the promise should be converted to a $q.reject internally. The stack trace shows that it's not happening. The other odd thing about this is that the failure is occurring in afterEach when we call $httpBackend.verifyNoOutstandingExpectation. So the expectations are passing in the body of the tests.

I don't have time to go in and debug this right now. There's a chance I could get to it later this afternoon or sometime next week, but if someone else wants to take a look, that would be appreciated.

ksheedlo · 2013-07-19T20:29:04Z

If anyone else is going to try and reproduce the behavior I'm getting, I've published a branch: https://github.com/ksheedlo/angular.js/tree/pr3054

petebacondarwin · 2013-07-21T06:11:09Z

Is it that you are not yet in a promise handler?

Pete
...from my mobile.
On 19 Jul 2013 21:29, "Ken Sheedlo" notifications@github.com wrote:

If anyone else is going to try and reproduce the behavior I'm getting,
I've published a branch:
https://github.com/ksheedlo/angular.js/tree/pr3054

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3054#issuecomment-21275365
.

petebacondarwin · 2013-07-30T20:23:04Z

Hey @ksheedlo - I think you have uncovered a pretty major bug in $q!
On this line: https://github.com/angular/angular.js/blob/master/src/ng/q.js#L240 and this line: https://github.com/angular/angular.js/blob/master/src/ng/q.js#L249, exceptionHandler(e) is called before result.reject(e), which actually re-throws the error and so reject() is never called.

This is not picked up in the $q spec since $exceptionHandler is mocked out with effectively noop()

petebacondarwin · 2013-07-30T21:24:30Z

Actually the problem is simply that, by default, ngMock.$exceptionHandlerProvider is set to rethrow unless told otherwise (i.e. to "log"). Many specs do this: notably ngRepeat and $http. I propose that we do this here too.

petebacondarwin · 2013-07-30T21:54:05Z

Closing in favour of #3400

petebacondarwin mentioned this pull request Jun 25, 2013

fix(copy) $resource throws TypeError when json has both properties & arrays #2255

Closed

IgorMinar reviewed Jul 14, 2013
View reviewed changes

petebacondarwin mentioned this pull request Jul 30, 2013

fix(resource): check whether response matches action.isArray #3400

Closed

petebacondarwin closed this Jul 30, 2013

petebacondarwin deleted the resource-err-msg branch November 7, 2013 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resource): check whether response matches action.isArray #3054

fix(resource): check whether response matches action.isArray #3054

petebacondarwin commented Jun 25, 2013

btford commented Jul 12, 2013

IgorMinar Jul 14, 2013

IgorMinar commented Jul 14, 2013

petebacondarwin commented Jul 14, 2013

ghost commented Jul 18, 2013

IgorMinar commented Jul 18, 2013

petebacondarwin commented Jul 18, 2013

ksheedlo commented Jul 19, 2013

ksheedlo commented Jul 19, 2013

petebacondarwin commented Jul 21, 2013

petebacondarwin commented Jul 30, 2013

petebacondarwin commented Jul 30, 2013

petebacondarwin commented Jul 30, 2013

fix(resource): check whether response matches action.isArray #3054

fix(resource): check whether response matches action.isArray #3054

Conversation

petebacondarwin commented Jun 25, 2013

btford commented Jul 12, 2013

IgorMinar Jul 14, 2013

Choose a reason for hiding this comment

IgorMinar commented Jul 14, 2013

petebacondarwin commented Jul 14, 2013

ghost commented Jul 18, 2013

IgorMinar commented Jul 18, 2013

petebacondarwin commented Jul 18, 2013

ksheedlo commented Jul 19, 2013

ksheedlo commented Jul 19, 2013

petebacondarwin commented Jul 21, 2013

petebacondarwin commented Jul 30, 2013

petebacondarwin commented Jul 30, 2013

petebacondarwin commented Jul 30, 2013