From e5d71a7838b132478fc64cbc21cff2ceb14a03ef Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Tue, 16 Jun 2015 11:26:33 -0700 Subject: [PATCH 1/3] Add support for declaring status code defaults --- lib/http-context.js | 51 +++++++++++++++- lib/rest-adapter.js | 4 +- lib/shared-method.js | 34 +++++++++-- test/authorize-hook.test.js | 1 - test/rest.test.js | 113 +++++++++++++++++++++++++++++++++--- 5 files changed, 185 insertions(+), 18 deletions(-) diff --git a/lib/http-context.js b/lib/http-context.js index 5f7f962..4ef3ff1 100644 --- a/lib/http-context.js +++ b/lib/http-context.js @@ -38,6 +38,7 @@ function HttpContext(req, res, method, options) { this.args = this.buildArgs(method); this.methodString = method.stringName; this.supportedTypes = this.options.supportedTypes || DEFAULT_SUPPORTED_TYPES; + this.result = {}; if (this.supportedTypes === DEFAULT_SUPPORTED_TYPES && !this.options.xml) { // Disable all XML-based types by default @@ -304,6 +305,9 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { var pipe = http && http.pipe; var pipeDest = pipe && pipe.dest; var pipeSrc = pipe && pipe.source; + var ctx = this; + var defaultErrorStatus = http && http.errorStatus; + var res = this.res; if (pipeDest) { // only support response for now @@ -313,7 +317,7 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { this.res.header('Content-Type', 'application/json'); this.res.header('Transfer-Encoding', 'chunked'); - var stream = method.invoke(scope, args, this.options, fn); + var stream = method.invoke(scope, args, this.options, ctx, fn); stream.pipe(this.res); break; default: @@ -324,7 +328,7 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { // only support request for now switch (pipeDest) { case 'req': - this.req.pipe(method.invoke(scope, args, this.options, fn)); + this.req.pipe(method.invoke(scope, args, this.options, ctx, fn)); break; default: fn(new Error('unsupported pipe source')); @@ -332,7 +336,43 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { } } else { // simple invoke - method.invoke(scope, args, this.options, fn); + method.invoke(scope, args, this.options, ctx, function(err, result) { + if (err) { + if (defaultErrorStatus) { + if(!(err.status || err.statusCode)) { + res.status(defaultErrorStatus); + } + } + return fn(err); + } + fn(null, result); + }); + } +}; + +HttpContext.prototype.setReturnArgByName = function(name, value) { + var returnDesc = this.method.getReturnArgDescByName(name); + var result = this.result; + var res = this.res; + + if (!returnDesc) { + return debug('warning: cannot set return value for arg' + + ' (%s) without description!', name); + } + + if (returnDesc.root) { + this.result = value; + } else if (returnDesc.http) { + switch (returnDesc.http.target) { + case 'status': + res.status(value); + break; + case 'header': + res.set(returnDesc.http.header || name, value); + break; + } + } else { + result[name] = value; } }; @@ -384,6 +424,11 @@ HttpContext.prototype.done = function() { var data = this.result; var res = this.res; var accepts = this.req.accepts(this.supportedTypes); + var defaultStatus = this.method.http.status; + + if (defaultStatus) { + res.status(defaultStatus); + } if (this.req.query._format) { accepts = this.req.query._format.toLowerCase(); diff --git a/lib/rest-adapter.js b/lib/rest-adapter.js index 02391df..77cb0b6 100644 --- a/lib/rest-adapter.js +++ b/lib/rest-adapter.js @@ -359,7 +359,9 @@ RestAdapter.errorHandler = function(options) { err.status = err.statusCode = 500; } - res.statusCode = err.statusCode || err.status || 500; + if (res.statusCode === undefined || res.statusCode < 400) { + res.statusCode = err.statusCode || err.status || 500; + } debug('Error in %s %s: %s', req.method, req.url, err.stack); var data = { diff --git a/lib/shared-method.js b/lib/shared-method.js index 29f43df..2401885 100644 --- a/lib/shared-method.js +++ b/lib/shared-method.js @@ -24,6 +24,9 @@ var assert = require('assert'); * @param {Boolean} [options.isStatic] Is the method a static method or a * a `prototype` method * @param {Array} [options.aliases] A list of aliases for the + * @param {Array} [options.http] HTTP only options + * @param {Number} [options.http.status] The default status code + * @param {Number} [options.http.errorStatus] The default error status code * `sharedMethod.name` * @param {Array|Object} [options.accepts] An `Array` of argument definitions * that describe the arguments of the `SharedMethod`. @@ -144,6 +147,17 @@ SharedMethod.fromFunction = function(fn, name, sharedClass, isStatic) { }); }; +SharedMethod.prototype.getReturnArgDescByName = function(name) { + var returns = this.returns; + var desc; + for (var i = 0; i < returns.length; i++) { + desc = returns[i]; + if (desc && ((desc.arg || desc.name) === name)) { + return desc; + } + } +}; + /** * Execute the remote method using the given arg data. * @@ -153,7 +167,7 @@ SharedMethod.fromFunction = function(fn, name, sharedClass, isStatic) { * @param {Function} cb callback `fn(err, result)` containing named result data */ -SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) { +SharedMethod.prototype.invoke = function(scope, args, remotingOptions, ctx, cb) { var accepts = this.accepts; var returns = this.returns; var errors = this.errors; @@ -161,6 +175,11 @@ SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) { var sharedMethod = this; var formattedArgs = []; + if (typeof ctx === 'function') { + cb = ctx; + ctx = undefined; + } + if (cb === undefined && typeof remotingOptions === 'function') { cb = remotingOptions; remotingOptions = {}; @@ -189,8 +208,9 @@ SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) { if (err) { return cb(err); } - - var result = SharedMethod.toResult(returns, [].slice.call(arguments, 1)); + // args without err + var rawArgs = [].slice.call(arguments, 1); + var result = SharedMethod.toResult(returns, rawArgs, ctx); debug('- %s - result %j', sharedMethod.name, result); @@ -457,7 +477,7 @@ SharedMethod.convertArg = function(accept, raw) { * results from a remoting function, based on `returns`. */ -SharedMethod.toResult = function(returns, raw) { +SharedMethod.toResult = function(returns, raw, ctx) { var result = {}; if (!returns.length) { @@ -471,6 +491,7 @@ SharedMethod.toResult = function(returns, raw) { if (item.root) { result = convert(raw[index]); + if (ctx) ctx.setReturnArgByName(item.name || item.arg, raw[index]); return false; } @@ -478,7 +499,10 @@ SharedMethod.toResult = function(returns, raw) { }); returns.forEach(function(item, index) { - result[item.name || item.arg] = convert(raw[index]); + var value = convert(raw[index]); + var name = item.name || item.arg; + result[name] = value; + if (ctx) ctx.setReturnArgByName(name, value); }); return result; diff --git a/test/authorize-hook.test.js b/test/authorize-hook.test.js index bb87a4b..043aa70 100644 --- a/test/authorize-hook.test.js +++ b/test/authorize-hook.test.js @@ -55,4 +55,3 @@ describe('authorization hook', function() { remotes.invoke(method, args, callback); } }); - diff --git a/test/rest.test.js b/test/rest.test.js index 4674ec4..f84f87c 100644 --- a/test/rest.test.js +++ b/test/rest.test.js @@ -1753,17 +1753,114 @@ describe('strong-remoting-rest', function() { }); }); - it('returns 404 for unknown method of a shared class', function(done) { - var classUrl = givenSharedStaticMethod().classUrl; + describe('status codes', function() { + describe('using a custom satus code', function() { + it('returns a custom status code', function(done) { + var method = givenSharedStaticMethod( + function fn(cb) { + cb(); + }, + { + http: { status: 201 } + } + ); + json(method.url) + .expect(201, done); + }); + it('returns a custom error status code', function(done) { + var method = givenSharedStaticMethod( + function fn(cb) { + cb(new Error('test error')); + }, + { + http: { status: 201, errorStatus: 508 } + } + ); + json(method.url) + .expect(508, done); + }); + it('returns a custom error status code (using the err object)', function(done) { + var method = givenSharedStaticMethod( + function fn(cb) { + var err = new Error('test error'); + err.status = 555; + cb(err); + }, + { + http: { status: 201, errorStatus: 508 } + } + ); + json(method.url) + .expect(555, done); + }); + it('returns a custom status code from a callback arg', function(done) { + var exampleStatus = 222; + var method = givenSharedStaticMethod( + function fn(status, cb) { + cb(null, status); + }, + { + accepts: { arg: 'status', type: 'number' }, + returns: { + arg: 'status', + http: { target: 'status' } + } + } + ); + json(method.url + '?status=' + exampleStatus) + .expect(exampleStatus, done); + }); + }); + + it('returns 404 for unknown method of a shared class', function(done) { + var classUrl = givenSharedStaticMethod().classUrl; + + json(classUrl + '/unknown-method') + .expect(404, done); + }); - json(classUrl + '/unknown-method') - .expect(404, done); + it('returns 404 with standard JSON body for uknown URL', function(done) { + json('/unknown-url') + .expect(404) + .end(expectErrorResponseContaining({status: 404}, done)); + }); }); - it('returns 404 with standard JSON body for uknown URL', function(done) { - json('/unknown-url') - .expect(404) - .end(expectErrorResponseContaining({status: 404}, done)); + describe('result args as headers', function() { + it('sets the header using the callback arg', function(done) { + var val = 'foobar'; + var method = givenSharedStaticMethod( + function fn(input, cb) { + cb(null, input); + }, + { + accepts: {arg: 'input', type: 'string'}, + returns: {arg: 'output', type: 'string', http: { target: 'header' } } + } + ); + json(method.url + '?input=' + val) + .expect('output', val) + .expect(200, done); + }); + it('sets the custom header using the callback arg', function(done) { + var val = 'foobar'; + var method = givenSharedStaticMethod( + function fn(input, cb) { + cb(null, input); + }, + { + accepts: {arg: 'input', type: 'string'}, + returns: {arg: 'output', type: 'string', http: { + target: 'header', + header: 'X-Custom-Header' + } + } + } + ); + json(method.url + '?input=' + val) + .expect('X-Custom-Header', val) + .expect(200, done); + }); }); it('returns correct error response body', function(done) { From 86f39f2796fa37eec4463f273daf14ed9c738dd4 Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Tue, 16 Jun 2015 11:26:33 -0700 Subject: [PATCH 2/3] Add support for declaring status code defaults --- lib/http-context.js | 50 +++++++++++++++++++++++++++-- lib/rest-adapter.js | 4 ++- lib/shared-method.js | 34 +++++++++++++++++--- test/authorize-hook.test.js | 1 - test/rest.test.js | 64 +++++++++++++++++++++++++++++++------ 5 files changed, 134 insertions(+), 19 deletions(-) diff --git a/lib/http-context.js b/lib/http-context.js index 5f7f962..cd64dd6 100644 --- a/lib/http-context.js +++ b/lib/http-context.js @@ -38,6 +38,7 @@ function HttpContext(req, res, method, options) { this.args = this.buildArgs(method); this.methodString = method.stringName; this.supportedTypes = this.options.supportedTypes || DEFAULT_SUPPORTED_TYPES; + this.result = {}; if (this.supportedTypes === DEFAULT_SUPPORTED_TYPES && !this.options.xml) { // Disable all XML-based types by default @@ -304,6 +305,9 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { var pipe = http && http.pipe; var pipeDest = pipe && pipe.dest; var pipeSrc = pipe && pipe.source; + var ctx = this; + var defaultErrorStatus = http && http.errorStatus; + var res = this.res; if (pipeDest) { // only support response for now @@ -313,7 +317,7 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { this.res.header('Content-Type', 'application/json'); this.res.header('Transfer-Encoding', 'chunked'); - var stream = method.invoke(scope, args, this.options, fn); + var stream = method.invoke(scope, args, this.options, ctx, fn); stream.pipe(this.res); break; default: @@ -324,7 +328,7 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { // only support request for now switch (pipeDest) { case 'req': - this.req.pipe(method.invoke(scope, args, this.options, fn)); + this.req.pipe(method.invoke(scope, args, this.options, ctx, fn)); break; default: fn(new Error('unsupported pipe source')); @@ -332,7 +336,42 @@ HttpContext.prototype.invoke = function(scope, method, fn, isCtor) { } } else { // simple invoke - method.invoke(scope, args, this.options, fn); + method.invoke(scope, args, this.options, ctx, function(err, result) { + if (err) { + if (defaultErrorStatus && + (res.statusCode === undefined || res.statusCode === 200)) { + res.status(defaultErrorStatus); + } + return fn(err); + } + fn(null, result); + }); + } +}; + +HttpContext.prototype.setReturnArgByName = function(name, value) { + var returnDesc = this.method.getReturnArgDescByName(name); + var result = this.result; + var res = this.res; + + if (!returnDesc) { + return debug('warning: cannot set return value for arg' + + ' (%s) without description!', name); + } + + if (returnDesc.root) { + this.result = value; + } else if (returnDesc.http) { + switch (returnDesc.http.target) { + case 'status': + res.status(value); + break; + case 'header': + res.set(returnDesc.header || name, value); + break; + } + } else { + result[name] = value; } }; @@ -384,6 +423,11 @@ HttpContext.prototype.done = function() { var data = this.result; var res = this.res; var accepts = this.req.accepts(this.supportedTypes); + var defaultStatus = this.method.http.status; + + if (defaultStatus) { + res.status(defaultStatus); + } if (this.req.query._format) { accepts = this.req.query._format.toLowerCase(); diff --git a/lib/rest-adapter.js b/lib/rest-adapter.js index 02391df..5df4152 100644 --- a/lib/rest-adapter.js +++ b/lib/rest-adapter.js @@ -359,7 +359,9 @@ RestAdapter.errorHandler = function(options) { err.status = err.statusCode = 500; } - res.statusCode = err.statusCode || err.status || 500; + if (res.statusCode === undefined || res.statusCode === 200) { + res.statusCode = err.statusCode || err.status || 500; + } debug('Error in %s %s: %s', req.method, req.url, err.stack); var data = { diff --git a/lib/shared-method.js b/lib/shared-method.js index 29f43df..2401885 100644 --- a/lib/shared-method.js +++ b/lib/shared-method.js @@ -24,6 +24,9 @@ var assert = require('assert'); * @param {Boolean} [options.isStatic] Is the method a static method or a * a `prototype` method * @param {Array} [options.aliases] A list of aliases for the + * @param {Array} [options.http] HTTP only options + * @param {Number} [options.http.status] The default status code + * @param {Number} [options.http.errorStatus] The default error status code * `sharedMethod.name` * @param {Array|Object} [options.accepts] An `Array` of argument definitions * that describe the arguments of the `SharedMethod`. @@ -144,6 +147,17 @@ SharedMethod.fromFunction = function(fn, name, sharedClass, isStatic) { }); }; +SharedMethod.prototype.getReturnArgDescByName = function(name) { + var returns = this.returns; + var desc; + for (var i = 0; i < returns.length; i++) { + desc = returns[i]; + if (desc && ((desc.arg || desc.name) === name)) { + return desc; + } + } +}; + /** * Execute the remote method using the given arg data. * @@ -153,7 +167,7 @@ SharedMethod.fromFunction = function(fn, name, sharedClass, isStatic) { * @param {Function} cb callback `fn(err, result)` containing named result data */ -SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) { +SharedMethod.prototype.invoke = function(scope, args, remotingOptions, ctx, cb) { var accepts = this.accepts; var returns = this.returns; var errors = this.errors; @@ -161,6 +175,11 @@ SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) { var sharedMethod = this; var formattedArgs = []; + if (typeof ctx === 'function') { + cb = ctx; + ctx = undefined; + } + if (cb === undefined && typeof remotingOptions === 'function') { cb = remotingOptions; remotingOptions = {}; @@ -189,8 +208,9 @@ SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) { if (err) { return cb(err); } - - var result = SharedMethod.toResult(returns, [].slice.call(arguments, 1)); + // args without err + var rawArgs = [].slice.call(arguments, 1); + var result = SharedMethod.toResult(returns, rawArgs, ctx); debug('- %s - result %j', sharedMethod.name, result); @@ -457,7 +477,7 @@ SharedMethod.convertArg = function(accept, raw) { * results from a remoting function, based on `returns`. */ -SharedMethod.toResult = function(returns, raw) { +SharedMethod.toResult = function(returns, raw, ctx) { var result = {}; if (!returns.length) { @@ -471,6 +491,7 @@ SharedMethod.toResult = function(returns, raw) { if (item.root) { result = convert(raw[index]); + if (ctx) ctx.setReturnArgByName(item.name || item.arg, raw[index]); return false; } @@ -478,7 +499,10 @@ SharedMethod.toResult = function(returns, raw) { }); returns.forEach(function(item, index) { - result[item.name || item.arg] = convert(raw[index]); + var value = convert(raw[index]); + var name = item.name || item.arg; + result[name] = value; + if (ctx) ctx.setReturnArgByName(name, value); }); return result; diff --git a/test/authorize-hook.test.js b/test/authorize-hook.test.js index bb87a4b..043aa70 100644 --- a/test/authorize-hook.test.js +++ b/test/authorize-hook.test.js @@ -55,4 +55,3 @@ describe('authorization hook', function() { remotes.invoke(method, args, callback); } }); - diff --git a/test/rest.test.js b/test/rest.test.js index 4674ec4..8dd6a3f 100644 --- a/test/rest.test.js +++ b/test/rest.test.js @@ -1753,17 +1753,63 @@ describe('strong-remoting-rest', function() { }); }); - it('returns 404 for unknown method of a shared class', function(done) { - var classUrl = givenSharedStaticMethod().classUrl; + describe('status codes', function() { + describe('using a custom satus code', function() { + it('returns a custom status code', function(done) { + var method = givenSharedStaticMethod( + function fn(cb) { + cb(); + }, + { + http: { status: 201 } + } + ); + json(method.url) + .expect(201, done); + }); + it('returns a custom error status code', function(done) { + var method = givenSharedStaticMethod( + function fn(cb) { + cb(new Error('test error')); + }, + { + http: { status: 201, errorStatus: 508 } + } + ); + json(method.url) + .expect(508, done); + }); + it('returns a custom status code from a callback arg', function(done) { + var exampleStatus = 222; + var method = givenSharedStaticMethod( + function fn(status, cb) { + cb(null, status); + }, + { + accepts: { arg: 'status', type: 'number' }, + returns: { + arg: 'status', + http: { target: 'status' } + } + } + ); + json(method.url + '?status=' + exampleStatus) + .expect(exampleStatus, done); + }); + }); - json(classUrl + '/unknown-method') - .expect(404, done); - }); + it('returns 404 for unknown method of a shared class', function(done) { + var classUrl = givenSharedStaticMethod().classUrl; + + json(classUrl + '/unknown-method') + .expect(404, done); + }); - it('returns 404 with standard JSON body for uknown URL', function(done) { - json('/unknown-url') - .expect(404) - .end(expectErrorResponseContaining({status: 404}, done)); + it('returns 404 with standard JSON body for uknown URL', function(done) { + json('/unknown-url') + .expect(404) + .end(expectErrorResponseContaining({status: 404}, done)); + }); }); it('returns correct error response body', function(done) { From f01b8a6ba0b157f3e6562d11158bc3050a75d61a Mon Sep 17 00:00:00 2001 From: Ritchie Martori Date: Tue, 23 Jun 2015 12:47:26 -0700 Subject: [PATCH 3/3] Fix iojs error in tests --- test/streams.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/streams.js b/test/streams.js index 8454993..8f8b857 100644 --- a/test/streams.js +++ b/test/streams.js @@ -31,7 +31,10 @@ describe('strong-remoting', function() { it('should stream the file output', function(done) { remotes.fs = fs; fs.createReadStream.shared = true; - fs.createReadStream.accepts = [{arg: 'path', type: 'string'}]; + fs.createReadStream.accepts = [ + { arg: 'path', type: 'string' }, + { arg: 'encoding', type: 'string' } + ]; fs.createReadStream.returns = {arg: 'res', type: 'stream'}; fs.createReadStream.http = { verb: 'get', @@ -41,7 +44,7 @@ describe('strong-remoting', function() { } }; - json('get', '/fs/createReadStream?path=' + __dirname + '/data/foo.json') + json('get', '/fs/createReadStream?path=' + __dirname + '/data/foo.json&encoding=utf8') .expect({bar: 'baz'}, done); }); });