Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
KamilSzot opened this issue Mar 24, 2015 · 9 comments

Comments

@KamilSzot
Copy link

Here you can see that behaviour of interceptor added to $httpProvider.interceptors is completely different from interceptor declared in resource methods: http://jsfiddle.net/Lzgts/269/

I think we need to update the documentation to reflect this changing the behaviour now might break apps of people who adapted to this behaviour.

That's how I found out about this bug. Behaviour of my app was different when I stopped needing the resource interceptor. After removing it app stopped working properly, but leaving it doing nothing as shown at https://docs.angularjs.org/api/ng/service/$http in section on Interceptors kept the app working properly.

@petebacondarwin
Copy link
Contributor

That does seem very confusing. I think we either need to make some changes to $resource and how it uses interceptors/promises or we should indeed document this very clearly. Right now I am leaning toward the first option.

@gkalpak
Copy link
Member

gkalpak commented Mar 26, 2015

Adding a little context:

It doesn't really have to do with the interceptors, but with the internal workings of $resource.

For "its own reasons", $resource uses a default response interceptor that returns response.resource. If you overwrite that (using a custom action interceptor), you have to take care of this yourself (or live with the consequences 😃).

This has been discussed in more detail in #11201 (expecially #11201 (comment) and #11201 (comment)).

@petebacondarwin
Copy link
Contributor

@gkalpak thanks for linking to the previous discussion on this. The fact that this has come up again and, IMO is not very intuitive, I wonder if we ought to make a change to $resource. The comment that chimes with me is (#11201 (comment)):

This default interceptor is used only if there is no response interceptor specified by the user.

  1. Do we really need this default interceptor to return the resource instance rather than the response?
  2. Could we always run this default interceptor, either
    a) before any custom interceptor - meaning that the custom interceptor would receive the resource instance rather than the response
    b) after any custom interceptor - meaning that an interceptor would have to make changes to response.resource to have an impact outside $resource.

2 seems to be fairly arbitrary and in either case would break some uses irreparably.

1 seems to be a breaking change that doesn't really add any value except remove unexpected behaviour; but I would definitely consider it...

@gkalpak
Copy link
Member

gkalpak commented Mar 26, 2015

@petebacondarwin, I too agree that we could do better inside $resource to avoid surprizes.

Your 1st suggestion will be a breaking change indeed, but I wouldn't say it will remove unexpected behaviour. I see it more like making the bahaviour consistently unexpected (with both default and custom interceptor) 😃

2a/2b would probably be better than what is happening today, but still unexpected imo and also breaking changes, so less than ideal 😞


Looking further into it, it seems that $resource's "interceptor" is not an actual $http interceptor in the sense that it is not executed when the $http interceptors do (but in a later phase).

So, here are the several phases (in an (over)simplified version) between a call of a $resource (class or instance) method and the resolving of the the returned instance's $promise:

  1. $http gets called (this includes any "normal" $http interceptors). Interceptors executed at this phase are passed the "raw" response as an argument (not the resource instance).
  2. The properties on response.data are copied over to response.resource.
  3. $resource's interceptor is executed (either the user-defined one or the default (which returns response.resource)). The returned instance's $promise is resolved with the value returned by the "interceptor" executed at this phase.

I don't know how we can make things right, without a breaking change :(

Ideally, I would expect that $resource's interceptor property to define a normal $http interceptor (executed at phase 1). Any changes done to response.data would then be copied over to response.resource at phase 2. Phase 3 should always be executed.

Even if we choose to always execute the default interceptor and keep everything else unchanged, it is still kind of unexpected that a user-defined interceptor will be passed a response object, but any modification in response.data won't be reflected in the finally returned instance (because copying has already taken place in phase 2). On the other hand, if someone understands that they need to manipulate response.resource directly, they probably have enough understanding to know they need to return it too.
So, I am not sure we are actually gaining much.


Any ideas ?

@gkalpak
Copy link
Member

gkalpak commented Mar 26, 2015

One idea:

Since it is unlikely that the user wants to bypass the default $http interceptor from $resource (since JSON parsing seems pretty relevant here for example), we could add a new configuration option (e.g. additionalHttpInterceptor*), which will execute at phase 1 after the default $http interceptors.
Phase 3 will be executed as before.

*: WARNING - I am not good with names :)

Pros:

  1. The user gets to work with a "real" interceptor (the same way they are used to from $http).
  2. The user will be able to manipulate response and the modifications will be automatically reflected in the returned resource instance (since the properties of response.data will be copied during phase 2).
  3. The user doesn't have to worry about returning response.resource manually, as this will be taken care of during phase 3.
  4. No breaking change.

Cons:

  1. Having 2 similarly named configuration options (i.e. interceptor vs additionalHttpInterceptor) that behave quite differently, will be confusing.
    (Maybe we could deprecate interceptor and remove it in a future version.)
    Detailed documentation will be needed.
  2. It might be straight-forward, but I haven't given any thought to how errors should be handled/propagated.

@KamilSzot
Copy link
Author

For me it's not the problem that 'interceptor' in $resource replaces the default one, or even not so much that it behaves differently.

For me the problem is that it's named exactly the same as interceptors that you can add to $http and especially that the documentation of $resource here https://docs.angularjs.org/api/ngResource/service/$resource doesn't describe its specific behaviour and even links directly to page that describes $http interceptors: https://docs.angularjs.org/api/ng/service/$http

I updated the example here: http://jsfiddle.net/Lzgts/277/ to demonstrate how should you write $resource 'interceptor' to achieve same behaviour as from $http interceptor.

@BenWoodford
Copy link

I know I'm bumping an old discussion but has anything ever come of this? I'm finding myself in need of modifying the response to a $resource request but unable to do so because nothing fires before it turns everything into Resource objects. One would generally expect interceptors to fire before that (i.e. moving the .then() that fires the interceptor to be first in line) and transforms could then be fired afterwards.

@petebacondarwin
Copy link
Contributor

@BenWoodford There is work going on to fix this in #13273 - please take a look at the implementation there and comment. It is scheduled to be incorporated after we get to 1.6.0

@gkalpak
Copy link
Member

gkalpak commented Dec 11, 2017

WRT the original report, I would say that this partially works as intended now (especially after 823c7ed). Thre are still things to improve with $resource interceptors; let's track those in #9334.

@gkalpak gkalpak closed this as completed Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants