Skip to content

Commit e3e6a65

Browse files
mastermattmastermatt
authored
feat: Support http.request signatures added in Node 10.9+ (#1588)
* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.
1 parent 2bca61b commit e3e6a65

File tree

7 files changed

+179
-93
lines changed

7 files changed

+179
-93
lines changed

‎lib/common.js‎

Lines changed: 70 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
const _ = require('lodash')
44
const debug = require('debug')('nock.common')
5+
const url = require('url')
56

67
/**
78
* Normalizes the request options so that it always has `host` property.
@@ -93,24 +94,17 @@ const overrideRequests = function(newRequest) {
9394
request: overriddenRequest,
9495
get: overriddenGet,
9596
}
96-
97-
module.request = function(options, callback) {
98-
// debug('request options:', options);
99-
return newRequest(
100-
proto,
101-
overriddenRequest.bind(module),
97+
// https://nodejs.org/api/http.html#http_http_request_url_options_callback
98+
module.request = function(input, options, callback) {
99+
return newRequest(proto, overriddenRequest.bind(module), [
100+
input,
102101
options,
103-
callback
104-
)
102+
callback,
103+
])
105104
}
106-
107-
module.get = function(options, callback) {
108-
const req = newRequest(
109-
proto,
110-
overriddenRequest.bind(module),
111-
options,
112-
callback
113-
)
105+
// https://nodejs.org/api/http.html#http_http_get_options_callback
106+
module.get = function(input, options, callback) {
107+
const req = module.request(input, options, callback)
114108
req.end()
115109
return req
116110
}
@@ -490,6 +484,66 @@ function isStream(obj) {
490484
)
491485
}
492486

487+
/**
488+
* Converts the arguments from the various signatures of http[s].request into a standard
489+
* options object and an optional callback function.
490+
*
491+
* https://nodejs.org/api/http.html#http_http_request_url_options_callback
492+
*
493+
* Taken from the beginning of the native `ClientRequest`.
494+
* https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_client.js#L68
495+
*/
496+
function normalizeClientRequestArgs(input, options, cb) {
497+
if (typeof input === 'string') {
498+
input = urlToOptions(new url.URL(input))
499+
} else if (input instanceof url.URL) {
500+
input = urlToOptions(input)
501+
} else {
502+
cb = options
503+
options = input
504+
input = null
505+
}
506+
507+
if (typeof options === 'function') {
508+
cb = options
509+
options = input || {}
510+
} else {
511+
options = Object.assign(input || {}, options)
512+
}
513+
514+
return { options, callback: cb }
515+
}
516+
517+
/**
518+
* Utility function that converts a URL object into an ordinary
519+
* options object as expected by the http.request and https.request APIs.
520+
*
521+
* This was copied from Node's source
522+
* https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/internal/url.js#L1257
523+
*/
524+
function urlToOptions(url) {
525+
const options = {
526+
protocol: url.protocol,
527+
hostname:
528+
typeof url.hostname === 'string' && url.hostname.startsWith('[')
529+
? url.hostname.slice(1, -1)
530+
: url.hostname,
531+
hash: url.hash,
532+
search: url.search,
533+
pathname: url.pathname,
534+
path: `${url.pathname}${url.search || ''}`,
535+
href: url.href,
536+
}
537+
if (url.port !== '') {
538+
options.port = Number(url.port)
539+
}
540+
if (url.username || url.password) {
541+
options.auth = `${url.username}:${url.password}`
542+
}
543+
return options
544+
}
545+
546+
exports.normalizeClientRequestArgs = normalizeClientRequestArgs
493547
exports.normalizeRequestOptions = normalizeRequestOptions
494548
exports.isUtf8Representable = isUtf8Representable
495549
exports.overrideRequests = overrideRequests

‎lib/intercept.js‎

Lines changed: 23 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ const common = require('./common')
99
const { inherits } = require('util')
1010
const Interceptor = require('./interceptor')
1111
const http = require('http')
12-
const { parse: urlParse } = require('url')
13-
const { URL } = require('url')
1412
const _ = require('lodash')
1513
const debug = require('debug')('nock.intercept')
1614
const globalEmitter = require('./global_emitter')
@@ -147,7 +145,7 @@ function interceptorsFor(options) {
147145

148146
// First try to use filteringScope if any of the interceptors has it defined.
149147
let matchingInterceptor
150-
_.each(allInterceptors, function(interceptor, k) {
148+
_.each(allInterceptors, function(interceptor) {
151149
_.each(interceptor.scopes, function(scope) {
152150
const { filteringScope } = scope.__nock_scopeOptions
153151

@@ -238,7 +236,7 @@ function ErroringClientRequest(error) {
238236
inherits(ErroringClientRequest, http.ClientRequest)
239237

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

249247
// Define the overriding client request that nock uses internally.
250-
function OverriddenClientRequest(options, cb) {
251-
if (!options) {
248+
function OverriddenClientRequest(...args) {
249+
const { options, callback } = common.normalizeClientRequestArgs(...args)
250+
251+
if (Object.keys(options).length === 0) {
252252
// As weird as it is, it's possible to call `http.request` without
253253
// options, and it makes a request to localhost or somesuch. We should
254254
// support it too, for parity. However it doesn't work today, and fixing
@@ -271,14 +271,12 @@ function overrideClientRequest() {
271271
debug('using', interceptors.length, 'interceptors')
272272

273273
// Use filtered interceptors to intercept requests.
274-
const overrider = RequestOverrider(
275-
this,
276-
options,
277-
interceptors,
278-
remove,
279-
cb
280-
)
274+
const overrider = RequestOverrider(this, options, interceptors, remove)
281275
Object.assign(this, overrider)
276+
277+
if (callback) {
278+
this.once('response', callback)
279+
}
282280
} else {
283281
debug('falling back to original ClientRequest')
284282

@@ -373,20 +371,12 @@ function activate() {
373371

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

376-
common.overrideRequests(function(
377-
proto,
378-
overriddenRequest,
379-
options,
380-
callback
381-
) {
374+
common.overrideRequests(function(proto, overriddenRequest, args) {
382375
// NOTE: overriddenRequest is already bound to its module.
383-
let req, res
384376

385-
if (typeof options === 'string') {
386-
options = urlParse(options)
387-
} else if (options instanceof URL) {
388-
options = urlParse(options.toString())
389-
} else if (!options) {
377+
const { options, callback } = common.normalizeClientRequestArgs(...args)
378+
379+
if (Object.keys(options).length === 0) {
390380
// As weird as it is, it's possible to call `http.request` without
391381
// options, and it makes a request to localhost or somesuch. We should
392382
// support it too, for parity. However it doesn't work today, and fixing
@@ -399,23 +389,25 @@ function activate() {
399389
'Making a request with empty `options` is not supported in Nock'
400390
)
401391
}
392+
393+
// The option per the docs is `protocol`. Its unclear if this line is meant to override that and is misspelled or if
394+
// the intend is to explicitly keep track of which module was called using a separate name.
395+
// Either way, `proto` is used as the source of truth from here on out.
402396
options.proto = proto
403397

404398
const interceptors = interceptorsFor(options)
405399

406400
if (isOn() && interceptors) {
407-
let matches = false
408-
let allowUnmocked = false
409-
410-
matches = !!_.find(interceptors, function(interceptor) {
401+
const matches = !!_.find(interceptors, function(interceptor) {
411402
return interceptor.matchAddress(options)
412403
})
413404

414-
allowUnmocked = !!_.find(interceptors, function(interceptor) {
405+
const allowUnmocked = !!_.find(interceptors, function(interceptor) {
415406
return interceptor.options.allowUnmocked
416407
})
417408

418409
if (!matches && allowUnmocked) {
410+
let req
419411
if (proto === 'https') {
420412
const { ClientRequest } = http
421413
http.ClientRequest = originalClientRequest
@@ -430,13 +422,7 @@ function activate() {
430422

431423
// NOTE: Since we already overrode the http.ClientRequest we are in fact constructing
432424
// our own OverriddenClientRequest.
433-
req = new http.ClientRequest(options)
434-
435-
res = RequestOverrider(req, options, interceptors, remove)
436-
if (callback) {
437-
res.on('response', callback)
438-
}
439-
return req
425+
return new http.ClientRequest(options, callback)
440426
} else {
441427
globalEmitter.emit('no match', options)
442428
if (isOff() || isEnabledForNetConnect(options)) {

‎lib/recorder.js‎

Lines changed: 6 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,13 @@
44
// refactoring within this file, these lines should be cleaned up.
55
// https://github.com/nock/nock/issues/1607
66

7-
const { inspect } = require('util')
8-
const { parse: urlParse } = require('url')
9-
const common = require('./common')
10-
const intercept = require('./intercept')
117
const debug = require('debug')('nock.recorder')
128
const _ = require('lodash')
13-
const URL = require('url')
149
const qs = require('qs')
10+
const { inspect } = require('util')
11+
12+
const common = require('./common')
13+
const intercept = require('./intercept')
1514

1615
const SEPARATOR = '\n<<<<<<-- cut here -->>>>>>\n'
1716
let recordingInProgress = false
@@ -243,26 +242,10 @@ function record(recOptions) {
243242
intercept.restoreOverriddenClientRequest()
244243

245244
// We override the requests so that we can save information on them before executing.
246-
common.overrideRequests(function(
247-
proto,
248-
overriddenRequest,
249-
options,
250-
callback
251-
) {
245+
common.overrideRequests(function(proto, overriddenRequest, rawArgs) {
246+
const { options, callback } = common.normalizeClientRequestArgs(...rawArgs)
252247
const bodyChunks = []
253248

254-
if (typeof options === 'string') {
255-
// TODO-coverage: Investigate why this was added. Add a test if
256-
// possible. If we can't figure it out, remove it.
257-
const url = URL.parse(options)
258-
options = {
259-
hostname: url.hostname,
260-
method: 'GET',
261-
port: url.port,
262-
path: url.path,
263-
}
264-
}
265-
266249
// Node 0.11 https.request calls http.request -- don't want to record things
267250
// twice.
268251
/* istanbul ignore if */
@@ -274,12 +257,6 @@ function record(recOptions) {
274257
const req = overriddenRequest(options, function(res) {
275258
debug(thisRecordingId, 'intercepting', proto, 'request to record')
276259

277-
// TODO-coverage: Investigate why this was added. Add a test if
278-
// possible. If we can't figure it out, remove it.
279-
if (typeof options === 'string') {
280-
options = urlParse(options)
281-
}
282-
283260
// We put our 'end' listener to the front of the listener array.
284261
res.once('end', function() {
285262
debug(thisRecordingId, proto, 'intercepted request ended')

‎lib/request_overrider.js‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ function setRequestHeaders(req, options, interceptor) {
5757
}
5858
}
5959

60-
function RequestOverrider(req, options, interceptors, remove, cb) {
60+
function RequestOverrider(req, options, interceptors, remove) {
6161
const response = new IncomingMessage(new EventEmitter())
6262

6363
const requestBodyBuffers = []
@@ -144,7 +144,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
144144
if (typeof callback === 'function') {
145145
callback()
146146
}
147-
end(cb)
147+
end()
148148
req.emit('finish')
149149
req.emit('end')
150150
})
@@ -157,7 +157,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
157157
req.flushHeaders = function() {
158158
debug('req.flushHeaders')
159159
if (!req.aborted && !ended) {
160-
end(cb)
160+
end()
161161
}
162162
if (req.aborted) {
163163
emitError(new Error('Request aborted'))
@@ -214,7 +214,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
214214
})
215215
}
216216

217-
const end = function(cb) {
217+
const end = function() {
218218
debug('ending')
219219
ended = true
220220
let requestBody, responseBody, responseBuffers, interceptor
@@ -257,7 +257,7 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
257257
})
258258
if (interceptor && req instanceof ClientRequest) {
259259
if (interceptor.options.allowUnmocked) {
260-
const newReq = new ClientRequest(options, cb)
260+
const newReq = new ClientRequest(options)
261261
propagate(newReq, req)
262262
// We send the raw buffer as we received it, not as we interpreted it.
263263
newReq.end(requestBodyBuffer)
@@ -529,12 +529,6 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
529529
}
530530

531531
debug('emitting response')
532-
533-
if (typeof cb === 'function') {
534-
debug('callback with response')
535-
cb(response)
536-
}
537-
538532
req.emit('response', response)
539533

540534
if (common.isStream(responseBody)) {

‎tests/test_common.js‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
'use strict'
22

3+
const http = require('http')
34
const { test } = require('tap')
45
const common = require('../lib/common')
56
const matchBody = require('../lib/match_body')
@@ -453,3 +454,32 @@ test('percentEncode encodes extra reserved characters', t => {
453454
t.equal(common.percentEncode('foo+(*)!'), 'foo%2B%28%2A%29%21')
454455
t.done()
455456
})
457+
458+
test('normalizeClientRequestArgs throws for invalid URL', async t => {
459+
// no schema
460+
t.throws(() => http.get('example.test'), {
461+
input: 'example.test',
462+
name: /TypeError/,
463+
})
464+
})
465+
466+
test('normalizeClientRequestArgs can include auth info', async () => {
467+
const scope = nock('http://example.test')
468+
.get('/')
469+
.basicAuth({ user: 'user', pass: 'pw' })
470+
.reply()
471+
472+
http.get('http://user:pw@example.test')
473+
scope.isDone()
474+
})
475+
476+
test('normalizeClientRequestArgs with a single callback', async t => {
477+
// TODO: Only passing a callback isn't currently supported by Nock,
478+
// but should be in the future as Node allows it.
479+
const cb = () => {}
480+
481+
const { options, callback } = common.normalizeClientRequestArgs(cb)
482+
483+
t.deepEqual(options, {})
484+
t.is(callback, cb)
485+
})

0 commit comments

Comments
 (0)