search

jsonapi-suite / jsonapi_compliable

MIT License
20 stars 35 forks source link

Add before_commit hooks #72

Closed richmolj closed 6 years ago

richmolj commented 6 years ago

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 commented 6 years ago

@wadetandy

richmolj commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.