From e3e6a65e796ad777e4d41feffbdae5ec41eebd81 Mon Sep 17 00:00:00 2001
From: "Matt R. Wilson" <github.public@mattw.co>
Date: Mon, 8 Jul 2019 23:32:38 -0600
Subject: [PATCH] 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.
---
 lib/common.js                   | 86 +++++++++++++++++++++++++++------
 lib/intercept.js                | 60 +++++++++--------------
 lib/recorder.js                 | 35 +++-----------
 lib/request_overrider.js        | 16 ++----
 tests/test_common.js            | 30 ++++++++++++
 tests/test_intercept.js         | 32 ++++++++++++
 tests/test_request_overrider.js | 13 +++++
 7 files changed, 179 insertions(+), 93 deletions(-)

diff --git a/lib/common.js b/lib/common.js
index 8d15209ed..439917f0c 100644
--- 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.
@@ -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)
       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
index b3ff491b3..3fdeaefe7 100644
--- 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
 
@@ -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.
@@ -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
@@ -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)
+      }
     } else {
       debug('falling back to original ClientRequest')
 
@@ -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
@@ -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.
     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
@@ -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)
-      if (callback) {
-        res.on('response', callback)
-      }
-      return req
+      return new http.ClientRequest(options, callback)
     } else {
       globalEmitter.emit('no match', options)
       if (isOff() || isEnabledForNetConnect(options)) {
diff --git a/lib/recorder.js b/lib/recorder.js
index 777169064..203385a86 100644
--- a/lib/recorder.js
+++ b/lib/recorder.js
@@ -4,14 +4,13 @@
 // refactoring within this file, these lines should be cleaned up.
 // https://github.com/nock/nock/issues/1607
 
-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
@@ -243,26 +242,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.
     /* istanbul ignore if */
@@ -274,12 +257,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/lib/request_overrider.js b/lib/request_overrider.js
index f6ebf8c37..3ba9f3fb1 100644
--- a/lib/request_overrider.js
+++ b/lib/request_overrider.js
@@ -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 = []
@@ -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')
       })
@@ -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'))
@@ -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
@@ -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)
@@ -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)) {
diff --git a/tests/test_common.js b/tests/test_common.js
index 9d6eec664..61df01318 100644
--- a/tests/test_common.js
+++ b/tests/test_common.js
@@ -1,5 +1,6 @@
 'use strict'
 
+const http = require('http')
 const { test } = require('tap')
 const common = require('../lib/common')
 const matchBody = require('../lib/match_body')
@@ -453,3 +454,32 @@ 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(() => http.get('example.test'), {
+    input: 'example.test',
+    name: /TypeError/,
+  })
+})
+
+test('normalizeClientRequestArgs can include auth info', async () => {
+  const scope = nock('http://example.test')
+    .get('/')
+    .basicAuth({ user: 'user', pass: 'pw' })
+    .reply()
+
+  http.get('http://user:pw@example.test')
+  scope.isDone()
+})
+
+test('normalizeClientRequestArgs with a single callback', async t => {
+  // TODO: Only passing a callback isn't currently supported by Nock,
+  // but should be in the future as Node allows it.
+  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
index 9f662e48d..749305d61 100644
--- 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,34 @@ 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()
+
+  http.get(new url.URL('http://example.test'), () => {
+    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()
+    })
+  })
+})
diff --git a/tests/test_request_overrider.js b/tests/test_request_overrider.js
index 4c105f74a..ed78e956f 100644
--- a/tests/test_request_overrider.js
+++ b/tests/test_request_overrider.js
@@ -38,6 +38,19 @@ test('response is an http.IncomingMessage instance', t => {
     .end()
 })
 
+test('emits the response event', t => {
+  const scope = nock('http://example.test')
+    .get('/')
+    .reply()
+
+  const req = http.get('http://example.test')
+
+  req.on('response', () => {
+    scope.done()
+    t.end()
+  })
+})
+
 test('write callback called', t => {
   const scope = nock('http://filterboddiezregexp.com')
     .filteringRequestBody(/mia/, 'nostra')