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

fix: prerequest correlation for various retried and cached requests #27771

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ _Released 09/12/2023 (PENDING)_

**Bugfixes:**

- Edge cases where `cy.intercept()` would not properly intercept and asset response bodies would not properly be captured for test replay have been addressed. Addressed in [#27771](https://github.com/cypress-io/cypress/issues/27771).
- Fixed an issue where `enter`, `keyup`, and `space` events where not triggering `click` events properly in some versions of Firefox. Addressed in [#27715](https://github.com/cypress-io/cypress/pull/27715).

**Dependency Updates:**
Expand Down
20 changes: 18 additions & 2 deletions packages/proxy/lib/http/response-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,18 @@ const stringifyFeaturePolicy = (policy: any): string => {
return pairs.map((directive) => directive.join(' ')).join('; ')
}

const requestIdRegEx = /^(.*)-retry-([\d]+)$/
const getOriginalRequestId = (requestId: string) => {
let originalRequestId = requestId
const match = requestIdRegEx.exec(requestId)

if (match) {
[, originalRequestId] = match
}

return originalRequestId
}

const LogResponse: ResponseMiddleware = function () {
this.debug('received response %o', {
browserPreRequest: _.pick(this.req.browserPreRequest, 'requestId'),
Expand Down Expand Up @@ -674,8 +686,10 @@ const ClearCyInitialCookie: ResponseMiddleware = function () {
const MaybeEndWithEmptyBody: ResponseMiddleware = function () {
if (httpUtils.responseMustHaveEmptyBody(this.req, this.incomingRes)) {
if (this.protocolManager && this.req.browserPreRequest?.requestId) {
const requestId = getOriginalRequestId(this.req.browserPreRequest.requestId)

this.protocolManager.responseEndedWithEmptyBody({
requestId: this.req.browserPreRequest.requestId,
requestId,
isCached: this.incomingRes.statusCode === 304,
})
}
Expand Down Expand Up @@ -783,9 +797,11 @@ const MaybeRemoveSecurity: ResponseMiddleware = function () {

const GzipBody: ResponseMiddleware = async function () {
if (this.protocolManager && this.req.browserPreRequest?.requestId) {
const requestId = getOriginalRequestId(this.req.browserPreRequest.requestId)

const span = telemetry.startSpan({ name: 'gzip:body:protocol-notification', parentSpan: this.resMiddlewareSpan, isVerbose })
const resultingStream = this.protocolManager.responseStreamReceived({
requestId: this.req.browserPreRequest.requestId,
requestId,
responseHeaders: this.incomingRes.headers,
isAlreadyGunzipped: this.isGunzipped,
responseStream: this.incomingResStream,
Expand Down
2 changes: 1 addition & 1 deletion packages/proxy/lib/http/util/prerequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export class PreRequests {

removePending (requestId: string) {
this.pendingPreRequests.removeMatching(({ browserPreRequest }) => {
return browserPreRequest.requestId !== requestId
return (browserPreRequest.requestId.includes('-retry-') && !browserPreRequest.requestId.startsWith(`${requestId}-`)) || (!browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId !== requestId)
})
}

Expand Down
45 changes: 43 additions & 2 deletions packages/proxy/test/unit/http/response-middleware.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1847,14 +1847,14 @@ describe('http/response-middleware', function () {
})
})

it('calls responseEndedWithEmptyBody on protocolManager if protocolManager present and request is correlated and response must have empty body and response is not cached', function () {
it('calls responseEndedWithEmptyBody on protocolManager if protocolManager present and retried request is correlated and response must have empty body and response is not cached', function () {
prepareContext({
protocolManager: {
responseEndedWithEmptyBody: responseEndedWithEmptyBodyStub,
},
req: {
browserPreRequest: {
requestId: '123',
requestId: '123-retry-1',
},
},
incomingRes: {
Expand Down Expand Up @@ -2285,6 +2285,47 @@ describe('http/response-middleware', function () {
})
})

it('calls responseStreamReceived on protocolManager if protocolManager present and retried request is correlated', function () {
const stream = Readable.from(['foo'])
const headers = { 'content-encoding': 'gzip' }
const res = {
on: (event, listener) => {},
off: (event, listener) => {},
}

prepareContext({
protocolManager: {
responseStreamReceived: responseStreamReceivedStub,
},
req: {
browserPreRequest: {
requestId: '123-retry-1',
},
},
res,
incomingRes: {
headers,
},
isGunzipped: true,
incomingResStream: stream,
})

return testMiddleware([GzipBody], ctx)
.then(() => {
expect(responseStreamReceivedStub).to.be.calledWith(
sinon.match(function (actual) {
expect(actual.requestId).to.equal('123')
expect(actual.responseHeaders).to.equal(headers)
expect(actual.isAlreadyGunzipped).to.equal(true)
expect(actual.responseStream).to.equal(stream)
expect(actual.res).to.equal(res)

return true
}),
)
})
})

it('does not call responseStreamReceived on protocolManager if protocolManager present and request is not correlated', function () {
const stream = Readable.from(['foo'])
const headers = { 'content-encoding': 'gzip' }
Expand Down
15 changes: 15 additions & 0 deletions packages/proxy/test/unit/http/util/prerequests.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,19 @@ describe('http/util/prerequests', () => {

expectPendingCounts(0, 2)
})

it('removes a pre-request with a matching requestId with retries', () => {
preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235-retry-1', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235-retry-2', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1235-retry-3', url: 'foo', method: 'GET' } as BrowserPreRequest)
preRequests.addPending({ requestId: '1236', url: 'foo', method: 'GET' } as BrowserPreRequest)

expectPendingCounts(0, 6)

preRequests.removePending('1235')

expectPendingCounts(0, 2)
})
})
6 changes: 6 additions & 0 deletions packages/server/lib/browsers/cdp_automation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,12 @@ export class CdpAutomation implements CDPClient {
}

private onResponseReceived = (params: Protocol.Network.ResponseReceivedEvent) => {
if (params.response.fromDiskCache) {
this.automation.onRequestServedFromCache?.(params.requestId)

return
}

const browserResponseReceived: BrowserResponseReceived = {
requestId: params.requestId,
status: params.response.status,
Expand Down
17 changes: 17 additions & 0 deletions packages/server/test/unit/browsers/cdp_automation_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,23 @@ context('lib/browsers/cdp_automation', () => {
},
)
})

it('cleans up prerequests when response is cached from disk', function () {
const browserResponseReceived = {
requestId: '0',
response: {
status: 200,
headers: {},
fromDiskCache: true,
},
}

this.onFn
.withArgs('Network.responseReceived')
.yield(browserResponseReceived)

expect(this.automation.onRequestEvent).not.to.have.been.called
})
})

describe('.onRequestServedFromCache', function () {
Expand Down