From fedac459da51f5cea2469dca7ccbc25d039398c0 Mon Sep 17 00:00:00 2001 From: Michael Solomon Date: Sat, 6 Jul 2024 14:11:48 +0300 Subject: [PATCH] fix(ClientRequest): use `ServerResponse` to build the HTTP response string (#596) Co-authored-by: Artem Zakharchenko --- .../ClientRequest/MockHttpSocket.ts | 82 +++++++++---------- .../http/response/http-empty-response.test.ts | 4 + .../http-response-transfer-encoding.test.ts | 39 +++++++++ 3 files changed, 82 insertions(+), 43 deletions(-) create mode 100644 test/modules/http/response/http-response-transfer-encoding.test.ts diff --git a/src/interceptors/ClientRequest/MockHttpSocket.ts b/src/interceptors/ClientRequest/MockHttpSocket.ts index 5f7314c8..2e98cfff 100644 --- a/src/interceptors/ClientRequest/MockHttpSocket.ts +++ b/src/interceptors/ClientRequest/MockHttpSocket.ts @@ -4,7 +4,7 @@ import { type RequestHeadersCompleteCallback, type ResponseHeadersCompleteCallback, } from '_http_common' -import { STATUS_CODES } from 'node:http' +import { IncomingMessage, ServerResponse } from 'node:http' import { Readable } from 'node:stream' import { invariant } from 'outvariant' import { INTERNAL_REQUEST_ID_HEADER_NAME } from '../../Interceptor' @@ -272,36 +272,46 @@ export class MockHttpSocket extends MockSocket { // if it hasn't been flushed already (e.g. someone started reading request stream). this.flushWriteBuffer() - const httpHeaders: Array = [] + // Create a `ServerResponse` instance to delegate HTTP message parsing, + // Transfer-Encoding, and other things to Node.js internals. + const serverResponse = new ServerResponse(new IncomingMessage(this)) - httpHeaders.push( - Buffer.from( - `HTTP/1.1 ${response.status} ${ - response.statusText || STATUS_CODES[response.status] - }\r\n` - ) + /** + * Assign a mock socket instance to the server response to + * spy on the response chunk writes. Push the transformed response chunks + * to this `MockHttpSocket` instance to trigger the "data" event. + * @note Providing the same `MockSocket` instance when creating `ServerResponse` + * does not have the same effect. + * @see https://github.com/nodejs/node/blob/10099bb3f7fd97bb9dd9667188426866b3098e07/test/parallel/test-http-server-response-standalone.js#L32 + */ + serverResponse.assignSocket( + new MockSocket({ + write: (chunk, encoding, callback) => { + this.push(chunk, encoding) + callback?.() + }, + read() {}, + }) ) + serverResponse.statusCode = response.status + serverResponse.statusMessage = response.statusText + + /** + * @note Remove the `Connection` and `Date` response headers + * injected by `ServerResponse` by default. Those are required + * from the server but the interceptor is NOT technically a server. + * It's confusing to add response headers that the developer didn't + * specify themselves. They can always add these if they wish. + * @see https://www.rfc-editor.org/rfc/rfc9110#field.date + * @see https://www.rfc-editor.org/rfc/rfc9110#field.connection + */ + serverResponse.removeHeader('connection') + serverResponse.removeHeader('date') // Get the raw headers stored behind the symbol to preserve name casing. const headers = getRawFetchHeaders(response.headers) || response.headers for (const [name, value] of headers) { - httpHeaders.push(Buffer.from(`${name}: ${value}\r\n`)) - } - - // An empty line separating headers from the body. - httpHeaders.push(Buffer.from('\r\n')) - - const flushHeaders = (value?: Uint8Array) => { - if (httpHeaders.length === 0) { - return - } - - if (typeof value !== 'undefined') { - httpHeaders.push(Buffer.from(value)) - } - - this.push(Buffer.concat(httpHeaders)) - httpHeaders.length = 0 + serverResponse.setHeader(name, value) } if (response.body) { @@ -312,35 +322,21 @@ export class MockHttpSocket extends MockSocket { const { done, value } = await reader.read() if (done) { + serverResponse.end() break } - // Flush the headers upon the first chunk in the stream. - // This ensures the consumer will start receiving the response - // as it streams in (subsequent chunks are pushed). - if (httpHeaders.length > 0) { - flushHeaders(value) - continue - } - - // Subsequent body chukns are push to the stream. - this.push(value) + serverResponse.write(value) } } catch (error) { // Coerce response stream errors to 500 responses. - // Don't flush the original response headers because - // unhandled errors translate to 500 error responses forcefully. this.respondWith(createServerErrorResponse(error)) - return } + } else { + serverResponse.end() } - // If the headers were not flushed up to this point, - // this means the response either had no body or had - // an empty body stream. Flush the headers. - flushHeaders() - // Close the socket if the connection wasn't marked as keep-alive. if (!this.shouldKeepAlive) { this.emit('readable') diff --git a/test/modules/http/response/http-empty-response.test.ts b/test/modules/http/response/http-empty-response.test.ts index 3069b6a0..92b3a701 100644 --- a/test/modules/http/response/http-empty-response.test.ts +++ b/test/modules/http/response/http-empty-response.test.ts @@ -27,5 +27,9 @@ it('supports responding with an empty mocked response', async () => { const { res, text } = await waitForClientRequest(request) expect(res.statusCode).toBe(200) + // Must not set any response headers that were not + // explicitly provided in the mocked response. + expect(res.headers).toEqual({}) + expect(res.rawHeaders).toEqual([]) expect(await text()).toBe('') }) diff --git a/test/modules/http/response/http-response-transfer-encoding.test.ts b/test/modules/http/response/http-response-transfer-encoding.test.ts new file mode 100644 index 00000000..75fdc75f --- /dev/null +++ b/test/modules/http/response/http-response-transfer-encoding.test.ts @@ -0,0 +1,39 @@ +/** + * @vitest-environment node + */ +import { it, expect, beforeAll, afterEach, afterAll } from 'vitest' +import http from 'node:http' +import { waitForClientRequest } from '../../../helpers' +import { ClientRequestInterceptor } from '../../../../src/interceptors/ClientRequest' + +const interceptor = new ClientRequestInterceptor() + +beforeAll(() => { + interceptor.apply() +}) + +afterEach(() => { + interceptor.removeAllListeners() +}) + +afterAll(() => { + interceptor.dispose() +}) + +it('responds with a mocked "transfer-encoding: chunked" response', async () => { + interceptor.on('request', ({ request }) => { + request.respondWith( + new Response('mock', { + headers: { 'Transfer-Encoding': 'chunked' }, + }) + ) + }) + + const request = http.get('http://localhost') + const { res, text } = await waitForClientRequest(request) + + expect(res.statusCode).toBe(200) + expect(res.headers).toHaveProperty('transfer-encoding', 'chunked') + expect(res.rawHeaders).toContain('Transfer-Encoding') + expect(await text()).toBe('mock') +})