Repository: node-nock/nock
Pull request #917 (2017-05-26 03:52:55)

Fix recording query strings, where value is an array
This PR fixes an issue when using arrays inside a query string.

Prior to this change when using the following query string via superagent we get...
`.query({"foo":["bar","baz"]})`
The result is...
`.query({"foo":"baz"})`

Essentially the last value overrides any prior values due to the way the query string object is being formed. The fix is to simply use the `qs` library's `.parse` function which makes sense since the library is using `qs` elsewhere.

The result then becomes...
`.query({"foo":["bar","baz"]})`
Which now matches the superagent query input.

Resolves https://github.com/node-nock/nock/issues/896

coveralls: 
[![Coverage Status](https://coveralls.io/builds/11698126/badge)](https://coveralls.io/builds/11698126)

Coverage decreased (-0.005%) to 92.681% when pulling **aa6053c10be6d370ea6da7f7d3d0be61ca970479 on s-taylor:master** into **17d7c25682ea520b0a255e933b64d5c7f355330f on node-nock:master**.

s-taylor: BTW I think the coverage error doesn't make sense, it's probably because I deleted lines. Which has reduced the overall number of lines and therefore makes it look as though the coverage has dropped.
s-taylor: Any updates on this? I've had to use my forked version as recording doesn't match for us without this.
travi: i'd really appreciate getting this merged as well since this is the behavior that is expected for my project as well
paulmelnikow: The bug being fixed is that duplicate keys in the query string e.g. `?a=1&a=2&a=3` are lost. Is that correct?
s-taylor: Correct, it only records one value. The fix records this as an array.
paulmelnikow: Makes sense. Though the fix makes sense to me, the test seems very indirect. Could it be written in a simpler way?
eddiemoore: Would love to see this in nock as well. 
eddiemoore: Really like to see this in nock as well.
gr2m: I’ve rebased the PR on the latest master.

I agree with @paulmelnikow that the test seems indirect, but it very much follows the conventions of the other tests around it. About time to clean them up for good, but that’s out of scope. Let’s wait to see if CI is happy and if it is, let’s merge it in
coveralls: 
[![Coverage Status](https://coveralls.io/builds/13468642/badge)](https://coveralls.io/builds/13468642)

Coverage decreased (-0.005%) to 92.666% when pulling **dfa7dde573ac69639cfbaea95f24a9f8ea8c4222 on s-taylor:master** into **b050c492e34061ff8209a300d32359efc98dbda9 on node-nock:master**.


Patch:
diff --git a/lib/recorder.js b/lib/recorder.js
index 9881d3d..e98474d 100644
--- a/lib/recorder.js
+++ b/lib/recorder.js
@@ -8,6 +8,7 @@ var debug = require('debug')('nock.recorder');
 var _ = require('lodash');
 var URL = require('url');
 var semver = require('semver')
+var qs = require('qs');
 
 var SEPARATOR = '\n<<<<<<-- cut here -->>>>>>\n';
 var recordingInProgress = false;
@@ -110,15 +111,12 @@ function generateRequestAndResponse(req, bodyChunks, options, res, dataChunks) {
   var queryIndex = 0;
   var queryObj = {};
   if ((queryIndex = req.path.indexOf('?')) !== -1) {
+    // Remove the query from the path
     path = path.substring(0, queryIndex);
 
     // Create the query() object
-    var queries = req.path.slice(queryIndex + 1).split('&');
-
-    for (var i = 0; i < queries.length; i++) {
-      var query = queries[i].split('=');
-      queryObj[query[0]] = query[1];
-    }
+    var queryStr = req.path.slice(queryIndex + 1);
+    queryObj = qs.parse(queryStr);
   }
 
   var ret = [];


Test patch:
diff --git a/tests/test_recorder.js b/tests/test_recorder.js
index 1030c70..9f8ddc6 100644
--- a/tests/test_recorder.js
+++ b/tests/test_recorder.js
@@ -810,6 +810,34 @@ test('outputs query string parameters using query()', {skip: process.env.AIRPLAN
   });
 });
 
+test('outputs query string arrays correctly', {skip: process.env.AIRPLANE}, function(t) {
+  nock.restore();
+  nock.recorder.clear();
+  t.equal(nock.recorder.play().length, 0);
+
+  nock.recorder.rec(true);
+
+  var makeRequest = function(callback) {
+    superagent
+      .get('https://example.com/')
+      .query({'foo':['bar', 'baz']})
+      .end(callback);
+  };
+
+  makeRequest(function(err, resp) {
+    t.ok(!err, err && err.message || 'no error');
+    t.ok(resp, 'have response');
+    t.ok(resp.headers, 'have headers');
+
+    var ret = nock.recorder.play();
+    t.equal(ret.length, 1);
+    t.type(ret[0], 'string');
+    var match = "\nnock('https://example.com:443', {\"encodedQueryParams\":true})\n  .get('/')\n  .query({\"foo\":[\"bar\",\"baz\"]})\n  .reply(";
+    t.equal(ret[0].substring(0, match.length), match);
+    t.end();
+  });
+});
+
 test('removes query params from from that path and puts them in query()', {skip: process.env.AIRPLANE}, function(t) {
   nock.restore();
   nock.recorder.clear();