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

Add support for declaring status code defaults and custom http headers #218

Merged
merged 4 commits into from
Jun 23, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions lib/http-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -324,15 +328,50 @@ 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'));
break;
}
} 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(err.status || err.statusCode || 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;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a unit-test for this?

}
} else {
result[name] = value;
}
};

Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Should we check that res.statusCode === undefined || res.statusCode === 200 before applying defaultStatus?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the check...

}

if (this.req.query._format) {
accepts = this.req.query._format.toLowerCase();
Expand Down
4 changes: 3 additions & 1 deletion lib/rest-adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
34 changes: 29 additions & 5 deletions lib/shared-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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.
*
Expand All @@ -153,14 +167,19 @@ 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;
var method = this.getFunction();
var sharedMethod = this;
var formattedArgs = [];

if (typeof ctx === 'function') {
cb = ctx;
ctx = undefined;
}

if (cb === undefined && typeof remotingOptions === 'function') {
cb = remotingOptions;
remotingOptions = {};
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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) {
Expand All @@ -471,14 +491,18 @@ SharedMethod.toResult = function(returns, raw) {

if (item.root) {
result = convert(raw[index]);
if (ctx) ctx.setReturnArgByName(item.name || item.arg, raw[index]);
return false;
}

return true;
});

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;
Expand Down
1 change: 0 additions & 1 deletion test/authorize-hook.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,3 @@ describe('authorization hook', function() {
remotes.invoke(method, args, callback);
}
});

112 changes: 104 additions & 8 deletions test/rest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1753,17 +1753,113 @@ 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) {
Expand Down
7 changes: 5 additions & 2 deletions test/streams.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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);
});
});