feat($http): promise-based request interceptors

[inukshuk]

Currently there is no easy way to globally intercept requests sent with $http – similar to the way responses can be intercepted with response interceptors. Furthermore, there are many legitimate scenarios and use-cases where the need for request interception arises; for example

• if security tokens, parameters or headers need to be calculated for each individual request
• for collecting statistics like request counts, measuring turnaround times etc.
• for dynamic, client-side redirects (e.g., rewrite /api/x to /api/experimental/x for resource x but not resource y)

Please see #929 for a further discussion (and use-cases) on this topic.

Implementation

The request interceptor registration works exactly like the registration of response interceptors (in fact, I have put this into a separate routine to make it more explicit). If request interceptors are present, they are prepended to the promise chain returned by $http – this was mostly devised and written by @splondike – this gives the interceptors a lot of flexibility and fits very well into the Angular API.

Tests and Documentation

This pull requests includes both tests and documentation of the new feature.

A note on testing: a separate call to $rootScope.$apply is necessary to test the interceptors with the mocked http backend.

Example

// register interceptor using an anonymous function
$httpProvider.requestInterceptors.push(function() {
  return function(promise) {
    return promise.then(function(config) {

      // the config object will be used by the actual request
      // you can modify it here

      config.url = '/intercepted/' + config.url;

      // return the config object to pass it on to the next interceptor
      return config;

      // if you reject the promise, the promise
      // returned by $http will be rejected
    };
  };
});

Compatibility

When no request interceptors are registered, no promises are prepended to the chain. Therefore, this feature should be completely backwards compatible – if you register not interceptors everything stays the same.

[pkozlowski-opensource]

@inukshuk If I understand correctly this PR is meant to replace the #929, right?

[inukshuk]

@splondike the headers in the config object should be safe despite the shallow copy: we're actually changing the value of config.headers in the copied version of the config; therefore, the passed-in headers will be used by the request but the passed-in header object should never be modified.

Of course, if my header object contains a reference to another object, the request interceptors would be able to mutate this referenced object. So given the following $http call:

$http({ headers: { a: 'b', x: { y: 'z' } });

The object referenced by x could be modified by a request interceptor (the value of a would be safe) – however, I don't think that this is a valid use case. (To defend against this, we'd have to make a deep-copy of the config for every single request).

Nevertheless, this is a good point! I'll definitely add a test case to show that regular headers will not be mangled.

@pkozlowski-opensource Yes, this PR adds the features of #929 with the added benefit of using promises.

[splondike]

@inukshuk Yeah, a deep clone would probably be going a bit far.

These patchsets look like they cover everything. Well done!

[mhevery]

Thanks for your contribution! In order for us to be able to accept it, we ask you to sign our CLA (contributor's license agreement).

CLA is important for us to be able to avoid legal troubles down the road.

For individuals (a simple click-through form):
http://code.google.com/legal/individual-cla-v1.0.html

For corporations (print, sign and scan+email, fax or mail):
http://code.google.com/legal/corporate-cla-v1.0.html

[inukshuk]

@mhevery thanks! I just signed the CLA online.

[inukshuk]

@mhevery is there any update on this? I signed the CLA online a while ago, please let me know in case there were any problems or if you require anything else. Would hate for this to be delayed due to licensing issues. Thanks!

[ghost]

This is exactly what I was looking for.

My use case: I'm using the excellent http-auth-interceptor however unfortunately the API I am consuming passes the auth-token in the response body rather than as a cookie so I need a way of injecting the new token into the config when the authService retries the buffered requests. It also means I can abstract this auth mechanism from the API calls in my code.

[mhevery]

@inukshuk Sorry about the delay. We are getting swamped. Could you help by squashing this into single CL and rebasing it onto master. My attempts of rebasing have failed.

I have flagged this PR in my inbox, so I will not miss it once you update it, and I will get right on it.

[inukshuk]

@mhevery I squashed the commit and rebased on master. There was a little conflict that was ultimately easy to resolve, however, I now have four ngResource tests failing. The tests fail because they put expectations on the entire config object in the response. We decided to make a defensive copy of the config object and added both the headers and the data to that copy to make it easier to pass around and, more importantly, to expose both to the interceptors. This way, an interceptor can alter the headers, for example, without altering the original config object.

Perhaps you could quickly review this to let me know how to proceed? For all the tests to pass I'd have to add the headers and data to the expected data object.

Here are the relevant parts in the code for your reference:

Here we add the headers and data to the config object instead of using local variables. (The config object is beeing copied a few lines above):

inukshuk@4832868#L0R541

This is one of the tests that are failing:

https://github.com/angular/angular.js/blob/master/test/ngResource/resourceSpec.js#L468

As you can see, the tests fails because it does not expect the config object to contain the headers and data.

Finally, I have included a test that makes sure that the config object originally passed to $http is not altered by all this:

inukshuk@4832868#L1R168

[ashtuchkin]

IMHO these tests should not make assumptions on response.config value.
@mhevery, is there anything else stopping this valuable PR from merge? I'm designing an authentication system right now and this piece of code could be very helpful.

[inukshuk]

I'm looking into the issues today – at the moment, using the rebased PR I see some strange behaviour in regard to the new $promise feature added to $resource which I haven't quite wrapped my head around. Specifically, the headersGetter and the the $then proxy are present in the response when I run the tests – the ngResource tests do not expect them to be there, though, and I have yet to understand why this is.

I'll report back once this is is resolved. I would also change the ngResource expectations a little to be less restrictive if that's OK.

[inukshuk]

@mhevery I worked around thee issue now: I left the basic approach as is, i.e., we're making a defensive copy of the config object and add the headers to it. This copy is passed to the request interceptors as a promise. When the promise is resolved, the config object is passed on to sedReq which removes both data and headers from the config object again in order to be backwards compatible.

IMO we should still adjust the expectations in the the ngResource tests, because they really test a specific implementation and not the specification if they make such restrictive assumptions. I'll gladly make the adjustments in a separate PR if you give me the green light.

Meanwhile, this PR is rebased on master once more and all tests should pass (waiting anxiously for Travis to finish). I added one more test to make sure that intercepted headers are actually applied, too.

[inukshuk]

@pkozlowski-opensource thanks for your support on IRC earlier – I think this is ready for review now. Let me know if anything's amiss!

[al-the-x]

This is exactly what I need for a current project to inject the OAuth 1.0A signature into my $resource requests. Great job @inukshuk and I'll be using your branch until this gets merged into master...!

[YeFFreY]

Happy that this is implemented ! good job !

[inukshuk]

Guys, thanks for spotting the bad example! I've updated the docs and also added more examples that illustrate how to intercept the request's URL, headers, or data.

[lrlopez]

Nice work! Don't forget to squash both commits into one again.

[jbdeboer]

Our new, shiny checklist:

• Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
• Feature improves existing core functionality
• API is compatible with existing Angular apis and relevant standards (if applicable)
• PR doesn't contain a breaking change
• PR contains unit tests
• PR contains documentation update
• PR passes all tests on Travis (sanity)
• PR passes all tests on ci.angularjs.org (cross-browser compatibility)
• PR is rebased against recent master
• PR is squashed into one commit per logical change
• PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
• All changes requested in review have been implemented

[jbdeboer]

IE 8/9 on ci.angularjs.org don't seem to be happy. Any ideas?

http://ci.angularjs.org/job/angular.js-james/13/testReport/

[IgorMinar]

While reviewing #1851 I had a brief look at this PR as well and started thinking that we should handle goals of this as well as #1851 PR in a single generic way and that could be via "around" interceptors.

The idea is that instead of having two different kinds of interceptors that are defined in two different places we could have a single interceptor that allows us to hook into both request and response promise chains. The biggest benefit of this is that these two hooks can share a per-request context via a closure.

proposed api (names to be discussed):

myApp.factory('myAroundInterceptor', function($rootScope, $timeout) {
    return function myAroundInterceptorInstance(configPromise, responsePromise) {
        return {
            config: configPromise.then(function(config) {
                return config
            });
            response: responsePromise.then(function(response) {
                return 'ha!';
            }
        });
}

myApp.config(function($httpProvider){
    $httpProvider.aroundInterceptors.push('myAroundInterceptor'); 
});

there are several important things going on here:

1. myAroundInterceptorInstance is a per request instance which creates a closure that can be used for sharing state between request and response interceptors
2. the request is intercepted via a promise chain, just like is proposed in this PR - that allows all kinds of sync and async interceptors to be created
3. the response promise chain needs to be constructed in the reverse order so that we can achieve this important ordering of calls: ReqInterceptor1 -> ReqInterceptor2 ---- ResponseInterceptor2 -> ResponseInterceptor1 (note the reverse order of response interceptors). This can be done behind the scenes by manually wiring up the promise chain rather then relying on automatic chaining via Promise#then.

The resolution chain then looks like this:

httpDeferred --resolves--> interceptorDeferred3 --resolves-->
interceptorDeferred2 --resolves--> interceptorDeferred1 --resolves--> $httpPromise

I believe that this would solve your use case as well as many more others. How do you feel about it?

[inukshuk]

@IgorMinar I already tried to stream-line the interceptor logic a little by using a generic function to resolve them (see https://github.com/angular/angular.js/pull/1701/files#L0L809); combining them completely takes that a big step further of course.

You mentioned the biggest benefit as having the interceptors to share closure scope. This is something that's possible now, too, (simply wrap request and response interceptor in an immediate function for instance) but I take it you also like that this way they can be defined as a service together, having explicit shared dependencies etc.?

What I am a little concerned with is testing: in the current implementation we're skipping the config promise entirely if there are no interceptors – this was done for improved backwards compatibility but also because the tests relying on the mocked HTTP backend do no expect that there is another promise that needs to be resolved. Currently, it looks like one promise chain but there are of course two deferreds involved (plus, interceptors could inject their own, too) – would wiring up the promise chain the way you suggest take care of this issue? Otherwise we need to be aware that we might have to adjust the mocked HTTP backend for this feature.

Having said that, I like the idea of having a single interceptor feature – regardless of whether I want to intercept requests or responses. It makes for a cleaner API I think.

I'll gladly take a stab at this if you guys prefer this approach – if only to earn the t-shirt I got at RuPy last year! : )

[inukshuk]

@jbdeboer the failed tests on IE should be easy to fix – I'll hold off for now though, since we'll be going for the single interceptor approach.

[ashtuchkin]

How about adding a "promise chain" concept: an object which holds a chain of promises, and can be "wrapped" or included to another promise chain. It could be something along the lines of:

myApp.factory('myInterceptor', function($rootScope, $q) {
    return function myInterceptorInstance(httpRequestChain) {
        return $q.promiseChain()
            .then(function(config) {
                return config;
            })
            .then(httpRequestChain)
            .then(function(response) {
                return 'ha!';
            });
    }
}

Possible implementation:

$q.promiseChain = function(deferred, promise) {
    deferred = deferred || $q.defer()
    promise  = promise  || deferred.promise;
    // keep beginning (deferred) and end (promise) of a promise chain
    return {
        deferred: deferred,
        promise: promise,
        then: function(success, error) {
            if (isDefined(success) && !isDefined(error) && success.deferred) {
                // Add another promise chain to the end.
                anotherChain = success;
                promise.then(anotherChain.deferred.resolve, anotherChain.deferred.reject);
                return $q.promiseChain(deferred, anotherChain.promise);
            } else {
                // Usual callbacks.
                return $q.promiseChain(deferred, promise.then(success, error));
            }
        },
    };
}

In $http:

// Create basis http request chain.
chain = $q.requestChain()
    .then(sendReq)
    .then(transformResponse, transformResponse);

// Wrap chain by interceptors.
forEach(interceptors, function(interceptor) {
    chain = interceptor(chain);
});

// Start the chain.
chain.deferred.resolve(config);

// If we only need promise interface, return underlying promise.
return chain.promise;

PROs:

1. No pre/post interceptors, single scope.
2. Full control: ability to circumvent the http request at all (maybe replace it with websocket interaction).
3. The order of pre & post functions is automatically correct.
4. Expressive (IMHO).
5. Reusable async decoration pattern for other parts of Angular & users.

CONs:

1. Exposing defer interface to the user.
2. Users could forget to insert httpRequestChain.

[mhevery]

Thanks for your hard work. I have used your tests, but we have done a slightly different implementation here: #2130 which we believe is a bit more consistant.

[IgorMinar]

@ashtuchkin it's quite important for me that we don't leak deferred objects

[IgorMinar]

I'm closing this PR since it has been superseded by #2130