-
-
Notifications
You must be signed in to change notification settings - Fork 736
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
chore: handle transactions already started at the controller layer #4953
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
@@ -104,7 +104,7 @@ export interface IUnleashOptions { | |||
versionCheck?: Partial<IVersionOption>; | |||
telemetry?: boolean; | |||
authentication?: Partial<IAuthOption>; | |||
ui?: object; | |||
ui?: IUIConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice to have... probably I can remove it from this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very elegant solution. I'd like us to handle transactions around the service boundaries but it's great for the transitional period.
Yes, I've added a comment in the method we're using |
About the changes
This PR adds a method to safeguard us from opening a new transaction while inside another transaction, resulting in two isolated transactions that will not be atomic (if one fails, the other might still complete successfully).
https://github.com/knex/knex/blob/bbbe4d4637b3838e4a297a457460cd2c76a700d5/lib/knex-builder/make-knex.js#L143C5-L144C88
We're currently opening transactions at the controller layer
unleash/src/lib/features/export-import-toggles/export-import-controller.ts
Lines 206 to 208 in 2746bd1
but in some other places, we do it at the store level:
unleash/src/lib/db/access-store.ts
Line 577 in 2746bd1
Alternative
We can remove store-level transactions and move them to the controller following this approach:
unleash/src/lib/services/index.ts
Lines 282 to 284 in cb03497
unleash/src/lib/features/export-import-toggles/export-import-controller.ts
Lines 206 to 208 in cb03497
This option is more expensive because we have to: