feat(common): http client request metadata for use in interceptors #25751

FDIM · 2018-08-30T22:26:09Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently there is no way to configure interceptors in http client on per request basis and people (including myself) are abusing headers to disable / customize the behavior of certain interceptor (e.g. logging / authentication).

Issue Number: Fixes #18155

What is the new behavior?

This PR introduces a context option to any request that allows you to supply arbitrary data in a type safe way:

export const CacheInterceptorOptions = new HttpContextToken<{cache: boolean}>({ cache: false });
// compiles
this.http.get('/path', { context: new HttpContext().set(CacheInterceptorOptions, { cache: true }) })
this.http.get('/path', { context: new HttpContext().set(CacheInterceptorOptions, { cache: false }) })

// error
this.http.get('/path', { context: new HttpContext().set(CacheInterceptorOptions, { 
  foo: 123 // Argument of type '{ foo: number; }' is not assignable to parameter of type '{ cache: boolean; }'.
})})

There are also get, delete, and keys methods on HttpContext. set and remove can be chained to simplify usage.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

~~There is another PR open since last year but have seen no activity since January - probably due to other comments on the issue and lost type safety. ~~

~~Also, HttpInterceptorMetadata class is very generic (except for the name), maybe something like that could belong in core (or common?) package if there is a need?~~

EDIT: Description has been updated to match latest implementation

FDIM · 2019-02-02T19:58:03Z

I am still alive and interested in updating this PR, so let me know if I should do anything

un33k · 2019-04-28T13:55:57Z

@FDIM You may want to resolve the conflict if you are still around & interested.

FDIM · 2019-05-21T21:52:19Z

PR is ready again, failure in circle ci seems to be unrelated to my change

agroves333 · 2019-06-16T12:45:46Z

Hey guys. Where are we at with this? Any idea on why CI is failing?

un33k · 2019-06-16T13:18:38Z

FYI - I asked the Angular team in ngConf 2019 about this feature. The answer was, that this was one of the most requested feature and they have to wait for things to settle before adding this. So, it might be very unlikely for them to accepting this PR as-is.

BobDankert · 2019-06-17T14:01:45Z

@un33k - what does "wait for things to settle" refer to? Just curious as I'm not quite sure what that means.

mlc-mlapis · 2019-06-17T14:14:37Z

@BobDankert ... I interpret it as We are not 100% sure if this is the really right way how to it. So wait and discuss it for some time. It would be better than to introduce something and depreciate it 6 months later (with some breaking changes) because we find out something better.

un33k · 2019-06-18T11:29:35Z

@BobDankert Check out the closing remarks at ng-conf 2019 - Timeline: 21:04.

DutchKevv · 2020-02-21T15:55:02Z

Any update?

Can I help somehow .. ?

CesarD · 2020-07-23T10:44:18Z

Why is this PR not yet approved?!
Are you owners just stalling or wth?!

alxhub · 2020-07-23T17:17:11Z

@un33k oh that was you at ng-conf, awesome!

To expand on my answer there a little bit:

One of the big design questions here is whether metadata should be mutable or immutable. There are pros and cons to both:

immutable metadata avoids surprises when requests are retried via an Observable pipeline, where an interceptor will see metadata for a previously "failed" request. This can be especially confusing when a downstream interceptor added the metadata, but other aspects of the requests (e.g. the headers) do not reflect the same mutations.
mutable metadata would allow an interceptor to do exactly this on purpose (an obvious example here is a counter of retries on a request)

My instinct there is that immutable is the way to go. It's how the rest of the API already works, and you can always add an object to the metadata early and make use of its interior mutability if needed.

So assuming that's the direction we decide to go, the question becomes how to implement it. We have two immutable "map" types in HttpClient: HttpParams and HttpHeaders. Each has custom behavior (lazy parsing, encoding, etc) but similar functionality around append/set/delete, as well as get. Currently these are separate implementations.

If we introduce a third immutable container for metadata, it makes sense to refactor HttpParams and HttpHeaders to share a common immutable "core" that could be used to build the new container too.

BobDankert · 2020-07-23T17:20:42Z

Thank you @alxhub for the response. Immutable metadata would be sufficient for our projects use cases.

mlc-mlapis · 2020-07-23T17:29:03Z

@alxhub

If we introduce a third immutable container for metadata, it makes sense to refactor HttpParams and HttpHeaders to share a common immutable "core" that could be used to build the new container too.

Yep, it seems like a logical and necessary step. But from the timeline point of view, do you expect some extra phases to do that? Can they significantly delay the realization?

un33k · 2020-07-25T02:24:01Z

@alxhub Yep that was me asking the question and hopefully we can once again attend ng-config in person soon.

As far the immutability goes, I think it can be just like the HttpParams and/or HttpHeaders.
The only diff would be to have a hidden interceptor to peel the data off so it never gets on the wire.

Right now, I piggyback some metadata onto HttpHeaders and have a no-op interceptor placed at the end which is responsible to peel the data off. It works well, but it would be great to have a native solution.

CesarD · 2020-07-25T16:39:46Z

@alxhub

If we introduce a third immutable container for metadata, it makes sense to refactor HttpParams and HttpHeaders to share a common immutable "core" that could be used to build the new container too.

Yep, it seems like a logical and necessary step. But from the timeline point of view, do you expect some extra phases to do that? Can they significantly delay the realization?

There's been already 2 years of this PR trying to go into release... Perhaps a quick and separate implementation to make it fully available for us and leave the refactor with all the nice details for later? Just for the sake of availability and readiness.

Right now, I piggyback some metadata onto HttpHeaders

I do the exact same thing on my end. Have to add some extra header so I can use as flag for some functionality I need and then remove it so it doesn't get sent with the actual backend request.

david0603 · 2020-11-17T20:18:45Z

bttt

un33k · 2020-11-20T15:36:48Z

@FDIM be nice to have this ticket closed, so we can open a new one. Otherwise the new one is going to be redirected to this one and we end up in an infinite no-op loop.

FDIM · 2020-11-20T16:20:11Z

@un33k I'm still interested to finish this, but I have no idea who can make the decision. So this is stuck until someone from core team gets back.

CodeTroopers · 2020-12-17T15:34:54Z

@FDIM Thanks for your cool PR. I'm also interested by this feature.

Just a details, I will rename meta and HttpInterceptorMetadata by context and HttpContext since this object could be interpreted by something else than an interceptor. I think HttpContext is a term used for this in the ASP.Net Core framework

alxhub · 2021-01-29T21:49:14Z

@FDIM thanks for your continued work on this feature. I think there's still some work to be done on the design, but this is definitely functionality that's been missing from HttpClient for far too long. Let's iterate on this PR and fix that.

After reviewing the current version and considering the API at a high level, I think a very simple design is preferable. @CodeTroopers suggestion of the HttpContext terminology really helps frame the idea.

So here's what I'm thinking:

Every HttpRequest, when created, has an HttpContext, which is effectively a metadata map. If you don't provide one, an empty default context is created internally.
This context is mutable and stays with the request for as long as the request is live, even across multiple retries.

This second point is a departure from my previous intuition, which is that request metadata should use the same immutable API as the rest of HttpRequest. Thinking about the metadata as a "context" changed my mind here. The mental model would be that every request has one, and only one, context, which persists across the entire lifetime of the request, including any retry operations and any response. Through this context, the operation of interceptors can be coordinated, even across request retries.

The amount of code to implement this is surprisingly small. Here's my concept of what HttpContext might look like:

export class HttpContextKey<T> {
    constructor(readonly defaultValue: T) {}
}

export class HttpContext {
    private map = new Map<HttpContextKey<unknown>, unknown>();

    read<K>(key: HttpContextKey<K>): K {
        return this.map.has(key) ? this.map.get(key) as K : key.defaultValue;
    }

    write<K>(key: HttpContextKey<K>, value: K): void {
        this.map.set(key, value);
    }

    keys(): IterableIterator<HttpContextKey<unknown>> {
        return this.map.keys();
    }
}

