Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract the querystring logic into separate module: #1600

Merged
merged 3 commits into from May 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions README.md
Expand Up @@ -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]));
Expand Down Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions 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
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -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",
Expand Down
42 changes: 12 additions & 30 deletions request.js
Expand Up @@ -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')
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 = {}
}
Expand All @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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')
Expand Down
42 changes: 27 additions & 15 deletions tests/test-qs.js
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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'
})
11 changes: 8 additions & 3 deletions tests/test-rfc3986.js
Expand Up @@ -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)
})
})
})