From 9513deb13103986c4f456935e73022c3fb334607 Mon Sep 17 00:00:00 2001 From: simov Date: Mon, 25 May 2015 21:41:45 +0300 Subject: [PATCH 1/2] Extract the querystring logic into separate module: - Update `qs` to version 3.0.0 with support for rfc3986 encoding - Use request's rfc3986 only when using the `querystring` module - Add test branch for rfc3986 encoding tests using `querystring` - Support for `eq`, `sep` and `options` arguments for `querystring` `parse` and `stringify` methods via option keys --- lib/querystring.js | 51 +++++++++++++++++++++++++++++++++++++++++++ package.json | 2 +- request.js | 42 ++++++++++------------------------- tests/test-rfc3986.js | 11 +++++++--- 4 files changed, 72 insertions(+), 34 deletions(-) create mode 100644 lib/querystring.js diff --git a/lib/querystring.js b/lib/querystring.js new file mode 100644 index 000000000..baf5e8021 --- /dev/null +++ b/lib/querystring.js @@ -0,0 +1,51 @@ +'use strict' + +var qs = require('qs') + , querystring = require('querystring') + + +function Querystring (request) { + this.request = request + this.lib = null + this.useQuerystring = null + this.parseOptions = null + this.stringifyOptions = null +} + +Querystring.prototype.init = function (options) { + if (this.lib) {return} + + this.useQuerystring = options.useQuerystring + this.lib = (this.useQuerystring ? querystring : qs) + + this.parseOptions = options.qsParseOptions || {} + this.stringifyOptions = options.qsStringifyOptions || {} +} + +Querystring.prototype.stringify = function (obj) { + return (this.useQuerystring) + ? this.rfc3986(this.lib.stringify(obj, + this.stringifyOptions.sep || null, + this.stringifyOptions.eq || null, + this.stringifyOptions)) + : this.lib.stringify(obj, this.stringifyOptions) +} + +Querystring.prototype.parse = function (str) { + return (this.useQuerystring) + ? this.lib.parse(str, + this.parseOptions.sep || null, + this.parseOptions.eq || null, + this.parseOptions) + : this.lib.parse(str, this.parseOptions) +} + +Querystring.prototype.rfc3986 = function (str) { + return str.replace(/[!'()*]/g, function (c) { + return '%' + c.charCodeAt(0).toString(16).toUpperCase() + }) +} + +Querystring.prototype.unescape = querystring.unescape + +exports.Querystring = Querystring diff --git a/package.json b/package.json index a0dbaba23..6aa028422 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "json-stringify-safe": "~5.0.0", "mime-types": "~2.0.1", "node-uuid": "~1.4.0", - "qs": "~2.4.0", + "qs": "~3.0.0", "tunnel-agent": "~0.4.0", "tough-cookie": ">=0.12.0", "http-signature": "~0.10.0", diff --git a/request.js b/request.js index 055c7bfec..359f7df6e 100644 --- a/request.js +++ b/request.js @@ -5,8 +5,6 @@ var http = require('http') , url = require('url') , util = require('util') , stream = require('stream') - , qs = require('qs') - , querystring = require('querystring') , zlib = require('zlib') , helpers = require('./lib/helpers') , bl = require('bl') @@ -22,6 +20,7 @@ var http = require('http') , cookies = require('./lib/cookies') , copy = require('./lib/copy') , getProxyFromURI = require('./lib/getProxyFromURI') + , Querystring = require('./lib/querystring').Querystring , Har = require('./lib/har').Har , Auth = require('./lib/auth').Auth , OAuth = require('./lib/oauth').OAuth @@ -229,13 +228,6 @@ function responseToJSON() { } } -// encode rfc3986 characters -function rfc3986 (str) { - return str.replace(/[!'()*]/g, function(c) { - return '%' + c.charCodeAt(0).toString(16).toUpperCase() - }) -} - function Request (options) { // if given the method property in options, set property explicitMethod to true @@ -265,6 +257,7 @@ function Request (options) { if (options.method) { self.explicitMethod = true } + self._qs = new Querystring(self) self._auth = new Auth(self) self._oauth = new OAuth(self) self._multipart = new Multipart(self) @@ -341,15 +334,7 @@ Request.prototype.init = function (options) { self.localAddress = options.localAddress } - if (!self.qsLib) { - self.qsLib = (options.useQuerystring ? querystring : qs) - } - if (!self.qsParseOptions) { - self.qsParseOptions = options.qsParseOptions - } - if (!self.qsStringifyOptions) { - self.qsStringifyOptions = options.qsStringifyOptions - } + self._qs.init(options) debug(options) if (!self.pool && self.pool !== false) { @@ -576,14 +561,12 @@ Request.prototype.init = function (options) { } if (self.uri.auth && !self.hasHeader('authorization')) { - var uriAuthPieces = self.uri.auth.split(':').map(function(item) { return querystring.unescape(item) }) + var uriAuthPieces = self.uri.auth.split(':').map(function(item) {return self._qs.unescape(item)}) self.auth(uriAuthPieces[0], uriAuthPieces.slice(1).join(':'), true) } if (!self.tunnel && self.proxy && self.proxy.auth && !self.hasHeader('proxy-authorization')) { - var proxyAuthPieces = self.proxy.auth.split(':').map(function(item) { - return querystring.unescape(item) - }) + var proxyAuthPieces = self.proxy.auth.split(':').map(function(item) {return self._qs.unescape(item)}) var authHeader = 'Basic ' + toBase64(proxyAuthPieces.join(':')) self.setHeader('proxy-authorization', authHeader) } @@ -1295,7 +1278,7 @@ Request.prototype.qs = function (q, clobber) { var self = this var base if (!clobber && self.uri.query) { - base = self.qsLib.parse(self.uri.query, self.qsParseOptions) + base = self._qs.parse(self.uri.query) } else { base = {} } @@ -1304,13 +1287,13 @@ Request.prototype.qs = function (q, clobber) { base[i] = q[i] } - if (self.qsLib.stringify(base, self.qsStringifyOptions) === '') { + if (self._qs.stringify(base) === '') { return self } - var qs = self.qsLib.stringify(base, self.qsStringifyOptions) + var qs = self._qs.stringify(base) - self.uri = url.parse(self.uri.href.split('?')[0] + '?' + rfc3986(qs)) + self.uri = url.parse(self.uri.href.split('?')[0] + '?' + qs) self.url = self.uri self.path = self.uri.path @@ -1321,9 +1304,8 @@ Request.prototype.form = function (form) { if (form) { self.setHeader('content-type', 'application/x-www-form-urlencoded') self.body = (typeof form === 'string') - ? form.toString('utf8') - : self.qsLib.stringify(form, self.qsStringifyOptions).toString('utf8') - self.body = rfc3986(self.body) + ? self._qs.rfc3986(form.toString('utf8')) + : self._qs.stringify(form).toString('utf8') return self } // create form-data object @@ -1359,7 +1341,7 @@ Request.prototype.json = function (val) { if (!/^application\/x-www-form-urlencoded\b/.test(self.getHeader('content-type'))) { self.body = safeStringify(self.body) } else { - self.body = rfc3986(self.body) + self.body = self._qs.rfc3986(self.body) } if (!self.hasHeader('content-type')) { self.setHeader('content-type', 'application/json') diff --git a/tests/test-rfc3986.js b/tests/test-rfc3986.js index cfd27f11b..50868ab47 100644 --- a/tests/test-rfc3986.js +++ b/tests/test-rfc3986.js @@ -97,8 +97,13 @@ var cases = [ } ] -cases.forEach(function (options) { - tape('rfc3986 ' + options._name, function(t) { - runTest(t, options) +var libs = ['qs', 'querystring'] + +libs.forEach(function (lib) { + cases.forEach(function (options) { + options.useQuerystring = (lib === 'querystring') + tape(lib + ' rfc3986 ' + options._name, function(t) { + runTest(t, options) + }) }) }) From 5cdcbe5a8fcb973db7731c448ed1ff5788f4017b Mon Sep 17 00:00:00 2001 From: simov Date: Thu, 28 May 2015 20:33:47 +0300 Subject: [PATCH 2/2] Add docs and tests for passing options to the querystring's parse and stringify methods --- README.md | 7 +++---- tests/test-qs.js | 42 +++++++++++++++++++++++++++--------------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 2dc526b2b..d8bd40570 100644 --- a/README.md +++ b/README.md @@ -199,8 +199,7 @@ For advanced cases, you can access the form-data object itself via `r.form()`. T ```js // NOTE: Advanced use-case, for normal use see 'formData' usage above -var r = request.post('http://service.com/upload', function optionalCallback(err, httpResponse, body) { // ... - +var r = request.post('http://service.com/upload', function optionalCallback(err, httpResponse, body) {...}) var form = r.form(); form.append('my_field', 'my_value'); form.append('my_buffer', new Buffer([1, 2, 3])); @@ -723,8 +722,8 @@ The first argument can be either a `url` or an `options` object. The only requir --- - `qs` - object containing querystring values to be appended to the `uri` -- `qsParseOptions` - object containing options to pass to the [qs.parse](https://github.com/hapijs/qs#parsing-objects) method or [querystring.parse](https://nodejs.org/docs/v0.12.0/api/querystring.html#querystring_querystring_parse_str_sep_eq_options) method -- `qsStringifyOptions` - object containing options to pass to the [qs.stringify](https://github.com/hapijs/qs#stringifying) method or to the [querystring.stringify](https://nodejs.org/docs/v0.12.0/api/querystring.html#querystring_querystring_stringify_obj_sep_eq_options) method. For example, to change the way arrays are converted to query strings pass the `arrayFormat` option with one of `indices|brackets|repeat` +- `qsParseOptions` - object containing options to pass to the [qs.parse](https://github.com/hapijs/qs#parsing-objects) method. Alternatively pass options to the [querystring.parse](https://nodejs.org/docs/v0.12.0/api/querystring.html#querystring_querystring_parse_str_sep_eq_options) method using this format `{sep:';', eq:':', options:{}}` +- `qsStringifyOptions` - object containing options to pass to the [qs.stringify](https://github.com/hapijs/qs#stringifying) method. Alternatively pass options to the [querystring.stringify](https://nodejs.org/docs/v0.12.0/api/querystring.html#querystring_querystring_stringify_obj_sep_eq_options) method using this format `{sep:';', eq:':', options:{}}`. For example, to change the way arrays are converted to query strings using the `qs` module pass the `arrayFormat` option with one of `indices|brackets|repeat` - `useQuerystring` - If true, use `querystring` to stringify and parse querystrings, otherwise use `qs` (default: `false`). Set this option to `true` if you need arrays to be serialized as `foo=bar&foo=baz` instead of the diff --git a/tests/test-qs.js b/tests/test-qs.js index 198621d46..85d741403 100644 --- a/tests/test-qs.js +++ b/tests/test-qs.js @@ -13,23 +13,18 @@ var request = require('../index') // - expectedQuerystring : expected path when using the querystring library function runTest(name, options) { var uri = 'http://www.google.com' + (options.suffix || '') - , requestOptsQs = { - uri : uri, - qsParseOptions: options.qsParseOptions, - qsStringifyOptions: options.qsStringifyOptions - } - , requestOptsQuerystring = { - uri : uri, - useQuerystring : true - } + var opts = { + uri : uri, + qsParseOptions: options.qsParseOptions, + qsStringifyOptions: options.qsStringifyOptions + } if (options.qs) { - requestOptsQs.qs = options.qs - requestOptsQuerystring.qs = options.qs + opts.qs = options.qs } - tape(name + ' using qs', function(t) { - var r = request.get(requestOptsQs) + tape(name + ' - using qs', function(t) { + var r = request.get(opts) if (typeof options.afterRequest === 'function') { options.afterRequest(r) } @@ -40,8 +35,9 @@ function runTest(name, options) { }) }) - tape(name + ' using querystring', function(t) { - var r = request.get(requestOptsQuerystring) + tape(name + ' - using querystring', function(t) { + opts.useQuerystring = true + var r = request.get(opts) if (typeof options.afterRequest === 'function') { options.afterRequest(r) } @@ -121,3 +117,19 @@ runTest('pass options to the qs module via the qsStringifyOptions key', { expected : esc('/?order[]=bar&order[]=desc'), expectedQuerystring : '/?order=bar&order=desc' }) + +runTest('pass options to the querystring module via the qsParseOptions key', { + suffix : '?a=1;b=2', + qs: {}, + qsParseOptions: { sep : ';' }, + qsStringifyOptions: { sep : ';' }, + expected : esc('/?a=1%3Bb%3D2'), + expectedQuerystring : '/?a=1;b=2' +}) + +runTest('pass options to the querystring module via the qsStringifyOptions key', { + qs : { order : ['bar', 'desc'] }, + qsStringifyOptions: { sep : ';' }, + expected : esc('/?order[0]=bar&order[1]=desc'), + expectedQuerystring : '/?order=bar;order=desc' +})