Repository: pinojs/pino
Pull request #684 (2019-07-19 16:08:25)

fix(levels): useLevelLabels permanently mutates initial cache
### Issue

Once a logger instance is created with `useLevelLabels: true` this can never be undone.

- All loggers using this module will update to use level labels.
- Creating new instances without the option specified will not use the default value, but will continue to use labels.
- Creating a logger  `useLevelLabels: false` will have no effect
- NOTE: any `pino` instances created by child packages, using incompatible semver (and thus a different/dup package) will not be impacted by this, and will continue to operate independently

#### Opinion

I can see some benefit in keeping the log records appearing uniformly everywhere. However, I might expect that to be a static method, or otherwise very explicitly documented. Since this is documented as a constructor-level option, I expect it to behave accordingly.

### Root Cause

An [initial cache](https://github.com/pinojs/pino/blob/master/lib/levels.js#L29) of levels (as numbers) is created when the package is required. This is [assigned to the instance prototype](https://github.com/pinojs/pino/blob/master/lib/proto.js#L56). When `useLevelLabels` is `true`, [genCache](https://github.com/pinojs/pino/blob/master/lib/levels.js#L34) is called to update the instance level display labels. _This modifies the instance property, stored on the prototype, which points to the cached, initial object._

### Solution

As I see it, there are two ways to approach this:

#### Generate the prototype property

My preferred solution would be to prevent ever allowing the defaults to be mutable but wrapping them in a getter function. Instead of assigning `initialLsCache` directly, we would call `getInitialLsCache` to get a copy, and preserve the original.

```js
const prototype = {
 // ...
  [lsCacheSym]: getInitialLsCache(),
```

However, this would be executed for _every_ Pino instance created. This library has been optimized for performance, so this is probably not the preferred solution.

#### Update the instance with a copy

Since this is an uncommon use case, we want to avoid adding extra complexity into the commonly used code paths. Instead, we'll make a shallow copy of the initial cache object when we need to make changes. We can do this in the existing `genLsCache` method.

```js
instance[lsCacheSym] = Object.assign({}, instance[lsCacheSym])
```
(or use `Object.assign` directly with the `reduce` initial value)

This code will only be executed in the cases when it is needed. It prioritizes overall performance over cache integrity.

pgoldrbx: I'm trying to figure out why coverage is reporting a missing branch. I feel like the "else" case should be well covered.

Patch:
diff --git a/lib/levels.js b/lib/levels.js
index dc6ba06..ea8c33b 100644
--- a/lib/levels.js
+++ b/lib/levels.js
@@ -38,7 +38,7 @@ function genLsCache (instance) {
       ? `{"${levelName}":"${instance.levels.labels[k]}"`
       : flatstr(`{"${levelName}":` + Number(k))
     return o
-  }, instance[lsCacheSym])
+  }, Object.assign({}, instance[lsCacheSym]))
   return instance
 }
 


Test patch:
diff --git a/test/levels.test.js b/test/levels.test.js
index f8ee7ae..eacb744 100644
--- a/test/levels.test.js
+++ b/test/levels.test.js
@@ -242,6 +242,21 @@ test('produces labels when told to', async ({ is }) => {
   instance.info('hello world')
 })
 
+test('resets levels from labels to numbers', async ({ is }) => {
+  const expected = [{
+    level: 30,
+    msg: 'hello world'
+  }]
+  pino({ useLevelLabels: true })
+  const instance = pino({ useLevelLabels: false }, sink((result, enc, cb) => {
+    const current = expected.shift()
+    check(is, result, current.level, current.msg)
+    cb()
+  }))
+
+  instance.info('hello world')
+})
+
 test('changes label naming when told to', async ({ is }) => {
   const expected = [{
     priority: 30,