Response headers/raw headers to more closely match Node's functionality. by mastermatt · Pull Request #1564 · nock/nock

mastermatt · 2019-05-24T04:36:11Z

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:

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.

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:

Cache-Control: no-cache, no-store

or

Cache-Control: no-cache
Cache-Control: no-store

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 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.

Bugs Addressed

Dynamic reply headers field names were not lowercase in the header object.
Default header field names were incorrectly lowercased in the raw header array.
Direct reply header field names were incorrectly lowercased in the raw header array if default headers were also provided.
Dynamic reply headers were not accepted in the raw header array format.
Default headers were included in the response's raw headers even if the same header field was provided in a dynamic reply.
Direct reply headers provided as an object would always use an array for duplicate field names.

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:

The field names in 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.
Duplicate headers are now folded as Node does. Example:

scope.reply(200, "", {
  "X-Some-Header": "one",
  "x-some-header": "two",
  "Content-Type": "application/json",
  "content-type": "text/xml",
})

// Before
response.headers === {
  "x-some-header": ["one", "two"],
  "content-type": ["application/json", "text/xml"],
}

// After
response.headers === {
  "x-some-header": "one, two", // joined as string
  "content-type": "application/json", // rejects duplicate
}

mastermatt · 2019-05-24T16:13:37Z

Alright. @gr2m and @paulmelnikow this is ready for review.

paulmelnikow

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

paulmelnikow · 2019-05-26T14:26:00Z

lib/common.js

