Repository: node-nock/nock
Pull request #1015 (2017-11-02 19:43:13)

fix: Encoding query parameters when recording to string.
I stumbled upon a bug when trying to record and reuse a recording that was using query parameters with the `+` sign. The problem is that the recording, when generated using `generateRequestAndResponse`, [always sets `encodedQueryParams: true`](https://github.com/node-nock/nock/blob/master/lib/recorder.js#L126). When reusing those recordings, [the `+` sign is then interpreted as a space](https://github.com/node-nock/nock/blob/master/lib/common.js#L312), and so it is never able to reuse that recording for the same query.

I added a test to make sure that the query parameters are encoded when using `generateRequestAndResponse`, and tested that it does indeed allows to replay those requests when a `+` sign is in the query.

Thanks for reviewing!

ierceg: @louib Thank you for creating a PR to solve this issue. If I understand it correctly this would eliminate the support for multi-value query parameters and there is also a change in the current behavior for which there is no test (the added `return this`)
louib: @ierceg thanks for the review! I answered directly on your comments.
louib: @gr2m @ierceg I reduced the size of the changes to fix the issue. Can you give this PR another review?

Patch:
diff --git a/README.md b/README.md
index 0740f21..4bf689e 100644
--- a/README.md
+++ b/README.md
@@ -1082,7 +1082,7 @@ nock.enableNetConnect();
 
 This is a cool feature:
 
-Guessing what the HTTP calls are is a mess, specially if you are introducing nock on your already-coded tests.
+Guessing what the HTTP calls are is a mess, especially if you are introducing nock on your already-coded tests.
 
 For these cases where you want to mock an existing live system you can record and playback the HTTP calls like this:
 
diff --git a/lib/common.js b/lib/common.js
index 9d6be55..932259e 100644
--- a/lib/common.js
+++ b/lib/common.js
@@ -327,20 +327,29 @@ function matchStringOrRegexp(target, pattern) {
   return pattern instanceof RegExp  ? str.match(pattern) : str === String(pattern);
 }
 
-// return [newKey, newValue]
-function formatQueryValue(key, value, options) {
+/**
+ * Formats a query parameter.
+ *
+ * @param key                The key of the query parameter to format.
+ * @param value              The value of the query parameter to format.
+ * @param stringFormattingFn The function used to format string values. Can
+ *                           be used to encode or decode the query value.
+ *
+ * @returns the formatted [key, value] pair.
+ */
+function formatQueryValue(key, value, stringFormattingFn) {
   switch (true) {
-    case _.isNumber(value): // fall-though
+    case _.isNumber(value): // fall-through
     case _.isBoolean(value):
       value = value.toString();
       break;
-    case _.isUndefined(value): // fall-though
+    case _.isUndefined(value): // fall-through
     case _.isNull(value):
       value = '';
       break;
     case _.isString(value):
-      if(options.encodedQueryParams) {
-        value = percentDecode(value);
+      if(stringFormattingFn) {
+        value = stringFormattingFn(value);
       }
       break;
     case (value instanceof RegExp):
@@ -348,21 +357,21 @@ function formatQueryValue(key, value, options) {
     case _.isArray(value):
       var tmpArray = new Array(value.length);
       for (var i = 0; i < value.length; ++i) {
-        tmpArray[i] = formatQueryValue(i, value[i], options)[1];
+        tmpArray[i] = formatQueryValue(i, value[i], stringFormattingFn)[1];
       }
       value = tmpArray;
       break;
     case _.isObject(value):
       var tmpObj = {};
       _.forOwn(value, function(subVal, subKey){
-        var subPair = formatQueryValue(subKey, subVal, options);
+        var subPair = formatQueryValue(subKey, subVal, stringFormattingFn);
         tmpObj[subPair[0]] = subPair[1];
       });
       value = tmpObj;
       break;
   }
 
-  if (options.encodedQueryParams) key = percentDecode(key);
+  if (stringFormattingFn) key = stringFormattingFn(key);
   return [key, value];
 }
 
diff --git a/lib/interceptor.js b/lib/interceptor.js
index e670b74..585dfed 100755
--- a/lib/interceptor.js
+++ b/lib/interceptor.js
@@ -410,9 +410,11 @@ Interceptor.prototype.basicAuth = function basicAuth(options) {
  */
 Interceptor.prototype.query = function query(queries) {
     this.queries = this.queries || {};
+
     // Allow all query strings to match this route
     if (queries === true) {
         this.queries = queries;
+        return this;
     }
 
     if (_.isFunction(queries)) {
@@ -420,11 +422,15 @@ Interceptor.prototype.query = function query(queries) {
         return this;
     }
 
-    for (var q in queries) {
-        if (_.isUndefined(this.queries[q])) {
-            var value = queries[q];
-            var formatedPair = common.formatQueryValue(q, value, this.scope.scopeOptions);
-            this.queries[formatedPair[0]] = formatedPair[1];
+    var stringFormattingFn;
+    if (this.scope.scopeOptions.encodedQueryParams) {
+        stringFormattingFn = common.percentDecode;
+    }
+
+    for (var key in queries) {
+        if (_.isUndefined(this.queries[key])) {
+            var formattedPair = common.formatQueryValue(key, queries[key], stringFormattingFn);
+            this.queries[formattedPair[0]] = formattedPair[1];
         }
     }
 
diff --git a/lib/match_body.js b/lib/match_body.js
index e909fb6..eec18aa 100644
--- a/lib/match_body.js
+++ b/lib/match_body.js
@@ -1,7 +1,7 @@
 'use strict';
 
 var deepEqual = require('deep-equal');
-var qs = require('querystring');
+var qs = require('qs');
 
 module.exports =
 function matchBody(spec, body) {
diff --git a/lib/recorder.js b/lib/recorder.js
index e98474d..43bd74b 100644
--- a/lib/recorder.js
+++ b/lib/recorder.js
@@ -118,6 +118,12 @@ function generateRequestAndResponse(req, bodyChunks, options, res, dataChunks) {
     var queryStr = req.path.slice(queryIndex + 1);
     queryObj = qs.parse(queryStr);
   }
+  // Always encoding the query parameters when recording.
+  var encodedQueryObj = {};
+  for (var key in queryObj) {
+    var formattedPair = common.formatQueryValue(key, queryObj[key], common.percentEncode);
+    encodedQueryObj[formattedPair[0]] = formattedPair[1];
+  }
 
   var ret = [];
   ret.push('\nnock(\'');
@@ -143,7 +149,7 @@ function generateRequestAndResponse(req, bodyChunks, options, res, dataChunks) {
 
   if (queryIndex !== -1) {
     ret.push('  .query(');
-    ret.push(JSON.stringify(queryObj));
+    ret.push(JSON.stringify(encodedQueryObj));
     ret.push(')\n');
   }
 
diff --git a/package.json b/package.json
index f38248e..93b512e 100644
--- a/package.json
+++ b/package.json
@@ -174,7 +174,7 @@
     "lodash": "~4.17.2",
     "mkdirp": "^0.5.0",
     "propagate": "0.4.0",
-    "qs": "^6.0.2",
+    "qs": "^6.5.1",
     "semver": "^5.3.0"
   },
   "devDependencies": {


Test patch:
diff --git a/tests/test_body_match.js b/tests/test_body_match.js
index 96a1d60..b97b07b 100644
--- a/tests/test_body_match.js
+++ b/tests/test_body_match.js
@@ -88,6 +88,27 @@ test('match body with empty object inside', function (t) {
   });
 })
 
+test('match body with nested object inside', function (t) {
+
+  nock('http://encodingsareus.com')
+    .post('/', /x/)
+    .reply(200);
+
+  mikealRequest({
+    url: 'http://encodingsareus.com/',
+    method: 'post',
+    json: {
+      obj: {
+        x: 1
+      },
+    },
+  }, function(err, res) {
+    if (err) throw err;
+    assert.equal(res.statusCode, 200);
+    t.end();
+  });
+})
+
 test('doesn\'t match body with mismatching keys', function (t) {
   nock('http://encodingsareus.com')
     .post('/', { a: 'a' })
diff --git a/tests/test_recorder.js b/tests/test_recorder.js
index 9f8ddc6..03ec045 100644
--- a/tests/test_recorder.js
+++ b/tests/test_recorder.js
@@ -719,6 +719,29 @@ test('includes query parameters from superagent', {skip: process.env.AIRPLANE},
     });
 });
 
+test('encodes the query parameters when not outputing objects', {skip: process.env.AIRPLANE}, function(t) {
+
+  nock.restore();
+  nock.recorder.clear();
+  t.equal(nock.recorder.play().length, 0);
+
+  nock.recorder.rec({
+    dont_print: true,
+    output_objects: false
+  });
+
+  superagent.get('http://google.com')
+    .query({q: 'test search++' })
+    .end(function(res) {
+      nock.restore();
+      var recording = nock.recorder.play();
+      t.true(recording.length >= 1);
+      t.true(recording[0].indexOf('test%20search%2B%2B') !== -1);
+      t.end();
+    });
+
+});
+
 test('works with clients listening for readable', {skip: process.env.AIRPLANE}, function(t) {
   nock.restore();
   nock.recorder.clear();
@@ -838,7 +861,7 @@ test('outputs query string arrays correctly', {skip: process.env.AIRPLANE}, func
   });
 });
 
-test('removes query params from from that path and puts them in query()', {skip: process.env.AIRPLANE}, function(t) {
+test('removes query params from that path and puts them in query()', {skip: process.env.AIRPLANE}, function(t) {
   nock.restore();
   nock.recorder.clear();
   t.equal(nock.recorder.play().length, 0);