Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignorable HttpClient Interceptors #20203

Closed
jcimoch opened this issue Nov 5, 2017 · 45 comments
Closed

Ignorable HttpClient Interceptors #20203

jcimoch opened this issue Nov 5, 2017 · 45 comments
Labels
area: common/http feature Issue that requests a new feature

Comments

@jcimoch
Copy link

jcimoch commented Nov 5, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

In current angular 5 implementation there is no elegant way to ignore particular http interceptor.

Expected behavior

It would be nice to be able to pass additional option as array of interceptors we whish to ignore for particual request.
It could be done like so

http.get('/someurl', {excludeInterceptors: [InterceptorClass1, InterceptorClass2]})

What is the motivation / use case for changing the behavior?

You may ask why do you want to ignore it in a first place? Well there are multiple usecases, for example your application may be composed of different feature modules which act like standalone applications. Each of these modules has own submodules. You may want to use authentication interceptor for all submodules but one http call may need different authorization header than others. To solve this we can create another submodule which won't have all interceptors included but it can end up in many unnecessary submodules. Another approach (which I'm using now) is to write interceptor in a special fashion where before applying transformation on http call I look for unique header with id values, each id represent an interceptor, if id match, then I simply return from the function and do not perform transformations at all. This solution works but requires additional overhead while implementing interceptor and setting special header when we want to ignore it.

I'm willing to help and contribute with such functionality on my own. I just need to be sure that I have team and community blessing to start work on that ;)

@kuncevic
Copy link
Contributor

kuncevic commented Nov 6, 2017

It is seams like the list of exclusions could be quite big so it might be look a bit weired to pass that as a parameter. Also you want sometimes not only exclude but include only. Custom decorator would look much nicer to approach that.

My current solution that just checking the request url inside my interceptors and based on that I am making a decision to apply interceptor or not but always wanted to review that part.

@jcimoch
Copy link
Author

jcimoch commented Nov 6, 2017

@kuncevic How do you see decorator usage here? We can decorate properties and functions/class declarations but I'm not sure about actual function call. Maybe both excludeInterceptors and includeInterceptors option would be sufficient?

@aitboudad
Copy link
Contributor

Duplicate of #18155

@seedy
Copy link

seedy commented Nov 8, 2017

Interceptors are designed to intercept any action with exactly no exception.
If you happen to need to exclude certain requests, then you shouldn't be using interceptors to implement your use case in my opinion.
There are other solutions to group specific http behaviours.

@ChristianUlbrich
Copy link

@jcimoch See #18155 (comment) for a way of achieving this. We have used this strategy and certain conventions to allow generic disabling of interceptors in our app on a per-request basis.

@seedy I disagree. It really depends on your use cases. A typical example, where you would have this behavior is a loadingNotification. Implemented using an interceptor it can be transparently activated / de-activated across your whole app. But there are times, where you might not want, the global loading notification (that might be blocking), but a local one - maybe non-blocking and thus disable the interceptor for a certain request.

Although I agree, that this should not be the focus of Angular. Allowing custom meta data to send along with each request is the broader solution, that can be used to achieve this. This seems to be planned -> #18155

@seedy
Copy link

seedy commented Nov 9, 2017

@ChristianUlbrich App wise, I'd definitely not give the responsibility to an interceptor in your use case.
Why? Because interceptors are aware of requests, headers and urls. They're designed to be very poor in responsibilities.
Plus, interceptors are a bad pattern because easy to forget and cut from the calling chain.

In your use case it should really be the caller who knows which call should or should not trigger notifications - for instance - and wrap your application calls within them.
-> use exception logic only if you have a single black sheep case in there.

Custom headers? I'd be against putting app logic in a custom header. You might someday end up having multiple particular cases, hence multiplying their number.

I understand what you guys are looking for, but I feel like it's a very complicated way to answer the problem.

Simple solution to me :

return this.http.get(url)
    .do(this.notify)

@alxhub alxhub added the feature Issue that requests a new feature label Nov 28, 2017
@alxhub
Copy link
Member

alxhub commented Nov 28, 2017

This isn't something we plan on implementing for interceptors currently (but feel free to try and convince me!)

@alxhub alxhub closed this as completed Nov 28, 2017
@jcimoch
Copy link
Author

jcimoch commented Nov 29, 2017

@alxhub I understand, however would be nice to see #18155 in 5.1 release so I could implement this feature on my own in project.

@jabbera
Copy link

