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.

Sign up for GitHub

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

Jump to bottom

feat: Support http.request signatures added in Node 10.9+ #1588

Merged
merged 10 commits into from
Jul 9, 2019
Merged

feat: Support http.request signatures added in Node 10.9+ #1588

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f598816
Remove unnecessary overrider in intercept.
mastermatt Jun 16, 2019
3c93c3d
Consolidate var declarations in intercept.
mastermatt Jun 16, 2019
e2e59e4
Register ClientRequest callback on response event.
mastermatt Jun 16, 2019
fd54a81
Allow three arg form of `request` and `ClientRequest`.
mastermatt Jun 17, 2019
a03480b
Merge branch 'beta' into support-new-request-signature
paulmelnikow Jul 7, 2019
9dac372
Prefer Object.keys over lodash.isEmpty.
mastermatt Jul 8, 2019
759783a
Apply suggestions from code review
mastermatt Jul 8, 2019
e8e90b5
Update tests per PR feedback.
mastermatt Jul 8, 2019
fdd0688
Better use of .basicAuth for test.
mastermatt Jul 9, 2019
17f203c
Merge remote-tracking branch 'upstream/beta' into support-new-request…
mastermatt Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 70 additions & 16 deletions lib/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const _ = require('lodash')
const debug = require('debug')('nock.common')
const url = require('url')

/**
* Normalizes the request options so that it always has `host` property.
Expand Down Expand Up @@ -93,24 +94,17 @@ const overrideRequests = function(newRequest) {
request: overriddenRequest,
get: overriddenGet,
}

module.request = function(options, callback) {
// debug('request options:', options);
return newRequest(
proto,
overriddenRequest.bind(module),
// https://nodejs.org/api/http.html#http_http_request_url_options_callback
module.request = function(input, options, callback) {
return newRequest(proto, overriddenRequest.bind(module), [
input,
options,
callback
)
callback,
])
}

module.get = function(options, callback) {
const req = newRequest(
proto,
overriddenRequest.bind(module),
options,
callback
)
// https://nodejs.org/api/http.html#http_http_get_options_callback
module.get = function(input, options, callback) {
const req = module.request(input, options, callback)
Copy link

Choose a reason for hiding this comment

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

Why is this not calling newRequest instead? This breaks the Node API as well

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done to mimic native Node behavior in case other libs are monkey-patching the function.
Can you provide more details on "This breaks the Node API as well"? Preferably in a new issue.

Copy link

Choose a reason for hiding this comment

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

I can make a new issue. But writing the details here as well.

New Relic (https://github.com/newrelic/node-newrelic) is overwriting http.get and http.request like you do. But when http.get is calling http.request like here, a call to http.get will call http.request twice when using nock in a New Relic test.

To prove that this is not native Node behaviour (at least on Node 12):

const http = require('http');

function request() {
  console.log('http.request', arguments);
}

http.request = request;

// our request function is not called
http.get('http://google.com', () => {});

req.end()
return req
}
Expand Down Expand Up @@ -490,6 +484,66 @@ function isStream(obj) {
)
}

/**
* Converts the arguments from the various signatures of http[s].request into a standard
* options object and an optional callback function.
*
* https://nodejs.org/api/http.html#http_http_request_url_options_callback
*
* Taken from the beginning of the native `ClientRequest`.
* https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_client.js#L68
*/
function normalizeClientRequestArgs(input, options, cb) {
if (typeof input === 'string') {
input = urlToOptions(new url.URL(input))
} else if (input instanceof url.URL) {
input = urlToOptions(input)
} else {
cb = options
options = input
input = null
}

if (typeof options === 'function') {
cb = options
options = input || {}
} else {
options = Object.assign(input || {}, options)
}

return { options, callback: cb }
}

/**
* Utility function that converts a URL object into an ordinary
* options object as expected by the http.request and https.request APIs.
*
* This was copied from Node's source
* https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/internal/url.js#L1257
*/
function urlToOptions(url) {
const options = {
protocol: url.protocol,
hostname:
typeof url.hostname === 'string' && url.hostname.startsWith('[')
? url.hostname.slice(1, -1)
: url.hostname,
hash: url.hash,
search: url.search,
pathname: url.pathname,
path: `${url.pathname}${url.search || ''}`,
href: url.href,
}
if (url.port !== '') {
options.port = Number(url.port)
}
if (url.username || url.password) {
options.auth = `${url.username}:${url.password}`
}
return options
}

