-
Notifications
You must be signed in to change notification settings - Fork 46
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
Proxy Requests for Unknown Paths #56 #84
base: master
Are you sure you want to change the base?
Changes from 7 commits
1ef34f2
e0e3886
94c958c
0b6d68c
dfa1600
933f996
3899fd4
86ae63e
8e816ac
58e2074
24ff7df
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 |
---|---|---|
|
@@ -14,6 +14,8 @@ var canned = require('../index') | |
.describe('cors', 'disable cors support') | ||
.default('headers', false) | ||
.describe('headers', 'add custom headers allowed in cors requests') | ||
.default('proxy', '') | ||
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. do you think and alias to 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. Probably want this to default to 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. @sideshowcoder yes, it would be more semantic, will make the change 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 not assign an alias, as |
||
.describe('proxy', 'proxy unknown paths to this domain') | ||
.default('h', false) | ||
.alias('h', 'help') | ||
.describe('h', 'show the help') | ||
|
@@ -29,6 +31,7 @@ var dir = '' | |
, port = argv.p | ||
, cors = argv.cors | ||
, cors_headers = argv.headers | ||
, proxy = argv.proxy | ||
, logger | ||
, cannedDir | ||
, wildcard = argv.wildcard | ||
|
@@ -42,6 +45,12 @@ if (argv.q) { | |
process.stdout.write('starting canned on port ' + port + ' for ' + cannedDir + '\n') | ||
} | ||
|
||
var can = canned(dir, { logger: logger, cors: cors, cors_headers: cors_headers, wildcard: wildcard}) | ||
var can = canned(dir, { | ||
logger: logger, | ||
cors: cors, | ||
cors_headers: cors_headers, | ||
wildcard: wildcard, | ||
proxy: proxy | ||
}) | ||
http.createServer(can).listen(port) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,12 @@ var url = require('url') | |
var cannedUtils = require('./utils') | ||
var lookup = require('./lookup') | ||
var _ = require('lodash') | ||
var request = require('request') | ||
|
||
function Canned(dir, options) { | ||
this.logger = options.logger | ||
this.wildcard = options.wildcard || 'any' | ||
this.proxy = options.proxy | ||
this.response_opts = { | ||
cors_enabled: options.cors, | ||
cors_headers: options.cors_headers | ||
|
@@ -224,27 +226,26 @@ Canned.prototype._responseForFile = function (httpObj, files, cb) { | |
fs.readFile(filePath, { encoding: 'utf8' }, function (err, data) { | ||
var response | ||
if (err) { | ||
response = new Response(getContentType('html'), '', 404, httpObj.res, that.response_opts) | ||
cb('Not found', response) | ||
cb('Not found', httpObj.res) | ||
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. the error return value should be a true error object returning just a string can make things hard to debug as it contains no context where the error will capture the current stack. 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. Possibly you can even use the 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. @sideshowcoder i am keeping the implementation as close to the one before the changes, i would propose we do the refactoring, better error handling as a new PR, i have sugessions, we can discuss later. But i don't think it should be part of this PR, to not mix up things. |
||
} else { | ||
var _data = that.getVariableResponse(data, httpObj.content, httpObj.headers) | ||
data = _data.data | ||
var statusCode = _data.statusCode | ||
var content = that.sanatizeContent(data, fileObject) | ||
|
||
if (content !== false) { | ||
response = new Response(_data.contentType || getContentType(fileObject.mimetype), content, statusCode, httpObj.res, that.response_opts, _data.customHeaders) | ||
cb(null, response) | ||
} else { | ||
content = 'Internal Server error invalid input file' | ||
response = new Response(getContentType('html'), content, 500, httpObj.res, that.response_opts) | ||
cb(null, response) | ||
} | ||
that._extractRequestContent(httpObj.req, function (content) { | ||
var _data = that.getVariableResponse(data, content, httpObj.headers) | ||
data = _data.data | ||
var statusCode = _data.statusCode | ||
content = that.sanatizeContent(data, fileObject) | ||
if (content !== false) { | ||
response = new Response(_data.contentType || getContentType(fileObject.mimetype), content, statusCode, httpObj.res, that.response_opts, _data.customHeaders) | ||
cb(null, response) | ||
} else { | ||
content = 'Internal Server error invalid input file' | ||
response = new Response(getContentType('html'), content, 500, httpObj.res, that.response_opts) | ||
cb(null, response) | ||
} | ||
}); | ||
} | ||
}) | ||
} else { | ||
var response = new Response(getContentType('html'), '', 404, httpObj.res, that.response_opts) | ||
cb('Not found', response) | ||
cb('Not found', httpObj.res) | ||
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. same here a true error object would be preferable. 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. @sideshowcoder i agree, but i would propose to do this not in the PR, but a another PR on refactor, i can do that, i have some ideas i want to present to better code structuring than currently done. |
||
} | ||
} | ||
|
||
|
@@ -278,21 +279,21 @@ Canned.prototype.respondWithAny = function (httpObj, files, cb) { | |
}) | ||
} | ||
|
||
Canned.prototype.responder = function(body, req, res) { | ||
Canned.prototype.responder = function(req, res) { | ||
var responseHandler | ||
var httpObj = {} | ||
var that = this | ||
var parsedurl = url.parse(req.url) | ||
httpObj.headers = req.headers | ||
httpObj.accept = (req.headers && req.headers.accept) ? req.headers.accept.trim().split(',') : [] | ||
httpObj.content = body | ||
httpObj.pathname = parsedurl.pathname.split('/') | ||
httpObj.dname = httpObj.pathname.pop() | ||
httpObj.fname = '_' + httpObj.dname | ||
httpObj.path = this.dir + httpObj.pathname.join('/') | ||
httpObj.query = parsedurl.query | ||
httpObj.method = req.method.toLowerCase() | ||
httpObj.res = res | ||
httpObj.req = req | ||
httpObj.ctype = '' | ||
|
||
this._log('request: ' + httpObj.method + ' ' + req.url) | ||
|
@@ -320,8 +321,11 @@ Canned.prototype.responder = function(body, req, res) { | |
httpObj.path = that.dir + paths.splice(0, 1)[0]; | ||
httpObj.fname = '_' + httpObj.dname; | ||
return that.findResponse(httpObj, responseHandler); | ||
} else { | ||
} else if (that.proxy){ | ||
return that._proxyRequest(httpObj) | ||
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 think a line in the log saying "falling back to proxy for xyz" would be helpful in th output. 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. @sideshowcoder it does, inside the implementation of
check line number 345 |
||
}else { | ||
that._log(' not found\n'); | ||
resp = new Response(getContentType('html'), '', 404, httpObj.res, that.response_opts) | ||
} | ||
} else { | ||
that._logHTTPObject(httpObj) | ||
|
@@ -334,6 +338,19 @@ Canned.prototype.responder = function(body, req, res) { | |
|
||
} | ||
|
||
Canned.prototype._proxyRequest = function (httpObj) { | ||
var that = this; | ||
var parsedurl = url.parse(httpObj.req.url) | ||
var proxyUrl = that.proxy + (parsedurl.path || '') + (parsedurl.query || '')+ (parsedurl.hash || ''); | ||
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. this sould be a function like 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. yep i agree, will do that |
||
that._log(' proxying request to ' + proxyUrl + '\n'); | ||
return httpObj.req.pipe(request(proxyUrl)) | ||
.on('error', function(err) { | ||
that._log(' proxy gave error ' + err.code + '\n'); | ||
var resp = new Response(getContentType('html'), '', 404, httpObj.res, that.response_opts) | ||
resp.send(); | ||
}).pipe(httpObj.res); | ||
} | ||
|
||
Canned.prototype.findResponse = function(httpObj, cb) { | ||
var that = this; | ||
fs.readdir(httpObj.path, function (err, files) { | ||
|
@@ -357,11 +374,10 @@ Canned.prototype.findResponse = function(httpObj, cb) { | |
}) | ||
} | ||
|
||
Canned.prototype.responseFilter = function (req, res) { | ||
Canned.prototype._extractRequestContent = function (req, callback) { | ||
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. callbacks are named 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 agree, didn't really knew that was a style standard we use, will stick to that now on |
||
var that = this | ||
var body = '' | ||
|
||
// assemble response body if GET/POST/PUT | ||
// assemble request body if GET/POST/PUT | ||
switch(req.method) { | ||
case 'PUT': | ||
case 'POST': | ||
|
@@ -377,18 +393,18 @@ Canned.prototype.responseFilter = function (req, res) { | |
that._log('Invalid json content') | ||
} | ||
} | ||
that.responder(responderBody, req, res) | ||
callback(responderBody); | ||
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. first argument to a callback is always an error so this should be 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. @sideshowcoder as you can see its an internal function, and not a exposed public api, and because of that i don't think it makes sense to return a first error parameter, if we are never supposed to sent back error, which is current logic. |
||
}) | ||
break | ||
case 'GET': | ||
var query = url.parse(req.url).query | ||
if (query && query.length > 0) { | ||
body = querystring.parse(query) | ||
} | ||
that.responder(body, req, res) | ||
callback(body) | ||
break | ||
default: | ||
that.responder(body, req, res) | ||
callback(body) | ||
break | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -702,4 +702,56 @@ describe('canned', function () { | |
}) | ||
}) | ||
|
||
describe("proxy requests for unknown paths", function () { | ||
it('should return 404 if path unknown and proxy not configured', function (done) { | ||
req.url = '/unkown_path' | ||
res.end = function (content) { | ||
expect(res.statusCode).toEqual(404); | ||
done() | ||
} | ||
can(req, res) | ||
}) | ||
|
||
it('should return mock if path known and proxy configured', function (done) { | ||
var can = canned('./spec/test_responses', { | ||
proxy: 'http://localhost:9615' | ||
}) | ||
|
||
req.url = '/a' | ||
res.end = function (content) { | ||
expect(res.statusCode).toEqual(200); | ||
done() | ||
} | ||
can(req, res) | ||
}) | ||
|
||
it('should proxy request if path unknown and proxy is configured', function (done) { | ||
var proxy = require('http').createServer(function (req, res) { | ||
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. Really like the test making use of an actual server makes this some much more useful! 👍 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. @sideshowcoder glad you like it, it was almost impossible to test it otherwise |
||
res.writeHead(200, {'Content-Type': 'text/plain'}); | ||
res.end('OK'); | ||
req.on('data', function (data) { | ||
expect(data.toString()).toEqual('test'); | ||
}) | ||
}).listen(9615); | ||
|
||
var can = canned('./spec/test_responses', { | ||
proxy: 'http://localhost:9615' | ||
}) | ||
|
||
var req = new require('stream').Readable(); | ||
req._read = function noop() {}; | ||
req.push('test'); | ||
req.method = 'POST' | ||
req.url = '/unkown_path' | ||
|
||
var res = new require('stream').Writable(); | ||
res._write = function noop(data) { | ||
expect(data.toString()).toEqual('OK'); | ||
proxy.close() | ||
done() | ||
}; | ||
can(req, res) | ||
}) | ||
}) | ||
|
||
}) |
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.
after the https://github.com/sideshowcoder/canned/blob/feature-56_proxy_request/README.md#variable-responses section you could add a new section to the README detailing how the proxy stuff works, this would need to contain some examples on how to use it with the commands to enter and the flow of a request through canned with a proxy enabled.
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.
@sideshowcoder yep i will do that, but give me till this weekend, because i am travelling till wednesday