jabbera commented Dec 28, 2017

I've got a great reason. We get JWT tokens from a kerberos enabled endpoint, we pass those tokens to every other API call our application makes. An interceptor makes great sense here, but we need to be able to exclude the call that gets the token, otherwise we end up in a circular dependency.

@jabbera
Copy link

jabbera commented Dec 28, 2017

I worked around this by using window.fetch for the only call that needed to be excluded.

@ChristianUlbrich
Copy link

@jabbera Just use the approach with custom headers. It isn't that complex. On a side note - from a security standpoint, you don't want your application to have access to the token. An app having access to auth tokens is prone to XSS attacks -> https://stormpath.com/blog/where-to-store-your-jwts-cookies-vs-html5-web-storage . window.fetch may need to be polyfilled.

@ChristianUlbrich
Copy link

@seedy I strongly disagree with interceptors are a bad pattern - they are very useful for cross-cutting aspects - namely "things that need to be done in your app" independently from other logic.

That is why they are used for thing like logging, auth or loading notifications. This is the reason why they are used by any 3rd party modules implementing a generic loading notification or auth logic without touching your existing code. Before interceptors you had to use a custom API / extended HttpModule, forcing you to change your existing code.

However this is OT, because this issue is not against Interceptors but for an option to ignore them through a specific API.

@jccazeaux
Copy link

Would it be possible to have interceptors declared as global (current behavior) and as local to an instance of HttpClient.

The idea would be for a project to provide several httpClient using a factory with a list of activated interceptors

For example i want to have a specific HttpClient instance for my Rest APIs and an other configured differently to call a partner's API. Different auth, different interceptor.

We would not have to enable or ignore interceptor, just use the configured HttpClient. We could add or remove interceptors for a specific subset of requests with no impact on business code, only on the provider and injection dependency.

For backward compatibility reasons, just keep current behavior of global interceptors.

@jcimoch
Copy link
Author

jcimoch commented Jan 8, 2018

@jccazeaux It is possible, every httpClient instance gets own interceptors, I'm using the solution which is using this fact. I also enabled disabling particular interceptor by sending unique header metadata. But is still solution far from perfect. I think it should be possible to send additional data to interceptor like it was possible in angular 1.x. Even if for some reason people consider this as anit pattern it is still only the option and life would be much easier for some corner cases. I refactored my code and deleted all custom httpServices which were inheriting the httpModule in favour of simple HttpClient, it just simplifies a lot of things and most of a time we were creating wrapper around HttpModule to provide some default headers with every request etc. Now when we have interceptors we don't have to do that. Yet still one solution of the problem we are talking about here is to extend HttpClient again with custom implementation, but seriously it isn't the best pattern. I like to work with http api directly, not with some wrappers written to achieve simple things.

@jccazeaux
Copy link

I tried extending HttpClient, but as you say it's not a good idea.

@jcimoch What do you mean by "every httpClient instance gets own interceptors". It's currently possible ?

I think it may be possible to configure interceptors without using headers or metadat which are really intrusive. All should be possible with provider configuration. Isn't it?

@mlc-mlapis
Copy link
Contributor

mlc-mlapis commented Jan 9, 2018

@jccazeaux ... when you import HttpClientModule to imports section of a module ... like lazy loaded one (instead of injection HttpClient through DI) then interceptors are reset ...

"every httpClient instance gets own interceptors"

@jccazeaux
Copy link

OK, but on a same module we cannot have several httpClients with different interceptors.

I don't know if i'm clear about the idea

@mlc-mlapis
Copy link
Contributor

@jccazeaux ... hmm, why do want that at all? The interceptors should be written to understand what should they do ...

@jccazeaux
Copy link

jccazeaux commented Jan 9, 2018

Because i don't want my interceptors to apply to all my requests.

I have a mid size web application which has several REST backends

  • My business API
  • A partner API
  • A technical API (see it as a node API called on localhost)

For my application to be maintainable I need 3 different HttpClients with 3 configurations

  • My business API must be csrf proof
  • Partner does not support it, can't activate otherwise CORS will fail because of the CSRF header
  • Technical API is very specific and need specific behaviors

Tomorrow, the partner will have CSRF. I will have to activate it with minimal code impact.

  • If i have a configured HttpClient specific for this partners's requests, i just add the interceptor on it. No impact in business code.
  • If each request added a "ignore header or metadata" i would have to modify each request. Do not forget one.

For sure, if you don't have mutliple backends, this would be totally useless. But for mid-size business apps this will be useful. In our previous (internal) framework, we add this and it revealed to be very useful. A complete framework as Angular should (IMHO) have this kind of stuff

NB : servers for each backend is not predictable and depends on user. So it cannot be used in interceptor to filter

@mlc-mlapis
Copy link
Contributor

@jccazeaux ... I understand your arguments ... actually using a lazy loaded module with importing the HttpClientModule means creating a new HttpClient instance + the possibility to provide its own set of interceptors on that module level. It would mean that you have to have a separate lazy loaded module (+ its services) for each extra API you need.

Although technically it will work smoothly I am not sure if it is the best solution. Maybe it would be better to consult this theme with the author of HttpClientModule ... @alxhub ... he is present very often on Gitter channel so it shouldn't be a problem to reach him.

@alxhub
Copy link
Member

alxhub commented Jan 9, 2018

@mlc-mlapis I'm working on a design to allow registration of interceptors in lazy loaded modules, but they would be installed globally so there'd still be a single set of interceptors in the application. I don't like the way it works currently, where you can accidentally bind HttpClient in a lazy module and get an instance that doesn't share the root interceptors (what if some of those interceptors are critical to security, for example).

@mlc-mlapis
Copy link
Contributor

mlc-mlapis commented Jan 9, 2018

@alxhub ... I understand your idea ... but still I am trying to imagine what would be possible to do to allow a creation of sets of interceptors ... identifying them and allowing to use optionally their ids to control which set should be called ... default would be still = calling all interceptors.

@jccazeaux
Copy link

@mlc-mlapis multiple sets of interceptors will do the job nicely too !

@alxhub do you think it is possible?

@jabbera
Copy link

jabbera commented Jan 10, 2018

@ChristianUlbrich thanks for the link about localstorage versus cookie for the token! I'll look into it.

@jabbera
Copy link

jabbera commented Jan 11, 2018

@ChristianUlbrich Can't use HttpClient from within an interceptor because it's a circular dependency. Still stuck with fetch unless you have another idea. I tried chaining the calls together but they fire out of order. (IE: the token is not in local storage before the original call fires)

const tokenEvent = next.handle(tokenRequest);

    return tokenEvent
    .flatMap ((x: HttpEvent<any>) => {
       // This is firing after the entry below.
        if (x.type !== 0) {
          let tokenData: TokenData;
          tokenData = <TokenData>x.body;
          localStorage.setItem(this.TOKEN_KEY, tokenData.token);
        }
        return Observable.of(x);
      }
    )
    .flatMap ((x: HttpEvent<any>) => {
        const token: string = this.getTokenFromStorage();

        return next.handle(request.clone(
          {
            setHeaders: {
              Authorization: `Bearer ${token}`
            },
            withCredentials: true
          }));
        }
    );

@OlegBabkin
Copy link

Is it needed to be complicated? Why not just skip urls? (I'm newbie in angular)
const excludedUrl = [
'/api/world/city/all',
'/api/world/country/all'
];

if (excludedUrl.some(x => x === req.url)) {
this.log('Excluded request: ' + req.url);
return next.handle(req);
}

@OlegBabkin
Copy link

jabbera Try to decomposite your service from where you try to get the token. I resolved that problem by removing all calls to HttpClient from service with token logic and new service deals only with tokens, without requests to http.

@jccazeaux
Copy link

jccazeaux commented Jan 11, 2018

@OlegBabkin This will work with our own interceptors, but not with community interceptors. And with many interceptors on many applications with many urls, it will soon become pretty complicated.

On backend I can have this configuration possibilities with Spring's RestTemplate which allows to instantiate as many as needed, each with a set of interceptors.

    @Bean
    public RestTemplate restTemplate() {
        RestTemplate restTemplate = new RestTemplate(clientHttpRequestFactory());
        restTemplate.setInterceptors(Collections.singletonList(new XUserAgentInterceptor()));
        return restTemplate;
    }

Is it possible to have something similar in Angular?

@skorunka
Copy link

I just encountered the same problem, in my interceptor I need to do a HTTP request to get a new token :(

@mlc-mlapis
Copy link
Contributor

mlc-mlapis commented Jan 13, 2018

@alxhub ... once more about sets of interceptors ...

I'm working on a design to allow registration of interceptors in lazy loaded modules, but they would be installed globally so there'd still be a single set of interceptors in the application.

The idea of a single set of interceptors in the application = all are installed globally (including those defined in any lazy loaded module) is generally fine and to allow easy filtering and using sub-sets of interceptors the following would be enough:

  • Ability to set some optional mark on an interceptor probably by id of a set when it is defined (maybe Array of id ... so more combinations would be easy available);
  • Later inject all registered interceptors with the HTTP_INTERCEPTORS and chain those from the last to the first including skipping those interceptors that are not mark by the requested id of the set (or not mark with any id of Array of id).
  • Ability to specify the id set of interceptors (or Array of id) on each request (get, post, ...) and pass it through ... to be able to get it when chaining interceptors.
  • If no id set of interceptors is used when calling a request then all interceptors should be used.

@jccazeaux
Copy link

@alxhub Just thought of something. If interceptors declared in lazy loaded modules become global when the module is loaded, it will lead to strange behaviour where ajax requets will not behave the same depending on the user navigation.

I'm still conviced Angular should allow to define sets of interceptors, @mlc-mlapis describes it well. Is it considered ?

@skorunka
Copy link

I have to agree with @jccazeaux, this is really bad idea <interceptors> all are installed globally (**including those defined in any lazy loaded module**). This might bring unpredictable behaviour on the whole application.

@TheSilvermind
Copy link

TheSilvermind commented Feb 26, 2018

For anyone else looking for a solution. I found the solution by navigating to the definition of the http handler that is used as an argument for the constructor of the HttpClient.
I used Visual Studio code to navigate to the definition of the HttpClient. From there I navigated to the definition of the HttpHandler and found this:

/**
 * Transforms an `HttpRequest` into a stream of `HttpEvent`s, one of which will likely be a
 * `HttpResponse`.
 *
 * `HttpHandler` is injectable. When injected, the handler instance dispatches requests to the
 * first interceptor in the chain, which dispatches to the second, etc, eventually reaching the
 * `HttpBackend`.
 *
 * In an `HttpInterceptor`, the `HttpHandler` parameter is the next interceptor in the chain.
 *
 * @stable
 */
export declare abstract class HttpHandler {
    abstract handle(req: HttpRequest<any>): Observable<HttpEvent<any>>;
}
/**
 * A final `HttpHandler` which will dispatch the request via browser HTTP APIs to a backend.
 *
 * Interceptors sit between the `HttpClient` interface and the `HttpBackend`.
 *
 * When injected, `HttpBackend` dispatches requests directly to the backend, without going
 * through the interceptor chain.
 *
 * @stable
 */
export declare abstract class HttpBackend implements HttpHandler {
    abstract handle(req: HttpRequest<any>): Observable<HttpEvent<any>>;
}

If you look at the commentary of the HttpBackend you might notice this very important message:

When injected, HttpBackend dispatches requests directly to the backend, without going through the interceptor chain.

So I got it working properly doing it like this:

import { Injectable } from '@angular/core';
import { HttpClient, HttpBackend } from '@angular/common/http';
import { Observable } from 'rxjs/Observable';
@Injectable()
export class AuthService {

    private http: HttpClient;

    constructor(
        handler: HttpBackend) {
        this.http = new HttpClient(handler);
    }

    requestToken(username: string, password: string): Observable<string> {
        const form = new FormData();
        form.append('username', username);
        form.append('password', password);
        return this.http.post<string>('/api/auth/requestToken', form);
    }
}

HttpBackend is injected by Angular and this does indeed not call the interceptor.

@ultrarunner
Copy link

Thank you so much for this last post. That is exactly the scenario I needed to exclude a global bearer token interceptor to be called on the end point requesting the token. Thanks.

@TheSilvermind
Copy link

I changed the logic a bit to this:

import { Injectable } from '@angular/core';
import { HttpClient, HttpBackend } from '@angular/common/http';

@Injectable()
export class HttpBackendClient extends HttpClient {
    constructor(handler: HttpBackend) {
        super(handler);
    }
}

And then added it to my module:

@NgModule({
    providers: [
        HttpBackendClient,
        AuthService
    ],
    imports: [HttpClientModule]
})
export class ServicesModule {}

And use it like this:

@Injectable()
export class AuthService {

    constructor(private http: HttpBackendClient) {
    }
}

@k11k2
Copy link

k11k2 commented Mar 7, 2018

any update regarding this ?

@aoakeson
Copy link

aoakeson commented Mar 9, 2018

@TheSilvermind solution works great, and in my opinion a very clean way to exclude an interceptor being called. Thanks!

@alexmgrant
Copy link

@TheSilvermind Thanks for this solution. I'm concerned though that this is not a sustainable long term solution as the api might change.

@TheSilvermind
Copy link

TheSilvermind commented Mar 18, 2018

@alexmgrant You're welcome and I agree with your concerns. I think this is not an issue until it really becomes one. Also I think it is better to use the second implementation I suggested, so you can swap out with a custom client of your own when the BackendHandler is no longer usable in this manner. The only reason I use this method is so I don't have to write my own handler :). If the time comes I will have to do some work to write my own, but until then I gladly rely on the built-in system.

@muhammadmukhtiar
Copy link

I have managed this in a simple way Here is my solution for this.

if (req.url.indexOf('http://') < 0 ) {
req = this.getModifiedReq(req);
}
If a request has full URL and you do not need to add interceptor then request not modified otherwise
simple do with a request that you need.

@alexander-heimbuch
Copy link

Hey Folks,

I'm facing an very similar issue, I need for different communication with backend services different interceptors. Previously the old Http client had a very streamlined request interface so that I was able to have a middleware like concept to create custom clients. Unfortunately the new request is more complex and as stated here the interface may change very easily.

After a good read of this article from @MaximusK I come up with this solution:

import { Injectable } from '@angular/core';
import {
  HttpClient,
  HttpBackend,
  HttpHandler,
  HttpInterceptor,
  HttpRequest,
  HttpEvent
} from '@angular/common/http';

import { Observable } from 'rxjs/Observable';

export class HttpInterceptorHandler implements HttpHandler {
  constructor(private next: HttpHandler, private interceptor: HttpInterceptor) {}

  handle(req: HttpRequest<any>): Observable<HttpEvent<any>> {
      // execute an interceptor and pass the reference to the next handler
      return this.interceptor.intercept(req, this.next);
  }
}

export class Interceptor1 implements HttpInterceptor {
  intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
      const modified = req.clone({setHeaders: {'Custom-Header-1': '2'}});
      return next.handle(modified);
  }
}

