-
-
Notifications
You must be signed in to change notification settings - Fork 127
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): support "rawHeaders" in Fetch API Headers #598
Changes from all commits
31db9ee
3e4fcf8
50bdac0
6800c35
c814337
5d2f998
5d47f15
d0c4383
37a5513
a295abd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { | |
type RequestHeadersCompleteCallback, | ||
type ResponseHeadersCompleteCallback, | ||
} from '_http_common' | ||
import { IncomingMessage, ServerResponse } from 'node:http' | ||
import { STATUS_CODES, IncomingMessage, ServerResponse } from 'node:http' | ||
import { Readable } from 'node:stream' | ||
import { invariant } from 'outvariant' | ||
import { INTERNAL_REQUEST_ID_HEADER_NAME } from '../../Interceptor' | ||
|
@@ -13,12 +13,12 @@ import type { NormalizedSocketWriteArgs } from '../Socket/utils/normalizeSocketW | |
import { isPropertyAccessible } from '../../utils/isPropertyAccessible' | ||
import { baseUrlFromConnectionOptions } from '../Socket/utils/baseUrlFromConnectionOptions' | ||
import { parseRawHeaders } from '../Socket/utils/parseRawHeaders' | ||
import { getRawFetchHeaders } from '../../utils/getRawFetchHeaders' | ||
import { | ||
createServerErrorResponse, | ||
RESPONSE_STATUS_CODES_WITHOUT_BODY, | ||
} from '../../utils/responseUtils' | ||
import { createRequestId } from '../../createRequestId' | ||
import { getRawFetchHeaders } from './utils/recordRawHeaders' | ||
|
||
type HttpConnectionOptions = any | ||
|
||
|
@@ -185,11 +185,12 @@ export class MockHttpSocket extends MockSocket { | |
const chunkAfterRequestHeaders = chunkString.slice( | ||
chunk.indexOf('\r\n\r\n') | ||
) | ||
const requestHeaders = | ||
getRawFetchHeaders(this.request!.headers) || this.request!.headers | ||
const requestHeadersString = Array.from(requestHeaders.entries()) | ||
const rawRequestHeaders = getRawFetchHeaders(this.request!.headers) | ||
const requestHeadersString = rawRequestHeaders | ||
// Skip the internal request ID deduplication header. | ||
.filter(([name]) => name !== INTERNAL_REQUEST_ID_HEADER_NAME) | ||
.filter(([name]) => { | ||
return name.toLowerCase() !== INTERNAL_REQUEST_ID_HEADER_NAME | ||
}) | ||
.map(([name, value]) => `${name}: ${value}`) | ||
.join('\r\n') | ||
|
||
|
@@ -304,8 +305,6 @@ export class MockHttpSocket extends MockSocket { | |
read() {}, | ||
}) | ||
) | ||
serverResponse.statusCode = response.status | ||
serverResponse.statusMessage = response.statusText | ||
|
||
/** | ||
* @note Remove the `Connection` and `Date` response headers | ||
|
@@ -319,17 +318,24 @@ export class MockHttpSocket extends MockSocket { | |
serverResponse.removeHeader('connection') | ||
serverResponse.removeHeader('date') | ||
|
||
const rawResponseHeaders = getRawFetchHeaders(response.headers) | ||
|
||
/** | ||
* @note Call `.writeHead` in order to set the raw response headers | ||
* in the same case as they were provided by the developer. Using | ||
* `.setHeader()`/`.appendHeader()` normalizes header names. | ||
*/ | ||
serverResponse.writeHead( | ||
response.status, | ||
response.statusText || STATUS_CODES[response.status], | ||
rawResponseHeaders | ||
) | ||
|
||
// If the developer destroy the socket, gracefully destroy the response. | ||
this.once('error', () => { | ||
serverResponse.destroy() | ||
}) | ||
|
||
// 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) { | ||
serverResponse.setHeader(name, value) | ||
} | ||
|
||
if (response.body) { | ||
try { | ||
const reader = response.body.getReader() | ||
|
@@ -535,7 +541,7 @@ export class MockHttpSocket extends MockSocket { | |
|
||
// Similarly, create a new stream for each response. | ||
if (canHaveBody) { | ||
this.responseStream = new Readable() | ||
this.responseStream = new Readable({ read() {} }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was also erroring. Likely an oversight. |
||
} | ||
|
||
const response = new Response( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,10 @@ import { toInteractiveRequest } from '../../utils/toInteractiveRequest' | |
import { normalizeClientRequestArgs } from './utils/normalizeClientRequestArgs' | ||
import { isNodeLikeError } from '../../utils/isNodeLikeError' | ||
import { createServerErrorResponse } from '../../utils/responseUtils' | ||
import { | ||
recordRawFetchHeaders, | ||
restoreHeadersPrototype, | ||
} from './utils/recordRawHeaders' | ||
|
||
export class ClientRequestInterceptor extends Interceptor<HttpRequestEventMap> { | ||
static symbol = Symbol('client-request-interceptor') | ||
|
@@ -104,12 +108,19 @@ export class ClientRequestInterceptor extends Interceptor<HttpRequestEventMap> { | |
}, | ||
}) | ||
|
||
// Spy on `Header.prototype.set` and `Header.prototype.append` calls | ||
// and record the raw header names provided. This is to support | ||
// `IncomingMessage.prototype.rawHeaders`. | ||
recordRawFetchHeaders() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm enabling the raw headers recording as a part of the ClientRequest interceptor. This is the only place we need it for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about unmocked requests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The behavior should be the same for bypassed requests. Let me check tomorrow if I have tests to prove that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kettanaito Seems like it is working 🎉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kettanaito For some reason, I get this error:
I'm wondering if I do something wrong: function createResponse(message: IncomingMessage) {
...
const rawHeaders = new Headers()
for (let i = 0; i < message.rawHeaders.length; i += 2) {
rawHeaders.append(message.rawHeaders[i], message.rawHeaders[i + 1])
}
const response = new Response(responseBodyOrNull, {
status: message.statusCode,
statusText: message.statusMessage || STATUS_CODES[message.statusCode],
headers: rawHeaders,
})
return response
} The error is thrown from this line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The errors disappear when I set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also see a problem with the apply and restore functionality. It could be how I implemented it in Nock. I'll continue to investigate this tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mikicho, thanks for the tests! I will look at them once I have a moment.
Please, this would be best introduced on its own. Let's not add it as a part of this pull request. The "cannot set symbol" error is caused because something tries to set it when it's already set. Interesting, I thought I put guards against that.
This isn't a solution, it simply means one actor can override previously stored raw headers. This is actually quite dangerous because fetch api primitives repeatedly construct Headers instances. const h = new Headers()
// ^ This is one headers instance.
const r = new Response(null, { headers: h })
r.headers
// ^ And this will be a completely different
// headers instance constructed from "h" as init. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Reverted the change
Got you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will look at that test today, see what I can find. |
||
|
||
this.subscriptions.push(() => { | ||
http.get = originalGet | ||
http.request = originalRequest | ||
|
||
https.get = originalHttpsGet | ||
https.request = originalHttpsRequest | ||
|
||
restoreHeadersPrototype() | ||
}) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
type HeaderTuple = [string, string] | ||
type RawHeaders = Array<HeaderTuple> | ||
|
||
const kRawHeaders = Symbol('kRawHeaders') | ||
const kRestorePatches = Symbol('kRestorePatches') | ||
|
||
function recordRawHeader(headers: Headers, args: HeaderTuple) { | ||
if (Reflect.get(headers, kRawHeaders) == null) { | ||
Object.defineProperty(headers, kRawHeaders, { | ||
value: [], | ||
enumerable: false, | ||
}) | ||
} | ||
const rawHeaders = Reflect.get(headers, kRawHeaders) as RawHeaders | ||
rawHeaders.push(args) | ||
} | ||
|
||
/** | ||
* Patch the global `Headers` class to store raw headers. | ||
* This is for compatibility with `IncomingMessage.prototype.rawHeaders`. | ||
* | ||
* @note Node.js has their own raw headers symbol but it | ||
* only records the first header name in case of multi-value headers. | ||
* Any other headers are normalized before comparing. This makes it | ||
* incompatible with the `rawHeaders` format. | ||
* | ||
* let h = new Headers() | ||
* h.append('X-Custom', 'one') | ||
* h.append('x-custom', 'two') | ||
* h[Symbol('headers map')] // Map { 'X-Custom' => 'one, two' } | ||
*/ | ||
export function recordRawFetchHeaders() { | ||
// Prevent patching the Headers prototype multiple times. | ||
if (Reflect.get(Headers, kRestorePatches)) { | ||
return Reflect.get(Headers, kRestorePatches) | ||
} | ||
|
||
const { Request: OriginalRequest, Response: OriginalResponse } = globalThis | ||
const { set, append, delete: headersDeleteMethod } = Headers.prototype | ||
|
||
Object.defineProperty(Headers, kRestorePatches, { | ||
value: () => { | ||
Headers.prototype.set = set | ||
Headers.prototype.append = append | ||
Headers.prototype.delete = headersDeleteMethod | ||
|
||
globalThis.Request = OriginalRequest | ||
globalThis.Response = OriginalResponse | ||
}, | ||
enumerable: false, | ||
}) | ||
|
||
Headers = new Proxy(Headers, { | ||
construct(target, args, newTarget) { | ||
const headers = Reflect.construct(target, args, newTarget) | ||
const initialHeaders = args[0] || [] | ||
const initialRawHeaders = Array.isArray(initialHeaders) | ||
? initialHeaders | ||
: Object.entries(initialHeaders) | ||
|
||
// Request/Response constructors will set the symbol | ||
// upon creating a new instance, using the raw developer | ||
// input as the raw headers. Skip the symbol altogether | ||
// in those cases because the input to Headers will be normalized. | ||
if (!Reflect.has(headers, kRawHeaders)) { | ||
Object.defineProperty(headers, kRawHeaders, { | ||
value: initialRawHeaders, | ||
enumerable: false, | ||
}) | ||
} | ||
|
||
return headers | ||
}, | ||
}) | ||
|
||
Headers.prototype.set = new Proxy(Headers.prototype.set, { | ||
apply(target, thisArg, args: HeaderTuple) { | ||
recordRawHeader(thisArg, args) | ||
return Reflect.apply(target, thisArg, args) | ||
}, | ||
}) | ||
|
||
Headers.prototype.append = new Proxy(Headers.prototype.append, { | ||
apply(target, thisArg, args: HeaderTuple) { | ||
recordRawHeader(thisArg, args) | ||
return Reflect.apply(target, thisArg, args) | ||
}, | ||
}) | ||
|
||
Headers.prototype.delete = new Proxy(Headers.prototype.delete, { | ||
apply(target, thisArg, args: [string]) { | ||
const rawHeaders = Reflect.get(thisArg, kRawHeaders) as RawHeaders | ||
|
||
if (rawHeaders) { | ||
for (let index = rawHeaders.length - 1; index >= 0; index--) { | ||
if (rawHeaders[index][0].toLowerCase() === args[0].toLowerCase()) { | ||
rawHeaders.splice(index, 1) | ||
} | ||
} | ||
} | ||
|
||
return Reflect.apply(target, thisArg, args) | ||
}, | ||
}) | ||
|
||
Request = new Proxy(Request, { | ||
construct(target, args, newTarget) { | ||
const request = Reflect.construct(target, args, newTarget) | ||
|
||
if ( | ||
typeof args[1] === 'object' && | ||
args[1].headers != null && | ||
!request.headers[kRawHeaders] | ||
) { | ||
request.headers[kRawHeaders] = inferRawHeaders(args[1].headers) | ||
} | ||
|
||
return request | ||
}, | ||
}) | ||
|
||
Response = new Proxy(Response, { | ||
construct(target, args, newTarget) { | ||
const response = Reflect.construct(target, args, newTarget) | ||
|
||
if (typeof args[1] === 'object' && args[1].headers != null) { | ||
/** | ||
* @note Pass the init argument directly because it gets | ||
* transformed into a normalized Headers instance once it | ||
* passes the Response constructor. | ||
*/ | ||
response.headers[kRawHeaders] = inferRawHeaders(args[1].headers) | ||
} | ||
|
||
return response | ||
}, | ||
}) | ||
} | ||
|
||
export function restoreHeadersPrototype() { | ||
if (!Reflect.get(Headers, kRestorePatches)) { | ||
return | ||
} | ||
|
||
Reflect.get(Headers, kRestorePatches)() | ||
} | ||
|
||
export function getRawFetchHeaders(headers: Headers): RawHeaders { | ||
// Return the raw headers, if recorded (i.e. `.set()` or `.append()` was called). | ||
// If no raw headers were recorded, return all the headers. | ||
return Reflect.get(headers, kRawHeaders) || Array.from(headers.entries()) | ||
} | ||
|
||
/** | ||
* Infers the raw headers from the given `HeadersInit` provided | ||
* to the Request/Response constructor. | ||
* | ||
* If the `init.headers` is a Headers instance, use it directly. | ||
* That means the headers were created standalone and already have | ||
* the raw headers stored. | ||
* If the `init.headers` is a HeadersInit, create a new Headers | ||
* instace out of it. | ||
*/ | ||
function inferRawHeaders(headers: HeadersInit): RawHeaders { | ||
if (headers instanceof Headers) { | ||
return Reflect.get(headers, kRawHeaders) | ||
} | ||
|
||
return Reflect.get(new Headers(headers), kRawHeaders) | ||
} |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've deleted this prematurely. Probably cached test results but this was failing. We guarantee to set a response status code even if none was explicitly provided. This isn't what Fetch API does but we do this.