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
Response headers/raw headers to more closely match Node's functionality. #1564
Conversation
Alright. @gr2m and @paulmelnikow this is ready for review. |
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.
Wow! Again, great work on cleaning this up. I left some comments. There is a lot here so I might take another pass through it now that I've a good understanding.
Thanks also for your comment explaining the changes. That was immensely helpful in understanding this!
lib/common.js
Outdated
* https://nodejs.org/api/http.html#http_message_rawheaders | ||
*/ | ||
const headersInputToRawArray = function(headers) { | ||
if (!headers) { |
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.
Should this check for undefined or null? That way it would reject likely mistakes like an empty string.
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.
👍 undefined
is a valid input so I'll check for that and let other falsy values error out.
let values | ||
if (typeof value === 'function') { | ||
// Function values are evaluated towards the end of the response, before that we use a placeholder | ||
// string just to designate that the header exists. Useful when `Content-Type` is set with a function. |
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.
I wonder if it would be better to use a boolean true
to represent this condition. Or better yet, true
in place of an array.
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.
values
needs to be an array of strings to behave.
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.
Which part does? I think it's a good idea not to evaluate the function values twice, though it makes me nervous to pass around objects with half-evaluated header values.
I haven't followed exactly how this is used, though some of it seems related to checking whether other headers are present. Would it work to pass around the list of keys?
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.
Augh I see it's really involved and complicated. Would like this to be cleaner but it can be left for another day!
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.
To clarify, the functions are called at most once and we have a test to assert that.
addHeaderLine
is a private helper function for headersArrayToObject
, which is called in two places.
First in RequestOverrider.end
, in the case all the functions have already been evaluated so value
will never be a function.
The second place headersArrayToObject
is called is in Interceptor.reply
where headers are temporarily assembled to aid in other logic between the Interceptor
and the RequestOverrider
.
I left a comment there to explain why this is done:
Lines 112 to 119 in 89f3ebf
// Prepare the headers temporarily so we can make best guesses about content-encoding and content-type | |
// below as well as while the response is being processed in RequestOverrider.end(). | |
// Including all the default headers is safe for our purposes because of the specific headers we introspect. | |
// A more thoughtful process is used to merge the default headers when the response headers are finally computed. | |
this.headers = common.headersArrayToObject([ | |
...this.rawHeaders, | |
...this.scope._defaultReplyHeaders, | |
]) |
All of that is roughly the same as it was before. Previously, in Interceptor.reply
, the headers would get grouped and the ones that happened to be functions where just left as functions in the headers object. The change I created here was caused by enforcing a parity with the headers object and what Node would create. Node's http.IncomingMessage.headers
object always has values of strings (expect set-cookie
which is an array of strings). For the case of values that are functions, I had to make a decision on how to handle those given it's not a feature of Node. I didn't want to skip them because there are cases, eg Content-Encoding
, where the existence of the field name made a difference. I chose to use the functions name as I thought it might help users when debugging.
Passing around a list of header field names wouldn't work as there are sometimes, eg Content-Type
, where the value in introspected. Now in those cases, if the value is a function, it's not compared correctly. But that's a shortcoming of the current code, and probably one everyone is ok living with as the alternative would be to evaluate the header functions before we have a response.
]) | ||
|
||
/** | ||
* Set key/value data in accordance with Node's logic for folding duplicate headers. |
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.
From reading this, it's not clear which parts of the code below is related to specific behaviors in Node. Enumerating the specifics is important here. Could you paste in some of the notes from your PR message, and include the link?
tests/test_default_reply_headers.js
Outdated
t.deepEqual(headers, { | ||
'x-custom-header': 'boo!', | ||
'x-another-header': 'foobar', | ||
'x-powered-by': 'Meeee', |
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.
Same comment as above.
|
||
if (this.scope.contentLen) { | ||
// https://tools.ietf.org/html/rfc7230#section-3.3.2 | ||
this.rawHeaders.push('Content-Length', body.length) |
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.
👍
lib/request_overrider.js
Outdated
const key = response.rawHeaders[rawHeaderIndex] | ||
const value = response.rawHeaders[rawHeaderIndex + 1] | ||
// Evaluate functional headers. | ||
for (let i = 1, len = response.rawHeaders.length; i < len; i = i + 2) { |
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.
I don't think this optimization is necessary and it sort of obscures what this is doing.
for (let i = 1, len = response.rawHeaders.length; i < len; i = i + 2) { | |
for (let i = 1; i < response.rawHeaders.length; i += 2) { |
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.
I was following existing convention, but would be happy to change.
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.
I definitely like to not make the perfect the enemy of the good, and make a point to avoid requesting changes on things that were just moved intact. Sorry I didn't realize from the diff that was the case.
} | ||
} | ||
|
||
response.headers = common.headersArrayToObject(response.rawHeaders) |
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.
👍
lib/request_overrider.js
Outdated
response.headers = { | ||
...response.headers, | ||
...castHeaders, | ||
for (let i = 0, len = existingHeaders.length; i < len; i = i + 2) { |
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.
Maybe it would be helpful to define forEachHeaderValue
that invokes a callback with (value, key, indexOfKey)
parameters? It could be used in the three places here where these headers are being iterated.
Coverage looks good. I'm not seeing any new uncovered lines. It would be really great to get to 100% so we won't have to keep checking for that! |
* Node's implementation is larger because it highly optimizes for not having to call `toLowerCase()`. | ||
* We've opted to always call `toLowerCase` in exchange for a more concise function. | ||
* | ||
* While Node has the luxury of knowing `value` is always a string, we do an extra step of coercion at the top. |
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.
Thanks for these doc additions 🙌
let values | ||
if (typeof value === 'function') { | ||
// Function values are evaluated towards the end of the response, before that we use a placeholder | ||
// string just to designate that the header exists. Useful when `Content-Type` is set with a function. |
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.
Augh I see it's really involved and complicated. Would like this to be cleaner but it can be left for another day!
}) | ||
common.forEachHeader(defaultHeaders, (value, fieldName) => { | ||
if (!definedHeaders.has(fieldName.toLowerCase())) { | ||
result.push(fieldName, value) |
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.
This turned out nice!
'X-Custom-Header': 'boo!', | ||
'x-another-header': 'foobar', | ||
'X-Reply-Only': 'from-reply', | ||
'x-overridden': 'from-reply', |
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.
Good call on changing these field names.
Thanks for the updates and responses. I'll take another read through this, hopefully by tomorrow. |
Fixes #539. |
Updates the header handling in the `Interceptor` and `RequestOverrider` with the intention of mimicking the native behavior of `http.IncomingMessage.rawHeaders`. > The raw request/response headers list exactly as they were received. There are three fundamental changes in this changeset: 1) Header Input Type Previously, headers could be provided to: - `Scope.defaultReplyHeaders` as a plain object - `Interceptor.reply(status, body, headers)` as a plain object or an array of raw headers - `Interceptor.reply(() => [status, body, headers]` as a plain object Now, all three allow consistent inputs where the headers can be provided as a plain object, an array of raw headers, or a `Map`. 2) Duplicate Headers Folding This change deviates from the suggested guidelines laid out in nock#1553 because those guidelines didn't properly handle duplicate headers, especially when some are defined as defaults. This change was modeled to duplicate [Node's implementation](https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_incoming.js#L245) ([relevant docs](https://nodejs.org/api/http.html#http_message_headers)). It specifically lays out how duplicate headers are handled depending on the field name. In the case of default headers, they are not included on the `Response` (not even in the raw headers) if the field name exists in the reply headers (using a case-insensitive comparison). 3) Raw Headers are the Source of Truth Previously, the `Interceptor` and `RequestOverrider` mostly keep track of headers as a plain object and the array of raw headers was created by looping that object. This was the cause for inconsistencies with the final result of the raw headers. The problem with that approach is that converting raw headers to an object is a lossy process, so going backwards makes it impossible to guarantee the correct results. This change reverses that logic and now the `Interceptor` and `RequestOverrider` maintain the header data in raw arrays. All additions to headers are only added to raw headers. The plain headers object is never mutated directly, and instead is [re]created from the raw headers as needed.
Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>
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.
Let's get this in!
🎉 This PR is included in version 11.0.0-beta.18 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…ity. (#1564) * Response headers to more closely match Node. Updates the header handling in the `Interceptor` and `RequestOverrider` with the intention of mimicking the native behavior of `http.IncomingMessage.rawHeaders`. > The raw request/response headers list exactly as they were received. There are three fundamental changes in this changeset: 1) Header Input Type Previously, headers could be provided to: - `Scope.defaultReplyHeaders` as a plain object - `Interceptor.reply(status, body, headers)` as a plain object or an array of raw headers - `Interceptor.reply(() => [status, body, headers]` as a plain object Now, all three allow consistent inputs where the headers can be provided as a plain object, an array of raw headers, or a `Map`. 2) Duplicate Headers Folding This change deviates from the suggested guidelines laid out in #1553 because those guidelines didn't properly handle duplicate headers, especially when some are defined as defaults. This change was modeled to duplicate [Node's implementation](https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_incoming.js#L245) ([relevant docs](https://nodejs.org/api/http.html#http_message_headers)). It specifically lays out how duplicate headers are handled depending on the field name. In the case of default headers, they are not included on the `Response` (not even in the raw headers) if the field name exists in the reply headers (using a case-insensitive comparison). 3) Raw Headers are the Source of Truth Previously, the `Interceptor` and `RequestOverrider` mostly keep track of headers as a plain object and the array of raw headers was created by looping that object. This was the cause for inconsistencies with the final result of the raw headers. The problem with that approach is that converting raw headers to an object is a lossy process, so going backwards makes it impossible to guarantee the correct results. This change reverses that logic and now the `Interceptor` and `RequestOverrider` maintain the header data in raw arrays. All additions to headers are only added to raw headers. The plain headers object is never mutated directly, and instead is [re]created from the raw headers as needed.
…ity. (#1564) * Response headers to more closely match Node. Updates the header handling in the `Interceptor` and `RequestOverrider` with the intention of mimicking the native behavior of `http.IncomingMessage.rawHeaders`. > The raw request/response headers list exactly as they were received. There are three fundamental changes in this changeset: 1) Header Input Type Previously, headers could be provided to: - `Scope.defaultReplyHeaders` as a plain object - `Interceptor.reply(status, body, headers)` as a plain object or an array of raw headers - `Interceptor.reply(() => [status, body, headers]` as a plain object Now, all three allow consistent inputs where the headers can be provided as a plain object, an array of raw headers, or a `Map`. 2) Duplicate Headers Folding This change deviates from the suggested guidelines laid out in #1553 because those guidelines didn't properly handle duplicate headers, especially when some are defined as defaults. This change was modeled to duplicate [Node's implementation](https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_incoming.js#L245) ([relevant docs](https://nodejs.org/api/http.html#http_message_headers)). It specifically lays out how duplicate headers are handled depending on the field name. In the case of default headers, they are not included on the `Response` (not even in the raw headers) if the field name exists in the reply headers (using a case-insensitive comparison). 3) Raw Headers are the Source of Truth Previously, the `Interceptor` and `RequestOverrider` mostly keep track of headers as a plain object and the array of raw headers was created by looping that object. This was the cause for inconsistencies with the final result of the raw headers. The problem with that approach is that converting raw headers to an object is a lossy process, so going backwards makes it impossible to guarantee the correct results. This change reverses that logic and now the `Interceptor` and `RequestOverrider` maintain the header data in raw arrays. All additions to headers are only added to raw headers. The plain headers object is never mutated directly, and instead is [re]created from the raw headers as needed.
…ity. (#1564) * Response headers to more closely match Node. Updates the header handling in the `Interceptor` and `RequestOverrider` with the intention of mimicking the native behavior of `http.IncomingMessage.rawHeaders`. > The raw request/response headers list exactly as they were received. There are three fundamental changes in this changeset: 1) Header Input Type Previously, headers could be provided to: - `Scope.defaultReplyHeaders` as a plain object - `Interceptor.reply(status, body, headers)` as a plain object or an array of raw headers - `Interceptor.reply(() => [status, body, headers]` as a plain object Now, all three allow consistent inputs where the headers can be provided as a plain object, an array of raw headers, or a `Map`. 2) Duplicate Headers Folding This change deviates from the suggested guidelines laid out in #1553 because those guidelines didn't properly handle duplicate headers, especially when some are defined as defaults. This change was modeled to duplicate [Node's implementation](https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_incoming.js#L245) ([relevant docs](https://nodejs.org/api/http.html#http_message_headers)). It specifically lays out how duplicate headers are handled depending on the field name. In the case of default headers, they are not included on the `Response` (not even in the raw headers) if the field name exists in the reply headers (using a case-insensitive comparison). 3) Raw Headers are the Source of Truth Previously, the `Interceptor` and `RequestOverrider` mostly keep track of headers as a plain object and the array of raw headers was created by looping that object. This was the cause for inconsistencies with the final result of the raw headers. The problem with that approach is that converting raw headers to an object is a lossy process, so going backwards makes it impossible to guarantee the correct results. This change reverses that logic and now the `Interceptor` and `RequestOverrider` maintain the header data in raw arrays. All additions to headers are only added to raw headers. The plain headers object is never mutated directly, and instead is [re]created from the raw headers as needed.
For #1553
Updates the header handling in the
Interceptor
andRequestOverrider
with the intention of mimicking the native behavior ofhttp.IncomingMessage.rawHeaders
.There are three fundamental changes in this changeset:
Header Input Type
Previously, headers could be provided to:
Scope.defaultReplyHeaders
as a plain objectInterceptor.reply(status, body, headers)
as a plain object or an array of raw headersInterceptor.reply(() => [status, body, headers]
as a plain objectNow, all three allow consistent inputs where the headers can be provided as a plain object, an array of raw headers, or a
Map
.Duplicate Headers Folding
This change deviates from the suggested guidelines laid out in #1553 because those guidelines didn't properly handle duplicate headers, especially when some are defined as defaults.
Duplicate values for a headers are valid (rfc) and can be represented in the raw HTTP text as either:
or
Note that order is important.
The changes included in this PR were modeled to duplicate Node's implementation (relevant docs). It specifically lays out how duplicate headers are handled depending on the field name.
In the case of default headers, they are not included on the
Response
(not even in the raw headers) if the field name exists in the reply headers (using a case-insensitive comparison).Raw Headers are the Source of Truth
Previously, the
Interceptor
andRequestOverrider
mostly keep track of headers as a plain object and the array of raw headers was created by looping that object. This was the cause for inconsistencies with the final result of the raw headers. The problem with that approach is that converting raw headers to an object is a lossy process, so going backwards makes it impossible to guarantee the correct results.This change reverses that logic and now the
Interceptor
andRequestOverrider
maintain the header data in raw arrays. All additions to headers are only added to raw headers. The plain headers object is never mutated directly, and instead is [re]created from the raw headers as needed.Bugs Addressed
Breaking Changes
The good news is that the breaking changes will not affect the vast majority of users. The main use case tends to be using plain objects, without duplicates, when specifying reply headers. The net result of that input has not changed.
That being said, changes that may throw some users for a loop:
response.rawHeaders
will no longer be lowercased if the headers were provided as defaults or in a direct reply call. They will now stay in their original case.