Merge pull request #185 from maxwellgerber/fix/throw-err-on-malformed…

…-logger

Fix/throw err on malformed logger

configuration.js

@@ -1,6 +1,7 @@
 const winston = require('winston');
 const InMemoryFeatureStore = require('./feature_store');
 const messages = require('./messages');
+const LoggerWrapper = require('./logger_wrapper');
 
 module.exports = (function() {
   const defaults = function() {
@@ -147,20 +148,23 @@ module.exports = (function() {
     }
   }
 
+  function fallbackLogger() {
+    return new winston.Logger({
+      level: 'info',
+      transports: [
+        new winston.transports.Console({
+          formatter: function(options) {
+            return '[LaunchDarkly] ' + (options.message ? options.message : '');
+          },
+        }),
+      ],
+    });
+  }
+
   function validate(options) {
     let config = Object.assign({}, options || {});
-    config.logger =
-      config.logger ||
-      new winston.Logger({
-        level: 'info',
-        transports: [
-          new winston.transports.Console({
-            formatter: function(options) {
-              return '[LaunchDarkly] ' + (options.message ? options.message : '');
-            },
-          }),
-        ],
-      });
+
+    config.logger = config.logger ? LoggerWrapper(config.logger, fallbackLogger()) : fallbackLogger();
 
     checkDeprecatedOptions(config);
 

logger_wrapper.js

@@ -0,0 +1,39 @@
+const logLevels = ['error', 'warn', 'info', 'debug'];
+
+/**
+ * Asserts that the caller-supplied logger contains all required methods
+ * and wraps it in an exception handler that falls back to the fallbackLogger
+ * @param {LDLogger} logger
+ * @param {LDLogger} fallbackLogger
+ */
+function LoggerWrapper(logger, fallbackLogger) {
+  validateLogger(logger);
+
+  const wrappedLogger = {};
+  logLevels.forEach(level => {
+    wrappedLogger[level] = wrapLoggerLevel(logger, fallbackLogger, level);
+  });
+
+  return wrappedLogger;
+}
+
+function validateLogger(logger) {
+  logLevels.forEach(level => {
+    if (!logger[level] || typeof logger[level] !== 'function') {
+      throw new Error('Provided logger instance must support logger.' + level + '(...) method');
+    }
+  });
+}
+
+function wrapLoggerLevel(logger, fallbackLogger, level) {
+  return function wrappedLoggerMethod() {
+    try {
+      return logger[level].apply(logger, arguments);
+    } catch (err) {
+      fallbackLogger.error('Error calling provided logger instance method ' + level + ': ' + err);
+      fallbackLogger[level].apply(fallbackLogger, arguments);
+    }
+  };
+}
+
+module.exports = LoggerWrapper;

test/configuration-test.js

@@ -2,9 +2,15 @@ var configuration = require('../configuration');
 
 describe('configuration', function() {
   const defaults = configuration.defaults();
-  
+
   function emptyConfigWithMockLogger() {
-    return { logger: { warn: jest.fn() } };
+    const logger = {
+      error: jest.fn(),
+      warn: jest.fn(),
+      info: jest.fn(),
+      debug: jest.fn(),
+    };
+    return { logger };
   }
 
   function expectDefault(name) {
@@ -16,17 +22,12 @@ describe('configuration', function() {
 
   function checkDeprecated(oldName, newName, value) {
     it('allows "' + oldName + '" as a deprecated equivalent to "' + newName + '"', function() {
-      var logger = {
-        warn: jest.fn()
-      };
-      var config0 = {
-        logger: logger
-      };
-      config0[oldName] = value;
-      var config1 = configuration.validate(config0);
+      const configIn = emptyConfigWithMockLogger();
+      configIn[oldName] = value;
+      const config1 = configuration.validate(configIn);
       expect(config1[newName]).toEqual(value);
       expect(config1[oldName]).toBeUndefined();
-      expect(logger.warn).toHaveBeenCalledTimes(1);
+      expect(configIn.logger.warn).toHaveBeenCalledTimes(1);
     });
   }
 
@@ -106,7 +107,7 @@ describe('configuration', function() {
   checkNumericProperty('userKeysCapacity', 500);
   checkNumericProperty('userKeysFlushInterval', 45);
   checkNumericProperty('diagnosticRecordingInterval', 110);
-  
+
   function checkNumericRange(name, minimum, maximum) {
     if (minimum !== undefined) {
       it('enforces minimum for "' + name + '"', () => {
@@ -182,4 +183,14 @@ describe('configuration', function() {
     configuration.validate(configIn);
     expect(configIn.logger.warn).toHaveBeenCalledTimes(1);
   });
+
+  it('throws an error if you pass in a logger with missing methods', () => {
+    const methods = ['error', 'warn', 'info', 'debug'];
+
+    methods.forEach(method => {
+      const configIn = emptyConfigWithMockLogger();
+      delete configIn.logger[method];
+      expect(() => configuration.validate(configIn)).toThrow(/Provided logger instance must support .* method/);
+    });
+  });
 });

test/logger_wrapper-test.js

@@ -0,0 +1,53 @@
+const LoggerWrapper = require('../logger_wrapper');
+
+describe('LoggerWrapper', function () {
+
+  function mockLogger() {
+    return {
+      error: jest.fn(),
+      warn: jest.fn(),
+      info: jest.fn(),
+      debug: jest.fn(),
+    };
+  }
+
+  const levels = ['error', 'warn', 'info', 'debug'];
+
+  it('throws an error if you pass in a logger that does not conform to the LDLogger schema', () => {
+    const fallbackLogger = mockLogger();
+
+    // If the method does not exist
+    levels.forEach(method => {
+      const logger = mockLogger();
+      delete logger[method];
+      expect(() => LoggerWrapper(logger, fallbackLogger)).toThrow(/Provided logger instance must support .* method/);
+    });
+
+    // If the method is not a function
+    levels.forEach(method => {
+      const logger = mockLogger();
+      logger[method] = 'invalid';
+      expect(() => LoggerWrapper(logger, fallbackLogger)).toThrow(/Provided logger instance must support .* method/);
+    });
+  });
+
+  it('If a logger method throws an error, the error is caught and logged, then the fallback logger is called', () => {
+    const err = Error('Something bad happened');
+
+    levels.forEach(level => {
+      const logger = mockLogger();
+      logger[level] = jest.fn(() => {
+        throw err
+      });
+      const fallbackLogger = mockLogger();
+      const wrappedLogger = LoggerWrapper(logger, fallbackLogger);
+
+      expect(() => wrappedLogger[level]('this is a logline', 'with multiple', 'arguments')).not.toThrow();
+
+      expect(fallbackLogger.error).toHaveBeenNthCalledWith(1, 'Error calling provided logger instance method ' + level + ': ' + err);
+
+      const nthCall = level === 'error' ? 2 : 1;
+      expect(fallbackLogger[level]).toHaveBeenNthCalledWith(nthCall, 'this is a logline', 'with multiple', 'arguments');
+    });
+  });
+});