Skip to content

fix(browser): keep the merging object when setBindings is used on the root logger - #2512

Open
dylanpulver wants to merge 1 commit into
pinojs:mainfrom
dylanpulver:fix/browser-set-bindings-merging-object
Open

fix(browser): keep the merging object when setBindings is used on the root logger#2512
dylanpulver wants to merge 1 commit into
pinojs:mainfrom
dylanpulver:fix/browser-set-bindings-merging-object

Conversation

@dylanpulver

Copy link
Copy Markdown

setBindings() on the root browser logger destroys the merging object of every later call:

const logger = pino({ browser: { asObject: true, write: console.log } })
logger.setBindings({ requestId: 'abc' })
logger.info({ userId: 42 }, 'order placed')
// browser: { level: 30, requestId: 'abc', msg: '{"userId":42} ' }
// node:    { level: 30, requestId: 'abc', userId: 42, msg: 'order placed' }

The fields and the message are both lost. asObject() takes the number of prepended bindings from _childLevel, but they are prepended from getBindingChain(), which also counts the logger's own bindings. Before #2471 only children had bindings, so the two agreed by accident. Children created after the setBindings() call are affected too.

So the fix records the length of the chain set() actually prepends. Recomputing getBindingChain() inside asObject() looks equivalent but isn't: the chain is captured in a closure, so a child created before setBindings() would over-merge and swallow a second object argument.

Not fixed here, and I'd rather ask than guess. setBindings() pushes newBindings into _logEvent.bindings while storing a different merged object, so transmit's reference filter misses it and those bindings also appear in logEvent.messages. What that array should hold for a root logger isn't obvious to me.

All six existing browser-set-bindings tests log with a string first argument, which is why this was invisible. Added four object-first cases; all four fail with the browser.js change reverted. The naughty-numbers guard test is re-pointed at the field the loop now reads so it keeps guarding. npm run test-ci green, 381 assertions vs 369 before.

… root logger

asObject() used _childLevel to count how many leading arguments are
prepended bindings, but the arguments are prepended from getBindingChain(),
which also includes the logger's own bindings. Until setBindings() was added
to the browser logger only children could have bindings, so the two counts
happened to agree.

Once the root logger has bindings the chain is one longer than the count, so
the caller's merging object is consumed as a binding and their message is
discarded. Record the length of the chain that is actually prepended instead.

The naughty-numbers guard test is re-pointed at the field the merge loop now
reads so it keeps guarding rather than passing vacuously.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this head (d57cf8f) on Windows with Node 24.12.0.

The focused browser-set-bindings and browser suites passed all 217 assertions. I also exercised multiple root updates, child setBindings(), children created before and after a parent update, grandchild precedence, and asObjectBindingsOnly; the child-before-parent case matches the Node implementation's fixed-at-creation semantics.

The full runtime suite reached 537 passing / 4 skipped with one environment-only failure: current main's transpile helper invokes Unix mv on Windows, so the generated to-file-transport.es2017.cjs fixture was missing (the existing #2499 addresses that). After generating the same three fixtures with PowerShell, the affected bundler test passed 3/3. Lint also passed before the known transpile gate.

I did not find a regression in the _bindingsDepth change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants