feat: add options.rootProperty for json/xml

Using `error` as the root property is a good default but we should be able to use a different one or don't use it at all.
2020-05-20 14:33:11 -07:00 · 2020-05-20 14:33:11 -07:00 · bda981feb8
parent 36d2b18a17
commit bda981feb8
7 changed files with 126 additions and 41 deletions
--- a/README.md
+++ b/README.md
@ -14,9 +14,9 @@ In debug mode, `strong-error-handler` returns full error stack traces and intern

 ## Supported versions

-Current|Long Term Support|Maintenance
-:-:|:-:|:-:
-3.x|2.x|1.x
+| Current | Long Term Support | Maintenance |
+| :-----: | :---------------: | :---------: |
+|   4.x   |        3.x        |     2.x     |

 Learn more about our LTS plan in [docs](http://loopback.io/doc/en/contrib/Long-term-support.html).

@ -109,12 +109,13 @@ The content type of the response depends on the request's `Accepts` header.
 ## Options

 | Option               | Type                      | Default   | Description                                                                                                                                                                                                                                                                                                                                               |
-| ---- | ---- | ---- | ---- |
+| -------------------- | ------------------------- | --------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | debug                | Boolean&nbsp;&nbsp;&nbsp; | `false`   | If `true`, HTTP responses include all error properties, including sensitive data such as file paths, URLs and stack traces. See [Example output](#example) below.                                                                                                                                                                                         |
 | log                  | Boolean                   | `true`    | If `true`, all errors are printed via `console.error`, including an array of fields (custom error properties) that are safe to include in response messages (both 4xx and 5xx). <br/> If `false`, sends only the error back in the response.                                                                                                              |
 | 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.
+| defaultType          | String                    | `"json"`  | Specifies the default response content type to use when the client does not provide any Accepts header.                                                                                                                                                                                                                                                   |
+| rootProperty         | String or false           | `"error"` | Specifies the root property name for json or xml. If the value is set to `false`, no wrapper will be added to the json object. The false value is ignored by XML as a root element is always required.                                                                                                                                                    |
+| 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. |

 ### Customizing log format

@ -125,7 +126,7 @@ For example, in an Express application:

 ```js
 app.use(myErrorLogger());
-app.use(errorHandler({ log: false }));
+app.use(errorHandler({log: false}));
 ```

 In general, add `strong-error-handler` as the last middleware function, just before calling `app.listen()`.
--- a/index.d.ts
+++ b/index.d.ts
@ -53,6 +53,7 @@ declare namespace errorHandlerFactory {
    safeFields?: string[];
    defaultType?: string;
    negotiateContentType?: boolean;
+    rootProperty?: string | false;
  }

  /**
--- a/lib/handler.js
+++ b/lib/handler.js
@ -10,7 +10,6 @@ const SG = require('strong-globalize');
 SG.SetRootDir(path.resolve(__dirname, '..'));
 const buildResponseData = require('./data-builder');
 const debug = require('debug')('strong-error-handler');
-const format = require('util').format;
 const logToConsole = require('./logger');
 const negotiateContentProducer = require('./content-negotiation');

@ -50,7 +49,7 @@ function writeErrorToResponse(err, req, res, options) {

  options = options || {};

-  if (res._header) {
+  if (res.headersSent) {
    debug('Response was already sent, closing the underlying connection');
    return req.socket.destroy();
  }
@ -66,7 +65,7 @@ function writeErrorToResponse(err, req, res, options) {
  res.statusCode = data.statusCode;

  const sendResponse = negotiateContentProducer(req, warn, options);
-  sendResponse(res, data);
+  sendResponse(res, data, options);

  function warn(msg) {
    res.header('X-Warning', msg);
--- a/lib/send-html.js
+++ b/lib/send-html.js
@ -17,10 +17,10 @@ const compiledTemplates = {
 module.exports = sendHtml;

 function sendHtml(res, data, options) {
-  const toRender = {options: {}, data: data};
+  const toRender = {options, data};
  // TODO: ability to call non-default template functions from options
  const body = compiledTemplates.default(toRender);
-  sendReponse(res, body);
+  sendResponse(res, body);
 }

 /**
@ -41,7 +41,7 @@ function loadDefaultTemplates() {
  return compileTemplate(defaultTemplate);
 }

-function sendReponse(res, body) {
+function sendResponse(res, body) {
  res.setHeader('Content-Type', 'text/html; charset=utf-8');
  res.end(body);
 }
--- a/lib/send-json.js
+++ b/lib/send-json.js
@ -7,8 +7,14 @@

 const safeStringify = require('fast-safe-stringify');

-module.exports = function sendJson(res, data) {
-  const content = safeStringify({error: data});
+module.exports = function sendJson(res, data, options) {
+  options = options || {};
+  // Set `options.rootProperty` to not wrap the data into an `error` object
+  const err = options.rootProperty === false ? data : {
+    // Use `options.rootProperty`, if not set, default to `error`
+    [options.rootProperty || 'error']: data,
+  };
+  const content = safeStringify(err);
  res.setHeader('Content-Type', 'application/json; charset=utf-8');
  res.end(content, 'utf-8');
 };
--- a/lib/send-xml.js
+++ b/lib/send-xml.js
@ -7,8 +7,12 @@

 const js2xmlparser = require('js2xmlparser');

-module.exports = function sendXml(res, data) {
-  const content = js2xmlparser.parse('error', data);
+module.exports = function sendXml(res, data, options) {
+  options = options || {};
+  // Xml always requires a root element.
+  // `options.rootProperty === false` is not honored
+  const root = options.rootProperty || 'error';
+  const content = js2xmlparser.parse(root, data);
  res.setHeader('Content-Type', 'text/xml; charset=utf-8');
  res.end(content, 'utf-8');
 };
--- a/test/handler.test.js
+++ b/test/handler.test.js
@ -78,7 +78,7 @@ describe('strong-error-handler', function() {
      request.get('/').expect(
        507,
        {error: {statusCode: 507, message: 'Insufficient Storage'}},
-        done
+        done,
      );
    });
  });
@ -146,7 +146,7 @@ describe('strong-error-handler', function() {
    it('handles array argument', function(done) {
      givenErrorHandlerForError(
        [new TypeError('ERR1'), new Error('ERR2')],
-        {log: true}
+        {log: true},
      );

      request.get('/api').end(function(err) {
@ -541,6 +541,32 @@ describe('strong-error-handler', function() {
      });
    });

+    it('honors rootProperty', function(done) {
+      givenErrorHandlerForError('Error Message', {rootProperty: 'data'});
+      requestJson().expect(500).end(function(err, res) {
+        if (err) return done(err);
+
+        expect(res.body.data).to.eql({
+          statusCode: 500,
+          message: 'Internal Server Error',
+        });
+        done();
+      });
+    });
+
+    it('honors rootProperty=false', function(done) {
+      givenErrorHandlerForError('Error Message', {rootProperty: false});
+      requestJson().expect(500).end(function(err, res) {
+        if (err) return done(err);
+
+        expect(res.body).to.eql({
+          statusCode: 500,
+          message: 'Internal Server Error',
+        });
+        done();
+      });
+    });
+
    function requestJson(url) {
      return request.get(url || '/')
        .set('Accept', 'text/plain')
@ -581,10 +607,10 @@ describe('strong-error-handler', function() {
            expect(res.statusCode).to.eql(404);
            const body = res.error.text;
            expect(body).to.match(
-              /<title>Error&lt;img onerror=alert\(1\) src=a&gt;<\/title>/
+              /<title>Error&lt;img onerror=alert\(1\) src=a&gt;<\/title>/,
            );
            expect(body).to.match(
-              /with id &lt;img onerror=alert\(1\) src=a&gt; found for Model/
+              /with id &lt;img onerror=alert\(1\) src=a&gt; found for Model/,
            );
            done();
          });
@ -602,7 +628,7 @@ describe('strong-error-handler', function() {
          .expect(/<title>ErrorWithProps<\/title>/)
          .expect(
            /500(.*?)a test error message&lt;img onerror=alert\(1\) src=a&gt;/,
-            done
+            done,
          );
      });

@ -696,7 +722,7 @@ describe('strong-error-handler', function() {
          expect(body).to.not.match(/<extra>sensitive data<\/extra>/);
          expect(body).to.match(/<name>ValidationError<\/name>/);
          expect(body).to.match(
-            /<message>The model instance is not valid.<\/message>/
+            /<message>The model instance is not valid.<\/message>/,
          );
          done();
        });
@ -728,6 +754,54 @@ describe('strong-error-handler', function() {
        });
    });

+    it('honors options.rootProperty', function(done) {
+      const error = new ErrorWithProps({
+        name: 'ValidationError',
+        message: 'The model instance is not valid.',
+        statusCode: 422,
+        details: 'some details',
+        extra: 'sensitive data',
+      });
+      givenErrorHandlerForError(error, {rootProperty: 'myRoot'});
+      requestXML()
+        .end(function(err, res) {
+          expect(res.statusCode).to.eql(422);
+          const body = res.error.text;
+          expect(body).to.match(/<myRoot>/);
+          expect(body).to.match(/<details>some details<\/details>/);
+          expect(body).to.not.match(/<extra>sensitive data<\/extra>/);
+          expect(body).to.match(/<name>ValidationError<\/name>/);
+          expect(body).to.match(
+            /<message>The model instance is not valid.<\/message>/,
+          );
+          done();
+        });
+    });
+
+    it('ignores options.rootProperty = false', function(done) {
+      const error = new ErrorWithProps({
+        name: 'ValidationError',
+        message: 'The model instance is not valid.',
+        statusCode: 422,
+        details: 'some details',
+        extra: 'sensitive data',
+      });
+      givenErrorHandlerForError(error, {rootProperty: false});
+      requestXML()
+        .end(function(err, res) {
+          expect(res.statusCode).to.eql(422);
+          const body = res.error.text;
+          expect(body).to.match(/<error>/);
+          expect(body).to.match(/<details>some details<\/details>/);
+          expect(body).to.not.match(/<extra>sensitive data<\/extra>/);
+          expect(body).to.match(/<name>ValidationError<\/name>/);
+          expect(body).to.match(
+            /<message>The model instance is not valid.<\/message>/,
+          );
+          done();
+        });
+    });
+
    function requestXML(url) {
      return request.get(url || '/')
        .set('Accept', 'text/xml')