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-12-02T16:56:29Z

@derrabus this is now working again.

src/Driver/OCI8/Exception/Error.php

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 <[email protected]>

github-actions · 2024-06-03T03:01:45Z

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.

github-actions · 2024-06-11T03:01:46Z

This pull request was closed due to inactivity.

| Q | A |------------- | ----------- | Type | bug | Fixed issues | #4846 #### Summary Let's do the same thing as for ORM in doctrine/orm#11646 This somehow solves issue we tried to resolve in #4846 by providing original exception in Previous.

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

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 <[email protected]>

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

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

github-actions bot closed this Jun 11, 2024

simPod mentioned this pull request Oct 11, 2024

Run risky code in finally block #6543

Merged

This was referenced Oct 12, 2024

Fix incorrect handling of transactions using deferred constraints simPod/dbal#8

Closed

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

Merged

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

Loading

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

Loading

simPod Nov 20, 2021 •

edited

Loading

derrabus Dec 3, 2023

morozov Oct 20, 2021

simPod Oct 21, 2021 •

edited

Loading

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 Dec 2, 2023

derrabus Dec 3, 2023

derrabus Dec 3, 2023

github-actions bot commented Mar 3, 2024

github-actions bot commented Jun 3, 2024

github-actions bot commented Jun 11, 2024

Fix incorrect handling of transactions using deferred constraints #4846

Fix incorrect handling of transactions using deferred constraints #4846

Conversation

simPod commented Oct 4, 2021 • edited Loading

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 Loading

Choose a reason for hiding this comment

simPod Nov 20, 2021 • edited Loading

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 Loading

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 Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2024

github-actions bot commented Jun 3, 2024

github-actions bot commented Jun 11, 2024

simPod commented Oct 4, 2021 •

edited

Loading

simPod Nov 20, 2021 •

edited

Loading

simPod Nov 20, 2021 •

edited

Loading

simPod Oct 21, 2021 •

edited

Loading