fix: logger validator throws (fastify#4520)

Author: victortosts

docs/Reference/Errors.md

@@ -281,6 +281,12 @@ A callback for a hook timed out
 
 The logger accepts either a `'stream'` or a `'file'` as the destination.
 
+#### FST_ERR_LOG_INVALID_LOGGER
+<a id="FST_ERR_LOG_INVALID_LOGGER"></a>
+
+The logger should have all these methods: `'info'`, `'error'`,
+`'debug'`, `'fatal'`, `'warn'`, `'trace'`, `'child'`.
+
 #### FST_ERR_REP_INVALID_PAYLOAD_TYPE
 <a id="FST_ERR_REP_INVALID_PAYLOAD_TYPE"></a>
 

lib/errors.js

@@ -177,6 +177,11 @@ const codes = {
     'Cannot specify both logger.stream and logger.file options'
   ),
 
+  FST_ERR_LOG_INVALID_LOGGER: createError(
+    'FST_ERR_LOG_INVALID_LOGGER',
+    "Invalid logger object provided. The logger instance should have these functions(s): '%s'."
+  ),
+
   /**
    * reply
   */

lib/logger.js

@@ -9,17 +9,17 @@
 const nullLogger = require('abstract-logging')
 const pino = require('pino')
 const { serializersSym } = pino.symbols
-const { FST_ERR_LOG_INVALID_DESTINATION } = require('./errors')
+const {
+  FST_ERR_LOG_INVALID_DESTINATION,
+  FST_ERR_LOG_INVALID_LOGGER
+} = require('./errors')
 
-function createPinoLogger (opts, stream) {
-  stream = stream || opts.stream
-  delete opts.stream
-
-  if (stream && opts.file) {
+function createPinoLogger (opts) {
+  if (opts.stream && opts.file) {
     throw new FST_ERR_LOG_INVALID_DESTINATION()
   } else if (opts.file) {
     // we do not have stream
-    stream = pino.destination(opts.file)
+    opts.stream = pino.destination(opts.file)
     delete opts.file
   }
 
@@ -39,7 +39,7 @@ function createPinoLogger (opts, stream) {
     opts.logger = prevLogger
     opts.genReqId = prevGenReqId
   } else {
-    logger = pino(opts, stream)
+    logger = pino(opts, opts.stream)
   }
 
   return logger
@@ -70,50 +70,60 @@ function now () {
 }
 
 function createLogger (options) {
-  if (isValidLogger(options.logger)) {
+  if (!options.logger) {
+    const logger = nullLogger
+    logger.child = () => logger
+    return { logger, hasLogger: false }
+  }
+
+  if (validateLogger(options.logger)) {
     const logger = createPinoLogger({
       logger: options.logger,
       serializers: Object.assign({}, serializers, options.logger.serializers)
     })
     return { logger, hasLogger: true }
-  } else if (!options.logger) {
-    const logger = nullLogger
-    logger.child = () => logger
-    return { logger, hasLogger: false }
-  } else {
-    const localLoggerOptions = {}
-    if (Object.prototype.toString.call(options.logger) === '[object Object]') {
-      Reflect.ownKeys(options.logger).forEach(prop => {
-        Object.defineProperty(localLoggerOptions, prop, {
-          value: options.logger[prop],
-          writable: true,
-          enumerable: true,
-          configurable: true
-        })
-      })
-    }
-    localLoggerOptions.level = localLoggerOptions.level || 'info'
-    localLoggerOptions.serializers = Object.assign({}, serializers, localLoggerOptions.serializers)
-    options.logger = localLoggerOptions
-    const logger = createPinoLogger(options.logger)
-    return { logger, hasLogger: true }
   }
-}
 
-function isValidLogger (logger) {
-  if (!logger) {
-    return false
+  const localLoggerOptions = {}
+  if (Object.prototype.toString.call(options.logger) === '[object Object]') {
+    Reflect.ownKeys(options.logger).forEach(prop => {
+      Object.defineProperty(localLoggerOptions, prop, {
+        value: options.logger[prop],
+        writable: true,
+        enumerable: true,
+        configurable: true
+      })
+    })
   }
+  localLoggerOptions.level = localLoggerOptions.level || 'info'
+  localLoggerOptions.serializers = Object.assign({}, serializers, localLoggerOptions.serializers)
+  options.logger = localLoggerOptions
+  const logger = createPinoLogger(options.logger)
+  return { logger, hasLogger: true }
+}
 
-  let result = true
+/**
+ * Determines if a provided logger object meets the requirements
+ * of a Fastify compatible logger.
+ *
+ * @param {object} logger Object to validate.
+ *
+ * @returns {boolean} `true` when the logger meets the requirements.
+ *
+ * @throws {FST_ERR_LOG_INVALID_LOGGER} When the logger object is
+ * missing required methods.
+ */
+function validateLogger (logger) {
   const methods = ['info', 'error', 'debug', 'fatal', 'warn', 'trace', 'child']
-  for (let i = 0; i < methods.length; i += 1) {
-    if (!logger[methods[i]] || typeof logger[methods[i]] !== 'function') {
-      result = false
-      break
-    }
+  const missingMethods = methods.filter(method => !logger[method] || typeof logger[method] !== 'function')
+
+  if (!missingMethods.length) {
+    return true
+  } else if (missingMethods.length === methods.length) {
+    return false
+  } else {
+    throw FST_ERR_LOG_INVALID_LOGGER(missingMethods.join(','))
   }
-  return result
 }
 
 module.exports = {

test/logger.test.js

@@ -1,7 +1,6 @@
 'use strict'
 
 const { test, teardown, before } = require('tap')
-const helper = require('./helper')
 const http = require('http')
 const stream = require('stream')
 const split = require('split2')
@@ -13,6 +12,9 @@ const fs = require('fs')
 const sget = require('simple-get').concat
 const dns = require('dns')
 
+const helper = require('./helper')
+const { FST_ERR_LOG_INVALID_LOGGER } = require('../lib/errors')
+
 const files = []
 let count = 0
 let localhost
@@ -268,6 +270,34 @@ test('can use external logger instance with custom serializer', t => {
   })
 })
 
+test('should throw in case the external logger provided does not have a child method', t => {
+  t.plan(1)
+  const loggerInstance = {
+    info: console.info,
+    error: console.error,
+    debug: console.debug,
+    fatal: console.error,
+    warn: console.warn,
+    trace: console.trace
+  }
+
+  t.throws(
+    () => Fastify({ logger: loggerInstance }),
+    FST_ERR_LOG_INVALID_LOGGER,
+    "Invalid logger object provided. The logger instance should have these functions(s): 'child'."
+  )
+})
+
+test('should throw in case a partially matching logger is provided', t => {
+  t.plan(1)
+
+  t.throws(
+    () => Fastify({ logger: console }),
+    FST_ERR_LOG_INVALID_LOGGER,
+    "Invalid logger object provided. The logger instance should have these functions(s): 'fatal,child'."
+  )
+})
+
 test('expose the logger', t => {
   t.plan(2)
   let fastify = null

types/errors.d.ts

@@ -20,6 +20,7 @@ export type FastifyErrorCodes = Record<
 'FST_ERR_MISSING_MIDDLEWARE' |
 'FST_ERR_HOOK_TIMEOUT' |
 'FST_ERR_LOG_INVALID_DESTINATION' |
+'FST_ERR_LOG_INVALID_LOGGER' |
 'FST_ERR_REP_INVALID_PAYLOAD_TYPE' |
 'FST_ERR_REP_ALREADY_SENT' |
 'FST_ERR_REP_SENT_VALUE'|