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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -1163,6 +1153,10 @@ Request.prototype.qs = function (q, clobber) { | |
self.url = self.uri | ||
self.path = self.uri.path | ||
|
||
if ( self.uri.host === 'unix' ) { | ||
self.enableUnixSocket() | ||
} | ||
|
||
return self | ||
} | ||
Request.prototype.form = function (form) { | ||
|
@@ -1411,6 +1405,19 @@ Request.prototype.destroy = function () { | |
self.response.destroy() | ||
} | ||
} | ||
Request.prototype.enableUnixSocket = function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @simov Line 1249 sets There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// 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() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
}) | ||
|
@@ -27,7 +31,12 @@ tape('setup', function(t) { | |
}) | ||
|
||
tape('unix socket connection', function(t) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
There was a problem hiding this comment.
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.