Fix incorrect `transactional()` handling when DB auto-rolled back the transaction #6545

simPod · 2024-10-12T12:06:04Z

Q	A
Type	bug
Fixed issues	#3423

Summary

#6543 partially solved what we tried to achieve in #4846. With that There's no active transaction exception contains the actual exception in $previous.

Now, let's get rid of There's no active transaction exception which occurs e.g. when using deferred constraints so the violation is checked at the end of the transaction and not during it. Database then rolls back the transaction automatically.

The current faulty flow is following:

on commit(), Constrain violation occurs
transaction is implicitly aborted and rolled back in database
DBAL calls rollback()
Since there's no active transaction, There's no active transaction exception is thrown:

This PR adjusts the flow so:

on commit(), Constrain violation occurs
transaction is implicitly aborted and rolled back in database
DBAL DOES NOT call rollback() 🥳

For DBAL v4 it looks like this #6546 (draft, does not contain review changes)

src/Connection.php

Address doctrine#6545 (comment)

src/Connection.php

Address doctrine#6545 (comment)

greg0ire · 2024-10-12T15:46:27Z

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

simPod · 2024-10-12T15:48:12Z

Thanks, just did not notice that basic stuff, u were faster 🙃 fixed.

Address doctrine#6545 (comment)

src/Connection.php

doctrine#6545 (comment)

@greg0ire

…6563) | Q | A |------------- | ----------- | Type | improvement | Fixed issues |  #### Summary Covers the concern expressed by @greg0ire #6545 (comment) This ensures the transaction is (not)active based on `autocommmit` mode when interacting with `transactional()`.

src/Exception/TransactionRolledBack.php

… transaction Let's get rid of There's no active transaction exception which occurs e.g. when using deferred constraints so the violation is checked at the end of the transaction and not during it. - Do not rollback only on exceptions where we know the transaction is no longer active - Introduce TransactionRolledBack exception - Transform Oracle's "transaction rolled back" exception and use the underlying one that DBAL supports

morozov · 2024-11-04T17:16:08Z

It will take me some time to understand the details in order to provide a meaningful feedback. Please feel free to merge it as is. The number of "line not covered by tests" warnings and enumeration of specific exception classes in handling a commit failure is a bit concerning, though. On the positive side, the fix doesn't introduce any new API, so if something goes seriously wrong, it will be safe to revert.

simPod · 2024-11-04T17:36:42Z

I guess there's some issue with coverage configuration since e.g. my test explicitly covers this line

greg0ire · 2024-11-04T19:09:41Z

I think that for some reason, codecov only processes the reports from appveyor, and not from github-action. I have yet to figure out why.

greg0ire · 2024-11-04T19:11:02Z

Thanks @simPod 🎉

simPod · 2024-11-04T19:19:10Z

Let me know if you need help resolving v4 conflicts.

greg0ire · 2024-11-04T19:45:45Z

Thanks for offering your help, I do need it 🙏

simPod marked this pull request as ready for review October 12, 2024 12:17

greg0ire reviewed Oct 12, 2024

View reviewed changes

src/Connection.php Outdated Show resolved Hide resolved

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

cf80105

Address doctrine#6545 (comment)

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

4acb3a9

Address doctrine#6545 (comment)

simPod force-pushed the df-c_v3 branch from cf80105 to 4acb3a9 Compare October 12, 2024 14:29

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

72af9b8

Address doctrine#6545 (comment)

simPod force-pushed the df-c_v3 branch from 4acb3a9 to 72af9b8 Compare October 12, 2024 14:38

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

7b67f7b

Address doctrine#6545 (comment)

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

2a1a80d

Address doctrine#6545 (comment)

simPod requested a review from greg0ire October 12, 2024 14:48

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

cb1389d

Address doctrine#6545 (comment)

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

a450cad

Address doctrine#6545 (comment)

simPod mentioned this pull request Oct 12, 2024

Fix incorrect transactional() handling when DB auto-rolled back the transaction (v4) #6546

Merged

greg0ire reviewed Oct 12, 2024

View reviewed changes

src/Connection.php Show resolved Hide resolved

src/Connection.php Show resolved Hide resolved

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

0bdc850

Address doctrine#6545 (comment)

simPod force-pushed the df-c_v3 branch from 72af9b8 to 0bdc850 Compare October 12, 2024 15:39

simPod added a commit to simPod/dbal that referenced this pull request Oct 12, 2024

Cover possible no-db related throw before touching the db

88879b3

Address doctrine#6545 (comment)

simPod force-pushed the df-c_v3 branch 3 times, most recently from 71e5b43 to 9879edf Compare October 13, 2024 10:15

simPod requested a review from greg0ire October 13, 2024 10:19

