| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| ); | ||
| } | ||
| return handlerFromParent; | ||
| return new HttpParentBackend(handlerFromParent!); |
There was a problem hiding this comment.
What would you think of defining a separate token instead of creating a separate HttpBackend instance ?
Sorry, something went wrong.
There was a problem hiding this comment.
Are you referring to using a DI token as a boolean flag here?
I considered it, but after reviewing the git history, it seems we might have a case like this #55652
A boolean token could become stale if another provider overrides HttpBackend, causing inherited root interceptors to be skipped incorrectly.
Or did you mean using a token for the parent handler and comparing it with the current HttpBackend instance?
In that case, I believe it would be something like the following (I haven't tested it), personally I would prefer the class that seemed simpler to me
const HTTP_PARENT_HANDLER = new InjectionToken<HttpHandler>('HTTP_PARENT_HANDLER');
// withRequestsMadeViaParent()
[
{
provide: HTTP_PARENT_HANDLER,
useFactory: () => inject(HttpHandler, {skipSelf: true}),
},
{
provide: HttpBackend,
useExisting: HTTP_PARENT_HANDLER,
},
];
// HttpInterceptorHandler
private readonly parentHandler = inject(HTTP_PARENT_HANDLER, {
self: true,
optional: true,
});
const rootInterceptorFns = this.injector.get(
HTTP_ROOT_INTERCEPTOR_FNS,
[],
this.backend === this.parentHandler ? {self: true} : undefined,
);
Sorry, something went wrong.
There was a problem hiding this comment.
Was more think of something like:
export const ɵHTTP_CLIENT_IS_DELEGATING = new InjectionToken<boolean>('ɵHTTP_CLIENT_IS_DELEGATING');
// packages/common/http/src/provider.ts
export function withRequestsMadeViaParent(): HttpFeature<HttpFeatureKind.RequestsMadeViaParent> {
return makeHttpFeature(HttpFeatureKind.RequestsMadeViaParent, [
{
provide: HttpBackend,
useFactory: () => {
const handlerFromParent = inject(HttpHandler, {skipSelf: true, optional: true});
// ... error handling ...
return handlerFromParent; // Return directly
},
},
{
provide: ɵHTTP_CLIENT_IS_DELEGATING,
useValue: true
}
]);
}
// packages/common/http/src/backend.ts (Inside HttpInterceptorHandler)
handle(initialRequest: HttpRequest<any>): Observable<HttpEvent<any>> {
if (this.chain === null) {
// Query with {self: true} to strictly check if the *current* client configuration is delegating
const isDelegating = this.injector.get(ɵHTTP_CLIENT_IS_DELEGATING, false, {self: true});
const rootInterceptorFns = this.injector.get(
HTTP_ROOT_INTERCEPTOR_FNS,
[],
isDelegating ? {self: true} : undefined,
);
// ...
Sorry, something went wrong.
There was a problem hiding this comment.
I tried this approach, but the token can become stale if another feature overrides HttpBackend.
Eg : with withRequestsMadeViaParent() and withXhr(), XHR becomes the effective backend while the token remains true, so inherited root interceptors are skipped. This test reproduces it.
it('should inherit root interceptors when withXhr overrides parent delegation', () => {
TestBed.configureTestingModule({
providers: [
provideHttpClient(),
{
provide: HTTP_ROOT_INTERCEPTOR_FNS,
useValue: makeLiteralTagInterceptorFn('root'),
multi: true,
},
],
});
const child = createEnvironmentInjector(
[
provideHttpClient(withRequestsMadeViaParent(), withXhr()),
{provide: XhrFactory, useClass: MockXhrFactory},
],
TestBed.inject(EnvironmentInjector),
);
try {
child.get(HttpClient).get('/test').subscribe();
const factory = child.get(XhrFactory) as MockXhrFactory;
expect(factory.mock.mockHeaders['X-Tag']).toBe('root');
factory.mock.mockFlush(200, 'OK', '{}');
} finally {
child.destroy();
}
});
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe the issue here is that we allow this contradictory usage. Both withRequestsMadeViaParent() and withXhr() register a different backend.
Sorry, something went wrong.
There was a problem hiding this comment.
I thought that could be the case too; in fact, I don't know if it's intentional or not.
But I think a feature on some route might want to use withXhr while maintaining interceptors for the upload report, which isn't supported in fetch.
Sorry, something went wrong.
There was a problem hiding this comment.
Both provide HttpBackend only the latest registred will effectively be injected.
Sorry, something went wrong.
There was a problem hiding this comment.
My bad , you're right, we'll most likely have to throw an exception in this case.
Sorry, something went wrong.
There was a problem hiding this comment.
Updated, we will now throw an exception at development time, although I still think we should allow a custom HttpBackend for example in provideHttpClientTesting
Sorry, something went wrong.
|
Found a regression/change: import { TestBed } from '@angular/core/testing';
import { EnvironmentInjector, createEnvironmentInjector } from '@angular/core';
import { HttpClient, provideHttpClient, withRequestsMadeViaParent } from '@angular/common/http';
import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing';
import { HTTP_ROOT_INTERCEPTOR_FNS } from '@angular/common/http/src/interceptor';
describe('Delegating HttpClient root interceptors', () => {
it('should only run root interceptors once in a delegated chain', () => {
let rootInterceptorRunCount = 0;
TestBed.configureTestingModule({
providers: [
provideHttpClient(),
provideHttpClientTesting(),
{
provide: HTTP_ROOT_INTERCEPTOR_FNS,
multi: true,
useValue: (req, next) => {
rootInterceptorRunCount++;
return next(req);
}
}
],
});
const childInjector = createEnvironmentInjector(
[provideHttpClient(withRequestsMadeViaParent())],
TestBed.inject(EnvironmentInjector)
);
const client = childInjector.get(HttpClient);
const httpMock = TestBed.inject(HttpTestingController);
client.get('/api/data').subscribe();
httpMock.expectOne('/api/data').flush({});
// ❌ FAILS BEFORE PR: The count would be 2 because the child executed the parent's
// root interceptors, and then the parent executed them again.
expect(rootInterceptorRunCount).toBe(1);
});
}); |
Sorry, something went wrong.
In fact, I believe it was a bug and not something expected to happen twice (or so I think), since that's why the problem described in #69777 occurs. I doubt we would expect or that it would make any sense to expect an interceptor to execute twice (when you create an interceptor in an app, this doesn't happen). |
Sorry, something went wrong.
There was a problem hiding this comment.
AGENT: Great PR! The tests are very thorough and it effectively fixes the duplicate root interceptor execution. I have left a few inline suggestions for your consideration to simplify the implementation by removing the internal injection token.
Sorry, something went wrong.
There was a problem hiding this comment.
AGENT: Thank you for working on this fix for root interceptors and transfer cache! I have left a few inline feedback comments regarding dev mode bundle optimizations, runtime null safety, and test coverage for your consideration.
Sorry, something went wrong.
Represent withRequestsMadeViaParent() with an internal delegating backend so the interceptor handler can distinguish delegated clients from independent child configurations. Delegated clients leave inherited root interceptors to the parent chain. This prevents duplicate execution and lets HttpTransferCache evaluate authentication and cache filters after parent request interceptors, while independent child clients continue to inherit framework root interceptors. Add coverage for independent root inheritance, request and response ordering, authenticated requests, cache filters, and public cache hits. Fixes angular#69777
| * "bubble up" until either reaching the root level or an `HttpClient` which was not configured with | ||
| * this option. | ||
| * | ||
| * This feature is incompatible with the `withFetch` and `withXhr` features. |
There was a problem hiding this comment.
We should probably rephrase that a bit. It works will with both backend if they are in the parent.
| * This feature is incompatible with the `withFetch` and `withXhr` features. | |
| * This feature is incompatible with the `withFetch` and `withXhr` features in the `provideHttpClient()`. |
Or something else if you can come up with a better phrasing :)
Sorry, something went wrong.
There was a problem hiding this comment.
Thinking about it (or asking GPT 5.6 Sol Ultra Max 😄), it gave me this option, wdyt?
* This feature cannot be combined with `withFetch` or `withXhr` in the same `provideHttpClient()` call.
Sorry, something went wrong.
|
This PR was merged into the repository. The changes were merged into the following branches: |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Represent withRequestsMadeViaParent() with an internal delegating backend so the interceptor handler can distinguish delegated clients from independent child configurations.
Delegated clients leave inherited root interceptors to the parent chain. This prevents duplicate execution and lets HttpTransferCache evaluate authentication and cache filters after parent request interceptors, while independent child clients continue to inherit framework root interceptors.
Add coverage for independent root inheritance, request and response ordering, authenticated requests, cache filters, and public cache hits.
Fixes #69777
This seems similar to GHSA-q6f4-qqrg-jv6x ( In fact, no filter or exclusion is currently applied )