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

Added compatibility with node-http-proxy header altering #1484

Merged
merged 9 commits into from
Jul 22, 2019
7 changes: 7 additions & 0 deletions lib/request_overrider.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,13 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
/// like to change request.path in mid-flight.
options.path = req.path

// similarly, node-http-proxy will modify headers in flight, so we have to put the headers back
// into options
const reqHeaders = req.getHeaders ? req.getHeaders() : req._headers
if (Object.keys(reqHeaders).length > 0) {
options.headers = req.getHeaders ? req.getHeaders() : req._headers
}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be replaced with options.headers = req.getHeaders()? The req.getHeaders truthiness check is unnecessary in the Node versions we support. That would also fix the coverage issue you mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep sounds simpler - the only problem is that is causes some of the other tests that are expecting headers: undefined to fail because they now get headers: {}, but that's easy enough to change in the test if you're happy with it :).


// fixes #976
options.protocol = `${options.proto}:`

Expand Down
47 changes: 44 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"eslint-plugin-promise": "^4.0.1",
"eslint-plugin-standard": "^4.0.0",
"got": "^9.4.0",
"http-proxy": "^1.17.0",
"hyperquest": "^2.1.3",
"isomorphic-fetch": "^2.2.0",
"lolex": "^3.0.0",
Expand Down
41 changes: 41 additions & 0 deletions tests/test_intercept.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const restify = require('restify-clients')
const hyperquest = require('hyperquest')
const got = require('got')
const nock = require('..')
const httpProxy = require('http-proxy')

require('./cleanup_after_each')()

Expand Down Expand Up @@ -1711,3 +1712,43 @@ test('teardown', t => {
t.deepEqual(leaks, [], 'No leaks')
t.end()
})

test('works when headers removed by http-proxy', t => {
Copy link
Member

Choose a reason for hiding this comment

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

We're in the process of changing the way we test nock–client integration issues. These issues are important, though it's unwieldy to include large-bracket tests in this repo because it doesn't tell nock developers what the problem is. In addition, the internals of client libraries can change. While this is a decent regression test for the integration, it doesn't prevent against regression of the specific behaviors we want to ensure.

The idea is to build nock–client integration tests in separate repositories, which test the latest development version of nock against. We'd really welcome help getting that started! See a discussion in #1427 which goes into more detail about it.

What we need here is a unit test, probably using the http module, which exercises the issue you're seeing, in a way that a future developer working on nock can understand. Is it possible to express the bug that way?

In general we want to do the standard thing, not implement workarounds for libraries, so writing the test using http should help make it clear whether what we're doing here is sufficiently standard. (It sounds like it is, though TBH it's we're all still getting up to speed with the Nock codebase.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough - I think I can probably pull out what http-proxy is doing into the test itself and remove the dependency :).

const serviceScope = nock('http://service', {
badheaders: ['authorization'],
})
.get('/endpoint')
.reply(200)

const proxy = httpProxy.createProxyServer({
prependUrl: false,
})

proxy.on('proxyReq', (proxyReq, req, res) => {
proxyReq.removeHeader('authorization')
})

const server = http.createServer((request, response) => {
proxy.web(request, response, { target: 'http://service' })
})

server.listen(() => {
const options = {
uri: `http://localhost:${server.address().port}/endpoint`,
method: 'GET',
headers: {
authorization: 'blah',
},
}

mikealRequest(options, (err, resp, body) => {
if (err) {
t.error(err)
} else {
t.equal(200, resp.statusCode)
serviceScope.done()
server.close(t.end)
}
})
})
})