Use database transactions in saving proces of Resources and Actions

[johanzandstra]

Description

Currently no transactions are used in the saving process of a Resource and Actions. When when we database connection errors, memory errors or other application related exceptions or errors occurred during the saving process, the data will be stored only party.

Using Transactions we can be sure the whole saving process is successful before commit the transaction and all the changes to (related) models to the database.

Code style

• [x] composer cs command has been run.

Testing

• [x] Changes have been tested.

Documentation

• [x] Documentation is up-to-date.

[wychoong]

Been wanting this since v2 but didn’t get to work on it

[johanzandstra]

@danharrin can you please let me know if there is anything I can do in the PR?

[danharrin]

I just need to find time to review it and check there aren't side effects

[danharrin]

I've re-implemented this in #11544, made it opt-in for custom actions, added customization for Halt and Cancel behaviours, and removed unrelated changes made in this PR

[brkfun]

is there any option implemented to not make begin transaction? I'm asking because there are lots of file changes here.

[micraux]

is there any option implemented to not make begin transaction? I'm asking because there are lots of file changes here.

Quite the same question for me: how can we choose to use (or not) the database transaction feature?

I can't find anything related to database transaction in the docs.

[wychoong]

is there any option implemented to not make begin transaction? I'm asking because there are lots of file changes here.

This is not the merged implementation

Quite the same question for me: how can we choose to use (or not) the database transaction feature?

$action->databaseTransaction( true / false)

I can't find anything related to database transaction in the docs.

if missing in doc

• read the actual PR, or
• source dive
• then PR update the doc

[danharrin]

And only create/edit has database transactions enabled by default, everything else is opt in

[johanzandstra]

@wychoong @micraux only the internal saving proces is covered by transactions and it’s not wrapped around events. Just a question out of interest, why would you disable transactions? All default Laravel database drivers should support them.

[brkfun]

@wychoong @micraux only the internal saving proces is covered by transactions and it’s not wrapped around events. Just a question out of interest, why would you disable transactions? All default Laravel database drivers should support them.

it has to be a choice that you can make. Any application written by any user maybe needs to create database or schema at the observer level. Which is cannot be doable because of begin transaction.

By the way i'm not talking about this is a bad feature. This is an excellent feature but it doesn't documented. Thats why i just commented. If thats the issue sorry for that i'm commented here. I'll try my best to try to update the docs or someone will be faster than me to do that.

[poldixd]

I'm running into following issue with LazilyRefreshDatabase::class: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT trans2 does not exist

After adding ->databaseTransaction(false) to my CreateAction::make my test works fine.

[danharrin]

Just too many issues with this I'm afraid @johanzandstra, I've made it opt-in in #11640.

[johanzandstra]

@danharrin sounds good to me!

I can imagine some database drivers don't like double start of transactions and we should be able to use them when running tests and want to speed-up.

I agree we should take some more time with resolving these issues before we enable them globally indeed! An opt-in solution sounds like a good starting point for now! In my opinion we still need to have transactions in a production environment to make sure the data integrity is correct, but in this way we are able to use them!

[cheesegrits]

@danharrin @johanzandstra

Just wanted to chime in here, supporting the feature, but also strongly supporting keeping it optional for quite some time.

The feature as initially merged (without opt-in) broke some of my critical existing code in very bizarre and hard to debug ways. I extensively use model observers on create/update, in both parent and related models on forms. Once wrapped in a transaction, some of my code broke in unexpected ways. First problem was I was using queued event handling, which just doesn't jive with transactions. But even after removing the queueing and firing the code synchronously it still didn't work right. I didn't have time to fully debug it, but long story short, changes I was making on the parent record on create were no longer visible to the subsequent create events on the related records, via their relation to the parent.

Another issue that didn't bite me but quite possibly could with transactions is that I use model observers to delete files from S3, as recommended in the Filament docs. For example when a related record (in a Repeater relationship) with a file upload is deleted from the parent. If for whatever reason the EditRecord transaction got rolled back, I'd then have a related record with no associated file, and no way to recover it.

So while I totally support implementing transactions, please be aware that there is code out here in userland which flat out breaks, or has unintended / unrecoverable consequences, when wrapped in transactions. It will take time and understanding for people to adapt their code to support this.

My only other comment is that we should probably add a hook like $this->callHook('transactionRolledBack') (for record create/edit) and $action->callTransactionRolledBack() (for actions), to be called whenever a transaction is rolled back, so users have the opportunity to undo anything they may have done during observer handling.

We may also wish to consider an 'afterCommit' hook in CreateRecord and EditRecord, as it is possible that a DB:commit() itself could fail, and code that REALLY cares would want to run after the commit rather than before it (where afterCreate and afterEdit currently run).

[brkfun]

My issue was as same as yours @cheesegrits. I was using stancl/tenancy implementing to filament as db scheme per tenant. And in observer it triggers the job for creating scheme, because of the pgsql's lack of exception handling, i couldn't understand what did i get at first(it was: "Invalid schema name: 7 ERROR: no schema has been selected to create in at character" it means pgsql doesnt create a new scheme). So after that i change that scheme to database i will see the actual error "CREATE DATABASE cannot run inside a transaction block". Which then i try to search over all code. But no worries thanks for the fix and fast information 🙏