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

Query strings now cooperate with unix sockets #1767

Merged
merged 2 commits into from Sep 11, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
29 changes: 18 additions & 11 deletions request.js
Expand Up @@ -256,17 +256,7 @@ Request.prototype.init = function (options) {

// Support Unix Sockets
if (self.uri.host === 'unix') {
// Get the socket & request paths from the URL
var unixParts = self.uri.path.split(':')
, host = unixParts[0]
, path = unixParts[1]
// Apply unix properties to request
self.socketPath = host
self.uri.pathname = path
self.uri.path = path
self.uri.host = host
self.uri.hostname = host
self.uri.isUnix = true
self.enableUnixSocket()
}

if (self.strictSSL === false) {
Expand Down Expand Up @@ -1163,6 +1153,10 @@ Request.prototype.qs = function (q, clobber) {
self.url = self.uri
self.path = self.uri.path

if ( self.uri.host === 'unix' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style issue - remove spacing.

self.enableUnixSocket()
}

return self
}
Request.prototype.form = function (form) {
Expand Down Expand Up @@ -1411,6 +1405,19 @@ Request.prototype.destroy = function () {
self.response.destroy()
}
}
Request.prototype.enableUnixSocket = function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the best place to put this function. Notice the comment above - Streams API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure if that needs to be defined on the prototype as a public method, probably a function that accepts an uri object will be just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simov Line 1249 sets this.socketPath, which requires access to this, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed all the things you highlighted, but I can make it a seperate by doing fn (uri, context){} or something if you prefer.

// Get the socket & request paths from the URL
var unixParts = this.uri.path.split(':')
, host = unixParts[0]
, path = unixParts[1]
// Apply unix properties to request
this.socketPath = host
this.uri.pathname = path
this.uri.path = path
this.uri.host = host
this.uri.hostname = host
this.uri.isUnix = true
}

Request.defaultProxyHeaderWhiteList =
Tunnel.defaultProxyHeaderWhiteList.slice()
Expand Down
13 changes: 11 additions & 2 deletions tests/test-unix.js
Expand Up @@ -6,16 +6,20 @@ var request = require('../index')
, rimraf = require('rimraf')
, assert = require('assert')
, tape = require('tape')
, url = require('url')

var path = [null, 'test', 'path'].join('/')
, searchString = '?foo=bar'
, socket = [__dirname, 'tmp-socket'].join('/')
, expectedBody = 'connected'
, statusCode = 200

rimraf.sync(socket)

var s = http.createServer(function(req, res) {
assert.equal(req.url, path, 'requested path is sent to server')
var incomingUrl = url.parse(req.url)
assert.equal(incomingUrl.pathname, path, 'requested path is sent to server')
assert.equal(incomingUrl.search, searchString, 'query string is sent to server')
res.statusCode = statusCode
res.end(expectedBody)
})
Expand All @@ -27,7 +31,12 @@ tape('setup', function(t) {
})

tape('unix socket connection', function(t) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have two separate tests, the original one without querystring and a new one with querystring. That way if someone removes the unix bits from the init function the tests will throw an error.

request('http://unix:' + socket + ':' + path, function(err, res, body) {
request({
uri: 'http://unix:' + socket + ':' + path,
qs: {
foo: 'bar'
}
}, function(err, res, body) {
t.equal(err, null, 'no error in connection')
t.equal(res.statusCode, statusCode, 'got HTTP 200 OK response')
t.equal(body, expectedBody, 'expected response body is received')
Expand Down