Merge pull request #986 from mvastola/fix-appender-load-order

fix: Allow appenders to depend on not-yet-loaded appenders.

lib/appenders/index.js

@@ -40,6 +40,22 @@ const loadAppenderModule = (type, config) => coreAppenders.get(type)
   || (require.main && tryLoading(path.join(path.dirname(require.main.filename), type), config))
   || tryLoading(path.join(process.cwd(), type), config);
 
+const appendersLoading = new Set();
+
+const getAppender = (name, config) => {
+  if (appenders.has(name)) return appenders.get(name);
+  if (!config.appenders[name]) return false;
+  if (appendersLoading.has(name)) throw new Error(`Dependency loop detected for appender ${name}.`);
+  appendersLoading.add(name);
+
+  debug(`Creating appender ${name}`);
+  // eslint-disable-next-line no-use-before-define
+  const appender = createAppender(name, config);
+  appendersLoading.delete(name);
+  appenders.set(name, appender);
+  return appender;
+};
+
 const createAppender = (name, config) => {
   const appenderConfig = config.appenders[name];
   const appenderModule = appenderConfig.type.configure
@@ -63,7 +79,7 @@ const createAppender = (name, config) => {
     return appenderModule.configure(
       adapters.modifyConfig(appenderConfig),
       layouts,
-      appender => appenders.get(appender),
+      appender => getAppender(appender, config),
       levels
     );
   }, () => {});
@@ -71,10 +87,9 @@ const createAppender = (name, config) => {
 
 const setup = (config) => {
   appenders.clear();
-
+  appendersLoading.clear();
   Object.keys(config.appenders).forEach((name) => {
-    debug(`Creating appender ${name}`);
-    appenders.set(name, createAppender(name, config));
+    getAppender(name, config);
   });
 };
 

test/tap/appender-dependencies-test.js

@@ -0,0 +1,114 @@
+const { test } = require("tap");
+
+const categories = {
+  default: { appenders: ["filtered"], level: "debug" }
+}
+
+let log4js;
+let recording;
+
+test("log4js appender dependencies", batch => {
+  batch.beforeEach(done => {
+    log4js = require("../../lib/log4js");
+    recording = require("../../lib/appenders/recording");
+    done();
+  });
+  batch.afterEach(done => {
+    recording.erase();
+    done();
+  });
+  batch.test("in order", t => {
+    const config = {
+      categories,
+      appenders: {
+        recorder: { type: "recording" },
+        filtered: {
+          type: "logLevelFilter",
+          appender: "recorder",
+          level: "ERROR"
+        }
+      }
+    };
+    t.test('should resolve if defined in dependency order', assert => {
+      assert.doesNotThrow(() => {
+        log4js.configure(config);
+      }, 'this should not trigger an error');
+      assert.end();
+    });
+    const logger = log4js.getLogger("logLevelTest");
+    logger.debug("this should not trigger an event");
+    logger.error("this should, though");
+
+    const logEvents = recording.replay();
+    t.test(
+      "should process log events normally",
+      assert => {
+        assert.equal(logEvents.length, 1);
+        assert.equal(logEvents[0].data[0], "this should, though");
+        assert.end();
+      }
+    );
+    t.end();
+  });
+
+  batch.test("not in order", t => {
+    const config = {
+      categories,
+      appenders: {
+        filtered: {
+          type: "logLevelFilter",
+          appender: "recorder",
+          level: "ERROR"
+        },
+        recorder: { type: "recording" },
+      }
+    };
+    t.test('should resolve if defined out of dependency order', assert => {
+      assert.doesNotThrow(() => {
+        log4js.configure(config);
+      }, 'this should not trigger an error');
+      assert.end();
+    });
+    const logger = log4js.getLogger("logLevelTest");
+    logger.debug("this should not trigger an event");
+    logger.error("this should, though");
+
+    const logEvents = recording.replay();
+    t.test(
+      "should process log events normally",
+      assert => {
+        assert.equal(logEvents.length, 1);
+        assert.equal(logEvents[0].data[0], "this should, though");
+        assert.end();
+      }
+    );
+    t.end();
+  });
+
+  batch.test("with dependency loop", t => {
+    const config = {
+      categories,
+      appenders: {
+        filtered: {
+          type: "logLevelFilter",
+          appender: "filtered2",
+          level: "ERROR"
+        },
+        filtered2: {
+          type: "logLevelFilter",
+          appender: "filtered",
+          level: "ERROR"
+        },
+        recorder: { type: "recording" },
+      }
+    };
+    t.test('should throw an error if if a dependency loop is found', assert => {
+      assert.throws(() => {
+        log4js.configure(config);
+      }, 'Dependency loop detected for appender filtered.');
+      assert.end();
+    });
+    t.end();
+  });
+  batch.end();
+});