That's it. Each new HttpRequest would then be either initialized with a context or have one created for it, and each HttpResponse would carry through the context of its HttpRequest. Easy, and very little new code.

Note that using a Map keyed on the instances of HttpContextKey means that there's perfect type safety here - there's no chance for two keys to collide by using the same string id. Each key also carries a default value, which allows for unambiguous reads as well as enables convenient type inference when creating a key.

Beyond the implementation above, we'll need:

support for the context in the HttpClient testing framework
documentation and examples in the HttpClient docs

mary-poppins · 2021-03-09T20:22:26Z

mary-poppins · 2021-03-09T20:29:55Z

You can preview 557dae0 at https://pr25751-557dae0.ngbuilds.io/.

AndrewKushnir

Reviewed-for: public-api
Reviewed-for: size-tracking

packages/common/http/src/request.ts

jessicajaniuk

LGTM 🍪

reviewed-for: size-tracking

mary-poppins · 2021-03-12T19:30:38Z

You can preview 4cba236 at https://pr25751-4cba236.ngbuilds.io/.

goldens/size-tracking/aio-payloads.json

A long-requested feature for HttpClient is the ability to store and retrieve custom metadata for requests, especially in interceptors. This commit implements this functionality via a new context object for requests. Each outgoing HttpRequest now has an associated "context", an instance of the HttpContext class. An HttpContext can be provided when making a request, or if not then an empty context is created for the new request. This context shares its lifecycle with the entire request, even across operations that change the identity of the HttpRequest instance such as RxJS retries. The HttpContext functions as an expando. Users can create typed tokens as instances of HttpContextToken, and read/write a value for the key from any HttpContext object. This commit implements the HttpContext functionality. A followup commit will add angular.io documentation.

mary-poppins · 2021-03-12T22:59:04Z

You can preview 7ec7207 at https://pr25751-7ec7207.ngbuilds.io/.

jessicajaniuk · 2021-03-15T17:34:20Z

Caretaker Note: This has been merged.

un33k · 2021-03-15T18:20:53Z

@FDIM You finally did it! Thank you very much for your perseverance, and ultimately this contribution.

FDIM · 2021-03-15T21:54:06Z

I'm so happy that this finally made it into the framework and looking forward to cleaning up misused headers!

jcimoch · 2021-03-19T22:28:59Z

so many years but it's finally there!!!!
thanks @FDIM

nasreddineskandrani · 2021-03-20T18:39:53Z

great work! this is big :)

angular-automatic-lock-bot · 2021-04-20T16:08:02Z

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.}

googlebot added the cla: yes label Aug 30, 2018

FDIM force-pushed the feature/http-client-metadata branch 2 times, most recently from a439a18 to 9688390 Compare September 4, 2018 16:30

lazarljubenovic mentioned this pull request Sep 20, 2018

Allow passing misc data to http client options #18155

Closed

jasonaden added the area: common/http label Jan 25, 2019

ngbot bot added this to the needsTriage milestone Jan 25, 2019

jasonaden requested a review from alxhub January 25, 2019 19:29

FDIM force-pushed the feature/http-client-metadata branch from 9688390 to 59ae2ee Compare May 21, 2019 19:19

FDIM requested a review from a team as a code owner May 21, 2019 19:19

FDIM force-pushed the feature/http-client-metadata branch from 59ae2ee to 053e2f0 Compare May 21, 2019 19:31

ngbot bot modified the milestones: needsTriage, Backlog Mar 9, 2021

AndrewKushnir approved these changes Mar 10, 2021

View reviewed changes

pullapprove bot requested a review from jelbourn March 10, 2021 01:41

un33k reviewed Mar 11, 2021

View reviewed changes

packages/common/http/src/request.ts Show resolved Hide resolved

packages/common/http/src/request.ts Show resolved Hide resolved

packages/common/http/src/request.ts Show resolved Hide resolved

jessicajaniuk approved these changes Mar 11, 2021

View reviewed changes

alxhub force-pushed the feature/http-client-metadata branch from 557dae0 to 4cba236 Compare March 12, 2021 19:22

alxhub added the action: merge The PR is ready for merge by the caretaker label Mar 12, 2021

AndrewKushnir reviewed Mar 12, 2021

View reviewed changes

goldens/size-tracking/aio-payloads.json Outdated Show resolved Hide resolved

alxhub force-pushed the feature/http-client-metadata branch from 4cba236 to 7ec7207 Compare March 12, 2021 22:47

AndrewKushnir removed request for IgorMinar, atscott and jelbourn March 12, 2021 22:49

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Mar 13, 2021

jessicajaniuk closed this in 1644d64 Mar 15, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): http client request metadata for use in interceptors #25751

feat(common): http client request metadata for use in interceptors #25751

FDIM commented Aug 30, 2018 •

edited

FDIM commented Feb 2, 2019 •

edited

un33k commented Apr 28, 2019

FDIM commented May 21, 2019

agroves333 commented Jun 16, 2019

un33k commented Jun 16, 2019 •

edited

BobDankert commented Jun 17, 2019

mlc-mlapis commented Jun 17, 2019 •

edited

un33k commented Jun 18, 2019 •

edited

DutchKevv commented Feb 21, 2020

CesarD commented Jul 23, 2020

alxhub commented Jul 23, 2020

BobDankert commented Jul 23, 2020

mlc-mlapis commented Jul 23, 2020

un33k commented Jul 25, 2020 •

edited

CesarD commented Jul 25, 2020

david0603 commented Nov 17, 2020

un33k commented Nov 20, 2020 •

edited

FDIM commented Nov 20, 2020

CodeTroopers commented Dec 17, 2020

alxhub commented Jan 29, 2021

mary-poppins commented Mar 9, 2021

mary-poppins commented Mar 9, 2021

AndrewKushnir left a comment

jessicajaniuk left a comment

mary-poppins commented Mar 12, 2021

mary-poppins commented Mar 12, 2021

jessicajaniuk commented Mar 15, 2021

un33k commented Mar 15, 2021 •

edited

FDIM commented Mar 15, 2021

jcimoch commented Mar 19, 2021

nasreddineskandrani commented Mar 20, 2021

angular-automatic-lock-bot bot commented Apr 20, 2021

feat(common): http client request metadata for use in interceptors #25751

feat(common): http client request metadata for use in interceptors #25751

Conversation

FDIM commented Aug 30, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

FDIM commented Feb 2, 2019 • edited

un33k commented Apr 28, 2019

FDIM commented May 21, 2019

agroves333 commented Jun 16, 2019

un33k commented Jun 16, 2019 • edited

BobDankert commented Jun 17, 2019

mlc-mlapis commented Jun 17, 2019 • edited

un33k commented Jun 18, 2019 • edited

DutchKevv commented Feb 21, 2020

CesarD commented Jul 23, 2020

alxhub commented Jul 23, 2020

BobDankert commented Jul 23, 2020

mlc-mlapis commented Jul 23, 2020

un33k commented Jul 25, 2020 • edited

CesarD commented Jul 25, 2020

david0603 commented Nov 17, 2020

un33k commented Nov 20, 2020 • edited

FDIM commented Nov 20, 2020

CodeTroopers commented Dec 17, 2020

alxhub commented Jan 29, 2021

mary-poppins commented Mar 9, 2021

mary-poppins commented Mar 9, 2021

AndrewKushnir left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

mary-poppins commented Mar 12, 2021

mary-poppins commented Mar 12, 2021

jessicajaniuk commented Mar 15, 2021

un33k commented Mar 15, 2021 • edited

FDIM commented Mar 15, 2021

jcimoch commented Mar 19, 2021

nasreddineskandrani commented Mar 20, 2021

angular-automatic-lock-bot bot commented Apr 20, 2021

FDIM commented Aug 30, 2018 •

edited

FDIM commented Feb 2, 2019 •

edited

un33k commented Jun 16, 2019 •

edited

mlc-mlapis commented Jun 17, 2019 •

edited

un33k commented Jun 18, 2019 •

edited

un33k commented Jul 25, 2020 •

edited

un33k commented Nov 20, 2020 •

edited

un33k commented Mar 15, 2021 •

edited