Add support for declaring status code defaults and custom http headers

[ritch]

/to @bajtos
/cc @raymondfeng @dashby3000

This adds a few new features:

• declare a default status and errorStatus code in your method.http options
• returns.http.target
  • status sets the res.statusCode to the provided value
  • header sets the returns.header or returns.arg named header to the value
• set a custom header name for a header arg: returns: {arg: 'length', {http: {target: 'header', header: 'Content-Length'}}}

describe('using a custom status 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);
  });
});

Connect to #186

[ritch]

test please

[bajtos]

How about other 2xx status codes, like 204 No Content? I am proposing to rewrite the condition to res.statusCode === undefined || res.statusCode < 400

[bajtos]

I am not sure if it's a good idea to add a constant errorStatus code, I am concerned that it may mask useful status codes like 422 (validation failed), 400 (bad request, caused by a missing argument) etc. Could you please add a unit- test to verify that err.statusCode takes precedence over errorStatus defined in remoting metadata?

       var method = givenSharedStaticMethod(
         function fn(cb) {
           var err = new Error('test error');
           err.statusCode = 400;
           cb(err);
         },
         {
           http: { status: 201, errorStatus: 508 }
         }
       );
       json(method.url)
         .expect(400, done);

[bajtos]

Awesome stuff!

It seems to me that passing ctx to SharedMethod.prototype.invoke is a bit inelegant and creates coupling that may make future refactoring more difficult. I don't have a better solution at hand though, so don't worry about it too much.

[ritch]

I am not sure if it's a good idea to add a constant errorStatus code, I am concerned that it may mask useful status codes like 422 (validation failed), 400 (bad request, caused by a missing argument) etc.

Well we need some way of declaring the status code for a specific result (eg. when the result is ok send this status, when the result is an error send this status).

I think the err.status should take precedence. So if that is set we will just ignore the errorStatus default.

[crandmck]

@ritch Remote methods' accepts & returns are documented here: http://apidocs.strongloop.com/strong-remoting/#sharedmethod (in source file https://github.com/strongloop/strong-remoting/blob/master/lib/shared-method.js). It might be best for you to add docs, since I'm not familiar with this PR. But if you want me to do it, please just open a doc issue.

[bajtos]

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

[ritch]

Added the check...

[bajtos]

The code LGTM, although I left one comment/question for your consideration. Please make sure the CI builds are green before landing this patch.

[auguster]

I'm having a use case with long video processing, sometimes the video is ready (status code 200) or processing (202). You can get the detailed status on a specific method that returns a status 201 if the processing has finished with the URI on where to get it.

So far I'm setting the status code through new Error(...).statusCode the problem is that 2xx are not errors and strong-error-handler is replacing status code < 400 by 500.

What has been proposed so far (setting static return status) doesn't work in my use case as I can return 200 or 202.

I'm looking for the best way to return non-error status codes. I guess I could go into the ctx.res and manually set this but that feels so wrong. What should I do ?

[bajtos]

@auguster Your method should have a callback argument that's annotated with http: {target: 'status'}. See https://loopback.io/doc/en/lb3/Remote-methods.html#argument-descriptions
and the test-cases checking this functionality:

strong-remoting/test/rest.test.js

Lines 2052 to 2095 in 4ef9baa

	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 a custom status code from a promise returned value', function(done) {
	var exampleStatus = 222;
	var sentBody = {eiste: 'ligo', kopries: true};
	var method = givenSharedStaticMethod(
	function fn() {
	return Promise.resolve([exampleStatus, sentBody]);
	},
	{
	returns: [{
	arg: 'status',
	http: {target: 'status'},
	}, {
	arg: 'result',
	root: true,
	type: 'object',
	}],
	}
	);
	json(method.url)
	.expect(exampleStatus)
	.then(function(response) {
	expect(response.body).to.deep.equal(sentBody);
	done();
	})
	.catch(done);
	});
	});

[auguster]

@bajtos I will try that, thanks.