Add custom response renderer options. #67

charlie-s · 2018-01-04T22:53:20Z

Description

Adds functionality to overwrite existing renderer functions (sendHTML, sendJSON, sendXML) and adds a new sendText option for text/plain.

One issue that I saw while writing tests is that all JSON tests are running with .set('Accept', 'text/plain'). Why is this not application/json? Will this break expectations? See this comment for similar concern.

I'm not a fan of having to have test/custom-renderers/send-*.js files, but since they're being included with require() I don't know the best way to handle.

Note that https://loopback.io/doc/ja/lb3/Using-strong-error-handler.html would need updating if this PR eventually happens.

Related issues

connect to Customize HTML templates #3
connect to Plain-text output #6

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

bajtos · 2018-01-05T11:43:06Z

Hello @charlie-s, thank you for the pull request. Your proposal looks reasonably, I'll take a look at the code changes next week.

bajtos · 2018-01-12T16:18:42Z

I am afraid I did not manage to find time to review this patch yet. Sorry for letting your patch open for so long 😢

charlie-s · 2018-01-12T17:05:00Z

No worries @bajtos, whenever you have the time.

bajtos

Thank you for the work you put into this pull request! Most of the code itself looks reasonable, however I have different opinions about the high-level design, which I would like to discuss with you, so that we can find the best solution both for our users and us, the module maintainers. See my comments below.

bajtos · 2018-01-15T08:11:20Z

README.md

@@ -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`). |


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:

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.

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.

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

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 $!.

Good point.

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?

bajtos · 2018-01-15T08:16:57Z

README.md

+}
+```
+
+The `$!` characters indicate that the path is relative to the location of `middleware.json`.


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.)

bajtos · 2018-01-15T08:17:44Z

lib/content-negotiation.js

@@ -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');


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

Your suggestion would make me happier than this current method.

bajtos · 2018-01-15T08:21:21Z

lib/content-negotiation.js

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

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


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:

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

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

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);

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

bajtos · 2018-01-15T08:22:28Z

lib/send-text.js

+
+'use strict';
+
+var format = require('util').format;


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

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.

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.

bajtos · 2018-01-15T08:25:06Z

lib/send-text.js

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


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.

bajtos · 2018-01-15T08:25:22Z

test/custom-renderers/send-html.js

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


Use const.

bajtos · 2018-01-15T08:28:08Z

test/custom-renderers/send-html.js

@@ -0,0 +1,12 @@
+// Copyright IBM Corp. 2016. All Rights Reserved.


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

bajtos · 2018-01-15T08:33:51Z

test/custom-renderers/send-html.js

+module.exports = function sendHtml(res, data) {
+  var content = '<html><body>' + data.message + '</body></html>';
+  res.setHeader('Content-Type', 'text/html; charset=utf-8');
+  res.end(content, 'utf-8');


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?

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.

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

bajtos · 2018-02-02T13:39:13Z

@charlie-s ping, what's the status of this pull request? Are you still keen to get it finished?

charlie-s · 2018-02-02T16:55:44Z

@bajtos Still keen to get it finished. Apologies on the delay, I quite appreciate your responses and feedback on this. I should be able to get back on this within the next 2 weeks.

bajtos · 2018-02-06T14:42:37Z

Sounds great! Please leave a comment when you have new code to review, I am not receiving notifications about pushed commits.

iamsenorespana · 2018-03-14T13:35:03Z

@charlie-s I can help finish this. Where are we with this? I would love to see this done and implemeted. Let me know.

charlie-s · 2018-03-14T18:32:57Z

@jamilhassanspain Some help would be great! I've been busier than expected and haven't been able to jump back on this. All of the hunks above that bajtos pointed out need to be addressed, either by following his recommendations or suggesting alternatives. Most are straightforward, only a couple need some back-and-forth to finalize the code. I can give you write access to the fork and you can help on whichever items you'd like – sound good?

bajtos · 2018-07-31T15:20:13Z

I am closing this pull request as abandoned. Feel free to re-open (or submit a new one) if you get time to work on this again and address my review comments above.

Add custom response renderer options.

7dc750c

charlie-s requested review from bajtos and zbarbuto as code owners January 4, 2018 22:53

bajtos self-assigned this Jan 5, 2018

bajtos suggested changes Jan 15, 2018

View reviewed changes

bajtos mentioned this pull request Jan 19, 2018

Escape strings in HTML output (XSS fix) #69

Merged

2 tasks

bajtos closed this Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom response renderer options. #67

Add custom response renderer options. #67

charlie-s commented Jan 4, 2018 •

edited

bajtos commented Jan 5, 2018

bajtos commented Jan 12, 2018

charlie-s commented Jan 12, 2018

bajtos left a comment

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 19, 2018

bajtos Jan 15, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 19, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 15, 2018

charlie-s Jan 18, 2018

bajtos Jan 19, 2018

bajtos commented Feb 2, 2018

charlie-s commented Feb 2, 2018

bajtos commented Feb 6, 2018

iamsenorespana commented Mar 14, 2018

charlie-s commented Mar 14, 2018

bajtos commented Jul 31, 2018

		@@ -0,0 +1,12 @@
		// Copyright IBM Corp. 2016. All Rights Reserved.

Add custom response renderer options. #67

Add custom response renderer options. #67

Conversation

charlie-s commented Jan 4, 2018 • edited

Description

Related issues

Checklist

bajtos commented Jan 5, 2018

bajtos commented Jan 12, 2018

charlie-s commented Jan 12, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 2, 2018

charlie-s commented Feb 2, 2018

bajtos commented Feb 6, 2018

iamsenorespana commented Mar 14, 2018

charlie-s commented Mar 14, 2018

bajtos commented Jul 31, 2018

charlie-s commented Jan 4, 2018 •

edited