-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
simple implementation of transactions #12715
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12715 +/- ##
==========================================
- Coverage 57.52% 57.50% -0.02%
==========================================
Files 1262 1262
Lines 30729 30739 +10
Branches 5763 5766 +3
==========================================
Hits 17676 17676
- Misses 11224 11232 +8
- Partials 1829 1831 +2
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. |
|
Great to see this in a PR! it looks a lot like what we planned during in the RFC. Gonna take a while to review but we have it on the books 💯 |
|
May I add my 2 cents to this PR from the client of this code point of view? What is EntityManager? It is not part of the documented public Strapi interface. The only reference to it I could find in the documentation is an |
|
@tuxuuman I patched my version of the strapi/db with this PR and been using it for a while and faced an issue with this approach. Specifically, I can't seem to find the way to create entries with repeatable components using EntityManager. Official documentation here shows how to create components alongside the content-type entity using EntityService API. It works, I can confirm it. But if I want to create an entity within a transaction, I have to use EntityManager API. So, either the EntityManager should be fixed to handle the components or, perhaps, Strapi team may have different suggestion, like somehow expose an instance of EntityService that uses the transaction object internally. I guess @Convly or @alexandrebodin should be best to comment here. |
|
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/8 |
|
@tuxuuman @alexandrebodin As I continue using this PR patched over the strapi v4.1.5. I encountered one more limitation with this implementation. But the proposed implementation does not allow it. Without locking, you can't reliably run the code that is given as an example in the description here. If you run it multiple times in parallel, you may get multiple purchases created with the negative balance. Because each transaction allow reads to succeed without waiting for transactions to finish first. |
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.
My comments suggest how to update this implementation to support high level of transaction isolation.
Without the isolation, it is not possible to do ACID transactions with Strapi, which makes them unsuitable for many applications (think of payment transactions for example).
| entityManager: EntityManager; | ||
|
|
||
| query<T extends keyof AllTypes>(uid: T): QueryFromContentType<T>; | ||
| transaction(cb: (em: EntityManager) => Promise<any>): Promise<void>; |
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.
Extend the transaction method to allow specifying locking (optionally)
type LockType = 'shared' | 'exclusive';
type TransactionOptions = {
lock: LockType;
}
-transaction(cb: (em: EntityManager) => Promise<any>): Promise<void>;
+transaction(cb: (em: EntityManager) => Promise<any>, options?: TransactionOptions): Promise<any>;| } | ||
|
|
||
| getConnection(tableName) { | ||
| async transaction(cb) { |
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.
Transaction needs to take in an options object that allows specifying locking
-async transaction(cb) {
+async transaction(cb, options = {}) {
await this.connection.transaction(async trx => {
- const em = createEntityManager(this, trx);
+ const em = createEntityManager(this, trx, options.lock);
await cb(em);
});
}| const connection = tableName ? this.connection(tableName) : this.connection; | ||
| const connOrTrx = trx ? trx : this.connection; | ||
| const connection = tableName ? connOrTrx(tableName) : connOrTrx; | ||
| return schema ? connection.withSchema(schema) : connection; |
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.
The getConnection method should take the lock option and return locked transacting connection.
-getConnection(tableName, trx) {
+getConnection(tableName, trx, lock) {
const schema = this.connection.getSchemaName();
const connOrTrx = trx ? trx : this.connection;
const connection = tableName ? connOrTrx(tableName) : connOrTrx;
- return schema ? connection.withSchema(schema) : connection;
+ const schemaConn = schema ? connection.withSchema(schema) : connection;
+ if (lock) {
+ return lock === 'exclusive' ? schemaConn.transacting(trx).forUpdate() : schemaConn.transacting(trx).forShare()
+ }
+ return schemaConn;
}| }; | ||
|
|
||
| const createEntityManager = db => { | ||
| const createEntityManager = (db, trx) => { |
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.
The createEntityManager should take the lock parameter and pass it to the createQueryBuilder.
And the queryBuilder should pass it along the trx everywhere and into the getConnection method call.
|
Could this possibly be resolved be reviewed and resolved soon? This is critical requirement for our business and we can't launch our app without it. We have a lot of related collections whose entries need to be created independently then put together, without transactions we can't guarantee that, particularly given our multi-instance deployment. If we'd have known v4 didn't include the ability to use transactions, amongst many other unwanted changes, we would either have stuck with v3 or changed framework. This is a serious limitation, as pointed out in https://github.com/strapi/strapi/issues/11792. |
If you really need to, you can use patch-package to implement this PR directly on your project. We aren't planning to rush things as we need to take care to properly test things and ensure it's not going to break anything else. |
|
Will do, thank you for your suggestion. Do you have any ETA on this? |
|
Hello, We are not going to be merging this anytime soon as it raises a lot of problems to implement that on the entity manager layer. We added the transaction on the underlying layer where there are far less issues if you still want to use transactions. It will be part of the upcoming beta (next week) and live ~ end of july :) |
@alexandrebodin Can I have a gist of how the changes are going to look like? Can I have a commit/branch or PR to have a look? |
|
Sure it was introduced in a very simple manner to support some core queries here #13350 |
|
Thanks! Or did you mean that it is still on the plate but will take much longer? The implementation that you referenced is pretty low level. And using it in backend customisation code is just cutting straight through all the levels of abstractions that Strapi has built and making any solution based on this functionality susceptible to breaks in the future when Strapi change its internal handling of models. It also does not take care of relations as far as I can tell, so everything is pretty much back down to Therefore I feel like the high-level API is still needed. If can't be done now, should at least planned with approx. delivery timeline. Also, since I have patched my own implementation inspired by this PR into my version of Strapi, could you share what issues I may be facing because of this? |
|
Hi @benderillo, This low-level implementation is done on purpose as it avoids all the issues mentioned in this PR at the moment and solves our internal issues and it is just a 1st step in the right direction :) Hopefully we can add transactions into higher layers of Strapi but it will require a lot of thinking as it opens a lot of difficult questions and tradeoffs. To mention a few. lifecycles can create deadlocks, and relations with circular dependencies can create deadlocks too. components same. cross DB support and consistency and so on. To be transparent with you it is not something that we will have in our roadmap for a long while as we work more on higher-level features requested by users at the moment in the core team. I'll try to do a review in the upcoming month to share some direction if I can so it can still move forward and be implemented by community contributors. If you want to can organize a chat on discord at some point too :) I hope this gives you as much context as you need :) |
|
Until this gets merged, and until the db.query connector is also available, may I propose a possible solution for now: It seems that strapi uses Would that be a viable solution for now @alexandrebodin ? |
Yes exactly |
|
Hi @alexandrebodin, You said you'd try to do a review in a month to share some direction about how this might go forward if community contributors implement it, I was just wondering if you'd had the chance to do that? Specifically, what are the challenges for transactions being implemented at the strapi.db.query level? Do the same risks of deadlocks, etc. apply or were these issues specific to higher levels such as the entityManager? I'm not 100% sure whether strapi.db.query triggers lifecycle hooks or not. I tested @csotiriou 's solution and it worked but it made a simple one, two line strapi query many lines and a lot more complexity to deal with. Given that for my company's use case we need transactions for all queries, this poses a big issue, as we'd need to rewrite a lot of controllers to do this, only to then potentially have to rewrite them again when strapi does a major version update. It'd be really great to better understand some of the complexities surrounding adding transactions to Strapi V4 and how long it might take to implement this functionality, as I'd be happy to help contribute if it means this could get added quicker. Could I also ask why transactions were included in v3 but were completely removed in v4? I know you completely reworked how strapi deals with databases but I'm really confused as to why you'd not prioritise transactions and would love it if you could share some insights into what the process behind that decision was. Thanks in advance |
@callum-gander v3 was using the |
|
It's basically as you said, removing bookshelf changed a lot and we had to cut some things. Alex is currently out on holiday due to the French holiday right now but I'm sure once he is back he can give a more detailed answer. |
|
I'm surprised this isn't viewed as a higher priority to be honest, database transactions feel like a fundamental feature of any framework so to read that it's not even on the roadmap is disappointing. I'll try the low level implementation, however |
|
Can some1 show an example of a low-level transaction implementation pls? |
|
@alexandrebodin I'm guessing you've just missed this since you came back from holiday, but could I get possibly get an answer to my questions? I know you guys are probably very busy, but it'd be nice to know if it is possibly to contribute towards resolving this and to have a better understanding as to why this feature was not included in v4. Thanks in advance! |
|
try this nexus.extendType({
type: "Mutation",
definition: (t) => {
t.field("YOURT_FIELD", {
type: "Boolean",
args: {data: "YOUR_INPUT_TYPE"},
async resolve(parent, {data}, ctx) {
const trx = await strapi.db.connection.transaction();
try {
// update
await trx("YOUR_TABLE_NAME")
.where({...YOUR_CONDITION})
.update({...YOUR_DATA});
// you can test by throw an error
// throw new Error("test")
// insert
await trx("YOUR_TABLE_NAME").insert({...YOUR_DATA});
await trx.commit();
} catch (e) {
await trx.rollback();
}
return true
}
});
},
})Reference API: https://knexjs.org/guide/query-builder.html |
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: |
|
I know a pull request might not be the best place to put this but it's directly related to this pull request, anyone still interested in getting transactions moved up the priorities list for Strapi, please voice your support on the forum and vote on the feedback page so we can hopefully get this moved up https://forum.strapi.io/t/strapi-v4-isnt-acid-compliant-and-doesnt-natively-support-transactions/21312 https://feedback.strapi.io/developer-experience/p/support-database-transactions-with-knex |
|
I really dislike how features required for 'adult' requirements (like this) are low priorized while 'cheap' (for the billionth) blog/ simple-homepage requirements and shinyness is priorized much higher. |
|
Any updates on a timeline for this or a plan for what community members can do to help get this feature implemented? |
|
Hi Let's try to make some progress :) @callum-gander something that could be helpful to move in the right direction is sharing some specific use cases you need transactions for. (the more the better) Strapi hides a lot of complexity away but that also means it's a lot more difficult to expose low-level features on higher-level APIs. We will need to make compromises on what's doable as transactions can create a lot of "locked" situations (lifecycles, components, relations with circular deps, medias...) Knowing what kind of "classic" usage you would want to do with it could really help :) Thank you! |
|
Hello all, @alexandrebodin, I'm in the same situation as @callum-gander , using Strapi v4 as a backend for a complex application. He made it clear already : transactions is a must when you want something reliable. I don't really get why it would require specific use-cases, ACID is self explanatory, there's no compromise. What happens when a user loads data that are currently being written? It may load it with part of the relations inside. Simple Example :
Without transactions, the article is created and is made available for users (even on admin) on a tiny time-frame before tags are created. This is awful, the whole thing must be saved in the same transaction. I've already faced some situations like this where mandatory relations aren't loaded on a complex ContentType because of a user loading data at the exact moment another is writing it. For every single use case you may imagine, we need that a single modifying call to a specific endpoint uses a transaction. I don't see use cases where it wouldn't be needed. Even more needed if you consider error handling : like a time-out happening when saving mandatory relations... your data will be broken and the client app may then be too. To sum-it :
If there are some compromises on that, that means we, professionals wanting to use Strapi for applications, will need to switch to another product. And non-professionals will face issues too anyway. BR |
|
@NateR42, I couldn't have said it better! We won’t go in production without transactions! |
Thank you for the insights. We indeed have to wrap the current Strapi logic into transactions and make the default behaviors transactional. However this PR isn't about that. It only exposes transactions for users to use them directly. Thus my question. What kind of usage would you have for transactions outside the "normal expected/implicit" behavior? Do you want to be able to wrap EntityService calls with it? Do you want to be able to wrap an entire request context with it? Where would you need them to work overall? Do you want to be able to have fine grained control on locking or not? Exposing those as a high level API (wrapping components/relations/lifecycles) logic in it is not the same as exposing a transaction at the query level were you will have to implement the high level logic on your own. |
|
@tuxuuman I hope you don't mind I'll start pushing on top of this PR so you can stay as co-author :) |
|
@alexandrebodin perhaps too simplistic but I find the following use case a very much an essential ability:
On the server side, when creating a review, I could first check if the review for the given User + Course already exist and bail out. But without transaction, this logic is not guaranteed to work if at the same time the given user submits a review for the given course. |
|
And while I am using some custom-crafted version of the transactions from this PR in my code. I do have the issues that @alexandrebodin mentioned with regards to locks and lifecycle hooks. Otherwise, if not using the EntityManager passed down to lifecycle hook, you don't see the changes as they are not yet visible outside the transaction. Right now lifecycle hooks executed in the midst of the transaction so I have to pass the EM given to the lifecycle hooks to all the sub-calls to engine to ensure I work with the up-to-date data. But the risk of locks is high. |
This one is quite tricky. you want the changes you make to the data in the lifecycles to impact the current transaction but the other entityService or strapi.query() calls you make to be done outside of the transaction (or not actually) We would need something to explicitly queue an operation to run after the current transaction is done. I was thinking of returning a function to be executed after the current transaction for ex. All the other queries you would run in the function would be run inside the transaction. Another approach is to make events "pure" and add a middleware approach for side effects where we could split the transactional logic.
It looks like it really depends what you need here. sometimes in the beforeEvents it's fine unless you want to rely on data from other models that are also being created/uddated/deleted. We will have to make a choice of behaviors I think
|
|
Hi FYI I'm starting some work in #14389. @benderillo I implemented an implicit transaction system that allows calling the entity service transparently thanks to a shared async context. Feel free to play around with this. It's not addressing lifecycles issues at the moment and is mostly and experiment. As of now it wraps the entityService.create/update/delete functions only. |
|
I'm closing this in favor of #14389. |
Re lifecycle hooks, it is indeed a tricky subject. Some hooks run before-event others after. Thanks for the work in #14389! The fact that it allows to use entityService rather than EntityManager is a huge plus. |
|
This pull request has been mentioned on Strapi Community Forum. There might be relevant details there: |
What does it do?
Added transaction method to strapi.db
Usage example:
Also added em property to the lifecycle event object. This is the entity manager associated with the current transaction.
Related issue(s)/PR(s)
#11792