fix: allow unmocked when providing literal search params. (#1614)

mastermatt · web-flow · commit f8d6cbbe1472 · 2019-07-15T08:58:06.000-06:00
fixes: #1421 There was inconsistent behavior around how search params were handled when they were provided as part of a URI string to new Interceptors. The issue happen to come to light when `allowUnmocked` was set. The fix normalizes the behavior by refactoring the constructor of Interceptor to look for literal search params. If present, they're stripped from the `path` attribute and set via the `query` method. As part of the work, `Interceptor.query` was modified to accept `URLSearchParams` instances as valid input.
diff --git a/README.md b/README.md
@@ -254,7 +254,15 @@ nock('http://www.example.com')
 
 ### Specifying request query string
 
-Nock understands query strings. Instead of placing the entire URL, you can specify the query part as an object:
+Nock understands query strings. Search parameters can be included as part of the path:
+
+```js
+nock('http://example.com')
+  .get('/users?foo=bar')
+  .reply(200)
+```
+
+Instead of placing the entire URL, you can specify the query part as an object:
 
 ```js
 nock('http://example.com')
@@ -278,6 +286,17 @@ nock('http://example.com')
   .reply(200, { results: [{ id: 'pgte' }] })
 ```
 
+A `URLSearchParams` instance can be provided.
+
+```js
+const params = new URLSearchParams({ foo: 'bar' })
+
+nock('http://example.com')
+  .get('/')
+  .query(params)
+  .reply(200)
+```
+
 Nock supports passing a function to query. The function determines if the actual query matches or not.
 
 ```js
diff --git a/lib/interceptor.js b/lib/interceptor.js
@@ -6,6 +6,7 @@ const _ = require('lodash')
 const debug = require('debug')('nock.interceptor')
 const stringify = require('json-stringify-safe')
 const qs = require('qs')
+const url = require('url')
 
 let fs
 try {
@@ -16,30 +17,40 @@ try {
 
 module.exports = Interceptor
 
+/**
+ *
+ * Valid argument types for `uri`:
+ *  - A string used for strict comparisons with pathname.
+ *    The search portion of the URI may also be postfixed, in which case the search params
+ *    are striped and added via the `query` method.
+ *  - A RegExp instance that tests against only the pathname of requests.
+ *  - A synchronous function bound to this Interceptor instance. It's provided the pathname
+ *    of requests and must return a boolean denoting if the request is considered a match.
+ */
 function Interceptor(scope, uri, method, requestBody, interceptorOptions) {
+  const uriIsStr = typeof uri === 'string'
   // Check for leading slash. Uri can be either a string or a regexp, but
   // we only need to check strings.
-  if (typeof uri === 'string' && /^[^/*]/.test(uri)) {
+  if (uriIsStr && /^[^/*]/.test(uri)) {
     throw Error(
       "Non-wildcard URL path strings must begin with a slash (otherwise they won't match anything)"
     )
   }
 
-  this.scope = scope
-  this.interceptorMatchHeaders = []
-
-  if (typeof method === 'undefined' || !method) {
+  if (!method) {
     throw new Error('The "method" parameter is required for an intercept call.')
   }
+
+  this.scope = scope
+  this.interceptorMatchHeaders = []
   this.method = method.toUpperCase()
   this.uri = uri
   this._key = `${this.method} ${scope.basePath}${scope.basePathname}${
-    typeof uri === 'string' ? '' : '/'
+    uriIsStr ? '' : '/'
   }${uri}`
   this.basePath = this.scope.basePath
-  this.path = typeof uri === 'string' ? scope.basePathname + uri : uri
+  this.path = uriIsStr ? scope.basePathname + uri : uri
 
-  this.baseUri = `${this.method} ${scope.basePath}${scope.basePathname}`
   this.options = interceptorOptions || {}
   this.counter = 1
   this._requestBody = requestBody
@@ -56,6 +67,15 @@ function Interceptor(scope, uri, method, requestBody, interceptorOptions) {
   this.delayConnectionInMs = 0
 
   this.optional = false
+
+  // strip off literal query parameters if they were provided as part of the URI
+  if (uriIsStr && uri.includes('?')) {
+    // localhost is a dummy value because the URL constructor errors for only relative inputs
+    const parsedURL = new url.URL(this.path, 'http://localhost')
+    this.path = parsedURL.pathname
+    this.query(parsedURL.searchParams)
+    this._key = `${this.method} ${scope.basePath}${this.path}`
+  }
 }
 
 Interceptor.prototype.optionally = function optionally(value) {
@@ -463,18 +483,17 @@ Interceptor.prototype.query = function query(queries) {
     return this
   }
 
-  let stringFormattingFn
+  let strFormattingFn
   if (this.scope.scopeOptions.encodedQueryParams) {
-    stringFormattingFn = common.percentDecode
+    strFormattingFn = common.percentDecode
   }
 
-  for (const key in queries) {
-    if (_.isUndefined(this.queries[key])) {
-      const formattedPair = common.formatQueryValue(
-        key,
-        queries[key],
-        stringFormattingFn
-      )
+  const entries =
+    queries instanceof url.URLSearchParams ? queries : Object.entries(queries)
+
+  for (const [key, value] of entries) {
+    if (this.queries[key] === undefined) {
+      const formattedPair = common.formatQueryValue(key, value, strFormattingFn)
       this.queries[formattedPair[0]] = formattedPair[1]
     }
   }
diff --git a/tests/test_allow_unmocked.js b/tests/test_allow_unmocked.js
@@ -259,6 +259,17 @@ test('match multiple paths to domain using regexp with allowUnmocked', async t =
   scope2.done()
 })
 
+test('match domain and path with literal query params and allowUnmocked', async t => {
+  const scope = nock('http://example.test', { allowUnmocked: true })
+    .get('/foo?bar=baz')
+    .reply()
+
+  const { statusCode } = await got('http://example.test/foo?bar=baz')
+
+  t.is(statusCode, 200)
+  scope.done()
+})
+
 test('match domain and path using regexp with query params and allowUnmocked', t => {
   const imgResponse = 'Matched Images Page'
   const opts = { allowUnmocked: true }
diff --git a/tests/test_query.js b/tests/test_query.js
@@ -2,7 +2,9 @@
 
 const mikealRequest = require('request')
 const { test } = require('tap')
+const url = require('url')
 const nock = require('..')
+const got = require('./got_client')
 
 require('./cleanup_after_each')()
 
@@ -32,6 +34,17 @@ test('query() matches multiple query strings of the same name=value', t => {
   })
 })
 
+test('literal query params have the same behavior as calling query() directly', async t => {
+  const scope = nock('http://example.test')
+    .get('/foo?bar=baz')
+    .reply()
+
+  const { statusCode } = await got('http://example.test/foo?bar=baz')
+
+  t.is(statusCode, 200)
+  scope.done()
+})
+
 test('query() matches multiple query strings of the same name=value regardless of order', t => {
   nock('http://example.test')
     .get('/')
@@ -92,6 +105,35 @@ test('query() matches a query string using regexp', t => {
   })
 })
 
+test('query() accepts URLSearchParams as input', async t => {
+  const params = new url.URLSearchParams({
+    foo: 'bar',
+  })
+
+  const scope = nock('http://example.test')
+    .get('/')
+    .query(params)
+    .reply()
+
+  const { statusCode } = await got('http://example.test?foo=bar')
+
+  t.is(statusCode, 200)
+  scope.done()
+})
+
+test('multiple set query keys use the first occurrence', async t => {
+  const scope = nock('http://example.test')
+    .get('/')
+    .query({ foo: 'bar' })
+    .query({ foo: 'baz' })
+    .reply()
+
+  const { statusCode } = await got('http://example.test?foo=bar')
+
+  t.is(statusCode, 200)
+  scope.done()
+})
+
 test('query() matches a query string that contains special RFC3986 characters', t => {
   nock('http://example.test')
     .get('/')
