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(ClientRequest): use ServerResponse to build the HTTP response string #596

Merged
merged 5 commits into from
Jul 6, 2024
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
82 changes: 39 additions & 43 deletions src/interceptors/ClientRequest/MockHttpSocket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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<Buffer> = []
// 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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I passed this socket instance as the socket of the IncomingMessage used here. I think it should be more error-proof in case this server response decides to terminate the socket (rightfully), for example.

Copy link
Contributor Author

@mikicho mikicho Jul 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a new socket because I was afraid of infinity loops, but if it makes sense to you, I'm ok with this

Copy link
Member

@kettanaito kettanaito Jul 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't arrive at the infinite loops. This logic is only triggered on respondWith(), and that's called by the developer. If I spot any issues with this, will revert back to use an empty socket.


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({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the MockSocket we already have instead of Writable. While sockets extend streams, they implement additional methods on top. I know Node.js uses a simple Writable in their test but we have to acknowledge that test case is controlled. Our use cases are going to be everything out there. Using an actual socket-compatible instance is crucial here.

write: (chunk, encoding, callback) => {
this.push(chunk, encoding)
callback?.()
},
read() {},
})
)
serverResponse.statusCode = response.status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these server response headers before the developer-sent headers. They can always set these if they wish to.

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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that we can drop this now. Fantastic!

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) {
Expand All @@ -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')
Expand Down
4 changes: 4 additions & 0 deletions test/modules/http/response/http-empty-response.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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('')
})
39 changes: 39 additions & 0 deletions test/modules/http/response/http-response-transfer-encoding.test.ts
Original file line number Diff line number Diff line change
@@ -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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few assertions that the client also receives the right response headers.

expect(res.rawHeaders).toContain('Transfer-Encoding')
expect(await text()).toBe('mock')
})
Loading