finale
finale copied to clipboard
tommybananas
Add transaction support
Adds transaction. With this PR, hooks can optionally make use of the transaction by adding it to the context in a pre-hook and committing in a post-hook.
Addresses https://github.com/tommybananas/finale/issues/16
Couple questions:
- is this optional? Where is the flag?
- Can you provide test cases and documentation?
- REST operations are typically one db operation anyway, what use cases are you targeting specifically?
@tommybananas
- is this optional? Where is the flag?
In the hooks, a user does not have to pass the options.transaction to operations being performed there. However, the rollback on failed promise chain would still be there. If I were to add an option on the resource as transactions or useTransactions true/false suffice?
- Can you provide test cases and documentation?
Yes, but I'd like to nail down the approach first.
REST operations are typically one db operation anyway
Many endpoints have to schedule jobs, hit third party services, or alter another part of the db. The specific use case I am looking for is writing an event to another table within the dband hitting a 3rd-party service.
"A Note About Transactions" in the sequelize hooks docs (sorry, no anchor tags on the headers) has a more info as well.
Correct me if I’m wrong but right now your implementation uses managed transactions which might make it hard to work with non promise based third party tooling. We probably want to expose a way to pass in transaction options to use with an unmanaged transaction style that the user can commit and rollback more freely? So maybe the implantation takes in translation options in any pre write hook and then be accessed to finalize in any post write hook? Let me know what you were thinking though
On Mon, Oct 1, 2018 at 4:50 PM Andrew Madonna [email protected] wrote:
"A Note About Transactions" in the sequelize hooks docs http://docs.sequelizejs.com/manual/tutorial/hooks.html (sorry, no anchor tags on the headers) has a more info as well.
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/tommybananas/finale/pull/31#issuecomment-426075307, or mute the thread https://github.com/notifications/unsubscribe-auth/ACELE3wHxIrwIOnYyS590R2EhiJQ6WxXks5ugo4QgaJpZM4XCeTx .
@tommybananas A simple approach may be that a user can add an unmanaged transaction to the context in one of the pre-write hooks, if the read or write hook sees a transaction on the context, then it passes it down to any db operations. They would have to commit is a post-write hook, but if they are using a transaction, they probably want to control when that happens anyway.
@tommybananas I updated the code with the new approach. One concern I have with this, is if an error occurs somewhere along all the hooks, including internal lib hooks, what's the best way to rollback?
@awm33 couple thoughts.
One is that it might make sense to change default error and final milestone handlers to check for transactions and commit/abort automatically for simple cases and minimizing boilerplate.
I'm not a huge fan of requiring the override of the error handler because they are separate things and making them reimplement the error handler seems like a bad idea. Another option would be a separate milestone for transactions.. just brainstorming.
but I am open to getting the first run of this feature out asap so long as we document it and write a test or two.
@tommybananas I think the approach is close and I'm starting on tests. I think I need to add some error handling, since we can't really handle that by adding a hook. An error can occur anywhere, and would short circuit, and we would probably want to rollback on error.
It looks like the error handling is here? https://github.com/tommybananas/finale/blob/master/lib/Controllers/base.js#L179
It looks like we would want to add transaction error handling to each catch case except for the errors.RequestCompleted handler.
I'm thinking there if context.transaction is set, calling context.transaction.rollback().
If that error handling is in place, I think the rest can be managed using milestone hooks.
Thank you for helping me through this approach.
hmm.. I guess that makes sense. I'm trying to figure out if there would ever be a reason they would not want to rollback the tx on error. In which case they would have to basically override all the error handlers in order to prevent the automatic rollback which would be a pain. But seeing as you are the only one requesting this functionality.. I'm fine with it. If you can add documentation at the very least I will merge this. Thanks for your contribution
Once a complete PR is created I’ll go ahead and merge
On Tue, Feb 26, 2019 at 6:03 AM hiradimir [email protected] wrote:
I want this feature...
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tommybananas/finale/pull/31#issuecomment-467412611, or mute the thread https://github.com/notifications/unsubscribe-auth/ACELE-u-LU6v2hWKHqJHvyfi5_kzQVloks5vRSKegaJpZM4XCeTx .