ngResource interceptors called inconsistently #9334

jrm-airwatch · 2014-09-29T23:27:08Z

Action-specific interceptors defined in a $resource call are called inconsistently. A response interceptor is called before the success callback, but a responseError interceptor is called after the error callback. Further, the success callback is called even if the response interceptor rejects, and the error callback is called even if the responseError interceptor resolves. And response data is populated before any interceptors run.

This behavior runs contrary to the expectation that an interceptor should transparently intercept results before passing them to the consumer. A more sensible flow would be

http success -> interceptor.response (resolves) -> populate results -> success callback
http success -> interceptor.response (rejects) -> error callback
http error -> interceptor.responseError (resolves) -> populate results -> success callback
http error -> interceptor.responseError (rejects) -> error callback

As it stands, even if a responseError interceptor successfully retries, the error callback is still called, and no response data is populated!

plunker

nicole-ashley · 2016-02-12T04:24:03Z

This is quite a frustrating issue. If it helps, here is a plunker that demonstrates the responseError issues. Check the console logs to see the issue.

Note that:

interceptors are called after callbacks
modified payloads (both content and success/rejection) are ignored

gkalpak · 2016-02-17T22:30:35Z

Indeed the concept of interceptors in the context of $resource is very vague. In the documentation, it is implied that interceptors are essentially $http interceptors, but they are used quite differently. Furthermore, their behavior is inconsistent and not well defined.

Similar issues have been discussed in #11409, as well.
IMO, $resource interceptors should be behave like this:

They should be applied to the promise returned by the call to $http (before anything $resource-specific).
The appropriate $resource interceptor should be applied (if specified by the user). Interceptors should be passed the response - nothing $resource-specific yet.
If the interceptor rejects, the error callback should be called (and the data should not be updated).
If the interceptor resolves, $resource should update the returned instance based on the content of response.data and call the success callback.

Thoughts anyone ?

krotscheck · 2016-02-22T13:24:54Z

I overall agree with the original issue description and @gkalpak's assessment. Also, I'm making the assumption that transformRequest and transformResponse both execute before/after the interceptors are considered, so I'm deliberately not including them.

Errors on response interceptors aren't the only problem - request interceptors are not actually supported by $resource at this time (see my patch), and introducing them makes the resolution of request interceptor errors tricky. If an $http request interceptor errors, which handler in $resource should get it? Since you've already passed down to the $http level, I can argue that the request has already been handed off, and everything should be treated as a response (i.e. $resource.interceptor.responseError). Alternatively, I can argue that since the error occurred during request preprocessing, it's still a request and therefore should be handled by $resource.interceptor.requestError.

My mental model of this is basically a hole. At the bottom of the hole is your response data, and you can only climb down the hole far enough until you get your first error. After that, you have to climb back up through the error interceptors instead of the response interceptors. Thus I'm more in favor of the "Everything from $http is considered a response error" way of thinking of the problem, which from what I understand is how it works now. I'd have to dig into it a bit more to see how the responseError headers and data are constructed to see if that makes sense.

gkalpak · 2016-02-22T19:36:03Z

@krotscheck, good points raised. I too lean towards treating anything coming back from the call to $http as "response" (we wouldn't be able to treat is otherwise without hacking $http's behavior too and that's something I'd like to avoid).

