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

Exception handling for delivery lines #37

Open
synth opened this issue Sep 8, 2024 · 1 comment
Open

Exception handling for delivery lines #37

synth opened this issue Sep 8, 2024 · 1 comment
Assignees
Labels
docs enhancement New feature or request

Comments

@synth
Copy link

synth commented Sep 8, 2024

With respect to our ApplicationDelivery class:

class ApplicationDelivery < ActiveDelivery::Base
  self.abstract_class = true

  register_line :mailer, ActiveDelivery::Lines::Mailer,
                resolver: ->(klass) { klass.name&.gsub(/Delivery$/, 'Notifier')&.safe_constantize }

  register_line :sms, ApplicationLine,
                resolver: ->(klass) { "#{klass.name}::SmsNotifier".safe_constantize }

  register_line :webhook, ApplicationLine,
                resolver: ->(klass) { "#{klass.name}::WebhookNotifier".safe_constantize }

  register_line :slack, ApplicationLine,
                resolver: ->(klass) { "#{klass.name}::SlackNotifier".safe_constantize }
end

Is your feature request related to a problem? Please describe.

We have noticed a situation where an exception is raised in one of the delivery lines, namely the :sms line, and then other lines declared after it are not running. I tried looking for information on the expectation of exceptions in this gem, but couldn't find much. My initial personal expectation is that each line should be robust and an exception in one line shouldn't affect the ability for the others to run. But perhaps that is out of scope for this gem and the behavior is too application specific, not sure.

Describe the solution you'd like

Perhaps, a flag or option to have a consolidated exception handler, so we can generally log and not block additional line processing.

Describe alternatives you've considered

Building exception handling ourselves into each delivery line.

@synth synth added the enhancement New feature or request label Sep 8, 2024
@palkan
Copy link
Owner

palkan commented Sep 17, 2024

each line should be robust and an exception in one line shouldn't affect the ability for the others to run

Yeah, that's something I considered initially but decided to go with the

that is out of scope for this gem and the behavior is too application specific

The problem is that I want to ensure that developers observe the exceptions, but there was no good one API to rule them all solution at the time (like, logger.error is just sweeping the dirt under rug). In modern Rails, however, we have the Rails.error.report interface which could be used here.

Though I'm still not convinced. In general, I prefer libraries not to make assumptions regarding exceptions and just let them go up the stack. However, we must clearly state in the docs that an exception in one line would interrupt the notification propagation.

@palkan palkan added the docs label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants