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

Throw if headersFieldNamesToLowerCase is not provided an object. #1574

Merged

Conversation

mastermatt
Copy link
Member

Adds a test to cover the exception case.

Going through the git history for this function, all the way back to
when it was added with 4048734, I can't find a case where a non-object
input was valid.

Once PR # 1564 is merged in, this utility will only be called in two
places:

  • Creating an Interceptor if the Scope was created with options
  • When http.request is called with an options object

In both places, providing defined, but non-object values causes chaos.
If the value is a truthy, non-iterable then the header was ignored
during matching. For example, the following will match Interceptors as
if reqheaders had not been provided.

nock('http://example.com', { reqheaders: 1 }).get('/').reply

However, if the value was provided as a non-plain object iterable, such
as an array or string, the header matching logic would attempt to match
header field names with numerical keys, rendering the whole Scope
useless.

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

@paulmelnikow
Copy link
Member

I was going to commit this as a new feature, though I'm not sure how best to describe this from the caller's perspective. Do we still support header arrays passed as e.g. reqheaders or do they have to be objects?

Adds a test to cover the exception case.

Going through the git history for this function, all the way back to
when it was added with 4048734, I can't find a case where a non-object
input was valid.

Once PR # 1564 is merged in, this utility will only be called in two
places:
 - Creating an Interceptor if the Scope was created with `options`
 - When http.request is called with an `options` object

In both places, providing defined, but non-object values causes chaos.
If the value is a truthy, non-iterable then the header was ignored
during matching. For example, the following will match Interceptors as
if `reqheaders` had not been provided.
```js
nock('http://example.com', { reqheaders: 1 }).get('/').reply
```

However, if the value was provided as a non-plain object iterable, such
as an array or string, the header matching logic would attempt to match
header field names with numerical keys, rendering the whole Scope
useless.
@mastermatt mastermatt force-pushed the fix-todo/headers-field-names-to-lower-case branch from bafe6e7 to 7ce75b8 Compare June 8, 2019 15:06
@mastermatt
Copy link
Member Author

The changes to headers in #1564 (that allowed headers as arrays) was only for reply headers. Since that PR was merged, headersFieldNamesToLowerCase is now only used for request headers which must be an object.
From a caller's POV, no functionality or expectations of args have changed. But if the caller provides an invalid arg, it will error out with a specific message instead of continuing with undefined results. I agree this should be committed as a feature.

@mastermatt mastermatt merged commit 8ba0fc7 into nock:beta Jun 8, 2019
@mastermatt mastermatt deleted the fix-todo/headers-field-names-to-lower-case branch June 8, 2019 15:27
@nockbot
Copy link
Collaborator

nockbot commented Jun 8, 2019

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

The release is available on:

Your semantic-release bot 📦🚀

@nockbot
Copy link
Collaborator

nockbot commented Aug 12, 2019

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

gr2m pushed a commit that referenced this pull request Sep 4, 2019
Throw if headersFieldNamesToLowerCase is not provided an object.
Adds a test to cover the exception case.

Going through the git history for this function, all the way back to
when it was added with 4048734, I can't find a case where a non-object
input was valid.

Once PR # 1564 is merged in, this utility will only be called in two
places:
 - Creating an Interceptor if the Scope was created with `options`
 - When http.request is called with an `options` object

In both places, providing defined, but non-object values causes chaos.
If the value is a truthy, non-iterable then the header was ignored
during matching. For example, the following will match Interceptors as
if `reqheaders` had not been provided.
```js
nock('http://example.com', { reqheaders: 1 }).get('/').reply
```

However, if the value was provided as a non-plain object iterable, such
as an array or string, the header matching logic would attempt to match
header field names with numerical keys, rendering the whole Scope
useless.
gr2m pushed a commit that referenced this pull request Sep 4, 2019
Throw if headersFieldNamesToLowerCase is not provided an object.
Adds a test to cover the exception case.

Going through the git history for this function, all the way back to
when it was added with 4048734, I can't find a case where a non-object
input was valid.

Once PR # 1564 is merged in, this utility will only be called in two
places:
 - Creating an Interceptor if the Scope was created with `options`
 - When http.request is called with an `options` object

In both places, providing defined, but non-object values causes chaos.
If the value is a truthy, non-iterable then the header was ignored
during matching. For example, the following will match Interceptors as
if `reqheaders` had not been provided.
```js
nock('http://example.com', { reqheaders: 1 }).get('/').reply
```

However, if the value was provided as a non-plain object iterable, such
as an array or string, the header matching logic would attempt to match
header field names with numerical keys, rendering the whole Scope
useless.
gr2m pushed a commit that referenced this pull request Sep 5, 2019
Throw if headersFieldNamesToLowerCase is not provided an object.
Adds a test to cover the exception case.

Going through the git history for this function, all the way back to
when it was added with 4048734, I can't find a case where a non-object
input was valid.

Once PR # 1564 is merged in, this utility will only be called in two
places:
 - Creating an Interceptor if the Scope was created with `options`
 - When http.request is called with an `options` object

In both places, providing defined, but non-object values causes chaos.
If the value is a truthy, non-iterable then the header was ignored
during matching. For example, the following will match Interceptors as
if `reqheaders` had not been provided.
```js
nock('http://example.com', { reqheaders: 1 }).get('/').reply
```

However, if the value was provided as a non-plain object iterable, such
as an array or string, the header matching logic would attempt to match
header field names with numerical keys, rendering the whole Scope
useless.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants