-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Use transactions and expose a transactional API #14389
Conversation
b5d18b2
to
fadbb1a
Compare
fadbb1a
to
0d73def
Compare
Codecov ReportBase: 66.15% // Head: 66.15% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #14389 +/- ##
=======================================
Coverage 66.15% 66.15%
=======================================
Files 1058 1058
Lines 23026 23026
Branches 4125 4125
=======================================
Hits 15233 15233
Misses 6870 6870
Partials 923 923
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Question for you @alexandrebodin (forgive me because I'm not super familiar with transactions) but what if you wanted to rollback the transaction manually such as throwing a custom error within the transaction? |
Looks like the code is automatically rolling back on exception in try catch here |
The transactions will rollback automatically on error. What do you have in mind for a manual rollback ? There is another API where you can init the transaction object and commit / rollback manually that I plan to expose. Is this what you would expect? |
I can't think of a reason for manual rollback, however, in .NET afaik the options for rollback/commit are exposed for executing manually on error. If there is another API that can give these controls to users should be good enough who want to do it manually. |
Nothing specific honestly, more so thinking out loud. If you already plan to expose that type of API it should be fine. |
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: |
This may be something of a stupid question, but will transactions still be implicit even if they are within services? Something like
Additionally, if this was within a try catch block and an error was thrown, would it automatically rollback? Or would you have to manually call something like |
This API will wrap anything you do in it within a single transaction (I have to figure out nested transactions that strapi will run by default too) and auto commit/rollback if the function throws (you need to make sure the function is either async, or return a promise though) The main difficulty is how I can expose some locking options to set the forUpdate/forShare & so on when call services you won't have it but I probably will have to expose it in the entityService |
Any updates on the progress of this pull request? |
Alex is currently on holiday so it'll prob be a week or two until this gets any action. |
is there a fixed delivery date for this feature? |
I wait for it |
It's been almost 3 weeks now, any progress on this? What relation does #14564 have to this? |
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: https://forum.strapi.io/t/using-database-transactions-to-write-queries-in-strapi/14963/13 |
Hello, There is no fixed delivery date on this. I'm doing this outside of the current planned work when I have time. I'll get back to it as soon as I can. In the meantime if you want to help make this move forward, try the PR out and share the bugs you encounter. This 1st Proof of concept should be testable. Also feel free to propose changes to the code if you review it. |
Thanks for the update. I'll try and patch this on our development backend this week and test it to see what errors I can get and then give any feedback I can and if I'm able to contribute |
Sorry for the very late reply, I was on holiday. I patched this pull request and the patch was applied fine with no issues, the server still started fine. However, when I wrapped my code as you suggested
Whatever endpoint I used it on would always return a 404. I tested some things that I thought may be causing some sort of side effects, such as the fact that some queries were still using db.query rather than the entityService and I still got the same error. I presumed that the transaction wasn't executing whatever was within the block, but this seems not to be the case. I tested it on two different endpoints, both of which are quite complex and use multiple services, so may not be the best example on which to test this basic implementation. However, on our authentication endpoint, when I submitted a second request after the first one, I would still get 404 but on my server, I would get an error specific to the second service, essentially checking whether an email already exists or not on an external service, see the following
This implies that the code inside the block was actually running and that for some reason something internal to Strapi was causing a 404 to be returned. Do you have any idea why this might be and any suggestions for further testing? |
Are you awaiting the transaction to return the result of it @callum-gander ? if the response is empty Strapi will automatically return a 404 that might be the reason const result = await strapi.transaction(() => { /* ... */ });
return result |
Ah, I wasn't, my mistake, it's working now on both endpoints. When I throw an error inside the block, transactions correctly rollback. Both endpoints also have lifecycle hooks that also seem to be working fine. I have two additional questions. Firstly, when logging the transaction, it's returning undefined, should this be the case? The transaction clearly is still successful and all behaviour is correctly, I'm just wondering whether this is supposed to be returned? Secondly, are there any specific tests or test cases you'd be interested in having me run? I'm not sure how else I can test this to verify everythings behaving correctly other than by putting it in production which I can't really do. |
A bit more on this, I've written a basic k6 test on one of our endpoints and simulated multiple concurrent users using the service, which looks something like the below test
This wasn't a particularly thought out test, just seeing what responses I can get from multiple concurrent users. In total, 30 requests should be made, 15 at 0s and 15 at 15s. Only 15 were made, none of the 15 requests were responded to until the test timed out after 30s. Of those 15, 9 succeeded and all the data was correctly created (and updated) in the database. A further 2 got 401s, I'm presuming because they triggered some internal Strapi rate limiter per JWT that I'm not aware of but please correct me if I'm wrong, and then finally 4 got the following error
The code is definitely wrapped in a transaction block, so what does it mean by the
It would then seemingly correctly transactionally create orders. However, there was one very odd problem. For each order, two orders were being created in the orders collection, so 60 orders rather than equal to the amount of requests, 30. All the fields that used data in the request body or were calculated in the controller were in one order minus two fields related to an external service that is called in the controller. Another order was also created with all the fields empty except for the two fields related to an external API call. This external call essentially creates the order on a separate external service. We already have rollback logic for if an error occurs, the service will be called and the order deleted with periodic retries if that fails. But I'm not sure what's going on here at all. Any ideas? |
Further update on this, after reading this https://softwareengineering.stackexchange.com/questions/280324/is-it-better-to-make-database-calls-or-external-api-calls-first-in-the-context-o, I realised I should probably move the request completely out of the transaction and that this was a mistake. I did so the code looks something like below
Still getting 200 responses to all requests but still getting the weird error where they're being split into separate orders |
- fix the typings - add rollback and commit to the callback params
commit: () => Promise<void>; | ||
}) => Promise<void> | ||
): | ||
| Promise<unknown> |
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.
I'm assuming this is the equivalent of ReturnType<typeof trx.run>
? What does the unknown means here? (basically, what does a trx.run can return?)
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.
it returns what the callback returns, which could be anything
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.
Thanks 🙂
packages/core/database/lib/index.js
Outdated
}; | ||
} | ||
|
||
return transactionCtx.run({ trx, commit, rollback }, async () => { |
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.
I must admit I'm a bit confused by the fact you're creating a trx
variable, then a commit
& rollback
function, and then get three variables with the same name from the transactionCtx.run
callback. Could you explain the process, please?
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.
oh no that's just wrong it should be in cb({ trx, commit, rollback }), will fix it
- fix callback return type - fix callback params
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.
Could we have at least one unit test that verifies that rollback or commit are actually called?
Other than that, LGTM, nice work
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.
Waiting for the tests to pass & LGTM
Will this be included in todays release? |
Yes, however, we won't document it for now as we're making sure we have a stable & clean API for the long term before making it official/public. Basically, feel free to use it, but it might change a bit in the future. |
Hi, thank you for bringing this feature to the latest release, much appreciated! (I don’t know if this PR is now followed by @alexandrebodin or @Bassel17) I’m trying to understand how to use this, especially how to lock a row for update while reading its value (e.g. read current balance before running an update query on that same balance). With a direct knex TX I would select the row containing the balance with a How can I achieve the same result here? Thank you for your time. |
valid point |
Please write a doc or article for this soon |
@liarco Have you found a way to solve your problem? |
Hi @NikitaKemarskiy, I might be wrong but I think what you need is a bit different since transactions alone cannot solve your problem.
If that's the case, then it will probably fail due to the fact that transactions just ensure all or none the statements are committed, but it doesn't lock all resources for you automatically. Some automated locks will occur to ensure data integrity (e.g. if you alter a row, then that row will be locked until the transaction passes or fails), but if your check is about data presence, then I'm afraid you might have to lock the entire table during each transaction. By locking the table at the beginning of your transaction you should have no concurrency problem. (Please note that locks might work inconsistently across different DB platforms, e.g. MySQL vs Postgres, so make sure you verify your SQL works as expected before implementing it on Strapi). I hope this helps. |
@liarco Thanks for response! In this case I'd consider solution with some unique identifier. Just for example: if we want to restrict number of entities per day, we could come up with a unique constraint where key = ddmmyyyy. It will allow fast operation without table lock. |
@NikitaKemarskiy if you can come up with a unique identifier then that's great. Remember SQL DBs also support uniqueness on compound indexes (e.g. you can combine two columns like Also please remember Strapi doesn't handle custom indexes for you at the moment so you have to create them manually and also make sure they are kept during migrations. |
@liarco Yes, also as far as I understand in theory "Repeatable read" transaction isolation level should cover my case. But of course tougher isolation level means slower DB operation. |
What does it do?
This is a working PR to introduce and use transactions within Strapi.
They can be used as follows
This PR takes it's root from the discussions in #12715