Add request and requestError interception in ngResource #13273

krotscheck · 2015-11-07T00:07:56Z

This patch adds request and requestError interception for ngResource actions,
as per the documentation found for $http interceptors. It is important to
note that returning an error at this stage of the request - before the
call to $http - will completely bypass any global interceptors and/or recovery
handlers, as those are added to a separate context. This is intentional; intercepting
a request before it is passed to the HTTP stack indicates that the resource itself
has made a decision, and that it accepts the responsibility for recovery.

Closes #5146

googlebot · 2015-11-07T00:07:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2015-11-07T00:14:45Z

CLAs look good, thanks!

gkalpak · 2015-11-07T19:46:01Z

src/ngResource/resource.js

@@ -593,7 +601,13 @@ angular.module('ngResource', ['ng']).
              extend({}, extractParams(data, action.params || {}), params),
              action.url);

-            var promise = $http(httpConfig).then(function(response) {
+            var promise = $q.resolve(httpConfig);


Why not directly: $q.when(requestInterceptor(httpConfig), null, requestErrorInterceptor)

Well, that's how my brain addressed the problem. First, peel everything into a promise chain. Second, add the desired step into that chain. While I can collapse it easily enough, I personally find it more legible, as each statement is discrete and easily understood.

I don't mind updating the commit though, if there's consensus that your proposed syntax is more desirable.

krotscheck · 2015-11-10T18:22:12Z

As an additional note: The reason I'd like to land this in angular, is because I'd like to build resources who get their root API url from a promise.

Resource action is requested (GET /foo/:bar)
request interceptor decorates the relative URL with the discovered API endpoint (GET https://example.com/api/v1/foo/:bar)
request is made.

Since I live in a multi-api environment, resolving this in the resource made the most sense.

krotscheck · 2016-02-08T17:08:37Z

@petebacondarwin Heya- with 1.5 out now, what are the next steps for this PR? Should I rebase, update, add docs... any guidance?

gkalpak · 2016-02-08T21:31:48Z

@krotscheck, now that 1.5.0 is out (and since this is on the 1.6.x milestone), I will take a look soon and get back to you. You don't need to rebase for now. I'll let you know if you need to update anything.

Thx for working on this !

krotscheck · 2016-02-09T19:23:06Z

@gkalpak Awesome! Looking forward to your comments :)

gkalpak · 2016-02-19T17:04:50Z

src/ngResource/resource.js

@@ -664,7 +672,9 @@ angular.module('ngResource', ['ng']).
              extend({}, extractParams(data, action.params || {}), params),
              action.url);

-            var promise = $http(httpConfig).then(function(response) {
+            var promise = $q.when(requestInterceptor(httpConfig), null, requestErrorInterceptor);


This will not catch errors thrown in the requestInterceptor. The requestErrorInterceptor will only be called if requestInterceptor returns a rejecting promise.

I think the following is better:

$q.when(httpConfig).then(requestInterceptor).catch(requestErrorInterceptor)

gkalpak · 2016-02-19T17:23:17Z

@krotscheck, I left a couple of comments.

I think it's a good change overall, but I would like to have more tests. E.g.:

Test that $http is called with the value returned from requestInterceptor.
Test that throwing inside the requestInterceptor calls the requestErrorInterceptor.
Test that a rejecting requestInterceptor aborts the operation.
Test that a rejecting requestErrorInterceptor aborts the operation.
Test that a requestErrorInterceptor can recover from rejection (and the operation continues as usual).
...something else that I have missed...

Feel free to ping me when/if you make any changes, so I can take another look.

BTW, there is some ongoing discussion about response interceptors (or interceptors in general) in the context of $resource (e.g. in #9334 and elsewhere). Their behavior is currently kind of inconsistent and confusing - I would like to get it right for v1.6.x. @krotscheck, if you have any insights, please do chime in 😃

krotscheck · 2016-02-22T16:48:52Z

@gkalpak Tests added, feedback incorporated. I also concatenated the promise chain, which had the unexpected impact of creating a gross diff (indentation had to be updated). I can go back and separate the two if you'd like me to.

gkalpak · 2016-02-22T18:24:58Z

@krotscheck, thx ! The diff is fine if you ignore whitespace 😉
Taking a look...

gkalpak · 2016-02-22T18:34:15Z

test/ngResource/resourceSpec.js

+
+        // Ensure the resource promise was rejected
+        expect(card.$resolved).toBeTruthy();
+        expect(card.$promise.$$state.status).toEqual(2);


This is private API surface. We don't need to verify that. (It might change anytime anyway.)

gkalpak · 2016-02-22T18:48:23Z

I left a couple of comments. Additionally, where applicable, I think it's a good idea to use success/error callbacks to verify that the appropriate callback has been called in each case.

krotscheck · 2016-02-23T17:28:11Z

@gkalpak Alright, the most recent patch has the changes you asked for.

gkalpak · 2016-04-13T12:47:40Z

I left some minor comments, but generally LGTM.

WRT #13273 (comment), I think we should do it.
It's better to be consistent, than have people wondering why their tests are not making a request when they add an interceptor. Besides, in most cases people will call $httpBackend.flush(), which will take care of $rootScope.$digest(), so this shouldn't affect many people.

krotscheck · 2016-04-13T12:56:34Z

Okies, I'm on it.

This patch adds request and requestError interception for ngResource actions, as per the documentation found for $http interceptors. It is important to note that returning an error at this stage of the request - before the call to $http - will completely bypass any global interceptors and/or recovery handlers, as those are added to a separate context. This is intentional; intercepting a request before it is passed to the HTTP stack indicates that the resource itself has made a decision, and that it accepts the responsibility for recovery. Closes #5146

krotscheck · 2016-05-05T15:46:54Z

Theeeere we go. Was temporarily derailed by #13123, but used the workaround for great justice.

krotscheck · 2016-06-03T17:47:17Z

@gkalpak Hey there! It's been a month since my last patch, which incorporates all the changes you asked for. Is there anything else we're missing?

gkalpak · 2016-06-07T10:57:42Z

@krotscheck: Time 😃

I still wish to get the ngResource interceptors/transforms story fixed for 1.6, but I have to talk people into this 😛
I'll do my best!

gkalpak · 2016-06-07T12:35:00Z

BTW, this should also fix #12095. I would be nice to add a testcase for that (i.e. testing that the request interceptor will be able to modify the headers).

krotscheck · 2016-06-07T17:05:28Z

Oh, all you need is time? I had some of that lying around here somewhere... now where did it... hold on... drat!

A few questions:

What's the cutoff for 1.6? I don't have a lot of extra time, but I may be able to convince someone in the OpenStack community to look at the interceptor implementation.
When you say: "It would be nice to add a testcase", is that a "Add this testcase or I won't merge it", or is this a "If you get around to it, yay, but otherwise this is fine."?
What's the process from here? Do you just merge it? Will things get approved in a batch? Do I have to do a rain dance?

petebacondarwin · 2016-06-07T17:32:31Z

The aim is to release 1.6 in September. So there is time (of one kind or another) but perhaps not the kind of time that you need :-)

gkalpak · 2016-06-07T20:53:28Z

I may be able to convince someone in the OpenStack community to look at the interceptor implementation.

If this is a response to my "I have to talk people into this", I meant I people on the ng1 team 😃
(Not that they need much talking into - just a tiny bit 😃 )

is that a "Add this testcase or I won't merge it", or is this a "If you get around to it, yay, but otherwise this is fine."

The latter

What's the process from here?

Review --> LGTM --> LGT another team member --> Merge
(All sprinkled with a good overall strategy/plan for moving forward. But I prefer to make small incremental changes.)

Do you just merge it?

We don't merge - we rebase 😛

Will things get approved in a batch?

I hope not.

Do I have to do a rain dance?

It's not necessary, but people have reported it helps 😛

gercheq · 2016-12-02T18:15:26Z

@gkalpak When do you think this will be released? Currently it's not possible to transform the request headers with $resource dynamically.

                        search: {
                            method: 'GET',
                            params: {
                                organizationId: "@organizationId",
                                q: '@q'
                            },
                            transformRequest: function(data, getHeaders) {
                            
                                var token = ContactsContinuationService.getToken(),
                                    headers = getHeaders();
                            
                                headers['Continuation'] = token;
                                headers['RECEP'] = "SUCKS";
                            
                                return data;
                            },

                            url: ApiPath('/organisations/:organizationId/contacts')
                        },

Declaration above doesn't work. It transforms the header but the final request does not have these new headers.

I ended up wrapping the $resource with another function as mentioned here : http://stackoverflow.com/questions/20566593/dynamic-resource-headers

Thanks for the PR @krotscheck

gkalpak · 2016-12-02T21:46:40Z

The milestone is 1.6.x (post 1.6.0), so after 1.6.0 has been released (probably in 1.6.1).

Narretz · 2017-01-30T16:51:39Z

test/ngResource/resourceSpec.js

+            isArray: true,
+            interceptor: {
+              request: function(httpConfig) {
+                callback(httpConfig);


Is the callback defined somewhere?

Narretz · 2017-01-30T16:53:32Z

There's now a merge conflict (due to 39a90a9 I think). Must be resolved, but should be easy. Can you update the PR @krotscheck ?

krotscheck · 2017-01-30T16:58:14Z

Sadly, I've switched jobs. You'll need to find someone else to update this patch.

Narretz · 2017-01-30T17:20:33Z

Ok. Thanks for the head's up

Narretz · 2017-02-03T12:26:23Z

I wrote this in Slack but it must have been overlooked: I know we talked previously about how the interceptor API is different between ngResource and $http. Did you take this into consideration when approving the PR @gkalpak ? I wonder if we could theoretically move the intereption to $http. Basically, have request-level interceptors in http, which are followed by the app-global interceptors? We don't have to do this in this PR, I just don't want to introduce an API that is blocking us in the future

gkalpak · 2017-02-03T12:44:54Z

Yeah, I have proposed something similar here and there. My "problem" with the current implementation is that is is too $resource-specific and the interceptor has to know about $resource-internals (e.g. manipulate and return response.resource as explained here). This only affects response interceptors, so I don't think requestInterceptors will pose any problems.

Even if we decided to make this an $http feature, this PR ~~would make~~would not make it more difficult, because the requestInterceptors introduced here have the same signature as the ~~per-request~~ $http ones.

So, I think we are good.

Narretz · 2017-02-03T13:17:18Z

Regarding if we decided to make this an $http feature, this PR would make it more difficult, because the requestInterceptors introduced here have the same signature as the per-request $http ones.

I guess you mean, "would not make it more difficult"?
And there's currently no per-request http interceptor, only the global ones, and transformRequest / transformResponse

gkalpak · 2017-02-03T13:30:25Z

That's what I said 😛

gkalpak · 2017-02-03T13:43:04Z

I now realize I have no idea why I didn't put it on the 1.6.0 milestone, as I clearly think this is (tiny) breaking change (see here, here and here) 🤔

We can make the BC go away by undoing this, but then it will be a little inconsistent (in practice only tests would be affected).

gkalpak · 2017-02-03T13:57:08Z

Here is the rebased version btw: #15674

petebacondarwin · 2017-02-06T19:39:40Z

Closing in favour of #15674

googlebot added the cla: no label Nov 7, 2015

googlebot added cla: yes and removed cla: no labels Nov 7, 2015

gkalpak reviewed Nov 7, 2015
View reviewed changes

petebacondarwin added this to the 1.6.x milestone Nov 26, 2015

petebacondarwin assigned gkalpak Nov 26, 2015

petebacondarwin added type: feature component: ngResource labels Nov 26, 2015

Narretz force-pushed the master branch from 70049ae to b77e14b Compare January 16, 2016 21:59

gkalpak reviewed Feb 19, 2016
View reviewed changes

gkalpak added needs: work needs: test labels Feb 19, 2016

gkalpak reviewed Feb 22, 2016
View reviewed changes

gkalpak mentioned this pull request Jun 7, 2016

Allow transformRequest to modify headers #12095

Closed

gkalpak mentioned this pull request Jul 18, 2016

ngResource does not apply request interceptor #5146

Closed

petebacondarwin mentioned this pull request Oct 28, 2016

Resource interceptor is not actually an interceptor, just a function that returns what becomes result of resource promise. #11409

Closed

gkalpak modified the milestones: 1.6.2, 1.6.x Dec 23, 2016

Narretz reviewed Jan 30, 2017

View reviewed changes

gkalpak mentioned this pull request Feb 3, 2017

feat($resource): add support for request and requestError interceptors #15674

Merged

petebacondarwin closed this Feb 6, 2017

Add request and requestError interception in ngResource #13273

Add request and requestError interception in ngResource #13273

Conversation

krotscheck commented Nov 7, 2015

googlebot commented Nov 7, 2015

googlebot commented Nov 7, 2015

gkalpak Nov 7, 2015

Choose a reason for hiding this comment

krotscheck Nov 7, 2015

Choose a reason for hiding this comment

krotscheck commented Nov 10, 2015

krotscheck commented Feb 8, 2016

gkalpak commented Feb 8, 2016

krotscheck commented Feb 9, 2016

gkalpak Feb 19, 2016

Choose a reason for hiding this comment

gkalpak commented Feb 19, 2016

krotscheck commented Feb 22, 2016

gkalpak commented Feb 22, 2016

gkalpak Feb 22, 2016

Choose a reason for hiding this comment

gkalpak commented Feb 22, 2016

krotscheck commented Feb 23, 2016

gkalpak commented Apr 13, 2016 • edited Loading

krotscheck commented Apr 13, 2016

krotscheck commented May 5, 2016

krotscheck commented Jun 3, 2016

gkalpak commented Jun 7, 2016

gkalpak commented Jun 7, 2016

krotscheck commented Jun 7, 2016

petebacondarwin commented Jun 7, 2016

gkalpak commented Jun 7, 2016 • edited Loading

gercheq commented Dec 2, 2016 • edited Loading

gkalpak commented Dec 2, 2016

Narretz Jan 30, 2017

Choose a reason for hiding this comment

Narretz commented Jan 30, 2017

krotscheck commented Jan 30, 2017

Narretz commented Jan 30, 2017 via email

Narretz commented Feb 3, 2017

gkalpak commented Feb 3, 2017 • edited Loading

Narretz commented Feb 3, 2017 • edited Loading

gkalpak commented Feb 3, 2017

gkalpak commented Feb 3, 2017 • edited Loading

gkalpak commented Feb 3, 2017

petebacondarwin commented Feb 6, 2017

gkalpak commented Apr 13, 2016 •

edited

Loading

gkalpak commented Jun 7, 2016 •

edited

Loading

gercheq commented Dec 2, 2016 •

edited

Loading

gkalpak commented Feb 3, 2017 •

edited

Loading

Narretz commented Feb 3, 2017 •

edited

Loading

gkalpak commented Feb 3, 2017 •

edited

Loading