From 377285d6563f8ff35ca3961c4b1f214e231fff9c Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Sat, 3 Feb 2024 23:03:59 -0800 Subject: [PATCH] Resolve circular dependency issues downstream in Winston (#207) * Fix circular dependency between transport streams There is a circular dependency between the `TransportStream` (formerly in `index.js`) and `LegacyTransportStream` (in `legacy.js`). This causes some funky problems when you import the legacy module and later import the main module, as seen in https://github.com/winstonjs/winston/issues/1886. I've resolved the circular issue here by definiting the main implementation of `TransportStream` in a separate module (which `LegacyTransportStream` now depends on) and then doing the work of decorating it with a `.LegacyTransportStream` property in `index.js` (and that's pretty much all the index module does). * Resolve lint warning while I'm here --- index.js | 212 +-------------------------------------------- legacy.js | 2 +- modern.js | 211 ++++++++++++++++++++++++++++++++++++++++++++ test/index.test.js | 2 +- 4 files changed, 215 insertions(+), 212 deletions(-) create mode 100644 modern.js diff --git a/index.js b/index.js index 59e8421..6a62012 100644 --- a/index.js +++ b/index.js @@ -1,215 +1,7 @@ 'use strict'; -const util = require('util'); -const Writable = require('readable-stream/lib/_stream_writable.js'); -const { LEVEL } = require('triple-beam'); - -/** - * Constructor function for the TransportStream. This is the base prototype - * that all `winston >= 3` transports should inherit from. - * @param {Object} options - Options for this TransportStream instance - * @param {String} options.level - Highest level according to RFC5424. - * @param {Boolean} options.handleExceptions - If true, info with - * { exception: true } will be written. - * @param {Function} options.log - Custom log function for simple Transport - * creation - * @param {Function} options.close - Called on "unpipe" from parent. - */ -const TransportStream = module.exports = function TransportStream(options = {}) { - Writable.call(this, { objectMode: true, highWaterMark: options.highWaterMark }); - - this.format = options.format; - this.level = options.level; - this.handleExceptions = options.handleExceptions; - this.handleRejections = options.handleRejections; - this.silent = options.silent; - - if (options.log) this.log = options.log; - if (options.logv) this.logv = options.logv; - if (options.close) this.close = options.close; - - // Get the levels from the source we are piped from. - this.once('pipe', logger => { - // Remark (indexzero): this bookkeeping can only support multiple - // Logger parents with the same `levels`. This comes into play in - // the `winston.Container` code in which `container.add` takes - // a fully realized set of options with pre-constructed TransportStreams. - this.levels = logger.levels; - this.parent = logger; - }); - - // If and/or when the transport is removed from this instance - this.once('unpipe', src => { - // Remark (indexzero): this bookkeeping can only support multiple - // Logger parents with the same `levels`. This comes into play in - // the `winston.Container` code in which `container.add` takes - // a fully realized set of options with pre-constructed TransportStreams. - if (src === this.parent) { - this.parent = null; - if (this.close) { - this.close(); - } - } - }); -}; - -/* - * Inherit from Writeable using Node.js built-ins - */ -util.inherits(TransportStream, Writable); - -/** - * Writes the info object to our transport instance. - * @param {mixed} info - TODO: add param description. - * @param {mixed} enc - TODO: add param description. - * @param {function} callback - TODO: add param description. - * @returns {undefined} - * @private - */ -TransportStream.prototype._write = function _write(info, enc, callback) { - if (this.silent || (info.exception === true && !this.handleExceptions)) { - return callback(null); - } - - // Remark: This has to be handled in the base transport now because we - // cannot conditionally write to our pipe targets as stream. We always - // prefer any explicit level set on the Transport itself falling back to - // any level set on the parent. - const level = this.level || (this.parent && this.parent.level); - - if (!level || this.levels[level] >= this.levels[info[LEVEL]]) { - if (info && !this.format) { - return this.log(info, callback); - } - - let errState; - let transformed; - - // We trap(and re-throw) any errors generated by the user-provided format, but also - // guarantee that the streams callback is invoked so that we can continue flowing. - try { - transformed = this.format.transform(Object.assign({}, info), this.format.options); - } catch (err) { - errState = err; - } - - if (errState || !transformed) { - // eslint-disable-next-line callback-return - callback(); - if (errState) throw errState; - return; - } - - return this.log(transformed, callback); - } - this._writableState.sync = false; - return callback(null); -}; - -/** - * Writes the batch of info objects (i.e. "object chunks") to our transport - * instance after performing any necessary filtering. - * @param {mixed} chunks - TODO: add params description. - * @param {function} callback - TODO: add params description. - * @returns {mixed} - TODO: add returns description. - * @private - */ -TransportStream.prototype._writev = function _writev(chunks, callback) { - if (this.logv) { - const infos = chunks.filter(this._accept, this); - if (!infos.length) { - return callback(null); - } - - // Remark (indexzero): from a performance perspective if Transport - // implementers do choose to implement logv should we make it their - // responsibility to invoke their format? - return this.logv(infos, callback); - } - - for (let i = 0; i < chunks.length; i++) { - if (!this._accept(chunks[i])) continue; - - if (chunks[i].chunk && !this.format) { - this.log(chunks[i].chunk, chunks[i].callback); - continue; - } - - let errState; - let transformed; - - // We trap(and re-throw) any errors generated by the user-provided format, but also - // guarantee that the streams callback is invoked so that we can continue flowing. - try { - transformed = this.format.transform( - Object.assign({}, chunks[i].chunk), - this.format.options - ); - } catch (err) { - errState = err; - } - - if (errState || !transformed) { - // eslint-disable-next-line callback-return - chunks[i].callback(); - if (errState) { - // eslint-disable-next-line callback-return - callback(null); - throw errState; - } - } else { - this.log(transformed, chunks[i].callback); - } - } - - return callback(null); -}; - -/** - * Predicate function that returns true if the specfied `info` on the - * WriteReq, `write`, should be passed down into the derived - * TransportStream's I/O via `.log(info, callback)`. - * @param {WriteReq} write - winston@3 Node.js WriteReq for the `info` object - * representing the log message. - * @returns {Boolean} - Value indicating if the `write` should be accepted & - * logged. - */ -TransportStream.prototype._accept = function _accept(write) { - const info = write.chunk; - if (this.silent) { - return false; - } - - // We always prefer any explicit level set on the Transport itself - // falling back to any level set on the parent. - const level = this.level || (this.parent && this.parent.level); - - // Immediately check the average case: log level filtering. - if ( - info.exception === true || - !level || - this.levels[level] >= this.levels[info[LEVEL]] - ) { - // Ensure the info object is valid based on `{ exception }`: - // 1. { handleExceptions: true }: all `info` objects are valid - // 2. { exception: false }: accepted by all transports. - if (this.handleExceptions || info.exception !== true) { - return true; - } - } - - return false; -}; - -/** - * _nop is short for "No operation" - * @returns {Boolean} Intentionally false. - */ -TransportStream.prototype._nop = function _nop() { - // eslint-disable-next-line no-undefined - return void undefined; -}; - +// Expose modern transport directly as the export +module.exports = require('./modern'); // Expose legacy stream module.exports.LegacyTransportStream = require('./legacy'); diff --git a/legacy.js b/legacy.js index 8025bc9..6cc9522 100644 --- a/legacy.js +++ b/legacy.js @@ -2,7 +2,7 @@ const util = require('util'); const { LEVEL } = require('triple-beam'); -const TransportStream = require('./'); +const TransportStream = require('./modern'); /** * Constructor function for the LegacyTransportStream. This is an internal diff --git a/modern.js b/modern.js new file mode 100644 index 0000000..37eeb3d --- /dev/null +++ b/modern.js @@ -0,0 +1,211 @@ +'use strict'; + +const util = require('util'); +const Writable = require('readable-stream/lib/_stream_writable.js'); +const { LEVEL } = require('triple-beam'); + +/** + * Constructor function for the TransportStream. This is the base prototype + * that all `winston >= 3` transports should inherit from. + * @param {Object} options - Options for this TransportStream instance + * @param {String} options.level - Highest level according to RFC5424. + * @param {Boolean} options.handleExceptions - If true, info with + * { exception: true } will be written. + * @param {Function} options.log - Custom log function for simple Transport + * creation + * @param {Function} options.close - Called on "unpipe" from parent. + */ +const TransportStream = module.exports = function TransportStream(options = {}) { + Writable.call(this, { objectMode: true, highWaterMark: options.highWaterMark }); + + this.format = options.format; + this.level = options.level; + this.handleExceptions = options.handleExceptions; + this.handleRejections = options.handleRejections; + this.silent = options.silent; + + if (options.log) this.log = options.log; + if (options.logv) this.logv = options.logv; + if (options.close) this.close = options.close; + + // Get the levels from the source we are piped from. + this.once('pipe', logger => { + // Remark (indexzero): this bookkeeping can only support multiple + // Logger parents with the same `levels`. This comes into play in + // the `winston.Container` code in which `container.add` takes + // a fully realized set of options with pre-constructed TransportStreams. + this.levels = logger.levels; + this.parent = logger; + }); + + // If and/or when the transport is removed from this instance + this.once('unpipe', src => { + // Remark (indexzero): this bookkeeping can only support multiple + // Logger parents with the same `levels`. This comes into play in + // the `winston.Container` code in which `container.add` takes + // a fully realized set of options with pre-constructed TransportStreams. + if (src === this.parent) { + this.parent = null; + if (this.close) { + this.close(); + } + } + }); +}; + +/* + * Inherit from Writeable using Node.js built-ins + */ +util.inherits(TransportStream, Writable); + +/** + * Writes the info object to our transport instance. + * @param {mixed} info - TODO: add param description. + * @param {mixed} enc - TODO: add param description. + * @param {function} callback - TODO: add param description. + * @returns {undefined} + * @private + */ +TransportStream.prototype._write = function _write(info, enc, callback) { + if (this.silent || (info.exception === true && !this.handleExceptions)) { + return callback(null); + } + + // Remark: This has to be handled in the base transport now because we + // cannot conditionally write to our pipe targets as stream. We always + // prefer any explicit level set on the Transport itself falling back to + // any level set on the parent. + const level = this.level || (this.parent && this.parent.level); + + if (!level || this.levels[level] >= this.levels[info[LEVEL]]) { + if (info && !this.format) { + return this.log(info, callback); + } + + let errState; + let transformed; + + // We trap(and re-throw) any errors generated by the user-provided format, but also + // guarantee that the streams callback is invoked so that we can continue flowing. + try { + transformed = this.format.transform(Object.assign({}, info), this.format.options); + } catch (err) { + errState = err; + } + + if (errState || !transformed) { + // eslint-disable-next-line callback-return + callback(); + if (errState) throw errState; + return; + } + + return this.log(transformed, callback); + } + this._writableState.sync = false; + return callback(null); +}; + +/** + * Writes the batch of info objects (i.e. "object chunks") to our transport + * instance after performing any necessary filtering. + * @param {mixed} chunks - TODO: add params description. + * @param {function} callback - TODO: add params description. + * @returns {mixed} - TODO: add returns description. + * @private + */ +TransportStream.prototype._writev = function _writev(chunks, callback) { + if (this.logv) { + const infos = chunks.filter(this._accept, this); + if (!infos.length) { + return callback(null); + } + + // Remark (indexzero): from a performance perspective if Transport + // implementers do choose to implement logv should we make it their + // responsibility to invoke their format? + return this.logv(infos, callback); + } + + for (let i = 0; i < chunks.length; i++) { + if (!this._accept(chunks[i])) continue; + + if (chunks[i].chunk && !this.format) { + this.log(chunks[i].chunk, chunks[i].callback); + continue; + } + + let errState; + let transformed; + + // We trap(and re-throw) any errors generated by the user-provided format, but also + // guarantee that the streams callback is invoked so that we can continue flowing. + try { + transformed = this.format.transform( + Object.assign({}, chunks[i].chunk), + this.format.options + ); + } catch (err) { + errState = err; + } + + if (errState || !transformed) { + // eslint-disable-next-line callback-return + chunks[i].callback(); + if (errState) { + // eslint-disable-next-line callback-return + callback(null); + throw errState; + } + } else { + this.log(transformed, chunks[i].callback); + } + } + + return callback(null); +}; + +/** + * Predicate function that returns true if the specfied `info` on the + * WriteReq, `write`, should be passed down into the derived + * TransportStream's I/O via `.log(info, callback)`. + * @param {WriteReq} write - winston@3 Node.js WriteReq for the `info` object + * representing the log message. + * @returns {Boolean} - Value indicating if the `write` should be accepted & + * logged. + */ +TransportStream.prototype._accept = function _accept(write) { + const info = write.chunk; + if (this.silent) { + return false; + } + + // We always prefer any explicit level set on the Transport itself + // falling back to any level set on the parent. + const level = this.level || (this.parent && this.parent.level); + + // Immediately check the average case: log level filtering. + if ( + info.exception === true || + !level || + this.levels[level] >= this.levels[info[LEVEL]] + ) { + // Ensure the info object is valid based on `{ exception }`: + // 1. { handleExceptions: true }: all `info` objects are valid + // 2. { exception: false }: accepted by all transports. + if (this.handleExceptions || info.exception !== true) { + return true; + } + } + + return false; +}; + +/** + * _nop is short for "No operation" + * @returns {Boolean} Intentionally false. + */ +TransportStream.prototype._nop = function _nop() { + // eslint-disable-next-line no-undefined + return void undefined; +}; diff --git a/test/index.test.js b/test/index.test.js index e14fd23..71d9764 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -24,7 +24,7 @@ function infoify(info) { info[LEVEL] = info.level; info[MESSAGE] = info.message; return info; -}; +} describe('TransportStream', () => { it('should have the appropriate methods defined', () => {