search

tommybananas / finale

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

Missing use of transactions #16

Closed Tmassery closed 6 years ago

Tmassery commented 6 years ago

I'd like to start a discussion (with my vote for yes) on if we would like Finale to support the use of transactions, either by default or by optional configuration.

My use-case:
I have a Person model, which uses the standard Finale/Sequelize setup. On my Person Sequelize model, I have some after hooks that are defined to publish AWS SNS messages enforming other systems of the updates. If the SNS messages are not published (which would only happen if SNS is down, or if our credentials/setup of it was incorrect), I'd like the whole operation to fail, and rollback. These SNS Publishes have to be on the Sequelize model not the Finale model, in order for the rollbacks to be transactional, as well as to support updates that didn't come from Finale defined routes.

Problem Sequelize Managed Transactions will handle the above use-case out of the box (http://docs.sequelizejs.com/manual/tutorial/transactions.html#managed-transaction-auto-callback-) but only if you use the transactions, which Finale does not currently have the capability to do so. So errors thrown in the after hooks on my models have no impact on the update. Whereas if I create a set of routes to call the same functionality with a transaction, the errors thrown in the after hooks have the desired effect of triggering the Sequelize auto-rollback feature.

Discussion

If people are interested, then I'm happy to discuss at exactly which step the transaction would be created, and how it would be passed in.

Thanks.

tommybananas commented 6 years ago

I haven't looked into this too closely yet but I would love to hear your thoughts.

Tmassery commented 6 years ago

@tommybananas interesting, I was only thinking of the transaction living locally at the time of the write milestone, but good call that having it around in the context for other milestones could be useful as well. Ideally, I don't think as a user I would want to have to pass it in in a Finale before hook for all routes, but maybe just a configuration option when setting up the resource? That could then set it in the context, and then each milestone just optionally use it in their model interaction if the key exists.

I'm new to this repo, (thanks again for Forking though since Epilogue bled to death) how do requests like this usually move forward?

jorrit commented 6 years ago

I think a PR is a good way to demonstrate feasability and allow the maintainer to comment on a concrete proposal.

tommybananas commented 6 years ago

Yea go ahead and submit a PR and I’ll try to get it merged in ASAP. I do think it would be ideal to keep it scoped as tight as possible unless the transaction api would actually contain useful information post write. But I’m not sure that it would? A transaction write failure should throw an error and reaching a subsequent milestone would imply a successful write. Feel free to give us your thoughts or submit a proposed implementation.

On Sun, Apr 1, 2018 at 3:20 PM Jorrit Schippers notifications@github.com wrote:

I think a PR is a good way to demonstrate feasability and allow the maintainer to comment on a concrete proposal.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/tommybananas/finale/issues/16#issuecomment-377813847, or mute the thread https://github.com/notifications/unsubscribe-auth/ACELE1Ji0yMOjufRl75AzopDKywgZgMNks5tkTaBgaJpZM4TC7xY .

Tmassery commented 6 years ago

Great thanks. I’ll play around with a couple different implementations and see what makes sense, and submit a PR when I get some time.

Thanks!

tommybananas commented 6 years ago

Closing because this seems to be stale