Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply serializers to args once before asObject or transmit #1971

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ function createWrap (self, opts, rootLogger, level) {
const proto = (Object.getPrototypeOf && Object.getPrototypeOf(this) === _console) ? _console : this
for (var i = 0; i < args.length; i++) args[i] = arguments[i]

if (opts.serialize && !opts.asObject) {
if (opts.serialize && !opts.transmit) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the required change to fix the bug

applySerializers(args, this._serialize, this.serializers, this._stdErrSerialize)
}
if (opts.asObject || opts.formatters) {
Expand Down Expand Up @@ -330,17 +330,22 @@ function createWrap (self, opts, rootLogger, level) {

function asObject (logger, level, args, ts, formatters = {}) {
const {
level: levelFormatter = () => logger.levels.values[level],
level: levelFormatter,
log: logObjectFormatter = (obj) => obj
} = formatters
if (logger._serialize) applySerializers(args, logger._serialize, logger.serializers, logger._stdErrSerialize)
const argsCloned = args.slice()
let msg = argsCloned[0]
const logObject = {}
if (ts) {
logObject.time = ts
}
logObject.level = levelFormatter(level, logger.levels.values[level])

if (levelFormatter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

restore previous implementation to fix this bug

const formattedLevel = levelFormatter(level, logger.levels.values[level])
Object.assign(logObject, formattedLevel)
} else {
logObject.level = logger.levels.values[level]
}

let lvl = (logger._childLevel | 0) + 1
if (lvl < 1) lvl = 1
Expand All @@ -361,9 +366,9 @@ function applySerializers (args, serialize, serializers, stdErrSerialize) {
for (const i in args) {
if (stdErrSerialize && args[i] instanceof Error) {
args[i] = pino.stdSerializers.err(args[i])
} else if (typeof args[i] === 'object' && !Array.isArray(args[i])) {
} else if (typeof args[i] === 'object' && !Array.isArray(args[i]) && serialize) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to start a for loop, if serialize is not true

for (const k in args[i]) {
if (serialize && serialize.indexOf(k) > -1 && k in serializers) {
if (serialize.indexOf(k) > -1 && k in serializers) {
args[i][k] = serializers[k](args[i][k])
}
}
Expand All @@ -385,6 +390,7 @@ function transmit (logger, opts, args) {
logger.serializers,
logger._stdErrSerialize === undefined ? true : logger._stdErrSerialize
)

logger._logEvent.ts = ts
logger._logEvent.messages = args.filter(function (arg) {
// bindings can only be objects, so reference equality check via indexOf is fine
Expand Down
2 changes: 1 addition & 1 deletion docs/browser.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ Unlike server pino the serializers apply to every object passed to the logger me
if the `asObject` option is `true`, this results in the serializers applying to the
first object (as in server pino).

For more info on serializers see https://github.com/pinojs/pino/blob/master/docs/api.md#parameters.
For more info on serializers see https://github.com/pinojs/pino/blob/master/docs/api.md#mergingobject.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix docs id references


### `transmit` (Object)

Expand Down
2 changes: 0 additions & 2 deletions test/browser-serializers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ test('children inherit parent serializers', ({ end, is }) => {

test('children serializers get called', ({ end, is }) => {
const parent = pino({
test: 'this',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not part of the pino config, or log parameters, seems like a mistake

browser: {
serialize: true,
write (o) {
Expand All @@ -301,7 +300,6 @@ test('children serializers get called', ({ end, is }) => {

test('children serializers get called when inherited from parent', ({ end, is }) => {
const parent = pino({
test: 'this',
serializers: parentSerializers,
browser: {
serialize: true,
Expand Down
52 changes: 39 additions & 13 deletions test/browser.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ test('opts.browser.asObject logs pino-like object to console', ({ end, ok, is })
end()
})

test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is }) => {
test('opts.browser.formatters (level) logs pino-like object to console', ({ end, ok, is }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test names were the same, adding context for the different formatters config

const info = console.info
console.info = function (o) {
is(o.level, 30)
Expand All @@ -189,7 +189,7 @@ test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is
end()
})

test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is }) => {
test('opts.browser.formatters (log) logs pino-like object to console', ({ end, ok, is }) => {
const info = console.info
console.info = function (o) {
is(o.level, 30)
Expand All @@ -213,40 +213,66 @@ test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is
end()
})

test('opts.browser.write func log single string', ({ end, ok, is }) => {
const instance = pino({
test('opts.browser.serialize and opts.browser.transmit only serializes log data once', ({ end, ok, is }) => {
const instance = require('../browser')({
serializers: {
extras (data) {
return { serializedExtras: data }
}
},
browser: {
serialize: ['extras'],
transmit: {
level: 'info',
send (level, o) {
is(o.messages[0].extras.serializedExtras, 'world')
}
}
}
})

instance.info({ extras: 'world' }, 'test')
end()
})

test('opts.browser.serialize and opts.asObject only serializes log data once', ({ end, ok, is }) => {
const instance = require('../browser')({
serializers: {
extras (data) {
return { serializedExtras: data }
}
},
browser: {
serialize: ['extras'],
asObject: true,
write: function (o) {
is(o.level, 30)
is(o.msg, 'test')
ok(o.time)
is(o.extras.serializedExtras, 'world')
}
}
})
instance.info('test')

instance.info({ extras: 'world' }, 'test')
end()
})

test('opts.browser.write func string joining', ({ end, ok, is }) => {
test('opts.browser.write func log single string', ({ end, ok, is }) => {
const instance = pino({
browser: {
write: function (o) {
is(o.level, 30)
is(o.msg, 'test test2 test3')
is(o.msg, 'test')
ok(o.time)
}
}
})
instance.info('test %s %s', 'test2', 'test3')
instance.info('test')

end()
})

test('opts.browser.write func string joining when asObject is true', ({ end, ok, is }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test was repeated, see line 262 below, I deleted this, git somehow is reconciling my changes adding new test using the these lines

test('opts.browser.write func string joining', ({ end, ok, is }) => {
const instance = pino({
browser: {
asObject: true,
write: function (o) {
is(o.level, 30)
is(o.msg, 'test test2 test3')
Expand Down