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.
nock · Jun 18, 2019 · 9a7ec1b · 9a7ec1b
1 parent 25d6017
commit 9a7ec1b
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 62 deletions.
diff --git a/lib/back.js b/lib/back.js
@@ -53,7 +53,7 @@ function Back(fixtureName, options, nockedFn) {
     throw new Error(
       'Back requires nock.back.fixtures to be set\n' +
         'Ex:\n' +
-        "\trequire(nock).back.fixtures = '/path/to/fixures/'"
+        "\trequire(nock).back.fixtures = '/path/to/fixtures/'"
     )
   }
 

diff --git a/lib/common.js b/lib/common.js
@@ -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.
@@ -94,23 +95,16 @@ const overrideRequests = function(newRequest) {
       get: overriddenGet,
     }
 
-    module.request = function(options, callback) {
-      // debug('request options:', options);
-      return newRequest(
-        proto,
-        overriddenRequest.bind(module),
+    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
-      )
+    module.get = function(input, options, callback) {
+      const req = module.request(input, options, callback)
       req.end()
       return req
     }
@@ -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

diff --git a/lib/intercept.js b/lib/intercept.js
@@ -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')
@@ -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
 
@@ -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 (_.isEmpty(options)) {
       // 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
@@ -278,8 +278,8 @@ function overrideClientRequest() {
         }
       }
 
-      if (cb) {
-        this.once('response', cb)
+      if (callback) {
+        this.once('response', callback)
       }
     } else {
       debug('falling back to original ClientRequest')
@@ -375,19 +375,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.
 
-    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 (_.isEmpty(options)) {
       // 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
@@ -400,6 +393,10 @@ 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.
     options.proto = proto
 
     const interceptors = interceptorsFor(options)

diff --git a/lib/recorder.js b/lib/recorder.js
@@ -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
@@ -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') {
-      // 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) {
@@ -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')

diff --git a/tests/test_common.js b/tests/test_common.js
@@ -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 => {
+  // 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)
+})
diff --git a/tests/test_intercept.js b/tests/test_intercept.js
@@ -8,6 +8,7 @@ const superagent = require('superagent')
 const restify = require('restify-clients')
 const assertRejects = require('assert-rejects')
 const hyperquest = require('hyperquest')
+const url = require('url')
 const nock = require('..')
 const got = require('./got_client')
 
@@ -1308,3 +1309,36 @@ test('no new keys were added to the global namespace', t => {
   t.deepEqual(leaks, [], 'No leaks')
   t.end()
 })
+
+// These tests use `http` directly because `got` never calls `http` with the three arg form
+test('first arg as URL instance', t => {
+  const scope = nock('http://example.test')
+    .get('/')
+    .reply(201, 'this is data')
+
+  const req = http.get(new url.URL('http://example.test'))
+
+  req.on('response', () => {
+    scope.done()
+    t.end()
+  })
+})
+
+test('three argument form of http.request: URL, options, and callback', t => {
+  t.plan(2)
+
+  const scope = nock('http://example.test')
+    .get('/hello')
+    .reply(201, 'this is data')
+
+  http.get(new url.URL('http://example.test'), { path: '/hello' }, res => {
+    t.equal(res.statusCode, 201)
+    res.on('data', chunk => {
+      t.equal(chunk.toString(), 'this is data')
+    })
+    res.on('end', () => {
+      scope.done()
+      t.done()
+    })
+  })
+})