Fix incorrect handling of transactions using deferred constraints #4846

simPod · 2021-10-04T08:52:35Z

Closes #3424 as it's very long and outdated so I decided to start again here

Q	A
Type	bug/feature
BC Break	yes
Fixed issues	#3423, doctrine/orm#7545

Summary

Rollback is being called when outside of a transaction when using e.g. deferred constraints or when using ORM and database throws error like https://www.cockroachlabs.com/docs/v21.1/transaction-retry-error-reference.html#retry_write_too_old which is then effectively hidden after PDO's There's no active transaction.

morozov · 2021-10-08T17:13:25Z

@simPod thank you for the patch. I remember looking into this issue a couple of years ago. IIRC, it wasn't something trivial, so it may take some time before I post the review. But rest assured, it won't be ignored.

simPod · 2021-10-09T10:01:55Z

Thank you, note taken. Take your time.

morozov

I'm still wrapping my head around the changes but see a few comments below.

src/Connection.php

src/Driver/PDO/Connection.php

morozov · 2021-10-20T23:31:04Z

src/Driver/OCI8/Exception/Error.php

+            [$firstMessage, $secondMessage] = explode("\n", $message, 2);
+
+            [$code, $message] = explode(': ', $secondMessage, 2);
+            $code             = (int) str_replace('ORA-', '', $code);


Why do we need to replace the error code obtained from the driver with the one parsed from the error message?

Because it's the underlying one that interests us (UC violation). OCI chains errors so there are two errors returned
in this case. 2091 and 1.

But the code from the driver is related only to the first error (transaction roll back 2091) while we are interested in the second one to get information about the actual underlying issue (uc violated 1). We then know we can create UCViolation exception.

The unique constraint violation error is of interest to us in this specific case but there may be other cases. I don't think that replacing the error reported by the driver based on a personal preference is a good idea.

Instead, such errors could be represented as a chain (see Exception::$previous) of exceptions. So it would be 1) a transaction exception caused by 2) a unique constraint violation.

~~I have to introduce custom TransactionRolledBack exception~~

How do I make it work with ExceptionConverter then? It does not support chained errors from the driver.

This is what I have since I'm able to read multiple errors from the server now:

Error - Transaction rolled back <- Error - UC violation

This is what exception converter does right now

Driver Exception (Error - Transaction rolled back <- Error - UC violation)

We need to somehow inject UniqueConstraintViolationException into 👆🏾 but cannot simply use previous as OCI Error is used as previous now.

See 4f6487e

How do I make it work with ExceptionConverter then? It does not support chained errors from the driver.

That problem sounds solvable to me.

Instead, such errors could be represented as a chain (see Exception::$previous) of exceptions. So it would be 1) a transaction exception caused by 2) a unique constraint violation.

This is the way.

morozov · 2021-10-20T23:32:44Z

tests/Driver/PDO/ExceptionTest.php

+        $pdoException = new PDOException(<<<TEXT
+OCITransCommit: ORA-02091: transaction rolled back
+ORA-00001: unique constraint (DOCTRINE.C1_UNIQUE) violated
+ (/private/tmp/php-20211003-35441-1sggrmq/php-8.0.11/ext/pdo_oci/oci_driver.c:410)


Please do not add unit tests for such runtime-specific cases. If necessary, this logic should be tested in an integration way across all relevant platforms.

It is tested in integration way in the main test here.

Wanted to clarify and validate the custom logic so I isolated the error message parsing behaviour into unit tests as well to better document it. You asked exactly about it here #4846 (comment) and this tests covers it. But I can remove it before merging for sure.

morozov · 2021-10-20T23:33:59Z

tests/Functional/DeferrableConstraintsTest.php

+        } elseif ($platform instanceof PostgreSQLPlatform) {
+            $constraintName = 'c1_unique';
+        } else {
+            $constraintName = 'c1_unique';


How is it different from the above? Is it possible to use strtolower() where necessary instead of branching based on the platform?

It's better to store constraint name in advance as it is not used for assertions only but in SQL statements as well

e.g. SET CONSTRAINTS "%s" DEFERRED

morozov · 2021-10-20T23:35:42Z

tests/Functional/DeferrableConstraintsTest.php

+            );
+        } elseif ($platform instanceof SqlitePlatform) {
+            $createConstraint = sprintf(
+                'CREATE UNIQUE INDEX %s ON deferrable_constraints(unique_field)',


So it looks like we're testing a feature that isn't supported by the DBAL. Does SQLite support deferrable constraints?

No, it does not. But we wanted to run the test on all platforms #3424 (review)

In this case, it would make more sense to have two tests: each covers its own feature on the platforms that support it.

I renamed the test. The initial motive of previous author was to handle deferred constraints only in the original PR.

Then we realised unique constraints were not really tested so we've expanded the test. So this tests unique constraint violations while it also tests deferring the violations where eligible. So I don't really see how to split it but I've adapted the naming as it was misleading for sure.

morozov · 2021-10-20T23:36:34Z

tests/Functional/DeferrableConstraintsTest.php

+final class DeferrableConstraintsTest extends FunctionalTestCase
+{
+    /** @var string */
+    private $constraintName = '';


Why should it be part of the test state? It's the test data that should be static.

Should I convert it to method then? I need to know the name of constraint for each platform (#4846 (comment))

morozov · 2021-10-20T23:38:26Z

Could you try rebasing so that all the required build jobs get executed?

github-actions · 2022-07-30T03:35:57Z

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

simPod · 2023-09-16T05:39:35Z

.

…

On Sat, Sep 16, 2023, 06:01 github-actions[bot] ***@***.***> wrote: There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment. — Reply to this email directly, view it on GitHub <#4846 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQAJN62KQ2QROWMBMXIXLX2UJAZANCNFSM5FI2IL7A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

simPod · 2023-09-16T05:46:08Z

Bot

simPod · 2023-09-16T05:48:57Z

Fix urself

derrabus · 2023-11-17T08:13:32Z

Can somebody fill me in what's up with this PR? It does not look like the kind of change we would accept on the 4.0.x branch.

simPod · 2023-11-17T10:39:04Z

@derrabus see doctrine/orm#7545 Current behavior section. The problem is that we're trying to rollback already rollbacked transaction and therefore the underlying issue (like unique constraint violation) is suppressed by There's no active transaction.

derrabus · 2023-11-17T12:27:01Z

Understood. Let me take over the review then. But as I said, we don't accept this kind of change on the 4.0.x branch. You need to target 3.7.x currently if this is a bugfix and 3.8.x otherwise.

simPod · 2023-11-17T14:42:16Z

Great. I have rebased this like trillion times over the years so I'm glad there's a promise of a movement now. I'll target 3.8.x I guess.

simPod · 2023-12-02T16:56:29Z

@derrabus this is now working again.

derrabus · 2023-12-03T10:42:27Z

src/Driver/OCI8/Exception/Error.php

+        $code    = $error['code'];
+        $message = $error['message'];
+        if ($code === self::CODE_TRANSACTION_ROLLED_BACK) {
+            // There's no way this can be unit-tested as it's impossible to mock $resource


This kind of comment does not belong into production code.

derrabus · 2023-12-03T10:48:46Z

src/Driver/OCI8/Exception/Error.php

+            [$firstMessage, $secondMessage] = explode("\n", $message, 2);
+
+            [$code, $message] = explode(': ', $secondMessage, 2);
+            $code             = (int) str_replace('ORA-', '', $code);


How do I make it work with ExceptionConverter then? It does not support chained errors from the driver.

That problem sounds solvable to me.

Instead, such errors could be represented as a chain (see Exception::$previous) of exceptions. So it would be 1) a transaction exception caused by 2) a unique constraint violation.

This is the way.

derrabus · 2023-12-03T10:51:09Z

src/Driver/OCI8/Exception/Error.php

-        return new self($error['message'], null, $error['code']);
+        $code    = $error['code'];
+        $message = $error['message'];
+        if ($code === self::CODE_TRANSACTION_ROLLED_BACK) {


I don't feel comfortable with making this exception class aware of specific error codes. That logic belongs into ExceptionConverter. Can we replace this logic with a generic detection of chained errors?

github-actions · 2024-03-03T03:01:47Z

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

- Let root Errors bubble up - Catch concrete Exception in TransactionTest as we can do it now - Expose underlying errors on Oracle platform Co-authored-by: Jakub Chábek <jakub.chabek@cdn77.com>

simPod force-pushed the df-c branch 8 times, most recently from cded7d3 to 82ce962 Compare October 4, 2021 11:38

simPod marked this pull request as ready for review October 4, 2021 13:02

simPod force-pushed the df-c branch from 48a9eda to 3df5e8f Compare October 4, 2021 13:14

morozov added Foreign Keys Improvement Oracle PostgreSQL labels Oct 8, 2021

morozov reviewed Oct 20, 2021

View reviewed changes

simPod force-pushed the df-c branch from 3df5e8f to 179daaa Compare October 21, 2021 06:22

DoobleD mentioned this pull request Nov 19, 2021

[2.10.2] Ignore dependencies having onDelete="SET NULL" when computing commit order for removal operations doctrine/orm#9192

Closed

simPod force-pushed the df-c branch 3 times, most recently from 9310602 to c7a0ef8 Compare November 20, 2021 14:05

morozov added the Requires Feedback label Dec 29, 2021

derrabus changed the base branch from 3.2.x to 3.4.x January 18, 2022 00:50

simPod force-pushed the df-c branch 2 times, most recently from 534123e to d0c9c41 Compare March 23, 2022 11:04

simPod force-pushed the df-c branch from d0c9c41 to bf790e6 Compare April 30, 2022 10:30

github-actions bot added the Stale label Jul 30, 2022

github-actions bot removed the Stale label Sep 17, 2023

simPod force-pushed the df-c branch from 9cb4053 to 301f93c Compare November 20, 2023 11:02

simPod changed the base branch from 4.0.x to 3.8.x November 20, 2023 11:03

simPod force-pushed the df-c branch 7 times, most recently from 9e21e40 to 14dcd62 Compare November 20, 2023 13:00

simPod marked this pull request as draft November 20, 2023 21:03

simPod force-pushed the df-c branch 2 times, most recently from d94da30 to 7ac4705 Compare December 2, 2023 16:55

simPod marked this pull request as ready for review December 2, 2023 16:56

derrabus requested changes Dec 3, 2023

View reviewed changes

github-actions bot added the Stale label Mar 3, 2024

Fix incorrect handling of transactions using deferred constraints

8edffa9

- Let root Errors bubble up - Catch concrete Exception in TransactionTest as we can do it now - Expose underlying errors on Oracle platform Co-authored-by: Jakub Chábek <jakub.chabek@cdn77.com>

simPod force-pushed the df-c branch from 7ac4705 to 8edffa9 Compare March 4, 2024 10:26

simPod changed the base branch from 3.8.x to 4.0.x March 4, 2024 10:32

simPod marked this pull request as draft March 4, 2024 10:35

github-actions bot removed the Stale label Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect handling of transactions using deferred constraints #4846

Fix incorrect handling of transactions using deferred constraints #4846

simPod commented Oct 4, 2021 •

edited

morozov commented Oct 8, 2021

simPod commented Oct 9, 2021

morozov left a comment

morozov Oct 20, 2021

simPod Oct 21, 2021

morozov Nov 14, 2021

simPod Nov 20, 2021 •

edited

simPod Nov 20, 2021 •

edited

derrabus Dec 3, 2023

morozov Oct 20, 2021

simPod Oct 21, 2021 •

edited

morozov Oct 20, 2021

simPod Nov 20, 2021

morozov Oct 20, 2021

simPod Oct 21, 2021

morozov Nov 14, 2021

simPod Nov 20, 2021

morozov Oct 20, 2021

simPod Nov 20, 2021

morozov commented Oct 20, 2021

github-actions bot commented Jul 30, 2022

simPod commented Sep 16, 2023 via email

simPod commented Sep 16, 2023

simPod commented Sep 16, 2023

derrabus commented Nov 17, 2023

simPod commented Nov 17, 2023

derrabus commented Nov 17, 2023

simPod commented Nov 17, 2023

simPod commented Dec 2, 2023

derrabus Dec 3, 2023

derrabus Dec 3, 2023

derrabus Dec 3, 2023

github-actions bot commented Mar 3, 2024

Fix incorrect handling of transactions using deferred constraints #4846

Are you sure you want to change the base?

Fix incorrect handling of transactions using deferred constraints #4846

Conversation

simPod commented Oct 4, 2021 • edited

Summary

morozov commented Oct 8, 2021

simPod commented Oct 9, 2021

morozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simPod Nov 20, 2021 • edited

Choose a reason for hiding this comment

simPod Nov 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simPod Oct 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Oct 20, 2021

github-actions bot commented Jul 30, 2022

simPod commented Sep 16, 2023 via email

simPod commented Sep 16, 2023

simPod commented Sep 16, 2023

derrabus commented Nov 17, 2023

simPod commented Nov 17, 2023

derrabus commented Nov 17, 2023

simPod commented Nov 17, 2023

simPod commented Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2024

simPod commented Oct 4, 2021 •

edited

simPod Nov 20, 2021 •

edited

simPod Nov 20, 2021 •

edited

simPod Oct 21, 2021 •

edited