Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): implement transaction lifecycle hooks #1213

Merged
merged 2 commits into from
Dec 17, 2020
Merged

feat(core): implement transaction lifecycle hooks #1213

merged 2 commits into from
Dec 17, 2020

Conversation

rhyek
Copy link
Contributor

@rhyek rhyek commented Dec 15, 2020

Current lifecycle hooks cover entity and unit of work events, but
transaction-related ones are missing. Add beforeTransactionStart,
afterTransactionStart, beforeTransactionCommit, afterTransactionCommit,
beforeTransactionRollback, and afterTransactionRollback

Closes #1175


Although #1175 mentions implementing "after" entity hooks post commit, they are not included in this PR as they are not needed. Refer to the UserSubscriber example class in test file GH1175.test.ts to see how to implement an ad-hoc afterCommitCreate hook using a combination of afterTransactionStart, afterCreate, and afterTransactionCommit.

Currently, all this PR is missing right now is documentation. Please let me know if you need me to do that as well, but I was thinking maybe @B4nan could lend a hand with that as he might have a better idea regarding form, style, and content.

Current lifecycle hooks cover entity and unit of work events, but
transaction-related ones are missing. Add beforeTransactionStart,
afterTransactionStart, beforeTransactionCommit, afterTransactionCommit,
beforeTransactionRollback, and afterTransactionRollback

Closes #1175
@rhyek
Copy link
Contributor Author

rhyek commented Dec 15, 2020

Btw, these hooks work correctly with nested transactions (AbstractSqlConnection). They are not run for nested transactions, only root ones. Testing is very exhaustive and covers many cases including those.

@B4nan
Copy link
Member

B4nan commented Dec 15, 2020

Looking good, thanks! Will have few code style hints, might change it myself as I want to fiddle around with this locally anyway to understand it better.

Also few lines in MongoConnection are not covered (probably just branches caused by the optional chaining - if so, let's just force ignore those via istanbul ignore comments).

That TransactionEventBroadcaster thing looks a bit unneeded, but will need to dig deeper to better understand the code, might be missig something now.

One thing that looks stinky is that you are using isRootTransaction only in the SQL driver, not in mongo one.

@rhyek
Copy link
Contributor Author

rhyek commented Dec 15, 2020

@B4nan glad to help.

Also few lines in MongoConnection are not covered (probably just branches caused by the optional chaining - if so, let's just force ignore those via istanbul ignore comments).

I will check this today.

That TransactionEventBroadcaster thing looks a bit unneeded, but will need to dig deeper to better understand the code, might be missig something now.

The idea here is that what the broadcaster is is agnostic to the connection and we could provide any extra context we want from the call site in the future. For example, we could choose to provide the unit of work when doing implicit transactions. I actually had planned to define the EventBroadcaster interface in the Connection core package. I will do that today. (Edit: TransactionEventBroadcaster is already a class defined in the core package).

One thing that looks stinky is that you are using isRootTransaction only in the SQL driver, not in mongo one.

Currently mongo doesn't allow for child transactions (save points), but they might in the future and in that case checking to see if it is a root transaction or not sounds like a general concern. Let me know your thoughts. I can easily move that to AbstractSqlConnection.

this may be useful. also add tests for transaction hooks
using mongo

Related #1175
@rhyek
Copy link
Contributor Author

rhyek commented Dec 15, 2020

Pushed some of the changes mentioned, added tests for mongo, and fixed coverage errors.

@B4nan B4nan merged commit 0f81ff1 into mikro-orm:master Dec 17, 2020
@rhyek rhyek deleted the tx-hooks branch December 17, 2020 20:41
@rhyek
Copy link
Contributor Author

rhyek commented Dec 17, 2020

Thank you, @B4nan.

@mcasarrubios
Copy link
Contributor

Hi @rhyek, I was struggling with afterTransactionCommit hook because I didn't understand why uow param was undefined.
Watching the GH1175.test.ts file I realize that it only is defined in implicit transactions.

Is there a reason why uowis undefined in explicit transactions? I would love that it was defined as in afterFlush event. The thing is that I don't know if it is a constraint of mikro-orm or if it could be done.

I've created the question in SO too: https://stackoverflow.com/questions/76851480/in-mikro-orm-is-it-possible-to-track-unit-of-work-changes-in-the-aftertransacti

Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Implement afterTransactionCompletion, afterCommitCreate, et al similar to Hibernate counterparts
3 participants