fix: Do not override context when capturing non-error exceptions #444
Conversation
| @@ -331,7 +331,8 @@ extend(Raven.prototype, { | |||
| cb = kwargs; | |||
| kwargs = {}; | |||
| } else { | |||
| kwargs = kwargs || {}; | |||
| // Do not use reference, as we'll modify this object later on | |||
There was a problem hiding this comment.
why did you need to do this one too? because of line 353?
There was a problem hiding this comment.
Yes, because, we don't want to mutate someone else's objects.
var extraInfo = {
extra: {
something: 'fromDev'
}
}
function foo () {
try {
// something, something
} catch (e) {
Raven.captureException(e, extraInfo);
}
}
function bar () {
try {
// something else
} catch (e) {
Raven.captureMessage('something broke here', extraInfo);
}
}
foo();
bar(); // whoops, this will already be modified `foo` callI also wanted to be consistent across captureException/captureMessage.
| @@ -367,25 +368,24 @@ extend(Raven.prototype, { | |||
| cb = kwargs; | |||
| kwargs = {}; | |||
| } else { | |||
| kwargs = kwargs || {}; | |||
| // Do not use reference, as we'll modify this object later on | |||
| kwargs = kwargs ? JSON.parse(stringify(kwargs)) : {}; | |||
There was a problem hiding this comment.
is there anything not serializable that we should be worried about being passed here, like functions?
There was a problem hiding this comment.
did you consider only copying this at the point of mutation to avoid this code running on all execution paths?
not sure if that's actually a good idea or not.
There was a problem hiding this comment.
Yes, we may run into issues with req object from some frameworks... Although, I'm not sure why we even pass it down to errorHandler if it's already covered by adding req/res to context in requestHandler which is used in all our framework docs - https://github.com/getsentry/raven-node/blob/master/lib/client.js#L540-L568
There was a problem hiding this comment.
I read through the codebase once again and it appears that we should be perfectly fine.
https://github.com/getsentry/raven-node/blob/master/lib/client.js#L549-L562
This middleware is passing just an err itself, so we only have to preserve req just in case someone wants to use it inside dataCallback.
| kwargs.extra = { | ||
| __serialized__: serializedException | ||
| }; | ||
| kwargs = extend(kwargs, { |
| client.send = function mockSend(kwargs) { | ||
| client.send = old; | ||
| kwargs.extra.should.have.property('hello', 'there'); | ||
| kwargs.extra.should.not.equal(info); |
There was a problem hiding this comment.
should this test be should.not.equal(info.extra)?
There was a problem hiding this comment.
maybe this test should assert on the shape of info.extra anyway to be doubly sure
There was a problem hiding this comment.
We want to check the reference of a whole object that was passed to capture calls as the second argument.
We have a shape-based tests like this one https://github.com/getsentry/raven-node/pull/444/files#diff-c953753f049964fe98760ee9244e055cR341
There was a problem hiding this comment.
I still think line 430 is a false positive and it should be should.not.equal(info.extra)
(this test in its current form passes without your change)
There was a problem hiding this comment.
You're totally right. Fixed.
| @@ -367,25 +368,24 @@ extend(Raven.prototype, { | |||
| cb = kwargs; | |||
| kwargs = {}; | |||
| } else { | |||
| kwargs = kwargs || {}; | |||
| // Do not use reference, as we'll modify this object later on | |||
There was a problem hiding this comment.
is this a normal way to accomplish a multiple arity function with an optional second arg? it seems a bit complicated but it's worked well so far
There was a problem hiding this comment.
Unfortunately yes :(
|
Merged manually |
Resolves #442