Repository navigation
fix(splat): stop mutating the shared info[SPLAT] array - #363
Open
yfwmaniish wants to merge 1 commit into
Open
yfwmaniish wants to merge 1 commit into
yfwmaniish wants to merge 1 commit into
Conversation
Both code paths in Splatter (the tokens path in _splat(), and the no-tokens path in transform()) separated "meta" from "splat" by calling .splice() on info[SPLAT] directly. Since info[SPLAT] is an array reference, and winston shallow-clones info per transport (giving each transport's format pipeline its own info object but the same info[SPLAT] array), the first transport's splice mutated that shared array in place - so every subsequent transport saw an already-truncated (or emptied) splat, and silently lost the meta winston had attached to the log call. Both call sites now extract the same values via slice() instead, which reads from the array without mutating it. Fixes winstonjs/winston#2365.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes winstonjs/winston#2365.
What
Both code paths in `Splatter` separate "meta" (extra arguments beyond what the format string's tokens consume) from "splat" by calling `.splice()` directly on `info[SPLAT]`:
```js
// _splat() - tokens path
const metas = extraSplat < 0
? splat.splice(extraSplat, -1 * extraSplat)
: [];
...
info.message = util.format(msg, ...splat); // splat has now been mutated
// transform() - no-tokens path
const metas = splat.length > 1
? splat.splice(0)
: splat;
```
`info[SPLAT]` is an array reference. winston shallow-clones `info` once per transport (each transport's own format pipeline gets its own `info` object), but a shallow clone still shares the same `info[SPLAT]` array across all of them. So the first transport's `.splice()` mutates that shared array in place - by the time the second transport's format pipeline runs `splat()` over its own `info` object, `info[SPLAT]` is already truncated (or emptied entirely), and the meta winston attached to the log call is silently gone.
Reproduces exactly as described in the issue: two `Console` transports with `format.combine(format.splat(), format.json())`, only the first one gets `extraData`.
Fix
Both call sites now read the same values via `.slice()` instead of `.splice()` - same values extracted, but the original array (and thus `info[SPLAT]`) is left untouched.
_splat()'s localsplat(used forutil.format) is likewise derived via.slice()instead of relying on the post-splice, already-mutated array.Testing
Added two regression tests (tokens path and no-tokens path), each simulating two transports as two separate `info` objects sharing one `SPLAT` array reference - reusing the same `info` object across both transform() calls wouldn't actually catch this bug (a stale property surviving on the same object would mask it), so the test mirrors winston's real shallow-clone-per-transport shape.
Separate from #362 (which fixes a different bug - primitive metas getting char-spread via `Object.assign`, tracked as #109). This PR doesn't touch that code path.