Query-string refactor

lib/client.js

@@ -15,7 +15,6 @@ if (typeof window !== 'undefined') { // Browser window
 var Emitter = require('component-emitter');
 var RequestBase = require('./request-base');
 var isObject = require('./is-object');
-var isFunction = require('./is-function');
 var ResponseBase = require('./response-base');
 var shouldRetry = require('./should-retry');
 
@@ -643,32 +642,6 @@ Request.prototype.pipe = Request.prototype.write = function(){
   throw Error("Streaming is not supported in browser version of superagent");
 };
 
-/**
- * Compose querystring to append to req.url
- *
- * @api private
- */
-
-Request.prototype._appendQueryString = function(){
-  var query = this._query.join('&');
-  if (query) {
-    this.url += (this.url.indexOf('?') >= 0 ? '&' : '?') + query;
-  }
-
-  if (this._sort) {
-    var index = this.url.indexOf('?');
-    if (index >= 0) {
-      var queryArr = this.url.substring(index + 1).split('&');
-      if (isFunction(this._sort)) {
-        queryArr.sort(this._sort);
-      } else {
-        queryArr.sort();
-      }
-      this.url = this.url.substring(0, index) + '?' + queryArr.join('&');
-    }
-  }
-};
-
 /**
  * Check if `obj` is a host object,
  * we don't want to serialize these :)
@@ -701,7 +674,7 @@ Request.prototype.end = function(fn){
   this._callback = fn || noop;
 
   // querystring
-  this._appendQueryString();
+  this._finalizeQueryString();
 
   return this._end();
 };

lib/node/index.js

@@ -24,7 +24,6 @@ var zlib = require('zlib');
 var util = require('util');
 var pkg = require('../../package.json');
 var RequestBase = require('../request-base');
-var isFunction = require('../is-function');
 var shouldRetry = require('../should-retry');
 
 var request = exports = module.exports = function(method, url) {
@@ -144,7 +143,8 @@ function Request(method, url) {
   this.redirects(method === 'HEAD' ? 0 : 5);
   this.cookies = '';
   this.qs = {};
-  this.qsRaw = [];
+  this._query = [];
+  this.qsRaw = this._query; // Unused, for backwards compatibility only
   this._redirectList = [];
   this._streamRequest = false;
   this.once('end', this.clearTimeout.bind(this));
@@ -306,11 +306,10 @@ Request.prototype.accept = function(type){
 
 Request.prototype.query = function(val){
   if ('string' == typeof val) {
-    this.qsRaw.push(val);
-    return this;
+    this._query.push(val);
+  } else {
+    extend(this.qs, val);
   }
-
-  extend(this.qs, val);
   return this;
 };
 
@@ -452,7 +451,7 @@ Request.prototype._redirect = function(res){
   this._endCalled = false;
   this.url = url;
   this.qs = {};
-  this.qsRaw = [];
+  this._query.length = 0;
   this.set(headers);
   this.emit('redirect', res);
   this._redirectList.push(this.url);
@@ -485,12 +484,12 @@ Request.prototype.auth = function(user, pass, options){
   }
   switch (options.type) {
     case 'bearer':
-      return this.set('Authorization', 'Bearer ' + user);    
-      
+      return this.set('Authorization', 'Bearer ' + user);
+
     default: // 'basic'
       if (!~user.indexOf(':')) user = user + ':';
       var str = new Buffer(user + pass).toString('base64');
-      return this.set('Authorization', 'Basic ' + str);    
+      return this.set('Authorization', 'Basic ' + str);
   }
 };
 
@@ -558,6 +557,18 @@ Request.prototype.request = function(){
 
   var self = this;
   var options = {};
+
+  try {
+    var query = qs.stringify(this.qs, { indices: false, strictNullHandling: true });
+    if (query) {
+      this.qs = {};
+      this._query.push(query);
+    }
+    this._finalizeQueryString();
+  } catch (e) {
+    return this.emit('error', e);
+  }
+
   var url = this.url;
   var retries = this._retries;
 
@@ -573,13 +584,13 @@ Request.prototype.request = function(){
     // get the socket, path
     var unixParts = url.path.match(/^([^/]+)(.+)$/);
     options.socketPath = unixParts[1].replace(/%2F/g, '/');
-    url.pathname = unixParts[2];
+    url.path = unixParts[2];
   }
 
   // options
   options.method = this.method;
   options.port = url.port;
-  options.path = url.pathname;
+  options.path = url.path;
   options.host = url.hostname;
   options.ca = this._ca;
   options.key = this._key;
@@ -621,10 +632,6 @@ Request.prototype.request = function(){
     this.auth(auth[0], auth[1]);
   }
 
-  // query
-  if (url.search)
-    this.query(url.search.substr(1));
-
   // add cookies
   if (this.cookies) req.setHeader('Cookie', this.cookies);
 
@@ -633,12 +640,6 @@ Request.prototype.request = function(){
       req.setHeader(key, this.header[key]);
   }
 
-  try {
-    this._appendQueryString(req);
-  } catch (e) {
-    return this.emit('error', e);
-  }
-
   return req;
 };
 
@@ -688,32 +689,6 @@ Request.prototype.callback = function(err, res){
   fn(err, res);
 };
 
-/**
- * Compose querystring to append to req.path
- *
- * @return {String} querystring
- * @api private
- */
-
-Request.prototype._appendQueryString = function(req){
-  var query = qs.stringify(this.qs, { indices: false, strictNullHandling: true });
-  query += ((query.length && this.qsRaw.length) ? '&' : '') + this.qsRaw.join('&');
-  req.path += query.length ? (~req.path.indexOf('?') ? '&' : '?') + query : '';
-
-  if (this._sort) {
-    var index = req.path.indexOf('?');
-    if (index >= 0) {
-      var queryArr = req.path.substring(index + 1).split('&');
-      if (isFunction(this._sort)) {
-        queryArr.sort(this._sort);
-      } else {
-        queryArr.sort();
-      }
-      req.path = req.path.substring(0, index) + '?' + queryArr.join('&');
-    }
-  }
-};
-
 /**
  * Check if `obj` is a host object,
  *

lib/request-base.js

@@ -554,6 +554,35 @@ RequestBase.prototype.sortQuery = function(sort) {
   return this;
 };
 
+/**
+ * Compose querystring to append to req.url
+ *
+ * @api private
+ */
+RequestBase.prototype._finalizeQueryString = function(){
+  var query = this._query.join('&');
+  if (query) {
+    this.url += (this.url.indexOf('?') >= 0 ? '&' : '?') + query;
+  }
+  this._query.length = 0; // Makes the call idempotent
+
+  if (this._sort) {
+    var index = this.url.indexOf('?');
+    if (index >= 0) {
+      var queryArr = this.url.substring(index + 1).split('&');
+      if ('function' === typeof this._sort) {
+        queryArr.sort(this._sort);
+      } else {
+        queryArr.sort();
+      }
+      this.url = this.url.substring(0, index) + '?' + queryArr.join('&');
+    }
+  }
+};
+
+// For backwards compat only
+RequestBase.prototype._appendQueryString = function() {console.trace("Unsupported");}
+
 /**
  * Invoke callback with timeout error.
  *

test/retry.js

@@ -142,15 +142,18 @@ describe('.retry(count)', function(){
   });
 
   it('should handle successful request after repeat attempt from server timeout', function(done) {
+    var url = '/delay/400/ok/' + uniqid() + '?built=in';
     request
-    .get(base + '/delay/400/ok/' + uniqid())
+    .get(base + url)
+    .query("string=ified")
+    .query({"json":"ed"})
     .timeout(200)
     .retry(2)
     .end(function(err, res){
       try {
       assert.ifError(err);
       assert(res.ok, 'response should be ok');
-      assert(res.text, 'res.text');
+      assert.equal(res.text, 'ok = ' + url + '&string=ified&json=ed');
       done();
       } catch(err) {
         done(err);

test/support/server.js

@@ -448,7 +448,7 @@ app.get('/delay/:ms/ok/:id', function(req, res){
       res.sendStatus(200);
     }, ms);
   } else {
-    res.send('ok');
+    res.send('ok = ' + req.url);
     delete called[id];
   }
 });