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

Expose exception message via #to_s #89

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Expose exception message via #to_s #89

merged 1 commit into from
Jan 7, 2020

Conversation

tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Jan 6, 2020

What problem does the pull request solve?

Exceptions in Ruby return the exception message when calling #to_s on it:

> irb
irb(main):001:0> StandardError.new("Hi").to_s
=> "Hi"

Because this is default behaviour, libraries sometimes depend on to_s to get a readable version of the exception message. For example, the Sentry client does it to give the exception a name:

https://github.com/getsentry/raven-ruby/blob/065cf973cb0ad3672d0d213e80b6e8a9a8e6f8cb/lib/raven/event.rb#L144

Currently, Sentry reports do not show the error message set when raising the exception - it only displays Notifications::Client::BadRequestError:

Screenshot 2020-01-06 at 11 53 26

This is because we’re only defining RequestError#message. This is apparently not the same as initialising an exception with a message, as you can see here:

> be rails c
Loading development environment (Rails 6.0.2.1)
[1] pry(main)>
Notifications::Client::BadRequestError.new(OpenStruct.new(code: "hey", body: {errors: [{error: "BadRequestError", message: "Can’t send to this recipient using a team-only API key"}] }.to_json )).to_s
=> "Notifications::Client::BadRequestError"
[2] pry(main)>
Notifications::Client::BadRequestError.new(OpenStruct.new(code: "hey", body: {errors: [{error: "BadRequestError", message: "Can’t send to this recipient using a team-only API key"}] }.to_json )).message 
=> "BadRequestError: Can’t send to this recipient using a team-only API key"

The issue was introduced in #73 / #72, which removed the explicit to_s method.

This commit fixes the issue by constructing the message in the constructor. Note that we’ve renamed the message method, but calling e.message still works because it’s part of the exception interface.

Another option would be to add the to_s method back (like we experimented with in DFE-Digital/apply-for-teacher-training#1024), but this solution seems the most Ruby-esque and relies on less knowledge of Exception internals.

After DFE-Digital/apply-for-teacher-training#1024, the exceptions are properly reported:

Before After
Screenshot 2020-01-06 at 20 44 21 Screenshot 2020-01-06 at 20 05 21

This change is backwards compatible.

Checklist

  • I’ve used the pull request template
  • I’ve written unit tests for these changes
  • I’ve updated the documentation (in DOCUMENTATION.md)
  • I’ve bumped the version number (in lib/notifications/client/version.rb)

Exceptions in Ruby return the exception message when calling `#to_s` on
it:

```
> irb
irb(main):001:0> StandardError.new("Hi").to_s
=> "Hi"
```

Because this is default behaviour, libraries sometimes depend on `to_s`
to get a readable version of the exception message. For example, Sentry
does it to the exception a name:

https://github.com/getsentry/raven-ruby/blob/065cf973cb0ad3672d0d213e80b
6e8a9a8e6f8cb/lib/raven/event.rb#L144

Currently, Sentry reports do not show the error message set when
raising the exception - only “Notifications::Client::BadRequestError”.

This is because we’re only defining `RequestError#message`. This is
apparently not the same as initialising an exception with a message, as
you can see here:

```
> be rails c
Loading development environment (Rails 6.0.2.1)
[1] pry(main)>
Notifications::Client::BadRequestError.new(OpenStruct.new(code: "hey",
body: {errors: [{error: "BadRequestError", message: "Can’t send to this
recipient using a team-only API key"}] }.to_json )).to_s
=> "Notifications::Client::BadRequestError"
[2] pry(main)>
Notifications::Client::BadRequestError.new(OpenStruct.new(code: "hey",
body: {errors: [{error: "BadRequestError", message: "Can’t send to this
recipient using a team-only API key"}] }.to_json )).message
=> "BadRequestError: Can’t send to this recipient using a team-only API
key"
```

The issue was introduced in
#73 /
#72, which
removed the explicit `to_s` method.

This commit fixes the issue by constructing the message . Note that
we’ve renamed the `message` method, but calling `e.message` still works
because it’s part of the exception interface. Another option would be
to add the `to_s` method back (like I experimented with in
https://github.com/DFE-Digital/apply-for-postgraduate-teacher-training/p
ull/1024), but this solution seems the most Ruby-esque and relies on
less knowledge of Exception internals.

This change is backwards compatible.
Copy link
Contributor

@leohemsted leohemsted left a comment

Choose a reason for hiding this comment

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

@tijmenb thanks for this! It seems sensible, and great PR description/context. I'll get this deployed now

@leohemsted leohemsted closed this Jan 7, 2020
@leohemsted leohemsted reopened this Jan 7, 2020
@leohemsted leohemsted merged commit 53ac230 into alphagov:master Jan 7, 2020
@tijmenb
Copy link
Contributor Author

tijmenb commented Jan 7, 2020

Thanks @leohemsted !

@benlovell
Copy link
Contributor

Just came here to add this myself then realised @tijmenb got it in before me! Thanks <3

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.

None yet

3 participants