Merge pull request #57 from osher/patch-1

use request logger whenever it's available
apigee-127 · Sep 20, 2016 · a44711b · a44711b
2 parents 90f8dcd + 73f7557
commit a44711b
Show file tree

Hide file tree

Showing 2 changed files with 137 additions and 13 deletions.
diff --git a/fittings/json_error_handler.js b/fittings/json_error_handler.js
@@ -12,31 +12,50 @@ module.exports = function create(fittingDef, bagpipes) {
     if (!util.isError(context.error)) { return next(); }
 
     var err = context.error;
+    var log;
+    var body;
 
     debug('exec: %s', context.error.message);
-
-    try {
-      if (!context.statusCode || context.statusCode < 400) {
-        if (context.response && context.response.statusCode && context.response.statusCode >= 400) {
-          context.statusCode = context.response.statusCode;
-        } else if (err.statusCode && err.statusCode >= 400) {
-          context.statusCode = err.statusCode;
-          delete(err.statusCode);
-        } else {
-          context.statusCode = 500;
-        }
+
+    if (!context.statusCode || context.statusCode < 400) {
+      if (context.response && context.response.statusCode && context.response.statusCode >= 400) {
+        context.statusCode = context.response.statusCode;
+      } else if (err.statusCode && err.statusCode >= 400) {
+        context.statusCode = err.statusCode;
+        delete(err.statusCode);
+      } else {
+        context.statusCode = 500;
       }
+    }
 
+    try {
+      //TODO: find what's throwing here...
       if (context.statusCode === 500 && !fittingDef.handle500Errors) { return next(err); }
+      //else - from here we commit to emitting error as JSON, no matter what.
 
       context.headers['Content-Type'] = 'application/json';
       Object.defineProperty(err, 'message', { enumerable: true }); // include message property in response
 
       delete(context.error);
       next(null, JSON.stringify(err));
     } catch (err2) {
-      debug('jsonErrorHandler unable to stringify error: %j', err);
-      next();
+      log = context.request && ( 
+               context.request.log 
+            || context.request.app && context.request.app.log
+            )
+         || context.response && context.response.log;
+
+      body = { 
+        message: "unable to stringify error properly",
+        stringifyErr: err2.message,
+        originalErrInspect: util.inspect(err)
+      };
+      context.statusCode = 500;
+
+      debug('jsonErrorHandler unable to stringify error: ', err);
+      if (log) log.error(err2, "onError: json_error_handler - unable to stringify error", err);
+
+      next(null, JSON.stringify(body));
     }
   }
 };
diff --git a/test/fittings/json_error_handler.js b/test/fittings/json_error_handler.js
@@ -124,7 +124,111 @@ describe('json_error_handler', function() {
     });
   });
 
+  describe('handle500Errors:true and error fails to stringify', function() { 
+    var jsonErrorHandler;
+    var mockErr;
+    var context;
+    var err;
+
+    before(function() {
+      jsonErrorHandler = json_error_handler({ handle500Errors: true });
+      mockErr          = new Error('this is a test');
+      mockErr.circular = mockErr; //force stringification error
+    });
+
+    describe('and context has a logger on request', function() {
+      before(function(done) {
+        context = {
+          headers: {},
+          request: {
+            log: { 
+              error: function() { this.lastErr = arguments } 
+            }
+          },
+          error: mockErr
+        };
+        jsonErrorHandler(context, function(e) {
+            err = e;
+            done()
+        });
+      });
+      it('should not fail', function() {
+          should.not.exist(err);
+      })
+      it('should remove the error from the context', function() {
+          should.not.exist(context.error);
+      });
+      it('should pass stringification error to the logger', function() {
+          should.exist(context.request.log.lastErr, "error was not passed to log");
+          should(context.request.log.lastErr.length).eql(3);
+          should(context.request.log.lastErr[2]).equal(mockErr);
+      });
+    });
+
+    describe('and context has no logger on req.log, but has on request.app.log', function() {
+      before(function(done) {
+        context = {
+          headers: {},
+          request: {
+            app: {
+              log: { 
+                error: function() { this.lastErr = arguments } 
+              }
+            }
+          },
+          error: mockErr
+        };
+        jsonErrorHandler(context, function(e) {
+            err = e;
+            done()
+        });
+      });
+      it('should not fail', function() {
+          should.not.exist(err);
+      })
+      it('should remove the error from the context', function() {
+          should.not.exist(context.error);
+      });
+      it('should pass stringification error to the logger', function() {
+          should.exist(context.request.app.log.lastErr, "error was not passed to log");
+          should(context.request.app.log.lastErr.length).eql(3);
+          should(context.request.app.log.lastErr[2]).equal(mockErr);
+      });
+    });
+
+    describe('and context has no logger on request, but has on response', function() {
+
+      beforeEach(function(done) {
+        context = {
+          headers: {},
+          response: {
+            log: { 
+              error: function() { this.lastErr = arguments } 
+            }
+          },
+          error: mockErr
+        };
+        jsonErrorHandler(context, function(e) {
+            err = e;
+            done()
+        });
+
+      });
 
+      it('should not fail', function() {
+          should.not.exist(err);
+      });
+      it('should remove the error from the context', function() {
+          should.not.exist(context.error);
+      });
+      it('should pass stringification error to the logger', function() {
+          should.exist(context.response.log.lastErr, "error was not passed to log");
+          should(context.response.log.lastErr.length).eql(3);
+          should(context.response.log.lastErr[2]).equal(mockErr);
+      });
+    });
+  });
+
 
   describe('no error in context', function() {
 
@@ -160,4 +264,5 @@ describe('json_error_handler', function() {
       });
     });
   });
+
 });