simPod force-pushed the df-c_v3 branch 2 times, most recently from ebc4aa1 to b512770 Compare October 13, 2024 11:20

greg0ire reviewed Oct 13, 2024

View reviewed changes

src/Connection.php Show resolved Hide resolved

simPod mentioned this pull request Oct 14, 2024

test: cover nested transactions #6547

Merged

simPod force-pushed the df-c_v3 branch 3 times, most recently from 675d1ac to e9ea2b1 Compare October 14, 2024 13:42

simPod added a commit to simPod/dbal that referenced this pull request Oct 20, 2024

Cover scenario BEGIN TRANSACTION; COMMIT; BEGIN TRANSACTION; --failure

0703cb8

doctrine#6545 (comment)

simPod mentioned this pull request Oct 20, 2024

Cover how transactional() behaves in different auto commit modes #6563

Merged

simPod added a commit to simPod/dbal that referenced this pull request Oct 20, 2024

Cover scenario `BEGIN TRANSACTION; COMMIT; BEGIN TRANSACTION --failure;

e21875a

doctrine#6545 (comment)

simPod force-pushed the df-c_v3 branch from 56b2aad to e21875a Compare October 20, 2024 12:11

simPod changed the title ~~Fix incorrect handling of transactions when using deferred constraints~~ Fix incorrect transactional() handling when DB auto-rolled back the transaction Oct 20, 2024

simPod added a commit to simPod/dbal that referenced this pull request Oct 20, 2024

Cover scenario BEGIN TRANSACTION; COMMIT; BEGIN TRANSACTION; --failure

30ead0b

doctrine#6545 (comment)

simPod added a commit to simPod/dbal that referenced this pull request Oct 20, 2024

Cover scenario BEGIN TRANSACTION; COMMIT; BEGIN TRANSACTION; --failure

0e2fa82

doctrine#6545 (comment)

simPod added a commit to simPod/dbal that referenced this pull request Oct 20, 2024

Cover scenario BEGIN TRANSACTION; COMMIT; BEGIN TRANSACTION; --failure

db467f0

doctrine#6545 (comment)

simPod force-pushed the df-c_v3 branch from e21875a to e3f30d2 Compare October 21, 2024 20:03

simPod force-pushed the df-c_v3 branch from e3f30d2 to 7bf7c9f Compare October 31, 2024 17:06

simPod requested a review from greg0ire October 31, 2024 17:08

greg0ire reviewed Nov 2, 2024

View reviewed changes

src/Exception/TransactionRolledBack.php Outdated Show resolved Hide resolved

simPod mentioned this pull request Nov 2, 2024

test: remove ->expects(self::any()) simPod/dbal#13

Closed

simPod force-pushed the df-c_v3 branch from 7bf7c9f to ec90463 Compare November 2, 2024 17:29

greg0ire approved these changes Nov 2, 2024

View reviewed changes

greg0ire requested review from morozov, SenseException and derrabus November 2, 2024 21:15

greg0ire added this to the 3.9.4 milestone Nov 4, 2024

greg0ire added the Bug label Nov 4, 2024

greg0ire merged commit dd4601c into doctrine:3.9.x Nov 4, 2024
98 checks passed

simPod deleted the df-c_v3 branch November 4, 2024 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect `transactional()` handling when DB auto-rolled back the transaction #6545

Fix incorrect `transactional()` handling when DB auto-rolled back the transaction #6545

simPod commented Oct 12, 2024 •

edited

Loading

greg0ire commented Oct 12, 2024

simPod commented Oct 12, 2024

morozov commented Nov 4, 2024 •

edited

Loading

simPod commented Nov 4, 2024

greg0ire commented Nov 4, 2024

greg0ire commented Nov 4, 2024

simPod commented Nov 4, 2024

greg0ire commented Nov 4, 2024

Fix incorrect transactional() handling when DB auto-rolled back the transaction #6545

Fix incorrect transactional() handling when DB auto-rolled back the transaction #6545

Conversation

simPod commented Oct 12, 2024 • edited Loading

Summary

greg0ire commented Oct 12, 2024

simPod commented Oct 12, 2024

morozov commented Nov 4, 2024 • edited Loading

simPod commented Nov 4, 2024

greg0ire commented Nov 4, 2024

greg0ire commented Nov 4, 2024

simPod commented Nov 4, 2024

greg0ire commented Nov 4, 2024

Fix incorrect `transactional()` handling when DB auto-rolled back the transaction #6545

Fix incorrect `transactional()` handling when DB auto-rolled back the transaction #6545

simPod commented Oct 12, 2024 •

edited

Loading

morozov commented Nov 4, 2024 •

edited

Loading