Add before_commit hooks

[richmolj]

These hooks run after validating the whole graph, but before closing the
transaction. Helpful for things like "contact this service after saving,
but rollback if the service is down".

Moves the existing sideload hooks to the same place (the previous
behavior was to fire before validations).

Implemented by a Hook accumulator that uses Thread.current. I've tried
this a few different ways but recursive functions that return a mash of
objects seem to add a lot of complexity to the code for no real reason.
The premise of registering hooks during a complex process, then calling
those hooks later, is simpler.

This does add a small amount of duplication between the create/update
actions and the destroy action. This is because we're currently not
supporting DELETE requests with a body (nested deletes) so the
processing logic is different. Think this can be assimliated in a
separate PR.

*suggest viewing the first

[richmolj]

@wadetandy

[wadetandy]

Functionality looks good here, though I think a really valuable corollary piece of functionality would be a corresponding after_commit_abort (or something) hook. These would run in reverse order to before_commit hooks and only be enqueued once the corresponding before_commit completed successfully. This would allow hooks making non-transactional changes to perform custom rollback actions if later services are down or the actions otherwise fail (saga pattern). Return value of the before hook could be passed into the after hook for mitigating action:

before_commit only: :create do
  remote_record = do_a_thing_on_a_remote_api
  if remote_record.success?
    remote_record
  else
    raise Error.new('nope')
  end
end

before_commit_abort only: :create do |record|
  destroy_record(record)
end

Fine merging this in the meantime, but I think a few classes of before hooks will be problematic until we have something like this.

[wadetandy]

Array(only) to support before_commit only: :create

[wadetandy]

why unshift instead of push?

[richmolj]

Because we process the edges of the graph first, and work inwards. But the hooks should fire in the opposite order. This is tested here and I added a comment about it.

[wadetandy]

Should make this private

[richmolj]

@wadetandy good feedback, I believe it is addressed. I think after_commit_abort can wait until later, but I'm wondering if you could just call destroy_record before raising the error in your example? I see the note about firing in reverse order but not sure what the problem would be.

[wadetandy]

Calling destroy_record would work in the case that there was an error in this hook, but if a subsequent hook had a problem, there wouldn’t be an easy way to back out. Another possibility would be to make any before_commit optionally yield, in which case the yield would be where later hooks ran, allowing a rescue/back out action.

[wadetandy]

Looks good. Feel free to merge after seeing my last comment. Can you make a follow up issue if you aren’t going to add it to this PR?

[richmolj]

Oh I see - this is more about rolling back associated resources that didn't have an error. In my head I was thinking the most likely case is where one resource in the chain is a service call, but the rest is ActiveRecord (which would rollback the transaction after getting the error). I guess let's see how often it occurs to prioritize, but created an issue for it.