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

Question about potential pitfall #137

Closed
bemurphy opened this issue May 23, 2016 · 7 comments

Comments

@bemurphy
Copy link

commented May 23, 2016

Hi Kris, thanks for your work on this library. I've been evaluating it recently, and the API is really nice to use. I was also impressed by the simplicity with which adapters like wisper-sidekiq hook into the broadcast setup.

One thing that came to mind looking through the examples, is how would I avoid a 'dead letter' style problem that might arise from changing event key names. Allow me to give an example:

class SignupUser
  def call(params)
    # Some business logic around validation and User.create

    # A listener is subscribed elsewhere to :user_created and sends a welcome email.
    # This might replace a hardcoded dependency call like
    # UserMailer.welcome_email(user).deliver_later
    broadcast(:user_created, user.id)
  end
end

I come back later and think, :user_created should really be :user_signup_successful and make some replacements. Due to some mistake, I miss updating the listener method responsible for the mailing with the new event key. If I was using hardcoded dependencies, even if I didn't have an integration test directly covering the UserMailer use, I'd get an exception if renaming things had broken the code and tests should fail. In the case of pub/sub, the tests would probably keep trucking and not alert me to the problem (even assuming I had unit tests asserting the broadcast was made).

In practice, is this a problem? I know this is not unique to Wisper specifically as it arises in many similar pub/sub setups. How do you approach this? Does it just require discipline to ensure your integration tests are up to snuff?

Thanks again for your time and effort.

@krisleech

This comment has been minimized.

Copy link
Owner

commented May 23, 2016

Yes, this is certainly an issue with having softer coupling between causes and effects.

It is sort of the same problem as doubles stubbing methods which no longer exist in the real implementation, which RSpec's "verifying doubles" aims to solve.

The best way I can see to solve this is to have listeners register the events they can handle. This wouldn't require any changes to Wisper itself, but a new module which listeners include that provides a method which lets them register the name(s) of events they handle. Before any tests are run you could subscribe a global listener which just records the name of every event broadcast. So we end up with a list of expected events and actual events, the diff should tell us events we expected to be broadcast but where not. I don't know if it is possible, with RSpec, to have a spec run last. Or if there is some other way to hook in to RSpec to do this...

@krisleech

This comment has been minimized.

Copy link
Owner

commented May 23, 2016

Another potential solution is to use constants instead of string/symbols for event names.

class MyPublisher
  UserCreated = 'user_created'

  def call
    broadcast(UserCreated, ...)
  end
end

class MyListener
  define_method MyPublisher::UserCreate do
    # send mail
  end 
end

You can then change the name of the constant and it will break the specs.

@bemurphy

This comment has been minimized.

Copy link
Author

commented May 24, 2016

@krisleech thanks so much for the reply. The use of constants would be a very low-code solution to this!

Come to think of it, we had a similar problem in a different domain. We use rollout for feature flags, and sometimes would manually roll out to the wrong keys (ie email_broadcast instead of email_broadcasts. We stopped having this problem by wrapping rollout with our own API and registering features up front (sort of like you've suggested here):

$features.register(:email_broadcasts, "Allow Users to Broadcast to Members")

Great food for thought, I'm confident if I ran into trouble in this area now, I'd have a good bag of tricks handy. It's also made me take a moment to reflect; it's a bit odd to be worried about looser coupling, as it's usually a better goal to shoot for anyhow.

Thanks again.

@bemurphy bemurphy closed this May 24, 2016
@krisleech

This comment has been minimized.

Copy link
Owner

commented May 24, 2016

No problem - let us know if you have any further ideas or successes in this area 👍

@bemurphy

This comment has been minimized.

Copy link
Author

commented Jun 19, 2016

Wanted to leave some further ideas here in case anybody else runs into this. I was refactoring a background job tonight that performs many notification mailings, and wanted to see what it would look like if I extracted them as they were cluttering it. In the process, I hit this very pitfall! I broadcast an event ending in _successful and I accidentally named a listener method with _success.

Constants indeed turned out a nice fix to this. I came up with two patterns based on them that are sugar to make it a little easier on the eyes.

First, a class factory method, which has the nice side effect of enforcing SRP if you want that:

def EventHandler(event)
  Class.new do
    define_method event do |*args|
      call(*args)
    end
  end
end

class OrderNotifier < EventHandler(Order::SUCCESS)
  def call(object)
    puts "#{self.class.name}: object = #{object}"
  end
end

Another involved a mixin that maps the event to a local method. I ended up with class method calls something like this:

subscribes_to Order::SUCCESS # defaults to a `#call` method
# or
subscribes_to Order::SUCCESS, with: :some_other_method
subscribes_to Order::FAILURE, with: :some_failure_related_method
@emq

This comment has been minimized.

Copy link

commented Nov 9, 2018

Hello 👋

I'm aware this is rather old issue, but what the hell 😛

As I finally had chance to integrate wisper in project that I'm currently working on I actually was thinking about this as well - I quickly realized that currently current approach is super error prone and I have to defend myself with integration specs.

Maybe we can just add some sort of strict configuration in Wisper itself? 🤔

Wisper.configure do |config|
  config.strict = true # false by default as it is now
end

then during broadcasting we would let it fail explicitly (that what I would personally expect in the first place)

def broadcast # ...
  if should_broadcast?(event) && (Wisper.strict || listener.respond_to?(method_to_call)) # ...

I mean that was my initial idea - I'm not sure if any potential side effects are lurking around this approach.

wdyt @bemurphy @krisleech ?

Plus I will give that class-factory-method a try as well, thanks! 😉

@bemurphy

This comment has been minimized.

Copy link
Author

commented Nov 9, 2018

@emq I end up relying on integration style specs often as well, in combination with the test_after_commit gem, if necessary depending on Rails version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.