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

Add singleton class extension support to Bugsnag::Middleware::ExceptionMetaData #426

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

jnraine
Copy link
Contributor

@jnraine jnraine commented Feb 5, 2018

Using Bugsnag::MetaData to attach metadata or other Bugsnag-specific data directly to an exception class works great for the common case:

class MySpecialError < StandardError
  include Bugsnag::MetaData
end

# when something breaks
error = MySpecialError.new("Oh no!")
error.bugsnag_meta_data = {foo: :bar}

However, it doesn't support dynamically extending an exception class at runtime.

begin
  do_a_thing
rescue => e
  e.extend(Bugsnag::MetaData)       # extend singleton class
  e.bugsnag_meta_data = {foo: :bar} # add context
  raise e                           # re-raise, let other code send to Bugsnag
end

This used to work in Bugsnag 5.1.0 but broke when a type check was added in 6.0.0:

if exception.class.include?(Bugsnag::MetaData)

This pull request changes the implementation to use respond_to? over a type check. This supports the singleton class case above and any other case we've forgotten to handle. For example, a developer need not include Bugsnag::MetaData directly, they need only to define the bugsnag_meta_data method and Bugsnag::Middleware::ExceptionMetaData will pick up the data.

This change also applies to bugsnag_user_id, bugsnag_context, and bugsnag_grouping_hash.

Note: originally, I fixed my issue with a change to the type check – exception.is_a?(Bugsnag::MetaData) – which handles class and singleton class extension. After thinking on it for a bit and noticing that the code used to rely on respond_to?, I decided to go that route instead.

…onMetaData

Using `Bugsnag::MetaData` to attach metadata or other Bugsnag-specific data directly to an exception class works great for the common case:

```
class MySpecialError < StandardError
  include Bugsnag::MetaData
end

error = MySpecialError.new("Oh no!")
error.bugsnag_meta_data = {foo: :bar}
```

However, it doesn't support dynamically extending an exception class at runtime.

```
begin
  do_a_thing
rescue => e
  e.extend(Bugsnag::MetaData)       # extend singleton class
  e.bugsnag_meta_data = {foo: :bar} # add context
  raise e                           # re-raise, let other code send to Bugsnag
end
```

This used to work in Bugsnag 5.1.0 [1] but broke when a type check was added in 6.0.0 [2].

This commit changes the implementation to use `respond_to?` over a type check. This offers a more flexible interface. For example, an exception need not include `Bugsnag::MetaData` directly, they need only to define the `bugsnag_meta_data` method.

This change also applies to `bugsnag_user_id`, `bugsnag_context`, and `bugsnag_grouping_hash`.

[1] https://github.com/bugsnag/bugsnag-ruby/blob/v5.1.0/lib/bugsnag/notification.rb#L311-L317
[2] https://github.com/bugsnag/bugsnag-ruby/blob/94cea3195d0fb3b3ed1c79ab38d9d4b1b9a732a3/lib/bugsnag/middleware/exception_meta_data.rb#L10
@jnraine
Copy link
Contributor Author

jnraine commented Feb 5, 2018

Coveralls thinks coverage went down but given the changeset I'm not sure how that's possible. If you'd like me to add other specs, let me know.

@jnraine
Copy link
Contributor Author

jnraine commented Feb 13, 2018

@Cawllec would you be able to review this?

@kattrali kattrali merged commit f62cf92 into bugsnag:master Feb 13, 2018
@kattrali
Copy link
Contributor

Thanks @jnraine for the contribution.

@jnraine
Copy link
Contributor Author

jnraine commented Feb 13, 2018

Thanks for merging! When do you expect to cut another gem release?

@kattrali
Copy link
Contributor

Released in v6.6.4

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.

2 participants