{WIP} Add on_rollback hook to resources

[wadetandy]

From @jsonapi-suite/jsonapi_compliable#137, this allows an API
maintainer to run a rollback hook on a given resource if an error occurs
after the resource's before_commit hook is successful, but before
the transaction actually completes. This will be helpful in situations
where a user would like to rollback something that cannot be
automatically undone in the transaction. The on_rollback hook will
receive as its argument the return value of its before_callback hook
(or the created/updated/deleted record if no before_callback hook is
provided).

class EmployeeResource
  has_many :positions

  before_commit only: :create do |record|
    # Makes HTTP request and returns ID of created record
    PayrollSystem::Employee.create(record)
  end

  on_rollback only: :create do |sap_record_id|
    # Delete created record
    PayrollSystem::Employee.find(sap_record_id).destroy
  end
end

class PositionResource
  before_commit do |record|
    raise 'boom'
  end
end

If creating an employee and a nested postion, the position will blow up,
and the rollback hook will clean up after itself.

[wadetandy]

@richmolj I'm not ready to merge this yet as I still want to do some clean up of the Util::Hooks code at least, but I wanted to get it out for feedback around the API before I do anymore on it.

@Startouf This addresses jsonapi-suite/jsonapi_compliable#137 (at least as far as I described and intended in jsonapi-suite/jsonapi_compliable#72 (review) ). Please let us know whether this addresses the problem you were trying to solve?

[richmolj]

I love that you took this on ❤️ 🎉 !

Had a few comments on particulars but the broad strokes look correct to me 👍

[richmolj]

FYI this is Hook.add but is specific to before_commit

[richmolj]

I know this is existing behavior, but we should try to run in the resource instance context if possible.

[richmolj]

I'm going back and forth on it myself, but I wonder how much this should be tied to a given before_commit hook. Something like:

before_commit :update_service do
  perform do
    # ... logic ...
  end

  rollback do
    # ... logic ...
  end
end

The fact that the add_hook method is accepting both makes it seem like they should be tied together, and would be easier to separate concerns

[richmolj]

I know this is how the existing spec is written, but I think that's compliable legacy. We can call .build and .save independent of a controller here, I think. Probably fits the "resource testing" context actually.

https://github.com/graphiti-api/graphiti/blob/master/spec/pagination_spec.rb#L4

https://github.com/graphiti-api/graphiti_spec_helpers/blob/master/lib/graphiti_spec_helpers/rspec.rb

[richmolj]

My comment on perform do ... end; rollback do ... end might make this easier - take the same array and reverse it.

[wadetandy]

closing in favor of #67