+ *  https://nodejs.org/api/http.html#http_message_rawheaders
+ */
+const headersInputToRawArray = function(headers) {
+  if (!headers) {


Should this check for undefined or null? That way it would reject likely mistakes like an empty string.

👍 undefined is a valid input so I'll check for that and let other falsy values error out.

paulmelnikow · 2019-05-26T14:29:31Z

lib/common.js

+  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.


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.

values needs to be an array of strings to behave.

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?

Augh I see it's really involved and complicated. Would like this to be cleaner but it can be left for another day!

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:

nock/lib/interceptor.js

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.

paulmelnikow · 2019-05-26T14:32:04Z

lib/common.js

+])
+
+/**
+ * Set key/value data in accordance with Node's logic for folding duplicate headers.


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?

paulmelnikow · 2019-05-26T14:47:49Z

tests/test_default_reply_headers.js

+  t.deepEqual(headers, {
+    'x-custom-header': 'boo!',
+    'x-another-header': 'foobar',
+    'x-powered-by': 'Meeee',


Same comment as above.

paulmelnikow · 2019-05-26T14:59:18Z

lib/interceptor.js

+
+    if (this.scope.contentLen) {
+      // https://tools.ietf.org/html/rfc7230#section-3.3.2
+      this.rawHeaders.push('Content-Length', body.length)


paulmelnikow · 2019-05-26T15:02:48Z

lib/request_overrider.js

-        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) {


I don't think this optimization is necessary and it sort of obscures what this is doing.

Suggested change

for (let i = 1, len = response.rawHeaders.length; i < len; i = i + 2) {

for (let i = 1; i < response.rawHeaders.length; i += 2) {

I was following existing convention, but would be happy to change.

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.

paulmelnikow · 2019-05-26T15:04:21Z

lib/request_overrider.js

        }
      }

+      response.headers = common.headersArrayToObject(response.rawHeaders)


paulmelnikow · 2019-05-26T15:07:42Z

lib/request_overrider.js

-    response.headers = {
-      ...response.headers,
-      ...castHeaders,
+  for (let i = 0, len = existingHeaders.length; i < len; i = i + 2) {


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.

paulmelnikow · 2019-05-26T15:14:58Z

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!

paulmelnikow · 2019-05-27T18:14:53Z

lib/common.js

+ * 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.


Thanks for these doc additions 🙌

paulmelnikow · 2019-05-27T18:15:36Z

lib/common.js

+  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.


Augh I see it's really involved and complicated. Would like this to be cleaner but it can be left for another day!

paulmelnikow · 2019-05-27T18:17:21Z

lib/request_overrider.js

+  })
+  common.forEachHeader(defaultHeaders, (value, fieldName) => {
+    if (!definedHeaders.has(fieldName.toLowerCase())) {
+      result.push(fieldName, value)


This turned out nice!

paulmelnikow · 2019-05-27T18:17:37Z

tests/test_default_reply_headers.js

-      'X-Custom-Header': 'boo!',
-      'x-another-header': 'foobar',
+      'X-Reply-Only': 'from-reply',
+      'x-overridden': 'from-reply',


Good call on changing these field names.

paulmelnikow · 2019-05-28T16:01:42Z

Thanks for the updates and responses. I'll take another read through this, hopefully by tomorrow.

mastermatt · 2019-05-29T01:48:20Z

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>

paulmelnikow

Let's get this in!

nockbot · 2019-06-08T15:00:49Z

🎉 This PR is included in version 11.0.0-beta.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

nockbot · 2019-08-12T23:57:25Z

🎉 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. (nock#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 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.

mastermatt changed the title ~~WIP Update raw headers.~~ WIP Response headers/raw headers to more closely match Node's functionality. May 24, 2019

mastermatt force-pushed the raw-headers branch from 7d91a72 to 89f3ebf Compare May 24, 2019 15:59

mastermatt marked this pull request as ready for review May 24, 2019 16:00

mastermatt changed the title ~~WIP Response headers/raw headers to more closely match Node's functionality.~~ Response headers/raw headers to more closely match Node's functionality. May 24, 2019

weekly-digest bot mentioned this pull request May 26, 2019

Weekly Digest (19 May, 2019 - 26 May, 2019) #1567

Closed

paulmelnikow reviewed May 26, 2019

View reviewed changes

paulmelnikow reviewed May 27, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jun 2, 2019

Weekly Digest (26 May, 2019 - 2 June, 2019) #1576

Closed

mastermatt force-pushed the raw-headers branch from 3eef69c to 492a922 Compare June 2, 2019 18:21

Apply suggestions from code review

a8e8c3d

Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>

mastermatt force-pushed the raw-headers branch from 492a922 to a8e8c3d Compare June 2, 2019 18:29

mastermatt added 2 commits June 2, 2019 12:58

Remove unused mixin util.

c2579f7

Make proper use of Array.concat().

0240201

paulmelnikow approved these changes Jun 8, 2019

View reviewed changes

paulmelnikow mentioned this pull request Jun 8, 2019

11.x release plan #1516

Closed

6 tasks

mastermatt merged commit b687592 into nock:beta Jun 8, 2019

mastermatt deleted the raw-headers branch June 8, 2019 14:53

nockbot added the released label Jun 8, 2019

This was referenced Jun 8, 2019

Throw if headersFieldNamesToLowerCase is not provided an object. #1574

Merged

Behavior of response.rawHeaders does not match native functionality #1553

Closed

Headers returned from function isn't lower cased #539

Closed

weekly-digest bot mentioned this pull request Jun 9, 2019

Weekly Digest (2 June, 2019 - 9 June, 2019) #1582

Closed

paulmelnikow mentioned this pull request Jul 23, 2019

Post requests failing in Nock beta 11 #1642

Closed

nockbot mentioned this pull request Sep 4, 2019

The automated release is failing 🚨 #1704

Closed

Birch-san mentioned this pull request May 23, 2021

RFC (attempt 2) Dev server support for customizing headers FredKSchott/snowpack#3357

Closed

mastermatt mentioned this pull request Jan 19, 2022

Request headers have array values when using node-fetch #2197

Closed

	// 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,
	])

	for (let i = 1, len = response.rawHeaders.length; i < len; i = i + 2) {
	for (let i = 1; i < response.rawHeaders.length; i += 2) {

Uh oh!

Conversation

mastermatt commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Header Input Type

Duplicate Headers Folding

Raw Headers are the Source of Truth

Bugs Addressed

Breaking Changes

Uh oh!

mastermatt commented May 24, 2019

Uh oh!

paulmelnikow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmelnikow commented May 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmelnikow commented May 28, 2019

Uh oh!

mastermatt commented May 29, 2019

Uh oh!

paulmelnikow left a comment

Choose a reason for hiding this comment

Uh oh!

nockbot commented Jun 8, 2019

Uh oh!

nockbot commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mastermatt commented May 24, 2019 •

edited

Loading