In a sense they are not request interceptors (as those in $http's config), they are more like "$http interceptors", i.e. intercepting the call to $http. At this point, I don't think it makes sense to change names (e.g. preHttp/preHttpError/postHttp/postHttpError*), as it would probably be more confusing to developers than helpful.

It's important to point out the bahavior in the documentation, though.

_{(*: Yeah, I know, I'm not good with names...)}

krotscheck · 2016-02-23T12:56:15Z

One more thing I discovered: requestInterceptors on $http are arrays of methods. For $resource they're individual methods. Probably something we should update.

Narretz · 2017-11-01T12:58:59Z

I'm not sure if it's useful to change the behavior of the resource interceptors at this point. It would be good for consistency but if we change responseInterceptors to run before the resource specific transformations, devs might have to change their code for little gain (because they relied on the previous behavior).

It might be enpugh to have requestInterceptors that work analogue to responseInterceptors for consistency: #15674, and document how exactly the interceptors work.

A bigger, non-breaking change would be to introduce local http interceptors that work like global http interceptors, but only for single requests. Those could then be passed to resource as a httpOption.
This would also make it possible to change the headers in an individual request again: #11438

gkalpak · 2018-02-10T13:00:48Z

I am working on a PR that will change the raltive order (and bahavior) response interceptors and success/error callbacks in the following way:

Before:

After:

I think it is reasonable enough (even if not perfect).

For people not using success/error callbacks, it should make no difference.
For people using success/error callbacks (with or without response interceptors), the main difference will be that throwing inside the callbacks will not propagate to $promise. (I don't think this matters much, since using these callbacks is discouraged anyway.)
In addition, for people using success/error callbacks with response interceptors, another difference will be that the callbacks will always be run after the interceptors and wait for them to resolve (in case they return a promise), which gives the interceptor the chance to "course correct" before hitting the callbacks (e.g. recovering from a failure or failing from a successful response ¯\_(ツ)_/¯).
For reference, previously the error callback was called before the interceptor and the success callback was called after the interceptor but without waiting for any returned promise to resolve.

Previously, action-specific interceptors and `success`/`error` callbacks were executed in inconsistent relative orders and in a way that did not meet the general expectation for interceptor behavior (e.g. ability to recover from errors, performing asynchronous operations, etc). This commit fixes the behavior to make it more consistent and expected. The main differences are that `success`/`error` callbacks will now be run _after_ `response`/`responseError` interceptors complete (even if they return a promise) and correct callback will be called based on the result of the interceptor (e.g. if the `responseError` interceptor recovers from an error, the `success` calback will be called). See also angular#9334 (comment). Fixes angular#9334 BREAKING CHANGE: If you are not using `success` or `error` callbacks with `$resource`, your app should not be affected by this commit. If you are using `success` or `error` callbacks (with or without response interceptors), one (subtle) difference is that throwing an error inside the callbacks will not propagate to the returned `$promise`. If you are using `success` or `error` callbacks with response interceptors, the callbacks will now always run _after_ the interceptors (and wait for them to resolve in case they return a promise). Previously, the `error` callback was called before the `responseError` interceptor and the `success` callback was synchronously called after the `response` interceptor.

@id

Previously, action-specific interceptors and `success`/`error` callbacks were executed in inconsistent relative orders and in a way that did not meet the general expectation for interceptor behavior (e.g. ability to recover from errors, performing asynchronous operations, etc). This commit fixes the behavior to make it more consistent and expected. The main differences are that `success`/`error` callbacks will now be run _after_ `response`/`responseError` interceptors complete (even if interceptors return a promise) and the correct callback will be called based on the result of the interceptor (e.g. if the `responseError` interceptor recovers from an error, the `success` callback will be called). See also angular#9334 (comment). This commit also replaces the use of `success`/`error` callbacks in the docs with using the returned promise. Fixes angular#6731 Fixes angular#9334 Closes angular#6865 BREAKING CHANGE: If you are not using `success` or `error` callbacks with `$resource`, your app should not be affected by this change. If you are using `success` or `error` callbacks (with or without response interceptors), one (subtle) difference is that throwing an error inside the callbacks will not propagate to the returned `$promise`. Therefore, you should try to use the promises whenever possible. E.g.: ```js // Avoid User.query(function onSuccess(users) { throw new Error(); }). $promise. catch(function onError() { /* Will not be called. */ }); // Prefer User.query(). $promise. then(function onSuccess(users) { throw new Error(); }). catch(function onError() { /* Will be called. */ }); ``` Finally, if you are using `success` or `error` callbacks with response interceptors, the callbacks will now always run _after_ the interceptors (and wait for them to resolve in case they return a promise). Previously, the `error` callback was called before the `responseError` interceptor and the `success` callback was synchronously called after the `response` interceptor. E.g.: ```js var User = $resource('/api/users/:id', {id: '@id'}, { get: { method: 'get', interceptor: { response: function(response) { console.log('responseInterceptor-1'); return $timeout(1000).then(function() { console.log('responseInterceptor-2'); return response.resource; }); }, responseError: function(response) { console.log('responseErrorInterceptor-1'); return $timeout(1000).then(function() { console.log('responseErrorInterceptor-2'); return $q.reject('Ooops!'); }); } } } }); var onSuccess = function(value) { console.log('successCallback', value); }; var onError = function(error) { console.log('errorCallback', error); }; // Assuming the following call is successful... User.get({id: 1}, onSuccess, onError); // Old behavior: // responseInterceptor-1 // successCallback, {/* Promise object */} // responseInterceptor-2 // New behavior: // responseInterceptor-1 // responseInterceptor-2 // successCallback, {/* User object */} // Assuming the following call returns an error... User.get({id: 2}, onSuccess, onError); // Old behavior: // errorCallback, {/* Response object */} // responseErrorInterceptor-1 // responseErrorInterceptor-2 // New behavior: // responseErrorInterceptor-1 // responseErrorInterceptor-2 // errorCallback, Ooops! ```

@id

Previously, action-specific interceptors and `success`/`error` callbacks were executed in inconsistent relative orders and in a way that did not meet the general expectation for interceptor behavior (e.g. ability to recover from errors, performing asynchronous operations, etc). This commit fixes the behavior to make it more consistent and expected. The main differences are that `success`/`error` callbacks will now be run _after_ `response`/`responseError` interceptors complete (even if interceptors return a promise) and the correct callback will be called based on the result of the interceptor (e.g. if the `responseError` interceptor recovers from an error, the `success` callback will be called). See also angular#9334 (comment). This commit also replaces the use of `success`/`error` callbacks in the docs with using the returned promise. Fixes angular#6731 Fixes angular#9334 Closes angular#6865 BREAKING CHANGE: If you are not using `success` or `error` callbacks with `$resource`, your app should not be affected by this change. If you are using `success` or `error` callbacks (with or without response interceptors), one (subtle) difference is that throwing an error inside the callbacks will not propagate to the returned `$promise`. Therefore, you should try to use the promises whenever possible. E.g.: ```js // Avoid User.query(function onSuccess(users) { throw new Error(); }). $promise. catch(function onError() { /* Will not be called. */ }); // Prefer User.query(). $promise. then(function onSuccess(users) { throw new Error(); }). catch(function onError() { /* Will be called. */ }); ``` Finally, if you are using `success` or `error` callbacks with response interceptors, the callbacks will now always run _after_ the interceptors (and wait for them to resolve in case they return a promise). Previously, the `error` callback was called before the `responseError` interceptor and the `success` callback was synchronously called after the `response` interceptor. E.g.: ```js var User = $resource('/api/users/:id', {id: '@id'}, { get: { method: 'get', interceptor: { response: function(response) { console.log('responseInterceptor-1'); return $timeout(1000).then(function() { console.log('responseInterceptor-2'); return response.resource; }); }, responseError: function(response) { console.log('responseErrorInterceptor-1'); return $timeout(1000).then(function() { console.log('responseErrorInterceptor-2'); return $q.reject('Ooops!'); }); } } } }); var onSuccess = function(value) { console.log('successCallback', value); }; var onError = function(error) { console.log('errorCallback', error); }; // Assuming the following call is successful... User.get({id: 1}, onSuccess, onError); // Old behavior: // responseInterceptor-1 // successCallback, {/* Promise object */} // responseInterceptor-2 // New behavior: // responseInterceptor-1 // responseInterceptor-2 // successCallback, {/* User object */} // Assuming the following call returns an error... User.get({id: 2}, onSuccess, onError); // Old behavior: // errorCallback, {/* Response object */} // responseErrorInterceptor-1 // responseErrorInterceptor-2 // New behavior: // responseErrorInterceptor-1 // responseErrorInterceptor-2 // errorCallback, Ooops! ```

@id

Previously, action-specific interceptors and `success`/`error` callbacks were executed in inconsistent relative orders and in a way that did not meet the general expectation for interceptor behavior (e.g. ability to recover from errors, performing asynchronous operations, etc). This commit fixes the behavior to make it more consistent and expected. The main differences are that `success`/`error` callbacks will now be run _after_ `response`/`responseError` interceptors complete (even if interceptors return a promise) and the correct callback will be called based on the result of the interceptor (e.g. if the `responseError` interceptor recovers from an error, the `success` callback will be called). See also angular#9334 (comment). This commit also replaces the use of `success`/`error` callbacks in the docs with using the returned promise. Fixes angular#6731 Fixes angular#9334 Closes angular#6865 BREAKING CHANGE: If you are not using `success` or `error` callbacks with `$resource`, your app should not be affected by this change. If you are using `success` or `error` callbacks (with or without response interceptors), one (subtle) difference is that throwing an error inside the callbacks will not propagate to the returned `$promise`. Therefore, you should try to use the promises whenever possible. E.g.: ```js // Avoid User.query(function onSuccess(users) { throw new Error(); }). $promise. catch(function onError() { /* Will not be called. */ }); // Prefer User.query(). $promise. then(function onSuccess(users) { throw new Error(); }). catch(function onError() { /* Will be called. */ }); ``` Finally, if you are using `success` or `error` callbacks with response interceptors, the callbacks will now always run _after_ the interceptors (and wait for them to resolve in case they return a promise). Previously, the `error` callback was called before the `responseError` interceptor and the `success` callback was synchronously called after the `response` interceptor. E.g.: ```js var User = $resource('/api/users/:id', {id: '@id'}, { get: { method: 'get', interceptor: { response: function(response) { console.log('responseInterceptor-1'); return $timeout(1000).then(function() { console.log('responseInterceptor-2'); return response.resource; }); }, responseError: function(response) { console.log('responseErrorInterceptor-1'); return $timeout(1000).then(function() { console.log('responseErrorInterceptor-2'); return $q.reject('Ooops!'); }); } } } }); var onSuccess = function(value) { console.log('successCallback', value); }; var onError = function(error) { console.log('errorCallback', error); }; // Assuming the following call is successful... User.get({id: 1}, onSuccess, onError); // Old behavior: // responseInterceptor-1 // successCallback, {/* Promise object */} // responseInterceptor-2 // New behavior: // responseInterceptor-1 // responseInterceptor-2 // successCallback, {/* User object */} // Assuming the following call returns an error... User.get({id: 2}, onSuccess, onError); // Old behavior: // errorCallback, {/* Response object */} // responseErrorInterceptor-1 // responseErrorInterceptor-2 // New behavior: // responseErrorInterceptor-1 // responseErrorInterceptor-2 // errorCallback, Ooops! ```

@id

Previously, action-specific interceptors and `success`/`error` callbacks were executed in inconsistent relative orders and in a way that did not meet the general expectation for interceptor behavior (e.g. ability to recover from errors, performing asynchronous operations, etc). This commit fixes the behavior to make it more consistent and expected. The main differences are that `success`/`error` callbacks will now be run _after_ `response`/`responseError` interceptors complete (even if interceptors return a promise) and the correct callback will be called based on the result of the interceptor (e.g. if the `responseError` interceptor recovers from an error, the `success` callback will be called). See also angular#9334 (comment). This commit also replaces the use of `success`/`error` callbacks in the docs with using the returned promise. Fixes angular#6731 Fixes angular#9334 Closes angular#6865 BREAKING CHANGE: If you are not using `success` or `error` callbacks with `$resource`, your app should not be affected by this change. If you are using `success` or `error` callbacks (with or without response interceptors), one (subtle) difference is that throwing an error inside the callbacks will not propagate to the returned `$promise`. Therefore, you should try to use the promises whenever possible. E.g.: ```js // Avoid User.query(function onSuccess(users) { throw new Error(); }). $promise. catch(function onError() { /* Will not be called. */ }); // Prefer User.query(). $promise. then(function onSuccess(users) { throw new Error(); }). catch(function onError() { /* Will be called. */ }); ``` Finally, if you are using `success` or `error` callbacks with response interceptors, the callbacks will now always run _after_ the interceptors (and wait for them to resolve in case they return a promise). Previously, the `error` callback was called before the `responseError` interceptor and the `success` callback was synchronously called after the `response` interceptor. E.g.: ```js var User = $resource('/api/users/:id', {id: '@id'}, { get: { method: 'get', interceptor: { response: function(response) { console.log('responseInterceptor-1'); return $timeout(1000).then(function() { console.log('responseInterceptor-2'); return response.resource; }); }, responseError: function(response) { console.log('responseErrorInterceptor-1'); return $timeout(1000).then(function() { console.log('responseErrorInterceptor-2'); return $q.reject('Ooops!'); }); } } } }); var onSuccess = function(value) { console.log('successCallback', value); }; var onError = function(error) { console.log('errorCallback', error); }; // Assuming the following call is successful... User.get({id: 1}, onSuccess, onError); // Old behavior: // responseInterceptor-1 // successCallback, {/* Promise object */} // responseInterceptor-2 // New behavior: // responseInterceptor-1 // responseInterceptor-2 // successCallback, {/* User object */} // Assuming the following call returns an error... User.get({id: 2}, onSuccess, onError); // Old behavior: // errorCallback, {/* Response object */} // responseErrorInterceptor-1 // responseErrorInterceptor-2 // New behavior: // responseErrorInterceptor-1 // responseErrorInterceptor-2 // errorCallback, Ooops! ```

jeffbcross added this to the Backlog milestone Oct 9, 2014

jeffbcross added needs: investigation component: ngResource severity: broken expected use labels Oct 9, 2014

gkalpak modified the milestones: 1.6.x, Backlog Feb 17, 2016

gkalpak mentioned this issue Feb 19, 2016

Add request and requestError interception in ngResource #13273

Closed

gkalpak mentioned this issue Mar 19, 2016

Issue with interceptors in $resource #11201

Closed

gkalpak mentioned this issue Jun 7, 2016

Allow transformRequest to modify headers #12095

Closed

gkalpak added the needs: breaking change label Sep 12, 2016

gkalpak mentioned this issue Sep 19, 2016

Composition of error handler + interceptors should be more flexible #4013

Closed

petebacondarwin modified the milestones: 1.6.x, 1.7.0 Oct 2, 2017

Narretz mentioned this issue Dec 8, 2017

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

Merged

gkalpak mentioned this issue Dec 11, 2017

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

Closed

petebacondarwin assigned gkalpak Jan 29, 2018

gkalpak mentioned this issue Feb 10, 2018

fix($resource): fix interceptors and success/error callbacks #16446

Closed

gkalpak closed this as completed in ea05857 Feb 20, 2018

dependencies bot mentioned this issue May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngResource interceptors called inconsistently #9334

ngResource interceptors called inconsistently #9334

jrm-airwatch commented Sep 29, 2014

nicole-ashley commented Feb 12, 2016

gkalpak commented Feb 17, 2016 •

edited

Loading

krotscheck commented Feb 22, 2016

gkalpak commented Feb 22, 2016 •

edited

Loading

krotscheck commented Feb 23, 2016

Narretz commented Nov 1, 2017

gkalpak commented Feb 10, 2018 •

edited

Loading

ngResource interceptors called inconsistently #9334

ngResource interceptors called inconsistently #9334

Comments

jrm-airwatch commented Sep 29, 2014

nicole-ashley commented Feb 12, 2016

gkalpak commented Feb 17, 2016 • edited Loading

krotscheck commented Feb 22, 2016

gkalpak commented Feb 22, 2016 • edited Loading

krotscheck commented Feb 23, 2016

Narretz commented Nov 1, 2017

gkalpak commented Feb 10, 2018 • edited Loading

gkalpak commented Feb 17, 2016 •

edited

Loading

gkalpak commented Feb 22, 2016 •

edited

Loading

gkalpak commented Feb 10, 2018 •

edited

Loading