From 6731d37546ea7130a2bc10b55fb6f5c1aa291333 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 16 Mar 2020 13:31:06 -0700 Subject: [PATCH] api(network): replace redirectChain with redirectedFrom/redirectedTo (#1401) --- docs/api.md | 35 ++++++++++++++-------------- src/chromium/crNetworkManager.ts | 11 ++++----- src/firefox/ffNetworkManager.ts | 20 +++++++--------- src/frames.ts | 6 ++--- src/network.ts | 25 +++++++++++++------- src/webkit/wkInterceptableRequest.ts | 7 +++--- src/webkit/wkPage.ts | 7 +++--- test/browsercontext.spec.js | 2 ++ test/interception.spec.js | 35 ++++++++++++++++------------ test/network.spec.js | 15 ++++++------ 10 files changed, 85 insertions(+), 78 deletions(-) diff --git a/docs/api.md b/docs/api.md index 3f790857a2..2e1255a95a 100644 --- a/docs/api.md +++ b/docs/api.md @@ -3138,7 +3138,8 @@ If request gets a 'redirect' response, the request is successfully finished with - [request.isNavigationRequest()](#requestisnavigationrequest) - [request.method()](#requestmethod) - [request.postData()](#requestpostdata) -- [request.redirectChain()](#requestredirectchain) +- [request.redirectedFrom()](#requestredirectedfrom) +- [request.redirectedTo()](#requestredirectedto) - [request.resourceType()](#requestresourcetype) - [request.response()](#requestresponse) - [request.url()](#requesturl) @@ -3176,31 +3177,29 @@ Whether this request is driving frame's navigation. #### request.postData() - returns: Request's post body, if any. -#### request.redirectChain() -- returns: <[Array]<[Request]>> +#### request.redirectedFrom() +- returns: Request that was redirected by the server to this one, if any. -A `redirectChain` is a chain of requests initiated to fetch a resource. -- If there are no redirects and the request was successful, the chain will be empty. -- If a server responds with at least a single redirect, then the chain will -contain all the requests that were redirected. - -`redirectChain` is shared between all the requests of the same chain. - -For example, if the website `http://example.com` has a single redirect to -`https://example.com`, then the chain will contain one request: +When the server responds with a redirect, Playwright creates a new [Request] object. The two requests are connected by `redirectedFrom()` and `redirectedTo()` methods. When multiple server redirects has happened, it is possible to construct the whole redirect chain by repeatedly calling `redirectedFrom()`. +For example, if the website `http://example.com` redirects to `https://example.com`: ```js const response = await page.goto('http://example.com'); -const chain = response.request().redirectChain(); -console.log(chain.length); // 1 -console.log(chain[0].url()); // 'http://example.com' +console.log(response.request().redirectedFrom().url()); // 'http://example.com' ``` -If the website `https://google.com` has no redirects, then the chain will be empty: +If the website `https://google.com` has no redirects: ```js const response = await page.goto('https://google.com'); -const chain = response.request().redirectChain(); -console.log(chain.length); // 0 +console.log(response.request().redirectedFrom()); // null +``` + +#### request.redirectedTo() +- returns: New request issued by the browser iff the server responded with redirect. + +This method is the opposite of [request.redirectedFrom()](#requestredirectedfrom): +```js +console.log(request.redirectedFrom().redirectedTo() === request); // true ``` #### request.resourceType() diff --git a/src/chromium/crNetworkManager.ts b/src/chromium/crNetworkManager.ts index 25de43f687..b73893ab2a 100644 --- a/src/chromium/crNetworkManager.ts +++ b/src/chromium/crNetworkManager.ts @@ -156,13 +156,13 @@ export class CRNetworkManager { _onRequest(workerFrame: frames.Frame | undefined, event: Protocol.Network.requestWillBeSentPayload, interceptionId: string | null) { if (event.request.url.startsWith('data:')) return; - let redirectChain: network.Request[] = []; + let redirectedFrom: network.Request | null = null; if (event.redirectResponse) { const request = this._requestIdToRequest.get(event.requestId); // If we connect late to the target, we could have missed the requestWillBeSent event. if (request) { this._handleRequestRedirect(request, event.redirectResponse); - redirectChain = request.request._redirectChain; + redirectedFrom = request.request; } } const frame = event.frameId ? this._page._frameManager.frame(event.frameId) : workerFrame; @@ -173,7 +173,7 @@ export class CRNetworkManager { } const isNavigationRequest = event.requestId === event.loaderId && event.type === 'Document'; const documentId = isNavigationRequest ? event.loaderId : undefined; - const request = new InterceptableRequest(this._client, frame, interceptionId, documentId, this._userRequestInterceptionEnabled, event, redirectChain); + const request = new InterceptableRequest(this._client, frame, interceptionId, documentId, this._userRequestInterceptionEnabled, event, redirectedFrom); this._requestIdToRequest.set(event.requestId, request); this._page._frameManager.requestStarted(request.request); } @@ -188,7 +188,6 @@ export class CRNetworkManager { _handleRequestRedirect(request: InterceptableRequest, responsePayload: Protocol.Network.Response) { const response = this._createResponse(request, responsePayload); - request.request._redirectChain.push(request.request); response._requestFinished(new Error('Response body is unavailable for redirect responses')); this._requestIdToRequest.delete(request._requestId); if (request._interceptionId) @@ -248,13 +247,13 @@ class InterceptableRequest implements network.RouteDelegate { _documentId: string | undefined; private _client: CRSession; - constructor(client: CRSession, frame: frames.Frame, interceptionId: string | null, documentId: string | undefined, allowInterception: boolean, event: Protocol.Network.requestWillBeSentPayload, redirectChain: network.Request[]) { + constructor(client: CRSession, frame: frames.Frame, interceptionId: string | null, documentId: string | undefined, allowInterception: boolean, event: Protocol.Network.requestWillBeSentPayload, redirectedFrom: network.Request | null) { this._client = client; this._requestId = event.requestId; this._interceptionId = interceptionId; this._documentId = documentId; - this.request = new network.Request(allowInterception ? this : null, frame, redirectChain, documentId, + this.request = new network.Request(allowInterception ? this : null, frame, redirectedFrom, documentId, event.request.url, (event.type || '').toLowerCase(), event.request.method, event.request.postData || null, headersObject(event.request.headers)); } diff --git a/src/firefox/ffNetworkManager.ts b/src/firefox/ffNetworkManager.ts index 536024a6a9..ed8de02f7a 100644 --- a/src/firefox/ffNetworkManager.ts +++ b/src/firefox/ffNetworkManager.ts @@ -52,17 +52,13 @@ export class FFNetworkManager { } _onRequestWillBeSent(event: Protocol.Network.requestWillBeSentPayload) { - const redirected = event.redirectedFrom ? this._requests.get(event.redirectedFrom) : null; - const frame = redirected ? redirected.request.frame() : (event.frameId ? this._page._frameManager.frame(event.frameId) : null); + const redirectedFrom = event.redirectedFrom ? (this._requests.get(event.redirectedFrom) || null) : null; + const frame = redirectedFrom ? redirectedFrom.request.frame() : (event.frameId ? this._page._frameManager.frame(event.frameId) : null); if (!frame) return; - let redirectChain: network.Request[] = []; - if (redirected) { - redirectChain = redirected.request._redirectChain; - redirectChain.push(redirected.request); - this._requests.delete(redirected._id); - } - const request = new InterceptableRequest(this._session, frame, redirectChain, event); + if (redirectedFrom) + this._requests.delete(redirectedFrom._id); + const request = new InterceptableRequest(this._session, frame, redirectedFrom, event); this._requests.set(request._id, request); this._page._frameManager.requestStarted(request.request); } @@ -91,7 +87,7 @@ export class FFNetworkManager { if (!request) return; const response = request.request._existingResponse()!; - // Keep redirected requests in the map for future reference in redirectChain. + // Keep redirected requests in the map for future reference as redirectedFrom. const isRedirected = response.status() >= 300 && response.status() <= 399; if (isRedirected) { response._requestFinished(new Error('Response body is unavailable for redirect responses')); @@ -146,7 +142,7 @@ class InterceptableRequest implements network.RouteDelegate { _id: string; private _session: FFSession; - constructor(session: FFSession, frame: frames.Frame, redirectChain: network.Request[], payload: Protocol.Network.requestWillBeSentPayload) { + constructor(session: FFSession, frame: frames.Frame, redirectedFrom: InterceptableRequest | null, payload: Protocol.Network.requestWillBeSentPayload) { this._id = payload.requestId; this._session = session; @@ -154,7 +150,7 @@ class InterceptableRequest implements network.RouteDelegate { for (const {name, value} of payload.headers) headers[name.toLowerCase()] = value; - this.request = new network.Request(payload.isIntercepted ? this : null, frame, redirectChain, payload.navigationId, + this.request = new network.Request(payload.isIntercepted ? this : null, frame, redirectedFrom ? redirectedFrom.request : null, payload.navigationId, payload.url, causeToResourceType[payload.cause] || 'other', payload.method, payload.postData || null, headers); } diff --git a/src/frames.ts b/src/frames.ts index 7740a27695..9e077f1633 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -387,7 +387,7 @@ export class Frame { disposer.dispose(); - return request ? request._finalRequest.response() : null; + return request ? request._finalRequest().response() : null; function throwIfError(error: Error|void): asserts error is void { if (!error) @@ -430,7 +430,7 @@ export class Frame { if (error) throw error; - return request ? request._finalRequest.response() : null; + return request ? request._finalRequest().response() : null; } async _waitForLoadState(options: types.NavigateOptions = {}): Promise { @@ -519,7 +519,7 @@ export class Frame { this._requestWatchers.delete(onRequest); }; const onRequest = (request: network.Request) => { - if (!request._documentId || request.redirectChain().length) + if (!request._documentId || request.redirectedFrom()) return; requestMap.set(request._documentId, request); }; diff --git a/src/network.ts b/src/network.ts index bf60b91450..137505fea7 100644 --- a/src/network.ts +++ b/src/network.ts @@ -95,8 +95,8 @@ export type Headers = { [key: string]: string }; export class Request { readonly _routeDelegate: RouteDelegate | null; private _response: Response | null = null; - _redirectChain: Request[]; - _finalRequest: Request; + private _redirectedFrom: Request | null; + private _redirectedTo: Request | null = null; readonly _documentId?: string; readonly _isFavicon: boolean; private _failureText: string | null = null; @@ -109,15 +109,14 @@ export class Request { private _waitForResponsePromise: Promise; private _waitForResponsePromiseCallback: (value: Response | null) => void = () => {}; - constructor(routeDelegate: RouteDelegate | null, frame: frames.Frame, redirectChain: Request[], documentId: string | undefined, + constructor(routeDelegate: RouteDelegate | null, frame: frames.Frame, redirectedFrom: Request | null, documentId: string | undefined, url: string, resourceType: string, method: string, postData: string | null, headers: Headers) { assert(!url.startsWith('data:'), 'Data urls should not fire requests'); this._routeDelegate = routeDelegate; this._frame = frame; - this._redirectChain = redirectChain; - this._finalRequest = this; - for (const request of redirectChain) - request._finalRequest = this; + this._redirectedFrom = redirectedFrom; + if (redirectedFrom) + redirectedFrom._redirectedTo = this; this._documentId = documentId; this._url = stripFragmentFromUrl(url); this._resourceType = resourceType; @@ -166,6 +165,10 @@ export class Request { this._waitForResponsePromiseCallback(response); } + _finalRequest(): Request { + return this._redirectedTo ? this._redirectedTo._finalRequest() : this; + } + frame(): frames.Frame { return this._frame; } @@ -174,8 +177,12 @@ export class Request { return !!this._documentId; } - redirectChain(): Request[] { - return this._redirectChain.slice(); + redirectedFrom(): Request | null { + return this._redirectedFrom; + } + + redirectedTo(): Request | null { + return this._redirectedTo; } failure(): { errorText: string; } | null { diff --git a/src/webkit/wkInterceptableRequest.ts b/src/webkit/wkInterceptableRequest.ts index db111533fb..33acc49d0d 100644 --- a/src/webkit/wkInterceptableRequest.ts +++ b/src/webkit/wkInterceptableRequest.ts @@ -46,11 +46,12 @@ export class WKInterceptableRequest implements network.RouteDelegate { _interceptedCallback: () => void = () => {}; private _interceptedPromise: Promise; - constructor(session: WKSession, allowInterception: boolean, frame: frames.Frame, event: Protocol.Network.requestWillBeSentPayload, redirectChain: network.Request[], documentId: string | undefined) { + constructor(session: WKSession, allowInterception: boolean, frame: frames.Frame, event: Protocol.Network.requestWillBeSentPayload, redirectedFrom: network.Request | null, documentId: string | undefined) { this._session = session; this._requestId = event.requestId; - this.request = new network.Request(allowInterception ? this : null, frame, redirectChain, documentId, event.request.url, - event.type ? event.type.toLowerCase() : 'Unknown', event.request.method, event.request.postData || null, headersObject(event.request.headers)); + const resourceType = event.type ? event.type.toLowerCase() : (redirectedFrom ? redirectedFrom.resourceType() : 'unknown'); + this.request = new network.Request(allowInterception ? this : null, frame, redirectedFrom, documentId, event.request.url, + resourceType, event.request.method, event.request.postData || null, headersObject(event.request.headers)); this._interceptedPromise = new Promise(f => this._interceptedCallback = f); } diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index c73116ad8c..d015bd802b 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -730,27 +730,26 @@ export class WKPage implements PageDelegate { _onRequestWillBeSent(session: WKSession, event: Protocol.Network.requestWillBeSentPayload) { if (event.request.url.startsWith('data:')) return; - let redirectChain: network.Request[] = []; + let redirectedFrom: network.Request | null = null; if (event.redirectResponse) { const request = this._requestIdToRequest.get(event.requestId); // If we connect late to the target, we could have missed the requestWillBeSent event. if (request) { this._handleRequestRedirect(request, event.redirectResponse); - redirectChain = request.request._redirectChain; + redirectedFrom = request.request; } } const frame = this._page._frameManager.frame(event.frameId)!; // TODO(einbinder) this will fail if we are an XHR document request const isNavigationRequest = event.type === 'Document'; const documentId = isNavigationRequest ? event.loaderId : undefined; - const request = new WKInterceptableRequest(session, this._page._needsRequestInterception(), frame, event, redirectChain, documentId); + const request = new WKInterceptableRequest(session, this._page._needsRequestInterception(), frame, event, redirectedFrom, documentId); this._requestIdToRequest.set(event.requestId, request); this._page._frameManager.requestStarted(request.request); } private _handleRequestRedirect(request: WKInterceptableRequest, responsePayload: Protocol.Network.Response) { const response = request.createResponse(responsePayload); - request.request._redirectChain.push(request.request); response._requestFinished(new Error('Response body is unavailable for redirect responses')); this._requestIdToRequest.delete(request._requestId); this._page._frameManager.requestReceivedResponse(response); diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index c52f0f7205..bf6cad1420 100644 --- a/test/browsercontext.spec.js +++ b/test/browsercontext.spec.js @@ -488,6 +488,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF page.evaluate(url => window.open(url), server.EMPTY_PAGE) ]); expect(otherPage.url()).toBe(server.EMPTY_PAGE); + await context.close(); }); it.fail(CHROMIUM)('should have url with domcontentloaded', async({browser, server}) => { const context = await browser.newContext(); @@ -497,6 +498,7 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, FF page.evaluate(url => window.open(url), server.EMPTY_PAGE) ]); expect(otherPage.url()).toBe(server.EMPTY_PAGE); + await context.close(); }); it('should report when a new page is created and closed', async({browser, server}) => { const context = await browser.newContext(); diff --git a/test/interception.spec.js b/test/interception.spec.js index 0ca4b2b9df..8cf190892e 100644 --- a/test/interception.spec.js +++ b/test/interception.spec.js @@ -204,16 +204,19 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p expect(response.url()).toContain('empty.html'); expect(requests.length).toBe(5); expect(requests[2].resourceType()).toBe('document'); - // Check redirect chain - const redirectChain = response.request().redirectChain(); - expect(redirectChain.length).toBe(4); - expect(redirectChain[0].url()).toContain('/non-existing-page.html'); - expect(redirectChain[2].url()).toContain('/non-existing-page-3.html'); - for (let i = 0; i < redirectChain.length; ++i) { - const request = redirectChain[i]; - expect(request.isNavigationRequest()).toBe(true); - expect(request.redirectChain().indexOf(request)).toBe(i); + const chain = []; + for (let r = response.request(); r; r = r.redirectedFrom()) { + chain.push(r); + expect(r.isNavigationRequest()).toBe(true); } + expect(chain.length).toBe(5); + expect(chain[0].url()).toContain('/empty.html'); + expect(chain[1].url()).toContain('/non-existing-page-4.html'); + expect(chain[2].url()).toContain('/non-existing-page-3.html'); + expect(chain[3].url()).toContain('/non-existing-page-2.html'); + expect(chain[4].url()).toContain('/non-existing-page.html'); + for (let i = 0; i < chain.length; i++) + expect(chain[i].redirectedTo()).toBe(i ? chain[i - 1] : null); }); it('should work with redirects for subresources', async({page, server}) => { const requests = []; @@ -231,12 +234,14 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p expect(response.url()).toContain('one-style.html'); expect(requests.length).toBe(5); expect(requests[0].resourceType()).toBe('document'); - expect(requests[1].resourceType()).toBe('stylesheet'); - // Check redirect chain - const redirectChain = requests[1].redirectChain(); - expect(redirectChain.length).toBe(3); - expect(redirectChain[0].url()).toContain('/one-style.css'); - expect(redirectChain[2].url()).toContain('/three-style.css'); + + let r = requests.find(r => r.url().includes('/four-style.css')); + for (const url of ['/four-style.css', '/three-style.css', '/two-style.css', '/one-style.css']) { + expect(r.resourceType()).toBe('stylesheet'); + expect(r.url()).toContain(url); + r = r.redirectedFrom(); + } + expect(r).toBe(null); }); it('should work with equal requests', async({page, server}) => { await page.goto(server.EMPTY_PAGE); diff --git a/test/network.spec.js b/test/network.spec.js index 59426d4b66..a833f3d5af 100644 --- a/test/network.spec.js +++ b/test/network.spec.js @@ -130,9 +130,9 @@ module.exports.describe = function({testRunner, expect, MAC, WIN, FFOX, CHROMIUM it('should throw when requesting body of redirected response', async({page, server}) => { server.setRedirect('/foo.html', '/empty.html'); const response = await page.goto(server.PREFIX + '/foo.html'); - const redirectChain = response.request().redirectChain(); - expect(redirectChain.length).toBe(1); - const redirected = await redirectChain[0].response(); + const redirectedFrom = response.request().redirectedFrom(); + expect(redirectedFrom).toBeTruthy(); + const redirected = await redirectedFrom.response(); expect(redirected.status()).toBe(302); let error = null; await redirected.text().catch(e => error = e); @@ -295,11 +295,10 @@ module.exports.describe = function({testRunner, expect, MAC, WIN, FFOX, CHROMIUM `200 ${server.EMPTY_PAGE}`, `DONE ${server.EMPTY_PAGE}` ]); - - // Check redirect chain - const redirectChain = response.request().redirectChain(); - expect(redirectChain.length).toBe(1); - expect(redirectChain[0].url()).toContain('/foo.html'); + const redirectedFrom = response.request().redirectedFrom(); + expect(redirectedFrom.url()).toContain('/foo.html'); + expect(redirectedFrom.redirectedFrom()).toBe(null); + expect(redirectedFrom.redirectedTo()).toBe(response.request()); }); });