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.

Sign up for GitHub

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 custom response renderer options. #67

Closed
wants to merge 1 commit into from
Closed

Add custom response renderer options. #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 46 additions & 14 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,16 @@ The content type of the response depends on the request's `Accepts` header.
| safeFields | [String] | `[]` | Specifies property names on errors that are allowed to be passed through in 4xx and 5xx responses. See [Safe error fields](#safe-error-fields) below. |
| defaultType | String | `"json"` | Specify the default response content type to use when the client does not provide any Accepts header.
| negotiateContentType | Boolean | true | Negotiate the response content type via Accepts request header. When disabled, strong-error-handler will always use the default content type when producing responses. Disabling content type negotiation is useful if you want to see JSON-formatted error responses in browsers, because browsers usually prefer HTML and XML over other content types.
| htmlRenderer | String | 'send-html' | Operation function to render HTML with signature `fn(res, data)`. Defaults to `./send-html` (`lib/send-html.js`). |
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of customizable renderer functions 👍

I am concerned about the design you are proposing though. Passing in a string path may seem like a convenient solution in the context of LoopBack's middleware.json, but there are drawbacks too:

  1. When using strong-error-handler in vanilla Express applications, configuring it directly from JavaScript code, the usual convention is to pass functions, not paths to source code files.

    const app = express();
app.use(strongErrorHandler({
  log: true,
  debug: false,
  htmlRenderer: function(res, data) {
    // my code
  },
});

    This can be addressed by modifying the implementation to recognize both String and Function values.

  2. Using a path that's directly passed to require can lead to situation difficult to debug. Consider what happens when you forget to include $! prefix in your middleware.json entry: the path will be resolved relatively to strong-error-handler. Either a wrong file will be loaded, or what's more likely, "module not found" error will be printed.

    I am proposing to address this usability issue by checking whether the user-supplied path is absolute, and throwing an error with a helpful error message otherwise.

Thoughts?

Also please change the description of the default value. Consumers of this module should not need to understand our internal implementation. IMO, it's better to tell them that by default the built-in renderer is used. In fact, I think the rendered options should not have any default value per se, instead we should interpret a missing value as "use the built-in renderer".

| htmlRenderer | String | | An optional function to render HTML, the function should have signature `fn(res, data)`. Use this option to override the built-in HTML renderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Absolutely agree, would prefer it be definable as a function as well.

  2. If a path is supplied, are you suggesting it would be required to start with $! or else we'd throw a helpful error? I was thinking it would behave however core LoopBack behaves in this regard, where I thought you could do it with or without the $!.

  3. Good point.

Copy link
Member

Choose a reason for hiding this comment

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

If a path is supplied, are you suggesting it would be required to start with $! or else we'd throw a helpful error? I was thinking it would behave however core LoopBack behaves in this regard, where I thought you could do it with or without the $!.

The prefix $! is something that loopback-boot recognizes and expands into an absolute path. LoopBack runtime itself (loopback, strong-remoting, express) never see that value!

  • Here in strong-error-handler, we should enforce absolute paths to prevent user confusion.
  • The documentation for LoopBack users should use $! in the examples, that's how this new feature should be used in LoopBack apps
  • loopback-boot will take care of the rest

Thoughts?

| jsonRenderer | String | 'send-json' | Operation function to render JSON with signature `fn(res, data)`. Defaults to `./send-json` (`lib/send-json.js`). |
| textRenderer | String | 'send-text' | Operation function to render text with signature `fn(res, data)`. Defaults to `./send-text` (`lib/send-text.js`). |
| xmlRenderer | String | 'send-xml' | Operation function to render XML with signature `fn(res, data)`. Defaults to `./send-xml` (`lib/send-xml.js`). |

### Customizing log format

**Express**
**Express**

To use a different log format, add your own custom error-handling middleware then disable `errorHandler.log`.
To use a different log format, add your own custom error-handling middleware then disable `errorHandler.log`.
For example, in an Express application:

```js
Expand Down Expand Up @@ -157,6 +161,34 @@ Using the above configuration, an error containing an `errorCode` property will
}
```

### Custom Renderer Functions

You can switch out the default renderers for HTML, JSON, text and XML responses with custom renderers:

```
{
// ...
"final:after": {
"strong-error-handler": {
"params": {
"htmlRenderer": "$!./lib/send-html"
}
}
}
```

The `$!` characters indicate that the path is relative to the location of `middleware.json`.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a note that $! prefix is a feature provided by LoopBack. When using strong-error-handler directly from JavaScript code, users should provide the actual function. (Please include an example code for that too.)


*./lib/send-html.js:*

```
module.exports = function(res, data) {
res.send("Hello.");
}
```

Using the above configuration, a request with `Content-type: text/html` that is handled by strong-error-handler will return a `Content-type: text/html` and a response body of `Hello.`.

## Migration from old LoopBack error handler

NOTE: This is only required for applications scaffolded with old versions of the `slc loopback` tool.
Expand Down Expand Up @@ -203,7 +235,7 @@ To migrate a LoopBack 2.x application to use `strong-error-handler`:
}
</pre>

For more information, see
For more information, see
[Migrating apps to LoopBack 3.0](http://loopback.io/doc/en/lb3/Migrating-to-3.0.html#update-use-of-rest-error-handler).

## Example
Expand All @@ -221,17 +253,17 @@ The same error generated when `debug: true` :
{ statusCode: 500,
name: 'Error',
message: 'a test error message',
stack: 'Error: a test error message
at Context.<anonymous> (User/strong-error-handler/test/handler.test.js:220:21)
at callFnAsync (User/strong-error-handler/node_modules/mocha/lib/runnable.js:349:8)
at Test.Runnable.run (User/strong-error-handler/node_modules/mocha/lib/runnable.js:301:7)
at Runner.runTest (User/strong-error-handler/node_modules/mocha/lib/runner.js:422:10)
at User/strong-error-handler/node_modules/mocha/lib/runner.js:528:12
at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:342:14)
at User/strong-error-handler/node_modules/mocha/lib/runner.js:352:7
at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:284:14)
at Immediate._onImmediate (User/strong-error-handler/node_modules/mocha/lib/runner.js:320:5)
at tryOnImmediate (timers.js:543:15)
stack: 'Error: a test error message
at Context.<anonymous> (User/strong-error-handler/test/handler.test.js:220:21)
at callFnAsync (User/strong-error-handler/node_modules/mocha/lib/runnable.js:349:8)
at Test.Runnable.run (User/strong-error-handler/node_modules/mocha/lib/runnable.js:301:7)
at Runner.runTest (User/strong-error-handler/node_modules/mocha/lib/runner.js:422:10)
at User/strong-error-handler/node_modules/mocha/lib/runner.js:528:12
at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:342:14)
at User/strong-error-handler/node_modules/mocha/lib/runner.js:352:7
at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:284:14)
at Immediate._onImmediate (User/strong-error-handler/node_modules/mocha/lib/runner.js:320:5)
at tryOnImmediate (timers.js:543:15)
at processImmediate [as _immediateCallback] (timers.js:523:5)' }}
```

Expand Down
18 changes: 15 additions & 3 deletions lib/content-negotiation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
'use strict';
var accepts = require('accepts');
var debug = require('debug')('strong-error-handler:http-response');
var sendJson = require('./send-json');
var sendHtml = require('./send-html');
var sendXml = require('./send-xml');
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to keep all require calls at the top level. Perhaps rename sendJson to defaultSendJson (etc.)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion would make me happier than this current method.

var util = require('util');

var sendHtml,
sendJson,
sendText,
sendXml;

module.exports = negotiateContentProducer;

/**
Expand All @@ -26,6 +28,7 @@ function negotiateContentProducer(req, logWarning, options) {
var SUPPORTED_TYPES = [
'application/json', 'json',
'text/html', 'html',
'text/plain', 'text',
'text/xml', 'xml',
];

Expand All @@ -43,6 +46,12 @@ function negotiateContentProducer(req, logWarning, options) {
}
}

// resolve renderer functions
sendHtml = require(options.htmlRenderer || './send-html');
Copy link
Member

Choose a reason for hiding this comment

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

Overriding sendHtml that's shared in a global scope can create subtle bugs when negotiateContentProducer is called multiple times in parallel, each time with different options object.

I am proposing a different approach here:

  1. Instead of storing renderer functions in variables, store them in the options object itself.

  2. Modify resolveOperation to accept the options object, so that it can read renderer functions from there.

  3. Modifying properties of an object passed in as an argument is considered as anti-pattern, it often surprises users. Please shallow-clone the options object before making any changes:

    options = Object.assign({}, options);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good. I'll combine this with the note above about top-level requires and figure it out.

sendJson = require(options.jsonRenderer || './send-json');
sendText = require(options.textRenderer || './send-text');
sendXml = require(options.xmlRenderer || './send-xml');

// decide to use resolvedType or defaultType
// Please note that accepts assumes the order of content-type is provided
// in the priority returned
Expand Down Expand Up @@ -96,6 +105,9 @@ function resolveOperation(contentType) {
case 'text/html':
case 'html':
return sendHtml;
case 'text/plain':
case 'text':
return sendText;
case 'text/xml':
case 'xml':
return sendXml;
Expand Down
15 changes: 15 additions & 0 deletions lib/send-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright IBM Corp. 2016. All Rights Reserved.
// Node module: strong-error-handler
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

'use strict';

var format = require('util').format;
Copy link
Member

Choose a reason for hiding this comment

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

Use const - see http://loopback.io/doc/en/contrib/style-guide.html#variable-declarations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do so. I think this is the case in quite a few places. I only saw var in the current codebase and so stuck with that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we are conservative when it comes to code style changes and haven't upgrade the existing codebase for our new ES6 conventions. I know it can be confusing for new contributors.


module.exports = function sendText(res, data) {
var content = format('%s: %s [%s]', data.name, data.message,
data.statusCode);
Copy link
Member

Choose a reason for hiding this comment

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

This is a good start 👍

I am concerned that this format is missing important additional information, e.g. err.details from validation errors (in both production and debug model), additional fields like stack in debug mode.

Please check how the built-in html renderer is iterating over error properties and implement a similar algorithm here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

res.setHeader('Content-Type', 'text/plain; charset=utf-8');
res.end(content, 'utf-8');
};
12 changes: 12 additions & 0 deletions test/custom-renderers/send-html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright IBM Corp. 2016. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

The copyright should say 2018 in all newly added files.
Please move custom renderers to a different location - it's our convention to put test fixtures in test/fixtures directory:

test/fixtures/custom-renderers

Please use file names that are different from files containing the built-in renderers, for example:

custom-send-html.js
send-html.custom.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

// Node module: strong-error-handler
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

'use strict';

module.exports = function sendHtml(res, data) {
var content = '<html><body>' + data.message + '</body></html>';
Copy link
Member

Choose a reason for hiding this comment

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

Use const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

res.setHeader('Content-Type', 'text/html; charset=utf-8');
res.end(content, 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. You are proposing that custom renderer functions are not only responsible for rendering the error data object into a representation to be sent back to the client, but you are also asking the rendered to actually send the HTTP response.

I think that's adding unnecessary complexity to custom renderer functions and also allow our users to shoot their own feet by setting wrong content-type (e.g. setting content-type to text/plain in htmlRenderer).

I am proposing to simplify the design of custom renderers, so that these functions only render the response body (return back the string to be sent).

I understand there may be situations where the user wants to configure the HTTP response headers too, but I think we should find a different solution for that, and only after there is user demand for this feature.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, I was trying to copy the current implementation of sending the response from https://github.com/strongloop/strong-error-handler/blob/master/lib/send-html.js.

I think I would prefer to allow users to have the full res Express object, which would allow for async operations and other custom logic as needed.

But, thinking through it as you've suggested, it does seem like bad design that a consumer would have to know to res.json(obj); (or do it by hand via headers and JSON.stringify). I suppose it would be documented that the expectation of a custom renderer is to set the correct response type. I'm open to your thoughts of course, I just don't want to complicate it too much.

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I was trying to copy the current implementation of sending the response from https://github.com/strongloop/strong-error-handler/blob/master/lib/send-html.js.

That was a sensible thing to do! It's just that the existing design was not meant for custom user-supplied renderers.

But, thinking through it as you've suggested, it does seem like bad design that a consumer would have to know to res.json(obj) (or do it by hand via headers and JSON.stringify). I suppose it would be documented that the expectation of a custom renderer is to set the correct response type. I'm open to your thoughts of course, I just don't want to complicate it too much.

Here is what I am thinking:

  • IMO, most users will want to change the response body only and won't want to deal with HTTP headers etc. We should shield them from the additional complexity of producing a valid HTTP response. In other words, I am advocating for htmlRenderer that accepts the error data object and returns a string.

  • If we want to support async scenarios, we can allow these rendered functions to return either a value (a string) or a promise (that will eventually be resolved to a string).

  • For users looking for more advanced customization, I think it may be better if we allow them full control over both content-type negotiation and response serialization, e.g. replace the following block of code with their custom function passed in the options. Unless you need this feature, I prefer to leave this out of scope of this pull request.

    https://github.com/strongloop/strong-error-handler/blob/4c69370751755c0eb1a96e30a7f62182704839ba/lib/handler.js#L51-L55

};
15 changes: 15 additions & 0 deletions test/custom-renderers/send-json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright IBM Corp. 2016. All Rights Reserved.
// Node module: strong-error-handler
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

'use strict';

module.exports = function sendJson(res, data) {
var content = JSON.stringify({
error: data,
custom: 'custom data',
});
res.setHeader('Content-Type', 'application/json; charset=utf-8');
res.end(content, 'utf-8');
};
15 changes: 15 additions & 0 deletions test/custom-renderers/send-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright IBM Corp. 2016. All Rights Reserved.
// Node module: strong-error-handler
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

'use strict';

var format = require('util').format;

module.exports = function sendText(res, data) {
var content = format('%s: %s {%s}', data.name, data.message,
data.statusCode);
res.setHeader('Content-Type', 'text/plain; charset=utf-8');
res.end(content, 'utf-8');
};
15 changes: 15 additions & 0 deletions test/custom-renderers/send-xml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright IBM Corp. 2016. All Rights Reserved.
// Node module: strong-error-handler
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

'use strict';

var js2xmlparser = require('js2xmlparser');

module.exports = function sendXml(res, data) {
data.custom = 'custom data';
var content = js2xmlparser.parse('error', data);
res.setHeader('Content-Type', 'text/xml; charset=utf-8');
res.end(content, 'utf-8');
};
147 changes: 146 additions & 1 deletion test/handler.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,9 +446,33 @@ describe('strong-error-handler', function() {
});
});

it('uses custom defined JSON response renderer', function(done) {
var error = new ErrorWithProps({
message: 'Error Message',
statusCode: 500,
});
givenErrorHandlerForError(error, {
debug: true,
jsonRenderer: __dirname + '/custom-renderers/send-json',
});
requestJson().expect(500).end(function(err, res) {
if (err) return done(err);
expect(res.body).to.have.property('error');
expect(res.body.error).to.eql({
message: 'Error Message',
name: 'ErrorWithProps',
stack: error.stack,
statusCode: 500,
});
expect(res.body).to.have.property('custom');
expect(res.body.custom).to.eql('custom data');
done();
});
});

function requestJson(url) {
return request.get(url || '/')
.set('Accept', 'text/plain')
.set('Accept', 'application/json')
.expect('Content-Type', /^application\/json/);
}
});
Expand Down Expand Up @@ -519,13 +543,117 @@ describe('strong-error-handler', function() {
});
});

it('uses custom defined HTML response renderer', function(done) {
var error = new ErrorWithProps({
name: 'Error',
message: 'Error Message',
});
givenErrorHandlerForError(error, {
debug: true,
htmlRenderer: __dirname + '/custom-renderers/send-html',
});
requestHTML()
.end(function(err, res) {
var body = res.error.text;
expect(body).to.eql('<html><body>Error Message</body></html>');
done();
});
});

function requestHTML(url) {
return request.get(url || '/')
.set('Accept', 'text/html')
.expect('Content-Type', /^text\/html/);
}
});

context('text response', function() {
it('contains all error properties when debug=true', function(done) {
var error = new ErrorWithProps({
message: 'a test error message',
});
error.statusCode = 500;
givenErrorHandlerForError(error, {debug: true});
requestText()
.end(function(err, res) {
var body = res.error.text;
expect(body).to.match(/a test error message/);
expect(body).to.match(/\[500\]/);
done();
});
});

it('contains subset of properties when status=4xx', function(done) {
var error = new ErrorWithProps({
name: 'ValidationError',
message: 'The model instance is not valid.',
statusCode: 422,
extra: 'sensitive data',
});
givenErrorHandlerForError(error, {debug: true});
requestText()
.end(function(err, res) {
expect(res.statusCode).to.eql(422);
var body = res.error.text;
expect(body).to.match(/The model instance is not valid./);
expect(body).to.not.match(/sensitive data/);
expect(body).to.match(/\[422\]/);
done();
});
});

it('contains only safe info when status=5xx', function(done) {
// Mock an error reported by fs.readFile
var error = new ErrorWithProps({
name: 'Error',
message: 'ENOENT: no such file or directory, open "/etc/passwd"',
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: '/etc/password',
});
givenErrorHandlerForError(error);

requestText()
.end(function(err, res) {
expect(res.statusCode).to.eql(500);
var body = res.error.text;
expect(body).to.not.match(/\/etc\/password/);
expect(body).to.not.match(/-2/);
expect(body).to.not.match(/ENOENT/);
// only have the following
expect(body).to.match(/\[500\]/);
expect(body).to.match(/Internal Server Error/);
done();
});
});

it('uses custom defined text response renderer', function(done) {
var error = new ErrorWithProps({
name: 'Error',
message: 'Error Message',
statusCode: 500,
});
givenErrorHandlerForError(error, {
debug: true,
textRenderer: __dirname + '/custom-renderers/send-text',
});
requestText()
.end(function(err, res) {
var body = res.error.text;
console.log(body);
expect(body).to.match(/\{500\}/);
done();
});
});

function requestText(url) {
return request.get(url || '/')
.set('Accept', 'text/plain')
.expect('Content-Type', /^text\/plain/);
}
});

context('XML response', function() {
it('contains all error properties when debug=true', function(done) {
var error = new ErrorWithProps({
Expand Down Expand Up @@ -594,6 +722,23 @@ describe('strong-error-handler', function() {
});
});

it('uses custom defined XML response renderer', function(done) {
var error = new ErrorWithProps({
message: 'Error Message',
statusCode: 500,
});
givenErrorHandlerForError(error, {
debug: true,
xmlRenderer: __dirname + '/custom-renderers/send-xml',
});
requestXML()
.end(function(err, res) {
var body = res.error.text;
expect(body).to.match(/<custom>custom data<\/custom>/);
done();
});
});

function requestXML(url) {
return request.get(url || '/')
.set('Accept', 'text/xml')
Expand Down