@Injectable()
export class CustomHttpClient extends HttpClient {
  constructor(handler: HttpBackend, interceptors: HttpInterceptor[]) {
    const chain = interceptors.reduceRight(
      (next, interceptor) => new HttpInterceptorHandler(next, interceptor), handler);

    super(chain);
  }
}

@Injectable()
export class MyCustomHttpClient extends CustomHttpClient {
  constructor(handler: HttpBackend) {
    const interceptors: HttpInterceptor[] = [new Interceptor1()];
    super(handler, interceptors);
  }
}

After registering the HttpClientModule and the MyCustomHttpClient provider you can simply inject it in your component/service.

...
import { HttpClientModule } from '@angular/common/http';
import { MyCustomHttpClient } from './custom-http-client.service';

@NgModule({
  imports: [
   ...
    HttpClientModule
  ],
  declarations: [...],
  providers: [MyCustomHttpClient]
})
export class TestModule { }
  constructor(private http: MyCustomHttpClient) {
    this.http.get('https://jsonplaceholder.typicode.com/users').subscribe(console.log);
  }

I don't know if the abstraction of the HttpClient is a good idea but maybe this helps someone in the same situation.

@atdiff
Copy link

atdiff commented Sep 24, 2018

@TheSilvermind - if I understand your solution of using HttpBackend, that skips all interceptors. Do you have a mechanism for still allowing some interceptors to be executed? In my scenario, I have an interceptor for caching (similar to the TOH example) and I want to have the ability to disable caching for some requests. But I can't turn off all interceptors since I have others that are needed.

@ChristianUlbrich
Copy link

As I've mentioned before the support to ignore Interceptors should not be in the core. We should "fight" for #18155, because allowing to pass misc data to interceptors allows for way more use cases than simply plainly ignoring certain interceptors.

If you must have this functionality it is trivial to add it right now. Either wrap httpClient with some sugar methods that allow ignoring certain Interceptors (per-request approach), or make your Interceptors aware of certain URLs they should be disabled for by themselves (whitelist approach). Or combine both approaches.

@atdiff
Copy link

atdiff commented Sep 24, 2018

@ChristianUlbrich I'm personally fine with #18155, it makes sense. Actually, I decided to implement one of the suggested solutions from there by @JWess - #18155 (comment). Not sure if it's the best to rely on the params but it looks like it should work. Open to ideas for alternatives till a complete core solution is made available in Angular.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: common/http feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests