search

tommybananas / finale

Create flexible REST endpoints and controllers from Sequelize models in your Express app
188 stars 36 forks source link

Add transaction support #31

Open awm33 opened 6 years ago

awm33 commented 6 years ago

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

tommybananas commented 6 years ago

Couple questions:

  1. is this optional? Where is the flag?
  2. Can you provide test cases and documentation?
  3. REST operations are typically one db operation anyway, what use cases are you targeting specifically?
awm33 commented 6 years ago

@tommybananas

  1. 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?

  1. 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.

awm33 commented 6 years ago

"A Note About Transactions" in the sequelize hooks docs (sorry, no anchor tags on the headers) has a more info as well.

tommybananas commented 6 years ago

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 notifications@github.com 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 .

awm33 commented 6 years ago

@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.

awm33 commented 6 years ago

@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?

tommybananas commented 6 years ago

@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.

tommybananas commented 6 years ago

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.

awm33 commented 6 years ago

@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.

tommybananas commented 6 years ago

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

hiradimir commented 5 years ago

I want this feature...

tommybananas commented 5 years ago

Once a complete PR is created I’ll go ahead and merge

On Tue, Feb 26, 2019 at 6:03 AM hiradimir notifications@github.com 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 .