collectiveidea / interactor

Interactor provides a common interface for performing complex user interactions.
MIT License
3.36k stars 212 forks source link

Organizer's top-level `rollback` should be called #151

Closed lazyatom closed 6 years ago

lazyatom commented 6 years ago

If I have an Organizer which defines a rollback method, I'd expect it to be called at some point if any of the organised interactors fail. For example:

class SomeInteractor
  include Interactor
  def call
    # do something
  end
  def rollback
    puts "rolling back SomeInteractor"
  end
end

class SomeOtherInteractor
  include Interactor
  def call
    context.fail!
  end
  def rollback
    puts "rolling back SomeOtherInteractor"
  end
end

class MyThing
  include Interactor::Organizer

  organize SomeInteractor, SomeOtherInteractor

  def rollback
    puts "rolling back organizer"
  end
end

Given the interactors above, when I call MyThing.call, I'd hope to see:

rolling back SomeInteractor
rolling back organiser

but instead, I see:

rolling back SomeInteractor

I think this is because when we run! the organiser interactor, it doesn't mark itself as having been called within the context. Is there a good reason for this?

(If we did change this such that the organiser's rollback was called, this might also go some way towards addressing the same use cases as #147.)

lazyatom commented 6 years ago

I've solved this locally with the following:

class MyThing
  include Interactor::Organizer

  # as above ...

  def call
    context.called!(self)
    super
  end
end
laserlemon commented 6 years ago

@lazyatom Hello! The reason an organizer wouldn't itself be rolled back is because it was never fully called. When one of its organized interactors fails, it stops short of its full successful execution. Rollbacks are specifically meant to act on an interactor or organizer that has been fully executed.

If there's some partial cleanup that needs to be done, I would expect that the individual interactors would handle the cleanup efforts related to their own manipulations.

The same is true for an individual interactor. If an interactor fails before it completes, its own rollback method is not called.

I hope that makes sense and helps!

lazyatom commented 6 years ago

Thanks @laserlemon, that makes sense. Let me give you a more concrete example -- perhaps you can suggest a better interactor structure that works with the existing rollback mechanism..

We have an organised group of interactors managing a purchase, roughly of the form:

class PurchaseThing
  organise CheckPermissionToPurchase,
           CreatePurchase,
           UpdateStockSystem

  around do |interactor|
    context.analytics_data = { ... build data for analytics ... }

    interactor.call # the organised interactors will add more data to the context, which we merge below

    context.analytics_data[:currency] = context.currency
    context.analytics_data[:amount] = context.amount.cents
    context.analytics_data[:seller_id] = context.purchase.seller_id
    send_to_analytics(context.analytics_data)
  end
end

Within the any of these steps, we might get an error, but in that case I'd still like to send some data back to the analytics service, normally including some details about the error. I had wanted to approach this with a single, generic rollback at the organiser level, e.g. the interactors fail as normal, putting some data in the context:

class CreatePurchase
  def call
    purchase = make_purchase
    if purchase.save
      context.purchase = purchase
   else
      context.fail!(errors: purchase.errors)
    end
  end
end

... and then in the organiser:

class PurchaseThing
  # organize ... etc (see above)

  def rollback
    context.analytics_data[:errors] = context.errors
    send_to_analytics(context.analytics_data)
  end
end

I can see now that the organiser rollback I was hoping for is a little inconsistent with the ones in the interactors themselves, but I think the only other solution is to duplicate the "inject error data and send it to analytics" bit from the end of the around hook, into the failure brach of every call method of each of the inner interactors. I don't think there's any way of checking within the organiser if any of the interactors failed, because calling fail! stops execution of any of the hooks too.

It was a while ago, but I think I spent some time looking at the possibility of a failed hook, but decided that perhaps that was just duplication of the rollback mechanism. Maybe adding a failed hook would be better? In plain Ruby terms, I am trying to get the equivalent of an ensure section -- even if there's an interactor failure, make sure this thing runs. How would you suggest approaching this? Thanks for your time & attention!

laserlemon commented 6 years ago

I assume that you're not getting to the send_to_analytics line of your around block if one of the organized interactors fails. Is that accurate?

lazyatom commented 6 years ago

That's correct, IIRC; any call to context.fail! within one of the interactors will stop execution at that point, and we never return to the around block.