Fix cached error deserialisation where the Throwable has a cause #418
Conversation
This file contains 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
…hrowable also has a cause
Java exceptions can have multiple throwables (e.g. a root throwable, and the 'cause' which triggered the exception). The ErrorReader class did not previously handle this case, which led to a failure to send cached error reports.
fractalwrench
commented
Jan 18, 2019
bengourley
reviewed
Jan 18, 2019
| ex.setExceptionType(type); | ||
| return ex; | ||
| BugsnagException bugsnagException = new BugsnagException(errorClass, message, frames); | ||
| bugsnagException.setType(type); |
There was a problem hiding this comment.
This type thing seems odd – it exists on the collection of items before it gets serialized, but gets added to every item during serialisation. It then has to be set on the individual items when deserialised, only to be added to the collection again.
Is there something better we can do than add the seemingly useless getter/setter to the BugsnagException class?
I understand that this change just gets the job done, which might be a good enough reason to leave as is.
bengourley
approved these changes
Jan 21, 2019
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.
Goal
When a
beforeSendcallback is specified, deserialisation of cached error reports fails if the exception has an additional cause. This prevents delivery of the error report to Bugsnag.This changeset fixes #417 by altering the
ErrorReaderclass to handle this scenario.Design
The
ErrorReaderclass assumed that only 1 exception could be present in an array. The implementation has therefore been altered to serialise every object in the array. The first object is considered the rootThrowable, and any additional exceptions are deserialised by usinginitCause. This is effectively just performing the inverse ofExceptions#toStream.Changeset
Tests
ErrorReaderdeserialises it correctly.beforeSendcallback, confirming that an error report was delivered.Note: none of the stack frames will be marked as in-project when delivered from a cached report that is modified in a
beforeSendcallback. This is independent from this bug and will be addressed in a separate PR.