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

Fix #42 #43

Merged
merged 5 commits into from
Dec 8, 2017
Merged
Show file tree
Hide file tree
Changes from 3 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
73 changes: 64 additions & 9 deletions logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ function pinoLogger (opts, stream) {

opts = opts || {}
opts.serializers = opts.serializers || {}
opts.serializers.req = opts.serializers.req || asReqValue
opts.serializers.req = wrapReqSerializer(opts.serializers.req || asReqValue)
opts.serializers.res = opts.serializers.res || pino.stdSerializers.res
opts.serializers.err = opts.serializers.err || pino.stdSerializers.err

Expand Down Expand Up @@ -64,16 +64,71 @@ function pinoLogger (opts, stream) {
}
}

var rawSymbol = Symbol.for('pino-raw-request')
var pinoReqProto = Object.create({}, {
id: {
enumerable: true,
writable: true,
value: ''
},
method: {
enumerable: true,
writable: true,
value: ''
},
url: {
enumerable: true,
writable: true,
value: ''
},
headers: {
enumerable: true,
writable: true,
value: {}
},
remoteAddress: {
enumerable: true,
writable: true,
value: ''
},
remotePort: {
enumerable: true,
writable: true,
value: ''
},
raw: {
Copy link
Member

Choose a reason for hiding this comment

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

at this point I fail to see the point of the symbol

@mcollina - thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

It gives a free non-enumerable. While writing this I learned:

const proto = Object.create({}, {
  foo: {
    enumerable: false,
    writable: true,
    value: {}
  }
})

const instance = Object.create(proto)
console.log('%j', instance) // {}

instance.foo = 'foo'
console.log('%j', instance) // {"foo":"foo"}

Copy link
Member

Choose a reason for hiding this comment

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

no sure – but you'd get that without the symbol and the getter/setter

Copy link
Member

Choose a reason for hiding this comment

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

whats req.raw for btw... I saw it in the hapi-pino implementation as well - whats the reasoning

Copy link
Member

@davidmarkclements davidmarkclements Dec 4, 2017

Choose a reason for hiding this comment

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

ah no no wait, you'd need a getter - but you could just return req

Copy link
Member Author

Choose a reason for hiding this comment

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

No, same thing happens with the getter and setter unless you wrap the object in a function with an enclosed variable to contain the value instead of set (val) { this._foo = val }.

As for req.raw, see comments hapijs/hapi-pino#34 (comment) and hapijs/hapi-pino#34 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

gotcha!

enumerable: false,
get: function () {
return this[rawSymbol]
},
set: function (val) {
this[rawSymbol] = val
}
}
})
Object.defineProperty(pinoReqProto, rawSymbol, {
writable: true,
value: {}
})

function wrapReqSerializer (serializer) {
if (serializer === asReqValue) return asReqValue
return function wrappedReqSerializer (req) {
return serializer(asReqValue(req))
}
}

function asReqValue (req) {
var connection = req.connection
return {
id: req.id,
method: req.method,
url: req.url,
headers: req.headers,
remoteAddress: connection && connection.remoteAddress,
remotePort: connection && connection.remotePort
}
const _req = Object.create(pinoReqProto)
_req.id = req.id
_req.method = req.method
_req.url = req.url
_req.headers = req.headers
_req.remoteAddress = connection && connection.remoteAddress
_req.remotePort = connection && connection.remotePort
_req.raw = req
return _req
}

function wrapChild (opts, stream) {
Expand Down
31 changes: 31 additions & 0 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,3 +303,34 @@ test('does not crash when no request connection object', function (t) {
res.end()
}
})

Copy link
Member

Choose a reason for hiding this comment

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

can we add a test for raw

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

// https://github.com/pinojs/pino-http/issues/42
test('does not return excessively long object', function (t) {
var dest = split(JSON.parse)
var logger = pinoHttp({
logger: pino(dest),
serializers: {
req: function (req) {
delete req.connection
return req
}
}
})
t.plan(1)

var server = http.createServer(handler)
server.unref()
server.listen(0, () => {
const port = server.address().port
http.get(`http://127.0.0.1:${port}`, () => {})
})

function handler (req, res) {
logger(req, res)
res.end()
}

dest.on('data', function (obj) {
t.is(Object.keys(obj.req).length, 6)
})
})