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

Handle 'nil' being notified #439

Merged
merged 7 commits into from
Apr 18, 2018
Merged

Handle 'nil' being notified #439

merged 7 commits into from
Apr 18, 2018

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Mar 19, 2018

  • Changes 'nil' to a string: "'nil' was notified as an exception"
  • Fixes Bugsnag throwing exception during notification process

- Changes 'nil' to a string: "'nil' was notified as an exception"
- Fixes Bugsnag throwing exception during notification process
@Cawllec Cawllec requested review from kattrali and snmaynard March 19, 2018 14:15
event = payload["events"][0]
exception = event["exceptions"][0]
expect(exception["errorClass"]).to eq("RuntimeError")
expect(exception["message"]).to eq(Bugsnag::Report::NIL_EXCEPTION_DESCRIPTION)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the stacktrace look like in this case? It would be good to verify that it is also sensible in a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the test to look at the stacktrace as well now.

@Cawllec
Copy link
Contributor Author

Cawllec commented Mar 19, 2018

This is waiting for the refactoring change to be merged first #440

@snmaynard
Copy link
Contributor

Why does this depend on the refactoring?

Why was this not done in the Bugsnag Client? We already have a bunch of validation of input in there.

@Cawllec
Copy link
Contributor Author

Cawllec commented Mar 20, 2018

To avoid continually adding exceptions to rubocop I thought small refactoring tasks (such as breaking helpers into separate files and moving generate_raw_exceptions into a helper from Report) could be done when appropriate. This can be merged without it, but I'll need to updated .rubo_todo.yml

@snmaynard
Copy link
Contributor

I still have a question about this being done in report rather than in the client.

@Cawllec
Copy link
Contributor Author

Cawllec commented Mar 21, 2018

It makes sense to me that the notify function handles whether a notification should be sent, whereas the report class handles the content of the notification. As this falls into the latter category, I put it in the Report.

@snmaynard
Copy link
Contributor

The notify method is the public interface, and so it validates user input - it should be there.

Cawllec added 2 commits March 29, 2018 10:02
- Moved nil handling logic from Report to Client
- Refactor of delivery flags into separate 'deliver_notification?' private method
@Cawllec
Copy link
Contributor Author

Cawllec commented Mar 29, 2018

I've moved the nil exception changes into the Client, and refactored the configuration checks into a private deliver_notification? method. This negates the need for the refactor PR, which I'll close. I'll put together a plan for a proper refactoring that will hit more than a couple of points which can be discussed at a later date.

@kattrali kattrali merged commit c05e4da into master Apr 18, 2018
@kattrali kattrali deleted the cawllec/handle-nil-exceptions branch April 18, 2018 00:46
@Cawllec Cawllec mentioned this pull request Apr 24, 2018
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.

3 participants