exports.normalizeClientRequestArgs = normalizeClientRequestArgs
exports.normalizeRequestOptions = normalizeRequestOptions
exports.isUtf8Representable = isUtf8Representable
exports.overrideRequests = overrideRequests
Expand Down
60 changes: 23 additions & 37 deletions lib/intercept.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ const common = require('./common')
const { inherits } = require('util')
const Interceptor = require('./interceptor')
const http = require('http')
const { parse: urlParse } = require('url')
const { URL } = require('url')
const _ = require('lodash')
const debug = require('debug')('nock.intercept')
const globalEmitter = require('./global_emitter')
Expand Down Expand Up @@ -147,7 +145,7 @@ function interceptorsFor(options) {

// First try to use filteringScope if any of the interceptors has it defined.
let matchingInterceptor
_.each(allInterceptors, function(interceptor, k) {
_.each(allInterceptors, function(interceptor) {
_.each(interceptor.scopes, function(scope) {
const { filteringScope } = scope.__nock_scopeOptions

Expand Down Expand Up @@ -238,7 +236,7 @@ function ErroringClientRequest(error) {
inherits(ErroringClientRequest, http.ClientRequest)

function overrideClientRequest() {
// Here's some background discussion about overridding ClientRequest:
// Here's some background discussion about overriding ClientRequest:
// - https://github.com/nodejitsu/mock-request/issues/4
// - https://github.com/nock/nock/issues/26
// It would be good to add a comment that explains this more clearly.
Expand All @@ -247,8 +245,10 @@ function overrideClientRequest() {
// ----- Extending http.ClientRequest

// Define the overriding client request that nock uses internally.
function OverriddenClientRequest(options, cb) {
if (!options) {
function OverriddenClientRequest(...args) {
const { options, callback } = common.normalizeClientRequestArgs(...args)

if (Object.keys(options).length === 0) {
// As weird as it is, it's possible to call `http.request` without
// options, and it makes a request to localhost or somesuch. We should
// support it too, for parity. However it doesn't work today, and fixing
Expand All @@ -271,14 +271,12 @@ function overrideClientRequest() {
debug('using', interceptors.length, 'interceptors')

// Use filtered interceptors to intercept requests.
const overrider = RequestOverrider(
this,
options,
interceptors,
remove,
cb
)
const overrider = RequestOverrider(this, options, interceptors, remove)
Object.assign(this, overrider)

if (callback) {
this.once('response', callback)
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

}
} else {
debug('falling back to original ClientRequest')

Expand Down Expand Up @@ -373,20 +371,12 @@ function activate() {

// ----- Overriding http.request and https.request:

common.overrideRequests(function(
proto,
overriddenRequest,
options,
callback
) {
common.overrideRequests(function(proto, overriddenRequest, args) {
// NOTE: overriddenRequest is already bound to its module.
let req, res

if (typeof options === 'string') {
options = urlParse(options)
} else if (options instanceof URL) {
options = urlParse(options.toString())
} else if (!options) {
const { options, callback } = common.normalizeClientRequestArgs(...args)

if (Object.keys(options).length === 0) {
// As weird as it is, it's possible to call `http.request` without
// options, and it makes a request to localhost or somesuch. We should
// support it too, for parity. However it doesn't work today, and fixing
Expand All @@ -399,23 +389,25 @@ function activate() {
'Making a request with empty `options` is not supported in Nock'
)
}

// The option per the docs is `protocol`. Its unclear if this line is meant to override that and is misspelled or if
// the intend is to explicitly keep track of which module was called using a separate name.
// Either way, `proto` is used as the source of truth from here on out.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what we ought to do here but really appreciate this comment!

options.proto = proto

const interceptors = interceptorsFor(options)

if (isOn() && interceptors) {
let matches = false
let allowUnmocked = false

matches = !!_.find(interceptors, function(interceptor) {
const matches = !!_.find(interceptors, function(interceptor) {
return interceptor.matchAddress(options)
})

allowUnmocked = !!_.find(interceptors, function(interceptor) {
const allowUnmocked = !!_.find(interceptors, function(interceptor) {
return interceptor.options.allowUnmocked
})

if (!matches && allowUnmocked) {
let req
if (proto === 'https') {
const { ClientRequest } = http
http.ClientRequest = originalClientRequest
Expand All @@ -430,13 +422,7 @@ function activate() {

// NOTE: Since we already overrode the http.ClientRequest we are in fact constructing
// our own OverriddenClientRequest.
req = new http.ClientRequest(options)

res = RequestOverrider(req, options, interceptors, remove)
paulmelnikow marked this conversation as resolved.
Show resolved Hide resolved
if (callback) {
res.on('response', callback)
}
return req
return new http.ClientRequest(options, callback)
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

} else {
globalEmitter.emit('no match', options)
if (isOff() || isEnabledForNetConnect(options)) {
Expand Down
35 changes: 6 additions & 29 deletions lib/recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
'use strict'

const { inspect } = require('util')
const { parse: urlParse } = require('url')
const common = require('./common')
const intercept = require('./intercept')
const debug = require('debug')('nock.recorder')
const _ = require('lodash')
const URL = require('url')
const qs = require('qs')
const { inspect } = require('util')

const common = require('./common')
const intercept = require('./intercept')

const SEPARATOR = '\n<<<<<<-- cut here -->>>>>>\n'
let recordingInProgress = false
Expand Down Expand Up @@ -235,26 +234,10 @@ function record(recOptions) {
intercept.restoreOverriddenClientRequest()

// We override the requests so that we can save information on them before executing.
common.overrideRequests(function(
proto,
overriddenRequest,
options,
callback
) {
common.overrideRequests(function(proto, overriddenRequest, rawArgs) {
const { options, callback } = common.normalizeClientRequestArgs(...rawArgs)
const bodyChunks = []

if (typeof options === 'string') {
paulmelnikow marked this conversation as resolved.
Show resolved Hide resolved
// TODO-coverage: Investigate why this was added. Add a test if
// possible. If we can't figure it out, remove it.
const url = URL.parse(options)
options = {
hostname: url.hostname,
method: 'GET',
port: url.port,
path: url.path,
}
}

// Node 0.11 https.request calls http.request -- don't want to record things
// twice.
if (options._recording) {
Expand All @@ -265,12 +248,6 @@ function record(recOptions) {
const req = overriddenRequest(options, function(res) {
debug(thisRecordingId, 'intercepting', proto, 'request to record')

// TODO-coverage: Investigate why this was added. Add a test if
// possible. If we can't figure it out, remove it.
if (typeof options === 'string') {
options = urlParse(options)
}

// We put our 'end' listener to the front of the listener array.
res.once('end', function() {
debug(thisRecordingId, proto, 'intercepted request ended')
Expand Down
16 changes: 5 additions & 11 deletions lib/request_overrider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function setRequestHeaders(req, options, interceptor) {
}
}

function RequestOverrider(req, options, interceptors, remove, cb) {
function RequestOverrider(req, options, interceptors, remove) {
const response = new IncomingMessage(new EventEmitter())

const requestBodyBuffers = []
Expand Down Expand Up @@ -144,7 +144,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
if (typeof callback === 'function') {
callback()
}
end(cb)
end()
req.emit('finish')
req.emit('end')
})
Expand All @@ -157,7 +157,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
req.flushHeaders = function() {
debug('req.flushHeaders')
if (!req.aborted && !ended) {
end(cb)
end()
}
if (req.aborted) {
emitError(new Error('Request aborted'))
Expand Down Expand Up @@ -214,7 +214,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
})
}

const end = function(cb) {
const end = function() {
debug('ending')
ended = true
let requestBody, responseBody, responseBuffers, interceptor
Expand Down Expand Up @@ -257,7 +257,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
})
if (interceptor && req instanceof ClientRequest) {
if (interceptor.options.allowUnmocked) {
const newReq = new ClientRequest(options, cb)
const newReq = new ClientRequest(options)
propagate(newReq, req)
// We send the raw buffer as we received it, not as we interpreted it.
newReq.end(requestBodyBuffer)
Expand Down Expand Up @@ -529,12 +529,6 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
}

debug('emitting response')

if (typeof cb === 'function') {
debug('callback with response')
cb(response)
}

req.emit('response', response)

if (common.isStream(responseBody)) {
Expand Down
25 changes: 25 additions & 0 deletions tests/test_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,3 +453,28 @@ test('percentEncode encodes extra reserved characters', t => {
t.equal(common.percentEncode('foo+(*)!'), 'foo%2B%28%2A%29%21')
t.done()
})

test('normalizeClientRequestArgs throws for invalid URL', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

@gr2m has expressed a preference for testing our helper functions through real use cases in the public API wherever possible, and it seems to pay off well. It also has the advantage of surfacing code that is no longer needed.

Would you be up for expressing these tests in terms of calls to http?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please 👍

// no schema
t.throws(() => common.normalizeClientRequestArgs('example.test'), {
input: 'example.test',
name: /TypeError/,
})
})

test('normalizeClientRequestArgs can include auth info', async t => {
const { options } = common.normalizeClientRequestArgs(
'https://user:pw@example.test'
)

t.equal(options.auth, 'user:pw')
})

test('normalizeClientRequestArgs with a single callback', async t => {
const cb = () => {}

const { options, callback } = common.normalizeClientRequestArgs(cb)

t.deepEqual(options, {})
t.is(callback, cb)
})